search

Automattic / woocommerce-payments

Accept payments via credit card. Manage transactions within WordPress.
https://wordpress.org/plugins/woocommerce-payments/
Other
173 stars 69 forks source link

SPIKE: Split UPE <> WC Payments as a platform changes integration #5554

Closed frosso closed 1 year ago

frosso commented 1 year ago

Description

For background information: paJDYF-6ZK-p2

Test the changes between the Split UPE and the WooCommerce Payments as a Platform project, ensuring that there are no major surprises.

timur27 commented 1 year ago

Currently waiting for the latest conflicts to be resolved: p1676889735372369-slack-CGGCLBN58. I will start the testing right afterward.

timur27 commented 1 year ago

The changes from WCPay as a Platform phase 1 mainly touched the flows of payment/setup intent creation of the split UPE project. There was also a small change that caused a conflict and was resolved - testMode property, that is provided to JS scripts. Below are the results of my testing round.

UPE gateway

:one: Creating a setup intent now uses the new Create_And_Confirm_Setup_Intention::create(), whereas $this->payments_api_client->create_setup_intention was used before the WCPay as a Platform.

:two: Creating a payment intent now uses Create_Intention::create().

Split UPE gateway

:one: Creating setup intent works well for CC - I went to the My account and successfully added a new payment method. ✅ :two: Creating a payment intent The page has loaded without errors on the UI with the create_payment_intent method. I was able to proceed with the checkout by using a credit card. ✅ I was also able to checkout with 3-4 different payment methods (giropay, Sofort, P24 and Bancontact) and the process went well. ✅

Non-UPE

:one: Setup intent - saved a new card on My account page successfully ✅ :two: Paid with the CC successfully ✅

Other changes

:one: testMode property, which is supplied for the UPE js scripts, is now provided by WC_Payments::mode()->is_test(); instead of $this->gateway->is_in_test_mode();. It looks good since I can see the test message. ✅

More details on the error that was catched:

  • When using any payment method other than the CC and authorizing it as a third party, I get the following error: py_3MhAQvR3FtIp0bJN1nbngyUx is not a valid Stripe identifier . If trying to pay once more with the same method, the checkout succeeds but it doesn’t even redirect to the test payment authorization page ❌

image.png

I am able to constantly reproduce it on develop, but at the same time, I can't reproduce it on trunk. I reverted WCPay as Platform commit on my local develop and the problem disappeared as well, but I'd like someone to confirm this, as my local became quite messy at that point due to some conflicts.

@FangedParakeet @mdmoore this issue is no-code so there is no PR, but I'd like to ask someone of you to review my report above, try to reproduce the error I caught and perform another round of testing on your side to make sure there are no more issues.

FangedParakeet commented 1 year ago

When using any payment method other than the CC and authorizing it as a third party, I get the following error: py_3MhAQvR3FtIp0bJN1nbngyUx is not a valid Stripe identifier . If trying to pay once more with the same method, the checkout succeeds but it doesn’t even redirect to the test payment authorization page ❌

I wasn't able to replicate this, be that fortunate or unfortunate. I had a pretty comprehensive read through the PR diff and after double-checking the code, I couldn't really find anything greatly out of place. There hasn't been too much that has been moved around inexplicably and it does appear to me to be a pretty honest refactor.

The code-changes looked pretty good to me, incorporated our split UPE changes, and after running through the usual checkout flows using additional UPE payment methods in both shortcode and blocks I wasn't able to break anything!

@timur27, can you give me more details on the error message you found? I'm not greatly familiar with that error message at all, so can't really diagnose on sight alone where it's coming from. I was just testing both the legacy and split UPE gateways on develop as well, so it's possible some newer change resolved the issues you were facing. Were you using a specific type of product, payment method, or checkout? Do let me know if I might be doing something improper in my own testing! @mdmoore, have you had any time to take a look at this? Perhaps you could be the tie-breaking vote in deciding whether there are any concerning issues with this WCPay refactor.

timur27 commented 1 year ago

