Administration: correction des formulaire d'ajout et de modification de diagnostic via l'admin by tonial · Pull Request #5439 · gip-inclusion/les-emplois

tonial · 2025-01-21T13:09:12Z

🤔 Pourquoi ?

L'idée est de permettre la régularisation par le support sans avoir à détourner les comptes, et en pouvant mettre un commentaire sur le diag créé.

Ça a l'inconvénient de dupliquer un peu la vérification des informations, mais je ne suis pas sur qu'envoyer sur les méthodes create_eligibility_diagnosis ou create_diagnosis soit mieux.

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

Mettre à jour le CHANGELOG_breaking_changes.md ?

🏝️ Comment tester

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

github-actions · 2025-01-21T13:19:10Z

🥁 La recette jetable est prête ! 👉 Je veux tester cette PR !

xavfernandez · 2025-01-23T10:55:18Z

itou/eligibility/admin.py

J'ai l'impression qu'il y a plus que la correction du formulaire dans ce commit 🕵️

J'ai modifié le titre : des formulaires d'ajout et de modification :)

Le limit_choices_to c'est aussi pour corriger le formulaire en vérifiant les données saisies

itou/eligibility/admin_form.py

francoisfreitag

Le fichier de tests donne très envie d’écrire un gros parametrize 😇

itou/eligibility/models/geiq.py

francoisfreitag · 2025-01-23T13:48:51Z

itou/eligibility/admin.py

        super().clean()
-        for line in self.cleaned_data:
-            if line["DELETE"] and line["id"].certified:
+        for line in getattr(self, "cleaned_data", []):


Revert cette ligne ne cause aucun échec de test, j’ai l’impression que c’est inutile?

Là encore impossible de retrouver comment j'avais fait planter avant de corriger cette ligne.
C'est peut être encore un autre élément du formulaire modifié après coup qui a corriger l'origine du self.cleaned_data==None

itou/eligibility/admin_form.py

francoisfreitag · 2025-01-23T14:28:03Z

itou/eligibility/models/geiq.py

+        author_geiq = getattr(self, "author_geiq", None)
+        job_seeker = getattr(self, "job_seeker", None)
+
+        if author_geiq and self.author_geiq.kind != CompanyKind.GEIQ:


Tu peux utiliser self.author_geiq_id pour vérifier si le champ auteur_geiq est rempli sans requête SQL (ni RelatedObjectDoesNotExist).

idem, je garde l'idée sous la main si jamais l'erreur revient

francoisfreitag · 2025-01-23T14:29:32Z

tests/eligibility/test_admin.py

+            "selected_administrative_criteria-TOTAL_FORMS": "1",
+            "selected_administrative_criteria-0-id": "",
+            "selected_administrative_criteria-0-eligibility_diagnosis": "",
+            "selected_administrative_criteria-0-administrative_criteria": "1",


Il serait plus juste d’utiliser une vraie PK. Il se trouve effectivement que la PK 1 existe, mais on devrait éviter de le supposer.

C'est défini en dur dans administrative_criteria.json
Je peux ajouter un commentaire l'expliquant ou effectivement appeler AdministrativeCriteria.objects.first().pk à la place

tests/eligibility/test_admin.py

tonial · 2025-01-23T14:47:23Z

