Skip to content
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

Ajout des actions pour valider ou rejeter les suggestions ligne par ligne #1267

Merged
merged 11 commits into from
Feb 4, 2025

Conversation

kolok
Copy link
Contributor

@kolok kolok commented Jan 27, 2025

Description succincte du problème résolu

Carte Notion : Titre de la carte

CleanShot 2025-01-27 at 16 33 07

Type de changement :

  • Bug fix
  • Nouvelle fonctionnalité
  • Mise à jour de données / DAG
  • Les changements nécessitent une mise à jour de documentation
  • Refactoring de code (explication à retrouver dans la description)

Auto-review

Les trucs à faire avant de demander une review :

  • J'ai bien relu mon code
  • La CI passe bien
  • En cas d'ajout de variable d'environnement, j'ai bien mis à jour le .env.template
  • J'ai ajouté des tests qui couvrent le nouveau code

Comment tester

En local / staging :

@kolok kolok force-pushed the admin_actions branch 4 times, most recently from 62f68b4 to 78144a9 Compare February 3, 2025 13:59
@kolok kolok marked this pull request as ready for review February 3, 2025 13:59
@kolok kolok requested a review from a team as a code owner February 3, 2025 13:59
@kolok kolok requested review from fabienheureux and maxcorbeau and removed request for a team February 3, 2025 13:59
data/admin.py Outdated Show resolved Hide resolved
Copy link
Member

@fabienheureux fabienheureux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pour moi on mélange un peu les choses entre

  • des update qui pourraient vivre sur un manager custom
  • du rendering dans le modèle qui gagnerait à être déplacé dans la vue (admin ici)

Aussi il serait intéressant de rendre plus flexible les templates d'admin.

Ok pour en discuter s'il faut merger asap et voir ce qu'on peut faire comme compromis, mais avec les ambitions sur l'admin / ajout de wagtail, c'est important qu'on limite l'ajout de dette technique

data/models/suggestion.py Outdated Show resolved Hide resolved
data/models/suggestion.py Outdated Show resolved Hide resolved
data/models/suggestion.py Show resolved Hide resolved
data/admin.py Outdated Show resolved Hide resolved
data/admin.py Show resolved Hide resolved
data/admin.py Show resolved Hide resolved
data/models/suggestion.py Outdated Show resolved Hide resolved
Copy link
Contributor

@maxcorbeau maxcorbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GO, on va vers le mieux c'est sur 🚀

Super:

  • db_apply_suggestion.py: super car 0 logique spécifique aux changements des suggestions dans Airflow (on prend 1 suggestion et on applique apply)

Pistes d'améliorations:

  • cas à gérer: le cas où Airflow a échoué et donc les suggestions restents inachevée EN COURS
  • logique django admin: à mon avis bcp trop de logique spécifique aux changements, à migrer vers les modèles de suggestions/changements pour être agnostic
  • db_read_acteur.py: toute la logique de recréer touts les propositions/sous-propositions etc.... pourrait être déplacer vers les modèles django, pour avoir le moins de custom possible dans Airflow
  • génération des df: à faire valider par les modèles de suggestion
  • messages d'actions: ajouter un compteur pour + de feedback métier
  • cohorte: idée: à mettre en read-only d'un point de vue des suggestions sous-jacentes (à prendre ou à laisser mais on vient pas les changer une fois générées)

Grosse PR à venir:

  • PR de refacto sur cet existant + intégration clustering, sur laquelle je m'attellerai la semaine prochaine, et je pense que se faire une session sync sera utile

@kolok
Copy link
Contributor Author

kolok commented Feb 3, 2025

Pour moi on mélange un peu les choses entre

* des update qui pourraient vivre sur un manager custom

* du rendering dans le modèle qui gagnerait à être déplacé dans la vue (admin ici)

Aussi il serait intéressant de rendre plus flexible les templates d'admin.

Ok pour en discuter s'il faut merger asap et voir ce qu'on peut faire comme compromis, mais avec les ambitions sur l'admin / ajout de wagtail, c'est important qu'on limite l'ajout de dette technique

Merci pour ce commentaire @fabienheureux

C'est parce qu'on est dans un état transitoire avant d'utiliser des classes PYDANTIC sur lesquelles on s'appuyera pour résoudre tout ça au bon endroit. Donc sur toute cette partie, je l'ai fait un peu brute force, tout va être revu avec la prochaine tache de @maxcorbeau

J'aurai besoin de faire un peu de pair coding avec toi pour bien maitriser les Manager

Je te propose de débloquer cette PR même si je n'ai pas encore fini d'intégrer certains commentaires (entre autre sur la geston des template). Je finirait ça avant de merger

Copy link
Member

@fabienheureux fabienheureux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je m'en doutais un peu idéalement on aurait fait ça en pair mais j'ai manqué de temps aujourd'hui

Aucun souci pour moi je valide, on verra ensemble si besoin de pérenniser les méthodes ou interfaces pour faire ça différemment

@kolok kolok merged commit 9ca4bae into main Feb 4, 2025
11 checks passed
@kolok kolok deleted the admin_actions branch February 4, 2025 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants