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

2492: Remove PreLaunchChecklist component #2532

Conversation

asvinb
Copy link
Collaborator

@asvinb asvinb commented Aug 16, 2024

Changes proposed in this Pull Request:

Closes #2492 .

  • Removed the PreLaunchChecklist component from the codebase, including any associated tests.
  • Removed the check if the form is valid or not before continuing because there are no more checkboxes.
  • Disabled the Continue button in case the phone number is not verified or store address not filled in.
  • Updated E2E tests to account for the previous point.

Screenshots:

image

Detailed test instructions:

  1. Go through the onboarding process.
  2. On the third step, i.e Confirm store requirements, the Pre Launch Checklist section should no longer be present.
  3. Ensure the user can proceed with the next steps in the onboarding process.

Additional details:

Changelog entry

Update - Remove the Pre-Launch Checklist from the onboarding flow.

Copy link

codecov bot commented Aug 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.2%. Comparing base (8096fe1) to head (40bbf82).
Report is 68 commits behind head on feature/2458-streamline-onboarding.

Additional details and impacted files

Impacted file tree graph

@@                           Coverage Diff                           @@
##             feature/2458-streamline-onboarding   #2532      +/-   ##
=======================================================================
+ Coverage                                  63.8%   65.2%    +1.4%     
- Complexity                                    0    4581    +4581     
=======================================================================
  Files                                       325     800     +475     
  Lines                                      5072   22899   +17827     
  Branches                                   1230    1230              
=======================================================================
+ Hits                                       3237   14928   +11691     
- Misses                                     1667    7803    +6136     
  Partials                                    168     168              
Flag Coverage Δ
js-unit-tests 63.8% <100.0%> (-<0.1%) ⬇️
php-unit-tests 65.6% <100.0%> (?)

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

Files with missing lines Coverage Δ
...ponents/free-listings/setup-free-listings/index.js 6.8% <100.0%> (ø)
js/src/data/action-types.js 100.0% <ø> (ø)
js/src/data/actions.js 7.9% <ø> (ø)
js/src/data/reducer.js 83.5% <ø> (ø)
js/src/data/resolvers.js 7.6% <ø> (ø)
js/src/data/selectors.js 58.6% <ø> (ø)
.../Controllers/MerchantCenter/SettingsController.php 97.6% <100.0%> (ø)
src/MerchantCenter/MerchantCenterService.php 95.0% <100.0%> (ø)

... and 474 files with indirect coverage changes

@asvinb asvinb requested a review from joemcgill August 16, 2024 12:14
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 @asvinb, this is looking pretty good. I left feedback on the disabled button change inline. Additionally, there is one other E2E test that needs to be updated here. This test from Step 2 is confirming that the REST requests for the policy checks are being made after moving on from step 2. Now that those are removed, these can also be removed from that test.

