Repository navigation
Conversation
Contributor
|
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:
📝 WalkthroughRecorridoSe introduce un nuevo sistema de gestión de asignaciones de entrega con controladores, servicios y rutas para crear, recuperar y completar asignaciones. Se refactoriza el módulo de deliveries, trasladando funcionalidades a una nueva estructura modular. Se agrega el estado Cambios
Diagramas de SecuenciasequenceDiagram
participant Client
participant AssignmentController
participant AssignmentService
participant Database as Prisma DB
Client->>AssignmentController: POST /api/assignments<br/>(fk_order, fk_delivery, status?)
AssignmentController->>AssignmentController: Validar con Zod schema
AssignmentController->>AssignmentService: createAssignmentService(data)
AssignmentService->>Database: Verificar orders existe
AssignmentService->>Database: Verificar deliveries existe
AssignmentService->>Database: Calcular max(assignment_sequence)
AssignmentService->>Database: Verificar no hay PENDING existente
AssignmentService->>Database: INSERT deliveryAssignments<br/>(PENDING status)
Database-->>AssignmentService: Nueva asignación creada
AssignmentService-->>AssignmentController: Resultado
AssignmentController-->>Client: 201 Creado
sequenceDiagram
participant Client
participant AssignmentController
participant AssignmentService
participant Database as Prisma DB
Client->>AssignmentController: POST /api/assignments/:id/complete<br/>(usuario autenticado)
AssignmentController->>AssignmentService: completeAssignmentService(id, user_id)
AssignmentService->>Database: Obtener deliveryAssignments by id
alt Asignación no existe
AssignmentService-->>AssignmentController: Error 404
else Asignación no está ACCEPTED
AssignmentService-->>AssignmentController: Error 409
else Usuario no es propietario
AssignmentService-->>AssignmentController: Error 403
else Válido
AssignmentService->>Database: UPDATE assignment_status = DELIVERED
AssignmentService->>Database: UPDATE order_status = DELIVERED
Database-->>AssignmentService: Asignación completada
AssignmentService-->>AssignmentController: Resultado
AssignmentController-->>Client: 200 OK
end
Esfuerzo Estimado de Revisión🎯 4 (Complejo) | ⏱️ ~60 minutos PRs Posiblemente Relacionados
Revisores Sugeridos
Poema
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (13)
src/modules/delivery/delivery/delivery.service.js-29-54 (1)
29-54:⚠️ Potential issue | 🟠 MajorFalta validación del rol en el login de delivery.
El endpoint de login para deliveries no verifica que el usuario tenga el rol
DELIVERY. Esto permite que cualquier usuario (CUSTOMER, SELLER, ADMIN) inicie sesión a través de este endpoint y obtenga un token válido, lo cual puede generar confusión en la autorización.🛡️ Corrección propuesta — validar el rol del usuario
export const loginDeliveryService = async (email, password) => { const user = await prisma.users.findUnique({ where: { email } }); if (!user) { throw { status: 404, message: "Usuario no encontrado" }; } + + if (user.role !== "DELIVERY") { + throw { status: 403, message: "El usuario no es un delivery" }; + } const passwordMatch = await bcrypt.compare(password, user.password_hash);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery/delivery.service.js` around lines 29 - 54, En loginDeliveryService valida que user.role === "DELIVERY" antes de generar el token: si el rol no es DELIVERY lanza un error con status 403 y mensaje claro (por ejemplo "Acceso no autorizado para este endpoint"), de modo que solo usuarios con role "DELIVERY" puedan autenticarse por este servicio; usa las variables existentes (user.role, user.id_user) y realiza la comprobación justo después de verificar la contraseña y antes de llamar a generateToken.src/modules/delivery/delivery-assignments/delivery-assignments.routes.js-29-45 (1)
29-45:⚠️ Potential issue | 🟠 MajorOrden de rutas incorrecto — rutas específicas serán capturadas por
/:id.La ruta
GET /:id(línea 30) está definida antes de las rutas más específicas como/orders/:orderId/assignments(línea 33). Express evalúa las rutas en orden, por lo que una petición a/orders/123/assignmentscoincidirá primero con/:iddondeid = "orders".🐛 Corrección propuesta — reordenar rutas
const router = Router(); // Crear asignación (comercio) router.post("/", authenticate, createAssignment); // Aceptar asignación (delivery) router.post("/:id/accept", authenticate, acceptAssignment); // Rechazar asignación (delivery) router.post("/:id/reject", authenticate, rejectAssignment); -// Obtener asignación por ID -router.get("/:id", authenticate, getAssignmentById); - // Obtener asignaciones de un pedido router.get("/orders/:orderId/assignments", authenticate, getOrderAssignments); // Obtener asignaciones de un delivery (con filtro opcional de status) router.get("/deliveries/:deliveryId/assignments", authenticate, getDeliveryAssignments); // Obtener asignaciones PENDING de un delivery router.get("/deliveries/:deliveryId/pending", authenticate, getDeliveryPendingAssignments); // Obtener la asignación aceptada de un pedido router.get("/orders/:orderId/accepted", authenticate, getAcceptedAssignment); // Obtener historial de asignaciones de un pedido router.get("/orders/:orderId/history", authenticate, getAssignmentHistory); +// Obtener asignación por ID (debe ir después de rutas específicas) +router.get("/:id", authenticate, getAssignmentById); + // Borrar asignacion router.delete("/:id", authenticate, deleteAssignment);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-assignments/delivery-assignments.routes.js` around lines 29 - 45, Move the generic route router.get("/:id", authenticate, getAssignmentById) so it is after all more specific routes (router.get("/orders/:orderId/assignments", authenticate, getOrderAssignments), router.get("/deliveries/:deliveryId/assignments", authenticate, getDeliveryAssignments), router.get("/deliveries/:deliveryId/pending", authenticate, getDeliveryPendingAssignments), router.get("/orders/:orderId/accepted", authenticate, getAcceptedAssignment), and router.get("/orders/:orderId/history", authenticate, getAssignmentHistory)) so that the specific handlers are registered first and won't be shadowed by the catch‑all ":id" route.src/modules/delivery/delivery-assignments/delivery-assignments.validation.js-10-16 (1)
10-16:⚠️ Potential issue | 🟠 MajorLos schemas
acceptAssignmentSchemayrejectAssignmentSchemano se están utilizando.Estos esquemas están importados en el controller pero nunca se aplican. Los handlers
acceptAssignmentyrejectAssignment(líneas 27-46) extraen el ID directamente dereq.paramssin validación, mientras que los schemas validanid_delivery_assignmentesperado en el body. Las rutas definen/:id/accepty/:id/reject, no un body conid_delivery_assignment. Remover estos schemas no utilizados o refactorizar el controller para validar correctamente los parámetros.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-assignments/delivery-assignments.validation.js` around lines 10 - 16, The two Zod schemas acceptAssignmentSchema and rejectAssignmentSchema are unused while acceptAssignment and rejectAssignment handlers pull and trust req.params.id directly; either remove the unused schemas or update the controller to validate route params with Zod. Best fix: replace or adapt the schemas to validate params (e.g., a schema expecting { id: z.string().regex(/^\d+$/) or z.number() after parsing) and then call schema.parse(req.params) at the start of acceptAssignment and rejectAssignment, converting the validated id into the numeric id used downstream; alternatively, delete acceptAssignmentSchema and rejectAssignmentSchema and any imports to avoid dead code.src/modules/delivery/delivery-assignments/delivery-assignments.controller.js-29-30 (1)
29-30:⚠️ Potential issue | 🟠 MajorValidá
id/orderId/deliveryIdy tambiénreq.query.statusen el controller.Hoy los params se convierten con
parseInt()sin chequearNaN, ystatuspasa crudo al service. Un valor inválido termina rebotando como 500 desde Prisma en lugar de 400.Also applies to: 40-41, 51-52, 62-63, 73-75, 85-86, 96-97, 107-108, 118-119, 129-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-assignments/delivery-assignments.controller.js` around lines 29 - 30, Validate numeric route params before using parseInt and validate req.query.status in the controller methods (e.g., where you call acceptAssignmentService(parseInt(id))); check that parseInt yields a finite number (not NaN) and that status is one of the allowed values (or a valid enum/format) before passing to services; if invalid, respond with 400 and a clear error message instead of forwarding invalid values to Prisma. Apply the same pattern to all similar handlers referenced (the blocks around lines that call parseInt on id/orderId/deliveryId and forward req.query.status) so each controller validates and sanitizes inputs and returns 400 for bad requests.src/modules/delivery/delivery-assignments/delivery-assignments.service.js-282-292 (1)
282-292:⚠️ Potential issue | 🟠 MajorCompletá assignment y order en una sola transacción.
Si el update de
deliveryAssignmentssale bien pero el deordersfalla, te queda la asignación en"DELIVERED"con la orden en otro estado. Son dos cambios del mismo evento y deberían ser atómicos.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js` around lines 282 - 292, You’re updating deliveryAssignments and orders separately which can leave them inconsistent if the second update fails; wrap both updates in a single Prisma transaction so they’re atomic: use prisma.$transaction to run prisma.deliveryAssignments.update({ where: { id_delivery_assignment }, data: { assignment_status: "DELIVERED" } }) and prisma.orders.update({ where: { id_order: assignment.fk_order }, data: { order_status: "DELIVERED" } }) together (either as an array or inside an async transaction callback) so both succeed or both are rolled back; return or handle the transaction result and propagate errors as before.src/modules/delivery/delivery-assignments/delivery-assignments.service.js-33-41 (1)
33-41:⚠️ Potential issue | 🟠 Major
status: falseno saca la asignación del resto del flujo.El borrado lógico de
deleteAssignmentServicedejaassignment_status: "PENDING", pero las búsquedas y transiciones posteriores no filtranstatus: true. Resultado: una asignación eliminada sigue bloqueando nuevas asignaciones, aparece en listados y todavía se puede aceptar, rechazar o completar.Also applies to: 55-69, 76-90, 149-181, 244-261, 269-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js` around lines 33 - 41, The pending-assignment checks and state-transition queries (e.g., the findFirst used in create/accept/reject/complete flows in delivery-assignments.service.js) do not filter by the logical-delete flag `status: true`, so logically deleted rows (`status: false`) still block new assignments and can be acted upon; update all queries and state transitions that reference `assignment_status` (the findFirst calls around lines shown and any lookup in functions like deleteAssignmentService, createAssignmentService, accept/reject/complete handlers) to include `status: true` in the Prisma where clauses, and also ensure any update/transition methods first verify `status: true` before proceeding to prevent operations on deleted assignments.src/modules/delivery/delivery-review/delivery-review.service.js-72-85 (1)
72-85:⚠️ Potential issue | 🟠 MajorEl borrado lógico no se respeta en lecturas ni updates.
deleteDeliveryReviewServicesólo ponestatus: false, pero el resto del archivo sigue usandofindUnique/findManysin filtrarstatus: true. Una reseña “eliminada” sigue apareciendo en listados, stats y además todavía se puede actualizar.Also applies to: 104-112, 137-147, 161-176, 193-203, 219-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-review/delivery-review.service.js` around lines 72 - 85, Las lecturas y actualizaciones no respetan el borrado lógico: aunque deleteDeliveryReviewService marca status: false, las consultas usan prisma.deliveryReviews.findUnique / findMany sin filtrar por status, permitiendo ver/editar reseñas "eliminadas". Fix: en todas las consultas y actualizaciones relativas a deliveryReviews (por ejemplo donde se usan prisma.deliveryReviews.findUnique, findMany y update en este archivo y en las funciones updateDeliveryReviewService/list/review stats) añada el filtro adicional { status: true } al where (o combine id_delivery_review con status: true) y para updates confirme primero que la reseña existe y tiene status: true antes de aplicar cambios; use los mismos símbolos prisma.deliveryReviews, deleteDeliveryReviewService, updateDeliveryReviewService y las llamadas a findUnique/findMany para localizar y corregir cada sitio.src/modules/delivery/delivery-review/delivery-review.controller.js-28-31 (1)
28-31:⚠️ Potential issue | 🟠 MajorValidá los params antes de hacer
parseInt.Si entra un
id,orderIdodeliveryIdno numérico, terminás mandandoNaNal service y la respuesta cae en 500 aguas abajo. Esto debería cortarse como 400 en el borde del controller.Also applies to: 41-42, 52-53, 63-65, 75-76, 86-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-review/delivery-review.controller.js` around lines 28 - 31, Valida los params antes de hacer parseInt: en el controller donde se usa getDeliveryReviewByIdService(parseInt(id)) (y en los otros handlers que parsean orderId/deliveryId) comprueba que req.params.id (o orderId/deliveryId) sea un número válido tras parseInt/Number, y si produce NaN responde inmediatamente con res.status(400).json({ error: 'Invalid id' }) (o mensaje equivalente), en lugar de pasar NaN al service; aplica el mismo patrón a los controladores que llaman a getDeliveryReviewByIdService y a los otros endpoints que parsean orderId/deliveryId para evitar 500 aguas abajo.src/modules/delivery/delivery/delivery.controller.js-60-62 (1)
60-62:⚠️ Potential issue | 🟠 MajorCortá los params inválidos antes de llegar al service.
Con
parseInt()sin validación, cualquieridostoreIdno numérico termina comoNaNy se transforma en un 500 aguas abajo. Validalo en el controller y devolvé 400.Also applies to: 72-73, 83-85, 95-96, 106-107, 117-118, 128-129, 139-140, 150-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery/delivery.controller.js` around lines 60 - 62, Valida los params numéricos en el controller antes de llamar a services: para cada handler que usa parseInt on req.params (e.g., the handler calling updateDeliveryStatusService and others referenced), convierte el param con Number(...) o parseInt y comprueba isNaN; si es NaN responde con status 400 y un mensaje de param inválido en lugar de dejar que pase al service; asegúrate de aplicar esto tanto a id como a storeId params y mantener la llamada a updateDeliveryStatusSchema.parse(req.body) intacta antes de invocar updateDeliveryStatusService.src/modules/delivery/delivery-assignments/delivery-assignments.service.js-25-46 (1)
25-46:⚠️ Potential issue | 🟠 MajorLa creación de asignaciones no es atómica.
lastAssignment, el check de"PENDING"y elcreate()corren por separado. Con dos requests concurrentes para el mismo pedido podés calcular el mismoassignment_sequenceo perder la ventana delPENDING, y terminar en un 500 o en estados inconsistentes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js` around lines 25 - 46, The code is vulnerable to race conditions because prisma.deliveryAssignments.findFirst (lastAssignment), the pending check, and prisma.deliveryAssignments.create run separately; wrap those operations in a single database transaction (use prisma.$transaction and use the transaction-scoped client, e.g., tx.deliveryAssignments.findFirst(...) and tx.deliveryAssignments.create(...)) so the sequencing and pending-check are atomic, and additionally enforce uniqueness at the DB level by adding a unique constraint/index on (fk_order, assignment_sequence) and/or a partial unique index to prevent multiple PENDING rows for the same fk_order to guarantee safety even if concurrent transactions race.src/modules/delivery/delivery-assignments/delivery-assignments.controller.js-16-23 (1)
16-23:⚠️ Potential issue | 🟠 MajorValidación que devuelve 500 en lugar de 400 para errores de esquema.
Cuando
createAssignmentSchema.parse()falla, lanza unZodErrorque no tiene propiedad.status. Por lo tanto,error.status || 500siempre evaluará a 500 para errores de validación, cuando debería ser 400. Los errores de validación son errores del cliente, no del servidor.Aplicar el patrón de middleware existente en el proyecto (
src/middlewares/validate.middleware.js) o capturar específicamenteZodErrory retornar 400:} catch (error) { if (error instanceof ZodError) { return res.status(400).json({ error: error.message }); } res.status(error.status || 500).json({ error: error.message }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-assignments/delivery-assignments.controller.js` around lines 16 - 23, The createAssignment controller currently returns 500 for schema validation failures because createAssignmentSchema.parse() throws a ZodError which has no .status; update the catch in createAssignment (exported function createAssignment) to detect ZodError and return 400 (e.g., if (error instanceof ZodError) return res.status(400).json({ error: error.message })), or switch to the project's validate middleware (src/middlewares/validate.middleware.js) to handle parsing errors uniformly; ensure other errors still use error.status || 500.src/modules/delivery/delivery-review/delivery-review.controller.js-17-24 (1)
17-24:⚠️ Potential issue | 🟠 MajorValidaciones de Zod devuelven 500 en lugar de 400
Cuando
schema.parse()falla, lanzaZodErrorsin propiedadstatus. El catch genérico intenta acceder aerror.statusque no existe, cayendo a 500 por defecto.Ubicaciones afectadas
createDeliveryReview(líneas 17-24)updateDeliveryReview(líneas 61-68)También ocurre en otros controladores de delivery que usan el mismo patrón.
Los errores de validación deben responder con status 400, no 500. Considera usar
safeParse()o capturarZodErrorexplícitamente para normalizar a 400.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-review/delivery-review.controller.js` around lines 17 - 24, Los handlers createDeliveryReview y updateDeliveryReview usan createDeliveryReviewSchema.parse / updateDeliveryReviewSchema.parse y en el catch asumen error.status, por eso errores de Zod llegan como 500; cambia a usar schema.safeParse(req.body) y, si safeParse.success es false, responder con res.status(400).json({ error: validationError }) o alternativamente atrapa explícitamente ZodError en el catch (instanceof ZodError) y hacer res.status(400).json({ error: error.errors }) dejando el resto de errores con res.status(500).json({ error: error.message }); actualiza createDeliveryReview y updateDeliveryReview para aplicar este patrón.src/modules/delivery/delivery/delivery.controller.js-25-31 (1)
25-31:⚠️ Potential issue | 🟠 MajorLas validaciones con Zod hoy caen como 500.
Todos estos handlers usan
schema.parse()y después atrapan el error como si fuera interno. Un body inválido debería responder 400, no 500.Also applies to: 36-42, 48-53, 59-65, 83-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery/delivery.controller.js` around lines 25 - 31, The controller currently treats all schema.parse() failures as 500; update the error handling in registerDelivery (and the other handlers using schema.parse()) to detect Zod validation errors and return 400 instead of 500: inside the catch block check for ZodError (e.g., error.name === 'ZodError' or instanceof ZodError) and respond res.status(400).json({ error: 'Validation failed', details: error.errors }) while preserving the existing behavior for other errors (res.status(error.status || 500).json({ error: error.message })); ensure you import ZodError from zod if you use instanceof.
🟡 Minor comments (1)
src/modules/delivery/delivery/delivery.service.js-325-354 (1)
325-354:⚠️ Potential issue | 🟡 MinorConsiderar bloquear eliminación con asignaciones ACCEPTED.
La función verifica que no haya asignaciones
PENDINGantes de eliminar, pero no verifica asignacionesACCEPTED(entregas en curso). Un delivery con entregas activas aceptadas podría ser eliminado lógicamente, lo que podría causar inconsistencias.🛡️ Corrección propuesta — incluir ACCEPTED en la verificación
// Verificar que no hay asignaciones PENDING activas - const pendingAssignments = await prisma.deliveryAssignments.count({ + const activeAssignments = await prisma.deliveryAssignments.count({ where: { fk_delivery: id_delivery, - assignment_status: "PENDING" + assignment_status: { in: ["PENDING", "ACCEPTED"] } } }); - if (pendingAssignments > 0) { - throw { status: 409, message: "No se puede eliminar: hay asignaciones pendientes" }; + if (activeAssignments > 0) { + throw { status: 409, message: "No se puede eliminar: hay asignaciones pendientes o en curso" }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery/delivery.service.js` around lines 325 - 354, La verificación en deleteDeliveryService solo cuenta asignaciones con assignment_status "PENDING"; actualízala para bloquear también cuando existan asignaciones con status "ACCEPTED". Modifica la llamada a prisma.deliveryAssignments.count (o reemplázala por prisma.deliveryAssignments.findMany según prefieras) para filtrar where: { fk_delivery: id_delivery, assignment_status: { in: ["PENDING","ACCEPTED"] } } y ajusta el mensaje de error lanzado para reflejar que no se puede eliminar por asignaciones pendientes o aceptadas (por ejemplo: "No se puede eliminar: hay asignaciones pendientes o aceptadas").
🧹 Nitpick comments (1)
src/modules/delivery/delivery/delivery.service.js (1)
277-323: Considerar optimizar las consultas de estadísticas.Se realizan 4 consultas
countseparadas a la base de datos. Podrían consolidarse en una sola consulta congroupBypara mejorar el rendimiento, especialmente si este endpoint se usa frecuentemente.♻️ Alternativa con groupBy
export const getDeliveryStatsService = async (id_delivery) => { const delivery = await prisma.deliveries.findUnique({ where: { id_delivery } }); if (!delivery) { throw { status: 404, message: "Delivery no encontrado" }; } const stats = await prisma.deliveryAssignments.groupBy({ by: ['assignment_status'], where: { fk_delivery: id_delivery }, _count: true }); const counts = stats.reduce((acc, s) => { acc[s.assignment_status] = s._count; return acc; }, { ACCEPTED: 0, REJECTED: 0, PENDING: 0 }); const total = Object.values(counts).reduce((a, b) => a + b, 0); return { delivery_id: id_delivery, total_assignments: total, accepted: counts.ACCEPTED, rejected: counts.REJECTED, pending: counts.PENDING, acceptance_rate: total > 0 ? ((counts.ACCEPTED / total) * 100).toFixed(2) + '%' : '0%' }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery/delivery.service.js` around lines 277 - 323, The current getDeliveryStatsService issues four separate prisma.count queries which is inefficient; replace them with a single prisma.deliveryAssignments.groupBy call (grouping by assignment_status with _count) inside getDeliveryStatsService, then reduce the returned stats into a counts object (ensure keys for ACCEPTED/REJECTED/PENDING default to 0), compute total as the sum of those counts, and derive acceptance_rate from counts.ACCEPTED/total; keep the initial deliveries.findUnique existence check and return the same response shape (delivery_id, total_assignments, accepted, rejected, pending, acceptance_rate).
🤖 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/app.js`:
- Around line 21-23: Actualiza las tres importaciones en src/app.js: reemplaza
las rutas incorrectas para los identificadores deliveryRoutes, assignmentRoutes
y deliveryReviewRoutes por las rutas reales del PR; específicamente apunta
deliveryRoutes a ./modules/delivery/delivery/delivery.routes.js,
assignmentRoutes a
./modules/delivery/delivery-assignments/delivery-assignments.routes.js y
deliveryReviewRoutes a
./modules/delivery/delivery-review/delivery-review.routes.js para que las
importaciones coincidan con la estructura de archivos.
In
`@src/modules/delivery/delivery-assignments/delivery-assignments.controller.js`:
- Around line 3-13: La importación actual de servicios en
delivery-assignments.controller.js omite deleteAssignmentService y
completeAssignmentService, causando ReferenceError cuando se llaman los
handlers; agrega deleteAssignmentService y completeAssignmentService a la lista
de importaciones desde './delivery-assignments.service.js' (donde están
createAssignmentService, acceptAssignmentService, etc.) y asegúrate de
actualizar la misma corrección en la sección duplicada alrededor de los handlers
(líneas señaladas 116-131) para que ambos bloques usen las nuevas importaciones.
- Around line 27-42: Los handlers acceptAssignment y rejectAssignment (y los
similares en 116-131) no verifican que la asignación pertenezca al delivery
autenticado: cambia las llamadas a
acceptAssignmentService/rejectAssignmentService para pasar el usuario
autenticado (por ejemplo req.user o req.user.id) y/o realiza en el controller
una comprobación previa (cargar la asignación por id y comparar su deliveryId
con req.user.id) antes de mutarla; además actualiza los servicios
correspondientes para aceptar un ownerId y validar/rechazar con esa restricción
para evitar que cualquier usuario pueda mutar asignaciones ajenas.
In `@src/modules/delivery/delivery-assignments/delivery-assignments.routes.js`:
- Around line 1-16: The import for authenticate in
delivery-assignments.routes.js is using an incorrect path
(../../../../config/jwt.config.js); update the import to point to the correct
relative location (../../../config/jwt.config.js) so the authenticate symbol
resolves correctly when used in this file (delivery-assignments.routes.js).
In `@src/modules/delivery/delivery-review/delivery-review.controller.js`:
- Around line 17-20: El controller no está vinculando el usuario autenticado a
las mutaciones; modifica createDeliveryReview (y las otras handlers
updateDeliveryReview / deleteDeliveryReview mencionadas) para extraer el actor
desde req.user (p. ej. req.user.id) y pasar ese contexto/ID al servicio en lugar
de confiar en fk_user de la orden o en IDs recibidos en el body/params; además
actualiza createDeliveryReviewService / updateDeliveryReviewService /
deleteDeliveryReviewService call sites para aceptar el userId/context y usarlo
para establecer/verificar fk_user y autorizar la operación antes de persistir o
permitir cambios.
In `@src/modules/delivery/delivery-review/delivery-review.routes.js`:
- Around line 14-37: The import that provides deliveryReviewRoutes in app.js is
pointing to the wrong module so deliveryReviewRoutes is undefined; update the
import statement that declares deliveryReviewRoutes to reference the module that
exports the router implemented in delivery-review.routes.js (the delivery ->
delivery-review module) so the default export (deliveryReviewRoutes) is imported
correctly and the router is mounted as intended.
In `@src/modules/delivery/delivery-review/delivery-review.service.js`:
- Around line 35-46: La verificación actual solo acepta assignment_status ===
"ACCEPTED" y por eso bloquea la creación de reseñas cuando el flujo avanzó a
"DELIVERED"; en prisma.deliveryAssignments.findFirst (variable
acceptedAssignment) cambia la condición de where para permitir ambos estados (p.
ej. assignment_status in ["ACCEPTED","DELIVERED"]) o ajustar la lógica a negar
solo statuses terminales inválidos, y actualiza el mensaje de error si quieres
distinguir ambos casos.
In `@src/modules/delivery/delivery/delivery.controller.js`:
- Around line 58-63: The handler updateDeliveryStatus is validating against the
wrong enum because updateDeliveryStatusSchema was overwritten in
delivery.validation.js by a second export that defines
ACTIVE/INACTIVE/SUSPENDED; fix by restoring or renaming the original schema
export that uses AVAILABLE/ON_THE_WAY/ASSIGNED/DELIVERED and ensure
updateDeliveryStatus imports that correct symbol (updateDeliveryStatusSchema) or
import it under a distinct name if you keep both schemas; update
delivery.validation.js to export unique names (e.g.,
updateDeliveryStatusSchemaAvailable) and update the controller import or
reference so updateDeliveryStatusService receives validated values from the enum
that matches the rest of the module.
In `@src/modules/delivery/delivery/delivery.routes.js`:
- Around line 1-17: The import path for authenticate in delivery.routes.js is
one level too high; update the relative import for authenticate so it points to
the project config file (replace "../../../../config/jwt.config.js" with the
correct relative path from src/modules/delivery/delivery/, e.g.
"../../../config/jwt.config.js") so the named import authenticate resolves
correctly in delivery.routes.js and matches the same fix applied in
delivery-assignments.routes.js.
In `@src/modules/delivery/delivery/delivery.validation.js`:
- Around line 23-29: Hay una declaración duplicada de la constante
updateDeliveryStatusSchema; elimina la variante incorrecta que usa
["AVAILABLE","ON_THE_WAY","ASSIGNED","DELIVERED","INACTIVE"] y conserva la única
exportación updateDeliveryStatusSchema que define delivery_status como
z.enum(["ACTIVE","INACTIVE","SUSPENDED"]) para que el schema coincida con el
enum DeliveryStatus de Prisma; asegúrate de dejar solo una exportada
updateDeliveryStatusSchema y eliminar la otra declaración duplicada.
- Around line 11-16: El enum en createDeliverySchema no coincide con el enum de
la BD (DeliveryStatus), por lo que ajusta delivery_status para aceptar solo
["ACTIVE","INACTIVE","SUSPENDED"] en lugar de los valores actuales y cambia el
default de "AVAILABLE" a "ACTIVE" para alinear con el fallback usado en
delivery.service.js (línea con fallback "ACTIVE"); mantén la propiedad optional
si se desea opcionalidad.
---
Major comments:
In
`@src/modules/delivery/delivery-assignments/delivery-assignments.controller.js`:
- Around line 29-30: Validate numeric route params before using parseInt and
validate req.query.status in the controller methods (e.g., where you call
acceptAssignmentService(parseInt(id))); check that parseInt yields a finite
number (not NaN) and that status is one of the allowed values (or a valid
enum/format) before passing to services; if invalid, respond with 400 and a
clear error message instead of forwarding invalid values to Prisma. Apply the
same pattern to all similar handlers referenced (the blocks around lines that
call parseInt on id/orderId/deliveryId and forward req.query.status) so each
controller validates and sanitizes inputs and returns 400 for bad requests.
- Around line 16-23: The createAssignment controller currently returns 500 for
schema validation failures because createAssignmentSchema.parse() throws a
ZodError which has no .status; update the catch in createAssignment (exported
function createAssignment) to detect ZodError and return 400 (e.g., if (error
instanceof ZodError) return res.status(400).json({ error: error.message })), or
switch to the project's validate middleware
(src/middlewares/validate.middleware.js) to handle parsing errors uniformly;
ensure other errors still use error.status || 500.
In `@src/modules/delivery/delivery-assignments/delivery-assignments.routes.js`:
- Around line 29-45: Move the generic route router.get("/:id", authenticate,
getAssignmentById) so it is after all more specific routes
(router.get("/orders/:orderId/assignments", authenticate, getOrderAssignments),
router.get("/deliveries/:deliveryId/assignments", authenticate,
getDeliveryAssignments), router.get("/deliveries/:deliveryId/pending",
authenticate, getDeliveryPendingAssignments),
router.get("/orders/:orderId/accepted", authenticate, getAcceptedAssignment),
and router.get("/orders/:orderId/history", authenticate, getAssignmentHistory))
so that the specific handlers are registered first and won't be shadowed by the
catch‑all ":id" route.
In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js`:
- Around line 282-292: You’re updating deliveryAssignments and orders separately
which can leave them inconsistent if the second update fails; wrap both updates
in a single Prisma transaction so they’re atomic: use prisma.$transaction to run
prisma.deliveryAssignments.update({ where: { id_delivery_assignment }, data: {
assignment_status: "DELIVERED" } }) and prisma.orders.update({ where: {
id_order: assignment.fk_order }, data: { order_status: "DELIVERED" } }) together
(either as an array or inside an async transaction callback) so both succeed or
both are rolled back; return or handle the transaction result and propagate
errors as before.
- Around line 33-41: The pending-assignment checks and state-transition queries
(e.g., the findFirst used in create/accept/reject/complete flows in
delivery-assignments.service.js) do not filter by the logical-delete flag
`status: true`, so logically deleted rows (`status: false`) still block new
assignments and can be acted upon; update all queries and state transitions that
reference `assignment_status` (the findFirst calls around lines shown and any
lookup in functions like deleteAssignmentService, createAssignmentService,
accept/reject/complete handlers) to include `status: true` in the Prisma where
clauses, and also ensure any update/transition methods first verify `status:
true` before proceeding to prevent operations on deleted assignments.
- Around line 25-46: The code is vulnerable to race conditions because
prisma.deliveryAssignments.findFirst (lastAssignment), the pending check, and
prisma.deliveryAssignments.create run separately; wrap those operations in a
single database transaction (use prisma.$transaction and use the
transaction-scoped client, e.g., tx.deliveryAssignments.findFirst(...) and
tx.deliveryAssignments.create(...)) so the sequencing and pending-check are
atomic, and additionally enforce uniqueness at the DB level by adding a unique
constraint/index on (fk_order, assignment_sequence) and/or a partial unique
index to prevent multiple PENDING rows for the same fk_order to guarantee safety
even if concurrent transactions race.
In
`@src/modules/delivery/delivery-assignments/delivery-assignments.validation.js`:
- Around line 10-16: The two Zod schemas acceptAssignmentSchema and
rejectAssignmentSchema are unused while acceptAssignment and rejectAssignment
handlers pull and trust req.params.id directly; either remove the unused schemas
or update the controller to validate route params with Zod. Best fix: replace or
adapt the schemas to validate params (e.g., a schema expecting { id:
z.string().regex(/^\d+$/) or z.number() after parsing) and then call
schema.parse(req.params) at the start of acceptAssignment and rejectAssignment,
converting the validated id into the numeric id used downstream; alternatively,
delete acceptAssignmentSchema and rejectAssignmentSchema and any imports to
avoid dead code.
In `@src/modules/delivery/delivery-review/delivery-review.controller.js`:
- Around line 28-31: Valida los params antes de hacer parseInt: en el controller
donde se usa getDeliveryReviewByIdService(parseInt(id)) (y en los otros handlers
que parsean orderId/deliveryId) comprueba que req.params.id (o
orderId/deliveryId) sea un número válido tras parseInt/Number, y si produce NaN
responde inmediatamente con res.status(400).json({ error: 'Invalid id' }) (o
mensaje equivalente), en lugar de pasar NaN al service; aplica el mismo patrón a
los controladores que llaman a getDeliveryReviewByIdService y a los otros
endpoints que parsean orderId/deliveryId para evitar 500 aguas abajo.
- Around line 17-24: Los handlers createDeliveryReview y updateDeliveryReview
usan createDeliveryReviewSchema.parse / updateDeliveryReviewSchema.parse y en el
catch asumen error.status, por eso errores de Zod llegan como 500; cambia a usar
schema.safeParse(req.body) y, si safeParse.success es false, responder con
res.status(400).json({ error: validationError }) o alternativamente atrapa
explícitamente ZodError en el catch (instanceof ZodError) y hacer
res.status(400).json({ error: error.errors }) dejando el resto de errores con
res.status(500).json({ error: error.message }); actualiza createDeliveryReview y
updateDeliveryReview para aplicar este patrón.
In `@src/modules/delivery/delivery-review/delivery-review.service.js`:
- Around line 72-85: Las lecturas y actualizaciones no respetan el borrado
lógico: aunque deleteDeliveryReviewService marca status: false, las consultas
usan prisma.deliveryReviews.findUnique / findMany sin filtrar por status,
permitiendo ver/editar reseñas "eliminadas". Fix: en todas las consultas y
actualizaciones relativas a deliveryReviews (por ejemplo donde se usan
prisma.deliveryReviews.findUnique, findMany y update en este archivo y en las
funciones updateDeliveryReviewService/list/review stats) añada el filtro
adicional { status: true } al where (o combine id_delivery_review con status:
true) y para updates confirme primero que la reseña existe y tiene status: true
antes de aplicar cambios; use los mismos símbolos prisma.deliveryReviews,
deleteDeliveryReviewService, updateDeliveryReviewService y las llamadas a
findUnique/findMany para localizar y corregir cada sitio.
In `@src/modules/delivery/delivery/delivery.controller.js`:
- Around line 60-62: Valida los params numéricos en el controller antes de
llamar a services: para cada handler que usa parseInt on req.params (e.g., the
handler calling updateDeliveryStatusService and others referenced), convierte el
param con Number(...) o parseInt y comprueba isNaN; si es NaN responde con
status 400 y un mensaje de param inválido en lugar de dejar que pase al service;
asegúrate de aplicar esto tanto a id como a storeId params y mantener la llamada
a updateDeliveryStatusSchema.parse(req.body) intacta antes de invocar
updateDeliveryStatusService.
- Around line 25-31: The controller currently treats all schema.parse() failures
as 500; update the error handling in registerDelivery (and the other handlers
using schema.parse()) to detect Zod validation errors and return 400 instead of
500: inside the catch block check for ZodError (e.g., error.name === 'ZodError'
or instanceof ZodError) and respond res.status(400).json({ error: 'Validation
failed', details: error.errors }) while preserving the existing behavior for
other errors (res.status(error.status || 500).json({ error: error.message }));
ensure you import ZodError from zod if you use instanceof.
In `@src/modules/delivery/delivery/delivery.service.js`:
- Around line 29-54: En loginDeliveryService valida que user.role === "DELIVERY"
antes de generar el token: si el rol no es DELIVERY lanza un error con status
403 y mensaje claro (por ejemplo "Acceso no autorizado para este endpoint"), de
modo que solo usuarios con role "DELIVERY" puedan autenticarse por este
servicio; usa las variables existentes (user.role, user.id_user) y realiza la
comprobación justo después de verificar la contraseña y antes de llamar a
generateToken.
---
Minor comments:
In `@src/modules/delivery/delivery/delivery.service.js`:
- Around line 325-354: La verificación en deleteDeliveryService solo cuenta
asignaciones con assignment_status "PENDING"; actualízala para bloquear también
cuando existan asignaciones con status "ACCEPTED". Modifica la llamada a
prisma.deliveryAssignments.count (o reemplázala por
prisma.deliveryAssignments.findMany según prefieras) para filtrar where: {
fk_delivery: id_delivery, assignment_status: { in: ["PENDING","ACCEPTED"] } } y
ajusta el mensaje de error lanzado para reflejar que no se puede eliminar por
asignaciones pendientes o aceptadas (por ejemplo: "No se puede eliminar: hay
asignaciones pendientes o aceptadas").
---
Nitpick comments:
In `@src/modules/delivery/delivery/delivery.service.js`:
- Around line 277-323: The current getDeliveryStatsService issues four separate
prisma.count queries which is inefficient; replace them with a single
prisma.deliveryAssignments.groupBy call (grouping by assignment_status with
_count) inside getDeliveryStatsService, then reduce the returned stats into a
counts object (ensure keys for ACCEPTED/REJECTED/PENDING default to 0), compute
total as the sum of those counts, and derive acceptance_rate from
counts.ACCEPTED/total; keep the initial deliveries.findUnique existence check
and return the same response shape (delivery_id, total_assignments, accepted,
rejected, pending, acceptance_rate).
🪄 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: 98e9269c-411a-480b-a76e-91d51592c1f5
📒 Files selected for processing (18)
prisma/migrations/20260428223931_add_delivered_to_assignment_status/migration.sqlprisma/schema.prismasrc/app.jssrc/modules/deliveries/delivery.controller.jssrc/modules/deliveries/delivery.routes.jssrc/modules/deliveries/delivery.service.jssrc/modules/delivery/delivery-assignments/delivery-assignments.controller.jssrc/modules/delivery/delivery-assignments/delivery-assignments.routes.jssrc/modules/delivery/delivery-assignments/delivery-assignments.service.jssrc/modules/delivery/delivery-assignments/delivery-assignments.validation.jssrc/modules/delivery/delivery-review/delivery-review.controller.jssrc/modules/delivery/delivery-review/delivery-review.routes.jssrc/modules/delivery/delivery-review/delivery-review.service.jssrc/modules/delivery/delivery-review/delivery-review.validation.jssrc/modules/delivery/delivery/delivery.controller.jssrc/modules/delivery/delivery/delivery.routes.jssrc/modules/delivery/delivery/delivery.service.jssrc/modules/delivery/delivery/delivery.validation.js
💤 Files with no reviewable changes (3)
- src/modules/deliveries/delivery.routes.js
- src/modules/deliveries/delivery.controller.js
- src/modules/deliveries/delivery.service.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
src/modules/delivery/delivery-assignments/delivery-assignments.controller.js (1)
29-33:⚠️ Potential issue | 🔴 CriticalRestringí mutaciones al delivery autenticado (IDOR).
Estos handlers mutan por
idsin pasar contexto de usuario. Así, cualquier autenticado con un ID válido podría aceptar/rechazar/eliminar/completar asignaciones ajenas.🔧 Propuesta mínima en controller
export const acceptAssignment = async (req, res) => { try { const { id } = req.params; - const result = await acceptAssignmentService(parseInt(id)); + const result = await acceptAssignmentService(Number(id), req.user.id); res.json(result); } catch (error) { res.status(error.status || 500).json({ error: error.message }); } }; export const rejectAssignment = async (req, res) => { try { const { id } = req.params; - const result = await rejectAssignmentService(parseInt(id)); + const result = await rejectAssignmentService(Number(id), req.user.id); res.json(result); } catch (error) { res.status(error.status || 500).json({ error: error.message }); } }; export const deleteAssignment = async (req, res) => { try { const { id } = req.params; - const result = await deleteAssignmentService(parseInt(id)); + const result = await deleteAssignmentService(Number(id), req.user.id); res.json(result); } catch (error) { res.status(error.status || 500).json({ error: error.message }); } }; export const completeAssignment = async (req, res) => { try { const { id } = req.params; - const result = await completeAssignmentService(parseInt(id)); + const result = await completeAssignmentService(Number(id), req.user.id); res.json(result); } catch (error) { res.status(error.status || 500).json({ error: error.message }); } };Also applies to: 40-44, 118-123, 129-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-assignments/delivery-assignments.controller.js` around lines 29 - 33, Los handlers (por ejemplo acceptAssignment) están mutando asignaciones usando solo el param id (IDOR); cambia el controller para obtener el delivery autenticado (p. ej. req.user.id o req.auth.id), validar que ese delivery sea el propietario de la assignment o, mejor, pasar ese deliveryId al servicio (por ejemplo acceptAssignmentService(assignmentId, deliveryId)) y deja que el servicio verifique propiedad antes de mutar; aplica el mismo patrón a los otros handlers que mutan (rejectAssignment, deleteAssignment, completeAssignment) y actualiza sus firmas/llamadas para incluir el deliveryId y realizar la comprobación de propiedad en la capa de servicio.
🤖 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/delivery/delivery-assignments/delivery-assignments.controller.js`:
- Around line 76-77: Validate req.query.status against the allowed enum before
calling getDeliveryAssignmentsService: read req.query.status into the local
status variable, normalize (trim/uppercase) and check it is one of
"PENDING","ACCEPTED","REJECTED"; if it's present and invalid, return
res.status(400).json({ error: 'invalid status' }) (don't call
getDeliveryAssignmentsService), otherwise pass either the normalized status or
null to getDeliveryAssignmentsService(parseInt(deliveryId), status || null).
- Around line 31-32: Los endpoints que extraen IDs deben validar que los
parámetros sean enteros antes de llamar a los servicios; crea o reutiliza un
helper (ej. validateNumericId(paramName, rawValue, res)) y en cada controlador
donde se hace parseInt (p. ej. acceptAssignmentService(parseInt(id)) y en los
controladores/referencias rejectAssignment, getAssignmentById,
getOrderAssignments, getDeliveryAssignments, getDeliveryPendingAssignments,
getAcceptedAssignment, getAssignmentHistory, deleteAssignment,
completeAssignment) reemplaza el parseInt directo por llamar al helper, devolver
un 400 con un mensaje claro si el valor es NaN y solo entonces llamar al
servicio con el ID convertido (Number or parseInt result).
- Around line 20-25: The controller currently treats all exceptions as 500
because ZodError has no status; update each handler (createAssignment,
acceptAssignment, rejectAssignment, etc.) to detect Zod validation failures and
return 400: import ZodError from 'zod' (or use error.name === 'ZodError') and in
the catch block if (error instanceof ZodError) respond res.status(400).json({
errors: error.errors }) otherwise keep res.status(error.status || 500).json({
error: error.message }); apply this pattern to every function that calls
createAssignmentSchema.parse(...) so validation errors produce HTTP 400.
In `@src/modules/delivery/delivery/delivery.controller.js`:
- Around line 118-123: El endpoint updateDelivery está recibiendo :id como
id_delivery pero llama updateDeliveryService directamente (que busca en
prisma.users por id_user), provocando updates sobre el usuario equivocado; en
updateDelivery (controller) primero busca el delivery por id_delivery (ej. via
getDeliveryById / prisma.deliveries) para obtener fk_user y luego llama
updateDeliveryService pasando ese fk_user (o alternativamente cambiate a una
ruta explícita con user-scoped id), asegurate de validar existence del delivery
y devolver 404 si no existe; referencia: función updateDelivery, servicio
updateDeliveryService, entidad prisma.users, campo id_delivery y fk_user.
- Around line 25-33: Invalid request bodies parsed by Zod are returning 500
because the catch blocks in handlers like registerDelivery (uses
registerDeliverySchema.parse), plus the handlers that call
loginDeliverySchema.parse, createDeliverySchema.parse, and
updateDeliverySchema.parse, do res.status(error.status || 500). Update these
catch blocks to detect Zod errors (e.g., error instanceof ZodError or error.name
=== 'ZodError') and respond with res.status(400).json({ error: error.errors })
or a concise validation message; otherwise keep the existing error.status || 500
behavior. Ensure you update the catch logic in the functions/registerDelivery,
loginDelivery, createDelivery, and updateDelivery handlers so malformed bodies
return 400 Bad Request.
In `@src/modules/delivery/delivery/delivery.routes.js`:
- Around line 28-39: Routes are currently protected only by authenticate but
lack role/ownership checks so authenticated users can act on others' deliveries;
update the route middleware and handlers to enforce proper authorization: for
routes using handlers getPendingAssignments, updateDelivery, getDeliveryById,
getDeliveryStats, deleteDelivery, getActiveAssignments (and store-related
getStoreDeliveries/getAvailableDeliveries if needed) add requireRole(ROLES.*)
where appropriate or an ownership middleware (e.g., ensureDeliveryOwner or
ensureAssignmentOwner) before the handler, and modify the handlers/services
(getPendingAssignmentsService, updateDeliveryService, getDeliveryStatsService,
deleteDeliveryService, getActiveAssignmentsService) to accept and use req.user
(or the validated user id) to verify ownership/permissions before performing
actions. Ensure the unique symbols referenced (createDelivery,
updateDeliveryStatus, getPendingAssignments, updateDelivery, getDeliveryById,
getDeliveryStats, deleteDelivery, getActiveAssignments, getStoreDeliveries,
getAvailableDeliveries and the services named in the comment) are updated
accordingly so authorization is enforced consistently.
In `@src/modules/delivery/delivery/delivery.service.js`:
- Around line 60-76: Estás validando sólo existencia de fk_user
(prisma.users.findUnique) y fk_store pero no verificás el rol del usuario;
actualizar la validación después de obtener el usuario para comprobar user.role
=== 'DELIVERY' (o el enum/const correspondiente) y si no coincide lanzar un
error de autorización (por ejemplo status 403, mensaje "Usuario no autorizado
como delivery"). Aplicá la misma verificación adicional donde se hace la segunda
validación mencionada (líneas 79-86) para evitar asociar
compradores/sellers/admins como deliverys.
- Around line 339-355: The precondition only checks for assignment_status
"PENDING" but the service treats "ACCEPTED" (and any other active statuses) as
active work, so update the existence/count check that uses
prisma.deliveryAssignments.count (variable pendingAssignments) to include all
active statuses (e.g., "PENDING" OR "ACCEPTED" or a set like ACTIVE_STATUSES)
for fk_delivery = id_delivery; if any match, throw the 409 error and do not
proceed to the logical delete (prisma.deliveries.update setting status:false).
Also apply the same expanded-status check to the duplicate block referenced
later (the other prisma.deliveryAssignments.count check).
- Around line 22-26: La creación de usuario en prisma.users.create está
devolviendo password_hash al cliente; modifica la llamada en delivery.service
para excluir ese campo (usar la opción select para listar únicamente id, name,
email, phone, role, createdAt, etc.) o eliminar password_hash de newUser antes
de retornarlo, y devuelve el objeto filtrado para que el controller no serialice
la contraseña hasheada.
---
Duplicate comments:
In
`@src/modules/delivery/delivery-assignments/delivery-assignments.controller.js`:
- Around line 29-33: Los handlers (por ejemplo acceptAssignment) están mutando
asignaciones usando solo el param id (IDOR); cambia el controller para obtener
el delivery autenticado (p. ej. req.user.id o req.auth.id), validar que ese
delivery sea el propietario de la assignment o, mejor, pasar ese deliveryId al
servicio (por ejemplo acceptAssignmentService(assignmentId, deliveryId)) y deja
que el servicio verifique propiedad antes de mutar; aplica el mismo patrón a los
otros handlers que mutan (rejectAssignment, deleteAssignment,
completeAssignment) y actualiza sus firmas/llamadas para incluir el deliveryId y
realizar la comprobación de propiedad en la capa de servicio.
🪄 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: 798045bc-e31d-489f-b4db-b8fa38779dcd
📒 Files selected for processing (8)
src/app.jssrc/modules/delivery/delivery-assignments/delivery-assignments.controller.jssrc/modules/delivery/delivery-assignments/delivery-assignments.routes.jssrc/modules/delivery/delivery-review/delivery-review.routes.jssrc/modules/delivery/delivery/delivery.controller.jssrc/modules/delivery/delivery/delivery.routes.jssrc/modules/delivery/delivery/delivery.service.jssrc/modules/delivery/delivery/delivery.validation.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app.js
- src/modules/delivery/delivery-assignments/delivery-assignments.routes.js
- src/modules/delivery/delivery/delivery.validation.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/modules/delivery/delivery-assignments/delivery-assignments.controller.js (1)
42-47:⚠️ Potential issue | 🟠 Major
parseInt()sigue dejando pasar IDs malformados.Con este patrón, valores como
"12abc"o"-1"no devuelven400: terminan consultando o mutando otro recurso distinto al pedido. Conviene validar el parámetro completo y exigir entero positivo antes de llamar al service, idealmente con un helper compartido para todo el archivo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-assignments/delivery-assignments.controller.js` around lines 42 - 47, El código usa Number.parseInt(id, 10) y Number.isNaN(assignmentId) que acepta entradas como "12abc" o "-1"; crea y usa un helper compartido (por ejemplo isValidPositiveInt(value)) en este controlador para validar que el parámetro sea sólo dígitos y mayor que 0 (p.ej. comprobar con una regex ^\d+$ antes de parsear), rechaza con res.status(400).json(...) si la validación falla y sólo entonces convierte a Number y llama al service usando assignmentId; reemplaza la lógica actual que usa parseInt/Number.isNaN por este helper en todas las rutas del archivo.
🤖 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/delivery/delivery-assignments/delivery-assignments.controller.js`:
- Around line 49-50: The controller is passing req.user.id_user to the service
but the service (acceptAssignmentService, and the similar calls at the other
locations) checks ownership against assignment.fk_delivery, so replace the user
id argument with the authenticated delivery identifier from req.user (e.g.
req.user.id_delivery or the actual delivery id property present on req.user)
when calling acceptAssignmentService and the other affected controller calls
(the occurrences around lines 72-73, 229-230, 251-252); ensure the argument name
you use matches the property on the decoded user object so the service's
fk_delivery check receives the delivery id it expects.
In `@src/modules/delivery/delivery-review/delivery-review.controller.js`:
- Around line 41-46: The ID parsing is too permissive: replace the
Number.parseInt/Number.isNaN pattern by validating the raw id param against a
strict integer pattern (e.g., /^\d+$/) and ensure it represents a positive
integer before converting; if the check fails, return the 400 JSON error
immediately and do not call the service. Update the logic around the variables
id and reviewId (the code that currently uses Number.parseInt and Number.isNaN)
to perform the regex test on id, then safely convert to Number(reviewId) and
proceed.
In `@src/modules/delivery/delivery-review/delivery-review.service.js`:
- Around line 48-60: El permiso y la autoría están mezclados: en la comprobación
debes verificar que el usuario autenticado (id_user) sea o el cliente
(order.fk_user) o el repartidor del pedido (order.fk_delivery) — reemplaza la
condición actual que compara fk_delivery contra id_user por (order.fk_user !==
id_user && order.fk_delivery !== id_user). Al crear la reseña
(prisma.deliveryReviews.create) asigna fk_user: id_user (el autor real) y
fk_delivery: order.fk_delivery (o la variable fk_delivery validada contra
order.fk_delivery), no siempre order.fk_user; así la autoría y los permisos
coinciden correctamente.
- Around line 77-90: La lectura y estadísticas no respetan el borrado lógico:
todas las consultas que recuperan o agregan reseñas (p. ej. la consulta que crea
const review y las consultas en las zonas señaladas 109-117, 142-152, 166-168,
217-219, 235-237) deben filtrar status: true; para la consulta mostrada
reemplaza findUnique por findFirst o añade status: true en el where ({
id_delivery_review, status: true }), aplica el mismo where a listados y
agregaciones (promedios/counts) y antes de permitir actualizaciones en
updateDeliveryReviewService valida que la reseña exista con status: true para
evitar modificar reseñas "eliminadas".
In `@src/modules/delivery/delivery/delivery.validation.js`:
- Around line 11-15: Los schemas de validación (por ejemplo
createDeliverySchema) usan z.enum(["ACTIVE","INACTIVE"]) y deben aceptar el
nuevo estado SUSPENDED; actualizar todas las instancias de z.enum que validan
delivery_status en este archivo (incluyendo los schemas de
creación/actualización y cualquier schema que valide delivery_status en las
líneas señaladas) para incluir "SUSPENDED" en la lista de valores permitidos,
manteniendo las mismas llamadas a .default("ACTIVE")/.optional() según
corresponda.
---
Duplicate comments:
In
`@src/modules/delivery/delivery-assignments/delivery-assignments.controller.js`:
- Around line 42-47: El código usa Number.parseInt(id, 10) y
Number.isNaN(assignmentId) que acepta entradas como "12abc" o "-1"; crea y usa
un helper compartido (por ejemplo isValidPositiveInt(value)) en este controlador
para validar que el parámetro sea sólo dígitos y mayor que 0 (p.ej. comprobar
con una regex ^\d+$ antes de parsear), rechaza con res.status(400).json(...) si
la validación falla y sólo entonces convierte a Number y llama al service usando
assignmentId; reemplaza la lógica actual que usa parseInt/Number.isNaN por este
helper en todas las rutas del archivo.
🪄 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: 6f5401cd-00aa-4a49-8e59-49cf60a8eba7
📒 Files selected for processing (14)
src/modules/delivery/delivery-assignments/delivery-assignments.controller.jssrc/modules/delivery/delivery-assignments/delivery-assignments.routes.jssrc/modules/delivery/delivery-assignments/delivery-assignments.service.jssrc/modules/delivery/delivery-assignments/delivery-assignments.validation.jssrc/modules/delivery/delivery-review/delivery-review.controller.jssrc/modules/delivery/delivery-review/delivery-review.service.jssrc/modules/delivery/delivery/delivery.controller.jssrc/modules/delivery/delivery/delivery.routes.jssrc/modules/delivery/delivery/delivery.service.jssrc/modules/delivery/delivery/delivery.validation.jstests/unit/delivery/delivery-assignments.test.jstests/unit/delivery/delivery-review.test.jstests/unit/delivery/delivery-status.test.jstests/unit/delivery/delivery.test.js
✅ Files skipped from review due to trivial changes (2)
- tests/unit/delivery/delivery-status.test.js
- src/modules/delivery/delivery-assignments/delivery-assignments.routes.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/modules/delivery/delivery/delivery.routes.js
- src/modules/delivery/delivery-assignments/delivery-assignments.validation.js
- src/modules/delivery/delivery-assignments/delivery-assignments.service.js
- src/modules/delivery/delivery/delivery.controller.js
- src/modules/delivery/delivery/delivery.service.js
Comment on lines
+41
to
+46
| const reviewId = Number.parseInt(id, 10); | ||
| if (Number.isNaN(reviewId)) { | ||
| return res.status(400).json({ | ||
| error: { code: 400, message: "ID inválido" } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
La validación de IDs sigue siendo demasiado permisiva.
Number.parseInt() acepta entradas parcialmente numéricas, así que rutas como /api/reviews/delivery/1abc no se rechazan: terminan operando sobre la reseña 1. Acá también conviene validar el parámetro completo y exigir entero positivo antes de invocar al service.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/delivery/delivery-review/delivery-review.controller.js` around
lines 41 - 46, The ID parsing is too permissive: replace the
Number.parseInt/Number.isNaN pattern by validating the raw id param against a
strict integer pattern (e.g., /^\d+$/) and ensure it represents a positive
integer before converting; if the check fails, return the 400 JSON error
immediately and do not call the service. Update the logic around the variables
id and reviewId (the code that currently uses Number.parseInt and Number.isNaN)
to perform the regex test on id, then safely convert to Number(reviewId) and
proceed.
Comment on lines
+48
to
+60
| // Verificar que el usuario autenticado es el dueño del pedido o el delivery | ||
| if (order.fk_user !== id_user && fk_delivery !== id_user) { | ||
| throw { status: 403, message: "No tienes permiso para crear esta reseña" }; | ||
| } | ||
|
|
||
| // Crear la reseña | ||
| const review = await prisma.deliveryReviews.create({ | ||
| data: { | ||
| fk_order, | ||
| fk_user: order.fk_user, | ||
| fk_delivery, | ||
| rating, | ||
| comment: comment || null |
Contributor
There was a problem hiding this comment.
La autoría y los permisos de creación no cierran entre sí.
El check compara fk_delivery contra id_user, pero fk_delivery es el id del delivery, no el del usuario autenticado. Así, el delivery real no pasa salvo coincidencia accidental de IDs. Además, la reseña siempre se persiste con fk_user: order.fk_user, así que si mañana corregís el permiso para permitir al delivery, la reseña quedaría atribuida al cliente y después la podría editar/eliminar el usuario equivocado.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/delivery/delivery-review/delivery-review.service.js` around lines
48 - 60, El permiso y la autoría están mezclados: en la comprobación debes
verificar que el usuario autenticado (id_user) sea o el cliente (order.fk_user)
o el repartidor del pedido (order.fk_delivery) — reemplaza la condición actual
que compara fk_delivery contra id_user por (order.fk_user !== id_user &&
order.fk_delivery !== id_user). Al crear la reseña
(prisma.deliveryReviews.create) asigna fk_user: id_user (el autor real) y
fk_delivery: order.fk_delivery (o la variable fk_delivery validada contra
order.fk_delivery), no siempre order.fk_user; así la autoría y los permisos
coinciden correctamente.
Comment on lines
+77
to
+90
| const review = await prisma.deliveryReviews.findUnique({ | ||
| where: { id_delivery_review }, | ||
| include: { | ||
| user: { | ||
| select: { id_user: true, name: true, avatar_url: true } | ||
| }, | ||
| delivery: { | ||
| select: { id_delivery: true } | ||
| }, | ||
| order: { | ||
| select: { id_order: true } | ||
| } | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
El borrado lógico no se está respetando en lecturas ni stats.
Desde deleteDeliveryReviewService dejás la reseña con status: false, pero el resto del archivo la sigue trayendo sin filtrar por status. En la práctica, una reseña “eliminada” sigue apareciendo en los listados, sigue entrando en el promedio y hasta puede volver a actualizarse.
Also applies to: 109-117, 142-152, 166-168, 217-219, 235-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/delivery/delivery-review/delivery-review.service.js` around lines
77 - 90, La lectura y estadísticas no respetan el borrado lógico: todas las
consultas que recuperan o agregan reseñas (p. ej. la consulta que crea const
review y las consultas en las zonas señaladas 109-117, 142-152, 166-168,
217-219, 235-237) deben filtrar status: true; para la consulta mostrada
reemplaza findUnique por findFirst o añade status: true en el where ({
id_delivery_review, status: true }), aplica el mismo where a listados y
agregaciones (promedios/counts) y antes de permitir actualizaciones en
updateDeliveryReviewService valida que la reseña exista con status: true para
evitar modificar reseñas "eliminadas".
Comment on lines
+11
to
+15
| export const createDeliverySchema = z.object({ | ||
| fk_user: z.number().int().positive("Debe ser un número entero positivo"), | ||
| fk_store: z.number().int().positive("Debe ser un número entero positivo"), | ||
| delivery_status: z.enum(["ACTIVE", "INACTIVE"]).default("ACTIVE").optional(), | ||
| status: z.boolean().default(true).optional() |
Contributor
There was a problem hiding this comment.
Falta SUSPENDED en los schemas de estado.
El módulo quedó preparado para DeliveryStatus = ACTIVE | INACTIVE | SUSPENDED, pero estos dos schemas siguen rechazando SUSPENDED. Hoy cualquier alta o cambio de estado a ese valor va a caer en 400 aunque sea válido para el dominio del PR.
Also applies to: 23-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/delivery/delivery/delivery.validation.js` around lines 11 - 15,
Los schemas de validación (por ejemplo createDeliverySchema) usan
z.enum(["ACTIVE","INACTIVE"]) y deben aceptar el nuevo estado SUSPENDED;
actualizar todas las instancias de z.enum que validan delivery_status en este
archivo (incluyendo los schemas de creación/actualización y cualquier schema que
valide delivery_status en las líneas señaladas) para incluir "SUSPENDED" en la
lista de valores permitidos, manteniendo las mismas llamadas a
.default("ACTIVE")/.optional() según corresponda.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/unit/delivery/delivery-assignments.test.js (2)
355-382: Falta validar el efecto secundario sobre la orden al completar.Buen caso happy-path, pero en Line 355-382 conviene afirmar que también se ejecuta
orders.update; así evitás regresiones donde solo cambie la asignación.💡 Aserciones extra sugeridas
expect(res.status).toBe(200); expect(res.body.assignment_status).toBe("DELIVERED"); + expect(prisma.orders.update).toHaveBeenCalledTimes(1); + expect(prisma.orders.update).toHaveBeenCalledWith({ + where: { id_order: 1 }, + data: { order_status: "DELIVERED" }, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/delivery/delivery-assignments.test.js` around lines 355 - 382, Agregar en la prueba del endpoint POST "/api/assignments/1/complete" una aserción que verifique el efecto sobre la orden: comprobar que la función mock prisma.orders.update fue llamada (y opcionalmente con los argumentos esperados: where: { id_order: 1 } y data: { order_status: "DELIVERED" }) después de completar la asignación; referencia prisma.orders.update y el caso de prueba que hace la petición al endpoint para ubicar dónde insertar la aserción.
179-197: Agregar cobertura de autorización (403) en endpoints mutables.En estos bloques no se cubre la rama de ownership (
fk_userdistinto al autenticado). Es una rama crítica del servicio y vale la pena testearla explícitamente.Also applies to: 214-232, 355-382, 413-430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/delivery/delivery-assignments.test.js` around lines 179 - 197, Agregar un test que cubra la rama de ownership: en el bloque que prueba POST /api/assignments/1/accept (donde se usan prisma.deliveryAssignments.findUnique, prisma.deliveryAssignments.update, authCookie y mockAssignment) mockear findUnique para devolver un delivery con fk_user distinto al id autenticado (por ejemplo fk_user: 999), hacer la misma petición con authCookie y esperar status 403; además verificar que prisma.deliveryAssignments.update no fue invocado. Repetir el mismo patrón de test (mock findUnique con fk_user distinto, esperar 403 y asegurar que no se realiza el update) para los otros endpoints mutables cubiertos en este archivo que siguen la misma lógica de ownership.
🤖 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/delivery/delivery-assignments/delivery-assignments.service.js`:
- Around line 59-63: La consulta que recupera assignment
(prisma.deliveryAssignments.findUnique) no filtra asignaciones con status ===
false, por lo que registros "borrados lógicamente" siguen siendo operables;
actualiza las búsquedas por id_delivery_assignment para excluir status:false
(ej. añadir status: true al where) o, si prefieres validar después de la
consulta, comprueba assignment.status y lanza un error/return si es false antes
de permitir accept/reject/complete; aplica el mismo cambio a las otras llamadas
que usan id_delivery_assignment (las ubicadas alrededor de las referencias a
assignment en las secciones que actualmente cubren las líneas 59-63, 90-93 y
307-310).
- Around line 24-41: Hay una condición de carrera entre las consultas que buscan
lastAssignment/pendingAssignment y el posterior create, lo que permite
duplicados y secuencias repetidas; arregla envolviendo la lógica en una
transacción atómica (usar prisma.$transaction) y dentro de ella volver a
leer/controlar con bloqueo lógico (obtener el último assignment con
prisma.deliveryAssignments.findFirst y/o consulta raw con FOR UPDATE según el
dialecto) y sólo entonces llamar a prisma.deliveryAssignments.create;
alternativamente añade una restricción única a nivel BD (por ejemplo
UNIQUE(fk_order, assignment_status) o UNIQUE(fk_order, assignment_sequence)) y
maneja el error de violación de restricción para garantizar idempotencia cuando
se intente crear dos PENDING simultáneos.
- Around line 327-336: Las dos escrituras (prisma.deliveryAssignments.update y
prisma.orders.update usando id_delivery_assignment y assignment.fk_order) deben
ejecutarse de forma atómica para evitar inconsistencias si una falla;
refactoriza para envolver ambas actualizaciones en una única transacción (por
ejemplo usando prisma.$transaction o el callback prisma.$transaction(async tx =>
...)) y usa el mismo cliente transaccional (tx.deliveryAssignments.update y
tx.orders.update) de modo que si cualquiera falla se haga rollback y no quede la
asignación marcada como DELIVERED sin actualizar el pedido.
---
Nitpick comments:
In `@tests/unit/delivery/delivery-assignments.test.js`:
- Around line 355-382: Agregar en la prueba del endpoint POST
"/api/assignments/1/complete" una aserción que verifique el efecto sobre la
orden: comprobar que la función mock prisma.orders.update fue llamada (y
opcionalmente con los argumentos esperados: where: { id_order: 1 } y data: {
order_status: "DELIVERED" }) después de completar la asignación; referencia
prisma.orders.update y el caso de prueba que hace la petición al endpoint para
ubicar dónde insertar la aserción.
- Around line 179-197: Agregar un test que cubra la rama de ownership: en el
bloque que prueba POST /api/assignments/1/accept (donde se usan
prisma.deliveryAssignments.findUnique, prisma.deliveryAssignments.update,
authCookie y mockAssignment) mockear findUnique para devolver un delivery con
fk_user distinto al id autenticado (por ejemplo fk_user: 999), hacer la misma
petición con authCookie y esperar status 403; además verificar que
prisma.deliveryAssignments.update no fue invocado. Repetir el mismo patrón de
test (mock findUnique con fk_user distinto, esperar 403 y asegurar que no se
realiza el update) para los otros endpoints mutables cubiertos en este archivo
que siguen la misma lógica de ownership.
🪄 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: c644a0de-ae27-49b7-a3ff-3531b7d85865
📒 Files selected for processing (5)
src/modules/delivery/delivery-assignments/delivery-assignments.service.jssrc/modules/delivery/delivery/delivery.controller.jssrc/modules/delivery/delivery/delivery.validation.jstests/unit/delivery/delivery-assignments.test.jstests/unit/delivery/delivery.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/modules/delivery/delivery/delivery.controller.js
- src/modules/delivery/delivery/delivery.validation.js
Comment on lines
+59
to
+63
| const assignment = await prisma.deliveryAssignments.findUnique({ | ||
| where: { id_delivery_assignment }, | ||
| include: { delivery: true } | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
Las asignaciones con borrado lógico siguen siendo operables.
En Line 59-63, Line 90-93 y Line 307-310 falta bloquear status === false. Hoy una asignación “eliminada” puede aceptarse/rechazarse/completarse por id.
💡 Propuesta de guard común
if (!assignment) {
throw { status: 404, message: "Asignación no encontrada" };
}
+ if (assignment.status === false) {
+ throw { status: 404, message: "Asignación no encontrada" };
+ }Also applies to: 90-93, 307-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js`
around lines 59 - 63, La consulta que recupera assignment
(prisma.deliveryAssignments.findUnique) no filtra asignaciones con status ===
false, por lo que registros "borrados lógicamente" siguen siendo operables;
actualiza las búsquedas por id_delivery_assignment para excluir status:false
(ej. añadir status: true al where) o, si prefieres validar después de la
consulta, comprueba assignment.status y lanza un error/return si es false antes
de permitir accept/reject/complete; aplica el mismo cambio a las otras llamadas
que usan id_delivery_assignment (las ubicadas alrededor de las referencias a
assignment en las secciones que actualmente cubren las líneas 59-63, 90-93 y
307-310).
…ear al Code Rabbit
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/modules/delivery/delivery-assignments/delivery-assignments.service.js (3)
206-216:⚠️ Potential issue | 🔴 CriticalFinalización no atómica: puede quedar assignment DELIVERED sin actualizar la orden.
Line 206-216 ejecuta dos updates separados. Si falla el segundo, rompés consistencia de negocio.
💡 Propuesta de fix transaccional
- // Actualizar asignación a DELIVERED - const updated = await prisma.deliveryAssignments.update({ - where: { id_delivery_assignment }, - data: { assignment_status: "DELIVERED" } - }); - - // Actualizar orden a DELIVERED - await prisma.orders.update({ - where: { id_order: assignment.fk_order }, - data: { order_status: "DELIVERED" } - }); + const [updated] = await prisma.$transaction([ + prisma.deliveryAssignments.update({ + where: { id_delivery_assignment }, + data: { assignment_status: "DELIVERED" } + }), + prisma.orders.update({ + where: { id_order: assignment.fk_order }, + data: { order_status: "DELIVERED" } + }) + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js` around lines 206 - 216, The two separate updates (prisma.deliveryAssignments.update for id_delivery_assignment and prisma.orders.update for assignment.fk_order) must be made atomic to avoid leaving the assignment marked DELIVERED while the order update fails; wrap both operations in a single transaction (e.g., prisma.$transaction or the equivalent transaction API) and perform both updates inside it so they commit or rollback together, and propagate or log errors from the transaction rather than executing them independently.
59-73:⚠️ Potential issue | 🟠 MajorAsignaciones con borrado lógico siguen siendo accesibles/operables.
En Line 59 y Line 187 sólo se valida existencia por id. Si
status === false, hoy igual se devuelve/usa la asignación.💡 Guard común recomendado
if (!assignment) { throw { status: 404, message: "Asignación no encontrada" }; } + if (assignment.status === false) { + throw { status: 404, message: "Asignación no encontrada" }; + }Also applies to: 187-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js` around lines 59 - 73, The fetch currently only checks id via prisma.deliveryAssignments.findUnique and returns/uses assignments even when logically deleted (status === false); update the lookup to exclude deleted rows (e.g., include status: true in the where clause of prisma.deliveryAssignments.findUnique) or, if you prefer to keep the existing query, immediately check assignment.status and throw the same 404 ({ status: 404, message: "Asignación no encontrada" }) when status is falsy; apply the same change to the other occurrence around the block that handles assignment usage (the second fetch/validation at lines ~187-194) so both prisma.deliveryAssignments.findUnique usages enforce status === true.
24-52:⚠️ Potential issue | 🟠 MajorCondición de carrera en alta de asignación (lectura+create no atómico).
Entre Line 24 y Line 52 hacés chequeos (
lastAssignment/pendingAssignment) y despuéscreatefuera de transacción. Con concurrencia podés terminar en colisión de secuencia y error no controlado en vez de conflicto de negocio (409).💡 Propuesta de ajuste
export const createAssignmentService = async (data) => { const { fk_order, fk_delivery, status } = data; - - // Verificar que la orden existe - const order = await prisma.orders.findUnique({ - where: { id_order: fk_order } - }); - if (!order) { - throw { status: 404, message: "Pedido no encontrado" }; - } - - // Verificar que el delivery existe - const delivery = await prisma.deliveries.findUnique({ - where: { id_delivery: fk_delivery } - }); - if (!delivery) { - throw { status: 404, message: "Delivery no encontrado" }; - } - - // Obtener el último intento del pedido - const lastAssignment = await prisma.deliveryAssignments.findFirst({ - where: { fk_order }, - orderBy: { assignment_sequence: 'desc' }, - select: { assignment_sequence: true } - }); - const nextSequence = (lastAssignment?.assignment_sequence || 0) + 1; - - // Verificar que no hay un intento PENDING activo - const pendingAssignment = await prisma.deliveryAssignments.findFirst({ - where: { - fk_order, - assignment_status: "PENDING" - } - }); - - if (pendingAssignment) { - throw { status: 409, message: "Ya hay una asignación pendiente para este pedido" }; - } - - // Crear el delivery assignment - const newDeliveryAssignment = await prisma.deliveryAssignments.create({ - data: { - fk_order, - fk_delivery, - assignment_status: "PENDING", - assignment_sequence: nextSequence, - status: status !== false - } - }); - - return newDeliveryAssignment; + try { + return await prisma.$transaction(async (tx) => { + const [order, delivery] = await Promise.all([ + tx.orders.findUnique({ where: { id_order: fk_order } }), + tx.deliveries.findUnique({ where: { id_delivery: fk_delivery } }) + ]); + + if (!order) throw { status: 404, message: "Pedido no encontrado" }; + if (!delivery) throw { status: 404, message: "Delivery no encontrado" }; + + const pendingAssignment = await tx.deliveryAssignments.findFirst({ + where: { fk_order, assignment_status: "PENDING", status: true } + }); + if (pendingAssignment) { + throw { status: 409, message: "Ya hay una asignación pendiente para este pedido" }; + } + + const lastAssignment = await tx.deliveryAssignments.findFirst({ + where: { fk_order }, + orderBy: { assignment_sequence: "desc" }, + select: { assignment_sequence: true } + }); + + const nextSequence = (lastAssignment?.assignment_sequence || 0) + 1; + + return tx.deliveryAssignments.create({ + data: { + fk_order, + fk_delivery, + assignment_status: "PENDING", + assignment_sequence: nextSequence, + status: status !== false + } + }); + }); + } catch (error) { + if (error?.code === "P2002") { + throw { status: 409, message: "Conflicto al crear asignación para el pedido" }; + } + throw error; + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js` around lines 24 - 52, El problema es una condición de carrera entre las lecturas (lastAssignment, pendingAssignment) y la creación (prisma.deliveryAssignments.create) que puede producir secuencias duplicadas; envuelve las lecturas y el create en una transacción atómica usando prisma.$transaction y usa el cliente transaccional (tx.deliveryAssignments) para volver a verificar pendingAssignment y lastAssignment dentro de la transacción, calcular nextSequence y luego crear el registro; además maneja errores de constraint (p. ej. única en fk_order+assignment_sequence) capturando la excepción de índice único y rejeterla como {status:409,...} si ocurre, y considera añadir un índice único sobre (fk_order, assignment_sequence) para protección adicional.src/modules/delivery/delivery/delivery.service.js (1)
61-84:⚠️ Potential issue | 🟠 MajorFalta control de ownership en lecturas por
id_delivery(riesgo IDOR).En Line 61 y Line 88 se devuelve información de cualquier
id_deliveryválido sin comprobar que pertenezca al usuario autenticado. Esto permite leer datos de otros repartidores.💡 Ajuste propuesto (servicio + impacto en controller)
-export const getPendingAssignmentsService = async (id_delivery) => { +export const getPendingAssignmentsService = async (authenticatedUserId, id_delivery) => { const delivery = await prisma.deliveries.findUnique({ @@ if (!delivery) { throw { status: 404, message: "Delivery no encontrado" }; } + if (delivery.fk_user !== authenticatedUserId) { + throw { status: 403, message: "No tienes permiso para ver estas asignaciones" }; + } @@ -export const getDeliveryByIdService = async (id_delivery) => { +export const getDeliveryByIdService = async (authenticatedUserId, id_delivery) => { const delivery = await prisma.deliveries.findUnique({ @@ if (!delivery) { throw { status: 404, message: "Delivery no encontrado" }; } + if (delivery.fk_user !== authenticatedUserId) { + throw { status: 403, message: "No tienes permiso para ver este delivery" }; + }Y en
src/modules/delivery/delivery/delivery.controller.jspasarreq.user.id_useren las llamadas de Line 84 y Line 106.Also applies to: 88-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/delivery/delivery/delivery.service.js` around lines 61 - 84, The getPendingAssignmentsService currently returns any delivery by id_delivery without verifying ownership; change its signature to accept the authenticated user's id (e.g., id_user or requesterId) and update the Prisma query's where clause to require both id_delivery and the delivery FK to match that user (e.g., where: { id_delivery, fk_user: requesterId } or equivalent FK column), so queries for other users return 404; also update the delivery controller calls to pass req.user.id_user into getPendingAssignmentsService (and apply the same ownership pattern to the other similar read service(s) referenced in the comment).
🤖 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/delivery/delivery-assignments/delivery-assignments.routes.js`:
- Around line 17-37: The POST "/" (createAssignment) and POST "/:id/complete"
(completeAssignment) routes currently only use authenticate and must enforce
role-based authorization; update delivery-assignments.routes.js to add the
role-check middleware (e.g., authorizeRole or requireRole) after authenticate
for those two routes and restrict to the appropriate roles (e.g., "delivery",
"driver", "admin") so only allowed roles can call createAssignment and
completeAssignment; ensure you import the middleware at the top and pass the
correct role(s) argument(s) when attaching it to the router.post calls that
reference createAssignment and completeAssignment.
---
Duplicate comments:
In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js`:
- Around line 206-216: The two separate updates
(prisma.deliveryAssignments.update for id_delivery_assignment and
prisma.orders.update for assignment.fk_order) must be made atomic to avoid
leaving the assignment marked DELIVERED while the order update fails; wrap both
operations in a single transaction (e.g., prisma.$transaction or the equivalent
transaction API) and perform both updates inside it so they commit or rollback
together, and propagate or log errors from the transaction rather than executing
them independently.
- Around line 59-73: The fetch currently only checks id via
prisma.deliveryAssignments.findUnique and returns/uses assignments even when
logically deleted (status === false); update the lookup to exclude deleted rows
(e.g., include status: true in the where clause of
prisma.deliveryAssignments.findUnique) or, if you prefer to keep the existing
query, immediately check assignment.status and throw the same 404 ({ status:
404, message: "Asignación no encontrada" }) when status is falsy; apply the same
change to the other occurrence around the block that handles assignment usage
(the second fetch/validation at lines ~187-194) so both
prisma.deliveryAssignments.findUnique usages enforce status === true.
- Around line 24-52: El problema es una condición de carrera entre las lecturas
(lastAssignment, pendingAssignment) y la creación
(prisma.deliveryAssignments.create) que puede producir secuencias duplicadas;
envuelve las lecturas y el create en una transacción atómica usando
prisma.$transaction y usa el cliente transaccional (tx.deliveryAssignments) para
volver a verificar pendingAssignment y lastAssignment dentro de la transacción,
calcular nextSequence y luego crear el registro; además maneja errores de
constraint (p. ej. única en fk_order+assignment_sequence) capturando la
excepción de índice único y rejeterla como {status:409,...} si ocurre, y
considera añadir un índice único sobre (fk_order, assignment_sequence) para
protección adicional.
In `@src/modules/delivery/delivery/delivery.service.js`:
- Around line 61-84: The getPendingAssignmentsService currently returns any
delivery by id_delivery without verifying ownership; change its signature to
accept the authenticated user's id (e.g., id_user or requesterId) and update the
Prisma query's where clause to require both id_delivery and the delivery FK to
match that user (e.g., where: { id_delivery, fk_user: requesterId } or
equivalent FK column), so queries for other users return 404; also update the
delivery controller calls to pass req.user.id_user into
getPendingAssignmentsService (and apply the same ownership pattern to the other
similar read service(s) referenced in the comment).
🪄 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: 51ef4d82-e83f-4ee0-a284-ca388222e916
📒 Files selected for processing (10)
src/app.jssrc/modules/delivery/delivery-assignments/delivery-assignments.controller.jssrc/modules/delivery/delivery-assignments/delivery-assignments.routes.jssrc/modules/delivery/delivery-assignments/delivery-assignments.service.jssrc/modules/delivery/delivery/delivery.controller.jssrc/modules/delivery/delivery/delivery.routes.jssrc/modules/delivery/delivery/delivery.service.jssrc/modules/delivery/delivery/delivery.validation.jstests/unit/delivery/delivery-complete.test.jstests/unit/delivery/delivery-register.test.js
✅ Files skipped from review due to trivial changes (1)
- src/modules/delivery/delivery-assignments/delivery-assignments.controller.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/delivery/delivery/delivery.validation.js
|
CrisNAC
approved these changes
Apr 29, 2026
This was referenced Apr 29, 2026
Merged
Merged
This was referenced May 29, 2026
Merged
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.
Descripción
Implementación completa del módulo de deliveries para la tarea OM-489.
Cambios
Pendiente
Endpoints
Summary by CodeRabbit
Nuevas Funcionalidades
Chores
Tests