Repository navigation
Implement notification system and update .gitignore - #152
Merged
Merged
Conversation
…direction purposes
…rs, services, and tests
Contributor
📝 WalkthroughResumen generalSe implementa un sistema completo de notificaciones que permite crear, recuperar y marcar notificaciones como leídas. Las notificaciones se generan automáticamente cuando se confirma una orden, almacenando una referencia a la entidad relacionada y permitiendo que los usuarios las consulten a través de una API HTTP autenticada. CambiosSistema de notificaciones para órdenes
Diagrama de secuenciasequenceDiagram
participant Client
participant OrderAPI as Order API
participant NotifService as Notification Service
participant DB
participant NotifAPI as Notification API
Client->>OrderAPI: POST crear orden
OrderAPI->>OrderAPI: Validar carrito
OrderAPI->>NotifService: createNotificationService(tx, ORDER_CONFIRMED)
NotifService->>DB: INSERT notification
OrderAPI-->>Client: 200 orden creada
Client->>NotifAPI: GET /api/notifications
NotifAPI->>NotifService: getNotificationsService(userId)
NotifService->>DB: SELECT active notifications
NotifService-->>NotifAPI: notificaciones con unreadCount
NotifAPI-->>Client: 200 lista de notificaciones
Client->>NotifAPI: PATCH /api/notifications/1/read
NotifAPI->>NotifService: markNotificationAsReadService(userId, id)
NotifService->>DB: UPDATE notification read=true
NotifService-->>NotifAPI: notificación actualizada
NotifAPI-->>Client: 200 marcada como leída
Esfuerzo estimado de revisión de código🎯 3 (Moderado) | ⏱️ ~20 minutos PRs posiblemente relacionados
Revisores sugeridos
Poema 🐰
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
CrisNAC
requested review from
Andoumeda,
SebaKisser and
leoAchu16
and removed request for
Andoumeda and
leoAchu16
May 13, 2026 19:58
|
![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: 1
🧹 Nitpick comments (5)
src/modules/users/orders/order.service.js (1)
384-386: 💤 Low valueSimplificar la creación de la notificación usando todos los campos del mensaje.
La destructuración en la línea 385 extrae solo
titleymessage, pero luego se pasareference_id: order.id_orderpor separado. Sin embargo,NOTIFICATION_MESSAGES.ORDER_CONFIRMED(order.id_order)ya incluyereference_idcon el mismo valor. Podrías simplificar el código usando el spread operator.♻️ Simplificación propuesta
// crear notificación de nuevo pedido para el cliente - const { title, message } = NOTIFICATION_MESSAGES.ORDER_CONFIRMED(order.id_order); - await createNotificationService(tx, { userId: resolvedUserId, title, message, reference_id: order.id_order }); + const notificationData = NOTIFICATION_MESSAGES.ORDER_CONFIRMED(order.id_order); + await createNotificationService(tx, { userId: resolvedUserId, ...notificationData });Esto evita duplicar
order.id_ordery hace el código más mantenible si se agregan más campos al mensaje en el futuro.🤖 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/users/orders/order.service.js` around lines 384 - 386, La creación de la notificación duplica el campo reference_id: llama NOTIFICATION_MESSAGES.ORDER_CONFIRMED(order.id_order) y solo extraes title y message antes de pasar reference_id por separado; en su lugar, pasa todo el objeto de mensaje retornado por NOTIFICATION_MESSAGES.ORDER_CONFIRMED junto con userId y tx a createNotificationService (manteniendo resolvedUserId y tx), por ejemplo usando el spread del resultado de NOTIFICATION_MESSAGES.ORDER_CONFIRMED en el segundo argumento de createNotificationService para evitar duplicación y facilitar futuras extensiones.src/modules/notifications/notification.service.js (1)
18-47: ⚡ Quick winConsiderar optimizar el cálculo de notificaciones no leídas.
El
unreadCountse calcula filtrando en memoria todas las notificaciones después de traerlas desde la base de datos (línea 34). Para usuarios con muchas notificaciones, esto puede ser ineficiente. Una consulta separada con agregación o el uso de_countde Prisma sería más performante.♻️ Optimización propuesta
export const getNotificationsService = async (authenticatedUserId) => { const resolvedUserId = parsePositiveInteger(authenticatedUserId, "userId"); - const notifications = await prisma.notifications.findMany({ + const [notifications, unreadCount] = await Promise.all([ + prisma.notifications.findMany({ - where: { fk_user: resolvedUserId, status: true }, + where: { fk_user: resolvedUserId, status: true }, - orderBy: { created_at: "desc" }, + orderBy: { created_at: "desc" }, - select: { + select: { - id_notification: true, + id_notification: true, - title: true, + title: true, - message: true, + message: true, - read: true, + read: true, - reference_id: true, + reference_id: true, - created_at: true + created_at: true + } + }), + prisma.notifications.count({ + where: { fk_user: resolvedUserId, status: true, read: false } + }) - } - }); + ]); - const unreadCount = notifications.filter((n) => !n.read).length; - return {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/notifications/notification.service.js` around lines 18 - 47, In getNotificationsService replace the in-memory unreadCount calculation (currently using notifications.filter(...) after the prisma.notifications.findMany call) with a DB-side count to avoid loading all rows: run a separate Prisma count/aggregate query (e.g., prisma.notifications.count or prisma.notifications.aggregate) with where: { fk_user: resolvedUserId, status: true, read: false } to compute unreadCount, keep the existing prisma.notifications.findMany only for the list mapping, and assign the count result to unreadCount.tests/unit/notifications/notification.test.js (2)
23-29: ⚡ Quick winFalta validar
referenceIden el mapeo de notificaciones.Con el nuevo contrato del PR, estos tests pueden pasar aunque se rompa el mapeo de
reference_idareferenceId.💡 Propuesta de ajuste
const mockNotificationUnread = { id_notification: 1, + reference_id: 100, title: "¡Pedido confirmado!", message: "Tu pedido `#100` fue confirmado correctamente.", read: false, created_at: "2026-01-01T00:00:00.000Z", }; const mockNotificationRead = { id_notification: 2, + reference_id: 101, title: "¡Pedido confirmado!", message: "Tu pedido `#101` fue confirmado correctamente.", read: true, created_at: "2026-01-01T00:00:00.000Z", }; @@ expect(notification).toMatchObject({ id: 1, + referenceId: 100, title: "¡Pedido confirmado!", message: "Tu pedido `#100` fue confirmado correctamente.", read: false, createdAt: "2026-01-01T00:00:00.000Z", }); @@ expect(result).toMatchObject({ id: 1, + referenceId: 100, title: "¡Pedido confirmado!", read: true, createdAt: "2026-01-01T00:00:00.000Z", });Also applies to: 31-37, 73-85, 136-153
🤖 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/notifications/notification.test.js` around lines 23 - 29, The tests are missing assertions for the new referenceId mapping so they don't catch regressions; update the notification mocks (e.g., mockNotificationUnread and the other mocks at the referenced blocks) to include the backend field reference_id, and add assertions in the relevant tests (the test that maps notifications and any expectations around mapped objects) to check that the mapped object has referenceId === mock.reference_id; locate the mapping logic test where notifications are transformed (look for variable names like mockNotificationUnread, the mapping function under test, and assertions checking title/message/read) and add the referenceId assertion for each corresponding mock.
3-6: ⚡ Quick winLa suite no cubre
createNotificationService.Queda sin testear la escritura de notificaciones (incluyendo validaciones y persistencia de
reference_id) en el mismo módulo.Also applies to: 39-41, 95-97
🤖 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/notifications/notification.test.js` around lines 3 - 6, The tests currently cover getNotificationsService and markNotificationAsReadService but do not exercise createNotificationService; add unit tests for createNotificationService in the notification.test.js suite that assert (1) input validation errors are thrown for missing/invalid fields, (2) a notification is persisted to the store when valid input is provided, and (3) the reference_id field is saved and retrievable; use the existing test setup/mocks used for getNotificationsService/markNotificationAsReadService (reusing the same mocked repository or DB helper) and call createNotificationService with both invalid and valid payloads, then verify persistence by querying via getNotificationsService or the mocked repository and asserting reference_id equals the provided value.tests/unit/order/order.test.js (1)
55-57: ⚡ Quick winSe mockea
notifications.create, pero no se verifica que se use correctamente.Así los tests pasan aunque el servicio deje de crear notificaciones o las cree con payload inválido.
💡 Propuesta de aserción mínima
it("crea la orden correctamente con dirección válida", async () => { @@ const result = await createOrderService(1, { @@ expect(result).toMatchObject({ id: 100, status: "PENDING", total: 200, }); + expect(prisma.notifications.create).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + fk_user: 1, + read: false, + }), + }) + ); @@ });it("SELLER puede pasar de PENDING a PROCESSING", async () => { + const txNotificationsCreate = vi.fn().mockResolvedValue({}); prisma.$transaction.mockImplementation(async (fn) => fn({ @@ - notifications: { create: vi.fn().mockResolvedValue({}) }, + notifications: { create: txNotificationsCreate }, }) ); @@ const result = await updateOrderStatusService(1, 100, "PROCESSING"); expect(result.status).toBe("PROCESSING"); + expect(txNotificationsCreate).toHaveBeenCalled(); });Also applies to: 257-257, 282-282, 306-306, 331-331, 544-545, 565-566, 586-587, 636-637, 680-681, 744-745, 777-778
🤖 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 55 - 57, Se está haciendo mock de notifications.create (vi.fn()) pero no se verifica su uso; añade en cada test donde se mockea (por ejemplo en order.test.js donde se define notifications: { create: vi.fn() }) una aserción que confirme que notifications.create fue llamada y con el payload esperado (por ejemplo usando expect(notifications.create).toHaveBeenCalled() y expect(notifications.create).toHaveBeenCalledWith(<expectedPayload>/expect.objectContaining({...}) según el caso); aplica la misma verificación a todas las demás instancias señaladas (líneas 257, 282, 306, 331, 544-545, 565-566, 586-587, 636-637, 680-681, 744-745, 777-778) para evitar falsos positivos cuando el servicio deje de crear notificaciones o cambie el formato del payload.
🤖 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 `@prisma/schema.prisma`:
- Line 53: Add a discriminator field to accompany reference_id so the schema can
identify which entity the ID points to (e.g., add a new enum type like
ReferenceType and a field such as reference_type or reference_entity on the same
model that contains reference_id); update the model that holds reference_id
(look for the reference_id field) to include the new enum-typed field and
migrate the DB accordingly, and adjust application logic to validate
reference_id against reference_type when resolving relations or performing
cascades (this preserves integrity and enables safer deletes/queries).
---
Nitpick comments:
In `@src/modules/notifications/notification.service.js`:
- Around line 18-47: In getNotificationsService replace the in-memory
unreadCount calculation (currently using notifications.filter(...) after the
prisma.notifications.findMany call) with a DB-side count to avoid loading all
rows: run a separate Prisma count/aggregate query (e.g.,
prisma.notifications.count or prisma.notifications.aggregate) with where: {
fk_user: resolvedUserId, status: true, read: false } to compute unreadCount,
keep the existing prisma.notifications.findMany only for the list mapping, and
assign the count result to unreadCount.
In `@src/modules/users/orders/order.service.js`:
- Around line 384-386: La creación de la notificación duplica el campo
reference_id: llama NOTIFICATION_MESSAGES.ORDER_CONFIRMED(order.id_order) y solo
extraes title y message antes de pasar reference_id por separado; en su lugar,
pasa todo el objeto de mensaje retornado por
NOTIFICATION_MESSAGES.ORDER_CONFIRMED junto con userId y tx a
createNotificationService (manteniendo resolvedUserId y tx), por ejemplo usando
el spread del resultado de NOTIFICATION_MESSAGES.ORDER_CONFIRMED en el segundo
argumento de createNotificationService para evitar duplicación y facilitar
futuras extensiones.
In `@tests/unit/notifications/notification.test.js`:
- Around line 23-29: The tests are missing assertions for the new referenceId
mapping so they don't catch regressions; update the notification mocks (e.g.,
mockNotificationUnread and the other mocks at the referenced blocks) to include
the backend field reference_id, and add assertions in the relevant tests (the
test that maps notifications and any expectations around mapped objects) to
check that the mapped object has referenceId === mock.reference_id; locate the
mapping logic test where notifications are transformed (look for variable names
like mockNotificationUnread, the mapping function under test, and assertions
checking title/message/read) and add the referenceId assertion for each
corresponding mock.
- Around line 3-6: The tests currently cover getNotificationsService and
markNotificationAsReadService but do not exercise createNotificationService; add
unit tests for createNotificationService in the notification.test.js suite that
assert (1) input validation errors are thrown for missing/invalid fields, (2) a
notification is persisted to the store when valid input is provided, and (3) the
reference_id field is saved and retrievable; use the existing test setup/mocks
used for getNotificationsService/markNotificationAsReadService (reusing the same
mocked repository or DB helper) and call createNotificationService with both
invalid and valid payloads, then verify persistence by querying via
getNotificationsService or the mocked repository and asserting reference_id
equals the provided value.
In `@tests/unit/order/order.test.js`:
- Around line 55-57: Se está haciendo mock de notifications.create (vi.fn())
pero no se verifica su uso; añade en cada test donde se mockea (por ejemplo en
order.test.js donde se define notifications: { create: vi.fn() }) una aserción
que confirme que notifications.create fue llamada y con el payload esperado (por
ejemplo usando expect(notifications.create).toHaveBeenCalled() y
expect(notifications.create).toHaveBeenCalledWith(<expectedPayload>/expect.objectContaining({...})
según el caso); aplica la misma verificación a todas las demás instancias
señaladas (líneas 257, 282, 306, 331, 544-545, 565-566, 586-587, 636-637,
680-681, 744-745, 777-778) para evitar falsos positivos cuando el servicio deje
de crear notificaciones o cambie el formato del payload.
🪄 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: 29283320-20d9-497f-bba1-83266d90342a
📒 Files selected for processing (11)
.gitignoreprisma/schema.prismasrc/app.jssrc/modules/commerce/orders/.gitkeepsrc/modules/notifications/notification.constant.jssrc/modules/notifications/notification.controller.jssrc/modules/notifications/notification.routes.jssrc/modules/notifications/notification.service.jssrc/modules/users/orders/order.service.jstests/unit/notifications/notification.test.jstests/unit/order/order.test.js
SebaKisser
approved these changes
May 13, 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
Notas de Lanzamiento
New Features
Tests
Chores