Feat disable duplicate user or group permission #242
Conversation
Signed-off-by: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
wanglam
force-pushed
the
feat-disable-duplicate-user-or-group-permission
branch
from
d203dd4 to
ab9465e
Compare
Codecov Report
@@ Coverage Diff @@
## workspace #242 +/- ##
==========================================
Coverage 66.21% 66.21%
==========================================
Files 3438 3438
Lines 66262 66269 +7
Branches 10691 10693 +2
==========================================
+ Hits 43878 43883 +5
- Misses 19703 19705 +2
Partials 2681 2681
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| permissions: WorkspaceRoutePermissionItem[]; | ||
| } | ||
| attributes: Omit<WorkspaceAttribute, 'id'>, | ||
| permissions: WorkspaceRoutePermissionItem[] |
There was a problem hiding this comment.
Maybe not in the scope, but should permissions be optional? Because user may not have permission control turned on.
There was a problem hiding this comment.
Sure, the permissions could be optional.
| combination.length === permissionModes.length && | ||
| combination.every((mode) => permissionModes.includes(mode)) | ||
| ); | ||
| const isValidatePermissions = (permissions: Permissions) => { |
There was a problem hiding this comment.
If we move convertToACL to workspace_client, and passing the raw permissions(type is WorkspaceRoutePermissionItem[]) to create/update, perhaps we don't need to convert the data here? It would be much easier to validate a flatten array of WorkspaceRoutePermissionItem[].
Not only make it easier to validate the permission mode combinations, but also make it easier to validate the user/group duplications here as well.
There was a problem hiding this comment.
That's a good idea. The WorkspaceRoutePermissionItem[] in route level was only used in the front end page. I kept the ACL parameter in case of passing ACL to workspace client directly. From current implementation, the create or update method only be used in workspace routes. I think we can change to parameter type to WorkspaceRoutePermissionItem[].
There was a problem hiding this comment.
we also can rename WorkspaceRoutePermissionItem to not include word Route so that it's more generic,
Signed-off-by: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
Description
Issues Resolved
Screenshot
Testing the changes
Check List
yarn test:jestyarn test:jest_integrationyarn test:ftr