[codex] issue 221: admin de catálogo de insumos - #246
Conversation
|
@abiatarprado is attempting to deploy a commit to the GlobalEmergency Team on Vercel. A member of the Team first needs to authorize it. |
vgpastor
left a comment
There was a problem hiding this comment.
Revisión centrada en API pública/privada (lo que veníamos hablando) + SOLID/DDD/Clean Code. La dirección es buena y la cobertura de tests está bien; dejo puntos por fichero y un par de cosas del gate antes de mergear.
1) Pública vs. privada (el tema central)
- Lo bien hecho: el
GET /categoriespúblico sigue exponiendo sololabel/labelEs/labelEny filtra archivadas;translations[]yarchivedAtquedan solo enCategoryAdminDto. Es la separación que buscábamos (en la línea de #245). - A corregir: el controller admin comparte
@ApiTags('categories')y cuelga de/categories/admin, así que en el/docs(único y público) el CRUD privilegiado queda mezclado con la lectura pública. Propongo tag propio (categories-admin/admin), registrarlo enmain.ts, y valorar si esa superficie debe figurar en los docs públicos. Detalle en el comentario del controller.
2) DDD / hexagonal
- Regla de dominio ("slug núcleo inmutable/no borrable") dispersa entre
categories-admin.controller.tsyupdate-category.tsvíaObject.values(Category).includes(...). Centralizar en el dominio desupplies. - El adaptador Drizzle re-valida invariantes que ya hace el use-case (slug vacío, existe, padre, propio-padre) → misma regla en dos capas, round-trips duplicados y acoplamiento infra→application. Dejar el adaptador como persistencia "tonta".
3) Clean Code
update-category.ts:nextSluges lógica muerta — todo rename se rechaza siempre, así queslugen el update DTO es engañoso y, de paso, bloquea renombrar subcategorías no-núcleo (contradice #221). Decidir: quitarslugdel update o soportar rename de no-núcleo.create-category.ts: relecturas redundantes; que el puertocreateCategorydevuelva la entidad creada.selectCategoryRows/hydrateCategories: el cartesiano con centinela[null]cuesta leerlo; agrupar traducciones en unMap.
4) Gate antes de mergear (AGENTS.md)
⚠️ pnpm gen:apino ejecutado: el diff no incluyepackages/api-client/src/schema.tsniopenapi.json, pese a nuevos endpoints (/categories/admin) y el nuevolabelenCategoryDto. Regenerar y commitear.- La validación corrió jest con
--globalSetup=""(salta migraciones), así que no se probó contra el esquema real (category_translationsya existe en0037,archived_atlo añade este0039). Pasar el gate completo:pnpm --filter api build+testcon global-setup +eslint --max-warnings=0+prettier --check. catalogue:managesolo lo recibeplatform_admin(víaALL_PERMISSIONS);org_adminno. Para un catálogo global parece correcto, pero confirmadlo (criterio "sin permiso → 403" cubierto).
Nada de esto es estructural; con el ajuste de tag/ruta, mover la validación a un único sitio y regenerar el cliente, queda redondo.
Generated by Claude Code
vgpastor
left a comment
There was a problem hiding this comment.
Seguimiento: contrastado contra el main actual del repo destino
Importante: esta PR está basada en un main anterior a #245 (refactor(supplies): catálogo público sin fuga de datos + cache y limpieza DDD), y al contrastarla contra el main real de destino aparecen conflictos y solapamientos que cambian el sentido de parte de mi review anterior. #245 ya implementó buena parte de esto:
| Lo que la PR "añade" | Estado real en main |
|---|---|
locale.ts (resolveLocale/localizedText) |
Ya existe (#245), con otra firma → conflicto |
label en CategoryDto |
Ya existe (#245) |
Localización en categories.controller.ts |
Ya existe (#245) |
runCategoryCommand/toHttpError en el controller |
main ya tiene SuppliesDomainExceptionFilter |
archivedAt/translations en CategoryDefinition |
main lo prohíbe explícitamente (proyección pública) |
Lo más relevante para el tema público/privado: en main, category-definition.ts documenta que CategoryDefinition es la proyección PÚBLICA y no debe crecer con datos de gestión interna (lo interno va a la API interna con su propio modelo). Esta PR mete ahí archivedAt + translations, deshaciendo justo la separación que #245 dejó montada.
Recomendación principal: rebase sobre main actual y reconciliar:
- Modelo separado para la API interna/admin (read-model/DTO propio), dejando
CategoryDefinitioncomo proyección pública mínima. Ideal: separar también el puerto de escritura/archivado del de lectura (ISP) en vez de engordar el únicoCategoryRepository. - Reutilizar
locale.tsySuppliesDomainExceptionFilterexistentes en vez de re-añadirlos / mapear a mano (y alinear códigos HTTP con la convención del contexto: validación → 422). - Decidir un enfoque de i18n sobre el
mainactual (es/en de #245 vs.category_translationsmulti-idioma de esta PR), no dos en paralelo.
Corrección a mi review anterior: el comentario donde sugería "factorizar un mapper público compartido" entre el público y el admin queda invalidado — la dirección correcta es la contraria: mantener separados el modelo público (ya en main) y el interno, no unificarlos. El resto de puntos (regla de slug-núcleo dispersa, validación duplicada en el adaptador, lógica muerta del rename, pnpm gen:api) siguen vigentes, pero deben reevaluarse después del rebase porque varios ficheros cambian de base.
Generated by Claude Code
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…l filtro de dominio global
|
ℹ️ Reimplementado sobre el Generated by Claude Code |
… 404 en admin de categorías (#326) Salda parte de la deuda técnica de #326 (del merge de #246), sin cambios funcionales salvo dos correcciones de código HTTP: - La regla de dominio "no archivar/borrar categoría núcleo" sale del controller (que lanzaba BadRequestException) y vive en el caso de uso UpdateCategory como CategoryProtectedError, mapeado a 409 por el SuppliesDomainExceptionFilter global. El controller admin ya no conoce la regla ni lanza HttpException. - Fix (regresión latente): PATCH/DELETE de una categoría inexistente devolvía 500 porque el filtro capturaba el CategoryNotFoundError de domain/supply-errors y el caso de uso lanza el de application. Ahora el filtro captura ambos → 404. Pendiente en #326 (más transversal, toca create-supply/edit-supply): separar puerto de lectura pública y escritura admin (ISP) y el modelo interno. Refs #326
Cambios
archived_aty soporte de traducciones.Motivo
Validación
pnpm --filter api exec prettier --check ...pnpm --filter api exec eslint ... --max-warnings=0pnpm --filter api exec jest --runInBand --runTestsByPath ... --globalSetup=""Closes #221