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: add Klarna to confirmPayment payment methods #821

Merged
merged 14 commits into from
Mar 7, 2022

Conversation

charliecruzan-stripe
Copy link
Collaborator

Summary

Add the bindings for users to pay with Klarna programmatically

Motivation

Klarna was supported on paymentsheet, but there were no bindings so folks couldn't use it programatically

Testing

  • I tested this manually
  • I added automated tests

Documentation

Looks like there used to be a Klarna docs page, but it was removed? I'm not sure why. Typescript will auto-doc that confirmPayment now accepts Klarna

@charliecruzan-stripe charliecruzan-stripe marked this pull request as ready for review February 24, 2022 23:13
@charliecruzan-stripe charliecruzan-stripe requested review from csabol-stripe and removed request for acomley-stripe February 24, 2022 23:13
@charliecruzan-stripe
Copy link
Collaborator Author

requesting reviews from android/ios run cc @csabol-stripe @skyler-stripe whenever you get a chance to take a peak 😄

Copy link

@skyler-stripe skyler-stripe left a comment

Choose a reason for hiding this comment

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

LGTM. I'm not good at react but the logic for the requirements makes sense.

@@ -344,6 +359,8 @@ extension PaymentMethodError: LocalizedError {
return NSLocalizedString("You must provide CVC number", comment: "Create payment error")
case .weChatPayPaymentMissingParams:
return NSLocalizedString("You must provide appId", comment: "Create payment error")
case .klarnaPaymentMissingParams:
return NSLocalizedString("Klarna requires that you provide the following billing details: email, country", comment: "Create payment error")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know we're just copying the pattern here, but this raises some qs for me:

  1. Why are these localized strings
  2. Why hardcode the client to throw these errors instead of letting the backend serve the error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah tbh, I was following the pattern here 😅

It looks like the error message we get back from the server is essentially the same:

Screen Shot 2022-03-03 at 3 37 02 PM

I'm okay with simply letting the server error bubble up from here. Can delete a lot of code if we do that for all the payment methods

country: 'US',
},
},
// Edit the following to support different payment methods in your PaymentSheet
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting, looks like this is user-facing. I wonder why we aren't using the example server(s) the iOS and Android platforms use (you can see them here: https://glitch.com/@stripe-mobile-payments)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good question! I'm honestly not sure why this approach was opted for instead...on CI, we rely on our glitch server rather than the local one here. May have just been added before glitch was considered and is still around. Suppose we could probably get rid of this server altogether from the repo

@charliecruzan-stripe
Copy link
Collaborator Author

Created tickets for both of those points, merging this now though

@charliecruzan-stripe charliecruzan-stripe merged commit 7b7030f into master Mar 7, 2022
@charliecruzan-stripe charliecruzan-stripe deleted the charliecruzan/klarna-bindings branch March 7, 2022 14:43
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