Thanks, @FangedParakeet, for your tests! I was able to reproduce it once again on develop with the latest commit from today with a basic Beanie, below is the screen recording:

https://images.zenhubusercontent.com/173384525/5e1a8a93-5516-45f7-940a-7b54bdf85e47/legacy_upe_checkout.mp4

The error happens when getting a charge after the payment has been authorized and the error is thrown here.

It seems that the payment intent doesn't pass the pattern match here, as the prefix used for charges, is ch and not pi.

I didn't go deeper into the root cause and fix since I'd like to ensure it's a reproducible problem or it's just me who has some wrong assumption or broken setup. I hope this helps, and I agree with you that asking Mike to perform the test on his side would allow us to confirm or deny some of the theories here.

FangedParakeet commented 1 year ago

Had to poke my nose around a few places, but I eventually found out that it seems GlobalStep encountered this issue as well in #5341. There's also a PR to fix it at #5358 that appears to have been approved and then abandoned. I think this is the relevant change that possibly fixes this problem.

@timur27, can you try this out and let me know if it resolves the issue for you? If so, we can probably look to implement this change ourselves, if the aforementioned PR is indeed frozen in place.

timur27 commented 1 year ago

@FangedParakeet, thanks, I think we now have more data.

To reproduce the issue, one needs to ensure that the store and account currencies are different to cause the conversion of funds. In my case, I've had EUR in My account -> Account details -> Default currency vs USD in _wpadmin -> WooCommerce -> Settings -> General -> Currency and perform an attempt to checkout with any of the UPE gateways.

And yes, the fix helps.

Hey @RadoslavGeorgiev 👋🏼 Are there any plans to merge this PR? To be more precise, these|two commits. When testing WCPay as a Platform <-> split UPE integration, we were able to reproduce this issue. We can also raise a separate PR with this fix, but I wanted to confirm with you first if there were any reasons not to merge this PR yet, which we might be unaware of.

RadoslavGeorgiev commented 1 year ago

Hey @timur27 and @FangedParakeet 👋

Sorry for wasting your time with this. I opened the PR right before our meetup in January, and it was somehow forgotten. I'll fix the PR and merge it shortly.

It will fix the issue described in this comment, and my testing instructions actually explain why @FangedParakeet was not able to reproduce it initially. Makes you wonder the issue was not reported again for more than two months by GS though :\

timur27 commented 1 year ago

@RadoslavGeorgiev Many thanks for such a quick response! It's great we are aligned now. The main reason I contacted you was to look for potential blockers, which could be why the PR was not merged.

That said, don't hesitate to contact us if any input from our side is needed - we're happy to help.

RadoslavGeorgiev commented 1 year ago

Since you mentioned it, would you mind reviewing https://github.com/Automattic/woocommerce-payments/pull/5358? It's rebased on top of develop already, and another 🟢 would be welcome.

FWIW only now I realize that you had linked this issue in Slack when you started that thread, but I assumed it's here only to track effort. If you are testing in the context of another team's work, please don't hesitate to ping us in the description or some of the comments within the issue, so that we'd get updates. I would have noticed the root cause for the issue five days ago :)

timur27 commented 1 year ago

Another thing that was found and fixed (https://github.com/Automattic/woocommerce-payments/pull/5710) pretty quickly is the WooPay compatibility, which is not directly related to the split UPE but was encountered while managing the state of WCPay with the UPE feature flags.

I will perform another round of testing, and if I recall correctly, @FangedParakeet has such plans as well (but please let me know in case I misunderstood anything).

FangedParakeet commented 1 year ago

I quickly ran through the usual testing flows for with the legacy, UPE, and split UPE checkouts enabled and wasn't able to uncover anything unusual on develop this time. @timur27, let me know if you are still having any issues, after the latest fixes have been merged--or if there's a particular area I should focus my testing on, if I'm missing anything. Cheers!

timur27 commented 1 year ago

I also ran through the usual testing flow and haven't found any error explicitly related to the WCPay as a Platform <-> split UPE work. However, another issue was found on the Pay for order page. It's not related to WCPay as a platform, hence I will be closing this issue.