Merchant Center Settings Sync #255

JPry · 2021-02-25T23:10:43Z

Changes proposed in this Pull Request:

This PR facilitates saving of our settings to the Merchant Center via the API. Here are the notable components:

The \Automattic\WooCommerce\GoogleListingsAndAds\API\Google\Settings class – This class contains all the logic needed to synchronize settings with Google. This is accomplished via two public methods: sync_shipping() and sync_taxes().
The \Automattic\WooCommerce\GoogleListingsAndAds\API\Site\Controllers\MerchantCenter\SettingsSyncController class – This class adds an API endpoint to trigger the settings to be synchronized with Google. The endpoint is:

POST /wp-json/wc/gla/mc/settings/sync

This endpoint does not take any data. It uses the data already saved to the database to generate the request to Google.

The endpoint handles calling the methods mentioned in the Settings class above. It will also trigger a new action gla_mc_settings_sync. This action can be used elsewhere in the codebase to facilitate other activity that needs to be handled after settings have been sent to Google.

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.

Part of #149

JPry · 2021-03-03T22:48:04Z

There's one outstanding question: How does this setting impact what we send to Google?

jconroy · 2021-03-03T22:58:48Z

How does this setting impact what we send to Google?

Ok refreshed my memory - the idea is this would add a post code and estimated delivery value to the conversion tag. So I don’t think you need to do anything here"

