From fa5c9bbe8fdf1639a612404567be71d63eebdf34 Mon Sep 17 00:00:00 2001 From: Stevche Radevski Date: Mon, 8 Apr 2024 15:29:41 +0200 Subject: [PATCH] feat: Improve how options are defined and handled for product (#7007) * feat: Improve how options are defined and handled for product * fix: Updating option values supports passing without id --- .../api/__tests__/admin/product.js | 48 +++--- .../use-variant-table-columns.tsx | 4 +- .../product-edit-variant-form.tsx | 6 +- .../products/product-edit-variant/loader.ts | 34 +++++ .../src/api-v2/admin/products/query-config.ts | 2 - .../product-variants.spec.ts | 39 ++++- .../product-module-service/products.spec.ts | 103 ++++++++----- .../migrations/.snapshot-medusa-products.json | 137 ++++-------------- .../migrations/InitialSetup20240401153642.ts | 9 +- packages/product/src/models/index.ts | 1 - .../src/models/product-option-value.ts | 8 +- .../src/models/product-variant-option.ts | 92 ------------ .../product/src/models/product-variant.ts | 19 ++- .../src/services/product-module-service.ts | 91 +++++++----- packages/types/src/product/common.ts | 41 +----- .../src/dal/mikro-orm/db-error-mapper.ts | 20 ++- .../__tests__/mikro-orm-repository.spec.ts | 51 +++---- .../src/dal/mikro-orm/mikro-orm-repository.ts | 52 +++---- 18 files changed, 320 insertions(+), 437 deletions(-) create mode 100644 packages/admin-next/dashboard/src/v2-routes/products/product-edit-variant/loader.ts delete mode 100644 packages/product/src/models/product-variant-option.ts diff --git a/integration-tests/api/__tests__/admin/product.js b/integration-tests/api/__tests__/admin/product.js index 8d42554284..d65b796fd5 100644 --- a/integration-tests/api/__tests__/admin/product.js +++ b/integration-tests/api/__tests__/admin/product.js @@ -806,10 +806,8 @@ medusaIntegrationTestRunner({ () => expect.arrayContaining([ expect.objectContaining({ - id: expect.stringMatching(/^varopt_*/), - option_value: expect.objectContaining({ - value: "100", - }), + id: expect.stringMatching(/^optval_*/), + value: "100", }), ]) ), @@ -910,10 +908,8 @@ medusaIntegrationTestRunner({ () => expect.arrayContaining([ expect.objectContaining({ - id: expect.stringMatching(/^varopt_*/), - option_value: expect.objectContaining({ - value: "large", - }), + id: expect.stringMatching(/^optval_*/), + value: "large", }), ]) ), @@ -963,10 +959,8 @@ medusaIntegrationTestRunner({ () => expect.arrayContaining([ expect.objectContaining({ - id: expect.stringMatching(/^varopt_*/), - option_value: expect.objectContaining({ - value: "green", - }), + id: expect.stringMatching(/^optval_*/), + value: "green", }), ]) ), @@ -1394,21 +1388,17 @@ medusaIntegrationTestRunner({ () => expect.arrayContaining([ expect.objectContaining({ - id: expect.stringMatching(/^varopt_*/), - option_value: expect.objectContaining({ - value: "large", - option: expect.objectContaining({ - title: "size", - }), + id: expect.stringMatching(/^optval_*/), + value: "large", + option: expect.objectContaining({ + title: "size", }), }), expect.objectContaining({ - id: expect.stringMatching(/^varopt_*/), - option_value: expect.objectContaining({ - value: "green", - option: expect.objectContaining({ - title: "color", - }), + id: expect.stringMatching(/^optval_*/), + value: "green", + option: expect.objectContaining({ + title: "color", }), }), ]) @@ -1661,12 +1651,10 @@ medusaIntegrationTestRunner({ () => expect.arrayContaining([ expect.objectContaining({ - id: expect.stringMatching(/^varopt_*/), - option_value: expect.objectContaining({ - value: "large", - option: expect.objectContaining({ - title: "size", - }), + id: expect.stringMatching(/^optval_*/), + value: "large", + option: expect.objectContaining({ + title: "size", }), }), ]) diff --git a/packages/admin-next/dashboard/src/v2-routes/products/product-detail/components/product-variant-section/use-variant-table-columns.tsx b/packages/admin-next/dashboard/src/v2-routes/products/product-detail/components/product-variant-section/use-variant-table-columns.tsx index 7ad26232d0..b4271b8eb3 100644 --- a/packages/admin-next/dashboard/src/v2-routes/products/product-detail/components/product-variant-section/use-variant-table-columns.tsx +++ b/packages/admin-next/dashboard/src/v2-routes/products/product-detail/components/product-variant-section/use-variant-table-columns.tsx @@ -82,7 +82,7 @@ export const useProductVariantTableColumns = (product?: Product) => { ), cell: ({ row }) => { const variantOpt: any = row.original.options.find( - (opt: any) => opt.option_value.option_id === option.id + (opt: any) => opt.option_id === option.id ) if (!variantOpt) { return @@ -94,7 +94,7 @@ export const useProductVariantTableColumns = (product?: Product) => { size="2xsmall" className="flex min-w-[20px] items-center justify-center" > - {variantOpt.option_value.value} + {variantOpt.value} ) diff --git a/packages/admin-next/dashboard/src/v2-routes/products/product-edit-variant/components/product-edit-variant-form/product-edit-variant-form.tsx b/packages/admin-next/dashboard/src/v2-routes/products/product-edit-variant/components/product-edit-variant-form/product-edit-variant-form.tsx index 957fd7ad7f..c62e2baa49 100644 --- a/packages/admin-next/dashboard/src/v2-routes/products/product-edit-variant/components/product-edit-variant-form/product-edit-variant-form.tsx +++ b/packages/admin-next/dashboard/src/v2-routes/products/product-edit-variant/components/product-edit-variant-form/product-edit-variant-form.tsx @@ -53,10 +53,8 @@ export const ProductEditVariantForm = ({ const { t } = useTranslation() const { handleSuccess } = useRouteModal() const defaultOptions = product.options.reduce((acc: any, option: any) => { - const varOpt = variant.options.find( - (o: any) => o.option_value.option_id === option.id - ) - acc[option.title] = varOpt?.option_value?.value + const varOpt = variant.options.find((o: any) => o.option_id === option.id) + acc[option.title] = varOpt?.value return acc }, {}) diff --git a/packages/admin-next/dashboard/src/v2-routes/products/product-edit-variant/loader.ts b/packages/admin-next/dashboard/src/v2-routes/products/product-edit-variant/loader.ts new file mode 100644 index 0000000000..4aab95e041 --- /dev/null +++ b/packages/admin-next/dashboard/src/v2-routes/products/product-edit-variant/loader.ts @@ -0,0 +1,34 @@ +import { LoaderFunctionArgs } from "react-router-dom" + +import { queryClient } from "../../../lib/medusa" +import { productsQueryKeys } from "../../../hooks/api/products" +import { client } from "../../../lib/client" + +const queryKey = (id: string) => { + return [productsQueryKeys.detail(id)] +} + +const queryFn = async (id: string) => { + const productRes = await client.products.retrieve(id) + return { + initialData: productRes, + isStockAndInventoryEnabled: false, + } +} + +const editProductVariantQuery = (id: string) => ({ + queryKey: queryKey(id), + queryFn: async () => queryFn(id), +}) + +export const editProductVariantLoader = async ({ + params, +}: LoaderFunctionArgs) => { + const id = params.id + const query = editProductVariantQuery(id!) + + return ( + queryClient.getQueryData>(query.queryKey) ?? + (await queryClient.fetchQuery(query)) + ) +} diff --git a/packages/medusa/src/api-v2/admin/products/query-config.ts b/packages/medusa/src/api-v2/admin/products/query-config.ts index 3b72e34cbd..e5eda87a62 100644 --- a/packages/medusa/src/api-v2/admin/products/query-config.ts +++ b/packages/medusa/src/api-v2/admin/products/query-config.ts @@ -24,7 +24,6 @@ export const defaultAdminProductsVariantFields = [ "barcode", "*prices", "*options", - "*options.option_value", ] export const retrieveVariantConfig = { @@ -85,7 +84,6 @@ export const defaultAdminProductFields = [ "*variants", "*variants.prices", "*variants.options", - "*variants.options.option_value", "*sales_channels", ] diff --git a/packages/product/integration-tests/__tests__/services/product-module-service/product-variants.spec.ts b/packages/product/integration-tests/__tests__/services/product-module-service/product-variants.spec.ts index 5b1a8913b2..5d08e8e9cf 100644 --- a/packages/product/integration-tests/__tests__/services/product-module-service/product-variants.spec.ts +++ b/packages/product/integration-tests/__tests__/services/product-module-service/product-variants.spec.ts @@ -28,6 +28,10 @@ moduleIntegrationTestRunner({ title: "size", values: ["large", "small"], }, + { + title: "color", + values: ["red", "blue"], + }, ], }) @@ -182,7 +186,7 @@ moduleIntegrationTestRunner({ expect(productVariant.title).toEqual("new test") }) - it("should update the options of a variant successfully", async () => { + it("should upsert the options of a variant successfully", async () => { await service.upsertVariants([ { id: variantOne.id, @@ -191,12 +195,33 @@ moduleIntegrationTestRunner({ ]) const productVariant = await service.retrieveVariant(variantOne.id, { - relations: ["options", "options.option_value", "options.variant"], + relations: ["options"], }) expect(productVariant.options).toEqual( expect.arrayContaining([ expect.objectContaining({ - option_value: expect.objectContaining({ value: "small" }), + value: "small", + }), + ]) + ) + }) + + it("should do a partial update on the options of a variant successfully", async () => { + await service.updateVariants(variantOne.id, { + options: { size: "small", color: "red" }, + }) + + const productVariant = await service.retrieveVariant(variantOne.id, { + relations: ["options"], + }) + + expect(productVariant.options).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + value: "small", + }), + expect.objectContaining({ + value: "red", }), ]) ) @@ -222,7 +247,7 @@ moduleIntegrationTestRunner({ const beforeDeletedVariants = await service.listVariants( { id: variantOne.id }, { - relations: ["options", "options.option_value", "options.variant"], + relations: ["options"], } ) @@ -230,7 +255,7 @@ moduleIntegrationTestRunner({ const deletedVariants = await service.listVariants( { id: variantOne.id }, { - relations: ["options", "options.option_value", "options.variant"], + relations: ["options"], withDeleted: true, } ) @@ -239,9 +264,7 @@ moduleIntegrationTestRunner({ expect(deletedVariants[0].deleted_at).not.toBeNull() for (const variantOption of deletedVariants[0].options) { - expect(variantOption.deleted_at).not.toBeNull() - // The value itself should not be affected - expect(variantOption?.option_value?.deleted_at).toBeNull() + expect(variantOption?.deleted_at).toBeNull() } }) }) diff --git a/packages/product/integration-tests/__tests__/services/product-module-service/products.spec.ts b/packages/product/integration-tests/__tests__/services/product-module-service/products.spec.ts index 39cdb7fbe4..ac9131f401 100644 --- a/packages/product/integration-tests/__tests__/services/product-module-service/products.spec.ts +++ b/packages/product/integration-tests/__tests__/services/product-module-service/products.spec.ts @@ -54,7 +54,6 @@ moduleIntegrationTestRunner({ let productTwo: Product let productCategoryOne: ProductCategory let productCategoryTwo: ProductCategory - let variantTwo: ProductVariant let productTypeOne: ProductType let productTypeTwo: ProductType let images = [{ url: "image-1" }] @@ -112,43 +111,57 @@ moduleIntegrationTestRunner({ productCategoryOne = categories[0] productCategoryTwo = categories[1] - productOne = testManager.create(Product, { + productOne = service.create({ id: "product-1", title: "product 1", status: ProductTypes.ProductStatus.PUBLISHED, + variants: [ + { + id: "variant-1", + title: "variant 1", + inventory_quantity: 10, + }, + ], }) - productTwo = testManager.create(Product, { + productTwo = service.create({ id: "product-2", title: "product 2", status: ProductTypes.ProductStatus.PUBLISHED, - categories: [productCategoryOne], + categories: [{ id: productCategoryOne.id }], collection_id: productCollectionOne.id, tags: tagsData, + options: [ + { + title: "size", + values: ["large", "small"], + }, + { + title: "color", + values: ["red", "blue"], + }, + ], + variants: [ + { + id: "variant-2", + title: "variant 2", + inventory_quantity: 10, + }, + { + id: "variant-3", + title: "variant 3", + inventory_quantity: 10, + options: { + size: "small", + color: "red", + }, + }, + ], }) - testManager.create(ProductVariant, { - id: "variant-1", - title: "variant 1", - inventory_quantity: 10, - product: productOne, - }) - - variantTwo = testManager.create(ProductVariant, { - id: "variant-2", - title: "variant 2", - inventory_quantity: 10, - product: productTwo, - }) - - testManager.create(ProductVariant, { - id: "variant-3", - title: "variant 3", - inventory_quantity: 10, - product: productTwo, - }) - - await testManager.persistAndFlush([productOne, productTwo]) + const res = await Promise.all([productOne, productTwo]) + productOne = res[0] + productTwo = res[1] }) it("should update a product and upsert relations that are not created yet", async () => { @@ -250,9 +263,7 @@ moduleIntegrationTestRunner({ options: expect.arrayContaining([ expect.objectContaining({ id: expect.any(String), - option_value: expect.objectContaining({ - value: data.options[0].values[0], - }), + value: data.options[0].values[0], }), ]), }), @@ -435,7 +446,7 @@ moduleIntegrationTestRunner({ // Note: VariantThree is already assigned to productTwo, that should be deleted variants: [ { - id: variantTwo.id, + id: productTwo.variants[0].id, title: "updated-variant", }, { @@ -456,7 +467,7 @@ moduleIntegrationTestRunner({ id: expect.any(String), variants: expect.arrayContaining([ expect.objectContaining({ - id: variantTwo.id, + id: productTwo.variants[0].id, title: "updated-variant", }), expect.objectContaining({ @@ -468,6 +479,32 @@ moduleIntegrationTestRunner({ ) }) + it("should do a partial update on the options of a variant successfully", async () => { + await service.update(productTwo.id, { + variants: [ + { + id: "variant-3", + options: { size: "small", color: "blue" }, + }, + ], + }) + + const fetchedProduct = await service.retrieve(productTwo.id, { + relations: ["variants", "variants.options"], + }) + + expect(fetchedProduct.variants[0].options).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + value: "small", + }), + expect.objectContaining({ + value: "blue", + }), + ]) + ) + }) + it("should createa variant with id that was passed if it does not exist", async () => { const updateData = { id: productTwo.id, @@ -583,9 +620,7 @@ moduleIntegrationTestRunner({ options: expect.arrayContaining([ expect.objectContaining({ id: expect.any(String), - option_value: expect.objectContaining({ - value: data.options[0].values[0], - }), + value: data.options[0].values[0], }), ]), }), diff --git a/packages/product/src/migrations/.snapshot-medusa-products.json b/packages/product/src/migrations/.snapshot-medusa-products.json index a345393b0e..72f811710f 100644 --- a/packages/product/src/migrations/.snapshot-medusa-products.json +++ b/packages/product/src/migrations/.snapshot-medusa-products.json @@ -112,17 +112,27 @@ "length": 6, "default": "now()", "mappedType": "datetime" + }, + "deleted_at": { + "name": "deleted_at", + "type": "timestamptz", + "unsigned": false, + "autoincrement": false, + "primary": false, + "nullable": true, + "length": 6, + "mappedType": "datetime" } }, "name": "product_category", "schema": "public", "indexes": [ { - "keyName": "IDX_product_category_path", "columnNames": [ - "mpath" + "deleted_at" ], "composite": false, + "keyName": "IDX_product_category_deleted_at", "primary": false, "unique": false }, @@ -314,15 +324,6 @@ "name": "image", "schema": "public", "indexes": [ - { - "columnNames": [ - "url" - ], - "composite": false, - "keyName": "IDX_product_image_url", - "primary": false, - "unique": false - }, { "columnNames": [ "deleted_at" @@ -754,15 +755,6 @@ "name": "product", "schema": "public", "indexes": [ - { - "columnNames": [ - "type_id" - ], - "composite": false, - "keyName": "IDX_product_type_id", - "primary": false, - "unique": false - }, { "columnNames": [ "deleted_at" @@ -996,15 +988,6 @@ "name": "product_option_value", "schema": "public", "indexes": [ - { - "columnNames": [ - "option_id" - ], - "composite": false, - "keyName": "IDX_product_option_value_option_id", - "primary": false, - "unique": false - }, { "columnNames": [ "deleted_at" @@ -1458,15 +1441,6 @@ "name": "product_variant", "schema": "public", "indexes": [ - { - "columnNames": [ - "product_id" - ], - "composite": false, - "keyName": "IDX_product_variant_product_id", - "primary": false, - "unique": false - }, { "columnNames": [ "deleted_at" @@ -1505,8 +1479,8 @@ }, { "columns": { - "id": { - "name": "id", + "variant_id": { + "name": "variant_id", "type": "text", "unsigned": false, "autoincrement": false, @@ -1520,88 +1494,26 @@ "unsigned": false, "autoincrement": false, "primary": false, - "nullable": true, - "mappedType": "text" - }, - "variant_id": { - "name": "variant_id", - "type": "text", - "unsigned": false, - "autoincrement": false, - "primary": false, - "nullable": true, - "mappedType": "text" - }, - "created_at": { - "name": "created_at", - "type": "timestamptz", - "unsigned": false, - "autoincrement": false, - "primary": false, "nullable": false, - "length": 6, - "default": "now()", - "mappedType": "datetime" - }, - "updated_at": { - "name": "updated_at", - "type": "timestamptz", - "unsigned": false, - "autoincrement": false, - "primary": false, - "nullable": false, - "length": 6, - "default": "now()", - "mappedType": "datetime" - }, - "deleted_at": { - "name": "deleted_at", - "type": "timestamptz", - "unsigned": false, - "autoincrement": false, - "primary": false, - "nullable": true, - "length": 6, - "mappedType": "datetime" + "mappedType": "text" } }, "name": "product_variant_option", "schema": "public", "indexes": [ - { - "columnNames": [ - "deleted_at" - ], - "composite": false, - "keyName": "IDX_product_variant_option_deleted_at", - "primary": false, - "unique": false - }, { "keyName": "product_variant_option_pkey", "columnNames": [ - "id" + "variant_id", + "option_value_id" ], - "composite": false, + "composite": true, "primary": true, "unique": true } ], "checks": [], "foreignKeys": { - "product_variant_option_option_value_id_foreign": { - "constraintName": "product_variant_option_option_value_id_foreign", - "columnNames": [ - "option_value_id" - ], - "localTableName": "public.product_variant_option", - "referencedColumnNames": [ - "id" - ], - "referencedTableName": "public.product_option_value", - "deleteRule": "cascade", - "updateRule": "cascade" - }, "product_variant_option_variant_id_foreign": { "constraintName": "product_variant_option_variant_id_foreign", "columnNames": [ @@ -1614,6 +1526,19 @@ "referencedTableName": "public.product_variant", "deleteRule": "cascade", "updateRule": "cascade" + }, + "product_variant_option_option_value_id_foreign": { + "constraintName": "product_variant_option_option_value_id_foreign", + "columnNames": [ + "option_value_id" + ], + "localTableName": "public.product_variant_option", + "referencedColumnNames": [ + "id" + ], + "referencedTableName": "public.product_option_value", + "deleteRule": "cascade", + "updateRule": "cascade" } } } diff --git a/packages/product/src/migrations/InitialSetup20240401153642.ts b/packages/product/src/migrations/InitialSetup20240401153642.ts index 3991f2c9d2..ccbfb65b1a 100644 --- a/packages/product/src/migrations/InitialSetup20240401153642.ts +++ b/packages/product/src/migrations/InitialSetup20240401153642.ts @@ -42,15 +42,12 @@ export class InitialSetup20240315083440 extends Migration { this.addSql('create unique index if not exists "IDX_option_value_option_id_unique" on "product_option_value" (option_id, value) where deleted_at is null;') this.addSql('create index if not exists "IDX_product_option_value_deleted_at" on "product_option_value" ("deleted_at");'); - this.addSql('create table if not exists "product_variant_option" ("id" text not null, "option_value_id" text null, "variant_id" text null, "created_at" timestamptz not null default now(), "updated_at" timestamptz not null default now(), "deleted_at" timestamptz null, constraint "product_variant_option_pkey" primary key ("id"));'); - this.addSql('create unique index if not exists "IDX_variant_option_option_value_unique" on "product_variant_option" (variant_id, option_value_id) where deleted_at is null;') - this.addSql('create index if not exists "IDX_product_variant_option_deleted_at" on "product_variant_option" ("deleted_at");'); - this.addSql('create table if not exists "image" ("id" text not null, "url" text not null, "metadata" jsonb null, "created_at" timestamptz not null default now(), "updated_at" timestamptz not null default now(), "deleted_at" timestamptz null, constraint "image_pkey" primary key ("id"));'); this.addSql('create index if not exists "IDX_product_image_url" on "image" ("url") where deleted_at is null;'); this.addSql('create index if not exists "IDX_product_image_deleted_at" on "image" ("deleted_at");'); this.addSql('create table if not exists "product_tag" ("id" text not null, "value" text not null, "metadata" jsonb null, "created_at" timestamptz not null default now(), "updated_at" timestamptz not null default now(), "deleted_at" timestamptz null, constraint "product_tag_pkey" primary key ("id"));'); + // TODO: We need to modify upsertWithReplace to handle unique constraints // this.addSql('create unique index if not exists "IDX_tag_value_unique" on "product_tag" (value) where deleted_at is null;') this.addSql('create index if not exists "IDX_product_tag_deleted_at" on "product_tag" ("deleted_at");'); @@ -72,7 +69,7 @@ export class InitialSetup20240315083440 extends Migration { this.addSql('create table if not exists "product_tags" ("product_id" text not null, "product_tag_id" text not null, constraint "product_tags_pkey" primary key ("product_id", "product_tag_id"));'); this.addSql('create table if not exists "product_images" ("product_id" text not null, "image_id" text not null, constraint "product_images_pkey" primary key ("product_id", "image_id"));'); this.addSql('create table if not exists "product_category_product" ("product_id" text not null, "product_category_id" text not null, constraint "product_category_product_pkey" primary key ("product_id", "product_category_id"));'); - + this.addSql('create table if not exists "product_variant_option" ("variant_id" text not null, "option_value_id" text not null, constraint "product_variant_option_pkey" primary key ("variant_id", "option_value_id"));'); /* --- FOREIGN KEYS AND CONSTRAINTS --- */ this.addSql('alter table if exists "product" add constraint "product_collection_id_foreign" foreign key ("collection_id") references "product_collection" ("id") on update cascade on delete set null;'); @@ -84,8 +81,8 @@ export class InitialSetup20240315083440 extends Migration { this.addSql('alter table if exists "product_option_value" add constraint "product_option_value_option_id_foreign" foreign key ("option_id") references "product_option" ("id") on update cascade on delete cascade;'); - this.addSql('alter table if exists "product_variant_option" add constraint "product_variant_option_option_value_id_foreign" foreign key ("option_value_id") references "product_option_value" ("id") on update cascade on delete cascade;'); this.addSql('alter table if exists "product_variant_option" add constraint "product_variant_option_variant_id_foreign" foreign key ("variant_id") references "product_variant" ("id") on update cascade on delete cascade;'); + this.addSql('alter table if exists "product_variant_option" add constraint "product_variant_option_option_value_id_foreign" foreign key ("option_value_id") references "product_option_value" ("id") on update cascade on delete cascade;'); this.addSql('alter table if exists "product_images" add constraint "product_images_product_id_foreign" foreign key ("product_id") references "product" ("id") on update cascade on delete cascade;'); this.addSql('alter table if exists "product_images" add constraint "product_images_image_id_foreign" foreign key ("image_id") references "image" ("id") on update cascade on delete cascade;'); diff --git a/packages/product/src/models/index.ts b/packages/product/src/models/index.ts index b71ef29316..f3f6a304fa 100644 --- a/packages/product/src/models/index.ts +++ b/packages/product/src/models/index.ts @@ -6,5 +6,4 @@ export { default as ProductType } from "./product-type" export { default as ProductVariant } from "./product-variant" export { default as ProductOption } from "./product-option" export { default as ProductOptionValue } from "./product-option-value" -export { default as ProductVariantOption } from "./product-variant-option" export { default as Image } from "./product-image" diff --git a/packages/product/src/models/product-option-value.ts b/packages/product/src/models/product-option-value.ts index 07de8f0633..2109c1d2ab 100644 --- a/packages/product/src/models/product-option-value.ts +++ b/packages/product/src/models/product-option-value.ts @@ -9,13 +9,13 @@ import { Entity, Filter, Index, + ManyToMany, ManyToOne, OnInit, - OneToMany, PrimaryKey, Property, } from "@mikro-orm/core" -import { ProductOption, ProductVariantOption } from "./index" +import { ProductOption, ProductVariant } from "./index" const optionValueOptionIdIndexName = "IDX_option_value_option_id_unique" const optionValueOptionIdIndexStatement = createPsqlIndexStatementHelper({ @@ -51,8 +51,8 @@ class ProductOptionValue { }) option: ProductOption | null - @OneToMany(() => ProductVariantOption, (value) => value.option_value, {}) - variant_options = new Collection(this) + @ManyToMany(() => ProductVariant, (variant) => variant.options) + variants = new Collection(this) @Property({ columnType: "jsonb", nullable: true }) metadata?: Record | null diff --git a/packages/product/src/models/product-variant-option.ts b/packages/product/src/models/product-variant-option.ts deleted file mode 100644 index a94d0c39ae..0000000000 --- a/packages/product/src/models/product-variant-option.ts +++ /dev/null @@ -1,92 +0,0 @@ -import { - DALUtils, - createPsqlIndexStatementHelper, - generateEntityId, -} from "@medusajs/utils" -import { - BeforeCreate, - Entity, - Filter, - Index, - ManyToOne, - OnInit, - PrimaryKey, - Property, -} from "@mikro-orm/core" -import { ProductOptionValue, ProductVariant } from "./index" - -const variantOptionValueIndexName = "IDX_variant_option_option_value_unique" -const variantOptionValueIndexStatement = createPsqlIndexStatementHelper({ - name: variantOptionValueIndexName, - tableName: "product_variant_option", - columns: ["variant_id", "option_value_id"], - unique: true, - where: "deleted_at IS NULL", -}) - -variantOptionValueIndexStatement.MikroORMIndex() -@Entity({ tableName: "product_variant_option" }) -@Filter(DALUtils.mikroOrmSoftDeletableFilterOptions) -class ProductVariantOption { - @PrimaryKey({ columnType: "text" }) - id!: string - - @ManyToOne(() => ProductOptionValue, { - columnType: "text", - nullable: true, - fieldName: "option_value_id", - mapToPk: true, - onDelete: "cascade", - }) - option_value_id!: string - - @ManyToOne(() => ProductOptionValue, { - persist: false, - nullable: true, - }) - option_value!: ProductOptionValue - - @ManyToOne(() => ProductVariant, { - columnType: "text", - nullable: true, - fieldName: "variant_id", - mapToPk: true, - onDelete: "cascade", - }) - variant_id: string | null - - @ManyToOne(() => ProductVariant, { - persist: false, - nullable: true, - }) - variant!: ProductVariant - - @Property({ - onCreate: () => new Date(), - columnType: "timestamptz", - defaultRaw: "now()", - }) - created_at: Date - - @Property({ - onCreate: () => new Date(), - onUpdate: () => new Date(), - columnType: "timestamptz", - defaultRaw: "now()", - }) - updated_at: Date - - @Index({ name: "IDX_product_variant_option_deleted_at" }) - @Property({ columnType: "timestamptz", nullable: true }) - deleted_at?: Date - - @OnInit() - @BeforeCreate() - onInit() { - this.id = generateEntityId(this.id, "varopt") - this.variant_id ??= this.variant?.id ?? null - this.option_value_id ??= this.option_value?.id ?? null - } -} - -export default ProductVariantOption diff --git a/packages/product/src/models/product-variant.ts b/packages/product/src/models/product-variant.ts index 906eccc5be..d3bdf65604 100644 --- a/packages/product/src/models/product-variant.ts +++ b/packages/product/src/models/product-variant.ts @@ -12,14 +12,14 @@ import { Entity, Filter, Index, + ManyToMany, ManyToOne, OneToMany, OnInit, PrimaryKey, Property, } from "@mikro-orm/core" -import { Product } from "@models" -import ProductVariantOption from "./product-variant-option" +import { Product, ProductOptionValue } from "@models" const variantSkuIndexName = "IDX_product_variant_sku_unique" const variantSkuIndexStatement = createPsqlIndexStatementHelper({ @@ -162,14 +162,13 @@ class ProductVariant { }) product: Product | null - @OneToMany( - () => ProductVariantOption, - (variantOption) => variantOption.variant, - { - cascade: [Cascade.PERSIST, "soft-remove" as any], - } - ) - options = new Collection(this) + @ManyToMany(() => ProductOptionValue, "variants", { + owner: true, + pivotTable: "product_variant_option", + joinColumn: "variant_id", + inverseJoinColumn: "option_value_id", + }) + options = new Collection(this) @Property({ onCreate: () => new Date(), diff --git a/packages/product/src/services/product-module-service.ts b/packages/product/src/services/product-module-service.ts index 02201c467a..43a089ea1c 100644 --- a/packages/product/src/services/product-module-service.ts +++ b/packages/product/src/services/product-module-service.ts @@ -17,7 +17,6 @@ import { ProductTag, ProductType, ProductVariant, - ProductVariantOption, } from "@models" import { ProductCategoryService, @@ -66,7 +65,6 @@ type InjectedDependencies = { productTypeService: ProductTypeService productOptionService: ProductOptionService productOptionValueService: ModulesSdkTypes.InternalModuleService - productVariantOptionService: ModulesSdkTypes.InternalModuleService eventBusModuleService?: IEventBusModuleService } @@ -77,11 +75,6 @@ const generateMethodForModels = [ { model: ProductTag, singular: "Tag", plural: "Tags" }, { model: ProductType, singular: "Type", plural: "Types" }, { model: ProductVariant, singular: "Variant", plural: "Variants" }, - { - model: ProductVariantOption, - singular: "VariantOption", - plural: "VariantOptions", - }, ] export default class ProductModuleService< @@ -93,8 +86,7 @@ export default class ProductModuleService< TProductImage extends Image = Image, TProductType extends ProductType = ProductType, TProductOption extends ProductOption = ProductOption, - TProductOptionValue extends ProductOptionValue = ProductOptionValue, - TProductVariantOption extends ProductVariantOption = ProductVariantOption + TProductOptionValue extends ProductOptionValue = ProductOptionValue > extends ModulesSdkUtils.abstractModuleServiceFactory< InjectedDependencies, @@ -130,11 +122,6 @@ export default class ProductModuleService< singular: "Variant" plural: "Variants" } - VariantOption: { - dto: ProductTypes.ProductVariantOptionDTO - singular: "VariantOption" - plural: "VariantOptions" - } } >(Product, generateMethodForModels, entityNameToLinkableKeysMap) implements ProductTypes.IProductModuleService @@ -154,8 +141,6 @@ export default class ProductModuleService< protected readonly productTypeService_: ProductTypeService protected readonly productOptionService_: ProductOptionService // eslint-disable-next-line max-len - protected readonly productVariantOptionService_: ModulesSdkTypes.InternalModuleService - // eslint-disable-next-line max-len protected readonly productOptionValueService_: ModulesSdkTypes.InternalModuleService protected readonly eventBusModuleService_?: IEventBusModuleService @@ -171,7 +156,6 @@ export default class ProductModuleService< productTypeService, productOptionService, productOptionValueService, - productVariantOptionService, eventBusModuleService, }: InjectedDependencies, protected readonly moduleDeclaration: InternalModuleDeclaration @@ -190,7 +174,6 @@ export default class ProductModuleService< this.productTypeService_ = productTypeService this.productOptionService_ = productOptionService this.productOptionValueService_ = productOptionValueService - this.productVariantOptionService_ = productVariantOptionService this.eventBusModuleService_ = eventBusModuleService } @@ -357,7 +340,7 @@ export default class ProductModuleService< const variantIdsToUpdate = data.map(({ id }) => id) const variants = await this.productVariantService_.list( { id: variantIdsToUpdate }, - { relations: ["options"], take: null }, + { take: null }, sharedContext ) if (variants.length !== data.length) { @@ -686,26 +669,63 @@ export default class ProductModuleService< @MedusaContext() sharedContext: Context = {} ): Promise { // Validation step - const normalizedInput = data.map((opt) => { - return { - ...opt, - ...(opt.values - ? { - values: opt.values.map((v) => { - return typeof v === "string" ? { value: v } : v - }), - } - : {}), - } as UpdateProductOptionInput - }) - - if (normalizedInput.some((option) => !option.id)) { + if (data.some((option) => !option.id)) { throw new MedusaError( MedusaError.Types.INVALID_DATA, "Tried to update options without specifying an ID" ) } + const dbOptions = await this.productOptionService_.list( + { id: data.map(({ id }) => id) }, + { take: null, relations: ["values"] }, + sharedContext + ) + + if (dbOptions.length !== data.length) { + throw new MedusaError( + MedusaError.Types.INVALID_DATA, + `Cannot update non-existing options with ids: ${arrayDifference( + data.map(({ id }) => id), + dbOptions.map(({ id }) => id) + ).join(", ")}` + ) + } + + // Data normalization + const normalizedInput = data.map((opt) => { + const dbValues = dbOptions.find(({ id }) => id === opt.id)?.values || [] + const normalizedValues = opt.values?.map((v) => { + return typeof v === "string" ? { value: v } : v + }) + + return { + ...opt, + ...(normalizedValues + ? { + // Oftentimes the options are only passed by value without an id, even if they exist in the DB + values: normalizedValues.map((normVal) => { + if ("id" in normVal) { + return normVal + } + + const dbVal = dbValues.find( + (dbVal) => dbVal.value === normVal.value + ) + if (!dbVal) { + return normVal + } + + return { + id: dbVal.id, + value: normVal.value, + } + }), + } + : {}), + } as UpdateProductOptionInput + }) + return await this.productOptionService_.upsertWithReplace( normalizedInput, { relations: ["values"] }, @@ -1188,7 +1208,7 @@ export default class ProductModuleService< sharedContext ) - // Since we handle the options and variants outside of the product upsert, we need to clean up manuallys + // Since we handle the options and variants outside of the product upsert, we need to clean up manually await this.productOptionService_.delete( { product_id: upsertedProduct.id, @@ -1345,8 +1365,7 @@ export default class ProductModuleService< } return { - variant_id: variant.id, - option_value_id: optionValue.id, + id: optionValue.id, } } ) diff --git a/packages/types/src/product/common.ts b/packages/types/src/product/common.ts index 2095ecdc1a..0708a753c2 100644 --- a/packages/types/src/product/common.ts +++ b/packages/types/src/product/common.ts @@ -237,7 +237,7 @@ export interface ProductVariantDTO { * * @expandable */ - options: ProductVariantOptionDTO[] + options: ProductOptionValueDTO[] /** * Holds custom data in key-value pairs. */ @@ -554,45 +554,6 @@ export interface ProductOptionDTO { deleted_at?: string | Date } -export interface ProductVariantOptionDTO { - /** - * The ID of the product variant option. - */ - id: string - /** - * The value of the product variant option. - * - * @expandable - */ - option_value?: ProductOptionValueDTO | null - /** - * The value of the product variant option id. - */ - option_value_id?: string | null - /** - * The associated product variant. - * - * @expandable - */ - variant?: ProductVariantDTO | null - /** - * The associated product variant id. - */ - variant_id?: string | null - /** - * When the product variant option was created. - */ - created_at: string | Date - /** - * When the product variant option was updated. - */ - updated_at: string | Date - /** - * When the product variant option was deleted. - */ - deleted_at?: string | Date -} - /** * @interface * diff --git a/packages/utils/src/dal/mikro-orm/db-error-mapper.ts b/packages/utils/src/dal/mikro-orm/db-error-mapper.ts index 03207c7f57..7893d778dc 100644 --- a/packages/utils/src/dal/mikro-orm/db-error-mapper.ts +++ b/packages/utils/src/dal/mikro-orm/db-error-mapper.ts @@ -13,7 +13,10 @@ export const dbErrorMapper = (err: Error) => { throw new MedusaError(MedusaError.Types.NOT_FOUND, err.message) } - if (err instanceof UniqueConstraintViolationException) { + if ( + err instanceof UniqueConstraintViolationException || + (err as any).code === "23505" + ) { const info = getConstraintInfo(err) if (!info) { throw err @@ -27,7 +30,10 @@ export const dbErrorMapper = (err: Error) => { ) } - if (err instanceof NotNullConstraintViolationException) { + if ( + err instanceof NotNullConstraintViolationException || + (err as any).code === "23502" + ) { throw new MedusaError( MedusaError.Types.INVALID_DATA, `Cannot set field '${(err as any).column}' of ${upperCaseFirst( @@ -36,7 +42,10 @@ export const dbErrorMapper = (err: Error) => { ) } - if (err instanceof InvalidFieldNameException) { + if ( + err instanceof InvalidFieldNameException || + (err as any).code === "42703" + ) { const userFriendlyMessage = err.message.match(/(column.*)/)?.[0] throw new MedusaError( MedusaError.Types.INVALID_DATA, @@ -44,7 +53,10 @@ export const dbErrorMapper = (err: Error) => { ) } - if (err instanceof ForeignKeyConstraintViolationException) { + if ( + err instanceof ForeignKeyConstraintViolationException || + (err as any).code === "23503" + ) { const info = getConstraintInfo(err) throw new MedusaError( MedusaError.Types.NOT_FOUND, diff --git a/packages/utils/src/dal/mikro-orm/integration-tests/__tests__/mikro-orm-repository.spec.ts b/packages/utils/src/dal/mikro-orm/integration-tests/__tests__/mikro-orm-repository.spec.ts index 733ae3804a..6457ce5391 100644 --- a/packages/utils/src/dal/mikro-orm/integration-tests/__tests__/mikro-orm-repository.spec.ts +++ b/packages/utils/src/dal/mikro-orm/integration-tests/__tests__/mikro-orm-repository.spec.ts @@ -656,8 +656,7 @@ describe("mikroOrmRepository", () => { expect(listedEntities[0].entity3.getItems()).toEqual( expect.arrayContaining([ expect.objectContaining({ - // title: "en3-1", - title: "newen3-1", + title: "en3-1", }), expect.objectContaining({ title: "en3-4", @@ -744,8 +743,7 @@ describe("mikroOrmRepository", () => { expect(listedEntities[0].entity3.getItems()).toEqual( expect.arrayContaining([ expect.objectContaining({ - // title: "en3-1", - title: "newen3-1", + title: "en3-1", }), expect.objectContaining({ title: "en3-4", @@ -817,31 +815,34 @@ describe("mikroOrmRepository", () => { ) }) - // it("should correctly handle many-to-many upserts with a uniqueness constriant on a non-primary key", async () => { - // const entity1 = { - // id: "1", - // title: "en1", - // entity3: [{ title: "en3-1" }, { title: "en3-2" }] as any, - // } + it("should correctly handle many-to-many upserts with a uniqueness constriant on a non-primary key", async () => { + const entity1 = { + id: "1", + title: "en1", + entity3: [ + { id: "4", title: "en3-1" }, + { id: "5", title: "en3-2" }, + ] as any, + } - // await manager1().upsertWithReplace([entity1], { - // relations: ["entity3"], - // }) + await manager1().upsertWithReplace([entity1], { + relations: ["entity3"], + }) - // await manager1().upsertWithReplace([{ ...entity1, id: "2" }], { - // relations: ["entity3"], - // }) + await manager1().upsertWithReplace([{ ...entity1, id: "2" }], { + relations: ["entity3"], + }) - // const listedEntities = await manager1().find({ - // where: {}, - // options: { populate: ["entity3"] }, - // }) + const listedEntities = await manager1().find({ + where: {}, + options: { populate: ["entity3"] }, + }) - // expect(listedEntities).toHaveLength(2) - // expect(listedEntities[0].entity3.getItems()).toEqual( - // listedEntities[1].entity3.getItems() - // ) - // }) + expect(listedEntities).toHaveLength(2) + expect(listedEntities[0].entity3.getItems()).toEqual( + listedEntities[1].entity3.getItems() + ) + }) }) describe("error mapping", () => { diff --git a/packages/utils/src/dal/mikro-orm/mikro-orm-repository.ts b/packages/utils/src/dal/mikro-orm/mikro-orm-repository.ts index 93a543de01..b7711c5062 100644 --- a/packages/utils/src/dal/mikro-orm/mikro-orm-repository.ts +++ b/packages/utils/src/dal/mikro-orm/mikro-orm-repository.ts @@ -602,8 +602,7 @@ export function mikroOrmBaseRepositoryFactory( return normalizedData } - // TODO: Currently we will also do an update of the data on the other side of the many-to-many relationship. Reevaluate if we should avoid that. - await this.upsertMany_(manager, relation.type, normalizedData) + await this.upsertMany_(manager, relation.type, normalizedData, true) const pivotData = normalizedData.map((currModel) => { return { @@ -721,39 +720,17 @@ export function mikroOrmBaseRepositoryFactory( return { id: (created as any).id, ...data } } - protected async upsertManyToOneRelations_( - manager: SqlEntityManager, - upsertsPerType: Record - ) { - const typesToUpsert = Object.entries(upsertsPerType) - if (!typesToUpsert.length) { - return [] - } - - return ( - await promiseAll( - typesToUpsert.map(([type, data]) => { - return this.upsertMany_(manager, type, data) - }) - ) - ).flat() - } - protected async upsertMany_( manager: SqlEntityManager, entityName: string, - entries: any[] + entries: any[], + skipUpdate: boolean = false ) { - const existingEntities: any[] = await manager.find( - entityName, - { - id: { $in: entries.map((d) => d.id) }, - }, - { - populate: [], - disableIdentityMap: true, - } - ) + const selectQb = manager.qb(entityName) + const existingEntities: any[] = await selectQb.select("*").where({ + id: { $in: entries.map((d) => d.id) }, + }) + const existingEntitiesMap = new Map( existingEntities.map((e) => [e.id, e]) ) @@ -762,12 +739,21 @@ export function mikroOrmBaseRepositoryFactory( await promiseAll( entries.map(async (data) => { - orderedEntities.push(data) const existingEntity = existingEntitiesMap.get(data.id) + orderedEntities.push(data) if (existingEntity) { + if (skipUpdate) { + return + } await manager.nativeUpdate(entityName, { id: data.id }, data) } else { - await manager.insert(entityName, data) + const qb = manager.qb(entityName) + if (skipUpdate) { + await qb.insert(data).onConflict().ignore().execute() + } else { + await manager.insert(entityName, data) + // await manager.insert(entityName, data) + } } }) )