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 captureMethod to createPaymentIntent on iOS #443

Closed
wants to merge 5 commits into from

Conversation

mihaildu
Copy link
Contributor

Summary

We can now call createPaymentIntent({ captureMethod: 'automatic', ... }) from RN

Motivation

Related to the discussion here #439 (comment)

Testing

  • I tested this manually
  • I added automated tests

I called createPaymentIntent({ captureMethod: 'automatic', ... }) in RN and saw the API request being made in stripe dashboard with capture_method set to automatic

Documentation

Select one:

  • I have added relevant documentation for my changes.
  • This PR does not result in any developer-facing changes.

Copy link
Collaborator

@billfinn-stripe billfinn-stripe left a comment

Choose a reason for hiding this comment

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

This is great, thanks. If you're feeling ambitious, we'd appreciate support for adding an automatic/manual picker to our example and dev apps. It'd mean adding a new Picker like we have for currency [0] to allow users to select between manual and automatic when starting the payment collection process. We'd pass the selected value to the existing calls to createPaymentIntent [1].

[0]

<List bolded={false} topSpacing={false} title="CURRENCY">
<Picker
selectedValue={inputValues?.currency}
style={styles.picker}
itemStyle={styles.pickerItem}
testID="select-currency-picker"
onValueChange={(value) =>
setInputValues((state) => ({ ...state, currency: value }))
}
>
{CURRENCIES.map((a) => (
<Picker.Item
key={a.value}
label={a.label}
testID={a.value}
value={a.value}
/>
))}
</Picker>
</List>

[1]

const response = await createPaymentIntent({

const response = await createPaymentIntent({

@@ -161,6 +161,7 @@ export type CreatePaymentIntentParams = CreatePaymentIntentIOSParams & {
transferGroup?: string;
metadata?: Record<string, string>;
paymentMethodOptions?: PaymentMethodOptions;
captureMethod?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you update this to be 'automatic' | 'manual'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@mihaildu mihaildu Mar 14, 2023

Choose a reason for hiding this comment

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

As a side note, maybe it would be nicer if you update in a future version the type for paymentMethodTypes as well. I see you only allow 'cardPresent' | 'interacPresent' | 'card' for paymentMethodType but on the backend the list is bigger https://stripe.com/docs/api/payment_intents/create#create_payment_intent-payment_method_types

I'm also confused why it's camel case for the SDK, I tested with snake case and it worked fine!

Copy link
Collaborator

Choose a reason for hiding this comment

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

As a side note, maybe it would be nicer if you update in a future version the type for paymentMethodTypes as well. I see you only allow 'cardPresent' | 'interacPresent' | 'card' for paymentMethodType but on the backend the list is bigger https://stripe.com/docs/api/payment_intents/create#create_payment_intent-payment_method_types

Terminal really only supports card_present and interac_present (we may even want to remove "card"). Terminal is meant to support in-person (aka, card-present) payments. Type card is used for card-not-present payments (e.g., for e-commerce).

I'm also confused why it's camel case for the SDK, I tested with snake case and it worked fine!

Yeah, that's a good point. It looks like for Stripe API responses, we translate to the camel-case versions [0] (only for Setup Attempts and Refunds -- not Payment Intents), but when setting the params, we expect and use snake case [1]. They are being passed through directly to the native iOS SDK, and they should be snake case (since that's also what the native SDK and backend expect).

Agree this could use some cleaning up. Thanks for pointing it out.

[0]

class func mapFromPaymentMethodDetailsType(_ type: PaymentMethodType) -> String {
switch type {
case PaymentMethodType.card: return "card"
case PaymentMethodType.cardPresent: return "cardPresent"
case PaymentMethodType.interacPresent: return "interacPresent"
default: return "unknown"
}
}

[1]
const paymentMethods = ['card_present'];
if (enableInterac) {
paymentMethods.push('interac_present');
}

ios/StripeTerminalReactNative.swift Outdated Show resolved Hide resolved


let paymentIntentParams = PaymentIntentParameters(amount: UInt(truncating: amount), currency: currency, paymentMethodTypes: paymentMethodTypes)
let paymentIntentParams = PaymentIntentParameters(amount: UInt(truncating: amount), currency: currency, paymentMethodTypes: paymentMethodTypes, captureMethod: captureMethod == "automatic" ? .automatic : .manual)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: this line is pretty long. Can you add newlines between each parameter like we do here?

let connectionConfig = LocalMobileConnectionConfiguration(
locationId: locationId ?? selectedReader.locationId ?? "",
merchantDisplayName: nil, // use the location name,
onBehalfOf: onBehalfOf ?? nil
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mihaildu
Copy link
Contributor Author

This is great, thanks. If you're feeling ambitious, we'd appreciate support for adding an automatic/manual picker to our example and dev apps.

I added for dev-app here d760bf1, for example-app you need to publish a new package version. I didn't get a chance to test yet, so if you can that would be great!

@billfinn-stripe
Copy link
Collaborator

@mihaildu -- this is great, thanks again. I appended a few more commits to add Android support and opened a new PR #447 (because I don't think I can ~easily push commits to this branch/PR).

@billfinn-stripe
Copy link
Collaborator

Closing this PR, now that #447 has landed.

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.

2 participants