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

Handling saving of methods when SPE is enabled #4126

Open
wants to merge 43 commits into
base: develop
Choose a base branch
from

Conversation

wjrosa
Copy link
Contributor

@wjrosa wjrosa commented Mar 21, 2025

Fixes #4049

Changes proposed in this Pull Request:

This PR fixes the saving of payment methods when the Smart Checkout (Single Payment Element) is enabled. Due to this feature's different flow, I had to add multiple checks on our default flow, mostly updating the selected payment method (retrieving it from the Stripe payment method object instead of the POST value).

Testing instructions

  • Checkout and build this branch on your test environment (fix/handling-saving-of-methods-with-spe-enabled)
  • Connect your Stripe account
  • On your Stripe dashboard, enable multiple payment methods
  • Copy your child payment method configuration ID from here: https://dashboard.stripe.com/settings/payment_methods (labeled "Your configuration")
  • Replace pmc_... on all our frontend codes with the ID above. This is temporary until the parent configuration, labeled WooCommerce Inc., supports all methods by default
  • Build the frontend files (npm run build:webpack)
  • Enable the SPE feature flag (_wcstripe_feature_spe). You can do it by either hardcoding the return value of is_spe_available to true or by running npm run wp option update _wcstripe_feature_spe 'yes'
  • As a merchant, disable the legacy checkout experience in settings (wp-admin/admin.php?page=wc-settings&tab=checkout&section=stripe&panel=settings)
  • Enable the Smart Checkout feature
  • Enabled the saving of payment methods
  • I recommend setting your store currency to EUR so you can have more methods available
  • As a shopper, add any product to your cart
  • Perform purchases on both the classic and the block checkout, selecting the checkbox to save the payment method
  • Confirm the payment method is saved after completing the purchases
  • Confirm you can also save a payment method in the My Account page

  • Covered with tests (or have a good reason not to test in description ☝️)
  • Tested on mobile (or does not apply)

Changelog entry

  • This Pull Request does not require a changelog entry. (Comment required below)
Changelog Entry Comment

Comment

Post merge

@wjrosa wjrosa self-assigned this Mar 21, 2025
Base automatically changed from tweak/visual-changes-to-spe-in-classic-checkout to develop March 21, 2025 18:42
@@ -0,0 +1,14 @@
import { NON_REUSABLE_METHODS } from 'wcstripe/stripe-utils/constants';
Copy link
Contributor Author

@wjrosa wjrosa Mar 25, 2025

Choose a reason for hiding this comment

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

I will merge both files on the next PR so this one doesn't get too big. I will also add tests for this there.

}

$payment_method_object = $this->stripe_request( 'payment_methods/' . $payment_method_id );

$payment_method = $this->payment_methods[ $payment_method_type ];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was redundant.

Copy link
Contributor

@Mayisha Mayisha left a comment

Choose a reason for hiding this comment

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

Thanks @wjrosa for implementing this.

✅ Saving a card from My account page works.
✅ Saving a card during checkout in the shortcode checkout page works.
✅ Saving a card during checkout in the block checkout page works.
⚠️ For a logged out user, the save payment method checkbox is visible even when the create account checkbox is not selected.

I have left a few comments.
Apart from that I notices a few UI issues. I am listing them here but they could be unrelated to this PR.

  • It looks different when only Stripe is enabled, the radio button and title is not there.
Only Stripe enabled Other Payment gateways enabled
Screenshot 2025-03-26 at 1 38 34 PM Screenshot 2025-03-26 at 3 04 20 PM
  • The gap between the payment method name and icon looks a bit off.
Screenshot 2025-03-26 at 1 39 22 PM

@wjrosa
Copy link
Contributor Author

wjrosa commented Mar 26, 2025

Thanks for the great review, @Mayisha !

⚠️ For a logged out user, the save payment method checkbox is visible even when the create account checkbox is not selected.

Good catch! That applies only to the classic/shortcode checkout, right? Because, for me, the saving checkbox does not appear for logged-out users in the blocks checkout. I'm not sure if that's expected or if it is a bug. Anyway, I have fixed the issue for the classic checkout in c3a62ea

It looks different when only Stripe is enabled, the radio button and title is not there.

That was initially intentional, as part of the effort to make smart checkout payment options look like any other payment method. Still, since we must make the "Stripe" label configurable by merchants in #4114, it may be worth reverting that.

The gap between the payment method name and icon looks a bit off.

That's the Stripe container's default style and card icon. I am unsure we can replace that icon, which looks different from the others, but I will take a look.

