Page MenuHomePhabricator

[Cashtab] Update ScanQRCode.js
ClosedPublic

Authored by bytesofman on Jan 20 2024, 23:35.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC906e51b5a12c: [Cashtab] Update ScanQRCode.js
Summary

T3387

Upgrade to latest available zxing. Update component logic to work with latest zxing.

Test Plan

npm test

Navigate to https://cashtab-local-dev.netlify.app/ which deploys this diff
Scan a QR code of an ecash address
Scan a QR code of a bip21 string, try entering ecash:qp89xgjhcqdnzzemts0aj378nfe2mhu9yvxj9nhgg6?amount=17&op_return_raw=04007461622263617368746162206D6573736167652077697468206F705F72657475726E5F726177 at https://www.qr-code-generator.com/
Scan a QR code of a URL and verify you get a validation error

Unfortunately I'm not able to mock an error thrown by the camera. Manually test that the camera modal goes away and the error msg is rendered by

  • change line 47 of ScanQRCode.js from if (error instanceof NotFoundException) { to if (!(error instanceof NotFoundException)) {
  • npm start

screenshot rel:

image.png (197×391 px, 11 KB)

Diff Detail

Repository
rABC Bitcoin ABC
Branch
qr-code-improvements
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 26519
Build 52608: Build Diffcashtab-tests
Build 52607: arc lint + arc unit

Event Timeline

TODO

  • test on iOS safari
  • test on non-safari iOS
  • add integration tests
  • Do you need the error and setError? at the moment it isn't doing anything, you need to know which errors need to be caught
bytesofman edited the test plan for this revision. (Show Details)
bytesofman added inline comments.
cashtab/src/components/Common/ScanQRCode.js
136 ↗(On Diff #44453)

Apple opened up this API. I've asked John to test on iOS safari and non-safari to confirm camera comes up.

Fabien requested changes to this revision.Jan 23 2024, 08:34
Fabien added a subscriber: Fabien.
Fabien added inline comments.
cashtab/src/components/Common/ScanQRCode.js
59 ↗(On Diff #44453)

How does this work ? This branch will be executed if there is no error, but you will log an error, display an error and stop scanning.
My guess is that the error is immediately hidden so you didn't notice.

This revision now requires changes to proceed.Jan 23 2024, 08:34
bytesofman added inline comments.
cashtab/src/components/Common/ScanQRCode.js
59 ↗(On Diff #44453)

good catch

bytesofman marked an inline comment as done.

timing bug, error raised often on frequent scans

Also ignore FormatException error

This revision is now accepted and ready to land.Jan 24 2024, 08:59
Fabien requested changes to this revision.Jan 24 2024, 09:00
Fabien added inline comments.
cashtab/src/components/Common/ScanQRCode.js
69 ↗(On Diff #44503)

You should return here, if there is an error you don't want to process the result

This revision now requires changes to proceed.Jan 24 2024, 09:00
bytesofman added inline comments.
cashtab/src/components/Common/ScanQRCode.js
69 ↗(On Diff #44503)

handled

also in testing I found another error to ignore. It's the 3 of 3 of subclasses of the ReaderException, so that should be all of them (the types of errors that come from the reader getting a bad scan of the barcode and thinking it is not a QR code, or has a bad checksum, or there is no qr code in the image ... in these cases we want to just keep scanning until we find a good qr code)

bytesofman marked an inline comment as done.

ignore all ReaderExceptions, do not return result if error, rebase

Fabien requested changes to this revision.Jan 24 2024, 20:28
Fabien added inline comments.
cashtab/src/components/Common/ScanQRCode.js
78 ↗(On Diff #44542)

You have the return above, so you don't need the else

This revision now requires changes to proceed.Jan 24 2024, 20:28
This revision is now accepted and ready to land.Jan 25 2024, 08:29
This revision was automatically updated to reflect the committed changes.