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

tweak: 'add payment method page' to honor WC rate limiter #3810

Merged
merged 7 commits into from
Feb 3, 2025

Conversation

frosso
Copy link
Contributor

@frosso frosso commented Jan 30, 2025

Fixes #3794

Changes proposed in this Pull Request:

Adding a similar check to the one made in WC Core, here: woocommerce/woocommerce@d252c8a/plugins/woocommerce/includes/class-wc-form-handler.php#L506-L510

Testing instructions

  • As a customer, navigate to the "My account > Payment methods" page
  • Ideally you shouldn't have any saved payment methods to make testing easier - but if you do, just take note of them and their expiry
  • Click on the "Add payment method" button
  • Save a new payment method with the 5555555555554444 card
  • You should land on the "My account > Payment methods" again, with a success message
  • Within 20 seconds:
    • Click on the "Add payment method" button, again
    • Attempt to save a new payment method with the 5200828282828210 card
    • You should see an error message
  • The Stripe Customer should not have any associated payment methods, either
  • Submit the form again with the same 5200828282828210 card, after the 20 seconds limit (you don't need to refresh the page)
  • The payment method should be added

  • 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

@frosso frosso marked this pull request as ready for review January 30, 2025 14:00
@frosso frosso requested review from a team and annemirasol and removed request for a team January 30, 2025 14:06
Copy link
Contributor

@annemirasol annemirasol left a comment

Choose a reason for hiding this comment

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

@frosso Thanks for working on this!

A few requests:

  1. Could you also add your logic to the main path create_and_confirm_setup_intent()? create_setup_intent() only gets hit by stores on legacy checkout mode.

    public function create_and_confirm_setup_intent( $payment_information ) {

    public function create_setup_intent() {

    • To disable legacy checkout, make sure this checkbox in Stripe settings is unchecked:
    Screenshot 2025-01-30 at 1 36 02 PM
  2. It would be great if we could still display an error explaining why the payment method failed to save -- in my testing I did not see one.

@frosso
Copy link
Contributor Author

frosso commented Jan 31, 2025

Could you also add your logic to the main path create_and_confirm_setup_intent()? create_setup_intent() only gets hit by stores on legacy checkout mode.

Thank you for pointing that out - I updated it in 7318eac

It would be great if we could still display an error explaining why the payment method failed to save -- in my testing I did not see one.

From what I see, any failure on the "My account > Add payment method" page is not displayed with WC Stripe.
For example, if you update this line to return an invalid nonce (e.g.: to return ''), the error message is not displayed to the customer.

This seems to be happening because dispatch( 'core/notices' ).createErrorNotice(...) (present on these lines) is not rendering any error notices on "My account > Payment methods > Add payment method".
The wc/checkout/payments context is only available at checkout, and not on "My account > Payment methods > Add payment method". "My account > Payment methods > Add payment method" doesn't use the block-based JS.

The check typeof wcSettings !== 'undefined' && wcSettings.wcBlocksConfig wrongly assumes to be in a block-based context if a block-based theme is being used.

If, instead, your merchant's site is using a non-block-based theme (e.g.: Storefront), the error is correctly displayed when the second attempt is made.

2025-01-31 16 05 50

It seems that this error message issue could be a bigger scope than this ticket and could necessitate a wider scope/fix - what do you think?

Copy link
Contributor

@annemirasol annemirasol left a comment

Choose a reason for hiding this comment

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

It seems that this error message issue could be a bigger scope than this ticket and could necessitate a wider scope/fix - what do you think?

Good point, it does sound like a separate, bigger issue. I will create a Github ticket for it, so someone can take a look.

Thanks for working on this again! I tested both legacy and new checkout, and rate limiting is working as expected.

@annemirasol
Copy link
Contributor

#3818 to document the error message and block-based theme issue.

@frosso frosso enabled auto-merge (squash) February 3, 2025 08:13
@frosso frosso merged commit 1d1f364 into develop Feb 3, 2025
37 checks passed
@frosso frosso deleted the fix/payment-method-add-rate-limiting branch February 3, 2025 08:19
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.

Ignored rate limiter in "Add payment method" page
2 participants