-
Notifications
You must be signed in to change notification settings - Fork 4
feat: [FC-0099] add button & modal for adding new team members #3
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
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. 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
b0c02c9 to
937dacb
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3 +/- ##
==========================================
+ Coverage 89.96% 91.89% +1.93%
==========================================
Files 25 28 +3
Lines 269 395 +126
Branches 34 54 +20
==========================================
+ Hits 242 363 +121
- Misses 27 31 +4
- Partials 0 1 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sarina, this PR is ready, thanks! |
src/types.ts
Outdated
| slug: string; | ||
| } | ||
|
|
||
| export interface TeamRole { |
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.
Important
The API contract for getting the roles and permissions is under validation here openedx/openedx-authz#16 (comment), waiting for the backend team feedback, it should not impact hardly what is managed in this PR.
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.
Regarding to this we decided that for the MVP the backend will retrieve only the mapping of roles and permission and any extra data will leave in the frontend (short-term). Here the PR with the metadata information #5
@bra-i-am hope for your review there and I hope the changes here are minimal after that.
|
@bra-i-am can you please ensure your PR title contains the FC number (FC-99 in this case) when you're working on Axim funded contribution projects? This helps the team who triages incoming PRs to the project know we have these under control. Thanks! |
src/authz-module/data/api.ts
Outdated
| return camelCaseObject(res.data); | ||
| }; | ||
|
|
||
| export const getTeamRoles = async (libraryId: string): Promise<TeamRole[]> => { |
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.
[enhancement] not blocking for the current iteration but this endpoint can be used for other scopes within the AuthZ module, a more generic parameter is better.
src/authz-module/data/hooks.ts
Outdated
| }); | ||
|
|
||
| /** | ||
| * React Query hook to add new team members to a specific library. |
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.
Same comment for both hooks https://github.com/openedx/frontend-app-admin-console/pull/3/files#r2389701104
src/authz-module/data/hooks.ts
Outdated
| * | ||
| * @example | ||
| * ```tsx | ||
| * const { data: teamRoles, isLoading, isError } = useTeamRoles('lib:123'); |
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.
[not blocking]
| * const { data: teamRoles, isLoading, isError } = useTeamRoles('lib:123'); | |
| * const { data: teamRoles } = useTeamRoles('lib:123'); |
useSuspenseQuery does not return isLoading, isError , those are managed by Suspense and ErrorBoundary respectively.
| <ModalDialog.CloseButton variant="tertiary" disabled={isLoading}> | ||
| {intl.formatMessage(messages['libraries.authz.manage.add.member.cancel.button'])} | ||
| </ModalDialog.CloseButton> | ||
| <Button |
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.
[enhancement] not blocking, Paragon has a component to work with buttons and states https://paragon-openedx.netlify.app/components/statefulbutton/
| })); | ||
| }; | ||
|
|
||
| const handleAddTeamMember = () => { |
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.
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.
Solved 👍
| /> | ||
| )} | ||
|
|
||
| {additionMessage && ( |
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.
[enhancement] In the future should be reusable component across the application, for know is perfect, thank you
[FC-99]
[FC-99]e3fae1d to
1d168cd
Compare
| <AddNewTeamMemberModal | ||
| isOpen={isOpen} | ||
| isError={isError} | ||
| close={close} |
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 think a extra function is needed here to reset the state when a error happens, something like:
const handleClose = () => {
setFormValues(DEFAULT_FORM_VALUES);
setIsError(false)
close();
}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.
Solved 👍
|
@brian-smith-tcril Screencast.from.2025-10-22.01-24-15.webmHowever, I create a issue for finding a better way to manage the issue #14 |
| render(<AuthZTitle {...defaultProps} actions={actions} />); | ||
|
|
||
| expect(screen.getByRole('button', { name: 'Save' })).toBeInTheDocument(); | ||
| expect(screen.getByTestId('custom-action')).toBeInTheDocument(); |
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.
A getByText query would be more focused on what the user sees on screen, since we are moving away from querying from testId
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 for highlighting this, I will manage all that testId refactors in a separate PR, after the ulmo cut.
Very odd. I just tried again and it's working as expected for me now. Must have been an environment issue on my side. Sorry about that! |
brian-smith-tcril
left a comment
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.
jacobo-dominguez-wgu
left a comment
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.
Tested on the sandbox and looks fine, good work!
…NewTeamMemberTrigger tests
… AddNewTeamMemberTrigger


Warning
This PR depends on #5 for getting the roles and permissions metadata.
TODO: Add a toast when trying to add an already-assigned role to a user
This pull request adds a complete "Add New Team Member" workflow to the Libraries Team Manager, allowing users to add new members to a library and assign them roles.
Main changes
addTeamMembers,getTeamRoles) and types (AddTeamMembersRequest,PutTeamMembersResponse,TeamRole) to support adding team members and fetching available roles. [1] [2]useAddTeamMember,useTeamRoles) for adding team members and fetching roles.AddNewTeamMemberTrigger(button and toast),AddNewTeamMemberModal(form and modal), and integrated them into the team manager page. [1] [2] [3]LibraryAuthZProvider) and props to use the newTeamRoletype for richer role information, ensuring role descriptions and user counts are available throughout the UI. [1] [2]AuthZTitlecomponent to accept React elements as actions, enabling the new trigger button and future extensibility. [1] [2]Evidence
Screencast.from.15-10-25.16.45.16.webm
Testing instructions
npm run devtutor dev exec lms pip install git+https://github.com/openedx/openedx-authztutor dev exec lms python manage.py lms migrate openedx_authztutor dev exec lms python manage.py lms load_policiesAdditional Information