refactor: remove jetpack dependency #2439

frosso · 2021-06-30T03:38:46Z

Description

Replace the Jetpack plugin dependency with usage of the Jetpack Connection package.

Related issue(s)

Steps to reproduce & screenshots/GIFs

(optional) connect to your sandbox & apply D63626-code to see a modified jetpack DNA connection UI (rather than the default one) - this also ensures that the JP connection doesn't land on a 404 page upon successfull connection

Before:

After:

Ensure you run composer install
You can demo this locally or on JN by running npm run build and uploading the package to your JN installation
Tested scenarios:
- Site with older version of Jetpack, then this is installed
- Site with older WCS & Jetpack connected, then upgrade to this => no need to connect
- Site with older WCS & Jetpack connected, then upgrade to this, then uninstall Jetpack => merchant is asked to connect site & plugin to Jetpack PLEASE NOTE: you'll need D63626-code
- Site with older WCS & Jetpack connected, then upgrade to this, then disconnect & uninstall Jetpack => merchant is asked to connect site to Jetpack PLEASE NOTE: you'll need D63626-code
- Brand new site with WCPay installed and connected, then merchant installs this => no need to connect to Jetpack
- Brand new site with this installed and connected, then WCPay is installed => merchant skips Jetpack connection, goes to Stripe onboarding
- Brand new site without Jetpack, then install this => merchant is asked to connect site to Jetpack
Untested scenarios:
- Brand new site, merchant installs WC and on onboarding also opt-in to install WCS&T
  - Should we modify WC onboarding to no longer require Jetpack?
    - The modification of the onboarding flows is happening in Remove dependency on Jetpack from Shipping and Tax task list items woocommerce/woocommerce#39992 and is working when locally building both plugins and then installing on a test site. See: paJDVv-2JR-p2
