Skip to content

feat(authz): add step 1 for the role assignment wizard#109

Merged
jacobo-dominguez-wgu merged 1 commit intoopenedx:masterfrom
eduNEXT:bc/add-role-assignment-wizard
Apr 21, 2026
Merged

feat(authz): add step 1 for the role assignment wizard#109
jacobo-dominguez-wgu merged 1 commit intoopenedx:masterfrom
eduNEXT:bc/add-role-assignment-wizard

Conversation

@bra-i-am
Copy link
Copy Markdown
Contributor

@bra-i-am bra-i-am commented Mar 25, 2026

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:

  • A new /assign-role route with a two-step Stepper component (AssignRoleWizardPage + AssignRoleWizard)
  • Step 1 — Who and Role: users can enter one or more usernames/emails (comma-separated) and select a role. Invalid users are highlighted inline in red via HighlightedUsersInput, a custom textarea with a synchronized overlay layer
  • User validation via a new POST /api/authz/v1/users/validate endpoint before advancing to Step 2
  • Role options are filtered based on the current user's actual permissions (MANAGE_LIBRARY_TEAM / MANAGE_COURSE_TEAM), with course roles visible but disabled pending a future update
  • Step 2 — Where it applies is scaffolded but not yet implemented; it will define the application scope in a follow-up PR
  • Constants for library and course roles/permissions are centralized in authz-module/constants.ts and re-exported where needed
  • The "Assign Role" button in LibrariesTeamManager now navigates to the wizard instead of opening a modal
    Not in scope for this PR: Step 2 implementation, course role assignment.

Additional Information

Resolves #91

Screenshots

image

How to test

1. Open the wizard from the home page
  1. Navigate to /authz.
  2. Click Assign Role in the top-right header.
  3. Confirm you land on /authz/assign-role and the wizard Step 1 is shown.
2. Open the wizard pre-filled from the user audit view
  1. Navigate to a library team page (/authz/libraries/<lib-id>).
  2. Click Assign Role in the header → confirm the wizard opens at /authz/assign-role with the users input empty and from= pointing back to the team page.
  3. Go back, click on any team member edit action to open their audit page (/authz/libraries/<lib-id>/<username>).
  4. Click Assign Role in the header → confirm the wizard opens with that team member's username already filled in the users input.
3. Role selection
  1. Confirm roles are grouped under Courses and Libraries.
  2. Select a library role, then click a course role — confirm only one radio is checked at a time.
  3. Confirm Course Editor and Course Auditor are visible but disabled (greyed out with a tooltip).
4. Users input — invalid user
  1. Type a username that does not exist, select any role, click Next.
  2. Confirm the invalid username is highlighted in red inside the input.
  3. Confirm the message "X username(s) not associated with an account" appears below.
  4. Edit the input → confirm the red highlight and error message both disappear immediately.
5. Advance to Step 2
  1. Type a valid username, select a role, and click Next.
  2. Confirm the wizard advances to Step 2.
  3. Click Back → confirm you return to Step 1 with the username and role still selected.
6. Cancel and breadcrumb return to the originating view
  1. Open the wizard from /authz, click Cancel → confirm you land on /authz.
  2. Open the wizard from a user audit page, click Cancel → confirm you return to that user audit page, not /authz.
  3. Repeat step 2 but click the breadcrumb link instead of Cancel.

AssignRoleWizard — Step 1 Test Coverage

Status Test
Cancel returns to the previous view
opens with the user pre-populated when provided via initialUsers
shows only course roles when user only has manage_course_team permission
shows only library roles when user only has manage_library_team permission
shows both course and library roles when user has both permissions
selecting a different role replaces the previous selection
blocks progression and highlights the unknown user
advances to Step 2 when all users are valid
shows a network error and blocks progression when the validation call fails
clears the invalid user highlights when the input is edited

@openedx-webhooks
Copy link
Copy Markdown

openedx-webhooks commented Mar 25, 2026

Thanks for the pull request, @bra-i-am!

This repository is currently maintained by @openedx/committers-frontend.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.
🔘 Provide context

