Repository navigation
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 24 minutes and 42 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughSe implementa autorización y validación en el flujo de asignaciones de delivery. Se agregan helpers centralizados de seguridad, se propaga el usuario autenticado a todos los servicios, se valida pertenencia de tienda en creación, se restringe acceso en lecturas según rol, y se exige una asignación ACCEPTED para que repartidores transicionen pedidos a DELIVERED. Se actualiza documentación de auditoría BAC. ChangesValidación de autorización en asignaciones de delivery y transiciones de pedidos
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
🧹 Nitpick comments (2)
tests/e2e/delivery-register.test.js (1)
138-142: ⚡ Quick winNo aflojen la cobertura del
phoneheredado.Ahora el setup agrega
phone, pero la expectativa delusers.updatesólo validarole. SiregisterDeliveryServicedeja de propagar el teléfono existente cuando no viene en el body, este caso seguiría pasando.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/delivery-register.test.js` around lines 138 - 142, The test currently only asserts that prisma.users.update is called with role "DELIVERY" but ignores preserving the existing phone added in the test setup; update the expectation in the delivery register test so prisma.users.update's data includes both role: "DELIVERY" and the existing phone value from the test setup (e.g., assert data contains { role: "DELIVERY", phone: <existingPhoneVariable> }) to ensure registerDeliveryService preserves inherited phone when none is provided in the request body.tests/unit/order/order.test.js (1)
249-270: ⚡ Quick winAserten también el
wherededeliveryAssignments.findFirst.Estos casos sólo mockean el retorno, así que un filtro Prisma mal armado en el servicio también queda verde. Una expectativa sobre la forma del
whereles cubre la autorización de DELIVERY y evita que se cuele de nuevo un query inválido.🧪 Ejemplo
const result = await updateOrderStatusService(1, 100, "DELIVERED"); expect(result.status).toBe("DELIVERED"); + expect(prisma.deliveryAssignments.findFirst).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ + fk_order: 100, + assignment_status: "ACCEPTED", + status: true, + delivery: expect.objectContaining({ + is: { fk_user: 1 } + }) + }) + }) + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/order/order.test.js` around lines 249 - 270, Añade en ambos tests una expectativa que verifique el filtro pasado a prisma.deliveryAssignments.findFirst para asegurarnos de que el where está correctamente construido; por ejemplo, después de preparar los mocks y antes/after de invocar updateOrderStatusService(1, 100, "DELIVERED") añade expect(prisma.deliveryAssignments.findFirst).toHaveBeenCalledWith(expect.objectContaining({ where: expect.objectContaining({ fk_delivery_user: 1, fk_order: 100 }) })) (ajusta los keys si en tu modelo se llaman distinto) para validar que la autorización de DELIVERY usa el where correcto en la llamada desde updateOrderStatusService.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js`:
- Around line 45-56: The DELIVERY authorization currently only checks fk_user on
deliveryAssignments queries (e.g., in getDeliveryPendingAssignmentsService using
prisma.deliveryAssignments.findFirst) and thus allows disabled delivery users;
replicate the same active-status check used in getDeliveryOrderHistoryService by
ensuring the query filters for delivery.status = true (or explicitly verify
delivery.status and throw the 403) wherever deliveries are authorized (including
the affected branches around the findFirst/select checks and the other instances
you noted) so disabled deliveries cannot list or read order/client data.
- Around line 73-87: The current access check in delivery-assignments.service.js
only compares order.store.fk_user to auth.id_user and misses store.status,
allowing suspended/inactive stores to act; update the code (same pattern at the
other occurrences noted) to select store.status when fetching the order and
enforce that the store is active before permitting assignment or reads: e.g.,
add store: { select: { fk_user: true, status: true } } in the
prisma.orders.findUnique call and throw a 403 (same shape as existing errors) if
store.status is not the expected "active" value (or reuse ensureOrderAccess()
behavior) so suspended/inactive merchants are blocked.
In `@src/modules/users/orders/order.service.js`:
- Around line 864-878: La validación de delivery está sujeta a TOCTOU porque se
hace con prisma.deliveryAssignments.findFirst antes de la actualización; arregla
envolviendo la revalidación y el update en la misma transacción: usar
prisma.$transaction(async (tx) => { const deliveryAssignment = await
tx.deliveryAssignments.findFirst({ where: { fk_order: resolvedOrderId,
assignment_status: "ACCEPTED", status: true, delivery: { fk_user: resolvedUserId
} }, select: { id_delivery_assignment: true } }); if (!deliveryAssignment) throw
new ForbiddenError("No tienes permisos para modificar este pedido."); await
tx.orders.update({ where: { id: resolvedOrderId }, data: { /* cambios que ya
haces */ } }); }); y asegurarte de reemplazar las llamadas a prisma.* por tx.*
dentro del cierre para que la comprobación y el orders.update sean atómicos.
- Around line 865-870: El where de la consulta
prisma.deliveryAssignments.findFirst usa una sintaxis inválida para una relación
to-one; reemplazá el filtro relacional delivery: { fk_user: resolvedUserId } por
delivery: { is: { fk_user: resolvedUserId } } dentro de la misma llamada a
prisma.deliveryAssignments.findFirst, y además mové la verificación de
autorización relacionada con esa asignación para ejecutarla dentro de la misma
prisma.$transaction que realiza la actualización del pedido (asegurate de usar
la misma transacción para leer la asignación aceptada y luego mutar el pedido)
para evitar condiciones de carrera.
---
Nitpick comments:
In `@tests/e2e/delivery-register.test.js`:
- Around line 138-142: The test currently only asserts that prisma.users.update
is called with role "DELIVERY" but ignores preserving the existing phone added
in the test setup; update the expectation in the delivery register test so
prisma.users.update's data includes both role: "DELIVERY" and the existing phone
value from the test setup (e.g., assert data contains { role: "DELIVERY", phone:
<existingPhoneVariable> }) to ensure registerDeliveryService preserves inherited
phone when none is provided in the request body.
In `@tests/unit/order/order.test.js`:
- Around line 249-270: Añade en ambos tests una expectativa que verifique el
filtro pasado a prisma.deliveryAssignments.findFirst para asegurarnos de que el
where está correctamente construido; por ejemplo, después de preparar los mocks
y antes/after de invocar updateOrderStatusService(1, 100, "DELIVERED") añade
expect(prisma.deliveryAssignments.findFirst).toHaveBeenCalledWith(expect.objectContaining({
where: expect.objectContaining({ fk_delivery_user: 1, fk_order: 100 }) }))
(ajusta los keys si en tu modelo se llaman distinto) para validar que la
autorización de DELIVERY usa el where correcto en la llamada desde
updateOrderStatusService.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd0d1605-c2f6-4779-84c3-f443a4c84553
📒 Files selected for processing (14)
docs/auditoria-bac.mdsrc/modules/delivery/delivery-assignments/delivery-assignments.controller.jssrc/modules/delivery/delivery-assignments/delivery-assignments.service.jssrc/modules/delivery/delivery/delivery.controller.jssrc/modules/users/orders/order.service.jstests/e2e/delivery-assignments-create.test.jstests/e2e/delivery-register.test.jstests/e2e/notifications.test.jstests/e2e/stock.test.jstests/unit/commerce/delivery-assignments.test.jstests/unit/delivery/delivery-assignments-controller.test.jstests/unit/delivery/delivery-assignments.test.jstests/unit/delivery/delivery-register.test.jstests/unit/order/order.test.js
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/users/orders/order.service.js`:
- Around line 890-904: El fallo viene del test: actualizar el mock de la
transacción para incluir un resultado para deliveryAssignments.findFirst usado
por order.service (TOCTOU ya resuelto). En la fábrica createMockTx o en el caso
de prueba "DELIVERY SHIPPED → DELIVERED: transición permitida" añade
deliveryAssignments: { findFirst: vi.fn().mockResolvedValue({
id_delivery_assignment: 1 }) } (o similar) para que la llamada dentro de la
transacción no devuelva undefined y no lance ForbiddenError; referencia
createMockTx y deliveryAssignments.findFirst para localizar dónde añadir el
mock.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3735b9cd-7a1d-4ca7-b9ed-d4b7ac004074
📒 Files selected for processing (3)
src/modules/delivery/delivery-assignments/delivery-assignments.service.jssrc/modules/users/orders/order.service.jstsconfig.json
✅ Files skipped from review due to trivial changes (1)
- tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/delivery/delivery-assignments/delivery-assignments.service.js
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/modules/delivery/delivery-assignments/delivery-assignments.service.js (1)
189-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFalta bloquear entidades inactivas en lecturas autorizadas (store/delivery).
En Line 203, 211, 272 y 331 se valida propietario (
fk_user) pero no estado activo. Un comercio o delivery desactivado podría seguir leyendo asignaciones/datos de pedido. Este punto ya había sido señalado previamente y reapareció en estas rutas.🔒 Ajuste sugerido
order: { select: { id_order: true, total: true, created_at: true, fk_user: true, - store: { select: { fk_user: true } } + store: { select: { fk_user: true, status: true } } } }, delivery: { - select: { id_delivery: true, delivery_status: true, fk_user: true } + select: { id_delivery: true, delivery_status: true, fk_user: true, status: true } } - if (assignment.order?.store?.fk_user !== auth.id_user) { + if (assignment.order?.store?.fk_user !== auth.id_user || !assignment.order?.store?.status) { throw { status: 403, message: "No tienes permiso para ver esta asignación" }; } - if (assignment.delivery?.fk_user !== auth.id_user) { + if (assignment.delivery?.fk_user !== auth.id_user || !assignment.delivery?.status) { throw { status: 403, message: "No tienes permiso para ver esta asignación" }; } const delivery = await prisma.deliveries.findUnique({ where: { id_delivery }, - select: { id_delivery: true, fk_user: true } + select: { id_delivery: true, fk_user: true, status: true } }); - if (delivery.fk_user !== auth.id_user) { + if (delivery.fk_user !== auth.id_user || !delivery.status) { throw { status: 403, message: "No tienes permiso para ver asignaciones de este delivery" }; }Also applies to: 202-213, 263-274, 322-333
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js` around lines 189 - 193, Las lecturas autorizadas están validando solo el propietario (fk_user) pero no el estado activo de las entidades store/delivery; actualiza las consultas y validaciones en delivery-assignments.service.js para incluir el filtro/chequeo de entidad activa (por ejemplo where: { fk_user: ..., active: true } o comprobar store.active/delivery.active junto a fk_user) en los lugares que usan store y delivery (referencias a store, delivery, fk_user, delivery_status en las selecciones) para bloquear comercios/deliveries desactivados al leer asignaciones; aplícalo de forma consistente en los puntos señalados (las validaciones que usan fk_user en este servicio).
🧹 Nitpick comments (1)
src/modules/delivery/delivery/delivery.controller.js (1)
54-54: ⚡ Quick winInconsistencia en el manejo de errores de validación.
Esta función ahora devuelve siempre el mensaje fijo
"Datos inválidos"cuando hay unZodError, pero las funcionesupdateDeliveryStatus(línea 94) yupdateDeliveryProfile(línea 175) en el mismo archivo aún usanerror.issues[0].messagepara extraer mensajes dinámicos de validación. Esto crea una experiencia de usuario inconsistente entre endpoints.Si la intención es unificar los mensajes de error, considerá aplicar el mismo patrón en los tres bloques catch de
ZodError.🔧 Propuesta para unificar el manejo de errores
Aplicá el mismo patrón en las líneas 94 y 175:
if (error instanceof ZodError) { return res.status(400).json({ error: { code: 400, - message: error.issues[0].message, + message: "Datos inválidos", details: error.issues } }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/delivery/delivery/delivery.controller.js` at line 54, En el bloque catch que actualmente responde con message: "Datos inválidos" reemplazá ese mensaje estático por la extracción dinámica del primer issue del ZodError (por ejemplo usar error.issues[0].message) para que coincida con el comportamiento ya usado en updateDeliveryStatus y updateDeliveryProfile; mantené la validación de tipo (instanceof ZodError) y devolvé el mensaje dinámico en el mismo formato de respuesta que usan esas funciones.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js`:
- Around line 46-51: En ensureOrderAccess y en la consulta que busca
asignaciones (ej. la query en deliveryAssignments.findFirst) cambia el filtro de
relación to-one de delivery: { ... } a delivery: { is: { fk_user: auth.id_user,
status: true, delivery_status: 'ACTIVE' } } (o usa el enum/valor correcto para
estado activo) para usar la sintaxis Prisma correcta y además en
getDeliveryAssignmentsService y getDeliveryPendingAssignmentsService añade el
mismo filtro sobre delivery.is para forzar que solo se devuelvan asignaciones
cuyo delivery pertenece al usuario y esté activo (incluye status y
delivery_status según el modelo).
In `@tests/e2e/delivery-assignments-create.test.js`:
- Around line 80-84: El mock de pedido usado en el test falla la nueva
precondición de store activa: en el objeto retornado por
prisma.orders.findUnique (usado por createAssignmentService) añade la propiedad
status: true dentro de store (p. ej. store: { fk_user: 1, status: true }) para
que la validación de store activa pase y el test no devuelva 403.
In `@tests/unit/commerce/delivery-assignments.test.js`:
- Around line 217-221: El mock de orders usado por los tests de
createAssignmentService está devolviendo store sin la propiedad status activa,
lo que provoca un 403 antes de llegar a las comprobaciones esperadas; actualiza
los mocks de vi.mocked(prisma.orders.findUnique) en los puntos donde se define
store (los casos alrededor del test que espera 409/PENDING) para incluir
store.status: true y así permitir que createAssignmentService continue hasta la
lógica que debe probarse (asegúrate de modificar ambos mocks que devuelven store
en esos tests).
In `@tests/unit/delivery/delivery-assignments.test.js`:
- Around line 77-83: El fixture mockOrder está desactualizado: su propiedad
store debe incluir status: true para evitar que la capa de autorización devuelva
403 y permitir que los tests de create lleguen a los escenarios 404/409;
actualiza el objeto mockOrder (símbolo: mockOrder en
delivery-assignments.test.js) añadiendo store.status = true y revisa otros casos
de prueba que reutilicen mockOrder para garantizar que esperan un store activo
cuando corresponda.
In `@tests/unit/delivery/delivery-register.test.js`:
- Around line 124-126: El objeto mock en el test de delivery tiene la clave
duplicada "phone" (aparecen `phone: "1234567890"` dos veces alrededor de la
propiedad `delivery`); elimina la segunda definición duplicada dejando solo una
entrada `phone` para ese objeto (mantén el valor existente) para evitar
sobrescritura redundante en el test.
---
Duplicate comments:
In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js`:
- Around line 189-193: Las lecturas autorizadas están validando solo el
propietario (fk_user) pero no el estado activo de las entidades store/delivery;
actualiza las consultas y validaciones en delivery-assignments.service.js para
incluir el filtro/chequeo de entidad activa (por ejemplo where: { fk_user: ...,
active: true } o comprobar store.active/delivery.active junto a fk_user) en los
lugares que usan store y delivery (referencias a store, delivery, fk_user,
delivery_status en las selecciones) para bloquear comercios/deliveries
desactivados al leer asignaciones; aplícalo de forma consistente en los puntos
señalados (las validaciones que usan fk_user en este servicio).
---
Nitpick comments:
In `@src/modules/delivery/delivery/delivery.controller.js`:
- Line 54: En el bloque catch que actualmente responde con message: "Datos
inválidos" reemplazá ese mensaje estático por la extracción dinámica del primer
issue del ZodError (por ejemplo usar error.issues[0].message) para que coincida
con el comportamiento ya usado en updateDeliveryStatus y updateDeliveryProfile;
mantené la validación de tipo (instanceof ZodError) y devolvé el mensaje
dinámico en el mismo formato de respuesta que usan esas funciones.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c99c60d3-29da-4114-ba6e-d4a746b7a884
📒 Files selected for processing (14)
docs/auditoria-bac.mdsrc/modules/delivery/delivery-assignments/delivery-assignments.controller.jssrc/modules/delivery/delivery-assignments/delivery-assignments.service.jssrc/modules/delivery/delivery/delivery.controller.jssrc/modules/users/orders/order.service.jstests/e2e/delivery-assignments-create.test.jstests/e2e/delivery-register.test.jstests/e2e/notifications.test.jstests/e2e/stock.test.jstests/unit/commerce/delivery-assignments.test.jstests/unit/delivery/delivery-assignments-controller.test.jstests/unit/delivery/delivery-assignments.test.jstests/unit/delivery/delivery-register.test.jstests/unit/order/order.test.js
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
Collaborator
Author
|
@coderabbitai resolve merge conflict |
Contributor
|
✅ Merge conflicts resolved successfully! Resolved 3 conflict file(s). Commit: 11 file operation(s)
View agent analysis |
Resolved conflicts in: - tests/e2e/delivery-register.test.js (content) - tests/e2e/notifications.test.js (content) - tests/e2e/stock.test.js (content) Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/modules/delivery/delivery-assignments/delivery-assignments.service.js (3)
221-228:⚠️ Potential issue | 🟠 Major
getAssignmentByIdServicequedó más permisivo paraSELLERque el helper común.
ensureOrderAccess()ya exigestores.status = true, pero este path manual sólo comparafk_user. Con un token todavía válido, un seller suspendido puede seguir leyendo la asignación por ID. Acá conviene traerstore.statusen elselecty rechazar!assignment.order?.store?.status, o directamente reutilizar el helper.Also applies to: 241-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js` around lines 221 - 228, La rama en getAssignmentByIdService está más permisiva para SELLER que el helper ensureOrderAccess: actualmente sólo selecciona order.store.fk_user pero no order.store.status, por lo que un vendedor suspendido aún puede leer la asignación; arregla esto trayendo store.status en el select (añadir store: { select: { fk_user: true, status: true } }) y luego rechaza el acceso cuando !assignment.order?.store?.status, o mejor: reutiliza el helper ensureOrderAccess() para validar permisos/estado de la tienda en lugar de la comprobación manual; aplica el mismo cambio al otro lugar similar en este archivo (el segundo path equivalente mencionado en el comentario).
46-54:⚠️ Potential issue | 🟠 MajorUsen el filtro
isde Prisma para esta relación to-one.
deliveryendeliveryAssignmentsse está filtrando como una relación to-one. Condelivery: { fk_user: ..., status: true }, Prisma valida elwherecomo inválido y la ramaDELIVERYdeensureOrderAccess()termina rompiendo en runtime antes de autorizargetOrderAssignmentsService()ygetAcceptedAssignmentService().🔧 Ajuste mínimo
const assignment = await prisma.deliveryAssignments.findFirst({ where: { fk_order: order.id_order, status: true, - delivery: { fk_user: auth.id_user, status: true } + delivery: { is: { fk_user: auth.id_user, status: true } } }, select: { id_delivery_assignment: true } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js` around lines 46 - 54, La rama que maneja auth.role === "DELIVERY" usa prisma.deliveryAssignments.findFirst con la relación to-one `delivery` pasada directamente; cambia ese filtro a usar el operador `is` para relaciones to-one (por ejemplo, reemplazar delivery: { fk_user: auth.id_user, status: true } por delivery: { is: { fk_user: auth.id_user, status: true } }) conservando el resto del where (fk_order, status) y el select (id_delivery_assignment) para evitar la validación inválida del where en ensureOrderAccess()/getOrderAssignmentsService()/getAcceptedAssignmentService().
231-233:⚠️ Potential issue | 🟠 MajorMantengan el bloqueo de deliveries inactivos en todas las lecturas.
Estas ramas siguen autorizando sólo por
fk_user. Un delivery constatus = falsetodavía puede leer una asignación puntual y, en los listados, alcanzarexpireStalePendingAssignments()antes de devolver la respuesta.getDeliveryOrderHistoryService()ya corta con!delivery.status, así que la política quedó inconsistente y sigue exponiendo datos de pedido/cliente a cuentas deshabilitadas.Also applies to: 249-252, 302-315, 361-374
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js` around lines 231 - 233, Las lecturas siguen exponiendo asignaciones de deliveries desactivados porque sólo filtran por fk_user; debes bloquear siempre deliveries inactivos añadiendo una condición que requiera delivery.delivery_status === true en todas las consultas/lecturas y/o comprobando inmediatamente después del fetch y rechazando si delivery.delivery_status es false. Revisa y corrige los lugares señalados donde aparece el objeto delivery (por ejemplo el bloque select mostrado y las consultas en las zonas 249-252, 302-315, 361-374), y mantén la misma política que usa getDeliveryOrderHistoryService() y la lógica de expireStalePendingAssignments(): o bien incorporar delivery: { where: { delivery_status: true } } en las consultas ORM/N+1, o validar y devolver error/empty cuando delivery.delivery_status sea falso para evitar exponer datos a cuentas deshabilitadas.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js`:
- Around line 221-228: La rama en getAssignmentByIdService está más permisiva
para SELLER que el helper ensureOrderAccess: actualmente sólo selecciona
order.store.fk_user pero no order.store.status, por lo que un vendedor
suspendido aún puede leer la asignación; arregla esto trayendo store.status en
el select (añadir store: { select: { fk_user: true, status: true } }) y luego
rechaza el acceso cuando !assignment.order?.store?.status, o mejor: reutiliza el
helper ensureOrderAccess() para validar permisos/estado de la tienda en lugar de
la comprobación manual; aplica el mismo cambio al otro lugar similar en este
archivo (el segundo path equivalente mencionado en el comentario).
- Around line 46-54: La rama que maneja auth.role === "DELIVERY" usa
prisma.deliveryAssignments.findFirst con la relación to-one `delivery` pasada
directamente; cambia ese filtro a usar el operador `is` para relaciones to-one
(por ejemplo, reemplazar delivery: { fk_user: auth.id_user, status: true } por
delivery: { is: { fk_user: auth.id_user, status: true } }) conservando el resto
del where (fk_order, status) y el select (id_delivery_assignment) para evitar la
validación inválida del where en
ensureOrderAccess()/getOrderAssignmentsService()/getAcceptedAssignmentService().
- Around line 231-233: Las lecturas siguen exponiendo asignaciones de deliveries
desactivados porque sólo filtran por fk_user; debes bloquear siempre deliveries
inactivos añadiendo una condición que requiera delivery.delivery_status === true
en todas las consultas/lecturas y/o comprobando inmediatamente después del fetch
y rechazando si delivery.delivery_status es false. Revisa y corrige los lugares
señalados donde aparece el objeto delivery (por ejemplo el bloque select
mostrado y las consultas en las zonas 249-252, 302-315, 361-374), y mantén la
misma política que usa getDeliveryOrderHistoryService() y la lógica de
expireStalePendingAssignments(): o bien incorporar delivery: { where: {
delivery_status: true } } en las consultas ORM/N+1, o validar y devolver
error/empty cuando delivery.delivery_status sea falso para evitar exponer datos
a cuentas deshabilitadas.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec4a11cb-848f-4d72-88b3-c98a2e8a55bc
📒 Files selected for processing (2)
src/modules/delivery/delivery-assignments/delivery-assignments.service.jssrc/modules/users/orders/order.service.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/users/orders/order.service.js
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Request timed out after 300000ms (requestId=99b1a606-576d-42a3-88b6-e6cf41c75443) |
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 5 file(s) based on 5 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 5 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
CrisNAC
approved these changes
Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Documentation
Bug Fixes
Tests