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

Update PaymentIntents support #1038

Merged
merged 16 commits into from
Nov 14, 2018
Merged

Conversation

danj-stripe
Copy link
Contributor

Summary

  • return_url removed from top-level PaymentIntent object
  • parsing & exposing nextSourceAction, with the only possible type being AuthorizeWithURL
  • using returnURL from the nextSourceAction instead of the top-level returnURL in
    STPRedirectContext

Motivation

IOS-1013

Testing

Updated automated tests

Added TODOs to pull the replacement property in the right places.
It's confusing to print `type = authorize_with_url` if decoding the source action details
as a `STPPaymentIntentSourceActionAuthorizeWithURL` object fails. Instead, log the value
that the enum currently has.
…edirectContext`

Also, since we're using our normal object parsing code, could simplify the whitebox test
that tried unexpected types during deserialization - covered elsewhere.
@danj-stripe danj-stripe changed the title WIP: Update PaymentIntents support Update PaymentIntents support Nov 8, 2018
@danj-stripe
Copy link
Contributor Author

This PR relies on #1037, so it is not using master as the base branch and the CI tests haven't run against it. It looked good locally, and worked when I used the example app to create & confirm PaymentIntents.

I think the code's ready to review, but it'll probably require another 👍 after #1037 is merged and the base branch is changed.

@danj-stripe danj-stripe changed the base branch from danj/feature/all-caps-URL to master November 13, 2018 20:55

These are created & owned by the containing `STPPaymentIntent`.
*/
@interface STPPaymentIntentSourceActionAuthorizeWithURL: NSObject<STPAPIResponseDecodable>
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of renaming this something like STPPaymentIntentSourceActionURLAuthorizationData? AuthorizeWithURL is a bit confusing to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured consistency with the API (where this shows up as authorize_with_url, plus the other stripe SDKs (which use essentially the same thing), was a good reason to go with this monstrosity of a name. It's my hope that users of the library never actually really interact with this class - I suspect they'll just access it's fields through the STPPaymentIntent object

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense

@danj-stripe danj-stripe merged commit 8894913 into master Nov 14, 2018
@danj-stripe danj-stripe deleted the danj/feature/paymentintents-update branch November 14, 2018 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants