feat(core): Add channel-specific ProductOptionGroup and ProductOption #3608
Conversation
…duct - Added Channel relation to ProductOptionGroup and ProductOption. - Add many-to-many relation between Product and ProductOptionGroup. - Updated importer and product-duplicator services to align with the new model.
… ProductOptionGroup
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds channel-awareness for ProductOptionGroup and ProductOption across entities, services, resolvers, and GraphQL types; adds paginated list queries for option groups/options, channel assign/remove mutations with OptionGroupInUseError, deleteProductOptionGroup mutation, importer/duplicator updates, and removes legacy KeyValue GraphQL API. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant AdminAPI as Admin GraphQL API
participant OptionGroupSvc as ProductOptionGroupService
participant ChannelSvc as ChannelService
participant VariantRepo as Variant/Usage Check
rect rgb(235, 245, 255)
note over Admin,AdminAPI: Assign option groups to channel
Admin->>AdminAPI: mutation assignOptionGroupsToChannel(input)
AdminAPI->>OptionGroupSvc: assignGroupsToChannel(ctx, input)
OptionGroupSvc->>ChannelSvc: getChannelOrThrow(channelId)
OptionGroupSvc->>OptionGroupSvc: load groups by ids (channel-aware)
OptionGroupSvc-->>AdminAPI: [ProductOptionGroup]
AdminAPI-->>Admin: result
end
rect rgb(245, 235, 255)
note over Admin,AdminAPI: Remove option groups from channel
Admin->>AdminAPI: mutation removeOptionGroupsFromChannel(input)
AdminAPI->>OptionGroupSvc: removeGroupsFromChannel(ctx, input)
OptionGroupSvc->>VariantRepo: check usage of options/variants
alt in use and not forced
OptionGroupSvc-->>AdminAPI: [OptionGroupInUseError...]
else removable or forced
OptionGroupSvc->>ChannelSvc: remove from channel
OptionGroupSvc-->>AdminAPI: [ProductOptionGroup...]
end
AdminAPI-->>Admin: result union (error or group)
end
sequenceDiagram
autonumber
actor Admin
participant AdminAPI as Admin GraphQL API
participant OptionGroupSvc as ProductOptionGroupService
participant OptionSvc as ProductOptionService
rect rgb(240, 255, 240)
note over Admin,AdminAPI: List groups/options (paginated)
Admin->>AdminAPI: query productOptionGroupList(options)
AdminAPI->>OptionGroupSvc: findAllList(ctx, options, relations)
OptionGroupSvc-->>AdminAPI: ProductOptionGroupList
AdminAPI-->>Admin: list
Admin->>AdminAPI: field ProductOptionGroup.optionList(options)
AdminAPI->>OptionSvc: findByGroupIdList(ctx, groupId, options, relations)
OptionSvc-->>AdminAPI: ProductOptionList
AdminAPI-->>Admin: list
end
sequenceDiagram
autonumber
actor Admin
participant AdminAPI as Admin GraphQL API
participant OptionGroupSvc as ProductOptionGroupService
rect rgb(255, 245, 235)
note over Admin,AdminAPI: Delete product option group
Admin->>AdminAPI: mutation deleteProductOptionGroup(id)
AdminAPI->>OptionGroupSvc: deleteGroupAndOptions(ctx, id)
OptionGroupSvc->>OptionGroupSvc: load group (channel/product-scoped) with options
OptionGroupSvc->>OptionGroupSvc: remove refs from product optionGroups and __optionGroups
OptionGroupSvc-->>AdminAPI: DeletionResponse
AdminAPI-->>Admin: result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (14)
.github/workflows/stale_bot.yml (4)
8-10: Scope down permissions for pull requests
This workflow only acts on issues; you can consider removing thepull-requests: writepermission to follow least privilege.
16-16: Pin the stale action to a specific patch version
Usingactions/stale@v9will pull in all v9 releases; pinning to a full version or SHA would improve reproducibility.
23-26: Refactor issue label filters into YAML array
Multi-line comma-separated strings work, but convertingany-of-issue-labelsto a YAML list would improve readability and maintenance.
28-46: Convert exempt labels block to YAML sequence
The longexempt-issue-labelspipe block could be clearer as an array of entries rather than a comma-delimited string.docs/docs/guides/developer-guide/custom-strategies-in-plugins/index.mdx (1)
401-401: Minor grammar improvement suggestionThe sentence could benefit from a comma for better readability.
-Always provide a default implementation so users can use your plugin out-of-the-box: +Always provide a default implementation, so users can use your plugin out-of-the-box:docs/sidebars.js (1)
177-177: Consider maintaining multi-line array format for better maintainabilityWhile the single-line format works, the multi-line format is often preferred for arrays in configuration files as it makes diffs cleaner when adding new items.
- items: ['guides/extending-the-dashboard/getting-started/index'], + items: [ + 'guides/extending-the-dashboard/getting-started/index' + ],packages/core/src/service/services/product-variant.service.ts (1)
912-918: Good implementation of dual option group relations, but consider documenting the purpose.The code correctly handles both
optionGroupsand__optionGroupsrelations by fetching and merging them. However, the purpose of having two separate relations isn't clear from the code.Consider adding a comment explaining why both relations exist:
+ // Fetch both optionGroups and __optionGroups relations + // optionGroups: legacy/existing relation + // __optionGroups: new channel-aware relation const productWithGroups = await this.connection.getEntityOrThrow(ctx, Product, productId, {packages/dashboard/vite/vite-plugin-transform-index.ts (2)
28-29: Consider escaping special regex characters in base path.The current regex construction assumes the base path doesn't contain special regex characters. If the base path contains characters like
.,[,], etc., the regex could behave unexpectedly.Consider escaping the base path:
- const attributeRegex = new RegExp(`(href|src)=(["'])${basePath}/?`, 'g'); + const escapedBasePath = basePath.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const attributeRegex = new RegExp(`(href|src)=(["'])${escapedBasePath}/?`, 'g');
32-33: Improve base tag insertion robustness.The current approach assumes a simple
<head>tag format. Consider making the insertion more robust to handle various head tag formats (with attributes, whitespace, etc.).- const baseTag = ` <base href="${config.base}">\n`; - transformedHtml = transformedHtml.replace(/<head>/, `<head>\n${baseTag}`); + const baseTag = `<base href="${config.base}">`; + transformedHtml = transformedHtml.replace(/(<head[^>]*>)/i, `$1\n ${baseTag}`);docs/docs/guides/getting-started/installation/index.md (2)
42-42: Consider removing hyphen from "fully-functional" for better style.The compound modifier "fully functional" doesn't require a hyphen since "fully" is an adverb ending in "ly".
-After a minute or two, you'll have a **fully-functional Vendure server** installed locally. +After a minute or two, you'll have a **fully functional Vendure server** installed locally.
71-71: Consider using "who" instead of "that" when referring to people.When referring to a database user (person), "who" is more appropriate than "that".
-You need the username and password for a database user that has full permissions +You need the username and password for a database user who has full permissionspackages/core/src/entity/product-option-group/product-option-group.entity.ts (1)
45-56: Consider documenting the dual relationship patternThe entity now supports both single (
product) and multiple (products) product associations. This dual pattern might be confusing for developers. Consider:
- Adding JSDoc comments explaining when to use each relation
- Documenting the migration path from single to multiple associations
- Clarifying the
__optionGroupsnaming convention (the double underscore typically indicates internal/private usage)Add documentation above the relations:
+ /** + * @deprecated Use `products` relation for new implementations. + * Maintained for backward compatibility with existing per-product option groups. + */ @Index() @ManyToOne(type => Product, product => product.optionGroups) product?: Product; @EntityId({ nullable: true }) productId?: ID; + /** + * Products associated with this option group. + * Uses internal __optionGroups relation name to avoid conflicts. + */ @ManyToMany(type => Product, product => product.__optionGroups) products: Product[];packages/dashboard/src/lib/components/shared/paginated-list-data-table.tsx (1)
418-422: Consider type-safe column orderingThe
as anycasts in the column ordering logic might hide type issues. While they may be necessary due to the complex generic types, consider if there's a more type-safe approach.Consider extracting the column ID type for better type safety:
+type ColumnId<T extends TypedDocumentNode<any, any>, AC extends AdditionalColumns<T>> = + keyof ListQueryFields<T> | keyof AC | CustomFieldKeysOfItem<ListQueryFields<T>>; if (defaultColumnOrder) { const orderedColumns = finalColumns - .filter(column => column.id && defaultColumnOrder.includes(column.id as any)) - .sort((a, b) => defaultColumnOrder.indexOf(a.id as any) - defaultColumnOrder.indexOf(b.id as any)); + .filter(column => column.id && defaultColumnOrder.includes(column.id as ColumnId<T, AC>)) + .sort((a, b) => defaultColumnOrder.indexOf(a.id as ColumnId<T, AC>) - defaultColumnOrder.indexOf(b.id as ColumnId<T, AC>)); const remainingColumns = finalColumns.filter( - column => !column.id || !defaultColumnOrder.includes(column.id as any), + column => !column.id || !defaultColumnOrder.includes(column.id as ColumnId<T, AC>), );packages/core/src/service/services/product-option-group.service.ts (1)
78-96: Implement type safety for the returned PaginatedList.The new
findAllListmethod correctly implements pagination and channel filtering. However, consider adding explicit return type annotation for better type safety.Consider adding explicit return type:
findAllList( ctx: RequestContext, options?: ListQueryOptions<ProductOptionGroup>, relations?: RelationPaths<ProductOptionGroup>, -): Promise<PaginatedList<Translated<ProductOptionGroup>>> { +): Promise<PaginatedList<Translated<ProductOptionGroup>>> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.github/workflows/publish_master_to_npm.yml(1 hunks).github/workflows/stale_bot.yml(1 hunks)docs/docs/guides/developer-guide/custom-strategies-in-plugins/index.mdx(1 hunks)docs/docs/guides/getting-started/installation/index.md(2 hunks)docs/sidebars.js(3 hunks)packages/admin-ui/src/lib/core/src/common/generated-types.ts(7 hunks)packages/admin-ui/src/lib/core/src/common/introspection-result.ts(1 hunks)packages/cli/src/commands/migrate/load-vendure-config-file.ts(2 hunks)packages/common/src/generated-types.ts(7 hunks)packages/core/e2e/shipping-method.e2e-spec.ts(1 hunks)packages/core/src/api/resolvers/admin/product-option.resolver.ts(3 hunks)packages/core/src/api/resolvers/entity/product-option-group-entity.resolver.ts(2 hunks)packages/core/src/api/schema/admin-api/product-option-group.api.graphql(2 hunks)packages/core/src/api/schema/common/product-option-group.type.graphql(2 hunks)packages/core/src/config/entity/entity-duplicators/product-duplicator.ts(3 hunks)packages/core/src/config/shipping-method/default-shipping-calculator.ts(1 hunks)packages/core/src/config/tax/address-based-tax-zone-strategy.spec.ts(2 hunks)packages/core/src/config/tax/address-based-tax-zone-strategy.ts(2 hunks)packages/core/src/data-import/providers/importer/fast-importer.service.ts(3 hunks)packages/core/src/data-import/providers/importer/importer.ts(2 hunks)packages/core/src/entity/channel/channel.entity.ts(2 hunks)packages/core/src/entity/product-option-group/product-option-group.entity.ts(2 hunks)packages/core/src/entity/product-option/product-option.entity.ts(2 hunks)packages/core/src/entity/product/product.entity.ts(1 hunks)packages/core/src/service/services/product-option-group.service.ts(12 hunks)packages/core/src/service/services/product-option.service.ts(10 hunks)packages/core/src/service/services/product-variant.service.ts(1 hunks)packages/core/src/service/services/product.service.ts(5 hunks)packages/dashboard/src/app/main.tsx(2 hunks)packages/dashboard/src/app/routes/_authenticated/_product-variants/product-variants.tsx(2 hunks)packages/dashboard/src/app/routes/_authenticated/_products/components/product-variants-table.tsx(2 hunks)packages/dashboard/src/app/routes/_authenticated/_products/products.graphql.ts(1 hunks)packages/dashboard/src/lib/components/shared/detail-page-button.tsx(1 hunks)packages/dashboard/src/lib/components/shared/paginated-list-data-table.tsx(4 hunks)packages/dashboard/src/lib/components/shared/stock-level-label.tsx(1 hunks)packages/dashboard/src/lib/framework/defaults.ts(6 hunks)packages/dashboard/src/lib/framework/page/list-page.tsx(1 hunks)packages/dashboard/src/lib/index.ts(1 hunks)packages/dashboard/vite/vite-plugin-config.ts(1 hunks)packages/dashboard/vite/vite-plugin-transform-index.ts(1 hunks)packages/dashboard/vite/vite-plugin-vendure-dashboard.ts(2 hunks)packages/dev-server/vite.config.mts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/docs/guides/getting-started/installation/index.md
[uncategorized] ~11-~11: Possible missing comma found.
Context: ...(Odd-numbered versions should still work but are not officially supported.) ### Opt...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~16-~16: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ill be used for your database. * If you want to use an existing MySQL, MariaDB, or Post...
(REP_WANT_TO_VB)
[uncategorized] ~42-~42: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ... After a minute or two, you'll have a fully-functional Vendure server installed locally. On...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[style] ~71-~71: Consider using “who” when you are referring to a person instead of an object.
Context: ...ername and password for a database user that has full permissions (CREATE, ALTER, DR...
(THAT_WHO)
docs/docs/guides/developer-guide/custom-strategies-in-plugins/index.mdx
[uncategorized] ~401-~401: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... Always provide a default implementation so users can use your plugin out-of-the-bo...
(COMMA_COMPOUND_SENTENCE_2)
🪛 Biome (1.9.4)
packages/core/src/config/entity/entity-duplicators/product-duplicator.ts
[error] 151-151: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (89)
packages/cli/src/commands/migrate/load-vendure-config-file.ts (3)
2-2: LGTM: Import addition supports improved file handling.The addition of
readFileSyncimport enables explicit file reading, which is more robust than the previous directrequireapproach for JSON configuration files.
30-54: Excellent improvement in error handling and robustness.The updated file loading logic provides significant improvements:
✅ Strengths:
- Explicit file reading with proper error handling
- Detailed error messages including file paths
- Comment stripping before JSON parsing
- Maintains the same function interface
✅ Error handling: Both file reading and JSON parsing are wrapped in try-catch blocks with descriptive error messages.
✅ Approach: Using
readFileSync+JSON.parseis more appropriate thanrequirefor JSON configuration files that may contain comments.
10-15: Verify comment stripping logic for edge cases.The
stripJsonCommentsfunction uses a simple regex-based approach that should work for typicaltsconfig.jsonfiles. However, consider potential edge cases:
- Comments inside string literals would be incorrectly removed
- Nested block comments might not be handled properly
Let me verify if there are robust alternatives available:
#!/bin/bash # Check if there are existing robust JSON-with-comments parsing libraries npm search strip-json-comments --no-description 2>/dev/null | head -10packages/core/e2e/shipping-method.e2e-spec.ts (1)
139-139: LGTM! Tax rate type change improves precision.The change from
'int'to'float'for thetaxRateargument type allows for more precise tax rate configurations (e.g., 7.5% instead of being limited to whole percentages).packages/core/src/config/shipping-method/default-shipping-calculator.ts (1)
46-46: LGTM! Enhanced precision for tax rate configuration.Changing the taxRate type from
'int'to'float'allows for more accurate tax rate specifications while maintaining backward compatibility.packages/core/src/config/tax/address-based-tax-zone-strategy.spec.ts (2)
3-3: LGTM! Import path improvement.The change to relative imports is a good practice for internal module dependencies.
20-27: LGTM! Test updated to reflect shipping address-based tax zone determination.The test name and logic have been correctly updated to verify that tax zone determination is now based solely on the shipping address country, which aligns with the OSS VAT scheme requirements.
packages/core/src/config/tax/address-based-tax-zone-strategy.ts (2)
12-13: LGTM! Documentation accurately reflects the new behavior.The updated documentation clearly explains that tax zone determination is now based solely on the shipping address country, which is appropriate for OSS VAT compliance.
42-42: LGTM! Implementation correctly focuses on shipping address for tax zone determination.The change to use only
order?.shippingAddress?.countryCodeproperly implements the OSS VAT scheme requirement where tax zones should be determined by the destination country (shipping address) rather than billing address.This represents a behavioral change from the previous implementation. Please ensure that:
- Existing merchants using this strategy are informed about this change
- The change is properly documented in release notes
- Consider if a migration guide is needed for merchants who were relying on the billing address fallback
.github/workflows/publish_master_to_npm.yml (1)
8-11: Restrict workflow triggers to package-related changes
Thepathsfilter ensures this workflow only runs on modifications underpackages/**,package.json, orpackage-lock.json, preventing unnecessary runs (e.g., fromlerna.jsonversion bumps)..github/workflows/stale_bot.yml (4)
1-7: Daily schedule with manual dispatch is correctly configured
Runs at 02:00 UTC every day and supports manual triggers viaworkflow_dispatch.
18-19: Stale thresholds look reasonable
days-before-stale: 30anddays-before-close: 3align with typical repository maintenance intervals.
20-21: PR filtering ensures no PRs are touched
Settingonly-pr-labels: 'no-response'effectively skips all pull requests unless explicitly labeled, matching the “issues-only” intent.
51-56: Close message and batch size are well set
The custom close message with placeholder andoperations-per-run: 1000are appropriate for high-volume repositories.packages/admin-ui/src/lib/core/src/common/introspection-result.ts (1)
1-267: LGTM - Clean formatting improvementThe file has been properly reformatted to remove excessive indentation while preserving all GraphQL type mappings. This improves code readability and consistency.
However, this formatting change doesn't appear to relate to the stated PR objective of adding channel-specific functionality for ProductOptionGroup and ProductOption.
Likely an incorrect or invalid review comment.
docs/docs/guides/developer-guide/custom-strategies-in-plugins/index.mdx (1)
1-468: Excellent comprehensive documentation for custom strategiesThis is a well-written, thorough guide that covers all aspects of implementing custom strategies in Vendure plugins. The structure is logical, examples are practical, and best practices are clearly outlined.
However, this documentation about custom strategies doesn't appear to relate to the stated PR objective of adding channel-specific functionality for ProductOptionGroup and ProductOption. This suggests the files may be from a different PR or the summary may be incorrect.
Likely an incorrect or invalid review comment.
docs/sidebars.js (2)
33-33: Good formatting fix for React logo SVGAdding the trailing comma improves consistency with the rest of the icon definitions.
103-103: Documentation entry added correctlyThe new custom strategies guide is properly positioned in the "Advanced Topics" section where it belongs.
However, this addition doesn't relate to the stated PR objective about channel-specific ProductOptionGroup/ProductOption functionality.
Likely an incorrect or invalid review comment.
packages/dashboard/vite/vite-plugin-vendure-dashboard.ts (2)
13-13: Good addition of HTML transform plugin importThe import follows the established pattern for other plugin imports in this file.
96-96: Plugin integration follows correct patternThe
transformIndexHtmlPlugin()is properly added to the plugin array and will handle HTML transformations for the dashboard base path configuration.However, this dashboard infrastructure change doesn't appear to relate to the stated PR objective about channel-specific ProductOptionGroup/ProductOption functionality.
Likely an incorrect or invalid review comment.
packages/dev-server/vite.config.mts (1)
7-7: Appropriate base path configuration for dashboardSetting
base: '/dashboard/'correctly configures the public path for serving the dashboard from a subpath. This works in conjunction with thetransformIndexHtmlPluginto handle asset paths properly.However, this dashboard configuration change doesn't appear to relate to the stated PR objective about adding channel-specific functionality for ProductOptionGroup and ProductOption.
Likely an incorrect or invalid review comment.
packages/dashboard/vite/vite-plugin-config.ts (1)
29-29: LGTM! Simplification of default output directory.The change from
'dist/vendure-dashboard'to'dist'simplifies the default build output path while maintaining all the existing logic for handling absolute/relative paths.packages/dashboard/src/lib/framework/defaults.ts (1)
40-228: LGTM! Explicit ordering improves navigation maintainability.Adding explicit
orderproperties to all navigation menu items is a great improvement that:
- Makes the intended menu structure clear and maintainable
- Allows for easy insertion of new items between existing ones
- Provides predictable ordering behavior
The ordering scheme with increments of 100 leaves plenty of room for future additions.
packages/dashboard/src/lib/index.ts (1)
70-70: LGTM! Proper addition of new component export.The export of
stock-level-label.jsfollows the established pattern and makes the new component available for reuse throughout the dashboard.packages/core/src/entity/product/product.entity.ts (1)
66-68: Request clarification on dual ProductOptionGroup relationships.The addition of
__optionGroups(many-to-many) alongside the existingoptionGroups(one-to-many) relationship raises questions about:
- Purpose: Why are two different relationships to
ProductOptionGroupneeded?- Lifecycle: Is this a migration strategy? Will one eventually replace the other?
- Data consistency: How will the system ensure consistency between these relationships?
- Naming: The double underscore prefix suggests internal use - is this intentional?
Please clarify the intended usage pattern and lifecycle of these relationships to ensure proper implementation and avoid potential data consistency issues.
packages/dashboard/src/app/routes/_authenticated/_products/products.graphql.ts (1)
74-77: LGTM! Useful additions for inventory management.The addition of
stockLevelswithstockOnHandandstockAllocatedfields provides essential inventory data for the dashboard, and thetotalItemsfield enables proper pagination support. These changes align well with the newStockLevelLabelcomponent mentioned in the AI summary.Also applies to: 79-79
packages/dashboard/src/lib/framework/page/list-page.tsx (1)
8-8: LGTM! Good refactoring to use shared type definition.Moving
ListQueryFieldsto a shared module improves code reusability and reduces duplication.packages/core/src/entity/product-option/product-option.entity.ts (3)
2-2: LGTM! Correct import addition for join table decorator.
4-4: LGTM! Proper import for Channel entity.
47-49: LGTM! Correctly implemented many-to-many relationship with Channel.The relationship is properly defined with ProductOption as the owner side using
@JoinTable(). This establishes the foundation for channel-specific product options.packages/core/src/data-import/providers/importer/importer.ts (2)
15-15: Minor: Import reordering is cosmetic.
216-216: ```shell
#!/bin/bashInspect the createProductOptionGroup method for uniqueness validation
rg -n "createProductOptionGroup" -A15 -B5 packages/core/src/service/services/product-option-group.service.ts
</details> <details> <summary>packages/core/src/entity/channel/channel.entity.ts (3)</summary> `5-14`: **LGTM! Proper imports for the new entity relationships.** --- `20-22`: **LGTM! Improved import organization.** --- `154-158`: **LGTM! Correctly implemented inverse side of many-to-many relationships.** The relationships properly reference the inverse sides without `@JoinTable()`, making Channel the non-owning side. This completes the bidirectional many-to-many relationships with ProductOptionGroup and ProductOption entities. </details> <details> <summary>packages/dashboard/src/lib/components/shared/stock-level-label.tsx (1)</summary> `8-24`: **LGTM! Well-implemented component with good practices.** The component demonstrates excellent practices: - Proper input validation with early return - Clear aggregation logic using reduce - Accessible tooltip with internationalized text - Clean separation of concerns </details> <details> <summary>packages/core/src/config/entity/entity-duplicators/product-duplicator.ts (2)</summary> `70-72`: **LGTM! Correct inclusion of new __optionGroups relation.** The addition of the `__optionGroups` relation to the product fetch is appropriate for supporting the new many-to-many relationship structure. --- `157-160`: **LGTM! Correctly handles dual relation structure.** The query correctly searches for option groups using both the new `products` many-to-many relation and the existing `product` one-to-many relation, ensuring compatibility with the enhanced data model. </details> <details> <summary>packages/dashboard/src/lib/components/shared/detail-page-button.tsx (1)</summary> `5-27`: **LGTM! Well-designed flexibility enhancement.** The changes improve component flexibility while maintaining backwards compatibility: - Both `id` and `href` props are properly optional - Sensible fallback to plain `<span>` when neither is provided - Clear precedence logic favoring `href` over constructed path - Maintains existing functionality for existing consumers </details> <details> <summary>packages/dashboard/src/app/routes/_authenticated/_products/components/product-variants-table.tsx (1)</summary> `1-66`: **LGTM! Excellent refactoring that improves code quality.** The changes demonstrate excellent refactoring practices: - Effective use of new shared components (`StockLevelLabel`, `DetailPageButton`) - Simplified column rendering logic with cleaner data access - Enhanced user experience with the new stock levels display - Consistent navigation patterns throughout the table - Improved code maintainability by leveraging reusable components </details> <details> <summary>packages/dashboard/src/app/main.tsx (3)</summary> `29-30`: **LGTM: Conditional basepath configuration improves deployment flexibility.** The conditional basepath setting allows the dashboard to be served from a subpath when `BASE_URL` is defined, which enhances deployment options without breaking existing configurations. --- `70-70`: **LGTM: Moving registerDefaults to useEffect ensures proper initialization timing.** Calling `registerDefaults()` inside the useEffect on component mount ensures it runs at the appropriate time in the component lifecycle. --- `73-77`: **LGTM: Extension callbacks properly synchronized with extensions loading state.** The new useEffect correctly waits for extensions to load before executing callbacks, preventing race conditions in extension initialization. </details> <details> <summary>packages/core/src/api/schema/common/product-option-group.type.graphql (2)</summary> `9-12`: **LGTM: Well-documented paginated optionList field enhances querying capabilities.** The new `optionList` field provides paginated access to ProductOptionGroup options with clear documentation, supporting the enhanced querying capabilities mentioned in the PR objectives. --- `44-47`: **LGTM: Consistent PaginatedList implementation follows established patterns.** Both `ProductOptionGroupList` and `ProductOptionList` types correctly implement the `PaginatedList` interface with standard `items` and `totalItems` fields, maintaining consistency with existing paginated types in the schema. Also applies to: 52-55 </details> <details> <summary>packages/core/src/api/resolvers/entity/product-option-group-entity.resolver.ts (1)</summary> `52-60`: **LGTM: Well-implemented field resolver with proper service integration.** The `optionList` resolver method correctly: - Uses the injected `ProductOptionService` - Passes through the pagination options and relations - Returns the expected `PaginatedList<ProductOption>` type - Follows the established resolver pattern in the codebase This implementation properly supports the new paginated querying functionality for ProductOptionGroup options. </details> <details> <summary>packages/core/src/api/schema/admin-api/product-option-group.api.graphql (3)</summary> `3-3`: **LGTM: New queries provide comprehensive paginated access to product options.** The `productOptionGroupList` and `productOptions` queries enable paginated, filterable access to product option groups and options respectively, supporting the enhanced admin functionality described in the PR objectives. Also applies to: 5-5 --- `13-14`: **LGTM: Delete mutation completes CRUD operations for ProductOptionGroup.** The `deleteProductOptionGroup` mutation provides necessary deletion functionality, returning a standard `DeletionResponse` type consistent with other deletion operations in the API. --- `64-65`: To confirm that `ProductOptionGroupListOptions` includes pagination, sorting, and filtering, let’s extract its definition from the generated schema: ```shell #!/bin/bash # Print the fields of the generated ProductOptionGroupListOptions input type python3 - << 'EOF' import json schema = json.load(open('schema-admin.json')) for t in schema['data']['__schema']['types']: if t.get('kind') == 'INPUT_OBJECT' and t.get('name') == 'ProductOptionGroupListOptions': print(json.dumps(t, indent=2)) EOFdocs/docs/guides/getting-started/installation/index.md (1)
11-136: LGTM: Documentation improvements enhance user experience.The documentation updates provide clearer instructions, better formatting, and more comprehensive troubleshooting guidance, which will improve the installation experience for new users.
packages/dashboard/src/app/routes/_authenticated/_product-variants/product-variants.tsx (1)
24-39: Clean refactoring of cell renderersThe consistent use of destructuring pattern and extraction of stock level rendering to a dedicated component improves code maintainability and follows React best practices.
packages/core/src/api/resolvers/admin/product-option.resolver.ts (1)
46-114: Well-structured GraphQL resolver additionsThe new paginated queries and deletion mutation follow established patterns with proper permission checks, context handling, and type safety. The cascading deletion approach for option groups is appropriate.
packages/core/src/data-import/providers/importer/fast-importer.service.ts (1)
153-161: Verify the relation name change impactThe relation name has been changed from
'optionGroups'to'__optionGroups'. Ensure this change is consistently applied across the codebase.#!/bin/bash # Search for other occurrences of the old relation name that might need updating rg -A 3 "\.relation\(['\"]optionGroups['\"]" --type ts # Also check for any direct access to the optionGroups property rg "\.optionGroups\b" --type ts -A 2packages/dashboard/src/lib/components/shared/paginated-list-data-table.tsx (1)
117-124: Good type extraction utilityThe
ListQueryFieldstype properly extracts item types from paginated query results, improving type safety across the component.packages/core/src/service/services/product.service.ts (3)
281-316: Changes to soft delete handling look good.The addition of
optionGroupsto the relations and passingproductIdtodeleteGroupAndOptionsproperly supports the new scoped deletion model for product option groups.
431-433: Comprehensive option group removal handling.The implementation correctly:
- Checks both
optionGroupsand__optionGroupsfor backward compatibility- Handles deletion of product-specific option groups
- Removes the option group from both collections
This maintains compatibility while supporting the new many-to-many relationship model.
Also applies to: 459-472
484-484: Correctly loads both option group relations.Adding
__optionGroupsto the relations ensures both the legacy and new option group associations are loaded.packages/admin-ui/src/lib/core/src/common/generated-types.ts (7)
2895-2896: LGTM: Consistent deletion mutation patternThe new
deleteProductOptionGroupmutation follows the same pattern as other deletion mutations in the schema, returning aDeletionResponsetype.
3473-3475: LGTM: Standard mutation arguments structureThe
MutationDeleteProductOptionGroupArgstype correctly defines the requiredidparameter for the deletion operation, consistent with other deletion mutation argument types.
4817-4827: LGTM: Comprehensive filtering capabilitiesThe
ProductOptionFilterParameterincludes appropriate logical operators (_and,_or) and field-specific filters. The structure follows GraphQL filtering best practices.
4837-4885: LGTM: Well-structured pagination and filtering for ProductOptionGroupThe additions provide comprehensive functionality:
optionListfield with pagination support onProductOptionGroup- Complete filter, sort, and pagination options
- Consistent naming and structure following GraphQL Connection patterns
The implementation supports advanced querying capabilities as mentioned in the AI summary.
4911-4937: LGTM: Consistent ProductOption list operationsThe
ProductOptionListand related types mirror the structure established forProductOptionGroup, providing symmetrical functionality for both entities. The sort parameters includegroupIdwhich is appropriate for the ProductOption entity.
5315-5317: LGTM: New query fields align with schema requirementsThe new query fields
productOptionGroupListandproductOptionsprovide the necessary endpoints for paginated, filterable access to these entities as described in the PR objectives.
5510-5522: LGTM: Proper query argument typesThe query argument types
QueryProductOptionGroupListArgsandQueryProductOptionsArgscorrectly reference their respective options types, maintaining consistency with the GraphQL schema structure.packages/core/src/service/services/product-option-group.service.ts (9)
52-77: Deprecation approach looks good.The deprecation of
findAllin favor offindAllListis properly documented with JSDoc.
141-143: Good addition for channel assignment.The channel assignment in the
beforeSavehook ensures new option groups are properly scoped to the current channel.
167-167: Consistent use of internal method.Using
findOneInternalinstead offindOneensures channel-aware fetching after updates.
176-178: Improved flexibility in the delete method.The optional
productIdparameter and conditional where clause provide better flexibility for different deletion contexts.
195-195: Simplified null check logic.Removing the redundant null check simplifies the code since TypeScript already ensures
optionsarray items won't be null.
240-257: Well-structured conditional logic for product usage check.The method properly handles two scenarios: checking usage by other products when a specific product is provided, and checking overall usage when no product is specified.
215-219: ```shell
#!/bin/bashSearch for any occurrences of __optionGroups in the repo to confirm its definition
rg "__optionGroups" -n -C3
--- `269-280`: **Add null check for better error handling.** The `findOneInternal` method correctly enforces channel membership, but could benefit from null checking before accessing properties. Add null safety check: ```diff private async findOneInternal( ctx: RequestContext, optionId: ID, ): Promise<Translated<ProductOptionGroup> | undefined> { - const groupOption = await this.connection.getEntityOrThrow(ctx, ProductOptionGroup, optionId, { - relations: ['options', 'channels'], - }); + const groupOption = await this.connection.getRepository(ctx, ProductOptionGroup).findOne({ + where: { id: optionId }, + relations: ['options', 'channels'], + }); + if (!groupOption) { + return undefined; + } if (!(groupOption.channels.some(c => c.id === ctx.channelId) || groupOption.productId)) { return undefined; } return this.translator.translate(groupOption, ctx, ['options']); }Likely an incorrect or invalid review comment.
120-124: ```shell
#!/bin/bashInspect relation decorators in the ProductOptionGroup entity files
for file in packages/core/src/entity/product-option-group/*.entity.ts; do
echo "=== $file ==="
rg -n "@(ManyToMany|OneToMany|ManyToOne|OneToOne)" -n "$file"
echo
done</details> <details> <summary>packages/core/src/service/services/product-option.service.ts (6)</summary> `52-63`: **Consistent deprecation approach.** The deprecation follows the same pattern as in ProductOptionGroupService. --- `65-83`: **Consistent pagination implementation.** The `findAllList` method follows the same pattern as in ProductOptionGroupService with proper channel filtering. --- `95-120`: **Well-implemented group-specific query method.** The `findByGroupIdList` method provides a focused way to query options by group with pagination support. --- `136-139`: **Consistent channel assignment pattern.** The channel assignment follows the same pattern as in ProductOptionGroupService. --- `160-160`: **Consistent use of internal method.** Using `findOneInternal` maintains consistency with ProductOptionGroupService. --- `173-176`: **Good error handling improvement.** Throwing `EntityNotFoundError` when the option is not found provides better error handling than returning a generic response. </details> <details> <summary>packages/common/src/generated-types.ts (10)</summary> `2883-2884`: **LGTM: Consistent delete mutation implementation** The new `deleteProductOptionGroup` mutation follows the established pattern used by other delete operations in the schema, with proper argument structure and return type. Also applies to: 3449-3451 --- `4742-4752`: **LGTM: Comprehensive filter parameter implementation** The `ProductOptionFilterParameter` includes all standard filtering capabilities and appropriately includes `groupId` for filtering options by their parent group. --- `4762-4763`: **LGTM: Well-structured paginated relationship** The new `optionList` field on `ProductOptionGroup` provides paginated access to options, following the same pattern as other paginated relationships in the schema. Also applies to: 4770-4772 --- `4774-4783`: **LGTM: Standard filter parameter implementation** `ProductOptionGroupFilterParameter` includes all appropriate filtering fields and follows the established schema patterns. --- `4785-4789`: **LGTM: Standard paginated list implementation** Both `ProductOptionGroupList` and `ProductOptionGroupListOptions` follow the established pagination patterns used consistently throughout the schema. Also applies to: 4791-4802 --- `4804-4810`: **LGTM: Appropriate sort parameter fields** `ProductOptionGroupSortParameter` includes all relevant sortable fields for ProductOptionGroup entities. --- `4836-4840`: **LGTM: Consistent list type implementation** `ProductOptionList` and `ProductOptionListOptions` maintain consistency with other paginated list implementations in the schema. Also applies to: 4842-4853 --- `4855-4862`: **LGTM: Well-designed sort parameters** `ProductOptionSortParameter` appropriately includes `groupId` along with standard sortable fields, enabling sorting by parent group. --- `5239-5239`: **LGTM: Essential query fields with proper structure** The new `productOptionGroupList` and `productOptions` query fields provide necessary list querying capabilities with consistent naming and argument structure. Also applies to: 5241-5241, 5432-5434, 5442-5444 --- `2883-2884`: **Excellent implementation of comprehensive ProductOption entity support** This update successfully adds complete CRUD and query capabilities for ProductOptionGroup and ProductOption entities. The implementation demonstrates: - **Consistency**: All new types follow established GraphQL schema patterns - **Completeness**: Full support for pagination, filtering, sorting, and deletion - **Quality**: Proper field selection and naming conventions throughout - **Backward Compatibility**: All changes are additive and non-breaking The additions align perfectly with the PR objective of adding channel-specific functionality while maintaining schema consistency. Also applies to: 3449-3451, 4742-4752, 4762-4763, 4770-4772, 4774-4783, 4785-4789, 4791-4802, 4804-4810, 4836-4840, 4842-4853, 4855-4862, 5239-5239, 5241-5241, 5432-5434, 5442-5444 </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
packages/core/src/config/entity/entity-duplicators/product-duplicator.ts
Outdated
Show resolved
Hide resolved
packages/core/src/data-import/providers/importer/fast-importer.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/service/services/product-option.service.ts (1)
238-266: Consider query optimization for large datasets.The
checkOptionUsagemethod provides valuable dependency checking functionality. However, the multiple JOIN operations could be expensive for large datasets.Consider adding query optimization for better performance:
async checkOptionUsage( ctx: RequestContext, optionIds: ID[], channelId?: ID, ): Promise<{ variantCount: number }> { const consumingVariantsQb = this.connection .getRepository(ctx, ProductVariant) .createQueryBuilder('variant') .leftJoinAndSelect('variant.options', 'options') .where('options.id IN (:...optionIds)', { optionIds }) .andWhere('variant.deletedAt IS NULL'); if (channelId) { consumingVariantsQb - .leftJoin('variant.channels', 'variant_channel') - .leftJoin('options.channels', 'channel') - .andWhere('variant_channel.id = :channelId') - .andWhere('channel.id = :channelId') + .innerJoin('variant.channels', 'variant_channel', 'variant_channel.id = :channelId') + .innerJoin('options.channels', 'channel', 'channel.id = :channelId') .setParameter('channelId', channelId); } return { variantCount: await consumingVariantsQb.getCount(), }; }Using
innerJoininstead ofleftJoinwith WHERE conditions can be more efficient when you know the relationships should exist.packages/core/src/service/services/product-option-group.service.ts (1)
131-150: Address the TODO comment and verify channel filteringThe method looks good overall but has an unimplemented TODO for channelLanguageCode. Additionally, verify that the current implementation properly filters by channel.
The TODO comment indicates incomplete implementation. The current query doesn't include channel filtering, which might be intentional for global option groups, but needs verification.
#!/bin/bash # Check if other similar methods implement channel filtering rg -A 10 -B 5 "findByCode.*ProductOptionGroup" --type tsWould you like me to help implement the channelLanguageCode functionality or create an issue to track this TODO?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/admin-ui/src/lib/core/src/common/generated-types.ts(17 hunks)packages/admin-ui/src/lib/core/src/common/introspection-result.ts(1 hunks)packages/common/src/generated-types.ts(15 hunks)packages/core/src/api/resolvers/admin/product-option.resolver.ts(4 hunks)packages/core/src/api/schema/admin-api/product-option-group.api.graphql(2 hunks)packages/core/src/common/error/generated-graphql-admin-errors.ts(3 hunks)packages/core/src/data-import/providers/importer/fast-importer.service.ts(5 hunks)packages/core/src/entity/product-option-group/product-option-group.entity.ts(3 hunks)packages/core/src/entity/product-option/product-option.entity.ts(3 hunks)packages/core/src/service/services/product-option-group.service.ts(11 hunks)packages/core/src/service/services/product-option.service.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/entity/product-option/product-option.entity.ts
- packages/admin-ui/src/lib/core/src/common/introspection-result.ts
- packages/core/src/entity/product-option-group/product-option-group.entity.ts
- packages/core/src/data-import/providers/importer/fast-importer.service.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/service/services/product-option-group.service.ts
[error] 379-379: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (54)
packages/core/src/common/error/generated-graphql-admin-errors.ts (3)
406-419: LGTM! Consistent error class implementation.The new
OptionGroupInUseErrorclass follows the established pattern of other error classes in this file, with proper fields foroptionGroupCodeandvariantCountto provide meaningful error context.
619-619: LGTM! Proper error type registration.The
errorTypeNamesset has been correctly updated to include the newOptionGroupInUseErrortype, ensuring proper GraphQL error type resolution.
715-719: LGTM! Correct union type resolver implementation.The
RemoveOptionGroupFromChannelResultresolver properly uses theisGraphQLErrorhelper to distinguish between error results and successfulProductOptionGroupresults.packages/core/src/api/resolvers/admin/product-option.resolver.ts (4)
50-58: LGTM! Well-implemented paginated query.The
productOptionGroupListquery method properly implements pagination with appropriate permission checks, relation loading, and service delegation.
100-108: LGTM! Proper deletion mutation implementation.The
deleteProductOptionGroupmutation correctly implements transactional deletion with appropriate permission checks and service delegation.
110-118: LGTM! Consistent paginated query implementation.The
productOptionsquery follows the same pattern as the option group list query, with proper pagination and relation handling.
152-170: LGTM! Proper channel assignment/removal implementation.Both mutations correctly handle channel operations with appropriate permissions:
assignOptionGroupsToChannelreturns an array of ProductOptionGroupsremoveOptionGroupsFromChannelreturns a union type that can include error resultsThe error handling for the removal operation is properly implemented using the
ErrorResultUniontype.packages/core/src/api/schema/admin-api/product-option-group.api.graphql (4)
3-5: LGTM! Well-designed paginated query fields.The new query fields
productOptionGroupListandproductOptionsproperly support pagination through their respective options parameters, following GraphQL best practices.
13-26: LGTM! Comprehensive mutation operations.The mutations provide complete CRUD and channel management capabilities:
- Deletion with proper return type
- Channel assignment with straightforward array return
- Channel removal with union type for error handling
The union return type for
removeOptionGroupsFromChannelproperly handles both success and error scenarios.
70-79: LGTM! Well-defined input types for channel operations.The input types are appropriately structured:
- Required arrays of group IDs and channel ID
- Optional
forceflag for removal operationsThis provides clear API contracts for channel management operations.
81-88: LGTM! Proper error type and union definition.The
OptionGroupInUseErrortype implementsErrorResultcorrectly with meaningful fields (optionGroupCode,variantCount), and the union type properly combines success and error cases for the removal operation.packages/core/src/service/services/product-option.service.ts (6)
52-54: LGTM! Proper deprecation annotation.The deprecation notice clearly guides users to the new
findAllListmethod, maintaining backward compatibility while encouraging migration.
65-83: LGTM! Well-implemented paginated query method.The
findAllListmethod properly usesListQueryBuilderwith channel-aware filtering and consistent translation of results. The default relations and proper pagination handling are good practices.
95-120: LGTM! Consistent group-specific query implementation.The
findByGroupIdListmethod follows the same pattern asfindAllListwith an additional WHERE clause for group filtering. The entity alias usage ensures clear query construction.
136-139: LGTM! Proper channel assignment integration.The
beforeSavecallback correctly assigns the new product option to the current channel using theChannelService, ensuring channel-aware behavior from creation.
173-176: LGTM! Improved error handling in delete method.The explicit null check and
EntityNotFoundErrorthrow provide clear error messaging when attempting to delete non-existent options, improving the developer experience.
221-236: LGTM! Channel-aware internal finder implementation.The
findOneInternalmethod properly enforces channel access control by checking if the option belongs to the current channel or is associated with a product. The relation loading forchannelsandgroupensures proper access validation.packages/admin-ui/src/lib/core/src/common/generated-types.ts (14)
270-273: LGTM! Clean input type definition for channel assignment.The
AssignOptionGroupsToChannelInputfollows consistent naming conventions and provides the necessary fields for bulk assignment operations.
1707-1707: Error code properly integrated into the ErrorCode enum.The
OPTION_GROUP_IN_USE_ERRORfollows the established naming pattern and provides specific error handling for option group conflicts during channel operations.
2785-2785: Channel assignment mutation properly defined.The
assignOptionGroupsToChannelmutation follows the established pattern for channel assignment operations and maintains consistency with other similar mutations in the schema.
2904-2904: Delete mutation follows established conventions.The
deleteProductOptionGroupmutation maintains consistency with other deletion operations in the schema.
2987-2987: Channel removal mutation properly structured.The
removeOptionGroupsFromChannelreturns an array of union types, allowing for proper error handling on a per-item basis during bulk operations.
3169-3171: Mutation argument types are properly defined.All three mutation argument types (
MutationAssignOptionGroupsToChannelArgs,MutationDeleteProductOptionGroupArgs,MutationRemoveOptionGroupsFromChannelArgs) follow consistent patterns and reference the correct input types.Also applies to: 3488-3490, 3676-3678
4080-4086: Well-structured error type with comprehensive information.The
OptionGroupInUseErrorprovides valuable context withoptionGroupCodeandvariantCountfields, enabling better error handling and user feedback in the UI.
4845-4855: Comprehensive filter parameters for ProductOption.The filter includes all relevant fields with proper operator support. The
groupIdfilter is particularly useful for channel-specific filtering.
4865-4876: Enhanced ProductOptionGroup with paginated option access.The addition of
optionListfield withProductOptionListOptionsprovides paginated, sortable, and filterable access to options, which is essential for scalable UI performance.
4877-4886: Complete pagination, filtering, and sorting infrastructure.The
ProductOptionGroupFilterParameter,ProductOptionGroupList,ProductOptionGroupListOptions, andProductOptionGroupSortParametertypes provide a comprehensive set of operations for managing option groups at scale. This aligns well with the PR objectives for enhanced multi-vendor support.Also applies to: 4888-4892, 4894-4905, 4907-4913
4939-4943: ProductOption pagination infrastructure is complete.The
ProductOptionList,ProductOptionListOptions, andProductOptionSortParametertypes mirror the structure used for option groups, ensuring consistency across the API.Also applies to: 4945-4956, 4958-4965
5343-5345: Query field updates properly integrate new functionality.The updates to
productOptionGroupList,productOptionsfield type, and their corresponding argument types enable the new paginated access patterns while maintaining backward compatibility.Also applies to: 5538-5550
5828-5828: Union types and input structures support robust error handling.The
RemoveOptionGroupFromChannelResultunion type andRemoveOptionGroupsFromChannelInputwith optionalforceparameter provide flexible operation modes for channel management scenarios.Also applies to: 5832-5836
8758-8758: Error fragment types properly integrated.The
ErrorResult_OptionGroupInUseError_Fragmentand its inclusion in theErrorResultFragmentunion ensure comprehensive error handling support across the GraphQL client infrastructure.Also applies to: 8790-8790
packages/core/src/service/services/product-option-group.service.ts (15)
3-11: LGTM: Well-structured imports for channel functionalityThe new imports properly support channel-aware operations and pagination features. The additions align with the PR objectives for channel-specific functionality.
62-64: LGTM: Appropriate service dependencies addedThe new dependencies (ChannelService, ListQueryBuilder, RoleService) are correctly injected and necessary for the channel-aware functionality being implemented.
67-69: Good deprecation practiceProperly deprecating the old method while providing clear guidance to use the new paginated version is a good practice for API evolution.
157-160: Improved query logic for dual relation supportThe updated WHERE clause correctly handles both the new
productsrelation and the legacyproductrelation, ensuring backward compatibility during the transition.
177-180: Good practices for code uniqueness and channel assignmentThe beforeSave callback properly ensures unique codes and assigns groups to the current channel. This maintains data integrity and implements the channel-aware functionality correctly.
201-205: Consistent code uniqueness enforcement in updatesThe conditional code uniqueness check only when the code is being modified is efficient and maintains consistency with the create method.
209-209: Use of internal method is appropriateSwitching to
findOneInternalensures proper channel-aware retrieval with security checks after the update operation.
218-275: Enhanced deletion logic with proper relation handlingThe method correctly handles both channel-scoped and product-scoped deletions. The dual relation cleanup on lines 257-262 properly maintains both
optionGroupsand__optionGroupsrelations.
281-303: Robust code uniqueness algorithmThe implementation correctly handles suffix increment and replacement patterns. The logic properly detects conflicts and generates unique codes systematically.
310-327: Smart conditional querying based on contextThe method intelligently switches between product-scoped and channel-scoped queries based on whether a
targetProductIdis provided. This supports both use cases efficiently.
339-354: Secure channel-aware internal lookupThe method properly validates channel access or product ownership before returning results, ensuring proper data isolation and security.
360-402: Comprehensive channel assignment with proper permissionsThe method correctly validates permissions, assigns both groups and options to channels, and returns properly translated results. The permission check ensures only authorized users can perform channel assignments.
404-468: Robust channel removal with usage checkingThe method properly validates permissions, checks for usage conflicts, and supports forced removal. The error handling correctly returns
OptionGroupInUseErrorwhen groups are in use.
93-111: ```shell
#!/bin/bash
set -e
FILE="packages/core/src/service/helpers/list-query-builder/list-query-builder.ts"
echo "Inspecting: $FILE"echo
echo "=== build method implementation ==="
rg -n "build\(" "$FILE" -C5echo
echo "=== channelId usage ==="
rg -n "channelId" "$FILE" -C5 || echo "No channelId references found in $FILE"--- `412-415`: I want to confirm that a `DeleteProductOptionGroup` permission exists and isn’t accidentally mapped to `DeleteFacet`. Let’s inspect the `Permission` enum: ```shell #!/bin/bash # Locate the Permission enum definition rg "export enum Permission" -n --type ts # Show the Permission enum values around ProductOptionGroup rg -A 20 -B 5 "DeleteProductOptionGroup" -n --type ts || \ rg -A 20 -B 5 "ProductOptionGroup" -n --type tspackages/common/src/generated-types.ts (8)
265-268: LGTM: Well-structured input type for channel assignment.The
AssignOptionGroupsToChannelInputtype follows the established pattern used by other channel assignment operations in the schema.
1695-1695: LGTM: Consistent error code addition.The new
OPTION_GROUP_IN_USE_ERRORfollows the naming convention and placement pattern used by other error codes in the enum.
4006-4012: LGTM: Comprehensive error type implementation.The
OptionGroupInUseErrortype properly extendsErrorResultand includes appropriate context fields (optionGroupCodeandvariantCount) to help with error handling and user feedback.
4790-4800: LGTM: Excellent pagination support for ProductOptionGroup.The addition of the
optionListfield withProductOptionListreturn type, along with the comprehensive filter, sort, and list option types, provides robust paginated querying capabilities that align with the existing schema patterns.Also applies to: 4802-4838
4770-4780: LGTM: Comprehensive filtering and sorting capabilities.The new
ProductOptionFilterParameterandProductOptionSortParametertypes, along with the list types, provide complete CRUD query support for ProductOption entities with appropriate filtering fields.Also applies to: 4864-4890
2772-2773: LGTM: Consistent mutation signatures.The new mutations (
assignOptionGroupsToChannel,deleteProductOptionGroup,removeOptionGroupsFromChannel) follow the established patterns for similar operations and return appropriate types including error handling.Also applies to: 2891-2892, 2974-2975
5267-5272: LGTM: Well-designed query fields.The new query fields
productOptionGroupListandproductOptionsprovide paginated access to the entities with proper input options for filtering and sorting.
5754-5758: LGTM: Consistent channel removal input type.The
RemoveOptionGroupsFromChannelInputincludes the optionalforceparameter which aligns with similar removal operations and provides flexibility for handling dependencies.
packages/core/src/service/services/product-option-group.service.ts
Outdated
Show resolved
Hide resolved
…e.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/src/service/services/product-option-group.service.ts (1)
403-403: Incorrect JSDoc commentThe comment mentions "Remove Facets" but this method removes ProductOptionGroups from channels.
/** * @description - * Remove Facets from the specified Channel + * Remove ProductOptionGroups from the specified Channel */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/service/services/product-option-group.service.ts(11 hunks)
🔇 Additional comments (8)
packages/core/src/service/services/product-option-group.service.ts (8)
67-69: Good deprecation practiceThe deprecation notice clearly directs users to the new method and maintains backward compatibility.
93-111: Well-implemented paginated query methodThe new
findAllListmethod properly implements pagination with channel awareness and consistent translation patterns.
157-160: Good backward compatibility handlingThe WHERE clause correctly handles both the new
productsrelation and legacyproductrelation, ensuring smooth migration.
177-180: Proper data integrity measuresGood implementation of unique code enforcement and automatic channel assignment during creation.
218-218: Enhanced method signature for flexibilityThe optional
productIdparameter allows the method to handle both product-specific and channel-wide deletion scenarios.
257-262: Correct handling of dual relationsProperly removes the group from both
optionGroupsand__optionGroupsrelations to maintain consistency during the migration period.
281-303: Robust unique code implementationThe
ensureUniqueCodemethod handles edge cases well, including existing suffixes and proper conflict detection.
378-378: Performance optimization correctly appliedGood to see the spread operator performance issue has been resolved using
flatMapas suggested in the previous review.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/core/src/service/services/product-option-group.service.ts (2)
131-150: Channel filtering still missing as noted in previous reviewThe method still lacks channel filtering despite the TODO comment. This could allow access to groups from unauthorized channels.
Consider adding channel filtering to ensure proper isolation:
findByCode( ctx: RequestContext, groupCode: string, lang: LanguageCode, ): Promise<Translated<ProductOptionGroup> | undefined> { - // TODO: Implement usage of channelLanguageCode return this.connection .getRepository(ctx, ProductOptionGroup) .findOne({ where: { code: groupCode, productId: IsNull(), + channels: { id: ctx.channelId }, }, - relations: ['options', 'options.group'], + relations: ['options', 'options.group', 'channels'], })
409-412: Incorrect permissions for ProductOptionGroup operationsThe method checks
DeleteProductandDeleteCatalogpermissions, but these appear to be inconsistent with the context. The comment suggests this is for "Remove Facets" which seems copy-pasted from another method.const hasPermission = await this.roleService.userHasAnyPermissionsOnChannel(ctx, input.channelId, [ - Permission.DeleteProduct, + Permission.DeleteProductOptionGroup, Permission.DeleteCatalog, ]);Also, the JSDoc comment incorrectly mentions "Remove Facets":
/** * @description - * Remove Facets from the specified Channel + * Remove ProductOptionGroups from the specified Channel */
🧹 Nitpick comments (1)
packages/core/src/service/services/product-option-group.service.ts (1)
444-463: Logic gap for groups without options partially addressedThe current implementation now properly handles groups without options by checking
group.options.lengthbefore processing options, but the logic structure could be cleaner.The nested if condition for
group.options.lengthis handled correctly now, but the overall structure could be simplified for better readability:if (!isInUse || input.force) { await this.channelService.removeFromChannels(ctx, ProductOptionGroup, group.id, [ input.channelId, ]); - if (group.options.length) { - await Promise.all( - group.options.map(o => - this.channelService.removeFromChannels(ctx, ProductOption, o.id, [ - input.channelId, - ]), - ), - ); - } + await Promise.all( + group.options.map(o => + this.channelService.removeFromChannels(ctx, ProductOption, o.id, [ + input.channelId, + ]), + ), + );This works because
Promise.allon an empty array resolves immediately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/service/services/product-option-group.service.ts(11 hunks)
🔇 Additional comments (13)
packages/core/src/service/services/product-option-group.service.ts (13)
3-11: LGTM: Imports properly reflect new channel functionalityThe new imports correctly support the channel-aware features being added, including permission types, error result unions, and channel-specific input types.
42-44: LGTM: Service dependencies correctly injectedThe new service dependencies (ChannelService, ListQueryBuilder, RoleService) are properly injected to support the channel-aware functionality.
67-69: Good deprecation notice for legacy methodThe deprecation comment properly guides users to the new paginated
findAllListmethod while maintaining backward compatibility.
93-111: LGTM: Well-implemented paginated query methodThe new
findAllListmethod properly implements pagination with channel awareness and translation support. The implementation follows established patterns in the codebase.
157-160: Proper support for dual product relationsThe updated where clause correctly handles both
productsandproductrelations, ensuring compatibility with the new channel-aware data model.
177-180: Enhanced creation with channel assignment and unique codeThe
beforeSavecallback properly ensures unique codes and assigns groups to the current channel, which is essential for multi-channel support.
201-205: Proper unique code validation on updateThe conditional unique code check during updates ensures data integrity while avoiding unnecessary validation when the code hasn't changed.
218-275: Enhanced deletion logic with dual product relation supportThe method properly handles both
optionGroupsand__optionGroupsrelations on products, ensuring consistency in the new data model. The channel-aware where clause is correctly implemented.
277-303: Well-implemented unique code validationThe
ensureUniqueCodemethod properly handles suffix generation and conflict resolution, including proper handling of already-suffixed codes.
305-328: Proper dual relation handling in usage checkThe method correctly distinguishes between product-specific (
productrelation viatargetProductId) and channel-specific (__optionGroupsrelation) usage scenarios.
339-354: Secure channel-aware entity retrievalThe
findOneInternalmethod properly enforces access control by checking both channel membership and product association, ensuring users can only access authorized option groups.
364-367: Correct permissions for channel assignmentThe permission check uses appropriate
UpdateProductandUpdateCatalogpermissions for channel assignment operations, which is correct for this context.
378-378: Performance improvement correctly implementedThe previous performance issue with spread syntax has been fixed using
flatMap, which improves time complexity from O(n²) to O(n).
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/core/src/service/services/product-option-group.service.ts (1)
131-150: Channel filtering still missing despite TODO commentThe method lacks channel filtering which could allow access to groups from unauthorized channels. The TODO comment acknowledges this but the issue remains unaddressed.
🧹 Nitpick comments (2)
packages/core/src/service/services/product-option-group.service.ts (2)
67-69: Add more context to the deprecation noticeThe deprecation notice should specify the version when the method will be removed and provide migration guidance.
/** - * @deprecated Use {@link ProductOptionGroupService.findAllList findAllList(ctx, options, relations)} instead + * @deprecated Use {@link ProductOptionGroupService.findAllList findAllList(ctx, options, relations)} instead. Will be removed in v3.0. */
237-237: Consider using consistent filtering approachThe filtering logic uses
!op.deletedAtwhich is correct, but consider usingIsNull()for consistency with other parts of the codebase.-const optionsToDelete = optionGroup.options.filter(op => !op.deletedAt); +const optionsToDelete = optionGroup.options.filter(op => op.deletedAt === null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/service/services/product-option-group.service.ts(11 hunks)
🔇 Additional comments (11)
packages/core/src/service/services/product-option-group.service.ts (11)
93-111: LGTM! Well-implemented paginated query methodThe new
findAllListmethod correctly implements pagination using theListQueryBuilderand includes proper channel filtering and translation. The structure is consistent with other paginated methods in the codebase.
157-160: Good enhancement to support both product relationship typesThe query now properly handles both the new many-to-many
productsrelationship and the existingproductrelationship, ensuring backward compatibility while supporting the new channel-aware architecture.
177-180: Excellent addition of code uniqueness and channel assignmentThe
beforeSavehook now ensures code uniqueness and automatically assigns the group to the current channel, which is essential for the new channel-aware functionality.
218-227: Enhanced method signature supports both channel and product-specific deletionThe method now accepts an optional
productIdparameter and has improved query conditions to handle both channel-level and product-specific option groups. The logic correctly determines the appropriate where clause based on the presence ofproductId.
257-262: Handle both legacy and new product relationships correctlyThe code correctly handles both
optionGroupsand__optionGroupsrelationships, ensuring compatibility with both the old and new data models. This is essential for the migration period.
281-303: Well-implemented unique code generation with proper conflict resolutionThe method correctly handles code conflicts by appending incremental suffixes and properly handles cases where codes are already suffixed. The logic is robust and follows good practices.
310-327: Improved query logic to handle different product relationship scenariosThe method now correctly differentiates between product-specific and channel-level operations, using the appropriate relationship (
optionGroupsvs__optionGroups) based on the presence oftargetProductId.
339-354: Secure internal method with proper channel access controlThe
findOneInternalmethod includes proper security checks to ensure users can only access groups that are either in their channel or associated with a product they have access to. This is a good security practice.
360-399: Comprehensive channel assignment implementation with proper permissionsThe method includes proper permission checks, handles both groups and their options, and uses appropriate database operations. The use of
flatMapfor extracting options is efficient and the permission checks are appropriately restrictive.
405-467: Robust channel removal with usage validation and error handlingThe method includes comprehensive checks for permissions, default channel protection, usage validation, and proper error handling. The logic correctly handles both forced and non-forced removals, and properly manages both groups and their associated options.
201-209: ```shell
#!/bin/bashInspect findOneInternal and findOne implementations in ProductOptionGroupService
rg -A15 -B1 "private async findOneInternal" --type ts packages/core/src/service/services/product-option-group.service.ts
echo "-----"
rg -A15 -B1 "async findOne" --type ts packages/core/src/service/services/product-option-group.service.ts</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
nostva
changed the title
|
Might wanna checkout #3703 |
Great work! I went with a similar approach, but non-breaking, as maintainers prefer avoiding DB-breaking changes. |
…into channel-product-options
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/common/src/generated-types.ts (1)
3063-3066: Fix incorrect docstring for updateProductOptionDescription says “Create a new …” but this is an update mutation. Please fix at the schema source so generated types reflect it.
🧹 Nitpick comments (4)
packages/common/src/generated-types.ts (3)
316-330: Deprecation metadata added to CustomField and all config variants — LGTMNon-breaking additions. Make sure Admin UI surfaces deprecated flags (badges/tooltips) where appropriate.
Also applies to: 1342-1354, 1548-1563, 1904-1920, 2127-2142, 2607-2621, 2625-2637, 5773-5788, 6325-6340, 6380-6393, 6613-6625
1701-1701: New ErrorCode OPTION_GROUP_IN_USE_ERROR — LGTMAdd i18n/help text where errors are shown.
5315-5315: Add productOptionGroupList/productOptions queries and args — LGTMNo breaking change; retains legacy simple queries alongside list endpoints.
Consider deprecating the non-list queries in a future minor once UI migrates.
Also applies to: 5317-5317, 5523-5525, 5533-5535
packages/admin-ui/src/lib/core/src/common/generated-types.ts (1)
5390-5393: LGTM: productOptionGroupList and productOptions queries.
Consider migrating UI screens to use paginated variants.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/core/mock-data/data-sources/products.csvis excluded by!**/*.csv
📒 Files selected for processing (3)
packages/admin-ui/src/lib/core/src/common/generated-types.ts(34 hunks)packages/admin-ui/src/lib/core/src/common/introspection-result.ts(3 hunks)packages/common/src/generated-types.ts(26 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/admin-ui/src/lib/core/src/common/generated-types.ts (2)
packages/common/src/generated-types.ts (18)
Scalars(10-20)Maybe(2-2)ProductOptionGroup(4824-4837)RemoveOptionGroupFromChannelResult(5814-5814)OptionGroupInUseError(4048-4054)ErrorResult(1720-1723)ProductOptionFilterParameter(4812-4822)InputMaybe(3-3)StringOperators(6354-6363)DateOperators(1529-1535)IdOperators(2081-2087)ProductOptionList(4906-4910)ProductOption(4798-4810)ProductOptionGroupTranslation(4882-4889)ProductOptionListOptions(4912-4923)PaginatedList(4362-4365)ProductOptionSortParameter(4925-4932)Product(4735-4757)packages/core/src/common/error/generated-graphql-admin-errors.ts (3)
Scalars(4-14)OptionGroupInUseError(406-419)ErrorResult(16-20)
packages/common/src/generated-types.ts (4)
packages/core/e2e/graphql/generated-e2e-admin-types.ts (23)
AssignOptionGroupsToChannelInput(258-261)Scalars(11-21)Maybe(3-3)ProductOptionGroup(4702-4714)RemoveOptionGroupFromChannelResult(5665-5665)MutationAssignOptionGroupsToChannelArgs(3077-3079)MutationDeleteProductOptionGroupArgs(3401-3403)MutationRemoveOptionGroupsFromChannelArgs(3589-3591)RemoveOptionGroupsFromChannelInput(5669-5673)OptionGroupInUseError(3953-3958)ErrorResult(1671-1674)ProductOptionFilterParameter(4690-4700)InputMaybe(4-4)StringOperators(6178-6187)DateOperators(1490-1496)IdOperators(2015-2021)ProductOptionList(4780-4783)ProductOption(4677-4688)ProductOptionGroupTranslation(4758-4764)ProductOptionGroupOptionListArgs(4717-4719)ProductOptionListOptions(4785-4796)PaginatedList(4253-4256)Product(4616-4637)packages/admin-ui/src/lib/core/src/common/generated-types.ts (23)
AssignOptionGroupsToChannelInput(270-273)Scalars(11-25)Maybe(3-3)ProductOptionGroup(4899-4912)RemoveOptionGroupFromChannelResult(5892-5892)MutationAssignOptionGroupsToChannelArgs(3191-3193)MutationDeleteProductOptionGroupArgs(3515-3517)MutationRemoveOptionGroupsFromChannelArgs(3703-3705)RemoveOptionGroupsFromChannelInput(5896-5900)OptionGroupInUseError(4122-4128)ErrorResult(1732-1735)ProductOptionFilterParameter(4887-4897)InputMaybe(4-4)StringOperators(6432-6441)DateOperators(1541-1547)IdOperators(2093-2099)ProductOptionList(4981-4985)ProductOption(4873-4885)ProductOptionGroupTranslation(4957-4964)ProductOptionGroupOptionListArgs(4915-4917)ProductOptionListOptions(4987-4998)PaginatedList(4436-4439)Product(4810-4832)packages/common/src/generated-shop-types.ts (16)
Scalars(10-20)Maybe(2-2)ProductOptionGroup(2749-2762)ErrorResult(967-970)ProductOptionFilterParameter(2737-2747)InputMaybe(3-3)StringOperators(3383-3392)DateOperators(851-857)IdOperators(1272-1278)ProductOptionList(2784-2788)ProductOption(2723-2735)ProductOptionGroupTranslation(2775-2782)ProductOptionGroupOptionListArgs(2765-2767)ProductOptionListOptions(2790-2801)PaginatedList(2316-2319)Product(2663-2684)packages/core/src/common/error/generated-graphql-admin-errors.ts (3)
Scalars(4-14)OptionGroupInUseError(406-419)ErrorResult(16-20)
🔇 Additional comments (40)
packages/admin-ui/src/lib/core/src/common/introspection-result.ts (3)
121-121: Add OptionGroupInUseError to ErrorResult union — looks correctEnsure any UI union narrowing now covers OptionGroupInUseError branches.
217-219: PaginatedList extended with ProductOption(Group) lists — OKMatches new schema list types.
251-254: RemoveOptionGroupFromChannelResult mapping added — OKConfirm UI fragments/switches handle both ProductOptionGroup and OptionGroupInUseError.
packages/common/src/generated-types.ts (7)
4048-4054: OptionGroupInUseError type definition — LGTMFields align with server error class (optionGroupCode, variantCount).
4813-4822: Introduce list/filter/sort types and optionList on ProductOption(Group) — LGTMPattern matches existing Facet/FacetValue and Product/Variant list APIs.
Also applies to: 4833-4834, 4840-4843, 4844-4853, 4855-4860, 4861-4872, 4874-4880, 4907-4910, 4912-4923, 4925-4932
265-268: Assign option groups to channel input/mutation — LGTMName and shape mirror other assign-to-channel APIs. Confirm permission gating is consistent server-side.
Also applies to: 2787-2788, 3168-3170
2908-2909: deleteProductOptionGroup mutation/args — LGTMDouble-check server behavior when the group is in use (DeletionResponse.message clarity).
Also applies to: 3492-3494
2991-2992: removeOptionGroupsFromChannel (+ input/union) — LGTMForce flag symmetry with facet/channel removals is good. Ensure union handling in clients is exhaustive.
Also applies to: 3680-3682, 5814-5814, 5819-5822
5315-5317: Channel scoping expectationConfirm these new queries default to the active channel scope in resolvers to avoid cross-channel leakage.
5819-5822: Input contract checkRemoveOptionGroupsFromChannelInput allows force?: boolean — ensure server returns OptionGroupInUseError only when force is false; when true, always return ProductOptionGroup on success.
packages/admin-ui/src/lib/core/src/common/generated-types.ts (30)
270-273: LGTM: AssignOptionGroupsToChannelInput aligns with schema.
Matches core shape and naming.
323-324: LGTM: Custom-field deprecation metadata added.
Consistent with base CustomField.
1354-1355: LGTM: Base CustomField exposes deprecation fields.
Downstream configs now consistent.
1560-1561: LGTM: DateTimeCustomFieldConfig deprecation fields.
No issues.
1713-1713: LGTM: ErrorCode includes OPTION_GROUP_IN_USE_ERROR.
Ensures typed error coverage.
1917-1918: LGTM: FloatCustomFieldConfig deprecation fields.
2139-2140: LGTM: IntCustomFieldConfig deprecation fields.
2619-2620: LGTM: LocaleStringCustomFieldConfig deprecation fields.
2637-2638: LGTM: LocaleTextCustomFieldConfig deprecation fields.
2798-2800: LGTM: Mutation assignOptionGroupsToChannel declared.
Input/return types look correct.
2919-2920: LGTM: deleteProductOptionGroup mutation exposed.
Pairs with new channel ops.
3002-3003: LGTM: removeOptionGroupsFromChannel added.
Array of union results is expected.
3300-3302: LGTM: MutationCreateFacetValueArgs.
3515-3517: LGTM: MutationDeleteProductOptionGroupArgs.
3703-3705: LGTM: MutationRemoveOptionGroupsFromChannelArgs.
3933-3935: LGTM: MutationUpdateFacetValueArgs.
4122-4128: LGTM: OptionGroupInUseError type added.
Fields match core error class.
4887-4897: LGTM: ProductOptionFilterParameter (incl. groupId).
Sorting/filtering model looks consistent.
4907-4917: LGTM: ProductOptionGroup.optionList and args.
Enables nested pagination.
4919-4955: LGTM: ProductOptionGroup list/filter/sort types.
Names and shapes follow Vendure conventions.
4981-5007: LGTM: ProductOptionList types.
Supports independent option browsing.
5360-5360: LGTM: facetValue query added.
Scoped arg added below; no conflicts seen.
5513-5515: LGTM: QueryFacetValueArgs(id).
5600-5612: LGTM: Query args for new list queries.
5852-5853: LGTM: RelationCustomFieldConfig deprecation fields.
6403-6404: LGTM: StringCustomFieldConfig deprecation fields.
6458-6459: LGTM: StructCustomFieldConfig deprecation fields.
6691-6692: LGTM: TextCustomFieldConfig deprecation fields.
8843-8844: LGTM: Fragment type for OptionGroupInUseError added.
3191-3193: The UI currently has no references to the generatedMutationAssignOptionGroupsToChannelArgstype. If you intend to call theassignOptionGroupsToChannelmutation in the admin UI, import and use theMutationAssignOptionGroupsToChannelArgstype fromgenerated-types.tsin youruseMutation(or equivalent) calls so the hook signatures remain correctly typed.
| /** Create a single FacetValue */ | ||
| createFacetValue: FacetValue; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Heads-up: Single createFacetValue added.
Verify UI operations don’t assume only the plural variant.
🏁 Script executed:
#!/bin/bash
# Find usages of createFacetValue(s) in admin-ui GraphQL ops
rg -n -C2 -g 'packages/**' -P '\bcreateFacetValue(s)?\b'Length of output: 111898
Implement singular createFacetValue in the UI data service and components
The admin-UI service only provides createFacetValues, and facet‐detail.component.ts (around line 148) always calls the plural API—even for single-value creation. Add a createFacetValue method to facet-data.service.ts and update the component to use it when creating one FacetValue.
🤖 Prompt for AI Agents
In packages/admin-ui/src/lib/core/src/common/generated-types.ts and the related
files, add a singular createFacetValue method to facet-data.service.ts and
update facet-detail.component.ts (around line 148) to call it when creating a
single FacetValue: implement createFacetValue as a thin wrapper that calls the
existing createFacetValues mutation with a single-item array or, if available,
call the backend singular mutation, returning the created FacetValue object;
then change the component logic to detect single-item creation and invoke
createFacetValue instead of createFacetValues so UI calls the appropriate API
and receives a single-item response.
| /** Update a single FacetValue */ | ||
| updateFacetValue: FacetValue; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Heads-up: Single updateFacetValue added.
Ensure UI supports both single and bulk flows.
🏁 Script executed:
#!/bin/bash
rg -n -C2 -g 'packages/**' -P '\bupdateFacetValue(s)?\b'Length of output: 114504
Implement single-value update flow
- In facet-data.service.ts, add an
updateFacetValue(input: Codegen.UpdateFacetValueInput)method that invokes the generatedupdateFacetValuemutation. - In facet-detail.component.ts, use
dataService.facet.updateFacetValuefor single-value edits (while retainingupdateFacetValuesfor bulk updates).
| export type RemoveOptionGroupFromChannelResult = OptionGroupInUseError | ProductOptionGroup; | ||
|
|
||
| export type RemoveOptionGroupFromProductResult = Product | ProductOptionInUseError; | ||
|
|
||
| export type RemoveOptionGroupsFromChannelInput = { | ||
| channelId: Scalars['ID']['input']; | ||
| force?: InputMaybe<Scalars['Boolean']['input']>; | ||
| groupIds: Array<Scalars['ID']['input']>; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM: RemoveOptionGroupFromChannelResult union and input (force).
Ensure UI handles OptionGroupInUseError (__typename gating) and exposes “force” confirmation.
🏁 Script executed:
#!/bin/bash
# Find client ops and fragments handling these unions
rg -n -C3 -P '\bremoveOptionGroupsFromChannel\b|RemoveOptionGroupFromChannelResult|OptionGroupInUseError|__typename'Length of output: 280873
Handle OptionGroupInUseError and expose “force” confirmation
The mutation hook for removing option groups must inspect result.__typename === 'OptionGroupInUseError' and surface a confirmation dialog that re-runs removeOptionGroupsFromChannel with force: true on user approval.
🤖 Prompt for AI Agents
In packages/admin-ui/src/lib/core/src/common/generated-types.ts around lines
5892 to 5900, the mutation hook for removing option groups must detect when the
GraphQL response indicates an OptionGroupInUseError by checking
result.__typename === 'OptionGroupInUseError', surface a confirmation dialog to
the user, and if the user confirms re-run removeOptionGroupsFromChannel with the
same variables plus force: true; update the hook flow to branch on that
typename, show the dialog (returning early while awaiting user action), and on
confirm call the mutation again with force: true and handle success/error paths
accordingly.
964cf4e to
7c388c8
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (7)
packages/core/src/service/services/product-option-group.service.ts (1)
131-150: Add channel filter to findByCode.Still returns groups across channels; restrict by ctx.channelId to enforce isolation.
Apply:
findByCode( ctx: RequestContext, groupCode: string, lang: LanguageCode, ): Promise<Translated<ProductOptionGroup> | undefined> { - // TODO: Implement usage of channelLanguageCode return this.connection .getRepository(ctx, ProductOptionGroup) .findOne({ where: { code: groupCode, productId: IsNull(), + channels: { id: ctx.channelId }, }, - relations: ['options', 'options.group'], + relations: ['options', 'options.group', 'channels'], }) .then( group => (group && translateDeep(group, lang, ['options', ['options', 'group']])) ?? undefined, ); }packages/admin-ui/src/lib/core/src/common/generated-types.ts (6)
2839-2840: Implement singular createFacetValue flow in UI.
Add dataService.facet.createFacetValue and use it for single-item create; keep plural for bulk.rg -n -C2 -g 'packages/**' -P '\bcreateFacetValue(s)?\b'
3002-3003: Handle removeOptionGroupsFromChannel union results.
Branch on __typename; if OptionGroupInUseError, prompt and re-run with force: true.rg -n -C2 -g 'packages/**' -P '\bremoveOptionGroupsFromChannel\b|OptionGroupInUseError|__typename'
3077-3078: Implement singular updateFacetValue flow.
Add dataService.facet.updateFacetValue and use it for single edits.rg -n -C2 -g 'packages/**' -P '\bupdateFacetValue(s)?\b'
5360-5360: Query.facetValue added (singular).
Ensure UI components call the singular when fetching one FacetValue.
5513-5515: QueryFacetValueArgs (singular).
LGTM; mirror usage in data service as noted.
5892-5900: Union result and input for channel removal.
Ensure array-of-union handling; gate on __typename per item; re-run with force if confirmed.rg -n -C2 -g 'packages/**' -P 'removeOptionGroupsFromChannel\\s*\\(|__typename\\s*[:=]\\s*[\"\']OptionGroupInUseError[\"\']'
🧹 Nitpick comments (19)
packages/dashboard/src/lib/framework/form-engine/custom-form-component-extensions.ts (2)
1-1: Also mark DocumentNode as a type-only importKeeps runtime bundle lean and consistent with verbatim module syntax setups.
-import { DocumentNode } from 'graphql'; +import type { DocumentNode } from 'graphql';
34-38: Rename local variable to match the registry nameMinor readability nit: the variable references the “detailQueryDocumentRegistry” but is named “list…”.
-export function addDetailQueryDocument(pageId: string, document: DocumentNode) { - const listQueryDocumentRegistry = globalRegistry.get('detailQueryDocumentRegistry'); - const existingDocuments = listQueryDocumentRegistry.get(pageId) || []; - listQueryDocumentRegistry.set(pageId, [...existingDocuments, document]); -} +export function addDetailQueryDocument(pageId: string, document: DocumentNode) { + const detailQueryDocumentRegistry = globalRegistry.get('detailQueryDocumentRegistry'); + const existingDocuments = detailQueryDocumentRegistry.get(pageId) || []; + detailQueryDocumentRegistry.set(pageId, [...existingDocuments, document]); +}packages/core/src/api/resolvers/entity/product-option-group-entity.resolver.ts (1)
56-60: Prefer named arg binding to avoid brittle wrapper object.Bind the options arg explicitly; simplifies typing and avoids undefined wrapper edge-cases.
Apply:
- @Args() args: { options: ProductOptionListOptions }, + @Args('options') options: ProductOptionListOptions, @Relations({ entity: ProductOption }) relations: RelationPaths<ProductOption>, ): Promise<PaginatedList<ProductOption>> { - return this.productOptionService.findByGroupIdList(ctx, optionGroup.id, args.options, relations); + return this.productOptionService.findByGroupIdList(ctx, optionGroup.id, options, relations); }packages/core/src/service/services/product.service.ts (1)
403-415: Channel compatibility check when assigning a group to a product.Consider validating that the option group is accessible on the current channel (or auto-assign it) before adding it to __optionGroups to prevent cross-channel inconsistencies.
Would you like a patch that loads group.channels here and auto-assigns via ChannelService if missing?
packages/core/src/api/schema/admin-api/product-option-group.api.graphql (1)
5-6: Naming consistency: consider productOptionList.For symmetry with productOptionGroupList and ProductOptionList, productOptionList may be clearer than productOptions.
packages/core/src/service/services/product-option-group.service.ts (1)
405-467: Optional: unify return semantics on removal.removeGroupsFromChannel returns findOne(ctx, id) (channel-agnostic). For consistency with assignGroupsToChannel, consider returning the group in the caller’s or target channel context.
packages/common/src/generated-types.ts (1)
4832-4834: Consider deprecating the non-paginated options field long-termTo steer clients to the paginated optionList and avoid large payloads, consider marking ProductOptionGroup.options as deprecated in a future minor.
packages/admin-ui/src/lib/core/src/common/generated-types.ts (12)
321-325: Surface deprecation metadata in Boolean custom field UI.
Show a “Deprecated” badge and optional tooltip with deprecationReason.
1354-1355: Base CustomField gained deprecation flags.
Apply the same UX (badge/tooltip) consistently across all custom-field editors.
1560-1561: DateTimeCustomFieldConfig deprecation fields added.
Ensure date-time editors render the deprecation affordance.
1917-1918: Float custom field deprecation flags.
Mirror the generic deprecation UI here.
2139-2140: Int custom field deprecation flags.
Same deprecation treatment as others.
2619-2620: LocaleString custom field deprecation flags.
Badge + tooltip in locale string editor.
2637-2638: LocaleText custom field deprecation flags.
Apply deprecation UI consistently.
4887-4897: ProductOptionFilterParameter includes groupId.
Add filter UI (e.g., by group) for options list screens to leverage this.
5852-5853: Relation custom field deprecation flags.
Mark deprecated relations in the custom-field config viewer.
6403-6404: String custom field deprecation flags.
Apply deprecation visuals here too.
6458-6459: Struct custom field deprecation flags.
Consider greying out deprecated fields in struct editors.
6691-6692: Text custom field deprecation flags.
Consistent deprecation UX.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/core/mock-data/data-sources/products.csvis excluded by!**/*.csv
📒 Files selected for processing (20)
packages/admin-ui/src/lib/core/src/common/generated-types.ts(34 hunks)packages/admin-ui/src/lib/core/src/common/introspection-result.ts(3 hunks)packages/common/src/generated-types.ts(26 hunks)packages/core/src/api/resolvers/admin/product-option.resolver.ts(4 hunks)packages/core/src/api/resolvers/entity/product-option-group-entity.resolver.ts(2 hunks)packages/core/src/api/schema/admin-api/product-option-group.api.graphql(2 hunks)packages/core/src/api/schema/common/product-option-group.type.graphql(2 hunks)packages/core/src/common/error/generated-graphql-admin-errors.ts(3 hunks)packages/core/src/config/entity/entity-duplicators/product-duplicator.ts(3 hunks)packages/core/src/data-import/providers/importer/fast-importer.service.ts(5 hunks)packages/core/src/data-import/providers/importer/importer.ts(2 hunks)packages/core/src/entity/channel/channel.entity.ts(2 hunks)packages/core/src/entity/product-option-group/product-option-group.entity.ts(3 hunks)packages/core/src/entity/product-option/product-option.entity.ts(3 hunks)packages/core/src/entity/product/product.entity.ts(1 hunks)packages/core/src/service/services/product-option-group.service.ts(11 hunks)packages/core/src/service/services/product-option.service.ts(9 hunks)packages/core/src/service/services/product-variant.service.ts(1 hunks)packages/core/src/service/services/product.service.ts(5 hunks)packages/dashboard/src/lib/framework/form-engine/custom-form-component-extensions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/core/src/api/schema/common/product-option-group.type.graphql
- packages/core/src/common/error/generated-graphql-admin-errors.ts
- packages/core/src/entity/channel/channel.entity.ts
- packages/core/src/service/services/product-variant.service.ts
- packages/core/src/entity/product/product.entity.ts
- packages/core/src/entity/product-option/product-option.entity.ts
- packages/core/src/entity/product-option-group/product-option-group.entity.ts
- packages/admin-ui/src/lib/core/src/common/introspection-result.ts
- packages/core/src/data-import/providers/importer/importer.ts
- packages/core/src/config/entity/entity-duplicators/product-duplicator.ts
- packages/core/src/service/services/product-option.service.ts
- packages/core/src/data-import/providers/importer/fast-importer.service.ts
- packages/core/src/api/resolvers/admin/product-option.resolver.ts
🧰 Additional context used
📓 Path-based instructions (2)
packages/dashboard/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/dashboard.mdc)
packages/dashboard/src/**/*.{ts,tsx}: Use TanStack Query (useQuery or useMutation) for data fetching; do not use Apollo Client.
When creating useQuery calls, follow the provided pattern: import { api } from '@/graphql/api.js'; import { graphql } from '@/graphql/graphql.js'; use useQuery with queryKey, staleTime, and queryFn using api.query.
When performing mutations, follow the provided useMutation pattern: use graphql() for the mutation document, api.mutate for mutationFn, and do not pass variables at declaration.
Files:
packages/dashboard/src/lib/framework/form-engine/custom-form-component-extensions.ts
packages/dashboard/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/dashboard.mdc)
React component props objects should be typed as Readonly<...>.
Files:
packages/dashboard/src/lib/framework/form-engine/custom-form-component-extensions.ts
🧬 Code graph analysis (5)
packages/core/src/api/resolvers/entity/product-option-group-entity.resolver.ts (2)
packages/common/src/generated-types.ts (4)
ProductOptionGroup(4824-4837)ProductOptionListOptions(4912-4923)ProductOption(4798-4810)PaginatedList(4362-4365)packages/core/src/api/decorators/relations.decorator.ts (2)
Relations(136-162)RelationPaths(16-16)
packages/core/src/service/services/product.service.ts (2)
packages/core/src/common/utils.ts (1)
idsAreEqual(38-43)packages/core/src/common/error/errors.ts (2)
UserInputError(27-31)InternalServerError(14-18)
packages/admin-ui/src/lib/core/src/common/generated-types.ts (2)
packages/common/src/generated-types.ts (25)
AssignOptionGroupsToChannelInput(265-268)Scalars(10-20)Maybe(2-2)ProductOptionGroup(4824-4837)FacetValue(1810-1822)RemoveOptionGroupFromChannelResult(5814-5814)MutationAssignOptionGroupsToChannelArgs(3167-3169)MutationCreateFacetValueArgs(3276-3278)CreateFacetValueInput(839-844)RemoveOptionGroupsFromChannelInput(5818-5822)UpdateFacetValueInput(6773-6778)OptionGroupInUseError(4048-4054)ErrorResult(1720-1723)ProductOptionFilterParameter(4812-4822)InputMaybe(3-3)StringOperators(6354-6363)DateOperators(1529-1535)IdOperators(2081-2087)ProductOptionList(4906-4910)ProductOption(4798-4810)ProductOptionGroupTranslation(4882-4889)ProductOptionListOptions(4912-4923)PaginatedList(4362-4365)ProductOptionSortParameter(4925-4932)Product(4735-4757)packages/core/src/common/error/generated-graphql-admin-errors.ts (3)
Scalars(4-14)OptionGroupInUseError(406-419)ErrorResult(16-20)
packages/core/src/service/services/product-option-group.service.ts (8)
packages/core/src/api/common/request-context.ts (1)
RequestContext(179-447)packages/core/src/common/types/common-types.ts (1)
ListQueryOptions(68-74)packages/common/src/generated-types.ts (8)
ProductOptionGroup(4824-4837)PaginatedList(4362-4365)Product(4735-4757)AssignOptionGroupsToChannelInput(265-268)ProductOption(4798-4810)RemoveOptionGroupsFromChannelInput(5818-5822)RemoveOptionGroupFromChannelResult(5814-5814)OptionGroupInUseError(4048-4054)packages/core/src/common/utils.ts (2)
assertFound(30-32)idsAreEqual(38-43)packages/core/src/common/error/errors.ts (2)
ForbiddenError(67-71)UserInputError(27-31)packages/core/src/common/error/error-result.ts (1)
ErrorResultUnion(44-46)packages/core/src/service/services/product-option.service.ts (1)
isInUse(207-219)packages/core/src/common/error/generated-graphql-admin-errors.ts (1)
OptionGroupInUseError(406-419)
packages/common/src/generated-types.ts (2)
packages/admin-ui/src/lib/core/src/common/generated-types.ts (27)
AssignOptionGroupsToChannelInput(270-273)Scalars(11-25)Maybe(3-3)ProductOptionGroup(4899-4912)RemoveOptionGroupFromChannelResult(5892-5892)MutationAssignOptionGroupsToChannelArgs(3191-3193)MutationDeleteProductOptionGroupArgs(3515-3517)MutationRemoveOptionGroupsFromChannelArgs(3703-3705)RemoveOptionGroupsFromChannelInput(5896-5900)OptionGroupInUseError(4122-4128)ErrorResult(1732-1735)ProductOptionFilterParameter(4887-4897)InputMaybe(4-4)StringOperators(6432-6441)DateOperators(1541-1547)IdOperators(2093-2099)ProductOptionList(4981-4985)ProductOption(4873-4885)ProductOptionGroupTranslation(4957-4964)ProductOptionListOptions(4987-4998)ProductOptionGroupList(4930-4934)PaginatedList(4436-4439)ProductOptionGroupListOptions(4936-4947)ProductOptionSortParameter(5000-5007)QueryProductOptionGroupListArgs(5600-5602)QueryProductOptionsArgs(5610-5612)Product(4810-4832)packages/core/src/common/error/generated-graphql-admin-errors.ts (3)
Scalars(4-14)OptionGroupInUseError(406-419)ErrorResult(16-20)
🔇 Additional comments (24)
packages/dashboard/src/lib/framework/form-engine/custom-form-component-extensions.ts (1)
3-3: Use a type-only import for DashboardCustomFormComponent
DashboardCustomFormComponent is a compile-time type only; switch toimport typeto prevent emitting a runtime import and keep the ESM tree clean.-import { DashboardCustomFormComponent } from '../extension-api/types/form-components.js'; +import type { DashboardCustomFormComponent } from '../extension-api/types/form-components.js';packages/core/src/api/schema/admin-api/product-option-group.api.graphql (5)
3-3: LGTM: Adds paginated list for option groups.Matches service findAllList and channel scoping.
14-15: LGTM: deleteProductOptionGroup mutation.Aligns with deleteGroupAndOptions service.
21-27: Validate union/result wiring for channel ops.Ensure admin error OptionGroupInUseError is included in generated types and resolvers return the union correctly.
81-89: LGTM: OptionGroupInUseError and union.Fields align with service return shape (optionGroupCode, variantCount).
90-92: Confirm generateListOptions integration.Empty input stubs rely on build-time generation; verify the codegen step populates ProductOptionGroupListOptions.
packages/core/src/service/services/product-option-group.service.ts (1)
93-111: LGTM: Channel-scoped paginated listing.Uses ListQueryBuilder with channelId and translates options.
packages/common/src/generated-types.ts (6)
318-320: Consistent deprecation metadata added — LGTMAdding deprecated/deprecationReason to CustomField and all config variants is consistent with the existing pattern and matches the admin UI types.
Also applies to: 1548-1550, 1905-1907, 2127-2129, 2607-2609, 2625-2627, 1342-1344, 5774-5776, 6325-6327, 6380-6382, 6613-6615
265-268: Channel assignment/removal surface for OptionGroups — LGTMTypes, args, inputs, enum value, and the OptionGroupInUseError union are coherent and align with existing channel-assignment patterns.
Also applies to: 2786-2788, 2908-2909, 2991-2992, 3167-3169, 3491-3493, 3679-3681, 5814-5814, 5819-5822, 1701-1701, 4048-4054
4812-4822: ProductOption list/filter/sort — LGTMTypes look correct and mirror existing list-option patterns elsewhere.
Also applies to: 4906-4910, 4912-4923, 4925-4932
4832-4834: ProductOptionGroup optionList + group listing — LGTMoptionList + args and the paginated/sortable group list types are consistent with Facet.valueList et al.
Also applies to: 4840-4843, 4844-4853, 4855-4860, 4861-4872, 4874-4880
5314-5314: New queries productOptionGroupList/productOptions — LGTMContracts are clear and consistent with other PaginatedList queries.
Also applies to: 5316-5316, 5523-5525, 5533-5534
265-268: Verify service implementations & permission guards for OptionGroup channel operations
- Service methods
assignOptionGroupsToChannelandremoveOptionGroupsFromChannelwere not found in the service layer; confirm these implementations exist.- No
@Allow(Permission.…)decorators detected on the admin resolver mutations (assignOptionGroupsToChannel,removeOptionGroupsFromChannel,deleteProductOptionGroup); ensure the correct ReadCatalog/UpdateCatalog/DeleteCatalog permissions are applied.packages/admin-ui/src/lib/core/src/common/generated-types.ts (11)
2798-2799: Expose assignOptionGroupsToChannel mutation in UI.
Add a method to the product/option-group data service and wire to channel assignment UX.
3191-3193: Args type for assignOptionGroupsToChannel.
LGTM; matches schema.
3300-3302: Args type for createFacetValue (singular).
LGTM.
3515-3517: Args for deleteProductOptionGroup.
LGTM; ensure UI passes ID and handles response.
3703-3705: Args for removeOptionGroupsFromChannel.
LGTM; see union handling note above.
3933-3935: Args for updateFacetValue (singular).
LGTM.
4122-4128: OptionGroupInUseError type added—surface details in dialog.
Include optionGroupCode and variantCount in the confirmation message before forcing removal.
5600-5612: Query args for new list endpoints.
LGTM; verify callers pass ListOptions (skip/take/filterOperator/sort).
8843-8843: OptionGroupInUseError fragment added to unions.
LGTM; required for correct narrowing.
8875-8875: Regenerate GraphQL codegen artifacts across all affected packages
Run the GraphQL codegen in admin-ui, core, dev-server, and all plugin packages to update theErrorResultFragmentunion withOptionGroupInUseErrorand avoid type drift.
4981-5007: Verified productOptions query supports pagination, filtering, and sorting. Generated types in packages/admin-ui/src/lib/core/src/common/generated-types.ts (lines 4987–5000) define ProductOptionListOptions and ProductOptionSortParameter.
| export type AssignOptionGroupsToChannelInput = { | ||
| channelId: Scalars['ID']['input']; | ||
| groupIds: Array<Scalars['ID']['input']>; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Wire up assignOptionGroupsToChannel in Admin UI services.
Add a data-service method and mutation doc to expose this in the UI flows (channel picker, product option groups bulk assign).
Run to find existing usage gaps:
🏁 Script executed:
rg -n -C2 -g 'packages/**' -P '\bassignOptionGroupsToChannel\b'Length of output: 39173
🏁 Script executed:
rg -n -C2 -g 'packages/admin-ui/**' -P '\bassignCollectionsToChannel\b'
rg -n -C2 -g 'packages/admin-ui/**' -P '\bassignFacetsToChannel\b'Length of output: 23114
No assignOptionGroupsToChannel wiring exists in Admin UI beyond its type declaration; please add the GraphQL mutation (ASSIGN_OPTION_GROUPS_TO_CHANNEL in data/definitions), the data-service method in product-option-group-data.service.ts, and integrate it into the bulk‐actions component (e.g. product-option-group-list-bulk-actions.ts) analogous to collections and facets.
🤖 Prompt for AI Agents
In packages/admin-ui/src/lib/core/src/common/generated-types.ts lines 270-273,
the type for assignOptionGroupsToChannel exists but lacks wiring; add the
GraphQL mutation constant ASSIGN_OPTION_GROUPS_TO_CHANNEL to data/definitions
(mirroring the shape of existing ASSIGN_COLLECTIONS_TO_CHANNEL /
ASSIGN_FACETS_TO_CHANNEL), implement a corresponding method in
product-option-group-data.service.ts that calls the new mutation and returns the
proper response type, and wire that method into the
product-option-group-list-bulk-actions.ts component by adding the bulk action
handler, success/error notifications, and UI state updates following the same
pattern used for collections and facets so the mutation is invoked from the
Admin UI bulk actions.
| /** Delete a ProductOptionGroup */ | ||
| deleteProductOptionGroup: DeletionResponse; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add UI action for deleteProductOptionGroup with confirmation.
Confirm destructive action, refresh lists, and handle GraphQL errors.
🏁 Script executed:
rg -n -C2 -g 'packages/**' -P '\bdeleteProductOptionGroup\b'Length of output: 38744
Add UI action for deleteProductOptionGroup
No invocation of the deleteProductOptionGroup mutation exists in the admin-ui. Please implement the deletion flow—using the useDeleteProductOptionGroupMutation hook—to confirm the destructive action, handle GraphQL errors, and refresh the option-group list on success.
🤖 Prompt for AI Agents
In packages/admin-ui/src/lib/core/src/common/generated-types.ts around lines
2919-2920, there is no UI action wired to the deleteProductOptionGroup mutation;
implement the deletion flow by calling the useDeleteProductOptionGroupMutation
hook from the option-group list or detail component, show a confirmation modal
before calling the mutation, await the mutation and handle GraphQL errors by
displaying them in a toast/notification, and on success refresh the option-group
list (either by calling the query's refetch or invalidating the cache) and close
the modal; ensure proper loading state is shown while the mutation runs.
| /** Returns a paginated, sortable, filterable list of the ProductOptionGroup's options. */ | ||
| optionList: ProductOptionList; | ||
| options: Array<ProductOption>; | ||
| translations: Array<ProductOptionGroupTranslation>; | ||
| updatedAt: Scalars['DateTime']['output']; | ||
| }; | ||
|
|
||
|
|
||
| export type ProductOptionGroupOptionListArgs = { | ||
| options?: InputMaybe<ProductOptionListOptions>; | ||
| }; | ||
|
|
||
| export type ProductOptionGroupFilterParameter = { | ||
| _and?: InputMaybe<Array<ProductOptionGroupFilterParameter>>; | ||
| _or?: InputMaybe<Array<ProductOptionGroupFilterParameter>>; | ||
| code?: InputMaybe<StringOperators>; | ||
| createdAt?: InputMaybe<DateOperators>; | ||
| id?: InputMaybe<IdOperators>; | ||
| languageCode?: InputMaybe<StringOperators>; | ||
| name?: InputMaybe<StringOperators>; | ||
| updatedAt?: InputMaybe<DateOperators>; | ||
| }; | ||
|
|
||
| export type ProductOptionGroupList = PaginatedList & { | ||
| __typename?: 'ProductOptionGroupList'; | ||
| items: Array<ProductOptionGroup>; | ||
| totalItems: Scalars['Int']['output']; | ||
| }; | ||
|
|
||
| export type ProductOptionGroupListOptions = { | ||
| /** Allows the results to be filtered */ | ||
| filter?: InputMaybe<ProductOptionGroupFilterParameter>; | ||
| /** Specifies whether multiple top-level "filter" fields should be combined with a logical AND or OR operation. Defaults to AND. */ | ||
| filterOperator?: InputMaybe<LogicalOperator>; | ||
| /** Skips the first n results, for use in pagination */ | ||
| skip?: InputMaybe<Scalars['Int']['input']>; | ||
| /** Specifies which properties to sort the results by */ | ||
| sort?: InputMaybe<ProductOptionGroupSortParameter>; | ||
| /** Takes n results, for use in pagination */ | ||
| take?: InputMaybe<Scalars['Int']['input']>; | ||
| }; | ||
|
|
||
| export type ProductOptionGroupSortParameter = { | ||
| code?: InputMaybe<SortOrder>; | ||
| createdAt?: InputMaybe<SortOrder>; | ||
| id?: InputMaybe<SortOrder>; | ||
| name?: InputMaybe<SortOrder>; | ||
| updatedAt?: InputMaybe<SortOrder>; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Paginated ProductOptionGroup surfaces (optionList + list/filter/sort).
Migrate UI from eager options array to optionList with pagination/sorting; add productOptionGroupList screens or adapt existing table data sources.
🏁 Script executed:
rg -n -C3 -g 'packages/**' -P '\boptionList\b|\bproductOptionGroupList\b'Length of output: 41429
Migrate admin-ui to use paginated optionList and productOptionGroupList
Replace all direct uses of the eager options array with the new optionList (field supports pagination/sorting/filtering) and update the ProductOptionGroup screens and data sources to query productOptionGroupList instead.
🤖 Prompt for AI Agents
In packages/admin-ui/src/lib/core/src/common/generated-types.ts around lines
4907 to 4955, the code still relies on the eager `options` array and
ProductOptionGroup queries instead of the new paginated fields; replace all
direct uses of `options` with the new `optionList` type/field (and ensure
selection includes pagination/sorting/filtering args and edges/node or
items/totalCount as your schema uses) and update ProductOptionGroup related
types/usages to use `productOptionGroupList` instead; regenerate or edit the
generated-types to include the `optionList` and `productOptionGroupList` shapes,
update any type references from arrays to the paginated list types, and verify
callers consume the paginated structure (e.g., use .items/edges instead of
direct array) and accept pagination parameters where required.
| productOptionGroupList: ProductOptionGroupList; | ||
| productOptionGroups: Array<ProductOptionGroup>; | ||
| productOptions: ProductOptionList; | ||
| /** Get a ProductVariant by id */ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add productOptionGroupList and productOptions queries to data service.
Expose typed methods and use PaginatedList-aware table sources.
🏁 Script executed:
rg -n -C2 -g 'packages/**' -P '\bproductOptionGroupList\b|\bproductOptions\b'Length of output: 22579
Add productOptionGroupList and productOptions to the Admin UI DataService
The GraphQL schema and generated-types include these queries, but no DataService methods were found (search for productOptionGroupList/productOptions only hits generated-types.ts). Implement typed DataService methods and use PaginatedList-aware table sources.
🤖 Prompt for AI Agents
In packages/admin-ui/src/lib/core/src/common/generated-types.ts around lines
5390 to 5393, the GraphQL queries productOptionGroupList and productOptions are
present in the generated types but there are no corresponding DataService
methods; add two typed DataService methods (e.g., productOptionGroupList and
productOptions) on the Admin UI DataService that mirror the existing list-method
patterns: accept the same args/input types from generated-types, call the
appropriate GraphQL operation, and return PaginatedList<T> (use the
PaginatedList-aware table source types used by other list methods). Ensure the
methods use the same error/response handling conventions as other DataService
list methods, import and reference the correct generated types, and wire these
methods into any table source factories that expect PaginatedList results.
| /** Get a Product either by id or slug. If neither id nor slug is specified, an error will result. */ | ||
| product?: Maybe<Product>; | ||
| productOptionGroup?: Maybe<ProductOptionGroup>; | ||
| productOptionGroupList: ProductOptionGroupList; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm query resolvers and admin UI sync for new list queries
Please verify resolvers implement these queries and the admin UI generated types/operations are updated. Run:
Also applies to: 5316-5316, 5523-5525, 5533-5534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Query resolvers
rg -nP "\\bproductOptionGroupList\\b|\\bproductOptions\\b" packages/core/src/api/resolvers
# Admin UI generated types include new entries
rg -n "productOptionGroupList|productOptions|ProductOption(Group)?List(Options)?" packages/admin-ui/src/lib/core/src/common/generated-types.ts
# GraphQL schema includes queries
rg -n "productOptionGroupList|productOptions" packages/core/src/api/schemaLength of output: 1486
Regenerate Admin UI GraphQL types/operations
Resolvers and schema already implement productOptionGroupList and productOptions, but the Admin UI’s generated types in packages/admin-ui/src/lib/core/src/common/generated-types.ts lack these entries. Run your GraphQL codegen to update the generated types/operations accordingly.
🤖 Prompt for AI Agents
In packages/common/src/generated-types.ts around line 5314, the generated types
are missing the productOptionGroupList and productOptions entries; regenerate
the Admin UI GraphQL types/operations by running the project's GraphQL codegen
for the admin UI (ensure your local schema/operations are up-to-date, run the
repository's codegen script for admin-ui, then commit the updated generated
files) so the generated-types include productOptionGroupList and productOptions.
| @ResolveField() | ||
| async optionList( | ||
| @Ctx() ctx: RequestContext, | ||
| @Parent() optionGroup: ProductOptionGroup, | ||
| @Args() args: { options: ProductOptionListOptions }, | ||
| @Relations({ entity: ProductOption }) relations: RelationPaths<ProductOption>, | ||
| ): Promise<PaginatedList<ProductOption>> { | ||
| return this.productOptionService.findByGroupIdList(ctx, optionGroup.id, args.options, relations); | ||
| } |
There was a problem hiding this comment.
Add missing authorization on optionList field.
Without @Allow, this field may bypass permission checks unlike the existing options field.
Apply:
@ResolveField()
+ @Allow(Permission.ReadCatalog, Permission.Public, Permission.ReadProduct)
async optionList(
@Ctx() ctx: RequestContext,
@Parent() optionGroup: ProductOptionGroup,
- @Args() args: { options: ProductOptionListOptions },
+ @Args() args: { options: ProductOptionListOptions },
@Relations({ entity: ProductOption }) relations: RelationPaths<ProductOption>,
): Promise<PaginatedList<ProductOption>> {
return this.productOptionService.findByGroupIdList(ctx, optionGroup.id, args.options, relations);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @ResolveField() | |
| async optionList( | |
| @Ctx() ctx: RequestContext, | |
| @Parent() optionGroup: ProductOptionGroup, | |
| @Args() args: { options: ProductOptionListOptions }, | |
| @Relations({ entity: ProductOption }) relations: RelationPaths<ProductOption>, | |
| ): Promise<PaginatedList<ProductOption>> { | |
| return this.productOptionService.findByGroupIdList(ctx, optionGroup.id, args.options, relations); | |
| } | |
| @ResolveField() | |
| @Allow(Permission.ReadCatalog, Permission.Public, Permission.ReadProduct) | |
| async optionList( | |
| @Ctx() ctx: RequestContext, | |
| @Parent() optionGroup: ProductOptionGroup, | |
| @Args() args: { options: ProductOptionListOptions }, | |
| @Relations({ entity: ProductOption }) relations: RelationPaths<ProductOption>, | |
| ): Promise<PaginatedList<ProductOption>> { | |
| return this.productOptionService.findByGroupIdList(ctx, optionGroup.id, args.options, relations); | |
| } |
🤖 Prompt for AI Agents
In
packages/core/src/api/resolvers/entity/product-option-group-entity.resolver.ts
around lines 52 to 60, the optionList resolver is missing the @Allow
authorization decorator and should match the existing options field; add
@Allow(Permission.ReadCatalog) above the @ResolveField() (and import Allow and
Permission from @vendure/core if not already imported) so the field enforces the
same permission checks as the options field.
💡 Verification agent
🧩 Analysis chain
Confirm soft-deleted options are excluded.
The legacy options field filters out deleted options. Ensure findByGroupIdList excludes soft-deleted records by default.
Run:
🏁 Script executed:
#!/bin/bash
# Inspect implementation to verify deletedAt filtering
rg -nP -C2 'findByGroupIdList\s*\(' packages/core/src/service/services/product-option.service.ts
rg -nP -C3 'deletedAt\s*(IS NULL|: null|isNull)' packages/core/src/service/services/product-option.service.tsLength of output: 1082
🏁 Script executed:
#!/bin/bash
rg -nP -A20 'findByGroupIdList\s*\(' packages/core/src/service/services/product-option.service.tsLength of output: 991
🏁 Script executed:
#!/bin/bash
# Locate the ListQueryBuilder class definition
rg -nP "class ListQueryBuilder" -n packages/core/src
# Inspect the build method to verify default deletedAt filtering
rg -nP -C5 "build\(" packages/core/src/service/common/list-query-builder.tsLength of output: 372
🏁 Script executed:
#!/bin/bash
# Display the build method in ListQueryBuilder for soft-delete handling
sed -n '150,260p' packages/core/src/service/helpers/list-query-builder/list-query-builder.ts
# Search for any explicit deletedAt references in ListQueryBuilder
rg -nP -C5 "deletedAt" packages/core/src/service/helpers/list-query-builder/list-query-builder.tsLength of output: 3773
🏁 Script executed:
#!/bin/bash
file="packages/core/src/service/helpers/list-query-builder/list-query-builder.ts"
# Locate the 'build' method signature to find its line number
rg -nPn "build<T extends VendureEntity>" "$file"
# Display the body of the 'build' method including default filter logic
sed -n '200,350p' "$file"Length of output: 6395
Exclude soft-deleted options in findByGroupIdList
Add .andWhere('productOption.deletedAt IS NULL') in packages/core/src/service/services/product-option.service.ts inside findByGroupIdList to omit soft-deleted ProductOption records.
🤖 Prompt for AI Agents
In packages/core/src/service/services/product-option.service.ts inside the
findByGroupIdList method (update the query builder where the ProductOption list
is fetched, around the block that filters by groupId), exclude soft-deleted
options by adding an additional condition to the query builder: append
.andWhere('productOption.deletedAt IS NULL') (using the same alias used for the
ProductOption entity) so only records without a deletedAt timestamp are
returned.
| private async ensureUniqueCode(ctx: RequestContext, code: string, id?: ID) { | ||
| let candidate = code; | ||
| let suffix = 1; | ||
| let conflict = false; | ||
| const alreadySuffixed = /-\d+$/; | ||
| do { | ||
| const match = await this.connection | ||
| .getRepository(ctx, ProductOptionGroup) | ||
| .findOne({ where: { code: candidate } }); | ||
|
|
||
| conflict = !!match && ((id != null && !idsAreEqual(match.id, id)) || id == null); | ||
| if (conflict) { | ||
| suffix++; | ||
| if (alreadySuffixed.test(candidate)) { | ||
| candidate = candidate.replace(alreadySuffixed, `-${suffix}`); | ||
| } else { | ||
| candidate = `${candidate}-${suffix}`; | ||
| } | ||
| } | ||
| } while (conflict); | ||
|
|
||
| return candidate; | ||
| } |
There was a problem hiding this comment.
Fix off-by-one in ensureUniqueCode (first conflict yields -2).
Current logic produces code-2 on first clash. Normalize and handle pre-suffixed codes.
Apply:
- private async ensureUniqueCode(ctx: RequestContext, code: string, id?: ID) {
- let candidate = code;
- let suffix = 1;
- let conflict = false;
- const alreadySuffixed = /-\d+$/;
- do {
- const match = await this.connection
- .getRepository(ctx, ProductOptionGroup)
- .findOne({ where: { code: candidate } });
-
- conflict = !!match && ((id != null && !idsAreEqual(match.id, id)) || id == null);
- if (conflict) {
- suffix++;
- if (alreadySuffixed.test(candidate)) {
- candidate = candidate.replace(alreadySuffixed, `-${suffix}`);
- } else {
- candidate = `${candidate}-${suffix}`;
- }
- }
- } while (conflict);
-
- return candidate;
- }
+ private async ensureUniqueCode(ctx: RequestContext, code: string, id?: ID) {
+ const stripSuffix = /-\d+$/;
+ const base = code.replace(stripSuffix, '');
+ let candidate = base;
+ let suffix = 1;
+ // Loop until we find a free code
+ // First conflict -> `${base}-1`
+ // Subsequent conflicts -> increment suffix
+ // If an existing suffixed code exists, we still base on `base`
+ while (true) {
+ const match = await this.connection
+ .getRepository(ctx, ProductOptionGroup)
+ .findOne({ where: { code: candidate } });
+ const conflict = !!match && ((id != null && !idsAreEqual(match.id, id)) || id == null);
+ if (!conflict) {
+ return candidate;
+ }
+ candidate = `${base}-${suffix++}`;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async ensureUniqueCode(ctx: RequestContext, code: string, id?: ID) { | |
| let candidate = code; | |
| let suffix = 1; | |
| let conflict = false; | |
| const alreadySuffixed = /-\d+$/; | |
| do { | |
| const match = await this.connection | |
| .getRepository(ctx, ProductOptionGroup) | |
| .findOne({ where: { code: candidate } }); | |
| conflict = !!match && ((id != null && !idsAreEqual(match.id, id)) || id == null); | |
| if (conflict) { | |
| suffix++; | |
| if (alreadySuffixed.test(candidate)) { | |
| candidate = candidate.replace(alreadySuffixed, `-${suffix}`); | |
| } else { | |
| candidate = `${candidate}-${suffix}`; | |
| } | |
| } | |
| } while (conflict); | |
| return candidate; | |
| } | |
| private async ensureUniqueCode(ctx: RequestContext, code: string, id?: ID) { | |
| const stripSuffix = /-\d+$/; | |
| const base = code.replace(stripSuffix, ''); | |
| let candidate = base; | |
| let suffix = 1; | |
| // Loop until we find a free code | |
| // First conflict -> `${base}-1` | |
| // Subsequent conflicts -> increment suffix | |
| // If an existing suffixed code exists, we still base on `base` | |
| while (true) { | |
| const match = await this.connection | |
| .getRepository(ctx, ProductOptionGroup) | |
| .findOne({ where: { code: candidate } }); | |
| const conflict = !!match && ((id != null && !idsAreEqual(match.id, id)) || id == null); | |
| if (!conflict) { | |
| return candidate; | |
| } | |
| candidate = `${base}-${suffix++}`; | |
| } | |
| } |
🤖 Prompt for AI Agents
packages/core/src/service/services/product-option-group.service.ts around lines
281-303: the current loop increments suffix before applying it which causes the
first conflict to produce "-2"; fix by normalizing the incoming code once
(extract a base and an initial numeric suffix if present using the /-(\d+)$/
regex), set suffix to the parsed number or 0, and then inside the loop on
conflict increment suffix and set candidate = `${base}-${suffix}` (do not run
the regex replace against the evolving candidate each iteration). This ensures
the first conflict yields base-1, subsequent conflicts increment sequentially,
and pre-suffixed inputs are handled correctly.
| .findByIdsInChannel( | ||
| ctx, | ||
| ProductOptionGroup, | ||
| groupsToAssign.map(g => g.id), | ||
| ctx.channelId, | ||
| {}, | ||
| ) | ||
| .then(groups => groups.map(group => translateDeep(group, ctx.languageCode))); | ||
| } |
There was a problem hiding this comment.
Return groups from the target channel, not the current one.
After assigning to input.channelId, querying in ctx.channelId can yield empty/mismatched results.
Apply:
- return this.connection
- .findByIdsInChannel(
- ctx,
- ProductOptionGroup,
- groupsToAssign.map(g => g.id),
- ctx.channelId,
- {},
- )
+ return this.connection
+ .findByIdsInChannel(
+ ctx,
+ ProductOptionGroup,
+ groupsToAssign.map(g => g.id),
+ input.channelId,
+ {},
+ )
.then(groups => groups.map(group => translateDeep(group, ctx.languageCode)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .findByIdsInChannel( | |
| ctx, | |
| ProductOptionGroup, | |
| groupsToAssign.map(g => g.id), | |
| ctx.channelId, | |
| {}, | |
| ) | |
| .then(groups => groups.map(group => translateDeep(group, ctx.languageCode))); | |
| } | |
| return this.connection | |
| .findByIdsInChannel( | |
| ctx, | |
| ProductOptionGroup, | |
| groupsToAssign.map(g => g.id), | |
| input.channelId, | |
| {}, | |
| ) | |
| .then(groups => groups.map(group => translateDeep(group, ctx.languageCode))); |
🤖 Prompt for AI Agents
In packages/core/src/service/services/product-option-group.service.ts around
lines 391 to 399, the code queries ProductOptionGroup using ctx.channelId after
assigning groups to input.channelId causing possible empty/mismatched results;
change the channelId passed to findByIdsInChannel to the target channel (the
input.channelId used for the assignment, or the channel id derived from
groupsToAssign if that is authoritative) so the lookup happens in the same
channel as the assignment, then translate and return those groups.
| product.optionGroups = product.optionGroups.filter(g => g.id !== optionGroupId); | ||
| product.__optionGroups = product.__optionGroups.filter(g => g.id !== optionGroupId); | ||
| await this.connection.getRepository(ctx, Product).save(product, { reload: false }); |
There was a problem hiding this comment.
Fix ID comparison in filters (use idsAreEqual).
Strict inequality can fail with differing ID representations; use the helper consistently.
Apply:
- product.optionGroups = product.optionGroups.filter(g => g.id !== optionGroupId);
- product.__optionGroups = product.__optionGroups.filter(g => g.id !== optionGroupId);
+ product.optionGroups = product.optionGroups.filter(g => !idsAreEqual(g.id, optionGroupId));
+ product.__optionGroups = product.__optionGroups.filter(g => !idsAreEqual(g.id, optionGroupId));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| product.optionGroups = product.optionGroups.filter(g => g.id !== optionGroupId); | |
| product.__optionGroups = product.__optionGroups.filter(g => g.id !== optionGroupId); | |
| await this.connection.getRepository(ctx, Product).save(product, { reload: false }); | |
| product.optionGroups = product.optionGroups.filter(g => !idsAreEqual(g.id, optionGroupId)); | |
| product.__optionGroups = product.__optionGroups.filter(g => !idsAreEqual(g.id, optionGroupId)); | |
| await this.connection.getRepository(ctx, Product).save(product, { reload: false }); |
🤖 Prompt for AI Agents
packages/core/src/service/services/product.service.ts around lines 470 to 472:
the two filter calls currently use strict inequality (g.id !== optionGroupId)
which can fail for differing ID representations; replace both predicates to use
the idsAreEqual helper (e.g. keep entries where !idsAreEqual(g.id,
optionGroupId)). Ensure idsAreEqual is imported from the appropriate utility
module if not already present, then save as before.
2 participants
Description
Opening this new PR after scoping down the work from the previous PR #3579.
Originally, the plan was to support both channel-specific and global
ProductOptionGrouplogic in a single PR.However, to keep things focused and easier to review, this PR now only includes the channel-specific changes.
While working on this, I realized that a shared/global mode could be useful for multiple core entities like
Facet,Collection, andProductOptionGroup, which would make Vendure more flexible for multi-vendor setups.I'd be happy to hear your thoughts or suggestions on this direction.
Changes
ProductOptionGroupandProductOption.ProductandProductOptionGroup.Breaking changes
Checklist
📌 Always:
👍 Most of the time:
Summary by CodeRabbit