Page MenuHomePhabricator

[keyserver][web][native] Handle link creation / update errors
ClosedPublic

Authored by tomek on Jul 13 2023, 5:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 22, 2:30 PM
Unknown Object (File)
Tue, Oct 22, 2:30 PM
Unknown Object (File)
Tue, Oct 22, 2:30 PM
Unknown Object (File)
Tue, Oct 22, 2:30 PM
Unknown Object (File)
Tue, Oct 22, 2:28 PM
Unknown Object (File)
Tue, Oct 22, 5:03 AM
Unknown Object (File)
Sep 23 2024, 3:58 AM
Unknown Object (File)
Sep 20 2024, 1:55 AM
Subscribers

Details

Summary

In this diff we're handling two errors:

  1. When invalid characters were used in the link

invalid.png (1×1 px, 106 KB)

  1. When an already used link was used

in-use.png (992×1 px, 104 KB)

Copies are based on https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=11341%3A284796&mode=dev, but with some changes:

  1. Replaced the first chars to the lowercase so that they are consistent e.g. with our login form.
  2. Removed a set of invalid chars from invalid_characters error message because it is impossible to present a complete set of invalid chars (we only support [a-zA-Z0-9]).

https://linear.app/comm/issue/ENG-3916/better-error-handling

Test Plan

Check if a proper error message is displayed on both web and native when a user tries to:

  1. create a link with ? as a secret
  2. create a link with the same secret as some other link
  3. update a link to have the same secret as some other link

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.Jul 13 2023, 6:15 AM

Let's actually capitalize the first letter. We should probably do the same in log-in form, but to be fair it's a little different visually... the errors there are shorter, and are not given their own full-width container (if I recall correctly)

ashoat added inline comments.
keyserver/src/database/database.js
16 ↗(On Diff #28671)

Current pattern for these is that they're defined next to callsite. Open to defining them all in database.js, but figure we should be consistent. Maybe you can submit a follow-up diff to update all locations to match your new convention?

This revision is now accepted and ready to land.Jul 17 2023, 5:14 AM

Capitalize the first letters of error messages

Let's actually capitalize the first letter. We should probably do the same in log-in form, but to be fair it's a little different visually... the errors there are shorter, and are not given their own full-width container (if I recall correctly)

Yeah, the errors in login form are a lot shorter, so keeping them as they are also makes sense

keyserver/src/database/database.js
16 ↗(On Diff #28671)

Created a diff with that D8608

lib/shared/invite-links.js
9

Should the first letter here be capitalized as well?

lib/shared/invite-links.js
9

We probably should do that to improve consistency (but don't have to as it is a lot shorter)

Use uppercase and periods