Update/implement connect to jetpack #38674

moon0326 · 2023-06-11T04:42:02Z

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

This is still WIP, but I'm opening it now to receive feedback and unblock other Jetpack login page PRs.

Jetpack Authorization is functioning as expected, but I'm not 100% comfortable.

Both Jetpack Connection Rest Auth and Config are loaded on every page. It seems like this is required for Jetpack to work, but I would like to find a way to load them conditionally, as we only need Jetpack when the user selects Jetpack during the core profiler.
I'm setting slug as woocommerce in the config, but I'm not sure how/where this value is used exactly. I could not find the value on WPCOM.

I'll look into these two next week as we learn more about how Jetpack works.

You can find more about Jetpack Connection setup from the following repos:

This was part of https://github.com/woocommerce/team-ghidorah/issues/211, but never implemented in the original PR.

How to test the changes in this Pull Request:

In order to generate Jetpack auth URL, your local env must be publicly accessible. Either use Jurassic Tube or Ngrok to expose your local env.

Install and activate WooCommerce on a brand new site
Install the WooCommerce Beta Tester plugin
Go to /wp-admin/tools.php?page=woocommerce-admin-test-helper
Enable the 'core-profiler' feature flag
Visit your public URL/wp-admin/admin.php?page=wc-admin&path=%2Fsetup-wizard
Go through the new core profiler flow.
Make sure to select Jetpack on the plugins page.
Click Continue
You should be redirected to Jetpack Auth page.

If you want to change from value for testing purposes, change the value from this line

github-actions · 2023-06-11T05:04:35Z

Hi @jorgeatorres, @chihsuan, @adrianduffell,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

github-actions · 2023-06-11T05:41:33Z

Test Results Summary

Commit SHA: cb64a45

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 49s
E2E Tests	195	0	0	10	0	205	13m 10s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

adrianduffell

@moon0326 Nice work getting started and thanks for providing an early draft! 👍

I've left some thoughts about the endpoint

adrianduffell · 2023-06-12T04:15:53Z

plugins/woocommerce/src/Admin/API/OnboardingPlugins.php

+		$redirect_url = apply_filters( 'woocommerce_admin_onboarding_jetpack_connect_redirect_url', esc_url_raw( admin_url( 'admin.php?page=wc-admin' ) ) );
+		$calypso_env  = defined( 'WOOCOMMERCE_CALYPSO_ENVIRONMENT' ) && in_array( WOOCOMMERCE_CALYPSO_ENVIRONMENT, [ 'development', 'wpcalypso', 'horizon', 'stage' ], true ) ? WOOCOMMERCE_CALYPSO_ENVIRONMENT : 'production';
+
+		return [
+			'url' => add_query_arg(
+				[
+					'from'        => apply_filters( 'woocommerce_admin_onboarding_jetpack_connect_from_arg', 'woocommerce-onboarding' ),
+					'calypso_env' => $calypso_env,
+				],
+				$manager->get_authorization_url( null, $redirect_url )
+			),
+		];


Can the redirect_url and from values be passed to the endpoint as parameters?

Looking into the future a bit, I think the connection screen will need to render differently (progress bar, copy, images) depending on what the user was doing:

Selecting Jetpack in Core Profiler

Setting up mobile app

Using an AI feature

Setting up shipping / mobile task

I'm thinking that each of those features above would perform the REST API request with granular redirect_url and from values relating to the feature. If we did that, the connection screen could then be customized for each feature as needed, and have redirection back to where the user started from.

I think maybe you had the same idea with the filters but I'm not sure it can support this level of granularity?

I agree, I think it might be better as URL parameters. Using filters requires PHP for it to work, and I think it risks "leaks" (i.e.: A plugin unintentionally registers the filter in a global code), whereas URL param requires more deliberate use.

Thank you both! Updated in a5f3581

adrianduffell · 2023-06-12T04:25:52Z

plugins/woocommerce/src/Admin/API/OnboardingPlugins.php

