feat(product-sync): add support for changeMasterVariant, refactor product sync #272
Conversation
junajan
changed the title
There was a problem hiding this comment.
Thanks a lot for this PR. Code looked fine to me. However, I don't have a ton of expertise on this syncer so please also await @butenkor and @lojzatran's reviews
src/coffee/sync/product-sync.coffee
Outdated
|
|
||
| if diff | ||
| actions = [] | ||
| # TODO check action group for base variant update actions - original was attributes |
There was a problem hiding this comment.
Is this TODO supposed to be added in this PR?
There was a problem hiding this comment.
Removed todo here - 01fd315 .. these actions for updating sku/key were under attributes action group before so it is I guess ok if we keep it that way ..
| variant1 and variant2 and ( | ||
| (!isNil(variant1.id) and variant1.id == variant2.id) or | ||
| (!isNil(variant1.key) and variant1.key == variant2.key) or | ||
| (!isNil(variant1.sku) and variant1.sku == variant2.sku) |
There was a problem hiding this comment.
this isNil threw me a little, is it not enough to do
variant1.id === variant2.id
or
variant1.id and variant1.id === variant2.id)
?
There was a problem hiding this comment.
If we don't use isNil, it would match incorrectly variants where existing and the new variant id is undefined even when skus and keys are different.
If we change it to variant1.id and variant1.id === variant2.id it won't work when id is 0.
There was a problem hiding this comment.
ahh.. I see. Thanks for explaining.
| buildAddVariantActions: (newVariant) -> | ||
| addAction = _.deepClone(newVariant) | ||
| delete addAction._NEW_ARRAY_INDEX | ||
| delete addAction._MATCH_CRITERIA |
There was a problem hiding this comment.
huh, where did these two come from? _MATCH_CRITERIA and _NEW_ARRAY_INDEX
There was a problem hiding this comment.
Hi @daern91 - these properties are created by jsondiffpatcher
| # | ||
| # diff - {Object} The result of diff from `jsondiffpatch` | ||
| # old_obj - {Object} The existing product | ||
| # new_obj - {Object} The product to be updated | ||
| # | ||
| # Returns {Array} The list of actions, or empty if there are none | ||
| actionsMapReferences: (diff, old_obj, new_obj) -> | ||
| actionsMapReferences: (diff, new_obj, old_obj) -> |
There was a problem hiding this comment.
ahh, thanks for this one.. nice with some more consistency.
There was a problem hiding this comment.
Sadly, there are much more places which would need this change ..
| @@ -228,14 +249,14 @@ class ProductUtils extends BaseUtils | |||
|
|
|||
| _.sortBy actions, (a) -> a.action is 'addToCategory' | |||
|
|
|||
| # Private: map product prices | |||
| # Map product prices | |||
There was a problem hiding this comment.
Im not aware of these ones. What does this change do?
There was a problem hiding this comment.
just removes private flag from documentation I guess.. because we call this method from outside
| # { action: 'addVariant', attributes: [ { name: 'some', value: 'thing' } ] } | ||
| # { action: 'addVariant', attributes: [ { name: 'yet', value: 'another' } ] } | ||
| # { action: 'addVariant', sku: 'v10', attributes: [ { name: 'something', value: 'else' } ] } | ||
| # { action: 'addVariant', sku: 'SKUwins' } |
There was a problem hiding this comment.
instead of removing and adding all variants, we remove just those which are not present in the product draft, update the rest and do not care about the order of variants
|
Hello @daern91, @lojzatran - it looks like I don't have rights to merge this PR.. could you please merge it and do a release? so we can bump sdk dependency in the importer. |
Summary
Resolves #271
Description
This PR changes how we generate product update actions. Before we used
jsondiffpatchon the whole product. Now, when syncing variants, we match variants byid/key/skuand diff only matched variants which are present in both old and new product.Todo