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

Fix duplicate token retrieval by using the correct payment method ID #4038

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

Conversation

ricardo
Copy link
Member

@ricardo ricardo commented Mar 11, 2025

This PR fixes an issue where duplicate payment tokens were being created for non-card payment methods due to an incorrect payment gateway ID being used. Previously, tokenization worked correctly only for card payments but failed for other reusable payment methods like ACH or ACSS (in progress).

Changes proposed in this Pull Request:

  • Ensure the correct payment gateway ID is used for token retrieval.
  • Prevent duplicate payment methods from being saved in "My Account" and during checkout.

Testing instructions

Test 1: Reproducing the issue (on develop)

  1. Switch to develop.
  2. Add a product to the cart and proceed to checkout.
  3. Select a reusable payment method other than card (e.g., ACH).
  4. Enable "Save payment information to my account for future purchases.".
  5. Complete the checkout.
  6. Repeat steps 2–5.
  7. Navigate to My Account > Payment Methods or return to checkout and observe that the payment method has been duplicated.
  8. Delete the duplicated payment methods from My Account.
  9. Add the payment methods again through My Account and observe that the duplication issue also happens from My account.

Test 2: Verify the Fix (on this branch)

  1. Switch to this branch.
  2. Repeat the same steps as Test 1 from checkout and from My Account.
  3. Observe that the payment method is no longer duplicated.

Test 3: Ensure No Regressions (Card payments)

  1. Switch to this branch.
  2. Add a product to the cart and proceed to checkout.
  3. Select the card payment method and enable "Save payment information to my account for future purchases.".
  4. Complete the checkout process.
  5. Repeat steps 2–4.
  6. Verify that the card payment method is stored correctly without duplication.

  • 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

@ricardo ricardo self-assigned this Mar 11, 2025
@ricardo ricardo requested review from a team and cesarcosta99 and removed request for a team March 11, 2025 15:51
@ricardo
Copy link
Member Author

ricardo commented Mar 11, 2025

Setting an estimate of 1 as this took me some time to troubleshoot.

@bborman22 bborman22 requested review from bborman22 and removed request for cesarcosta99 March 12, 2025 18:19
Copy link
Contributor

@bborman22 bborman22 left a comment

Choose a reason for hiding this comment

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

When I was testing this with ACH, it didn't work as expected so I dug in a bit more. What was happening was that the two updated calls to get_duplicate_token was using us_bank_account which wasn't finding any existing tokens. I looked into the tokens and their gateway ID is stripe_us_bank_account. I see we have a good reference for those IDs in the WC_Stripe_Payment_Tokens class as a const. There are a couple examples of that being used as a static, so I assume we can do the same here.

One other thing I'd note that is probably beyond the scope of this PR, but wanted your opinion since you're more familiar with this area now. I noticed we have a private WC_Stripe_Payment_Tokens::add_token_to_user. I was wondering if it would be useful to combine this logic into one place in the future? It seems there is very similar flows in at least three places when counting the two changes you made here.

@ricardo
Copy link
Member Author

ricardo commented Mar 14, 2025

When I was testing this with ACH, it didn't work as expected

@bborman22 sorry about that 🤦‍♂️. I had implemented and tested the fix with reference to $payment_method_instance->id, but just before opening this PR I looked for a getter method and found get_id(), however, as you found, it returns $stripe_id and not $id. I fixed this in 6445d1c.

Since we were using $this->id, I think it's fine to reference the public $id from the correct instance with $payment_method_instance->id.

I noticed we have a private WC_Stripe_Payment_Tokens::add_token_to_user. I was wondering if it would be useful to combine this logic into one place in the future?

Good call!

I agree we should try to combine the logic from add_payment_method and handle_saving_payment_method with something like WC_Stripe_Payment_Tokens::add_or_update_token.

Related to this issue we also have #4041 where add_token_to_user is currently getting called unnecessarily because the tokens in Stripe are still duplicated.

@ricardo ricardo requested a review from bborman22 March 14, 2025 20:21
Copy link
Contributor

@bborman22 bborman22 left a comment

Choose a reason for hiding this comment

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

This tested well for ACH now as the gateway ID look up was successful, but it did cause a regression with credit cards now, in that I can save the same CC multiple times. I believe this is again due to the gateway IDs not matching up. Currently CC gateway ID (in my testing at least) was just stripe while this new approach passes in the ID of stripe_card. This was with a newly saved CC payment method so I'm not sure if that's related? Is there any opportunity here to do a "refresh" on tokens and save them with stripe_card gateway ID? I'd hate to carve out an exception here for naming?

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.

2 participants