Page MenuHomePhabricator

[web] Introduce client-side error handling to prevent duplicate role names
ClosedPublic

Authored by rohan on Jul 21 2023, 9:27 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 4:25 AM
Unknown Object (File)
Sat, Nov 23, 4:25 AM
Unknown Object (File)
Sat, Nov 23, 4:25 AM
Unknown Object (File)
Sat, Nov 23, 4:25 AM
Unknown Object (File)
Sat, Nov 16, 2:24 AM
Unknown Object (File)
Sat, Nov 16, 2:22 AM
Unknown Object (File)
Sun, Nov 10, 2:22 AM
Unknown Object (File)
Sat, Nov 9, 11:40 PM

Details

Summary

This diff adds client-side error handling for preventing a user from trying to create a role with a name that already exists in the community. This is already handled on the keyserver and on the database with a unique key, but ideally we prevent the client from reaching a bad state in the first place. I confirmed with ted that the designs for this are fine

Depends on D8596

Test Plan

Tried to create a role with an identical name to an existing role and saw the message, then tried to create a role with a unique name to confirm that still works.

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 added a subscriber: ted.

Spoke with @ted a bit more, and we agreed that the modal should not 'jump' when the error text shows. Resolved this by setting the visibility: none so the error message takes up space, but is just not shown. If we need to show the error, we can make the visibility visible so it displays.

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 21 2023, 10:26 AM
Harbormaster failed remote builds in B21126: Diff 28948!
rohan requested review of this revision.Jul 21 2023, 3:56 PM
atul added inline comments.
web/roles/create-roles-modal.react.js
51 ↗(On Diff #28948)

Would prefer the state to be named something like roleCreationFailed/setRoleCreationFailed

Whether or not the "error is shown" should be a function of the state, not the state itself imo (more extreme example may be storing JSX in state vs toggling JSX based on state)

Doesn't really matter, so whatever you prefer

This revision is now accepted and ready to land.Jul 26 2023, 12:19 AM

Rebase on D8595 and add support for an unknown error

Reintroduce try/catch here with a more useful action on error

Memoize threadRoleNames following feedback on the native counterpart

web/roles/create-roles-modal.react.js
213–214 ↗(On Diff #29099)

Why is this condition necessary?

Update conditional

web/roles/create-roles-modal.react.js
213–214 ↗(On Diff #29099)

I don't actually need it if it's repeating the same error, and I know RoleCreationErrorVariant will be of two types

This revision was landed with ongoing or failed builds.Jul 31 2023, 6:15 PM
This revision was automatically updated to reflect the committed changes.