-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds product/save to message queue #144
Open
jordanallain
wants to merge
15
commits into
develop
Choose a base branch
from
202111_product_save_webhook
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
5aba402
Adds product/save to message queue
jordanallain ff657f6
Remove comments
jordanallain b60705e
Reduce conditional to single line
jordanallain a140aa6
Whitespace
jordanallain 43c53ee
Use json_encode for makeWebhookRequest's second argument
jordanallain 905c644
Fix order of paren and bracket
jordanallain 682cc17
Restructure product/save payload
jordanallain fcfe92b
Nest categories under product key
jordanallain b9c26fc
Sidd review 1 of 2
jordanallain 67d5e2d
Interpolate with double quotes
jordanallain d2b1ea9
Move the category name hydration into cron
jordanallain dae44cb
Sidd feedback 2
jordanallain 2699a28
Respond with array of arrays in getWebhooks
jordanallain cd37f4d
Scott feedback
jordanallain 440ebc8
Update capitalization of filename
jordanallain File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
<?php | ||
|
||
namespace Klaviyo\Reclaim\Cron; | ||
|
||
use Klaviyo\Reclaim\Helper\CategoryMapper; | ||
use Klaviyo\Reclaim\Helper\Logger; | ||
use Klaviyo\Reclaim\Model\SyncsFactory; | ||
use Klaviyo\Reclaim\Model\ResourceModel\Products; | ||
use Klaviyo\Reclaim\Model\ResourceModel\Products\CollectionFactory; | ||
|
||
class ProductsTopic | ||
{ | ||
/** | ||
* Klaviyo Logger | ||
* @var Logger | ||
*/ | ||
protected $_klaviyoLogger; | ||
|
||
/** | ||
* Klaviyo helper for mapping category ids to names | ||
* @var CategoryMapper $categoryMapperactory | ||
*/ | ||
protected $_categoryMapper; | ||
|
||
/** | ||
* Klaviyo Products Resource Model | ||
* @var Products | ||
*/ | ||
protected $_klProduct; | ||
|
||
/** | ||
* Klaviyo Products Collection | ||
* @var CollectionFactory | ||
*/ | ||
protected $_klProductCollectionFactory; | ||
|
||
/** | ||
* Klaviyo Syncs Model | ||
* @var SyncsFactory | ||
*/ | ||
protected $_klSyncFactory; | ||
|
||
/** | ||
* @param Logger $klaviyoLogger | ||
* @param Products $klProduct | ||
* @param SyncsFactory $klSyncFactory | ||
* @param CollectionFactory $klProductCollectionFactory | ||
* @param CategoryMapper $categoryMapper | ||
*/ | ||
public function __construct( | ||
Logger $klaviyoLogger, | ||
Products $klProduct, | ||
CategoryMapper $categoryMapper, | ||
SyncsFactory $klSyncFactory, | ||
CollectionFactory $klProductCollectionFactory | ||
) | ||
{ | ||
$this->_klaviyoLogger = $klaviyoLogger; | ||
$this->_klProduct = $klProduct; | ||
$this->_categoryMapper = $categoryMapper; | ||
$this->_klSyncFactory = $klSyncFactory; | ||
$this->_klProductCollectionFactory = $klProductCollectionFactory; | ||
} | ||
|
||
public function queueKlProductsForSync() | ||
{ | ||
$klProductsCollection = $this->_klProductCollectionFactory->create(); | ||
$klProductsToSync = $klProductsCollection->getRowsForSync('NEW') | ||
->addFieldToSelect(['id','payload','status','topic', 'klaviyo_id']) | ||
->getData(); | ||
|
||
if (empty($klProductsToSync)) {return;} | ||
|
||
$idsToUpdate = []; | ||
|
||
foreach ($klProductsToSync as $klProductToSync) | ||
{ | ||
$klProductToSync['payload'] = json_encode($this->_categoryMapper->addCategoryNames($klProductToSync['payload'])); | ||
$klSync = $this->_klSyncFactory->create(); | ||
$klSync->setData([ | ||
'payload'=> $klProductToSync['payload'], | ||
'topic'=> $klProductToSync['topic'], | ||
'klaviyo_id'=>$klProductToSync['klaviyo_id'], | ||
'status'=> 'NEW' | ||
]); | ||
try { | ||
$klSync->save(); | ||
array_push($idsToUpdate, $klProductToSync['id']); | ||
} catch (\Exception $e) { | ||
$this->_klaviyoLogger->log(sprintf('Unable to move row: %s', $e->getMessage())); | ||
} | ||
} | ||
|
||
$klProductsCollection->updateRowStatus($idsToUpdate, 'MOVED'); | ||
} | ||
|
||
public function clean() | ||
{ | ||
$klProductsCollection = $this->_klProductCollectionFactory->create(); | ||
$idsToDelete = $klProductsCollection->getIdsToDelete('MOVED'); | ||
|
||
$klProductsCollection->deleteRows($idsToDelete); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
<?php | ||
|
||
namespace Klaviyo\Reclaim\Helper; | ||
|
||
use Magento\Catalog\Model\CategoryFactory; | ||
|
||
class CategoryMapper extends \Magento\Framework\App\Helper\AbstractHelper | ||
{ | ||
/** | ||
* Magento Category Factory | ||
* @var CategoryFactory | ||
*/ | ||
protected $_categoryFactory; | ||
|
||
/** | ||
* Category Map of ids to names | ||
* @var array | ||
*/ | ||
protected $categoryMap = []; | ||
|
||
/** | ||
* @param CategoryFactory $categoryFactory | ||
*/ | ||
public function __construct( | ||
CategoryFactory $categoryFactory | ||
){ | ||
$this->_categoryFactory = $categoryFactory; | ||
} | ||
|
||
/** | ||
* Replace all CategoryIds in payload with their respective names | ||
* @param $payload | ||
* @return array | ||
*/ | ||
public function replaceCategoryIdsWithNames(array $payload): array | ||
{ | ||
$this->updateCategoryMap($payload['Categories']); | ||
|
||
foreach ($payload['Items'] as &$item){ | ||
$item['Categories'] = $this->searchCategoryMapAndReturnNames($item['Categories']); | ||
} | ||
|
||
$payload['AddedItemCategories'] = $this->searchCategoryMapAndReturnNames($payload['AddedItemCategories']); | ||
$payload['Categories'] = $this->searchCategoryMapAndReturnNames($payload['Categories']); | ||
|
||
return $payload; | ||
} | ||
|
||
/** | ||
* Adds all category names in payload to their respective ids | ||
* @param $payload json encoded string of the payload | ||
* @return array | ||
*/ | ||
public function addCategoryNames(string $payload): array | ||
{ | ||
$decoded_payload = json_decode($payload, true); | ||
$this->updateCategoryMap($decoded_payload['product']['Categories']); | ||
|
||
$decoded_payload['product']['Categories'] = $this-> | ||
searchCategoryMapAndReturnIdsAndNames( | ||
$decoded_payload['product']['Categories'] | ||
); | ||
|
||
return $decoded_payload; | ||
} | ||
|
||
/** | ||
* Retrieve categoryNames using categoryIds | ||
* @param array $categoryIds | ||
*/ | ||
public function updateCategoryMap(array $categoryIds) | ||
{ | ||
$categoryFactory = $this->_categoryFactory->create(); | ||
|
||
foreach($categoryIds as $categoryId){ | ||
if (!in_array($categoryId, $this->categoryMap)){ | ||
$this->categoryMap[$categoryId] = $categoryFactory->load($categoryId)->getName(); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Return array of CategoryNames from CategoryMap using ids | ||
* @param array $categoryIds | ||
* @return array | ||
*/ | ||
public function searchCategoryMapAndReturnNames(array $categoryIds): array | ||
{ | ||
return array_values( | ||
array_intersect_key($this->categoryMap, array_flip($categoryIds)) | ||
); | ||
} | ||
|
||
/** | ||
* Return array of arrays mapping category ids to their names | ||
* @param array $categoryIds | ||
* @return array | ||
*/ | ||
public function searchCategoryMapAndReturnIdsAndNames(array $categoryIds): array | ||
{ | ||
$categoryIdsAndNames = []; | ||
foreach ($categoryIds as $categoryId){ | ||
$categoryIdsAndNames[$categoryId] = $this->categoryMap[$categoryId]; | ||
} | ||
return $categoryIdsAndNames; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is actually used for more than the inspector task, it is used in
setup_webhooks
to update the integration blob settingregistered_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the confusion around this! should be updated now though to what you were describing.