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 split PE support for APMs (eg GiroPay) with deferred intent on the classic Checkout #2827

Merged
merged 33 commits into from
Jan 18, 2024

Conversation

mattallan
Copy link
Contributor

@mattallan mattallan commented Jan 12, 2024

Fixes #2788

Changes proposed in this Pull Request:

This PR is based of #2820 and brings Split-PE with deferred intent support for our APMs (giropay, eps, iDeal) to the classic checkout

Screen.Capture.on.2024-01-12.at.14-42-03.mp4

The changes in this PR should be pretty straightforward, but here's a quick summary of what was needed to implement split PE on the classic checkout:

  1. 6161bbe 527959c removes all non-split code found in JS that we put in place to be able to test deferred intents.
  2. 6492c7a 2351511 74a5809 be433ad introduces some of the fundamental payment method functions that were missing from the WC_Stripe_UPE_Payment_Method class like payment_fields() to properly render each UPE payment method on the classic checkout page.
  3. 8871208 74a5809 updates the JS, mostly in classic/upe/ and stripe-utils/ to correctly navigate the multiple stripe elements now being mounted on the checkout page.

Open questions:

  • Should we remove "Pay with" from the titles of our UPE payment methods so that they don't show on the checkout as "Pay with EPS", and just have EPS?
  • Should we look at calling the main UPE class payment_fields() function inside our UPE payment methods, instead of creating/duplicate the existing function and putting it in the base WC_Stripe_UPE_Payment_Method class?

Testing instructions

Core checkout functionality:

  1. Checkout this branch and run npm run build:webpack
  2. Make sure you have a Stripe account that has access to test some of our APMs (giropay, iDeal etc)
  3. Go to WC > Settings > General and update your store's currency to Euros
  4. Go into the Stripe settings and make sure you've enabled multiple APMs:
    image
  5. Add a simple WC product to the cart and visit the classic checkout page.
  6. Confirm each individual gateway is now loading as a seperate gateway on the checkout:
    image
  7. Test purchasing a product with a regular card, and then repeat the above with another payment method like giropay.

Other tests:

  1. Confirm the save payment method checkbox is only loading when Enabled Saved Cards setting is enabled and on payment methods that are considered reusable.
  2. Change your currency to USD and confirm only the card payment method is available.

  • Covered with tests (or have a good reason not to test in description ☝️)
  • Added changelog entry in both changelog.txt and readme.txt (or does not apply)
  • Tested on mobile (or does not apply)

Post merge

james-allan and others added 30 commits January 4, 2024 15:37
…lass

- This function was copied over from the main UPE gateway class.
- This function calls other methods from the main UPE gateway class and so they've been copied over as well.
@mattallan mattallan marked this pull request as ready for review January 16, 2024 07:09
@mattallan mattallan requested review from a team and diegocurbelo and removed request for a team January 16, 2024 07:09
Copy link
Member

@diegocurbelo diegocurbelo left a comment

Choose a reason for hiding this comment

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

Code looks good and it tests great!

Should we remove "Pay with" from the titles of our UPE payment methods so that they don't show on the checkout as "Pay with EPS", and just have EPS?

This is a really good question @mattallan.
I think it's best to keep changes to a minimum.. the screenshot at https://woo.com/products/stripe/ does not have the Pay with so I think we should remove them in the classic checkout.
But I'm fine with merging this as it is, and discussing the naming in a P2 or slack, and then changing them 🤷🏼

Screenshot 2024-01-17 at 17 55 24

Should we look at calling the main UPE class payment_fields() function inside our UPE payment methods, instead of creating/duplicate the existing function and putting it in the base WC_Stripe_UPE_Payment_Method class?

Do you mean using WC_Stripe_UPE_Payment_Gateway::payment_fields() instead adding the function WC_Stripe_UPE_Payment_Method::payment_fields()?

I think it makes more sense for that rendering code to be in the WC_Stripe_UPE_Payment_Method class, right?

Base automatically changed from add/deferred-intent-split-apms-block-checkout to add/deferred-intent January 18, 2024 00:58
@mattallan
Copy link
Contributor Author

Thanks @diegocurbelo for the review!

I've just pushed up a small change to remove the description from the payment fields as I didn't realise this information was not intended for customers to see on the frontend and is for merchants to get more information about the payment method from the list of methods in the settings 😅

I replaced the description with gateway testing instructions if they exist.

Old New
image image

I think it's best to keep changes to a minimum.. the screenshot at https://woo.com/products/stripe/ does not have the Pay with so I think we should remove them in the classic checkout.
But I'm fine with merging this as it is, and discussing the naming in a P2 or slack, and then changing them 🤷🏼

That sounds good to me 👍

Do you mean using WC_Stripe_UPE_Payment_Gateway::payment_fields() instead adding the function WC_Stripe_UPE_Payment_Method::payment_fields()?

I think it makes more sense for that rendering code to be in the WC_Stripe_UPE_Payment_Method class, right?

Thanks! Yeah you were correct in your understanding. I agree that it makes sense to have the rendering code in the WC_Stripe_UPE_Payment_Method 🙂

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.

3 participants