Skip to content
87 changes: 37 additions & 50 deletions packages/sync-actions/src/product-actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,15 @@ export function actionsMapVariants (diff, oldObj, newObj) {
...newObject,
action: 'addVariant',
}),
[REMOVE_ACTIONS]: objectToRemove => ({
action: 'removeVariant',
id: objectToRemove.id,
}),
[REMOVE_ACTIONS]: ({ id }, index) => {
// The master variant can not be removed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (index <= 0) return null

return {
action: 'removeVariant',
id,
}
},
})

return handler(diff, oldObj, newObj)
Expand Down Expand Up @@ -137,29 +142,7 @@ export function actionsMapAttributes (
sameForAllAttributeNames = [],
) {
let actions = []
const { masterVariant, variants } = diff

if (masterVariant) {
const skuAction = _buildSkuActions(
masterVariant,
oldObj.masterVariant,
)
const keyAction = _buildKeyActions(
masterVariant,
oldObj.masterVariant,
)
if (skuAction) actions.push(skuAction)
if (keyAction) actions.push(keyAction)

const { attributes } = masterVariant
const attrActions = _buildVariantAttributesActions(
attributes,
oldObj.masterVariant,
newObj.masterVariant,
sameForAllAttributeNames,
)
actions = actions.concat(attrActions)
}
const { variants } = diff

if (variants)
forEach(variants, (variant, key) => {
Expand All @@ -172,6 +155,7 @@ export function actionsMapAttributes (
if (keyAction) actions.push(keyAction)

const { attributes } = variant

const attrActions = _buildVariantAttributesActions(
attributes,
oldObj.variants[key],
Expand All @@ -196,16 +180,7 @@ export function actionsMapAttributes (

export function actionsMapImages (diff, oldObj, newObj) {
let actions = []
const { masterVariant, variants } = diff

if (masterVariant) {
const mActions = _buildVariantImagesAction(
masterVariant.images,
oldObj.masterVariant,
newObj.masterVariant,
)
actions = actions.concat(mActions)
}
const { variants } = diff
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


if (variants)
forEach(variants, (variant, key) => {
Expand All @@ -225,18 +200,7 @@ export function actionsMapPrices (diff, oldObj, newObj) {
let changePriceActions = []
let removePriceActions = []

const { masterVariant, variants } = diff

if (masterVariant) {
const [ a, c, r ] = _buildVariantPricesAction(
masterVariant.prices,
oldObj.masterVariant,
newObj.masterVariant,
)
addPriceActions = addPriceActions.concat(a)
changePriceActions = changePriceActions.concat(c)
removePriceActions = removePriceActions.concat(r)
}
const { variants } = diff

if (variants)
forEach(variants, (variant, key) => {
Expand All @@ -256,6 +220,30 @@ export function actionsMapPrices (diff, oldObj, newObj) {
.concat(addPriceActions)
}

export function generateChangeMasterVariantAction (oldObj, newObj) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specific handler to create the action.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

const createChangeMasterVariantAction = variantId => ({
action: 'changeMasterVariant',
variantId,
})
const extractMasterVariantId = (fromObj) => {
const variants = Array.isArray(fromObj.variants) ? fromObj.variants : []

return variants[0] ? variants[0].id : undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

const newMasterVariantId = extractMasterVariantId(newObj)
const oldMasterVariantId = extractMasterVariantId(oldObj)

// Previosuly no master variant existed
if (!oldMasterVariantId && newMasterVariantId)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

  1. Both have an id and in now the id changed?
  2. Only now has an id (as else if-branch)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@wizzy25 wizzy25 Sep 14, 2017

Choose a reason for hiding this comment

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

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

PS. You should also bump the test coverage 😉

Copy link
Contributor Author

@tdeekens tdeekens Sep 14, 2017

Choose a reason for hiding this comment

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

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

return createChangeMasterVariantAction(newMasterVariantId)
// Old and new master master variant differ and a new master variant id exists
if (newMasterVariantId && oldMasterVariantId !== newMasterVariantId)
return createChangeMasterVariantAction(newMasterVariantId)

return null
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}


/**
* HELPER FUNCTIONS
Expand Down Expand Up @@ -542,7 +530,6 @@ function _buildVariantPricesAction (diffedPrices, oldVariant, newVariant) {
}
} else if (REGEX_UNDERSCORE_NUMBER.test(key)) {
const index = key.substring(1)

removePriceActions.push({
action: 'removePrice', priceId: oldVariant.prices[index].id,
})
Expand Down
28 changes: 27 additions & 1 deletion packages/sync-actions/src/products.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ function createProductMapActions (mapActionGroup) {
allActions.push(mapActionGroup('variants', () =>
productActions.actionsMapVariants(diff, oldObj, newObj)))

const changeMasterVariantAction =
Copy link
Contributor

Choose a reason for hiding this comment

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

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

productActions.generateChangeMasterVariantAction(oldObj, newObj)
if (changeMasterVariantAction) allActions.push(changeMasterVariantAction)

allActions.push(mapActionGroup('attributes', () =>
productActions.actionsMapAttributes(diff, oldObj, newObj,
sameForAllAttributeNames || [])))
Expand All @@ -58,10 +62,32 @@ function createProductMapActions (mapActionGroup) {
}
}

function moveMasterVariantsIntoVariants (before, now) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

const move = obj => ({
...obj,
masterVariant: undefined,
variants: [
obj.masterVariant,
...obj.variants || [],
],
})
const hasMasterVariant = obj => obj && obj.masterVariant

return [
hasMasterVariant(before) ? move(before) : before,
hasMasterVariant(now) ? move(now) : now,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤔

Copy link
Contributor Author

@tdeekens tdeekens Sep 15, 2017

Choose a reason for hiding this comment

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

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

  1. The before can be assumed to always have a master variant
  2. 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.

]
}

export default (config: Array<ActionGroup>): SyncAction => {
const mapActionGroup = createMapActionGroup(config)
const doMapActions = createProductMapActions(mapActionGroup)
const buildActions = createBuildActions(diffpatcher.diff, doMapActions)

const buildActions = createBuildActions(
diffpatcher.diff,
doMapActions,
moveMasterVariantsIntoVariants,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

...third argument of the action builder.

)

return { buildActions }
}
Expand Down
18 changes: 14 additions & 4 deletions packages/sync-actions/src/utils/create-build-actions.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,23 @@
export default function createBuildActions (diff, doMapActions) {
function applyOnBeforeDiff (before, now, fn) {
return fn && typeof fn === 'function' ? fn(before, now) : [before, now]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eases code on call site.

}

export default function createBuildActions (
differ, doMapActions, onBeforeDiff,
) {
return function buildActions (now, before, options = {}) {
if (!now || !before)
throw new Error('Missing either `newObj` or `oldObj` ' +
'in order to build update actions')

// diff 'em
const diffed = diff(before, now)
const [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

proccedBefore, processedNow,
] = applyOnBeforeDiff(before, now, onBeforeDiff)

const diffed = differ(proccedBefore, processedNow)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


if (!diffed) return []

return doMapActions(diffed, now, before, options)
return doMapActions(diffed, processedNow, proccedBefore, options)
}
}
40 changes: 28 additions & 12 deletions packages/sync-actions/src/utils/create-build-array-actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,26 +23,42 @@ export default function createBuildArrayActions (key, config) {

if (diff[key]) {
const arrayDelta = diff[key]

Object.keys(arrayDelta).forEach((index) => {
if (config[ADD_ACTIONS] && isCreateAction(arrayDelta, index))
addActions.push(
// 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]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// When adding a new element you don't need the oldObj
const action = actionGenerator(
newObj[key][index],
parseInt(index, 10),
)
else if (config[CHANGE_ACTIONS] && isChangeAction(arrayDelta, index))
changeActions.push(
// When changing an existing element you need both old + new
config[CHANGE_ACTIONS](oldObj[key][index], newObj[key][index]),

if (action) addActions.push(action)
} else if (
config[CHANGE_ACTIONS] && isChangeAction(arrayDelta, index)
) {
const actionGenerator = config[CHANGE_ACTIONS]
// When changing an existing element you need both old + new
const action = actionGenerator(
oldObj[key][index],
newObj[key][index],
parseInt(index, 10),
)
else if (

if (action) changeActions.push(action)
} else if (
config[REMOVE_ACTIONS] &&
isRemoveAction(arrayDelta, index)
) {
const realIndex = index.replace('_', '')
removeActions.push(
// When removing an existing element you don't need the newObj
config[REMOVE_ACTIONS](oldObj[key][realIndex]),
const actionGenerator = config[REMOVE_ACTIONS]
// When removing an existing element you don't need the newObj
const action = actionGenerator(
oldObj[key][realIndex],
parseInt(realIndex, 10),
)

if (action) removeActions.push(action)
}
})
}
Expand Down
Loading