Skip to content
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

feature: cannot delete last admin user/group in workspace settings #235

Merged
merged 2 commits into from
Oct 19, 2023
Merged

feature: cannot delete last admin user/group in workspace settings #235

merged 2 commits into from
Oct 19, 2023

Conversation

yuye-aws
Copy link
Collaborator

@yuye-aws yuye-aws commented Oct 18, 2023

Description

In workspace update page, users cannot delete the last user or group with admin permission.

Issues Resolved

Screenshot

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: yuye-aws <yuyezhu@amazon.com>
@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2023

Codecov Report

Merging #235 (4e83670) into workspace (7a72a73) will increase coverage by 0.01%.
Report is 1 commits behind head on workspace.
The diff coverage is 68.29%.

@@              Coverage Diff              @@
##           workspace     #235      +/-   ##
=============================================
+ Coverage      66.19%   66.21%   +0.01%     
=============================================
  Files           3431     3438       +7     
  Lines          65952    66262     +310     
  Branches       10616    10691      +75     
=============================================
+ Hits           43659    43876     +217     
- Misses         19645    19704      +59     
- Partials        2648     2682      +34     
Flag Coverage Δ
Linux_1 30.84% <68.29%> (+0.27%) ⬆️
Linux_2 55.41% <ø> (ø)
Linux_3 42.77% <ø> (ø)
Linux_4 34.52% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/plugins/workspace/common/constants.ts 100.00% <ø> (ø)
...nents/workspace_creator/workspace_cancel_modal.tsx 100.00% <100.00%> (ø)
...components/workspace_creator/workspace_creator.tsx 76.47% <ø> (ø)
...ents/workspace_creator/workspace_icon_selector.tsx 100.00% <100.00%> (ø)
src/plugins/workspace/public/plugin.ts 40.29% <ø> (ø)
...ponents/workspace_creator/workspace_bottom_bar.tsx 75.00% <75.00%> (ø)
...ic/components/workspace_creator/workspace_form.tsx 71.42% <74.60%> (ø)
...ace_creator/workspace_permission_setting_panel.tsx 57.95% <51.16%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines 362 to 363
const [userNonDeletableIndex, setUserNonDeletableIndex] = useState<number>(-1);
const [groupNonDeletableIndex, setGroupNonDeletableIndex] = useState<number>(-1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two states are derived values from userPermissionSettings, groupPermissionSettings, I do not think it is a good practice to declare these two states. You can get these two value by using

const { userNonDeletableIndex, groupNonDeletableIndex } = useMemo(() => {}, [userPermissionSettings, groupPermissionSettings]);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I will update this.

Signed-off-by: yuye-aws <yuyezhu@amazon.com>
@yuye-aws yuye-aws merged commit 1ac7ec4 into ruanyl:workspace Oct 19, 2023
23 checks passed
@yuye-aws yuye-aws deleted the feature/last_admin_not_deletable branch October 19, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants