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

Limits steps in onboarding based on previously completed state #2568

Draft
wants to merge 7 commits into
base: feature/2458-streamline-onboarding
Choose a base branch
from

Conversation

dsawardekar
Copy link
Collaborator

Changes proposed in this Pull Request:

Closes #2493

This PR hides the confirm registration step if the user has sufficient data filled in already. If the user was on the confirm registration step previously, they are sent to the next step.

Screenshots:

With incomplete store requirements: (Confirm store requirements is present)
image

With complete store requirements: (Confirm store requirements is absent)
image

Detailed test instructions:

  1. Start the onboarding process with incomplete store data.
  2. You should see the Step no. 3 Confirm Store Requirements
  3. Reset your env, and delete some store data (eg:- Delete City under WooCommerce Store Address)
  4. Restart the onboarding process
  5. You should not see the Step no. 3

Additional details:

Update: Hides the Confirm Store Requirements step when feasible

Changelog entry

Update: Hides the Confirm Store Requirements step when feasible

@github-actions github-actions bot added the changelog: update Big changes to something that wasn't broken. label Aug 27, 2024
@dsawardekar dsawardekar self-assigned this Aug 27, 2024
@dsawardekar dsawardekar linked an issue Aug 27, 2024 that may be closed by this pull request
2 tasks
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 40.81633% with 29 lines in your changes missing coverage. Please review.

Project coverage is 63.0%. Comparing base (766e351) to head (b3d8730).
Report is 319 commits behind head on feature/2458-streamline-onboarding.

Files with missing lines Patch % Lines
js/src/hooks/useStoreAddress.js 26.1% 8 Missing and 9 partials ⚠️
js/src/hooks/useGoogleMCPhoneNumber.js 40.0% 6 Missing and 3 partials ⚠️
.../src/setup-mc/setup-stepper/saved-setup-stepper.js 72.7% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                         Coverage Diff                          @@
##           feature/2458-streamline-onboarding   #2568     +/-   ##
====================================================================
- Coverage                                63.8%   63.0%   -0.8%     
====================================================================
  Files                                     326     321      -5     
  Lines                                    5088    5116     +28     
  Branches                                 1232    1255     +23     
====================================================================
- Hits                                     3247    3225     -22     
- Misses                                   1673    1708     +35     
- Partials                                  168     183     +15     
Flag Coverage Δ
js-unit-tests 63.0% <40.8%> (-0.8%) ⬇️

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

Files with missing lines Coverage Δ
.../src/setup-mc/setup-stepper/saved-setup-stepper.js 84.3% <72.7%> (-3.5%) ⬇️
js/src/hooks/useGoogleMCPhoneNumber.js 47.1% <40.0%> (ø)
js/src/hooks/useStoreAddress.js 34.6% <26.1%> (ø)

... and 62 files with indirect coverage changes

@dsawardekar
Copy link
Collaborator Author

@joemcgill This is working but I needed to move the implementation to the parent container component. Having the logic inside the SavedSetupStepper was causing a broken UX like below when the user already had completed store requirements.

Screenshot from 2024-08-21 16-31-16

Can you review and let me know if this looks good to proceed on the e2e tests?

Thanks!

Copy link
Collaborator

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

@dsawardekar I think this approach makes sense. Particularly since it's the SetupStepper that manages the loading state prior to rendering the SavedSetupStepper.

Once you've updated the E2E tests, feel free to send this back for review.

Comment on lines 48 to 53
// If the user has already completed the store requirements, but is currently still on the
// store requirements step, we should skip the store requirements step and go to the paid ads step.
// else they will get stuck on a non-existent step #3
if ( step === 'store_requirements' && hasConfirmedStoreRequirements ) {
step = 'paid_ads';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Smart. Rather than mutating the step variable here, it may be better to do something like this:

const { status, step } = mcSetup;

const currentStep = ( step === 'store_requirements' && hasConfirmedStoreRequirements ) ?  'paid_ads' : step;

@dsawardekar
Copy link
Collaborator Author

@joemcgill I've added the e2e tests for this, but I need assistance with mocking the email and mc account address. When I try to set mockMCSetup to use 'complete', it redirects to the Home page, and skips the onboarding flow completely.

image

Test File:
https://github.com/woocommerce/google-listings-and-ads/blob/31a3ba5017434a3d4f6995f89839dd64546eb888/tests/e2e/specs/setup-mc/step-3-hide-store-requirements.test.js

Can you please take a look? Thanks!

Copy link
Collaborator

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

Thanks @dsawardekar. I've added a bit of initial feedback. I'm going to see if I can get these e2e tests working, but in the mean time can you address the other feedback?

Comment on lines 45 to 46
const { status } = mcSetup;
let { step } = mcSetup;
const { step } = mcSetup;
Copy link
Collaborator

Choose a reason for hiding this comment

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

These can be combined:

const { status, step } = mcSetup;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than creating a whole new spec here, I think we should test for these scenarios in either the existing tests/e2e/specs/setup-mc/step-3-confirm-store-requirements.test.js spec, or test for this scenario in tests/e2e/specs/setup-mc/step-2-product-listings.test.js whenever the continue button is clicked with the address info already correctly mocked. I think that should look something like this:

productListingsPage.mockContactInformation( {
	phone_number: '+15555555',
	phone_verification_status: 'verified',
	mc_address: {
		street_address: '556 Woo St.',
		locality: 'City',
		region: 'California',
		postal_code: '90210',
		country: 'US',
	},
	wc_address: {
		street_address: '556 Woo St.',
		locality: 'City',
		region: 'California',
		postal_code: '90210',
		country: 'US',
	},
	is_mc_address_different: false,
	wc_address_errors: [],
} );

One tricky part here is that the step is only removed when the stepper is first loaded, so we'll need a beforeAll step that reloads the page after the state is correctly mocked.

I'm going to take a look tomorrow to see if I can get the state for this set up properly, since it's a bit tricky.

Copy link
Collaborator

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

@dsawardekar while testing this again, I've realized that part of the problem that has made the E2E difficult to debug is that these updates won't work when an MC account is not connected when the onboarding flow is loaded, which is a pretty significant problem.

If you test this with all accounts disconnected, you'll see an error message pop up when the following hooks are called:

const { data: address, loaded: addressLoaded } = useStoreAddress();
const { data: phone, loaded: phoneLoaded } = useGoogleMCPhoneNumber();

Sorry I didn't catch this when you initially suggested moving these hooks here. I assume I was testing by just refreshing the page after already completing step 1 of the onboarding. I'm curious if we're able to initially show step 3 and then hide it once someone connects their MC account in step 1. If not, then we'll likely need to rethink this issue.

@dsawardekar
Copy link
Collaborator Author

@joemcgill That might work, but the UX would be less than ideal. Maybe we need to consider folding the things that in that extra dissapearing step into sections inside Step 2 / 3. Then if the user has sufficient data, we could hide that section.

This updates both the `useStoreAddress` and `useGoogleMCPhoneNumber` hooks to check for a connected MC account before fetching contact information so the saved setup stepper can determine if the user has the necessary information to proceed prior to connecting an MC account.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: update Big changes to something that wasn't broken.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Onboarding: Remove “Confirm store requirements” step if possible
2 participants