Page MenuHomePhabricator

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

Authored by rohan on Jun 7 2023, 5:11 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 6:53 PM
Unknown Object (File)
Fri, Nov 22, 6:42 PM
Unknown Object (File)
Fri, Nov 22, 2:34 PM
Unknown Object (File)
Sat, Nov 9, 3:18 PM
Unknown Object (File)
Sat, Nov 9, 1:23 PM
Unknown Object (File)
Sat, Nov 9, 11:34 AM
Unknown Object (File)
Sat, Nov 9, 11:34 AM
Unknown Object (File)
Sat, Nov 9, 11:34 AM

Details

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 requested review of this revision.Jun 7 2023, 5:29 AM
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

atul added a subscriber: ted.

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.

This revision is now accepted and ready to land.Jun 12 2023, 12:51 PM

(We're also ensuring that this isn't possible from the keyserver side, right?)

web/modals/threads/members/change-member-role-modal.react.js
66–98 ↗(On Diff #27583)

Agree with @atul

In D8141#241851, @atul wrote:

(We're also ensuring that this isn't possible from the keyserver side, right?)

I'll actually add that in a new diff, probably just before this diff in the stack

Agree with @atul as well.

Here is a design with the dropdown text and Save button grayed out.

An 'alert' text container is also present to let the users know why they cannot change their role during these instances. CC @rohan @ashoat

Screenshot 2023-06-13 at 2.05.18 PM.png (982×1 px, 138 KB)

Edit: Updated SS using better container color

Address design feedback (video below)

This revision is now accepted and ready to land.Jun 13 2023, 1:14 PM
This revision was landed with ongoing or failed builds.Jun 21 2023, 8:42 AM
This revision was automatically updated to reflect the committed changes.
web/modals/threads/members/change-member-role-modal.react.js
76 ↗(On Diff #27962)

This <> doesn't appear to be doing anything