fix(core): Filter StockMovements by type when fetching by product variant #3713
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@knoid has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughMade the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as GraphQL Client
participant API as GraphQL Resolver
participant Service as StockMovementService
participant DB as Database
Client->>API: query GetStockMovementByType(id, type)
API->>Service: getStockMovementsByProductVariantId(ctx, variantId, { type })
Service->>DB: build query (WHERE productVariantId = ... [AND type = ...])
DB-->>Service: rows, count
Service-->>API: { items, totalItems }
API-->>Client: GraphQL response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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 (
|
knoid
force-pushed
the
stockMovement-type
branch
from
0dfb0dc to
be419e2
Compare
knoid
force-pushed
the
stockMovement-type
branch
from
be419e2 to
2701c4c
Compare
knoid
force-pushed
the
stockMovement-type
branch
from
2701c4c to
43349ff
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/e2e/stock-control.e2e-spec.ts (2)
24-24: Use CreateAddressInput from shop codegen to match the mutation’s variable typesYou’re casting inputs for SET_SHIPPING_ADDRESS to CreateAddressInput, but importing the type from the admin codegen. It works due to structural typing, but it’s confusing and easy to mix up. Prefer importing CreateAddressInput from the shop codegen to match the mutation.
Apply this diff to adjust the imports:
-import { - ErrorCode as AdminErrorCode, - CreateAddressInput, - FulfillmentFragment, - GlobalFlag, - StockMovementType, - UpdateProductVariantInput, - VariantWithStockFragment, -} from './graphql/generated-e2e-admin-types'; +import { + ErrorCode as AdminErrorCode, + FulfillmentFragment, + GlobalFlag, + StockMovementType, + UpdateProductVariantInput, + VariantWithStockFragment, +} from './graphql/generated-e2e-admin-types';And include CreateAddressInput in the shop types import:
-import { ErrorCode, PaymentInput } from './graphql/generated-e2e-shop-types'; +import { CreateAddressInput, ErrorCode, PaymentInput } from './graphql/generated-e2e-shop-types';
357-373: Rename test description to reflect ALLOCATION filterThe test asserts ALLOCATION movements, not sales. Rename for clarity.
Apply this diff:
-it('returns all sales', async () => { +it('returns allocations filtered by type', async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/e2e/graphql/shared-definitions.ts(1 hunks)packages/core/e2e/stock-control.e2e-spec.ts(4 hunks)packages/core/src/service/services/stock-movement.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/service/services/stock-movement.service.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/e2e/stock-control.e2e-spec.ts (2)
packages/core/e2e/graphql/generated-e2e-admin-types.ts (2)
GetStockMovementByTypeQuery(9679-9696)GetStockMovementByTypeQueryVariables(9674-9677)packages/core/e2e/graphql/shared-definitions.ts (1)
GET_STOCK_MOVEMENT_BY_TYPE(297-315)
🔇 Additional comments (4)
packages/core/e2e/graphql/shared-definitions.ts (1)
296-315: New typed query for per-type stock movements looks correctThe GraphQL query is well-formed, uses the StockMovement interface fragment appropriately, and scopes the filter to the stockMovements field via options: { type: $type }. Export and naming are consistent with existing patterns.
packages/core/e2e/stock-control.e2e-spec.ts (3)
17-17: Import additions/reorganization look goodThe added imports (TEST_SETUP_TIMEOUT_MS, Codegen namespace, shop types, and GET_STOCK_MOVEMENT_BY_TYPE) align with the new helper/test usage.
Also applies to: 21-21, 32-32, 38-38
115-121: Helper for type-filtered stock movements is clear and typedThe helper cleanly mirrors the existing getProductWithStockMovement and correctly wires Codegen types with the new GET_STOCK_MOVEMENT_BY_TYPE query.
1246-1248: Non-functional formatting changeThe reformatting of the activeOrder query is fine and has no functional impact.
knoid
force-pushed
the
stockMovement-type
branch
2 times, most recently
from
69220a6 to
2dc0157
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/e2e/stock-control.e2e-spec.ts (3)
21-25: Use CreateAddressInput from shop types for consistencyThe CreateAddressInput is used in shop API calls (SetShippingAddress). Importing it from admin types works due to structural typing, but it’s confusing and couples a shop concern to admin types. Prefer importing from the shop-generated types.
Apply these diffs to align imports:
- import { - ErrorCode as AdminErrorCode, - CreateAddressInput, - FulfillmentFragment, - GlobalFlag, - StockMovementType, - UpdateProductVariantInput, - VariantWithStockFragment, - } from './graphql/generated-e2e-admin-types'; + import { + ErrorCode as AdminErrorCode, + FulfillmentFragment, + GlobalFlag, + StockMovementType, + UpdateProductVariantInput, + VariantWithStockFragment, + } from './graphql/generated-e2e-admin-types';And update the shop types import (see lines 32-33).
32-33: Co-locate CreateAddressInput with shop typesFollow-up to the previous comment: import CreateAddressInput from the shop types to match its usage locations.
-import { ErrorCode, PaymentInput } from './graphql/generated-e2e-shop-types'; +import { CreateAddressInput, ErrorCode, PaymentInput } from './graphql/generated-e2e-shop-types';
357-373: Rename test: it asserts ALLOCATIONs, not SalesThe test name says “returns all sales” but it filters for
StockMovementType.ALLOCATIONand asserts allocations. Rename to avoid confusion and future maintenance mistakes.- it('returns all sales', async () => { + it('returns allocations when filtered by type', async () => {Optional: Consider adding a follow-up test after “creates a Sale on Fulfillment creation” that filters by
StockMovementType.SALEto validate filtering across multiple types (SALE, RELEASE, CANCELLATION) and strengthen coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/e2e/graphql/shared-definitions.ts(1 hunks)packages/core/e2e/stock-control.e2e-spec.ts(4 hunks)packages/core/src/service/services/stock-movement.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/service/services/stock-movement.service.ts
- packages/core/e2e/graphql/shared-definitions.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/e2e/stock-control.e2e-spec.ts (1)
packages/core/e2e/graphql/shared-definitions.ts (1)
GET_STOCK_MOVEMENT_BY_TYPE(316-334)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: codegen / codegen
- GitHub Check: e2e tests (22.x, postgres)
- GitHub Check: e2e tests (22.x, sqljs)
- GitHub Check: e2e tests (24.x, mariadb)
- GitHub Check: e2e tests (22.x, mariadb)
- GitHub Check: e2e tests (20.x, mysql)
- GitHub Check: e2e tests (20.x, sqljs)
- GitHub Check: build (24.x)
- GitHub Check: e2e tests (20.x, mariadb)
- GitHub Check: build (20.x)
- GitHub Check: unit tests (24.x)
- GitHub Check: publish_install (macos-latest, 24.x)
- GitHub Check: publish_install (macos-latest, 20.x)
- GitHub Check: publish_install (windows-latest, 22.x)
- GitHub Check: publish_install (windows-latest, 20.x)
- GitHub Check: publish_install (ubuntu-latest, 20.x)
- GitHub Check: publish_install (windows-latest, 24.x)
- GitHub Check: publish_install (macos-latest, 22.x)
- GitHub Check: publish_install (ubuntu-latest, 24.x)
- GitHub Check: publish_install (ubuntu-latest, 22.x)
🔇 Additional comments (4)
packages/core/e2e/stock-control.e2e-spec.ts (4)
17-17: LGTM: import usage is correct
TEST_SETUP_TIMEOUT_MSandtestConfig()are both used and consistent with the test environment setup.
38-39: LGTM: new query import
GET_STOCK_MOVEMENT_BY_TYPEimport is correct and used by the new e2e test.
115-121: LGTM: helper for type-filtered stock movementsHelper correctly queries the Admin API using the new GraphQL operation and typed variables. Straightforward and reusable.
1246-1248: LGTM: minor formattingFormatting-only change; no functional impact.
knoid
force-pushed
the
stockMovement-type
branch
2 times, most recently
from
538efde to
1218859
Compare
knoid
force-pushed
the
stockMovement-type
branch
from
1218859 to
9315d4b
Compare
knoid
force-pushed
the
stockMovement-type
branch
from
9315d4b to
6c48a55
Compare
|
2 participants
Description
Function
getStockMovementsByProductVariantIdaccepts a type filter but was not really filtering by that field. This PR adds that condition when the type filter is requested.Breaking changes
Does this PR include any breaking changes we should be aware of? No.
Screenshots
You can add screenshots here if applicable.
Checklist
📌 Always:
👍 Most of the time:
Summary by CodeRabbit
New Features
Refactor
Tests