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: product import strategy #1706

Merged
merged 90 commits into from
Aug 25, 2022
Merged

feat: product import strategy #1706

merged 90 commits into from
Aug 25, 2022

Conversation

fPolic
Copy link
Contributor

@fPolic fPolic commented Jun 21, 2022

What

  • implemented product import strategy
  • fix a bug where a product variant couldn't be created if an MA record with region_id was sent in the payload
  • return a fileKey from MinIO file service (needs to be stored in the BatchJob context)
  • useRedis testing util that spins up a Redis container for integration testing
  • handling a batch job failure case in the subscriber
  • add ioredis types

How

  • In the preprocessing faze of a batch job a CSV file is parsed, validated and processed. From row data, 4 types of instructions for creating or updating products and variants are generadet and stored in helper files in a bucket.
  • In the processing phase one set of instructions is pulled at a time, and each instruction is handled by the corresponding service method.

Testing

  • unit testing of data structures that are stored in Redis after preprocessing
  • integration snapshot test of a product endpoint response which contains products, variants, options and prices after a batch job has been completed

TODO

  • expend on integration testing
  • documentation/usage guide

Testing locally with Postman

  1. POST http://localhost:9000/admin/auth
  • set body content {"email": "admin@medusa-test.com", "password": "supersecret"} and type to JSON
  1. POST http://localhost:9000/admin/uploads
  • note: minio file service needs to be added to the store instance
  • body should be a form-data where the CSV file should be uploaded under key files
  • response will contain a key which is used by the file service to locate file e.g. "key": "product-import.csv"
  • the key is needed for the next step
  1. POST http://localhost:9000/admin/batch-jobs/
  • send a request with following body to create a batch job and trigger a run:
    {"type": "product_import", "dry_run": false, "context": {"fileKey": "product-import.csv"}}

Fixes CORE-413

@fPolic fPolic changed the title wip: product import strategy feat(BatchJob): product import strategy Jun 27, 2022
@fPolic fPolic changed the title feat(BatchJob): product import strategy feat: product import strategy Jun 27, 2022
@fPolic fPolic marked this pull request as ready for review June 27, 2022 20:46
@fPolic fPolic requested a review from a team as a code owner June 27, 2022 20:46
@fPolic fPolic self-assigned this Jun 27, 2022
@fPolic fPolic linked an issue Jun 27, 2022 that may be closed by this pull request
4 tasks
@fPolic fPolic force-pushed the feat/pi-strategy branch 3 times, most recently from 8d0b728 to e5d328a Compare June 30, 2022 09:19
@changeset-bot
Copy link

changeset-bot bot commented Jun 30, 2022

⚠️ No Changeset found

Latest commit: f23b4e7

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

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.

Nice work - will do one more pass on this soon; but have added a few comments here :)

packages/medusa/src/services/batch-job.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/product/import.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/product/import.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/product/import.ts Outdated Show resolved Hide resolved
Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

Very nice work 👍 - I ve done a first review and will do another one once seb and my comments are processed. It is a super clean work man 🚀

packages/medusa/src/strategies/product/import.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/product/import.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/product/import.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/product/import.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/product/import.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/product/import.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/product/import.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/product/import.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/product/import.ts Outdated Show resolved Hide resolved
@fPolic fPolic force-pushed the feat/pi-strategy branch 2 times, most recently from 720bb8d to b1877d1 Compare July 6, 2022 19:12
@fPolic fPolic requested review from srindom and adrien2p July 6, 2022 21:39
Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

Few comments, but very nice work 💪 I've also seen that there is still some of seb comments and that there is still some redis stuff, is that intended?

packages/medusa-file-minio/src/services/minio.js Outdated Show resolved Hide resolved
packages/medusa/src/services/region.js Outdated Show resolved Hide resolved
@fPolic
Copy link
Contributor Author

fPolic commented Jul 7, 2022

there is still some of seb comments and that there is still some redis stuff, is that intended?

I made the change as Seb suggested. The import strategy doesn't depend on Redis anymore. Other Redis changes aren't related to the import strategy (I added type support for ioredis and useRedis testing utility).

The other unresolved comments still need to be discussed a bit more :)

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.

Really nice work @frane - just a few small tweaks left; I assume that we will include the Sales Channel related stuff in a separate ticket?

packages/medusa/src/services/region.ts Outdated Show resolved Hide resolved
Comment on lines 417 to 419
const { id } = (await productOptionRepo.findOne({
where: { title: o._title },
})) as ProductOption
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo: this is insufficient as we would have to get the correct product option for the product being worked on. For example, this could exist in the DB:

  • Product Option 1: { id: "option_1", title: "Size", product_id: "prod_a" }
  • Product Option 2: { id: "option_2", title: "Size", product_id: "prod_b" }

Now let's say we are adding a variant to Product B; we could end up having a situation where the variant has a product option value like this:

  • Product Option Value: { id: "optval_1", value: "Medium", option_id: "option_1", variant_id: "new_variant" }

In this case, the option value refers to a ProductVariant belonging to Product B but a ProductOption belonging to Product A.

In the case where we are working with an existing product option we should be able to structure the query to be something along the lines of:

{ title: o._title, product_id: variantOp.product_id },

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.

LGTM! Can't wait to start rolling this out 😍

@srindom srindom merged commit 03221c4 into develop Aug 25, 2022
@fPolic fPolic deleted the feat/pi-strategy branch February 21, 2023 14:25
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.

Implement ProductImportStrategy
3 participants