Support pickup-point delivery#28
Conversation
📝 WalkthroughWalkthroughImplements delivery-method-driven order flow with Sendcloud pickup-point support: env/config parsing for multi-carrier method IDs, order validation/input normalization for deliveryMethod, pickup-point normalization and snapshot storage on orders, shipping-method resolution for pickup_point vs home, shipment creation mapping to Sendcloud service points, tests, and docs updates. ChangesPickup-Point Delivery Backend
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
CherryCIC
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e85319fc69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const methodId = String( | ||
| configuredMethod?.id || order.shippingOptionId || '', | ||
| ).trim(); |
There was a problem hiding this comment.
resolveShippingMethodId currently prioritizes configuredMethod?.id ahead of order.shippingOptionId, so a stale or mismatched SENDCLOUD_PICKUP_POINT_SHIPPING_METHOD_IDS entry can override the method that was validated and stored at checkout. In that case, shipment creation can use the wrong Sendcloud method (or fail) even though the order has a valid selected method, and the shipment record can diverge from the order’s shipping option.
Useful? React with 👍 / 👎.
| shippingMethodId: | ||
| body.shippingMethodId || body.shipping_method_id || body.shippingOptionId, | ||
| shippingCarrier: body.shippingCarrier || body.shipping_carrier, | ||
| shippingWeight: body.shippingWeight || body.shipping_weight, |
There was a problem hiding this comment.
Using || here coerces shippingWeight: 0 to the fallback path, which then lets Joi apply the default weight instead of failing the min(1) constraint. That means an invalid payload can silently pass and produce a shipment label with an unintended weight, masking client errors and creating incorrect shipping data. Use nullish checks so only truly absent values default.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.env.example (1)
43-43: ⚡ Quick winClarify the example format for multi-carrier mapping.
The example
SENDCLOUD_PICKUP_POINT_SHIPPING_METHOD_IDS=inpost_gb=shows the key format but leaves the value incomplete. Consider either showing a complete example (e.g.,inpost_gb=12345) or leaving it empty like other variables to avoid confusion.📝 Suggested improvement
-SENDCLOUD_PICKUP_POINT_SHIPPING_METHOD_IDS=inpost_gb= +SENDCLOUD_PICKUP_POINT_SHIPPING_METHOD_IDS=inpost_gb=12345Or alternatively, leave it empty to match the pattern of other variables:
-SENDCLOUD_PICKUP_POINT_SHIPPING_METHOD_IDS=inpost_gb= +SENDCLOUD_PICKUP_POINT_SHIPPING_METHOD_IDS=🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example at line 43, Update the .env.example entry for SENDCLOUD_PICKUP_POINT_SHIPPING_METHOD_IDS to show a complete example or leave it empty to match others; specifically, replace the incomplete value `inpost_gb=` with either a full example like `inpost_gb=12345` to demonstrate the expected "carrier_key=shipping_method_id" mapping, or set it to an empty value `SENDCLOUD_PICKUP_POINT_SHIPPING_METHOD_IDS=` so it isn’t confusing—refer to the variable name SENDCLOUD_PICKUP_POINT_SHIPPING_METHOD_IDS and the example carrier key `inpost_gb` when making the change.src/modules/order/controllers/orderController.ts (2)
138-194: ⚡ Quick winResolve shipping after payment verification to reduce wasted Sendcloud calls.
resolvePickupPointShippingperforms a live SendcloudgetDeliveryOptionscall beforeverifySucceededPaymentIntentForUser. Any request with an invalid/unsucceededpaymentIntentId(including potential abuse traffic) will still hit Sendcloud, adding latency and external load with no benefit. Verifying the payment first would short-circuit those requests cheaply.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/order/controllers/orderController.ts` around lines 138 - 194, The current flow calls resolvePickupPointShipping (which triggers a Sendcloud getDeliveryOptions) before verifying payment; move the payment verification step (verifySucceededPaymentIntentForUser in PaymentService) above any shipping resolution to short-circuit invalid/unsucceeded paymentIntentIds and avoid external Sendcloud calls; after successful verifySucceededPaymentIntentForUser, run the existing shipping resolution logic (resolvePickupPointShipping for 'pickup_point' or resolveHomeShippingMethodId + normaliseCarrier for home delivery), preserving the same ResponseHandler error responses and catch blocks (including the specific handling for errors that include 'shipping method').
163-178: ⚡ Quick winDistinguish validation vs. unexpected errors via an error type instead of message substring matching.
err.message.includes('shipping method')is fragile — any future error string change inresolvePickupPointShipping,getDeliveryOptions, or downstream Sendcloud calls can silently flip 500s into 400s or vice versa. Throwing a typed/sentinel error (e.g.InvalidShippingMethodError) from the resolver andinstanceof-checking here would be more robust and explicit.♻️ Sketch
- } catch (err) { - if (err instanceof Error && err.message.includes('shipping method')) { + } catch (err) { + if (err instanceof InvalidShippingMethodError) { ResponseHandler.badRequest( res, 'Invalid shipping method', err.message, ); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/order/controllers/orderController.ts` around lines 163 - 178, The catch block in orderController currently classifies validation vs unexpected errors by checking err.message.includes('shipping method'), which is fragile; instead define and throw a specific error class (e.g., InvalidShippingMethodError) from the resolver functions (resolvePickupPointShipping, getDeliveryOptions and any Sendcloud wrapper) when the shipping method is invalid, then update the catch in orderController (the catch handling around the resolve flow) to use instanceof InvalidShippingMethodError to return ResponseHandler.badRequest('Invalid shipping method', ...), and fall back to ResponseHandler.internalServerError for all other errors.src/modules/order/tests/orderController.test.ts (1)
241-285: 💤 Low valueConsider asserting the negative case as well.
This test confirms the auto-resolution path when exactly one pickup-point option exists, but the controller behaviour when multiple options are returned and no
shippingMethodIdis submitted isn't asserted anywhere in this file. Worth adding a follow-up case (e.g., returns 400 / requires explicit selection) to lock down the contract and prevent ambiguous future regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/order/tests/orderController.test.ts` around lines 241 - 285, Add a negative test to cover the case where multiple pickup-point shipping options exist but no shippingMethodId is submitted: in src/modules/order/tests/orderController.test.ts add a new it(...) similar to the existing "uses the only available pickup-point..." test but have the mocked service that lists pickup options return more than one option and call createOrder(req,res) with payloadWithoutShippingMethod; then assert that createOrder does NOT call mockCreateOrder (or is not invoked with auto-populated shippingOptionId) and that the response returns the expected error status (e.g., res.status called with 400) and error body; reference the existing symbols createOrder, mockCreateOrder, payloadWithoutShippingMethod and mockCreateShipmentForPaidOrder to locate where to add and how to mock the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/shipping/services/CheckoutShippingService.ts`:
- Around line 134-139: The carrier fallback can end up as a plain object (e.g.,
{name: 'InPost'}) causing String(carrier).toLowerCase() to become "[object
Object]"; update the fallback logic around the carrier variable (the expression
using point?.carrier?.code, point?.carrier_code, point?.carrier,
point?.carrierName) to prefer carrier.name and carrierName before falling back
to the raw point?.carrier object, and ensure downstream normalization (where
String(carrier).toLowerCase() is used) always receives a string (e.g., select
point?.carrier?.code || point?.carrier?.name || point?.carrierName ||
point?.carrier_code || null and coerce to a string only after choosing a scalar
value).
In `@src/modules/shipping/services/ShipmentService.ts`:
- Around line 29-57: The parcelData currently always sets house_number from
order.shipping.address.house_number which mismatches pickup-point destinations;
update the logic in ShipmentService so that when deliveryType === 'pickup_point'
(i.e. when destinationAddress is returned by getPickupPointDestination and
parcelData.to_service_point is set using toServicePointId(pickupPoint.id)) the
parcelData.house_number is omitted or taken from the pickupPoint data (from
getPickupPointDestination) instead of order.shipping.address.house_number;
adjust the construction of parcelData (or post-process it) to source
house_number from destinationAddress when present or delete it for service-point
deliveries to keep parcelData consistent with destinationAddress and
to_service_point.
---
Nitpick comments:
In @.env.example:
- Line 43: Update the .env.example entry for
SENDCLOUD_PICKUP_POINT_SHIPPING_METHOD_IDS to show a complete example or leave
it empty to match others; specifically, replace the incomplete value
`inpost_gb=` with either a full example like `inpost_gb=12345` to demonstrate
the expected "carrier_key=shipping_method_id" mapping, or set it to an empty
value `SENDCLOUD_PICKUP_POINT_SHIPPING_METHOD_IDS=` so it isn’t confusing—refer
to the variable name SENDCLOUD_PICKUP_POINT_SHIPPING_METHOD_IDS and the example
carrier key `inpost_gb` when making the change.
In `@src/modules/order/controllers/orderController.ts`:
- Around line 138-194: The current flow calls resolvePickupPointShipping (which
triggers a Sendcloud getDeliveryOptions) before verifying payment; move the
payment verification step (verifySucceededPaymentIntentForUser in
PaymentService) above any shipping resolution to short-circuit
invalid/unsucceeded paymentIntentIds and avoid external Sendcloud calls; after
successful verifySucceededPaymentIntentForUser, run the existing shipping
resolution logic (resolvePickupPointShipping for 'pickup_point' or
resolveHomeShippingMethodId + normaliseCarrier for home delivery), preserving
the same ResponseHandler error responses and catch blocks (including the
specific handling for errors that include 'shipping method').
- Around line 163-178: The catch block in orderController currently classifies
validation vs unexpected errors by checking err.message.includes('shipping
method'), which is fragile; instead define and throw a specific error class
(e.g., InvalidShippingMethodError) from the resolver functions
(resolvePickupPointShipping, getDeliveryOptions and any Sendcloud wrapper) when
the shipping method is invalid, then update the catch in orderController (the
catch handling around the resolve flow) to use instanceof
InvalidShippingMethodError to return ResponseHandler.badRequest('Invalid
shipping method', ...), and fall back to ResponseHandler.internalServerError for
all other errors.
In `@src/modules/order/tests/orderController.test.ts`:
- Around line 241-285: Add a negative test to cover the case where multiple
pickup-point shipping options exist but no shippingMethodId is submitted: in
src/modules/order/tests/orderController.test.ts add a new it(...) similar to the
existing "uses the only available pickup-point..." test but have the mocked
service that lists pickup options return more than one option and call
createOrder(req,res) with payloadWithoutShippingMethod; then assert that
createOrder does NOT call mockCreateOrder (or is not invoked with auto-populated
shippingOptionId) and that the response returns the expected error status (e.g.,
res.status called with 400) and error body; reference the existing symbols
createOrder, mockCreateOrder, payloadWithoutShippingMethod and
mockCreateShipmentForPaidOrder to locate where to add and how to mock the
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e9a126c3-3c76-404b-8fd3-b0bc306e8be4
📒 Files selected for processing (26)
.env.exampleREADME.mddocs/pickup-point-delivery.mdsrc/modules/order/controllers/orderController.tssrc/modules/order/model/Order.tssrc/modules/order/repositories/OrderRepository.tssrc/modules/order/routes/orderRoutes.tssrc/modules/order/tests/orderController.test.tssrc/modules/order/tests/orderRepository.test.tssrc/modules/order/tests/orderValidator.test.tssrc/modules/order/validators/orderValidator.tssrc/modules/shipping/controllers/shippingController.tssrc/modules/shipping/models/Shipment.tssrc/modules/shipping/routes/shippingRoutes.tssrc/modules/shipping/services/CheckoutShippingService.tssrc/modules/shipping/services/SendcloudService.tssrc/modules/shipping/services/ShipmentService.tssrc/modules/shipping/services/shippingMethodResolver.tssrc/modules/shipping/tests/checkoutShippingService.test.tssrc/modules/shipping/tests/sendcloudService.test.tssrc/modules/shipping/tests/shipmentService.test.tssrc/modules/shipping/tests/shippingController.test.tssrc/modules/shipping/tests/shippingValidator.test.tssrc/modules/shipping/validators/shippingValidator.tssrc/shared/config/sendcloudConfig.tstsconfig.json
| const carrier = | ||
| point?.carrier?.code || | ||
| point?.carrier_code || | ||
| point?.carrier || | ||
| point?.carrierName || | ||
| null; |
There was a problem hiding this comment.
Carrier fallback can yield "[object object]" when carrier is an object without a code.
The fallback chain matches point?.carrier?.code first (anticipating object form), but if point.carrier is an object like { name: 'InPost' } (no code), point?.carrier_code is undefined, then point?.carrier resolves to the object itself. String(carrier).toLowerCase() on line 157 then produces "[object object]" as the normalized carrier value, which will mismatch downstream carrier-based shipping-method resolution.
🛡️ Suggested fix: prefer carrier.name before falling back to the raw object
const carrier =
point?.carrier?.code ||
+ point?.carrier?.name ||
point?.carrier_code ||
- point?.carrier ||
point?.carrierName ||
+ (typeof point?.carrier === 'string' ? point.carrier : null) ||
null;Also applies to: 157-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/shipping/services/CheckoutShippingService.ts` around lines 134 -
139, The carrier fallback can end up as a plain object (e.g., {name: 'InPost'})
causing String(carrier).toLowerCase() to become "[object Object]"; update the
fallback logic around the carrier variable (the expression using
point?.carrier?.code, point?.carrier_code, point?.carrier, point?.carrierName)
to prefer carrier.name and carrierName before falling back to the raw
point?.carrier object, and ensure downstream normalization (where
String(carrier).toLowerCase() is used) always receives a string (e.g., select
point?.carrier?.code || point?.carrier?.name || point?.carrierName ||
point?.carrier_code || null and coerce to a string only after choosing a scalar
value).
- Fix shipping method resolution to prefer order.shippingOptionId over config for pickup-point delivery - Fix shippingWeight || operator to ?? to prevent 0 bypassing min(1) validation - Fix normaliseCarrier to handle object inputs and prevent "[object Object]" carrier codes - Fix house_number to use empty string for pickup-point parcel destinations
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/modules/order/validators/orderValidator.ts (1)
65-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winImplicit
'home'fallback can mask malformed client payloads.When the request body omits all of
deliveryMethod/delivery_method/deliveryType/delivery_typeandpickupPoint, this normalisesdeliveryMethodto'home'. The Joirequired()rule ondeliveryMethodwill therefore never trip for this case, and a malformed client payload will be silently accepted as a home order rather than rejected as a missing field. Consider leavingdeliveryMethodundefined when no signal is present so the schema'sany.requirederror is surfaced.🛡️ Suggested fix
const deliveryMethod = body.deliveryMethod || body.delivery_method || body.deliveryType || body.delivery_type || - (body.pickupPoint ? 'pickup_point' : 'home'); + (body.pickupPoint ? 'pickup_point' : undefined);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/order/validators/orderValidator.ts` around lines 65 - 70, The current normalization sets deliveryMethod to 'home' by default which hides missing-field errors; change the logic in orderValidator.ts around the deliveryMethod computation (the deliveryMethod variable) so it only takes body.deliveryMethod, body.delivery_method, body.deliveryType, body.delivery_type, or 'pickup_point' when body.pickupPoint is present, and otherwise leave deliveryMethod undefined (no implicit 'home' default) so the Joi required() validation on deliveryMethod can surface a missing-field error.src/modules/shipping/services/ShipmentService.ts (1)
173-178:⚠️ Potential issue | 🔴 Critical | 💤 Low valueFail fast on invalid
to_service_pointvalues.Sendcloud’s API defines
to_service_pointas an integer; passing a non-numeric fallback string risks unexpected coercion/rejection away from the source of the error. SincetoServicePointIdcurrently returns the original string for invalid inputs, changing it to throw early on non-positive/non-integer IDs (e.g.,numberreturn type + error with the received value) will make misuse obvious and improve debugging signal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/shipping/services/ShipmentService.ts` around lines 173 - 178, The toServicePointId helper currently returns the original string on invalid input which hides bad data; change private toServicePointId(servicePointId: string): string | number to instead return a number (remove string return type) and throw a clear error when the parsed value is not a positive integer (use Number(servicePointId), Number.isInteger(...) and > 0) including the received servicePointId in the error message so failures fail fast; update any callers to expect a number or to handle the thrown error accordingly.
🧹 Nitpick comments (1)
src/modules/order/controllers/orderController.ts (1)
164-179: ⚡ Quick winAvoid classifying errors by substring matching.
Deciding between
badRequestandinternalServerErrorbased onerr.message.includes('shipping method')is brittle: any rewording of the messages inresolvePickupPointShipping/resolveHomeShippingMethodId, or any unrelated downstream error that happens to contain the phrase (e.g. fromCheckoutShippingService.getDeliveryOptions), will silently change the HTTP status returned to clients. Prefer a typed sentinel — for example, a dedicatedInvalidShippingMethodErrorsubclass — andinstanceof-check it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/order/controllers/orderController.ts` around lines 164 - 179, Replace brittle substring matching on err.message with a typed sentinel error: create a dedicated error class (e.g., InvalidShippingMethodError) and throw it from the functions that validate shipping (e.g., resolvePickupPointShipping, resolveHomeShippingMethodId, or CheckoutShippingService.getDeliveryOptions) when the shipping method is invalid; then change the catch in orderController.ts to check err instanceof InvalidShippingMethodError and call ResponseHandler.badRequest, otherwise call ResponseHandler.internalServerError with err.message. Ensure the new error class is exported from its module and imported into the controller for the instanceof check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/modules/order/validators/orderValidator.ts`:
- Around line 65-70: The current normalization sets deliveryMethod to 'home' by
default which hides missing-field errors; change the logic in orderValidator.ts
around the deliveryMethod computation (the deliveryMethod variable) so it only
takes body.deliveryMethod, body.delivery_method, body.deliveryType,
body.delivery_type, or 'pickup_point' when body.pickupPoint is present, and
otherwise leave deliveryMethod undefined (no implicit 'home' default) so the Joi
required() validation on deliveryMethod can surface a missing-field error.
In `@src/modules/shipping/services/ShipmentService.ts`:
- Around line 173-178: The toServicePointId helper currently returns the
original string on invalid input which hides bad data; change private
toServicePointId(servicePointId: string): string | number to instead return a
number (remove string return type) and throw a clear error when the parsed value
is not a positive integer (use Number(servicePointId), Number.isInteger(...) and
> 0) including the received servicePointId in the error message so failures fail
fast; update any callers to expect a number or to handle the thrown error
accordingly.
---
Nitpick comments:
In `@src/modules/order/controllers/orderController.ts`:
- Around line 164-179: Replace brittle substring matching on err.message with a
typed sentinel error: create a dedicated error class (e.g.,
InvalidShippingMethodError) and throw it from the functions that validate
shipping (e.g., resolvePickupPointShipping, resolveHomeShippingMethodId, or
CheckoutShippingService.getDeliveryOptions) when the shipping method is invalid;
then change the catch in orderController.ts to check err instanceof
InvalidShippingMethodError and call ResponseHandler.badRequest, otherwise call
ResponseHandler.internalServerError with err.message. Ensure the new error class
is exported from its module and imported into the controller for the instanceof
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a238df40-11b9-405d-b4ae-915b940621ed
📒 Files selected for processing (7)
src/modules/order/controllers/orderController.tssrc/modules/order/tests/orderValidator.test.tssrc/modules/order/validators/orderValidator.tssrc/modules/shipping/services/ShipmentService.tssrc/modules/shipping/services/shippingMethodResolver.tssrc/modules/shipping/tests/shipmentService.test.tssrc/modules/shipping/tests/shippingMethodResolver.test.ts
2 participants
Summary
data.pickupPoints.pickup_pointorders and persist a pickup-point snapshot on the order.to_service_point.Review Fixes
Addressed the PR #28 review follow-up in
5e12567 fix: address PR #28 review issues:order.shippingOptionIdbefore carrier-based Sendcloud config fallback for pickup-point shipments.shippingWeight: 0through normalisation so Joi rejects it withmin(1)instead of applying the default weight.normaliseCarrierusingcode, thenname, avoiding"[object Object]"carrier keys.house_numberfor pickup-point parcel destinations so locker addresses are not mixed with the buyer's home house number.house_number.Validation
npm run buildnpm test -- --runInBandgit diff --checkRelated
And FE PR:
Summary by CodeRabbit