🎁 Suggestion de changement v1 #1141

maxcorbeau · 2024-12-16T11:26:28Z

🎁 Suggestion de changement v1

Carte Notion : CLUSTERING: Modèles & UI django pour le stockage des clusters

quoi: un modèle et une UI Django pour suggérer et revoir (rejeter, approuver) des suggestions de changements
pourquoi: initialement pour le clustering, mais on veut s'en servir à l'avenir pour suggérer d'autres changements (diff d'ingestion des acteurs)
comment: en rendant le schema/nomenclature générique mais en validant les données fournies

Approche

Suite à une réunion ce matin on a décidé avec @kolok et @chrischarousset de partir sur une approche abstraite de la notion de changement, pour l'utiliser dans d'autres contextes que le clustering (ex: diff d'ingestion)
Par conséquent on a besoin d'un champ JSON pour représenter le changement, car tout changement n'aura pas la même structure (ex: pour le clustering on a besoin d'un cluster_id, pas pour le diff d'ingestion des acteurs)
En ce qui concerne la validation du JSON
- Manque d’XP django de ma part pour savoir ce qui est le mieux
- Je vois des champs JSONField dans notre code (ex: DagRun.meta_data) mais je ne trouve pas de validation associée
- Donc par défaut je suis parti sur JSONField avec de la validation dans le modèle sous clean

➡️ Restant à faire

Comprendre/résoudre problème d'affichage dans Django Admin UI
Une fois le modèle finalisé: écrire des tests unitaires pour valider le cas clustering et s'assurer que les problèmes de formatage de données soulèvent bien des exceptions
Tester le modèle en important les clusters de déchetteries

fabienheureux · 2024-12-16T18:48:44Z

qfdmo/models/change_suggestion.py

+        verbose_name=dedent(
+            """Données de debug de la suggestion
+        (quelles données ont été utilisées pour la suggestion)"""
+        ),


Suggested change

verbose_name=dedent(

"""Données de debug de la suggestion

(quelles données ont été utilisées pour la suggestion)"""

),

verbose_name="Données de debug de la suggestion"

"(quelles données ont été utilisées pour la suggestion)",

Suggestion : python permet d'écrire des strings sur plusieurs lignes dans ce genre de situations

fabienheureux · 2024-12-16T18:49:10Z

qfdmo/models/change_suggestion.py

+        verbose_name=dedent(
+            """Données du changement
+         (stocker uniquement le strict minimum pour
+         gérer le changement)"""
+        ),


Suggested change

verbose_name=dedent(

"""Données du changement

(stocker uniquement le strict minimum pour

gérer le changement)"""

),

verbose_name="Données du changement"

"(stocker uniquement le strict minimum pour

gérer le changement)",

Suggestion : idem ci-dessus

fabienheureux · 2024-12-16T19:37:21Z

qfdmo/models/change_suggestion.py

+        # On remplit review_at si la revue est effectuée ET
+        # que c'est la première fois qu'on change le statut (sinon
+        # on garde la valeur originale)


Remarque : Je ne suis pas forcément pour ajouter ce type de commentaires dans le code qui paraphrasent ce qui est lisible à la lecture des conditions

fabienheureux · 2024-12-16T19:38:13Z

qfdmo/models/change_suggestion.py

