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 duplicated tokens and tokens type mismatch #2926

Merged
merged 7 commits into from
Feb 23, 2024

Conversation

a-danae
Copy link
Contributor

@a-danae a-danae commented Feb 20, 2024

Fixes #2894

Changes proposed in this Pull Request:

  • Update the check for a mismatch in the token's ID and the payment method instance ID. We were comparing them against the wrong strings before.
  • When the token's ID and the payment method instance ID don't match, remove this token from the returned list. This mismatch happens for SEPA tokens created before Split PE (See [Split PE] Duplicated saved APM tokens when coming from non-PE #2894). We want to prevent them from being displayed while keeping them in the DB. (Or should we delete them from the DB?)
  • When creating tokens, update the value of $payment_method_instance to match the type of the payment method we're processing.

Testing instructions

Creating the tokens in an older version

  1. Check out develop
  2. Select EUR as the store currency
  3. Under the Stripe settings, enable UPE and enable SEPA
  4. As a shopper, go to the checkout page
  5. Select "Use a new payment method"
  6. Select SEPA
  7. Enter AT611904300234573201 and check off "Save payment information to my account for future purchases"
  8. Place the order
  9. Go to My Account > Payment methods
  10. Notice there's a token for the SEPA payment method, all good

Upgrading to 8.0.0

  1. Check out add/deferred-intent
  2. Reload the My Account > Payment methods page
  3. Confirm that there's a single token for SEPA, not two
  4. Go to the shortcode checkout page
  5. Confirm that the SEPA payment method only exists under the SEPA gateway, and not under Cards
  6. Reload the My Account > Payment methods page
  7. Confirm that no tokens got duplicated
  8. Access the site database > wp_woocommerce_payment_tokens > search for the tokens for the shopper you're using
  9. Confirm there's a single entry for the SEPA token. It must have a "stripe_sepa_debit" value under the "payment_gateway_id" column

Retrieving other APM tokens

  1. Buy a subscription using Bancontact, iDEAL, or Sofort
  2. Reload the My Account > Payment methods page
  3. Access the site database > wp_woocommerce_payment_tokens > search for the tokens for the shopper you're using
  4. Confirm a token was created, and that it has "stripe_bancontact" under "payment_gateway_id"
  5. Reload the My Account > Payment methods page
  6. Confirm the Bancontact token doesn't get duplicated

  • 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

@a-danae a-danae marked this pull request as ready for review February 21, 2024 00:49
@a-danae a-danae requested review from wjrosa and removed request for wjrosa February 21, 2024 00:50
@a-danae a-danae marked this pull request as draft February 21, 2024 17:19
@a-danae
Copy link
Contributor Author

a-danae commented Feb 21, 2024

@wjrosa, I'm making this back a draft for now. Gotta double-check the behavior with other APMs besides SEPA.

@wjrosa
Copy link
Contributor

wjrosa commented Feb 21, 2024

@wjrosa, I'm making this back a draft for now. Gotta double-check the behavior with other APMs besides SEPA.

Oh ok, I was testing it now (code is OK). I will wait for once it is ready again

Copy link
Contributor

@mattallan mattallan left a comment

Choose a reason for hiding this comment

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

Hey @a-danae, thanks for all your work on this PR! Loving the look of those changes in the woocommerce_get_customer_upe_payment_tokens() function.

I see this PR is still draft but I've been testing this PR and I can confirm the Bancontact, ideal, SEPA tokens are stored with stripe_{bancontact|ideal|sepa_debit} gateway ID on this branch 👍 however, when I save an APM card on develop i.e. with iDeal, the card is saved with gateway_id set to stripe, then when I switch to this branch and navigate to the checkout page, I'm seeing the tokens duplicated still:
image

@a-danae
Copy link
Contributor Author

a-danae commented Feb 23, 2024

Thanks so much for reviewing this, @mattallan !

however, when I save an APM card on develop i.e. with iDeal, the card is saved with gateway_id set to stripe, then when I switch to this branch and navigate to the checkout page, I'm seeing the tokens duplicated still:

Mind checking it again, if you can? This should be solved by 52c9db4

The original approach of keeping the old tokens was meant to allow people to roll back if needed. I decided to remove them now because the old tokens would get re-created by this method if the merchants rolled back. But glad to go for a different approach!

@a-danae a-danae marked this pull request as ready for review February 23, 2024 03:59
Copy link
Contributor

@mattallan mattallan left a comment

Choose a reason for hiding this comment

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

Nice one @a-danae!! I've tested the latest changes and confirmed my previous issues have been fixed!

I almost don't want to ask this, but what's the expected behaviour when a merchant toggles between UPE and legacy checkout? 🙉

In my testing I'm seeing ideal/bancontact tokens get duplicated/converted into stripe_sepa tokens when I turn off UPE and refresh the checkout page:
image

When I view the checkout page, it looks like ideal/bancontact doesn't list saved cards and doesn't allow saving cards anyway, so I actually think this is all working as intended, but I wanted to double-check with you. Enabling the UPE again all seems fine as well so I can't see any issues with this.

Approving this! 💯💃 Well done 👏

@a-danae
Copy link
Contributor Author

a-danae commented Feb 23, 2024

Biig thanks for checking this out again, @mattallan 🙌 Glad to hear the previous behavior is now working as expected 🎉

what's the expected behaviour when a merchant toggles between UPE and legacy checkout?
In my testing I'm seeing ideal/bancontact tokens get duplicated/converted into stripe_sepa tokens when I turn off UPE and refresh the checkout page

I certainly wouldn't expect them to be duplicated/converted. Great catch there btw!

I'm getting a different behavior on my end, though. Mind sharing the replication steps for yours?

This is what I see over here:

  1. Create tokens for iDeal, Bancontact, and SEPA
  2. Notice the tokens are created in the DB and displayed on the Payment methods page
    MwwSNS.png
    W3wvMY.png
  3. Disable UPE
  4. Reload the Payment methods page
  5. Notice the tokens aren't displayed on the Payment methods page. They remain the same in the DB
    nfmox4.png

I'll create an issue for this, but it's different and less critical from what you're getting. I'll continue diving there.

@a-danae a-danae merged commit 4592ab1 into add/deferred-intent Feb 23, 2024
32 checks passed
@a-danae a-danae deleted the fix/type-mismatch-creating-tokens branch February 23, 2024 16:59
@a-danae
Copy link
Contributor Author

a-danae commented Feb 24, 2024

Ooook, I think I'm getting the behavior you described. If this is it, looks like it's also happening in develop (phew). Would these be the replication steps?

  1. Check out develop
  2. Enable UPE, and APMs like iDEAL or Bancontact
  3. Create a token by checking out a saving the payment method
  4. In the DB, notice the tokens are created with stripe as the gateway ID
    f4YTVP.png
  5. Disable UPE
  6. Visit the checkout page
  7. Notice new tokens were created for the APMs, but with stripe_sepa as the value under gateway_id
    anLqOX.png

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