+		// Register the site to wp.com.
+		if ( ! $manager->is_connected() ) {
+
+			$result = $manager->try_registration();


It looks like try_registration() has a $tos_agree param. I think we'll need to pass that state through here - with the plugin guidelines we would want to avoid Jetpack servers until users have knowingly agreed to it.

On the plugins page, we show the following statement when you select Jetpack. This is how the Core has been working AFAIK.

@moon0326 ,what about in contexts outside of the onboarding wizard?

I think it could be solved with a tos_agree endpoint parameter as well. I think we need to consider that various parties will use this endpoint and there should be protection against accidental requests to Jetpack servers

Oops, didn't mean to approve a draft 😅

ilyasfoo

Jetpack connection tests well!

Both Jetpack Connection Rest Auth and Config are loaded on every page. It seems like this is required for Jetpack to work, but I would like to find a way to load them conditionally, as we only need Jetpack when the user selects Jetpack during the core profiler.

I agree, we should find a way to selectively load the classes. A naive way to do it might be via detecting relevant URLs?

I'm setting slug as woocommerce in the config, but I'm not sure how/where this value is used exactly. I could not find the value on WPCOM.

I found it stored in jetpack_connection_active_plugins. This is then used in multiple places, one notable is that it's synced with WPCOM (1, 2). AFAIK it's not a requirement for our use case, but I imagine a possibility is we could have some WC related data periodically synced to WPCOM.

However, note that we already have WooCommerce-related data sync built-in into Jetpack plugin - which powers mobile app AFAIK. I don't think we have any need to utilize this feature yet.

packages/js/data/src/onboarding/resolvers.ts

plugins/woocommerce/src/Admin/API/OnboardingPlugins.php

plugins/woocommerce/woocommerce.php

ilyasfoo · 2023-06-12T10:39:44Z

plugins/woocommerce-admin/client/core-profiler/index.tsx

+								target: 'sendToJetpackAuthPage',
+								cond: ( _context ) =>
+									_context.pluginsSelected.find(
+										( plugin ) => plugin === 'jetpack'


I wonder if this is sufficient, I remember being redirected to Jetpack auth when Jetpack is installed but not connected and I selected WooCommerce Tax extension in OBW.

However I'm not sure if that logic came from WC Core or Tax plugin

Good suggestion. I'll check WooCommerce Tax and Shipping.

The current OBW only considers Jetpack. I still think it's a good idea to redirect to Jetpack auth when WooCommerce Tax or Shipping is selected. Let's implement that in a separate PR, though.

Sounds good!

Actually, I just tested in a JN with the current WooCommerce. It seems like if Jetpack is activated but the site is not connected, it will still go to auth screen even though I didn't select any extension to install.

Do we need to adopt the same logic?

As far as I understand, we should only redirect to Jetpack auth when Jetpack is selected. Let's check with @verofasulo ✋

We've clarified this in the design call, we're fine with the current logic of only going with the auth flow when Jetpack is selected 👍

moon0326 · 2023-06-12T22:52:15Z

I found it stored in jetpack_connection_active_plugins. This is then used in multiple places, one notable is that it's synced with WPCOM (1, 2). AFAIK it's not a requirement for our use case, but I imagine a possibility is we could have some WC related data periodically synced to WPCOM.
However, note that we already have WooCommerce-related data sync built-in into Jetpack plugin - which powers mobile app AFAIK. I don't think we have any need to utilize this feature yet.

Thank you for looking into it 🙏

plugins/woocommerce/src/Admin/API/OnboardingPlugins.php

moon0326 · 2023-06-13T21:04:55Z

Hey @woocommerce/proton ✋

Ghidorah is working on adding Jetpack Connection for the new core profiler project. We've made the following changes.

Added a few composer packages here.
Added some codes in woocommerce.php here.
We use the package here to generate Jetpack Auth URL.

Could you please review these changes when you get a chance please 🙏 ?

chihsuan · 2023-06-14T03:34:43Z

Go through the new core profiler flow.

Nice work, @moon0326! 👍 Should we add more instructions on how to go to the core profiler flow to help other teams know how to test this? 🙂

I think we can copy the instructions from previous PR such as #38620. I've also added a snippet command to TextExpander for core profiler wct.profiler.

moon0326 · 2023-06-14T20:47:52Z

Go through the new core profiler flow.

Nice work, @moon0326! 👍 Should we add more instructions on how to go to the core profiler flow to help other teams know how to test this? 🙂

I think we can copy the instructions from previous PR such as #38620. I've also added a snippet command to TextExpander for core profiler wct.profiler.

Thanks! Updated!

…o communicate back to the connected site

…ck was selected

Co-authored-by: Ilyas Foo <foo.ilyas@gmail.com>

* Extracted out isJetpackConnected cond to a guard * Added meta data for isJetpackConnected to prevent unwanted spinner

adrianduffell

LGTM @moon0326 thanks for the great work on this! 🚀

moon0326 marked this pull request as draft June 11, 2023 04:42

github-actions bot added focus: react admin [team:Ghidorah] package: @woocommerce/data issues related to @woocommerce/data plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Jun 11, 2023

moon0326 changed the base branch from trunk to update/38436-additional-changes-for-plugins-page June 11, 2023 04:42

moon0326 force-pushed the update/implement-connect-to-jetpack branch 2 times, most recently from c703a7b to 41f25dc Compare June 11, 2023 05:03

moon0326 requested review from a team, ilyasfoo, chihsuan and rjchow June 11, 2023 05:04

adrianduffell previously approved these changes Jun 12, 2023

View reviewed changes

ilyasfoo reviewed Jun 12, 2023

View reviewed changes

packages/js/data/src/onboarding/resolvers.ts Outdated Show resolved Hide resolved

plugins/woocommerce/src/Admin/API/OnboardingPlugins.php Outdated Show resolved Hide resolved

ilyasfoo reviewed Jun 12, 2023

View reviewed changes

plugins/woocommerce/woocommerce.php Outdated Show resolved Hide resolved

ilyasfoo reviewed Jun 12, 2023

View reviewed changes

moon0326 mentioned this pull request Jun 13, 2023

Customize Jetpack connection logged-in screen for woocommerce core profiler flow Automattic/wp-calypso#77882

Merged

7 tasks

moon0326 marked this pull request as ready for review June 13, 2023 03:45

ilyasfoo reviewed Jun 13, 2023

View reviewed changes

plugins/woocommerce/src/Admin/API/OnboardingPlugins.php Show resolved Hide resolved

barryhughes requested review from a team and jorgeatorres and removed request for a team June 13, 2023 21:10

Base automatically changed from update/38436-additional-changes-for-plugins-page to trunk June 13, 2023 22:03

moon0326 changed the base branch from trunk to 3.4 June 14, 2023 21:11

moon0326 and others added 21 commits June 15, 2023 19:35

Add getJetpackAuthUrl to data/onboarding

51844f3

Add Connection Rest Auth -- seems like this is required for Jetpack t…

7614315

…o communicate back to the connected site

Add jetpack-authorization-url REST API

36c8d8b

Redirect to Jetpack Auth page after the installatino process if jetpa…

43fe400

…ck was selected

Require can_install_plugins permission for jetpack auth url endpoint

0f46c71

Update packages/js/data/src/onboarding/resolvers.ts

3b85763

Co-authored-by: Ilyas Foo <foo.ilyas@gmail.com>

Update plugins/woocommerce/src/Admin/API/OnboardingPlugins.php

5e62687

Co-authored-by: Ilyas Foo <foo.ilyas@gmail.com>

Remove automattic/jetpack-sync and its config

3cd5cd9

Support redirect_url and from params

0a0790f

Do not redirect to Jetpack auth if it is already connected

544bfec

Add installed_ext_success=1

101d83c

Use woocommerce-core-profiler for from value

4156de1

Revert unrelated lock file changes

690bef3

Minor refactor

912fc14

* Extracted out isJetpackConnected cond to a guard * Added meta data for isJetpackConnected to prevent unwanted spinner

Add Changelog

60848c4

Lint fix

7e534b6

Move Jetpack Connection config init to class-woocommerce

3477a00

Add changelog

20222a7

Remove unnecessary require

6dd0343

Note jetpack-authorization-url endpoint is experimental

002f9fe

Fix broken test

7bb26f7

moon0326 force-pushed the update/implement-connect-to-jetpack branch from dc3029d to 7bb26f7 Compare June 16, 2023 02:35

Add component to sendToJetPackAuthPage state

5bca7d8

adrianduffell self-requested a review June 16, 2023 02:39

adrianduffell approved these changes Jun 16, 2023

View reviewed changes

Fix lint error

cb64a45

moon0326 merged commit 630faad into trunk Jun 16, 2023
17 checks passed

moon0326 deleted the update/implement-connect-to-jetpack branch June 16, 2023 03:28

github-actions bot added this to the 7.9.0 milestone Jun 16, 2023

chihsuan mentioned this pull request Jun 27, 2023

Include JP connection package #32135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update/implement connect to jetpack #38674

Update/implement connect to jetpack #38674

moon0326 commented Jun 11, 2023 •

edited

github-actions bot commented Jun 11, 2023 •

edited

github-actions bot commented Jun 11, 2023 •

edited

adrianduffell left a comment

adrianduffell Jun 12, 2023

ilyasfoo Jun 12, 2023

moon0326 Jun 13, 2023

adrianduffell Jun 12, 2023

moon0326 Jun 12, 2023 •

edited

adrianduffell Jun 14, 2023

adrianduffell Jun 14, 2023

ilyasfoo left a comment •

edited

ilyasfoo Jun 12, 2023

ilyasfoo Jun 12, 2023

moon0326 Jun 12, 2023

moon0326 Jun 13, 2023

ilyasfoo Jun 13, 2023

ilyasfoo Jun 13, 2023 •

edited

moon0326 Jun 13, 2023

ilyasfoo Jun 14, 2023 •

edited

moon0326 commented Jun 12, 2023 •

edited

moon0326 commented Jun 13, 2023 •

edited

chihsuan commented Jun 14, 2023

moon0326 commented Jun 14, 2023

adrianduffell left a comment

Update/implement connect to jetpack #38674

Update/implement connect to jetpack #38674

Conversation

moon0326 commented Jun 11, 2023 • edited

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

github-actions bot commented Jun 11, 2023 • edited

github-actions bot commented Jun 11, 2023 • edited

Test Results Summary

adrianduffell left a 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

moon0326 Jun 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyasfoo left a comment • edited

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

ilyasfoo Jun 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyasfoo Jun 14, 2023 • edited

Choose a reason for hiding this comment

moon0326 commented Jun 12, 2023 • edited

moon0326 commented Jun 13, 2023 • edited

chihsuan commented Jun 14, 2023

moon0326 commented Jun 14, 2023

adrianduffell left a comment

Choose a reason for hiding this comment

moon0326 commented Jun 11, 2023 •

edited

github-actions bot commented Jun 11, 2023 •

edited

github-actions bot commented Jun 11, 2023 •

edited

moon0326 Jun 12, 2023 •

edited

ilyasfoo left a comment •

edited

ilyasfoo Jun 13, 2023 •

edited

ilyasfoo Jun 14, 2023 •

edited

moon0326 commented Jun 12, 2023 •

edited

moon0326 commented Jun 13, 2023 •

edited