+        (ex: pour un cluster d'acteur = cluster_id -> acteur_id). Ne pas stocker
+        d'autres données qui pourraient devenir obsolètes au moment de la revue.
+        Si on souhaite fournir de la donnée supplémentaire, il faut le faire à
+        la vollée (ex: au moment de l'affichage ou de l'export)


Suggested change

la vollée (ex: au moment de l'affichage ou de l'export)

la volée (ex: au moment de l'affichage ou de l'export)

Petite typo

fabienheureux · 2024-12-16T19:41:36Z

qfdmo/models/change_suggestion.py

+        # Validations spécifiques aux différents types de changements
+        if self.change_type == "acteur_cluster":
+            if not isinstance(self.change_data, dict):
+                raise ValueError("acteur_cluster: change_data doit être un dict pour ")


Remarque : Je pense qu'il manque un bout du commentaire ici

fabienheureux · 2024-12-16T19:43:34Z

qfdmo/models/__init__.py

@@ -1,6 +1,7 @@
 from .acteur import *  # noqa
 from .action import *  # noqa
 from .categorie_objet import *  # noqa
+from .change_suggestion import *  # noqa


Suggestion : ici on suit ce qui se faisait, mais tant qu'à faire ignorons explicitement la règle qui nous ennuie

Suggested change

from .change_suggestion import * # noqa

from .change_suggestion import * # noqa: F403

Cf https://www.flake8rules.com/rules/F403.html

fabienheureux · 2024-12-16T19:43:48Z

qfdmo/admin/change_suggestion.py

+
+
+class ChangeSuggestionAdmin(admin.ModelAdmin):
+    # Pour l'instant on consèrve le modèle admin par défaut


Suggested change

# Pour l'instant on consèrve le modèle admin par défaut

# Pour l'instant on conserve le modèle admin par défaut

fabienheureux

J'ai fait quelques retours sur la forme (non bloquants).
Sur le fond, les séries de champs préfixés me font me dire qu'on aurait presque intérêt à avoir deux modèles distincts, change et review :

Un changement peut avoir n reviews
Une fois que toutes les reviews sont fermées / validées, on passe le changement en approuvé

Mais j'ai peut-être mal compris quelque chose ?

Concernant le JSONField, aucune objection si ce n'est que un jour on doit migrer des données (via des migrations Django), les JSONField demandent pas mal de travail, donc il faudra être vigilant à la rétrocompatibilité si on construit une UI autour de ces champs.

Mais globalement je trouve ça top !

fabienheureux · 2024-12-16T19:57:14Z

qfdmo/models/change_suggestion.py

+"""
+Modèle de données Django pour les suggestions de changements
+"""
+


Suggested change

"""

Modèle de données Django pour les suggestions de changements

"""

Suggestion : je pense que c'est un reliquat, le commentaire est répété plus bas en docstring de la classe

fabienheureux · 2024-12-16T20:00:29Z

qfdmo/models/change_suggestion.py

+        if not isinstance(self.change_data, (dict, list)):
+            raise ValueError("change_data doit être un dict ou list")


Question : c'est possible ça ? Que la valeur ne soit pas du bon type ?
Je pense que le validateur django (du fait de l'utilisation du JSONField) va exploser de lui même si tu tentes de lui passer autre chose. Mais à tester quand même

Suggestion : Aussi, comme on est dans clean, est-ce qu'on ne lèverait pas plutôt une ValidationError
Cf https://docs.djangoproject.com/en/5.1/ref/forms/validation/#raising-validationerror

fabienheureux · 2024-12-16T20:12:27Z

qfdmo/models/change_suggestion.py

+            raise ValueError("change_data doit être un dict ou list")
+
+        # Validations spécifiques aux différents types de changements
+        if self.change_type == "acteur_cluster":


Suggestion :
Je ne me rends pas compte du nombre de change_type qu'on va gérer à terme, mais en prévision d'un nombre approchant la dizaine, je serais assez partisan de sortir la validation du modèle, et mapper ça tout en haut de notre module, car plus je review et moins c'est lisible pour moi ce qu'on fait.

Genre quelque chose comme

# models.py from .validators import validate_acteur_cluster # ... change_data_validators = { "acteur_cluster": validate_acteur_cluster } # ... class ChangeSuggestion(models.Model): def clean(self): # ... try: change_data_validators[self.change_type](self.change_data) except KeyError: raise NotImplementedError(f"Pas de gestion change_data pour: {self.change_type}")

# validators.py def validate_acteur_cluster(value): if not isinstance(value, dict): raise ValidationError("acteur_cluster: change_data doit être un dict pour ") acteur_id = value.get("acteur_id") cluster_id = value.get("cluster_id") if not acteur_id: raise ValidationError("acteur_cluster: change_data à besoin de acteur_id") if not cluster_id: raise ValidationError("acteur_cluster: change_data à besoin de cluster_id")

fabienheureux · 2024-12-16T20:17:32Z

qfdmo/models/change_suggestion.py

+        verbose_name="Type de changement",
+    )
+    change_data = models.JSONField(
+        verbose_name=dedent(


Suggestion (à tester) : les champs Django disposent d'un attribut validators auquel tu peux passer des validateurs sur ton champ.

Cf ce commentaire je me demande si ce n'est pas ce qu'on voudrait faire ici

au save, executer un validateur custom sur notre champ

faire redescendre toute la logique de validation au niveau du champ lui-même

Je précise à tester car je n'ai jamais testé avec un JSONField, mais je ne vois pas pourquoi ça ne pourrait pas fonctionner.
C'est un simple callable qu'on passe à notre définition de champ, qui sera appelé durant la validation du champ côté python

initial commit

8eeabc5

kolok force-pushed the change_suggestion_v1 branch from 42a0906 to 8eeabc5 Compare December 16, 2024 16:11

maxcorbeau mentioned this pull request Dec 16, 2024

🚀 CLUSTERING: DAG airflow pour générer les suggestions #1145

Draft

3 tasks

Merge branch 'main' into change_suggestion_v1

b8c1856

fabienheureux reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎁 Suggestion de changement v1 #1141

🎁 Suggestion de changement v1 #1141

maxcorbeau commented Dec 16, 2024 •

edited

Loading

fabienheureux Dec 16, 2024 •

edited

Loading

fabienheureux Dec 16, 2024 •

edited

Loading

fabienheureux Dec 16, 2024 •

edited

Loading

fabienheureux Dec 16, 2024 •

edited

Loading

fabienheureux Dec 16, 2024

fabienheureux Dec 16, 2024

fabienheureux Dec 16, 2024

fabienheureux left a comment •

edited

Loading

fabienheureux Dec 16, 2024

fabienheureux Dec 16, 2024

fabienheureux Dec 16, 2024 •

edited

Loading

fabienheureux Dec 16, 2024

	la vollée (ex: au moment de l'affichage ou de l'export)
	la volée (ex: au moment de l'affichage ou de l'export)

	from .change_suggestion import * # noqa
	from .change_suggestion import * # noqa: F403



		class ChangeSuggestionAdmin(admin.ModelAdmin):
		# Pour l'instant on consèrve le modèle admin par défaut

	# Pour l'instant on consèrve le modèle admin par défaut
	# Pour l'instant on conserve le modèle admin par défaut

	"""
	Modèle de données Django pour les suggestions de changements
	"""

		if not isinstance(self.change_data, (dict, list)):
		raise ValueError("change_data doit être un dict ou list")

🎁 Suggestion de changement v1 #1141

Are you sure you want to change the base?

🎁 Suggestion de changement v1 #1141

Conversation

maxcorbeau commented Dec 16, 2024 • edited Loading

🎁 Suggestion de changement v1

Approche

➡️ Restant à faire

fabienheureux Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

fabienheureux Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

fabienheureux Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

fabienheureux Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

fabienheureux Dec 16, 2024

Choose a reason for hiding this comment

fabienheureux Dec 16, 2024

Choose a reason for hiding this comment

fabienheureux Dec 16, 2024

Choose a reason for hiding this comment

fabienheureux left a comment • edited Loading

Choose a reason for hiding this comment

fabienheureux Dec 16, 2024

Choose a reason for hiding this comment

fabienheureux Dec 16, 2024

Choose a reason for hiding this comment

fabienheureux Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

fabienheureux Dec 16, 2024

Choose a reason for hiding this comment

maxcorbeau commented Dec 16, 2024 •

edited

Loading

fabienheureux Dec 16, 2024 •

edited

Loading

fabienheureux Dec 16, 2024 •

edited

Loading

fabienheureux Dec 16, 2024 •

edited

Loading

fabienheureux Dec 16, 2024 •

edited

Loading

fabienheureux left a comment •

edited

Loading

fabienheureux Dec 16, 2024 •

edited

Loading