Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Woo POS] We don't handle the low battery event during IPP flow properly #12568

Draft
wants to merge 7 commits into
base: trunk
Choose a base branch
from
Draft

[Woo POS] We don't handle the low battery event during IPP flow properly #12568

wants to merge 7 commits into from

Conversation

backwardstruck
Copy link
Contributor

@backwardstruck backwardstruck commented Sep 9, 2024 •
edited
Loading

Closes: #12565

Description

Discussed in Slack.

This PR addresses the issue where a LOW_BATTERY event incorrectly shows a toast and an empty dialog instead of triggering the appropriate low battery warning dialog similar to the LOW_BATTERY_SUCCEED_CONNECT event.

The underlying problem was that LOW_BATTERY events were causing the bluetoothReaderListener to be reset to an Unknown state before the low battery event could be appropriately handled as Failed.

Steps to reproduce

  1. Go to POS.
  2. Connect to virtual card reader in low battery state OR physical card reader that's low on battery.
  3. Observe the behavior. Previously, it would show a toast and reset to an unknown state.
  4. With the fix, it should now display the low battery warning dialog correctly.

Testing information

  • Devices used: tablet emulator and physical tablet/card reader
  • Workflow tested: Initiated a reader connection and simulated low battery events during the update process.
  • Affected areas: Card reader connection workflow, software update process.

The tests that have been performed

  1. Verified that LOW_BATTERY triggers the correct dialog.
  2. Confirmed the state transitions are handled correctly without prematurely resetting to Unknown.
  3. Checked no regressions occur in LOW_BATTERY_SUCCEED_CONNECT.

  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

  • I have Removed any TEST CODE.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

backwardstruck
backwardstruck commented Sep 9, 2024
View reviewed changes
...reader/src/main/java/com/woocommerce/android/cardreader/internal/wrappers/TerminalWrapper.kt
@@ -108,7 +108,7 @@ internal class TerminalWrapper {
private fun mapFrequencyOptions(updateFrequency: CardReaderManager.SimulatorUpdateFrequency): SimulateReaderUpdate {
return when (updateFrequency) {
CardReaderManager.SimulatorUpdateFrequency.NEVER -> SimulateReaderUpdate.NONE
CardReaderManager.SimulatorUpdateFrequency.ALWAYS -> SimulateReaderUpdate.REQUIRED
CardReaderManager.SimulatorUpdateFrequency.ALWAYS -> SimulateReaderUpdate.LOW_BATTERY
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is test code, to be removed before merging.

@backwardstruck backwardstruck added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Sep 9, 2024
@backwardstruck backwardstruck changed the title 12565 woo pos we dont handle the low battery event during ipp flow properly [Woo POS] We don't handle the low battery event during IPP flow properly Sep 9, 2024
@dangermattic
Copy link
Collaborator

1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 9, 2024 •
edited
Loading

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commitafcb619
Direct Downloadwoocommerce-wear-prototype-build-pr12568-afcb619.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 9, 2024 •
edited
Loading

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commitafcb619
Direct Downloadwoocommerce-prototype-build-pr12568-afcb619.apk

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 40.58%. Comparing base (56404b5) to head (afcb619).

Files with missing lines Patch % Lines
...ardreader/internal/connection/ConnectionManager.kt 0.00% 2 Missing and 1 partial ⚠️
...id/cardreader/internal/wrappers/TerminalWrapper.kt 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##              trunk   #12568      +/-   ##
============================================
- Coverage     40.58%   40.58%   -0.01%     
  Complexity     5674     5674              
============================================
  Files          1229     1229              
  Lines         69287    69289       +2     
  Branches       9579     9581       +2     
============================================
- Hits          28119    28118       -1     
- Misses        38584    38586       +2     
- Partials       2584     2585       +1
Flag Coverage Δ
40.58% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status: do not merge Dependent on another PR, ready for review but not ready for merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Woo POS] We don’t handle the low battery event during IPP flow properly
4 participants
@backwardstruck @dangermattic @wpmobilebot @codecov-commenter