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

[GEN-557] Candidature : préparation à la certification AAH #5481

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

celine-m-s
Copy link
Collaborator

@celine-m-s celine-m-s commented Jan 27, 2025

🤔 Pourquoi ?

A l’instar du critère BRSA on veut interroger l’API Particuliers pour certifier le critère AAH.
AAH est un critère pour lequel on demande un justificatif de < de 3 mois.

🍰 Comment ?

Règles identiques au critère RSA.

🚨 À vérifier

  • Mettre à jour le CHANGELOG_breaking_changes.md ?
  • Ajouter l'étiquette « Bug » ?

🏝️ Comment tester ?

En tant que SIAE, réaliser une auto-prescription avec un critère AAH.

💻 Captures d'écran

@celine-m-s celine-m-s self-assigned this Jan 27, 2025
@celine-m-s celine-m-s changed the title Candidature : ajout du critère AAH [GEN-557] Candidature : ajout du critère AAH Jan 30, 2025
@celine-m-s celine-m-s force-pushed the celinems/aah branch 3 times, most recently from cf23844 to d23e09b Compare February 3, 2025 11:01
@celine-m-s celine-m-s added the ajouté Ajouté dans le changelog. label Feb 3, 2025
@celine-m-s celine-m-s added the 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC label Feb 3, 2025
Copy link

github-actions bot commented Feb 3, 2025

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

@celine-m-s celine-m-s marked this pull request as ready for review February 3, 2025 18:54
Copy link
Contributor

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

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

Yaay! Un nouveau critère sera bientôt certifié 😀


Cette PR mériterait d’être découpée.
La revue est par conséquent un peu éparpillée et je trouve dommage de retenir des améliorations qui n’ont pas grand chose à voir avec l’AAH le temps de gérer l’intégration de l’AAH.

tests/eligibility/test_iae.py Outdated Show resolved Hide resolved
itou/eligibility/tasks.py Outdated Show resolved Hide resolved
itou/utils/apis/api_particulier.py Outdated Show resolved Hide resolved
tests/eligibility/test_tasks.py Outdated Show resolved Hide resolved
tests/eligibility/test_tasks.py Outdated Show resolved Hide resolved
tests/eligibility/factories.py Outdated Show resolved Hide resolved
tests/eligibility/test_geiq.py Show resolved Hide resolved
Comment on lines 755 to 753
diagnosis = iae_eligibility_with_criteria_factory(criteria_kind=CRITERIA_KIND)
admin_criterion = AdministrativeCriteria.objects.get(kind=CRITERIA_KIND)
diagnosis.administrative_criteria.add(admin_criterion)
Copy link
Contributor

Choose a reason for hiding this comment

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

On pourra même ajouter ce cas aux nouvelles factories, pour passer une liste de critères ?

Copy link
Collaborator Author

@celine-m-s celine-m-s Feb 11, 2025

Choose a reason for hiding this comment

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

Fait dans le dernier commit. Désolée par avance car plusieurs certifiable=True se sont glissés dans le commit. J'ai tenté de découper avec des git add -p mais les changements étaient trop petits. Je sais que ça va te tendre et je comprends. 😬

Tout est squashé dans le commit c97ac08

tests/job_applications/tests.py Show resolved Hide resolved
tests/www/apply/test_process.py Show resolved Hide resolved
rsebille

This comment was marked as off-topic.

@celine-m-s celine-m-s changed the title [GEN-557] Candidature : ajout du critère AAH [GEN-557] Candidature : préparation à la certification AAH Feb 10, 2025
@celine-m-s
Copy link
Collaborator Author

PR découpée en deux : celle-ci et la #5584 .

@celine-m-s celine-m-s mentioned this pull request Feb 11, 2025
7 tasks
@celine-m-s
Copy link
Collaborator Author

Voilà ! J'ai pris en compte tous tes retours @francoisfreitag . Je travaille demain (mercredi) mais n'hésite pas à tout squasher si tu veux MEP jeudi ou vendredi. Je peux aussi passer une tête le soir pour MEP par moi-même.

