Feet(core) shop api mutation to update order currency code #4033
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ |
WalkthroughAdds a Shop API mutation setCurrencyCodeForOrder to re-price an active order in a different currency, implements OrderService.updateOrderCurrency and resolver wiring, records a new ORDER_CURRENCY_UPDATED history entry, updates schema/types and generated GraphQL artifacts, and adds e2e tests exercising the flow. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant ShopOrderResolver as Resolver
participant OrderService as Service
participant RequestContextService as RCService
participant HistoryService as History
participant Database as DB
participant EventBus as Events
Client->>Resolver: setCurrencyCodeForOrder(currencyCode)
Resolver->>Resolver: permission checks
Resolver->>Service: updateOrderCurrency(ctx, orderId, currencyCode)
Service->>DB: load active order (strict)
Service->>Service: validate mutability & channel currency availability
alt currency not available
Service-->>Resolver: throw UserInputError
Resolver-->>Client: Error
else no change
Service-->>Resolver: return existing Order
Resolver-->>Client: Success
else currency change required
Service->>RCService: create context(with currency)
RCService-->>Service: new context
Service->>DB: update order.currencyCode & persist
Service->>History: record ORDER_CURRENCY_UPDATED(previous,new)
History->>DB: persist history entry
Service->>Service: apply price adjustments under new context
Service->>DB: persist updated prices
Service->>Events: emit Order updated event
Events-->>Service: dispatched
Service-->>Resolver: return updated Order
Resolver-->>Client: Success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 (3)
packages/core/src/service/services/history.service.ts (1)
4-10:ORDER_CURRENCY_UPDATEDhistory payload should useCurrencyCodefornewCurrency
OrderService.updateOrderCurrency()records history withdata: { previousCurrency: previousCurrencyCode, newCurrency: currencyCode }, where both values areCurrencyCode. TypingnewCurrencyasIDhere is inconsistent and can confuse consumers ofOrderHistoryEntryData, even though it’s assignable at runtime.Recommend aligning the type:
- [HistoryEntryType.ORDER_CURRENCY_UPDATED]: { - previousCurrency?: CurrencyCode; - newCurrency?: ID; - }; + [HistoryEntryType.ORDER_CURRENCY_UPDATED]: { + previousCurrency?: CurrencyCode; + newCurrency?: CurrencyCode; + };Also applies to: 120-123
packages/core/e2e/order.e2e-spec.ts (1)
609-615: Optional chaining onorder?.history.itemsdoesn’t guard againstorderbeing undefinedThe expression
expect(order?.history.items.map(pick(['type', 'data']))).toEqual([...]);still throws if
orderwere everundefined, becauseorder?.historywould yieldundefinedand.itemswould then be accessed onundefined. Since this test expects a real order, the non‑null assertion was safer and clearer.Suggest either reverting to the original non‑null form:
- expect(order?.history.items.map(pick(['type', 'data']))).toEqual([ + expect(order!.history.items.map(pick(['type', 'data']))).toEqual([or fully guarding the chain if you truly expect
orderto sometimes be missing.packages/core/src/service/services/order.service.ts (1)
3-12: Currency update flow is solid overall, but state validation is currently ignoredThe new currency‑change support is nicely integrated (channel validation, new
RequestContext, full repricing viaapplyPriceAdjustments, history entry, and event emission), but there are a couple of issues and small improvements:
assertAddingItemsState(order)is called inupdateOrderCurrency()but its return value is ignored. Everywhere else this helper is used, its result gates the operation (e.g.addItemsToOrder,removeItemsFromOrder). As written, currency changes will still proceed even if the order is not inAddingItemsorDraft, which is inconsistent with other “modification” operations.Suggest handling the validation result as you do elsewhere:
async updateOrderCurrency( ctx: RequestContext, orderId: ID, currencyCode: CurrencyCode, relations?: RelationPaths<Order>, ): Promise<ErrorResultUnion<UpdateOrderItemsResult, Order>> { const order = await this.getOrderOrThrow(ctx, orderId);// Check if order can be modifiedthis.assertAddingItemsState(order);
// Check if order can be modifiedconst validationError = this.assertAddingItemsState(order);if (validationError) {return validationError;}
For the history payload you currently use
previousCurrency: ctx.currencyCode. Usingorder.currencyCodebefore you mutate it is slightly more robust if a caller ever invokes this with a context whose currency differs from the order:const previousCurrencyCode = ctx.currencyCode;
const previousCurrencyCode = order.currencyCode;
Per the static analysis hint,
requestContextService(and other injected services) can be markedreadonly:private requestContextService: RequestContextService,
private readonly requestContextService: RequestContextService,These changes keep the new behavior consistent with existing order‑modification invariants and address the Sonar warning.
Also applies to: 47-50, 82-83, 119-120, 145-171, 589-641
🧹 Nitpick comments (4)
packages/core/e2e/graphql/generated-e2e-shop-types.ts (1)
3696-3733: Align newsetCurrencyCodeForOrderoperation types with existing generated patternsThese two aliases look hand-edited in an otherwise generated file and differ from nearby conventions:
- All other operation types use PascalCase (e.g.
AddItemToOrderMutationVariables,GetActiveOrderQueryVariables), whereas these are lower camelCase. Consider renaming toSetCurrencyCodeForOrderMutationVariables/SetCurrencyCodeForOrderMutationfor consistency.- Existing mutation result types model the GraphQL union as a discriminated union of concrete shapes (different field sets per member) and generally don’t expose
__typename. Here you collapse everything into a single object with many optional fields plus a__typenameunion, which weakens type‑safety in tests.- The
__typenameunion only lists'Order' | 'OrderModificationError' | 'InsufficientStockError', butUpdateOrderItemsResultelsewhere in this file also includes other members (e.g.OrderInterceptorError,OrderLimitError,NegativeQuantityError). If any of those can ever be returned bysetCurrencyCodeForOrder, they’ll be effectively untyped here.If possible, I’d recommend:
- Adding the
setCurrencyCodeForOrderGraphQL operation to whatever documents feed your codegen (e.g.shop-definitions.ts) and re‑running codegen so this file stays purely generated.- Letting codegen produce the usual discriminated union result type, or manually shaping this alias to mirror the union members you truly expect for this mutation and documenting that it’s a strict subset of
UpdateOrderItemsResult.packages/core/src/api/schema/shop-api/shop.api.graphql (1)
73-73: NewsetCurrencyCodeForOrdermutation shape looks consistent with existing APISignature and return type align with other order‑mutating operations (
UpdateOrderItemsResult!). Consider adding a short description above the field to clarify that it re-prices the active order in the new channel currency and is only valid when that currency is available on the current channel.packages/core/e2e/graphql/shop-definitions.ts (1)
133-148: ASSIGN_PRODUCT_VARIANTS_TO_CHANNEL mutation is fine; selection set could be trimmed if unusedThe mutation definition matches the schema (
AssignProductVariantsToChannelInput!) and returns enough fields to inspect assigned variants. If the test code never reads most of these fields (e.g.,name,sku,channels), you can safely trim the selection set to just what’s asserted to keep generated types slightly leaner, but it’s not required. Based on learnings, these gql constants are correctly used as inputs to codegen and should not be treated as dead code.packages/core/e2e/order.e2e-spec.ts (1)
97-97: Remove unusedSECOND_CHANNEL_TOKENand stale commentedchannelsconfig
SECOND_CHANNEL_TOKENis declared but never used, and the commented‑outchannelsarray ininitialDatais superseded by the runtimeUPDATE_CHANNELmutation in the new tests. Both add noise without value; consider deleting them to keep the fixture setup focused.Also applies to: 155-165
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/admin-ui/src/lib/core/src/common/generated-types.ts(1 hunks)packages/asset-server-plugin/e2e/graphql/generated-e2e-asset-server-plugin-types.ts(1 hunks)packages/common/src/generated-shop-types.ts(3 hunks)packages/common/src/generated-types.ts(1 hunks)packages/core/e2e/graphql/generated-e2e-admin-types.ts(1 hunks)packages/core/e2e/graphql/generated-e2e-shop-types.ts(1 hunks)packages/core/e2e/graphql/shop-definitions.ts(1 hunks)packages/core/e2e/order.e2e-spec.ts(5 hunks)packages/core/src/api/resolvers/shop/shop-order.resolver.ts(4 hunks)packages/core/src/api/schema/common/history-entry.type.graphql(1 hunks)packages/core/src/api/schema/shop-api/shop.api.graphql(1 hunks)packages/core/src/service/services/history.service.ts(2 hunks)packages/core/src/service/services/order.service.ts(4 hunks)packages/elasticsearch-plugin/e2e/graphql/generated-e2e-elasticsearch-plugin-types.ts(1 hunks)packages/payments-plugin/e2e/graphql/generated-admin-types.ts(1 hunks)packages/payments-plugin/e2e/graphql/generated-shop-types.ts(3 hunks)packages/payments-plugin/src/mollie/graphql/generated-shop-types.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-21T18:29:30.413Z
Learnt from: DanielBiegler
Repo: vendure-ecommerce/vendure PR: 3815
File: packages/core/e2e/api-key.e2e-spec.ts:160-224
Timestamp: 2025-11-21T18:29:30.413Z
Learning: In the vendure-ecommerce/vendure repository, hand-written `gql` template literals (e.g., `CREATE_API_KEY`, `API_KEY`, etc.) in test files like `packages/core/e2e/api-key.e2e-spec.ts` are the source documents for GraphQL code generation. The code generator processes these to produce typed `*Document` exports (e.g., `CreateApiKeyDocument`, `ApiKeyDocument`) that are then imported and used in the tests. These hand-written gql constants should not be considered unused or dead code—they are essential inputs to the code generation process.
Applied to files:
packages/core/e2e/graphql/shop-definitions.ts
📚 Learning: 2025-11-26T14:08:43.342Z
Learnt from: PavanendraBaahubali
Repo: vendure-ecommerce/vendure PR: 3999
File: packages/dashboard/src/app/routes/_authenticated/_products/components/create-product-variants-dialog.tsx:133-138
Timestamp: 2025-11-26T14:08:43.342Z
Learning: The Vendure dashboard codebase uses awesome-graphql-client (not Apollo Client) for GraphQL operations. The api wrapper is located at `packages/dashboard/src/vdb/graphql/api.ts`. When mutations throw errors, the error object contains a `fieldErrors` property (an array of GraphQL error objects), not `graphQLErrors`. Access error messages via `error.fieldErrors[0].message`.
Applied to files:
packages/core/src/api/resolvers/shop/shop-order.resolver.ts
🧬 Code graph analysis (5)
packages/common/src/generated-shop-types.ts (1)
packages/common/src/generated-types.ts (1)
UpdateOrderItemsResult(6739-6739)
packages/core/e2e/order.e2e-spec.ts (2)
packages/common/src/generated-types.ts (1)
AssignProductVariantsToChannelInput(270-274)packages/core/e2e/graphql/shop-definitions.ts (2)
ASSIGN_PRODUCT_VARIANTS_TO_CHANNEL(133-148)SET_CURRENCY_CODE_FOR_ORDER(166-185)
packages/core/src/service/services/history.service.ts (1)
packages/common/src/shared-types.ts (1)
ID(79-79)
packages/core/e2e/graphql/generated-e2e-shop-types.ts (1)
packages/common/src/generated-types.ts (1)
Exact(4-4)
packages/core/src/api/resolvers/shop/shop-order.resolver.ts (2)
packages/core/e2e/graphql/generated-e2e-shop-types.ts (2)
Order(1989-2044)Mutation(1703-1804)packages/core/src/config/order/active-order-strategy.ts (1)
ACTIVE_ORDER_INPUT_FIELD_NAME(7-7)
🪛 GitHub Check: SonarCloud Code Analysis
packages/core/src/service/services/order.service.ts
[warning] 170-170: Member 'requestContextService: RequestContextService' is never reassigned; mark it as readonly.
🔇 Additional comments (18)
packages/core/src/api/schema/common/history-entry.type.graphql (1)
9-36: NewORDER_CURRENCY_UPDATEDhistory type looks consistent; ensure downstream mappings are updatedEnum naming/placement align with existing
ORDER_*history types, so the schema change itself is fine. Please double-check any switch/if mappings (e.g. admin UI labels, icons, filters, history serializers) that branch onHistoryEntryTypeso this new value doesn’t fall through to an “unknown”/default case at runtime.packages/payments-plugin/e2e/graphql/generated-admin-types.ts (1)
2007-2034: GeneratedHistoryEntryType.ORDER_CURRENCY_UPDATEDenum entry matches schemaThe added enum member is correctly named and consistent with the schema change; for a generated file this is exactly what we want. Just make sure all other generated type bundles (admin/shop, plugins, e2e fixtures) were regenerated from the same updated schema so tests and tooling stay in sync.
packages/common/src/generated-types.ts (1)
2059-2067: NewORDER_CURRENCY_UPDATEDhistory type looks consistentThe new
ORDER_CURRENCY_UPDATEDentry fits the existingORDER_*naming pattern and enum grouping; no issues here. Just ensure any exhaustive handling ofHistoryEntryType(e.g. switch statements, filters) in non‑generated code has been updated to include this new value.packages/asset-server-plugin/e2e/graphql/generated-e2e-asset-server-plugin-types.ts (1)
2020-2020: LGTM! Generated enum member for currency update tracking.The addition of
ORDER_CURRENCY_UPDATEDto theHistoryEntryTypeenum correctly reflects the new history entry type for tracking order currency changes. The generated code follows the established naming convention and integrates properly with the existing enum members.packages/elasticsearch-plugin/e2e/graphql/generated-e2e-elasticsearch-plugin-types.ts (1)
2002-2029:HistoryEntryType.ORDER_CURRENCY_UPDATEDaddition is consistentThe new enum member is named and valued consistently with the other
ORDER_*history types and is a safe, backward-compatible extension of the union. No issues from this file’s perspective.packages/payments-plugin/src/mollie/graphql/generated-shop-types.ts (3)
1262-1289: HistoryEntryType addition is consistent
ORDER_CURRENCY_UPDATEDfollows existing naming and string-literal conventions and cleanly extends the history enum set.
1845-1948: New mutation field wiring looks correct
setCurrencyCodeForOrder: UpdateOrderItemsResultaligns with other order-edit mutations and uses the same union result type, which should keep client handling uniform.
2025-2027: Args type matches schema expectations
MutationSetCurrencyCodeForOrderArgswithcurrencyCode: CurrencyCodematches the core arg shape and enum usage; no issues spotted.packages/common/src/generated-shop-types.ts (3)
1258-1285: HistoryEntryType extension is well-formed
ORDER_CURRENCY_UPDATEDintegrates cleanly into the existing order-related history enum values; no typing or naming concerns.
1763-1866: setCurrencyCodeForOrder mutation field is consistentThe new
setCurrencyCodeForOrder: UpdateOrderItemsResultmirrors other order mutations’ result type and fits the existing Mutation API surface.
1939-1942: Args type matches core definition
MutationSetCurrencyCodeForOrderArgswithcurrencyCode: CurrencyCodematches the corepackages/common/src/generated-types.tsdefinition, so shared consumers should remain in sync.packages/payments-plugin/e2e/graphql/generated-shop-types.ts (2)
1215-1242: Enum extension forORDER_CURRENCY_UPDATEDlooks consistentAdding
ORDER_CURRENCY_UPDATEDtoHistoryEntryTypeis an additive, backwards-compatible change and fits the existing naming pattern for order-related history events. No issues from the typings side.
1780-1883: NewsetCurrencyCodeForOrdermutation and args type align with existing order APIsThe
setCurrencyCodeForOrder: UpdateOrderItemsResultfield andMutationSetCurrencyCodeForOrderArgs { currencyCode: CurrencyCode }mirror the existing order mutations’ patterns (single-arg input, shared union return type). The typings here are coherent and should integrate cleanly with the rest of the generated schema.Also applies to: 1960-1962
packages/core/e2e/graphql/generated-e2e-admin-types.ts (1)
2017-2023: LGTM!The new
ORDER_CURRENCY_UPDATEDenum member is correctly positioned in alphabetical order and follows the existing naming convention for history entry types. As this is a generated file, ensure it was regenerated from the GraphQL schema rather than manually edited.packages/admin-ui/src/lib/core/src/common/generated-types.ts (1)
2068-2074: LGTM!The new
ORDER_CURRENCY_UPDATEDenum member follows the established naming conventions and is correctly positioned in alphabetical order. This aligns with the new currency update mutation feature.As this is a generated file, ensure it was regenerated from the source GraphQL schema (
history-entry.type.graphql) rather than manually edited.packages/core/e2e/graphql/shop-definitions.ts (2)
156-160: Formatting‑only change inADD_MULTIPLE_ITEMS_TO_ORDERThis hunk only adjusts indentation around the
... on ErrorResultfragment and has no behavioral impact; nothing to change here.
166-185: SET_CURRENCY_CODE_FOR_ORDER operation matches the new mutation and union contractThe
ChangeOrderCurrencymutation correctly callssetCurrencyCodeForOrderand handles theUpdateOrderItemsResultunion via__typename,... on Order { ...UpdatedOrder }, and explicit error fragments. This is in line with the other order‑item mutations and will generate the expected operation types for the e2e clients.packages/core/src/api/resolvers/shop/shop-order.resolver.ts (1)
12-18:setCurrencyCodeForOrderresolver is wired correctly; verify if type cast is unnecessaryThe new mutation:
- Uses the same active‑order lookup pattern as
addItemToOrder/adjustOrderLine.- Is guarded by
@Allow(Permission.UpdateOrder, Permission.Owner)and wrapped in@Transaction().- Delegates cleanly to
orderService.setCurrencyCodeForOrderwith the provided relations.The
as CurrencyCodecast onargs.currencyCodeis likely redundant since GraphQL code generation should already typeMutationSetCurrencyCodeForOrderArgs['currencyCode']asCurrencyCode. Confirm the generated type shape and remove the cast if the field is already properly typed.Also applies to: 31-31, 356-376, 378-398
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/service/services/order.service.ts (3)
589-598: Clarify the need for this wrapper method.
setCurrencyCodeForOrderis a direct pass-through toupdateOrderCurrencywith no additional logic. Having two public methods that perform identical operations creates API confusion and maintenance overhead.Consider one of the following:
- Option 1 (Recommended): Remove
setCurrencyCodeForOrderand useupdateOrderCurrencydirectly in resolvers.- Option 2: If
setCurrencyCodeForOrderis the intended public API, makeupdateOrderCurrencyprivate and rename it (e.g.,applyOrderCurrencyUpdate).- Option 3: Document why both methods exist if there's a specific architectural reason (e.g., one for shop API, one for admin API).
Apply this diff if choosing Option 1:
- async setCurrencyCodeForOrder( - ctx: RequestContext, - orderId: ID, - currencyCode: CurrencyCode, - relations?: RelationPaths<Order>, - ): Promise<ErrorResultUnion<UpdateOrderItemsResult, Order>> { - const result = await this.updateOrderCurrency(ctx, orderId, currencyCode, relations); - - return result; - } -
617-621: Optimize channel lookup and standardize error handling.Two concerns here:
Performance: Line 618 fetches the channel from the database, but
ctx.channelshould already be available. IfavailableCurrencyCodesis not eagerly loaded, consider optimizing the channel service to include it or checking if it's already present.Error handling inconsistency: Line 620 throws
UserInputErrorinstead of returning a typed GraphQL error result. This deviates from the pattern used elsewhere in this service (e.g.,OrderModificationErroron line 609-611).Apply this diff to return a typed error:
+import { ..., CurrencyNotAvailableError } from '../../common/error/generated-graphql-shop-errors'; // Verify the new currency is available in the channel const channel = await this.channelService.getChannelFromToken(ctx.channel.token); if (!channel.availableCurrencyCodes.includes(currencyCode)) { - throw new UserInputError('error.currency-not-available', { currencyCode }); + return new CurrencyNotAvailableError({ currencyCode }); }Note: You may need to define
CurrencyNotAvailableErrorif it doesn't already exist in the generated shop errors.
600-647: Add JSDoc documentation for the public API method.Public methods in this service are typically documented with JSDoc comments describing their purpose, parameters, and behavior. This helps API consumers understand when and how to use the method.
Add documentation like:
+ /** + * @description + * Updates the currency code for an Order and recalculates all prices. + * This operation is only allowed when the Order is in the 'AddingItems' or 'Draft' state. + * The new currency must be available in the current Channel. + * + * @param ctx - The RequestContext + * @param orderId - The ID of the Order to update + * @param currencyCode - The new currency code to apply + * @param relations - Optional relations to load on the returned Order + * @returns The updated Order or an error result if validation fails + */ async updateOrderCurrency( ctx: RequestContext, orderId: ID, currencyCode: CurrencyCode, relations?: RelationPaths<Order>, ): Promise<ErrorResultUnion<UpdateOrderItemsResult, Order>> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/e2e/graphql/generated-e2e-shop-types.ts(5 hunks)packages/core/e2e/graphql/shop-definitions.ts(1 hunks)packages/core/e2e/order.e2e-spec.ts(7 hunks)packages/core/src/api/schema/shop-api/shop.api.graphql(1 hunks)packages/core/src/service/services/history.service.ts(2 hunks)packages/core/src/service/services/order.service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/api/schema/shop-api/shop.api.graphql
- packages/core/e2e/graphql/shop-definitions.ts
- packages/core/src/service/services/history.service.ts
- packages/core/e2e/order.e2e-spec.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/service/services/order.service.ts (3)
packages/core/src/api/common/request-context.ts (1)
RequestContext(179-447)packages/common/src/shared-types.ts (1)
ID(79-79)packages/core/src/api/resolvers/entity/channel-entity.resolver.ts (1)
currencyCode(21-23)
packages/core/e2e/graphql/generated-e2e-shop-types.ts (1)
packages/common/src/generated-shop-types.ts (2)
UpdateOrderItemsResult(3566-3572)MutationSetCurrencyCodeForOrderArgs(1939-1941)
🔇 Additional comments (6)
packages/core/src/service/services/order.service.ts (1)
600-647: The existingassertAddingItemsStatecheck already prevents currency changes on orders with payments. Orders inAddingItemsorDraftstates cannot have payments becausecanAddPaymentToOrderonly allows payments onArrangingPaymentstate or states that can transition toPaymentAuthorized/PaymentSettled. SinceAddingItemsmust transition toArrangingPaymentbefore reaching payment states, no explicit payment check is needed.Likely an incorrect or invalid review comment.
packages/core/e2e/graphql/generated-e2e-shop-types.ts (5)
1211-1238: HistoryEntryType: ORDER_CURRENCY_UPDATED addition looks consistent.
Just ensure the server schema and all generated type packages include the same enum value to avoid runtime GraphQL validation errors in e2e.
1704-1806: Mutation.setCurrencyCodeForOrder added appropriately in Shop e2e types.
Return typeUpdateOrderItemsResultmatches the “order mutation-style” unions used elsewhere in this file.
1879-1882: Args shape for setCurrencyCodeForOrder is correct (required currencyCode).
Matches the expected GraphQL signature pattern (field args modeled via a dedicatedMutation*Argstype).
3702-3783: SetCurrencyCodeForOrderMutation result union is coherent with existing UpdateOrderItemsResult patterns.
IncludingOrderInterceptorError+ genericErrorResultfragments matches how other mutations are modeled here.
6080-6259: SetCurrencyCodeForOrderDocument: variable + selections look correct and consistent with other documents.
Note: the repeatedUpdatedOrderfragment definition inside the document is typical of codegen output—avoid manual edits; rely on regeneration.
packages/core/e2e/order.e2e-spec.ts
Outdated
| // channels: [ | ||
| // { | ||
| // token: 'default', | ||
| // defaultLanguageCode: LanguageCode.en, | ||
| // // Set the base currency () | ||
| // currencyCode: CurrencyCode.USD, | ||
| // availableLanguageCodes: [LanguageCode.en], | ||
| // availableCurrencyCodes: [CurrencyCode.USD, CurrencyCode.EGP], | ||
| // }, | ||
| // ], |
There was a problem hiding this comment.
lil leftover thingy? :)
| const order = await this.activeOrderService.getActiveOrder( | ||
| ctx, | ||
| args[ACTIVE_ORDER_INPUT_FIELD_NAME], | ||
| true, | ||
| ); | ||
| return this.orderService.setCurrencyCodeForOrder( | ||
| ctx, | ||
| order.id, | ||
| args.currencyCode as CurrencyCode, | ||
| relations, | ||
| ); |
There was a problem hiding this comment.
I'm aware that you're following the current pattern of passing in the orderId instead of the order object because thats how the other methods work, and it's good to follow conventions, but I want to pose a general question to the vendure team here that we could get rid of database queries by replacing the IDs with a real order objects. Food for thought.
There was a problem hiding this comment.
Can you explain how the order object will be represented without hitting the DB? Does it exist in the context?
There was a problem hiding this comment.
Can you explain how the order object will be represented without hitting the DB?
Sorry I dont mean to get rid off all the requests, just some. The .getActiveOrder function may hit the DB but then we only pass the ID to .setCurrencyCodeForOrder which fetches the order again. This holds true for other methods which also re-fetch orders. We could change function signatures to accept the order object directly, getting rid off this re-fetching.
Like I said, its correct of you to follow this convention, I'm posing this new shift as a general question, no worries
And will this postpone this PR?
No probably not
There was a problem hiding this comment.
@DanielBiegler it's a good point and one we can revisit in some future performance work I think.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/e2e/order.e2e-spec.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T18:29:30.413Z
Learnt from: DanielBiegler
Repo: vendure-ecommerce/vendure PR: 3815
File: packages/core/e2e/api-key.e2e-spec.ts:160-224
Timestamp: 2025-11-21T18:29:30.413Z
Learning: In the vendure-ecommerce/vendure repository, hand-written `gql` template literals (e.g., `CREATE_API_KEY`, `API_KEY`, etc.) in test files like `packages/core/e2e/api-key.e2e-spec.ts` are the source documents for GraphQL code generation. The code generator processes these to produce typed `*Document` exports (e.g., `CreateApiKeyDocument`, `ApiKeyDocument`) that are then imported and used in the tests. These hand-written gql constants should not be considered unused or dead code—they are essential inputs to the code generation process.
Applied to files:
packages/core/e2e/order.e2e-spec.ts
🧬 Code graph analysis (1)
packages/core/e2e/order.e2e-spec.ts (6)
packages/core/e2e/graphql/generated-e2e-admin-types.ts (1)
AssignProductVariantsToChannelInput(277-281)packages/common/src/generated-types.ts (1)
AssignProductVariantsToChannelInput(270-274)packages/core/e2e/graphql/shared-definitions.ts (1)
ASSIGN_PRODUCTVARIANT_TO_CHANNEL(412-419)packages/core/e2e/graphql/generated-e2e-shop-types.ts (2)
SetCurrencyCodeForOrderMutation(3706-3783)SetCurrencyCodeForOrderMutationVariables(3702-3704)packages/core/e2e/graphql/shop-definitions.ts (2)
SET_CURRENCY_CODE_FOR_ORDER(150-170)ADD_MULTIPLE_ITEMS_TO_ORDER(133-148)packages/core/src/service/services/order.service.ts (2)
addItemsToOrder(660-772)setCurrencyCodeForOrder(589-598)
🔇 Additional comments (5)
packages/core/e2e/order.e2e-spec.ts (5)
5-5: LGTM: Imports are correctly added for the new currency test suite.The new imports (
CurrencyCode,ASSIGN_PRODUCTVARIANT_TO_CHANNEL,UPDATE_CHANNEL,SET_CURRENCY_CODE_FOR_ORDER) are all utilized in the test suite added below and sourced from the appropriate modules.Also applies to: 62-62, 77-77, 89-89
601-601: LGTM: Non-null assertion is appropriate here.The non-null assertion on
order!is consistent with patterns used throughout this test file and is safe in this controlled test context.
2943-2951: LGTM: Good negative test case.This test properly verifies that attempting to set a currency not available in the channel results in an error. The use of
rejects.toThrowis appropriate for testing async errors.
2953-2980: LGTM: Well-structured positive test case.This test properly validates the happy path:
- Creates an order with items in the default currency (USD)
- Verifies the initial currency
- Updates the currency to EGP
- Confirms the currency change and order integrity
The type assertions and test structure are consistent with patterns used throughout this file.
2908-2981: Well-structured test suite for currency update feature.The test suite provides appropriate coverage with both error and success scenarios. The structure follows established patterns in this test file, and the use of
beforeAllto set up channel currencies and product variants is consistent with the approach used throughout.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/e2e/order.e2e-spec.ts (1)
2908-2981:update order currencytests look solid; a couple of small cleanupsThe setup and both test cases correctly exercise the new mutation (invalid currency vs successful change) and use the generated types as expected. A few minor polish points you might consider:
- The comment above the
ASSIGN_PRODUCTVARIANT_TO_CHANNELcall says// Add 1st variant of product 1 to the channel, but the code assigns all variants:product1.variants.map(v => v.id). Either change the comment to match the behavior or, if you really only care about the first variant, restrict the list to[product1.variants[0].id].- For consistency with the rest of the file, you could use the generated variables type instead of an inline object type in the
AssignProductVariantsToChannelcall, e.g.:ifawait adminClient.query< Codegen.AssignProductVariantsToChannelMutation, Codegen.AssignProductVariantsToChannelMutationVariables >(ASSIGN_PRODUCTVARIANT_TO_CHANNEL, { input: { channelId: 'T_1', productVariantIds: product1.variants.map(v => v.id), priceFactor: 1, }, });AssignProductVariantsToChannelMutationVariablesis available from codegen.These are all non-blocking—tests are clear and should behave correctly as written.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/e2e/order.e2e-spec.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T18:29:30.413Z
Learnt from: DanielBiegler
Repo: vendure-ecommerce/vendure PR: 3815
File: packages/core/e2e/api-key.e2e-spec.ts:160-224
Timestamp: 2025-11-21T18:29:30.413Z
Learning: In the vendure-ecommerce/vendure repository, hand-written `gql` template literals (e.g., `CREATE_API_KEY`, `API_KEY`, etc.) in test files like `packages/core/e2e/api-key.e2e-spec.ts` are the source documents for GraphQL code generation. The code generator processes these to produce typed `*Document` exports (e.g., `CreateApiKeyDocument`, `ApiKeyDocument`) that are then imported and used in the tests. These hand-written gql constants should not be considered unused or dead code—they are essential inputs to the code generation process.
Applied to files:
packages/core/e2e/order.e2e-spec.ts
🔇 Additional comments (2)
packages/core/e2e/order.e2e-spec.ts (2)
4-10: New currency & GraphQL imports are consistent and well-scopedThe added
CurrencyCodeimport and the new document imports (ASSIGN_PRODUCTVARIANT_TO_CHANNEL,UPDATE_CHANNEL,SET_CURRENCY_CODE_FOR_ORDER) line up cleanly with their usages in the new tests and follow the existing typing patterns in this file. No issues from my side here.Also applies to: 62-79, 81-92
596-647: History expectation remains precise and readableThe updated assertion on
order!.history.items.map(pick(['type', 'data']))still fully specifies the expected sequence of state and payment transitions for the payment flow onT_2. This keeps the test robust against unintended history changes and matches the surrounding style.
michaelbromley
left a comment
michaelbromley
left a comment
There was a problem hiding this comment.
Thanks for this!
I left one bit of feedback about the service, and more generally since this is a new feature it should be made against the minor branch.
The minor branch also features a re-worked way of handling the codegen for the e2e tests. We are now using a library called gql.tada. Check the other examples to see how it works.
setCurrencyCodeForOrder was a no-op wrapper; this simplifies the service by removing it.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/service/services/order.service.ts (1)
590-637: LGTM: Currency update implementation is robust and well-structured.The
updateOrderCurrencymethod correctly handles the currency change workflow:
- Validates order modification state and currency availability
- Creates a properly scoped
RequestContextwith the new currency for accurate price recalculation- Records history entry for audit trail
- Applies price adjustments to all order lines
- All operations are transactional (via the
@Transactiondecorator in the resolver)The implementation follows established patterns and handles edge cases appropriately.
💡 Optional optimization consideration
At line 608, the channel is fetched from the database to access
availableCurrencyCodes:const channel = await this.channelService.getChannelFromToken(ctx.channel.token);If
ctx.channelalready has theavailableCurrencyCodesproperty loaded, this fetch could be avoided. However, fetching ensures fresh data and is the safer approach. This is a minor optimization opportunity that could be explored if this code path becomes a performance bottleneck.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/api/resolvers/shop/shop-order.resolver.tspackages/core/src/service/services/order.service.ts
🔇 Additional comments (4)
packages/core/src/api/resolvers/shop/shop-order.resolver.ts (2)
12-12: LGTM: Import added for new mutation.The
MutationSetCurrencyCodeForOrderArgsimport is correctly added to support the newsetCurrencyCodeForOrdermutation method.
378-393: LGTM: New mutation follows established patterns correctly.The
setCurrencyCodeForOrdermutation is well-implemented:
- Appropriate permissions (
UpdateOrderandOwner)- Consistent with existing mutation patterns in this resolver
- Proper active order retrieval with strict mode
- Clean delegation to service layer
packages/core/src/service/services/order.service.ts (2)
5-5: LGTM: Imports added to support currency update functionality.The
CurrencyCodetype andRequestContextServiceimports are correctly added for the newupdateOrderCurrencymethod.Also applies to: 119-119
170-170: LGTM: RequestContextService properly injected.The service is correctly injected to support creating currency-aware request contexts.
|
@michaelbromley Regarding the e2e test and codegen replacement, could you please clarify what examples you are referring to? And does this require a change in the PR? |
|
@aelhor yes, please rebase to minor and apply the new way how GraphQL codegen works there. Thank you! |
4 participants
Description
Added new mutation to update order currency and trigger price recalculations
Added e2e tests to cover the new added mutation
Fixes issue #3897
Breaking changes
No
Screenshots
You can add screenshots here if applicable.
Checklist
📌 Always:
👍 Most of the time:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.