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

Streamline onboarding: Clean up the access to the pre-launch checklist in the mc/settings API #2555

Conversation

kt-12
Copy link
Collaborator

@kt-12 kt-12 commented Aug 22, 2024

Changes proposed in this Pull Request:

This PR removed any backend related to a pre-launch checklist.

Closes #2494.

The PR addresses it's issue. Check screenshots section for after the change screenshots

Screenshots:

After:
All pre-launch checklist attributes removed from API
Screenshot 2024-08-22 at 09 44 58

We are also able to move to last screen after the removal.
Screenshot 2024-08-22 at 10 44 18

Detailed test instructions:

  1. Switch to this branch and run build command npm run build
  2. Delete gla_merchant_center saved options use npm run wp-env run cli wp option delete gla_merchant_center
  3. On the 3rd screen, verify the Phone number (if not already), add the address(if not already), then press continue; it should take you to the next screen. You should also be able to perform the next steps and create a campaign successfully.

Additional details:

Changelog entry

@kt-12 kt-12 changed the base branch from develop to feature/2492-remove-pre-launch-checklist August 22, 2024 09:52
Copy link

codecov bot commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.2%. Comparing base (32bbb30) to head (c5c547a).

Additional details and impacted files

Impacted file tree graph

@@                              Coverage Diff                              @@
##             feature/2492-remove-pre-launch-checklist   #2555      +/-   ##
=============================================================================
+ Coverage                                        63.8%   65.2%    +1.4%     
- Complexity                                          0    4581    +4581     
=============================================================================
  Files                                             325     800     +475     
  Lines                                            5083   22899   +17816     
  Branches                                         1231    1230       -1     
=============================================================================
+ Hits                                             3241   14928   +11687     
- Misses                                           1674    7803    +6129     
  Partials                                          168     168              
Flag Coverage Δ
js-unit-tests 63.8% <ø> (+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 Δ
.../Controllers/MerchantCenter/SettingsController.php 97.6% <100.0%> (ø)
src/MerchantCenter/MerchantCenterService.php 95.0% <100.0%> (ø)

... and 488 files with indirect coverage changes

---- 🚨 Try these New Features:

@kt-12 kt-12 changed the title Feature/2494 remove pre launch checklist items Remove pre launch checklist items Aug 22, 2024
@mikkamp
Copy link
Contributor

mikkamp commented Aug 22, 2024

Just adding a note that the failing unit test is not related to this PR and is being resolved in #2559

@kt-12
Copy link
Collaborator Author

kt-12 commented Aug 23, 2024

Just adding a note that the failing unit test is not related to this PR and is being resolved in #2559
@mikkamp Thank you for letting us know. @joemcgill Given that the unit test failure is unrelated, I am moving this to 10up CR now.

@joemcgill
Copy link
Collaborator

joemcgill commented Aug 23, 2024

@kt-12 pointed out that after this is removed from the back-end, we may also want to clean up the settings where these are referenced in the SetupFreeListings component (ref)

@joemcgill
Copy link
Collaborator

Merging develop to this branch in e913578 has made reviewing this challenging, and trying to update all of the target branches that this will end up merging with is causing some merge conflicts. It's getting late in my day today, so I'm going to try to clean this up with fresher brain tomorrow.

@joemcgill joemcgill force-pushed the feature/2494-remove-pre-launch-checklist-items branch from e913578 to b3e838f Compare August 27, 2024 20:44
@joemcgill joemcgill force-pushed the feature/2494-remove-pre-launch-checklist-items branch from e45c634 to dd73ec6 Compare August 27, 2024 20:57
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 @kt-12. This looks right to me.

In order to simplify the review and merging process, I restored this branch to commit 7b29364, prior to when you merged in develop and cherry-picked the two commits from #2559 so that the Unit Test will pass.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Changes are cherry picked from https://github.com/woocommerce/google-listings-and-ads/pull/2559/commits to fix unit test failures. This can be ignored.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Changes are cherry picked from https://github.com/woocommerce/google-listings-and-ads/pull/2559/commits to fix unit test failures. This can be ignored.

@ankitguptaindia ankitguptaindia self-requested a review August 28, 2024 10:40
@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.3
  • PHP: 8.3
  • Web Server: Nginx
  • Browser: Chrome - Version 127
  • OS: macOS Sonoma 14.6

Test Results - The changes are working as expected. The pre-launch checklist items are no longer available. The user able to complete onboarding without any trouble. The console and error logs are clear during the onboarding steps.

Functional Demo / Screencast -

image

Copy link
Contributor

@mikkamp mikkamp 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 changes. I can confirm that the payload no longer has the additional fields:

{
  "shipping_rate": "automatic",
  "shipping_time": "flat",
  "tax_rate": "destination"
}

In the response they are also no longer being included:

{
    "status": "success",
    "message": "Merchant Center Settings successfully updated.",
    "data": {
        "shipping_rate": "automatic",
        "shipping_time": "flat",
        "tax_rate": "destination"
    }
}

If I refresh the page I can confirm that the setup status returns that onboarding is incomplete and on the paid_ads step:

{
  "status": "incomplete",
  "step": "paid_ads"
}

I left one comment about the unit test replacement as that's going to be confusing if revisited, so I'd suggest to commit a follow up change to fix the test.
Since the code is working as expected and the tests still pass I'll go ahead and mark this as approved.

'refund_tos_visible' => true,
'contact_info_visible' => true,
]
[]
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I wanted to mention here was that this test data is not a valid replacement. Because we no longer use the function checked_pre_launch_checklist the options are not fetched a fourth time.

We can confirm that by checking specifically for how many times it's called by using something like:

$this->options->expects( $this->exactly( 4 ) )->method( 'get' )

Which will fail with a message:

Expectation failed for method name is "get" when invoked 4 time(s).
Method was expected to be called 4 times, actually called 3 times.

So I'd suggest to actually check that it's invoked 3 times as well as remove the parameters and response for the fourth time.

		$this->options->expects( $this->exactly( 3 ) )->method( 'get' )
			->withConsecutive(
				[ OptionsInterface::MC_SETUP_COMPLETED_AT, false ],
				[ OptionsInterface::TARGET_AUDIENCE ],
				[ OptionsInterface::MERCHANT_CENTER, [] ]
			)->willReturnOnConsecutiveCalls(
				false,
				[
					'location'  => 'selected',
					'countries' => [ 'GB' ],
				],
				[]
			);

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏻 @kt-12 can you make these updates and request another review from me?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you @mikkamp and @joemcgill, I have addressed this comment.

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.

The PHPUnit updates look right and tests are still passing. Since this was already approved I'm going to go ahead and merge.

@joemcgill joemcgill merged commit 40bbf82 into feature/2492-remove-pre-launch-checklist Sep 5, 2024
15 checks passed
@joemcgill joemcgill deleted the feature/2494-remove-pre-launch-checklist-items branch September 5, 2024 16:45
@eason9487 eason9487 added the changelog: update Big changes to something that wasn't broken. label Nov 20, 2024
@eason9487 eason9487 changed the title Remove pre launch checklist items Streamline onboarding: Clean up the access to the pre-launch checklist in the mc/settings API Nov 20, 2024
@ianlin ianlin mentioned this pull request Nov 26, 2024
22 tasks
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: Clean up settings API for pre-launch checklist items
5 participants