Page MenuHomePhabricator

[native] Prevent duplicate role names in one community on the client
ClosedPublic

Authored by rohan on Jul 7 2023, 10:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 26, 5:43 AM
Unknown Object (File)
Fri, Nov 22, 11:37 AM
Unknown Object (File)
Sat, Nov 9, 5:23 PM
Unknown Object (File)
Sat, Nov 9, 2:34 PM
Unknown Object (File)
Sat, Nov 9, 1:20 PM
Unknown Object (File)
Sat, Nov 9, 12:59 PM
Unknown Object (File)
Sat, Nov 9, 10:46 AM
Unknown Object (File)
Sat, Nov 9, 10:45 AM

Details

Summary

This is the client-side error handling for when a user tries to create a role with the same name as an existing role in the community. keyserver support for this is added in D8432 by creating a unique key, but we want to ideally prevent the user from getting to a point where a duplicate role name is sent to the keyserver and an error is thrown.

Here, when the 'Create' button is clicked, we check the role names in the current threadInfo, and if the same role name exists, we use the ActionResultModal to indicate why this role name cannot be used. Using ActionResultModal was discussed in DES-44.

Part of ENG-4178.

Depends on D8432

Test Plan

Create a role with the same name as Admins (exists), then create a role with a new name that does not exist. Expect the error to display the first time, but the role to create the second time.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan edited the test plan for this revision. (Show Details)
rohan requested review of this revision.Jul 7 2023, 10:37 AM
ginsu requested changes to this revision.Jul 13 2023, 9:48 AM

Code looks good, but I feel that navigating the user away from the create role screen on an error is not a great experience. If I encountered an error because I chose a duplicate name, as a user I would expect that I would stay on the same page so that I could quickly fix the name and try again.

This revision now requires changes to proceed.Jul 13 2023, 9:48 AM
In D8445#250540, @ginsu wrote:

Code looks good, but I feel that navigating the user away from the create role screen on an error is not a great experience. If I encountered an error because I chose a duplicate name, as a user I would expect that I would stay on the same page so that I could quickly fix the name and try again.

I agree, navigating away from the screen seems to be the default behavior of using displayActionResultModal.

cc @ted @ashoat from what we discussed in Linear, wondering if an alternative makes more sense here, like a red variant of the 'notice box' component used in the change-roles screen may be better? It's not actually a component yet but might be simple enough to refactor into one

simulator_screenshot_F4AB4181-C263-4C97-A495-1D2424FE9EE8.png (2×1 px, 245 KB)

I agree, navigating away from the screen seems to be the default behavior of using displayActionResultModal.

Why is that? Looking at the code, I don't follow why that should be the case. I suspect something weird is going on here.

cc @ted @ashoat from what we discussed in Linear, wondering if an alternative makes more sense here, like a red variant of the 'notice box' component used in the change-roles screen may be better? It's not actually a component yet but might be simple enough to refactor into one

simulator_screenshot_F4AB4181-C263-4C97-A495-1D2424FE9EE8.png (2×1 px, 245 KB)

Is the reason we're considering this because we can't figure out a way to show the "toast" (ActionResultModal) without dismissing the Change Role screen? Or was it in the designs that we should be dismissing the Change Role screen?

I agree, navigating away from the screen seems to be the default behavior of using displayActionResultModal.

Why is that? Looking at the code, I don't follow why that should be the case. I suspect something weird is going on here.

I'm not certain, I'm looking into it to determine whether it's with the way I'm handling navigation perhaps, or if there is another reason. My calling it 'default' behavior was incorrect though

cc @ted @ashoat from what we discussed in Linear, wondering if an alternative makes more sense here, like a red variant of the 'notice box' component used in the change-roles screen may be better? It's not actually a component yet but might be simple enough to refactor into one

simulator_screenshot_F4AB4181-C263-4C97-A495-1D2424FE9EE8.png (2×1 px, 245 KB)

Is the reason we're considering this because we can't figure out a way to show the "toast" (ActionResultModal) without dismissing the Change Role screen? Or was it in the designs that we should be dismissing the Change Role screen?

There weren't any official designs for handling this behavior, we just talked about it on the Linear task linked in the diff description. I only suggested it because I realized for change roles we use a box alert to prevent the user from reaching a state of error, so I was curious if it would be good to have similar behavior across actions surrounding roles, like here when creating a role.

I'm open to it if it improves consistency, but I'd want to see side-by-side examples of what we're talking about here (changes roles screen, what we're currently doing here, and what you're proposing we do here). I'd also like to hear from Ted about it

Hey @rohan, thanks for the tag! I agree on keeping things consistent. I'll take a look at the design we had for the web version of this state so the native one can match. Will get back on this diff Friday afternoon, thanks!

For web, I believe we have the notice box within the change role modal that is always displayed. @rohan In the Linear comments DES-44, we talked about toast alerts. We're you thinking of using the notice box to behave as a toast? Just making sure.

Confirmed with @ted offline that a toast alert is the way to go here

Introduce a red-text error message for now (used opacity to prevent the screen from jumping around when the error is shown). Used a setter in the CreateRolesHeaderRight to 'lift' the state up to the main create roles screen.

Using displayActionResultModal is now tracked as a follow up in ENG-4519

ginsu requested changes to this revision.Jul 26 2023, 10:18 PM

I like the red text experience a lot better, thanks for iterating on that! Requesting changes on a few things I left comments for inline

native/roles/create-roles-header-right-button.react.js
22 ↗(On Diff #29065)

Let's use mixed instead of void

35 ↗(On Diff #29065)

I think this could be improved by memoizing this line outside the callback. We then don't need to run this line every time we run this callback

This revision now requires changes to proceed.Jul 26 2023, 10:18 PM

thanks for addressing my feedback!

This revision is now accepted and ready to land.Jul 28 2023, 1:49 PM