-
Notifications
You must be signed in to change notification settings - Fork 125
Improve permissions UI for users. #2198
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
Improve permissions UI for users. #2198
Conversation
Summary of ChangesHello @anup39, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the user interface for permission management by implementing a more robust and secure system. It ensures that users are only presented with permission options they are authorized to modify, specifically for anonymous and registered user groups. By dynamically filtering available choices and making unauthorized fields read-only, the change improves both the clarity and integrity of the permission settings within the application. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a valuable UI improvement by filtering the available permission options based on the user's management rights. The logic is encapsulated in new helper functions within ResourceUtils.js, which is a good approach. My review focuses on improving the robustness and maintainability of these new functions and ensuring they are adequately tested. I've suggested adding more specific JSDoc types, avoiding argument mutation to prevent side effects, and adding a default parameter value. Most importantly, I've highlighted the need for comprehensive unit tests to cover the new permission filtering logic, which is currently missing.
| const groups = []; | ||
| const permissionOptions = getResourcePermissions(data[0].allowed_perms.compact, groups); |
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 test for getResourcePermissions has been updated for the new function signature, but the new permission filtering logic is not covered by any tests. It is crucial to add tests for this new functionality to ensure it works correctly and to prevent future regressions.
Please add test cases that cover:
- A user who can manage permissions (options should not be filtered).
- A user who cannot manage permissions (options should be filtered to only the current permission).
- Scenarios for both
anonymousandregistered-membersgroups. - Edge cases, such as when a group's current permission is not set.
| * @param {*} groupName | ||
| * @param {*} groups |
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.
For better code clarity and maintainability, it's recommended to use specific types in JSDoc instead of the generic *. For instance, groupName is a string and groups is an array of group objects.
| * @param {*} groupName | |
| * @param {*} groups | |
| * @param {string} groupName | |
| * @param {object[]} groups |
| */ | ||
| export const getResourcePermissions = (options) => { | ||
| const permissionsOptions = {}; | ||
| export const getResourcePermissions = (options , groups, manageAnonymousPermissions=false, manageRegisteredMemberPermissions=false) => { |
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 getResourcePermissions function has default values for manageAnonymousPermissions and manageRegisteredMemberPermissions, but not for groups. To improve robustness and make the function signature clearer, consider adding a default value for groups, such as an empty array.
| export const getResourcePermissions = (options , groups, manageAnonymousPermissions=false, manageRegisteredMemberPermissions=false) => { | |
| export const getResourcePermissions = (options , groups = [], manageAnonymousPermissions=false, manageRegisteredMemberPermissions=false) => { |
| filterGroupPermissions(options, groups, 'anonymous', manageAnonymousPermissions); | ||
| filterGroupPermissions(options, groups, 'registered-members', manageRegisteredMemberPermissions); | ||
| let permissionsOptions = {}; | ||
| Object.keys(options).forEach((key) => { |
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 filterGroupPermissions function mutates the options object. It's a good practice to avoid mutating function arguments to prevent potential side effects. Consider creating a copy of the options object before filtering to ensure the original object remains unchanged.
| filterGroupPermissions(options, groups, 'anonymous', manageAnonymousPermissions); | |
| filterGroupPermissions(options, groups, 'registered-members', manageRegisteredMemberPermissions); | |
| let permissionsOptions = {}; | |
| Object.keys(options).forEach((key) => { | |
| const optionsCopy = JSON.parse(JSON.stringify(options)); | |
| filterGroupPermissions(optionsCopy, groups, 'anonymous', manageAnonymousPermissions); | |
| filterGroupPermissions(optionsCopy, groups, 'registered-members', manageRegisteredMemberPermissions); | |
| let permissionsOptions = {}; | |
| Object.keys(optionsCopy).forEach((key) => { |
|
@anup39 this is blocked with no reviewers assigned. |
| */ | ||
|
|
||
| import React from 'react'; | ||
| import PropTypes from 'prop-types'; |
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.
Note previous PR moved code about share to another component #2204
We should not include the plugin again if it was removed previously. Now share is a tab in resources details component see DetailsShare.jsx. Please review the code
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.
Done
| const filterGroupPermissions = (options, groups, groupName, canManage) => { | ||
| if (!canManage && options[groupName]) { | ||
| const permissionValue = getPermissionForGroup(groupName, groups); | ||
| const currentPermission = options[groupName].find(p => p.name === permissionValue); | ||
| if (currentPermission) { | ||
| options[groupName] = [currentPermission]; | ||
| } | ||
| } | ||
| }; |
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.
let's avoid to mutate the argument object and return a new object instead, see the example below (please double check the code):
We could also try to check all groups in same function
const filterGroupPermissions = (options, groups, groupNames) => {
return groupNames.length
? Object.fromEntries(Object.keys(options)
.map((key) => {
if (groupNames.some(name => name === key)) {
const permissionValue = getPermissionForGroup(key, groups);
const currentPermission = options[key].find(p => p.name === permissionValue);
return currentPermission ? [key, currentPermission] : [key, options[key]]
}
return [key, options[key]]
}))
: options;
};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.
Done
| filterGroupPermissions(options, groups, 'anonymous', manageAnonymousPermissions); | ||
| filterGroupPermissions(options, groups, 'registered-members', manageRegisteredMemberPermissions); |
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.
based on previous comment, please double check this
const options = filterGroupPermissions(_options, groups, [
...(manageAnonymousPermissions ? [] : ['anonymous']),
...(manageRegisteredMemberPermissions? [] : ['registered-members']),
])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.
Done
This pull request refactors the permission handling logic to ensure that the UI accurately reflects a user's ability to manage permissions for a resource. Specifically, it restricts the available permission options for the "Anyone" (anonymous) and "Registered users" (registered-members) groups if the current user does not have the appropriate management rights.
The Problem
Previously, the permissions dropdown in the "Share" panel would always show all possible permission levels (e.g., "None", "View", "Download"), even if the user lacked the can_manage_anonymous_permissions or can_manage_registered_member_permissions rights.
The Solution
The logic has been updated to dynamically filter the list of available permissions.
filterGroupPermissionsUtility: A new, reusable helper functionfilterGroupPermissionshas been added toResourceUtils.js. This function is responsible for the core filtering logic.getResourcePermissionsfunction now uses this helper to process options for both the anonymous and registered-members groups before returning the final list to the UI component.As a result, if a user cannot manage permissions for a group, the dropdown for that group will appear as a read-only field displaying the current permission, preventing any attempts to change it.
This array represents the currently saved permissions for the resource.(from compactPermissions.groups)
(Initial permission options from the API)
This object contains all possible permission levels for each group type before filtering.
Flow Diagram

The following diagram illustrates the new logic implemented: