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.

Sign up for GitHub

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

Jump to bottom

feat: import strategy - sales channels support #2124

Merged
merged 20 commits into from
Sep 2, 2022
Merged

feat: import strategy - sales channels support #2124

merged 20 commits into from
Sep 2, 2022

Conversation

fPolic
Copy link
Contributor

@fPolic fPolic commented Aug 29, 2022 •
edited

What

  • add support for specifying sales channel with import strategy
  • additional:
    • refactor SC service to use retrieve_ pattern
    • fix: pass arguments from startServerWithEnvironment to setup server
    • fix: minio undefined resolve/reject calls
    • fix: csv parser - detect missing columns from schema only if the column is required

How

  • extending schema to expect sales channels columns in an import CSV file

RESOLVES CORE-304

@changeset-bot
Copy link

changeset-bot bot commented Aug 29, 2022 •
edited

⚠️ No Changeset found

Latest commit: 777d3af

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

fPolic added 13 commits August 31, 2022 17:21
wip: add sales channels support to PI, setup flag router, setup schema
81583bc
feat: handle sales channels import
f2a066a
fix: do not create new sales channels
c87878d
feat: remove channel description for now
c78d305
feat: setup integration testing, use a schema with sales channel colu…
28187f9 
…mns only if flag is set
fix: setup flag router for unit tests
3fff737
fix: minio and stream fixes, change file path, handle region error me…
fd319bc 
…ssage differently, change identifier
fix: reading file, descriptive errors
151989a
fix: change strategy identifier in tests
058191f
fix: propagate all props from startServerWithEnvironment to `setupS…
422d9fc 
…erver`
feat: complete integration test
7790845
fix: file path in unit test
258403f
feat: support setting SC on product update, refactor
63571e1
@fPolic fPolic force-pushed the feat/import-strategy-sales-chanels branch from 730b6f5 to 63571e1 Compare August 31, 2022 15:21
@fPolic fPolic marked this pull request as ready for review August 31, 2022 17:39
@fPolic fPolic requested a review from a team as a code owner August 31, 2022 17:39
srindom
srindom reviewed Aug 31, 2022
View reviewed changes
Copy link
Collaborator

@srindom srindom left a comment

Choose a reason for hiding this comment

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

Great work! 🙏 Have a few small comments

packages/medusa/src/services/sales-channel.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/batch-jobs/product/import.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/batch-jobs/product/import.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/batch-jobs/product/import.ts Outdated Show resolved Hide resolved
pKorsholm
pKorsholm reviewed Sep 1, 2022
View reviewed changes
Copy link
Contributor

@pKorsholm pKorsholm left a comment

Choose a reason for hiding this comment

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

I think it looks good! I have a comment about the pattern used for retrieveByName since we've handled it differently in different services and it might be something to take a look at 😄

packages/medusa/src/services/sales-channel.ts Show resolved Hide resolved
pKorsholm
pKorsholm reviewed Sep 2, 2022
View reviewed changes
Copy link
Contributor

@pKorsholm pKorsholm left a comment

Choose a reason for hiding this comment

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

lgtm, very nice!

olivermrbl
olivermrbl approved these changes Sep 2, 2022
View reviewed changes
Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

LGTM! Great work 💪

@kodiakhq kodiakhq bot merged commit 546a963 into develop Sep 2, 2022
@kodiakhq kodiakhq bot deleted the feat/import-strategy-sales-chanels branch September 2, 2022 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pKorsholm pKorsholm pKorsholm left review comments

@olivermrbl olivermrbl olivermrbl approved these changes

@srindom srindom Awaiting requested review from srindom

Assignees
No one assigned
Labels
automerge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fPolic @srindom @olivermrbl @pKorsholm