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

feat: support for Bacs debit payments #1352

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

Conversation

khzouroussama
Copy link

@khzouroussama khzouroussama commented Apr 11, 2023

Summary

This PR adds support for bacs debit payments

References:

Motivation

The confirmPayment function was returning This paymentMethodType is not supported yet for bacs payments, this PR adds the bindings for bacs payments for the react native sdk

Testing

  • I tested this manually
  • I added automated tests

Note: I wasn't able to test if the payment was successful, as I kept getting this error
"Your account is not configured to allow passing 'mandate_data' directly when confirming PaymentIntents for Bacs Direct Debits. Please see https://stripe.com/docs/payments/payment-methods/bacs-debit for more information or contact [bacs-debits@stripe.com](mailto:bacs-debits@stripe.com)."
According to support Bacs mandate collection will only happen on a live payment

Documentation

Select one:

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

@CLAassistant
Copy link

CLAassistant commented Apr 11, 2023

CLA assistant check
All committers have signed the CLA.

@khzouroussama khzouroussama marked this pull request as ready for review April 13, 2023 09:58
@charliecruzan-stripe
Copy link
Collaborator

Thanks for the PR! I need to find out if accountNumber and sortNumber are safe values to transmit across the RN bridge before we add support for this

@khzouroussama
Copy link
Author

khzouroussama commented Apr 14, 2023

@charliecruzan-stripe Thank you, Is this related to security concerns?

@khzouroussama
Copy link
Author

@charliecruzan-stripe any updates on this?

@andrewoons
Copy link

hi @charliecruzan-stripe , would you have an update here for us in terms of timelines?

@andrewoons
Copy link

hi @charliecruzan-stripe , would you have an estimated timeline here?

@andrewoons
Copy link

@charliecruzan-stripe any news here?

Comment on lines +173 to +174
accountNumber: string;
sortCode: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that, although not required, it would be best for us to encrypt this information before transferring it over the Bridge

Copy link
Author

Choose a reason for hiding this comment

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

@charliecruzan-stripe Could you please provide advice on what is the best way to do this? Also, does the new architecture remove the necessity for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The new architecture would remove the necessity for this, so we should wait for that migration before working on this feature

Choose a reason for hiding this comment

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

@charliecruzan-stripe Any idea the type of timeframes for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

not at the moment, the new architecture is still very experimental

Choose a reason for hiding this comment

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

@charliecruzan-stripe Is there anything I can do to help get this over the line? If the new architecture isn't ready I'm happy to try and add encryption to this PR if that helps.

@hwildwood
Copy link

Any updates on this @charliecruzan-stripe ?

@hwildwood
Copy link

@khzouroussama Did you end up building a fork of the SDK and using that?

@charliecruzan-stripe The radio silence is a bit concerning, I appreciate you're probably busy and have lots on your plate but an update on why this can't be merged would be really helpful.

@khzouroussama
Copy link
Author

@hwildwood It have been a while since I worked on this, but you can build your own package to support this (i suggest using Expo Modules since they support the new arch out of the box), or patch your existing package using the changes from this PR (although I am not sure about the security concern that @charliecruzan-stripe mentioned).

Also note that implementing the BACS payment will not be straight forward and you will have to contact stripe support to enable that for your account, and review your implementation.

https://github.com/stripe/stripe-ios/blob/7b8b320beeb54023b6c8680097a5ecfc2067f524/Example/Non-Card%20Payment%20Examples/Non-Card%20Payment%20Examples/BacsDebitViewController.swift#L14

@hwildwood
Copy link

@khzouroussama Thanks for your reply, that's really helpful! I hadn't appreciated that using the feature in this PR would still require sign off from Stripe but that makes sense.

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.

None yet

6 participants