Use HMIS Project Policy for enrollment detail visibility checks - #6779
Conversation
| return true if source_project_permissions.include?(:can_manage_outgoing_referrals) | ||
|
|
||
| # Users who can view the target enrollment. Bakes in the assumption that the target enrollment is in the referral's project, which is validated on the referral | ||
| # TODO(8549) - encapsulate this check requiring both can_view_enrollment_details and can_view_project in the Enrollment Policy |
There was a problem hiding this comment.
This is already baked in from permission requirements, just updating the comment to reflect that
martha
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks for the cleanup 🙏
| hmis_user.policy_for(hmis_entity, policy_type: :hmis_client).can_view? | ||
| when Hmis::Hud::Enrollment | ||
| hmis_user.policy_for(hmis_entity, policy_type: :hmis_enrollment).can_view_details? | ||
| when Hmis::Hud::CustomAssessment, Hmis::Hud::CustomService |
There was a problem hiding this comment.
We could instead gate on hmis_entity.respond_to?(:enrollment) and then we would check permissions on a broader range of enrollment-related records, instead of just Hmis::Hud::CustomAssessment, Hmis::Hud::CustomService.
I don't know the full context of when this is called, so maybe this is irrelevant.
There was a problem hiding this comment.
Hmm good question. It feels like an unsafe pattern to check authorization on a record where we don't know explicitly what it is, that would be my hesitation. I'll leave it as-is for now which has parity with the old behavior
| 'Hmis::Hud::Project' => [:can_view_project], | ||
| # 'Hmis::Hud::Organization' => [:can_view_organization], | ||
| 'Hmis::Hud::Client' => [:can_view_clients], | ||
| 'Hmis::Hud::Enrollment' => [:can_view_projects, :can_view_enrollment_details], |
There was a problem hiding this comment.
Note from AI review:
[warning]app/models/grda_warehouse/data_source.rb:824— This is a larger behavior change than the description implies. The oldknown_permissionsmap required:can_view_projects(plural) forHmis::Hud::EnrollmentandHmis::Hud::CustomAssessment.can_view_projectsis a warehouseRolepermission, not anHmis::Rolepermission, andHmis::Useris not a subclass of::User— soHmis::User#permission_for?hit itsrespond_to?guard and returned false. Withmode: :all, that made the check unconditionally false, meaning the "Open in HMIS" link was always suppressed for enrollments and assessments. The new code will start rendering those links whereverhmis_external_linkis used. That is almost certainly the desired outcome, but it should be called out in the PR description and confirmed with someone who knows the intended warehouse UX, since it is a user-visible change rather than a refactor.
This surprised me, but I think it does seem to be true. I checked by viewing an enrollment in my local warehouse that I do have permission to see in HMIS (https://hmis-warehouse.dev.test/source_data/xxxx?type=Enrollment). On main I don't see a link to HMIS, but on your branch, I do.
There was a problem hiding this comment.
Yes this looks like a bug in the old code that is now fixed. "can_view_projects" on lines 825-826 are a typo, it should be "can_view_project" like on line 822. These were always checking HMIS permissions (line 822 hmis_user.permissions_for?(hmis_entity, *perms, mode: :all)) it was just incorrectly returning false because of the typos.
| can :delete_project | ||
| can :edit_project_details | ||
| can :view_enrollment_details | ||
| define_method(:policy) { @policy ||= policy_for(object, policy_type: :hmis_project) } |
There was a problem hiding this comment.
Maybe, add a project_access_field_spec.rb mirroring client_access_field_spec.rb. It looks like the underlying policy methods themselves are well tested though, so I think this is optional.
Merging this PR
mainstagingandproductionDescription
Related Issues (Does not close):
Adds project/client policy methods for enrollment-detail visibility, and migrates GraphQL callers (and warehouse “Open in HMIS” links) off raw permission helpers so permission dependencies apply consistently.
can_view_enrollment_details?/can_edit_enrollments?on project policycan_view_some_enrollment_details?on client policyProject#with_enrollment_detail_visibility_for(user), to remove repeated usage ofwith_access(user, :can_view_enrollment_details, :can_view_project, mode: :all)across enrollment and household visibility scopesWhy now:
can_view_clientsas a required permission for enrollment visibility. Enrollment visibility permission checks are sprinkled throughout the codebase; this PR consolidates so we can make the change in fewer places.Type of change
Code clean-up
Checklist before requesting review