Correctifs post-review M2 fournisseurs (P1 + P2/P3 + alignement M1) #74

Merged
malio merged 11 commits from fix/review-m2-correctifs into develop 2026-06-08 08:47:43 +00:00
14 changed files with 220 additions and 45 deletions
+22 -12
View File
@@ -126,7 +126,7 @@ Toutes les entités métier nouvelles implémentent `TimestampableInterface` + `
Notes (miroir M1) :
- **Compta édite uniquement l'onglet Comptabilité** (`accounting.manage`) d'un fournisseur existant. Compta ne peut pas **créer** un fournisseur (pas de `manage` global).
- **Commerciale** a `view` + `manage` mais **pas** `accounting.view` → l'onglet Comptabilité est masqué (front) et filtré (back, 2 niveaux : `security` API Platform + `SupplierProvider`).
- **Commerciale** a `view` + `manage` mais **pas** `accounting.view` → l'onglet Comptabilité est masqué (front) et filtré (back). Mécanisme réel (le code fait foi) : le groupe de lecture `supplier:read:accounting` n'est **pas** dans le contexte de sérialisation par défaut ; le `SupplierReadGroupContextBuilder` ne l'**ajoute** dynamiquement que si l'utilisateur porte `commercial.suppliers.accounting.view` (gating **par ajout** de groupe, jamais par retrait). Sans la permission, les champs comptables (et les RIB) ne sont donc jamais sérialisés. La colonne SIREN de l'export XLSX suit la même règle (`accounting.view`).
- **Bureau** : `view` + `manage` (tout sauf Comptabilité).
- **Usine** : aucune permission → item sidebar invisible, accès direct 403.
@@ -159,9 +159,11 @@ final class SupplierFieldNormalizer
Le formatage `XX XX XX XX XX` est fait à l'affichage côté front. Le back stocke `0612345678` (chiffres seuls).
### 2.12 Liste : embed catégories + sites + fetch-joins (cohérence M1/ERP-62)
### 2.12 Liste : embed catégories + sites + hydratation anti-N+1 (cohérence M1/ERP-62)
Décision d'alignement (02/06/2026) : la **liste** `GET /api/suppliers` **embarque** les `categories[]` (avec `code`/`name`) et les `sites[]` (avec `name`/`postalCode` — pas de `code`), comme la liste Clients après ERP-62 — et **non** des champs dérivés aplatis. Conséquence performance : le `DoctrineSupplierRepository` **DOIT** poser des **fetch-joins** (`leftJoin`+`addSelect`) sur `categories` et `addresses.sites` dans la requête de liste pour éviter le N+1. Les `sites` de la liste sont agrégés/dédoublonnés via `Supplier::getSites()` (cf. § 3.3). Le contrat de sérialisation (groupes `category:read` / `site:read` dans le contexte) est posé **une seule fois** sur l'entité — source de vérité unique, le front ne le redéfinit pas.
Décision d'alignement (02/06/2026) : la **liste** `GET /api/suppliers` **embarque** les `categories[]` (avec `code`/`name`) et les `sites[]` (avec `name`/`postalCode` — pas de `code`), comme la liste Clients après ERP-62 — et **non** des champs dérivés aplatis.
Conséquence performance — **implémentation réelle (le code fait foi)** : le `DoctrineSupplierRepository` **ne fetch-joine PAS** les to-many dans la requête de sélection (`createListQueryBuilder` ne fait que filtres + tri). L'anti-N+1 passe par `hydrateListCollections()` (puis `hydrateContacts()`) : une fois le jeu de fournisseurs borné (page ou export), des requêtes **`IN` bornées séparées** remplissent `categories`, puis `addresses.sites`, puis `contacts` sur les **mêmes** instances `Supplier` (identity map). Ce découpage évite le **produit cartésien** qu'un fetch-join combiné `categories × addresses.sites` imposerait aux chemins non paginés (export, `?pagination=false`). Les `sites` de la liste sont agrégés/dédoublonnés via `Supplier::getSites()` (cf. § 3.3). Le contrat de sérialisation (groupes `category:read` / `site:read` dans le contexte) est posé **une seule fois** sur l'entité — source de vérité unique, le front ne le redéfinit pas.
> Dépendance confirmée sur le JSON réel (#82 mergé) : `Category` expose `code`/`name` sous `category:read` ; `Site` expose `name`/`postalCode`/`city`/`color` sous `site:read` (**pas de `code`**). L'embed est pleinement matérialisé.
@@ -213,6 +215,8 @@ Namespace : **`DoctrineMigrations` (racine `migrations/`)** — fichier `migrati
> **Rappel règle ABSOLUE n°12** : chaque colonne créée ci-dessous DOIT recevoir son `COMMENT ON COLUMN`. Les 4 colonnes Timestampable/Blamable passent par le helper `addStandardTimestampableBlamableComments($schema, '<table>')`. Le SQL ci-dessous montre la structure ; les `COMMENT ON COLUMN` (un par colonne métier) sont à écrire dans la migration (exemples §3.2.bis).
> **Types réels de la migration (le code fait foi)** : le SQL ci-dessous est *illustratif*. La migration mergée (`Version20260605130000`) utilise le **style aligné M1** : clés primaires en `INT GENERATED BY DEFAULT AS IDENTITY` (et **non** `SERIAL`) et horodatages en `TIMESTAMP(0) WITHOUT TIME ZONE` (et **non** `TIMESTAMPTZ`, car le `TimestampableBlamableTrait` mappe `datetime_immutable`). Garantit que `schema:update` reste un no-op une fois les entités mappées.
```sql
-- =====================================================================
-- Seed taxonomie : nouveau type FOURNISSEUR (référentiels comptables = M1, non recréés)
@@ -422,8 +426,10 @@ use Symfony\Component\Validator\Constraints as Assert;
// Cohérence M1/ERP-62 : la LISTE embarque catégories + sites (pas de
// champ dérivé aplati). Maillon (c) : category:read + site:read dans
// le contexte pour exposer Category(code/name) + Site(name/postalCode).
// ⚠ Le SupplierRepository DOIT fetch-join categories + addresses.sites
// pour éviter le N+1 sur la liste (cf. § 2.12).
// ⚠ Anti-N+1 : pas de fetch-join dans la requête de liste — le
// SupplierRepository hydrate categories/sites/contacts via des requêtes
// IN bornées séparées (hydrateListCollections), pour éviter le produit
// cartésien sur les chemins non paginés (export) — cf. § 2.12.
normalizationContext: ['groups' => [
'supplier:read',
'category:read',
@@ -442,13 +448,14 @@ use Symfony\Component\Validator\Constraints as Assert;
normalizationContext: ['groups' => [
'supplier:read',
'supplier:item:read', // embed contacts / addresses
'supplier:read:accounting', // scalaires compta + embed ribs (filtré par le Provider selon accounting.view)
// ⚠ supplier:read:accounting est volontairement ABSENT ici : il est
// AJOUTÉ dynamiquement par le SupplierReadGroupContextBuilder quand
// l'user porte accounting.view (gating par ajout, pas par retrait —
// parade bug #4 M1). Il porte les scalaires compta + l'embed ribs.
'category:read', // embed des Category (id/code/name) — relation imbriquée
'site:read', // embed des Site (id/name/postalCode/city/color, pas de code) — relation imbriquée
'default:read',
]],
// Le Provider RETIRE supplier:read:accounting du contexte si l'user
// n'a pas is_granted('commercial.suppliers.accounting.view').
provider: SupplierProvider::class,
),
new Post(
@@ -458,10 +465,13 @@ use Symfony\Component\Validator\Constraints as Assert;
processor: SupplierProcessor::class,
),
new Patch(
security: "is_granted('commercial.suppliers.manage')",
// Le SupplierProcessor inspecte les groupes envoyés pour autoriser
// onglet par onglet (cf. § 2.10 + § 5). Patch des champs comptables
// exige is_granted('commercial.suppliers.accounting.manage') ;
// Security élargie : `manage` OU `accounting.manage` — le rôle Compta
// n'a pas `manage` mais doit pouvoir éditer l'onglet Comptabilité d'un
// fournisseur existant (§ 2.9). Le SupplierProcessor re-gate ensuite
// onglet par onglet (mode strict RG-2.16) :
security: "is_granted('commercial.suppliers.manage') or is_granted('commercial.suppliers.accounting.manage')",
// Patch des champs comptables exige accounting.manage (guardAccounting) ;
// champs main/information exigent manage (guardManage) ;
// patch isArchived exige is_granted('commercial.suppliers.archive').
normalizationContext: ['groups' => ['supplier:read', 'default:read']],
denormalizationContext: ['groups' => [
+8 -8
View File
@@ -82,14 +82,14 @@ final class Version20260605130000 extends AbstractMigration
// Ordre inverse des dependances FK : jointures et sous-collections
// d'abord, puis supplier. Les referentiels comptables et le
// CategoryType FOURNISSEUR ne sont pas touches (crees ailleurs).
$this->addSql('DROP TABLE supplier_address_category');
$this->addSql('DROP TABLE supplier_address_contact');
$this->addSql('DROP TABLE supplier_address_site');
$this->addSql('DROP TABLE supplier_rib');
$this->addSql('DROP TABLE supplier_address');
$this->addSql('DROP TABLE supplier_contact');
$this->addSql('DROP TABLE supplier_category');
$this->addSql('DROP TABLE supplier');
$this->addSql('DROP TABLE IF EXISTS supplier_address_category');
$this->addSql('DROP TABLE IF EXISTS supplier_address_contact');
$this->addSql('DROP TABLE IF EXISTS supplier_address_site');
$this->addSql('DROP TABLE IF EXISTS supplier_rib');
$this->addSql('DROP TABLE IF EXISTS supplier_address');
$this->addSql('DROP TABLE IF EXISTS supplier_contact');
$this->addSql('DROP TABLE IF EXISTS supplier_category');
$this->addSql('DROP TABLE IF EXISTS supplier');
}
// =================================================================
@@ -43,7 +43,10 @@ final class ClientInformationCompletenessValidator
foreach ($fields as $property => $value) {
if ($this->isMissing($value)) {
$violations->add(new ConstraintViolation(
sprintf('Ce champ est obligatoire pour le role Commerciale (champ "%s").', $property),
// Pas de nom de champ technique dans le message : la violation est
// deja rattachee au bon champ via son propertyPath (mappe inline
// cote front par useFormErrors).
'Ce champ est obligatoire pour le rôle Commerciale.',
null,
[],
$client,
@@ -47,7 +47,10 @@ final class SupplierInformationCompletenessValidator
foreach ($fields as $property => $value) {
if ($this->isMissing($value)) {
$violations->add(new ConstraintViolation(
sprintf('Ce champ est obligatoire pour le rôle Commerciale (champ "%s").', $property),
// Pas de nom de champ technique dans le message : la violation est
// deja rattachee au bon champ via son propertyPath (mappe inline
// cote front par useFormErrors).
'Ce champ est obligatoire pour le rôle Commerciale.',
null,
[],
$supplier,
@@ -188,7 +188,7 @@ class Client implements TimestampableInterface, BlamableInterface
private ?string $description = null;
#[ORM\Column(length: 255, nullable: true)]
#[Assert\Length(max: 255, maxMessage: 'Ce champ ne peut dépasser {{ limit }} caractères.', normalizer: 'trim')]
#[Assert\Length(max: 255, maxMessage: 'La liste des concurrents ne peut dépasser {{ limit }} caractères.', normalizer: 'trim')]
#[Groups(['client:read', 'client:write:information'])]
private ?string $competitors = null;
@@ -181,7 +181,7 @@ class Supplier implements TimestampableInterface, BlamableInterface
private ?string $description = null;
#[ORM\Column(length: 255, nullable: true)]
#[Assert\Length(max: 255, maxMessage: 'Ce champ ne peut dépasser {{ limit }} caractères.', normalizer: 'trim')]
#[Assert\Length(max: 255, maxMessage: 'La liste des concurrents ne peut dépasser {{ limit }} caractères.', normalizer: 'trim')]
#[Groups(['supplier:read', 'supplier:write:information'])]
private ?string $competitors = null;
@@ -121,6 +121,12 @@ final class ClientProcessor implements ProcessorInterface
return $this->persistProcessor->process($data, $operation, $uriVariables, $context);
}
// Reinitialisation de la memoisation du payload en debut de traitement :
// le service est partage (stateful), on repart du corps de LA requete
// courante et on n'herite jamais des cles decodees d'une requete passee.
$this->decodedContent = null;
$this->decodedPayloadKeys = [];
$writableKeys = $this->writablePayloadKeys();
$isArchiveRequest = $this->guardArchive($data, $writableKeys);
@@ -114,6 +114,12 @@ final class SupplierProcessor implements ProcessorInterface
return $this->persistProcessor->process($data, $operation, $uriVariables, $context);
}
// Reinitialisation de la memoisation du payload en debut de traitement :
// le service est partage (stateful), on repart du corps de LA requete
// courante et on n'herite jamais des cles decodees d'une requete passee.
$this->decodedContent = null;
$this->decodedPayloadKeys = [];
$writableKeys = $this->writablePayloadKeys();
$isArchiveRequest = $this->guardArchive($data, $writableKeys);
@@ -57,6 +57,11 @@ final class SupplierExportController
#[IsGranted('commercial.suppliers.view')]
public function __invoke(Request $request): Response
{
// Memes filtres d'archivage que la vue liste (SupplierProvider) pour que
// l'export reflete exactement ce que l'utilisateur voit a l'ecran :
// - includeArchived : inclut les archives en plus des actifs ;
// - archivedOnly : restreint aux seules archives (prioritaire, cf.
// createListQueryBuilder).
$includeArchived = $this->readBool($request->query->get('includeArchived'));
$archivedOnly = $this->readBool($request->query->get('archivedOnly'));
$search = $request->query->getString('search') ?: null;
@@ -82,6 +82,12 @@ use Symfony\Component\DependencyInjection\Attribute\Autowire;
*/
class SupplierFixtures extends Fixture implements DependentFixtureInterface
{
/**
* Type de categorie exige pour un fournisseur et ses adresses (RG-2.10).
* Miroir de Supplier::REQUIRED_CATEGORY_TYPE_CODE (non importable — regle n°1).
*/
private const string SUPPLIER_CATEGORY_TYPE_CODE = 'FOURNISSEUR';
/** Cache des categories resolues par nom (evite des requetes repetees). */
private array $categoryCache = [];
@@ -415,19 +421,27 @@ class SupplierFixtures extends Fixture implements DependentFixtureInterface
return $this->categoryCache[$name];
}
$category = $manager->getRepository(CategoryInterface::class)->findOneBy([
// RG-2.10 : on filtre explicitement sur le type FOURNISSEUR. Un lookup par
// le seul `name` rattacherait une categorie homonyme d'un autre type (ex.
// futur PRESTA) — donc du MAUVAIS type — ce qui violerait « au moins une
// categorie de type FOURNISSEUR ». Le filtre type est porte cote PHP
// (findBy ne sait pas filtrer une propriete imbriquee categoryType.code).
$candidates = $manager->getRepository(CategoryInterface::class)->findBy([
'name' => $name,
'deletedAt' => null,
]);
if (!$category instanceof CategoryInterface) {
throw new RuntimeException(sprintf(
'Categorie "%s" introuvable : CategoryFixtures doit tourner avant SupplierFixtures.',
$name,
));
foreach ($candidates as $candidate) {
if ($candidate instanceof CategoryInterface
&& self::SUPPLIER_CATEGORY_TYPE_CODE === $candidate->getCategoryTypeCode()) {
return $this->categoryCache[$name] = $candidate;
}
}
return $this->categoryCache[$name] = $category;
throw new RuntimeException(sprintf(
'Categorie FOURNISSEUR "%s" introuvable : CategoryFixtures doit tourner avant SupplierFixtures.',
$name,
));
}
/**
@@ -15,8 +15,9 @@ use PhpOffice\PhpSpreadsheet\IOFactory;
* Couvre : reponse 200 (Content-Type + Content-Disposition), exclusion des
* archives par defaut, respect du filtre ?search, peuplement des colonnes
* contact principal / categories / sites, gating de la colonne SIREN selon
* commercial.suppliers.accounting.view, 403 sans commercial.suppliers.view,
* 401 anonyme.
* commercial.suppliers.accounting.view (admin ET user minimal a permission
* explicite), dedup F3 (fournisseur multi-categories rendu sur une seule ligne),
* 403 sans commercial.suppliers.view, 401 anonyme.
*
* @internal
*/
@@ -178,6 +179,60 @@ final class SupplierExportControllerTest extends AbstractSupplierApiTestCase
self::assertStringNotContainsString('987654321', $this->flatten($grid));
}
/**
* Gating SIREN prouve via une permission EXPLICITE (et non le bypass admin) :
* un user minimal portant uniquement commercial.suppliers.view +
* commercial.suppliers.accounting.view voit bien la colonne SIREN et sa
* valeur. Complement de testSirenColumnPresentWithAccountingView (admin), qui
* ne prouve pas que accounting.view SEULE suffit (l'admin bypasse le RBAC).
* Le pendant negatif (sans accounting.view -> colonne absente) est couvert par
* testSirenColumnAbsentWithoutAccountingView.
*/
public function testSirenColumnPresentForMinimalUserWithAccountingView(): void
{
// Seed via admin, puis relecture par un user non-admin a 2 permissions.
$this->createAdminClient();
$supplier = $this->seedSupplier('Gated Siren Co');
$em = $this->getEm();
$supplier->setSiren('456789123');
$em->flush();
$creds = $this->createUserWithPermissions([
'commercial.suppliers.view',
'commercial.suppliers.accounting.view',
]);
$viewer = $this->authenticatedClient($creds['username'], $creds['password']);
$grid = $this->gridFromResponse($viewer->request('GET', self::EXPORT_URL)->getContent());
self::assertContains('SIREN', $grid[0]);
self::assertStringContainsString('456789123', $this->flatten($grid));
}
/**
* Dedup F3 : un fournisseur portant >= 2 categories FOURNISSEUR est multiplie
* par la jointure (selection/hydratation des collections) ; l'export doit le
* rendre sur UNE SEULE ligne. On seede un fournisseur a 2 categories et on
* assert qu'il n'apparait qu'une fois dans la colonne « Nom fournisseur ».
*/
public function testExportDeduplicatesSupplierWithMultipleCategories(): void
{
$client = $this->createAdminClient();
$supplier = $this->seedSupplier('Multi Cat Co', false, 'NEGOCIANT');
// 2e categorie FOURNISSEUR sur le meme fournisseur (RG-2.10).
$supplier->addCategory($this->supplierCategory('GROSSISTE'));
$this->getEm()->flush();
$names = $this->companyNames($client->request('GET', self::EXPORT_URL)->getContent());
$occurrences = count(array_filter($names, static fn (string $name): bool => 'MULTI CAT CO' === $name));
self::assertSame(
1,
$occurrences,
'Un fournisseur multi-categories doit apparaitre sur une seule ligne (dedup F3).',
);
}
public function testForbiddenWithoutSuppliersViewPermission(): void
{
$creds = $this->createUserWithPermission('core.users.view');
@@ -126,6 +126,10 @@ final class SupplierSubResourceApiTest extends AbstractSupplierApiTestCase
public function testPostAddressWithoutSiteReturns422(): void
{
// Sans cette garde, un module Sites desactive renverrait 404 (route
// /addresses indisponible) et le test passerait pour la MAUVAISE raison
// au lieu de prouver RG-2.06 (Assert\Count min 1 sur sites).
$this->skipIfSitesModuleDisabled();
$client = $this->createAdminClient();
$seed = $this->seedSupplier('Address No Site');
@@ -101,6 +101,24 @@ final class SupplierProcessorTest extends TestCase
self::assertInstanceOf(Supplier::class, $processor->process($supplier, $this->operation()));
}
public function testAdminIncompleteInformationPasses(): void
{
// Distinct du cas user=null : un utilisateur AUTHENTIFIE mais non-Commerciale
// (ici un admin, BusinessRoleAwareInterface renvoyant false pour tout role
// metier) n'est pas soumis a la completude Information -> 200 malgre un
// onglet Information incomplet. Prouve que le gate porte bien sur le ROLE
// metier Commerciale, et pas sur « il y a un utilisateur connecte ».
$supplier = $this->minimalSupplier();
$supplier->setDescription('Une description');
$processor = $this->makeProcessor(
payload: ['description' => 'Une description'],
user: $this->adminUser(),
);
self::assertInstanceOf(Supplier::class, $processor->process($supplier, $this->operation()));
}
/**
* @param list<string> $granted Permissions accordees a l'utilisateur courant
* @param array<string, mixed> $payload Corps JSON simule de la requete
@@ -175,6 +193,33 @@ final class SupplierProcessorTest extends TestCase
return $this->createStub(Operation::class);
}
/**
* Utilisateur authentifie non-Commerciale (profil admin) : porte
* BusinessRoleAwareInterface mais ne reconnait aucun role metier. Sert a
* distinguer « pas de role Commerciale » de « pas d'utilisateur » (null).
*/
private function adminUser(): UserInterface
{
return new class implements UserInterface, BusinessRoleAwareInterface {
public function hasBusinessRole(string $roleCode): bool
{
return false;
}
public function getRoles(): array
{
return ['ROLE_ADMIN'];
}
public function eraseCredentials(): void {}
public function getUserIdentifier(): string
{
return 'admin-test';
}
};
}
private function commercialeUser(): UserInterface
{
return new class implements UserInterface, BusinessRoleAwareInterface {
+36 -12
View File
@@ -90,6 +90,26 @@ abstract class AbstractApiTestCase extends ApiTestCase
* @return array{username: string, password: string} Les identifiants pour authenticatedClient()
*/
protected function createUserWithPermission(string $permissionCode): array
{
return $this->createUserWithPermissions([$permissionCode]);
}
/**
* Variante multi-permissions de {@see createUserWithPermission()} : cree un
* utilisateur non-admin portant PLUSIEURS permissions via un unique role
* jetable. Utile pour prouver qu'une combinaison precise de permissions
* (sans le bypass admin) suffit a debloquer un comportement — ex. la colonne
* SIREN de l'export, gatee par accounting.view EN PLUS de suppliers.view.
*
* Memes garanties que le singulier : suffixe aleatoire, password "testpass",
* rattachement a tous les sites, echec explicite si une permission est
* introuvable en base.
*
* @param list<string> $permissionCodes codes des permissions a accorder
*
* @return array{username: string, password: string} identifiants pour authenticatedClient()
*/
protected function createUserWithPermissions(array $permissionCodes): array
{
if (!self::$kernel) {
self::bootKernel();
@@ -97,17 +117,6 @@ abstract class AbstractApiTestCase extends ApiTestCase
$em = $this->getEm();
/** @var null|Permission $permission */
$permission = $em->getRepository(Permission::class)->findOneBy(['code' => $permissionCode]);
self::assertNotNull(
$permission,
sprintf(
'Permission "%s" introuvable en base. Assurez-vous que `app:sync-permissions` a ete execute.',
$permissionCode,
),
);
$suffix = substr(bin2hex(random_bytes(4)), 0, 8);
$username = 'testuser_'.$suffix;
$password = 'testpass';
@@ -116,7 +125,22 @@ abstract class AbstractApiTestCase extends ApiTestCase
$hasher = self::getContainer()->get(UserPasswordHasherInterface::class);
$role = new Role('test_'.$suffix, 'Test Role '.$suffix, false);
$role->addPermission($permission);
foreach ($permissionCodes as $permissionCode) {
/** @var null|Permission $permission */
$permission = $em->getRepository(Permission::class)->findOneBy(['code' => $permissionCode]);
self::assertNotNull(
$permission,
sprintf(
'Permission "%s" introuvable en base. Assurez-vous que `app:sync-permissions` a ete execute.',
$permissionCode,
),
);
$role->addPermission($permission);
}
$em->persist($role);
$user = new User();