[WOOMOB-2784] Let POS discover phone card readers without Bluetooth - #16406
Conversation
Phone readers are found over Wi-Fi, so the Bluetooth prerequisites no longer gate discovery. The Bluetooth prompts stay for Bluetooth merchants, and a discovered phone takes over from them.
kidinov
added
type: enhancement
Generated by 🚫 Danger |
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review - Found 2 potential issues
PR housekeeping
- Applied repo POS guidance from AGENTS.md: the change stays within the POS card-reader connection controller/state/dialog and follows POS conventions.
Automatic review · claude-opus-4-8 · Workflow run
How to reply to a finding
Reply on this review (or inline at the line the finding refers to) with one of:
@claude addressed- I made the change. Bot verifies against the next diff before marking resolved.@claude rejected: <reason>- Will not fix; reason gets quoted on the next review.@claude not-applicable- Finding does not apply (wrong file, already covered elsewhere, etc.).
The bot honours these on the next review pass.
| unmetBluetoothRequirement?.let { requirement -> | ||
| WooPosCardReaderConnectionState.BluetoothUnavailable( | ||
| requirement = requirement, | ||
| onFixClicked = { _state.value = requirement.toBlockingState() }, |
There was a problem hiding this comment.
AI Code Review [follow-up]
Issue: When phones are shown with the "Bluetooth unavailable" strip, tapping it sets the state to a blocking requirement screen (e.g. BluetoothDisabled) but the phone discoveryJob keeps running. The next NSD ReadersFound snapshot (a phone re-registering with a new session, or another phone appearing) flows into handleReadersFound, whose 1/else branches unconditionally set ReaderFound/MultipleReadersFound and bounce the user off the fix screen. Note the asymmetry: the Failed branch already guards with !isShowingBluetoothRequirement(), but the readers-found path does not — so a merchant can struggle to actually reach the enable/permission screen from the strip.
Suggestion: Cancel discoveryJob when navigating to the blocking state from onFixClicked (mirroring how showRemoteTapToPayExplainer cancels discovery before switching state); discovery is restarted anyway via checkRequirementsAndStartDiscovery() once the requirement is satisfied.
There was a problem hiding this comment.
Good catch, fixed in 05f258d. onFixClicked now cancels discoveryJob before switching to the blocking state, so a later NSD snapshot cannot bounce the merchant off the fix screen. This also covers the auto-connect variant, where a returning known phone would connect straight out of that screen via findLastKnownPhone.
Discovery restarts on onResume through recheckPermissions(), which already guards on isShowingBluetoothRequirement().
| } | ||
|
|
||
| @Suppress("DEPRECATION") | ||
| private fun checkRequirementsAndStartDiscovery() { |
There was a problem hiding this comment.
AI Code Review [follow-up]
Issue: The controller gained substantial new branching — prefersPhoneReader() gating, includeBluetooth propagation, the bluetoothUnavailable strip, enterNoReadersState, and the hasUsableReaders/pendingDiscoveryFailure discovery-failure gating — but WooPosCardReaderConnectionControllerTest was not extended to cover it. Only WooPosUnifiedDiscoveryStream received new tests, so the core decision logic of this PR is untested.
Suggestion: Add controller tests for the key paths: (1) a phone-preferring merchant with an unmet Bluetooth requirement stays in Scanning and discovery starts with includeBluetooth = false; (2) a discovery Failed event does not overwrite an already-found reader list (hasUsableReaders); and (3) phones dropping to zero re-derives the blocking / scanning-failed / scanning state via enterNoReadersState.
There was a problem hiding this comment.
Not taking all three. The branch names here are stale anyway, the code uses pendingBluetoothFailure and isShowingSomethingActionable().
Keeping controller coverage to the branch this PR is actually about, if anything: unmet Bluetooth requirement with no previously connected phone stays blocking and starts discovery with includeBluetooth = false.
There was a problem hiding this comment.
AI Code Review - Follow-up
- Bluetooth-unavailable strip can bounce the user off the fix screen (
onFixClickeddoesn't canceldiscoveryJob;handleReadersFoundhas noisShowingBluetoothRequirement()guard) [follow-up]: still-open - the only new commit renamed symbols;onFixClicked(WooPosCardReaderConnectionController.kt:299) still just sets the blocking state, and the size-1/else branches ofhandleReadersFoundstill overwrite it on the next phone snapshot. WooPosCardReaderConnectionControllerTestnot extended for the new branching [follow-up]: still-open - the latest push only renamed symbols (prefersPhoneReader->hasConnectedPhoneReaderBefore, etc.); the phone-preferring /includeBluetooth/enterNoReadersStatedecision logic remains untested (onlyWooPosUnifiedDiscoveryStreamTestgained coverage).
New issues: 0
PR housekeeping
- Applied repo POS guidance from AGENTS.md: the change stays within the POS card-reader connection controller/state/dialog/discovery stream and follows POS conventions (plain state machine, no store-app patterns).
Automatic review (follow-up) · claude-opus-4-8 · Workflow run
How to reply to a finding
Reply on this review (or inline at the line the finding refers to) with one of:
@claude addressed- I made the change. Bot verifies against the next diff before marking resolved.@claude rejected: <reason>- Will not fix; reason gets quoted on the next review.@claude not-applicable- Finding does not apply (wrong file, already covered elsewhere, etc.).
The bot honours these on the next review pass.
4 participants
Description
Fixes WOOMOB-2784
POS gated all card reader discovery behind Bluetooth scan/connect permissions, Bluetooth being on, fine location and location services. Phone card readers are found over Wi-Fi (NSD) and need none of that, so a merchant who only wanted to use a phone hit a Bluetooth prompt that has no close button, and their phone was never discovered. On a tablet without a Bluetooth radio it is a dead end — the "Enable Bluetooth" button has no adapter to enable.
Discovery now always runs the Wi-Fi side; the Bluetooth requirement only decides whether the Bluetooth branch starts. The prompts themselves are unchanged for merchants who need Bluetooth — they still block, and they still appear when no phone is around — but a discovered phone now takes over from them, and they offer a way into the phone-reader explainer, which was previously unreachable from that screen. Once a phone has been connected once, the prompt is skipped.
They were kept as blocking screens rather than downgraded to an inline hint because Bluetooth is by far the common flow, and for those merchants the prompt is the correct instruction, not noise.
Two related robustness fixes came out of this: a Bluetooth discovery failure no longer tears down Wi-Fi discovery with it, and
discoverReaders()is now called inside the caught flow, since it throws eagerly when Terminal is not initialised.Test Steps
Needs a tablet and a phone signed into the same store on the same Wi-Fi.
Images/gif
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.