Hides jetpack during onboarding if already connected #2516

dsawardekar · 2024-08-09T02:36:02Z

Changes proposed in this Pull Request:

This PR improves the plugin setup process by conditionally hiding the WP.com account connection card when the store is already connected to WP.com.

Closes: #2487

Screenshots:

Disconnected:

Connected:

Settings:

Detailed test instructions:

Switch to this branch, and rebuild the plugin with npm run build
With Jetpack not connected, go to the onboarding flow.

You should see the WP.com / Jetpack Connection Card

Connect Jetpack, and then refresh the page to restart the onboarding steps

The WP.com / Jetpack Connection Card should be hidden

Verify that the WP.com card is still shown on the settings tab.

You can use the below code in an mu-plugin to temporarily test this by toggling the active state to 'yes' / 'no'.

add_filter( 'rest_pre_echo_response', function( $result, $server, $request ) {
	if ( '/wc/gla/jetpack/connected' === $request->get_route() ) {
		$result['active'] = 'no';
	}

	return $result;
}, 10, 3 );

Additional details:

Update - Hides WP.com card if already connected

To run the e2e tests for the onboarding process you can use,

$ npm run test:e2e -- tests/e2e/specs/setup-mc/step-1-accounts.test.js'

This PR also includes some refactoring of the E2E test playwright utilities to avoid relying on nth-child locations for account cards, now that the WP.com account card will not be displayed in some scenarios.

Changelog entry

Update - Hides WordPress.com account connection setting from the onboarding flow if already connected.

codecov · 2024-08-09T02:41:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.8%. Comparing base (f398161) to head (ecd25fc).
Report is 149 commits behind head on feature/2458-streamline-onboarding.

Additional details and impacted files

@@                           Coverage Diff                           @@
##             feature/2458-streamline-onboarding   #2516      +/-   ##
=======================================================================
- Coverage                                  64.5%   63.8%    -0.7%     
=======================================================================
  Files                                       795     326     -469     
  Lines                                     22844    5086   -17758     
  Branches                                   1220    1229       +9     
=======================================================================
- Hits                                      14739    3247   -11492     
+ Misses                                     7938    1672    -6266     
  Partials                                    167     167

Flag	Coverage Δ
js-unit-tests	`63.8% <ø> (+0.3%)`	⬆️
php-unit-tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 479 files with indirect coverage changes

joemcgill

This looks right to me. One small code quality nitpick, but it looks like it's functioning correctly. 👍🏻

joemcgill · 2024-08-12T14:59:20Z

js/src/setup-mc/setup-stepper/setup-accounts/index.js

