-
Notifications
You must be signed in to change notification settings - Fork 138
[HACK][Woo POS] PR 4/5: Integrate card reader connection into WooPos screens #15072
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
[HACK][Woo POS] PR 4/5: Integrate card reader connection into WooPos screens #15072
Conversation
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## woomob-1854-woopos-card-reader-connection-ui #15072 +/- ##
==================================================================================
- Coverage 38.68% 38.21% -0.47%
- Complexity 10482 10550 +68
==================================================================================
Files 2182 2197 +15
Lines 124004 126311 +2307
Branches 17119 17518 +399
==================================================================================
+ Hits 47967 48270 +303
- Misses 71203 73162 +1959
- Partials 4834 4879 +45 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull request overview
This PR integrates card reader connection and update dialogs into WooPos screens, moving from direct facade calls to an event-driven architecture using dialog states and parent-child event communication.
Key changes:
- Connection and update operations now trigger dialog events instead of direct facade calls
- Settings and Home screens show card reader connection/update dialogs via state management
- Toolbar and totals ViewModels use connection controller for disconnect operations
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| WooPosSettingsHardwareCardReaderViewModelTest.kt | Updated tests to verify connection/update events are sent instead of direct facade calls; added controller factory mock |
| WooPosTotalsViewModelTest.kt | Updated test to verify connection dialog event is sent when reader connection is needed |
| WooPosHomeFloatingToolbarViewModelTest.kt | Updated tests to verify connection dialog events and controller disconnect usage; added controller factory mock |
| WooPosSettingsHardwareCardReaderViewModel.kt | Changed connect/update to send events; disconnect now uses controller; added controller factory dependency |
| WooPosSettingsViewModel.kt | Added handlers for card reader connection/update dialog events from child components |
| WooPosSettingsState.kt | Added CardReaderConnectionDialog and CardReaderUpdateDialog dialog states |
| WooPosSettingsScreen.kt | Added UI integration to show card reader connection and update dialogs based on state |
| WooPosTotalsViewModel.kt | Changed connect reader click to send dialog event instead of calling facade |
| WooPosHomeFloatingToolbarViewModel.kt | Changed connection to send dialog event; disconnect now uses controller; added controller factory dependency |
| WooPosHomeViewModel.kt | Added handler for ShowCardReaderConnectionDialog event and dismiss event |
| WooPosHomeUIEvent.kt | Added DismissCardReaderConnectionDialog event |
| WooPosHomeState.kt | Added CardReaderConnectionDialog dialog state |
| WooPosHomeScreen.kt | Added UI integration to show card reader connection dialog based on state |
| WooPosHomeChildToParentCommunication.kt | Added ShowCardReaderConnectionDialog event and SettingsEvent variants for connection/update dialogs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../kotlin/com/woocommerce/android/ui/woopos/home/toolbar/WooPosHomeFloatingToolbarViewModel.kt
Show resolved
Hide resolved
| fun onDisconnectClicked() { | ||
| viewModelScope.launch { | ||
| cardReaderFacade.disconnectFromReader() | ||
| controller.disconnect() | ||
| } |
Copilot
AI
Dec 16, 2025
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.
The onDisconnectClicked method now uses controller.disconnect instead of cardReaderFacade.disconnectFromReader, but there is no test coverage for this functionality. Consider adding a test case to verify that controller.disconnect is called when onDisconnectClicked is invoked, similar to how the toolbar ViewModel tests verify disconnect behavior.
| private val controller: WooPosCardReaderConnectionController by lazy { | ||
| controllerFactory.create(viewModelScope) | ||
| } |
Copilot
AI
Dec 16, 2025
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.
The controller is initialized lazily but only used for the disconnect operation. This means the controller won't be created until the first disconnect call, which could lead to unexpected behavior if other controller lifecycle management is expected. Consider either initializing it eagerly in the init block or documenting why lazy initialization is appropriate here.
| private val controller: WooPosCardReaderConnectionController by lazy { | |
| controllerFactory.create(viewModelScope) | |
| } | |
| private val controller: WooPosCardReaderConnectionController = | |
| controllerFactory.create(viewModelScope) |
samiuelson
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.
Looks good overall, @kidinov, thanks!
There's are two scenarios that are worth improving:
- Multiple, quick taps on "Connect your reader" button - the connect flow seems to be opened multiple times.
- "Pending requirements" full screen warning - consider moving to dialog 🤔
pending-req.mp4
Add guard to prevent starting a new connection flow if one is already in progress. This fixes an issue where clicking the connect button multiple times would start multiple concurrent connection flows. Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ob-1854-pr4-integration # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/cardreader/connection/WooPosCardReaderConnectionController.kt
Use custom ic_close_24dp drawable instead of Material Icons default Close icon for consistency with the app's design system. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
@samiuelson thanks for review!
Fixed here: 21101c3
This not really related to this flow/PR and as far as I remember, this is a tricky job to do. Created an issue in backlog |
samiuelson
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, thanks!
55817f5
into
woomob-1854-woopos-card-reader-connection-ui
WOOMOB-1854
Description
Wire up the card reader connection dialog into WooPos screens:
Test Steps
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.