Copy link
Contributor

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

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

Un bon coup de ménage. 🧹

Si tu as le temps aujourd’hui, tu peux squasher les commits et les rendre atomiques, minimaux. J’ai aussi laissé quelques commentaires pour des petites améliorations.

Je relirai et intègrerai jeudi.

tests/eligibility/factories.py Show resolved Hide resolved
tests/eligibility/factories.py Outdated Show resolved Hide resolved
Comment on lines 465 to 466
job_seeker__with_address=True,
job_seeker__born_in_france=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

On pourrait utiliser certifiable=True, non ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Le Trait certifiable définit trois paramètres, dont from_employer=True (pour l'IAE) et from_geiq=True (pour les GEIQ) qui sont ici passés en paramètre (factory_params).
Je trouvais plus lisible de simplement définir les paramètres utiles, mais je propose quand même le changement suivant :

        diagnosis = IAEEligibilityDiagnosisFactory(
            certifiable=True,
            criteria_kinds=[CRITERIA_KIND],
            from_employer=factory_params.get("from_employer", False),
            from_prescriber=factory_params.get("from_prescriber", False),
        )
        assert diagnosis.criteria_can_be_certified() == expected

À toi de voir si tu veux le garder. Je le laisse dans un commit séparé.

tests/eligibility/test_iae.py Outdated Show resolved Hide resolved
assert criterion.certification_period == InclusiveDateRange(datetime.date(2024, 8, 1), datetime.date(2024, 12, 13))


@pytest.mark.parametrize(
"EligibilityDiagnosisFactory",
"eligibility_diagnosis_factory",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ce renommage ne me semble pas nécessaire ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Étant donné qu'on passe maintenant une classe et non une fonction, je trouve que c'est plus cohérent. Non ?

tests/eligibility/test_iae.py Outdated Show resolved Hide resolved
Comment on lines 2648 to 2653
# TODO(cms): selecting twice the same criteria cannot happen.
# Better iterate over each criterion.
diagnosis = IAEEligibilityDiagnosisFactory(
job_seeker=self.job_seeker,
author_siae=self.company,
certifiable=True,
criteria_kinds=[CRITERIA_KIND, CRITERIA_KIND],
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Même remarque ici, mieux vaut créer un state valide.

tests/www/apply/test_templates.py Outdated Show resolved Hide resolved
tests/www/apply/test_templates.py Outdated Show resolved Hide resolved
@rsebille
Copy link
Contributor

Je suis preneur d'un petit squash/fixup afin de pouvoir comprendre et réutiliser ces commits, et aussi éviter un rebase de la mort, pour les critères attachés au candidat 👼.

itou/eligibility/tasks.py Outdated Show resolved Hide resolved
tests/www/apply/test_templates.py Outdated Show resolved Hide resolved
@celine-m-s
Copy link
Collaborator Author

Je suis preneur d'un petit squash/fixup afin de pouvoir comprendre et réutiliser ces commits, et aussi éviter un rebase de la mort, pour les critères attachés au candidat 👼.

Oui. :) J'avais laissé tous les Squash me afin que François puisse bien distinguer les changements. Je suis en train de tout fusionner au bon endroit.

Having `GEIQEligibilityDiagnosisFactory.with_certifiable_criteria` choose
randomly from a list of criteria is unpredictable and can cause flaky tests.
Better create objects when needed with a PostGeneration.
Even if the badge is not displayed, a call to the API is made
as checking criteria can be certifiable is made upstream.
@celine-m-s
Copy link
Collaborator Author

C'est à jour @francoisfreitag . 💪 Je n'ai pas touché à la PR qui ajoute le critère (#5584 ) car c'est possible que tu ajoutes des changements à celle-ci demain ou vendredi, or beaucoup du code de cette PR se base sur celui-ci.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC ajouté Ajouté dans le changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants