woocommerce / google-listings-and-ads

Sync your store with Google to list products for free, run paid ads, and track performance straight from your store dashboard.
https://woo.com/products/google-listings-and-ads/
GNU General Public License v3.0
45 stars 21 forks source link

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

Open joemcgill opened 1 month ago

joemcgill commented 1 month ago

Part of https://github.com/woocommerce/google-listings-and-ads/issues/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

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

mikkamp commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

I don't know if it belongs in another separate issue or is more part of https://github.com/woocommerce/google-listings-and-ads/issues/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 commented 2 weeks ago

@ankitguptaindia this one is ready for QA.

joemcgill commented 1 week ago

@mikkamp this is ready for review.

joemcgill commented 1 week ago

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