-
Notifications
You must be signed in to change notification settings - Fork 68
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
Introduce Klarna PM #7282
Introduce Klarna PM #7282
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
Size Change: +697 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did quick-check, leaving nit-picks and questions. But overall looks straightforward and clean!
Starting from 6.4.0 version deferred Split UPE is the default UPE type but we haven't implemented the logic to restrict payment method to specific countries in it. I've created a related bug #7313 that needs to be addressed as part of Klarna launch but to keep things separate, I'll tackle that as separate task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes read well and I manually tested and was able to complete the checkout with Klarna. I noticed a couple of things:
- Since we still need to include Klarna assets, we can take the chance to add Klarna logo in the banner that appears when UPE is not enabled in the payment settings. I checked with designers in the slack thread: p1695896798254629/1695318697.479009-slack-C04VCT34PCP
- Noticed an error when the Legacy UPE is enabled and the shopper does not fill the phone number. I created this ticket Improve Klarna phone number validation on checkout #7319 to look into it.
Did testing and the success path worked well with block checkout. Will do another round next week. |
As far as happy path testing, I've tested from different merchant countries and currencies for all (flavors of UPE) x blocks/classic. As for unhappy path, the biggest problem is that we always show generic error message upon redirecting back from provider page. I'll create a ticket for that as it impacts not only Klarna and should be tackled separately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a new manual testing round of happy path with shortcode based checkout + Legacy and Deferred UPE. Works as expected (I'm using US based merchant account for my tests).
But I noticed that the Klarna icon is missing in the Source column of the transaction list and also not showing in payment details page:
@KarlisJ @dmvrtx are we adding those icons in a separate PR?
@mgascam The asset is there. The problem seems that the payment method is not recognised in those views. I'll investigate, thanks for finding this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provisionally approved, as comments are non-blocking.
isLoading={ isLoading } | ||
label={ __( 'Category', 'woocommerce-payments' ) } | ||
> | ||
{ paymentMethodCategory } | ||
</Detail> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice touch! I wonder if we should add mapping somewhere to show possible values of pay_later
, pay_now
, pay_with_financing
, or pay_in_installments
as a readable/translatable text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably TS it. But I'm not sure about translating them. At the end of the day, those are codes not messages. If merchant tries to figure out what it means, and it's translated, then it will be hard to Google it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stripe translates those in their dashboard, I don't think we should hesitate to do the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving some comments after reviewing the payment method details section (great catch!)
isLoading={ isLoading } | ||
label={ __( 'Category', 'woocommerce-payments' ) } | ||
> | ||
{ paymentMethodCategory } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should translate the paymentMethodCategory? According to Stripe docs this can be pay_later
, pay_now
, pay_with_financing
, pay_in_installments
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #7282 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I left a couple of minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something needs to be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (left a couple of comments about comments tweaks).
Fixes #7243
Changes proposed in this Pull Request
Introduces Klarna payment method.
Testing instructions
payments
->settings
and enable Klarna PMnpm run changelog
to add a changelog file, choosepatch
to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge