Repository navigation
Costo de envío al confirmar pedidos - #84
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughSe agregan columnas de envío a Orders, se introduce un pipeline de cotización de envío (endpoint y servicio) que consulta rutas en OpenRouteService, se actualizan creación/actualización de tiendas para manejar precios de envío y se eliminan campos region/postal_code del modelo ShippingZones. Changes
Sequence Diagram(s)sequenceDiagram
actor Usuario
participant Cliente
participant OrderController
participant OrderService
participant DB as BaseDeDatos
participant ORS as OpenRouteService
Usuario->>Cliente: solicita cotización (cartId, addressId)
Cliente->>OrderController: POST /shipping-quote
OrderController->>OrderService: getOrderShippingQuoteService(userId, {cartId,addressId})
OrderService->>DB: obtiene carrito y artículos
DB-->>OrderService: datos del carrito
OrderService->>DB: obtiene dirección del usuario (coords)
DB-->>OrderService: lat/lng destino
OrderService->>DB: obtiene config de envío de la tienda (base_price,distance_price)
DB-->>OrderService: pricing de la tienda
OrderService->>ORS: request distancia de ruta (origen→destino)
ORS-->>OrderService: distance_km
OrderService->>OrderService: buildShippingQuote(distance_km, base_price, distance_price)
Note right of OrderService: aplica DISTANCE_THRESHOLD_KM = 2 km
OrderService-->>OrderController: {shipping_cost, distance_km, rate_type, ...}
OrderController-->>Cliente: 200 JSON {quote}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutos Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/modules/users/orders/order.routes.js (1)
17-18: Sumá validación de body enPOST /shipping-quote.La ruta sólo autentica y después reenvía
req.bodytal cual al servicio. Para una cotización que depende de IDs del carrito/dirección y puede disparar integraciones externas, conviene cortar antes payloads inválidos con un DTO o middleware de validación.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/users/orders/order.routes.js` around lines 17 - 18, Add request-body validation to the POST /shipping-quote route: instead of forwarding req.body directly in orderRouter.post("/shipping-quote", authenticate, getOrderShippingQuote), introduce a validation middleware or DTO (e.g., ShippingQuoteDTO or validateShippingQuote) that checks required fields (cartId, addressId, maybe items/quantities or currency) and types/format before calling getOrderShippingQuote; update the route to orderRouter.post("/shipping-quote", authenticate, validateShippingQuote, getOrderShippingQuote) and make getOrderShippingQuote assume a validated payload (or pull validated data from req.validated/body) so invalid requests are rejected early with a 400 and logging.src/modules/users/orders/order.service.js (1)
39-39: CentralizáDISTANCE_THRESHOLD_KMen un solo lugar.El mismo
2también quedó hardcodeado ensrc/modules/commerce/commerces/store.service.js:329-336. Si cambia en uno solo, la UI va a exponer un umbral distinto al que usa checkout para cotizar/cobrar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/users/orders/order.service.js` at line 39, Centraliza el valor mágico 2 usado como umbral de distancia: crea y exporta una constante compartida (ej. DELIVERY_DISTANCE_THRESHOLD_KM) en el módulo de configuración/global (o settings) y reemplaza el uso directo de DISTANCE_THRESHOLD_KM = 2 en order.service.js y el hardcode equivalente en store.service.js por una importación de esa constante; asegúrate de actualizar cualquier referencia en funciones que calculen tarifas o validen distancia para usar la nueva constante exportada y ajustar/ejecutar tests relacionados.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/docs/schemas/commerce/store.schema.js`:
- Around line 53-55: El schema actual declara base_price y distance_price como
números independientes, pero la API exige que vengan como par (ambos presentes o
ninguno). Updatea el schema en store.schema.js (referencia a las propiedades
base_price y distance_price) para forzar la relación "ambos o ninguno":
reemplaza la simple declaración por una combinación de validadores JSON
Schema/OpenAPI (por ejemplo un oneOf que contenga 1) un objeto con required:
["base_price","distance_price"] y 2) un objeto que niegue la presencia de
cualquiera de los dos cuando el otro falta — de ese modo el spec refleja que
solo se aceptan payloads con ambos campos o sin ninguno.
- Around line 27-33: Los campos city, region y postal_code aparecen en el schema
de input pero son calculados por createStoreService a partir de
latitude/longitude; actualiza el schema para que esos tres campos no formen
parte del payload editable (por ejemplo marcándolos como readOnly: true o
moviéndolos al schema de response y quitándolos de required), dejando
latitude/longitude como inputs requeridos; asegúrate de ajustar las propiedades
city, region y postal_code en src/docs/schemas/commerce/store.schema.js (los
nombres city, region, postal_code) para reflejar que el cliente no debe
enviarlos.
In `@src/modules/commerce/commerces/store.service.js`:
- Around line 771-785: Aquí se asume una única zona activa pero el código usa
prisma.shippingZones.findFirst y sólo actualiza una fila; en lugar de eso usa
prisma.shippingZones.findMany para localizar todas las filas activas para la
tienda (fk_store) y sanea duplicados: actualiza todas las filas relevantes con
shippingZoneDataToUpdate (o marca inactivas las duplicadas) y, si necesitas un
id único, elige/consolida uno (ajustando la variable shippingZoneId) o crea una
nueva fila única; modifica las llamadas que usan
prisma.shippingZones.findFirst/update para usar findMany/updateMany/delete según
corresponda y aplica el mismo cambio en los bloques donde aparece la misma
lógica (referencias: prisma.shippingZones, shippingZoneDataToUpdate,
shippingZoneId).
In `@src/modules/users/orders/order.service.js`:
- Around line 44-46: The code currently throws inside getRouteDistanceKm() when
ORS_API_KEY is missing which causes late runtime failures; update the app
startup validation to fail-fast by enforcing presence of process.env.ORS_API_KEY
during boot. Specifically, add a required-check for ORS_API_KEY in the existing
validateEnv() routine (or the env config initializer) so that validateEnv()
throws a clear ValidationError if ORS_API_KEY is unset, ensuring the server
refuses to start rather than allowing getRouteDistanceKm() to error later.
- Around line 231-239: The createOrderService currently silently defaults
invalid or missing shippingMethod to "pickup", which can ignore addressId and
set shipping_cost to 0; change this by validating shippingMethod in
createOrderService: accept only explicit allowed values (e.g., "pickup" or
"standard"), and if missing or invalid throw a 4xx error instead of defaulting;
additionally enforce that when normalizedShippingMethod === "standard" the
resolvedAddressId must be non-null (otherwise reject) so orders that require
shipping cannot be created without a valid address.
---
Nitpick comments:
In `@src/modules/users/orders/order.routes.js`:
- Around line 17-18: Add request-body validation to the POST /shipping-quote
route: instead of forwarding req.body directly in
orderRouter.post("/shipping-quote", authenticate, getOrderShippingQuote),
introduce a validation middleware or DTO (e.g., ShippingQuoteDTO or
validateShippingQuote) that checks required fields (cartId, addressId, maybe
items/quantities or currency) and types/format before calling
getOrderShippingQuote; update the route to orderRouter.post("/shipping-quote",
authenticate, validateShippingQuote, getOrderShippingQuote) and make
getOrderShippingQuote assume a validated payload (or pull validated data from
req.validated/body) so invalid requests are rejected early with a 400 and
logging.
In `@src/modules/users/orders/order.service.js`:
- Line 39: Centraliza el valor mágico 2 usado como umbral de distancia: crea y
exporta una constante compartida (ej. DELIVERY_DISTANCE_THRESHOLD_KM) en el
módulo de configuración/global (o settings) y reemplaza el uso directo de
DISTANCE_THRESHOLD_KM = 2 en order.service.js y el hardcode equivalente en
store.service.js por una importación de esa constante; asegúrate de actualizar
cualquier referencia en funciones que calculen tarifas o validen distancia para
usar la nueva constante exportada y ajustar/ejecutar tests relacionados.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73b5943d-0798-4527-86d6-d29da12fda57
📒 Files selected for processing (10)
prisma/migrations/20260407023727_change_datatype_to_longitude/migration.sqlprisma/migrations/20260407033841_deleted_unused_shipping_zones_fields/migration.sqlprisma/migrations/20260407043739_added_shipping_costs_to_orders/migration.sqlprisma/schema.prismasrc/docs/schemas/commerce/store.schema.jssrc/modules/commerce/commerces/store.service.jssrc/modules/global/dtos/shipping-zones/shipping-zone.dto.jssrc/modules/users/orders/order.controller.jssrc/modules/users/orders/order.routes.jssrc/modules/users/orders/order.service.js
💤 Files with no reviewable changes (1)
- src/modules/global/dtos/shipping-zones/shipping-zone.dto.js
Comment on lines
+27
to
+33
| latitude: { type: "number", example: -25.2961 }, | ||
| longitude: { type: "number", example: -57.6222 }, | ||
| city: { type: "string", maxLength: 100, example: "Asunción" }, | ||
| region: { type: "string", maxLength: 100, example: "Villa Morra" }, | ||
| postal_code: { type: "string", maxLength: 20, nullable: true, example: "1209" } | ||
| postal_code: { type: "string", maxLength: 20, nullable: true, example: "1209" }, | ||
| base_price: { type: "number", minimum: 0, example: 2500 }, | ||
| distance_price: { type: "number", minimum: 0, example: 4000 } |
Contributor
There was a problem hiding this comment.
No documentes como input campos que el alta recalcula sola.
createStoreService deriva city, region y postal_code desde latitude y longitude. Si Swagger los sigue mostrando como parte del request, parece que el cliente puede controlarlos cuando en realidad esos valores se ignoran.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/docs/schemas/commerce/store.schema.js` around lines 27 - 33, Los campos
city, region y postal_code aparecen en el schema de input pero son calculados
por createStoreService a partir de latitude/longitude; actualiza el schema para
que esos tres campos no formen parte del payload editable (por ejemplo
marcándolos como readOnly: true o moviéndolos al schema de response y
quitándolos de required), dejando latitude/longitude como inputs requeridos;
asegúrate de ajustar las propiedades city, region y postal_code en
src/docs/schemas/commerce/store.schema.js (los nombres city, region,
postal_code) para reflejar que el cliente no debe enviarlos.
Comment on lines
+53
to
+55
| , | ||
| base_price: { type: "number", minimum: 0 }, | ||
| distance_price: { type: "number", minimum: 0 } |
Contributor
There was a problem hiding this comment.
Expresá en OpenAPI que base_price y distance_price son un par.
El update del comercio rechaza payloads donde llegue uno sin el otro, pero este schema los deja como opcionales independientes. La documentación termina validando requests que siempre van a caer en 400.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/docs/schemas/commerce/store.schema.js` around lines 53 - 55, El schema
actual declara base_price y distance_price como números independientes, pero la
API exige que vengan como par (ambos presentes o ninguno). Updatea el schema en
store.schema.js (referencia a las propiedades base_price y distance_price) para
forzar la relación "ambos o ninguno": reemplaza la simple declaración por una
combinación de validadores JSON Schema/OpenAPI (por ejemplo un oneOf que
contenga 1) un objeto con required: ["base_price","distance_price"] y 2) un
objeto que niegue la presencia de cualquiera de los dos cuando el otro falta —
de ese modo el spec refleja que solo se aceptan payloads con ambos campos o sin
ninguno.
Comment on lines
+771
to
+785
| if (Object.keys(shippingZoneDataToUpdate).length > 0) { | ||
| const existingZone = await prisma.shippingZones.findFirst({ | ||
| where: { | ||
| fk_store: store.id_store, | ||
| status: true | ||
| }, | ||
| orderBy: { | ||
| created_at: "asc" | ||
| }, | ||
| select: { | ||
| id_shipping_zone: true | ||
| } | ||
| }); | ||
|
|
||
| shippingZoneId = existingZone?.id_shipping_zone ?? null; |
Contributor
There was a problem hiding this comment.
No asumas una única zona activa por comercio sin respaldarlo en la base.
Acá se busca y actualiza sólo la primera shippingZone activa. Como la PR ya eliminó los campos que distinguían zonas (region/postal_code), cualquier comercio con varias filas históricas queda con registros indistinguibles y sólo uno se corrige; el resto puede seguir saliendo en la API con precios viejos. Hace falta sanear duplicados y forzar unicidad por fk_store.
Also applies to: 832-845
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/commerce/commerces/store.service.js` around lines 771 - 785, Aquí
se asume una única zona activa pero el código usa prisma.shippingZones.findFirst
y sólo actualiza una fila; en lugar de eso usa prisma.shippingZones.findMany
para localizar todas las filas activas para la tienda (fk_store) y sanea
duplicados: actualiza todas las filas relevantes con shippingZoneDataToUpdate (o
marca inactivas las duplicadas) y, si necesitas un id único, elige/consolida uno
(ajustando la variable shippingZoneId) o crea una nueva fila única; modifica las
llamadas que usan prisma.shippingZones.findFirst/update para usar
findMany/updateMany/delete según corresponda y aplica el mismo cambio en los
bloques donde aparece la misma lógica (referencias: prisma.shippingZones,
shippingZoneDataToUpdate, shippingZoneId).
Comment on lines
+44
to
+46
| if (!process.env.ORS_API_KEY) { | ||
| throw new ValidationError("Servicio de geolocalización no disponible"); | ||
| } |
Contributor
There was a problem hiding this comment.
Hacé fail-fast si falta ORS_API_KEY.
getRouteDistanceKm() ya depende de esta variable, pero validateEnv() corre al boot (src/app.js:1-2) y src/config/env.config.js:1-22 todavía no la exige. Si falta en producción, el server arranca igual y recién rompe en la primera cotización o confirmación con envío estándar.
🔧 Cambio sugerido fuera de este archivo
const requiredEnvVars = [
'DATABASE_URL',
'DIRECT_URL',
'JWT_SECRET',
+ 'ORS_API_KEY',
'SUPABASE_URL',
'SUPABASE_SERVICE_ROLE_KEY',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/users/orders/order.service.js` around lines 44 - 46, The code
currently throws inside getRouteDistanceKm() when ORS_API_KEY is missing which
causes late runtime failures; update the app startup validation to fail-fast by
enforcing presence of process.env.ORS_API_KEY during boot. Specifically, add a
required-check for ORS_API_KEY in the existing validateEnv() routine (or the env
config initializer) so that validateEnv() throws a clear ValidationError if
ORS_API_KEY is unset, ensuring the server refuses to start rather than allowing
getRouteDistanceKm() to error later.
Comment on lines
+231
to
+239
| export const createOrderService = async ( | ||
| authenticatedUserId, | ||
| { cartId, addressId, notes, shippingMethod = "pickup" } | ||
| ) => { | ||
| const resolvedUserId = parsePositiveInteger(authenticatedUserId, "userId"); | ||
| const resolvedCartId = parsePositiveInteger(cartId, "cartId"); | ||
| const resolvedAddressId = addressId ? parsePositiveInteger(addressId, "addressId") : null; | ||
| const normalizedShippingMethod = | ||
| shippingMethod === "standard" ? "standard" : "pickup"; |
Contributor
There was a problem hiding this comment.
El fallback implícito a pickup cambia silenciosamente el contrato del endpoint.
Si un cliente existente no manda shippingMethod —o lo manda mal— acá se crea un pedido pickup, se ignora la dirección y shipping_cost queda en 0. Eso puede dejar pedidos imposibles de despachar en vez de rechazar la solicitud.
🔧 Propuesta
export const createOrderService = async (
authenticatedUserId,
- { cartId, addressId, notes, shippingMethod = "pickup" }
+ { cartId, addressId, notes, shippingMethod }
) => {
const resolvedUserId = parsePositiveInteger(authenticatedUserId, "userId");
const resolvedCartId = parsePositiveInteger(cartId, "cartId");
const resolvedAddressId = addressId ? parsePositiveInteger(addressId, "addressId") : null;
- const normalizedShippingMethod =
- shippingMethod === "standard" ? "standard" : "pickup";
+ const normalizedShippingMethod =
+ shippingMethod ?? (resolvedAddressId ? "standard" : "pickup");
+
+ if (!["pickup", "standard"].includes(normalizedShippingMethod)) {
+ throw new ValidationError("Método de envío inválido");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/users/orders/order.service.js` around lines 231 - 239, The
createOrderService currently silently defaults invalid or missing shippingMethod
to "pickup", which can ignore addressId and set shipping_cost to 0; change this
by validating shippingMethod in createOrderService: accept only explicit allowed
values (e.g., "pickup" or "standard"), and if missing or invalid throw a 4xx
error instead of defaulting; additionally enforce that when
normalizedShippingMethod === "standard" the resolvedAddressId must be non-null
(otherwise reject) so orders that require shipping cannot be created without a
valid address.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/modules/commerce/commerces/store.service.js (1)
10-10: ConstanteDISTANCE_THRESHOLD_KMduplicada en el codebase.Esta constante también está definida en
order.service.js:39. Si algún archivo se modifica sin actualizar el otro, las cotizaciones de envío serán inconsistentes (ej: un umbral de 2 km en un lado y otro valor diferente en el otro).Extraer a un módulo compartido o archivo de configuración centralizado.
♻️ Sugerencia de refactor
Crear un archivo compartido como
src/lib/shipping-config.js:export const DISTANCE_THRESHOLD_KM = 2;Luego importarlo en ambos servicios:
-const DISTANCE_THRESHOLD_KM = 2; +import { DISTANCE_THRESHOLD_KM } from "../../../lib/shipping-config.js";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/commerce/commerces/store.service.js` at line 10, La constante DISTANCE_THRESHOLD_KM está duplicada; extraela a un módulo compartido (por ejemplo exportar DISTANCE_THRESHOLD_KM desde un nuevo archivo como shipping-config.js) y reemplaza la definición local en store.service.js y en order.service.js por una importación desde ese módulo; asegúrate de exportar la constante con el mismo nombre y actualizar las referencias en ambos servicios para que ambos usen la misma fuente de verdad.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/commerce/commerces/store.service.js`:
- Around line 194-215: The validateNonNegativeNumberField function currently
throws the same "no puede ser negativo" message for non-numeric/Infinite values;
change the validation order and messages: after computing parsedValue =
Number(value), first check if Number.isFinite(parsedValue) is false and throw a
400 with a message like `${fieldName} debe ser un número válido` (to cover
NaN/Infinity/non-numeric strings), then separately check if parsedValue < 0 and
throw the existing `${fieldName} no puede ser negativo` error; keep the final
return Number(parsedValue.toFixed(2)). Use the existing function name
validateNonNegativeNumberField and the parsedValue variable to locate where to
adjust the checks and messages.
---
Nitpick comments:
In `@src/modules/commerce/commerces/store.service.js`:
- Line 10: La constante DISTANCE_THRESHOLD_KM está duplicada; extraela a un
módulo compartido (por ejemplo exportar DISTANCE_THRESHOLD_KM desde un nuevo
archivo como shipping-config.js) y reemplaza la definición local en
store.service.js y en order.service.js por una importación desde ese módulo;
asegúrate de exportar la constante con el mismo nombre y actualizar las
referencias en ambos servicios para que ambos usen la misma fuente de verdad.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f387ba6-26a9-4206-beff-2b4d1dc6ea2e
📒 Files selected for processing (1)
src/modules/commerce/commerces/store.service.js
Comment on lines
+194
to
+215
| const validateNonNegativeNumberField = (value, fieldName) => { | ||
| if ( | ||
| value === null || | ||
| value === undefined || | ||
| (typeof value === "string" && value.trim() === "") | ||
| ) { | ||
| throw { | ||
| status: 400, | ||
| message: `${fieldName} es requerido` | ||
| }; | ||
| } | ||
|
|
||
| const parsedValue = Number(value); | ||
| if (!Number.isFinite(parsedValue) || parsedValue < 0) { | ||
| throw { | ||
| status: 400, | ||
| message: `${fieldName} no puede ser negativo` | ||
| }; | ||
| } | ||
|
|
||
| return Number(parsedValue.toFixed(2)); | ||
| }; |
Contributor
There was a problem hiding this comment.
Mensaje de error impreciso para valores no numéricos.
Cuando el valor es NaN o Infinity (ej: "abc"), el mensaje dice "no puede ser negativo" lo cual es incorrecto. Deberían diferenciarse los casos.
🔧 Corrección sugerida
const validateNonNegativeNumberField = (value, fieldName) => {
if (
value === null ||
value === undefined ||
(typeof value === "string" && value.trim() === "")
) {
throw {
status: 400,
message: `${fieldName} es requerido`
};
}
const parsedValue = Number(value);
- if (!Number.isFinite(parsedValue) || parsedValue < 0) {
+ if (!Number.isFinite(parsedValue)) {
+ throw {
+ status: 400,
+ message: `${fieldName} debe ser un número válido`
+ };
+ }
+
+ if (parsedValue < 0) {
throw {
status: 400,
message: `${fieldName} no puede ser negativo`
};
}
return Number(parsedValue.toFixed(2));
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const validateNonNegativeNumberField = (value, fieldName) => { | |
| if ( | |
| value === null || | |
| value === undefined || | |
| (typeof value === "string" && value.trim() === "") | |
| ) { | |
| throw { | |
| status: 400, | |
| message: `${fieldName} es requerido` | |
| }; | |
| } | |
| const parsedValue = Number(value); | |
| if (!Number.isFinite(parsedValue) || parsedValue < 0) { | |
| throw { | |
| status: 400, | |
| message: `${fieldName} no puede ser negativo` | |
| }; | |
| } | |
| return Number(parsedValue.toFixed(2)); | |
| }; | |
| const validateNonNegativeNumberField = (value, fieldName) => { | |
| if ( | |
| value === null || | |
| value === undefined || | |
| (typeof value === "string" && value.trim() === "") | |
| ) { | |
| throw { | |
| status: 400, | |
| message: `${fieldName} es requerido` | |
| }; | |
| } | |
| const parsedValue = Number(value); | |
| if (!Number.isFinite(parsedValue)) { | |
| throw { | |
| status: 400, | |
| message: `${fieldName} debe ser un número válido` | |
| }; | |
| } | |
| if (parsedValue < 0) { | |
| throw { | |
| status: 400, | |
| message: `${fieldName} no puede ser negativo` | |
| }; | |
| } | |
| return Number(parsedValue.toFixed(2)); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/commerce/commerces/store.service.js` around lines 194 - 215, The
validateNonNegativeNumberField function currently throws the same "no puede ser
negativo" message for non-numeric/Infinite values; change the validation order
and messages: after computing parsedValue = Number(value), first check if
Number.isFinite(parsedValue) is false and throw a 400 with a message like
`${fieldName} debe ser un número válido` (to cover NaN/Infinity/non-numeric
strings), then separately check if parsedValue < 0 and throw the existing
`${fieldName} no puede ser negativo` error; keep the final return
Number(parsedValue.toFixed(2)). Use the existing function name
validateNonNegativeNumberField and the parsedValue variable to locate where to
adjust the checks and messages.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/stores.test.js (1)
308-321: Falta validar el contrato deshipping_zonesen la respuesta del alta.En Line 308-321 ya se mockea
shipping_zones, pero el test exitoso solo verifica201. Conviene afirmar al menos estructura/campos clave para evitar falsos positivos ante regresiones delSTORE_RESPONSE_SELECT.✅ Aserción sugerida
expect(res.status).toBe(201); + expect(res.body).toHaveProperty("shipping_zones"); + expect(Array.isArray(res.body.shipping_zones)).toBe(true); + expect(res.body.shipping_zones[0]).toMatchObject({ + id_shipping_zone: 1, + base_price: 10000, + distance_price: 15000, + status: true, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/stores.test.js` around lines 308 - 321, The test currently mocks shipping_zones (via the mocked findUnique returning mockStore with shipping_zones) but only asserts a 201 status; update the test that posts the store to also validate the shipping_zones contract by asserting response.body.shipping_zones is an array and that at least one element contains the expected keys/values (e.g., id_shipping_zone, fk_store, base_price, distance_price, status, created_at, updated_at) to match the mocked data and guard against STORE_RESPONSE_SELECT regressions; locate the test that calls the route and make these assertions using the existing mockStore/shipping_zones fixture and the response object.tests/unit/order/order.test.js (1)
229-241: Aislamiento mejorable: restaurarORS_API_KEYyfetchaunque el test falle antes.Hoy la limpieza depende de llegar a Line 261. Usá
try/finally(oafterEach) para evitar fugas de estado entre tests.♻️ Patrón sugerido con cleanup seguro
- process.env.ORS_API_KEY = "test-ors-key"; - vi.stubGlobal("fetch", vi.fn().mockResolvedValue({ + const prevOrsApiKey = process.env.ORS_API_KEY; + process.env.ORS_API_KEY = "test-ors-key"; + vi.stubGlobal("fetch", vi.fn().mockResolvedValue({ ok: true, json: vi.fn().mockResolvedValue({ routes: [ { summary: { distance: 1500, }, }, ], }), })); - prisma.$transaction.mockImplementation(async (fn) => fn(prisma)); - prisma.orders.create.mockResolvedValue({ id_order: 100 }); - prisma.orderItems.createMany.mockResolvedValue({}); - prisma.carts.update.mockResolvedValue({}); - prisma.orders.findUnique.mockResolvedValue(mockOrderFromDB); - - const result = await createOrderService(1, { - cartId: 1, - addressId: 1, - notes: "entregar en la mañana", - shippingMethod: "standard", - }); - - expect(result).toMatchObject({ - id: 100, - status: "PENDING", - total: 200, - }); - - vi.unstubAllGlobals(); + try { + prisma.$transaction.mockImplementation(async (fn) => fn(prisma)); + prisma.orders.create.mockResolvedValue({ id_order: 100 }); + prisma.orderItems.createMany.mockResolvedValue({}); + prisma.carts.update.mockResolvedValue({}); + prisma.orders.findUnique.mockResolvedValue(mockOrderFromDB); + + const result = await createOrderService(1, { + cartId: 1, + addressId: 1, + notes: "entregar en la mañana", + shippingMethod: "standard", + }); + + expect(result).toMatchObject({ + id: 100, + status: "PENDING", + total: 200, + }); + } finally { + vi.unstubAllGlobals(); + process.env.ORS_API_KEY = prevOrsApiKey; + }Also applies to: 261-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/order/order.test.js` around lines 229 - 241, El test está dejando mutado el estado global (process.env.ORS_API_KEY y el stub de fetch creado con vi.stubGlobal("fetch")), por lo que hay que asegurarse de restaurarlos aunque el test falle; envolvé el bloque de preparación/ejecución en un try/finally (o mové la limpieza a un afterEach) que guarde los valores originales (e.g. const _origApiKey = process.env.ORS_API_KEY; const _origFetch = global.fetch) y en finally los restaure (process.env.ORS_API_KEY = _origApiKey; global.fetch = _origFetch) o llamá a las utilidades de vitest para limpiar mocks (p. ej. vi.restoreAllMocks()/vi.resetAllMocks()/vi.unstubAllGlobals()) después del test, asegurándote de referenciar explícitamente ORS_API_KEY y la stub creada por vi.stubGlobal("fetch").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/order/order.test.js`:
- Around line 225-228: El mock de prisma.shippingZones.findFirst debe incluir la
propiedad id_shipping_zone para coincidir con el contrato consumido en
src/modules/users/orders/order.service.js; actualiza el mock en
tests/unit/order/order.test.js agregando id_shipping_zone (con el mismo tipo que
usa el servicio, p. ej. number o string) y cualquier otro campo que
order.service.js lea entre las líneas donde selecciona/consume id_shipping_zone
para evitar pasar una forma inválida.
---
Nitpick comments:
In `@tests/e2e/stores.test.js`:
- Around line 308-321: The test currently mocks shipping_zones (via the mocked
findUnique returning mockStore with shipping_zones) but only asserts a 201
status; update the test that posts the store to also validate the shipping_zones
contract by asserting response.body.shipping_zones is an array and that at least
one element contains the expected keys/values (e.g., id_shipping_zone, fk_store,
base_price, distance_price, status, created_at, updated_at) to match the mocked
data and guard against STORE_RESPONSE_SELECT regressions; locate the test that
calls the route and make these assertions using the existing
mockStore/shipping_zones fixture and the response object.
In `@tests/unit/order/order.test.js`:
- Around line 229-241: El test está dejando mutado el estado global
(process.env.ORS_API_KEY y el stub de fetch creado con vi.stubGlobal("fetch")),
por lo que hay que asegurarse de restaurarlos aunque el test falle; envolvé el
bloque de preparación/ejecución en un try/finally (o mové la limpieza a un
afterEach) que guarde los valores originales (e.g. const _origApiKey =
process.env.ORS_API_KEY; const _origFetch = global.fetch) y en finally los
restaure (process.env.ORS_API_KEY = _origApiKey; global.fetch = _origFetch) o
llamá a las utilidades de vitest para limpiar mocks (p. ej.
vi.restoreAllMocks()/vi.resetAllMocks()/vi.unstubAllGlobals()) después del test,
asegurándote de referenciar explícitamente ORS_API_KEY y la stub creada por
vi.stubGlobal("fetch").
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e586b5d6-8e51-474a-92c8-1fca1f463f4a
📒 Files selected for processing (2)
tests/e2e/stores.test.jstests/unit/order/order.test.js
Comment on lines
+225
to
+228
| prisma.shippingZones.findFirst.mockResolvedValue({ | ||
| base_price: 10000, | ||
| distance_price: 15000, | ||
| }); |
Contributor
There was a problem hiding this comment.
El mock de shippingZones.findFirst está incompleto para el contrato real.
En src/modules/users/orders/order.service.js (Line 108-155) se selecciona y consume id_shipping_zone. Si el mock no lo trae, el test puede pasar con una forma inválida.
🧩 Ajuste sugerido del mock
prisma.shippingZones.findFirst.mockResolvedValue({
+ id_shipping_zone: 1,
base_price: 10000,
distance_price: 15000,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/order/order.test.js` around lines 225 - 228, El mock de
prisma.shippingZones.findFirst debe incluir la propiedad id_shipping_zone para
coincidir con el contrato consumido en
src/modules/users/orders/order.service.js; actualiza el mock en
tests/unit/order/order.test.js agregando id_shipping_zone (con el mismo tipo que
usa el servicio, p. ej. number o string) y cualquier otro campo que
order.service.js lea entre las líneas donde selecciona/consume id_shipping_zone
para evitar pasar una forma inválida.
SebaKisser
approved these changes
Apr 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests