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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(france-travail): add rendez-vous-partenaire api webhooks for participation v2 #2497
base: staging
Are you sure you want to change the base?
feat(france-travail): add rendez-vous-partenaire api webhooks for participation v2 #2497
Changes from 21 commits
682cfe8
db8f8d0
015eb97
23b8d0d
5acae51
4c4cd94
54370b0
fe76fd0
85fa6b3
8258b60
e9fd685
1c23cf4
1d04806
d195e15
d3bb779
8159960
819ce60
991f3e0
8a84592
91e53d9
13d1727
5539acf
375c08c
d10d0e5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
C'est très cool d'avoir présenté ça comme ça dans cette classe.
Malheureusement comme l'API de nos services est pas ouf, quand tu vas appeler ces méthodes depuis les services d'origine, il ne va rien se passer quand les services appelés par ces méthodes vont fail (ce qui risque d'arriver souvent malheureusement vu les API capricieuses de FT 😢 ).
Aujourd'hui le seul moyen d'appeler un service correctement depuis un autre service c'est la méthode
call_service!
, je pense qu'on pourrait faire mieux. Mais du coup je pense que soit on trouve un moyen ici de gérer ça autrement que via la méthodecall_service!
, soit au déplace l'appel de ces services dans les services directement.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.
Bien vu merci pour ce rappel au sujet de
call_service!
J'ai mis ca en place comme tu le suggére ici : 5539acfThis file was deleted.
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.
Je n'avais pas fait attention pendant ma première review mais on va avoir un problème ici: lorsque tu vas appeler
Participation.find
dans le service la participation aura probablement déjà disparu. C'est pour ça que dans l'implémentation précédente le payload était généré ici avant le job.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.
Bien vu pour ca effectivement....
Comme on a vu ensemble le plus propre pour éviter d'avoir des arguments conditionnés au delete uniquement dans un Job (
ProcessParticipationJob
) qui faisait déjà beaucoup de choses c'est de partir sur une meilleure séparation des responsabilité (comme tu me l'avais suggéré initialement d'ailleurs ;) ) Je met en place des jobs spécifiques pour le create/l'update et le delete. C'est fait dans le dernier commit : 375c08cThis file was deleted.