Conversation
… permissions for folders requires a new role a new role for every folder
…rmission-management
… a new folder group
…till see the process editor view and "edit" in the editor, their actions are just not saved in the backend
This comment has been minimized.
This comment has been minimized.
…rmission-management
|
✅ Successfully created Preview Deployment. |
| ); | ||
|
|
||
| form.setFieldsValue(values); | ||
| }, [groups, form]); |
There was a problem hiding this comment.
I think including form in the dependency array could cause circular updates and I guess it doesn't need to be a dependency. This could also lead to unnecessary re-renders or infinite loops in edge cases. May be try to remove form from the dependencies.
|
|
||
| const values = (await form.validateFields()) as Record< | ||
| string, | ||
| Record<ResourceType, Record<ResourceActionType, boolean>> |
There was a problem hiding this comment.
If validateFields() or handleFolderRoleChanges() fails, then loading will never be set to false and users will not see any error message. Wrapping in try-catch with user feedback and a finally block for loading state might be a better option.
| title="Choose a folder" | ||
| open={true} | ||
| onOk={() => onSubmit(selectedFolders)} | ||
| onCancel={() => onSubmit([])} |
There was a problem hiding this comment.
I am not quite sure what will happen on code level if we submit it with empty array here in this case. Can you please specify what are we trying to achieve with this. Because normally, what I would have done here is to just onCancel={() => onSubmit(initialFolders)} i.e., restoring the initial state
|
|
||
| groups[index].folders.forEach((folder) => { | ||
| if (existingFolderRoles[folder.id]) { | ||
| updates.push({ roleId: existingFolderRoles[folder.id], permissions }); |
There was a problem hiding this comment.
what would happen if a folder is moved from one group to another. I mean, if I have understood it correctly, when a folder X moves from group A to group B, it will be in additions for group B, but the old role from group A won't be in removals unless group A is completely empty. This could create duplicate child roles for the same folder.
| @@ -132,12 +136,15 @@ export const FolderTree = <TContentType extends FolderChildren = FolderChildren> | |||
| const rootFolder = await getFolder(spaceId); | |||
| if ('error' in rootFolder) return; | |||
There was a problem hiding this comment.
Although it’s not related to the changes you made, I just noticed that the error has been silenced. Should not we at least display the error or log it?
| await asyncForEach(toUpdate, async ({ roleId, permissions }) => { | ||
| await _updateRole(roleId, { permissions }, ability, trx); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Is the sequential asyncForEach for removals and updates is really unnecessary? I mean, I think they can run in parallel. what we need sequential is only the addition. So may be, we can use Promise.all([...toRemove.map(...), ...toUpdate.map(...)]) here.
| })); | ||
|
|
||
| await addRoleMappings(roleMappings, ability, trx); | ||
| }); |
There was a problem hiding this comment.
If parentRole.members is empty, no mappings are created, which is correct. But if members are added to the parent role later, existing child roles would not get updated. Having said that, if I have understood it correctly (which I’m not entirely sure about) then for this case, would not we need a separate sync operation when adding members to parent roles? Is this handled elsewhere?
2 participants
Summary
Simplified the UI/UX for settings access rights on specific folders.
Details