feat(core): Correct handling of product option groups

There were various non-optimum aspects to how we have been handling option groups which this commit
resolves:

- It was possible to re-assign a option group to a new Product, which would silently remove
it from the former Product. This now raises an exception.
- Removal on an option group will now also (soft-) delete it if it is no longer referenced anywhere.

packages/core/e2e/product.e2e-spec.ts

@@ -13,6 +13,9 @@ import {
     AddOptionGroupToProduct,
     ChannelFragment,
     CreateProduct,
+    CreateProductOptionGroup,
+    CreateProductOptionGroupMutation,
+    CreateProductOptionGroupMutationVariables,
     CreateProductVariants,
     DeleteProduct,
     DeleteProductVariant,
@@ -43,6 +46,7 @@ import {
 import {
     ADD_OPTION_GROUP_TO_PRODUCT,
     CREATE_PRODUCT,
+    CREATE_PRODUCT_OPTION_GROUP,
     CREATE_PRODUCT_VARIANTS,
     DELETE_PRODUCT,
     DELETE_PRODUCT_VARIANT,
@@ -1224,15 +1228,26 @@ describe('Product resolver', () => {
         );
 
         it('addOptionGroupToProduct adds an option group', async () => {
+            const optionGroup = await createOptionGroup('Quark-type', ['Charm', 'Strange']);
             const result = await adminClient.query<
                 AddOptionGroupToProduct.Mutation,
                 AddOptionGroupToProduct.Variables
             >(ADD_OPTION_GROUP_TO_PRODUCT, {
-                optionGroupId: 'T_2',
+                optionGroupId: optionGroup.id,
                 productId: newProduct.id,
             });
             expect(result.addOptionGroupToProduct.optionGroups.length).toBe(1);
-            expect(result.addOptionGroupToProduct.optionGroups[0].id).toBe('T_2');
+            expect(result.addOptionGroupToProduct.optionGroups[0].id).toBe(optionGroup.id);
+
+            // not really testing this, but just cleaning up for later tests
+            const { removeOptionGroupFromProduct } = await adminClient.query<
+                RemoveOptionGroupFromProduct.Mutation,
+                RemoveOptionGroupFromProduct.Variables
+            >(REMOVE_OPTION_GROUP_FROM_PRODUCT, {
+                optionGroupId: optionGroup.id,
+                productId: newProduct.id,
+            });
+            removeOptionGuard.assertSuccess(removeOptionGroupFromProduct);
         });
 
         it(
@@ -1243,13 +1258,28 @@ describe('Product resolver', () => {
                         ADD_OPTION_GROUP_TO_PRODUCT,
                         {
                             optionGroupId: 'T_1',
-                            productId: '999',
+                            productId: 'T_999',
                         },
                     ),
                 `No Product with the id '999' could be found`,
             ),
         );
 
+        it(
+            'addOptionGroupToProduct errors if the OptionGroup is already assigned to another Product',
+            assertThrowsWithMessage(
+                () =>
+                    adminClient.query<AddOptionGroupToProduct.Mutation, AddOptionGroupToProduct.Variables>(
+                        ADD_OPTION_GROUP_TO_PRODUCT,
+                        {
+                            optionGroupId: 'T_1',
+                            productId: 'T_2',
+                        },
+                    ),
+                `The ProductOptionGroup "laptop-screen-size" is already assigned to the Product "Laptop"`,
+            ),
+        );
+
         it(
             'addOptionGroupToProduct errors with an invalid optionGroupId',
             assertThrowsWithMessage(
@@ -1266,20 +1296,20 @@ describe('Product resolver', () => {
         );
 
         it('removeOptionGroupFromProduct removes an option group', async () => {
+            const optionGroup = await createOptionGroup('Length', ['Short', 'Long']);
             const { addOptionGroupToProduct } = await adminClient.query<
                 AddOptionGroupToProduct.Mutation,
                 AddOptionGroupToProduct.Variables
             >(ADD_OPTION_GROUP_TO_PRODUCT, {
-                optionGroupId: 'T_1',
+                optionGroupId: optionGroup.id,
                 productId: newProductWithAssets.id,
             });
             expect(addOptionGroupToProduct.optionGroups.length).toBe(1);
-
             const { removeOptionGroupFromProduct } = await adminClient.query<
                 RemoveOptionGroupFromProduct.Mutation,
                 RemoveOptionGroupFromProduct.Variables
             >(REMOVE_OPTION_GROUP_FROM_PRODUCT, {
-                optionGroupId: 'T_1',
+                optionGroupId: optionGroup.id,
                 productId: newProductWithAssets.id,
             });
             removeOptionGuard.assertSuccess(removeOptionGroupFromProduct);
@@ -1369,23 +1399,22 @@ describe('Product resolver', () => {
             let optionGroup3: GetOptionGroup.ProductOptionGroup;
 
             beforeAll(async () => {
+                optionGroup2 = await createOptionGroup('group-2', ['group2-option-1', 'group2-option-2']);
+                optionGroup3 = await createOptionGroup('group-3', ['group3-option-1', 'group3-option-2']);
                 await adminClient.query<AddOptionGroupToProduct.Mutation, AddOptionGroupToProduct.Variables>(
                     ADD_OPTION_GROUP_TO_PRODUCT,
                     {
-                        optionGroupId: 'T_3',
+                        optionGroupId: optionGroup2.id,
                         productId: newProduct.id,
                     },
                 );
-                const result1 = await adminClient.query<GetOptionGroup.Query, GetOptionGroup.Variables>(
-                    GET_OPTION_GROUP,
-                    { id: 'T_2' },
-                );
-                const result2 = await adminClient.query<GetOptionGroup.Query, GetOptionGroup.Variables>(
-                    GET_OPTION_GROUP,
-                    { id: 'T_3' },
+                await adminClient.query<AddOptionGroupToProduct.Mutation, AddOptionGroupToProduct.Variables>(
+                    ADD_OPTION_GROUP_TO_PRODUCT,
+                    {
+                        optionGroupId: optionGroup3.id,
+                        productId: newProduct.id,
+                    },
                 );
-                optionGroup2 = result1.productOptionGroup!;
-                optionGroup3 = result2.productOptionGroup!;
             });
 
             it(
@@ -1404,7 +1433,7 @@ describe('Product resolver', () => {
                             ],
                         },
                     );
-                }, 'ProductVariant optionIds must include one optionId from each of the groups: curvy-monitor-monitor-size, laptop-ram'),
+                }, 'ProductVariant optionIds must include one optionId from each of the groups: group-2, group-3'),
             );
 
             it(
@@ -1423,7 +1452,7 @@ describe('Product resolver', () => {
                             ],
                         },
                     );
-                }, 'ProductVariant optionIds must include one optionId from each of the groups: curvy-monitor-monitor-size, laptop-ram'),
+                }, 'ProductVariant optionIds must include one optionId from each of the groups: group-2, group-3'),
             );
 
             it('createProductVariants works', async () => {
@@ -2059,6 +2088,23 @@ describe('Product resolver', () => {
             expect(product.slug).toBe(productToDelete.slug);
         });
     });
+
+    async function createOptionGroup(name: string, options: string[]) {
+        const { createProductOptionGroup } = await adminClient.query<
+            CreateProductOptionGroupMutation,
+            CreateProductOptionGroupMutationVariables
+        >(CREATE_PRODUCT_OPTION_GROUP, {
+            input: {
+                code: name.toLowerCase(),
+                translations: [{ languageCode: LanguageCode.en, name }],
+                options: options.map(option => ({
+                    code: option.toLowerCase(),
+                    translations: [{ languageCode: LanguageCode.en, name: option }],
+                })),
+            },
+        });
+        return createProductOptionGroup;
+    }
 });
 
 export const REMOVE_OPTION_GROUP_FROM_PRODUCT = gql`

packages/core/src/api/resolvers/admin/product-option.resolver.ts

@@ -109,6 +109,6 @@ export class ProductOptionResolver {
         @Ctx() ctx: RequestContext,
         @Args() { id }: MutationDeleteProductOptionArgs,
     ): Promise<DeletionResponse> {
-        return this.productOptionService.softDelete(ctx, id);
+        return this.productOptionService.delete(ctx, id);
     }
 }

packages/core/src/entity/product-option-group/product-option-group-translation.entity.ts

@@ -10,8 +10,10 @@ import { CustomProductOptionGroupFieldsTranslation } from '../custom-entity-fiel
 import { ProductOptionGroup } from './product-option-group.entity';
 
 @Entity()
-export class ProductOptionGroupTranslation extends VendureEntity
-    implements Translation<ProductOptionGroup>, HasCustomFields {
+export class ProductOptionGroupTranslation
+    extends VendureEntity
+    implements Translation<ProductOptionGroup>, HasCustomFields
+{
     constructor(input?: DeepPartial<Translation<ProductOptionGroup>>) {
         super(input);
     }
@@ -20,6 +22,7 @@ export class ProductOptionGroupTranslation extends VendureEntity
 
     @Column() name: string;
 
+    // TODO: V2 need to add onDelete: CASCADE here
     @ManyToOne(type => ProductOptionGroup, base => base.translations)
     base: ProductOptionGroup;
 

packages/core/src/entity/product-option/product-option-translation.entity.ts

@@ -10,8 +10,10 @@ import { CustomProductOptionFieldsTranslation } from '../custom-entity-fields';
 import { ProductOption } from './product-option.entity';
 
 @Entity()
-export class ProductOptionTranslation extends VendureEntity
-    implements Translation<ProductOption>, HasCustomFields {
+export class ProductOptionTranslation
+    extends VendureEntity
+    implements Translation<ProductOption>, HasCustomFields
+{
     constructor(input?: DeepPartial<Translation<ProductOption>>) {
         super(input);
     }
@@ -20,6 +22,7 @@ export class ProductOptionTranslation extends VendureEntity
 
     @Column() name: string;
 
+    // TODO: V2 need to add onDelete: CASCADE here
     @ManyToOne(type => ProductOption, base => base.translations)
     base: ProductOption;
 

packages/core/src/event-bus/events/product-option-group-event.ts

@@ -25,7 +25,7 @@ export class ProductOptionGroupEvent extends VendureEntityEvent<
     constructor(
         ctx: RequestContext,
         entity: ProductOptionGroup,
-        type: 'created' | 'updated',
+        type: 'created' | 'updated' | 'deleted',
         input?: ProductOptionGroupInputTypes,
     ) {
         super(entity, type, ctx, input);

packages/core/src/i18n/messages/en.json

@@ -40,6 +40,7 @@
     "products-cannot-be-removed-from-default-channel": "Products cannot be removed from the default Channel",
     "product-id-or-slug-must-be-provided": "Either the Product id or slug must be provided",
     "product-id-slug-mismatch": "The provided id and slug refer to different Products",
+    "product-option-group-already-assigned": "The ProductOptionGroup \"{ groupCode }\" is already assigned to the Product \"{ productName }\"",
     "product-variant-option-ids-not-compatible": "ProductVariant optionIds must include one optionId from each of the groups: {groupNames}",
     "product-variant-options-combination-already-exists": "A ProductVariant with the selected options already exists: {variantName}",
     "promotion-channels-can-only-be-changed-from-default-channel": "Promotions channels may only be changed from the Default Channel",

packages/core/src/service/services/product-option-group.service.ts

@@ -1,6 +1,7 @@
 import { Injectable } from '@nestjs/common';
 import {
     CreateProductOptionGroupInput,
+    DeletionResult,
     UpdateProductOptionGroupInput,
 } from '@vendure/common/lib/generated-types';
 import { ID } from '@vendure/common/lib/shared-types';
@@ -10,7 +11,9 @@ import { RequestContext } from '../../api/common/request-context';
 import { RelationPaths } from '../../api/index';
 import { Translated } from '../../common/types/locale-types';
 import { assertFound } from '../../common/utils';
+import { Logger } from '../../config/index';
 import { TransactionalConnection } from '../../connection/transactional-connection';
+import { Product, ProductOptionTranslation, ProductVariant } from '../../entity/index';
 import { ProductOptionGroupTranslation } from '../../entity/product-option-group/product-option-group-translation.entity';
 import { ProductOptionGroup } from '../../entity/product-option-group/product-option-group.entity';
 import { EventBus } from '../../event-bus';
@@ -19,6 +22,8 @@ import { CustomFieldRelationService } from '../helpers/custom-field-relation/cus
 import { TranslatableSaver } from '../helpers/translatable-saver/translatable-saver';
 import { translateDeep } from '../helpers/utils/translate-entity';
 
+import { ProductOptionService } from './product-option.service';
+
 /**
  * @description
  * Contains methods relating to {@link ProductOptionGroup} entities.
@@ -31,6 +36,7 @@ export class ProductOptionGroupService {
         private connection: TransactionalConnection,
         private translatableSaver: TranslatableSaver,
         private customFieldRelationService: CustomFieldRelationService,
+        private productOptionService: ProductOptionService,
         private eventBus: EventBus,
     ) {}
 
@@ -115,4 +121,85 @@ export class ProductOptionGroupService {
         this.eventBus.publish(new ProductOptionGroupEvent(ctx, group, 'updated', input));
         return assertFound(this.findOne(ctx, group.id));
     }
+
+    /**
+     * @description
+     * Deletes the ProductOptionGroup and any associated ProductOptions. If the ProductOptionGroup
+     * is still referenced by a soft-deleted Product, then a soft-delete will be used to preserve
+     * referential integrity. Otherwise a hard-delete will be performed.
+     */
+    async deleteGroupAndOptionsFromProduct(ctx: RequestContext, id: ID, productId: ID) {
+        const optionGroup = await this.connection.getEntityOrThrow(ctx, ProductOptionGroup, id, {
+            relations: ['options', 'product'],
+        });
+        const inUseByActiveProducts = await this.isInUseByOtherProducts(
+            ctx,
+            optionGroup,
+            productId,
+            'active',
+        );
+        if (0 < inUseByActiveProducts) {
+            return {
+                result: DeletionResult.NOT_DELETED,
+                message: ctx.translate('message.product-option-group-used', {
+                    code: optionGroup.code,
+                    count: inUseByActiveProducts,
+                }),
+            };
+        }
+
+        for (const option of optionGroup.options) {
+            const { result, message } = await this.productOptionService.delete(ctx, option.id);
+            if (result === DeletionResult.NOT_DELETED) {
+                await this.connection.rollBackTransaction(ctx);
+                return { result, message };
+            }
+        }
+        const isInUseBySoftDeletedVariants = await this.isInUseByOtherProducts(
+            ctx,
+            optionGroup,
+            productId,
+            'soft-deleted',
+        );
+        if (0 < isInUseBySoftDeletedVariants) {
+            // soft delete
+            optionGroup.deletedAt = new Date();
+            await this.connection.getRepository(ctx, ProductOptionGroup).save(optionGroup, { reload: false });
+        } else {
+            // hard delete
+            // TODO: V2 rely on onDelete: CASCADE rather than this manual loop
+            for (const translation of optionGroup.translations) {
+                await this.connection
+                    .getRepository(ctx, ProductOptionGroupTranslation)
+                    .remove(translation as ProductOptionGroupTranslation);
+            }
+            try {
+                await this.connection.getRepository(ctx, ProductOptionGroup).remove(optionGroup);
+            } catch (e) {
+                Logger.error(e.message, undefined, e.stack);
+            }
+        }
+        this.eventBus.publish(new ProductOptionGroupEvent(ctx, optionGroup, 'deleted', id));
+        return {
+            result: DeletionResult.DELETED,
+        };
+    }
+
+    private async isInUseByOtherProducts(
+        ctx: RequestContext,
+        productOptionGroup: ProductOptionGroup,
+        targetProductId: ID,
+        productState: 'active' | 'soft-deleted',
+    ): Promise<number> {
+        const [products, count] = await this.connection
+            .getRepository(ctx, Product)
+            .createQueryBuilder('product')
+            .leftJoin('product.optionGroups', 'optionGroup')
+            .where(productState === 'active' ? 'product.deletedAt IS NULL' : 'product.deletedAt IS NOT NULL')
+            .andWhere('optionGroup.id = :id', { id: productOptionGroup.id })
+            .andWhere('product.id != :productId', { productId: targetProductId })
+            .getManyAndCount();
+
+        return count;
+    }
 }

packages/core/src/service/services/product-option.service.ts

@@ -11,6 +11,7 @@ import { ID } from '@vendure/common/lib/shared-types';
 import { RequestContext } from '../../api/common/request-context';
 import { Translated } from '../../common/types/locale-types';
 import { assertFound } from '../../common/utils';
+import { Logger } from '../../config/index';
 import { TransactionalConnection } from '../../connection/transactional-connection';
 import { ProductVariant } from '../../entity/index';
 import { ProductOptionGroup } from '../../entity/product-option-group/product-option-group.entity';
@@ -93,31 +94,64 @@ export class ProductOptionService {
         return assertFound(this.findOne(ctx, option.id));
     }
 
-    async softDelete(ctx: RequestContext, id: ID): Promise<DeletionResponse> {
+    /**
+     * @description
+     * Deletes a ProductOption.
+     *
+     * - If the ProductOption is used by any ProductVariants, the deletion will fail.
+     * - If the ProductOption is used only by soft-deleted ProductVariants, the option will itself
+     *   be soft-deleted.
+     * - If the ProductOption is not used by any ProductVariant at all, it will be hard-deleted.
+     */
+    async delete(ctx: RequestContext, id: ID): Promise<DeletionResponse> {
         const productOption = await this.connection.getEntityOrThrow(ctx, ProductOption, id);
-        const consumingVariants = await this.connection
-            .getRepository(ctx, ProductVariant)
-            .createQueryBuilder('variant')
-            .leftJoin('variant.options', 'option')
-            .where('variant.deletedAt IS NULL')
-            .andWhere('option.id = :id', { id })
-            .getMany();
-        if (consumingVariants.length) {
-            const message = ctx.translate('message.product-option-used', {
-                code: productOption.code,
-                count: consumingVariants.length,
-            });
+        const inUseByActiveVariants = await this.isInUse(ctx, productOption, 'active');
+        if (0 < inUseByActiveVariants) {
             return {
                 result: DeletionResult.NOT_DELETED,
-                message,
+                message: ctx.translate('message.product-option-used', {
+                    code: productOption.code,
+                    count: inUseByActiveVariants,
+                }),
             };
-        } else {
+        }
+        const isInUseBySoftDeletedVariants = await this.isInUse(ctx, productOption, 'soft-deleted');
+        if (0 < isInUseBySoftDeletedVariants) {
+            // soft delete
             productOption.deletedAt = new Date();
             await this.connection.getRepository(ctx, ProductOption).save(productOption, { reload: false });
-            this.eventBus.publish(new ProductOptionEvent(ctx, productOption, 'deleted', id));
-            return {
-                result: DeletionResult.DELETED,
-            };
+        } else {
+            // hard delete
+            try {
+                // TODO: V2 rely on onDelete: CASCADE rather than this manual loop
+                for (const translation of productOption.translations) {
+                    await this.connection
+                        .getRepository(ctx, ProductOptionTranslation)
+                        .remove(translation as ProductOptionTranslation);
+                }
+                await this.connection.getRepository(ctx, ProductOption).remove(productOption);
+            } catch (e: any) {
+                Logger.error(e.message, undefined, e.stack);
+            }
         }
+        this.eventBus.publish(new ProductOptionEvent(ctx, productOption, 'deleted', id));
+        return {
+            result: DeletionResult.DELETED,
+        };
+    }
+
+    private async isInUse(
+        ctx: RequestContext,
+        productOption: ProductOption,
+        variantState: 'active' | 'soft-deleted',
+    ): Promise<number> {
+        const [variants, count] = await this.connection
+            .getRepository(ctx, ProductVariant)
+            .createQueryBuilder('variant')
+            .leftJoin('variant.options', 'option')
+            .where(variantState === 'active' ? 'variant.deletedAt IS NULL' : 'variant.deletedAt IS NOT NULL')
+            .andWhere('option.id = :id', { id: productOption.id })
+            .getManyAndCount();
+        return count;
     }
 }

packages/core/src/service/services/product.service.ts

@@ -15,7 +15,7 @@ import { FindOptionsUtils } from 'typeorm';
 import { RequestContext } from '../../api/common/request-context';
 import { RelationPaths } from '../../api/decorators/relations.decorator';
 import { ErrorResultUnion } from '../../common/error/error-result';
-import { EntityNotFoundError } from '../../common/error/errors';
+import { EntityNotFoundError, InternalServerError, UserInputError } from '../../common/error/errors';
 import { ProductOptionInUseError } from '../../common/error/generated-graphql-admin-errors';
 import { ListQueryOptions } from '../../common/types/common-types';
 import { Translated } from '../../common/types/locale-types';
@@ -23,7 +23,6 @@ import { assertFound, idsAreEqual } from '../../common/utils';
 import { TransactionalConnection } from '../../connection/transactional-connection';
 import { Channel } from '../../entity/channel/channel.entity';
 import { FacetValue } from '../../entity/facet-value/facet-value.entity';
-import { Order } from '../../entity/index';
 import { ProductOptionGroup } from '../../entity/product-option-group/product-option-group.entity';
 import { ProductTranslation } from '../../entity/product/product-translation.entity';
 import { Product } from '../../entity/product/product.entity';
@@ -41,6 +40,7 @@ import { AssetService } from './asset.service';
 import { ChannelService } from './channel.service';
 import { CollectionService } from './collection.service';
 import { FacetValueService } from './facet-value.service';
+import { ProductOptionGroupService } from './product-option-group.service';
 import { ProductVariantService } from './product-variant.service';
 import { RoleService } from './role.service';
 import { TaxRateService } from './tax-rate.service';
@@ -69,6 +69,7 @@ export class ProductService {
         private eventBus: EventBus,
         private slugValidator: SlugValidator,
         private customFieldRelationService: CustomFieldRelationService,
+        private productOptionGroupService: ProductOptionGroupService,
     ) {}
 
     async findAll(
@@ -259,15 +260,31 @@ export class ProductService {
     async softDelete(ctx: RequestContext, productId: ID): Promise<DeletionResponse> {
         const product = await this.connection.getEntityOrThrow(ctx, Product, productId, {
             channelId: ctx.channelId,
-            relations: ['variants'],
+            relations: ['variants', 'optionGroups'],
         });
         product.deletedAt = new Date();
         await this.connection.getRepository(ctx, Product).save(product, { reload: false });
         this.eventBus.publish(new ProductEvent(ctx, product, 'deleted', productId));
-        await this.productVariantService.softDelete(
+
+        const variantResult = await this.productVariantService.softDelete(
             ctx,
             product.variants.map(v => v.id),
         );
+        if (variantResult.result === DeletionResult.NOT_DELETED) {
+            await this.connection.rollBackTransaction(ctx);
+            return variantResult;
+        }
+        for (const optionGroup of product.optionGroups) {
+            const groupResult = await this.productOptionGroupService.deleteGroupAndOptionsFromProduct(
+                ctx,
+                optionGroup.id,
+                productId,
+            );
+            if (groupResult.result === DeletionResult.NOT_DELETED) {
+                await this.connection.rollBackTransaction(ctx);
+                return groupResult;
+            }
+        }
         return {
             result: DeletionResult.DELETED,
         };
@@ -344,10 +361,17 @@ export class ProductService {
         const product = await this.getProductWithOptionGroups(ctx, productId);
         const optionGroup = await this.connection
             .getRepository(ctx, ProductOptionGroup)
-            .findOne(optionGroupId);
+            .findOne(optionGroupId, { relations: ['product'] });
         if (!optionGroup) {
             throw new EntityNotFoundError('ProductOptionGroup', optionGroupId);
         }
+        if (optionGroup.product) {
+            const translated = translateDeep(optionGroup.product, ctx.languageCode);
+            throw new UserInputError(`error.product-option-group-already-assigned`, {
+                groupCode: optionGroup.code,
+                productName: translated.name,
+            });
+        }
 
         if (Array.isArray(product.optionGroups)) {
             product.optionGroups.push(optionGroup);
@@ -378,9 +402,17 @@ export class ProductService {
         if (optionIsInUse) {
             return new ProductOptionInUseError(optionGroup.code, product.variants.length);
         }
+        const result = await this.productOptionGroupService.deleteGroupAndOptionsFromProduct(
+            ctx,
+            optionGroupId,
+            productId,
+        );
         product.optionGroups = product.optionGroups.filter(g => g.id !== optionGroupId);
-
         await this.connection.getRepository(ctx, Product).save(product, { reload: false });
+        if (result.result === DeletionResult.NOT_DELETED) {
+            // tslint:disable-next-line:no-non-null-assertion
+            throw new InternalServerError(result.message!);
+        }
         this.eventBus.publish(new ProductOptionGroupChangeEvent(ctx, product, optionGroupId, 'removed'));
         return assertFound(this.findOne(ctx, productId));
     }