To 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:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads
🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details
Where 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:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 25, 2026
@github-project-automation github-project-automation Bot moved this to Needs Triage in Contributions Mar 25, 2026
@bra-i-am bra-i-am force-pushed the bc/add-role-assignment-wizard branch 4 times, most recently from b1d55c0 to e2a71c0 Compare March 26, 2026 14:39
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 26, 2026

Codecov Report

❌ Patch coverage is 96.46465% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.13%. Comparing base (78f78cc) to head (48da926).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...odule/role-assignation-wizard/AssignRoleWizard.tsx 91.78% 6 Missing ⚠️
...-module/libraries-manager/LibrariesTeamManager.tsx 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   95.88%   96.13%   +0.24%     
==========================================
  Files          70       76       +6     
  Lines        1602     1783     +181     
  Branches      337      385      +48     
==========================================
+ Hits         1536     1714     +178     
- Misses         64       67       +3     
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bra-i-am bra-i-am force-pushed the bc/add-role-assignment-wizard branch 2 times, most recently from 3b13e69 to 5c518c8 Compare March 26, 2026 19:39
@mphilbrick211 mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Mar 27, 2026
@bra-i-am bra-i-am force-pushed the bc/add-role-assignment-wizard branch from 6e294c5 to d5990f0 Compare March 30, 2026 13:39
@bra-i-am bra-i-am marked this pull request as ready for review April 13, 2026 14:14
@bra-i-am bra-i-am requested a review from dcoa April 13, 2026 19:34
@bra-i-am bra-i-am force-pushed the bc/add-role-assignment-wizard branch 2 times, most recently from eddffbd to 15be337 Compare April 14, 2026 15:08
@bra-i-am bra-i-am requested a review from arbrandes April 14, 2026 18:47
@bra-i-am bra-i-am force-pushed the bc/add-role-assignment-wizard branch from 319a15a to bf1004c Compare April 14, 2026 21:43
@bra-i-am bra-i-am marked this pull request as draft April 14, 2026 21:51
@bra-i-am bra-i-am force-pushed the bc/add-role-assignment-wizard branch from 4ac83a8 to 49aeb2c Compare April 14, 2026 22:53
@bra-i-am bra-i-am marked this pull request as ready for review April 14, 2026 23:03
@arbrandes
Copy link
Copy Markdown
Contributor

What is the PR (or PRs) that introduces the required endpoints? Has it already merged?

@bra-i-am
Copy link
Copy Markdown
Contributor Author

bra-i-am commented Apr 15, 2026

@arbrandes, sorry for not including it in the cover letter earlier. The PR is openedx/openedx-authz#245 and has already been merged.

@dcoa
Copy link
Copy Markdown
Contributor

dcoa commented Apr 16, 2026

As a minor comment in the UI can we keep the space between the text area and the feedback?

The design

image

Currently

image

Also the background for the form section in the current implementation is white but the design presented in the issue is light - Nothing blocking but I mention it just in case.

Comment thread src/authz-module/wizard/messages.ts Outdated
Comment thread src/authz-module/data/api.test.ts Outdated
Comment thread src/authz-module/wizard/DefineApplicationScopeStep.tsx Outdated
Comment thread src/authz-module/wizard/HighlightedUsersInput.tsx Outdated
Comment thread src/authz-module/wizard/AssignRoleWizard.test.tsx Outdated
Comment thread src/authz-module/wizard/messages.ts Outdated
onChange={setUsers}
invalidUsers={invalidUsers}
placeholder={intl.formatMessage(messages['wizard.step1.users.placeholder'])}
hasNetworkError={!!validationError || invalidUsers.length > 0}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Image

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error (2) When is happened this kind of error? In the proposal the error happen when the user click next and the user is not part of the initiative

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@maguilarUXUI

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I think the best option is a toast with the retry option if is possible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @dcoa and @maguilarUXUI

I already added the toast:

image

