Add Shipping notifications #2252
Conversation
github-actions
bot
added
changelog: add
There was a problem hiding this comment.
Thanks @puntope for adding this new notification. I left a couple of minor comments about the code.
As discussed for now we don't send Item IDs and just send a generic "shipping.update" method with no Item ID
Can you point me to that discussion? What I see is that without that ID, Google would have to make multiple requests to figure out which Shipping Zone has been modified.
When I send a request to wc/v3/shipping/zones, it only gives me the Shipping Zone name without details about the shipping zone methods. So, if a shipping zone method gets updated, Google would have to make separate calls to wc/v3/shipping/zones/ZONE_ID/methods for each zone.
Additionally, what do you think in adding tests for the ShippingNotification Job?
I'm guessing there'll be a follow-up PR for create/delete notifications, is that correct?
src/Google/NotificationsService.php
Outdated
| * @param int $item_id | ||
| * @param string $topic | ||
| * @param string $topic The topic to use in the notification. | ||
| * @param int|null $item_id Tme item ID to notify. It can be null for topics that doesn't need Item ID |
src/Shipping/SyncerHooks.php
Outdated
| /** | ||
| * @var NotificationsService $notifications_service | ||
| */ | ||
| protected $notifiations_service; |
There was a problem hiding this comment.
Typo: $notifiations_service -> $notifications_service
src/Shipping/SyncerHooks.php
Outdated
| @@ -128,7 +143,13 @@ protected function handle_update_shipping_settings() { | |||
| if ( $this->already_scheduled ) { | |||
| return; | |||
| } | |||
| $this->update_shipping_job->schedule(); | |||
|
|
|||
| if ( $this->notifiations_service->is_enabled() ) { | |||
There was a problem hiding this comment.
WDYT if we also add this checking in the method can_schedule()?
There was a problem hiding this comment.
Adjusted here 34e1329
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feature/google-api-project #2252 +/- ##
==============================================================
+ Coverage 58.8% 59.0% +0.3%
- Complexity 4249 4276 +27
==============================================================
Files 458 460 +2
Lines 16902 18041 +1139
==============================================================
+ Hits 9935 10653 +718
- Misses 6967 7388 +421
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Added here a98ca65 |
This was discussed in the dev call as we decided to operate on ourselves and wait to google for feedback. (instead of asking Google what to do beforehand). In my 1:1 with Mik we decided to just send I will open a thread in P2 to discuss further. We can add them in future PRs I created tho some example PR with that approach. #2256
I suggest to go for now with this PR and discuss further in P2. I will create the discussion tomorrow. |
There was a problem hiding this comment.
Thanks @puntope for the adjustments and the tests.
However, it comes with more questions like:
Shall we also add notifications for methods?
How do we handle shipping class updates?
Do we support more options than flat or free shipping?
I feel like we should at least include the Shipping Zone ID to avoid unnecessary requests. I don't really see the need to add notifications for methods or the shipping class. For instance, if I query wc/v3/shipping/zones/ZONE_ID/methods, I can retrieve all the methods for that zone ID, making it easier to tell which method has changed.
I suggest to go for now with this PR and discuss further in P2. I will create the discussion tomorrow.
Okay let's do that!
Changes proposed in this Pull Request:
This PR implement Notifications for Shipping Updates.
As discussed for now we don't send Item IDs and just send a generic "shipping.update" method with no Item ID
Detailed test instructions:
gla/jobs/notifications/shipping/process_itemappearingshipping.updatehappeningAdditional details:
Changelog entry