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

feat(sync/customers): add setStores #1468

Merged
merged 1 commit into from
Jan 20, 2020

Conversation

adnasa
Copy link
Contributor

@adnasa adnasa commented Jan 20, 2020

Summary

@codecov
Copy link

codecov bot commented Jan 20, 2020

Codecov Report

Merging #1468 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1468   +/-   ##
=======================================
  Coverage   98.31%   98.31%           
=======================================
  Files         126      126           
  Lines        3272     3272           
  Branches      751      751           
=======================================
  Hits         3217     3217           
  Misses         51       51           
  Partials        4        4
Impacted Files Coverage Δ
packages/sync-actions/src/customer-actions.js 100% <ø> (ø) ⬆️

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 70e7ec6...2403522. Read the comment docs.

Copy link
Contributor

@katmatt katmatt left a comment

Choose a reason for hiding this comment

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

See my comment, would be great if you could check it 😄

expect(actual).toEqual([
{
action: 'setStores',
stores: ['canada', 'usa'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you really sure this works against our api? according to our docs (https://docs.commercetools.com/http-api-projects-customers#set-stores-beta) you have to provide a ResourceIdentifier (https://docs.commercetools.com/http-api-types.html#resourceidentifier) and not just a string...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!
I was working under the MC context the whole time, which we always use string for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adnasa adnasa force-pushed the adnasa-update-customers-with-setStores branch from 7b3e351 to 2403522 Compare January 20, 2020 13:22
@katmatt
Copy link
Contributor

katmatt commented Jan 20, 2020

Looks very good to me 👍

@adnasa
Copy link
Contributor Author

adnasa commented Jan 20, 2020

the lint passed. Do I need to wait for the other checks too? it doesn't look like they are running

@katmatt
Copy link
Contributor

katmatt commented Jan 20, 2020

@adnasa Which other checks do you mean?

@adnasa
Copy link
Contributor Author

adnasa commented Jan 20, 2020

oh well never mind... here we go 🎉

@adnasa adnasa merged commit 20461b6 into master Jan 20, 2020
@adnasa adnasa deleted the adnasa-update-customers-with-setStores branch January 20, 2020 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants