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

Onboarding: Clean up settings API for pre-launch checklist items #2494

Closed
3 tasks
Tracked by #2458
joemcgill opened this issue Aug 5, 2024 · 7 comments · Fixed by #2555
Closed
3 tasks
Tracked by #2458

Onboarding: Clean up settings API for pre-launch checklist items #2494

joemcgill opened this issue Aug 5, 2024 · 7 comments · Fixed by #2555

Comments

@joemcgill
Copy link
Collaborator

joemcgill commented Aug 5, 2024

Part of #2458

This is a follow-up to #2492. Once the UI for the pre-launch checklist is removed, we can remove any backend functionality that is used for saving these options from the /wc/gla/mc/settings endpoint.

Acceptance Criteria

  • The schema for the /wp-json/wc/gla/mc/setup endpoint is updated to remove references to pre-launch fields: 'website_live', 'checkout_process_secure', 'payment_methods_visible', 'refund_tos_visible', and 'contact_info_visible' so those values no longer return.
  • Once the user has successfully added an address and validated a phone number, a GET request to the /wp-json/wc/gla/mc/setup endpoint should return paid_ads to show that the store_requirements step is completed.
  • The MerchantCenterSettings class is removed, since it's not in use.

Implementation Brief

The REST API controller for these settings is in src/API/Site/Controllers/MerchantCenter/SettingsController.php, specifically the get_schema_properties() method.

MerchantCenterService::get_setup_status() should be updated to remove the checked_pre_launch_checklist() requirement. MerchantCenterService::checked_pre_launch_checklist() is no longer needed and can be removed.

The MerchantCenterSettings class also references these settings but is not in use. We can remove that class as part of this work. See this comment.

Test Coverage

  • PHPUnit tests in tests/Unit/MerchantCenter/MerchantCenterServiceTest.php will need to be updated to remove the need to setup/verify pre-launch checks.
  • The endpoint may be used in E2E tests so those need to be cleaned up as well after removing this logic.
@joemcgill joemcgill changed the title Clean up settings API for pre-launch checklist items Onboarding: Clean up settings API for pre-launch checklist items Aug 5, 2024
@joemcgill joemcgill assigned joemcgill, mikkamp and eason9487 and unassigned joemcgill Aug 5, 2024
@mikkamp
Copy link
Contributor

mikkamp commented Aug 6, 2024

The REST API controller for these settings is in src/API/Site/Controllers/MerchantCenter/SettingsController.php

The MerchantCenterSettings class also references these settings. However it seems this class is intentionally unused as mentioned in PR #255

There are some classes added to the \Automattic\WooCommerce\GoogleListingsAndAds\Value namespace. These are not currently being used, but I would like to see them used in the future. They can be reviewed or ignored, or I can pull them out of this PR and save them for another time since they are not used.

Since it's gone unused for a while, my vote would be towards removing it, we can always revert if it's needed again.

@mikkamp
Copy link
Contributor

mikkamp commented Aug 6, 2024

I don't know if it belongs in another separate issue or is more part of #2492

Both the classes PolicyComplianceCheckController and PolicyComplianceCheck are solely used to confirm whether these pre-launch items can be marked as completed. So the UI will call mc/policy_check to check what values they return. This won't be needed/used anymore.

@joemcgill
Copy link
Collaborator Author

I don't know if it belongs in another separate issue or is more part of #2492

#2458 mentions that we may use them later:

We can check these things post onboarding and surface to merchants if they’re missing something, like we currently do with MC issues.

For that reason, I'm thinking that we not remove the policy check functionality as a part of this issue, however, removing the unused MerchantCenterSettings class makes sense.

@joemcgill joemcgill added the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Aug 8, 2024
@kt-12 kt-12 self-assigned this Aug 20, 2024
@macka61 macka61 removed the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Aug 21, 2024
@kt-12 kt-12 assigned joemcgill and unassigned kt-12 Aug 23, 2024
@joemcgill
Copy link
Collaborator Author

@ankitguptaindia this one is ready for QA.

@joemcgill
Copy link
Collaborator Author

@mikkamp this is ready for review.

@joemcgill joemcgill assigned kt-12 and unassigned mikkamp Sep 4, 2024
@joemcgill
Copy link
Collaborator Author

@kt-12 just some small feedback to handle on this PR and this should be good to go.

@mikkamp
Copy link
Contributor

mikkamp commented Dec 2, 2024

Closing this as completed since it was part of the 2.9 release.

@mikkamp mikkamp closed this as completed Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants