Règles plus restrictives pour la suppression de certain objets afin de ne pas créer d'incohérences de données #3150
Conversation
rsebille
requested review from
celine-m-s,
francoisfreitag,
ikarius,
tonial,
vincentporte,
vperron and
xavfernandez
xavfernandez
left a comment
xavfernandez
left a comment
There was a problem hiding this comment.
Super initiative.
Personnellement je suis plutôt dans l'équipe RESTRICT par défaut avec un commentaire dans le cas contraire 😅
| null=True, | ||
| blank=True, | ||
| on_delete=models.SET_NULL, | ||
| on_delete=models.RESTRICT, # For traceability and accountability, the dates can be edited |
There was a problem hiding this comment.
Si on veut aller à fond là-dedans (et tous les autres "traceability & accountability du même genre) je dirais:
- ni nullable, ni blankable (pour tous ces
created_by/updated_by...) - RESTRICT ou CASCADE suivant le cas
- créer ou réutiliser un utilisateur singleton (genre
robot <machine@internal.com>) pour tous les cas où on fait des updates par script.
There was a problem hiding this comment.
Vu que tout le monde semblait être plutôt d'avis d'être restrictif ( 🥁 💇 ) j'ai modifié les on_delete dans ce sens.
Pour les null/blank vu qu'il faut rester compatible avec les NULL existant faudra gérer ça au cas par cas.
| verbose_name="SIAE du déclarant", | ||
| null=True, | ||
| on_delete=models.SET_NULL, | ||
| on_delete=models.RESTRICT, # For traceability and accountability, people's organization can change |
There was a problem hiding this comment.
👍 mais ça me donne envie de lancer un chantier "désactivation des SIAEs" plutot que de les supprimer, pour garder la traçabilité dans le temps. Non ?
There was a problem hiding this comment.
La plupart du temps, on désactive les SIAE de toute façon.
https://www.notion.so/plateforme-inclusion/SIAE-ou-DDETS-demande-suppression-SIAE-cl-ture-compte-7e1337dd41b640faac0df86877e65150
| null=True, | ||
| blank=True, | ||
| on_delete=models.SET_NULL, | ||
| on_delete=models.RESTRICT, # For traceability and accountability |
There was a problem hiding this comment.
pareil pour la désactivation des prescripteurs, je sens que retirer ces SET_NULL va faire péter les scripts de syncho, l'admin, etc.
There was a problem hiding this comment.
Le support est prévenu. Mieux vaut une erreur que perdre de la données sur ces modèles critiques.
rsebille
force-pushed
the
rsebille/fk_on_delete
branch
from
894c94c to
2dad337
Compare
rsebille
force-pushed
the
rsebille/fk_on_delete
branch
from
2dad337 to
1ffb0f1
Compare
francoisfreitag
left a comment
francoisfreitag
left a comment
There was a problem hiding this comment.
💯 👍 🥇
Mais pourquoi avons-nous attendu si longtemps pour merger cette PR ?
On ne veut pas vivre dans un monde aussi dangereux. Merci d’avoir ramené quelques gardes fous (et le petit coup de pression pour les intégrer 😁) 🙇
| verbose_name="SIAE du déclarant", | ||
| null=True, | ||
| on_delete=models.SET_NULL, | ||
| on_delete=models.RESTRICT, # For traceability and accountability, people's organization can change |
There was a problem hiding this comment.
La plupart du temps, on désactive les SIAE de toute façon.
https://www.notion.so/plateforme-inclusion/SIAE-ou-DDETS-demande-suppression-SIAE-cl-ture-compte-7e1337dd41b640faac0df86877e65150
| null=True, | ||
| blank=True, | ||
| on_delete=models.SET_NULL, | ||
| on_delete=models.RESTRICT, # For traceability and accountability |
There was a problem hiding this comment.
Le support est prévenu. Mieux vaut une erreur que perdre de la données sur ces modèles critiques.
itou/eligibility/models/geiq.py
Outdated
| blank=True, | ||
| null=True, | ||
| on_delete=models.SET_NULL, | ||
| on_delete=models.RESTRICT, # Prevent promoting a criteria form child to parent |
There was a problem hiding this comment.
| on_delete=models.RESTRICT, # Prevent promoting a criteria form child to parent | |
| on_delete=models.RESTRICT, # Prevent promoting a criteria from child to parent |
itou/job_applications/models.py
Outdated
| settings.AUTH_USER_MODEL, | ||
| verbose_name="utilisateur émetteur", | ||
| on_delete=models.SET_NULL, | ||
| on_delete=models.SET_NULL, # FIXME: Do we need traceability and accountability? |
There was a problem hiding this comment.
Le code a été adapté pour gérer un prescripteur qui a été supprimé dans l’admin, sans cris ni larmes. (avant, on avait une 500 car on récupérait un None)
Je pense qu’on est OK tant qu’il reste le diagnostique et l’organisation prescriptrice ?
There was a problem hiding this comment.
Hésitation résolue par le fait de passer en mode RESTRICT-first.
itou/job_applications/models.py
Outdated
| null=True, | ||
| blank=True, | ||
| on_delete=models.SET_NULL, | ||
| on_delete=models.SET_NULL, # FIXME: Do we need traceability and accountability? |
There was a problem hiding this comment.
Peut-être plus intéressant de garder, et on n’a sûrement moins de raisons (e.g. RGPD) de supprimer (sans transfert) une orga prescriptrice ?
There was a problem hiding this comment.
Hésitation résolue par le fait de passer en mode RESTRICT-first.
itou/siae_evaluations/models.py
Outdated
| "job_applications.JobApplication", | ||
| verbose_name="candidature", | ||
| on_delete=models.CASCADE, | ||
| on_delete=models.CASCADE, # FIXME: RESTRICT because CaP is an auditability tool? |
itou/siae_evaluations/models.py
Outdated
| EvaluatedSiae, | ||
| verbose_name="SIAE évaluée", | ||
| on_delete=models.CASCADE, | ||
| on_delete=models.CASCADE, # FIXME: RESTRICT because CaP is an auditability tool? |
There was a problem hiding this comment.
Il arrive qu'on souhaite retirer une SIAE d'une campagne (par exemple, parce qu'elle n'est plus accessible suite à une convention expirée) donc le CASCADE pourrait être pratique.
Après, les cas sont assez rares donc on pourra tout à fait le faire auto-prescription par auto-prescription avant de supprimer l'EvaluatedSiae
itou/siae_evaluations/models.py
Outdated
| "eligibility.AdministrativeCriteria", | ||
| verbose_name="critère administratif", | ||
| on_delete=models.CASCADE, | ||
| on_delete=models.CASCADE, # FIXME: RESTRICT because CaP is an auditability tool? |
There was a problem hiding this comment.
Yep. Au pire, on passera gérer ces cas manuellement si un jour on supprime un critère d’éligibilité.
itou/siae_evaluations/models.py
Outdated
| proof = models.ForeignKey("files.File", on_delete=models.CASCADE, blank=True, null=True) | ||
| proof = models.ForeignKey( | ||
| "files.File", on_delete=models.CASCADE, blank=True, null=True | ||
| ) # FIXME: RESTRICT because CaP is an auditability tool? |
There was a problem hiding this comment.
Ça, tu peux être sûr que RESTRICT ne cassera rien. Donc pas d’hésitation.
itou/siae_evaluations/models.py
Outdated
| EvaluatedJobApplication, | ||
| verbose_name="candidature évaluée", | ||
| on_delete=models.CASCADE, | ||
| on_delete=models.CASCADE, # FIXME: RESTRICT because CaP is an auditability tool? |
There was a problem hiding this comment.
J’hésite pour celui-ci. Les critères sélectionnés pour justifier une candidature ne sont pas si important, c’est déclaratif pour l’employeur et si on supprime la candidature à évaluer, les critères sélectionnés peuvent suivre. Je laisserais bien CASCADE.
There was a problem hiding this comment.
Je resterais également sur CASCADE.
itou/siae_evaluations/models.py
Outdated
| institution = models.ForeignKey( | ||
| "institutions.Institution", | ||
| on_delete=models.CASCADE, | ||
| on_delete=models.CASCADE, # FIXME: Seems dangerous to allow campaign deletion on institution deletion |
itou/siae_evaluations/models.py
Outdated
| EvaluatedJobApplication, | ||
| verbose_name="candidature évaluée", | ||
| on_delete=models.CASCADE, | ||
| on_delete=models.CASCADE, # FIXME: RESTRICT because CaP is an auditability tool? |
There was a problem hiding this comment.
Je resterais également sur CASCADE.
itou/siae_evaluations/models.py
Outdated
| EvaluatedSiae, | ||
| verbose_name="SIAE évaluée", | ||
| on_delete=models.CASCADE, | ||
| on_delete=models.CASCADE, # FIXME: RESTRICT because CaP is an auditability tool? |
There was a problem hiding this comment.
Il arrive qu'on souhaite retirer une SIAE d'une campagne (par exemple, parce qu'elle n'est plus accessible suite à une convention expirée) donc le CASCADE pourrait être pratique.
Après, les cas sont assez rares donc on pourra tout à fait le faire auto-prescription par auto-prescription avant de supprimer l'EvaluatedSiae
celine-m-s
left a comment
celine-m-s
left a comment
There was a problem hiding this comment.
Merci pour cette remise à plat ! 🎉
Je suis d'accord avec tous les changements suggérés par les relecteurs (donc j'approuve en anticipant les changements à venir).
This may happen when the siae was removed by the import_siae script. This removal should soon be blocked (see #3150)
This may happen when the siae was removed by the import_siae script. This removal should soon be blocked (see #3150)
This may happen when the siae was removed by the import_siae script. This removal should soon be blocked (see #3150)
This may happen when the siae was removed by the import_siae script. This removal should soon be blocked (see #3150)
This may happen when the siae was removed by the import_siae script. This removal should soon be blocked (see #3150)
This may happen when the siae was removed by the import_siae script. This removal should soon be blocked (see #3150)
rsebille
force-pushed
the
rsebille/fk_on_delete
branch
from
1ffb0f1 to
d8d0ab9
Compare
rsebille
force-pushed
the
rsebille/fk_on_delete
branch
2 times, most recently
from
c949994 to
3b8c6fa
Compare
rsebille
force-pushed
the
rsebille/fk_on_delete
branch
2 times, most recently
from
3f65f59 to
1156086
Compare
rsebille
force-pushed
the
rsebille/fk_on_delete
branch
from
1156086 to
dac1b2c
Compare
rsebille
changed the title
ForeignKey.on_delete
Note
Je met tout le monde en relecteur mais il n'y a pas forcément besoin de tout relire, vous pouvez seulement vous concentrer sur un ou des domaines en particuliers.
Warning
Ces changements étant particulièrement lié aux aspects métiers, donc diffus dans l'équipe et assez impactant, n'hésitez pas à regarder les parties modifiées ET non-modifiées !
Il y a au final assez peu de modification évidente et qui, je pense, ferons consensus [1]. Le but est donc de lancer les réflexions à partir d'une première proposition.
tl;dr : C'est le moment de sortir vos meilleures pinaillage 😁.
Pourquoi ?
Cela fait maintenant plusieurs fois que des problèmes de données liés à des
ForeignKeysont détectés [1], faisons donc l'inventaire de celles-ci ainsi que de leur clauseon_delete.On utilise quasiment que
CASCADEetSET_NULL:CASCADEme semblant être un tropisme "la suppression n'intervient que dans l'admin et il y a un message de validation"SET_NULLsont souvent là par simplicité au prix de l'auditabilité, a voir où on veux mettre le curseur au global et/ou en fonction des objets[1] Je pense au
CASCADEpour lesMembershipAbstract.updated_by