-
Notifications
You must be signed in to change notification settings - Fork 136
Barcode scanning - Handle failure case #9082
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
Barcode scanning - Handle failure case #9082
Conversation
…with retry action.
…73-handle-error-case-barcode-scanning
…rrectly on scanned.
…arch by SKU returns empty result.
…le message when the product search by SKU fails.
…age when the product search by SKU fails.
… by SKU fails and retry clicked.
|
You can test the changes on this Pull Request by downloading an installable build, or scanning this QR code: |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## trunk #9082 +/- ##
============================================
+ Coverage 43.48% 43.59% +0.11%
- Complexity 4032 4094 +62
============================================
Files 833 834 +1
Lines 43900 43985 +85
Branches 5734 5763 +29
============================================
+ Hits 19088 19176 +88
Misses 23137 23137
+ Partials 1675 1672 -3
☔ View full report in Codecov by Sentry. |
| } | ||
| } | ||
|
|
||
| data class OnBarcodeScanningFailed( |
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.
Both these event models might look similar in code but their behavior might change in the near future. Also, the code can be confusing to understand when we merge both into one. Let me know your thoughts.
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'd go with 1 event at the beggining, e.g. OnAddingProductViaScaningFailed (we don't only support barcodes but other types of encoding of the data) and will pass 2 different messages there: Scanning failed with error: %s and Unable to find product with SKU: %s. That should give more information to a user
… to the MLKit barcode scanning.
…readyExists type returned
…fRange type returned
… PermissionDenied type returned
…n ResourceExhausted type returned
…nAuthenticated type returned
…ilable type returned
…mplemented type returned
…en Other type returned
kidinov
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 left a few comments, please take a look
| private fun sendBarcodeScanningFailedEvent(error: Throwable?) { | ||
| triggerEvent( | ||
| OnBarcodeScanningFailed( | ||
| error, |
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.
Error property is never used in both events in the production code? In OnBarcodeScanningFailed and OnProductSearchBySKUFailed
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.
Strange. I had already removed the error property from both events. In the latest file change, it's not shown.
| } | ||
| } | ||
|
|
||
| data class OnBarcodeScanningFailed( |
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'd go with 1 event at the beggining, e.g. OnAddingProductViaScaningFailed (we don't only support barcodes but other types of encoding of the data) and will pass 2 different messages there: Scanning failed with error: %s and Unable to find product with SKU: %s. That should give more information to a user
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/creation/CodeScanner.kt
Outdated
Show resolved
Hide resolved
...e/src/main/kotlin/com/woocommerce/android/ui/orders/creation/GoogleCodeScannerErrorMapper.kt
Outdated
Show resolved
Hide resolved
...merce/src/main/kotlin/com/woocommerce/android/ui/orders/creation/OrderCreateEditViewModel.kt
Outdated
Show resolved
Hide resolved
Generated by 🚫 dangerJS |
…and merge them into one event OnAddingProductViaScanningFailed
| private fun sendAddingProductsViaScanningFailedEvent() { | ||
| triggerEvent( | ||
| OnAddingProductViaScanningFailed( | ||
| string.order_creation_barcode_scanning_unable_to_add_product |
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.
Now you show the same text Product not found. Unable to add to new order for both cases - when scanning failed (including when the scanning is not available at all) and when we could not find a product by SKU
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.
Added a separate message for both failure events here: aaecceb
Right now, the messages are static without much information on what went wrong. This will be taken on priority and will be discussed with Ann and should be out by next week after the initial release.
kidinov
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 left a comment regarding the text currently used for error indication - it may be missliding
kidinov
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 left a comment regarding the text currently used for error indication - it may be missliding
kidinov
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.
LGTM!
Closes: #9073
Description
This PR handles failure when
We show a
Snackbarwith a "retry" action in both cases. Clicking on the "retry" action will trigger the scanning process.More context: pecCkj-GT-p2
Testing instructions
Order listing screen
Order detail screen
Images/gif
screen-20230522-154828.mp4
RELEASE-NOTES.txtif necessary.