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(productSync): Update categories before categoryOrderHints #226

Merged
merged 3 commits into from Apr 21, 2017

Conversation

junajan
Copy link
Contributor

@junajan junajan commented Apr 5, 2017

Summary

This PR fixes #218

Description

When generating product update actions category actions should be placed before categoryOrderHint actions so we can add product to category and create a categoryOrderHint in one request.

Todo

  • Tests
    • Unit
    • Integration
    • Acceptance
  • Documentation

@coveralls
Copy link

coveralls commented Apr 5, 2017

Coverage Status

Coverage remained the same at 97.555% when pulling 91e7f9d on 218-fix-category-order-hints-creation-order into aaaf4d9 on master.

Copy link
Contributor

@adnasa adnasa left a comment

Choose a reason for hiding this comment

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

After a short discussion with @wizzy25 ,
I would rather see that the setCategoryHint is moved out of the actionMapsBase and that you create a separate mapper for this action, with the specified criteria (such as categories should be linked to the product) to fulfil the action

code

// in utils/product.coffee
const actionCategoryOrderHint = {
  action: 'setCategoryOrderHint',
  key: 'categoryOrderHints',
}

actionsMapCategoryOrderHint: () => {
  const action = this.buildBaseAttributesAction(actionCategoryOrderHint, diff, old_obj)
  // do your thing then push to the actions list!
}


// in product-sync.coffee
// below 'categories' mapper configuration
allActions.push(
  this._mapActionOrNot(
    'categoryOrderHint',
    () => this._utils.actionsMapCategoryOrderHint(diff)
  )
)

Once you take this approach, make sure to remove the setCategoryHint from actionsBaseList, and you can move the categories action configuration back to it original order

@junajan
Copy link
Contributor Author

junajan commented Apr 6, 2017

@wizzy25 @adnasa - I refactored code according to your comments.. please check it now.

Copy link
Contributor

@adnasa adnasa left a comment

Choose a reason for hiding this comment

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

Looks good to me; with one minor change request from me.
you need to update @actionGroups with the new categoryOrderHints key.

good work

@coveralls
Copy link

coveralls commented Apr 6, 2017

Coverage Status

Coverage increased (+0.007%) to 97.562% when pulling bc7d436 on 218-fix-category-order-hints-creation-order into aaaf4d9 on master.

@coveralls
Copy link

coveralls commented Apr 6, 2017

Coverage Status

Coverage increased (+0.007%) to 97.562% when pulling 576b0b6 on 218-fix-category-order-hints-creation-order into aaaf4d9 on master.

@junajan
Copy link
Contributor Author

junajan commented Apr 7, 2017

@wizzy25 can you please merge this PR? I am not authorized to do that 😢

@wizzy25
Copy link
Contributor

wizzy25 commented Apr 7, 2017

@emmenko Could you review (and merge?) this PR? I don't have write access on this repo

actions = Object.keys(action.categoryOrderHints)
.map (categoryId) ->
orderHint = action.categoryOrderHints[categoryId]
if orderHint is null or orderHint is undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

in coffee this line is same as orderHint?

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.

CategoryOrderHints are being imported before the product is linked with a category
5 participants