Copy link
Contributor

@Mayisha Mayisha left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @wjrosa.

  • I am getting this console error for logged in user.
Screenshot 2025-03-27 at 11 47 09 AM
  • For a logged out user, the checkbox won't go away when create account is checked.
Screen.Recording.2025-03-27.at.11.52.06.AM.mov
  • This is unrelated to the PR but noting this as I noticed. The card testing instruction is always there irrespective of the selected method.
Screenshot 2025-03-27 at 11 44 27 AM
  • The radio button and label are still absent when Stripe is the only available gateway. For example, this is what I see if I only have COD and it has the label with radio button.
Screenshot 2025-03-26 at 4 03 46 PM

@wjrosa
Copy link
Contributor Author

wjrosa commented Mar 27, 2025

I am getting this console error for logged in user.

I am not able to reproduce this bug for some reason 🤔 . The previous logic already checked for the lack of the saving checkbox, but I have improved it in b32cdaf anyway adding a ? before the .checked property.

For a logged out user, the checkbox won't go away when create account is checked.

I thought that was the expected behavior: for logged-out users, only show the saving checkbox if "create account" is checked. Since if the account is not created, we won't be able to save the payment method. Isn't that correct?

This is unrelated to the PR but noting this as I noticed. The card testing instruction is always there irrespective of the selected method.

That is a known issue that will be handled in #4048

The radio button and label are still absent when Stripe is the only available gateway. For example, this is what I see if I only have COD and it has the label with radio button.

That's intentional. I will remove this behavior on another PR. I was going to remove it in #4114, but I can do it before.

@Mayisha
Copy link
Contributor

Mayisha commented Mar 27, 2025

I thought that was the expected behavior: for logged-out users, only show the saving checkbox if "create account" is checked. Since if the account is not created, we won't be able to save the payment method. Isn't that correct?

Sorry about not being clear. The save payment method checkbox should be present only when we select a method which allows saving a token, example - card. But in this case the checkbox is present even when I switch to Multibanco or iDeal which does not support payment method saving.

@wjrosa
Copy link
Contributor Author

wjrosa commented Mar 27, 2025

I thought that was the expected behavior: for logged-out users, only show the saving checkbox if "create account" is checked. Since if the account is not created, we won't be able to save the payment method. Isn't that correct?

Sorry about not being clear. The save payment method checkbox should be present only when we select a method which allows saving a token, example - card. But in this case the checkbox is present even when I switch to Multibanco or iDeal which does not support payment method saving.

Oh right! I was able to reproduce and fix it in e8b8d08 . The logic got reversed 🤦

@Mayisha
Copy link
Contributor

Mayisha commented Mar 30, 2025

Thanks @wjrosa.

✅ I do not see the console error anymore.
✅ Saving a payment method is working from My Account page
✅ Saving a payment method is working from the checkout page - both for logged in and logged out user (during account creation).
⚠️ The save payment method with create account checkbox combination is still a bit faulty.

I was able to reproduce and fix it in e8b8d08 . The logic got reversed 🤦

I think this did not completely fix the issue. To reproduce-

  • Enable EPS.
  • Go to the checkout page as a shopper.
  • Select EPS from the list.
  • Now select the Create account checkbox.
  • Notice that the save payment method checkbox is displayed (⚠️)
  • Now if you select the save payment method and try to checkout, you will get an error (⚠️) (getting the error is expected because EPS does not support tokenization)
Screenshot 2025-03-30 at 4 25 28 PM

If you would like to handle this edge case in a separate PR, I will approve this one as the main functionality implemented in the PR is working 👀

@wjrosa wjrosa closed this in #4153 Mar 31, 2025
@wjrosa wjrosa reopened this Mar 31, 2025
@wjrosa
Copy link
Contributor Author

wjrosa commented Mar 31, 2025

I think this did not completely fix the issue. To reproduce-

You're right. I properly fixed it in 7a8afd2. This was tricky, as I had to bind the listener to the create account checkbox once the element was mounted and every time a method was selected. I could not find a way to retrieve the current payment method chosen from the root JS files

Copy link

github-actions bot commented Apr 1, 2025

📈 PHP Unit Code Coverage Report

Package Line Rate Health
includes/class-wc-stripe-intent-controller.php 25%
includes/payment-methods/class-wc-stripe-upe-payment-gateway.php 48%
includes/payment-tokens/class-wc-stripe-payment-tokens.php 18%
Summary 44% (6985 / 15870)

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.

SPE: Correctly handle the saving of payment methods
2 participants