Comment thread src/authz-module/constants.test.ts Outdated
Comment thread src/authz-module/roles-permissions/library/constants.ts
Comment thread .eslintrc.js Outdated
@bra-i-am bra-i-am force-pushed the bc/add-role-assignment-wizard branch from e6a5b2c to 12c25ea Compare April 16, 2026 17:17
}
} catch (error) {
setValidationError(intl.formatMessage(messages['wizard.validate.error']));
showErrorToast(error, validateUsersAndProceed);
Copy link
Copy Markdown
Contributor

@jesusbalderramawgu jesusbalderramawgu Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this correct? here you are passing the same function as a retry handler

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jesusbalderramawgu, yees, that's the expected behavior when there is an unexpected error (e.g. 5xx)

#109 (comment)

const handleUsersChange = useCallback((value: string) => {
setInvalidUsers((prev) => (prev.length > 0 ? [] : prev));
setValidationError(null);
setUsers(value);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure but I think you should add here a setValidatedUsers([])
because validateUsers becomes stale if user edits the input after validation

Copy link
Copy Markdown
Contributor Author

@bra-i-am bra-i-am Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is already covered; the only way to reach step 2 (where Save is available and a stale validatedUsers really matters) is through validateUsersAndProceed, which always overwrites validatedUsers with the fresh list. Do you think it is better to change it?

const isError = invalidUsers.length > 0 || !!validationError;

useEffect(() => {
if (isError && usersInputRef.current) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will trigger on any error change even unrelates one, don't you need to refine it a little bit?
something like
if ((invalidUsers.length > 0 || validationError) && usersInputRef.current)

}
}, [isError]);

const canProceed = !!users.trim() && !!selectedRole;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here if we have a string "a,,," it passes right?
maybe you could do something like

const parsed = parseUsers(users);
const canProceed = parsed.length > 0 && !!selectedRole;

onClose();
};

const parseUsers = (input: string): string[] => input
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is recreated on every render, I don't see any dependency so you could jus hoist it outside

)}

{/* Actual textarea — text is transparent when overlay is active */}
<textarea
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing ARIA attributes

)}

{/* Actual textarea — text is transparent when overlay is active */}
<textarea
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, wouldn't be better to use the textarea from paragon? instead of this?

Copy link
Copy Markdown
Contributor Author

@bra-i-am bra-i-am Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not here... this component deliberately uses a raw <textarea> because it needs pixel-perfect alignment between the textarea and the overlay div for the individual usernames red highlighting. Paragon's Form.Control wraps the textarea in extra markup and applies its own styles, which would break that alignment :(

} from 'react';

// Shared styles between overlay and textarea to keep text positions in sync
const INPUT_STYLE: React.CSSProperties = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for styling you should add paragon classes or if you need custom css, you should add it to the scss files

...INPUT_STYLE,
position: 'absolute',
inset: 0,
background: '#fff',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check my comment above about styles

course: intl.formatMessage(messages['wizard.step1.roles.contextLabel.course']),
};

const rolesByContext = roles.reduce<Record<string, RoleMetadata[]>>((acc, role) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has a performance issue, it runs on every render.
you could wrap it in useMemo using roles as dependency

onChange={(e) => setSelectedRole(e.target.value)}
className="pl-4"
>
{rolesByContext[context].map((role) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest this to be safer
(rolesByContext[context] || []).map()


const orderedContexts = CONTEXT_ORDER.filter((ctx) => rolesByContext[ctx]);

const adminUrl = `${getConfig().LMS_BASE_URL}/admin`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could move this outside the component

<div className="select-users-and-role-step">
{/* Users Section */}
<h3 className="text-primary mb-2">{intl.formatMessage(messages['wizard.step1.users.heading'])}</h3>
<Form.Group controlId="users-input">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the form is missing ARIA attributes

Copy link
Copy Markdown

@rodmgwgu rodmgwgu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in my local and functionality looks good, I'll approve once Jesus' comments are addressed. Thanks!

defaultMessage: 'Retry',
description: 'Label for retry button.',
},
'library.authz.manage.assign.role.wizard.button': {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see where this message is being used.

});
}, []);

// TODO: replace with real assignment API call using validatedUsers, selectedRole, selectedScopes
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this TODO still pending?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, this handleSave is addressed in the Step 2 PR

@jacobo-dominguez-wgu
Copy link
Copy Markdown
Contributor

In the figma design, when there is an error, an error icon is shown on the step number, I do not see it implemented.

