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

Don't mark Stripe Link as enabled in the admin #3516

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

senadir
Copy link
Member

@senadir senadir commented Oct 16, 2024

Fixes #3356

Changes proposed in this Pull Request:

This PR resolves an issue in which Stripe Link would be marked as incompatible with Checkout block. To decide if a payment method is compatible or not, Checkout block would compare all registered gateways in woocommerce_payment_gateways and make sure they have a corresponding JS paymentGateway or expressPaymentGateway. Stripe Link alongside Google Pay and Apple Pay all use the same stripe express payment registry, however, in PHP, Stripe Link register its own gateway class WC_Stripe_UPE_Payment_Method_Link. This ends up marking it as incompatible.

There are several possible solutions to this, the least intrusive so far, is to mark WC_Stripe_UPE_Payment_Method_Link as disabled on all admin requests. This seems to work fine in admin and frontend, and doesn't break anything from what I saw.

Sadly changing $this->enabled broke certain unit tests that I couldn't figure out how to fix. I moved the logic to the main woocommerce_payment_gateways filter call.

This code might break any admin logic that depends on Stripe Link being marked enabled. I checked and couldn't find anything.

Another option was to limit disable to only Gutenberg pages, but it was too early to reach for get_current_screen.

Before: (those are before and after from a PR woocommerce/woocommerce#52070 but for this PR, both are considred before).
image
image

After:
image

Testing instructions

  1. Enable Link.
  2. Make sure your Checkout page is Checkout block.
  3. Go to editor, select the main Checkout block, make sure the sidebar doesn't have any warnings about an incompatible plugin.
  4. Do regression testing for Link, making sure it loads on both checkouts and you can place an order with it.

  • 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

@senadir senadir added type: bug The issue is a confirmed bug. component: blocks Cart and checkout blocks component: stripe link Issues related to Stripe Link labels Oct 16, 2024
@senadir senadir self-assigned this Oct 16, 2024
@senadir senadir requested review from a team and diegocurbelo and removed request for a team October 16, 2024 14:52
@senadir senadir force-pushed the fix/dont-mark-as-enabled-in-admin branch from f8f0aef to b06e32c Compare October 17, 2024 11:00
@senadir
Copy link
Member Author

senadir commented Oct 17, 2024

Updated logic so it removes Link from the main gateway filter instead of adding logic in the gateway class WC_Stripe_UPE_Payment_Method_Link, for some reason that kept failing no matter what value I put in enabled, even though it was not being reset manually, the test test_upe_method_enabled didn't propogate the value to Link when flipping. WC_Stripe_Helper::update_main_stripe_settings( $stripe_settings ); setting value was not making maybe. This is even with deleting the is_admin check, just simply doing $this->enabled = self::is_link_enabled() ? 'yes' : 'no'

The check here still only does is_admin because it's too early to check for get_current_screen. I think I can have another filter down the road that reaches directly to wc()->payment_gateways to remove Link. Let's see if this causes problems. All tests passed to me.

@annemirasol
Copy link
Contributor

@senadir maybe fixes #3356?

$methods = array_filter(
$methods,
function( $method ) {
return ! is_a( $method, 'WC_Stripe_UPE_Payment_Method_Link' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit-pick: I would use WC_Stripe_UPE_Payment_Method_Link::class instead, just for a more straightforward lookup and to avoid issues with class renaming.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

Copy link
Member

@diegocurbelo diegocurbelo 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 fixing this @senadir!

I could not find any side effects, all critical flows test well... (the legacy checkout E2E test is failing in guthub because of a timeout, it works fine locally.. and I also tested it manually)

@senadir senadir force-pushed the fix/dont-mark-as-enabled-in-admin branch from 48c7630 to 2606489 Compare October 24, 2024 12:37
@senadir senadir merged commit 8e8c559 into develop Oct 24, 2024
34 of 35 checks passed
@senadir senadir deleted the fix/dont-mark-as-enabled-in-admin branch October 24, 2024 13:05
@senadir
Copy link
Member Author

senadir commented Oct 25, 2024

I prematurely merged this before testing the refactor 😞 Pushing another PR to fix it. #3547

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: blocks Cart and checkout blocks component: stripe link Issues related to Stripe Link type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty incompatibility plugin registered with Checkout block
4 participants