- Atomic sites (shouldn't be an issue with the connection, the atomic site checks are just used to send data to the connect server)

Checklist

unit tests

changelog.txt entry added

frosso · 2021-06-30T03:41:15Z

classes/class-wc-connect-api-client.php

@@ -466,7 +466,7 @@ protected function request_body( $initial_body = array() ) {
 					'dimension_unit'       => strtolower( get_option( 'woocommerce_dimension_unit' ) ),
 					'weight_unit'          => strtolower( get_option( 'woocommerce_weight_unit' ) ),
 					'wcs_version'          => WC_Connect_Loader::get_wcs_version(),
-					'jetpack_version'      => JETPACK__VERSION,
+					'jetpack_version'      => defined( 'JETPACK__VERSION' ) ? JETPACK__VERSION : 'embed',


It doesn't look like we're doing anything on the server with this string. However, the server is still requiring it in the payload.

I think it was only ever used for troubleshooting. If it's easy to get the version of the jetpack-connection package I would include it, otherwise embed is fine.

Yep, just checked and we are not using it anywhere aside from Tracks calls. We require it to be a string if it is present.

I've changed this from embed to embed-{jetpack_connection_package_version}.

frosso · 2021-06-30T03:42:55Z

classes/class-wc-connect-api-client.php

@@ -529,7 +529,7 @@ protected function authorization_header() {
 			}

 			list( $token_key, $token_secret ) = explode( '.', $token->secret );
-			$token_key                        = sprintf( '%s:%d:%d', $token_key, JETPACK__API_VERSION, $token->external_user_id );
+			$token_key                        = sprintf( '%s:%d:%d', $token_key, defined( 'JETPACK__API_VERSION' ) ? JETPACK__API_VERSION : 'embed', $token->external_user_id );


The server seems ok with this 🤷

The format is %s:%d:%d so the second and third placeholders should be numbers (i.e. can't be "embed"). Testing on my local, it looks like the code below returns 1 anyway so I suggest we hardcode it to that, and in the future use Jetpack's header building function instead of our own, if possible.

Constants::get_constant( 'JETPACK__API_VERSION' )

frosso · 2021-06-30T03:43:04Z

classes/class-wc-connect-debug-tools.php

@@ -28,7 +28,7 @@ function woocommerce_debug_tools( $tools ) {
 		function test_connection() {
 			$test_request = $this->api_client->auth_test();
 			if ( $test_request && ! is_wp_error( $test_request ) && $test_request->authorized ) {
-				echo '<div class="updated inline"><p>' . __( 'Your site is succesfully communicating to the WooCommerce Shipping & Tax API.', 'woocommerce-services' ) . '</p></div>';
+				echo '<div class="updated inline"><p>' . __( 'Your site is successfully communicating to the WooCommerce Shipping & Tax API.', 'woocommerce-services' ) . '</p></div>';


frosso · 2021-06-30T03:43:24Z

classes/class-wc-connect-help-view.php

@@ -102,14 +85,19 @@ protected function get_health_items() {
 					'state'   => 'warning',
 					'message' => __( 'This is a Jetpack staging site', 'woocommerce-services' ),
 				);
-			} else {
+			} elseif ( defined( 'JETPACK__VERSION' ) ) {


Does this really matter? IDK

I'm going to say no.

frosso · 2021-06-30T04:02:17Z

woocommerce-services.php

@@ -708,7 +722,6 @@ public function load_dependencies() {
 			$shipping_label        = new WC_Connect_Shipping_Label( $api_client, $settings_store, $schemas_store, $payment_methods_store );
 			$nux                   = new WC_Connect_Nux( $tracks, $shipping_label );
 			$taxjar                = new WC_Connect_TaxJar_Integration( $api_client, $taxes_logger, $this->wc_connect_base_url );
-			$options               = new WC_Connect_Options();


There's no constructor in this class

frosso · 2021-06-30T04:03:13Z

woocommerce-services.php

 	define( 'WOOCOMMERCE_CONNECT_MAX_JSON_DECODE_DEPTH', 32 );

 	if ( ! defined( 'WOOCOMMERCE_CONNECT_SERVER_API_VERSION ' ) ) {
 		define( 'WOOCOMMERCE_CONNECT_SERVER_API_VERSION', '5' );
 	}

-	// Check for CI environment variable to trigger test mode.
-	if ( false !== getenv( 'WOOCOMMERCE_SERVICES_CI_TEST_MODE' ) ) {


When WOOCOMMERCE_SERVICES_CI_TEST_MODE is defined and !false, the other constants are already defined as true - this simplifies things a bit

frosso · 2021-06-30T04:03:23Z

woocommerce-services.php

 require_once __DIR__ . '/classes/class-wc-connect-extension-compatibility.php';
 require_once __DIR__ . '/classes/class-wc-connect-functions.php';
 require_once __DIR__ . '/classes/class-wc-connect-jetpack.php';
 require_once __DIR__ . '/classes/class-wc-connect-options.php';

 if ( ! class_exists( 'WC_Connect_Loader' ) ) {
 	define( 'WOOCOMMERCE_CONNECT_MINIMUM_WOOCOMMERCE_VERSION', '2.6' );
-	define( 'WOOCOMMERCE_CONNECT_MINIMUM_JETPACK_VERSION', '7.5' );


Not needed anymore

frosso · 2021-06-30T04:03:32Z

tasks/release.js

@@ -15,6 +15,7 @@ const dirsToCopy = [
 	'dist',
 	'i18n',
 	'images',
+	'vendor',


Used to release - maybe we could just include the non-dev packages in the release?

Absolutely. The dev packages add a lot of bloat. Here's how we do it in WCPay, we make sure that the task that's run when we're preparing the release executes composer install --no-dev.

Done in 573a363

frosso · 2021-06-30T18:11:06Z

composer.json

@@ -13,5 +13,10 @@
 		"phpcbf": [
 			"phpcbf -p"
 		]
+	},
+    "require": {
+		"automattic/jetpack-connection": "1.20.0",


These are not the latest versions.
The latest version of the JP packages has minimum compatibility with WP 5.5 - but we're still testing for 5.4

Automattic/jetpack#17820

frosso · 2021-07-01T22:07:20Z

woocommerce-services.php

 		public function __construct() {
 			$this->wc_connect_base_url = self::get_wc_connect_base_url();
+			add_action( 'plugins_loaded', array( $this, 'jetpack_on_plugins_loaded' ), 1 );


It's important that this code is executed with priority 1 - I banged my head for a while trying to figure out why it wasn't working before 😄

Context: https://github.com/Automattic/woocommerce-payments/pull/637/files#diff-ad243b7f0e0ae2cc54963fec106a5c54ece9c5bb8b272acaeece1836d71418a2R36

frosso · 2021-07-01T22:09:31Z

classes/class-wc-connect-tracks.php

@@ -66,10 +66,6 @@ public function saved_service_settings( $service_id ) {
 		}

 		public function record_user_event( $event_type, $data = array() ) {
-			if ( ! function_exists( 'jetpack_tracks_record_event' ) && ! class_exists( 'Automattic\\Jetpack\\Tracking' ) ) {


Not used anymore - Automattic\Jetpack\Tracking will always be defined

frosso · 2021-07-01T22:32:01Z

.github/workflows/e2e_tests.yml

@@ -13,9 +13,16 @@ jobs:
      - uses: actions/checkout@v2
        with:
          submodules: recursive
+      - uses: shivammathur/setup-php@v2


The plugin now has composer dependencies for releasing - we need them when executing the tests

frosso · 2021-07-02T13:14:22Z

classes/class-wc-connect-help-view.php

 			$is_connected = WC_Connect_Jetpack::is_active() || WC_Connect_Jetpack::is_development_mode();
-			if ( ! is_plugin_active( 'jetpack/jetpack.php' ) ) {


No need for all this branching logic anymore - checking whether jetpack is installed or not won't matter, we are now using the composer packages directly.

frosso · 2021-07-02T13:22:51Z

classes/class-wc-connect-nux.php

 		 */
-		public function ajax_activate_jetpack() {


No need to activate jetpack anymore

changelog.txt

waclawjacek · 2023-10-19T08:35:09Z

classes/class-wc-connect-api-client-live.php

@@ -17,20 +17,6 @@ class WC_Connect_API_Client_Live extends WC_Connect_API_Client {
 		protected function request( $method, $path, $body = array() ) {

 			// TODO - incorporate caching for repeated identical requests
-			if ( ! class_exists( '\Automattic\Jetpack\Connection\Manager' ) && ! class_exists( '\Automattic\Jetpack\Connection\Tokens' ) ) {


These classes are always present because we bundle Jetpack Connection with WCS&T now.

This reverts commit 2104872.

waclawjacek · 2023-10-19T10:37:44Z

classes/class-wc-connect-nux.php

-		 *
-		 * @return bool
-		 */
-		public function can_accept_tos() {


Since the logic now boils down to "is offline mode or is current user connected", I am inlining this logic in set_up_nux_notices() instead.

This reverts commit 9f319c4.

harriswong

I have some questions but LGTM otherwise.

I tested buying and printing a USPS label, works.

I tested 2 nux flows by hacking around this condition:


			$banner_to_display = self::get_banner_type_to_display(
				array(
					'jetpack_connection_status'       => $jetpack_install_status,
					'tos_accepted'                    => WC_Connect_Options::get_option( 'tos_accepted' ),
					'can_accept_tos'                  => WC_Connect_Jetpack::is_current_user_connection_owner() || WC_Connect_Jetpack::is_offline_mode(),
					'should_display_after_cxn_banner' => WC_Connect_Options::get_option( self::SHOULD_SHOW_AFTER_CXN_BANNER ),
				)
			);

The ToS one looks good and I traced the code a bit, looks good to me.

The second nux flow gave me an error:

client/banner.js

classes/class-wc-connect-tracks.php

waclawjacek · 2023-10-23T23:22:26Z

@harriswong Thank you so much for the review! It's hard to say why exactly you got the error. Is there any chance of getting a list of steps to reproduce to achieve that result? It might be something not caused by this PR.

harriswong · 2023-10-24T16:10:30Z

I think it could be me setting the following:

array(
					'jetpack_connection_status'       => 'not-connected',
					'tos_accepted'                    => false, //WC_Connect_Options::get_option( 'tos_accepted' ),
					'can_accept_tos'                  => false, //WC_Connect_Jetpack::is_current_user_connection_owner() || WC_Connect_Jetpack::is_offline_mode(),
					'should_display_after_cxn_banner' => WC_Connect_Options::get_option( self::SHOULD_SHOW_AFTER_CXN_BANNER ),
				)

We can upload this plugin to a brand new site, and if that is fine, then we should be good! Have you tested that already? If so, I think we are good.

harriswong · 2023-10-24T16:58:54Z

👋 @waclawjacek For sift, I added these 2 lines:
https://github.com/Automattic/woocommerce-services/blob/trunk/woocommerce-services.php#L1605
https://github.com/Automattic/woocommerce-services/blob/trunk/woocommerce-services.php#L1610

I think we will have to rename these. My recent changelog also conflicted with your PR. Let me know if you want me to fix these! Thanks.

waclawjacek · 2023-10-25T11:59:08Z

👋 @waclawjacek For sift, I added these 2 lines: trunk/woocommerce-services.php#L1605 trunk/woocommerce-services.php#L1610

I think we will have to rename these. My recent changelog also conflicted with your PR. Let me know if you want me to fix these! Thanks.

Thank you! Great catch. I've just fixed these.

I think it could be me setting the following:

array(
					'jetpack_connection_status'       => 'not-connected',
					'tos_accepted'                    => false, //WC_Connect_Options::get_option( 'tos_accepted' ),
					'can_accept_tos'                  => false, //WC_Connect_Jetpack::is_current_user_connection_owner() || WC_Connect_Jetpack::is_offline_mode(),
					'should_display_after_cxn_banner' => WC_Connect_Options::get_option( self::SHOULD_SHOW_AFTER_CXN_BANNER ),
				)

I don't think that's it. Here's the line of code reporting that error that I found in OpenGrok: fbhepr%2Skers%2Sjcpbz%2Sjc%2Qpbagrag%2Szh%2Qcyhtvaf%2Swrgcnpx%2Spynff.wrgcnpx%2Qfreire%2Qirefvba.cuc%3Se%3Qp86809os%26zb%3Q43279%26sv%3Q1292%231292-og I've just tried hardcoding the same arguments that you tried locally and I was able to connect my site (with the exception of having to remove this later to get the NUX banner to disappear :)). Could you please retry this on a fresh site?

We can upload this plugin to a brand new site, and if that is fine, then we should be good! Have you tested that already? If so, I think we are good.

Yep! In so many test cases. 😄 Also just tested now. I'm attaching a build of the latest commit in this PR if you'd like to test on JN:

woocommerce-services-702256c7de757357fd36b93c040406a383502e69.zip

harriswong · 2023-10-25T22:46:45Z

Tested on JN and there is no error, woot!

I have one question. After clicking the image

It bounces me to Jetpack page, is this expected? I was thinking the authentication should just connects the site but not bring to this page.

waclawjacek · 2023-10-26T07:16:24Z

Thanks for checking! Did you apply the sandbox patch mentioned in the original post? Without it, you go through the old auth flows instead.

waclawjacek · 2023-10-26T08:43:01Z

We've just merged the WPCOM changes so you can now test without sandboxing. Could you please give it a try?

harriswong · 2023-10-26T21:15:03Z

Hi @waclawjacek, I gave this another test
🧪 With staging
✅ print USPS label + refund using staging, reprinting
✅ international shipping DHL + refund. Reprinting + reprinting custom form
✅ First onboarding nux flow
✅ Saving settings, credit card updates
✅ Refreshing connect server statuses

🧪 Without staging, using a new build with the latest commit(1d90a66)
✅ Installing WCS&T
✅ Activating and the NUX onboarding
✅ Tested creating an order, all the way to printing a label but without actually buying it with a real credit card.

🧪 I didn't test a fresh local dev setup env.

🧪 Tested locally
✅ Pulled this branch, npm run start still works with Jetpack installed.

frosso added 3 commits June 29, 2021 22:38

POC: remove jetpack dependency

210c2bf

WIP

a903c8a

WIP

c3ff595

frosso commented Jun 30, 2021

View reviewed changes

frosso added 6 commits June 29, 2021 23:06

WIP

072b365

older version to fix some little things

d909cd9

WIP

a41901d

WIP

ec39d7c

WIP

20bfdc0

WIP

3ccc8de

frosso commented Jun 30, 2021

View reviewed changes

frosso and others added 6 commits June 30, 2021 13:12

unused stuff

d4dfb15

WIP

68a61ee

all working, no touchy

314db9c

Merge branch 'trunk' into frosso/jetpack-dependency-removal

e17b5ee

WIP

e5e377f

WIP

97cffb2

frosso commented Jul 1, 2021

View reviewed changes

frosso and others added 7 commits July 1, 2021 17:40

Update e2e_tests.yml

4aef84a

Update e2e_tests.yml

9e94de0

Update e2e_tests.yml

5d1026f

WIP

d1234f3

Update e2e_tests.yml

5293e86

WIP

86a8356

WIP

5ed5d2e

frosso commented Jul 2, 2021

View reviewed changes

classes/class-wc-connect-nux.php

*/

public function ajax_activate_jetpack() {

Copy link

Contributor Author

frosso Jul 2, 2021

Choose a reason for hiding this comment

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

No need to activate jetpack anymore

diff cleanup

09a23cc

frosso marked this pull request as ready for review July 2, 2021 14:45

Composer: Move Jetpack packages to "require" section

e8ed1d2

waclawjacek reviewed Oct 19, 2023

View reviewed changes

changelog.txt Outdated Show resolved Hide resolved

waclawjacek added 2 commits October 19, 2023 10:02

Update changelog.txt

e5a96d3

E2E: Give container more time to build

2104872

waclawjacek reviewed Oct 19, 2023

View reviewed changes

waclawjacek added 3 commits October 19, 2023 10:48

Revert "E2E: Give container more time to build"

cf22022

This reverts commit 2104872.

E2E: Add composer install

4140263

Restore ability to accept TOS in offline mode

07b003e

waclawjacek reviewed Oct 19, 2023

View reviewed changes

Revert "Tests: Update NUX tests to the "no banners when offline" logic"

9433d62

This reverts commit 9f319c4.

harriswong reviewed Oct 23, 2023

View reviewed changes

client/banner.js Show resolved Hide resolved

classes/class-wc-connect-tracks.php Show resolved Hide resolved

waclawjacek mentioned this pull request Oct 23, 2023

Add notice about disconnection when uninstalling Jetpack #2690

Merged

waclawjacek added 3 commits October 25, 2023 13:12

Merge branch 'trunk' into frosso/jetpack-dependency-removal

1d462d3

Update Sift tracker rendering to use new WC_Connect_Jetpack methods

627283a

Silence QIT warning about wp_redirect

702256c

waclawjacek and others added 2 commits October 26, 2023 18:36

Merge branch 'trunk' into frosso/jetpack-dependency-removal

5f6fc1a

Update wporg assets, details and installation steps

1d90a66

waclawjacek merged commit 64cf6eb into trunk Oct 27, 2023
14 checks passed

waclawjacek deleted the frosso/jetpack-dependency-removal branch October 27, 2023 07:46

This was referenced Nov 2, 2023

Add automated tests for Jetpack Connection flows #2695

Open

"Install Jetpack and connect" button does not work on WC Orders page #2668

Closed

sergeymitr mentioned this pull request Dec 6, 2023

When user disconnects Jetpack, WooCommerce Shipping & Tax is not listed as one of the plugins that will stop working Automattic/jetpack#28870

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove jetpack dependency #2439

refactor: remove jetpack dependency #2439

frosso commented Jun 30, 2021 •

edited by waclawjacek

Loading

frosso Jun 30, 2021

DanReyLop Jul 5, 2021

waclawjacek Jun 1, 2023 •

edited

Loading

frosso Jun 30, 2021

waclawjacek Jun 1, 2023

frosso Jun 30, 2021

frosso Jul 2, 2021

frosso Jun 30, 2021

frosso Jun 30, 2021

frosso Jun 30, 2021

frosso Jun 30, 2021

frosso Jun 30, 2021

frosso Jun 30, 2021 •

edited

Loading

DanReyLop Jul 5, 2021

waclawjacek Oct 18, 2023

frosso Jun 30, 2021

frosso Jul 1, 2021

waclawjacek May 31, 2023 •

edited

Loading

frosso Jul 1, 2021

frosso Jul 1, 2021

frosso Jul 2, 2021

frosso Jul 2, 2021

waclawjacek Oct 19, 2023

waclawjacek Oct 19, 2023

harriswong left a comment

waclawjacek commented Oct 23, 2023

harriswong commented Oct 24, 2023 •

edited

Loading

harriswong commented Oct 24, 2023 •

edited

Loading

waclawjacek commented Oct 25, 2023 •

edited

Loading

harriswong commented Oct 25, 2023 •

edited

Loading

waclawjacek commented Oct 26, 2023

waclawjacek commented Oct 26, 2023

harriswong commented Oct 26, 2023 •

edited

Loading

		$is_connected = WC_Connect_Jetpack::is_active() \|\| WC_Connect_Jetpack::is_development_mode();
		if ( ! is_plugin_active( 'jetpack/jetpack.php' ) ) {

refactor: remove jetpack dependency #2439

refactor: remove jetpack dependency #2439

Conversation

frosso commented Jun 30, 2021 • edited by waclawjacek Loading

Description

Related issue(s)

Steps to reproduce & screenshots/GIFs

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waclawjacek Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frosso Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waclawjacek May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harriswong left a comment

Choose a reason for hiding this comment

waclawjacek commented Oct 23, 2023

harriswong commented Oct 24, 2023 • edited Loading

harriswong commented Oct 24, 2023 • edited Loading

waclawjacek commented Oct 25, 2023 • edited Loading

harriswong commented Oct 25, 2023 • edited Loading

waclawjacek commented Oct 26, 2023

waclawjacek commented Oct 26, 2023

harriswong commented Oct 26, 2023 • edited Loading

frosso commented Jun 30, 2021 •

edited by waclawjacek

Loading

waclawjacek Jun 1, 2023 •

edited

Loading

frosso Jun 30, 2021 •

edited

Loading

waclawjacek May 31, 2023 •

edited

Loading

harriswong commented Oct 24, 2023 •

edited

Loading

harriswong commented Oct 24, 2023 •

edited

Loading

waclawjacek commented Oct 25, 2023 •

edited

Loading

harriswong commented Oct 25, 2023 •

edited

Loading

harriswong commented Oct 26, 2023 •

edited

Loading