feat(authz): add step 1 for the role assignment wizard#109
feat(authz): add step 1 for the role assignment wizard#109bra-i-am wants to merge 1 commit intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @bra-i-am! 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. |
b1d55c0 to
e2a71c0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #109 +/- ##
==========================================
+ Coverage 95.35% 95.73% +0.38%
==========================================
Files 53 59 +6
Lines 1055 1244 +189
Branches 208 251 +43
==========================================
+ Hits 1006 1191 +185
- Misses 46 50 +4
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3b13e69 to
5c518c8
Compare
6e294c5 to
d5990f0
Compare
eddffbd to
15be337
Compare
319a15a to
bf1004c
Compare
4ac83a8 to
49aeb2c
Compare
|
What is the PR (or PRs) that introduces the required endpoints? Has it already merged? |
|
@arbrandes, sorry for not including it in the cover letter earlier. The PR is openedx/openedx-authz#245 and has already been merged. |
| onChange={setUsers} | ||
| invalidUsers={invalidUsers} | ||
| placeholder={intl.formatMessage(messages['wizard.step1.users.placeholder'])} | ||
| hasNetworkError={!!validationError || invalidUsers.length > 0} |
There was a problem hiding this comment.
I'm not a big fan of this presentation of the error in terms of the service availability (network errors), because can be confused as a validation error.
I think is better having a separate component for it, maybe a alert on top or maybe the toast message (if still relevant for the redesign)
I would like to have others option including @maguilarUXUI
There was a problem hiding this comment.
Yes, and the validation error is presented as is showing in the proposal design. I'm not discussing that.
What I am talking about is error 500, the validation service is not available. I think those ones should be presented in another way, such as Alert on top instead of the red border and red message at the bottom.
There was a problem hiding this comment.
In this case I think the best option is a toast with the retry option if is possible.
There was a problem hiding this comment.
e6a5b2c to
12c25ea
Compare
980922e to
ecb68bd
Compare
8ef1ba6 to
383375b
Compare
arbrandes
left a comment
There was a problem hiding this comment.
Overall the implementation is solid. But I found a few issues with Claude's help (see below). Also, can you please add some explicit test instructions so I know what to look for?
Acceptance criteria gaps
The PR resolves most of the Step 1 acceptance criteria, but three items from issue #91 are not met:
1. Pre-populate Step 1 when arriving from the user audit view
AC: "When accessing the wizard from the user audit view, the user input in Step 1 is pre-populated with that user."
AssignRoleWizardPage.tsx#L11-L12 reads ?users= off the URL and forwards it to the wizard, but nothing in the diff actually navigates to the wizard with that query param. git diff master -- src/authz-module/libraries-manager/ is empty, so LibrariesUserManager still cannot launch the wizard with a pre-filled user. The plumbing exists; the caller does not.
2. Role groups filtered by the current user's scopes
AC: "A user only sees the groups they have permissions to assign. If they have no library scopes, they do not see libraries. If they have no course scopes, they do not see courses."
AssignRoleWizard.tsx#L16 passes the unconditional union [...courseRolesMetadata, ...libraryRolesMetadata] to the step, and no permission lookup is performed anywhere in the wizard. The two ❌ rows in the PR body's test table (shows only course roles when user only has manage_course_team permission / shows only library roles when user only has manage_library_team permission) confirm this is a known gap.
3. Breadcrumb and Cancel must return to the previous view
AC: "The wizard has a breadcrumb that exits the flow and returns to the previous view at any point." / "A Cancel button is available at the bottom of both steps. Clicking it returns to the previous view."
Both actions hardcode ROUTES.HOME_PATH:
- Breadcrumb:
AssignRoleWizardPage.tsx#L17 - Cancel:
AssignRoleWizardPage.tsx#L24
A user who reaches the wizard from LibrariesUserManager or LibrariesTeamManager will be dropped on /authz rather than returned to the scoped view they came from. Consider navigate(-1) or threading the origin through the wizard URL (e.g., a from= query param) so the exit paths honor the AC.
Issues
1. Drop all console.log statements before landing
Two debug logs remain in the diff and should be removed:
DefineApplicationScopeStep.tsx#L12—console.log(selectedRole, selectedScopes, onScopeToggle);runs on every render once the user reaches Step 2.AssignRoleWizard.tsx#L97-L99—console.log('[AssignRoleWizard] handleSave', ...)is guarded by aneslint-disable-next-lineand should be replaced with aTODOfor the follow-up API call.
2. Network validationError is not cleared when the user edits the input
src/authz-module/wizard/AssignRoleWizard.tsx#L45-L48
const handleUsersChange = useCallback((value: string) => {
setInvalidUsers((prev) => (prev.length > 0 ? [] : prev));
setUsers(value);
}, []);invalidUsers is cleared on edit (covered by the clears the invalid user highlights when the input is edited test), but validationError is not. After a transient network failure the red "An error occurred while validating users..." line remains visible while the user types a new address, which is inconsistent with how invalid-user feedback behaves and can be confusing. Reset validationError here as well.
3. Catch-all * route is declared before ASSIGN_ROLE_WIZARD_PATH
src/authz-module/index.tsx#L33-L34
<Route path="*" element={<NotFoundError />} />
<Route path={ROUTES.ASSIGN_ROLE_WIZARD_PATH} element={<AssignRoleWizardPage />} />React Router v6 ranks matches by specificity regardless of source order, so the wizard route does win and this works today. But declaring "*" in the middle of the list is a footgun: a future edit that (for example) replaces Routes with source-order matching, or adds a child route that shares a prefix, will silently break. Move the catch-all to be the last <Route> in the block.
4. Dead fallback for contextType
src/authz-module/wizard/SelectUsersAndRoleStep.tsx#L48-L49
const rolesByContext = roles.reduce<Record<string, RoleMetadata[]>>((acc, role) => {
const context = role.contextType || 'library';Every entry in courseRolesMetadata and libraryRolesMetadata now sets contextType explicitly, and the only caller passes those arrays, so the || 'library' fallback is unreachable and misleading (a future course role added without contextType would silently render under Libraries). Either drop the fallback or tighten RoleMetadata.contextType to a required field.
26fd465 to
f6b054f
Compare
|
@dcoa and @arbrandes, thank you so much for your time reviewing this PR ✨ @dcoa: I already addressed the changes to keep consistency with @arbrandes: I already addressed the changes you asked for f6b054f. Here is a summary of all the points with a brief description of each one
|
de07733 to
3fa07f3
Compare
7979429 to
d340ee9
Compare
- Added AssignRoleWizard component for assigning roles to users. - Created AssignRoleWizardPage to handle routing and initial user input. - Implemented SelectUsersAndRoleStep for user and role selection with validation. - Developed DefineApplicationScopeStep as a placeholder for defining application scopes. - Introduced HighlightedUsersInput for enhanced user input with invalid user highlighting. - Added tests for all new components to ensure functionality and correctness. - Updated messages for internationalization support. - Enhanced types for RoleMetadata to include contextType and disabled properties.
d340ee9 to
e04f840
Compare




Caution
This PR requires the validation endpoint introduced in the PR openedx/openedx-authz#245
This PR introduces Step 1 of the Role Assignment Wizard, replacing the previous modal-based approach for adding team members.
What's included:
/assign-roleroute with a two-stepSteppercomponent (AssignRoleWizardPage + AssignRoleWizard)HighlightedUsersInput, a custom textarea with a synchronized overlay layerPOST /api/authz/v1/users/validateendpoint before advancing to Step 2authz-module/constants.tsand re-exported where neededLibrariesTeamManagernow navigates to the wizard instead of opening a modalNot in scope for this PR: Step 2 implementation, course role assignment.
Additional Information
Resolves #91
Screenshots
How to test
1. Open the wizard from the home page
/authz./authz/assign-roleand the wizard Step 1 is shown.2. Open the wizard pre-filled from the user audit view
/authz/libraries/<lib-id>)./authz/assign-rolewith the users input empty andfrom=pointing back to the team page.editaction to open their audit page (/authz/libraries/<lib-id>/<username>).3. Role selection
4. Users input — invalid user
5. Advance to Step 2
6. Cancel and breadcrumb return to the originating view
/authz, click Cancel → confirm you land on/authz./authz.AssignRoleWizard — Step 1 Test Coverage
Cancel returns to the previous viewopens with the user pre-populated when provided via initialUsersshows only course roles when user only has manage_course_team permissionshows only library roles when user only has manage_library_team permissionshows both course and library roles when user has both permissionsselecting a different role replaces the previous selectionblocks progression and highlights the unknown useradvances to Step 2 when all users are validshows a network error and blocks progression when the validation call failsclears the invalid user highlights when the input is edited