Adds product/save to message queue #144
Conversation
- Adds a getWebhooks method/endpoint to the Reclaim interface for querying the customizable webhooks. - Adds the observer that runs when a product is saved. - Adds the Cron class for managing product data that needs to be synced to Klaviyo. - Admin HTML for configuring the webhook. - Modify the webhook helper so we only encode the json once.
- Remove some comments. - Reduce if statements to a single line.
- Move store_ids to top level property.
- In the observer we'll just get the cateroy ids. - In the cron job we'll look up the category names based on the ids so this code runs in the background instead of when the user is saving a product.
Cron/ProductsTopic.php
Outdated
| * @param string $payload | ||
| * @return array | ||
| */ | ||
| public function addCategoryNames(string $payload): array |
There was a problem hiding this comment.
Over here, we will be iterating over each categoryId for all payloads passed, so we will be running the for loop 500 times, if categoryIds, exist and possibly loading duplicate categroyModels since it is likely more than one product will have a category repeated. Can we make it such that we create a class level attribute to have Ids and categoryNames associated so don't duplicate the effort?
#142 (comment) > as advised here?
Or do you think it is worth having a method in the helper class which can be used by both the Cron?
Helper/Webhook.php
Outdated
| @@ -66,24 +65,23 @@ public function makeWebhookRequest($webhookType, $data, $klaviyoId=null) | |||
| $err = curl_errno($curl); | |||
|
|
|||
| if ($err) { | |||
| $this->_klaviyoLogger->log(sprintf('Unable to send webhook to %s with data: %s', $url, json_encode($data))); | |||
| $this->_klaviyoLogger->log(sprintf("Unable to send webhook to $url with data: $data")); | |||
There was a problem hiding this comment.
super nit:
| $this->_klaviyoLogger->log(sprintf("Unable to send webhook to $url with data: $data")); | |
| $this->_klaviyoLogger->log("Unable to send webhook to $url with data: $data"); |
Helper/Webhook.php
Outdated
| return $response; | ||
| } | ||
|
|
||
| /** | ||
| * @param array data | ||
| * @param string data |
There was a problem hiding this comment.
Could we add a description to this method?
There was a problem hiding this comment.
Does this work?
* @param string $data json payload used to create hmac signature
There was a problem hiding this comment.
| * @param string data | |
| * Returns an HMAC signature for webhooks | |
| * @param string data |
| $product_id = $product->getId(); | ||
|
|
||
| $product_info = array( | ||
| 'store_ids' => $product->getStoreIds(), |
There was a problem hiding this comment.
Are all array_keys supposed to start lower case/upper case? Could we standardize across the payload?
There was a problem hiding this comment.
I only use uppercase letters on some of the properties because Klaviyo wants them that way so this avoids normalizing the payload in the app. otherwise I user lowercase where I can. Let me know if that's alright or not!
There was a problem hiding this comment.
Is there any restriction on making other start upper case too? Just looking for standardize keys. it's a nitpick so if it is too much work to accommodate this, we can skip it
- Change the response from the getWebhooks endpoint so that it returns an array of arrays instead of an array of associative arrays. This makes it unpackable app-side.
Helper/ScopeSetting.php
Outdated
| return $registeredWebhooks = [ | ||
| ['product/delete', $this->getProductDeleteWebhookSetting()], | ||
| ['product/save', $this->getProductSaveWebhookSetting()], | ||
| ]; |
There was a problem hiding this comment.
Is there value in returning topics that aren't enabled? Thinking about other platforms for which we subscribe to certain webhooks (Shopify, WooCommerce), I'm pretty sure the endpoints that return this info only return the topics to which we are registered rather than a comprehensive list of all available.
I think it might be cleaner (and more easily extended in the future) to format this as an indexed array of associative arrays, similar to what those other platforms return. ex:
[
[
'topic' => 'product/delete',
'enabled' => $this->getProductDeleteWebhookSetting(),
],
[
'topic' => 'product/save',
'enabled' => $this-> getProductSaveWebhookSetting(),
]
]
There was a problem hiding this comment.
We could also allow for a param in this request to optionally fetch all available webhooks vs those that are enabled
There was a problem hiding this comment.
I'd be fine with only sending back the enabled webhooks if we want to go that direction. In the app code that hits this endpoint I essentially filter out the ones that aren't enabled anyway. If we're only going to return the enabled ones then I'm not sure we need anything other than a list of the topics though?
There was a problem hiding this comment.
Yea, I guess since this is only for an inspector task it's probably fine however. Is there any other info stored about this? like when this setting was last updated for example? That could be useful for troubleshooting but may not be available
There was a problem hiding this comment.
It is actually used for more than the inspector task, it is used in setup_webhooks to update the integration blob setting registered_webhooks. Then in the product sync I have it check that before making any queries for product info.
There was a problem hiding this comment.
Ahh, forgot about that. In that case if it's used in two different scenarios, I'd let the app code contain the logic to process the response/format the value to be saved and return as much info here for troubleshooting purposes which can be accessed via the inspector task.
There was a problem hiding this comment.
I'd love to see this return an indexed array containing an associative array for each webhook topic. Makes the logic in the app way easier to read instead of relying on the index for the topic and enabled boolean.
There was a problem hiding this comment.
sorry for the confusion around this! should be updated now though to what you were describing.
LGTM!
This is the next PR in the message queue feature that @siddwarkhedkar and I have been working on. In this PR I add the observer and cron jobs for syncing product data to Klaviyo.
Also in this PR is a new /webhooks endpoint for querying which configurable webhooks are enabled and disabled. For now it is just the product/delete and product/save webhooks.
I also changed the makeWebhookRequest function to accept a json encoded string as its second argument instead of an array. This saves us from needing to call json_encode on the argument three times, the argument was never used as an array in the method. This necessitated me updating the calls to makeWebhookRequest to ensure it always receives a json encoded string as the second argument.