(We'll likely end up removing this functionality / delaying it)

jconroy · 2021-03-03T23:18:15Z

Looks like this doesn't include tax settings yet

jconroy · 2021-03-04T09:00:20Z

I've updated the main PR description to indicate this is currently only a part fix for #149

From what I can tell it still needs to be updated to sync tax settings at a minimum.

We also still need to be able to do the following

update mc_setup_completed_at timestamp/option
initiate product syncing

We may need an additional approach that covers all the things - as it is currently, it doesn't appear to do enough for the complete setup button.

@layoutd sorry to take you off other items, could you please jump in to review and lend a hand on this to get it across the line as a priority (given you've looked at tax and shipping settings during our research spikes). Based on a convo with @JPry today I don't think he had anything else in to push up so I don't believe there would be any stepping on toes if you were to make some additions before he jumps on (famous last words I guess).

layoutd

I was reviewing a bit earlier, but it looks like you're still cranking along so I'll just leave a few observations and notes.

Works well for me, syncs the shipping rates with the MC account I linked. Hopefully one-way sync won't be problematic for users who already have an MC account set up and are just linking it to the extension (it would overwrite all their configuration). Should this be considered or at least the user notified that any current settings will be overwritten (if they don't choose the I'll set this up manually in the Merchant Center option)? Maybe @j111q knows?

A few other notes:

This is the first time I've explicitly considered add vs share in the ServiceProviders; it may be worthwhile to audit the adds and see if any would be better as a share.
Are exception messages generally translatable? I know in MC and Ads a lot of them are, but it looks like none of the native Exceptions are.
The exception when not linked to an MC account is a bit janky, not sure if some of the info should be going to the debugger before returning a cleaner error:

{
    "code": "gla_setting_sync_error",
    "message": "{\n  \"error\": {\n    \"code\": 401,\n    \"message\": \"User cannot access account 0\",\n    \"errors\": [\n      {\n        \"message\": \"User cannot access account 0\",\n        \"domain\": \"content.ContentErrorDomain\",\n        \"reason\": \"auth/account_access_denied\"\n      }\n    ]\n  }\n}\n",
    "data": {
        "status": 500,
        "message": "{\n  \"error\": {\n    \"code\": 401,\n    \"message\": \"User cannot access account 0\",\n    \"errors\": [\n      {\n        \"message\": \"User cannot access account 0\",\n        \"domain\": \"content.ContentErrorDomain\",\n        \"reason\": \"auth/account_access_denied\"\n      }\n    ]\n  }\n}\n"
    }
}

vs

google-listings-and-ads/src/API/Google/Proxy.php

Lines 269 to 274 in c58f132

    
           } catch ( ApiException $e ) { 
        
           	do_action( 'gla_ads_client_exception', $e, __METHOD__ ); 
        
           	/* translators: %s Error message */ 
        
           	throw new Exception( sprintf( __( 'Error retrieving accounts: %s', 'google-listings-and-ads' ), $e->getBasicMessage() ) ); 
        
           }

src/API/Google/Settings.php

j111q · 2021-03-05T04:32:07Z

Should this be considered or at least the user notified that any current settings will be overwritten (if they don't choose the I'll set this up manually in the Merchant Center option)? Maybe @j111q knows?

Hmmm good question here. I'm assuming it will be pretty clear they're configuring something new and we don't need to explicitly call it out that we'll be overwriting any existing settings, but who knows if that assumption is correct. 🤔 Let's leave it as is for now - I feel like we're already bombarding them with a lot of tiny text - and let me add it into the Knowledge Base as documentation: Any existing settings will be overwritten by your new configurations in WooCommerce or something.

And let me add this to our feedback sheet as one thing to consider for the next iteration. Thanks @layoutd !

jconroy · 2021-03-05T05:25:30Z

I'll set this up manually in the Merchant Center

I haven't checked in the code, but just to confirm - if the user picks the manual options we definitely don't want to override anything.

layoutd · 2021-03-05T09:11:13Z

I haven't checked in the code, but just to confirm - if the user picks the manual options we definitely don't want to override anything.

Yeah, this is taken into consideration and I suppose it will be possible to update the shipping rates later on in case a user realizes they've overwritten their previous manual setup.

jconroy · 2021-03-05T20:40:54Z

Thanks @layoutd! @JPry can you post an update before the end of your day pretty please? (So I know where things are at come Monday morning)

JPry · 2021-03-06T03:36:35Z

@layoutd Thanks for the feedback!

This is the first time I've explicitly considered add vs share in the ServiceProviders; it may be worthwhile to audit the adds and see if any would be better as a share.

To provide more context, the add method is used when a new instance of the class should be returned whenever the container retrieves it. The share method is used when we always want to return the same instance of the class wherever it is used.

I agree an audit is probably needed, but with that in mind I think we should generally prefer using add() unless we know we'll need the same instance of a class for some reason.

Are exception messages generally translatable? I know in MC and Ads a lot of them are, but it looks like none of the native Exceptions are.

My opinion is that exception messages generally shouldn't be translated. The exception (😅 no pun intended!) is when they're conveying information to the end user.

Many of our exceptions are essentially guarding against bad development practices, or handling errors that we can't control. If they still get passed to the end user in some form, I'm OK with them being wrapped with a translated message that says "here's the error message".

One of the reasons I think that exceptions generally shouldn't be translated is because it aids in users, support, and developers in easily tracking down problems. Trying to figure out where in the code a translated exception is thrown can be challenging. But of course there are times when I think the messages should be translated, and that's OK.

The exception when not linked to an MC account is a bit janky, not sure if some of the info should be going to the debugger before returning a cleaner error:

I've added some extra handling in 8b42ab2 to try to help with this.

JPry · 2021-03-06T04:02:57Z

@jconroy

✅ From what I can tell it still needs to be updated to sync tax settings at a minimum.

update mc_setup_completed_at timestamp/option

initiate product syncing

I've added an action gla_mc_settings_sync that can be utilized for this purpose with separate handlers. Should those items be added to this PR or should we add them separately?

layoutd

Nice! My shipping and tax settings were synced correctly generally. Is the "Free shipping under X" setting correct (see comment)?

The only other notes I had:

I assume hardcoding the LocationID / Parent ID from Google's Geotargeting CSV into LocationIDTrait is OK since the state codes won't change?
Looking at the shipping services in the MC panel, there's one small difference (but not particularly important). When you create a "Flat Rate" in the panel, it shows in the "Cost" section:

But for whatever reason, when we create a flat rate in the extension with the API it still ends up displaying as a rate table ("Cost" section):

I was fudging around with it in the Content API tester but even when I used shippingsettings.update with the exact same data as provided in shippingsettings.get, the manual shipping service also ended up switching to a rate table, so I haven't seen any way to change it.

layoutd · 2021-03-07T16:44:37Z

src/API/Google/Settings.php

+			$services[] = $this->create_main_service( $country, $currency, $rate );
+
+			if ( $this->has_free_shipping_option() ) {
+				$services[] = $this->create_free_shipping_service( $country, $currency );


This creates two services for the country I think, is that correct?

Also, I think the free shipping service is created with a minimum price of 0, but the Figma shows the user selecting free shipping above a certain price.

In the MC panel, this ends up being done with a rate table:

This creates two services for the country I think, is that correct?

Yes. I didn't see another way to do it. It looks like multiple rates within the same service need some kind of shipping label on the products to distinguish the rates.

Also, I think the free shipping service is created with a minimum price of 0, but the Figma shows the user selecting free shipping above a certain price.

Ah yes, good catch. I'll get that fixed up.

It looks like multiple rates within the same service need some kind of shipping label on the products to distinguish the rates.

When I set it up in the panel, it created this rate group, where I specified "20 € shipping up to 100 €, then free above that". I guess it's set up as a rate table (mainTable) with rowHeaders->prices and rows->cells, but if the double services work, then that's definitely easier.

jconroy · 2021-03-08T07:03:14Z

Should those items be added to this PR or should we add them separately?

I don't mind, they're a priority for today.

JPry · 2021-03-08T15:59:59Z

@layoutd

I assume hardcoding the LocationID / Parent ID from Google's Geotargeting CSV into LocationIDTrait is OK since the state codes won't change?

Correct, these aren't going to be changing. Since their CSV file is quite large with all possible location IDs, I think it makes more sense to just copy the portion we need.

If we ever get to the point of adding more locations, we could revisit this.

JPry · 2021-03-08T22:26:51Z

@jconroy @layoutd I've made some updates here to address feedback. I've also added the code to kick off the product sync, and I confirmed that the sync happened on my test site with my test account. It also sets the mc_setup_completed_at time.

jconroy

✅ Confirmed gla_mc_setup_completed_at updated
✅ Confirmed products submitted
✅ No errors in my PHP logs or JS console errors
🟠 confirmed shipping settings updated - probably need an extra check for the use "manual" option
🟠 ✅ I couldn't tell if taxes were updated on my account (Update got it working)

Can also confirm that if we upload settings it definitely deletes/overrides any existing services set 😅 So need to be careful about this

I used the UI (from trunk) and it seemed to work well for my testing

Shipping Settings

I think this is related to one of @layoutd 's points - it's bit of a bummer that there doesn't seem to be an easy way to use the equivalent "Basic Cost settings" (or is there 🤔 - I'm guessing not by the looks)

One issue I hit is... if I select that I have simple shipping, add a value etc. and then decide "No, wait my setup is too complex, I'll just do things manually" and change my choice - the shipping settings are still updated (and override what I have already - confirmed this as well).

I think we need to an extra check before syncing to make sure that "shipping_rate" is not actually set to "manual". Looks like we just check if there are rates in the DB.

Alternatively, I think we'd need to clear rates in the DB if the manual option is chosen.

Also a couple of questions for @j111q

1. we have the option for

I also offer free shipping for all countries for ** products** over a certain price.

Looks like the implementation for this in the MC dashboard is actually worded as

Minimum order value (optional)

Another example when using their "basic settings" - it also mentions order value

@j111q did you come across something in the MC that allows free shipping for products over a certain price? Or should we change this to say "orders" for consistency? (Not really a blocker for this PR if decide to change the wording)

2. I think the shipping times and rates are pretty closely coupled. If a Merchant decides to set up shipping rates manually, I'm thinking we don't really need to ask them to tell us shipping times. This isn't specific for this PR, we can have a think and do this a follow-up but at the moment I think we still ask for times when the manual option is set for rates and we won't actually do anything as we don't create any shipping services without a rate set.

Tax Settings

~~I don't get any errors but mine just shows up as~~ Got it, see below

jconroy · 2021-03-09T05:16:38Z

🟠 I couldn't tell if taxes were updated on my account

Ok I can see what is happening - there is a disconnect between the UI and handling here. The UI shows the onboarding US tax settings if we are targeting the US but in this PR we only sync tax settings if the store is "located" in the USA.

Got it working and confirmed it synced.

Something for the feedback/ideas doc @j111q - do we want to add an option here for "Is shipping and handling taxable?" To cover the checkbox in this screenshot >>

JPry · 2021-03-09T17:43:38Z

The UI shows the onboarding US tax settings if we are targeting the US but in this PR we only sync tax settings if the store is "located" in the USA.

Which are we actually wanting to go with? I did it this way thinking that it was only relevant for stores that are located in the US vs. shipping to the US. But if that was just a misunderstanding on my end we can change it.

I think we need to an extra check before syncing to make sure that "shipping_rate" is not actually set to "manual". Looks like we just check if there are rates in the DB.

👍🏻

jconroy · 2021-03-09T23:17:34Z

Merging. I'm going to create a couple follow up issues to investigate as we do more testing:

Adjust the UI to only show the tax settings if the store is also located in the USA (not simply targeting the USA)
We've confirmed the shipping settings are synced to the MC but there is still some uncertainty if the results of that are correct in the listings themselves - needs further testing - which the tranches will help with.

j111q · 2021-03-10T06:13:54Z

@j111q did you come across something in the MC that allows free shipping for products over a certain price? Or should we change this to say "orders" for consistency? (Not really a blocker for this PR if decide to change the wording)

The reason why I used "products" and not "orders" is because this is relevant for the shipping fee that will be displayed in the product listing in Google. I think that's a fair point to align with Google and change it to "orders" - which the merchant will be more familiar with, offering free shipping over $x in an order - and I suppose Google does the work of translating that "free shipping over $x" to individual product listings which are priced over "$x".

I think the shipping times and rates are pretty closely coupled. If a Merchant decides to set up shipping rates manually, I'm thinking we don't really need to ask them to tell us shipping times. This isn't specific for this PR, we can have a think and do this a follow-up but at the moment I think we still ask for times when the manual option is set for rates and we won't actually do anything as we don't create any shipping services without a rate set.

I'm trying to redesign this now, to combine "shipping rates and times" together. Will hopefully update soon!

j111q · 2021-03-10T09:47:26Z

Okay, here is the updated design for the "combined shipping rates and times": https://d.pr/i/aPCjIt

I tried to keep it as close to the original as possible, while still making it user-friendly - it IS an intimidating setting.

I need some help creating follow-up issues 'cause I'm not sure where they'd go at this point 😅 Appreciate any feedback or questions on the design though.

Also, if this design change doesn't make it in for Tranche 2 (likely won't, since we're pushing for that this week), I think it's fine. We can just make a special note of it to testers and ask them to choose manual for both or neither.

tomalec · 2021-03-10T13:53:04Z

@j111q I'm still in the process of setting the shipping rate and time for edit campaign flow. I guess, your change above applies also there. Should I try to incorporate it there, or for v0, let's leave it as is in Setup flow, to keep it consistent? (keeping it consistent, naturally would go slightly faster)

j111q · 2021-03-11T02:34:01Z

Thanks for calling that out @tomalec - yes, the key is that it should be consistent, so whenever this is incorporated in the Setup flow, it should also be changed in the Edit flow. 🙏 Thank you!

jconroy · 2021-03-11T05:48:26Z

Consistent is ideal - If you start making the changes in the edit flow now @tomalec can we then port them back to the main onboarding flow when done? (That's a simplistic view of it obviously - but I believe that is roughly the approach you are taking updating components there at the moment anyway) - we can chat tonight.

tomalec · 2021-03-11T18:34:21Z

Created a follow-up issue to track redesigned shipping rates & times #335

JPry self-assigned this Feb 25, 2021

JPry force-pushed the feature/merchant_settings_sync branch from f6eb5bf to edf06a4 Compare March 2, 2021 21:52

JPry changed the title ~~[WIP] Merchant Center Settings Sync~~ Merchant Center Settings Sync Mar 3, 2021

JPry requested a review from a team March 3, 2021 22:38

layoutd reviewed Mar 4, 2021

View reviewed changes

src/API/Google/Settings.php Show resolved Hide resolved

layoutd reviewed Mar 5, 2021

View reviewed changes

src/API/Google/Settings.php Outdated Show resolved Hide resolved

ecgan mentioned this pull request Mar 5, 2021

Setup MC Complete Setup: Display product feed page with submission success guide #295

Merged

JPry added merchant center priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. labels Mar 6, 2021

layoutd reviewed Mar 7, 2021

View reviewed changes

JPry added 8 commits March 8, 2021 15:16

Create the Settings sync class

5166b61

Remove default limit from query

af0d386

Register the Settings class in the container

b814045

Create the settings sync API controller

4e2bc96

Include a response for setting sync

4478388

Type-hint against ValueInterface

fc030b9

Separate method for the time object

656e5d8

Import classes used in the logger

9d2ff57

JPry added 6 commits March 8, 2021 15:17

Add action when exception is caught

5ce3248

Clean up exception handling a bit for setting save

9f0149e

Add an action when a settings sync has taken place

db0784f

Use the correct value for free shipping minimum

6a85d97

Create ContainerAware trait and interface

acae3d8

Create class to handle starting the product sync

8fc1c4e

JPry force-pushed the feature/merchant_settings_sync branch from 824253e to 099c576 Compare March 8, 2021 20:17

Fix spacing for ConnectionTest file

5815210

JPry force-pushed the feature/merchant_settings_sync branch from 099c576 to 5815210 Compare March 8, 2021 20:23

JPry added 2 commits March 8, 2021 15:37

Add class to update the setup completed option

901f40f

Add initial version of MerchantCenterSettings value object

86f20ac

jconroy mentioned this pull request Mar 8, 2021

Set default destinations for Google products submission #287

Closed

jconroy reviewed Mar 9, 2021

View reviewed changes

Check for whether shipping settings should be synced or not

eb42f33

jconroy merged commit e5f2fb6 into trunk Mar 9, 2021

jconroy deleted the feature/merchant_settings_sync branch March 9, 2021 23:19

This was referenced Mar 10, 2021

Confirm Shipping Settings work as expected #312

Closed

Also check store location before showing tax settings #313

Closed

tomalec mentioned this pull request Mar 11, 2021

Remove auto-save logic from EditFreeCampaign's shipping rate and tax configuration #326

Merged

3 tasks

mikkamp mentioned this pull request Aug 6, 2024

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

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merchant Center Settings Sync #255

Merchant Center Settings Sync #255

JPry commented Feb 25, 2021 •

edited

Loading

JPry commented Mar 3, 2021

jconroy commented Mar 3, 2021 •

edited

Loading

jconroy commented Mar 3, 2021

jconroy commented Mar 4, 2021 •

edited

Loading

layoutd left a comment

j111q commented Mar 5, 2021 •

edited

Loading

jconroy commented Mar 5, 2021 •

edited

Loading

layoutd commented Mar 5, 2021

jconroy commented Mar 5, 2021

JPry commented Mar 6, 2021 •

edited

Loading

JPry commented Mar 6, 2021

layoutd left a comment

layoutd Mar 7, 2021

JPry Mar 8, 2021

layoutd Mar 8, 2021

jconroy commented Mar 8, 2021

JPry commented Mar 8, 2021

JPry commented Mar 8, 2021

jconroy left a comment •

edited

Loading

jconroy commented Mar 9, 2021 •

edited

Loading

JPry commented Mar 9, 2021

jconroy commented Mar 9, 2021 •

edited

Loading

j111q commented Mar 10, 2021 •

edited

Loading

j111q commented Mar 10, 2021 •

edited

Loading

tomalec commented Mar 10, 2021 •

edited

Loading

j111q commented Mar 11, 2021

jconroy commented Mar 11, 2021 •

edited

Loading

tomalec commented Mar 11, 2021

	} catch ( ApiException $e ) {
	do_action( 'gla_ads_client_exception', $e, __METHOD__ );

	/* translators: %s Error message */
	throw new Exception( sprintf( __( 'Error retrieving accounts: %s', 'google-listings-and-ads' ), $e->getBasicMessage() ) );
	}

Merchant Center Settings Sync #255

Merchant Center Settings Sync #255

Conversation

JPry commented Feb 25, 2021 • edited Loading

Changes proposed in this Pull Request:

JPry commented Mar 3, 2021

jconroy commented Mar 3, 2021 • edited Loading

jconroy commented Mar 3, 2021

jconroy commented Mar 4, 2021 • edited Loading

layoutd left a comment

Choose a reason for hiding this comment

j111q commented Mar 5, 2021 • edited Loading

jconroy commented Mar 5, 2021 • edited Loading

layoutd commented Mar 5, 2021

jconroy commented Mar 5, 2021

JPry commented Mar 6, 2021 • edited Loading

JPry commented Mar 6, 2021

layoutd left a comment

Choose a reason for hiding this comment

layoutd Mar 7, 2021

Choose a reason for hiding this comment

JPry Mar 8, 2021

Choose a reason for hiding this comment

layoutd Mar 8, 2021

Choose a reason for hiding this comment

jconroy commented Mar 8, 2021

JPry commented Mar 8, 2021

JPry commented Mar 8, 2021

jconroy left a comment • edited Loading

Choose a reason for hiding this comment

jconroy commented Mar 9, 2021 • edited Loading

JPry commented Mar 9, 2021

jconroy commented Mar 9, 2021 • edited Loading

j111q commented Mar 10, 2021 • edited Loading

j111q commented Mar 10, 2021 • edited Loading

tomalec commented Mar 10, 2021 • edited Loading

j111q commented Mar 11, 2021

jconroy commented Mar 11, 2021 • edited Loading

tomalec commented Mar 11, 2021

JPry commented Feb 25, 2021 •

edited

Loading

jconroy commented Mar 3, 2021 •

edited

Loading

jconroy commented Mar 4, 2021 •

edited

Loading

j111q commented Mar 5, 2021 •

edited

Loading

jconroy commented Mar 5, 2021 •

edited

Loading

JPry commented Mar 6, 2021 •

edited

Loading

jconroy left a comment •

edited

Loading

jconroy commented Mar 9, 2021 •

edited

Loading

jconroy commented Mar 9, 2021 •

edited

Loading

j111q commented Mar 10, 2021 •

edited

Loading

j111q commented Mar 10, 2021 •

edited

Loading

tomalec commented Mar 10, 2021 •

edited

Loading

jconroy commented Mar 11, 2021 •

edited

Loading