-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GYR1-690 allow site coordinators to edit site coordinator and team member accounts #5688
GYR1-690 allow site coordinators to edit site coordinator and team member accounts #5688
Conversation
Heroku app: https://gyr-review-app-5688-41b74de38d4e.herokuapp.com/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! But I would check on actions, see comment
app/lib/ability.rb
Outdated
@@ -189,6 +188,9 @@ def initialize(user) | |||
end | |||
|
|||
if user.site_coordinator? | |||
can :manage, User, id: User.where(role: SiteCoordinatorRole.assignable_to_sites(user.role.sites)).pluck(:id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just check with product that we want to include all these actions, for example I'm not sure if we want to allow them to destroy users (we use suspend/unsuspend instead) so something like might be more accurate:
can :manage, User, id: User.where(role: SiteCoordinatorRole.assignable_to_sites(user.role.sites)).pluck(:id) | |
can [:update, :create, :update, :unsuspend, :suspend (and any other custom actions)], User, id: User.where(role: SiteCoordinatorRole.assignable_to_sites(user.role.sites)).pluck(:id) |
app/lib/ability.rb
Outdated
@@ -189,6 +188,9 @@ def initialize(user) | |||
end | |||
|
|||
if user.site_coordinator? | |||
can :manage, User, id: User.where(role: SiteCoordinatorRole.assignable_to_sites(user.role.sites)).pluck(:id) | |||
can :manage, User, id: User.where(role: TeamMemberRole.assignable_to_sites(user.role.sites)).pluck(:id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess instead of including all those custom actions you could also just add a cannot after the lines like for example
can :manage, User, id: User.where(role: TeamMemberRole.assignable_to_sites(user.role.sites)).pluck(:id) | |
can :manage, User, id: User.where(role: TeamMemberRole.assignable_to_sites(user.role.sites)).pluck(:id) | |
cannot :destroy, User |
Co-authored-by: Drew Proebstel <[email protected]>
@@ -189,6 +188,9 @@ def initialize(user) | |||
end | |||
|
|||
if user.site_coordinator? | |||
can [:suspend, :unsuspend, :update, :unlock, :resend_invitation], User, id: User.where(role: SiteCoordinatorRole.assignable_to_sites(user.role.sites)).pluck(:id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @embarnard for flagging the actions! These are the updated ones.
Co-authored-by: Drew Proebstel <[email protected]>
This reverts commit 67d7450.
app/lib/pdf_filler/id39r_pdf.rb
Outdated
@@ -8,7 +8,6 @@ def source_pdf_name | |||
|
|||
def initialize(submission) | |||
@submission = submission | |||
@intake = submission.data_source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈 i was switching between branches and did the wrong thing
…oordinators-to-edit-site-coordinator-and-team-member-accounts
Link to pivotal/JIRA issue
Is PM acceptance required? (delete one)
Reminder: merge main into this branch and get green tests before merging to main
What was done?
accessible_vita_partners
) that wasn't being used in the relevant part of the ability file so we replaced the mocking with just setting up the actual db associationsHow to test?
Screenshots (for visual changes)