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
44 stars 21 forks source link

[API Pull] Tweak Variations Notifications #2450

Closed puntope closed 1 month ago

puntope commented 1 month ago

Changes proposed in this Pull Request:

This PR tweaks the logic for notifications when handling variations. In particular:

Detailed test instructions:

  1. Checkout PR
  2. Test that creation/update/deletion of simple product works as always
  3. Create a Variable Product and generate variations for It
  4. Go to Woo - Status - Scheduled actions - That step should not schedule any notifications
  5. Asign price to some of the variations and publish the product
  6. Go to Woo - Status - Scheduled actions - See scheduled notifications create for the variations with price + the variable product
  7. Run the actions and see if they are logged in the gla log under Woo - Status - logs
  8. Edit one of the variations without price
  9. It doesn't schedule or log anything as it was not created yet
  10. Remove one of the variations without price
  11. It doesn't schedule or log anything as it was not created yet
  12. Edit one of the variations with price
  13. Go to Woo - Status - Scheduled actions - See scheduled notifications update for the variations with price + the variable product. Run them
  14. Remove the variation with price
  15. Go to Woo - Status - Scheduled actions - See scheduled notifications update for the variations with price + the variable product and one delete notification in the logs (directly without schedule) for the one you deleted with offer_id
  16. Set the variable product as DONT SYNC AND SHOW
  17. Go to Woo - Status - Scheduled actions - See scheduled notifications delete for the variations with price + the variable product
  18. Before running it. Go back and set the variable product as SYNC AND SHOW
  19. Go to Woo - Status - Scheduled actions - See scheduled notifications update for the variations with price + the variable product
  20. Run the delete notifications of the step 17
  21. See in the logs they are not logged (a filter prevented it, because we set is as visible in step 18)
  22. Run the tasks from step 19
  23. See them logged in the log
  24. Try to delete products and variations in different ways to see if everything is handled properly.

Additional details:

Changelog entry

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 80.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 64.7%. Comparing base (7ff8a9e) to head (f03a407). Report is 33 commits behind head on feature/google-api-project.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2450/graphs/tree.svg?width=650&height=150&src=pr&token=UROWUPF1LX&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce)](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) ```diff @@ Coverage Diff @@ ## feature/google-api-project #2450 +/- ## ============================================================== + Coverage 64.6% 64.7% +0.1% - Complexity 4552 4560 +8 ============================================================== Files 473 473 Lines 17757 17769 +12 ============================================================== + Hits 11478 11501 +23 + Misses 6279 6268 -11 ``` | [Flag](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2450/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | Coverage Δ | | |---|---|---| | [php-unit-tests](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2450/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | `64.7% <80.0%> (+0.1%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2450?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | Coverage Δ | | |---|---|---| | [src/Jobs/Notifications/ProductNotificationJob.php](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2450?src=pr&el=tree&filepath=src%2FJobs%2FNotifications%2FProductNotificationJob.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-c3JjL0pvYnMvTm90aWZpY2F0aW9ucy9Qcm9kdWN0Tm90aWZpY2F0aW9uSm9iLnBocA==) | `100.0% <100.0%> (ø)` | | | [src/Product/ProductHelper.php](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2450?src=pr&el=tree&filepath=src%2FProduct%2FProductHelper.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-c3JjL1Byb2R1Y3QvUHJvZHVjdEhlbHBlci5waHA=) | `92.1% <100.0%> (+0.2%)` | :arrow_up: | | [src/Product/SyncerHooks.php](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2450?src=pr&el=tree&filepath=src%2FProduct%2FSyncerHooks.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-c3JjL1Byb2R1Y3QvU3luY2VySG9va3MucGhw) | `90.0% <90.5%> (+5.3%)` | :arrow_up: | | [...Jobs/Notifications/AbstractItemNotificationJob.php](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2450?src=pr&el=tree&filepath=src%2FJobs%2FNotifications%2FAbstractItemNotificationJob.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-c3JjL0pvYnMvTm90aWZpY2F0aW9ucy9BYnN0cmFjdEl0ZW1Ob3RpZmljYXRpb25Kb2IucGhw) | `88.4% <37.5%> (-11.6%)` | :arrow_down: | ... and [3 files with indirect coverage changes](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2450/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce)
puntope commented 1 month ago
  • but when I restore the simple product, we send an UPDATE topic instead of a CREATE topic.

Good catch. Fixed here. https://github.com/woocommerce/google-listings-and-ads/pull/2450/commits/413c7c54312d0b2fa76b2b708d61016af9463540

puntope commented 1 month ago

I am unsure if this is ideal because we only updated one variation. Will Google make X (number of variations + parent product) REST API requests? It seems unnecessary to make so many requests. For example, if a product has 30 variations and only 1 is updated, we'll schedule 30 + 1 jobs, and the merchant will receive 31 API requests?

Seems like when we did activate the MC Push again we forgot to handle that https://github.com/woocommerce/google-listings-and-ads/blob/develop/src/Product/SyncerHooks.php#L180

I did some tweaks for it also. Now should be trigger per variation and parent only.

See https://github.com/woocommerce/google-listings-and-ads/pull/2450/commits/ff8468e3daadcdbb0b57214b2967b6c4e39ed284

puntope commented 1 month ago

Thanks @jorgemd24 for all your feedback. I applied some changes. Can you take another look?

puntope commented 1 month ago

As far as I can tell, we only need to notify the product ID, topic, and offer_id, so can't we just schedule the job to notify that? Or do we need the actual product?

The logic know uses get_item that needs the product from DB. We can refactor that in future iterations. Since DarkL is next week I would suggest to have it working even if the performance is not the optimal.

p.s I also added the check for WC_Product. This is ready for a new round.