[Debt] Update how roles are set as part of communities changes #10791
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10791 +/- ##
============================================
- Coverage 38.05% 38.02% -0.04%
- Complexity 1751 1792 +41
============================================
Files 1060 1061 +1
Lines 30913 31006 +93
Branches 6574 6568 -6
============================================
+ Hits 11764 11790 +26
- Misses 18974 19179 +205
+ Partials 175 37 -138
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Okay @tristan-orourke I did the suggested sync removal in 7e2d841 and the replace/deprecate in 7c660bb Thoughts on things so far? Right direction? |
There was a problem hiding this comment.
Great job on the frontend, I like how you got replaced sync with attach & detach.
The downside is that the UserPolicy->updateRoles method is real ugly, now. 😆 And I expected that, but now that I see it I have a suggestion. What if we create a mutation class to handle updateUserRoles, and move some of the logic (especially for processing the input) from the Policy to there? The goal would be to end up with a policy method (or possibly break it down into several) which is straightforward and easy to read.
I'm not sure I follow. The policy check precedes the mutation, so I don't see how you can shift the logic into the latter. The operation itself is complex given it involves attach and detach, team-based roles. non-team roles, and all the possible roles and permissions present. I don't know how we can circumvent a complex policy check given it is a complex authorization process short of refactoring the role mutations in the front and back into separate granular mutations for each single role you can add/remove. |
There was a problem hiding this comment.
Okay, thanks for helping me think through this earlier.
In addition to the other comments, please update the HasRoleAssignments interface:
interface HasRoleAssignments {
roleAssignments: [RoleAssignment!]
teamId: ID! # Used to assign roles associated with this resource using the updateUserRoles mutation.
}
The Team, Pool and Community models will need a teamId accessor added, to return the team id.
api/app/Policies/UserPolicy.php
Outdated
| } | ||
|
|
||
| // adding or removing team based roles | ||
| if (isset($attachRoles['team']) || isset($detachRoles['team'])) { |
There was a problem hiding this comment.
I take back my previous comment about decoupling this policy from the schema. However, if we follow my suggested change to the schema, then it probably makes more sense to simply loop through all the attach roles then all the detach roles, instead of dealing with team roles first then non-team roles.
There was a problem hiding this comment.
Adjusting the policy to fit the new mutation input type simplifies the policy file a fair bit
4980080
| @@ -1795,7 +1794,7 @@ type Mutation { | |||
| updateUserRolesInput: UpdateUserRolesInput! @spread | |||
| ): UserAuthInfo | |||
| @update(model: "User") | |||
There was a problem hiding this comment.
If we change the UpdateUserRolesInput, we'll need to remove the @update directive which lets Lighthouse magically handle the update, and do it in a mutation class.
| @@ -1795,7 +1794,7 @@ type Mutation { | |||
| updateUserRolesInput: UpdateUserRolesInput! @spread | |||
| ): UserAuthInfo | |||
| @update(model: "User") | |||
| @canModel(ability: "updateRoles", model: "User") | |||
There was a problem hiding this comment.
We should also add a @guard directive
Sure 3af0b17 EDIT
Due to
|
|
Overall thoughts now? |
|
I'm getting below error in http://localhost:8000/graphiql and couldn't run any mutation. I took the latest of the branch |
There was a problem hiding this comment.
http://localhost:8000/graphiql interface throws validation error and couldn't verify
Its possible to use a mutation to assign new Community and Pool based rolesand couldn't exercise the new roles via mutation.
Everything else looks good 👍
Okay I think I just needed to do this to fix it ? f808353 |
|
Okay, using Renamed to a hopefully temporary name that can be changed afterwards in e4beaf0 |
|
You can test with the newer roles by removing the filtering in the frontend. There are a few places to remove the filter from then you can give them to people. I found it handy to test due to weird permission caching mysteries in graphiql Example below
|
|
Good catches! I think it should be possible to make it work without renaming teamId... I'll try playing with it a bit. |
|
I only see this problem with this user.
|
Can't seem to replicate 😕
|
|
So tried different browser firefox. Still the same. Tried few diff things & results are :
|
|
@vd1992, I didn't have a chance to look at teamId, but since it doesn't work, can I at least recommend a different name? I'm thinking teamIdForRoleAssignment, since that purpose is the only reason we're exposing it. |
There was a problem hiding this comment.
I've now been able to test it manually, and it all seems fine to me. I haven't reproduced any issues with [email protected] user.
Should be good to merge then. I'll keep an eye on UAT on this. |
Approval please |
🤖 Resolves #10351
👋 Introduction
Updates the shape of the update roles mutation, gets rid of syncing for roles, deprecates the other update roles mutation, and updates policy to account for new roles in the mutation.
Some TODOs have been identified for later
🧪 Testing
🚚 Deployment
Add any additional details that are required for deploying the application.
Examples of when this is required include: