This repository has been archived by the owner on Sep 12, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 0
[FEATURE] Ajouter un endpoint pour créer une entrée dans la base de journalisation (PIX-8516) #3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
igorissen
added
👀 Func Review Needed
👀 Tech Review Needed
and removed
Development in progress
labels
Jul 6, 2023
igorissen
commented
Aug 3, 2023
igorissen
force-pushed
the
pix-8516
branch
6 times, most recently
from
August 4, 2023 10:00
04a08e1
to
5bc6c16
Compare
Co-authored-by: Eric Lim <eric.lim@zenika.com> Co-authored-by: Thomas Evano <thomas.evano@pix.fr>
igorissen
force-pushed
the
pix-8516
branch
2 times, most recently
from
August 7, 2023 07:21
4e94ee1
to
08a911e
Compare
Co-authored-by: Eric Lim <eric.lim@zenika.com> Co-authored-by: Thomas Evano <thomas.evano@pix.fr>
Co-authored-by: Ismael Gorissen <ismael.gorissen@gmail.com> Co-authored-by: Eric Lim <eric.lim@zenika.com>
Co-authored-by: Eric Lim <eric.lim@zenika.com> Co-authored-by: Thomas Evano <thomas.evano@pix.fr>
Co-authored-by: Ismael Gorissen <ismael.gorissen@gmail.com> Co-authored-by: Eric Lim <eric.lim@zenika.com>
Co-authored-by: Ismael Gorissen <ismael.gorissen@gmail.com> Co-authored-by: Eric Lim <eric.lim@zenika.com>
Co-authored-by: Ismael Gorissen <ismael.gorissen@gmail.com> Co-authored-by: Eric Lim <eric.lim@zenika.com>
Co-authored-by: Ismael Gorissen <ismael.gorissen@gmail.com> Co-authored-by: Thomas Evano <thomas.evano@pix.fr>
Co-authored-by: Eric Lim <eric.lim@zenika.com> Co-authored-by: Thomas Evano <thomas.evano@pix.fr>
igorissen
force-pushed
the
pix-8516
branch
2 times, most recently
from
August 7, 2023 14:58
feccaff
to
2dc5d79
Compare
reibecca
approved these changes
Aug 7, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Testé fonctionnellement sur Postman
clemlatz
approved these changes
Aug 7, 2023
Testé fonctionnellement avec l'outil http de Webstorm ✅ |
igorissen
added
🚀 Ready to Merge
and removed
👀 Func Review Needed
👀 Tech Review Needed
labels
Aug 7, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🦄 Problème
Le worker côté API écoutant les jobs pgboss de type “journalisation” doit pouvoir communiquer à l’application de journalisation une instruction de création d’une nouvelle entrée dans la base de journalisation.
🤖 Proposition
🌈 Remarques
Authentification
pour la communication entre l'api Pix et l'api Pix Audit Logger. Ceci est à faire dans une autre PR.L'implémentation de l'API diffère de celle sur Pix, notamment grâce à l'utilisation du Typescript, et chaque point reste à valider avec l'équipe.
Une liste non exhaustive des différences :
Vitest vs Jest
Le support de la partie ESM par
Jest
est encore expérimental comme décrit sur cette doc. Après un test de Jest sur le projet, nous nous sommes heurtés à une erreur décrit sur cette issue dets-jest
qui n'est pour le moment pas corrigé sans avoir à faire un workaround. Le fait que Vitest vienne avec un support natif de ESM et Typescript, donc sans configuration spécifique, nous a poussé à partir dessus.Controller
Le dossier
application
a été renommé encontroller
pour mieux refléter ce qu'il y a dedans (controllers + routes). En clean archi, le dossierapplication
contient normalement les usecases comme sur ce schéma. Ce qui n'est pas le cas sur l'API Pix et cause une confusion sur l'architecture car elle mélange l'hexagonale et la clean archi. A voir pour garder le nommage.Sur l'API Pix, il y avait un fichier pour la/les routes du controller (ex: index.js) et un autre fichier pour le controller (ex: account-recovery-controller.js). C'est unifié sur cette PR dans un seul fichier
create-audit-log.controller.ts
. Le nom index.js n'est pas clair pour dire que c'est une route et non obligatoire en ESM.Un fichier
controller
par route afin d'améliorer la lisibilité. Cela évite d'avoir plusieurs endpoints dans un même fichier et donc d'avoir des centaines de lignes de code dessus.Un autre format de nommage qui ressemble à celui du usecase (action-sujet.controller.ts). Ceci aussi par clarté car cela permet, à la lecture du nom du fichier, de comprendre ce que fait le controller.
Models
la composition dessus.
models.definition.ts
.Repositories
Le nommage utilisé sur Pix API des repository (ex:
AuthenticationMethodRepository
) ne permet pas de savoir la source de donnée utilisée. Cela peut très bien être du Postgres, InMemory, Redis, d'une autre API...De plus, l'implémentation est directement utilisé dans le usecase ce qui entraîne un couplage entre les 2 comme par exemple le passage d'une transaction du usecase vers le repository.
Typescript, nous permettant d'utiliser les interfaces, introduit une couche d'abstraction permettant d'éviter ce couplage.
En effet, les consommateurs du repository utiliseront, comme typage, l'interface plutôt que l'implémentation.
Dans notre cas, l'interface est
AuditLogRepository
et son implémentationAuditLogPostgresRepository
. A noter que cela permet aussi la présence de la source de donnée utilisée dans le nom de l'implémentation grâce à la couche d'abstraction décrite plus haut 😄💯 Pour tester
npm ci
(si ce n'est pas fait)npm run db:reset
Database pix_audit_logging does not exist
npm run db:create
npm run db:migrate
npm start
Cas en succès
http://localhost:3001/api/audit-logs
avec la méthodePOST
et le payload suivantCas en erreur
http://localhost:3001/api/audit-logs
avec la méthodePOST
et le payload suivant