-
-
Notifications
You must be signed in to change notification settings - Fork 208
fix: handle wrong qr code scans #5565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
packages/shared/constants.ts
Outdated
| } | ||
|
|
||
| export const enum SCAN_CONTEXT_TYPE { | ||
| DEFAULT = 'DEFAULT', // default context, no restrictions on qr code types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBD: we have a lot of use cases where we just pass the "DEFAULT" and only 2 where we pass other contexts. So we could make the parameter optional and have less code. At the same time it increases the risk to "forget" the scan context parameter in new use cases and introduce new wrong handlings in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes more sense to protect against risk of forgetting cases by making things more explicit in this case IMO
|
|
||
| const onClick = () => { | ||
| processQr(accountId, inviteLinkTrimmed) | ||
| processQr(accountId, inviteLinkTrimmed, SCAN_CONTEXT_TYPE.DEFAULT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBD: the qr button here comes with the text "New contact" but in fact it opens the general qr code scanner. We could restrict this context to only allow qr codes that in fact "introduce a new contact".
In Android the scanner opened here has no limitations like that...
The same applies to the dialogs in InvalidUncryptedMail & ProtectionStatusDialog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although the multi-device setup dialog text should be updated, the account switcher is not behind a "logout" action anymore. ideally it should also just say "you are about to sync your existing profile to this device, this will not overwrite your current profile, continue?" or sth. like that. and the ok button would just create a new unconfigured profile and execute the code again, like it is done for scanning dcaccount: links
c31797c to
d523f73
Compare
dff6f14 to
86e0147
Compare
Introduces new Type SCAN_CONTEXT_TYPE
86e0147 to
db62e73
Compare
WofWca
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have confirmed that the "add as second device" and "scan invitation code" workflows are not broken, at least for askVerifyContact and login.
And this works otherwise.


related to #5563
Introduce new type SCAN_CONTEXT_TYPE to handle qr codes scans more appropriate.
For now it just has impact for the "Add second device" & the "Use other server" context.