Details

Reviewers

atul
ginsu

Commits

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

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

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Jul 21 2023, 9:27 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 21 2023, 9:27 AM

rohan edited the summary of this revision. (Show Details)Jul 21 2023, 9:28 AM

rohan edited the test plan for this revision. (Show Details)

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 completed remote builds in B21125: Diff 28947.Jul 21 2023, 9:53 AM

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 attached a referenced file: F648470: Screen Recording 2023-07-21 at 12.43.09 PM.mov. (Show Details)Jul 21 2023, 1:16 PM

Harbormaster completed remote builds in B21126: Diff 28948.Jul 21 2023, 3:56 PM

rohan requested review of this revision.Jul 21 2023, 3:56 PM

rohan added a child revision: D8623: [web] Add an option to edit and delete select roles.Jul 25 2023, 8:09 AM

atul accepted this revision.Jul 26 2023, 12:19 AM

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