Skip to content
Merged
5 changes: 4 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ node_modules
.npm
.released-packages

# Integration tests dependencies cache
integration-tests/package-lock.json

# Build artifacts
_book
.changelog
Expand All @@ -19,4 +22,4 @@ coverage
.DS_Store

# IDE tools
.idea
.idea
1,796 changes: 0 additions & 1,796 deletions integration-tests/package-lock.json

This file was deleted.

75 changes: 27 additions & 48 deletions packages/sync-actions/src/product-actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ export function actionsMapVariants (diff, oldObj, newObj) {
...newObject,
action: 'addVariant',
}),
[REMOVE_ACTIONS]: objectToRemove => ({
[REMOVE_ACTIONS]: ({ id }) => ({
action: 'removeVariant',
id: objectToRemove.id,
id,
}),
})

Expand Down Expand Up @@ -137,29 +137,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 +150,7 @@ export function actionsMapAttributes (
if (keyAction) actions.push(keyAction)

const { attributes } = variant

const attrActions = _buildVariantAttributesActions(
attributes,
oldObj.variants[key],
Expand All @@ -196,16 +175,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 +195,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 +215,27 @@ export function actionsMapPrices (diff, oldObj, newObj) {
.concat(addPriceActions)
}

export function actionsMapMasterVariant (oldObj, newObj) {
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)

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

return []
}


/**
* HELPER FUNCTIONS
Expand Down Expand Up @@ -542,7 +522,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
26 changes: 25 additions & 1 deletion packages/sync-actions/src/products.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ function createProductMapActions (mapActionGroup) {
allActions.push(mapActionGroup('variants', () =>
productActions.actionsMapVariants(diff, oldObj, newObj)))

allActions.push(productActions.actionsMapMasterVariant(oldObj, newObj))

allActions.push(mapActionGroup('attributes', () =>
productActions.actionsMapAttributes(diff, oldObj, newObj,
sameForAllAttributeNames || [])))
Expand All @@ -58,10 +60,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.

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

const diffed = differ(processedBefore, processedNow)

if (!diffed) return []

return doMapActions(diffed, now, before, options)
return doMapActions(diffed, processedNow, processedBefore, 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