fix(product-sync): fix matching variants (#273)

#### Summary
Fix how we match variants.

#### Description
Originally, we were matching variants in a single loop by comparing id, sku or key.
Because we used id as a primary matching condition, there was happening, that we matched variants with the same ids but with different skus which later failed due to a duplicate slug error.

For example when syncing these two products:
```js
// existing product
{
	masterVariant: {
		id: 1,
		sku: "sku1"
	},
	variants: [{
		id: 2,
		sku: "sku3"
	}, {
		id: 3,
		sku: "sku2"
	}]
}
// and new product
{
	masterVariant: {
		id: 1,
		sku: "sku1"
	},
	variants: [{
		id: 2,
		sku: "sku2"
	}, {
		id: 3,
		sku: "sku3"
	}]
}
```
It would match variants using id and generate update actions:
```js
[{
	action: "setSku",
	variantId: 2,
	sku: "sku2"
}, {
	action: "setSku",
	variantId: 3,
	sku: "sku3"
}]
```
Which would fail because we can't set `sku2` to the first variant because it already exists. Therefore we should match by skus, then by keys and then, as a fallback, by id.

#### Todo

- Tests
    - [x] Unit
    - [ ] Integration
    - [ ] Acceptance
- [ ] Documentation
<!-- Two persons should review a PR, don't forget to assign them. -->

src/coffee/sync/product-sync.coffee

@@ -126,7 +126,7 @@ class ProductSync extends BaseSync
 
     # divide removeVariant actions to two groups - one for masterVariant, second for normal variants
     partitionedRemoveActionsByMasterVariant = _.partition variantActionGroups.removeVariantActions, (action) =>
-      oldObj.masterVariant and @_utils.matchesByIdOrKeyOrSku(action, oldObj.masterVariant)
+      oldObj.masterVariant and @_utils.matchesBySkuOrKeyOrId(action, oldObj.masterVariant)
 
     allActions.push @_mapActionOrNot 'variants', => partitionedRemoveActionsByMasterVariant[1] # normal variants
     allActions.push variantActionGroups.variantUpdateActions # already white/black listed from @_diffVariant

src/coffee/sync/utils/product.coffee

@@ -100,26 +100,34 @@ class ProductUtils extends BaseUtils
       actions.push action if action
     actions
 
-  matchesByIdOrKeyOrSku: (variant1, variant2) ->
-    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)
-    )
+  matchesBySkuOrKeyOrId: (variant1, variant2) ->
+    @matchesBySku(variant1, variant2) or @matchesByKey(variant1, variant2) or @matchesById(variant1, variant2)
 
+  matchesById: (variant1, variant2) ->
+    variant1 and variant2 and not isNil(variant1.id) and variant1.id == variant2.id
+
+  matchesByKey: (variant1, variant2) ->
+    variant1 and variant2 and not isNil(variant1.key) and variant1.key == variant2.key
+
+  matchesBySku: (variant1, variant2) ->
+    variant1 and variant2 and not isNil(variant1.sku) and variant1.sku == variant2.sku
+
+  # match variant against variants in list - match first by sku, then by key and then by id
   findVariantInList: (variant, variantList) ->
-    variantList.find (testedVariant) =>
-      @matchesByIdOrKeyOrSku(testedVariant, variant)
+    return variantList.find((oldVariant) => @matchesBySku(variant, oldVariant)) or
+      variantList.find((oldVariant) => @matchesByKey(variant, oldVariant)) or
+      variantList.find((oldVariant) => @matchesById(variant, oldVariant)) or
+      undefined # if not found, return undefined
 
   buildChangeMasterVariantAction: (newMasterVariant, oldMasterVariant) ->
-    if newMasterVariant and oldMasterVariant and not @matchesByIdOrKeyOrSku(newMasterVariant, oldMasterVariant)
+    if newMasterVariant and oldMasterVariant and not @matchesBySkuOrKeyOrId(newMasterVariant, oldMasterVariant)
       action =
         action: 'changeMasterVariant'
 
-      if newMasterVariant.id
-        action.variantId = newMasterVariant.id
-      else if newMasterVariant.sku
+      if newMasterVariant.sku
         action.sku = newMasterVariant.sku
