[RFC] feat(core): Add partitionKey to StockLevel for sub-location stock tracking#4628
[RFC] feat(core): Add partitionKey to StockLevel for sub-location stock tracking#4628niko91i wants to merge 9 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a partitionKey field across stock-related GraphQL types, inputs, entities, services, schema, and tests. GraphQL types/inputs (StockLevel, StockLevelInput, StockMovement and implementing types) and generated typings now include Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/service/services/stock-level.service.ts (1)
37-62:⚠️ Potential issue | 🟠 MajorConfirm and document: omitting
partitionKeymatches any partition, not the default.The concern is valid. When
partitionKeyis undefined, thewhereclause doesn't includepartitionKey, sofindOne()returns the first matching record regardless of partition. With multiple partitions (e.g.,'BATCH-001'and''), this produces non-deterministic results.The test "returns existing StockLevel without partitionKey filter" (line 70) documents this behavior, but the JSDoc (lines 32-36) only explains the case when
partitionKeyis provided, leaving the omitted case undefined.All production calls in
stock-movement.service.tsexplicitly passpartitionKey, so this edge case isn't hit in practice. However, the API allows omitting it, which creates ambiguity.The suggested fix is appropriate: defaulting to
''(the entity's default value) makes behavior deterministic and prevents unexpected partition matches. Since no production code omitspartitionKey, this change is safe.Alternatively, if the "match any" behavior is intentional for backward compatibility, update the JSDoc to explicitly document it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/services/stock-level.service.ts` around lines 37 - 62, getStockLevel currently omits partitionKey from the findOne where clause when partitionKey is undefined, causing non-deterministic cross-partition matches; change getStockLevel in stock-level.service.ts so that when partitionKey is omitted it defaults to the entity default (e.g. '') and is included in both the findOne and save payloads (ensure the new StockLevel(...) creation also uses the same default), and update the JSDoc for getStockLevel to state that an omitted partitionKey will be treated as the empty-string partition rather than "match any".
🧹 Nitpick comments (6)
packages/core/src/config/catalog/stock-location-strategy.ts (1)
35-52: Clarify precedence when bothstockLevelIdandpartitionKeyare supplied.The interface now permits both selectors; documenting conflict/precedence rules in this contract would prevent divergent custom strategy implementations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/config/catalog/stock-location-strategy.ts` around lines 35 - 52, Update the documentation for the StockLocationStrategy contract to specify the precedence rule when both selectors are provided: state whether stockLevelId takes precedence over partitionKey (or vice versa), and describe the expected behavior (e.g., if stockLevelId is present, implementations must use that specific StockLevel and ignore partitionKey; if stockLevelId is absent but partitionKey present, implementations must target the matching partition). Reference the existing fields stockLevelId and partitionKey and make the rule explicit so custom strategies implementers have a single authoritative precedence to follow.packages/core/src/api/schema/admin-api/product.api.graphql (1)
132-136: Add a short schema description forpartitionKeysemantics.A brief docstring here would help API consumers understand default-partition behavior when
partitionKeyis omitted or empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/api/schema/admin-api/product.api.graphql` around lines 132 - 136, Add a short GraphQL schema description (docstring) to the partitionKey field on the StockLevelInput input type so API consumers understand its semantics: state that when partitionKey is omitted or empty the system uses the default partition, and document any effects (e.g., scope of stock levels or tenant partitioning) and expected format (if any). Update the input StockLevelInput definition to include this brief description next to the partitionKey field to make default-partition behavior explicit.packages/core/e2e/stock-partition-key.e2e-spec.ts (1)
173-175: Consider a stronger assertion for verifying unchanged default stock level.The assertion
toBeGreaterThanOrEqual(0)doesn't verify that the default level'sstockOnHandremained unchanged. Consider capturing the initial value and asserting equality:💡 Suggested improvement
+ // Capture initial default stock level before the update + const { productVariant: initialVariant } = await adminClient.query(getStockLevelsForVariantDocument, { + id: variantId, + }); + const initialDefaultStock = initialVariant?.stockLevels.find(sl => sl.partitionKey === '')?.stockOnHand; + await adminClient.query(setStockLevelWithPartitionDocument, { // ... mutation input }); // ... after fetching updated levels - expect(defaultLevel?.stockOnHand).toBeGreaterThanOrEqual(0); // Unchanged + expect(defaultLevel?.stockOnHand).toBe(initialDefaultStock); // Actually unchanged🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/e2e/stock-partition-key.e2e-spec.ts` around lines 173 - 175, The test currently asserts defaultLevel?.stockOnHand >= 0 which doesn't ensure it stayed unchanged; before performing the stock update capture the initial value (e.g., const initialDefaultStock = defaultLevel?.stockOnHand) and after the update assert equality (expect(defaultLevel?.stockOnHand).toBe(initialDefaultStock)). Update the assertions that reference defaultLevel in the test (symbols: defaultLevel, batch1, batch2) so the default level comparison uses the captured initial value rather than a >= 0 check.packages/core/src/service/services/stock-level.service.ts (2)
132-151: Asymmetric behavior:updateStockAllocatedForLocationsilently ignores missing partitions.Unlike
updateStockOnHandForLocationwhich creates a new record when the partition doesn't exist, this method silently does nothing. This asymmetry could mask bugs where an allocation strategy targets a non-existent partition.Consider either:
- Throwing an error when the partition doesn't exist (fail-fast)
- Creating the record like
updateStockOnHandForLocationdoes (consistent behavior)- Documenting this as intentional behavior
💡 Option 1: Throw error for missing partition
const stockLevel = await this.connection.getRepository(ctx, StockLevel).findOne({ where: { productVariantId, stockLocationId, - ...(partitionKey != null ? { partitionKey } : {}), + partitionKey: partitionKey ?? '', }, }); - if (stockLevel) { + if (!stockLevel) { + throw new Error( + `StockLevel not found for productVariantId=${productVariantId}, ` + + `stockLocationId=${stockLocationId}, partitionKey='${partitionKey ?? ''}'` + ); + } + await this.connection + .getRepository(ctx, StockLevel) + .update(stockLevel.id, { stockAllocated: stockLevel.stockAllocated + change }); - await this.connection - .getRepository(ctx, StockLevel) - .update(stockLevel.id, { stockAllocated: stockLevel.stockAllocated + change }); - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/services/stock-level.service.ts` around lines 132 - 151, The method updateStockAllocatedForLocation currently returns early when no StockLevel is found, causing asymmetric behavior with updateStockOnHandForLocation; update updateStockAllocatedForLocation to mirror updateStockOnHandForLocation: if no StockLevel exists for the given productVariantId, stockLocationId and optional partitionKey, create a new StockLevel record (using the same fields/population logic as updateStockOnHandForLocation) and set stockAllocated to the passed change (or initialize appropriately), otherwise update the existing StockLevel.stockAllocated by adding change; refer to updateStockAllocatedForLocation, updateStockOnHandForLocation, and the StockLevel entity when implementing.
95-125: Same ambiguity issue inupdateStockOnHandForLocation.When
partitionKeyis omitted, this method may find and update an arbitrary partitioned record instead of the default one.Additionally, the control flow could be simplified:
♻️ Suggested refactor for clearer control flow
const stockLevel = await this.connection.getRepository(ctx, StockLevel).findOne({ where: { productVariantId, stockLocationId, - ...(partitionKey != null ? { partitionKey } : {}), + partitionKey: partitionKey ?? '', }, }); - if (!stockLevel) { + if (stockLevel) { + await this.connection + .getRepository(ctx, StockLevel) + .update(stockLevel.id, { stockOnHand: stockLevel.stockOnHand + change }); + } else { await this.connection.getRepository(ctx, StockLevel).save( new StockLevel({ productVariantId, stockLocationId, stockOnHand: change, stockAllocated: 0, - ...(partitionKey != null ? { partitionKey } : {}), + partitionKey: partitionKey ?? '', }), ); } - if (stockLevel) { - await this.connection - .getRepository(ctx, StockLevel) - .update(stockLevel.id, { stockOnHand: stockLevel.stockOnHand + change }); - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/services/stock-level.service.ts` around lines 95 - 125, The updateStockOnHandForLocation method can match an arbitrary partition when partitionKey is omitted; make partitionKey explicit by normalizing it at the top of updateStockOnHandForLocation (e.g., const pk = partitionKey ?? DEFAULT_PARTITION or null) and use pk in the findOne, save/create, and update calls so the lookup and mutation always target the same partition; also simplify control flow to: find by productVariantId/stockLocationId/pk, if found update via repository.update(stockLevel.id, { stockOnHand: stockLevel.stockOnHand + change }) else create/save a new StockLevel with stockOnHand = change and partitionKey = pk (or use repository.upsert if available) so there is no ambiguity between partitions.packages/core/src/service/services/stock-level.service.spec.ts (1)
149-154: This asymmetry is intentional and validated by tests, but documentation should clarify the design rationale.The test correctly validates that
updateStockAllocatedForLocationsilently does nothing when thepartitionKeydoesn't exist. This behavior differs intentionally fromupdateStockOnHandForLocation, which creates a new partition (test at line 122–128 explicitly validates this).The design makes sense: receiving stock to a non-existent partition is valid (new batch arrival), while allocating to a non-existent partition likely indicates a bug in allocation logic. However, both methods have identical documentation ("only the matching partitioned StockLevel will be updated"), which obscures this intentional difference. Consider expanding the docstring for
updateStockAllocatedForLocationto clarify that it does not create partitions, making the behavior explicit to callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/services/stock-level.service.spec.ts` around lines 149 - 154, Update the docstring for updateStockAllocatedForLocation to explicitly state that unlike updateStockOnHandForLocation it will not create a new partition when the provided partitionKey does not exist and will instead be a no-op; mention the rationale that allocating to a non-existent partition is treated as a logic error while receiving stock may create partitions, and reference both method names (updateStockAllocatedForLocation and updateStockOnHandForLocation) in the comment so callers understand the intentional asymmetry validated by tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/service/services/stock-level.service.spec.ts`:
- Around line 70-75: The test for getStockLevel in stock-level.service.spec.ts
is non-deterministic because omitting partitionKey allows any matching
StockLevel to be returned; update the test to be deterministic by providing an
explicit partitionKey when calling service.getStockLevel(ctx, variantId,
locationId, partitionKey) (use the same partitionKey used when creating the
expected StockLevel in the test setup), or alternatively change the assertion to
not assume id === 1 (e.g., assert the returned record matches the expected
partitionKey and variant/location instead of a specific id); target the
getStockLevel call and its expectations to ensure the mock and assertions align.
---
Outside diff comments:
In `@packages/core/src/service/services/stock-level.service.ts`:
- Around line 37-62: getStockLevel currently omits partitionKey from the findOne
where clause when partitionKey is undefined, causing non-deterministic
cross-partition matches; change getStockLevel in stock-level.service.ts so that
when partitionKey is omitted it defaults to the entity default (e.g. '') and is
included in both the findOne and save payloads (ensure the new StockLevel(...)
creation also uses the same default), and update the JSDoc for getStockLevel to
state that an omitted partitionKey will be treated as the empty-string partition
rather than "match any".
---
Nitpick comments:
In `@packages/core/e2e/stock-partition-key.e2e-spec.ts`:
- Around line 173-175: The test currently asserts defaultLevel?.stockOnHand >= 0
which doesn't ensure it stayed unchanged; before performing the stock update
capture the initial value (e.g., const initialDefaultStock =
defaultLevel?.stockOnHand) and after the update assert equality
(expect(defaultLevel?.stockOnHand).toBe(initialDefaultStock)). Update the
assertions that reference defaultLevel in the test (symbols: defaultLevel,
batch1, batch2) so the default level comparison uses the captured initial value
rather than a >= 0 check.
In `@packages/core/src/api/schema/admin-api/product.api.graphql`:
- Around line 132-136: Add a short GraphQL schema description (docstring) to the
partitionKey field on the StockLevelInput input type so API consumers understand
its semantics: state that when partitionKey is omitted or empty the system uses
the default partition, and document any effects (e.g., scope of stock levels or
tenant partitioning) and expected format (if any). Update the input
StockLevelInput definition to include this brief description next to the
partitionKey field to make default-partition behavior explicit.
In `@packages/core/src/config/catalog/stock-location-strategy.ts`:
- Around line 35-52: Update the documentation for the StockLocationStrategy
contract to specify the precedence rule when both selectors are provided: state
whether stockLevelId takes precedence over partitionKey (or vice versa), and
describe the expected behavior (e.g., if stockLevelId is present,
implementations must use that specific StockLevel and ignore partitionKey; if
stockLevelId is absent but partitionKey present, implementations must target the
matching partition). Reference the existing fields stockLevelId and partitionKey
and make the rule explicit so custom strategies implementers have a single
authoritative precedence to follow.
In `@packages/core/src/service/services/stock-level.service.spec.ts`:
- Around line 149-154: Update the docstring for updateStockAllocatedForLocation
to explicitly state that unlike updateStockOnHandForLocation it will not create
a new partition when the provided partitionKey does not exist and will instead
be a no-op; mention the rationale that allocating to a non-existent partition is
treated as a logic error while receiving stock may create partitions, and
reference both method names (updateStockAllocatedForLocation and
updateStockOnHandForLocation) in the comment so callers understand the
intentional asymmetry validated by tests.
In `@packages/core/src/service/services/stock-level.service.ts`:
- Around line 132-151: The method updateStockAllocatedForLocation currently
returns early when no StockLevel is found, causing asymmetric behavior with
updateStockOnHandForLocation; update updateStockAllocatedForLocation to mirror
updateStockOnHandForLocation: if no StockLevel exists for the given
productVariantId, stockLocationId and optional partitionKey, create a new
StockLevel record (using the same fields/population logic as
updateStockOnHandForLocation) and set stockAllocated to the passed change (or
initialize appropriately), otherwise update the existing
StockLevel.stockAllocated by adding change; refer to
updateStockAllocatedForLocation, updateStockOnHandForLocation, and the
StockLevel entity when implementing.
- Around line 95-125: The updateStockOnHandForLocation method can match an
arbitrary partition when partitionKey is omitted; make partitionKey explicit by
normalizing it at the top of updateStockOnHandForLocation (e.g., const pk =
partitionKey ?? DEFAULT_PARTITION or null) and use pk in the findOne,
save/create, and update calls so the lookup and mutation always target the same
partition; also simplify control flow to: find by
productVariantId/stockLocationId/pk, if found update via
repository.update(stockLevel.id, { stockOnHand: stockLevel.stockOnHand + change
}) else create/save a new StockLevel with stockOnHand = change and partitionKey
= pk (or use repository.upsert if available) so there is no ambiguity between
partitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c51b6ba3-59cd-4e68-bdb8-9a093d49b578
📒 Files selected for processing (13)
packages/admin-ui/src/lib/core/src/common/generated-types.tspackages/asset-server-plugin/e2e/graphql/graphql-env-admin.d.tspackages/common/src/generated-types.tspackages/core/e2e/graphql/graphql-env-admin.d.tspackages/core/e2e/stock-partition-key.e2e-spec.tspackages/core/src/api/schema/admin-api/product.api.graphqlpackages/core/src/api/schema/admin-api/stock-level.type.graphqlpackages/core/src/config/catalog/stock-location-strategy.tspackages/core/src/entity/stock-level/stock-level.entity.tspackages/core/src/service/services/stock-level.service.spec.tspackages/core/src/service/services/stock-level.service.tspackages/core/src/service/services/stock-movement.service.tsschema-admin.json
- Default partitionKey to '' instead of omitting from where clause,
preventing non-deterministic cross-partition matches
- Add JSDoc documenting default partition behavior, asymmetric
creation semantics, and stockLevelId/partitionKey precedence rule
- Add GraphQL description to StockLevelInput.partitionKey field
- Fix non-deterministic unit test assertion
- Capture initial stock values in E2E test for precise comparison
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/config/catalog/stock-location-strategy.ts`:
- Around line 35-60: The documented precedence where stockLevelId overrides
partitionKey is not implemented; update the service layer to honor it by (1)
extending StockLevelService methods (getStockLevel,
updateStockOnHandForLocation, updateStockAllocatedForLocation) to accept an
optional stockLevelId parameter and use it to fetch/update the exact StockLevel
when provided, (2) modifying StockMovementService
allocation/sale/release/cancellation flows to propagate
LocationWithQuantity.stockLevelId (prefer stockLevelId over partitionKey when
both present), and (3) adjusting related type signatures/interfaces to include
stockLevelId so custom StockLocationStrategy values are not ignored;
alternatively, if you prefer the simpler route, remove stockLevelId from
LocationWithQuantity and update the JSDoc to reflect partitionKey-only support.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 162fad51-8708-4925-a720-337ff0c109fd
📒 Files selected for processing (5)
packages/core/e2e/stock-partition-key.e2e-spec.tspackages/core/src/api/schema/admin-api/product.api.graphqlpackages/core/src/config/catalog/stock-location-strategy.tspackages/core/src/service/services/stock-level.service.spec.tspackages/core/src/service/services/stock-level.service.ts
✅ Files skipped from review due to trivial changes (2)
- packages/core/src/service/services/stock-level.service.spec.ts
- packages/core/e2e/stock-partition-key.e2e-spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/service/services/stock-level.service.ts
The documented precedence rule (stockLevelId > partitionKey) was not implemented in the service layer. Rather than adding complexity, remove stockLevelId entirely — partitionKey alone is sufficient to identify a specific StockLevel partition.
StockMovement records now track which stock partition they affect, enabling per-batch/lot movement history and full traceability.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/service/services/stock-movement.service.ts (1)
175-191: Consider normalizingpartitionKeyonce per loop iteration for DRY/consistency.This reduces duplication and prevents accidental divergence between entity value and service-call argument in future edits.
Refactor pattern (example)
for (const saleLocation of saleLocations) { + const partitionKey = saleLocation.partitionKey ?? ''; const sale = new Sale({ productVariant, quantity: lineRow.quantity * -1, orderLine, stockLocation: saleLocation.location, - partitionKey: saleLocation.partitionKey ?? '', + partitionKey, }); sales.push(sale); if (this.trackInventoryForVariant(productVariant, globalTrackInventory)) { await this.stockLevelService.updateStockAllocatedForLocation( ctx, orderLine.productVariantId, saleLocation.location.id, -saleLocation.quantity, - saleLocation.partitionKey, + partitionKey, ); await this.stockLevelService.updateStockOnHandForLocation( ctx, orderLine.productVariantId, saleLocation.location.id, -saleLocation.quantity, - saleLocation.partitionKey, + partitionKey, ); } }Also applies to: 232-255, 296-312, 348-363
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/services/stock-movement.service.ts` around lines 175 - 191, The loop constructs an Allocation and then calls updateStockAllocatedForLocation passing allocationLocation.partitionKey directly, which duplicates the partitionKey expression and risks divergence; normalize partitionKey at the top of the iteration (e.g., const partitionKey = allocationLocation.partitionKey ?? '') and use that variable for the Allocation constructor (Allocation's partitionKey) and for the updateStockAllocatedForLocation call; apply the same change for the other similar blocks referenced (around the Allocation creation and calls to this.trackInventoryForVariant and this.stockLevelService.updateStockAllocatedForLocation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/service/services/stock-movement.service.ts`:
- Around line 175-191: The loop constructs an Allocation and then calls
updateStockAllocatedForLocation passing allocationLocation.partitionKey
directly, which duplicates the partitionKey expression and risks divergence;
normalize partitionKey at the top of the iteration (e.g., const partitionKey =
allocationLocation.partitionKey ?? '') and use that variable for the Allocation
constructor (Allocation's partitionKey) and for the
updateStockAllocatedForLocation call; apply the same change for the other
similar blocks referenced (around the Allocation creation and calls to
this.trackInventoryForVariant and
this.stockLevelService.updateStockAllocatedForLocation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d4416ea3-4bf8-4542-9665-df0fa5b7544c
📒 Files selected for processing (3)
packages/core/src/api/schema/admin-api/stock-movement.type.graphqlpackages/core/src/entity/stock-movement/stock-movement.entity.tspackages/core/src/service/services/stock-movement.service.ts
|
Tested this PR against a production marketplace codebase via patch-package. Batch/lot tracking with multiple StockLevels per variant+location works as expected. Also added partitionKey propagation to StockMovement entities for full traceability. |
| quantity: delta, | ||
| stockLocation: { id: input.stockLocationId }, | ||
| productVariant: { id: productVariantId }, | ||
| partitionKey: input.partitionKey ?? '', |
There was a problem hiding this comment.
this is a bit nitpicky (sorry) because i do understand that once the default partitionkey exists it generally wont change, but i'd really like to see some form of constant like DEFAULT_STOCK_LOCATION_PARTITION_KEY (or something shorter) because the current default of "" is being reused throughout many different files, comments, services, tests, .. and having a named constant makes it easier to understand unfamiliar parts of code intuitively
other than that really cool PR! 🙏
…eview feedback Replace hardcoded empty-string defaults with a named constant as suggested by @DanielBiegler. Exported from @vendure/core for use in custom StockLocationStrategy implementations.
|
Thanks for the review @DanielBiegler ! I'm already using this pattern in production via patch-package for a marketplace with batch/lot tracking. Before I build further on it, is this approach likely to be accepted (with potential refinements), or would you recommend a different direction? |
Can't say, gotta wait for a comment from the maintainers. I've not looked into this topic deeply, so I don't have informed opinions specifically there, but the pr looks generally well made from my short glance! I think a research section comparing how other ecom frameworks deal with this task, can help. Generally I'd say keeping track of batches/lots sounds reasonable to support because Vendure aims to be enterprisy/B2B, where this kind of stuff happens throughout different types of businesses |
The MultiChannelStockLocationStrategy.forAllocation() previously used find() to get a single StockLevel per location, ignoring other partitions. Now uses filter() to iterate over all partitions at each location, allocating from each until the requested quantity is fulfilled. Updated BaseStockLocationStrategy.getLocationsBasedOnAllocations() to group by locationId+partitionKey for correct sale/release/cancellation. Added 8 unit tests covering multi-partition allocation, partition-aware sale/release/cancellation, and single-partition backward compatibility.
|
Nice work on this PR — the problem is well-defined, the entity/service plumbing is clean, and the test coverage is nice. Just wanted to flag one issue
This causes allocations from different partitions at the same location to get merged. Since ImpactIn a normal order lifecycle:
Result:
Extensibility Issue
To fix behavior, users would need to reimplement:
This is unnecessarily complex and error-prone. Suggested FixUpdate
This ensures partition context is preserved across the full order lifecycle. Minor IssueIn
Risk: I’m not a maintainer, so probably its best to wait for a maintainer’s input on this |
StockLocationService.delete() now matches and creates StockLevel records by partitionKey when transferring stock to another location, preventing partition merge and preserving batch traceability.
|
Worth noting the dashboard doesn't query or send partitionkey yet - admins would see |
|
Hi @niko91i, thanks for taking the time to share your solution here. I've not got time for a deep-dive review on this right now, as it is quite a complex topic touching many parts of the inventory & order flow. I just wanted to let you know it's on my radar and might be something we are interested in if it aligns with our overall strategy in terms of what features we want to work on in the coming months. Sorry I can't be more specific than that right now. |
4 participants
Context
This PR is a working proof-of-concept related to a discussion on Discord:
https://discord.com/channels/1100672177260478564/1488690570296692909
The current
StockLevelentity enforces a unique constraint on[productVariantId, stockLocationId],meaning only one stock record can exist per variant per location. This forces use cases like batch/lot
tracking, serial number management, or expiration-based stock rotation to create one
StockLocationper batch — polluting a table designed for physical locations.
At scale (e.g. 10,000 vendors × 30 products × 6 batches/year = 1.8M rows/year), the
stock_locationtable becomes unmanageable for its original purpose.
Proposed solution
Add an optional
partitionKeycolumn toStockLevel, allowing multiple stock records for the samevariant+location pair, each identified by a unique key.
partitionKeydefaults to''(empty string) — existing behavior is 100% unchanged[productVariantId, stockLocationId, partitionKey]LocationWithQuantitygains optionalpartitionKeyandstockLevelIdfieldsStockLevelServicemethods accept an optionalpartitionKeyparameterStockMovementServicepropagatespartitionKeythrough all allocation/sale/release/cancellation flowsChanges
stock-level.entity.ts— NewpartitionKeycolumn (default'') + updated unique indexstock-location-strategy.ts— OptionalstockLevelIdandpartitionKeyonLocationWithQuantitystock-level.service.ts— OptionalpartitionKeyparameter ongetStockLevel,updateStockOnHand,updateStockAllocatedstock-movement.service.ts— PropagatepartitionKeythrough 5 call sitespartitionKeyonStockLeveltype andStockLevelInputWhat does NOT change
StockLocationentity — untouchedStockLocationStrategyinterface — method signatures unchangedDefaultStockLocationStrategy/MultiChannelStockLocationStrategy— no modificationspartitionKeywork identicallyTest plan
StockLevelServicewith/withoutpartitionKey