Page MenuHomePhabricator
Differential D8597

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

Authored by rohan on Jul 21 2023, 9:27 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 17, 9:32 AM
Unknown Object (File)
Wed, Apr 16, 3:13 PM
Unknown Object (File)
Wed, Apr 16, 2:36 PM
Unknown Object (File)
Wed, Apr 16, 10:21 AM
Unknown Object (File)
Wed, Apr 16, 8:11 AM
Unknown Object (File)
Wed, Apr 16, 7:32 AM
Unknown Object (File)
Wed, Apr 16, 5:01 AM
Unknown Object (File)
Wed, Apr 16, 2:35 AM
View All Files
Subscribers
ashoat
• ted
tomek

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)
rohan updated this revision to Diff 28948.EditedJul 21 2023, 9:43 AM
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 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

This revision is now accepted and ready to land.Jul 26 2023, 12:19 AM
rohan updated this revision to Diff 29063.Jul 26 2023, 8:39 AM

Address feedback

Harbormaster completed remote builds in B21219: Diff 29063.Jul 26 2023, 8:56 AM
rohan updated this revision to Diff 29072.Jul 26 2023, 12:16 PM

Rebase on D8595 and add support for an unknown error

Harbormaster completed remote builds in B21228: Diff 29072.Jul 26 2023, 12:34 PM
rohan updated this revision to Diff 29087.Jul 27 2023, 12:47 AM

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

rohan updated this revision to Diff 29088.Jul 27 2023, 12:51 AM

Memoize threadRoleNames following feedback on the native counterpart

Harbormaster completed remote builds in B21243: Diff 29087.Jul 27 2023, 1:05 AM
Harbormaster completed remote builds in B21244: Diff 29088.Jul 27 2023, 1:12 AM
rohan updated this revision to Diff 29099.Jul 27 2023, 2:21 AM

Rebase

Harbormaster completed remote builds in B21255: Diff 29099.Jul 27 2023, 3:00 AM
ashoat added inline comments.Jul 28 2023, 7:35 AM
web/roles/create-roles-modal.react.js
213–214 ↗(On Diff #29099)

Why is this condition necessary?

rohan updated this revision to Diff 29235.Jul 29 2023, 11:08 AM

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

Harbormaster completed remote builds in B21344: Diff 29235.Jul 29 2023, 11:42 AM
rohan updated this revision to Diff 29341.Jul 31 2023, 12:06 PM

Rebase

This revision was landed with ongoing or failed builds.Jul 31 2023, 6:15 PM
Closed by commit rCOMMa2398d47ac08: [web] Introduce client-side error handling to prevent duplicate role names (authored by rohan). · Explain Why
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMMa2398d47ac08: [web] Introduce client-side error handling to prevent duplicate role names.
Harbormaster completed remote builds in B21429: Diff 29341.Jul 31 2023, 9:30 PM

Revision Contents
Changeset List

PathSize
web/
roles/
11 lines
47 lines

Diff 29341

View Options

web/roles/create-roles-modal.css

// @flow // @flow
import olm from '@commapp/olm'; import olm from '@commapp/olm';
import type { import type {
Account as OlmAccount, Account as OlmAccount,
Utility as OlmUtility, Utility as OlmUtility,
Session as OlmSession,
} from '@commapp/olm'; } from '@commapp/olm';
import uuid from 'uuid'; import uuid from 'uuid';
type PickledOlmAccount = { type PickledOlmAccount = {
+picklingKey: string, +picklingKey: string,
+pickledAccount: string, +pickledAccount: string,
}; };
Show All 22 Lines): Promise<OlmAccount> {
account.unpickle( account.unpickle(
pickledOlmAccount.picklingKey, pickledOlmAccount.picklingKey,
pickledOlmAccount.pickledAccount, pickledOlmAccount.pickledAccount,
); );
return account; return account;
} }
async function createPickledOlmSession(
account: OlmAccount,
accountPicklingKey: string,
initialEncryptedMessage: string,
): Promise<string> {
await olm.init();
const session = new olm.Session();
session.create_inbound(account, initialEncryptedMessage);
account.remove_one_time_keys(session);
session.decrypt(0, initialEncryptedMessage);
ashoatUnsubmitted
Not Done
Inline Actions

Let's avoid passing constants as number literals like this. Instead, please define something like eg. threadTypes, so that this code is more "readable" at the callsite (eg. we would see something like olmMessageTypes.TEXT or something)

ashoat: Let's avoid passing constants as number literals like this. Instead, please define something…
return session.pickle(accountPicklingKey);
}
async function unpickleOlmSession(
pickledSession: string,
picklingKey: string,
): Promise<OlmSession> {
await olm.init();
const session = new olm.Session();
session.unpickle(picklingKey, pickledSession);
return session;
}
let cachedOLMUtility: OlmUtility; let cachedOLMUtility: OlmUtility;
function getOlmUtility(): OlmUtility { function getOlmUtility(): OlmUtility {
if (cachedOLMUtility) { if (cachedOLMUtility) {
return cachedOLMUtility; return cachedOLMUtility;
} }
cachedOLMUtility = new olm.Utility(); cachedOLMUtility = new olm.Utility();
return cachedOLMUtility; return cachedOLMUtility;
} }
Show All 26 Lines if (
currentDate - lastPrekeyPublishDate >= maxOldPrekeyAge currentDate - lastPrekeyPublishDate >= maxOldPrekeyAge
) { ) {
account.forget_old_prekey(); account.forget_old_prekey();
} }
} }
export { export {
createPickledOlmAccount, createPickledOlmAccount,
createPickledOlmSession,
getOlmUtility, getOlmUtility,
unpickleOlmAccount, unpickleOlmAccount,
unpickleOlmSession,
validateAccountPrekey, validateAccountPrekey,
}; };
View Options

