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

Remove legacy Jetpack + Woo onboarding flow and replace with Woo JPC flow #99558

Merged
merged 14 commits into from
Feb 12, 2025

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Feb 10, 2025

Follow-up to #99266

Proposed Changes

  • Remove the legacy Jetpack + Woo onboarding flow. The flow is apparently not in use anymore, as confirmed by @chihsuan and @ilyasfoo in Jetpack WooCommerce login: fix logo #99266 (comment)
  • the old flow (represented in code generally by the isJetpackWooCommerceFlow and isWooOnboarding variables, and associated to the from='woocommerce-onboarding' query string) is removed and "merged" into the Woo JPC flow
  • The PR follows the general steps proposed in p1738776609659759-slack-C01SFMVEYAK
  • The following tracks events are not being fired anymore:
    • wcadmin_storeprofiler_create_jetpack_account
    • wcadmin_storeprofiler_login_jetpack_account
    • wcadmin_storeprofiler_connect_store
  • Related styles are also being removed

Why are these changes being made?

  • Removing unneeded code keeps our codebase tidy

Testing Instructions

  1. Check how the legacy, JPC and Woo DNA onboarding flows behave on trunk:
  • To test the old legacy flow (ie. jetpack woo onboarding) on trunk, visit this link
  • To test the new onboarding flow (ie. Woo JPC) on trunk, visit this link
  • To test the Woo DNA flow on trunk, visit this link
  • Note: the main difference between those two links is in the last from= query parameter: woocommerce-onboarding for the legacy onboarding flow, woocommerce-core-profiler for the Woo JPC flow
  1. Make sure that, on this PR, the same URL used for the legacy jetpack woo onboarding flow navigates to the Woo JPC flow
  2. Make sure that the Woo DNA flow is unaffected by the changes in this PR and works like on trunk
  3. Make sure that the flow described in this comment also works as expected.

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@ciampo ciampo self-assigned this Feb 10, 2025
@matticbot
Copy link
Contributor

matticbot commented Feb 10, 2025

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~362 bytes removed 📉 [gzipped])

name                 parsed_size           gzip_size
entry-login              -2061 B  (-0.1%)     -361 B  (-0.1%)
entry-main                -362 B  (-0.0%)      -49 B  (-0.0%)
entry-subscriptions       -131 B  (-0.0%)      -26 B  (-0.0%)
entry-stepper             -131 B  (-0.0%)      -27 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~549 bytes removed 📉 [gzipped])

name               parsed_size           gzip_size
jetpack-connect        -2840 B  (-0.2%)     -508 B  (-0.1%)
purchase-product        -422 B  (-0.2%)      -47 B  (-0.1%)
accept-invite           -387 B  (-0.2%)      -95 B  (-0.2%)
plugins                 -361 B  (-0.0%)      -45 B  (-0.0%)
media                   -361 B  (-0.0%)      -45 B  (-0.0%)
stepper-user-step        -92 B  (-0.0%)      -52 B  (-0.1%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~304 bytes removed 📉 [gzipped])

