From 68bdb6ff726e2ee6300c0e2182478594812c7c8d Mon Sep 17 00:00:00 2001 From: Matthieu Date: Thu, 9 Apr 2026 14:16:33 +0200 Subject: [PATCH] docs : add code review report for PR #1 Co-Authored-By: Claude Opus 4.6 (1M context) --- doc/review-PR1-ERP7-modular-monolith.md | 267 ++++++++++++++++++++++++ 1 file changed, 267 insertions(+) create mode 100644 doc/review-PR1-ERP7-modular-monolith.md diff --git a/doc/review-PR1-ERP7-modular-monolith.md b/doc/review-PR1-ERP7-modular-monolith.md new file mode 100644 index 0000000..8e758c8 --- /dev/null +++ b/doc/review-PR1-ERP7-modular-monolith.md @@ -0,0 +1,267 @@ +# Code Review — PR #1 [ERP-7] Mise en place du modular monolith + +**Branche** : `feature/ERP-7-mise-en-place-du-modular-monolith` → `develop` +**Auteur** : Tristan +**Date de review** : 2026-04-09 +**Scope** : 55 commits, 85 fichiers modifiés, ~30 000 lignes ajoutées + +--- + +## Résumé de la PR + +Cette PR restructure Coltura (CRM/ERP) en **architecture modulaire DDD** (Domain-Driven Design) : + +- **Backend** : introduction de bounded contexts (`Module/Core`, `Module/Commercial`) avec séparation Domain / Application / Infrastructure +- **Shared** : couche partagée (events, value objects, contracts, bus interfaces) +- **Modules activables** : `config/modules.php` comme source de vérité, activation/désactivation sans toucher au code +- **Sidebar dynamique** : `config/sidebar.php` déclare la navigation, le backend filtre selon les modules actifs, le frontend consomme via `/api/sidebar` +- **Frontend** : réorganisation en `app/` (shell), `shared/` (composables, stores, types), `modules/` (Nuxt layers auto-détectés) +- **Infra** : migration Doctrine, Dockerfile prod, deploy.sh, nginx-proxy, maintenance mode + +--- + +## Issues trouvées + +### Issue 1 — CHANGELOG mentionne le mauvais projet + +| | | +|---|---| +| **Sévérité** | Critique | +| **Fichier** | `CHANGELOG.md`, ligne 3 | +| **Confiance** | 100/100 | + +**Constat** : Le header du CHANGELOG dit : + +``` +Liste des évolutions du projet Ferme +``` + +Ce fichier appartient à **Coltura**, pas au projet Ferme. C'est une erreur de copier-coller lors du scaffolding initial. + +**Correction** : Remplacer "Ferme" par "Coltura". + +--- + +### Issue 2 — Sidebar link `/suppliers` pointe vers une page inexistante (404) + +| | | +|---|---| +| **Sévérité** | Majeure | +| **Fichier** | `config/sidebar.php`, ligne 49 | +| **Confiance** | 75/100 (confirmé par 3 agents indépendants) | + +**Constat** : La sidebar déclare un lien vers `/suppliers` pour le module commercial : + +```php +// config/sidebar.php +[ + 'label' => 'sidebar.commercial.suppliers', + 'icon' => 'i-heroicons-truck', + 'to' => '/suppliers', // ← route inexistante + 'module' => 'commercial', +], +``` + +Mais la seule page du module commercial est `frontend/modules/commercial/pages/commercial.vue`, que Nuxt mappe sur la route `/commercial` (pas `/suppliers`). + +**Impact** : Cliquer sur ce lien dans la sidebar donnera un 404. + +**Correction** : Soit renommer la page en `suppliers.vue`, soit changer le `'to'` en `'/commercial'`. + +--- + +### Issue 3 — Port PostgreSQL changé de 5436 à 5437 + +| | | +|---|---| +| **Sévérité** | Majeure | +| **Fichier** | `infra/dev/.env.docker` | +| **Règle violée** | Workspace `CLAUDE.md` : "Coltura — 8083 / 3003 / **5436**" | +| **Confiance** | 75/100 | + +**Constat** : Le fichier `.env.docker` définit `POSTGRES_PORT=5437`, alors que le port documenté pour Coltura est `5436`. + +**Impact** : Tout développeur qui suit les ports documentés (ou qui utilise des scripts basés sur ces ports) ne pourra pas se connecter à la base. + +**Correction** : Revenir à `5436` ou mettre à jour les CLAUDE.md (workspace + projet). + +--- + +### Issue 4 — Port frontend changé de 3003 à 3004 sans mise à jour de la documentation + +| | | +|---|---| +| **Sévérité** | Majeure | +| **Fichiers** | `frontend/nuxt.config.ts` (ligne 40), `docker-compose.yml` (ligne 33) | +| **Règle violée** | Workspace `CLAUDE.md` : "Coltura — 8083 / **3003** / 5436" et `CLAUDE.md` projet : "make dev-nuxt # port 3003" | +| **Confiance** | 75/100 (confirmé par 3 agents indépendants) | + +**Constat** : + +```typescript +// nuxt.config.ts +devServer: { port: 3004 } // était 3003 +``` + +```yaml +# docker-compose.yml +ports: + - "3004:3004" # était 3004:3003 +``` + +Les deux `CLAUDE.md` documentent toujours le port 3003. + +**Correction** : Mettre à jour les deux CLAUDE.md pour refléter le nouveau port, ou revenir à 3003. + +--- + +### Issue 5 — `useSidebar` : state singleton jamais réinitialisé au logout + +| | | +|---|---| +| **Sévérité** | Majeure | +| **Fichier** | `frontend/shared/composables/useSidebar.ts`, lignes 3-5 | +| **Confiance** | 75/100 | + +**Constat** : Les refs `sections`, `disabledRoutes` et `loaded` sont déclarées au niveau module (en dehors de la fonction composable) : + +```typescript +const sections = ref([]) +const disabledRoutes = ref([]) +const loaded = ref(false) +``` + +Ce sont des singletons partagés sur toute la durée de vie de l'app. Après un logout + re-login : +1. `loaded.value` reste `true` +2. `loadSidebar()` n'est jamais rappelé +3. La sidebar affiche les données de la session précédente + +Le middleware `auth.global.ts` ne recharge que si `!loaded.value`, et `logout.vue` ne reset jamais `loaded`. + +**Impact** : Sidebar périmée après re-connexion. Si les modules changent côté serveur, le frontend ne le saura jamais sans un hard refresh. + +**Correction** : Ajouter une fonction `resetSidebar()` appelée au logout, ou conditionner le rechargement autrement (ex: toujours recharger après login). + +--- + +### Issue 6 — `UserOutput` DTO : type mismatch `int` vs `?int` + dead code + +| | | +|---|---| +| **Sévérité** | Moyenne | +| **Fichier** | `src/Module/Core/Application/DTO/UserOutput.php`, lignes 13 et 23 | +| **Confiance** | 75/100 | + +**Constat** : + +```php +// Le constructeur attend un int non-nullable +public function __construct( + public int $id, // ← int, pas ?int + // ... +) + +// Mais User::getId() retourne ?int +public static function fromEntity(User $user): self +{ + return new self( + id: $user->getId(), // ← peut être null + // ... + ); +} +``` + +Avec `declare(strict_types=1)`, passer `null` à un paramètre `int` lève un `TypeError`. + +**De plus** : Ce DTO n'est utilisé nulle part. `MeProvider` retourne directement l'entité `User` via `$this->security->getUser()`. Le DTO est du dead code. + +**Correction** : Soit utiliser le DTO dans `MeProvider` (comme l'architecture le prévoit), soit le supprimer. Dans tous les cas, changer `int $id` en `?int $id`. + +--- + +### Issue 7 — `CreateUserCommand` contourne `UserRepositoryInterface` + +| | | +|---|---| +| **Sévérité** | Moyenne | +| **Fichier** | `src/Module/Core/Infrastructure/Console/CreateUserCommand.php`, lignes 22-27 | +| **Règle violée** | `CLAUDE.md` projet : "les repositories sont des interfaces" | +| **Confiance** | 75/100 (confirmé par 2 agents indépendants) | + +**Constat** : + +```php +public function __construct( + private EntityManagerInterface $em, // ← injection directe de Doctrine + private UserPasswordHasherInterface $hasher, +) {} + +// ... +$this->em->persist($user); +$this->em->flush(); +``` + +Le `UserRepositoryInterface::save(User $user)` existe et est implémenté par `DoctrineUserRepository`. La commande devrait l'utiliser : + +```php +// Correction attendue +public function __construct( + private UserRepositoryInterface $userRepository, + private UserPasswordHasherInterface $hasher, +) {} + +// ... +$this->userRepository->save($user); +``` + +**Impact** : Viole le pattern DDD introduit dans cette même PR et crée un second chemin de persistance non contrôlé. + +--- + +### Issue 8 — `auth.vue` : indentation 2 espaces au lieu de 4 + +| | | +|---|---| +| **Sévérité** | Mineure | +| **Fichier** | `frontend/app/layouts/auth.vue` | +| **Règle violée** | `CLAUDE.md` projet : "4 espaces d'indentation" | +| **Confiance** | 75/100 | + +**Constat** : Le fichier utilise 2 espaces d'indentation : + +```vue + +``` + +Tous les autres fichiers Vue de la PR (`default.vue`, `login.vue`, `index.vue`, `logout.vue`, `commercial.vue`) utilisent correctement 4 espaces. + +**Correction** : Passer en 4 espaces. + +--- + +## Issues mineures non retenues (score < 75) + +Pour information, ces points ont été identifiés mais jugés moins critiques : + +- **`/api/sidebar` et `/api/modules` en PUBLIC_ACCESS** : intentionnel selon le CLAUDE.md qui documente ces endpoints comme publics +- **`doctrine.yaml` ne mappe que le module Core** : les entités de futurs modules ne seront pas détectées automatiquement (à documenter) +- **Middleware `modules.global.ts`** : boucle de redirection infinie possible si `/` est dans `disabledRoutes` (requiert une mauvaise config) +- **Lien sidebar `/admin`** : pointe vers une page inexistante (pré-existant sur develop) +- **Labels hardcodés en français dans `login.vue`** : `"Nom d'utilisateur"`, `"Mot de passe"`, `"Se connecter"` au lieu de `$t('auth.username')` etc. (les clés i18n existent dans `fr.json`) + +--- + +## Verdict + +La PR pose de bonnes bases architecturales (DDD, modules activables, sidebar dynamique). Les issues principales sont : + +- **2 bugs fonctionnels** : sidebar `/suppliers` en 404 et state `useSidebar` jamais reset +- **3 incohérences config/doc** : ports PG et frontend changés sans MAJ CLAUDE.md, CHANGELOG mauvais projet +- **2 incohérences architecturales** : `CreateUserCommand` et `UserOutput` ne respectent pas les patterns introduits dans la PR +- **1 style** : indentation auth.vue + +Aucun de ces problèmes ne bloque le merge mais les 2 bugs fonctionnels (issues 2 et 5) devraient être corrigés avant.