Repository navigation
Implement logical deletion of category by ID - #102
Merged
Merged
Conversation
Contributor
📝 WalkthroughDescripción GeneralSe añade un nuevo endpoint DELETE para eliminar categorías de productos en el módulo de administración. La funcionalidad realiza un soft-delete estableciendo el estado a falso y reasigna los productos relacionados a la categoría predeterminada, con validaciones para evitar la eliminación de categorías protegidas. Cambios
Esfuerzo Estimado de Revisión🎯 3 (Moderado) | ⏱️ ~25 minutos PRs Posiblemente Relacionados
Revisores Sugeridos
Poema
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/admin/admin-category.test.js (1)
7-11:⚠️ Potential issue | 🟡 MinorConsolidar los dos
vi.mockdel módulo Prisma en una única definición.El módulo
../../../src/lib/prisma.jsestá siendo mockeado en dos ocasiones (líneas 7-11 y 38-44). Solo la última definición será efectiva, lo que genera ambigüedad sobre qué métodos están realmente mockeados y complica el mantenimiento de los tests.Ubicaciones afectadas
- Líneas 7-11: Primer mock (solo
findUnique)- Líneas 38-44: Segundo mock (
findUniqueyupdate)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/admin/admin-category.test.js` around lines 7 - 11, Hay dos mocks separados de the Prisma module causing only the latter to take effect; consolidate into a single vi.mock for "../../../src/lib/prisma.js" that exports prisma.productCategories with both mocked methods used in tests (productCategories.findUnique and productCategories.update) so both are defined as vi.fn() and can be configured per-test; update any test setups that call vi.mock individually to use this single mock and adjust expect/restore usage accordingly.
🧹 Nitpick comments (3)
src/modules/admin/categories/admin-category.service.js (2)
40-49: Evitá el “magic number”1y centralizá la categoría fallback.El ID de fallback está hardcodeado en varios puntos; conviene centralizarlo para evitar desalineaciones futuras.
♻️ Refactor sugerido
+const DEFAULT_CATEGORY_ID = 1; export const deleteAdminProductCategoryService = async (id) => { - if (id === 1) { + if (id === DEFAULT_CATEGORY_ID) { throw new ValidationError("No se puede eliminar la categoría Sin categoría"); } await prisma.$transaction([ prisma.products.updateMany({ where: { fk_product_category: id }, - data: { fk_product_category: 1 } + data: { fk_product_category: DEFAULT_CATEGORY_ID } }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/admin/categories/admin-category.service.js` around lines 40 - 49, Se está usando el valor literal 1 como categoría fallback en prisma.products.updateMany (campo fk_product_category) lo cual es un "magic number"; reemplazá ese hardcode por una referencia centralizada (por ejemplo una constante DEFAULT_FALLBACK_CATEGORY_ID en un archivo de configuración o helper) o, mejor, resolvé dinámicamente la categoría fallback (p. ej. buscar la categoría por slug/nombre único) y usá ese id en prisma.products.updateMany y en cualquier otro lugar que dependa del fallback; actualizá las referencias a prisma.products.updateMany and prisma.productCategories.update para consumir esa única fuente de verdad en lugar del 1 literal.
2-2: UsáConflictErrorpara la categoría protegida (409), noValidationError(400).Acá no falla el formato del input; falla una regla de negocio de estado/conflicto. Mantener 400 confunde a clientes que diferencian validación vs conflicto operativo.
💡 Propuesta de ajuste
-import { NotFoundError, ValidationError } from "../../../lib/errors.js"; +import { NotFoundError, ConflictError } from "../../../lib/errors.js"; export const deleteAdminProductCategoryService = async (id) => { if (id === 1) { - throw new ValidationError("No se puede eliminar la categoría Sin categoría"); + throw new ConflictError("No se puede eliminar la categoría Sin categoría"); }Also applies to: 30-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/admin/categories/admin-category.service.js` at line 2, La importación y los lanzamientos usan ValidationError para el caso de categoría protegida pero deben usar ConflictError (HTTP 409) porque es una regla de negocio, no un fallo de formato; en admin-category.service.js reemplazá ValidationError por ConflictError en la lista de imports (junto a NotFoundError) y cambiá las instancias donde se arroja ValidationError para la categoría protegida por ConflictError (revisá también las otras ocurrencias en el archivo que actualmente usan ValidationError, p. ej. las ubicadas cerca de las comprobaciones de categoría protegida alrededor de las referencias a estas funciones/métodos).tests/unit/admin/admin-category.test.js (1)
214-224: El test de reasignación debería validar también los argumentos de update, no sólo$transaction.Con la aserción actual puede pasar aunque cambie incorrectamente el payload de reasignación/soft-delete.
✅ Aserciones adicionales sugeridas
it("reasigna los productos a la categoría 1 al eliminar", async () => { prisma.productCategories.findUnique.mockResolvedValue(mockCategory); prisma.$transaction.mockImplementation(async (ops) => ops); await asRole( request(app).delete("/api/admin/categories/2"), "admin" ); expect(prisma.$transaction).toHaveBeenCalled(); + expect(prisma.products.updateMany).toHaveBeenCalledWith({ + where: { fk_product_category: 2 }, + data: { fk_product_category: 1 } + }); + expect(prisma.productCategories.update).toHaveBeenCalledWith({ + where: { id_product_category: 2 }, + data: { status: false } + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/admin/admin-category.test.js` around lines 214 - 224, The test "reasigna los productos a la categoría 1 al eliminar" currently only asserts prisma.$transaction was invoked; change the prisma.$transaction.mockImplementation to capture the ops array (e.g., const capturedOps = ops) and assert the transaction contains the expected update operations: one operation that updates products (prisma.product.updateMany or equivalent) with where matching categoryId: 2 and data that sets categoryId: 1, and the other operation that soft-deletes the category (prisma.productCategories.update or equivalent) with where matching id: 2 and data setting the deleted/active flag; verify the exact argument objects passed to those operations rather than just that $transaction was called.
🤖 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/admin/categories/admin-category.routes.js`:
- Around line 44-65: Update the Swagger block for the DELETE
/api/admin/categories/{id} operation to include 401 and 403 responses and
clarify that this is a logical deletion (soft-delete/deactivate) rather than a
hard delete; specifically, in the JSDoc swagger for the DELETE route in
admin-category.routes.js, add responses for 401 (Unauthorized - missing/invalid
credentials) and 403 (Forbidden - requires admin role) and change the
204/description and possibly other descriptions to indicate the category is
"desactivada" or "eliminación lógica" to avoid implying a permanent hard delete.
---
Outside diff comments:
In `@tests/unit/admin/admin-category.test.js`:
- Around line 7-11: Hay dos mocks separados de the Prisma module causing only
the latter to take effect; consolidate into a single vi.mock for
"../../../src/lib/prisma.js" that exports prisma.productCategories with both
mocked methods used in tests (productCategories.findUnique and
productCategories.update) so both are defined as vi.fn() and can be configured
per-test; update any test setups that call vi.mock individually to use this
single mock and adjust expect/restore usage accordingly.
---
Nitpick comments:
In `@src/modules/admin/categories/admin-category.service.js`:
- Around line 40-49: Se está usando el valor literal 1 como categoría fallback
en prisma.products.updateMany (campo fk_product_category) lo cual es un "magic
number"; reemplazá ese hardcode por una referencia centralizada (por ejemplo una
constante DEFAULT_FALLBACK_CATEGORY_ID en un archivo de configuración o helper)
o, mejor, resolvé dinámicamente la categoría fallback (p. ej. buscar la
categoría por slug/nombre único) y usá ese id en prisma.products.updateMany y en
cualquier otro lugar que dependa del fallback; actualizá las referencias a
prisma.products.updateMany and prisma.productCategories.update para consumir esa
única fuente de verdad en lugar del 1 literal.
- Line 2: La importación y los lanzamientos usan ValidationError para el caso de
categoría protegida pero deben usar ConflictError (HTTP 409) porque es una regla
de negocio, no un fallo de formato; en admin-category.service.js reemplazá
ValidationError por ConflictError en la lista de imports (junto a NotFoundError)
y cambiá las instancias donde se arroja ValidationError para la categoría
protegida por ConflictError (revisá también las otras ocurrencias en el archivo
que actualmente usan ValidationError, p. ej. las ubicadas cerca de las
comprobaciones de categoría protegida alrededor de las referencias a estas
funciones/métodos).
In `@tests/unit/admin/admin-category.test.js`:
- Around line 214-224: The test "reasigna los productos a la categoría 1 al
eliminar" currently only asserts prisma.$transaction was invoked; change the
prisma.$transaction.mockImplementation to capture the ops array (e.g., const
capturedOps = ops) and assert the transaction contains the expected update
operations: one operation that updates products (prisma.product.updateMany or
equivalent) with where matching categoryId: 2 and data that sets categoryId: 1,
and the other operation that soft-deletes the category
(prisma.productCategories.update or equivalent) with where matching id: 2 and
data setting the deleted/active flag; verify the exact argument objects passed
to those operations rather than just that $transaction was called.
🪄 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: ee5cfc19-50ca-40e1-ab55-fa64f4715a59
📒 Files selected for processing (4)
src/modules/admin/categories/admin-category.controller.jssrc/modules/admin/categories/admin-category.routes.jssrc/modules/admin/categories/admin-category.service.jstests/unit/admin/admin-category.test.js
Comment on lines
+44
to
+65
| /** | ||
| * @swagger | ||
| * /api/admin/categories/{id}: | ||
| * delete: | ||
| * summary: Borrar categoría de productos (Admin) | ||
| * tags: [Admin] | ||
| * security: | ||
| * - cookieAuth: [] | ||
| * parameters: | ||
| * - in: path | ||
| * name: id | ||
| * required: true | ||
| * schema: | ||
| * type: integer | ||
| * responses: | ||
| * 204: | ||
| * description: Categoría eliminada correctamente | ||
| * 400: | ||
| * description: ID inválido o categoría protegida | ||
| * 404: | ||
| * description: Categoría no encontrada | ||
| */ |
Contributor
There was a problem hiding this comment.
La documentación Swagger del DELETE está incompleta para una ruta protegida.
Faltan 401 y 403 en responses, aunque la ruta exige autenticación/rol admin (Line 66). Además, como la operación es lógica, conviene describirla como “desactivar”/“eliminación lógica” para no sugerir hard delete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/admin/categories/admin-category.routes.js` around lines 44 - 65,
Update the Swagger block for the DELETE /api/admin/categories/{id} operation to
include 401 and 403 responses and clarify that this is a logical deletion
(soft-delete/deactivate) rather than a hard delete; specifically, in the JSDoc
swagger for the DELETE route in admin-category.routes.js, add responses for 401
(Unauthorized - missing/invalid credentials) and 403 (Forbidden - requires admin
role) and change the 204/description and possibly other descriptions to indicate
the category is "desactivada" or "eliminación lógica" to avoid implying a
permanent hard delete.
Benjakr04
approved these changes
Apr 15, 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
Nuevas Características