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

Add BankSelectionViewController for FPX #1375

Merged
merged 11 commits into from
Sep 23, 2019

Conversation

davidme-stripe
Copy link
Contributor

Summary

  • Adds BankSelectionViewController, which accepts a bank type and returns a PaymentMethodParams.

  • PaymentMethodParams is now an STPPaymentOption, and can be treated as sort of a PaymentMethod promise, allowing a user to send it directly to the Stripe API when confirming their PaymentIntent. This saves a roundtrip to the server to create separate FPX PaymentMethod, which isn't necessary as the only metadata will generally be the name of the bank.

  • The FPX example in Custom Integration has been modified to show how we expect developers to integrate this.

  • I've also added a new authenticationContextDidPresentSafariViewController delegate method to give the developer a chance to configure their view underneath the SafariViewController. This allows a developer to remove the FPX bank selector from the view controller stack while the user is handling the authentication request, providing a better experience when the payment completes. You can see an example of how this is used in FPXExampleViewController — I'm not super happy with it, but it seems like the least gross option of the options I considered. Feedback is welcome!

Testing

Tested using the Custom Integration app.

Copy link
Contributor

@csabol-stripe csabol-stripe left a comment

Choose a reason for hiding this comment

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

Main concern is with BankType name. Otherwise looks good!

Stripe/PublicHeaders/STPFPXBankBrand.h Outdated Show resolved Hide resolved
Stripe/STPBankSelectionTableViewCell.m Show resolved Hide resolved
Stripe/STPBankSelectionTableViewCell.m Show resolved Hide resolved
- (void)updateAppearance {
[super updateAppearance];

self.tableView.allowsSelection = YES;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, allowsSelection is the default, and this method was copied from another VC that doesn't seem to need it either. I'll clean that one up too.

}

- (NSInteger)tableView:(__unused UITableView *)tableView numberOfRowsInSection:(__unused NSInteger)section {
return STPFPXBankBrandUnknown;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how consistent we are with always making Unknown the last item in an enumeration (hopefully we are) but 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're pretty consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

sweet!

Stripe/STPBankSelectionViewController.m Show resolved Hide resolved
Stripe/STPPaymentMethodParams.m Outdated Show resolved Hide resolved
Stripe/STPBankSelectionTableViewCell.h Outdated Show resolved Hide resolved
Stripe/STPBankSelectionTableViewCell.m Outdated Show resolved Hide resolved
Example/Custom Integration/FPXExampleViewController.m Outdated Show resolved Hide resolved
Example/Custom Integration/FPXExampleViewController.m Outdated Show resolved Hide resolved
@@ -23,7 +23,7 @@ NS_ASSUME_NONNULL_BEGIN

@see https://stripe.com/docs/api/payment_methods/create
*/
@interface STPPaymentMethodParams : NSObject <STPFormEncodable>
@interface STPPaymentMethodParams : NSObject <STPFormEncodable, STPPaymentOption>
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this conformance needed for this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this was mistakenly left in from the Standard Integration support (which will be in another PR).

Stripe/STPBankSelectionViewController.h Outdated Show resolved Hide resolved
Stripe/STPBankSelectionViewController.h Outdated Show resolved Hide resolved
[self.navigationController pushViewController:vc animated:YES];
}

- (void)payWithBankAccount:(STPPaymentMethodParams *)pmParams {
Copy link
Collaborator

Choose a reason for hiding this comment

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

supernit: paymentMethodParams


- (void)authenticationContextWillDismissViewController:(UIViewController *)viewController {
// If we're launching directly into the SFSafariViewController for authentication from the bank selector,
// we'll want to remove the bank selector from the VC stack and pop back directly to our main controller on dismiss.
Copy link
Collaborator

Choose a reason for hiding this comment

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

supernit: as a user I might not understand why we want to do this (is it necessary? what happens if I don't do it?)
Suggestion: "// remove the bank selector from the VC stack and pop back directly to our main controller on dismiss for better UX".

@davidme-stripe davidme-stripe merged commit 18e511b into master Sep 23, 2019
@davidme-stripe davidme-stripe deleted the davidme/fpx-view-controller branch September 23, 2019 17:56
ramont-stripe pushed a commit that referenced this pull request Sep 1, 2022
* Fix sending incorrect analytic for autocomplete manual entry

* Update Configuration to match API review

* Update address types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants