Add FPX to Basic Integration #1390

davidme-stripe · 2019-09-26T01:19:20Z

Summary

Adds FPX to STPPaymentOptionsViewController.

To support sending FPX details without first creating a PaymentMethod on the Stripe end, this makes STPPaymentMethodParams conform to and implements the appropriate UI-level methods for display in an STPPaymentOptionsViewController. FPX STPPaymentOptions will be returned as STPPaymentMethodParams objects, and can be treated similarly to STPPaymentMethods in much of Basic Integration.
Added FPX as an STPPaymentOptionType. STPPaymentOptionTypeAll is now STPPaymentOptionTypeDefault and does not include FPX. This is a breaking change, and we've added a deprecation warning with guidance to switch to .Default.
STPPaymentResult is now initialized from an <STPPaymentOption> and filled with either an STPPaymentMethod or an STPPaymentMethodParams. This means .paymentMethod is now optional, which is a breaking change, but no one will encounter a nil .paymentMethod unless they're adopting FPX. We've added a convenience function, [STPPaymentIntentParams configureWithPaymentResult:], to easily set up an STPPaymentIntentParams from an STPPaymentResult. (See CheckoutViewController.swift for usage. We'll want to update the stripe.com docs to match.)
The Standard Integration app now has a Country selector in Settings, which allows the user to set the currency to myr for FPX testing. The selected country is also passed to example-ios-backend to configure the allowed_payment_methods and currency.
Custom Integration now also supports sending a country to example-ios-backend.

Testing

Tested in Custom Integration and Standard Integration apps.

csabol-stripe

Couple minor comments but overall looks good!

csabol-stripe · 2019-09-27T23:05:37Z

Example/Custom Integration/MyAPIClient.h

 @param completion completion block called with status of backend call & the client secret if successful.
 @see https://stripe.com/docs/payments/payment-intents/ios
 */
- (void)createPaymentIntentWithCompletion:(STPPaymentIntentCreationHandler)completion;
+- (void)createPaymentIntentWithCompletion:(STPPaymentIntentCreationHandler)completion additionalParameters:(NSString * _Nullable)additionalParameters;


Is this going to convert to swift strangely? I'd probalby expect createPaymentIntentWithAdditionalParameters:(NSString * _Nullable)additionalParameters completion:(STPPaymentIntentCreationHandler)completion;

I'd also probably expect additionalParameters to be an NSDictionary but since this is sample backend code don't feel strongly on either of these

Yeah, that was roughly my feeling. It should take a dictionary and convert all the parameters properly, but this was a quick change to make an Objective-C-only sample app work, so meh. I'm more worried about how this additional parameter adds incremental complexity to our sample app.

csabol-stripe · 2019-09-27T23:06:05Z

Example/Standard Integration/BrowseProductsViewController.swift

+        numberFormatter.numberStyle = .currency
+        numberFormatter.usesGroupingSeparator = true
+        return numberFormatter
+    }()


csabol-stripe · 2019-09-27T23:12:18Z

Stripe/STPPaymentIntentParams.m

@@ -78,6 +81,14 @@ - (nullable NSString *)setupFutureUsageRawString {
    }
 }

+- (void)configureWithPaymentResult:(STPPaymentResult *)paymentResult {
+    if (paymentResult.paymentMethod) {
+        _paymentMethodId = paymentResult.paymentMethod.stripeId;


good catch, thanks!

csabol-stripe · 2019-09-27T23:12:59Z

Stripe/STPPaymentMethodParams.m

+            if (self.card != nil) {
+                STPCardBrand brand = [STPCardValidator brandForNumber:self.card.number];
+                NSString *brandString = STPStringFromCardBrand(brand);
+                return [NSString stringWithFormat:@"%@ %@", brandString, self.card.last4];


this should probably be localized. Okay for a follopwup

This isn't localized in STPPaymentMethod.m or STPCard.m either, though we do localize the similar "%@ Ending In %@" strings. We should probably localize all of them.

csabol-stripe · 2019-09-27T23:13:58Z

Stripe/STPPaymentOptionTableViewCell.m

+    self.titleLabel.text = STPLocalizedString(@"Online Banking (FPX)", @"Button to pay with a Bank Account (using FPX).");
+
+    // Checkmark icon
+    self.checkmarkIcon.hidden = YES;


does this get reset if the cell is reused for a non-FPX option?

It does! For selectable cells, the hidden field is populated with the selected state of the card.

csabol-stripe · 2019-09-27T23:14:39Z

Stripe/STPPaymentOptionTuple.h

@@ -20,7 +20,8 @@ NS_ASSUME_NONNULL_BEGIN

 + (instancetype)tupleWithPaymentOptions:(NSArray<id<STPPaymentOption>> *)paymentOptions
                  selectedPaymentOption:(nullable id<STPPaymentOption>)selectedPaymentOption
-                      addApplePayOption:(BOOL)applePayEnabled;
+                      addApplePayOption:(BOOL)applePayEnabled


can we get rid of the apple pay specific param and just use additionalOptions?

I did originally, but found that applePayEnabled is a little more complex than that. We need the appleMerchantIdentifier from STPPaymentConfiguration, which the STPPaymentOptionTuple isn't aware of, and I don't really want to pipe it all the way down here.

For reference:

- (BOOL)applePayEnabled { return self.appleMerchantIdentifier && (self.additionalPaymentOptions & STPPaymentOptionTypeApplePay) && [Stripe deviceSupportsApplePay]; }

csabol-stripe · 2019-09-27T23:16:13Z

Stripe/STPPaymentOptionsInternalViewController.m

+    return [self.paymentOptions filteredArrayUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(id<STPPaymentOption> _Nullable evaluatedObject, NSDictionary<NSString *,id> * __unused _Nullable bindings) {
+        if ([evaluatedObject isKindOfClass:[STPPaymentMethodParams class]]) {
+            STPPaymentMethodParams *paymentMethodParams = (STPPaymentMethodParams *)evaluatedObject;
+            if (paymentMethodParams.type == STPPaymentMethodTypeFPX) {


can this be like if (paymentMethodParams.type != STPPaymentMethodTypeCard) so we don't accidentally regress it when adding a new type'?

Sure, that'd be more forward-thinking. (Though a regression here would be very visible!)

csabol-stripe · 2019-09-27T23:16:59Z

Stripe/STPPaymentOptionsInternalViewController.m

+    return [self.paymentOptions filteredArrayUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(id<STPPaymentOption> _Nullable evaluatedObject, NSDictionary<NSString *,id> * __unused _Nullable bindings) {
+        if ([evaluatedObject isKindOfClass:[STPPaymentMethodParams class]]) {
+            STPPaymentMethodParams *paymentMethodParams = (STPPaymentMethodParams *)evaluatedObject;
+            if (paymentMethodParams.type == STPPaymentMethodTypeFPX) {


i think == STPPaymentMethodTypeFPX makes sense here since we don't have basic integration support for other apms (would maybe add a comment to that effect)

Makes sense.

csabol-stripe · 2019-09-27T23:41:32Z

Stripe/STPPaymentOptionsInternalViewController.m

            }
        }
-        return YES;
+        return NO;


couldn'e this still be yes in case it's an STPPaymentMethod?

csabol-stripe · 2019-09-27T23:42:57Z

Stripe/STPPaymentOptionsInternalViewController.m

            }
        }
-        return NO;
+        return YES;


ah there we go 🙃

) Long-term fix is to revert this commit, add `animated: Bool` params to SectionContainerView, AddressSection, etc., and make the `updateBillingSameAsShippingDefaultAddress` call in `AddPaymentMethodViewController.willAppear` pass `animated: false`. If it animates during viewWillAppear, the view controller presentation animation becomes glitchy.

davidme-stripe added 6 commits September 25, 2019 17:36

Add FPX to Basic Integration

5811a27

Fix comments and custom integration

d1e9120

Fix entitlements

111b12c

Fix bank icons

77a0ca6

Fix test

97c9541

Add analytics, fix up table view to handle 0 cards with APMs

7b561e0

davidme-stripe changed the title ~~[WIP] Add FPX to Basic Integration~~ Add FPX to Basic Integration Sep 26, 2019

davidme-stripe requested review from yuki-stripe and csabol-stripe September 26, 2019 22:49

davidme-stripe added 2 commits September 26, 2019 16:05

Fix CollectionView to not lose items on reload

84bb535

Fix Apple Pay appearing when not supported

4b6a8ec

csabol-stripe reviewed Sep 27, 2019

View reviewed changes

davidme-stripe added 2 commits September 27, 2019 16:37

Update based on feedback

37b0300

Fix previous bad fix, the individual responsible has been sacked

4312a4c

csabol-stripe reviewed Sep 27, 2019

View reviewed changes

Stripe/STPPaymentOptionsInternalViewController.m

}

}

return NO;

return YES;

Copy link

Contributor

csabol-stripe Sep 27, 2019

Choose a reason for hiding this comment

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

ah there we go 🙃

csabol-stripe approved these changes Sep 27, 2019

View reviewed changes

stripe-ci added the approved label Sep 27, 2019

stripe-ci assigned davidme-stripe Sep 27, 2019

davidme-stripe merged commit a1df69a into master Sep 27, 2019

davidme-stripe deleted the davidme/fpx-basic-integration branch September 27, 2019 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FPX to Basic Integration #1390

Add FPX to Basic Integration #1390

davidme-stripe commented Sep 26, 2019 •

edited

Loading

csabol-stripe left a comment

csabol-stripe Sep 27, 2019

davidme-stripe Sep 27, 2019

csabol-stripe Sep 27, 2019

csabol-stripe Sep 27, 2019

davidme-stripe Sep 27, 2019

csabol-stripe Sep 27, 2019

davidme-stripe Sep 27, 2019

csabol-stripe Sep 27, 2019

csabol-stripe Sep 27, 2019

davidme-stripe Sep 27, 2019

csabol-stripe Sep 27, 2019

davidme-stripe Sep 27, 2019

davidme-stripe Sep 27, 2019

csabol-stripe Sep 27, 2019

davidme-stripe Sep 27, 2019

csabol-stripe Sep 27, 2019

davidme-stripe Sep 27, 2019

csabol-stripe Sep 27, 2019

csabol-stripe Sep 27, 2019

Add FPX to Basic Integration #1390

Add FPX to Basic Integration #1390

Conversation

davidme-stripe commented Sep 26, 2019 • edited Loading

Summary

Testing

csabol-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidme-stripe commented Sep 26, 2019 •

edited

Loading