API Pull - Notify when product changes #2190
Conversation
Release 2.5.13
Release 2.5.13 - Merge `trunk` to `develop`
…_paid_ads_button_click
…ing-setup-ads Add (budget and audiences) tracking for Onboarding completed with Ads
…ake its setup card always visible.
…e Ads account is connected.
…king PR #2174. Ref: - #2153 - #2174 (review)
…tion in step 4 of the onboarding.
…he page to load to proceed with assertions.
…oogle-ads-connection Change to require Google Ads connection during the onboarding
Include PHP 8.3 in tested versions for PHPunit
…rrect-in-purchase-event Fix item price in purchase event
Release 2.5.14
Release 2.5.14 - Merge `trunk` to `develop`
Update link for developer.woo.com
github-actions
bot
added
changelog: add
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feature/google-api-project #2190 +/- ##
==============================================================
+ Coverage 57.7% 58.6% +1.0%
- Complexity 4181 4211 +30
==============================================================
Files 454 456 +2
Lines 17950 16827 -1123
==============================================================
- Hits 10349 9864 -485
+ Misses 7601 6963 -638
Flags with carried forward coverage won't be shown. Click here to find out more.
|
puntope
changed the title
There was a problem hiding this comment.
Thanks, @puntope, for tackling this. It's running smoothly. I've dropped a few comments on the code and below are some doubts that I have:
I've noticed that when updating a variation, you receive the topic "product.updated" along with the variation's ID. I don't think it's an issue as you can call the WC REST API wc/v3/products/variation_id without explicitly stating it's a variation ID.
However, when creating a product, even as just a draft without any data, it triggers the notification endpoint. Is this expected behavior? It appears that with the pushing mechanism, we were bypassing these scenarios:
2023-12-28T13:23:04+00:00 DEBUG Automattic\WooCommerce\GoogleListingsAndAds\Product\WCProductAdapter::map_product_categories Product category (ID: 933): ["Uncategorized"]. 2023-12-28T13:23:04+00:00 DEBUG Automattic\WooCommerce\GoogleListingsAndAds\Product\BatchProductHelper::validate_and_generate_update_request_entries Skipping product (ID: 953) because it does not pass validation: {"woocommerce_id":953,"errors":["[description] This value should not be blank.","[imageLink] This value should not be blank.","[gtin] This value is not valid."]} 2023-12-28T13:23:06+00:00 ERROR Automattic\WooCommerce\GoogleListingsAndAds\Google\NotificationsService::notification_error Error sending notification for Item ID 1020 with topic product.created. {"code":"wpcom_partner_notifications_proxy_error","message":"Invalid x-woocommerce-topic","data":{"status":400}}
Another question that comes to my mind is, will we be notifying every product, regardless of its channel visibility? Or do you plan to implement some filtering in a following PR?
src/Google/NotificationsService.php
Outdated
| public const TOPIC_COUPON_CREATED = 'coupon.created'; | ||
| public const TOPIC_COUPON_DELETED = 'coupon.deleted'; | ||
| public const TOPIC_COUPON_UPDATED = 'coupon.updated'; | ||
| public const TOPIC_SHIPPING_SAVED = 'action.woocommerce_after_shipping_zone_object_save'; | ||
| public const TOPIC_SHIPPING_DELETED = 'action.woocommerce_delete_shipping_zone'; |
There was a problem hiding this comment.
Constants are misaligned.
src/Google/NotificationsService.php
Outdated
| if ( is_wp_error( $response ) ) { | ||
| $this->notification_error( $item_id, $topic, $response->get_error_message() ); | ||
| return false; | ||
| } | ||
|
|
||
| if ( wp_remote_retrieve_response_code( $response ) >= 400 ) { | ||
| $this->notification_error( $item_id, $topic, wp_remote_retrieve_body( $response ) ); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
What about:
if(is_wp_error( $response ) || wp_remote_retrieve_response_code( $response ) >= 400){
$error = is_wp_error( $response ) ? $response->get_error_message() : wp_remote_retrieve_body( $response );
$this->notification_error( $item_id, $topic, $error );
return false;
}
src/Google/NotificationsService.php
Outdated
| public const TOPIC_COUPON_CREATED = 'coupon.created'; | ||
| public const TOPIC_COUPON_DELETED = 'coupon.deleted'; | ||
| public const TOPIC_COUPON_UPDATED = 'coupon.updated'; | ||
| public const TOPIC_SHIPPING_SAVED = 'action.woocommerce_after_shipping_zone_object_save'; |
There was a problem hiding this comment.
Just curious, why the topics for shipping zones are so different from the rest?
There was a problem hiding this comment.
This is because of the Google Endpoint specification. They are using https://woocommerce.github.io/woocommerce-rest-api-docs/?shell#webhooks as reference.
action.woocommerce_after_shipping_zone_object_save means they are referring to woocommerce_after_shipping_zone_object_save action in WC.
There was a problem hiding this comment.
Maybe there is not a resource named shipping (yes they are product and coupon) or if it exists maybe they didn't know about it. In any case, the expect that one. action.woocommerce_after_shipping_zone_object_save
| } | ||
|
|
||
| /** | ||
| * Register a service. | ||
| */ | ||
| public function register(): void { | ||
| // only register the hooks if Merchant Center is connected and ready for syncing data. | ||
| // TODO: Potentially change this after API Pull is implemented as we don't need MC to be connected for the API Pull |
There was a problem hiding this comment.
I think we will still need the MC to be connected, but instead of pushing the data, they will pull the data.
There was a problem hiding this comment.
Mmm, I thought that authorizing the App and WPCOM Google can actually get data by themselves right?
There was a problem hiding this comment.
Yes, they will have access to the data however, from what I gather, they still require the MC ID, and the account needs to be connected.
| */ | ||
| public function get_mock() { | ||
| return $this->getMockBuilder( NotificationsService::class) | ||
| ->setMethods( [ 'do_request' ] ) |
There was a problem hiding this comment.
setMethods seems deprecated.
I also thought we should avoid notifying drafts. However, when running the tests I discovered the current batch syncer (MC push based) yes does actually load them I guess, I thought was expected and Google will handle that on their side (one advantage of doing Pull approach) . WHat do you think about this? |
I assume we will have filters on Google side avoiding access to data without channel visibility set as On. But yes we might implement that maybe. One question about it is: what happens if a merchant wants to remove the product from the Google App without deleting it? In case we only sync products with Channel Visibility as On, it would be impossible for the merchant to remove it from MC as setting channel visibility as Off will stop the notifications and hence Google won't be able to get the data anymore. I am thinking about some options here:
This is how is done in current implementation. |
|
Hi @jorgemd24 I applied most of the suggestions and answered your questions. In regards to the discussion about filtering the "not ready" products, I opened some discussions pcTzPl-1OX-p2#comment-2903 In my point of view, relying on the partner to handle all of that may be a valid solution, as well as doing it ourselves. So I prefer to wait for some input before spending time on the implementation. So in any case, it would be done in a future PR if that happens. If you agree, this is ready for another round. |
There was a problem hiding this comment.
Thanks @puntope for your comments! I'm aligned with the idea that ideally, the partner should manage those scenarios. When you look at the webhooks, they essentially send the data, and it's up to the consumer to decide how to handle it. However, I'm unsure, for instance, why we'd need to notify the partner if they prefer not to share certain products with Google. Nevertheless, with the filter we've put in place, when they request a product that isn't syncable, the response will be empty.
One question about it is: what happens if a merchant wants to remove the product from the Google App without deleting it? In case we only sync products with Channel Visibility as On, it would be impossible for the merchant to remove it from MC as setting channel visibility as Off will stop the notifications and hence Google won't be able to get the data anymore.
I am thinking about some options here:
1.We can maybe implement some kind of action when the product changes and call product.delete in case channel visibility is set as Off and product.create in case is set as On
2.We can rely on Google and on the WPCOM endpoints you created to be sure they don't load Channel Visibility "Off" products.
For option 1, let's see the opinions shared in the internal discussion you initiated 👍
Regarding option 2, in theory, when they use the WPCOM endpoints, if the channel visibility is switched off, they shouldn't receive the data. Hence, it should work as well. However, that merchant will receive unnecessary requests.
Changes proposed in this Pull Request:
Closes part of #2146
This PR adds logic for calling the Notification Partner endpoint when a Product changes.
In detail this PR:
Detailed test instructions:
trunksites-woocommerce-partner-notifications.phpin the PARTNERS variable (this is because we don't have yet the final Partner APP)Additional details:
woocommerce_delete_product_variationandwoocommerce_trash_product_variationas they are duplicated withtrashed_postanddeleted_postsrc/Infrastructure/GoogleListingsAndAdsPlugin.phpbut they are not introduced here. So they are out Scope this PRChangelog entry