feat: add bulk scope support to PUT /api/authz/v1/roles/users/#256
Conversation
|
Thanks for the pull request, @ccantillo! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
mariajgrimaldi
left a comment
There was a problem hiding this comment.
Thanks so much for this! I left a few comments for you to review.
Mainly, I'd like to understand why this API needs to be backward compatible. I think we are still in the early stages of the framework, so making things backward-compatible might just make it more complex for us to maintain going forward. Just a note to consider.
FYI @rodmgwgu
Thanks for the review. The issue #231 states "keep backwards compatibility", so that's a requirement rather than a design choice on my end. That said, it's a fair point to raise. if @rodmgwgu agrees we can break it |
bmtcril
left a comment
There was a problem hiding this comment.
Looks good to me. I don't know enough about the frontend impl to say whether maintaining backwards compatibility would save FE developer time at this point but that may have been a reason why it was in the requirements
I specified keeping backwards compatibility to avoid breaking the existing libraries team management view in the admin dashboard, mostly so we don't have a broken state while the new frontend view is fully implemented and merged. |
rodmgwgu
left a comment
There was a problem hiding this comment.
Looking good, please update the CHANGELOG and version.
8a41ef9 to
fdae762
Compare
Done! Bumped the version and added the entry to the CHANGELOG. |
Thanks!, it seems there are some conflicts yet caused by other merges, sorry for that. Can you update that please? |
Accept a new list field alongside the existing string, allowing role assignment across multiple scopes in one request while keeping full backward compatibility. Each response entry now includes a field.
…copeValidationMixin
fdae762 to
2d5ab84
Compare
Done! Conflicts resolved and branch updated |
| for user_identifier in data["users"]: | ||
| response_dict = {"user_identifier": user_identifier, "scope": scope_value} | ||
| try: | ||
| user = get_user_by_username_or_email(user_identifier) |
There was a problem hiding this comment.
Just a question: what happens if the user is not activated? Should we assign them a role?
There was a problem hiding this comment.
That's a good question, in this case, this logic already existed and we don't have a specific requirement for this, perhaps we should create a tech debt issue to review this.
rodmgwgu
left a comment
There was a problem hiding this comment.
Thanks for attending this, LGTM!
Made-with: Cursor
2b39ec6 to
f47d361
Compare
mariajgrimaldi
left a comment
There was a problem hiding this comment.
Thanks for addressing all my comments!
Description
Adds bulk scope support to the existing
PUT /api/authz/v1/roles/users/endpoint.Previously, the endpoint only accepted a single
scopestring, requiring one request per scope when assigning a role across multiple scopes. This change introduces a newscopeslist field that allows assigning a role to one or more users across multiple scopes in a single request.Closes #231
Changes
AddUsersToRoleWithScopeSerializer: madescopeoptional and added ascopeslist field. Thevalidate()method normalizes both formats into a unifiedscopeslist, validating existence and role availability for every scope.RoleUserAPIView.put(): updated to iterate overdata["scopes"], including ascopefield in every completed/error response entry.scopesfield, rejection of ambiguous inputs (both/neither field provided), and verification that response entries carry thescopefield.Backward Compatibility
The existing
scope(single string) field continues to work exactly as before. Existing clients are unaffected — the only additive change to the response is ascopefield on each entry.Old payload (still supported):
{ "users": ["contributor"], "role": "library_author", "scope": "lib:WGU:CSPROB" }New payload:
{ "users": ["contributor"], "role": "library_author", "scopes": ["lib:WGU:CSPROB", "lib:WGU:OTHER"] }Response (both formats):
{ "completed": [ {"user_identifier": "contributor", "scope": "lib:WGU:CSPROB", "status": "role_added"}, {"user_identifier": "contributor", "scope": "lib:WGU:OTHER", "status": "role_added"} ], "errors": [] }