name                                               parsed_size           gzip_size
async-load-design-blocks                               -1514 B  (-0.1%)     -279 B  (-0.0%)
async-load-design-playground                            -361 B  (-0.0%)      -45 B  (-0.0%)
async-load-design                                       -361 B  (-0.0%)      -45 B  (-0.0%)
async-load-calypso-post-editor-media-modal              -361 B  (-0.0%)      -45 B  (-0.0%)
async-load-calypso-post-editor-editor-media-modal       -361 B  (-0.0%)      -45 B  (-0.0%)
async-load-signup-steps-user                            -328 B  (-0.1%)      -81 B  (-0.1%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@ciampo
Copy link
Contributor Author

ciampo commented Feb 10, 2025

@chihsuan @ilyasfoo I'm not familiar with Woo's flows, and I need some guidance on how to proceed.

I left // code comments throughout all files; could you take a look and leave comments with the suggested path forward? Thank you 🙏

Copy link
Member

@chihsuan chihsuan 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 working on this! @ciampo 👍

The changes look good to me. We can remove all code related to the old flow (isJetpackWooCommerceFlow, isWooOnboarding) and merge/replace with isWooJPCFlow by adding the 'woocommerce-onboarding' === get( getCurrentQueryArguments( state ), 'from' ) check to isWooJPCFlow selector.

client/jetpack-connect/signup.js Outdated Show resolved Hide resolved
client/jetpack-connect/test/authorize.js Outdated Show resolved Hide resolved
client/layout/index.jsx Outdated Show resolved Hide resolved
client/layout/logged-out.jsx Outdated Show resolved Hide resolved
client/lib/login/index.js Outdated Show resolved Hide resolved
client/jetpack-connect/authorize.js Outdated Show resolved Hide resolved
client/blocks/login/index.jsx Outdated Show resolved Hide resolved
client/blocks/login/login-form.jsx Outdated Show resolved Hide resolved
client/blocks/signup-form/index.jsx Outdated Show resolved Hide resolved
client/blocks/signup-form/index.jsx Outdated Show resolved Hide resolved
@ciampo ciampo force-pushed the remove/legacy-woo-onboarding-flow branch from be15721 to fd1c127 Compare February 11, 2025 10:55
@@ -36,25 +34,6 @@ export class JetpackHeader extends PureComponent {
return null;
}

if ( isWooOnboarding ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @mirka in case this deletion unlocks further improvements to the JetpackHeader component

@matticbot
Copy link
Contributor

matticbot commented Feb 11, 2025

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • blaze-dashboard
  • command-palette-wp-admin
  • happy-blocks
  • help-center
  • notifications
  • odyssey-stats
  • whats-new
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug remove/legacy-woo-onboarding-flow on your sandbox.

Comment on lines 585 to 589
// Should we keep this around, since we otherwise removed other instances
// where we fired this tracks event?
// if ( 'woocommerce-onboarding' === from ) {
// recordTracksEvent( 'wcadmin_storeprofiler_connect_store', { use_account: true } );
// }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chihsuan just wanted to double-check if we're also ok deleting the firing of this tracks event, and more in general, of the other tracks event across this PR:

  • wcadmin_storeprofiler_create_jetpack_account
  • wcadmin_storeprofiler_login_jetpack_account
  • wcadmin_storeprofiler_connect_store

Copy link
Member

Choose a reason for hiding this comment

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

It's ok to delete them. To make sure we don't break any analytics, I searched for them in nosara and didn't find any usage. I also checked the Funnel, and the last usage of the Funnel is 3 years ago before the core profiler was introduced.

For core profiler, we're using the calypso_jpc_wc_coreprofiler_* events for tracking so I don't see the need to keep these.

@ciampo ciampo requested review from ilyasfoo, moon0326 and a team February 11, 2025 11:21
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 11, 2025
@ciampo ciampo marked this pull request as ready for review February 11, 2025 11:22
@ciampo
Copy link
Contributor Author

ciampo commented Feb 11, 2025

Thank you @chihsuan — I've pushed changes according to your feedback, including the merging of the from === 'woocommerce-onboarding' check into isWooJPCFlow, which allowed to clean up quite some logic.

Left one additional comment, and added tentative testing instructions (please don't hesitate to add better testing instructions if possible!)

I think this PR is ready for a wider round of review.

Copy link
Member

@chihsuan chihsuan left a comment

Choose a reason for hiding this comment

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

Thank you for tidying up the code! @ciampo It's great to see the removal of the old flow. 🥇 😃

I have verified both flows using the provided links and ensured that sites created with the old WC plugin function as expected.

Additionally, I reviewed the WooDNA flow (link) and confirmed that it remains unaffected by these changes.

// The legacy Jetpack Woo Onboarding flow is not in use anymore,
// and is now absorbed into the Woo JPC Onboarding flow.
// See https://github.com/Automattic/wp-calypso/pull/99558 for more details.
const isLegacyJetpackWooOnboardingFlow = ( state: AppState ) => {
Copy link
Member

Choose a reason for hiding this comment

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

👍

client/layout/index.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@ilyasfoo ilyasfoo left a comment

Choose a reason for hiding this comment

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

Thanks so much for working on this, @ciampo! I also tested this change with WooCommerce.com auth flow and found no regression.

I took another look in WooCommerce core, and I found an instance that still used woocommerce-onboarding. Apologies for missing it the first time around. Good news is that the flow still works as expected. To reproduce:

  1. Create a fresh JN site with Jetpack installed
  2. Skip core profiler onboarding
  3. Go to WooCommerce > Home
  4. Look for stats widget, and click on Connect Jetpack
  5. Observe it goes to Jetpack connection screen with from=woocommerce-onboarding

Since the connection still works with the WooJPC flow (recording), I think this PR is good to go as it is a step forward the right direction with streamlining flows, and Ghidorah can refine the UX as follow-ups if needed (for example, the copy about accessing features). Let me know what you think @chihsuan @Automattic/ghidorah

@chihsuan
Copy link
Member

Since the connection still works with the WooJPC flow (recording), I think this PR is good to go as it is a step forward the right direction with streamlining flows, and Ghidorah can refine the UX as follow-ups if needed (for example, the copy about accessing features). Let me know what you think @chihsuan @Automattic/ghidorah

Good catch! @ilyasfoo I agree with you. 🙌

@ciampo ciampo force-pushed the remove/legacy-woo-onboarding-flow branch from e19f76b to a3bad34 Compare February 12, 2025 10:32
@ciampo ciampo merged commit e64dd17 into trunk Feb 12, 2025
14 checks passed
@ciampo ciampo deleted the remove/legacy-woo-onboarding-flow branch February 12, 2025 13:39
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 12, 2025
Copy link
Member

@tyxla tyxla 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 the great cleanup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants