Diagnostic d'éligibilité : ajouter un type et supprimer `certifiable` #4540

celine-m-s · 2024-08-09T19:50:26Z

🤔 Pourquoi ?

C'est plus lisible. Il sera plus simple de retrouver les critères vérifiables et d'appeler le bon endpoint en fonction (RSA pour le critère RSA, AAH pour l'AAH et le parent isolé pour PI).
J'ai supprimé AAH et PI dans CAN_BE_CERTIFIED_KINDS car le premier endpoint à être mis en place sera le RSA.

🍰 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

notion-workspace · 2024-08-09T19:52:04Z

[technique] Ajouter un type et supprimer certifiable

xavfernandez

👍 avec quelques remarques sur les codes utilisés: comme la plupart sont des abréviations françaises, ne serait-il pas cohérent de rester dans le français ?

itou/eligibility/enums.py

tests/eligibility/test_iae.py

rsebille · 2024-08-14T16:19:43Z

itou/eligibility/admin.py

    @admin.display(boolean=True, description="vérifiable par l'API Particuliers")
    def certifiable(self, obj):
-        return obj.administrative_criteria.certifiable
+        return obj.administrative_criteria.certifiable()


J'ai le cerveau cramé mais je comprend pas ce changement, le champ est supprimé et rien ne le remplace 🤔.

C'est ce certifiable là qui est appelé:

les-emplois/itou/eligibility/models/common.py

Lines 103 to 104 in 444d56d

def certifiable(self):

return self.filter(self.certifiable_lookup)

itou/eligibility/admin.py

xavfernandez · 2024-09-05T09:01:17Z

itou/eligibility/admin.py

    @admin.display(boolean=True, description="vérifiable par l'API Particuliers")
    def certifiable(self, obj):
-        return obj.administrative_criteria.certifiable
+        return obj.administrative_criteria.certifiable()


C'est ce certifiable là qui est appelé:

les-emplois/itou/eligibility/models/common.py

Lines 103 to 104 in 444d56d

def certifiable(self):

return self.filter(self.certifiable_lookup)

itou/eligibility/admin.py

xavfernandez · 2024-09-05T09:05:01Z

itou/eligibility/enums.py

    GEIQ = "geiq", "GEIQ"
+
+
+class AdministrativeCriteriaKind(models.TextChoices):


Vu qu'on bikeshed sur les noms, je me demande si on ne devrait pas juste rajouter RSA & AUTRE pour le moment ? Voir AAH et PI en plus vu qu'on sait qu'on va bientôt les rajouter.

L'ajout des autres se fera au fur et à mesure des besoins. Comme il suffit de rajouter une clef dans l'enum et de mettre à jour les fichiers json, ça pourra très bien se faire au fil de l'eau...

C'est vrai mais maintenant que c'est fait, ce serait plus long de revenir en arrière, non ? J'aime autant garder tous les critères en tant qu'enum à un seul et même endroit, c'est plus clair.

Ça ne devrait pas être très long de supprimer des valeurs et mettre OTHER/AUTRE :)
Car là il y a de bonnes chances que sur les 29 valeurs ajoutées, un certain nombre ne servent pas tout de suite (voir jamais ?)
Mais ça reste du bikeshed, donc on peut aussi partir sur les valeurs définies.

Je pense que j'avais mal compris.

Si je comprends bien, tu proposes d'ajouter en tant qu'enum uniquement les valeurs utiles à la certification (donc RSA uniquement pour la moment), étant donné que cette classe ne sert qu'à cela. C'est bien ça ?
Alors en effet, ce n'est pas long du tout mais cela me pose un souci de cohérence. L'ajout de cette colonne servait à mieux filtrer les critères par type, pas forcément et uniquement pour la certification. Ce ne serait pas bizarre d'avoir RSA et AUTRE, alors qu'on en affiche bien plus à l'utilisateur ?

Oui c'était bien cela. Mais ça me va également de partir là dessus :)

celine-m-s · 2024-09-05T13:37:11Z

itou/eligibility/data/administrative_criteria_geiq.json

 			"parent_id": null,
 			"annex": "1",
 			"level": null,
 			"slug": "beneficiaire-des-minimas-sociaux",


Je n'ai pas touché au slug car les tests cassaient quand j'ai voulu le faire (et pas envie de me lancer dans ça maintenant).

