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.

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

Forwarding apple pay presentation completion flag to the Stripe SDK client #1888

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

MadGeorge
Copy link

@MadGeorge MadGeorge commented Oct 17, 2021

Summary

Forwarding apple pay presentation completion boolean parameter to the top level

Motivation

There is a case (or many of them) when PKPaymentAuthorizationController will not be shown on a presentation attempt.
Apple docs have no clue about the underlying reason or how to determine the error description, but it may happen.
The problem with the current Stripe SDK wrapper around PKPaymentAuthorizationController is it hides this flag assuming the authorization controller will never fail to show.

The real-life example demonstrated the problems it lead to:

  • iOS app prepares to call presentApplePay(completion:) and locks its UI with some progress indication
  • iOS app will unlock its UI only in response to applePayContext(:didCompleteWith:error:)
  • PKPaymentAuthorizationController failed to appear
  • applePayContext(:didCompleteWith:error:) will never called
  • iOS app will remain locked infinitely

Testing

We have a physical device in the Stripe production environment where applePayController.present completion block returns a negative flag. We tested these changes within our app and verified it allow us to handle this case on the top level.

Breaking changes

Clients who do not use completion closure are not affected.
Clients who rely on completion closure should update the current implementation either with a closure parameter placeholder or take the new flag into account and handle the negative case appropriately.

Other approaches

During implementation, we had two options

  1. flag forwarding (current implementation)
  2. propagate error through applePayContext(:didCompleteWith:error:)

The first thought was to call applePayContext(:didCompleteWith:error:) whenever applePayController.present sends false.
The problem is - we have no information about the actual error since Apple never return one.
The second reason - this case stands out far away from didAuthorizePayment delegate call and there are should be reasons why the completion flag propagated separately in PassKit (besides the indifference). Looks like we should follow the same way.

Notes and offtopic

  • Why do not reuse STPBooleanSuccessBlock with false and nil?
    Because it will mislead SDK users since it suggests there are maybe an error with a description but there are not

@CLAassistant
Copy link

CLAassistant commented Oct 17, 2021

CLA assistant check
All committers have signed the CLA.

@davidme-stripe
Copy link
Contributor

Thank you for this submission and finding this issue, and I'm sorry that we haven't responded sooner! As this is a breaking API change, we'll have to discuss it internally. We're considering some other changes to STPApplePayContext soon, so I'll try to batch this in with those.

@davidme-stripe davidme-stripe added the triaged Issue has been reviewed by Stripe and is being tracked internally label Jan 13, 2022
@davidme-stripe davidme-stripe dismissed a stale review via 7cd4453 October 29, 2022 02:31
@Metin6201

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triaged Issue has been reviewed by Stripe and is being tracked internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants