-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
feat(#6669): barcode scanner #8684
Conversation
Hi @m5r, @lorerod, @Benmuiruri This is ready for an initial review. Can you please have a look and provide feedback? 🙂
Most questions and feedback are related to a place in the code or file, I prefer those as comments in the file instead of the "PR timeline" because in the "PR timeline" I can't reply just under the comment. So the file flows better because of the treads.😬 Thanks! |
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.
Thank you, @latin-panda, for the unit test coverage!
I left some questions and minor suggestions. None of them are blockers, though.
webapp/tests/karma/ts/components/search-bar/search-bar.component.spec.ts
Show resolved
Hide resolved
webapp/tests/karma/ts/components/search-bar/search-bar.component.spec.ts
Show resolved
Hide resolved
webapp/tests/karma/ts/components/search-bar/search-bar.component.spec.ts
Show resolved
Hide resolved
webapp/tests/karma/ts/components/search-bar/search-bar.component.spec.ts
Show resolved
Hide resolved
@latin-panda It looks like the barcode scanning API is not supported in Firefox which is a browser we support. We might need to consider polyfilling with a library? |
…into 6669-search-by-barcode
@garethbowen I think we're fine for now in this Feature Release.
|
PWA includes Firefox. It's fine if you want to roll with this for the Feature Release but it can't be merged to |
@m5r just wondering if you have some time for a quick review of this draft :) |
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.
That's pretty advanced for a draft PR 😁
Well done Jenni! I've left a couple change requests
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.
All good! 🙌
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.
Looks good and working as expected!
expect(telemetryService.record.calledWith('search_by_barcode:failure')).to.be.true; | ||
})); | ||
|
||
it('should record telemetry when barcode is clicked.', fakeAsync(async () => { |
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.
Nice! Thanks!
Hi @latin-panda I tested this today. I will continue tomorrow. If you want something specific for me to try, let me know. Tested Phones:
Desktop: Admin users using desktop
Admin users using mobile
Offline users using desktop
Offline users using phones
22 With admin change config
|
Thanks a lot, @lorerod. All these scenarios are great! You even covered some I didn't think about, like green/blue codes and quiet zones (I had to google the meaning 😅). I think you covered all I can think of so far. I tested using the following:
I struggled to scan a white barcode with a transparent background because of the lack of contrast. It's probably the same issue with green/blue barcodes. I think it's not a blocker for a Feature Release; the partner's barcode is black and white, type code_128; we can observe if someone uses something with low contrast (green/blue, white/transparent) The cases with quiet zone 0 or incomplete barcodes/QR codes are probably because the tool's algorithm doesn't know with certainty the start and end of the barcode 🤔 |
I agree that this is working well. I will work on adding an e2e test to cover the happy path scenario. Thanks Jenny |
Description
Nb. This will be delivered in 4 different PRs:
This work uses the native Barcode Detector API from the browser by using a hidden input type
file
to natively open the camera in Android devices. This avoids custom code in the CHT Android and allows more accessibility since the feature can be used in CHT Android, PWA, and Android browsers.See Figma design
It works the following way:
More considerations:
can_use_barcode_scanner
permissionTelemetry:
search_by_barcode:open
-> When a user clicks on the barcode scanner iconsearch_by_barcode:not_supported
-> When the feature isn't supported because the device is a desktop or the Barcode Detector API isn't available in the browser.search_by_barcode:scan
-> When the user has taken the picture.search_by_barcode:trigger_search
-> When Barcode Detector gets the code from the image and triggers the search.search_by_barcode:barcode_no_detected
-> When Barcode Detector couldn't detect the code from the image.search_by_barcode:failure
-> Any system failure when the Barcode Detector tries to get the code from the image.Snackbar messages:
Failed to read the barcode. Retry
Videos
#6669
Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.