+      else if newMasterVariant.id
+        action.variantId = newMasterVariant.id
       else
         throw new Error(
           'ProductSync needs at least one of "id" or "sku" to generate changeMasterVariant update action'
@@ -133,10 +141,10 @@ class ProductUtils extends BaseUtils
         removeAction =
           action: 'removeVariant'
 
-        if oldVariant.id
-          removeAction.id = oldVariant.id
-        else if oldVariant.sku
+        if oldVariant.sku
           removeAction.sku = oldVariant.sku
+        else if oldVariant.id
+          removeAction.id = oldVariant.id
         else
           throw new Error('ProductSync does need at least one of "id" or "sku" to generate a remove action')
 

src/spec/sync/product-sync-sku-match.spec.coffee

@@ -54,7 +54,7 @@ describe 'ProductUtils SKU based matching', ->
     ]
 
     compareVariantActions @sync, @oldProduct, @newProduct,
-      [{ action: 'removeVariant', id: 7 }]
+      [{ action: 'removeVariant', sku: 'vX' }]
 
     compareAttributeActions @sync, @oldProduct, @newProduct, []
 
@@ -109,7 +109,7 @@ describe 'ProductUtils SKU based matching', ->
     ]
 
     compareVariantActions @sync, @oldProduct, @newProduct,  [
-      { action: 'removeVariant', id: 2 }
+      { action: 'removeVariant', sku: 'v2' }
       { action: 'addVariant', sku: 'v4', attributes: [{ name: 'attrib4', value: 'val4' }] }
       { action: 'addVariant', sku: 'v6', attributes: [{ name: 'attrib6', value: 'val6' }] }
     ]

src/spec/sync/product-sync.spec.coffee

@@ -178,7 +178,7 @@ describe 'ProductSync', ->
             sku: 'sku3'
           }, {
             action: 'removeVariant',
-            id: 1
+            sku: 'sku1'
           }
         ]
         version: oldProduct.version
@@ -208,7 +208,7 @@ describe 'ProductSync', ->
       update = @sync.buildActions(newProduct, oldProduct).getUpdatePayload()
       expected_update =
         actions: [
-          { action: 'changeMasterVariant', variantId: 2 }
+          { action: 'changeMasterVariant', sku: 'sku2' }
         ]
         version: oldProduct.version
       expect(update).toEqual expected_update
@@ -804,7 +804,7 @@ describe 'ProductSync', ->
         actions = @sync.buildActions(newProduct, oldProduct).getUpdateActions()
 
         expect(actions).toEqual [
-          { action: 'removeVariant', id: 4 }
+          { action: 'removeVariant', sku: 'v4' }
           { action: 'removeVariant', id: 5 }
           { action: 'setSku', variantId: 2, sku: 'SKUadded' }
           { action: 'setAttribute', variantId: 3, name: 'foo', value: 'CHANGED' }

src/spec/sync/utils/product.spec.coffee

@@ -397,6 +397,92 @@ describe 'ProductUtils', ->
         { action: 'removeVariant', sku: 'newVar' }
       ])
 
+  describe ':: findVariantInList', ->
+
+    it 'should find variant using sku', ->
+      oldVariants = [
+        {
+          id: 1
+          sku: 'sku1'
+        },
+        {
+          id: 2
+          sku: 'sku2'
+        }
+      ]
+      newVariant = {
+        id: 2,
+        sku: 'sku1'
+      }
+
+      actions = @utils.findVariantInList(newVariant, oldVariants)
+      expect(actions).toEqual({
+        id: 1
+        sku: 'sku1'
+      })
+
+    it 'should find variant using id', ->
+      oldVariants = [
+        {
+          id: 1
+          sku: 'sku1'
+        },
+        {
+          id: 2,
+          key: 'key'
+        }
+      ]
+      newVariant = {
+        id: 2,
+      }
+
+      actions = @utils.findVariantInList(newVariant, oldVariants)
+      expect(actions).toEqual({
+        id: 2
+        key: 'key'
+      })
+
+    it 'should find variant using key', ->
+      oldVariants = [
+        {
+          id: 1
+          sku: 'key1'
+        },
+        {
+          id: 2,
+          key: 'key2'
+        }
+      ]
+      newVariant = {
+        id: 1,
+        key: 'key2',
+      }
+
+      actions = @utils.findVariantInList(newVariant, oldVariants)
+      expect(actions).toEqual({
+        id: 2
+        key: 'key2'
+      })
+
+    it 'should not find an unknown variant', ->
+      oldVariants = [
+        {
+          id: 1
+          sku: 'key1'
+        },
+        {
+          id: 2,
+          key: 'key2'
+        }
+      ]
+      newVariant = {
+        id: 29,
+        key: 'key3',
+      }
+
+      actions = @utils.findVariantInList(newVariant, oldVariants)
+      expect(actions).toEqual(undefined)
+
   describe ':: buildVariantPriceActions', ->
 
     it 'should return an empty array when no price diff is provided', ->