Skip to content
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

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

Merged
merged 6 commits into from
Dec 7, 2018

Conversation

junajan
Copy link
Contributor

@junajan junajan commented Dec 6, 2018

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:

// 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:

[{
	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
    • Unit
    • Integration
    • Acceptance
  • Documentation

@junajan junajan self-assigned this Dec 6, 2018
@junajan
Copy link
Contributor Author

junajan commented Dec 6, 2018

Waiting for coverage reports - there is reported a degraded performance on coveralls.io status page - http://status.coveralls.io/


buildChangeMasterVariantAction: (newMasterVariant, oldMasterVariant) ->
if newMasterVariant and oldMasterVariant and not @matchesByIdOrKeyOrSku(newMasterVariant, oldMasterVariant)
if newMasterVariant and oldMasterVariant and not @matchesBySkuOrKeyOrId(newMasterVariant, oldMasterVariant)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not we change the order also here and here and here? I think it might be better to always use sku if defined and fallback on internal only when nothing else possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @butenkor .. updated here c8afbe7

@daern91 daern91 merged commit 2edc59a into master Dec 7, 2018
@daern91 daern91 deleted the 271-fix-match-variants-by-sku-key-id-separately branch December 7, 2018 10:37
@coveralls
Copy link

coveralls commented Dec 8, 2018

Coverage Status

Coverage increased (+0.01%) to 97.66% when pulling eb01715 on 271-fix-match-variants-by-sku-key-id-separately into d2dbb23 on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants