-
Notifications
You must be signed in to change notification settings - Fork 4
feat: [FC-0099] allow deleting user's roles #13
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. |
2dd55c8 to
b1ea8ef
Compare
| }, | ||
| onError: (error) => { | ||
| // eslint-disable-next-line no-console | ||
| console.error('Failed to revoke user role:', error); |
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.
use the logger for frontend-platform instead
b1ea8ef to
95af7f8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13 +/- ##
==========================================
+ Coverage 93.66% 93.86% +0.20%
==========================================
Files 41 43 +2
Lines 710 799 +89
Branches 147 145 -2
==========================================
+ Hits 665 750 +85
- Misses 42 46 +4
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 roles is correctly deleted, just a minor comment but it LGTM!
| actions={ | ||
| showDelete && <IconButton variant="danger" alt="Delete role action" src={Delete} /> | ||
| handleDelete && ( | ||
| <IconButton variant="danger" onClick={handleDelete} alt="Delete role action" src={Delete} /> |
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.
The alt should be an intl messsage.
f377944 to
12e2558
Compare
arbrandes
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.
The feature works and the code is mostly fine, thank you!
I would like to see us avoid getByTestId unless there's no other way, though. Also, there's that alt that requires internationalization.
| const user = userEvent.setup(); | ||
| renderComponent(); | ||
|
|
||
| const deleteButton = screen.getByTestId('delete-role-Admin'); |
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.
Can we please avoid getByTestId wherever possible? (See the docs.)
3db08f1 to
582b141
Compare
…erRoles functionality
582b141 to
64eb726
Compare
64eb726 to
de47612
Compare
|
@arbrandes I addressed the comments |
arbrandes
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.
Thank for addressing the review!
Warning
This PR depends on #6 because it manages the users' roles view
This pull request allows deleting users' roles from the users' roles view
Evidence
Screencast.from.15-10-25.17.53.05.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_policiesEditbutton at the end of the user rowAdd New Role, fill the form, and click saveAdditional Information
openedx/openedx-authz#45