feat(authz): expand permissions view#125
feat(authz): expand permissions view#125jacobo-dominguez-wgu merged 26 commits intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @jesusbalderramawgu! 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. |
7708f60 to
979521b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #125 +/- ##
==========================================
+ Coverage 96.13% 96.20% +0.07%
==========================================
Files 76 81 +5
Lines 1783 1869 +86
Branches 351 418 +67
==========================================
+ Hits 1714 1798 +84
- Misses 67 68 +1
- Partials 2 3 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I rebased this branch with the latest, made a minor fixes and added the proper tests. |
|
I see some diferencies with the Figma design related to Global Staff, roles, I see that in the design, these roles are highlighted in gray, and show "Partial Access" under Permissions instead of "0 permissions available" as it's now. Also, there seems to be some extra space at the bottom of the message for these cases that shouldn't be there. |
|
|
||
| type CellProps = ExpandableTableRow<UserRole>; | ||
|
|
||
| export const ViewAllPermissionsCell = ({ row }: CellProps) => { |
There was a problem hiding this comment.
Move the components on this file to src/authz-module/components/TableCells.tsx to have all the custom table cells in the same place.
| @@ -0,0 +1,24 @@ | |||
| import { useIntl } from '@edx/frontend-platform/i18n'; | |||
There was a problem hiding this comment.
Could you move all the subcomponents created inside src/authz-module/audit-user folder into src/authz-module/audit-user/components/ so it looks more organized?
There was a problem hiding this comment.
Thank you, I have updated this also I moved the components from the CustomCells component to TableCells component
| @@ -1,5 +1,5 @@ | |||
| import { renderHook, act } from '@testing-library/react'; | |||
| import { QuerySettings } from '@src/authz-module/data/api'; | |||
| import { QuerySettings } from '../data/api'; | |||
There was a problem hiding this comment.
I think it was better to use the absolute path for the import.
| @@ -1,5 +1,5 @@ | |||
| import { useCallback, useState } from 'react'; | |||
| import { QuerySettings } from '@src/authz-module/data/api'; | |||
| import { QuerySettings } from '../data/api'; | |||
thank you @rodmgwgu this has been fixed, the background stopped working due to another PR |
rodmgwgu
left a comment
There was a problem hiding this comment.
Thanks for this work, I requested some changes on details I found.
| import { useQuerySettings } from '@src/authz-module/hooks/useQuerySettings'; | ||
| import { useUserAssignedRoles } from '@src/authz-module/data/hooks'; | ||
| import messages from './messages'; | ||
| import UserPermissions from '../components/UserPermissions'; |
There was a problem hiding this comment.
this has been fixed!
| libraryPermissions, | ||
| rolesLibraryObject, | ||
| } from '../libraries/constants'; | ||
| import RenderPermissionColumn from '../components/RenderPermissionColumn'; |
There was a problem hiding this comment.
Same in these, I'm not sure what's the project standard.
There was a problem hiding this comment.
this has been fixed!
| @@ -0,0 +1,22 @@ | |||
| import { useIntl } from '@edx/frontend-platform/i18n'; | |||
| import messages from '../audit-user/messages'; | |||
There was a problem hiding this comment.
this has been fixed!
| @@ -0,0 +1,95 @@ | |||
| import { DJANGO_MANAGED_ROLES } from '@src/authz-module/constants'; | |||
There was a problem hiding this comment.
I see this file (UserPermissions.tsx) also exists under components, and this one seems to not be used, please remove if not needed.
There was a problem hiding this comment.
this has been fixed!
There was a problem hiding this comment.
not anymore I have removed it
| const expanded = (instance as any)?.state?.expanded || {}; | ||
| Object.keys(expanded).forEach(rowId => { | ||
| if (rowId !== row.id && expanded[rowId]) { | ||
| (instance as any).toggleRowExpanded?.(rowId, false); |
There was a problem hiding this comment.
Could we avoid using any here? this may silently break in the future if the Paragon DataTable changes in the future.
There was a problem hiding this comment.
thank you! I made the change!
| description: 'Description for the permissions of the Super Admin role', | ||
| }, | ||
| 'authz.user.table.permissions.role.staff': { | ||
| id: 'authz.user.table.permissions.role.staff ', |
There was a problem hiding this comment.
There is a trailing whitespace here, I guess it shouldn't be here.
There was a problem hiding this comment.
this has been fixed!
| }, | ||
| ]; | ||
|
|
||
| export const DEFAULT_TOAST_DELAY = 5000; |
There was a problem hiding this comment.
These constants seem to be repeated in libraries/constants and in src/authz-module/constants.ts, let's have them in just one place.
There was a problem hiding this comment.
this has been fixed!
| queryFn: async () => getUserAccount(username), | ||
| retry: false, | ||
| enabled: !!username, | ||
| refetchOnWindowFocus: false, |
There was a problem hiding this comment.
Was this change intentional? usually we don't want to refetch user information on every focus.
bra-i-am
left a comment
There was a problem hiding this comment.
It works as expected, and I see the code fine! I mainly point to the texts, but I think they are going to be received translated from the backend in the future, aren't they?
Additionally, I think it would be cool if the cursor were pointer on ViewMoreLink hover, and avoid the text 0 permissions available when the user has Global scope permissions, as it is contradictory.
Isn't this going to be added by now?
There was a problem hiding this comment.
I saw there is similar data at src/frontend-app-admin-console/src/authz-module/roles-permissions/courses/constants.ts
are we going to delete that file in favor of this?
| { | ||
| key: CONTENT_COURSE_PERMISSIONS.EDIT_COURSE_FILES, | ||
| resource: 'course_files', | ||
| description: 'Permanently remove files and assets from the course.', |
There was a problem hiding this comment.
| description: 'Permanently remove files and assets from the course.', | |
| description: 'Perform non-destructive actions on files, such as locking or unlocking them.', |
| key: CONTENT_LIBRARY_PERMISSIONS.EDIT_LIBRARY_COLLECTION, resource: 'library_collection', label: 'Publish', description: 'Update the name and contents of existing collections.', icon: EditOutline, | ||
| }, | ||
| { | ||
| key: CONTENT_LIBRARY_PERMISSIONS.DELETE_LIBRARY_COLLECTION, resource: 'library_collection', label: 'Edit', description: 'Permanently remove collections from the library.', icon: EditOutline, |
There was a problem hiding this comment.
| key: CONTENT_LIBRARY_PERMISSIONS.DELETE_LIBRARY_COLLECTION, resource: 'library_collection', label: 'Edit', description: 'Permanently remove collections from the library.', icon: EditOutline, | |
| key: CONTENT_LIBRARY_PERMISSIONS.DELETE_LIBRARY_COLLECTION, resource: 'library_collection', label: 'Delete', description: 'Permanently remove collections from the library.', icon: Delete, |
| { | ||
| key: CONTENT_COURSE_PERMISSIONS.MANAGE_COURSE_CERTIFICATES, | ||
| resource: 'course_advanced_certificates', | ||
| description: 'Access and edit the Advanced Settings page in Studio. This covers a wide range of technical course configurations, including proctoring, timed exams, LTI tools, enrollment limits, and custom display options.', |
There was a problem hiding this comment.
| description: 'Access and edit the Advanced Settings page in Studio. This covers a wide range of technical course configurations, including proctoring, timed exams, LTI tools, enrollment limits, and custom display options.', | |
| description: 'Create and edit course certificates, including certificate design and eligibility settings.', |
| { | ||
| key: CONTENT_COURSE_PERMISSIONS.VIEW_COURSE_DETAILS, | ||
| resource: 'course_schedule_details', | ||
| description: 'See course information including the course summary, pacing, and prerequisites..', |
There was a problem hiding this comment.
| description: 'See course information including the course summary, pacing, and prerequisites..', | |
| description: 'See course information including the course summary, pacing, and prerequisites.', |
| { | ||
| key: CONTENT_COURSE_PERMISSIONS.DELETE_COURSE_FILES, | ||
| resource: 'course_files', | ||
| description: 'Delete files.', |
There was a problem hiding this comment.
| description: 'Delete files.', | |
| description: 'Permanently remove files and assets from the course.', |
| key: CONTENT_LIBRARY_PERMISSIONS.MANAGE_LIBRARY_TEAM, resource: 'library_team', label: 'Manage', description: 'Add, change, or remove role assignments for this library from the Roles and Permissions console.', icon: Settings, | ||
| }, | ||
| { | ||
| key: CONTENT_LIBRARY_PERMISSIONS.CREATE_LIBRARY_COLLECTION, resource: 'library_collection', label: 'View', description: 'Create new collections to organize content within the library.', icon: RemoveRedEye, |
There was a problem hiding this comment.
| key: CONTENT_LIBRARY_PERMISSIONS.CREATE_LIBRARY_COLLECTION, resource: 'library_collection', label: 'View', description: 'Create new collections to organize content within the library.', icon: RemoveRedEye, | |
| key: CONTENT_LIBRARY_PERMISSIONS.CREATE_LIBRARY_COLLECTION, resource: 'library_collection', label: 'Create', description: 'Create new collections to organize content within the library.', icon: RemoveRedEye, |
| key: CONTENT_LIBRARY_PERMISSIONS.CREATE_LIBRARY_COLLECTION, resource: 'library_collection', label: 'View', description: 'Create new collections to organize content within the library.', icon: RemoveRedEye, | ||
| }, | ||
| { | ||
| key: CONTENT_LIBRARY_PERMISSIONS.EDIT_LIBRARY_COLLECTION, resource: 'library_collection', label: 'Publish', description: 'Update the name and contents of existing collections.', icon: EditOutline, |
There was a problem hiding this comment.
| key: CONTENT_LIBRARY_PERMISSIONS.EDIT_LIBRARY_COLLECTION, resource: 'library_collection', label: 'Publish', description: 'Update the name and contents of existing collections.', icon: EditOutline, | |
| key: CONTENT_LIBRARY_PERMISSIONS.EDIT_LIBRARY_COLLECTION, resource: 'library_collection', label: 'Edit', description: 'Update the name and contents of existing collections.', icon: EditOutline, |
| { | ||
| key: CONTENT_COURSE_PERMISSIONS.MANAGE_COURSE_ADVANCED_SETTINGS, | ||
| resource: 'course_advanced_certificates', | ||
| description: 'Access and edit the Advanced Settings page in Studio. This covers a wide range of technical course configurations, including proctoring, timed exams, LTI tools, enrollment limits, and custom display options..', |
There was a problem hiding this comment.
| description: 'Access and edit the Advanced Settings page in Studio. This covers a wide range of technical course configurations, including proctoring, timed exams, LTI tools, enrollment limits, and custom display options..', | |
| description: 'Access and edit the Advanced Settings page in Studio. This covers a wide range of technical course configurations, including proctoring, timed exams, LTI tools, enrollment limits, and custom display options.', |
| @import "~@edx/frontend-component-header/dist/index"; | ||
| @use "@openedx/paragon/dist/core.min.css"; | ||
| @use "@openedx/paragon/dist/light.min.css"; | ||
| @import "~@edx/frontend-component-header/dist/index"; No newline at end of file |
There was a problem hiding this comment.
| @import "~@edx/frontend-component-header/dist/index"; | |
| @import "~@edx/frontend-component-header/dist/index"; | |
Thank you @bra-i-am I have addressed your comments, I just didn't change the texts because the ones that I have right now were specified in the task |
jacobo-dominguez-wgu
left a comment
There was a problem hiding this comment.
LGTM and works as expected!
…vements on the header
16022e5 to
0f4610a
Compare






Description
this PR closes this task
this needs to be reviewed once Jacobo's PR is merged
includes code from
Update this PR has been merged