Details

Reviewers

atul
ginsu

Commits

rCOMM9fa20317fb62: [web] Prevent changing the only admin role in a community

Summary

We need to prevent the only admin from removing themselves as Admins in a community. Based on the designs, we should display an error in the modal indicating why the saved role changes failed.

https://linear.app/comm/issue/ENG-4041/prevent-changing-the-only-admin-role-in-a-community

Depends on D8200

Test Plan

Please see the video below covering the cases of changing the only admin role

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Jun 7 2023, 5:11 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJun 7 2023, 5:11 AM

rohan added a child revision: D8142: [lib] Update robotext for role changes.Jun 7 2023, 5:12 AM

Harbormaster completed remote builds in B20083: Diff 27518.Jun 7 2023, 5:29 AM

rohan requested review of this revision.Jun 7 2023, 5:29 AM

rohan edited the test plan for this revision. (Show Details)Jun 7 2023, 5:42 AM

rohan attached a referenced file: F575275: Screen Recording 2023-06-07 at 8.41.15 AM.mov. (Show Details)

ashoat added inline comments.Jun 7 2023, 2:17 PM

web/modals/threads/members/change-member-role-modal.react.js
97–107 ↗	(On Diff #27518)	Is this code copy-pasted from somewhere? Can it be factored out instead?

Use the otherUsersButNoOtherAdmins selector

Fix errorMessage string literal

Rebase

Harbormaster completed remote builds in B20099: Diff 27542.Jun 8 2023, 6:22 AM

Harbormaster completed remote builds in B20100: Diff 27543.

Harbormaster completed remote builds in B20105: Diff 27548.Jun 8 2023, 6:32 AM

Rebase on D8136

Harbormaster completed remote builds in B20133: Diff 27583.Jun 9 2023, 11:35 AM

Seems a little weird to me from a product standpoint, but code looks correct.

web/modals/threads/members/change-member-role-modal.react.js
38 ↗	(On Diff #27583)	I would instead name this something like `shouldErrorMessageBeDisplayed` or `errorMessageVisible` or `isErrorMessageVisible` or anything more "declarative"?
66–98 ↗	(On Diff #27583)	It seems weird that we're creating `showErrorMessage` state to determine whether or not to show the error message within `errorMessage` when the user tries to save. It would make more sense to me to prevent the user from getting to a state where they can submit a request for an action that we already know can't be completed. I'd prefer to gray out the dropdown and button with a little info box explaining what the action isn't possible. CC @ashoat @ted for their thoughts.