<StepContentFooter>
<AppButton
isPrimary
loading={ adapter.isSubmitting }
disabled={ ! settingsSaved }
disabled={
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be a good idea, but I think it's unnecessary. Currently, the form can be submitted as long as the settings aren't being saved. Once saved, if there is some validation that fails, the handleSubmitClick callback will show validation errors rather than moving on to the next step. Therefore, I only think we need to disable this button if we're in the middle of recording some state that would change the validation (not something that is currently applicable).

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.

Updates look good. Thanks @asvinb!

@ankitguptaindia
Copy link
Member

Hi @asvinb, thanks for the PR. I tested this PR and found it to be working as expected. The Pre-Launch Checklist has been removed from the onboarding flow ✅.

The "Continue" button should be disabled if the phone number has not been verified or the store address not filled in.

  1. I have some concerns related to the status of the "Continue" button. ⚠️

Please take a look at the use cases below and let me know if they are not related to the changes completed in this PR and if they are worth addressing in another PR/issue:

I noticed that when the store doesn’t have a verified phone number, the "Continue" button is active, even though it doesn’t allow you to move to the next step. This can be confusing. Please see the attached screenshots.

I observed inconsistent behavior in the onboarding flow. On the first step, when required information or accounts are not added/connected, the button remains inactive (works as expected). However, on the second screen, when required details like 'Shipping times' are not added, the "Continue" button remains active, as it does on the third screen. So on 1st screen, continue button appears inactive and on 2nd, 3rd screen it appear as active when required details are not added.

  1. If a merchant already has a verified phone number and changes the number on the third step, it sends a verification code to verify the new number. However, if the merchant clicks the "Continue" button, the onboarding flow completes based on the previous number.
Please look at below screenshots/screemcast
Recording.800.mp4

First screen:

image

Second/third screens:

image

image

Screenshot 2024-08-22 at 5 20 38 PM

cc: @joemcgill for progress update.

@asvinb
Copy link
Collaborator Author

asvinb commented Aug 22, 2024

@ankitguptaindia Thanks for your comment. Apologies, I did not update the QAB since disabling the continue button was something which I added. You can see @joemcgill 's comment here about it. Let me know if that answers your questions.

@ankitguptaindia
Copy link
Member

QA/Test Report-

Testing Environment -

  • WordPress: 6.6.1
  • Theme active on store: Storefront Version: 4.6.0
  • WooCommerce - Version 9.2.1
  • PHP: 8.3
  • Web Server: Nginx
  • Browser: Chrome - Version 127
  • OS: macOS Sonoma 14.6.1

Test Results - Changes working as expected. PreLaunchChecklist component has been removed and the onboarding flow working as expected.

Next Step- Ready to Code Review(Woo)

Plugin file with this fix-specific code: google-listings-and-ads.zip

@joemcgill joemcgill changed the base branch from develop to feature/2458-streamline-onboarding August 22, 2024 14:01
@joemcgill
Copy link
Collaborator

@eason9487 this one is ready for your review

@eason9487 eason9487 added the changelog: update Big changes to something that wasn't broken. label Aug 26, 2024
Copy link
Contributor

@jorgemd24 jorgemd24 left a comment

Choose a reason for hiding this comment

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

Hi @asvinb, it's working well, just a few comments:

This hook can be removed: #2492 (comment) since it's no longer used in the codebase.

I know this PR focuses on the JS part, but what about the backend code used for the pre-launch checklist? For instance, src/API/Site/Controllers/MerchantCenter/PolicyComplianceCheckController.php—is it still needed, or will it be handled in a different PR?

Also, there are other parts of the JS code that still reference policy_check. Should these be kept, or can they be removed as well?

case TYPES.POLICY_CHECK: {
const { data } = action;
return setIn( state, 'mc.policy_check', data );
}

* Fetch policy info for checking merchant onboarding policy setting.
*/
export function* getPolicyCheck() {
try {
const response = yield apiFetch( {
path: `${ API_NAMESPACE }/mc/policy_check`,
} );
return {
type: TYPES.POLICY_CHECK,
data: response,
};
} catch ( error ) {
handleApiError(
error,
__(
'There was an error loading policy check details.',
'google-listings-and-ads'
)
);
}
}

@@ -418,9 +418,6 @@ test.describe( 'Configure product listings', () => {
} );

test( 'should see the heading of next step and send two requests after clicking "Continue"', async () => {
const requestPromises =
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the /gla/mc/contact-information request is still being fetched, so instead of asserting the policy check response, could we assert the contact information instead? Otherwise, we're just asserting the heading without checking any of the requests. Also, the title could be updated to align with these changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good @jorgemd24 . I've now updated the PR for additional tests.

@asvinb
Copy link
Collaborator Author

asvinb commented Aug 29, 2024

Hello @jorgemd24 ,
Thanks for the review. Removing the PHP part is tackled in the following ticket: #2494
I've also removed any occurrences of policy_check :)

Copy link
Contributor

@jorgemd24 jorgemd24 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 adjustments, LGTM. I couldn’t find any more references to the policy check in the frontend, and it’s working well.

@joemcgill
Copy link
Collaborator

Thanks all. @asvinb feel free to merge this after resolving conflicts and then move to UAT.

@asvinb
Copy link
Collaborator Author

asvinb commented Sep 2, 2024

Thanks @joemcgill @jorgemd24 . Merge conflicts fixed now.

@eason9487
Copy link
Member

eason9487 commented Sep 3, 2024

Hi @asvinb, please hold off on the merge. There is an issue needs to be fixed. There are a few leftover codes need to be cleared together.

Copy link
Member

@eason9487 eason9487 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 work!

It would be appreciated if the following unused codes could be removed together:

  • The extendAdapter prop of AdaptiveForm in the js/src/setup-mc/setup-stepper/store-requirements/index.js as its only use will be removed by this PR
  • All PreLaunchCheckItem component codes under this directory
  • E2E utils:
    • /**
      * Get checklist card.
      *
      * @return {import('@playwright/test').Locator} Get checklist card.
      */
      getChecklistCard() {
      return this.page.locator( '.gla-pre-launch-checklist' );
      }
    • /**
      * Get pre-launch checklist checkboxes.
      *
      * @return {import('@playwright/test').Locator} Get pre-launch checklist checkboxes.
      */
      getPrelaunchChecklistCheckboxes() {
      return this.getChecklistCard().getByRole( 'checkbox' );
      }
      /**
      * Get pre-launch checklist panels.
      *
      * @return {import('@playwright/test').Locator} Get pre-launch checklist panels.
      */
      getPrelaunchChecklistPanels() {
      return this.getChecklistCard().locator( '.components-panel__body' );
      }
    • /**
      * Get read google merchant center requirements link.
      *
      * @return {import('@playwright/test').Locator} Get read google merchant center requirements link.
      */
      getReadGoogleMerchantCenterRequirementsLink() {
      return this.page.getByRole( 'link', {
      name: 'Read Google Merchant Center requirements',
      exact: true,
      } );
      }
    • /**
      * Register settings request when the confirm button or checkbox in pre-lauch checklist is checked.
      *
      * @param {Array} settings
      * @return {Promise<import('@playwright/test').Request[]>} The requests.
      */
      registerPrelaunchChecklistConfirmCheckedRequest( settings = [] ) {
      const promises = [];
      for ( const setting of settings ) {
      promises.push(
      this.page.waitForRequest(
      ( request ) =>
      request.url().includes( '/gla/mc/settings' ) &&
      request.method() === 'POST' &&
      request.postDataJSON()[ setting ] === true
      )
      );
      }
      return Promise.all( promises );
      }
  • All frontend codes and comments related to these keywords:
    • website_live
    • checkout_process_secure
    • payment_methods_visible
    • refund_tos_visible
    • contact_info_visible

js/src/setup-mc/setup-stepper/store-requirements/index.js Outdated Show resolved Hide resolved
@asvinb
Copy link
Collaborator Author

asvinb commented Sep 4, 2024

@eason9487 Kindly note I've updated your PR following your last comment. Can you check again please?

Copy link
Member

@eason9487 eason9487 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 further cleanup! LGTM.

…nch-checklist-items

Remove pre launch checklist items
@joemcgill joemcgill merged commit f39cf03 into feature/2458-streamline-onboarding Sep 6, 2024
15 checks passed
@joemcgill joemcgill deleted the feature/2492-remove-pre-launch-checklist branch September 6, 2024 00:55
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 Pre-Launch checklist UI
7 participants