search

Automattic / woocommerce-services

WooCommerce Services is a feature plugin that integrates hosted services into WooCommerce (3.0+), and currently includes automated tax rates and the ability to purchase and print USPS shipping labels.
GNU General Public License v2.0
105 stars 20 forks source link

Don't register functionality if both Woo Shipping and Woo Tax are active #2727

Closed waclawjacek closed 2 months ago

waclawjacek commented 3 months ago

Description

Related issue(s)

Fixes #2726.

Steps to reproduce & screenshots/GIFs

Screenshot 2024-04-04 at 11 36 08

Testing instructions

  1. Install & activate both Woo Shipping and WCS&T.
  2. See the notice in the screenshot.
  3. Go to Edit Order and see that WCS&T didn't register its functionality but Woo Shipping did.

The same should work for Woo Tax instead of Woo Shipping, as well as both Woo Shipping and Woo Tax, with an appropriate change in the message.

Checklist

waclawjacek commented 3 months ago

I'm not sure why the E2E tests are failing. I can run them locally. 🤔

waclawjacek commented 2 months ago

I'm not sure if we should disable WCS&T if they do not have Woo Tax installed?

Hmm, good point! WDYT, @laurajohnsonwoo, @kloon?

I suppose we could check what features a merchant has enabled and display different prompts based on that.

waclawjacek commented 2 months ago

I've updated the PR with the approach suggested by @kloon here: p1712745585197439/1712743452.598759-slack-C04KWSNPSE5

  • WCS&T and Woo Shipping enabled, then disabled Woo Shipping.
  • WCS&T and Woo Tax enabled, then disable Woo Tax.
  • WCS&T and Woo Tax and Woo Shipping enabled, then disable WCS&T.
waclawjacek commented 2 months ago

E2E tests continue to fail but not when trying locally:

npm run test:e2e-docker-up && npm run test:e2e

or

npm run test:e2e-docker-up && npm run test:e2e-dev
waclawjacek commented 2 months ago

@kallehauge Do you think you could review despite the E2E tests failing? The same happens in other PRs, e.g.: https://github.com/Automattic/woocommerce-services/actions/runs/8606963325/job/23586481611

Added an issue to fix E2E tests: https://github.com/Automattic/woocommerce-services/issues/2729

waclawjacek commented 2 months ago

Why is jetpack_on_plugins_loaded running in an earlier priority?

Unfortunately, there is this restriction: https://github.com/Automattic/woocommerce-payments/pull/637/files#diff-ad243b7f0e0ae2cc54963fec106a5c54ece9c5bb8b272acaeece1836d71418a2R36

After reading your (very valid) comment, I have considered loading the "coexistence" stuff with a negative priority (hey, WP docs say "integer") but that'd mean loading our stuff before WP core has done some of the stuff that it registers with priority 0 itself.

But anyway, we don't need to run this check during plugins_loaded. I'll post more info in my response to this comment: https://github.com/Automattic/woocommerce-services/pull/2727#discussion_r1562739028

waclawjacek commented 2 months ago

@kallehauge I've updated the code to not register the Jetpack config unless needed. Could you please re-review this PR?