-
Notifications
You must be signed in to change notification settings - Fork 4
feat(authz): [FC-0099] create toggle to make a library plublic read #25
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
feat(authz): [FC-0099] create toggle to make a library plublic read #25
Conversation
|
Thanks for the pull request, @dcoa! 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #25 +/- ##
==========================================
+ Coverage 93.86% 94.14% +0.27%
==========================================
Files 43 44 +1
Lines 799 837 +38
Branches 159 169 +10
==========================================
+ Hits 750 788 +38
- Misses 46 47 +1
+ Partials 3 2 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7fcea7f to
9d73b3d
Compare
9d73b3d to
b387aba
Compare
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.
Overall this looks great!
I left one comment with a change request that I consider blocking, but the rest of the comments don't need to be addressed for this to land.
Thank you so much for all the great work on this!
| 'libraries.authz.public.read.toggle.success': { | ||
| id: 'libraries.authz.public.read.toggle.success', | ||
| defaultMessage: 'The library setting has been updated successfully.', | ||
| description: 'Sucessfull message for allow public read', |
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.
| description: 'Sucessfull message for allow public read', | |
| description: 'Success message for allow public read', |
| }); | ||
| }); | ||
|
|
||
| describe('useUpdateLibrary', () => { |
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.
These tests appear to only be testing that the title can be updated properly. Not blocking, but it'd be nice to have tests verifying each part of LibraryMetadata that can be updated via this endpoint is updated properly.
218b4a9 to
8ee18c3
Compare
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.
I noticed some strange behavior with the divider after pulling latest and testing locally
Starting large
Screencast.From.2025-10-28.13-40-22.mp4
Starting small
Screencast.From.2025-10-28.13-41-26.mp4
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.
LGTM!
| const { width = window.innerWidth } = useWindowSize(); | ||
| const minWidth = breakpoints.large?.minWidth || 1200; | ||
| const isDesktop = width > minWidth; |
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'm not a huge fan of needing to do this, but I'm OK with it as a workaround.
I tried digging into why useMediaQuery wasn't responding to resizes and it's very strange. In my testing it even seemed to work as expected sometimes but not all, so it really isn't making sense to me.
I know Paragon just passes through useMediaQuery from react-responsive, but I couldn't find any issues on that repo that seemed to match what we're seeing here.
I made a Paragon issue to track this openedx/paragon#3958
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.
Seems like installing react-responsive directly works, but not sure why using the imported version for Paragon does not behaves correctly.
I'm happy to install the library for better workaround, if you agree.
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.
It looks like Paragon's version of react-responsive was updated to v10 in openedx/paragon#3895, which released in Paragon 23.14.8.
This MFE is using 23.14.2
frontend-app-admin-console/package-lock.json
Lines 6973 to 6974 in 8dc3139
| "node_modules/@openedx/paragon": { | |
| "version": "23.14.2", |
Does it work if you update the version of Paragon this MFE is using to ^23.14.8?
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.
It does!! 🎉
29289d8 to
1ce31ac
Compare
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.
Re-approving with the Paragon bump fixing useMediaQuery!
Description
This PR creates a toggle component that updates the library metadata to allow public read. This to keep compatibility with the current functionality in Libraries Manage team modal.
Behavior
Extra information
Evidence
Manage Team Permissions
Only View Permissions
Test Instructions
library_adminrole and the other one withlibrary_userrole