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:
📝 WalkthroughWalkthroughSe agregan módulos CRUD de direcciones para comercios y usuarios, se exige autenticación en endpoints de productos (crear/actualizar), se refactoriza y extiende el servicio de productos (create/update/search/getById), se exporta la función de autorización de tienda y se ajustan montajes de rutas en Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant Auth as AuthenticateMiddleware
participant Controller
participant Service
participant DB as Prisma
Client->>Router: POST /:id_store/addresses (body)
Router->>Auth: validar JWT
Auth-->>Router: req.user con id_user
Router->>Controller: createStoreAddress(req)
Controller->>Service: createStoreAddressService(req.user.id_user, id_store, payload)
Service->>DB: verificar tienda y propietario, validar payload, crear dirección (tx)
DB-->>Service: dirección creada
Service-->>Controller: address data
Controller-->>Client: 201 { success: true, data }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutos Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/modules/commerce/addresses/services/addresses.services.js (1)
19-118: Extraer validaciones compartidas para evitar drift entre módulos de direcciones.
parsePositiveInteger,validateRequiredStringField,validateOptionalStringFieldybuildAddressDataestán duplicados respecto asrc/modules/users/addresses/services/addresses.services.js(snippets: líneas 21-162). Conviene moverlos a un módulo común de validación para mantener reglas consistentes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/commerce/addresses/services/addresses.services.js` around lines 19 - 118, The three validators (parsePositiveInteger, validateRequiredStringField, validateOptionalStringField) and buildAddressData are duplicated; extract them into a shared validation module (e.g., export these functions from a new/common file) and replace the copies in this file and src/modules/users/addresses/services/addresses.services.js with imports from that module; ensure you export parsePositiveInteger, validateRequiredStringField, validateOptionalStringField and buildAddressData from the new module and update any call sites in create/update flows to import the same symbols so both address service implementations use the single shared logic.src/modules/commerce/products/product.controller.js (1)
44-46: Unificar el formato de error en respuestas del controlador.En el
catchdeupdateProductfaltasuccess: false, mientras en 401 sí se devuelve. Mantener el mismo envelope simplifica consumo del frontend.♻️ Propuesta de ajuste
return res.status(error.status || error.statusCode || 500).json({ + success: false, message: error.message || "Error interno del servidor" });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/commerce/products/product.controller.js` around lines 44 - 46, En el catch de la función updateProduct en product.controller.js la respuesta de error no incluye el campo success: false (mientras que la ruta 401 lo hace), por lo que debes unificar el envelope: modificar el return que usa res.status(error.status || error.statusCode || 500).json(...) para que devuelva { success: false, message: error.message || "Error interno del servidor" } (manteniendo las mismas claves que las otras respuestas de error) asegurándote de usar el mismo formato que en la rama 401.
🤖 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/index.js`:
- Line 38: La ruta registrada cambió de contrato a "/api/product-tags" y puede
romper clientes que usan la ruta previa (p.ej. "/products/tags"); en el archivo
donde se llama a app.use con productTagRoutes, restaura compatibilidad temporal
añadiendo también el antiguo prefijo (por ejemplo registrando productTagRoutes
en "/products/tags") o añade un redirect/alias que reenvíe requests de
"/products/tags" hacia "/api/product-tags" para mantener ambas rutas funcionando
mientras se comunica la migración.
In `@src/modules/commerce/products/product.service.js`:
- Around line 264-285: buildCreateProductData currently accepts
visibility/status from the payload (via parseVisibilityOverride) allowing
clients to override PRODUCT_INITIAL_STATUS; remove or ignore payload-provided
visibility/status in buildCreateProductData and ensure createProductService
always initializes product status/visibility to the configured
PRODUCT_INITIAL_STATUS (or maps visibilityOverride only when caller explicitly
permits admin-level overrides). Concretely, stop calling
parseVisibilityOverride(payload) inside buildCreateProductData (or discard its
result) and have createProductService set status/visible from
PRODUCT_INITIAL_STATUS (and only apply any parsed visibilityOverride behind an
explicit admin check).
- Around line 504-506: Validate and sanitize page and limit before computing
skip: ensure filters.page and filters.limit are finite integers (>0) (use
Number, Number.isFinite and Math.floor or similar), default to 1 and 20
respectively if invalid, and cap limit to a reasonable max (e.g., MAX_LIMIT =
100) to avoid huge queries; then compute skip = (page - 1) * limit using the
sanitized/capped values (update the variables page, limit and skip in
product.service.js where they are defined).
- Around line 562-565: The public getProductByIdService currently returns any
product regardless of visibility; update it so it only exposes visible products
like getProductsSearchService does by either (A) passing a visibility filter
into getProductResponseByIdService (add an options param if needed) or (B) after
awaiting getProductResponseByIdService(productId) check product.visible === true
and if not return null or throw a 404/NotFound so callers (e.g.,
product.controller.js invoking getProductByIdService) cannot read pending/hidden
products; ensure you reference getProductByIdService and
getProductResponseByIdService when making the change.
In `@src/modules/users/addresses/services/addresses.services.js`:
- Around line 201-224: The current count-then-create in prisma.$transaction
(tx.addresses.count then tx.addresses.create) is racey; serialize concurrent
requests by taking a DB lock on the user's row inside the same transaction
before counting. Inside prisma.$transaction, issue a SELECT ... FOR UPDATE on
the users row (via tx.$executeRaw or tx.$executeRawUnsafe) for user.id_user to
acquire a row-level lock, then run tx.addresses.count and conditionally
tx.addresses.create using MAX_USER_ADDRESSES; this ensures the check+insert is
atomic. Alternatively, if you prefer a DB-enforced guarantee, add a
database-level constraint/trigger that prevents more than MAX_USER_ADDRESSES
active addresses per user and handle the constraint error in the create flow.
---
Nitpick comments:
In `@src/modules/commerce/addresses/services/addresses.services.js`:
- Around line 19-118: The three validators (parsePositiveInteger,
validateRequiredStringField, validateOptionalStringField) and buildAddressData
are duplicated; extract them into a shared validation module (e.g., export these
functions from a new/common file) and replace the copies in this file and
src/modules/users/addresses/services/addresses.services.js with imports from
that module; ensure you export parsePositiveInteger,
validateRequiredStringField, validateOptionalStringField and buildAddressData
from the new module and update any call sites in create/update flows to import
the same symbols so both address service implementations use the single shared
logic.
In `@src/modules/commerce/products/product.controller.js`:
- Around line 44-46: En el catch de la función updateProduct en
product.controller.js la respuesta de error no incluye el campo success: false
(mientras que la ruta 401 lo hace), por lo que debes unificar el envelope:
modificar el return que usa res.status(error.status || error.statusCode ||
500).json(...) para que devuelva { success: false, message: error.message ||
"Error interno del servidor" } (manteniendo las mismas claves que las otras
respuestas de error) asegurándote de usar el mismo formato que en la rama 401.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7e4f290-dc91-4f84-829a-fd1aaf19baf6
📒 Files selected for processing (11)
src/index.jssrc/modules/commerce/addresses/controllers/addresses.controllers.jssrc/modules/commerce/addresses/routes/addresses.routes.jssrc/modules/commerce/addresses/services/addresses.services.jssrc/modules/commerce/commerces/store.service.jssrc/modules/commerce/products/product.controller.jssrc/modules/commerce/products/product.routes.jssrc/modules/commerce/products/product.service.jssrc/modules/users/addresses/controllers/addresses.controllers.jssrc/modules/users/addresses/routes/addresses.routes.jssrc/modules/users/addresses/services/addresses.services.js
| //Se encuentra indexado | ||
| app.use("/api/categories", categoriesRoutes); | ||
| app.use("/products/tags", productTagRoutes); | ||
| app.use("/api/product-tags", productTagRoutes); |
Contributor
There was a problem hiding this comment.
Posible cambio breaking de contrato en la ruta de product-tags.
En Line 38 el prefijo quedó en /api/product-tags. Si clientes ya consumen la ruta anterior (p. ej. /products/tags), esto rompe integración sin compatibilidad temporal ni aviso de migración.
💡 Sugerencia para compatibilidad backward temporal
+app.use("/products/tags", productTagRoutes); // compatibilidad transitoria
app.use("/api/product-tags", productTagRoutes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.js` at line 38, La ruta registrada cambió de contrato a
"/api/product-tags" y puede romper clientes que usan la ruta previa (p.ej.
"/products/tags"); en el archivo donde se llama a app.use con productTagRoutes,
restaura compatibilidad temporal añadiendo también el antiguo prefijo (por
ejemplo registrando productTagRoutes en "/products/tags") o añade un
redirect/alias que reenvíe requests de "/products/tags" hacia
"/api/product-tags" para mantener ambas rutas funcionando mientras se comunica
la migración.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/modules/commerce/products/product.service.js (2)
365-369: Optimización menor: usar Set para búsqueda de tags.
nextTagIds.includes()es O(n) por cada relación existente. Con muchos tags, esto se vuelve O(n×m). Considerando que existeMAX_TAGS_PER_PRODUCT, el impacto actual es mínimo, pero usar un Set sería más eficiente.♻️ Propuesta de mejora
const syncProductTagsService = async (tx, productId, nextTagIds) => { + const nextTagIdSet = new Set(nextTagIds); + const existingRelations = await tx.productTagRelations.findMany({ // ... }); const activeRelationIdsToDisable = existingRelations .filter( - (relation) => relation.status && !nextTagIds.includes(relation.fk_product_tag) + (relation) => relation.status && !nextTagIdSet.has(relation.fk_product_tag) ) .map((relation) => relation.id_product_tag_relation);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/commerce/products/product.service.js` around lines 365 - 369, The filter currently calls nextTagIds.includes(...) inside the existingRelations loop, making lookups O(n) per relation; create a Set from nextTagIds first (e.g., const nextTagIdSet = new Set(nextTagIds)) and then change the check in the activeRelationIdsToDisable computation to use nextTagIdSet.has(relation.fk_product_tag) so lookups become O(1); update references in the activeRelationIdsToDisable expression (and any other places that iterate using includes) to use the Set.
55-63: Considerar extraerparsePositiveIntegera un módulo utilitario compartido.Esta función está duplicada en
src/modules/commerce/commerces/store.service.js. Si bien mantener módulos independientes tiene mérito, extraerla a un helper compartido (ej:src/utils/validation.js) reduciría duplicación y facilitaría el mantenimiento.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/commerce/products/product.service.js` around lines 55 - 63, Extrae la función parsePositiveInteger a un helper compartido (por ejemplo crear src/utils/validation.js) y reemplaza las implementaciones duplicadas en product.service.js y store.service.js por importaciones de ese helper; específicamente, mover la lógica de parsePositiveInteger (validación de Number.isInteger y >0 y el throw con status/message) a una función exportada (p. ej. export const parsePositiveInteger) y actualizar ambos archivos para importar y usar esa función en lugar de las copias locales.src/modules/users/users/controllers/users.controllers.js (1)
113-118: Unificá estecatchcon el resto del controller.Acá sólo mirás
error.statusy devolvésmessagesin fallback, así que este endpoint queda con un contrato de error distinto aregisterUser,updateUseryupdateUserPassword.♻️ Ajuste sugerido
- return res.status(error.status || 500).json({ - success:false, - message:error.message - }); + const statusCode = error.statusCode || error.status || 500; + return res.status(statusCode).json({ + success: false, + message: error.message || "Error interno del servidor", + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/users/users/controllers/users.controllers.js` around lines 113 - 118, El catch actual sólo devuelve error.status y error.message sin fallback y rompe el contrato con los otros endpoints; cambia ese bloque de manejo de errores para que use el mismo patrón que registerUser, updateUser y updateUserPassword: calcular status = error.status || 500, message = error.message || 'Internal server error' (o el fallback textual que usan los otros controllers) y devolver el mismo JSON shape (por ejemplo { success: false, message, ... } con los mismos campos opcionales/nulos que usan los otros handlers) para mantener consistencia con las funciones registerUser, updateUser y updateUserPassword.
🤖 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/users/addresses/services/addresses.services.js`:
- Around line 295-307: Replace the separate ownership/status check + update with
an atomic conditional write: use prisma.addresses.updateMany({ where: {
id_address: existingAddress.id_address, id_user_owner: user.id_user, status:
true }, data: dataToUpdate }) and then verify the returned count is 1; if count
is 0 throw the same not-found/forbidden error you would from
getOwnedPersonalAddressOrThrow. After a successful updateMany, re-read the
record with prisma.addresses.findUnique({ where: { id_address:
existingAddress.id_address }, select: ADDRESS_SELECT }) to return the
updatedAddress. This ensures ownership and status are validated inside the write
and prevents races.
In `@src/modules/users/users/services/users.services.js`:
- Around line 367-379: Hay un TOCTOU: tras la verificación en
getAuthorizedUserService() vuelves a leer el usuario con
prisma.users.findUnique(...) sin revalidar el status, por lo que si el usuario
cambió a inactivo/ se eliminó entre ambas consultas puedes devolver un perfil
inválido; arreglalo incluyendo la condición de estado en la segunda lectura (por
ejemplo en la cláusula where de prisma.users.findUnique usar id_user:
userProfile.id_user y status: true) y, si la consulta devuelve null, lanzar o
devolver un 404 como lo hace getAuthorizedUserService(); referencia:
getAuthorizedUserService, prisma.users.findUnique y USER_PROFILE_SELECT.
---
Nitpick comments:
In `@src/modules/commerce/products/product.service.js`:
- Around line 365-369: The filter currently calls nextTagIds.includes(...)
inside the existingRelations loop, making lookups O(n) per relation; create a
Set from nextTagIds first (e.g., const nextTagIdSet = new Set(nextTagIds)) and
then change the check in the activeRelationIdsToDisable computation to use
nextTagIdSet.has(relation.fk_product_tag) so lookups become O(1); update
references in the activeRelationIdsToDisable expression (and any other places
that iterate using includes) to use the Set.
- Around line 55-63: Extrae la función parsePositiveInteger a un helper
compartido (por ejemplo crear src/utils/validation.js) y reemplaza las
implementaciones duplicadas en product.service.js y store.service.js por
importaciones de ese helper; específicamente, mover la lógica de
parsePositiveInteger (validación de Number.isInteger y >0 y el throw con
status/message) a una función exportada (p. ej. export const
parsePositiveInteger) y actualizar ambos archivos para importar y usar esa
función en lugar de las copias locales.
In `@src/modules/users/users/controllers/users.controllers.js`:
- Around line 113-118: El catch actual sólo devuelve error.status y
error.message sin fallback y rompe el contrato con los otros endpoints; cambia
ese bloque de manejo de errores para que use el mismo patrón que registerUser,
updateUser y updateUserPassword: calcular status = error.status || 500, message
= error.message || 'Internal server error' (o el fallback textual que usan los
otros controllers) y devolver el mismo JSON shape (por ejemplo { success: false,
message, ... } con los mismos campos opcionales/nulos que usan los otros
handlers) para mantener consistencia con las funciones registerUser, updateUser
y updateUserPassword.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79ad54cd-aef7-4750-bf58-8f724a55c2a7
📒 Files selected for processing (5)
src/modules/commerce/products/product.service.jssrc/modules/users/addresses/services/addresses.services.jssrc/modules/users/users/controllers/users.controllers.jssrc/modules/users/users/routes/users.routes.jssrc/modules/users/users/services/users.services.js
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/modules/users/addresses/services/addresses.services.js (1)
342-359:⚠️ Potential issue | 🟠 MajorHacé el borrado lógico como write condicional.
Acá quedó el mismo TOCTOU que ya habían corregido en update: primero validás ownership/estado y después hacés
update()sólo porid_address. Si otra request borra la dirección entre ambas operaciones, este endpoint igual puede responder200sobre un recurso que ya no estaba activo.💡 Ajuste propuesto
export const deleteAddressService = async ( authenticatedUserId, requestedUserId, requestedAddressId ) => { const user = await getAuthorizedUserService( authenticatedUserId, requestedUserId ); - const existingAddress = await getOwnedPersonalAddressOrThrow( - user.id_user, - requestedAddressId - ); - - const deletedAddress = await prisma.addresses.update({ - where: { - id_address: existingAddress.id_address, - }, - data: { - status: false, - }, - select: { - id_address: true, - status: true, - updated_at: true, - }, - }); + const addressId = parsePositiveInteger(requestedAddressId, "ID de direccion"); + + const deletedAddresses = await prisma.addresses.updateMany({ + where: { + id_address: addressId, + fk_user: user.id_user, + fk_store: null, + status: true, + }, + data: { + status: false, + }, + }); + + if (deletedAddresses.count !== 1) { + throw { + status: 404, + message: "Direccion no encontrada", + }; + } + + const deletedAddress = await prisma.addresses.findUnique({ + where: { + id_address: addressId, + }, + select: { + id_address: true, + status: true, + updated_at: true, + }, + }); + + if (!deletedAddress) { + throw { + status: 500, + message: "No se pudo recuperar la direccion eliminada", + }; + } return deletedAddress; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/users/addresses/services/addresses.services.js` around lines 342 - 359, Eliminá el TOCTOU: en vez de validar ownership con getOwnedPersonalAddressOrThrow y luego llamar prisma.addresses.update sólo por id_address, hacé la escritura condicional en un único paso (por ejemplo usando prisma.addresses.updateMany o un update con where que incluya tanto id_address como status: true y/o id_user) y luego verificá el número de filas afectadas; si no se actualizó ninguna fila, retorná/lanza el error de recurso no encontrado/ya inactivo. Esto altera el flujo en la sección que actualmente usa getOwnedPersonalAddressOrThrow seguido de prisma.addresses.update para que la actualización sea atómica y detecte si otro request ya inactivó la dirección.
🧹 Nitpick comments (1)
src/modules/users/users/services/users.services.js (1)
101-141: Centralizá este helper de autorización.La misma validación ya existe en
src/modules/users/addresses/services/addresses.services.js. Como es lógica de acceso, mantener dos copias facilita drift en mensajes y checks; conviene extraerla a un módulo compartido y reutilizarla desde ambos servicios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/users/users/services/users.services.js` around lines 101 - 141, Extract the duplicated authorization logic from getAuthorizedUserService into a shared helper (e.g., ensureAuthorizedUser or authorizeUser) that accepts (authenticatedUserId, requestedUserId), performs parsePositiveInteger checks, compares IDs, queries prisma.users.findUnique for id_user and status, and throws the same typed errors; export that helper and replace the body of getAuthorizedUserService and the equivalent function in the addresses service to call the shared helper instead of duplicating logic, ensuring error messages and thrown shapes remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/modules/users/addresses/services/addresses.services.js`:
- Around line 342-359: Eliminá el TOCTOU: en vez de validar ownership con
getOwnedPersonalAddressOrThrow y luego llamar prisma.addresses.update sólo por
id_address, hacé la escritura condicional en un único paso (por ejemplo usando
prisma.addresses.updateMany o un update con where que incluya tanto id_address
como status: true y/o id_user) y luego verificá el número de filas afectadas; si
no se actualizó ninguna fila, retorná/lanza el error de recurso no encontrado/ya
inactivo. Esto altera el flujo en la sección que actualmente usa
getOwnedPersonalAddressOrThrow seguido de prisma.addresses.update para que la
actualización sea atómica y detecte si otro request ya inactivó la dirección.
---
Nitpick comments:
In `@src/modules/users/users/services/users.services.js`:
- Around line 101-141: Extract the duplicated authorization logic from
getAuthorizedUserService into a shared helper (e.g., ensureAuthorizedUser or
authorizeUser) that accepts (authenticatedUserId, requestedUserId), performs
parsePositiveInteger checks, compares IDs, queries prisma.users.findUnique for
id_user and status, and throws the same typed errors; export that helper and
replace the body of getAuthorizedUserService and the equivalent function in the
addresses service to call the shared helper instead of duplicating logic,
ensuring error messages and thrown shapes remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 361bbe48-242c-4ff1-9983-1b5d9312577f
📒 Files selected for processing (2)
src/modules/users/addresses/services/addresses.services.jssrc/modules/users/users/services/users.services.js
Andoumeda
previously requested changes
Mar 14, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/modules/commerce/products/product.service.js (1)
55-63: Considerar extraerparsePositiveIntegera un módulo compartido.Esta función está duplicada en múltiples archivos (
store.service.js,users.services.js). Extraerla a un módulo de utilidades compartidas (src/utils/validators.jso similar) reduciría la duplicación y facilitaría el mantenimiento.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/commerce/products/product.service.js` around lines 55 - 63, Extract the duplicated function parsePositiveInteger into a shared validators module (e.g., create a validators.js exporting parsePositiveInteger) and replace the local implementations in product.service.js, store.service.js and users.services.js with imports from that module; ensure the new exported function preserves the same signature and error shape ({ status: 400, message: `${fieldName} invalido` }) and update all call sites to import { parsePositiveInteger } from the new validators module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/modules/commerce/products/product.service.js`:
- Around line 55-63: Extract the duplicated function parsePositiveInteger into a
shared validators module (e.g., create a validators.js exporting
parsePositiveInteger) and replace the local implementations in
product.service.js, store.service.js and users.services.js with imports from
that module; ensure the new exported function preserves the same signature and
error shape ({ status: 400, message: `${fieldName} invalido` }) and update all
call sites to import { parsePositiveInteger } from the new validators module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c825acb9-0496-4090-a2aa-c5863d589c97
📒 Files selected for processing (2)
src/modules/commerce/commerces/store.service.jssrc/modules/commerce/products/product.service.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/modules/commerce/products/product.service.js (1)
75-90: Considerar validación de tipo antes detoString().Si
valuees un objeto (ej.{foo: "bar"}),toString()retorna"[object Object]"que pasaría la validación. Aunque es un edge case poco probable, podría agregar una verificación de tipo.♻️ Propuesta de ajuste
const validateRequiredStringField = (value, fieldName, maxLength = null) => { + if (value !== null && value !== undefined && typeof value !== "string" && typeof value !== "number") { + throw { status: 400, message: `${fieldName} debe ser texto` }; + } + const normalizedValue = value?.toString().trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/commerce/products/product.service.js` around lines 75 - 90, validateRequiredStringField currently calls value?.toString() which allows objects (e.g. {foo:"bar"}) to become "[object Object]" and pass validation; update the function to first check the value's type and reject non-primitive/stringable types: ensure value is a string or number (or a String object) before calling toString(), otherwise throw the same 400 error for invalid type; then proceed to normalize (toString().trim()), apply the maxLength check and return the normalized value. Reference: validateRequiredStringField.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/commerce/products/product.service.js`:
- Around line 551-558: El campo name del modelo Products debe marcarse con
`@db.FullText` porque la ordenación por _relevance (usada donde se construye
orderBy con fields: ["name"]) requiere campos FullText; actualiza la definición
del campo name en el esquema Prisma (modelo Products) para incluir `@db.FullText`
junto a el tipo y longitud actuales, luego genera y aplica una migración de
Prisma (p. ej. prisma migrate dev / prisma migrate deploy según el entorno) para
que el cambio se refleje en la base de datos.
---
Nitpick comments:
In `@src/modules/commerce/products/product.service.js`:
- Around line 75-90: validateRequiredStringField currently calls
value?.toString() which allows objects (e.g. {foo:"bar"}) to become "[object
Object]" and pass validation; update the function to first check the value's
type and reject non-primitive/stringable types: ensure value is a string or
number (or a String object) before calling toString(), otherwise throw the same
400 error for invalid type; then proceed to normalize (toString().trim()), apply
the maxLength check and return the normalized value. Reference:
validateRequiredStringField.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49311e6e-90ff-4dd9-99d1-f087d352f016
📒 Files selected for processing (1)
src/modules/commerce/products/product.service.js
Comment on lines
551
to
558
| orderBy = { | ||
| _relevance: { | ||
| fields: ['name'], | ||
| fields: ["name"], | ||
| search: search, | ||
| sort: 'desc', | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| const [totalProducts, products] = await Promise.all( | ||
| [prisma.products.count({where}), // se calcula el total de productos que cumplen el filtro | ||
| prisma.products.findMany({ // se trae los productos segun el filtro | ||
| where, | ||
| skip, | ||
| take: limit, | ||
| orderBy, | ||
| select: { | ||
| id_product: true, | ||
| name: true, | ||
| description: true, | ||
| price: true, | ||
| store: { | ||
| select: { | ||
| id_store: true, | ||
| name: true, | ||
| } | ||
| sort: "desc" | ||
| } | ||
| } | ||
| })]) | ||
| return { | ||
| products, | ||
| pagination: { | ||
| totalProducts, | ||
| page, | ||
| limit, | ||
| totalPages: Math.ceil(totalProducts/limit) | ||
| }}; | ||
| }; | ||
| export const getProductByIdService = async (id)=>{ | ||
|
|
||
| const productId = Number(id); | ||
|
|
||
| if(!Number.isInteger(productId) || productId <= 0){ | ||
|
|
||
| throw { | ||
| status:400, | ||
| message:"ID de producto inválido" | ||
| }; | ||
|
|
||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verificar configuración de Prisma para full-text search
cat prisma/schema.prisma | grep -A5 "generator\|datasource\|fullTextSearch\|fullTextIndex"Repository: CrisNAC/BackendMarketplace
Length of output: 435
🏁 Script executed:
sed -n '540,570p' src/modules/commerce/products/product.service.jsRepository: CrisNAC/BackendMarketplace
Length of output: 891
🏁 Script executed:
rg -A 10 -B 10 "model Product" prisma/schema.prismaRepository: CrisNAC/BackendMarketplace
Length of output: 4287
🏁 Script executed:
rg "@db\.FullText" prisma/Repository: CrisNAC/BackendMarketplace
Length of output: 52
🏁 Script executed:
find prisma/migrations -type f -name "*.sql" | head -20 | xargs grep -l "tsvector\|fulltext\|GIN" 2>/dev/null || echo "No migration files with full-text found"Repository: CrisNAC/BackendMarketplace
Length of output: 109
El campo name debe estar marcado con @db.FullText en el esquema Prisma.
El ordenamiento por _relevance requiere que los campos buscados tengan el atributo @db.FullText. Aunque fullTextSearchPostgres está habilitado en la configuración de Prisma y PostgreSQL está configurado como proveedor, el campo name en el modelo Products no tiene este atributo. Sin él, las consultas con _relevance fallarán en runtime. Es necesario:
- Agregar
@db.FullTextal campo name:name String@db.VarChar(100)@db.FullText`` - Crear una migración para aplicar el cambio a la base de datos
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/commerce/products/product.service.js` around lines 551 - 558, El
campo name del modelo Products debe marcarse con `@db.FullText` porque la
ordenación por _relevance (usada donde se construye orderBy con fields:
["name"]) requiere campos FullText; actualiza la definición del campo name en el
esquema Prisma (modelo Products) para incluir `@db.FullText` junto a el tipo y
longitud actuales, luego genera y aplica una migración de Prisma (p. ej. prisma
migrate dev / prisma migrate deploy según el entorno) para que el cambio se
refleje en la base de datos.
Andoumeda
dismissed
their stale review
March 16, 2026 14:37
Ya se solucionaron y mergearon los conflictos anteriores
Andoumeda
approved these changes
Mar 16, 2026
This was referenced Mar 16, 2026
Closed
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.
-Address para commerce y customer
-Put de Productos
-actualizado logica de crear productos
Summary by CodeRabbit
Nuevas funcionalidades
Seguridad
Chores