Figma design:
image

Actual UI:
image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest a more descriptive name for this folder, like role-assignation-wizard or assign-role-wizard, role-assignation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And try to keep the main component files on the root of this folder and for the rest of the components create a /components folder

const [searchParams] = useSearchParams();
const initialUsers = searchParams.get('users') || '';
const raw = searchParams.get('from') ?? '';
const returnTo = raw.startsWith('/') ? raw : ROUTES.HOME_PATH;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested by Claude: In AssignRoleWizardPage.tsx, there's a check raw.startsWith('/') to prevent external URLs. However, //evil.com also starts with / and would be treated as a protocol-relative URL if ever used in an or
window.location. While navigate() from react-router likely handles this safely, the validation should be stricter:

// Current
const returnTo = raw.startsWith('/') ? raw : ROUTES.HOME_PATH;

// Safer — reject protocol-relative URLs
const returnTo = (raw.startsWith('/') && !raw.startsWith('//')) ? raw : ROUTES.HOME_PATH;

The test covers https://evil.com but not //evil.com.

.map((u) => u.trim())
.filter(Boolean);

const validateUsersAndProceed = async () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should guard here and check for validateUsersMutation.isPending, to avoid double invocation which can happen when the user clicks quickly, which could happen even if we are disabling the button (there is a delay while React re-renders the button as disabled), or when this is called on the retry toast.

@@ -26,10 +26,18 @@ export const CONTENT_LIBRARY_PERMISSIONS = {
// Note: this information will eventually come from the backend API
// but for the MVP we decided to manage it in the frontend
export const libraryRolesMetadata: RoleMetadata[] = [
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also defined in src/authz-module/constants.ts, I think we should keep just one.

Comment thread src/authz-module/constants.ts Outdated

export const DEFAULT_FILTER_PAGE_SIZE = 5;
// Course Permission Keys
export const COURSE_PERMISSIONS = {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this being used in this PR, can we remove?

@bra-i-am
Copy link
Copy Markdown
Contributor Author

@rodmgwgu, @jesusbalderramawgu, @jacobo-dominguez-wgu thank you for your help reviewing this — I’ve addressed the feedback you shared. Please let me know if there’s anything else that should be resolved.

I’ll let the tests run, and I’ll take care of any issues tomorrow morning if anything comes up.

@bra-i-am bra-i-am force-pushed the bc/add-role-assignment-wizard branch from d7d37a7 to ed75ec9 Compare April 21, 2026 03:13
Copy link
Copy Markdown
Contributor

@jacobo-dominguez-wgu jacobo-dominguez-wgu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much! I have tested it with manual valid and invalid usernames and emails input and with a preset user everything seems to be working well.

@jacobo-dominguez-wgu
Copy link
Copy Markdown
Contributor

Lets wait til tomorrow for @rodmgwgu´s comments before merging.

const intl = useIntl();
const { showErrorToast } = useToastManager();
const [activeStep, setActiveStep] = useState<StepKey>(STEPS.SELECT_USERS_AND_ROLE);
const [users, setUsers] = useState(initialUsers);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit in this component, so many local states can be better handled with a useReducer or even use react context, but lets do that in an upcoming issue.

… naming

Introduce a multi-step AssignRoleWizard with DefineApplicationScopeStep,
SelectUsersAndRoleStep, and HighlightedUsersInput components. Singularize
roles-permissions subfolders (courses → course, libraries → library) for
naming consistency. Update hooks, API, constants, and related tests.
@bra-i-am bra-i-am force-pushed the bc/add-role-assignment-wizard branch from ed75ec9 to 48da926 Compare April 21, 2026 13:13
Copy link
Copy Markdown
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank for the test instructions, everything works exactly as described!

Copy link
Copy Markdown

@rodmgwgu rodmgwgu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jacobo-dominguez-wgu jacobo-dominguez-wgu merged commit f1371b8 into openedx:master Apr 21, 2026
6 checks passed
@github-project-automation github-project-automation Bot moved this from Waiting on Author to Done in Contributions Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Task - RBAC AuthZ - US: M2.8 Assign role wizard - Step 1 usernames and role

9 participants