xavfernandez

Est-ce qu'on ne voudrait pas un test qui vérifie la cohérence entre les kinds des fixtures et les valeurs de enums (globalement appeler AdministrativeCriteriaKind(criteria.kind) sur toutes les critères ?

xavfernandez · 2024-09-05T14:10:09Z

itou/eligibility/enums.py

    GEIQ = "geiq", "GEIQ"
+
+
+class AdministrativeCriteriaKind(models.TextChoices):


Oui c'était bien cela. Mais ça me va également de partir là dessus :)

xavfernandez · 2024-09-05T14:13:45Z

itou/eligibility/enums.py

+    # Only for the migration
+    AUTRE = "AUTRE", "Autre"


Je pense que tu peux directement supprimer cette valeur et mettre "" en default. Le chargement des fixtures remplira très vite le champs.

celine-m-s · 2024-09-06T09:28:03Z

Est-ce qu'on ne voudrait pas un test qui vérifie la cohérence entre les kinds des fixtures et les valeurs de enums (globalement appeler AdministrativeCriteriaKind(criteria.kind) sur toutes les critères ?

Merci pour la suggestion. Deux critères s'étaient faufilés mais ont été repérés grâce à ce nouveau test ! 🕵️

Je pousse la mise à jour et je MEP.

celine-m-s self-assigned this Aug 9, 2024

celine-m-s added the modifié Modifié dans le changelog. label Aug 9, 2024

celine-m-s mentioned this pull request Aug 9, 2024

[Candidature] Certification du critère BRSA avec l'API Particulier #4524

Merged

1 task

xavfernandez approved these changes Aug 12, 2024

View reviewed changes

itou/eligibility/enums.py Outdated Show resolved Hide resolved

itou/eligibility/enums.py Outdated Show resolved Hide resolved

itou/eligibility/enums.py Outdated Show resolved Hide resolved

itou/eligibility/enums.py Outdated Show resolved Hide resolved

xavfernandez requested a review from rsebille August 14, 2024 14:44

rsebille reviewed Aug 14, 2024

View reviewed changes

celine-m-s force-pushed the celinems/eligibility_kinds branch from 904c41d to 1c623e4 Compare September 4, 2024 15:38

xavfernandez reviewed Sep 5, 2024

View reviewed changes

celine-m-s commented Sep 5, 2024

View reviewed changes

celine-m-s force-pushed the celinems/eligibility_kinds branch from 0c9faa3 to 941db74 Compare September 5, 2024 13:47

celine-m-s requested review from rsebille and xavfernandez September 5, 2024 13:48

xavfernandez approved these changes Sep 5, 2024

View reviewed changes

celine-m-s force-pushed the celinems/eligibility_kinds branch from 941db74 to 5667066 Compare September 6, 2024 09:28

celine-m-s enabled auto-merge September 6, 2024 09:30

Add a kind to Eligibility Diagnosis

2266e5b

celine-m-s force-pushed the celinems/eligibility_kinds branch from 5667066 to 2266e5b Compare September 6, 2024 09:43

celine-m-s added this pull request to the merge queue Sep 6, 2024

Merged via the queue into master with commit a2b2fd3 Sep 6, 2024

celine-m-s deleted the celinems/eligibility_kinds branch September 6, 2024 09:59

	def certifiable(self):
	return self.filter(self.certifiable_lookup)

		GEIQ = "geiq", "GEIQ"


		class AdministrativeCriteriaKind(models.TextChoices):

Diagnostic d'éligibilité : ajouter un type et supprimer certifiable #4540

Diagnostic d'éligibilité : ajouter un type et supprimer certifiable #4540

Uh oh!

Conversation

celine-m-s commented Aug 9, 2024

🤔 Pourquoi ?

🍰 Comment ?

🚨 À vérifier

🏝️ Comment tester

💻 Captures d'écran

Uh oh!

notion-workspace bot commented Aug 9, 2024

Uh oh!

xavfernandez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

celine-m-s Sep 5, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xavfernandez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

celine-m-s commented Sep 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Diagnostic d'éligibilité : ajouter un type et supprimer `certifiable` #4540

Diagnostic d'éligibilité : ajouter un type et supprimer `certifiable` #4540

celine-m-s Sep 5, 2024 •

edited

Loading