Skip to content

Conversation

@joshheald
Copy link
Contributor

@joshheald joshheald commented Dec 6, 2022

Closes: #8321
Review after: #8328

Description

In sTAP Away, we're adding support for Apple's built in card readers to take in-person payments. During this work, we are looking to share code where practical with the bluetooth reader flows, and fix issues in those flows. One such issue is the gap between the connection flow and the payment flow.

Connection (except multiple readers) and Payment screens are all shown in a CardPresentPaymentsModalViewController, which is configured to show different view models as we move through the flow. However, with the existing implementation we dismiss the view controller after the connection flow and show a new one for the payment flow.

This results in one screen sliding offscreen downwards, and a new one immediately sliding up in its place. This is not required, and distracting for the user.

This PR uses the shared alertsPresenter across both flows and delegates dismissal of alerts to the use case, which fixes this visual glitch in the new flows.

N.B. there is still a timing issue in the alerts after tapping the card, in both bluetooth and tap on mobile. This will be dealt with in #8289

Testing instructions

Using an iPhone XS or newer on iOS 16.0 or newer

  1. Navigate to Menu > Settings > Experimental features
  2. Turn on Tap to Pay on iPhone
  3. Navigate to Menu > Payments > Collect payment
  4. Go through the payment flow, and select Card on the payment method screen
  5. When asked for a reader type, tap Tap to Pay on iPhone and go through the Terms of Service Apple ID linking (if you've not done so before)
  6. Observe that the flow goes directly from Preparing iPhone card reader to Getting ready to collect payment, without sliding one off and the other back on.

Screenshots

Before: Tap on Mobile

before-changes-tap-on-mobile.mp4

After: Tap on Mobile

new-flow-tap-on-mobile.mp4

Before: Bluetooth

before-changes-bluetooth.mp4

After: Bluetooth

new-flow-bluetooth.mp4

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

We won’t _always_ want to dismiss the connection view when we’re done connecting, e.g. when we go straight on to payment we can keep the same VC to avoid a dismiss animation immediately followed by a presentation.
Sharing the alertsPresenter between Connection and Payment flows allows us to avoid a visual gap between these flows.
@joshheald joshheald added type: enhancement A request for an enhancement. feature: mobile payments Related to mobile payments / card present payments / Woo Payments. status: feature-flagged Behind a feature flag. Milestone is not strongly held. labels Dec 6, 2022
@joshheald joshheald added this to the 11.6 milestone Dec 6, 2022
@joshheald joshheald changed the base branch from trunk to issue/8321-duplicate-payment-capture-orchestrator-for-refactor December 6, 2022 17:22
@wpmobilebot
Copy link
Collaborator

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8329-46ac624 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@joshheald joshheald marked this pull request as ready for review December 6, 2022 17:53
@joshheald joshheald requested a review from toupper December 6, 2022 17:55
@toupper toupper self-assigned this Dec 7, 2022
@toupper
Copy link
Contributor

toupper commented Dec 7, 2022

@joshheald The code looks good and it tests well 👏 :shipit:

I noticed a couple of issues that we might consider, even if not strictly related to the purpose of this PR:

  • The alert shows an inconsistent message after a user cancels the payment from the native iOS tap-to-pay screen. Do we need that cancel message at all? Furthermore, the Back to Order naming is not accurate, as we are not in an order context.

  • When the simple payment is finished, I see an info icon displayed together with Save receipt and continue button randomly, e.g when it is the first time we pay through tap to pay.

With info icon:

Without it:

/// alert viewModels such a provider is expected to provide over the course of performind
/// a card present transaction (payment or refund.)
///
protocol CardReaderTransactionAlertsProviding {
Copy link
Contributor

Choose a reason for hiding this comment

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

Clear protocol naming here 👏

Base automatically changed from issue/8321-duplicate-payment-capture-orchestrator-for-refactor to trunk December 7, 2022 13:15
@peril-woocommerce
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@joshheald
Copy link
Contributor Author

Thanks for the review @toupper

  • The alert shows an inconsistent message after a user cancels the payment from the native iOS tap-to-pay screen. Do we need that cancel message at all? Furthermore, the Back to Order naming is not accurate, as we are not in an order context.

Thanks! That one's known, covered in #8085

  • When the simple payment is finished, I see an info icon displayed together with Save receipt and continue button randomly, e.g when it is the first time we pay through tap to pay.

Yep, that one's known too, @iamgabrielma has a PR to fix it which I'm reviewing today.

@joshheald joshheald merged commit 7a75434 into trunk Dec 7, 2022
@joshheald joshheald deleted the issue/8321-close-gap-between-connection-and-payment branch December 7, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: mobile payments Related to mobile payments / card present payments / Woo Payments. status: feature-flagged Behind a feature flag. Milestone is not strongly held. type: enhancement A request for an enhancement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Mobile Payments] Prevent gap between Connection and Payment flows in IPP

4 participants