Open POS to all countries; gate card payments per country#17252
Open POS to all countries; gate card payments per country#17252toupper wants to merge 26 commits into
Conversation
…bled Decouples POS tab visibility from country/currency eligibility — POS is now universally available on iPad (and phone with the phonePrototype flag). Card-present payments inside POS are gated by a new derived property on the existing IPP configuration: isPOSCardPaymentEnabled = isSupportedCountry && countryCode != .CA (CA is temporarily excluded pending Interac support). Also drops the .pointOfSale remote/local feature flag and the WC POS feature switch (POS going GA), and reshapes TotalsView with a promoted 1+n layout when card payments aren't available (Cash filled-primary on row 1; Scan-to-Pay + Mark-as-Paid as outlined peer buttons on row 2). The expansion-cache refresh moves from POSTabVisibilityChecker (iPad-only, inline) to MainTabBarController (all idioms) so IPP entry points still see fresh state — to be removed when the expansion cache itself goes away in 24.9/25.0. Tests: 654 PointOfSale tests pass (incl. new POSCheckoutPaymentMethodResolver + CardPresentPaymentsConfiguration+POSCardPayment suites). WooCommerce test target compiles; runtime verification of POSTab*CheckerTests blocked by unrelated Watch app icon build failure in this env. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # Modules/Sources/PointOfSale/Presentation/TotalsView.swift
toupper
added
type: enhancement
Generated by 🚫 Danger |
The app-target CardPresentPaymentService implements CardPresentPaymentFacade.isPOSCardPaymentEnabled by reading this extension property across the module boundary. Default `internal` access blocked that read with 'isPOSCardPaymentEnabled is inaccessible due to internal protection level'. Widening to `public` matches the visibility of the underlying CardPresentPaymentsConfiguration type itself (public in Yosemite), so no API surface is expanded beyond what the parent type already exposes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes 'Type CardPresentPaymentServiceScreenshotMock does not conform to protocol CardPresentPaymentFacade' build error. The screenshot mock is a fourth conformer I missed in the initial wiring pass alongside the real service, the test mock, and the preview service. Screenshots always show a connected reader, so card payments are enabled (true). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cases After dropping the .unsupportedCurrency / .featureSwitchDisabled cases from POSIneligibleReason, the refreshEligibility switch was no longer exhaustive — .siteSettingsNotAvailable fell through with no handler. Every remaining ineligibility reason now resolves to the same plugin-eligibility re-check, so the switch collapses to a single statement. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
iPad merchants in non-TTP card-supported countries (FR, DE, IE, NL, AT, BE, FI, IT, LU, PT, ES, SG, NZ, AU, PR) currently have no entry point to Scan-to-Pay / Mark-as-Paid: `useCashAndOtherMethodsBottomStrip` is TTP-tied, and the promoted-no-card layout only applies when `isPOSCardPaymentEnabled` is false. Adds an `.otherPaymentMethods` slot to POSCheckoutPaymentMethod and appends it to the card-enabled methods list whenever either secondary feature flag is on. Tapping it reuses the existing POSOtherPaymentMethodsSheet. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…TTP" This reverts commit 72a2215.
… TTP" This reverts commit b04d31e.
PR #17080 originally introduced an anchored popover for the Other payment methods picker on iPad — popover off the trigger button, with the totals context preserved behind it. Robin's phone POS Part 2 (8500620) deleted that wiring when unifying phone + iPad on a bottom sheet, which made sense for phone but regressed the iPad UX. Restore the iPad popover without disturbing phone: - POSCheckoutPaymentButtonsRow gains an optional `otherPaymentMethodsPopover` config; when provided, it attaches `.popover(attachmentAnchor: .point(.top), arrowEdge: .bottom)` directly to the `.otherPaymentMethods` button. - TotalsView routes the `.otherPaymentMethods` tap by horizontal size class: iPad (.regular) triggers the popover state; phone (.compact) keeps the existing bottom sheet path. - PointOfSaleSecondaryPaymentMethodsPopover (already in the repo from #17080, orphaned since the phone POS work) is re-wired with the existing startScanToPayPayment / startMarkAsPaidPayment callbacks. The phone bottom-sheet path is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Restores the side-by-side button layout that the original SecondaryPaymentButtons design used on iPad. Robin's phone POS work unified both idioms on a vertical stack, which fits phone-portrait but lost the iPad-landscape inline arrangement. Branches by horizontalSizeClass: - compact (phone) → VStack with first-as-primary filled CTA (phone POS design, unchanged) - regular (iPad) → HStack with equal-width peers, first still filled-primary (Card reader / Cash / Other payment methods inline) Adds `.frame(maxWidth: .infinity)` to the button label so HStack peers distribute evenly on iPad. No effect on the phone VStack since buttons there already span full width. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces my earlier reinvented iPad layout (HStack of [Card reader, Cash, Other] with first-as-filled and a row-attached popover) with a literal copy of the original SecondaryPaymentButtons from commit 8500620^. That is the design that shipped before Robin's phone POS Part 2 deleted it. Restored verbatim: - Cash + Other payment methods side-by-side in HStack with POSSpacing.small - Both buttons outlined (no filled-primary distinction) - Popover anchored to the Other button (.point(.top), .bottom arrow) - .renderedIf(isAnySecondaryPaymentMethodEnabled) on the Other button - Whole row gated by shouldShowCollectCashPaymentButton - TotalsView.Constants.buttonHorizontalPadding / cashButtonBottomPadding Branching: - iPad (horizontalSizeClass == .regular) → SecondaryPaymentButtons - Phone (.compact) → existing POSCheckoutPaymentButtonsRow VStack (unchanged) Card reader connect CTA is not in the iPad row — it lives in the hero region above as before. Cleanups: - POSCheckoutPaymentButtonsRow drops the optional OtherPaymentMethodsPopoverConfig and idiom branching I had added (no longer needed: phone uses sheet, iPad uses the dedicated SecondaryPaymentButtons view). - handlePaymentMethodSelection(.otherPaymentMethods) simplified — only the phone path reaches it now. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
On checkout entry, PointOfSaleAggregateModel.checkOut() unconditionally calls paymentModel.startPayment(). In a country where in-person payments aren't supported (e.g. Brazil, Japan, Mexico, India) or are explicitly disabled in POS (Canada), startPayment() was still entering the Stripe card-payments stack: - preferredConnectionMethod defaults to .bluetooth when TTP isn't available - the BT path immediately runs startPaymentFlow(using: .bluetooth) which begins auto-collect-on-connect — that triggers gateway onboarding / reader-scan / capability-check work that fails for IPP-unsupported countries and surfaces an "in-person payments not available" error Guard startPayment() on isPOSCardPaymentEnabled. When card is unavailable in POS for this store, return immediately — no TTP pre-connect, no BT auto-collect, no event subscriptions. The merchant will check out via Cash / Scan-to-Pay / Mark-as-Paid, all of which live in their own handlers and don't need any card-stack setup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
"Mark order as paid" wraps to two lines on iPhone while "Scan to Pay" stays on one line, pushing the row-2 peers out of vertical alignment. Add .lineLimit(1) + .minimumScaleFactor(0.7) so the longer label shrinks to fit its column on a single line. Also add .contentShape(Rectangle()) to the button label so the full button frame is the hit area — defensive against any edge case where Text-only hit testing would clip the tap target inside the rendered button bounds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The bespoke POSCheckoutPromotedPaymentButtons view (Cash filled + Scan/Mark inline peers) wasn't reacting to taps on iPhone in Brazil — root cause never isolated after end-to-end tracing of the wiring. Switching to the proven Cash + Other payment methods row that already works on US/GB phones. Changes: - Deleted POSCheckoutPromotedPaymentButtons.swift. - Resolver no-card branch now returns [.cashPayment, .otherPaymentMethods?] (single sheet entry) instead of [.cashPayment, .scanToPay?, .markOrderAsPaid?]. Tapping `.otherPaymentMethods` opens the existing POSOtherPaymentMethodsSheet which gates Scan-to-Pay / Mark-as-Paid by their own flags internally. - Dropped .scanToPay and .markOrderAsPaid from POSCheckoutPaymentMethod enum and their localized titles / accessibility ids (no longer in any array). - TotalsView body: removed the no-card-promoted top branch; iPad branch (SecondaryPaymentButtons) now fires for both card-enabled and no-card stores (it doesn't depend on card support); phone path uses POSCheckoutPaymentButtonsRow for both card-enabled and no-card stores via the resolver array. - handlePaymentMethodSelection trimmed to the four remaining cases. - Resolver tests updated for the [.cashPayment, .otherPaymentMethods?] shape. iPad UX unchanged. Phone no-card now mirrors phone card-enabled ([Cash, Other payment methods] VStack with the sheet) — fewer code paths, fewer surprises. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-existing trunk bug: `phoneContentView` only observed `paymentState.cash` to trigger navigation pushes. The `scanToPay` and `markAsPaid` handlers existed only in `tabletContentView` — phone path was missing them. Result on phone: tapping Scan-to-Pay or Mark-as-Paid inside the Other payment methods sheet flipped `paymentState.scanToPay` / `.markAsPaid` but nothing on the phone path observed the change to push the corresponding navigation destination. The sheet dismissed and nothing further happened. Mirror the tablet handlers in `phoneContentView` so the row taps actually navigate. Behaviour now matches tablet: tap → state flip → push. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After opening POS worldwide, POSTabVisibilityChecker no longer rejects by country — the cached POS tab visibility resolves to true for every store, including non-TTP card-supported countries like Spain, France, Germany. POSTapToPayAvailabilityChecker's existing site-eligibility gate read that cache and so stopped catching countries that don't support TTP at all. Result before this commit: on iPhone in Spain, the TTP hero would render on the totals view even though Spain's `CardPresentPaymentsConfiguration` only lists `.wisepad3` as a supported reader. The merchant would see a "Pay with Tap to Pay" CTA they can't actually use. Add a new gate (between the feature flag and the device-support checks) that consults `CardPresentPaymentsConfiguration.supportedReaders` for the store's country. On iOS today only US and GB pass — every other country gets `.unavailable(reason: .siteNotEligible)`, which the totals view treats correctly (no TTP hero, no TTP slot in the payment-methods row). The configuration loader is injected with a closure default so tests can stub if needed; existing test factory keeps the default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
samiuelson
left a comment
samiuelson
left a comment
There was a problem hiding this comment.
Looks good overall. I'm sharing a few suggestions inline.
| Task { @MainActor in | ||
| await paymentModel.startScanToPayPayment() | ||
| } | ||
| }, |
There was a problem hiding this comment.
💡 The iPad row wires paymentModel.startCashPayment() / startScanToPayPayment() / startMarkAsPaidPayment() directly with no isStartingPayment guard or disabled state. The phone cashAndOtherMethodsBottomStrip carefully funnels through handleCashPaymentTapped / handleOtherPaymentMethodsTapped (which check guard !isStartingPayment and set isStartingPayment = true) for exactly this reason — a quick double-tap on Cash here fires twice and pushes twice. Suggest routing the iPad button actions through the same handle…Tapped helpers, or a tiny wrapper that mirrors the phone guard.
There was a problem hiding this comment.
Good catch. Routing the iPad row through new handleScanToPayTapped / handleMarkAsPaidTapped helpers (mirroring the existing handleCashPaymentTapped / handleTapToPayTapped pattern) so all four SecondaryPaymentButtons callbacks pick up the same isStartingPayment guard the phone path uses. startCashPaymentAction now goes through handleCashPaymentTapped too — same guard.
| // which hosts its own anchored popover off the button, so `.otherPaymentMethods` | ||
| // never reaches the array-driven row on iPad. Phone presents | ||
| // `POSOtherPaymentMethodsSheet` as a bottom sheet. | ||
| handleOtherPaymentMethodsTapped() |
There was a problem hiding this comment.
💡 Same button, divergent telemetry: the iPad startOtherPaymentMethodsAction closure at L130 tracks .pointOfSaleOtherPaymentMethodsTapped before opening the popover, but the phone path here calls handleOtherPaymentMethodsTapped() (defined further down) which only sets isShowingOtherPaymentMethodsSheet = true without tracking. Add the track call to handleOtherPaymentMethodsTapped, or here right before invoking it.
There was a problem hiding this comment.
Moving analytics.track(.pointOfSaleOtherPaymentMethodsTapped) into handleOtherPaymentMethodsTapped() and adding a horizontalSizeClass branch so iPad (popover) and phone (sheet) both fire the event through the same helper. The iPad inline call site collapses to startOtherPaymentMethodsAction: handleOtherPaymentMethodsTapped — single source of truth for the tap, no divergence.
| private(set) var cardReaderUpdateState: CardReaderSoftwareUpdateState = .none | ||
|
|
||
| /// Whether card-present payments (external readers + Tap to Pay) should be exposed | ||
| /// in POS for this store. Static for the lifetime of the session — derived from the |
There was a problem hiding this comment.
💡 Doc says "Static for the lifetime of the session" but the implementation re-reads on every access via cardPresentPaymentService.isPOSCardPaymentEnabled, and CardPresentConfigurationLoader recomputes from SiteAddress() + the IPP expansion cache each time. Worth correcting — it'll mislead the next person debugging the cold-start race in MainTabBarController (commented separately).
There was a problem hiding this comment.
Right, the doc is from an earlier iteration where it was cached. Rewriting it to describe the dynamic re-read path — loader → CardPresentPaymentsCountryExpansionEligibilityService — and the fact that it can flip from false to true mid-session for expansion-flag-gated countries whose config arrives after the POS tab first renders.
| // for these countries. The merchant will check out via Cash / Scan-to-Pay / | ||
| // Mark-as-Paid only; those flows live in their own handlers and don't need any | ||
| // payment-session event subscription on the totals view. | ||
| guard isPOSCardPaymentEnabled else { |
There was a problem hiding this comment.
💡 Single-entry guard. If any other path (subscribeToPaymentSessionEvents, connectCardReader, the reader-status auto-collect, payment-intent restoration, retry helpers) dispatches into the card stack on a no-card store, the Stripe-onboarding storm this fixes can re-emerge. Defence-in-depth: add the same guard isPOSCardPaymentEnabled at the head of those entry points.
Also: this fix ships without a regression test. MockCardPresentPaymentService.isPOSCardPaymentEnabled defaults true, so POSPaymentModelTests.swift's await sut.startPayment() calls never exercise the false branch. Add a case that sets the mock false and asserts no subscribeToPaymentSessionEvents / collectPayment / connectReader dispatch.
There was a problem hiding this comment.
Defense-in-depth added to every entry point that could spin up a card-payment session: startPaymentWithMethod, connectCardReader, cancelThenCollectPayment (both sync + async forms), and subscribeToPaymentSessionEvents (stop-the-line at the subscription spin-up).
observeReaderReconnection left alone — it dispatches back to startPayment() which is already guarded, so its sink doesn't need its own gate.
New regression tests in POSPaymentModelTests:
test_startPayment_when_card_payments_disabled_then_short_circuitstest_startPaymentWithMethod_when_card_payments_disabled_then_short_circuitstest_connectCardReader_when_card_payments_disabled_then_short_circuitstest_cancelThenCollectPayment_when_card_payments_disabled_then_short_circuits
Each one sets MockCardPresentPaymentService.isPOSCardPaymentEnabled = false and asserts no subscribe / connect / collect / cancel is dispatched.
| /// either has external readers only (CA, PR, EEA, SG, NZ, AU) or no card support at all | ||
| /// (BR, JP, MX, IN, …). Without this gate the TTP hero would render on iPhone in any | ||
| /// country that has POS visibility cached, including non-TTP countries like Spain. | ||
| func countrySupportsTapToPay() -> Bool { |
There was a problem hiding this comment.
💡 configurationLoader: () -> CardPresentPaymentsConfiguration (L32, L38) is non-optional. Today the default loader fails-closed when site settings are stale, but the signature doesn't enforce it — a future loader that synthesises a .US default on a Spanish store would silently fail-open and render the TTP hero in Spain again. Either change the loader to () -> CardPresentPaymentsConfiguration? with default-deny on nil, or document the fail-closed contract on the loader.
Also uncovered by tests: POSTapToPayAvailabilityCheckerTests still uses the default loader and never injects a config without .tapToPay in supportedReaders. Add a .ES case asserting .unavailable(reason: .siteNotEligible) plus a .US happy path proving this gate sits after feature-flag and before device-support.
There was a problem hiding this comment.
Agreed. Changing configurationLoader to () -> CardPresentPaymentsConfiguration? and making the default impl map .unknown country → nil so a future synthesised-.US-default loader fails the type-check rather than silently passing through. countrySupportsTapToPay() guards on nil → false.
New tests in POSTapToPayAvailabilityCheckerTests:
checkAvailability_when_country_does_not_support_TTP_then_returns_siteNotEligible(uses.ES)checkAvailability_when_country_supports_TTP_then_continues_to_other_gates(uses.US)checkAvailability_when_configuration_loader_returns_nil_then_fails_closed(the explicit nil contract)
| case wooCommercePluginNotFound | ||
| case featureSwitchDisabled | ||
| case unsupportedCurrency(countryCode: CountryCode, supportedCurrencies: [CurrencyCode]) | ||
| case selfDeallocated |
There was a problem hiding this comment.
💡 siteSettingsNotAvailable (L6) and selfDeallocated (L8) are now unreachable — nothing in the simplified POSTabEligibilityChecker.checkPluginEligibility produces them, and refreshEligibility just re-runs the plugin check. They're still in the enum, the analyticsValue, the suggestion strings, and the previews. Either delete them, or wire them so the cases stay live (the refreshEligibility comment implies they should, but no caller does).
There was a problem hiding this comment.
You're right, those are vestigial. Removing both cases from the enum and updating every consumer:
POSIneligibleView.suggestionText— switch cases + theirNSLocalizedStrings deletedPOSIneligibleViewpreview that rendered.siteSettingsNotAvailabledeletedWooAnalyticsEvent+PointOfSaleIneligibleUI.analyticsValue— "other" bucket deleted (it only mapped these two)POSTabEligibilityChecker.refreshEligibilitydoc updated to drop the referencesPointOfSaleDashboardViewHelperTestsparameterized arguments trimmedPOSTabEligibilityCheckerTests.refreshEligibility_for_selfDeallocated_rechecks_eligibilitydeleted (redundant — the parameterizedrefreshEligibility_returns_eligible_when_plugin_now_meets_requirementsalready covers the same code path with the valid reasons)
| /// POS visibility no longer gates on this cache (POS is universal now), so the refresh | ||
| /// has to happen here regardless of idiom for IPP entry points to pick up the latest | ||
| /// remote-feature-flag state. | ||
| func refreshCardPresentExpansionEligibilityIfNeeded(for site: Site) { |
There was a problem hiding this comment.
💡 Cold-start race in expansion-flag-gated countries. With POS now opening immediately on iPad/phone (no inline visibility refresh), if this method hasn't completed before POSPaymentModel.isPOSCardPaymentEnabled is first read — Spain / FR / DE / AU / NZ / SG merchants on first session post-login or after a tab restore — they see cash-only checkout in a country that's actually card-eligible. No observer flips it back when the refresh resolves, because isPOSCardPaymentEnabled is queried-on-read and TotalsView doesn't re-observe. Worth either awaiting the refresh before letting TotalsView render, or surfacing a re-evaluate signal TotalsView can observe.
There was a problem hiding this comment.
Implementing your option (b) — surface a re-evaluate signal the View can observe. Wired end-to-end:
- Yosemite layer:
CardPresentPaymentsCountryExpansionEligibilityService.cacheEligibilitynow postsNotification.Name.cardPresentPaymentsCountryExpansionEligibilityDidChangeafter the cache write. - PointOfSale facade:
CardPresentPaymentFacadegainsisPOSCardPaymentEnabledPublisher: AnyPublisher<Bool, Never>. - App-target adaptor:
CardPresentPaymentServicebacks the publisher with aCurrentValueSubject<Bool, Never>, seeded from the loader at init and updated by subscribing to the notification.isPOSCardPaymentEnabledis now read fromsubject.value. - POSPaymentModel:
isPOSCardPaymentEnabledis now a stored@Observableproperty mirrored from the publisher via the newpublishPOSCardPaymentEnabled()helper. SwiftUI views observing the model re-render automatically when the value flips. - Mocks/previews:
MockCardPresentPaymentService,CardPresentPaymentPreviewService,CardPresentPaymentServiceScreenshotMockall implement the new publisher.
Regression test added: test_isPOSCardPaymentEnabled_updates_when_facade_publisher_emits_new_value — sets up the mock with isPOSCardPaymentEnabled = false, asserts the model mirror starts at false, flips the mock to true, uses withObservationTracking to await the model's update, asserts the mirror flipped to true.
There was a problem hiding this comment.
Update: dropping the publisher-pipeline approach in this PR after an internal nudge that introducing a new NotificationCenter contract + a new CardPresentPaymentFacade protocol requirement + a new observable-mirror pattern in POSPaymentModel qualifies as an architectural change that should be discussed in a P2 first. Reverted in 9df447886c.
The defense-in-depth guards (the other thread above, comment #4) are kept — they're not architectural and ship clean in e00946ea54.
The cold-start race itself stays open: I'll spin up a P2 outlining the publisher-based fix + simpler alternatives (await refresh before POS opens; reuse existing site-settings notification; pull-to-refresh in POS; just document the limitation) and follow up with a separate PR once a direction is picked. The race only affects first-cold-start for expansion-flag-gated merchants (Spain / FR / DE / AU / NZ / SG), so deferring is OK for now.
| /// `POSCheckoutPaymentButtonsRow` VStack. Brought back unchanged so the iPad UX matches | ||
| /// what shipped before the phone POS work landed — `.cardReader` isn't surfaced as a row | ||
| /// button on iPad because the "Connect your reader" CTA lives in the hero region above. | ||
| private struct SecondaryPaymentButtons: View { |
There was a problem hiding this comment.
💡 80+ lines of new view code here have no test coverage; the resolver tests deliberately don't help because iPad bypasses POSCheckoutPaymentMethodResolver. The isShowingPaymentView / useTapToPayHeroLayout defensive && isPOSCardPaymentEnabled gates at L313 and L846 are also unasserted, so a future refactor could silently strip them and CA / JP / MX merchants would see the TTP hero or card-reader region again. Plan v2 asks for ViewInspector assertions — at minimum, a couple of cases asserting card-region-present iff isPOSCardPaymentEnabled == true would catch the regression.
There was a problem hiding this comment.
Covering the gating logic at the model + resolver layers rather than wiring up ViewInspector for the View itself:
- POSPaymentModel layer: 5 new tests in
POSPaymentModelTests(see the defense-in-depth thread above) cover every entry point that drives the View's gates —isPOSCardPaymentEnabledshort-circuit on each, plus the publisher-mirror regression test for the dynamic update. - Resolver layer:
POSCheckoutPaymentMethodResolverTestsalready coversisPOSCardPaymentEnabled = false→ no card / TTP entries in the row (this drivescheckoutPaymentMethodsat L775). - View-layer gates:
useTapToPayHeroLayout(L846) andisShowingPaymentView(L310) are 1-lineguard isPOSCardPaymentEnabled else { return false }reads of the observable model. A ViewInspector test of these would essentially be testing SwiftUI's@Observablere-render behaviour, and adding ViewInspector toPointOfSaleTestsas a dependency for that marginal extra coverage felt disproportionate.
That said — happy to add ViewInspector and the two snapshot-style tests in a follow-up if you'd prefer explicit View-layer regression guards.
| // nothing observes the change here — the sheet dismisses and the merchant | ||
| // sees no follow-up screen. | ||
| .onChange(of: posModel.paymentState.scanToPay) { oldValue, newValue in | ||
| if newValue.isShowingQRCode, !oldValue.isShowingQRCode, |
There was a problem hiding this comment.
💡 The pre-existing trunk bug these onChange blocks fix ("sheet dismisses, no follow-up screen") ships without a UI-layer test. Resolver tests only assert the method list, not the navigation side-effect wiring. A ViewInspector-style assertion that the router's pushScanToPay / pushMarkAsPaid fires when paymentState transitions on phone would catch silent regressions of this exact bug.
There was a problem hiding this comment.
Same call as the TotalsView coverage thread above: the underlying paymentState.scanToPay / paymentState.markAsPaid transitions are exercised in POSPaymentModelTests, and the navigation handlers themselves are 4-line state-transition triggers (if newValue.isShowingQRCode, !oldValue.isShowingQRCode → pushScanToPay / if newValue == .confirming, oldValue == .idle → pushMarkAsPaid). They've been verified manually on phone.
Adding ViewInspector to PointOfSaleTests to assert the router spy receives the call is doable in a follow-up — I'd rather not introduce that dependency for a single test in this PR. Happy to do it if you prefer.
Closes the cold-start race for expansion-flag-gated countries (Spain / FR / DE / AU / NZ / SG …) by piping the country-expansion eligibility cache through a Combine publisher that POSPaymentModel mirrors into an observable property. Views re-render when the configuration arrives after the POS tab first renders. Adds defense-in-depth isPOSCardPaymentEnabled guards on every POSPaymentModel entry point that could spin up a Stripe card-payment session.
…mentEnabled" This reverts commit f44d3ff.
…ockStorageManager reference POSTabVisibilityCheckerTests uses UIUserInterfaceIdiom (UIKit) without importing it. POSTabEligibilityCheckerTests declared a MockStorageManager (YosemiteTestHelpers) it never used — drop the dead reference instead of importing the helper module just to satisfy the compiler.
…o-card stores When isPOSCardPaymentEnabled is false, both useTapToPayHeroLayout and isShowingPaymentView gate on the card-payment flag, leaving the upper region empty after a cash / scan-to-pay / mark-as-paid success — the PaymentViewContent (which holds the success message branches) never renders. Extend shouldPrioritizePaymentViewOverHero to also return true for those non-card success states so the success message takes over the region regardless of the card-payment gate.
# Conflicts: # Modules/Sources/Yosemite/PointOfSale/Eligibility/POSCountryCurrencyValidator.swift # Modules/Tests/YosemiteTests/PointOfSale/Eligibility/POSCountryCurrencyValidatorTests.swift # WooCommerce/WooCommerceTests/ViewRelated/Settings/POS/POSTabEligibilityCheckerTests.swift # WooCommerce/WooCommerceTests/ViewRelated/Settings/POS/POSTabVisibilityCheckerTests.swift
|
I have not checked the code, just got a general question regarding testing and stabilization: If we had to cherry-pick and setup a pipeline of gradual expansion through the next couple of releases (let's say 24.9, 25.0, 25.1), would you say this is independently testable and shippable without introducing much risk in one go? Let's say:
This would allow us to use the current countries with the new infrastructure, then enable cash-only usage data from JP/MX/BR/IN merchants for min 2 weeks before opening more card-enabled countries, as well as do some intermediate internal call for testing. Thoughts? |
Thanks for the comment @iamgabrielma! I agree that adding this gradually is the way to go. I will refine this into a project, and outline the best process to land this in the safest way. Once we'll have Phone POS and other RSM projects, this will become easier and clearer. |
5 participants
Description
Opens POS to every store, worldwide while keeping card-present payments (external readers + Tap to Pay) gated per-country. Drops the country/currency eligibility validator, the
pointOfSaleremote feature flag check, and the WC POS feature switch — POS is GA now and visibility is just iPad + WC plugin present (or phone +pointOfSalePhonePrototype). The card-payment availability inside POS is derived from the existingCardPresentPaymentsConfigurationvia a newisPOSCardPaymentEnabledextension (currentlyisSupportedCountry && countryCode != .CA— CA is temporarily excluded pending Interac).The checkout screen adapts per idiom + country:
SecondaryPaymentButtons— Cash + Other (outlined peers, anchored popover off Other). Literal restoration of the design from #17080 that Robin's phone POS Part 2 (8500620a69) deleted.cashAndOtherMethodsBottomStrip(unchanged from phone POS work)cashAndOtherMethodsBottomStrip(unchanged)POSCheckoutPaymentButtonsRow:[Card reader, Cash, Other payment methods]POSCheckoutPaymentButtonsRow:[Cash, Other payment methods]What's in the diff
Core feature work:
POSCountryCurrencyValidator+ its testsPOSTabVisibilityChecker: iPad-only (or phone +pointOfSalePhonePrototype) + plugin presence; drop country/currency validation and the.pointOfSaleremote-flag checkPOSTabEligibilityChecker: plugin presence + min version only; drop country/currency validation and the WC POS feature-switch branchPOSIneligibleReason.unsupportedCurrencyand.featureSwitchDisabled(and the associated UI / analytics branches)POSTabVisibilityChecker(iPad-only, inline) toMainTabBarController(all idioms) — required because POS visibility no longer drives the refresh. Marked with a TODO; the expansion cache itself is being removed in 24.9/25.0.CardPresentPaymentsConfiguration.isPOSCardPaymentEnabled(in PointOfSale module,public) — derived fromisSupportedCountry && countryCode != .CAisPOSCardPaymentEnabledthroughCardPresentPaymentFacade→ real service, preview service, screenshot mock, test mock →POSPaymentModelTotalsView reshape:
isShowingPaymentViewanduseTapToPayHeroLayoutonisPOSCardPaymentEnabled(defensive — no card UI on no-card stores)horizontalSizeClass == .regular): renderSecondaryPaymentButtonsfor both card-enabled and no-card cases — Cash + Other side-by-side outlined peers, popover anchored to Other (PointOfSaleSecondaryPaymentMethodsPopover, which was already in the repo but orphaned by the phone POS work)POSCheckoutPaymentButtonsRowwith the resolver-built methods array. Tapping.otherPaymentMethodsopens the existingPOSOtherPaymentMethodsSheetbottom sheetPOSCheckoutPaymentMethodResolver(pure helper) so the branching is unit-testable without a SwiftUI environmentBug fixes surfaced by testing the universal flow:
POSPaymentModel.startPayment()short-circuits when!isPOSCardPaymentEnabled— without this, on iPhone Brazil/Japan/Mexico/etc. the BT auto-collect-on-connect kicked in (no TTP →preferredConnectionMethod = .bluetooth) and triggered Stripe onboarding/gateway checks that fail for IPP-unsupported countriesphoneContentViewnow observespaymentState.scanToPayand.markAsPaidto push navigation (tablet side already did this; the phone path was missing both handlers — pre-existing trunk bug that became visible because the no-card phone flow needs the Other-payment-methods sheet on every store)POSTapToPayAvailabilityCheckergains acountrySupportsTapToPay()gate that consultsCardPresentPaymentsConfiguration.supportedReaders.contains(.tapToPay). Without it, after the POS visibility cache went universal, TTP availability resolved to.availablefor any country including ones that don't support TTP (Spain etc.) — the hero would render with a CTA the merchant couldn't actually useTest Steps
Country is switchable via WP admin → WooCommerce → Settings → General → Country/State. After changing, kill and relaunch the app so site settings reload.
Local feature flags relevant (in
Modules/Sources/Experiments/FeatureFlag.swift):pointOfSalePhonePrototype(gates phone POS),pointOfSaleScanToPay,pointOfSaleMarkOrderAsPaid(gate the sheet/inline rows). All three default to.localDeveloper/.alphaON.iPad regression — US / GB / EU / SG / NZ / AU / PR:
[Cash] [Other payment methods]side-by-side outlined peersiPad no-card — CA / JP / MX / IN / BR:
[Cash] [Other payment methods]rowiPhone TTP-eligible — US / GB (
pointOfSalePhonePrototypeON):iPhone non-TTP card — FR / DE / ES / etc. (
pointOfSalePhonePrototypeON):[Card reader (filled)] [Cash] [Other payment methods]iPhone no-card — BR / JP / MX / CA (
pointOfSalePhonePrototypeON):[Cash (filled)] [Other payment methods]Ineligible UI sanity:
Tests:
xcodebuild test -scheme PointOfSale→ 661 tests pass (incl. newPOSCheckoutPaymentMethodResolverTests,CardPresentPaymentsConfiguration+POSCardPaymentTests).Screenshots
N/A — visual changes covered by the test plan above. iPad layout matches the original design from #17080 verbatim; iPhone card-enabled inherits the existing phone POS UI; iPhone no-card uses the same Cash + Other row + bottom sheet shape proven on US/GB phones.
On the diff size
~−1300 net non-test lines (most of the diff is removing the eligibility validator + checker plumbing). Above the 300-line Danger cap. The two halves (cleanup + no-card surface) are tightly coupled, but if a split is wanted I can do it as a follow-up — let me know.
Out of scope (follow-ups)