J'ai envisagé l'enorme paramétrize, et j'ai eu la flemme parce qu'il y a quand même pas mal de choses qui changent d'un cas à l'autre (et c'est dommage).

Je vais regarder une fois de plus 🙈

tonial · 2025-01-23T22:28:21Z

Je confirme que c'était un poil pénible à mettre en place, mais c'est bon, les tests et la factory sont ré-écrit pour mutualiser le code

tonial · 2025-01-23T22:38:49Z

Merci @francoisfreitag pour tous ces commentaires très pertinents.
J'ai repris presque tout le code du coupe :D

Comme la complexité c'était surtout d'écrire la logique du formulaire et des tests, mutualiser le code était plus simple.

tests/eligibility/test_admin.py

francoisfreitag

C’est top, merci !

On peut saisir à la fois une SIAE autrice et une organisation prescriptrice. Pourquoi ne pas forcer l’un ou l’autre ?

(ça pourrait idéalement être un Check, à voir à quel point nos données actuelles sont pourries et si le nettoyage est raisonnable (genre set null en se basant sur le kind?))

Si on veut peaufiner, je pense qu’on doit pouvoir indiquer à l’admin has_delete_permission pour les éléments du formset, pour éviter de proposer un checkbox Supprimer ? qui affiche une erreur si on la sélectionne. Mais l’état actuel est acceptable.

tonial · 2025-01-24T15:43:31Z

On peut saisir à la fois une SIAE autrice et une organisation prescriptrice. Pourquoi ne pas forcer l’un ou l’autre ?

tu as raison, je vais mettre un check pour éviter d'utiliser le check constraints du modèle

Pour le le formset, je n'ai pas trouvé comment faire. Je vais regarder un peu plus

francoisfreitag · 2025-01-24T15:48:47Z

tu as raison, je vais mettre un check pour éviter d'utiliser le check constraints du modèle

Je n’ai pas eu d’erreur en entrant des données invalides, django a accepté de les sauvegarder. Le check ne doit pas fonctionner.

Je regarde.

francoisfreitag · 2025-01-24T15:57:08Z

C’est bien simple, il n’y a pas de check actuellement. On devrait en rajouter un.

tonial · 2025-01-24T15:58:44Z

Ah ! il y a bien une contrainte db mais uniquement sur les geiq :D

Nice argument !

xavfernandez · 2025-01-27T12:20:03Z

Ah ! il y a bien une contrainte db mais uniquement sur les geiq :D

Ça serait pas mal de mettre la même contrainte pour l'IAE non ?

tonial · 2025-01-27T13:58:40Z

Ce serait sans doute mieux, mais les données de la DB ne le permettent pas trop pour le moment -> https://gip-inclusion.slack.com/archives/C01AQKD7MAN/p1737986163451139

tonial added 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC modifié Modifié dans le changelog. bug labels Jan 21, 2025

tonial self-assigned this Jan 21, 2025

tonial removed the 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC label Jan 21, 2025

tonial force-pushed the alaurent/admin_diags branch 4 times, most recently from cd0ff03 to de1cff4 Compare January 21, 2025 21:54

tonial requested review from francoisfreitag and leo-naeka January 21, 2025 21:54

tonial force-pushed the alaurent/admin_diags branch 3 times, most recently from fbb9df6 to 3790fcd Compare January 22, 2025 21:17

tonial requested a review from xavfernandez January 22, 2025 21:22

tonial force-pushed the alaurent/admin_diags branch 2 times, most recently from 4ec1a83 to 0eb6867 Compare January 23, 2025 08:50

xavfernandez reviewed Jan 23, 2025

View reviewed changes

tonial changed the title ~~Administration: correction des formulaire d'ajout de diagnostic via l'admin~~ Administration: correction des formulaire d'ajout et de modification de diagnostic via l'admin Jan 23, 2025

tonial force-pushed the alaurent/admin_diags branch from 8e5a711 to 57eccc1 Compare January 23, 2025 12:34

tonial requested a review from xavfernandez January 23, 2025 12:37

tonial force-pushed the alaurent/admin_diags branch from 57eccc1 to ea1b042 Compare January 23, 2025 12:56

francoisfreitag reviewed Jan 23, 2025

View reviewed changes

tonial force-pushed the alaurent/admin_diags branch from ea1b042 to af9af40 Compare January 23, 2025 22:37

tonial requested a review from francoisfreitag January 23, 2025 22:38

tonial force-pushed the alaurent/admin_diags branch from af9af40 to a157ffe Compare January 24, 2025 05:04

tonial force-pushed the alaurent/admin_diags branch from a157ffe to 1c0515b Compare January 24, 2025 05:28

francoisfreitag mentioned this pull request Jan 24, 2025

Utiliser une socket UNIX pour se connecter à la base de données en dev #5455

Merged

tonial force-pushed the alaurent/admin_diags branch from 1c0515b to cfe0617 Compare January 24, 2025 10:10

xavfernandez reviewed Jan 24, 2025

View reviewed changes

tests/eligibility/test_admin.py Outdated Show resolved Hide resolved

tonial force-pushed the alaurent/admin_diags branch from cfe0617 to 902b4d6 Compare January 24, 2025 13:25

francoisfreitag reviewed Jan 24, 2025

View reviewed changes

tonial added 3 commits January 24, 2025 21:52

eligibility: Add limit_choice_to to EligibilityDiagnosis.author_siae

3ef013d

Nice argument !

eligibility: Fix admin add form

b3de273

fixup : add check

1feddbf

tonial force-pushed the alaurent/admin_diags branch from 902b4d6 to 1feddbf Compare January 24, 2025 20:52

tonial requested review from francoisfreitag and xavfernandez January 27, 2025 10:20

xavfernandez approved these changes Jan 27, 2025

View reviewed changes

tonial added this pull request to the merge queue Jan 27, 2025

Merged via the queue into master with commit f1cc241 Jan 27, 2025

tonial deleted the alaurent/admin_diags branch January 27, 2025 14:50

Conversation

tonial commented Jan 21, 2025

🤔 Pourquoi ?

🍰 Comment ?

🚨 À vérifier

🏝️ Comment tester

💻 Captures d'écran

Uh oh!

github-actions bot commented Jan 21, 2025

Uh oh!

xavfernandez Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

tonial Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

francoisfreitag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

francoisfreitag Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

tonial Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

francoisfreitag Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

tonial Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

francoisfreitag Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

tonial Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonial commented Jan 23, 2025

Uh oh!

tonial commented Jan 23, 2025

Uh oh!

tonial commented Jan 23, 2025

Uh oh!

Uh oh!

francoisfreitag left a comment

Choose a reason for hiding this comment

Uh oh!

tonial commented Jan 24, 2025

Uh oh!

francoisfreitag commented Jan 24, 2025

Uh oh!

francoisfreitag commented Jan 24, 2025

Uh oh!

tonial commented Jan 24, 2025

Uh oh!

xavfernandez commented Jan 27, 2025

Uh oh!

tonial commented Jan 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tonial Jan 23, 2025 •

edited

Loading

tonial Jan 23, 2025 •

edited

Loading