web/roles/create-roles-modal.react.js

// @flow // @flow
import olm from '@commapp/olm'; import olm from '@commapp/olm';
import type { import type {
Account as OlmAccount, Account as OlmAccount,
Utility as OlmUtility, Utility as OlmUtility,
Session as OlmSession,
} from '@commapp/olm'; } from '@commapp/olm';
import uuid from 'uuid'; import uuid from 'uuid';
type PickledOlmAccount = { type PickledOlmAccount = {
+picklingKey: string, +picklingKey: string,
+pickledAccount: string, +pickledAccount: string,
}; };
Show All 22 Lines): Promise<OlmAccount> {
account.unpickle( account.unpickle(
pickledOlmAccount.picklingKey, pickledOlmAccount.picklingKey,
pickledOlmAccount.pickledAccount, pickledOlmAccount.pickledAccount,
); );
return account; return account;
} }
async function createPickledOlmSession(
account: OlmAccount,
accountPicklingKey: string,
initialEncryptedMessage: string,
): Promise<string> {
await olm.init();
const session = new olm.Session();
session.create_inbound(account, initialEncryptedMessage);
account.remove_one_time_keys(session);
session.decrypt(0, initialEncryptedMessage);
ashoatUnsubmitted
Not Done
Inline Actions

Let's avoid passing constants as number literals like this. Instead, please define something like eg. threadTypes, so that this code is more "readable" at the callsite (eg. we would see something like olmMessageTypes.TEXT or something)

ashoat: Let's avoid passing constants as number literals like this. Instead, please define something…
return session.pickle(accountPicklingKey);
}
async function unpickleOlmSession(
pickledSession: string,
picklingKey: string,
): Promise<OlmSession> {
await olm.init();
const session = new olm.Session();
session.unpickle(picklingKey, pickledSession);
return session;
}
let cachedOLMUtility: OlmUtility; let cachedOLMUtility: OlmUtility;
function getOlmUtility(): OlmUtility { function getOlmUtility(): OlmUtility {
if (cachedOLMUtility) { if (cachedOLMUtility) {
return cachedOLMUtility; return cachedOLMUtility;
} }
cachedOLMUtility = new olm.Utility(); cachedOLMUtility = new olm.Utility();
return cachedOLMUtility; return cachedOLMUtility;
} }
Show All 26 Lines if (
currentDate - lastPrekeyPublishDate >= maxOldPrekeyAge currentDate - lastPrekeyPublishDate >= maxOldPrekeyAge
) { ) {
account.forget_old_prekey(); account.forget_old_prekey();
} }
} }
export { export {
createPickledOlmAccount, createPickledOlmAccount,
createPickledOlmSession,
getOlmUtility, getOlmUtility,
unpickleOlmAccount, unpickleOlmAccount,
unpickleOlmSession,
validateAccountPrekey, validateAccountPrekey,
}; };