Details

Reviewers

atul
ginsu
tomek
varun

Commits

rCOMM001b397857bd: [native] Set up the BarCodeScanner to scan QR codes

Summary

This diff introduces the usage of the Expo BarCodeScanner when the header right 'Add' is clicked on the linked devices screen.

The Expo docs were closely followed in setting this up.

For now, the onConnect will not do anything besides alert the user that the qr code has been scanned with the data encoded, but down the line this will be changed to read the secondary devices' keys encoded in the qr code.

Addresses both ENG-4475 and ENG-4477.

Depends on D8771

Test Plan

Confirmed that the scanner opened if I granted camera permissions, and also that it blocked launching the scanner if I denied permissions.

Accepting permissions:

Denying permissions:

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Aug 14 2023, 8:30 AM

Herald added a subscriber: ashoat. · View Herald TranscriptAug 14 2023, 8:30 AM

rohan edited the summary of this revision. (Show Details)Aug 14 2023, 8:33 AM

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

rohan added a parent revision: D8771: [native] Create Flow ExpoBarcodeScanner libdef.

rohan added inline comments.Aug 14 2023, 8:44 AM

native/profile/secondary-device-qr-code-scanner.react.js
78–80 ↗	(On Diff #29862)	Following the usage section in the expo docs, we should support three cases: `hasPermission === false` (user has denied permissions) - at this point, we navigate back to the Linked Devices screen `hasPermission === null` (user has not accepted or denied permissions) - this will happen the first time the user opens the expo barcode scanner screen. This will be an 'intermediate' step, so while the user is prompted, it makes sense to just render an empty view in the background until they either accept or deny. `hasPermission === true` (user has accepted permissions) - we show the barcode scanner

Harbormaster completed remote builds in B21729: Diff 29862.Aug 14 2023, 8:49 AM

rohan requested review of this revision.Aug 14 2023, 8:49 AM

rohan added a child revision: D8806: [native] Prevent simulators from attempting to open the barcode scanner.Aug 14 2023, 8:51 AM

rohan edited the summary of this revision. (Show Details)Aug 14 2023, 11:51 AM

tomek requested changes to this revision.Aug 16 2023, 2:53 AM

tomek added inline comments.

native/profile/secondary-device-qr-code-scanner.react.js
27 ↗	(On Diff #29862)	Are we going to ask for a permission every time we open this screen? If the permission is granted, we should automatically be able to know that and avoid asking again. If the permission is denied, is there a limit on how many times we can ask again for it?
30–41 ↗	(On Diff #29862)	Why do we need a separate effect for it? Can't we handle this in the previous effect?
49 ↗	(On Diff #29862)	Why do we set this flag?
86–88 ↗	(On Diff #29862)	What if a user pointed a camera at a QR code and then moved it to another QR code? Would we scan the second one, or stop scanning after the first one is detected?

This revision now requires changes to proceed.Aug 16 2023, 2:53 AM

rohan added inline comments.Aug 16 2023, 8:00 AM

native/profile/secondary-device-qr-code-scanner.react.js
27 ↗	(On Diff #29862)	No the permission will only be prompted once. According to the Expo Permissions docs: An operating-system level restriction on both Android and iOS prohibits an app from asking for the same permission more than once So we can only ask for it once (on the first time launch)
30–41 ↗	(On Diff #29862)	I think it had something to do with not having access to the most up to date state of `hasPermission` unless it was in another `useEffect` hook, but let me confirm while putting up a revision to this diff
49 ↗	(On Diff #29862)	No strong reason, just was following some examples of `Alert` in the codebase for whether we're allowing alerts to be dismissed by tapping outside of it on Android. I'm ok to remove it
86–88 ↗	(On Diff #29862)	By following this practice, we're only allowing one QR code to be scanned (in theory, once the infra side of this is done, the user will only need to scan one code anyways).

rohan added inline comments.Aug 16 2023, 11:02 AM

native/profile/secondary-device-qr-code-scanner.react.js
30–41 ↗	(On Diff #29862)	Yeah after some testing, I think it's needed to have the most up to date version of the `hasPermission` state (example on Stack Overflow) By merging the two hooks, we'll have a call to `setHasPermission(status === 'granted')`, but the check for `hasPermission` will not read the update state. Separating it into it's own `useEffect` that triggers when the state changes will make sure we evaluate the permissions correctly. Feel free to let me know if you disagree though!

Remove { cancelable: false } in onConnect

Remove extra cancelable property

native/profile/secondary-device-qr-code-scanner.react.js
70	We'll want to keep this one though so the user is required to click on one of the two options after scanning a QR code (cancel or connect)

Harbormaster completed remote builds in B21804: Diff 29968.Aug 16 2023, 11:22 AM

Harbormaster completed remote builds in B21805: Diff 29969.Aug 16 2023, 11:25 AM

tomek accepted this revision.Aug 17 2023, 4:20 AM

tomek added inline comments.

native/profile/secondary-device-qr-code-scanner.react.js
30–41 ↗	(On Diff #29862)	If we would like to use the just-set state, then yes, a new effect is necessary. But I was thinking about avoiding reading the state in an effect, something like this: React.useEffect(() => {(async () => { const { status } = await BarCodeScanner.requestPermissionsAsync(); setHasPermission(status === 'granted'); if (status !== 'granted') { Alert.alert( 'No access to camera', 'Please allow Comm to access your camera in order to scan the QR code.', [{ text: 'OK' }], ); navigation.goBack(); } })(); }, []); By doing this we could avoid one unnecessary render.
86–88 ↗	(On Diff #29862)	Is it a good practice from a UX point of view? For example, the camera app on iOS lets users choose which QR to open. We don't necessarily need something like that but wanted to make sure that we've considered alternatives.

This revision is now accepted and ready to land.Aug 17 2023, 4:20 AM

rohan added inline comments.Aug 17 2023, 12:20 PM

native/profile/secondary-device-qr-code-scanner.react.js
30–41 ↗	(On Diff #29862)	Ohh yeah that's a good suggestion! Sorry for the confusion, I didn't consider not reading from state
86–88 ↗	(On Diff #29862)	Ah yeah so if down the line we need multiple QR codes to be scanned one after the other, then this should be a very easy change. For now, while the log-in action doesn't actually occur though, I'll keep it as-is because it gets pretty annoying to constantly get re-prompted for the scan since nothing is actually happening. Thanks for pointing it out!

Implement feedback

Harbormaster completed remote builds in B21865: Diff 30039.Aug 17 2023, 12:41 PM

Closed by commit rCOMM001b397857bd: [native] Set up the BarCodeScanner to scan QR codes (authored by rohan). · Explain WhyAug 17 2023, 12:50 PM

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMM001b397857bd: [native] Set up the BarCodeScanner to scan QR codes.

[native] Set up the BarCodeScanner to scan QR codes
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 29969

native/navigation/route-names.js

native/profile/linked-devices-header-right-button.react.js

native/profile/profile.react.js

native/profile/secondary-device-qr-code-scanner.react.js

[native] Set up the BarCodeScanner to scan QR codesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 29969

native/navigation/route-names.js

native/profile/linked-devices-header-right-button.react.js

native/profile/profile.react.js

native/profile/secondary-device-qr-code-scanner.react.js

[native] Set up the BarCodeScanner to scan QR codes
ClosedPublic
Actions

Revision Contents
Changeset List