@@ -101,6 +101,8 @@ const SetupAccounts = ( props ) => {
 	 * and the whole page would display this AppSpinner, which is not desired.
 	 */
 	const isLoadingJetpack = ! jetpack;
+	const isJetpackInactive = jetpack?.active !== 'yes';


Nit: There are a couple other places below where we are checking for the jetpack.active value (e.g., isLoadingGoogle and isGoogleAccountDisabled. Let's consolidate all of this logic by updating those references to make use of the new variable you're adding here. For readability, I'd also suggest making the variable isJetpackActive rather than inactive, to avoid the double negative when checking for ! isJetpackInactive, ex:

const isJetpackActive = jetpack?.active === 'yes'; // ...etc. { ! isJetPackActive && ( <WPComAccountCard /> ) }

dsawardekar · 2024-08-13T04:57:37Z

@joemcgill I've refactored the code per the feedback. Also confirmed that settings still show the card, AC is also updated to reflect this. Back to you for another review.

joemcgill

Thanks @dsawardekar. The updates all look good to me. I tried running the e2e tests locally with these changes and noticed that this would require quite a few updates to the existing test suite, since many of the existing playwright tests depend on finding HTML elements in a particular order in the screen.

I went ahead and opened a follow-up PR that addresses those issues so you can review how these were previously set up.

Once you've taken a look, let's merge that PR into this branch prior to passing to QA for review.

…e-tests Update e2e tests for #2487

dsawardekar · 2024-08-14T05:06:03Z

Thanks, @joemcgill . I've merged the PR, and updated the AC to remove the non-mocking based instructions.

joemcgill

Thanks, @dsawardekar. Looks good to me.

ankitguptaindia · 2024-08-15T12:30:15Z

@joemcgill, I have reviewed the PR and found it to be functioning as described ✅. However, I came across a use case where I would appreciate your feedback:

Current Behavior: When a user completes the onboarding flow, the WordPress.com card is hidden if Jetpack is connected, and the status is displayed as "connected" on the plugin’s Settings tab.

Use Case: The button on the Settings screen labelled "Disconnect from all accounts" suggests that clicking it will disconnect all accounts, including the WordPress.com account. Furthermore, when the user clicks this button, a confirmation message (e.g., "I understand that I am disconnecting any WordPress.com account...") appears, reinforcing that the WordPress.com account will be disconnected.

Given this, a user might expect the WordPress.com card to reappear on the onboarding screen when they restart the plugin setup. However, with the current implementation, the WordPress.com card does not appear on the first screen of the onboarding steps after all accounts have been disconnected.

This discrepancy between the expected behaviour and the label/button details could cause some confusion for users.

Recording.794.mp4

Disconnection Screens

Additional Note: I tested the same flow with the stable build and found that, when the user disconnects all accounts and restarts the onboarding journey, the WordPress.com card appears on the onboarding screen with a "CONNECTED" status.

Screenshot

joemcgill · 2024-08-15T14:41:11Z

Thanks @ankitguptaindia! Re:

Use Case: The button on the Settings screen labelled "Disconnect from all accounts" suggests that clicking it will disconnect all accounts, including the WordPress.com account. Furthermore, when the user clicks this button, a confirmation message (e.g., "I understand that I am disconnecting any WordPress.com account...") appears, reinforcing that the WordPress.com account will be disconnected.

This behavior is something that both @dsawardekar and I noticed as well. This seems to be the current behavior in the develop branch and not something related to the changes made here. I'm going to create a new issue to address this.

joemcgill · 2024-08-15T14:42:52Z

@eason9487 this one is ready for your review.

eason9487

Thanks for the work and the change in SetupAccounts looks good.

Left a few suggestions for the e2e test. It would be great if unused methods could be removed.

tests/e2e/specs/setup-mc/step-1-accounts.test.js

tests/e2e/utils/pages/setup-mc/step-1-set-up-accounts.js

eason9487 · 2024-08-16T08:26:03Z

js/src/setup-mc/setup-stepper/setup-accounts/index.js

 	const isLoadingGoogleMCAccount =
 		google?.active === 'yes' && scope.gmcRequired && ! googleMCAccount;

 	if ( isLoadingJetpack || isLoadingGoogle || isLoadingGoogleMCAccount ) {
 		return <AppSpinner />;
 	}

-	const isGoogleAccountDisabled = jetpack?.active !== 'yes';


Nice improvement! Thanks!

joemcgill · 2024-08-16T21:03:37Z

Re: #2516 (comment)

Use Case: The button on the Settings screen labelled "Disconnect from all accounts" suggests that clicking it will disconnect all accounts, including the WordPress.com account. Furthermore, when the user clicks this button, a confirmation message (e.g., "I understand that I am disconnecting any WordPress.com account...") appears, reinforcing that the WordPress.com account will be disconnected.

This behavior is something that both @dsawardekar and I noticed as well. This seems to be the current behavior in the develop branch and not something related to the changes made here. I'm going to create a new issue to address this.

I'm no longer seeing this behavior today and the WP.com connection is being disconnected correctly when clicking "Disconnect all accounts" so this may have been due to a local configuration issue.

dsawardekar · 2024-08-19T06:02:18Z

@joemcgill PR updated, back to you for CR.

RE: Disconnection

I'm still seeing the disconnection issue. It appears to disconnect correctly, but shows an error on the FE. But if you refresh the page, everything except the Jetpack connection has disconnected. I'm currently using the jetpack REST override to reset the state. If there is a better way, please let me know.

tests/e2e/specs/setup-mc/step-1-accounts.test.js

joemcgill · 2024-08-19T12:35:56Z

tests/e2e/specs/setup-mc/step-1-accounts.test.js

+			await expect(
+				page.getByRole( 'button', { name: 'Connect' } ).first()
+			).toBeEnabled();


Let's remove this assertion once the once the previous suggestion is updated.

Suggested change

await expect(

page.getByRole( 'button', { name: 'Connect' } ).first()

).toBeEnabled();

joemcgill

Thanks for the updates

eason9487

Thanks for the update for e2e tests! LGTM.

Hides jetpack during onboarding if already connected

64f51bf

github-actions bot added the changelog: update Big changes to something that wasn't broken. label Aug 9, 2024

joemcgill linked an issue Aug 9, 2024 that may be closed by this pull request

Onboarding: Hide WP.com connection card if already connected #2487

Open

4 tasks

dsawardekar added 2 commits August 12, 2024 09:09

Limits jetpack check to only the WPComAccount card

88e0799

Adds e2e tests for jetpack connected & disconnected states

ad40c2c

dsawardekar self-assigned this Aug 12, 2024

dsawardekar marked this pull request as ready for review August 12, 2024 04:18

dsawardekar requested a review from joemcgill August 12, 2024 04:19

joemcgill requested changes Aug 12, 2024

View reviewed changes

Refactors variable names in setup flow

e2f2420

dsawardekar requested a review from joemcgill August 13, 2024 04:29

Update e2e tests

f1d84f6

joemcgill reviewed Aug 13, 2024

View reviewed changes

dsawardekar and others added 2 commits August 14, 2024 10:29

Fixes linter warnings

7523a05

Merge pull request #2521 from woocommerce/update/2487-hide-jetpack-e2…

9e606c1

…e-tests Update e2e tests for #2487

dsawardekar requested a review from joemcgill August 14, 2024 05:08

joemcgill approved these changes Aug 14, 2024

View reviewed changes

joemcgill requested a review from eason9487 August 15, 2024 14:42

eason9487 reviewed Aug 16, 2024

View reviewed changes

Updates e2e tests per PR feedack & removes unused code

223935e

joemcgill reviewed Aug 19, 2024

View reviewed changes

Updates e2e tests per CR feedback

ecd25fc

joemcgill approved these changes Aug 19, 2024

View reviewed changes

joemcgill requested a review from eason9487 August 19, 2024 12:59

eason9487 approved these changes Aug 20, 2024

View reviewed changes

joemcgill changed the base branch from develop to feature/2458-streamline-onboarding August 22, 2024 13:56

joemcgill merged commit 0bb6163 into feature/2458-streamline-onboarding Aug 22, 2024
7 checks passed

joemcgill deleted the update/2487-hide-jetpack-if-already-connected-onboarding branch August 22, 2024 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hides jetpack during onboarding if already connected #2516

Hides jetpack during onboarding if already connected #2516

dsawardekar commented Aug 9, 2024 •

edited by eason9487

Loading

codecov bot commented Aug 9, 2024 •

edited

Loading

joemcgill left a comment

joemcgill Aug 12, 2024

dsawardekar Aug 13, 2024

dsawardekar commented Aug 13, 2024

joemcgill left a comment

dsawardekar commented Aug 14, 2024

joemcgill left a comment

ankitguptaindia commented Aug 15, 2024

joemcgill commented Aug 15, 2024

joemcgill commented Aug 15, 2024

eason9487 left a comment

eason9487 Aug 16, 2024

joemcgill commented Aug 16, 2024

dsawardekar commented Aug 19, 2024

joemcgill Aug 19, 2024

joemcgill left a comment

eason9487 left a comment

	await expect(
	page.getByRole( 'button', { name: 'Connect' } ).first()
	).toBeEnabled();

Hides jetpack during onboarding if already connected #2516

Hides jetpack during onboarding if already connected #2516

Conversation

dsawardekar commented Aug 9, 2024 • edited by eason9487 Loading

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Additional details:

Changelog entry

codecov bot commented Aug 9, 2024 • edited Loading

Codecov Report

joemcgill left a comment

Choose a reason for hiding this comment

joemcgill Aug 12, 2024

Choose a reason for hiding this comment

dsawardekar Aug 13, 2024

Choose a reason for hiding this comment

dsawardekar commented Aug 13, 2024

joemcgill left a comment

Choose a reason for hiding this comment

dsawardekar commented Aug 14, 2024

joemcgill left a comment

Choose a reason for hiding this comment

ankitguptaindia commented Aug 15, 2024

joemcgill commented Aug 15, 2024

joemcgill commented Aug 15, 2024

eason9487 left a comment

Choose a reason for hiding this comment

eason9487 Aug 16, 2024

Choose a reason for hiding this comment

joemcgill commented Aug 16, 2024

dsawardekar commented Aug 19, 2024

joemcgill Aug 19, 2024

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

eason9487 left a comment

Choose a reason for hiding this comment

dsawardekar commented Aug 9, 2024 •

edited by eason9487

Loading

codecov bot commented Aug 9, 2024 •

edited

Loading