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(medusa): Added models + repo for products in multiple categories #3083

Merged
merged 7 commits into from
Jan 25, 2023
Merged

feat(medusa): Added models + repo for products in multiple categories #3083

merged 7 commits into from
Jan 25, 2023

Conversation

riqwan
Copy link
Contributor

@riqwan riqwan commented Jan 23, 2023 •
edited
Loading

What:

Introduces a model and a migration to assign multiple products to a category.

Why:

To allow products to be added to multiple categories

How:

  • Creates a migration that creates a table and index for product_category_product
  • Updates Product Model to add association
  • Updates Category Model to add association

RESOLVES CORE-1027

@changeset-bot
Copy link

changeset-bot bot commented Jan 23, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 3d79cb9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@medusajs/medusa Patch
@medusajs/inventory Patch

Not sure what this means? Click here to learn what changesets are.

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

@riqwan riqwan force-pushed the feat/product-category-product-model branch from 7ac58bf to 45d41e7 Compare January 23, 2023 07:39
@riqwan riqwan marked this pull request as ready for review January 23, 2023 07:42
@riqwan riqwan requested a review from a team as a code owner January 23, 2023 07:42
@riqwan riqwan requested review from adrien2p, olivermrbl and pKorsholm and removed request for adrien2p, olivermrbl and pKorsholm January 23, 2023 07:43
@riqwan riqwan marked this pull request as draft January 23, 2023 07:45
@riqwan riqwan force-pushed the feat/product-category-product-model branch from 45d41e7 to ab85222 Compare January 23, 2023 08:04
@riqwan riqwan force-pushed the feat/product-category-product-model branch from ab85222 to f2fe3ed Compare January 23, 2023 08:09
@riqwan riqwan marked this pull request as ready for review January 23, 2023 08:10
@riqwan riqwan changed the title chore: Added models + repo for products in multiple categories feat(medusa): Added models + repo for products in multiple categories Jan 23, 2023
pKorsholm
pKorsholm reviewed Jan 23, 2023
View reviewed changes
Copy link
Contributor

@pKorsholm pKorsholm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Comment: Regarding creating the join-table ProductCategoryProduct it's generally not required to create models explicitly for the join-tables for many-to-many relations and we've generally not done so previously without an explicit purpose (discount conditions).

I dont mind if we include it with a specific purpose, otherwise I think we should hold out on including it. wdyt?

(edit: I dont think it's a bad pattern I just want us to be aware if we start doing things differently so we know to create the rest of these relations if we choose that pattern, and to communicate that we follow a different standard going forward in order to ship consistent code)

@riqwan riqwan force-pushed the feat/product-category-product-model branch from 53ec031 to 550f8b1 Compare January 23, 2023 10:08
@riqwan riqwan force-pushed the feat/product-category-product-model branch from a20d8e0 to 08f2e6d Compare January 23, 2023 20:55
olivermrbl
olivermrbl approved these changes Jan 24, 2023
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, added a suggestion for the FKs

pKorsholm
pKorsholm approved these changes Jan 24, 2023
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 with comments 😄 I think we could remove a couple more of the db columns for the jointable as we dont have a model for them anymore and they just denote the relationship.

packages/medusa/src/migrations/1674455083104-product_category_product.ts Outdated Show resolved Hide resolved
packages/medusa/src/migrations/1674455083104-product_category_product.ts Outdated
await queryRunner.query(
`
CREATE TABLE "product_category_product" (
"id" character varying NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: remove id column since we don't use a model for this anymore

packages/medusa/src/migrations/1674455083104-product_category_product.ts Outdated
Comment on lines 11 to 12
"created_at" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(),
"updated_at" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(),
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: remove created_at and updated_at

adrien2p
adrien2p reviewed Jan 24, 2023
View reviewed changes
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.

I think the comments from phil are the last, I don't have other thing to say. Once it is done I will be able to approve 💪

olivermrbl
olivermrbl reviewed Jan 24, 2023
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, @adrien2p will you have a look to see if we can turn it green 💚

adrien2p
adrien2p approved these changes Jan 24, 2023
View reviewed changes
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.

LGTM 🚀

@riqwan riqwan merged commit 2e7e16b into develop Jan 25, 2023
@riqwan riqwan deleted the feat/product-category-product-model branch January 25, 2023 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olivermrbl olivermrbl olivermrbl approved these changes

@pKorsholm pKorsholm pKorsholm approved these changes

@adrien2p adrien2p adrien2p approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@riqwan @adrien2p @olivermrbl @pKorsholm