feat(elasticsearch-plugin): Extend ElasticSearch to also support groupBySKU for multi-vendor store scenarios #3528
Conversation
…se on sku.keyword instead of productId
|
@LeftoversTodayAppAdmin is attempting to deploy a commit to the Vendure Team on Vercel. A member of the Team first needs to authorize it. |
…ted based on type of grouping.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis change introduces support for grouping Elasticsearch search results by SKU in addition to the existing product grouping. It updates the GraphQL schema, service logic, type definitions, and tests to handle the new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQL API
participant ElasticsearchService
participant Elasticsearch
Client->>GraphQL API: Search with SearchInput (groupBySKU or groupByProduct)
GraphQL API->>ElasticsearchService: search(input)
ElasticsearchService->>ElasticsearchService: Check groupBySKU and groupByProduct (error if both true)
ElasticsearchService->>Elasticsearch: Build query with collapse (by SKU or product)
Elasticsearch-->>ElasticsearchService: Return search results
ElasticsearchService->>ElasticsearchService: Aggregate and map results (by SKU or product)
ElasticsearchService-->>GraphQL API: Return grouped results
GraphQL API-->>Client: Respond with search results
Possibly related issues
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (19)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/elasticsearch-plugin/e2e/elasticsearch-plugin-uuid.e2e-spec.ts (2)
69-79: Fix duplicate test description.The test logic is correct, but the test description is identical to the previous test, making it difficult to distinguish between them.
- it('no term or filters', async () => { + it('no term or filters grouped by SKU', async () => {
94-105: Fix duplicate test description.The test logic is correct, but the test description is identical to an earlier test.
- it('with search term', async () => { + it('with search term grouped by SKU', async () => {packages/elasticsearch-plugin/src/elasticsearch.service.ts (2)
195-199: Improve error message for mutual exclusivity.The validation logic is correct, but the error message could be more helpful to users.
Consider a more descriptive error message:
- throw new InternalServerError( - 'groupByProduct and groupBySKU cannot be used at the same time, you can only group by one attribute.', - ); + throw new InternalServerError( + 'Cannot use both groupByProduct and groupBySKU simultaneously. Please set only one of these options to true.', + );
553-553: Consider using a single parameter for grouping mode.Having two mutually exclusive boolean parameters can be confusing and error-prone.
Consider using an enum or a single parameter to represent the grouping mode:
enum GroupingMode { None = 'none', Product = 'product', SKU = 'sku' } private mapProductToSearchResult(hit: SearchHit<VariantIndexItem>, groupingMode: GroupingMode = GroupingMode.None): ElasticSearchResult { // Update the logic to use groupingMode }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/elasticsearch-plugin/e2e/fixtures/e2e-products-full.csvis excluded by!**/*.csv
📒 Files selected for processing (8)
packages/elasticsearch-plugin/e2e/e2e-helpers.ts(6 hunks)packages/elasticsearch-plugin/e2e/elasticsearch-plugin-uuid.e2e-spec.ts(2 hunks)packages/elasticsearch-plugin/e2e/elasticsearch-plugin.e2e-spec.ts(11 hunks)packages/elasticsearch-plugin/src/api/api-extensions.ts(1 hunks)packages/elasticsearch-plugin/src/build-elastic-body.ts(2 hunks)packages/elasticsearch-plugin/src/elasticsearch.service.ts(16 hunks)packages/elasticsearch-plugin/src/options.ts(2 hunks)packages/elasticsearch-plugin/src/types.ts(1 hunks)
🔇 Additional comments (15)
packages/elasticsearch-plugin/src/types.ts (1)
21-21: LGTM! Clean type definition extension.The addition of the optional
groupBySKUboolean property follows the established pattern and aligns with the GraphQL schema extension.packages/elasticsearch-plugin/src/api/api-extensions.ts (1)
41-41: LGTM! Consistent GraphQL schema extension.The
groupBySKUfield addition to the SearchInput type properly extends the GraphQL schema and aligns with the corresponding TypeScript type definition.packages/elasticsearch-plugin/e2e/elasticsearch-plugin-uuid.e2e-spec.ts (1)
57-67: Update expected count is correct.The test correctly verifies the groupByProduct functionality with the updated expected result count.
packages/elasticsearch-plugin/e2e/e2e-helpers.ts (3)
31-41: LGTM! Updated expected count for product grouping.The test helper correctly reflects the updated expected result count for product grouping.
43-54: Excellent test coverage for SKU grouping.The new
testGroupBySKUhelper function provides focused test coverage by testing with a specific search term "bonsai" to verify SKU grouping behavior.
56-67: Good explicit parameter setting.Explicitly setting
groupBySKU: falsein the no-grouping test helps ensure clarity about the expected behavior and prevents any ambiguity.packages/elasticsearch-plugin/src/build-elastic-body.ts (2)
25-25: LGTM! Consistent parameter extraction.The
groupBySKUparameter is properly extracted from the input alongside other search parameters.
151-153: Verify mutual exclusivity validation exists.The collapse logic for SKU grouping is correctly implemented. However, both
groupByProductandgroupBySKUcould potentially be true simultaneously, with the latter overriding the former. The PR objectives mention mutual exclusivity validation - please verify this validation is implemented in the service layer.#!/bin/bash # Description: Verify that mutual exclusivity validation exists for groupByProduct and groupBySKU # Search for validation logic that prevents both grouping options from being used together rg -A 10 -B 5 "groupByProduct.*groupBySKU|groupBySKU.*groupByProduct" --type tspackages/elasticsearch-plugin/e2e/elasticsearch-plugin.e2e-spec.ts (3)
53-53: LGTM!The import of
testGroupBySKUhelper function is correctly added to support the new SKU grouping test cases.
207-208: Test coverage for SKU grouping looks good.The new test cases appropriately cover the SKU grouping functionality for both shop and admin APIs, following the existing test pattern.
Also applies to: 485-486
251-252: Verify the test data changes are intentional.The count increments across multiple test assertions suggest changes to the test data. Please confirm these increases are related to the SKU grouping feature or document what test data was added.
Also applies to: 270-271, 318-319, 333-333, 347-347, 413-413, 426-426, 465-465, 478-478
packages/elasticsearch-plugin/src/options.ts (1)
195-197: Documentation updates are clear and accurate.The updated comments correctly document the mutual exclusivity of
groupByProductandgroupBySKUoptions and clarify when custom mappings are accessible.Also applies to: 249-251
packages/elasticsearch-plugin/src/elasticsearch.service.ts (3)
279-284: Cardinality aggregation correctly implemented.The use of
'sku.keyword'for SKU grouping is correct, as it ensures exact matching on the non-analyzed SKU field.
409-417: SKU aggregation follows consistent pattern.The implementation correctly mirrors the product grouping logic and uses the appropriate field for SKU cardinality.
636-638: Custom mappings logic correctly handles both grouping modes.The implementation properly exposes product mappings when grouping by product or SKU, and variant mappings when not grouping, which aligns with the documented behavior.
Also applies to: 647-649
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Description
groupByProduct works well when within a single channel a product has multiple variants and you want to group by the product to have a single row returned.
With multi-vendor stores, multiple vendors can list the same box of cookies with the same barcode as SKU to independently manage their own stock values etc. When querying the default channel we now see multiple products that are the same with the same SKU (barcode). By extending ES to also support groupBySKU, we can now get a unique list of all items across all vendors, grouped by SKU.
Please include a summary of the changes and the related issue.
#3527
Breaking changes
Does this PR include any breaking changes we should be aware of?
No, only adds support for a new type of grouping and is fully backwards compatible. You can however only use one type of grouping at a time, either groupByProduct or groupBySKU, if both are used at the same time a new error is thrown telling the user they can only use one at a time.
Screenshots
You can add screenshots here if applicable.
Checklist
📌 Always:
👍 Most of the time:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests