Feat/medusajs integration for checkout process#679
Conversation
- Introduced cart-related modules to Medusa.js integration with support for create, update, and delete operations. - Included mappings and services for handling carts across integrations. - Updated relevant configurations, docs, and seed data.
- Added models, services, and mappers for customers, payments, and checkout. - Enhanced carts and orders to support additional fields like email and payment session. - Integrated mocked module functionality for new services. - Refined Medusa.js mappings for customer addresses and shipping options.
- Renamed methods, parameters, and models for clarity and consistency (e.g., `setupAddresses` to `setAddresses`). - Centralized support for guest email handling using `email` field across carts, orders, and checkout processes. - Removed redundant calls and optimized order completion workflow.
- Added locale-based grouping for payment providers. - Introduced utility to retrieve localized payment method display info. - Updated cart and checkout services to support locale-based operations. - Enhanced cart item handling with improved product mapping and variant support. - Addressed stricter authentication for customer cart access and actions.
…oducts, customers, and orders
…d field mappings - Added documentation for Carts, Customers, and Checkout services, including API methods and data models. - Updated Products module to reference associated entities (e.g., Carts and Orders). - Enhanced Tickets API docs with clarified field mapping and integration-specific requirements. - Improved docs with consistent formatting.
|
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:
WalkthroughAdds Carts, Customers, Payments, and Checkout domains across framework, configs, Medusa.js and mocked integrations, including models, DTOs, controllers, abstract service contracts, concrete Medusa + mocked implementations, mappers, tests, app wiring, and extensive documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Harmonization API
participant Checkout as Checkout.Service
participant Carts as Carts.Service
participant Payments as Payments.Service
participant Customers as Customers.Service
rect rgba(200,230,255,0.5)
Client->>API: POST /checkout/:cartId/addresses (body, auth)
API->>Checkout: setAddresses(params, body, auth)
Checkout->>Carts: updateCartAddresses(params, body, auth)
Carts-->>Checkout: updated Cart
Checkout->>Payments: (optional) create payment session / update cart metadata
Checkout->>Customers: (optional) resolve saved addresses by id
Checkout-->>API: CheckoutSummary / Cart
API-->>Client: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/integrations/mocked/prisma/seed.ts (1)
9-18:⚠️ Potential issue | 🟡 MinorStale comment references old customer ID.
Line 9 still references
cust-001for Acme Corporation, but the actual value on line 18 is nowcus_01KH3J08TY40PYGVEG3A04CP8R. Update the comment to stay consistent with the code.Also,
cust-002andcust-003(lines 25, 32) still use the old ID format. If these haven't been migrated intentionally, that's fine — but if the plan is to migrate all customer IDs, this is incomplete.Suggested comment fix
// Customer permissions defined in customers.mapper.ts: - // - cust-001 (Acme Corporation): ADMIN_PERMISSIONS (full access) + // - cus_01KH3J08TY40PYGVEG3A04CP8R (Acme Corporation): ADMIN_PERMISSIONS (full access) // - cust-002 (Tech Solutions Inc): USER_PERMISSIONS (view + pay) // - cust-003 (Digital Services GmbH): READONLY_PERMISSIONS (view only)packages/integrations/mocked/src/modules/auth/auth.service.ts (1)
56-56:⚠️ Potential issue | 🟡 MinorInconsistent optional chaining — potential NPE on
decodedToken.permissions.Line 48 correctly uses
decodedToken?.rolesas a fallback, but line 56 usesdecodedToken.permissions(no?.). IfdecodedTokenisundefined(e.g., malformed token), this will throw aTypeError.Suggested fix
- return decodedToken?.customer?.permissions || decodedToken.permissions || {}; + return decodedToken?.customer?.permissions || decodedToken?.permissions || {};packages/integrations/mocked/src/modules/users/customers.mapper.ts (1)
34-51:⚠️ Potential issue | 🟡 MinorInconsistent customer ID format across mock customers.
MOCK_CUSTOMER_1now uses a Medusa-style ID (cus_01KH3J08TY40PYGVEG3A04CP8R), whileMOCK_CUSTOMER_2andMOCK_CUSTOMER_3retain the oldcust-002/cust-003format. If downstream code or tests depend on a consistent format, this could cause issues. Consider aligning all mock IDs to the same convention.Also applies to: 53-70, 72-89
packages/integrations/medusajs/src/modules/orders/orders.mapper.ts (1)
41-52:⚠️ Potential issue | 🟠 MajorUse
item.product_idforproductIdinstead ofitem.variant_id.Line 44 maps
productIdfromitem.variant_id, but it should useitem.product_id(available at line 62 inmapProduct). The field name indicates a product identifier, not a variant identifier, and this inconsistency causes the OrderItem to reference the variant ID when it should reference the product ID.Current code
const mapOrderItem = (item: HttpTypes.StoreOrderLineItem, currency: string): Orders.Model.OrderItem => { return { id: item.id, productId: item.variant_id || '', // ← should be item.product_id quantity: item.quantity, price: mapPrice(item.unit_price, currency) as Models.Price.Price, total: mapPrice(item.total, currency), subtotal: mapPrice(item.subtotal, currency), currency: currency as Models.Price.Currency, product: mapProduct(item.unit_price, currency, item) as Products.Model.Product, }; };packages/integrations/medusajs/src/modules/orders/orders.service.ts (2)
29-31:⚠️ Potential issue | 🟡 MinorDuplicate
+tax_totalinadditionalOrderListFields.The field string contains
+tax_totaltwice. This won't cause an error but is redundant.🔧 Proposed fix
private readonly additionalOrderListFields = - '+total,+subtotal,+tax_total,+discount_total,+shipping_total,+shipping_subtotal,+tax_total,+items.product.*'; + '+total,+subtotal,+tax_total,+discount_total,+shipping_total,+shipping_subtotal,+items.product.*';
37-37:⚠️ Potential issue | 🟡 MinorRemove unused
authServicedependency.The
Auth.Serviceis injected on line 37 but never referenced in the file. After migrating to the Medusa Store API, which automatically scopes orders to the authenticated customer, this dependency is no longer needed and should be removed from the constructor.
🤖 Fix all issues with AI agents
In `@packages/framework/src/modules/carts/carts.service.ts`:
- Line 3: Replace the circular barrel import "import * as Carts from './';" with
direct imports for the types used: "import * as Model from './carts.model';" and
"import * as Request from './carts.request';", then update all occurrences of
Carts.Model.* to Model.* and Carts.Request.* to Request.* in this file (e.g.,
references in type annotations, function params, and return types); this removes
the circular dependency caused by the index re-export of CartService (exported
as Service) while keeping all type usages intact.
In `@packages/framework/src/modules/checkout/checkout.controller.ts`:
- Around line 9-12: CheckoutController methods are missing the project's auth
decorator; add `@Auth.Decorators.Roles`({ roles: [] }) to every public endpoint
method in the CheckoutController class (e.g., the methods handling "place-order"
and "complete" and any other checkout operations) so they enforce authentication
like UserController; if guest checkout is allowed, instead apply a distinct auth
pattern (e.g., a separate decorator or conditional guard) only to the
guest-specific methods and keep secure endpoints using Roles on the
corresponding methods.
In `@packages/integrations/medusajs/src/integration.ts`:
- Around line 42-46: The payments integration is missing the Auth.Module in its
imports which PaymentsService requires for validating SSO tokens; update the
payments entry (symbol: payments) so its imports include Auth.Module alongside
MedusaJsModule (i.e., change imports: [MedusaJsModule] to imports:
[MedusaJsModule, Auth.Module]) to match the other integrations and satisfy
PaymentsService's auth dependency.
In `@packages/integrations/medusajs/src/modules/carts/carts.mapper.ts`:
- Around line 15-19: The mapper mapCart is casting Medusa's lowercase
cart.currency_code directly to Models.Price.Currency which allows invalid
lowercase values; change mapCart to normalize currency by calling
cart.currency_code.toUpperCase() and then cast to Models.Price.Currency (i.e.,
const currency = cart.currency_code.toUpperCase() as Models.Price.Currency) so
all price objects use the uppercase enum, and update the test in
carts.service.spec.ts to expect 'EUR' instead of 'eur'; mirror the pattern used
in checkout.mapper's currency handling for consistency.
In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts`:
- Around line 187-209: The authenticated branch for customerId calls
createCartAndAddItem without the authorization token, so Medusa can't associate
the cart with the customer; update the customerId branch to pass the
authorization argument (same position as in the guest call) to
createCartAndAddItem (refer to the createCartAndAddItem invocation in the
customerId conditional) so both authenticated and guest flows forward the auth
token correctly.
- Around line 252-270: The applyPromotion method currently calls
this.sdk.store.cart.update with promo_codes (in applyPromotion) which overwrites
existing codes; change it to call the dedicated store cart promotions endpoint
to add a code (POST /store/carts/{id}/promotions) instead of cart.update so it
appends rather than replaces; update the call in applyPromotion to use the SDK
method that executes the store cart promotions POST with params.cartId and
payload containing the code, passing
this.medusaJsService.getStoreApiHeaders(authorization), and preserve the same
response mapping (mapCart) and error handling (handleHttpError).
In `@packages/integrations/medusajs/src/modules/checkout/checkout.service.ts`:
- Around line 96-101: The object in checkout.service.ts currently uses hardcoded
returnUrl/cancelUrl which won't work in production; update the code that builds
the checkout payload (the object with cartId, providerId, metadata) to read
returnUrl and cancelUrl from the incoming request data (e.g., data.returnUrl,
data.cancelUrl) or from a config/provider (env/config service) if absent, and
validate they exist (throw or return a clear error) before sending to the
checkout provider; also mirror the same change in the mocked checkout service so
both implementations accept and propagate dynamic URLs rather than the
example.com placeholders.
- Around line 244-246: The ternary assigning const headers in
checkout.service.ts is redundant (both branches call
this.medusaJsService.getStoreApiHeaders(authorization)); remove the dead ternary
and call getStoreApiHeaders once; either use
this.medusaJsService.getStoreApiHeaders(authorization) if passing undefined is
acceptable, or call this.medusaJsService.getStoreApiHeaders() in the else-case
if you intend unauthenticated headers—update the single assignment accordingly
and keep getStoreApiHeaders, authorization references intact.
In `@packages/integrations/medusajs/src/modules/medusajs/medusajs.service.ts`:
- Around line 68-77: The Medusa client is instantiated with a hardcoded debug:
true in the Medusa constructor (this._sdk) which was left from development;
replace the hardcoded value with the intended conditional (use this.logLevel ===
'debug' or equivalent configuration flag) so debug is enabled only in debug
mode. Locate the Medusa initialization in medusajs.service.ts (the this._sdk =
new Medusa({...}) block) and change the debug property to a boolean expression
that checks the service's log level or environment setting (e.g., this.logLevel
=== 'debug') so production does not get verbose logging.
- Around line 126-132: getStoreApiHeaders currently omits the required Medusa
header causing raw HTTP calls to fail; update getStoreApiHeaders to always
include the Medusa publishable key by adding 'x-publishable-api-key' with
this.getPublishableKey() to the returned headers, and preserve the optional
Authorization header if provided (used by payments.service.ts calls). Locate
getStoreApiHeaders in medusajs.service.ts and change its headers initialization
to include the publishable key before returning.
In
`@packages/integrations/medusajs/src/modules/payments/payments.service.spec.ts`:
- Around line 213-218: Update the production code to stop calling .toPromise()
on HttpClient Observables: replace patterns like
from(this.httpClient.post(...).toPromise()).pipe(...) with
this.httpClient.post(...).pipe(...) (or, if a Promise is truly required, wrap
lastValueFrom(this.httpClient.post(...)) instead). Then update the tests in
payments.service.spec.ts where mockHttpService.post.mockReturnValue(...) returns
an object with toPromise — replace those mocks (the one returning { toPromise:
() => Promise.resolve({ data: { payment_session: ... } }) } and the other
similar instances) to return proper Observables using rxjs (e.g., of({ data: {
payment_session: ... } })) so they match the new Observable-based production
calls. Ensure all four test mocks referenced are changed consistently.
In `@packages/integrations/medusajs/src/modules/payments/payments.service.ts`:
- Around line 192-208: The cancelSession method is using the deprecated
.toPromise() on the Observable returned by this.httpClient.delete; replace the
.toPromise() usage with firstValueFrom to await the HTTP Observable instead.
Update the code in cancelSession (the call that wraps
this.httpClient.delete(...).toPromise()) to use
firstValueFrom(this.httpClient.delete(...)) and ensure firstValueFrom is
imported from 'rxjs', leaving the existing map/ catchError logic (including
NotFoundException and handleHttpError) unchanged so behavior and error handling
for medusaJsService.getBaseUrl()/getStoreApiHeaders remain intact.
- Around line 154-180: The current updateSession implementation incorrectly
converts the HTTP observable to a promise with .toPromise() and then wraps it
with from(), and it also injects an empty string as cartId into
mapPaymentSession; fix by removing the .toPromise() call and the outer from(...)
wrapper and instead operate directly on the Observable returned by
this.httpClient.post(...).pipe(...), keeping the existing map and catchError
operators on that observable (importing no new RxJS conversion helpers). In the
map operator replace the hard-coded const cartId = '' with a real extraction
from the response (e.g., response.data.payment_session.cart_id) or pass
undefined/null if absent and update mapPaymentSession usage accordingly (ensure
mapPaymentSession handles nullable cartId or accept cartId as an input parameter
instead). Reference symbols: httpClient.post, mapPaymentSession, updateSession
(the method containing this code), and the map/catchError operators.
In `@packages/integrations/medusajs/src/modules/products/products.mapper.ts`:
- Around line 238-271: mapRelatedProducts currently sets id to targetProduct.id
(a variant ID) which causes inconsistency with mapProduct (which uses
product?.id); change the id field in the mapping returned by mapRelatedProducts
to use the product-level id (use product?.id || targetProduct.product_id || ''
to cover shapes) so the Products.Model.Product.id is always the product ID;
update any other fields that might assume id is a variant to continue using
targetProduct.id if needed but keep the primary id as the product id.
In `@packages/integrations/medusajs/src/modules/products/products.service.ts`:
- Around line 78-101: The getProduct flow throws plain Error for missing
variants/variant (currently in the map() callback around the checks "No variants
found for product" and "Variant ... not found for product"), which causes
handleHttpError to convert them to 500s; replace those throws with Nest's
NotFoundException (imported from `@nestjs/common`) so missing-product/variant
cases return 404, or alternatively perform the variant existence validation
before the RxJS map() stage and emit a NotFoundException there; ensure the
change references the mapProduct/handleHttpError logic and keeps the error type
as NotFoundException so the HTTP pipeline returns 404 instead of 500.
In `@packages/integrations/mocked/src/modules/checkout/checkout.service.ts`:
- Around line 13-19: The CheckoutService currently declares unused dependencies
authService and customersService and incorrectly uses "implements
Checkout.Service" while Checkout.Service is an abstract class; remove the unused
constructor params/fields (authService, customersService), change the class
declaration to "extends Checkout.Service", and add a super(...) call in the
constructor passing the dependencies expected by the abstract base (e.g., pass
the required services such as cartsService and paymentsService per
Checkout.Service's constructor signature) so the subclass properly delegates
initialization to the base class.
🟡 Minor comments (13)
packages/integrations/medusajs/src/modules/products/products.mapper.ts-183-234 (1)
183-234:⚠️ Potential issue | 🟡 Minor
variantIddefaults to empty string when no variants exist.Line 207: if a product has no variants,
variantIdwill be''. If downstream code usesvariantIdto construct API calls (e.g., add-to-cart, pricing lookups), an empty string could cause silent failures or invalid requests. Consider whether products without variants should be filtered out of the list instead.packages/integrations/medusajs/src/modules/products/products.service.ts-56-71 (1)
56-71:⚠️ Potential issue | 🟡 MinorPush category filtering to the Store API server-side to maintain pagination accuracy.
getProductListcurrently filters bycategoryFilterclient-side (inmapProductslines ~191-193), which causes the returned product count to be less thanlimitand setstotaltoproducts.lengthinstead of the actual total for that category. The Medusa Store API supportscategory_idfiltering, so the filter should be passed as a param tothis.sdk.store.product.list()instead of applying it after the response. This ensures the API respects pagination bounds and returns the correct count metadata.packages/integrations/mocked/src/modules/checkout/checkout.mapper.ts-92-171 (1)
92-171:⚠️ Potential issue | 🟡 MinorAll locales use USD currency — intentional for mocks?
The German (
de) and Polish (pl) shipping options all use'USD'as the currency. If this mocked data is intended to test locale-specific behavior, consider using'EUR'or'PLN'for the respective locales. If the mock is purely for structural testing and currency is irrelevant, this is fine — but a brief comment would clarify intent.packages/integrations/medusajs/src/modules/customers/customers.service.ts-118-135 (1)
118-135:⚠️ Potential issue | 🟡 MinorFragile assumption: newly created address is the last in the array.
Line 122 assumes the newly created address is
addresses[addresses.length - 1]. If Medusa ever changes the ordering (e.g., sorts alphabetically, byis_default, or byid), this will silently return the wrong address. Consider matching by a known field (e.g., comparing the mapped address fields) or using the response more precisely if Medusa returns the created address ID.packages/integrations/mocked/src/modules/carts/carts.mapper.ts-304-316 (1)
304-316:⚠️ Potential issue | 🟡 MinorMetadata update on a potentially removed item.
When
data.quantity <= 0, the item is spliced from the array (Line 306), but the code continues to Line 314 where it may updateitem.metadataon the now-detached reference. This is harmless (the detached object is garbage-collected) but misleading. Add anelseorreturnafter the splice branch.Proposed fix
if (data.quantity !== undefined) { if (data.quantity <= 0) { cart.items.data.splice(itemIndex, 1); cart.items.total = cart.items.data.length; } else { item.quantity = data.quantity; item.subtotal = { value: item.price.value * data.quantity, currency: cart.currency }; item.total = { value: item.price.value * data.quantity, currency: cart.currency }; + if (data.metadata !== undefined) { + item.metadata = data.metadata; + } } + } else if (data.metadata !== undefined) { + item.metadata = data.metadata; } - if (data.metadata !== undefined) { - item.metadata = data.metadata; - }packages/integrations/medusajs/src/modules/checkout/checkout.mapper.ts-95-99 (1)
95-99:⚠️ Potential issue | 🟡 MinorUnsafe currency cast — unsupported currency codes pass through silently.
Line 99 casts the raw
currencyCodestring directly toModels.Price.Currencywithout validating it against the supported values (USD, EUR, GBP, PLN). If Medusa returns a valid ISO 4217 currency code that isn't supported (e.g., "JPY", "CHF"), it will silently produce invalid data.Add validation before the cast, either using a supported currencies array or a switch statement:
Proposed fix
const currencyCode = calculatedPrice?.currency_code?.toUpperCase(); if (!currencyCode) { throw new BadRequestException(`Shipping option ${option.id} has no currency information`); } + const supportedCurrencies = ['USD', 'EUR', 'GBP', 'PLN']; + if (!supportedCurrencies.includes(currencyCode)) { + throw new BadRequestException(`Shipping option ${option.id} has unsupported currency: ${currencyCode}`); + } const currency = currencyCode as Models.Price.Currency;Note: This pattern exists in other mappers as well (products, carts, orders).
apps/docs/docs/integrations/commerce/medusa-js/overview.md-58-99 (1)
58-99:⚠️ Potential issue | 🟡 MinorMissing config example for
payments.ts.The modules table (line 147) lists Payments as a supported module, but Step 1 does not include a corresponding
packages/configs/integrations/src/models/payments.tsconfig example. For completeness, consider adding it alongside the carts, checkout, and customers examples — users following this guide would otherwise miss configuring payments.apps/docs/docs/main-components/harmonization-app/normalized-data-model/core-model-orders.md-134-134 (1)
134-134:⚠️ Potential issue | 🟡 MinorAmbiguous type for
itemsfield.The type column for
itemsshowsdata, totalwhich is unclear for a reader. Other paginated types in this PR's docs use thePagination.Paginated<T>pattern (e.g.,Pagination.Paginated<Order>). Consider using a consistent type reference here.Suggested fix
-| items | data, total | Line items | +| items | Paginated\<OrderItem\> | Paginated line items |packages/integrations/medusajs/src/modules/customers/customers.mapper.ts-14-24 (1)
14-24:⚠️ Potential issue | 🟡 Minor
address_2mapped tostreetNumbermay lose the apartment data.Medusa's
address_2typically represents apartment/suite information, yet here it's mapped tostreetNumber. The framework model has bothstreetNumberandapartmentfields (seepackages/framework/src/utils/models/address.ts), butapartmentis never populated in the inbound mapping. In the reverse mapping (mapAddressToMedusa, Line 49),streetNumber || apartmentmeans theapartmentvalue is dropped ifstreetNumberis present.Consider splitting the mapping or documenting the intentional data loss:
Possible fix for inbound mapping
- streetNumber: medusaAddress.address_2, + apartment: medusaAddress.address_2,packages/integrations/medusajs/src/modules/carts/carts.mapper.ts-131-151 (1)
131-151:⚠️ Potential issue | 🟡 MinorPromotions are mapped with hardcoded
value: 0andtype: 'PERCENTAGE'.Every promotion will report zero value and "PERCENTAGE" type regardless of the actual discount. Consumers relying on
Promotion.valueto display savings will see nothing. If Medusa v2's cart response doesn't expose this data, a comment clarifying that limitation would help. Alternatively, consider computing the value fromcart.discount_totalif only one promotion is applied.packages/integrations/medusajs/src/modules/carts/carts.mapper.ts-89-104 (1)
89-104:⚠️ Potential issue | 🟡 Minor
address_2is mapped to bothstreetNumberandapartment, losing the distinction.Lines 98–99 both read from
address.address_2. Medusa'saddress_1is the primary street line andaddress_2is a secondary line (suite/apartment). Usingaddress_2forstreetNumberseems unintended — it should likely remain empty or be parsed fromaddress_1.🐛 Proposed fix
streetName: address.address_1 ?? '', - streetNumber: address.address_2 ?? '', - apartment: address.address_2 ?? '', + streetNumber: '', + apartment: address.address_2 ?? '',packages/integrations/medusajs/src/modules/carts/carts.service.ts-382-384 (1)
382-384:⚠️ Potential issue | 🟡 MinorRedundant ternary — both branches are identical.
Both branches of the conditional call
this.medusaJsService.getStoreApiHeaders(authorization)with the same argument, making the ternary a no-op. This same pattern is repeated on Lines 478-480.Proposed fix
- const headers = authorization - ? this.medusaJsService.getStoreApiHeaders(authorization) - : this.medusaJsService.getStoreApiHeaders(authorization); + const headers = this.medusaJsService.getStoreApiHeaders(authorization);Apply the same fix at Lines 478-480.
packages/integrations/mocked/src/modules/carts/carts.service.ts-170-184 (1)
170-184:⚠️ Potential issue | 🟡 MinorWrong exception type:
NotFoundExceptionused for a validation error.Lines 171 and 183 throw
NotFoundException("Currency is required when creating a new cart"). A missing required field is a client input error, which should beBadRequestException(HTTP 400), notNotFoundException(HTTP 404).Proposed fix
if (!data.currency) { - throw new NotFoundException('Currency is required when creating a new cart'); + throw new BadRequestException('Currency is required when creating a new cart'); }Apply at both Line 171 and Line 183.
🧹 Nitpick comments (52)
packages/integrations/mocked/src/modules/resources/resources.mapper.ts (2)
397-406: Inconsistent customer ID formats across switch cases.
'cus_01KH3J08TY40PYGVEG3A04CP8R'follows Medusa.js ID conventions while'cust-002'retains the old format. If the intent is to align mocked data with Medusa.js, consider updating the second case as well for consistency.
556-562: Typo:getCastomerId→getCustomerId.Pre-existing, but since this PR touches the call sites, it's a good time to fix it.
Proposed fix
-const getCastomerId = (authorization: string): string | null => { +const getCustomerId = (authorization: string): string | null => {And update references at lines 393 and 472 accordingly.
packages/integrations/medusajs/src/modules/utils/handle-http-error.ts (1)
11-22: Mixed error-propagation styles: synchronousthrowvs.return throwError(…).Lines 15–21 use synchronous
throw, while lines 12–14 and 22 return RxJSthrowErrorobservables. Callers must guard against both a synchronous exception and an observable error from the same function, which is fragile. Consider unifying onreturn throwError(() => …)throughout for consistency.♻️ Suggested unification
export const handleHttpError = (error: any) => { if (error instanceof HttpException) { return throwError(() => error); } if (error.status === 404) { - throw new NotFoundException(`Not found`); + return throwError(() => new NotFoundException('Not found')); } else if (error.status === 403) { - throw new ForbiddenException('Forbidden'); + return throwError(() => new ForbiddenException('Forbidden')); } else if (error.status === 401) { - throw new UnauthorizedException('Unauthorized'); + return throwError(() => new UnauthorizedException('Unauthorized')); } return throwError(() => new InternalServerErrorException(error.message)); };Note: this would require updating the existing tests for 404/403/401 to use
firstValueFrom+rejects.toThrowinstead of synchronousexpect().toThrow.packages/integrations/medusajs/src/modules/products/products.service.spec.ts (2)
156-168: Good edge-case coverage for missing-variant scenarios.These two new tests cover important defensive paths — empty variants and non-matching variant IDs — ensuring the service surfaces clear, actionable error messages.
One minor style note: the inline
as import('@o2s/framework/modules').Products.Request.GetProductParamstype assertions (Lines 165, 178) could be replaced by a top-level import for readability:import type { Products } from '@o2s/framework/modules';Then use
as Products.Request.GetProductParamsinline. This is purely a readability preference.Also applies to: 170-181
55-56: Consider removing the unusedmockHttpClienttype from the SDK-centric tests, or at least documenting why it's still needed.
mockHttpClientis still declared and passed to the constructor but is only exercised bygetRelatedProductList. This is fine since the service constructor requires it, but adding a brief comment (e.g.,// still needed for getRelatedProductList which uses admin API) would help future readers understand the split between SDK and HTTP paths.packages/integrations/medusajs/src/modules/products/products.service.ts (1)
33-35: Tight coupling between field strings and mapper expectations.These field strings are critical — if they drift from what the mapper expects (e.g.,
calculated_price,categories,tags,images,metadata), the mapper will silently produce incomplete data (prices default to 0, missing specs, etc.). Consider adding a brief inline comment mapping each field group to the mapper function that consumes it, or co-locating these constants with the mapper.packages/integrations/medusajs/src/modules/products/products.mapper.ts (3)
49-75: Prefer??over||for numeric fallback.Line 68:
amount: matchingPrice.amount || 0— using||means a legitimate price of0is treated the same asundefined. While0 || 0still evaluates to0, this is coincidental;??conveys the correct intent (fallback only onnull/undefined) and is consistent with the!= nullcheck on Line 55.Proposed fix
- amount: matchingPrice.amount || 0, + amount: matchingPrice.amount ?? 0,
80-113: Missing type validation on parsedkeySpecsitems.After
JSON.parse(Line 95) or the array branch (Line 100), the individual items inkeySpecsare not validated. If the parsed JSON contains objects wherevalueoriconare non-strings (e.g., numbers, nested objects), they'll be passed through to the model undetected.Given this is metadata from an external system, a lightweight runtime check would improve robustness:
Proposed defensive filter
return keySpecs.map((spec) => ({ - value: spec.value, - icon: spec.icon, + value: typeof spec.value === 'string' ? spec.value : String(spec.value ?? ''), + icon: typeof spec.icon === 'string' ? spec.icon : undefined, }));
117-136: Type-safety ofkeyofon union types.
variant[field as keyof AnyProductVariant]on Line 123 works at runtime butkeyof (A | B)yields only the intersection of keys. If anyVARIANT_SPEC_FIELDSentry isn't a shared key betweenStoreProductVariantandAdminProductVariant, TypeScript won't catch it — theascast silences the compiler. This is acceptable here since these are standard Medusa variant fields, but worth a note if the spec fields list grows.packages/integrations/medusajs/src/modules/payments/payments.service.spec.ts (1)
76-83: Minor: duplicate invocations to assert message and type separately.Each
expectcall invokesservice.getProviders(...)again. You could call once, capture the error, and assert both the type and message on the same caught error — reducing redundant execution.packages/integrations/medusajs/src/modules/customers/customers.service.spec.ts (1)
71-81: Minor: same duplicate invocation pattern as in payments spec.Same as noted in the payments spec — the two
expectcalls each invokeservice.getAddresses(undefined)separately. Consider capturing the thrown error once and asserting both the type and message.packages/framework/src/modules/payments/payments.model.ts (1)
16-25: Consider documentingclientSecretsensitivity and serialization expectations.
clientSecret(line 22) is a sensitive field. While Stripe's client secret is designed to be shared with the frontend for payment confirmation, ensure that response serialization doesn't inadvertently expose secrets from other providers where the semantics may differ. A brief doc comment noting the intended audience (client-side only) would help future maintainers.packages/integrations/medusajs/src/modules/customers/customers.service.ts (1)
35-58: Auth checks throw synchronously outside the Observable pipeline.The
throw new UnauthorizedException(...)at lines 37 and 42 executes synchronously before any Observable is returned. While this works with NestJS (the framework catches the thrown exception), it's inconsistent with the RxJS error-handling pattern used in the rest of the method (catchError,throwError). This pattern is repeated in every method.This is fine functionally — just noting it's a stylistic inconsistency. The synchronous throw is arguably preferable here as a "fail fast" guard.
packages/integrations/mocked/src/modules/checkout/checkout.mapper.ts (1)
183-183: Type castas Orders.Model.ShippingMethod[]may hide missing fields.The inline shipping option objects define
id,name,description,total, andsubtotal, butOrders.Model.ShippingMethodmay require additional fields. Theascast suppresses type checking here. Consider either extending the mock objects to match the full type or usingPartial<>/ a dedicated mock factory.packages/framework/src/modules/carts/carts.service.ts (1)
5-6: Unusual variadic constructorprotected constructor(..._services: unknown[]).The variadic
unknown[]constructor discards all arguments. If the intent is to allow subclasses to pass dependencies, a parameterlessprotected constructor()would be clearer — NestJS DI injects into the subclass constructor, not the super call.packages/integrations/medusajs/src/modules/checkout/checkout.mapper.ts (1)
84-86: Unused_defaultCurrencyparameter inmapShippingOption.The
_defaultCurrencyparameter is accepted but never used — the function throws when currency data is missing instead of falling back to the default. Either remove the parameter or use it as a fallback whencurrency_codeis absent.packages/integrations/mocked/src/modules/carts/carts.mapper.ts (2)
7-17: Unsafe type assertions inmapPaymentMethodFromMetadata.Multiple
ascasts without validation (Lines 8, 12-15). Ifmetadata.paymentMethodhas an unexpected shape, this will silently produce a malformedPaymentMethod. Since this is mocked code, impact is low, but consider adding basic guards.
249-254: Silent error swallowing when product is not found.The
catchblock silently returnsundefinedwithout logging. This makes debugging difficult when items fail to add. Consider logging the error.Proposed fix
try { product = mapProduct(data.productId); - } catch { + } catch (error) { + console.warn(`Product not found: ${data.productId}`, error); return undefined; // Product not found }packages/framework/src/modules/checkout/checkout.service.ts (1)
6-6: Replace barrel import with direct imports to avoid circular reference.Line 6:
import * as Checkout from './'creates a circular dependency—the barrel re-exports this very file. While TypeScript handles this at compile time, it's fragile and unconventional. Import directly from the specific modules instead:import * as Request from './checkout.request'; import * as Model from './checkout.model';Then update references from
Checkout.Request.*toRequest.*andCheckout.Model.*toModel.*throughout the class.packages/framework/src/modules/checkout/checkout.request.ts (1)
1-67: Add validation decorators to request DTOs.These DTOs lack
class-validatordecorators, and no globalValidationPipeis configured inmain.tsorapp.module.ts. Without request-level validation, invalid shapes and types will pass through to the service layer unchecked. Either add@IsString(),@IsOptional(),@IsNotEmpty()decorators to enforce request schema, or configure a globalValidationPipein the NestJS bootstrap.packages/configs/integrations/src/models/payments.ts (1)
1-9: Non-null assertion onConfig.payments!relies on mocked integration always defining this field.Since
Configis typed asPartial<ApiConfig['integrations']>, this will throw at runtime if the mocked integration ever removes or fails to define thepaymentsentry. This appears to be a pre-existing pattern across other config modules, so it's consistent — but worth noting the implicit contract.apps/docs/docs/main-components/harmonization-app/normalized-data-model/core-model-checkout.md (1)
149-180: Mermaid class diagram classes are empty — consider adding key fields.The class bodies are all empty, which reduces the diagram's usefulness as a quick reference. Adding at least the primary fields (e.g.,
cart: Cart,totals: objectonCheckoutSummary) would make the diagram self-documenting alongside the type tables below.apps/docs/docs/main-components/harmonization-app/normalized-data-model/core-model-carts.md (1)
230-250: Clarify theitemsfield type.Line 236 lists the type of
itemsasdata, total, which reads more like a shape description than a type. Consider using something like{ data: CartItem[], total: number }or a named type alias to be consistent with how other fields are documented.packages/integrations/medusajs/src/modules/checkout/checkout.mapper.spec.ts (1)
72-77: Consider combining the twoexpectcalls per error test to avoid invoking the mapper twice.Each error test calls the mapper function twice — once to check the exception type, once to check the message. You could use a single assertion with
toThrowErroror wrap in a try/catch, though this is a minor style point.♻️ Example consolidation
- expect(() => mapCheckoutSummary(cart)).toThrow(BadRequestException); - expect(() => mapCheckoutSummary(cart)).toThrow('Shipping address is required'); + expect(() => mapCheckoutSummary(cart)).toThrow( + expect.objectContaining({ + message: expect.stringContaining('Shipping address is required'), + }), + );This pattern checks both the exception type (via
objectContaining) and message in one invocation. Apply similarly to the other error tests.apps/docs/docs/main-components/harmonization-app/normalized-data-model/core-model-customers.md (2)
101-110: Mermaid class diagram has empty classes, providing minimal value.The diagram declares
CustomerAddressandAddresswith no fields. Consider populating them with key fields (e.g.,id,label,isDefault,firstName,city, etc.) to make the diagram self-explanatory without needing to scroll to the types tables below.
65-75: Missing body parameters table forupdateAddress.
createAddress(Line 57) documents its body parameters in a table, butupdateAddressdoes not. Since they likely share a similar shape, consider adding the same detail here for consistency. Similarly,deleteAddressandsetDefaultAddressare missing parameter tables.packages/integrations/medusajs/src/modules/customers/customers.mapper.spec.ts (1)
44-47: Weak date assertions — consider checking exact ISO strings.
toContain('2024')would pass for any string containing "2024". Consider asserting the exact expected value (e.g.,toEqual(new Date('2024-01-01'))or comparing ISO strings) for more deterministic tests.packages/integrations/mocked/src/modules/payments/payments.service.ts (2)
54-68: Redundant null check after successfulfindIndex.When
findIndexreturns a non-negative index (line 54),this.sessions[index]is guaranteed to exist. The check on lines 60–63 is unreachable dead code. The same pattern repeats incancelSession(lines 78–81).♻️ Proposed simplification for updateSession
const index = this.sessions.findIndex((s) => s.id === params.id); if (index === -1) { return throwError(() => new NotFoundException(`Payment session with ID ${params.id} not found`)); } - const existingSession = this.sessions[index]; - if (!existingSession) { - return throwError(() => new NotFoundException(`Payment session with ID ${params.id} not found`)); - } + const existingSession = this.sessions[index]!; const updatedSession = updatePaymentSession(existingSession, data);
22-38: Hardcoded'en'locale when resolving provider increateSession.
getProviderscorrectly usesparams.locale, butcreateSessionon line 26 always passes'en'togetMockProviderById. If a provider only exists in a non-English locale set, the lookup would fail with a falseNotFoundException. For a mock service this is low-risk, but worth noting for consistency.packages/integrations/medusajs/src/modules/checkout/checkout.service.spec.ts (1)
157-174: Consider adding error-path tests forsetShippingMethod.
setAddresseshas tests for cart-not-found and empty-cart, butsetShippingMethodonly tests the happy path. If the service performs similar validation (cart existence, items check) before delegating, those branches are untested here.packages/integrations/medusajs/src/modules/orders/orders.mapper.ts (1)
89-91:streetNumberandapartmentboth map toaddress_2.Both fields are derived from the same source (
address.address_2), which means they'll always hold identical values. If this is intentional (Medusa doesn't distinguish between them), a brief comment would clarify the intent. Otherwise, one of them should use a different source or be left empty.packages/integrations/medusajs/src/modules/customers/customers.mapper.ts (1)
12-12: Add a code comment explaining the name-based label fallback.The
CustomerAddress.labelfield is documented as a semantic label (e.g., "Home", "Work", "Billing"), but Medusa'sStoreCustomerAddresslacks a dedicated label field. Using the person's name as a fallback is pragmatic, but this design decision should be documented in the code comment to clarify why the field's semantic purpose differs from its implementation.packages/integrations/medusajs/src/modules/carts/carts.service.spec.ts (1)
483-502: Redundant double-invocation in "no items" prepareCheckout test.The test calls
firstValueFrom(service.prepareCheckout(...))twice — once to assertBadRequestExceptionand again to assert the message string. Each call re-executes the service method (and the underlying mock). Combine both assertions on a single call:♻️ Proposed fix: single invocation with chained assertion
it('should throw BadRequestException when cart has no items', async () => { mockSdk.store.cart.retrieve.mockResolvedValue({ cart: minimalCart }); - await expect( - firstValueFrom( - service.prepareCheckout( - { cartId: 'cart_1' } as Carts.Request.PrepareCheckoutParams, - 'Bearer token', - ), - ), - ).rejects.toThrow(BadRequestException); - await expect( - firstValueFrom( - service.prepareCheckout( - { cartId: 'cart_1' } as Carts.Request.PrepareCheckoutParams, - 'Bearer token', - ), - ), - ).rejects.toThrow('Cart must have items before preparing checkout'); + await expect( + firstValueFrom( + service.prepareCheckout( + { cartId: 'cart_1' } as Carts.Request.PrepareCheckoutParams, + 'Bearer token', + ), + ), + ).rejects.toThrow( + expect.objectContaining({ + message: 'Cart must have items before preparing checkout', + }), + ); });packages/integrations/medusajs/src/modules/payments/payments.mapper.ts (2)
5-14:requiresRedirectrelies on fragile substring matching against provider IDs.If Medusa is configured with a custom payment provider whose ID doesn't contain
'stripe'or'paypal'but still requires redirect (e.g., an Adyen redirect integration), this will incorrectly returnfalse. Consider extending the check or derivingrequiresRedirectfrommapProviderTypeto keep the logic centralized:♻️ Proposed refactor
export function mapPaymentProvider(medusaProvider: HttpTypes.StorePaymentProvider): Payments.Model.PaymentProvider { + const type = mapProviderType(medusaProvider.id); return { id: medusaProvider.id, name: medusaProvider.id, // Medusa doesn't provide a name, use ID - type: mapProviderType(medusaProvider.id), + type, isEnabled: true, // Assume enabled if returned by API - requiresRedirect: medusaProvider.id.includes('stripe') || medusaProvider.id.includes('paypal'), + requiresRedirect: type === 'STRIPE' || type === 'PAYPAL' || type === 'ADYEN', config: {}, }; }
54-67: Null/undefinedstatuswould cause a runtime crash on.toUpperCase().If Medusa ever returns a payment session with a missing or null
status,status.toUpperCase()will throw aTypeError. A defensive guard would prevent this:🛡️ Proposed fix
function mapPaymentSessionStatus(status: string): Payments.Model.PaymentSessionStatus { - switch (status.toUpperCase()) { + switch ((status ?? '').toUpperCase()) {packages/integrations/mocked/src/modules/payments/mocks/providers.mock.ts (2)
16-58: Consider extracting shared provider definitions to reduce duplication.The
stripeandpaypalentries are identical across all three locales — onlysystem.namediffers. You could define a base array and override the localizedsystementry per locale. That said, this is mock data, so the duplication is tolerable if readability is preferred.
76-83:'BANK_TRANSFER'is listed in the return type but never produced.
providerTypeToPaymentMethodTypedeclares'BANK_TRANSFER'in its return union, but no branch returns it. If it's intentionally reserved for future use, a brief comment would clarify; otherwise, remove it from the type to avoid confusion.packages/integrations/medusajs/src/modules/carts/carts.mapper.ts (2)
15-15:_defaultCurrencyparameter is accepted but unused.The underscore prefix suppresses the linter warning, but this parameter was presumably intended as a fallback when
cart.currency_codeis missing. Consider using it in the fallback path at line 16–18, or removing it from the signature if truly unnecessary.♻️ Proposed fix: use as fallback
-export const mapCart = (cart: HttpTypes.StoreCart, _defaultCurrency: string): Carts.Model.Cart => { +export const mapCart = (cart: HttpTypes.StoreCart, defaultCurrency: string): Carts.Model.Cart => { - if (!cart.currency_code) { - throw new Error(`Cart ${cart.id} has no currency code`); - } - const currency = cart.currency_code as Models.Price.Currency; + const rawCurrency = cart.currency_code || defaultCurrency; + if (!rawCurrency) { + throw new Error(`Cart ${cart.id} has no currency code`); + } + const currency = rawCurrency.toUpperCase() as Models.Price.Currency;
26-27:Date.toString()produces a non-ISO format when the value is aDateobject.If
cart.created_atis aDateinstance (as in the test fixture),.toString()returns a locale-dependent string like"Mon Jan 01 2024 ...", not ISO 8601. The fallback uses.toISOString(), creating an inconsistency. If the Medusa SDK deserializes timestamps asDateobjects rather than strings, downstream consumers expecting ISO strings will be affected.♻️ Proposed fix
- createdAt: cart.created_at?.toString() ?? new Date().toISOString(), - updatedAt: cart.updated_at?.toString() ?? new Date().toISOString(), + createdAt: cart.created_at instanceof Date ? cart.created_at.toISOString() : (cart.created_at ?? new Date().toISOString()), + updatedAt: cart.updated_at instanceof Date ? cart.updated_at.toISOString() : (cart.updated_at ?? new Date().toISOString()),packages/framework/src/modules/customers/customers.controller.ts (1)
9-12: Add auth decorator to controller to align with framework pattern.CustomersController lacks the
@Auth.Decorators.Rolesmetadata decorator that UsersController applies to its endpoints. While the service layer validatesheaders.authorizationand throwsUnauthorizedException, auth concerns should be declared at the controller level for consistency and clarity. Apply@Auth.Decorators.Roles({ roles: [] })to each endpoint to match the established pattern in UsersController.packages/integrations/mocked/src/modules/payments/payments.mapper.ts (1)
28-28: Consider adding randomness to the ID to avoid collisions.
Date.now()alone can produce duplicate IDs under concurrent calls. ThecreateCustomerAddressmapper inpackages/integrations/mocked/src/modules/customers/customers.mapper.tsuses a more robust pattern withMath.random().toString(36)appended.🔧 Proposed fix
- const id = `ps_${Date.now()}`; + const id = `ps_${Date.now()}-${Math.random().toString(36).slice(2, 9)}`;packages/integrations/mocked/src/modules/customers/customers.service.ts (2)
89-98: Redundant null check after a successfulfindIndex.Lines 95-98 check
existingAddressfor truthiness immediately after confirmingindex !== -1. In single-threaded JS,this.addresses[index]is guaranteed to exist iffindIndexjust returned a valid index. The same pattern repeats insetDefaultAddress(lines 148-151).♻️ Simplified version
const index = this.addresses.findIndex((addr) => addr.id === params.id && addr.customerId === customerId); if (index === -1) { return throwError(() => new NotFoundException(`Address with ID ${params.id} not found`)); } - const existingAddress = this.addresses[index]; - if (!existingAddress) { - return throwError(() => new NotFoundException(`Address with ID ${params.id} not found`)); - } + const existingAddress = this.addresses[index]!;
16-16:implementsvsextends— same consistency note as the mockedCheckoutService.The framework's
CustomerServiceis an abstract class. Usingimplementsworks but skips the base constructor. For consistency with the MedusaJS integrations (which useextends), consider switching.packages/integrations/medusajs/src/modules/checkout/checkout.service.ts (2)
209-216: Direct mutation of the mappedorderobject.
order.email = emailmutates the object returned bymapOrder. This works but can be surprising ifmapOrderis ever expected to produce an immutable result. A spread would be safer:♻️ Immutable alternative
- const order = mapOrder(response.order, this.defaultCurrency); - - // Attach email for order confirmation if provided (guest checkout) - if (email) { - order.email = email; - } - - return of(mapPlaceOrderResponse(order)); + const order = mapOrder(response.order, this.defaultCurrency); + const orderWithEmail = email ? { ...order, email } : order; + + return of(mapPlaceOrderResponse(orderWithEmail));
23-35: Remove unusedauthServiceandcustomersServiceinjections.
this.authServiceandthis.customersServiceare never referenced. Also, unlikeOrdersServicewhich throws ifDEFAULT_CURRENCYis empty (lines 43-45 of orders.service.ts),CheckoutServicesilently defaults to an empty string on line 34. Align the validation behavior by either adding a guard or documenting why an empty default is acceptable here.packages/integrations/medusajs/src/modules/payments/payments.service.ts (2)
46-69: Swallowing all errors ingetProvidersmay hide real issues.The
catchErroron Line 63 catches all errors (including 401/403/500) and silently returns an empty provider list. This makes sense for graceful degradation, but authentication or server errors will be invisible to callers. Consider re-throwing non-retriable errors (401, 403) so they propagate correctly.Proposed fix: re-throw auth errors
catchError((error) => { - // If endpoint doesn't exist or fails, return empty list - this.logger.warn('Failed to fetch payment providers from Medusa', error); - return of(mapPaymentProviders([], 10, 0)); + if (error.response?.status === 401 || error.response?.status === 403) { + return handleHttpError(error); + } + this.logger.warn('Failed to fetch payment providers from Medusa', error); + return of(mapPaymentProviders([], 10, 0)); }),
116-130:getSessionalways returnsundefined— consider a clearer contract.The method is fully stubbed and will always return
undefined. The extensive inline comments explain why, which is good. However, if this is a known limitation, consider throwingNotImplementedException(consistent with howgetCartListandgetCurrentCarthandle unsupported operations in the carts service) rather than silently returningundefined, which callers may misinterpret as "session not found."packages/integrations/medusajs/src/modules/carts/carts.service.ts (3)
521-541:resolveBillingAddressis unused dead code.This private method duplicates the billing address resolution logic that is already inlined in
updateCartAddresses(Lines 402-414). It is never called anywhere in the file.Remove the dead method, or refactor `updateCartAddresses` to use it
Option A — Remove it:
- /** - * Resolves the billing address from the request data. - * Returns `null` if no billing address is specified (caller should fall back to shipping address). - */ - private resolveBillingAddress( - data: Carts.Request.UpdateCartAddressesBody, - authorization: string | undefined, - ): Observable<HttpTypes.StoreAddAddress | null> { - if (data.billingAddressId && authorization) { - return this.customersService.getAddress({ id: data.billingAddressId }, authorization).pipe( - map((billingAddress) => { - if (!billingAddress) { - throw new NotFoundException(`Address with ID ${data.billingAddressId} not found`); - } - return mapAddressToMedusa(billingAddress.address); - }), - ); - } - - if (data.billingAddress) { - return of(mapAddressToMedusa(data.billingAddress)); - } - - return of(null); - }Option B — Use it in
updateCartAddressesto reduce inline duplication.
136-140:deleteCartsilently succeeds without performing any operation.This returns
of(undefined)without making any API call to Medusa, which means the cart remains intact on the server. While there's a debug log, callers will believe the deletion succeeded. Consider throwingNotImplementedException(likegetCartListandgetCurrentCart) to make the limitation explicit.Proposed fix
deleteCart(_params: Carts.Request.DeleteCartParams, _authorization?: string): Observable<void> { - this.logger.debug('Delete cart operation - not directly supported in Medusa Store API'); - - return of(undefined as void); + return throwError( + () => + new NotImplementedException( + 'Delete cart is not supported in Medusa.js integration. Medusa Store API does not provide a cart deletion endpoint.', + ), + ); }
30-44: Constructor throws synchronously on missing config, which crashes the DI container.Line 42 throws a plain
ErrorifDEFAULT_CURRENCYis not set. In NestJS, this will crash the entire application during bootstrap rather than providing a clear diagnostic. This is a valid guard but worth noting — aConfigServicevalidation (e.g., via@nestjs/configschema validation) would catch this earlier with a better error message.packages/framework/src/modules/carts/carts.request.ts (1)
50-58:quantitylacks a minimum value constraint.
AddCartItemBody.quantityis typed asnumberwith no minimum constraint. A value of0or negative would be semantically invalid. The Medusa integration'saddCartItempasses it directly to the SDK without validation.packages/integrations/mocked/src/modules/carts/carts.service.ts (1)
30-48: Repeated authorization boilerplate across 10+ methods.Every mutating method repeats the same pattern: fetch cart → check
customerId→ validate authorization → check ownership. Consider extracting a private helper likeassertCartAccess(cartId, authorization)that returns the cart or throws.Example helper
private assertCartAccess( cartId: string, authorization: string | undefined, ): Carts.Model.Cart { const cart = mapCart({ id: cartId }); if (!cart) { throw new NotFoundException('Cart not found'); } if (cart.customerId) { if (!authorization) { throw new UnauthorizedException('Authentication required to access this cart'); } const customerId = this.authService.getCustomerId(authorization); if (cart.customerId !== customerId) { throw new UnauthorizedException('Unauthorized to access this cart'); } } return cart; }Also applies to: 76-104, 106-130, 204-232, 234-261, 263-291, 293-320, 338-365, 457-506
packages/integrations/medusajs/src/modules/carts/carts.mapper.ts
Outdated
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/carts/carts.service.ts
Outdated
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/payments/payments.service.ts
Outdated
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/payments/payments.service.ts
Outdated
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/products/products.mapper.ts
Outdated
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/products/products.service.ts
Outdated
Show resolved
Hide resolved
- Replaced hardcoded URLs with dynamic inputs for better flexibility in checkout. - Enhanced promotion handling with direct API invocation for addition and removal. - Refactored `carts.mapper` to standardize currency to uppercase.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@packages/framework/src/modules/carts/carts.request.ts`:
- Around line 50-58: The DTO AddCartItemBody currently marks productId as
required and variantId optional, but Medusa's implementation
(CartsService.addCartItem) requires variantId and ignores productId; update
AddCartItemBody to make variantId required and productId optional (or remove
productId requirement) so the DTO matches concrete integrations, and then update
any call sites, tests, and validation that reference AddCartItemBody or expect
productId to be required to use variantId instead; ensure
CartsService.addCartItem and any other integration code still accept the DTO
shape.
In `@packages/integrations/medusajs/src/modules/carts/carts.mapper.ts`:
- Around line 96-99: The mapping currently assigns address.address_2 to both
streetNumber and apartment (see region: address.province, streetName:
address.address_1, streetNumber: address.address_2, apartment:
address.address_2), which duplicates the same value; change the mapper so
address.address_2 is mapped only to apartment (e.g., apartment:
address.address_2 ?? '' ) and set streetNumber to undefined or an empty string
(e.g., streetNumber: undefined or ''), updating the mapping function in
carts.mapper.ts where these symbols are defined.
- Line 39: The Cart mapping currently uses an unsafe cast (mapPrice(... ) as
Models.Price.Price) which can return undefined while Cart.total (and other price
fields mapped similarly) are non-optional; update the mapper to handle missing
prices by either throwing a clear error or supplying a safe fallback (e.g., {
value: 0, currency }) before assigning to Cart.total and the other affected
fields (the occurrences calling mapPrice for total and the other price fields).
Locate the calls to mapPrice in the carts mapper (references to mapPrice and
Cart.total / Models.Price.Price) and replace the direct cast with a guard that
checks the result and either throws a descriptive error or returns/uses the
fallback price object so the Cart model always receives a valid
Models.Price.Price.
In `@packages/integrations/medusajs/src/modules/carts/carts.service.spec.ts`:
- Around line 441-462: The removePromotion flow is using method: 'POST' but
Medusa v2 requires DELETE with a body containing promo_codes; update the service
method removePromotion (in CartsService / carts.service.ts) to call
mockSdk.client.fetch (or client.fetch) with method: 'DELETE' and body: {
promo_codes: [code] } for the '/store/carts/{cartId}/promotions' endpoint, and
update the unit test removePromotion in carts.service.spec.ts to expect method:
'DELETE' and promo_codes array in the body; keep the endpoint and response
assertions the same.
In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts`:
- Around line 142-145: The addCartItem method currently throws
BadRequestException synchronously (in addCartItem) which breaks the Observable
contract; change both synchronous throws (the initial variantId check in
addCartItem and the similar check later around the 183–185 area) to return
throwError(() => new BadRequestException('...')) so errors are emitted through
the Observable stream instead of thrown; update callers/tests that expect a sync
throw to instead await the Observable (e.g.,
firstValueFrom(service.addCartItem(...))) and assert rejects.toThrow when
needed.
- Around line 356-358: The ternary assigning headers is dead code because both
branches call the same method; replace the conditional with a single direct call
to this.medusaJsService.getStoreApiHeaders(authorization) when setting headers
(look for the variable headers in carts.service.ts and the getStoreApiHeaders
method), and apply the same simplification to the identical pattern found later
in the file (the repeated ternary at the second occurrence).
- Around line 259-275: The removePromotion function is calling the wrong HTTP
method and re-applies promo codes; change the request in removePromotion to use
method: 'DELETE' (still sending body: { promo_codes: [params.code] } and the
same headers from medusaJsService.getStoreApiHeaders(authorization)) so the
Medusa Store API removes the promotion; keep the response mapping
(mapCart(response.cart, this.defaultCurrency)) and the error handling
(handleHttpError) unchanged.
In `@packages/integrations/mocked/src/modules/carts/carts.mapper.ts`:
- Around line 388-416: In recalculateCartTotals, clamp the computed
discountTotal so it never exceeds the cart subtotal (e.g., discountTotal =
Math.min(discountTotal, subtotal)) and apply rounding after clamping; compute
taxTotal using the non-negative taxable base (Math.max(subtotal - discountTotal,
0)) and ensure the final total is also clamped to non-negative before assigning
to cart.total; update the assignments for cart.discountTotal, cart.taxTotal and
cart.total accordingly in the recalculateCartTotals function to prevent negative
tax or total values.
- Around line 304-316: The metadata update is being applied to a removed item
when data.quantity <= 0; inside the quantity-handling block in carts.mapper.ts
(the if (data.quantity !== undefined) branch that uses
cart.items.data.splice(itemIndex, 1) and sets cart.items.total), prevent further
updates to the detached item by returning early or adding an else guard so the
subsequent metadata assignment (item.metadata = data.metadata) only runs when
the item was not spliced out; ensure cart.items.total remains correct and no
metadata is written to the removed item reference.
In `@packages/integrations/mocked/src/modules/checkout/checkout.service.ts`:
- Around line 30-32: The current cart validation uses cart.items and
cart.items.data.length which can throw if cart.items.data is nullish; update the
checks to safely handle missing data (e.g., use optional chaining like
cart.items?.data?.length) where you currently return throwError(() => new
BadRequestException(...)) so the predicate becomes something like "if
(!cart.items?.data || cart.items.data.length === 0)" (or check
cart.items?.data?.length === 0) and apply the same defensive change at the
second occurrence on line 52 in checkout.service.ts to avoid TypeError.
- Around line 82-91: The mocked setPayment implementation is ignoring
caller-provided returnUrl and cancelUrl by hardcoding example URLs when calling
client.createSession; update the call in setPayment to pass data.returnUrl and
data.cancelUrl (instead of the literal 'https://example.com/checkout/return' and
cancel) so the method forwards the request body's URLs to createSession (ensure
null/undefined handling if necessary); references: setPayment and the
.createSession(...) invocation.
🧹 Nitpick comments (8)
packages/integrations/medusajs/src/modules/medusajs/medusajs.service.ts (1)
49-78: Consider validating config eagerly in the constructor or at module bootstrap.The lazy initialization defers config validation to first use, meaning a misconfigured environment (e.g., missing
MEDUSAJS_BASE_URL) won't surface until the first request hits a dependent service. Since all known consumers (OrdersService,CartsService,ProductsService,CheckoutService) callgetSdk()in their constructors anyway, the lazy pattern doesn't actually defer any cost — it just delays the error surface from app startup to module construction time.If the intent is to allow the module to load without Medusa config (e.g., when Medusa integration is disabled), this is fine as-is. Otherwise, consider adding a NestJS
OnModuleInithook to fail fast at startup:Optional: eager validation via OnModuleInit
+import { OnModuleInit } from '@nestjs/common'; + `@Global`() `@Injectable`() -export class MedusaJsService { +export class MedusaJsService implements OnModuleInit { // ... existing fields and constructor ... + onModuleInit(): void { + this.ensureInitialized(); + } + // ... rest of methods ... }packages/integrations/medusajs/src/modules/payments/payments.service.spec.ts (3)
76-83: Nit: Consolidate the two throw assertions into one block.Both assertions invoke the same function call. You could verify the exception type and message in a single assertion or at least avoid calling the function under test twice.
♻️ Suggested consolidation
- it('should throw BadRequestException when regionId is missing', () => { - expect(() => service.getProviders({} as Payments.Request.GetProvidersParams, 'Bearer token')).toThrow( - BadRequestException, - ); - expect(() => service.getProviders({} as Payments.Request.GetProvidersParams, 'Bearer token')).toThrow( - 'regionId is required', - ); + it('should throw BadRequestException when regionId is missing', () => { + expect(() => service.getProviders({} as Payments.Request.GetProvidersParams, 'Bearer token')).toThrow( + new BadRequestException('regionId is required'), + ); });
194-194: Nit: Unnecessaryas stringcast on a string literal.
'missing'is already of typestring; the cast is redundant.
202-209: Stub implementation acknowledged — consider adding a TODO or tracking issue.
getSessionalways returnsundefined, which suggests it's a placeholder. A comment or tracked issue would help ensure this doesn't get forgotten.packages/integrations/mocked/src/modules/checkout/checkout.service.ts (1)
172-173: Shared mutableMOCKED_ORDERSarray grows unboundedly and leaks state across tests.Each
placeOrdercall appends to a module-level array that is never cleared. In a long-running test suite this causes inter-test coupling and unbounded memory growth. Consider exposing a reset helper or scoping the store per test.packages/integrations/medusajs/src/modules/carts/carts.service.spec.ts (1)
1-6: Minor: consolidate the two@nestjs/commonimport lines.Lines 2 and 3 import from the same module. Merge them into a single import statement.
♻️ Suggested fix
-import { BadRequestException } from '@nestjs/common'; -import { NotFoundException, NotImplementedException, UnauthorizedException } from '@nestjs/common'; +import { BadRequestException, NotFoundException, NotImplementedException, UnauthorizedException } from '@nestjs/common';packages/integrations/medusajs/src/modules/carts/carts.service.ts (1)
495-515: Remove the deadresolveBillingAddressmethod.This private method (lines 495–515) is never called anywhere in the codebase, and its logic is already duplicated inline in
updateCartAddresses(lines 376–388). Both implementations follow the same pattern: resolve a billing address by ID or use the provided address, with no effective difference.Removal diff
- /** - * Resolves the billing address from the request data. - * Returns `null` if no billing address is specified (caller should fall back to shipping address). - */ - private resolveBillingAddress( - data: Carts.Request.UpdateCartAddressesBody, - authorization?: string, - ): Observable<HttpTypes.StoreAddAddress | null> { - if (data.billingAddressId && authorization) { - return this.customersService.getAddress({ id: data.billingAddressId }, authorization).pipe( - map((billingAddress) => { - if (!billingAddress) { - throw new NotFoundException(`Address with ID ${data.billingAddressId} not found`); - } - return mapAddressToMedusa(billingAddress.address); - }), - ); - } - - if (data.billingAddress) { - return of(mapAddressToMedusa(data.billingAddress)); - } - - return of(null); - }packages/framework/src/modules/carts/carts.request.ts (1)
1-115: Noclass-validatordecorators on DTO classes — runtime validation is not enforced by NestJS.All DTO classes in the framework (carts, customers, articles, products, etc.) rely on TypeScript's
!non-null assertions for required fields, which are compile-time only. Without decorators like@IsString(),@IsNotEmpty(), etc., NestJS'sValidationPipewon't validate or reject malformed requests. Additionally,class-validatoris not installed as a dependency—adding decorators to this file alone would require also installingclass-validatorandclass-transformerpackages. This approach is consistent across all request DTOs in the framework; if validation enforcement is desired, consider installing the necessary packages and adding decorators systematically, or clarify that validation is intentionally handled elsewhere.
packages/integrations/medusajs/src/modules/carts/carts.mapper.ts
Outdated
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/carts/carts.mapper.ts
Outdated
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/carts/carts.service.ts
Outdated
Show resolved
Hide resolved
packages/integrations/mocked/src/modules/checkout/checkout.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/integrations/mocked/src/modules/orders/orders.mapper.ts`:
- Around line 507-558: In mapOrderFromCart, the guest email on the Cart
(cart.email) is being ignored in favor of the optional email parameter; update
the function to set the returned order's email to the provided email if present,
otherwise fall back to cart.email (e.g., email ?? cart.email) so guest checkout
emails from cart are preserved; locate the email assignment in the returned
object (currently "email,") and replace it with the fallback expression,
ensuring typing/nullability is respected.
🧹 Nitpick comments (2)
.changeset/silent-bees-play.md (1)
5-5: Consider adding more detail to the changeset description.The current description is functional but could be enhanced to help consumers better understand the scope. Consider mentioning that this introduces new integration modules with services, controllers, and data models.
Example:
-Mock integrations updated with carts/customers/payments/checkout mocks and enhanced product/order handling. +Mock integrations updated with new modules for carts, customers, payments, and checkout, including mock services, controllers, and data models. Enhanced product and order handling..changeset/lucky-sides-battle.md (1)
5-9: Consider enriching the changeset description with implementation details.The changeset provides a good high-level summary but could be more informative for users reviewing the changelog. Consider adding:
- Integration support: Mention that this includes Medusa.js integration (per PR title) and mocked implementations
- Module structure: Note that this adds four new integration modules (Carts, Checkout, Customers, Payments)
- Configuration: Indicate if any new configuration is required in
AppConfig- Framework vs Integration: Clarify that this includes both framework abstractions and concrete integrations
This is especially important since the PR description is currently a placeholder.
📝 Example enhanced description
Added the normalized data model for a cart/checkout system with full CRUD operations for items and promotions: +- New integration modules: Carts, Checkout, Customers, and Payments +- Medusa.js integration and mocked implementation - Checkout flow supporting address, shipping, and payment setup - Customer address management for authenticated users - Payment provider integration and session handling +- Framework abstractions with REST controllers and services
…module - Updated models, mappers, and service logic to use `sku` for identifying product variants. - Adjusted tests and mocked data mappings to align with the new `sku` field. - Revised documentation to reflect the parameter changes and improve consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/integrations/mocked/src/modules/orders/orders.mapper.ts (1)
6-106:⚠️ Potential issue | 🟡 MinorAll mock products share the identical SKU
'ABC-12345-S-BL'.Every entry in
PRODUCT_DATAuses the sameskuvalue, which undermines any testing that relies on distinguishing products by SKU. If the intent of the refactor was to useskuas the primary identifier (per the commit message), the mock data should have unique SKUs.
🤖 Fix all issues with AI agents
In `@packages/integrations/mocked/src/modules/carts/carts.mapper.ts`:
- Around line 243-248: The call to mapProductBySku in addCartItem is missing a
locale, causing items to always use MOCK_PRODUCTS_EN; update addCartItem to
derive the correct locale (either from the incoming AddCartItemBody if present
or from the cart's regionId) and pass it into mapProductBySku (e.g.,
mapProductBySku(data.sku, locale)); ensure you adjust any function signature or
overloads if needed and update all call sites so product
names/descriptions/prices come from the correct locale-aware mock data instead
of always defaulting to EN.
- Around line 56-58: buildCartItemFromProduct currently creates item IDs using
the cart.items.data.length-derived itemIndex (e.g.,
`ITEM-${itemIndex.toString().padStart(3,'0')}`), which can collide after
removals; change the ID generation in buildCartItemFromProduct (and the similar
occurrence around lines referenced 264-267) to use a monotonic counter or
crypto.randomUUID() (matching how cart IDs are created) instead of the cart
length so newly added items cannot reuse existing IDs; update any tests or mocks
that assert the old format if necessary.
🧹 Nitpick comments (5)
packages/integrations/mocked/src/modules/products/products.mapper.ts (1)
22-35: Consider extracting the repeated locale-source resolution into a shared helper.The locale → source selection block is now duplicated four times in this file (
mapProduct,mapProductBySku,mapProducts,mapRelatedProducts). A small helper likegetProductsSource(locale?: string)would eliminate the repetition.♻️ Suggested helper
+const getProductsSource = (locale?: string): Products.Model.Product[] => { + if (locale === 'pl') return MOCK_PRODUCTS_PL; + if (locale === 'de') return MOCK_PRODUCTS_DE; + return MOCK_PRODUCTS_EN; +}; + export const mapProductBySku = (sku: string, locale?: string): Products.Model.Product => { - let productsSource = MOCK_PRODUCTS_EN; - if (locale === 'pl') { - productsSource = MOCK_PRODUCTS_PL; - } else if (locale === 'de') { - productsSource = MOCK_PRODUCTS_DE; - } - + const productsSource = getProductsSource(locale); const product = productsSource.find((product) => product.sku === sku);packages/integrations/mocked/src/modules/carts/carts.mapper.ts (3)
7-17: No runtime validation onstoredshape — silent type coercion.
mapPaymentMethodFromMetadatacastsstored.id,stored.name, etc. asstringwithout checking they exist or are actually strings. If metadata has a malformedpaymentMethodobject (e.g., missingid), this will silently produce aPaymentMethodwithundefinedfields cast asstring. Acceptable for a mock, but a guard on required fields (id,name) would make debugging easier.
169-219:updateCartsilently drops an invalidpaymentMethodIdwith no feedback.On line 197-198, if
data.paymentMethodIdis provided butgetPaymentMethodDisplayreturnsundefined(unknown provider), the result falls through tomapPaymentMethodFromMetadata(mergedMetadata) ?? cart.paymentMethod, silently keeping the old payment method. This could confuse callers who expect the update to either succeed or error. Consider returningundefinedor throwing to signal the invalid ID — though for a mock this may be acceptable.
332-357:applyPromotionreturnsundefinedfor an unknown code — same as "cart not found".Both "cart not found" (line 337) and "unknown promo code" (line 341) return
undefined, making it impossible for the caller to distinguish the two failure modes. Returning a distinct error or the cart unchanged (with an error flag) for an invalid code would improve debuggability.packages/integrations/medusajs/src/modules/carts/carts.service.ts (1)
492-512: Remove unusedresolveBillingAddressmethodThe
resolveBillingAddressprivate method (lines 492–512) is never called. The billing address resolution logic is fully duplicated inline inupdateCartAddresses(lines 373–385). Delete this unused method.
- Updated request parameters across cart-related models to include `locale` field. - Fixed incorrect HTTP method in promotion removal logic. - Updated checkout service to accept dynamic `returnUrl` and `cancelUrl` values.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts`:
- Around line 155-160: The ownership check inside the switchMap callback
(condition: cart.customerId && authorization && cart.customerId !== customerId)
is throwing a misleading BadRequestException with message 'Variant ID is
required for Medusa carts'; change this to throw a proper authorization error
(use ForbiddenException or UnauthorizedException) and update the message to
indicate the cart belongs to a different customer (e.g., 'Cart does not belong
to the authenticated customer' or similar) in the throwError call so
mapCart/authorization logic reports correct ownership failure.
In `@packages/integrations/medusajs/src/modules/checkout/checkout.mapper.ts`:
- Around line 8-11: The _paymentSession parameter on mapCheckoutSummary is
declared but never used; either implement mapping logic to include payment
session details (e.g., extract payment method id/type from the
Payments.Model.PaymentSession and populate the corresponding field on the
returned Checkout.Model.CheckoutSummary, with null checks) so
mapCheckoutSummary(cart, session) actually uses the session, or explicitly
document the intentional unused parameter by adding a short TODO comment inside
mapCheckoutSummary noting that _paymentSession is reserved for future use;
update the function body accordingly (referencing mapCheckoutSummary and the
_paymentSession parameter).
- Around line 86-101: The function mapShippingOption declares an unused
parameter _defaultCurrency and currently throws BadRequestException when
calculatedPrice?.currency_code is missing; either remove _defaultCurrency (and
update callers) if strict behavior is intended, or implement a fallback by
replacing the currency lookup with something like const currencyCode =
(calculatedPrice?.currency_code ?? _defaultCurrency)?.toUpperCase() and then use
parseCurrency(currencyCode) (and avoid throwing when fallback provides a value);
modify mapShippingOption and any call sites accordingly and ensure
BadRequestException is only thrown if both currency_code and _defaultCurrency
are absent.
In `@packages/integrations/medusajs/src/modules/checkout/checkout.service.ts`:
- Around line 295-347: completeCheckout currently calls setPayment
unconditionally which will break when data.paymentProviderId is missing; add the
same guard pattern used for shippingMethodId so that setPayment is only invoked
when data.paymentProviderId is present (otherwise return of(null) or noop in the
switchMap). Update the pipeline in completeCheckout to check
data.paymentProviderId before calling this.setPayment and ensure the payload
passed to setPayment still uses data.paymentProviderId, returnUrl, cancelUrl,
and metadata when present.
- Around line 161-192: In placeOrder, after the existing
shipping/billing/shippingMethod checks add a pre-check for payment by verifying
the cart has a configured payment method (e.g., cart.paymentMethod or
cart.paymentMethods as appropriate) and return throwError(() => new
BadRequestException('Payment method is required')) if missing; update the
control flow so the early email-update path still calls
completeCartAndCreateOrder(params.cartId, email, authorization) only when the
payment check passes. Ensure you reference placeOrder, cartsService.getCart, and
completeCartAndCreateOrder when locating where to insert the check.
In `@packages/integrations/medusajs/src/modules/resources/resources.mapper.ts`:
- Around line 92-93: The mapAddress function's parameter type is too
narrow—change the signature of mapAddress(address: AddressDTOWithNames) to
accept address: AddressDTOWithNames | null | undefined (or make
AddressDTOWithNames nullable) so it matches actual call sites that pass
asset.address (which can be null); update the function declaration for
mapAddress and any exported types/usages that rely on it so TypeScript no longer
errors while keeping the existing runtime guard if (!address) return undefined.
In `@packages/integrations/medusajs/src/utils/price.ts`:
- Around line 8-10: The current validation in utils/price.ts checks value for
undefined/null and type number but misses NaN and Infinity, so extend the guard
around the variable `value` (the same check that throws `new
Error(\`\${context}: price value is missing or invalid\`)`) to also reject
Number.isNaN(value) and !Number.isFinite(value); update the validation in the
function that uses `value` so it throws the same error for NaN and ±Infinity,
ensuring only finite numeric prices are accepted.
In `@packages/integrations/mocked/src/modules/carts/carts.service.ts`:
- Around line 170-183: Replace the incorrect NotFoundException being thrown when
currency is missing with a BadRequestException to reflect a validation error:
locate the currency-required checks inside carts.service.ts (the branches that
call createCart) and change the throws that read NotFoundException('Currency is
required when creating a new cart') to throw new BadRequestException('Currency
is required when creating a new cart') so the error type matches input
validation failure.
🧹 Nitpick comments (12)
packages/integrations/medusajs/src/modules/carts/carts.service.ts (2)
46-71: Ownership check ingetCartthen re-checked in callers — consider a shared helper.The ownership verification pattern (check
cart.customerId, compare withauthService.getCustomerId) is duplicated ingetCart(lines 54-60),prepareCheckout(lines 291-298), andaddCartItem(lines 158-160). SinceprepareCheckoutalready callsgetCartwhich performs this check, the second check inprepareCheckoutis redundant.
136-140:deleteCartsilently succeeds without actually deleting.The Medusa Store API doesn't support cart deletion, which is understandable — but returning
of(undefined)with only a debug log means callers believe the cart was deleted when it wasn't. Consider returningthrowError(() => new NotImplementedException(...))likegetCartListandgetCurrentCartfor consistency, or at minimum add a warning-level log.packages/integrations/mocked/src/modules/carts/carts.mapper.ts (1)
378-409: Tax and total calculation could yield floating-point drift on large carts.The rounding at line 402 (
Math.round(... * 100) / 100) is applied totaxTotalandtotalbut not tosubtotal. For carts with many items, the accumulated floating-point subtotal may carry precision artifacts. This is minor for a mock but worth noting.packages/integrations/mocked/src/modules/carts/carts.service.ts (1)
30-47: Inconsistent error handling — some methods throw synchronously, others usethrowError().Methods like
getCart(line 39-43) throw synchronously before the Observable is created, whileupdateCartAddresses(line 375-386) uses reactivethrowError(). In NestJS the sync throws work because the framework catches them, but mixing patterns within the same service is confusing and fragile if these methods are ever composed inside other Observables.packages/integrations/medusajs/src/utils/currency.ts (2)
3-3:VALID_CURRENCIESduplicates the frameworkCurrencytype union — consider deriving one from the other.The array
['USD', 'EUR', 'GBP', 'PLN']must be kept in sync withCurrency = 'USD' | 'EUR' | 'GBP' | 'PLN'inpackages/framework/src/utils/models/price.ts. If a currency is added to or removed from the type, this array silently diverges. You could use asatisfiesassertion to get compile-time enforcement:♻️ Suggested improvement
-const VALID_CURRENCIES: Models.Price.Currency[] = ['USD', 'EUR', 'GBP', 'PLN']; +const VALID_CURRENCIES = ['USD', 'EUR', 'GBP', 'PLN'] as const satisfies readonly Models.Price.Currency[];This way, adding a value not in
Currencycauses a compile error — though it won't catch a missing member. If exhaustive coverage matters, a record-based approach would close the gap entirely.
10-10: Consider whether a plainErroris the right exception type here.This throws a generic
Error, whichhandleHttpErrorwill wrap as a 500. If an invalid currency originates from client input, aBadRequestExceptionwould be more appropriate (as done incheckout.mapper.ts). If it's always a server-side data integrity issue, the currentErroris fine. Just flagging the inconsistency across the codebase.packages/integrations/medusajs/src/modules/resources/resources.mapper.ts (2)
80-85: Passing{} as Products.Model.Productcreates a product with no valid fields.When mapping assets within a service instance, the product is an empty object cast to
Products.Model.Product. If any downstream consumer accessesproduct.name,product.price, etc., it will getundefinedat runtime despite the type system saying otherwise. Consider using a dedicated "unknown product" sentinel or making the product field optional on theAssetmodel.
108-126: Silent defaults for unrecognized contract status and payment period — acceptable but worth documenting.
mapContractStatusdefaults to'ACTIVE'andmapPaymentPerioddefaults to'ONE_TIME'for unrecognized input. This is a reasonable approach to avoid runtime errors, but it can mask data quality issues. Alogger.warncall for unrecognized values would make debugging easier in production without breaking the flow.packages/integrations/medusajs/src/modules/products/products.service.ts (1)
81-86: TheArray.isArraycheck on line 84 is unreachable.If
!product?.variants?.lengthon line 81 is falsy (i.e., we pass through), thenproduct.variantsexists and has a truthy.length, which means it's already an array-like with elements. The!Array.isArray(product.variants)check on line 84 can never be true in practice after line 81 passes. Consider removing it to reduce dead code.packages/integrations/medusajs/src/modules/checkout/checkout.service.ts (1)
87-135: Payment method type is hardcoded to'OTHER'.Line 124 always sets
type: 'OTHER'regardless of the actual payment provider. This means the cart's payment method metadata won't reflect the real payment type (e.g., credit card, bank transfer). If downstream code or UI relies on this type for display or logic, it'll always see'OTHER'.packages/integrations/medusajs/src/modules/orders/orders.mapper.ts (2)
114-120:mapPriceparameter type saysnumberbut the function handlesundefined.The runtime check
typeof value === 'undefined'on line 115 is correct defensively, but the function signaturevalue: numberdoesn't reflect this. Call sites may pass potentially-undefined values from Medusa API responses. Aligning the type with actual usage improves type safety:♻️ Proposed fix
-const mapPrice = (value: number, currency: Models.Price.Currency): Models.Price.Price | undefined => { +const mapPrice = (value: number | undefined, currency: Models.Price.Currency): Models.Price.Price | undefined => {
84-99:address_2is mapped to bothstreetNumberandapartment.Lines 93 and 94 both derive from
address.address_2, so they'll always have identical values. This is presumably an approximation since Medusa'sAddressDTOdoesn't distinguish the two. No action required if intentional, but a brief comment would clarify the design decision for future maintainers.
packages/integrations/medusajs/src/modules/resources/resources.mapper.ts
Outdated
Show resolved
Hide resolved
packages/integrations/mocked/src/modules/carts/carts.service.ts
Outdated
Show resolved
Hide resolved
- Added stricter validation for payment and shipping methods in the checkout service. - Refactored currency handling to use default currency when missing. - Standardized import paths for `handleHttpError` utility across modules.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/integrations/medusajs/src/modules/orders/orders.service.ts (1)
29-31:⚠️ Potential issue | 🟡 MinorDuplicate
+tax_totalin field list.
+tax_totalappears twice inadditionalOrderListFields. This is likely a copy-paste typo — one occurrence was probably meant to be a different field, or it should simply be removed.- private readonly additionalOrderListFields = - '+total,+subtotal,+tax_total,+discount_total,+shipping_total,+shipping_subtotal,+tax_total,+items.product.*'; + private readonly additionalOrderListFields = + '+total,+subtotal,+tax_total,+discount_total,+shipping_total,+shipping_subtotal,+items.product.*';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/orders/orders.service.ts` around lines 29 - 31, The string constant additionalOrderListFields in OrdersService contains a duplicate "+tax_total"; remove the repeated "+tax_total" (or replace it with the intended field if you know which one was meant) so the field list contains each field only once (leave additionalOrderDetailsFields as-is); update the value of additionalOrderListFields in orders.service.ts accordingly.
🧹 Nitpick comments (8)
packages/integrations/medusajs/src/modules/resources/resources.mapper.ts (1)
108-126: Silent defaults may mask upstream data issues.
mapContractStatusdefaults to'ACTIVE'andmapPaymentPerioddefaults to'ONE_TIME'for unrecognized values. This silently hides bad data from Medusa. Consider logging a warning when the fallback is used so data mismatches are observable.♻️ Example: add a warning log
function mapContractStatus(status: string | undefined): Resources.Model.ContractStatus { const normalized = (status ?? '').toUpperCase(); if (VALID_CONTRACT_STATUSES.includes(normalized as Resources.Model.ContractStatus)) { return normalized as Resources.Model.ContractStatus; } + // TODO: inject logger or use console.warn for visibility + console.warn(`Unknown contract status "${status}", defaulting to ACTIVE`); return 'ACTIVE'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/resources/resources.mapper.ts` around lines 108 - 126, Both mapContractStatus and mapPaymentPeriod silently return defaults ('ACTIVE' and 'ONE_TIME') for unknown inputs; update these functions (mapContractStatus, mapPaymentPeriod) to log a warning when the normalized value is not in VALID_CONTRACT_STATUSES or VALID_PAYMENT_PERIODS before returning the default. Use the existing logger available in this module (or import the shared logger) and include the original input and the chosen fallback in the warning message so upstream data issues are observable while preserving the current fallback behavior.packages/integrations/mocked/src/modules/carts/carts.service.ts (2)
37-45: Repeated authorization/ownership guard could be extracted into a helper.The same three-step pattern (check
customerId→ requireauthorization→ verify ownership) is duplicated across ~10 methods. A small private helper would reduce noise and ensure consistent error messages:private assertCartOwnership(cart: Carts.Model.Cart, authorization: string | undefined): void { if (!cart.customerId) return; if (!authorization) { throw new UnauthorizedException('Authentication required to access this cart'); } const customerId = this.authService.getCustomerId(authorization); if (cart.customerId !== customerId) { throw new UnauthorizedException('Unauthorized to access this cart'); } }This is a mock service so the duplication is tolerable, but the helper would also make it trivial to keep the checks consistent if the logic evolves.
Also applies to: 87-96, 113-122, 149-157, 215-224, 244-253, 274-283, 303-312, 348-357, 468-477
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/mocked/src/modules/carts/carts.service.ts` around lines 37 - 45, Extract the repeated three-step ownership guard into a private helper method (e.g., private assertCartOwnership(cart: Carts.Model.Cart, authorization?: string): void) inside the Carts service class; the helper should return early if cart.customerId is falsy, throw UnauthorizedException('Authentication required to access this cart') when authorization is missing, then call this.authService.getCustomerId(authorization) and throw UnauthorizedException('Unauthorized to access this cart') if the IDs mismatch. Replace the duplicated blocks that perform these checks (the blocks using cart?.customerId, authorization, and this.authService.getCustomerId) in methods that currently contain them (the occurrences around lines shown in the review) by invoking assertCartOwnership(cart, authorization) to ensure consistent behavior.
367-455: Inconsistent error-handling style:throwError()vs synchronousthrow.
updateCartAddressesuses RxJSthrowError()to emit errors (lines 375, 381, 385, 394, 449), while every other method in this class uses synchronousthrow. Both work under NestJS's exception filter, but mixing the two patterns in one service is confusing and makes error behavior harder to reason about during testing or if the execution context changes (e.g., interceptors that catch only Observable errors).Pick one style and apply it consistently. Since the rest of the class (and the simpler mental model) favors synchronous
throw, consider convertingupdateCartAddressesto match:♻️ Suggested diff (partial — early guards)
updateCartAddresses( params: Carts.Request.UpdateCartAddressesParams, data: Carts.Request.UpdateCartAddressesBody, authorization: string | undefined, ): Observable<Carts.Model.Cart> { const existingCart = mapCart({ id: params.cartId }); if (!existingCart) { - return throwError(() => new NotFoundException('Cart not found')); + throw new NotFoundException('Cart not found'); } // Customer carts require authorization if (existingCart.customerId) { if (!authorization) { - return throwError(() => new UnauthorizedException('Authentication required to update this cart')); + throw new UnauthorizedException('Authentication required to update this cart'); } const customerId = this.authService.getCustomerId(authorization); if (existingCart.customerId !== customerId) { - return throwError(() => new UnauthorizedException('Unauthorized to update this cart')); + throw new UnauthorizedException('Unauthorized to update this cart'); } } // Guest: require inline addresses when only IDs provided const isGuest = !authorization; const hasAddressIdsOnly = (data.shippingAddressId && !data.shippingAddress) || (data.billingAddressId && !data.billingAddress); if (isGuest && hasAddressIdsOnly) { - return throwError(() => new BadRequestException('Inline addresses required for guest checkout')); + throw new BadRequestException('Inline addresses required for guest checkout'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/mocked/src/modules/carts/carts.service.ts` around lines 367 - 455, The method updateCartAddresses mixes RxJS throwError() with synchronous throws; make its error handling consistent with the rest of the class by using synchronous throws. Replace all occurrences of "return throwError(() => new ...)" in updateCartAddresses with "throw new ..." for the early guards (cart not found, UnauthorizedException, BadRequestException) and within resolveAddresses$ change resolveAddress to synchronously throw new NotFoundException when addr is missing (and keep successful branches returning of(...)); ensure callers (shipping$, billing$) still produce Observables via this.customersService.getAddress(...).pipe(switchMap(...)) but with the switchMap handler throwing synchronously on missing address so the Observable will error consistently.packages/integrations/mocked/src/modules/checkout/checkout.service.ts (2)
172-173: Order is persisted before the asyncgetSessioncompletes — side-effect ordering.
MOCKED_ORDERS.push(order)executes synchronously, but the subsequentgetSessioncall (line 176–178) could fail. If it does, the order remains in the mock store with no corresponding response emitted. Consider moving the push inside themapcallback so it only happens on success.Proposed fix
- const order = mapOrderFromCart(cart, email); - MOCKED_ORDERS.push(order); - - // Get payment session for redirect URL - return this.paymentsService - .getSession({ id: paymentSessionId }, authorization) - .pipe(map((session) => mapPlaceOrderResponse(order, session))); + const order = mapOrderFromCart(cart, email); + + // Get payment session for redirect URL + return this.paymentsService + .getSession({ id: paymentSessionId }, authorization) + .pipe( + map((session) => { + MOCKED_ORDERS.push(order); + return mapPlaceOrderResponse(order, session); + }), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/mocked/src/modules/checkout/checkout.service.ts` around lines 172 - 173, The code pushes the mapped order into MOCKED_ORDERS before the asynchronous getSession completes, which can leave a persisted order if getSession fails; change the flow so that the order is only added to MOCKED_ORDERS after getSession succeeds — i.e., call mapOrderFromCart(cart, email) to produce the order but defer MOCKED_ORDERS.push(order) into the getSession success callback/then block (or inside the map callback that emits the response) so the push happens only on successful session retrieval; reference mapOrderFromCart, MOCKED_ORDERS and getSession to locate and update the logic.
191-243: CumulativeresponseDelayfrom chained sub-methods.Each of the four sub-methods (
setAddresses,setShippingMethod,setPayment,placeOrder) applies its ownresponseDelay(). When orchestrated throughcompleteCheckout, the delays stack (up to 4×). If this noticeably slows down integration tests or dev workflows, consider adding a parameter to bypass the delay when called internally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/mocked/src/modules/checkout/checkout.service.ts` around lines 191 - 243, completeCheckout currently chains setAddresses, setShippingMethod, setPayment, and placeOrder which each apply responseDelay(), causing cumulative delays; add an optional boolean parameter (e.g., skipResponseDelay or internalCall) to completeCheckout and propagate it into the four sub-method calls so they can skip their responseDelay when invoked from completeCheckout (update signatures for setAddresses, setShippingMethod, setPayment, placeOrder to accept the flag and conditionally bypass responseDelay inside each method), and default the new parameter to false so external callers still get delays.packages/integrations/medusajs/src/modules/carts/carts.service.ts (2)
54-60: Synchronousthrowinsidemapoperator at ownership check.Line 59 uses
throw new UnauthorizedException(...)inside amapcallback. While this technically works (RxJS converts it to an error notification), the outercatchErrorat line 64 delegates tohandleHttpError, which re-throwsHttpExceptioninstances. The error will propagate correctly here, but it's worth noting this is inconsistent with thethrowError(...)pattern used elsewhere (e.g.,prepareCheckoutline 294).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts` around lines 54 - 60, Replace the synchronous "throw new UnauthorizedException(...)" inside the RxJS map callback with the project's consistent observable error pattern (use throwError or an equivalent Observable error) so the stream emits an error rather than throwing synchronously; locate the ownership check in the carts.service.ts map where cart.customerId is compared to this.authService.getCustomerId(authorization) and replace the throw with the observable error creation so it flows into the existing catchError/handleHttpError logic (matching how prepareCheckout uses throwError).
490-510: Remove unusedresolveBillingAddressmethod.This private method duplicates the billing address resolution logic already implemented inline in
updateCartAddresses(lines 371-383) and is never called anywhere in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts` around lines 490 - 510, Delete the unused private method resolveBillingAddress from the CartsService class in carts.service.ts; it duplicates logic already implemented inline in updateCartAddresses and is never referenced elsewhere—remove the entire resolveBillingAddress method (its signature and body) and any now-unreferenced local imports only if they become unused after removal.packages/integrations/medusajs/src/modules/checkout/checkout.mapper.spec.ts (1)
72-132: Each validation test invokes the mapper twice — consider a single assertion.Each "missing field" test calls
mapCheckoutSummary(cart)twice: once to assert the exception type and once for the message. Vitest's.toThrow()can match both in a single call sinceBadRequestExceptionembeds the message. This is a minor readability nit — current form is also fine.Example consolidation
it('should throw BadRequestException when shippingAddress is missing', () => { const cart = minimalCartWithAllFields(); cart.shippingAddress = undefined; - expect(() => mapCheckoutSummary(cart)).toThrow(BadRequestException); - expect(() => mapCheckoutSummary(cart)).toThrow('Shipping address is required'); + expect(() => mapCheckoutSummary(cart)).toThrow( + expect.objectContaining({ + message: expect.stringContaining('Shipping address is required'), + }), + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/checkout/checkout.mapper.spec.ts` around lines 72 - 132, Tests call mapCheckoutSummary(cart) twice to assert type and message; replace each pair of expect(...) calls with a single assertion that checks the thrown BadRequestException including its message (e.g. assert toThrow with an instance/new BadRequestException('...') or equivalent) so each case (shippingAddress, billingAddress, shippingMethod, paymentMethod, subtotal, shippingTotal, taxTotal, discountTotal, total) uses one expect for mapCheckoutSummary, referencing mapCheckoutSummary, minimalCartWithAllFields, and BadRequestException to locate the assertions to update.
🤖 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/integrations/medusajs/src/modules/customers/customers.service.ts`:
- Around line 118-135: The code inside the switchMap handling
HttpTypes.StoreCustomerResponse is fragile because it assumes the created
address is addresses[addresses.length - 1]; update the logic in that switchMap
(the block using variables customer, addresses, createdAddress and calling
mapCustomerAddress and setDefaultAddress) to reliably identify the newly created
address instead of using the last element — e.g., compare the response.addresses
list against the original customer's addresses (or use an identifier returned by
the API) to find the new entry, then pass that found address to
mapCustomerAddress(customerId) and to this.setDefaultAddress when data.isDefault
is true.
- Around line 237-253: The update call in setDefaultAddress only sets
is_default_shipping, but the mapper (mapCustomerAddress) treats an address as
default if is_default_shipping || is_default_billing; change the payload passed
to this.sdk.store.customer.updateAddress (the
Partial<HttpTypes.StoreUpdateCustomerAddress> object) to include
is_default_billing: true alongside is_default_shipping: true so the address
becomes the overall default; keep the rest of the call (authorization headers
via medusaJsService.getStoreApiHeaders) unchanged and ensure the updatedAddress
lookup/NotFoundException and mapCustomerAddress usage remain the same.
- Around line 35-43: The methods getAddresses, getAddress, createAddress,
updateAddress, deleteAddress, and setDefaultAddress currently throw
UnauthorizedException synchronously which breaks the Observable contract; change
each synchronous throw to return an Observable error using rxjs throwError
(e.g., return throwError(() => new UnauthorizedException('...'))) so callers can
catch via pipe(catchError(...)); also ensure you import throwError from 'rxjs'
and replace both checks (missing authorization and missing customerId) in each
method accordingly.
In `@packages/integrations/medusajs/src/modules/payments/payments.service.ts`:
- Around line 116-130: The getSession method currently always returns undefined
which causes getCheckoutSummary to silently omit payment session data; update
getSession (in payments.service.ts) to either throw a NotImplementedException or
at minimum log a clear warning with the missing capability and the provided
session ID, and then update callers (notably getCheckoutSummary) to guard on
paymentSessionId by catching the exception or handling the
logged-not-implemented case so the omission is surfaced (e.g., surface an error,
fallback behavior, or explicit null) rather than silently degrading the checkout
summary.
- Around line 46-52: In getProviders, avoid synchronously throwing
BadRequestException; instead return an Observable error so callers can catch it
reactively: replace the synchronous throw in getProviders with returning
throwError(() => new BadRequestException('regionId is required to list payment
providers')) and ensure throwError is imported from 'rxjs' so the method keeps
its Observable<Payments.Model.PaymentProviders> contract.
In `@packages/integrations/medusajs/src/modules/products/products.service.ts`:
- Around line 56-70: getProductList currently builds params without mapping
GetProductListQuery.sort or .type and relies on mapProducts to filter by
category after fetching, causing full-table fetches; update the params object
passed to this.sdk.store.product.list inside getProductList to include the
Medusa equivalents (e.g., map query.sort -> params.order or appropriate SDK
filter key, add query.type as a filter param or tag filter, and if Medusa
supports it map query.category into the API filter instead of post-filtering in
mapProducts), remove client-side category filtering in mapProducts for this
path, and preserve locale only if the SDK supports it; reference getProductList,
Products.Request.GetProductListQuery, params, this.sdk.store.product.list, and
mapProducts when making the changes.
In `@packages/integrations/medusajs/src/modules/resources/resources.mapper.ts`:
- Line 77: The call to parseCurrency inside the map operator (currency:
parseCurrency(serviceInstance?.totals?.currency ?? defaultCurrency)) can throw
for invalid codes; change it to a safe variant that falls back instead of
throwing — either wrap parseCurrency(...) in a try/catch that returns
parseCurrency(defaultCurrency) or a safeCurrency value on error, or implement a
safeParseCurrency helper used here; ensure you update the map assignment
(currency: ...) to use the safe call so unexpected
serviceInstance?.totals?.currency values do not propagate an exception.
In `@packages/integrations/mocked/src/modules/carts/carts.service.ts`:
- Around line 344-346: The NotFoundException thrown when a lookup fails
currently exposes the user-supplied params.cartId in the message; change the
error message to a generic one (e.g., 'Cart not found') to match other not-found
errors and avoid leaking IDs—update the throw new NotFoundException(...) in the
block that checks if (!cart) (referencing the cart variable, params.cartId and
NotFoundException) to use the generic message.
In `@packages/integrations/mocked/src/modules/checkout/checkout.service.ts`:
- Around line 148-172: placeOrder currently validates addresses, shipping method
and payment session but misses validating cart items, causing
mapOrderFromCart(cart, ...) to throw a TypeError when cart.items or
cart.items.data is null/empty; update the switchMap in the placeOrder flow to
check that cart.items and Array.isArray(cart.items.data) &&
cart.items.data.length > 0 (or equivalent truthy item list) and throw a
BadRequestException like 'Cart must contain at least one item' before calling
mapOrderFromCart; reference the placeOrder/switchMap block and mapOrderFromCart
to locate where to add this guard.
---
Outside diff comments:
In `@packages/integrations/medusajs/src/modules/orders/orders.service.ts`:
- Around line 29-31: The string constant additionalOrderListFields in
OrdersService contains a duplicate "+tax_total"; remove the repeated
"+tax_total" (or replace it with the intended field if you know which one was
meant) so the field list contains each field only once (leave
additionalOrderDetailsFields as-is); update the value of
additionalOrderListFields in orders.service.ts accordingly.
---
Duplicate comments:
In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts`:
- Around line 447-449: The ternary assigning headers in addShippingMethod is
dead code because both branches call the same method; simplify by replacing the
ternary with a single call to
this.medusaJsService.getStoreApiHeaders(authorization) and use that result for
headers, removing the redundant conditional expression referencing headers and
medusaJsService.getStoreApiHeaders.
- Around line 142-145: In addCartItem replace the synchronous throw new
BadRequestException(...) used for the SKU validation with an Observable error
return (return throwError(() => new BadRequestException('SKU is required for
Medusa carts'))) so the method consistently returns an Observable error (like
the currency check uses); ensure you import throwError from 'rxjs' if it's not
already imported and keep the method signature addCartItem(data:
Carts.Request.AddCartItemBody, authorization?: string):
Observable<Carts.Model.Cart>.
In `@packages/integrations/medusajs/src/modules/payments/payments.service.ts`:
- Around line 163-171: The code is still passing a hardcoded empty cartId to
mapPaymentSession; update the payments update flow to supply a real cartId
instead of ''. Either extend the surrounding method (e.g., the service method
that calls this observable) to accept a cartId parameter and pass it through, or
extract the cart id from the response object (e.g.,
response.data.payment_session.cart_id or
response.data.payment_session.metadata.cartId) before calling mapPaymentSession;
alternatively pass undefined and ensure mapPaymentSession handles undefined
appropriately. Locate the call site that returns
mapPaymentSession(response.data.payment_session, cartId) and propagate a real
cartId value from the caller or from response fields rather than using ''.
In `@packages/integrations/medusajs/src/modules/products/products.service.ts`:
- Around line 77-104: The getProduct implementation correctly throws
NotFoundException for missing/invalid variants and uses handleHttpError and
mapProduct appropriately, so no code changes are required; leave the logic in
getProduct (Products.Service.getProduct / function getProduct), the
NotFoundException usages, the variant selection and variantWithProduct mapping
as-is and ensure tests cover the variant-not-found and invalid-variants paths to
validate 404 behavior.
In `@packages/integrations/medusajs/src/modules/resources/resources.mapper.ts`:
- Around line 89-92: The mapAddress parameter type must accept null as well as
undefined; update the signature for mapAddress (and/or the AddressDTOWithNames
usage) so the parameter is typed as AddressDTOWithNames | null | undefined (or
use the optional plus null form) to match call sites that pass null; ensure you
only adjust the type annotation (e.g., the mapAddress function parameter) so the
existing runtime guard (!address) continues to work.
In `@packages/integrations/mocked/src/modules/carts/carts.service.ts`:
- Around line 170-183: The missing-currency validation should consistently throw
BadRequestException (not NotFoundException) in both authenticated and guest
branches; update any remaining NotFoundException usages to BadRequestException
and ensure the error message remains 'Currency is required when creating a new
cart' where createCart is invoked so both branches (the authenticated branch
that checks data.currency before createCart and the guest branch) behave
identically.
In `@packages/integrations/mocked/src/modules/checkout/checkout.service.ts`:
- Line 52: The null-safety check for the cart items is inconsistent and can
throw if cart.items exists but cart.items.data is nullish; update the
conditional to use optional chaining (the same pattern used at line 30) so the
check reads for emptiness using cart.items?.data?.length (i.e., replace the
current if that references cart.items.data.length with the safe optional-chained
check inside the same method in CheckoutService/checkout handling code).
- Around line 13-17: Change the class declaration and constructor to extend the
abstract class instead of implementing it: replace "export class CheckoutService
implements Checkout.Service" with "export class CheckoutService extends
Checkout.Service", add a super() call in the constructor to invoke the abstract
base class initializer, and keep the injected dependencies (private readonly
cartsService: Carts.Service, private readonly paymentsService: Payments.Service)
passed through or used as needed; update any constructor signature or
initialization to match the abstract base class expectation so the class
properly inherits from Checkout.Service.
---
Nitpick comments:
In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts`:
- Around line 54-60: Replace the synchronous "throw new
UnauthorizedException(...)" inside the RxJS map callback with the project's
consistent observable error pattern (use throwError or an equivalent Observable
error) so the stream emits an error rather than throwing synchronously; locate
the ownership check in the carts.service.ts map where cart.customerId is
compared to this.authService.getCustomerId(authorization) and replace the throw
with the observable error creation so it flows into the existing
catchError/handleHttpError logic (matching how prepareCheckout uses throwError).
- Around line 490-510: Delete the unused private method resolveBillingAddress
from the CartsService class in carts.service.ts; it duplicates logic already
implemented inline in updateCartAddresses and is never referenced
elsewhere—remove the entire resolveBillingAddress method (its signature and
body) and any now-unreferenced local imports only if they become unused after
removal.
In `@packages/integrations/medusajs/src/modules/checkout/checkout.mapper.spec.ts`:
- Around line 72-132: Tests call mapCheckoutSummary(cart) twice to assert type
and message; replace each pair of expect(...) calls with a single assertion that
checks the thrown BadRequestException including its message (e.g. assert toThrow
with an instance/new BadRequestException('...') or equivalent) so each case
(shippingAddress, billingAddress, shippingMethod, paymentMethod, subtotal,
shippingTotal, taxTotal, discountTotal, total) uses one expect for
mapCheckoutSummary, referencing mapCheckoutSummary, minimalCartWithAllFields,
and BadRequestException to locate the assertions to update.
In `@packages/integrations/medusajs/src/modules/resources/resources.mapper.ts`:
- Around line 108-126: Both mapContractStatus and mapPaymentPeriod silently
return defaults ('ACTIVE' and 'ONE_TIME') for unknown inputs; update these
functions (mapContractStatus, mapPaymentPeriod) to log a warning when the
normalized value is not in VALID_CONTRACT_STATUSES or VALID_PAYMENT_PERIODS
before returning the default. Use the existing logger available in this module
(or import the shared logger) and include the original input and the chosen
fallback in the warning message so upstream data issues are observable while
preserving the current fallback behavior.
In `@packages/integrations/mocked/src/modules/carts/carts.service.ts`:
- Around line 37-45: Extract the repeated three-step ownership guard into a
private helper method (e.g., private assertCartOwnership(cart: Carts.Model.Cart,
authorization?: string): void) inside the Carts service class; the helper should
return early if cart.customerId is falsy, throw
UnauthorizedException('Authentication required to access this cart') when
authorization is missing, then call
this.authService.getCustomerId(authorization) and throw
UnauthorizedException('Unauthorized to access this cart') if the IDs mismatch.
Replace the duplicated blocks that perform these checks (the blocks using
cart?.customerId, authorization, and this.authService.getCustomerId) in methods
that currently contain them (the occurrences around lines shown in the review)
by invoking assertCartOwnership(cart, authorization) to ensure consistent
behavior.
- Around line 367-455: The method updateCartAddresses mixes RxJS throwError()
with synchronous throws; make its error handling consistent with the rest of the
class by using synchronous throws. Replace all occurrences of "return
throwError(() => new ...)" in updateCartAddresses with "throw new ..." for the
early guards (cart not found, UnauthorizedException, BadRequestException) and
within resolveAddresses$ change resolveAddress to synchronously throw new
NotFoundException when addr is missing (and keep successful branches returning
of(...)); ensure callers (shipping$, billing$) still produce Observables via
this.customersService.getAddress(...).pipe(switchMap(...)) but with the
switchMap handler throwing synchronously on missing address so the Observable
will error consistently.
In `@packages/integrations/mocked/src/modules/checkout/checkout.service.ts`:
- Around line 172-173: The code pushes the mapped order into MOCKED_ORDERS
before the asynchronous getSession completes, which can leave a persisted order
if getSession fails; change the flow so that the order is only added to
MOCKED_ORDERS after getSession succeeds — i.e., call mapOrderFromCart(cart,
email) to produce the order but defer MOCKED_ORDERS.push(order) into the
getSession success callback/then block (or inside the map callback that emits
the response) so the push happens only on successful session retrieval;
reference mapOrderFromCart, MOCKED_ORDERS and getSession to locate and update
the logic.
- Around line 191-243: completeCheckout currently chains setAddresses,
setShippingMethod, setPayment, and placeOrder which each apply responseDelay(),
causing cumulative delays; add an optional boolean parameter (e.g.,
skipResponseDelay or internalCall) to completeCheckout and propagate it into the
four sub-method calls so they can skip their responseDelay when invoked from
completeCheckout (update signatures for setAddresses, setShippingMethod,
setPayment, placeOrder to accept the flag and conditionally bypass responseDelay
inside each method), and default the new parameter to false so external callers
still get delays.
packages/integrations/medusajs/src/modules/customers/customers.service.ts
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/customers/customers.service.ts
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/customers/customers.service.ts
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/products/products.service.ts
Outdated
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/resources/resources.mapper.ts
Show resolved
Hide resolved
…ove error handling across modules
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/integrations/medusajs/src/modules/orders/orders.service.ts (2)
54-61:⚠️ Potential issue | 🟠 MajorSynchronous
throwinObservable-returning methods.Both
getOrder(line 60) andgetOrderList(line 89) throwUnauthorizedExceptionsynchronously instead of returningthrowError(...). This is the same reactive-contract issue present across several services in this PR.Proposed fix for getOrder
if (!authorization) { this.logger.debug('Authorization token not found'); - throw new UnauthorizedException('Unauthorized'); + return throwError(() => new UnauthorizedException('Unauthorized')); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/orders/orders.service.ts` around lines 54 - 61, getOrder and getOrderList are returning Observables but currently throw UnauthorizedException synchronously; change these to return an RxJS error Observable instead. Replace the synchronous throw in getOrder and getOrderList with returning throwError(() => new UnauthorizedException('Unauthorized')) (importing throwError from 'rxjs') and keep the existing this.logger.debug('Authorization token not found') call so the methods comply with the reactive contract.
29-30:⚠️ Potential issue | 🟡 MinorDuplicate
+tax_totalinadditionalOrderListFields.
+tax_totalappears twice in the field string. Won't break anything, but is likely a copy-paste leftover.Proposed fix
private readonly additionalOrderListFields = - '+total,+subtotal,+tax_total,+discount_total,+shipping_total,+shipping_subtotal,+tax_total,+items.product.*'; + '+total,+subtotal,+tax_total,+discount_total,+shipping_total,+shipping_subtotal,+items.product.*';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/orders/orders.service.ts` around lines 29 - 30, Remove the duplicate +tax_total from the additionalOrderListFields string in OrdersService so the field list is unique; locate the private readonly additionalOrderListFields declaration in orders.service.ts and edit the string to include each field once (e.g., remove the extra +tax_total occurrence).
🧹 Nitpick comments (6)
packages/integrations/mocked/src/modules/checkout/checkout.service.ts (1)
195-247:completeCheckoutaccumulatesresponseDelayfrom each sub-call — 4× simulated latency.Each of
setAddresses,setShippingMethod,setPayment, andplaceOrderindividually appliesresponseDelay(). SincecompleteCheckoutchains all four, the total delay stacks up. Consider whether this is intentional for the mock or ifcompleteCheckoutshould bypass the individual delays (e.g., by calling internal helpers without the delay wrapper, then applying a singleresponseDelay()at the end of the chain).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/mocked/src/modules/checkout/checkout.service.ts` around lines 195 - 247, completeCheckout currently chains setAddresses, setShippingMethod, setPayment, and placeOrder which each call responseDelay(), causing quadruple simulated latency; update completeCheckout to avoid stacking delays by invoking the internal, no-delay implementations (or add new private helpers e.g., setAddressesNoDelay, setShippingMethodNoDelay, setPaymentNoDelay, placeOrderNoDelay) or by suppressing responseDelay on those calls and then apply a single responseDelay() once at the end of the observable chain; ensure references remain to completeCheckout and the four sub-call methods so reviewers can find the changes.packages/integrations/medusajs/src/modules/carts/carts.service.ts (2)
284-309: Redundant ownership check —getCartalready verifies ownership.
prepareCheckoutdelegates tothis.getCart(...)(line 285) which throwsUnauthorizedExceptionon ownership mismatch (lines 55–61). The second ownership check at lines 292–298 is redundant.Not harmful, but if the ownership logic in
getCartchanges, these could silently diverge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts` around lines 284 - 309, The prepareCheckout method contains a redundant ownership check duplicate of the logic in getCart; remove the manual verification block that calls this.authService.getCustomerId and throws UnauthorizedException so prepareCheckout simply relies on getCart's ownership enforcement (keep the initial not-found and items validation). Update prepareCheckout to no longer reference authService for ownership to avoid divergence with getCart's behavior.
427-434: Extra round-trip: re-fetching cart after update instead of using the update response.
sdk.store.cart.updatealready returns the updated cart in its response, but line 428 discards it and callsgetCartagain. This adds an unnecessary API call. The same pattern appears inaddShippingMethod(line 474).Proposed fix for updateCartAddresses
- return from(this.sdk.store.cart.update(params.cartId, cartUpdate, {}, headers)).pipe( - switchMap(() => this.getCart({ id: params.cartId }, authorization)), - map((updatedCart) => { - if (!updatedCart) { - throw new NotFoundException(`Cart with ID ${params.cartId} not found`); - } - return updatedCart; - }), + return from(this.sdk.store.cart.update(params.cartId, cartUpdate, {}, headers)).pipe( + map((response: HttpTypes.StoreCartResponse) => + mapCart(response.cart, this.defaultCurrency), + ), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts` around lines 427 - 434, The update flow is making an unnecessary second API call by discarding the result of sdk.store.cart.update and calling getCart again; change the code in carts.service.ts so that the Observable returned by sdk.store.cart.update is used directly (map the response to the updated cart and throw NotFoundException if the update response is null) instead of switchMapping to getCart; apply the same fix to addShippingMethod where sdk.store.cart.update is currently followed by getCart—use the update response, preserve headers/params.cartId handling, and ensure null-checks still raise NotFoundException.packages/integrations/medusajs/src/modules/customers/customers.service.spec.ts (1)
156-194: Consider extracting the duplicatedcreatedAddressfixture.The
createdAddressobject (lines 158–173) is repeated almost identically in the next test (lines 198–213). Extracting it to a shared constant alongsideminimalAddresswould reduce duplication and make updates easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/customers/customers.service.spec.ts` around lines 156 - 194, The two tests in customers.service.spec.ts duplicate the createdAddress fixture; extract that object into a shared constant (e.g., CREATED_ADDRESS) near minimalAddress and reuse it in both specs to remove duplication; update the mock setup lines that use createdAddress and any assertions in the tests that reference the variable so they import/point to the new constant while leaving service.createAddress and mockSdk.store.customer.createAddress usage unchanged.packages/integrations/medusajs/src/modules/payments/payments.mapper.ts (2)
5-14:requiresRedirectheuristic is brittle but acceptable for now.The
includes('stripe') || includes('paypal')check (line 11) works for common providers but won't catch other redirect-based providers (e.g., Klarna, iDEAL). Consider a comment noting this limitation or making the list configurable if more providers are expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/payments/payments.mapper.ts` around lines 5 - 14, The requiresRedirect heuristic in mapPaymentProvider (function mapPaymentProvider) currently sets requiresRedirect using medusaProvider.id.includes('stripe') || medusaProvider.id.includes('paypal'), which is brittle and misses other redirect-based providers; update mapPaymentProvider to either (a) replace the inline includes check with a configurable list/Set (e.g., redirectProviders) that can be extended for providers like Klarna or iDEAL, or (b) at minimum add a clear comment above the requiresRedirect assignment documenting the limitation and listing known redirect providers, and ensure the symbol requiresRedirect is set from that list/Set so future providers can be added without changing code logic.
27-41:metadataduplicatesredirectUrlandclientSecret— minor redundancy.Line 39 passes the entire
session.dataasmetadata, which already includes theredirect_urlandclient_secretfields extracted on lines 36–37. Not a bug, but downstream consumers could see the same data in two places. Acceptable if the model contract expects raw provider data inmetadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/payments/payments.mapper.ts` around lines 27 - 41, The mapPaymentSession function populates redirectUrl and clientSecret from medusaSession.data but then assigns the full medusaSession.data to metadata, causing duplicate fields; update mapPaymentSession to set metadata to a copy of medusaSession.data with redirect_url and client_secret removed (or undefined) so downstream consumers don't see the same values twice—use a shallow clone/omit of medusaSession.data before assigning metadata while keeping id, cartId, providerId, status, redirectUrl, clientSecret and expiresAt logic unchanged.
🤖 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/integrations/medusajs/src/modules/carts/carts.mapper.ts`:
- Around line 149-169: The mapPromotions function currently hardcodes
type='PERCENTAGE' and value=0 which loses real discount data; update
mapPromotions to inspect the incoming HttpTypes.StorePromotion shape (e.g.,
fields like rule?.type, value, amount, discount_amount, percentage or similar)
and map those into Carts.Model.Promotion.type and .value (convert absolute
amounts to a numeric value and percentage rules to a percent value), falling
back to a safe sentinel (e.g., type='UNKNOWN' or null and value=null) if the
real type/value cannot be determined; ensure you still populate id, code, name
and appliedTo consistently and keep the function signature mapPromotions(cart:
HttpTypes.StoreCart): Carts.Model.Promotion[] | undefined.
In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts`:
- Around line 137-141: The deleteCart method currently returns a successful
observable without doing anything; change it to explicitly indicate the
operation is unsupported by throwing the same NotImplementedException used by
getCartList/getCurrentCart: update deleteCart(_params:
Carts.Request.DeleteCartParams, _authorization?: string) to log the debug
message and then throw or return an Observable that errors with
NotImplementedException (instead of of(undefined)), referencing the deleteCart
function and the NotImplementedException used elsewhere so callers see the
unsupported-operation error.
- Around line 491-511: The private method resolveBillingAddress duplicates logic
already in updateCartAddresses and is unused; remove the dead code by deleting
the resolveBillingAddress method (including its signature and body) or,
alternatively, refactor updateCartAddresses to call resolveBillingAddress
instead of inlining the logic—if choosing refactor, replace the billing-address
branch in updateCartAddresses with a call to this.resolveBillingAddress(data,
authorization) and ensure it uses customersService.getAddress and
mapAddressToMedusa consistently and returns an
Observable<HttpTypes.StoreAddAddress | null>.
In
`@packages/integrations/medusajs/src/modules/customers/customers.service.spec.ts`:
- Around line 71-81: The tests for getAddresses must be updated to handle that
CustomersService.getAddresses returns an Observable (uses throwError) rather
than throwing synchronously; replace the synchronous expect(() =>
service.getAddresses(...)).toThrow(...) assertions with async assertions that
await firstValueFrom(service.getAddresses(...)) and use Jest's await
expect(...).rejects.toThrow(...) (or equivalent) to assert UnauthorizedException
and message; ensure you import/require firstValueFrom and keep the existing
mockAuthService.getCustomerId behavior (mockReturnValue/undefined) for the
second case so the Observable error path is exercised.
In `@packages/integrations/medusajs/src/modules/payments/payments.service.ts`:
- Around line 71-76: The catchError in getProviders currently swallows all
errors by logging and returning of(mapPaymentProviders([])); update it to
inspect the error (e.g., error.status or error.response?.status) inside the
catchError callback and re-throw authentication/authorization errors (401 and
403) using throwError(() => error) so callers can handle them, while only
falling back to returning of(mapPaymentProviders([])) for expected non-auth
cases (e.g., 404 or network/unavailable endpoints); keep the this.logger.warn
call but include the error details when falling back.
- Around line 129-156: The three methods getSession, updateSession, and
cancelSession currently synchronously throw NotImplementedException which breaks
the Observable contract; replace the synchronous throws with returning an RxJS
error Observable (e.g., return throwError(() => new NotImplementedException()))
so callers can catch via .pipe(catchError(...)), and add/import throwError from
'rxjs' at the top of the file; update each method (getSession, updateSession,
cancelSession) to return throwError(() => new NotImplementedException()) instead
of throw new NotImplementedException().
---
Outside diff comments:
In `@packages/integrations/medusajs/src/modules/orders/orders.service.ts`:
- Around line 54-61: getOrder and getOrderList are returning Observables but
currently throw UnauthorizedException synchronously; change these to return an
RxJS error Observable instead. Replace the synchronous throw in getOrder and
getOrderList with returning throwError(() => new
UnauthorizedException('Unauthorized')) (importing throwError from 'rxjs') and
keep the existing this.logger.debug('Authorization token not found') call so the
methods comply with the reactive contract.
- Around line 29-30: Remove the duplicate +tax_total from the
additionalOrderListFields string in OrdersService so the field list is unique;
locate the private readonly additionalOrderListFields declaration in
orders.service.ts and edit the string to include each field once (e.g., remove
the extra +tax_total occurrence).
---
Duplicate comments:
In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts`:
- Around line 143-146: The addCartItem method currently throws a
BadRequestException synchronously which violates its
Observable<Carts.Model.Cart> contract; change the synchronous throw in
addCartItem to return an Observable error instead (use throwError with a
function that creates the BadRequestException) so callers can catch it via RxJS
operators; update the branch that checks data.sku in addCartItem to return
throwError(() => new BadRequestException(...)) rather than using throw.
- Around line 448-450: The ternary creating headers is dead code because both
branches call the same method; replace the ternary with a single call: set const
headers = this.medusaJsService.getStoreApiHeaders(authorization) (remove the
conditional). Locate the usage around the headers variable in carts.service.ts
and ensure no downstream logic expected different branches; if there was intent
to call a different method when authorization is falsy, implement that distinct
call instead of the identical ternary.
In `@packages/integrations/medusajs/src/modules/customers/customers.service.ts`:
- Around line 71-78: Several Observable-returning methods (getAddress,
createAddress, updateAddress, deleteAddress, setDefaultAddress) still throw
synchronously instead of returning an RxJS error Observable; update each method
(getAddress, createAddress, updateAddress, deleteAddress, setDefaultAddress) to
mirror the getAddresses pattern by returning throwError(() => new
UnauthorizedException('...')) when authorization or customerId is missing,
ensuring you import/keep throwError from rxjs and preserve the same error
messages used now.
In `@packages/integrations/medusajs/src/modules/products/products.service.ts`:
- Around line 56-71: getProductList implementation is fine as-is; no functional
changes required. Keep the existing promise-to-observable conversion in
getProductList, the params construction using HttpTypes.StoreProductListParams,
the mapping via mapProducts(response, this.defaultCurrency, query.category), and
the catchError delegating to handleHttpError; no edits needed in
products.service.ts for this PR.
In `@packages/integrations/mocked/src/modules/checkout/checkout.service.ts`:
- Around line 52-56: The null-safety check for cart items is unsafe and can
throw if cart.items exists but cart.items.data is nullish; update the condition
in the checkout service (the method handling adding a shipping method in
checkout.service.ts) to use optional chaining like: if
(!cart.items?.data?.length) { return throwError(() => new
BadRequestException('Cart must have items before adding shipping method')); } so
it safely handles undefined/null data while preserving the existing
throwError/BadRequestException behavior.
- Around line 12-17: The class declaration uses "implements Checkout.Service"
but Checkout.Service is an abstract class, so change the declaration to "extends
Checkout.Service" and ensure you call super() in the constructor; update the
constructor of CheckoutService (which currently injects Carts.Service and
Payments.Service) to invoke super(...) with any required parameters expected by
the abstract Checkout.Service constructor, and remove "implements" in favor of
"extends" so the class properly inherits abstract behavior (refer to
CheckoutService and Checkout.Service symbols to locate the change).
---
Nitpick comments:
In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts`:
- Around line 284-309: The prepareCheckout method contains a redundant ownership
check duplicate of the logic in getCart; remove the manual verification block
that calls this.authService.getCustomerId and throws UnauthorizedException so
prepareCheckout simply relies on getCart's ownership enforcement (keep the
initial not-found and items validation). Update prepareCheckout to no longer
reference authService for ownership to avoid divergence with getCart's behavior.
- Around line 427-434: The update flow is making an unnecessary second API call
by discarding the result of sdk.store.cart.update and calling getCart again;
change the code in carts.service.ts so that the Observable returned by
sdk.store.cart.update is used directly (map the response to the updated cart and
throw NotFoundException if the update response is null) instead of switchMapping
to getCart; apply the same fix to addShippingMethod where sdk.store.cart.update
is currently followed by getCart—use the update response, preserve
headers/params.cartId handling, and ensure null-checks still raise
NotFoundException.
In
`@packages/integrations/medusajs/src/modules/customers/customers.service.spec.ts`:
- Around line 156-194: The two tests in customers.service.spec.ts duplicate the
createdAddress fixture; extract that object into a shared constant (e.g.,
CREATED_ADDRESS) near minimalAddress and reuse it in both specs to remove
duplication; update the mock setup lines that use createdAddress and any
assertions in the tests that reference the variable so they import/point to the
new constant while leaving service.createAddress and
mockSdk.store.customer.createAddress usage unchanged.
In `@packages/integrations/medusajs/src/modules/payments/payments.mapper.ts`:
- Around line 5-14: The requiresRedirect heuristic in mapPaymentProvider
(function mapPaymentProvider) currently sets requiresRedirect using
medusaProvider.id.includes('stripe') || medusaProvider.id.includes('paypal'),
which is brittle and misses other redirect-based providers; update
mapPaymentProvider to either (a) replace the inline includes check with a
configurable list/Set (e.g., redirectProviders) that can be extended for
providers like Klarna or iDEAL, or (b) at minimum add a clear comment above the
requiresRedirect assignment documenting the limitation and listing known
redirect providers, and ensure the symbol requiresRedirect is set from that
list/Set so future providers can be added without changing code logic.
- Around line 27-41: The mapPaymentSession function populates redirectUrl and
clientSecret from medusaSession.data but then assigns the full
medusaSession.data to metadata, causing duplicate fields; update
mapPaymentSession to set metadata to a copy of medusaSession.data with
redirect_url and client_secret removed (or undefined) so downstream consumers
don't see the same values twice—use a shallow clone/omit of medusaSession.data
before assigning metadata while keeping id, cartId, providerId, status,
redirectUrl, clientSecret and expiresAt logic unchanged.
In `@packages/integrations/mocked/src/modules/checkout/checkout.service.ts`:
- Around line 195-247: completeCheckout currently chains setAddresses,
setShippingMethod, setPayment, and placeOrder which each call responseDelay(),
causing quadruple simulated latency; update completeCheckout to avoid stacking
delays by invoking the internal, no-delay implementations (or add new private
helpers e.g., setAddressesNoDelay, setShippingMethodNoDelay, setPaymentNoDelay,
placeOrderNoDelay) or by suppressing responseDelay on those calls and then apply
a single responseDelay() once at the end of the observable chain; ensure
references remain to completeCheckout and the four sub-call methods so reviewers
can find the changes.
packages/integrations/medusajs/src/modules/carts/carts.mapper.ts
Outdated
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/carts/carts.service.ts
Outdated
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/customers/customers.service.spec.ts
Show resolved
Hide resolved
… type handling across payment and cart modules - Removed `type` field from `PaymentMethod` and `Cart` entities, mappers, and related services. - Standardized error handling in payment and customer address services with clearer exception usage.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/docs/docs/main-components/harmonization-app/normalized-data-model/overview.md (1)
24-24: Optional wording tweak for concision.“which can cover a variety of cases” could be shortened (e.g., “which cover many cases”) to tighten the sentence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/docs/main-components/harmonization-app/normalized-data-model/overview.md` at line 24, The sentence for the Tickets link is wordy; update the text “[Tickets](./core-model-tickets.md) - fetch and submit user tickets, which can cover a variety of cases” to a more concise phrasing such as “[Tickets](./core-model-tickets.md) - fetch and submit user tickets, which cover many cases” (or similar shorter variant) to tighten the wording while preserving meaning.packages/integrations/medusajs/src/modules/customers/customers.service.spec.ts (1)
160-175: Extract duplicatecreatedAddressfixture.The
createdAddressobject is duplicated verbatim in two test cases (lines 160–175 and 200–215). Consider extracting it alongsideminimalAddressat the top of the file to reduce duplication and ease maintenance.♻️ Proposed refactor
const minimalAddress = { id: 'addr_1', // ...existing fields... }; +const createdAddress = { + id: 'addr_new', + first_name: 'John', + last_name: 'Doe', + country_code: 'pl', + city: 'Warsaw', + address_1: '', + address_2: undefined, + postal_code: '', + province: undefined, + phone: undefined, + is_default_shipping: false, + is_default_billing: false, + created_at: new Date('2024-01-01'), + updated_at: new Date('2024-01-02'), +};Then reference
createdAddressin both test cases instead of redeclaring it inline.Also applies to: 200-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/customers/customers.service.spec.ts` around lines 160 - 175, Extract the duplicated createdAddress fixture into a shared constant near the existing minimalAddress fixture and reuse it in both specs instead of redeclaring it inline; specifically, add a top-level exported or file-scoped constant named createdAddress (matching the object used in the tests) and update the two test cases that currently redeclare createdAddress (the blocks in customers.service.spec.ts) to reference that shared createdAddress constant.packages/integrations/mocked/src/modules/carts/carts.service.ts (1)
30-48: Inconsistent error handling: synchronousthrowvs reactivethrowErroracross methods.Most methods (e.g.,
getCartline 39,updateCartline 84) use synchronousthrow, whileupdateCartAddresses(lines 373, 379, 383) correctly usesreturn throwError(...). For a mocked service where the throws happen before any async operation, synchronous throws work in practice (caught by NestJS exception filters). However, this inconsistency could cause issues if the service is ever adapted or if callers use.pipe(catchError(...)).Consider aligning all methods to use
throwError(...)for consistency with the Medusa implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/mocked/src/modules/carts/carts.service.ts` around lines 30 - 48, Replace synchronous throws in methods like getCart (and other places such as updateCart) with reactive errors using RxJS throwError so the method returns an Observable that emits an error; specifically, instead of throwing UnauthorizedException directly in getCart, return throwError(() => new UnauthorizedException('...')) and ensure throwError is imported from 'rxjs' and the method still returns an Observable<Carts.Model.Cart | undefined> (preserve the responseDelay() behavior where applicable). Also scan related methods (e.g., updateCart, updateCartAddresses) and make error handling consistent by using return throwError(() => new UnauthorizedException(...)) or other appropriate Http exceptions everywhere.packages/integrations/mocked/src/modules/carts/carts.mapper.ts (1)
7-16:mapPaymentMethodFromMetadatalacks type validation foridandname— may return non-string values.Unlike the Medusa version in
packages/integrations/medusajs/src/modules/carts/carts.mapper.ts(lines 114-116) which validatestypeof id !== 'string', this mocked version directly castsstored.idandstored.nameas strings without checking. If metadata contains a malformedpaymentMethodobject, this returns invalid data.♻️ Proposed fix
const mapPaymentMethodFromMetadata = (metadata: Record<string, unknown>): Carts.Model.PaymentMethod | undefined => { const stored = metadata?.paymentMethod as Record<string, unknown> | undefined; if (!stored || typeof stored !== 'object') return undefined; + const id = stored.id; + const name = stored.name; + if (typeof id !== 'string' || typeof name !== 'string') return undefined; + return { - id: stored.id as string, - name: stored.name as string, + id, + name, description: (stored.description as string) ?? undefined, }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/mocked/src/modules/carts/carts.mapper.ts` around lines 7 - 16, mapPaymentMethodFromMetadata currently casts stored.id and stored.name without validation and can return non-string values; update the function to check that stored is an object and that stored.id and stored.name are both typeof 'string' before returning a Carts.Model.PaymentMethod, otherwise return undefined. Keep the existing behavior for description (allow string or undefined) and reference stored.id, stored.name, and stored.description when performing the checks and constructing the returned object.
🤖 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/integrations/medusajs/src/modules/customers/customers.service.spec.ts`:
- Around line 111-122: The service mixes two error patterns for auth failures:
getAddresses returns an Observable via throwError() while getAddress,
createAddress, updateAddress, deleteAddress and setDefaultAddress throw
synchronously; pick one consistent approach and apply it across all
methods—either change
getAddress/createAddress/updateAddress/deleteAddress/setDefaultAddress to return
an Observable that emits the auth error using throwError() (so all methods
return Observables), or change getAddresses to throw synchronously like the
others; update the error path in the corresponding methods (getAddresses,
getAddress, createAddress, updateAddress, deleteAddress, setDefaultAddress) to
use the chosen pattern consistently and ensure the auth check that uses
mockAuthService.getCustomerId follows that pattern.
In `@packages/integrations/medusajs/src/modules/payments/payments.mapper.ts`:
- Around line 6-23: The requiresRedirect check is using provider.id
(case-sensitive) while type detection uses idLower; update the requiresRedirect
logic to use the lowercased id (idLower) or otherwise perform a case-insensitive
match so it aligns with the type inference. Locate the block that defines
idLower and type and modify the requiresRedirect expression (currently using
provider.id.includes('stripe') || provider.id.includes('paypal')) to use
idLower.includes('stripe') || idLower.includes('paypal') (or an equivalent
case-insensitive check) so STRIPE/PAYPAL providers with mixed-case IDs are
detected consistently.
---
Duplicate comments:
In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts`:
- Around line 446-448: The ternary assigning headers in carts.service.ts is dead
code because both branches call
this.medusaJsService.getStoreApiHeaders(authorization); replace the ternary with
a single call to this.medusaJsService.getStoreApiHeaders(authorization) to
produce headers, or if the original intent was to pass a different value when
authorization is falsy, update the false branch accordingly; locate the
assignment to headers in the CartsService method and simplify it to a single
call to getStoreApiHeaders.
- Around line 141-144: In addCartItem replace the synchronous throw with an
Observable error so callers' .pipe(catchError(...)) can catch it: instead of
throwing BadRequestException directly in addCartItem, return throwError(() =>
new BadRequestException('SKU is required for Medusa carts')) (import throwError
from 'rxjs'); mirror the pattern used elsewhere in this file (e.g., the
validation at line ~180) so the method still returns
Observable<Carts.Model.Cart>.
In
`@packages/integrations/medusajs/src/modules/customers/customers.service.spec.ts`:
- Around line 71-83: Tests for getAddresses duplicate the same expect invocation
causing the service observable to be consumed twice; capture the promise
returned by firstValueFrom(service.getAddresses(...)) in a const (e.g., const p
= firstValueFrom(service.getAddresses(...))) and then await
expect(p).rejects.toThrow(UnauthorizedException) and await
expect(p).rejects.toThrow('Authentication required') (similarly for the 'Invalid
authentication' case), so the observable is only consumed once and both
assertions run against the same promise; reference the service.getAddresses call
in the 'should throw UnauthorizedException when auth is missing' and 'should
throw UnauthorizedException when getCustomerId returns undefined' tests.
In `@packages/integrations/medusajs/src/modules/payments/payments.service.ts`:
- Around line 156-158: The cancelSession method currently throws synchronously
which breaks the Observable contract; change cancelSession(_params:
Payments.Request.CancelSessionParams, _authorization: string | undefined):
Observable<void> to return an Observable error like the other methods (e.g., use
return throwError(() => new NotImplementedException())) so callers can handle it
with .pipe(catchError(...)); mirror the pattern used in getSession and
updateSession to ensure consistency.
In `@packages/integrations/mocked/src/modules/carts/carts.service.ts`:
- Around line 340-363: The NotFoundException currently includes the
user-supplied ID (params.cartId) which leaks input; change the throw to use a
generic message consistent with other methods (e.g., 'Cart not found') where
mapCart(...) returns falsy. Locate the NotFoundException raised after calling
mapCart({ id: params.cartId }) in the carts service and replace the detailed
message with the generic one to match other methods.
---
Nitpick comments:
In
`@apps/docs/docs/main-components/harmonization-app/normalized-data-model/overview.md`:
- Line 24: The sentence for the Tickets link is wordy; update the text
“[Tickets](./core-model-tickets.md) - fetch and submit user tickets, which can
cover a variety of cases” to a more concise phrasing such as
“[Tickets](./core-model-tickets.md) - fetch and submit user tickets, which cover
many cases” (or similar shorter variant) to tighten the wording while preserving
meaning.
In
`@packages/integrations/medusajs/src/modules/customers/customers.service.spec.ts`:
- Around line 160-175: Extract the duplicated createdAddress fixture into a
shared constant near the existing minimalAddress fixture and reuse it in both
specs instead of redeclaring it inline; specifically, add a top-level exported
or file-scoped constant named createdAddress (matching the object used in the
tests) and update the two test cases that currently redeclare createdAddress
(the blocks in customers.service.spec.ts) to reference that shared
createdAddress constant.
In `@packages/integrations/mocked/src/modules/carts/carts.mapper.ts`:
- Around line 7-16: mapPaymentMethodFromMetadata currently casts stored.id and
stored.name without validation and can return non-string values; update the
function to check that stored is an object and that stored.id and stored.name
are both typeof 'string' before returning a Carts.Model.PaymentMethod, otherwise
return undefined. Keep the existing behavior for description (allow string or
undefined) and reference stored.id, stored.name, and stored.description when
performing the checks and constructing the returned object.
In `@packages/integrations/mocked/src/modules/carts/carts.service.ts`:
- Around line 30-48: Replace synchronous throws in methods like getCart (and
other places such as updateCart) with reactive errors using RxJS throwError so
the method returns an Observable that emits an error; specifically, instead of
throwing UnauthorizedException directly in getCart, return throwError(() => new
UnauthorizedException('...')) and ensure throwError is imported from 'rxjs' and
the method still returns an Observable<Carts.Model.Cart | undefined> (preserve
the responseDelay() behavior where applicable). Also scan related methods (e.g.,
updateCart, updateCartAddresses) and make error handling consistent by using
return throwError(() => new UnauthorizedException(...)) or other appropriate
Http exceptions everywhere.
packages/integrations/medusajs/src/modules/customers/customers.service.spec.ts
Outdated
Show resolved
Hide resolved
packages/integrations/medusajs/src/modules/payments/payments.mapper.ts
Outdated
Show resolved
Hide resolved
…tion usage in services
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
apps/docs/docs/main-components/harmonization-app/normalized-data-model/core-model-payments.md (2)
125-137: Clarify field formats and object shapes.
expiresAtis listed as a string but no format is specified. Also,metadata/configare described asobjectwithout shape guidance. Consider documentingexpiresAtas ISO 8601/RFC 3339 and usingRecord<string, unknown>(or an explicit schema) formetadata/configto reduce ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/docs/main-components/harmonization-app/normalized-data-model/core-model-payments.md` around lines 125 - 137, Update the PaymentSession docs to specify precise formats and shapes: annotate expiresAt as an ISO 8601 / RFC 3339 timestamp string (e.g., "YYYY-MM-DDTHH:MM:SSZ") and replace the vague "object" type for metadata (and any config fields) with either a typed map like Record<string, unknown> or an explicit schema example showing expected keys/values; reference the PaymentSession type and fields expiresAt, metadata (and config if present) so the reader knows which fields have the new format and shape guidance.
56-88: Add parameter sections for getSession/updateSession/cancelSession.These methods don’t document their required params, which makes the API surface inconsistent with
getProviders/createSessionand harder to consume. Please add parameter tables forGetSessionParams,UpdateSessionParams, andCancelSessionParams(e.g.,sessionIdor equivalent).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/docs/main-components/harmonization-app/normalized-data-model/core-model-payments.md` around lines 56 - 88, Add explicit parameter sections for getSession, updateSession, and cancelSession documenting the fields of GetSessionParams, UpdateSessionParams, and CancelSessionParams (e.g., sessionId: string) and whether authorization is required/optional; update the docs for getSession(getSession params, authorization?), updateSession(params, data, authorization?) to include a small table or bullet list for each param name, type, required/optional, and brief description (include UpdateSessionBody fields if relevant) so the API surface matches getProviders/createSession and consumers know required fields for sessionId and any other required keys.packages/integrations/medusajs/src/modules/payments/payments.mapper.ts (2)
5-14: Consider normalizingtypeto a known enum/constant instead of passing through the raw id.Currently
typeis set to the rawprovider.idstring (e.g.,pp_stripe_123). If downstream code switches ontypeto determine payment-specific behavior, raw ids will be fragile. Deriving a normalized type (e.g.,'STRIPE','PAYPAL','MANUAL') — similar to howmapPaymentSessionStatusnormalizes statuses — would make consumer logic more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/payments/payments.mapper.ts` around lines 5 - 14, mapPaymentProvider currently assigns the raw provider.id to the PaymentProvider.type which is fragile; update mapPaymentProvider to derive a normalized type constant/enum (e.g., 'STRIPE', 'PAYPAL', 'MANUAL', 'UNKNOWN') from provider.id instead of passing the raw id, using containment checks like provider.id.includes('stripe')/includes('paypal') and fallback rules for manual or unknown providers, and keep requiresRedirect and other fields as-is; update any tests or consumers if they expect the old raw id value for type.
43-56:mapPaymentSessionStatusdoesn't handle'PENDING'as an explicit case.If Medusa ever sends
status: 'pending'explicitly, it still falls through todefaultand returns'PENDING'— so the behavior is correct. However, adding an explicitcase 'PENDING'makes the intent clearer and future-proofs against accidental changes to the default branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/payments/payments.mapper.ts` around lines 43 - 56, Add an explicit 'PENDING' case to mapPaymentSessionStatus so that when status.toUpperCase() === 'PENDING' it explicitly returns 'PENDING' instead of relying on the default branch; update the switch in the mapPaymentSessionStatus function to include case 'PENDING': return 'PENDING'; to make intent explicit and future-proof the mapping.packages/integrations/medusajs/src/modules/payments/payments.mapper.spec.ts (1)
16-28: Add a test for mixed-case provider ids to cover the case-sensitivity concern.All
requiresRedirecttests use lowercase ids (pp_stripe,pp_paypal,pp_manual). A test with a mixed-case id like'pp_Stripe_checkout'would guard against the case-sensitivity issue flagged in the mapper and prevent regressions if the fix is applied.💡 Suggested test case
it('should set requiresRedirect false for other providers', () => { const result = mapPaymentProvider({ id: 'pp_manual' } as HttpTypes.StorePaymentProvider); expect(result.requiresRedirect).toBe(false); }); + + it('should set requiresRedirect for mixed-case provider ids', () => { + expect( + mapPaymentProvider({ id: 'pp_Stripe_checkout' } as HttpTypes.StorePaymentProvider).requiresRedirect, + ).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/payments/payments.mapper.spec.ts` around lines 16 - 28, Add a unit test in payments.mapper.spec.ts that verifies mapPaymentProvider handles mixed-case provider ids (e.g., pass { id: 'pp_Stripe_checkout' } and assert .requiresRedirect === true); this mirrors existing tests for lowercase ids and ensures mapPaymentProvider (the function under test) normalizes or otherwise correctly detects providers regardless of case. Add a similar mixed-case check for PayPal if desired (e.g., 'pp_PayPal_express') and run the spec to confirm behavior.scripts/generate-postman-collection.mjs (1)
631-633: Consider wrapping the file write in a try/catch for a friendlier error message.If the write fails (e.g., permissions), the raw Node stack trace is not very developer-friendly. This is a minor point for a dev-only script.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-postman-collection.mjs` around lines 631 - 633, The fs.writeFileSync call writing 'O2S-API.postman_collection.json' should be wrapped in a try/catch to provide a friendlier error message; catch errors thrown by fs.writeFileSync (and any JSON.stringify on collection) and log a clear, contextual error via console.error including the filename and error.message (or error stack), then exit non‑zero (e.g., process.exit(1)) or rethrow to signal failure; update the block around fs.writeFileSync, JSON.stringify(collection, null, 2), and console.log so success still logs the existing message on success and the catch logs the friendly error on failure..gitignore (1)
35-39: Duplicate.gitignoreentries for AI-related files.Lines 35–39 (under "IDEs") and lines 51–55 (under "AI rules") both ignore
.claude,AGENTS.md,CLAUDE.md, andagent-os. Consider consolidating into one section to avoid confusion.Also applies to: 51-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 35 - 39, The .gitignore contains duplicate entries for .claude, AGENTS.md, CLAUDE.md, and agent-os in two sections; remove the duplicated block (either the "IDEs" or "AI rules" section) and consolidate those AI-related ignores into a single clearly labeled section (e.g., "AI rules") so only one set of entries (.claude, AGENTS.md, CLAUDE.md, agent-os) remains; ensure any unrelated entries like .cursor stay in their appropriate section and update surrounding comments to reflect the consolidation.packages/integrations/medusajs/src/modules/customers/customers.service.ts (1)
129-141: Improved address matching, but still has an edge case with duplicate field values.If the customer already has an address with identical field values,
findwill return the first match (potentially the old one, not the newly created one). This is unlikely in practice but worth noting. Consider comparing against the pre-create address list if the Medusa API doesn't return the created address ID directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/customers/customers.service.ts` around lines 129 - 141, The current addresses.find(...) in customers.service.ts can return an existing identical address instead of the newly created one; before creating the new address capture the existing addresses list (e.g., oldAddresses = addresses.slice()), then after the API create call locate the created record by finding an address in the new addresses list that is not present in oldAddresses (compare unique id if the Medusa API returns it, otherwise compare all fields plus a created_at timestamp or use object reference difference) rather than relying on the first matching field-equality match in the createdAddress find; update the logic around the medusaAddress/create call and the createdAddress lookup to use this differential comparison so you reliably pick the newly created address.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/generate-postman-collection.mjs`:
- Around line 74-79: The generated url.raw currently uses .replace(/\/+/g, '/')
which collapses the protocol slashes; update the replace call used when building
the url object (the raw property created from '{{baseUrl}}/${urlPath.join('/')}'
and urlPath) to use a pattern that only collapses consecutive slashes that are
not part of the protocol (i.e., do not touch the '://'), so duplicate slashes
elsewhere are collapsed but 'http://' or 'https://' remain intact; keep the rest
of the url object (host, path, query) unchanged.
---
Duplicate comments:
In `@packages/integrations/medusajs/src/modules/customers/customers.service.ts`:
- Around line 262-268: Update is correct: the call to
this.sdk.store.customer.updateAddress(params.id, { is_default_shipping: true,
is_default_billing: true } as Partial<HttpTypes.StoreUpdateCustomerAddress>) now
sets both defaults; no functional change required—leave the two flags present
and keep the updateAddress invocation as-is (optionally remove the redundant
type cast to Partial<HttpTypes.StoreUpdateCustomerAddress> if TypeScript already
infers the correct type).
In `@packages/integrations/medusajs/src/modules/payments/payments.mapper.ts`:
- Line 11: The requiresRedirect check uses case-sensitive includes on
provider.id which can miss mixed-case ids; update the expression that sets
requiresRedirect (the code reading provider.id) to perform a case-insensitive
comparison by normalizing provider.id (e.g., provider.id?.toLowerCase() or
.toUpperCase()) before calling .includes('stripe') or .includes('paypal'),
ensuring you handle a possibly undefined provider.id safely (e.g., default to
empty string).
---
Nitpick comments:
In @.gitignore:
- Around line 35-39: The .gitignore contains duplicate entries for .claude,
AGENTS.md, CLAUDE.md, and agent-os in two sections; remove the duplicated block
(either the "IDEs" or "AI rules" section) and consolidate those AI-related
ignores into a single clearly labeled section (e.g., "AI rules") so only one set
of entries (.claude, AGENTS.md, CLAUDE.md, agent-os) remains; ensure any
unrelated entries like .cursor stay in their appropriate section and update
surrounding comments to reflect the consolidation.
In
`@apps/docs/docs/main-components/harmonization-app/normalized-data-model/core-model-payments.md`:
- Around line 125-137: Update the PaymentSession docs to specify precise formats
and shapes: annotate expiresAt as an ISO 8601 / RFC 3339 timestamp string (e.g.,
"YYYY-MM-DDTHH:MM:SSZ") and replace the vague "object" type for metadata (and
any config fields) with either a typed map like Record<string, unknown> or an
explicit schema example showing expected keys/values; reference the
PaymentSession type and fields expiresAt, metadata (and config if present) so
the reader knows which fields have the new format and shape guidance.
- Around line 56-88: Add explicit parameter sections for getSession,
updateSession, and cancelSession documenting the fields of GetSessionParams,
UpdateSessionParams, and CancelSessionParams (e.g., sessionId: string) and
whether authorization is required/optional; update the docs for
getSession(getSession params, authorization?), updateSession(params, data,
authorization?) to include a small table or bullet list for each param name,
type, required/optional, and brief description (include UpdateSessionBody fields
if relevant) so the API surface matches getProviders/createSession and consumers
know required fields for sessionId and any other required keys.
In `@packages/integrations/medusajs/src/modules/customers/customers.service.ts`:
- Around line 129-141: The current addresses.find(...) in customers.service.ts
can return an existing identical address instead of the newly created one;
before creating the new address capture the existing addresses list (e.g.,
oldAddresses = addresses.slice()), then after the API create call locate the
created record by finding an address in the new addresses list that is not
present in oldAddresses (compare unique id if the Medusa API returns it,
otherwise compare all fields plus a created_at timestamp or use object reference
difference) rather than relying on the first matching field-equality match in
the createdAddress find; update the logic around the medusaAddress/create call
and the createdAddress lookup to use this differential comparison so you
reliably pick the newly created address.
In `@packages/integrations/medusajs/src/modules/payments/payments.mapper.spec.ts`:
- Around line 16-28: Add a unit test in payments.mapper.spec.ts that verifies
mapPaymentProvider handles mixed-case provider ids (e.g., pass { id:
'pp_Stripe_checkout' } and assert .requiresRedirect === true); this mirrors
existing tests for lowercase ids and ensures mapPaymentProvider (the function
under test) normalizes or otherwise correctly detects providers regardless of
case. Add a similar mixed-case check for PayPal if desired (e.g.,
'pp_PayPal_express') and run the spec to confirm behavior.
In `@packages/integrations/medusajs/src/modules/payments/payments.mapper.ts`:
- Around line 5-14: mapPaymentProvider currently assigns the raw provider.id to
the PaymentProvider.type which is fragile; update mapPaymentProvider to derive a
normalized type constant/enum (e.g., 'STRIPE', 'PAYPAL', 'MANUAL', 'UNKNOWN')
from provider.id instead of passing the raw id, using containment checks like
provider.id.includes('stripe')/includes('paypal') and fallback rules for manual
or unknown providers, and keep requiresRedirect and other fields as-is; update
any tests or consumers if they expect the old raw id value for type.
- Around line 43-56: Add an explicit 'PENDING' case to mapPaymentSessionStatus
so that when status.toUpperCase() === 'PENDING' it explicitly returns 'PENDING'
instead of relying on the default branch; update the switch in the
mapPaymentSessionStatus function to include case 'PENDING': return 'PENDING'; to
make intent explicit and future-proof the mapping.
In `@scripts/generate-postman-collection.mjs`:
- Around line 631-633: The fs.writeFileSync call writing
'O2S-API.postman_collection.json' should be wrapped in a try/catch to provide a
friendlier error message; catch errors thrown by fs.writeFileSync (and any
JSON.stringify on collection) and log a clear, contextual error via
console.error including the filename and error.message (or error stack), then
exit non‑zero (e.g., process.exit(1)) or rethrow to signal failure; update the
block around fs.writeFileSync, JSON.stringify(collection, null, 2), and
console.log so success still logs the existing message on success and the catch
logs the friendly error on failure.
…price mapping for consistent error handling and field validation
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
packages/integrations/medusajs/src/modules/checkout/checkout.service.ts (3)
35-56: Repeated cart-exists-and-has-items validation could be extracted.
setAddresses(lines 41–49) andsetShippingMethod(lines 64–74) share identical validation logic: fetch cart → check existence → check items. A small private helper (e.g.,validateCartHasItems(cartId, authorization)) would reduce duplication.Also applies to: 58-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/checkout/checkout.service.ts` around lines 35 - 56, Extract the duplicated "fetch cart → check existence → check items" logic into a private helper, e.g., validateCartHasItems(cartId: string, authorization?: string): Observable<Carts.Model.Cart>, that calls this.cartsService.getCart({ id: cartId }, authorization) and emits the cart or throws NotFoundException/BadRequestException when missing or empty; then replace the duplicated blocks in setAddresses and setShippingMethod to call validateCartHasItems(params.cartId, authorization).pipe(switchMap(cart => /* existing delegate call */)) so both methods reuse the helper and keep existing error handling via catchError(handleHttpError).
219-226: Direct mutation of the freshly mappedorderobject.Line 223 mutates
order.emailaftermapOrderreturns. While safe today since the object isn't reused, this pattern can cause subtle bugs if the flow changes later. Consider passing- const order = mapOrder(response.order, this.defaultCurrency); - - // Attach email for order confirmation if provided (guest checkout) - if (email) { - order.email = email; - } - - return of(mapPlaceOrderResponse(order)); + const order = mapOrder(response.order, this.defaultCurrency); + return of(mapPlaceOrderResponse(email ? { ...order, email } : order));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/checkout/checkout.service.ts` around lines 219 - 226, Avoid mutating the mapped order by not assigning to order.email; instead create an order object that includes the email or pass email into the response mapper. Update the code around mapOrder and mapPlaceOrderResponse so you either call mapPlaceOrderResponse({...order, email}) or enhance mapPlaceOrderResponse/order mapper to accept an optional email parameter and incorporate it there, ensuring mapOrder remains pure.
87-135: Inconsistent error handling —setPaymentlackscatchError(handleHttpError).
setAddresses(line 54) andsetShippingMethod(line 83) both wrap their pipelines withcatchError((error) => handleHttpError(error)), butsetPayment,getCheckoutSummary,placeOrder, andcompleteCheckoutdo not. IfhandleHttpErrornormalizes error shapes for the controller layer, its absence here means raw/unexpected errors could leak through.Either apply
handleHttpErrorconsistently across all public methods, or remove it from the two that have it if the controller already handles normalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/checkout/checkout.service.ts` around lines 87 - 135, The pipeline in setPayment is missing consistent error normalization: add a catchError that invokes handleHttpError (same as used in setAddresses and setShippingMethod) to the observable chain returned by setPayment so any thrown errors are transformed before leaving the service; similarly ensure getCheckoutSummary, placeOrder, and completeCheckout also append catchError(handleHttpError) (or catchError((err) => handleHttpError(err))) to their returned observable chains so all public methods use the same error handling; reference the methods setPayment, setAddresses, setShippingMethod, getCheckoutSummary, placeOrder, completeCheckout and the handleHttpError helper when making the changes.packages/integrations/medusajs/src/modules/resources/resources.mapper.ts (2)
98-106: Same silent-default pattern for payment period — same suggestion applies.Consider adding a warning log here too for consistency. The
replace(/-/g, '_')normalization for hyphenated Medusa values is a nice touch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/resources/resources.mapper.ts` around lines 98 - 106, The mapPaymentPeriod function silently falls back to 'ONE_TIME' when given an unknown paymentType; update mapPaymentPeriod to log a warning when normalization yields a non‑valid period (using the module's logger or an injected logger), include the original paymentType and the normalized value in the message for debugging, and keep using VALID_PAYMENT_PERIODS for the membership check and the existing normalization logic (retain replace(/-/g,'_')) so behavior is unchanged except the added warning before returning 'ONE_TIME'.
88-96: Silent default to'ACTIVE'for unrecognized contract statuses may mask data issues.When an unexpected status value arrives from Medusa, silently falling back to
'ACTIVE'could hide integration bugs or upstream data problems. Consider adding aconsole.warn(or injected logger) when the fallback is triggered so issues are visible in operational logs.💡 Proposed enhancement
function mapContractStatus(status: string | undefined): Resources.Model.ContractStatus { const normalized = (status ?? '').toUpperCase(); if (VALID_CONTRACT_STATUSES.includes(normalized as Resources.Model.ContractStatus)) { return normalized as Resources.Model.ContractStatus; } + // Log to aid debugging unexpected values from Medusa + console.warn(`Unknown contract status "${status}", defaulting to ACTIVE`); return 'ACTIVE'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/resources/resources.mapper.ts` around lines 88 - 96, mapContractStatus currently silently returns the default 'ACTIVE' for unknown statuses; modify it to log a warning when the fallback is used so unexpected Medusa values are visible. In the mapContractStatus function (and using VALID_CONTRACT_STATUSES), detect when status is not included and call a logger (prefer an injected logger if available, else console.warn) with a clear message that includes the original status and the normalized value, then return 'ACTIVE' as before.packages/integrations/medusajs/src/utils/address.ts (1)
31-32:districtandregionare both mapped from the sameprovincefield.If this is intentional (e.g., the framework consumers use the two interchangeably), a brief inline comment would clarify the design choice. Otherwise, consider mapping only one and leaving the other
undefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/utils/address.ts` around lines 31 - 32, district and region are both assigned from address.province which is likely a mistake or at least confusing; update the mapping in the function that builds the output address so that district and region use the correct source fields (e.g., use address.district for district and address.region for region) or, if they are intentionally the same, add a short inline comment next to the district and region assignments explaining that province is deliberately used for both and why, otherwise leave the unused property undefined instead of duplicating province.packages/integrations/medusajs/src/modules/orders/orders.mapper.ts (1)
54-54:mapProducttakesunitPriceseparately despite always receivingitemwith the same value.The only call site (line 54) passes
item.unit_priceanditemtogether. Sinceitemis always provided and containsunit_price, the separateunitPriceparameter is redundant. The!itemguard on line 63 is dead code in practice.Consider simplifying the signature to
(item: HttpTypes.StoreOrderLineItem, currency: Models.Price.Currency)and readingitem.unit_priceinternally.Also applies to: 58-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/orders/orders.mapper.ts` at line 54, The mapProduct function currently accepts unitPrice and item separately although callers (e.g., the call at product: mapProduct(item.unit_price, currency, item)) always pass item.unit_price and item together; change mapProduct to a two-argument signature mapProduct(item: HttpTypes.StoreOrderLineItem, currency: Models.Price.Currency) and read unit_price from item internally, remove the redundant unitPrice parameter and the now-dead !item guard inside mapProduct, and update all call sites (including the product mapping call) to pass (item, currency) only so the function usage and internal checks are consistent.packages/integrations/medusajs/src/modules/carts/carts.service.ts (1)
438-453: DoublegetCartcall aftercart.updateadds latency.
updateCartAddressesfirst callsgetCart(line 398), thencart.update(line 439), thengetCartagain (line 446). The update response already contains the updated cart. Consider mapping the update response directly instead of the extra round trip.♻️ Suggested optimization
return from( this.sdk.store.cart.update( params.cartId, cartUpdate, { fields: this.cartItemsFields }, headers, ), ).pipe( - switchMap(() => this.getCart({ id: params.cartId }, authorization)), - map((updatedCart) => { - if (!updatedCart) { - throw new NotFoundException(`Cart with ID ${params.cartId} not found`); - } - return updatedCart; - }), + map((response: HttpTypes.StoreCartResponse) => mapCart(response.cart, this.defaultCurrency)), );
🤖 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/integrations/medusajs/src/modules/checkout/checkout.service.ts`:
- Line 32: The code sets this.defaultCurrency =
this.config.get('DEFAULT_CURRENCY') || '' which can pass an empty string into
functions like mapOrder and mapShippingOptions and then into parseCurrency(''),
producing invalid prices; change the initialization in the constructor (or
service init) to fail fast or provide a sensible fallback: either throw an
explicit error if DEFAULT_CURRENCY is missing (e.g., validate and throw in the
constructor of the checkout service) or set a sane default currency constant
(e.g., 'USD') instead of '' so parseCurrency receives a valid code; update any
callers relying on this.defaultCurrency (mapOrder, mapShippingOptions) to assume
a non-empty currency string.
- Around line 275-278: The catchError currently returns the original option
(option.id, calculatedOption: option) which preserves price_type="calculated"
without an amount; update the handler for fulfillment.calculate failures to
either (A) return null/omit the option so it can be filtered out when building
enrichedOptions, or (B) return a clearly marked failed-calculation object (e.g.,
calculatedOption: { ...option, amount: null, price_type: 'unavailable' or
'unknown', calculation_failed: true }) and ensure the enrichedOptions
construction filters nulls or inspects calculation_failed to prevent showing
$0.00/undefined prices; modify the catchError block and the logic that builds
enrichedOptions to implement one of these two behaviors and log the error with
this.logger.warn as before.
In `@packages/integrations/medusajs/src/utils/address.ts`:
- Around line 34-35: The mapping currently sets both streetNumber and apartment
from address.address_2, producing identical values; update the converter so
apartment is sourced from address.address_2 while streetNumber is not (set to
undefined) or explicitly parsed from address.address_1 if you implement parsing
logic; locate the mapping that sets streetNumber and apartment (reference
symbols: streetNumber, apartment, address.address_2, address.address_1) and
change streetNumber to undefined (or add a parseStreetNumber(address.address_1)
call) and keep apartment = address.address_2.
In `@scripts/generate-postman-collection.mjs`:
- Around line 20-23: The authToken variable in the variable array is set to
'Bearer mock-token', which causes a double "Bearer" prefix because Postman's
bearer auth type auto-prepends "Bearer "; change the authToken variable's value
to just 'mock-token' (i.e., update the entry with key: 'authToken' to value:
'mock-token') so the Authorization header becomes "Bearer mock-token" correctly.
---
Duplicate comments:
In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts`:
- Around line 147-150: In addCartItem (which returns
Observable<Carts.Model.Cart>) replace the synchronous throw with an RxJS error
Observable so the error flows through Observable pipelines; specifically, return
throwError(() => new BadRequestException('SKU is required for Medusa carts'))
(and ensure throwError is imported from 'rxjs') so downstream
.pipe(catchError(...)) can catch it consistently like the guard at line 190.
- Around line 466-468: The ternary assigning headers is dead code because both
branches call the same method; replace the ternary with a single call: remove
the conditional and set const headers =
this.medusaJsService.getStoreApiHeaders(authorization); (referencing the headers
variable and the getStoreApiHeaders method in the carts service).
In `@packages/integrations/medusajs/src/modules/resources/resources.mapper.ts`:
- Line 76: The mapping currently passes serviceInstance?.totals?.currency ??
defaultCurrency into parseCurrency which can still accept empty/invalid strings
and yield invalid values; update the currency mapping in resources.mapper.ts to
validate the raw currency string before calling parseCurrency (check non-empty
and membership in the allowed Currency set/enum), or wrap parseCurrency in a
safe helper (e.g., parseCurrencyOrFallback) that tries parseCurrency in a
try/catch and returns a known-good fallback (or defaultCurrency) and logs a
warning; ensure you reference parseCurrency, defaultCurrency, and the
serviceInstance?.totals?.currency source when implementing the check.
In `@packages/integrations/medusajs/src/utils/price.ts`:
- Around line 23-25: The current validation in
packages/integrations/medusajs/src/utils/price.ts only checks for undefined/null
so NaN and Infinity slip through; update the validation around the variable
`value` (the block that throws BadRequestException(`${context}: price value is
missing or invalid`)) to also require a finite number by using
Number.isFinite(value) (or an equivalent finite-number check) and throw the same
BadRequestException when it fails so NaN/Infinity are rejected.
In `@scripts/generate-postman-collection.mjs`:
- Around line 77-82: The current raw URL construction collapses all consecutive
slashes (`.replace(/\/+/g, '/')`) which breaks protocol `://`; change it to
preserve protocol by either trimming trailing slashes from the base token and
joining with the path (e.g., remove trailing slashes from `{{baseUrl}}` then
`/${urlPath.join('/')}`) or use a regex that ignores the protocol (e.g.,
`.replace(/([^:]\/)\/+/g, '$1')`) when building the `raw` value for the `url`
object (see `url`, `urlPath`, and `filteredQuery`).
---
Nitpick comments:
In `@packages/integrations/medusajs/src/modules/checkout/checkout.service.ts`:
- Around line 35-56: Extract the duplicated "fetch cart → check existence →
check items" logic into a private helper, e.g., validateCartHasItems(cartId:
string, authorization?: string): Observable<Carts.Model.Cart>, that calls
this.cartsService.getCart({ id: cartId }, authorization) and emits the cart or
throws NotFoundException/BadRequestException when missing or empty; then replace
the duplicated blocks in setAddresses and setShippingMethod to call
validateCartHasItems(params.cartId, authorization).pipe(switchMap(cart => /*
existing delegate call */)) so both methods reuse the helper and keep existing
error handling via catchError(handleHttpError).
- Around line 219-226: Avoid mutating the mapped order by not assigning to
order.email; instead create an order object that includes the email or pass
email into the response mapper. Update the code around mapOrder and
mapPlaceOrderResponse so you either call mapPlaceOrderResponse({...order,
email}) or enhance mapPlaceOrderResponse/order mapper to accept an optional
email parameter and incorporate it there, ensuring mapOrder remains pure.
- Around line 87-135: The pipeline in setPayment is missing consistent error
normalization: add a catchError that invokes handleHttpError (same as used in
setAddresses and setShippingMethod) to the observable chain returned by
setPayment so any thrown errors are transformed before leaving the service;
similarly ensure getCheckoutSummary, placeOrder, and completeCheckout also
append catchError(handleHttpError) (or catchError((err) =>
handleHttpError(err))) to their returned observable chains so all public methods
use the same error handling; reference the methods setPayment, setAddresses,
setShippingMethod, getCheckoutSummary, placeOrder, completeCheckout and the
handleHttpError helper when making the changes.
In `@packages/integrations/medusajs/src/modules/orders/orders.mapper.ts`:
- Line 54: The mapProduct function currently accepts unitPrice and item
separately although callers (e.g., the call at product:
mapProduct(item.unit_price, currency, item)) always pass item.unit_price and
item together; change mapProduct to a two-argument signature mapProduct(item:
HttpTypes.StoreOrderLineItem, currency: Models.Price.Currency) and read
unit_price from item internally, remove the redundant unitPrice parameter and
the now-dead !item guard inside mapProduct, and update all call sites (including
the product mapping call) to pass (item, currency) only so the function usage
and internal checks are consistent.
In `@packages/integrations/medusajs/src/modules/resources/resources.mapper.ts`:
- Around line 98-106: The mapPaymentPeriod function silently falls back to
'ONE_TIME' when given an unknown paymentType; update mapPaymentPeriod to log a
warning when normalization yields a non‑valid period (using the module's logger
or an injected logger), include the original paymentType and the normalized
value in the message for debugging, and keep using VALID_PAYMENT_PERIODS for the
membership check and the existing normalization logic (retain replace(/-/g,'_'))
so behavior is unchanged except the added warning before returning 'ONE_TIME'.
- Around line 88-96: mapContractStatus currently silently returns the default
'ACTIVE' for unknown statuses; modify it to log a warning when the fallback is
used so unexpected Medusa values are visible. In the mapContractStatus function
(and using VALID_CONTRACT_STATUSES), detect when status is not included and call
a logger (prefer an injected logger if available, else console.warn) with a
clear message that includes the original status and the normalized value, then
return 'ACTIVE' as before.
In `@packages/integrations/medusajs/src/utils/address.ts`:
- Around line 31-32: district and region are both assigned from address.province
which is likely a mistake or at least confusing; update the mapping in the
function that builds the output address so that district and region use the
correct source fields (e.g., use address.district for district and
address.region for region) or, if they are intentionally the same, add a short
inline comment next to the district and region assignments explaining that
province is deliberately used for both and why, otherwise leave the unused
property undefined instead of duplicating province.
…usajs-integration-for-checkout-process # Conflicts: # packages/integrations/medusajs/src/modules/medusajs/medusajs.service.spec.ts # packages/integrations/medusajs/src/modules/medusajs/medusajs.service.ts # packages/integrations/medusajs/src/modules/products/products.mapper.ts # packages/integrations/medusajs/src/modules/products/products.service.spec.ts # packages/integrations/medusajs/src/modules/products/products.service.ts
…d access to ProductList block
…API product methods
…nd remove unused address fields
- add setup and usage guides - update cart and checkout documentation
| const params: HttpTypes.StoreProductListParams = { | ||
| limit: query.limit, | ||
| offset: query.offset, | ||
| status: ['published'], |
There was a problem hiding this comment.
Have you checked if the Store API return only published products by default?
marcinkrasowski
deleted the
feat/medusajs-integration-for-checkout-process
branch
2 participants
Summary by CodeRabbit
New Features
Documentation