[FEATURE] Permettre la modification des sujets d'un profil cible non relié à une campagne (PIX-12436).

[Jeyffrey]

🦄 Problème

Nous avons l'objectif de rendre possible la duplication d'un profil cible existant en autorisant la modification de son référentiel.

Or, à l'heure actuelle, il n'est pas permis de modifier ce référentiel.

🤖 Proposition

La route PATCH /api/admin/target-profiles/{id} accepte maintenant les tubes en payload.

Pour respecter le DDD, dans le modèle TargetProfileForAdmin, on a ajouté une méthode dédiée à l'update qui gère les besoins métier :

• on permet la modification des tubes uniquement si le profil cible n'est pas relié à une campagne,
• on vérifie la validité de la nouvelle valeur de la propriété category,
• on vient mettre à jour les données du model directement dans cette méthode.

🌈 Remarques

1. La méthode du TargetProfileSerializer deserializeCreationCommand a été renommée deserialize car elle est maintenant aussi utilisée pour la modification.

2. Dans tous les cas, on ne permet pas la modification de la propriété isPublic.

3. Le allowUnknown (https://github.com/1024pix/pix/blob/dev/api/lib/application/target-profiles/index.js#L337) sur la payload a été supprimé, il convient donc de modifier l'adapter d'Ember.

💯 Pour tester

• Tests verts
• Vérifier que la modification de profil cible fonctionne toujours sur PixAdmin.

[pix-bot-github]

Une fois les applications déployées, elles seront accessibles via les liens suivants :

• App (.fr)
• App (.org)
• Orga (.fr)
• Orga (.org)
• Certif (.fr)
• Certif (.org)
• 1D
• Admin
• API
• Audit Logger

Les variables d'environnement seront accessibles via les liens suivants :

• scalingo front
• scalingo api
• scalingo audit-logger

[laura-bergoens]

j'ai un avis de plus en plus tranché sur la pertinence de l'existence des usecases de ce style.
On a un cas parfait où il n'a pas vraiment de responsabilités, de pouvoir de décision puisque tout a été déplacé dans le modèle.
On pourrait aussi bien déplacer tout ce code dans le controller que personnellement ca ne me ferait ni chaud ni froid, et sans casser l'archi hexagonale.
Le truc qui met le clou final dans le cercueil pour moi c'est qu'on se retrouve à écrire un fichier de test unitaire de ce usecase, qui est assez peu utile et surtout très difficilement maintenable car dès qu'on change un détail d'implémentation le test se transforme en guirlande de Noël. Il mock tout dans tous les sens, il est bien trop proche de l'implémentation

[frinyvonnick]

Je remets la définition des usecases dans le livre "Clean Architecture" pour qu'on ait la même base dans la discussion

Use cases contain the rules that specify how and when the Critical Business Rules within the Entities are invoked. Use cases control the dance of the Entities.
Notice also that the use case does not describe the user interface other than to informally specify the data coming in from that interface, and the data going back out through that interface. From the use case, it is impossible to tell whether the application is delivered on the web, or on a thick client, or on a console, or is a pure service.
This is very important.

Il y a certain usecases où je suis d'accord avec toi @laura-bergoens sur le fait qu'ils sont des passes plat, comme celui là par exemple :

const acceptPixCertifTermsOfService = function ({ userId, userRepository }) {
  return userRepository.updatePixCertifTermsOfServiceAcceptedToTrue(userId);
};

export { acceptPixCertifTermsOfService };

Dans celui-là, effectivement le usecase ne fait que transmettre au repository les informations qu'il a en entrée et ne manipule donc aucun objet du domain. Il ne répond donc pas à la définition d'un usecase.

Par contre on pourrait le refactorer de la manière suivante pour qu'il en soit plus proche :

const acceptPixCertifTermsOfService = function ({ userId, userRepository }) {
  const user = userRepository.get(userId);

  user.acceptTermsOfService({ certif: true });

  return userRepository.update(user)
};

export { acceptPixCertifTermsOfService };

Et donc pour moi le usecase updateTargetProfile n'est pas juste un passe plat :

• Il prend en entrée des données dans un certain format
• Il va chercher le model du domain concerné
• Appelle une méthode du model qui encapsule la logique métier
• Persiste le model

Pour moi, si on appelle directement les repository dans le controller et qu'on manipule les models à ce niveau là on casse le découplage.

Use cases do not describe how the system appears to the user. Instead, they describe the application-specific rules that govern the interaction between the users and the Entities. How the data gets in and out of the system is irrelevant to the use cases.

Dans un commentaire plus haut tu parles des différents type de validation #8842 (comment). Si tout le code est dans le controller qu'est ce qui nous empêche de céder à la facilité de tout mettre dans la configuration du endpoint ?

Use cases expect input data, and they produce output data. However, a well-formed use case object should have no inkling about the way that data is communicated to the user, or to any other component. We certainly don’t want the code within the use case class to know about HTML or SQL!

Les controllers sont pour moi couplé à notre contexte d'utilisation de l'API (sur le web) et les libraries ou patterns qu'on utilise (ex : Hapi, json-api, ...). Ce qui fait que si demain on décide d'en changer (exemple au hasard GraphQL ce que permet EmberData) on va devoir potentiellement toucher à du code qui est sensé être dans le domain si on a pas été en mesure de bien décorrelé les deux.

Le truc qui met le clou final dans le cercueil pour moi c'est qu'on se retrouve à écrire un fichier de test unitaire de ce usecase, qui est assez peu utile et surtout très difficilement maintenable car dès qu'on change un détail d'implémentation le test se transforme en guirlande de Noël. Il mock tout dans tous les sens, il est bien trop proche de l'implémentation

Cela me semble normal puisque le usecase est un chef d'orchestre son seul rôle est de s'assurer qu'un ensemble d'actions a été éxécuté dans le bon ordre. Pour moi un bon usecase ne doit pas avoir de logique. Car si c'est le cas c'est que vraisemblablement c'est du métier et donc a plus sa place dans un model. Ses tests sont sensés changer que si le flow d'actions (l'enchaînement métier) change. En règle générale, si il n'y a pas d'embranchements il n'a même qu'un seul test "nominal" et éventuellement un ou plusieurs tests d'erreurs.

Un dernier point qui m'embête avec le fait de mettre directement l'appel aux repository et la manipulation des models dans les controllers c'est faire reposer sur les épaules des développeurs la décision de ce qui merite un fichier de usecase ou pas. C'est une heuristique compliquée et très subjective (la réponse variera d'un dev à un autre).

Quand quelqu'un arrive chez Pix, il doit se familiariser avec la Clean Architecture et DDD. Pour les devs juniors cela n'est pas évident du tout. On a régulièrement des remarques sur les différences de faire à certains endroits. Si l'architecture varie en fonction des endpoints cela va renforcer ce sentiment et la réponse aux questions a apporté ne sera pas évidente de mon point de vue.

[frinyvonnick]

Précision suite à une discussion Slack Ce thread est plus un échange d'idée qu'une vraie demande de changement dans le cadre de cette PR.

[frinyvonnick]

Pour faire écho au commentaire de @laura-bergoens sur la validation sémantique. Cette validation a sa place dans le domain d'où cette liste est extraite.

[Steph0]

Je suis en phase sur le fait de (re)valider cela côté Domain. Néanmoins cela peut être intéressant de garder cependant cette version côté Swagger car cela vient documenter nos API (swagger.json) pour les consommateurs

Côté Domain ce serait un MustHave, là où le swagger en CouldHave :)

[frinyvonnick]

Je trouve ça dommage pour des besoins de documentation de changer le fonctionnement du code. Si on laisse cette validation ici on est pas en mesure de produire des erreurs du domain 😬

[frinyvonnick]

Revue fonctionnelle :
Je ne peux pas créer de nouveau profil cible :

Revue tech :
Cela me semble bon 👌 Merci pour tous les changements opérés 🙇