docs : add code review report for PR #1
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Matthieu
267
doc/review-PR1-ERP7-modular-monolith.md
Normal file
267
doc/review-PR1-ERP7-modular-monolith.md
Normal file
@@ -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<SidebarSection[]>([])
|
||||
const disabledRoutes = ref<string[]>([])
|
||||
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
|
||||
<template>
|
||||
<div class="min-h-screen">
|
||||
<slot />
|
||||
</div>
|
||||
</template>
|
||||
```
|
||||
|
||||
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.
|
||||
Reference in New Issue
Block a user