feat(sync-actions): add `changeMasterVariant` action generation #304

tdeekens · 2017-09-14T09:52:32Z

Summary

This changes the internal workings of sync-actions to allow for automatic generation of a changeMasterVariant action.

Description

The sync-actions package treated the masterVariant separately when calculating update actions. This comes with a couple of down sides:

Code duplication across a if (masterVariant) forEach(variants)
It restricts the module in detecting changing the master variant

The latter is the issue this tries to solve while also enabling the the generation of a changeMasterVariant update action. Previously, whenever the masterVariant changed the library would assume the whole variant would need creation and would create a update actions accordingly. While also creating update actions to delete the variant which actually became the new master variant.

The problem with this approach is that it is a bug and a feature at the same time. However, more of a bug than a feature in my opinion.

This feels unnecessary as it should just cover the following cases

masterVariant was changed but none of the attributes changed and the variant previously existed
masterVariant was changed some attributes changed and the variant existed previsouly
The masterVariant did not exist (must be new product) and all attributes are new

To achieve this and to minimize the amount of needed book keeping in the process I move the masterVariant into the variants array at the first position.

This removes the code duplication
Allowing tracking the identify of the variant (existence essentially) by id, name, etc (sku and key don't seem implemented yet)
Easier checking if the id of the old vs new master variant actually changed

Todo

Tests
- Unit

codecov · 2017-09-14T09:52:34Z

Codecov Report

Merging #304 into master will increase coverage by 0.3%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #304     +/-   ##
=========================================
+ Coverage   98.11%   98.42%   +0.3%     
=========================================
  Files          74       74             
  Lines        1647     1652      +5     
  Branches      363      366      +3     
=========================================
+ Hits         1616     1626     +10     
+ Misses         25       23      -2     
+ Partials        6        3      -3

Impacted Files	Coverage Δ
...nc-actions/src/utils/create-build-array-actions.js	`100% <100%> (ø)`	⬆️
packages/sync-actions/src/products.js	`100% <100%> (ø)`	⬆️
...ges/sync-actions/src/utils/create-build-actions.js	`87.5% <100%> (+4.16%)`	⬆️
packages/sync-actions/src/product-actions.js	`95.1% <100%> (-0.18%)`	⬇️
.../sync-actions/src/utils/create-map-action-group.js	`100% <0%> (+71.42%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec0ad42...f8a6616. Read the comment docs.

tdeekens

Just some explanatory comments.

tdeekens · 2017-09-14T09:53:10Z

packages/sync-actions/src/product-actions.js

-      id: objectToRemove.id,
-    }),
+    [REMOVE_ACTIONS]: ({ id }, index) => {
+      // The master variant can not be removed


We can't remove the master variant. This was done before by treating it as in the masterVariant property which would non run through the array detection process.

tdeekens · 2017-09-14T09:53:25Z

packages/sync-actions/src/product-actions.js

-    )
-    actions = actions.concat(mActions)
-  }
+  const { variants } = diff


...all this duplication of handling changes goes away.

tdeekens · 2017-09-14T09:53:41Z

packages/sync-actions/src/product-actions.js

    .concat(addPriceActions)
 }

+export function generateChangeMasterVariantAction (oldObj, newObj) {


Specific handler to create the action.

Is it possible to maintain the consistency with previous similar methods by having something like actionsMapMasterVariant or so

tdeekens · 2017-09-14T09:54:17Z

packages/sync-actions/src/product-actions.js

+  const extractMasterVariantId = (fromObj) => {
+    const variants = Array.isArray(fromObj.variants) ? fromObj.variants : []
+
+    return variants[0] ? variants[0].id : undefined


This feels a bit wonky but eases the code further down. Generally caused by variants not having to be set and hence the id not necessarily having to be present.

tdeekens · 2017-09-14T09:54:32Z

packages/sync-actions/src/products.js

  }
 }

+function moveMasterVariantsIntoVariants (before, now) {


This is the hook we pass in to process data before it hits the differ.

tdeekens · 2017-09-14T09:55:37Z

packages/sync-actions/src/utils/create-build-actions.js


-    // diff 'em
-    const diffed = diff(before, now)
+    const [


This function feels generally flaky to me. It shuffles the arguments of new and before around quite a bit and I wonder if that's the intention or actually a bug which we then work around when consuming the diff.

tdeekens · 2017-09-14T09:56:10Z

packages/sync-actions/src/utils/create-build-actions.js

+      proccedBefore, processedNow,
+    ] = applyOnBeforeDiff(before, now, onBeforeDiff)
+
+    const diffed = differ(proccedBefore, processedNow)


I renamed the arg. The library seems to call a diff the same as the differ creating a diff which confused me quite a bit with this argument shuffling.

Just a nitpick: maybe you should rename the variable to processedBefore, and also on line 21

tdeekens · 2017-09-14T09:56:36Z

packages/sync-actions/src/utils/create-build-array-actions.js

-            // When adding a new element you don't need the oldObj
-            config[ADD_ACTIONS](newObj[key][index]),
+        if (config[ADD_ACTIONS] && isCreateAction(arrayDelta, index)) {
+          const actionGenerator = config[ADD_ACTIONS]


Just general simplications in naming things here. The addition is to pass the index to the callback so it has the information.

tdeekens · 2017-09-14T09:57:48Z

packages/sync-actions/test/product-sync-variants.spec.js

      id: '123',
      masterVariant: {
-        sku: 'v1', key: 'v2', attributes: [{ name: 'foo', value: 'new value' }],
+        id: 1, sku: 'v1', key: 'v2', attributes: [{ name: 'foo', value: 'new value' }],


The test below felt like it didn't want to test for the creation of the masterVariant. However if there is no id it should be created.

tdeekens · 2017-09-14T09:57:59Z

packages/sync-actions/test/product-sync-variants.spec.js

-      ],
-    }
+  describe('without master variant in `now`', () => {
+    describe('with master variant in `before`', () => {


This tries to BDD down the tests

Personally I think the test description is not explanatory enough

A sample below

Actions › without master variant in `now` › with master variant in `before` › should not generate update action to remove master variant

compared to something like this

Actions › should build `addVariant` action

No hard opinion on this though. Usable for me

tdeekens · 2017-09-14T10:21:06Z

The test pass. Is the yarn it failing due to me?

wizzy25

Thanks for this. feels a bit hacky but looks good nonetheless. I dropped some comments

And the failing build is a build issue. I will push a commit to this branch to fix it

wizzy25 · 2017-09-14T10:34:49Z

packages/sync-actions/src/product-actions.js

+  const oldMasterVariantId = extractMasterVariantId(oldObj)
+
+  // Previosuly no master variant existed
+  if (!oldMasterVariantId && newMasterVariantId)


I don't understand this part; there will always be a [previous] master variant

Yeah I wasn't 100% about a. the cases here and b. how to guard against unexpected things. Moreover, the tests had cases where no masterVariant in before existed if I am not mistaken. I guess we could

Both have an id and in now the id changed?

Only now has an id (as else if-branch)

However, then the loads of tests fail and I assume it's due some "flawed" fixture data maybe.

I went through the code again and I think I like your implementation as it is.

PS. You should also bump the test coverage 😉

Voila, a completely untested module now tested. I am begging for the % to increase enough. It was the only "red" one in the coverage report e1a5d56

wizzy25 · 2017-09-14T10:40:00Z

packages/sync-actions/src/utils/create-build-actions.js

+      proccedBefore, processedNow,
+    ] = applyOnBeforeDiff(before, now, onBeforeDiff)
+
+    const diffed = differ(proccedBefore, processedNow)


Just a nitpick: maybe you should rename the variable to processedBefore, and also on line 21

remove package-lock file in int tests because we don't want to cache the dependencies as we always want to test the latest versions

tdeekens · 2017-09-14T15:33:21Z

I verified with the platform that a masterVariant will always be auto generated. If you create a project without any variants a masterVariant will be implicitly added. As a result all test cases with e.g.

masterVariant: {
  //No id
  name: 'foo',
  prices: {},
}

are "flawed" as they won't happen. The now and its masterVariant you diff will always have an id. The id is just a sequential value which can't be changed afterwards.

tdeekens · 2017-09-14T16:00:12Z

So from my side this is ready. To be released either on the @latest or @next tag. We can have it on @next first maybe, keep this open and test it in a branch in the MC?

tdeekens · 2017-09-14T16:01:55Z

packages/sync-actions/src/product-actions.js

  const oldMasterVariantId = extractMasterVariantId(oldObj)

-  // Previosuly no master variant existed
-  if (!oldMasterVariantId && newMasterVariantId)


Referring to the comment above and your statement too: this won't happen due to the workings of our API and the implicit generation of a masterVariant.

yanns · 2017-09-15T08:20:51Z

packages/sync-actions/test/product-sync-variants.spec.js

+
+          expect(actions).toEqual([
+            { action: 'addVariant', attributes: [{ name: 'foo', value: 'bar' }], id: 2, sku: 'v1' },
+            { action: 'changeMasterVariant', variantId: 2 },


It should also delete the variant with id 1, shouldn't it?

This is one of the semantics I wasn't 100% sure about. It felt safer not to remove than to just throw it away. I'll see what other reviewers say.

if the goal is that now = before + all update actions, then we have to remove the variant. Otherwise we have one variant too much in now.

Yeap. You're right. I am thinking how we can bake it in.

Yeap, I agree with us removing the update actions.

yanns · 2017-09-15T08:21:40Z

packages/sync-actions/test/product-sync-variants.spec.js

+
+            expect(actions).toEqual([
+              { action: 'changeMasterVariant', variantId: 1 },
+              { action: 'setAttribute', name: 'foo-2', value: 'bar-3', variantId: 2 },


hisabimbola

Thanks for the refactor, I added minor comments that I think we can discuss about.

hisabimbola · 2017-09-15T08:28:26Z

packages/sync-actions/src/product-actions.js

+  if (newMasterVariantId && oldMasterVariantId !== newMasterVariantId)
+    return createChangeMasterVariantAction(newMasterVariantId)
+
+  return null


I think we can make this more consistent by returning [] if no actions is generated

We can I didn't as it will and should always just return one action so the array there felt odd but I'll change it for consistency.

hisabimbola · 2017-09-15T08:29:31Z

packages/sync-actions/src/product-actions.js

    .concat(addPriceActions)
 }

+export function generateChangeMasterVariantAction (oldObj, newObj) {


Is it possible to maintain the consistency with previous similar methods by having something like actionsMapMasterVariant or so

hisabimbola · 2017-09-15T08:57:13Z

packages/sync-actions/src/products.js

    allActions.push(mapActionGroup('variants', () =>
      productActions.actionsMapVariants(diff, oldObj, newObj)))

+    const changeMasterVariantAction =


Buidling from previous comment, if we return empty array here, there will not be any need to check if value is present

hisabimbola · 2017-09-15T09:04:21Z

packages/sync-actions/src/products.js

+
+  return [
+    hasMasterVariant(before) ? move(before) : before,
+    hasMasterVariant(now) ? move(now) : now,


I am not sure how often we will have this scenario but what if the masterVariant object is not passed in the now object.

This is what can not happen in theory. Whenever a project has been created implicitly a master variant will be added. Theoretically we could remove these checks but our test data is sometimes a bit messy. E.g. not having a masterVariant in now of before.

I mean from the user end, I think what you describing is the before(what already exists in the CTP)

If we understand each other though 🤔

Hm, so I think there are a couple of invariants in this discussions

The before can be assumed to always have a master variant

The now should also always have a master variant
2.1 If it doesn't we can assume that the first variant ought to be the new master variant

If non the the cases hold be should either throw or do nothing which breaks things. By not breaking we can "hope" that the before (if without master variant) that the now also doesn't have one. Cause then we will not change the master variant. Which is non destructive in that sense.

hisabimbola · 2017-09-15T09:24:11Z

packages/sync-actions/test/product-sync-variants.spec.js

-      ],
-    }
+  describe('without master variant in `now`', () => {
+    describe('with master variant in `before`', () => {


Personally I think the test description is not explanatory enough

A sample below

Actions › without master variant in `now` › with master variant in `before` › should not generate update action to remove master variant

compared to something like this

Actions › should build `addVariant` action

No hard opinion on this though. Usable for me

hisabimbola · 2017-09-15T09:30:37Z

packages/sync-actions/test/product-sync-variants.spec.js

+        variants: [],
+      }
+
+      it('should not generate update action to remove master variant',


Building from comment above, this test fails

describe('no mastervariant in `now` obj', () => { const before = { id: '123', version: 1, masterVariant: { id: 1, sku: 'v1', attributes: [{ name: 'foo', value: 'bar' }], }, variants: [], } const now = { id: '123', // <-- no masterVariant variants: [ { id: 2, sku: 'v1', attributes: [{ name: 'foo', value: 'bar' }], }, ], } it('should generate a addVariant action', () => { const actions = productsSync.buildActions(now, before) expect(actions).toEqual([ { action: 'addVariant', sku: 'v1', attributes: [{ name: 'foo', value: 'bar' }] }, ]) }) })

Yeah, cause that test lives in the grey zone mentioned above. What do we assume to be intended here? You had a master variant and now you don't want one. Which basically is breaking with our invariants.

Moreover, the test fails as the id 1 doesn't exist anymore.

So should it be removed? (1)?

Should 2 be implicitly be the new master?
Did use user intend to do that?

Should we keep both?

I think with now you declare where you want to end up at. This test is flawed cause the state the user wants to end up in is not valid and we can only assume what was meant.

hisabimbola · 2017-09-15T09:32:27Z

packages/sync-actions/test/product-sync-variants.spec.js

+
+          expect(actions).toEqual([
+            { action: 'addVariant', attributes: [{ name: 'foo', value: 'bar' }], id: 2, sku: 'v1' },
+            { action: 'changeMasterVariant', variantId: 2 },


Yeap, I agree with us removing the update actions.

hisabimbola · 2017-09-15T09:33:19Z

packages/sync-actions/test/product-sync-variants.spec.js

+            id: '123',
+            version: 1,
+            masterVariant: {
+              id: 2,


masterVariant cannot have a variantId of 2. I think it's always 1

Cc @yanns

It can have an id of 2 otherwise you'd never be able to move the master variant. The id is immutable and sequentially assigned.

Oh, okay, good to know that variantId does not change.

I confirm when @tdeekens says.
Master variant id is not always 1. The variant id is the invariant 🤣

hisabimbola · 2017-09-15T09:34:58Z

packages/sync-actions/test/utils/create-map-action-group.spec.js

@@ -0,0 +1,89 @@
+import createMapActionGroup from '../../src/utils/create-map-action-group'
+
+describe('', () => {


I am not sure why this is empty string.

using createMapActionGroup might work since that's the method we are testing.

hisabimbola · 2017-09-15T09:43:53Z

I also think we should add more comments in the code to explain a couple of decisions and why.

I find the sync-actions a bit hard to read/understand, and mostly because of the custom rules of the jsondiffpatch and lack of comments explaining irregular decisions done in the code.

tdeekens · 2017-09-15T09:51:25Z

I find the sync-actions a bit hard to read/understand, and mostly because of the custom rules of the jsondiffpatch

You should have seen my face yesterday. I got a bold spot on my head from scratching 😄

…ion generation

tdeekens · 2017-09-15T13:42:53Z

Regarding the BDD on a comment above I can't comment on

Actions › without master variant in `now` › with master variant in `before` › should not generate update action to remove master variant

This gives context as in Given, When Then. Which I think is nicer to follow along as it sets up the context towards what the then is. Otherwise you have to peel the Given and When off the data which needs more knowledge.

…ot in variants

tdeekens · 2017-09-15T14:10:50Z

@hisabimbola I went over things a little. We now remove the variant if if doesn't exist anymore in variants. I also changed the test structure a bit to hopefully be nicer to read along.

tdeekens · 2017-09-15T15:09:04Z

After talking to @hisabimbola we decided not to have validation on this PR. Changes it might be a breaking change to loads of consumers of the library. Furthermore, our test data does not always contain all required fields.

This is ready then for a last round of review.

@hisabimbola you want to publish it on the @latest or @next tag?

hisabimbola · 2017-09-18T08:27:54Z

packages/sync-actions/src/products.js


+    const changeMasterVariantActions =
+      productActions.actionsMapMasterVariant(oldObj, newObj)
+    if (changeMasterVariantActions.length > 0)


I think we don't need the condition again, if it's empty array, it's fine for us to push it. it gets flattened out anyways below

Ah ok. Thought better to be safe than sorry. Here you go f8a6616

hisabimbola

This looks good now

Thanks for helping out @tdeekens 🚀 💯 🥇

The releases are done automatically base on the commit message. More info here

Let me know if you need help with that

feat(sync-actions): add changeMasterVariant action generation

710b3ce

tdeekens added Status: Review Type: Bug Type: New Feature labels Sep 14, 2017

tdeekens self-assigned this Sep 14, 2017

tdeekens requested review from emmenko and hisabimbola September 14, 2017 09:52

tdeekens commented Sep 14, 2017

View reviewed changes

wizzy25 reviewed Sep 14, 2017

View reviewed changes

wizzy25 and others added 2 commits September 14, 2017 12:58

chore(int-tests): remove package-lock file

0e605d3

remove package-lock file in int tests because we don't want to cache the dependencies as we always want to test the latest versions

feat(sync-actions): fixup variable namings

a675b76

tdeekens added 2 commits September 14, 2017 17:44

test(sync-actions/create-map-action-group): add spec

e1a5d56

feat(sync-actions): remove uncalled-for case when generating action

5c6c66e

tdeekens force-pushed the feature/change-master-variant-action branch from ac4b76a to 5c6c66e Compare September 14, 2017 15:46

tdeekens commented Sep 14, 2017

View reviewed changes

yanns reviewed Sep 15, 2017

View reviewed changes

hisabimbola suggested changes Sep 15, 2017

View reviewed changes

tdeekens added 2 commits September 15, 2017 15:27

feat(sync-actions): add missing describe name

f552049

feat(sync-actions): change fn and return of master variant change act…

7b7ca61

…ion generation

feat(sync-actions): change to remove variant when master variant is n…

833ee1b

…ot in variants

hisabimbola reviewed Sep 18, 2017

View reviewed changes

feat(sync-actions): remove conditional to check for action

f8a6616

hisabimbola approved these changes Sep 18, 2017

View reviewed changes

tdeekens merged commit 81df90b into master Sep 18, 2017

tdeekens deleted the feature/change-master-variant-action branch September 18, 2017 09:36

tdeekens removed the Status: Review label Sep 18, 2017

wizzy25 mentioned this pull request Sep 18, 2017

feat(product-exporter): new package #303

Merged

9 tasks

renovate bot mentioned this pull request Feb 5, 2018

chore(deps): update dependency eslint-plugin-flowtype to v2.42.0 #425

Closed

		@@ -0,0 +1,89 @@
		import createMapActionGroup from '../../src/utils/create-map-action-group'

		describe('', () => {

feat(sync-actions): add changeMasterVariant action generation #304

feat(sync-actions): add changeMasterVariant action generation #304

Uh oh!

Conversation

tdeekens commented Sep 14, 2017

Summary

Description

Todo

Uh oh!

codecov bot commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tdeekens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdeekens commented Sep 14, 2017

Uh oh!

wizzy25 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wizzy25 Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdeekens Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdeekens commented Sep 14, 2017

Uh oh!

tdeekens commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feat(sync-actions): add `changeMasterVariant` action generation #304

feat(sync-actions): add `changeMasterVariant` action generation #304

codecov bot commented Sep 14, 2017 •

edited

Loading

wizzy25 Sep 14, 2017 •

edited

Loading

tdeekens Sep 14, 2017 •

edited

Loading

tdeekens commented Sep 14, 2017 •

edited

Loading

tdeekens Sep 15, 2017 •

edited

Loading

tdeekens Sep 15, 2017 •

edited

Loading