search

stripe-archive / stripe-payments-demo

Sample store accepting universal payments on the web with Stripe Elements, Payment Request, Apple Pay, Google Pay, Microsoft Pay, and the PaymentIntents API. 💳🌍✨
https://stripe-payments-demo.appspot.com
MIT License
1.47k stars 547 forks source link

Adding AU BECS Debit #141

Closed rgultiano-stripe closed 4 years ago

rgultiano-stripe commented 4 years ago

This PR will include code changes to include AU BECs Direct Debit stripe element. In order to support this it also includes a multi-currency drop down.

This code is still WIP. As of opening the following is complete:

  1. Initial code for client side is complete (Though may still get some clean-up and commenting)
  2. Code for node.js server is complete
  3. Code for other server languages (However, I may need support in how to test these)

Latest update: Removed multi-currency code, and merged changes made to the master branch. Requesting re-review.

c.c. @thorsten-stripe

thorsten-stripe commented 4 years ago

This is great, thanks @rgultiano-stripe.

One questions: rather than having a separate select element for the currency, would it make sense to tie the currency to the selected country?

So when you select Australia the currency automatically changes to AUD etc.

I think that makes it easier to navigate the demo and see the correct payment methods. Wdyt?

rgultiano-stripe commented 4 years ago

@thorsten-stripe Yeah, I had initially done that but wasn't sure if indirectly overwriting the configuration currency was okay. If that is no problem, I could change it back to that logic/functionality! I do agree, it is much simpler for navigation that way.

thorsten-stripe commented 4 years ago

Yes, let's err on the side of simplicity for the demo viewer. You could add an FX item to the order summary sidebar, but since this is a demo which is only meant to be used in test mode it's fine to not even do that IMO 👍

The currency conversion config table is somewhat difficult, since most of the other server implementations don't have a config file, so not entirely sure how to elegantly handle that. I'd be fine with not doing any conversion and only changing the currency, since it's only a demo. Wdyt?

rgultiano-stripe commented 4 years ago

@willock-stripe Thanks for the review again! I really appreciate it! I've updated as per your recommendations and taken it out of WIP (Please let me know if I'm doing this process wrong, it's my first ever solo PR)

dalan-stripe commented 4 years ago

@rgultiano-stripe looks like there's now a conflict, can you update the PR?

dalan-stripe commented 4 years ago

ptal? @willock-stripe

dalan-stripe commented 4 years ago

ptal? @thorsten-stripe

rgultiano-stripe commented 4 years ago

@willock-stripe Thanks for the review! Took me a bit to find time to get the changes in but I've resolved most except the first comment where I went another direction and removed the variable and just returned out of the function if it fails. Do you think this this a suitable alternative?