PASS IAE: La date de début du contrat doit être avant la date de fin du PASS [GEN-2225] #5248

calummackervoy · 2024-12-11T13:26:06Z

🤔 Pourquoi ?

En ce moment c'est possible d'embaucher un candidat après son PASS IAE est expiré, car notre logique cible timezone.now() quand il devrait cibler hiring_start_at de la poste.

🍰 Comment ?

J'ai extendé les utilités des modèles et ses managers à considérer la date d'embauche où nécessaire.

J'ai rélu la logique autor des approbations et des diagnostiques d'éligibilité et j'ai fais des fixes impliqués par la cause du bug (que le hiring_start_at est impliqué dans la validité du pass).

Le champ expires_at utilise DateTimeField mais hiring_start_at du JobApplication utilise DateField. J'ai inclus un fonction utilitaire make_date_timezone_aware pour soutenir le gestion des dates.

Finalement j'ai fais quelques petites optimisations sur les queries, qui a causé des groses changements aux snapshots.

🚨 À vérifier

Mettre à jour le CHANGELOG_breaking_changes.md ?

Useful when providing a DateField to a query on the DateTimeField of another field/model Refactor make_date_timezone_aware to use duck-typing

…igibility Extend tests for PASS IAE validity

calummackervoy · 2024-12-11T13:30:47Z

itou/eligibility/models/iae.py

+        last_valid_diagnosis = self.last_considered_valid(job_seeker, for_siae=for_siae)
+        return job_seeker.has_valid_approval_for_date(on_date) or (
+            last_valid_diagnosis and last_valid_diagnosis.expires_at > make_date_timezone_aware(on_date)
+        )

    def last_considered_valid(self, job_seeker, for_siae=None, prefetch=None):


last_considered_valid m'a posé une colle. Avec la description du méthode, il me semble que c'est la dernière diagnostique aujourd'hui pour le candidat, et que je devrais ignorer le fonction dans ce PR.

Par contre, c'est utilisé dans le modèle JobApplication, dans get_eligibility_diagnosis :

return EligibilityDiagnosis.objects.last_considered_valid(self.job_seeker, for_siae=self.to_company)

Donc c'est possible que ce fonction devrait produire une éligibilité qui est valide pour la date de début d'embauche

Pour moi, tu devrais ajouter un on_date= dans last_considered_valid() car c'est utilisé dans hire_confirmation()

calummackervoy · 2024-12-11T13:34:29Z

tests/www/apply/test_list_for_siae.py

@@ -423,6 +423,14 @@ def test_list_for_siae_filtered_by_eligibility_validated(self, client):
        response = client.get(reverse("apply:list_for_siae"), {"eligibility_validated": True})
        assert response.context["job_applications_page"].object_list == []

+        # Not expired, but will be when the job starts
+        diagnosis.expires_at = timezone.now() + datetime.timedelta(days=1)


Dans une autre assertion de ce test nous avons :

# Make sure the diagnostic expired - it should be ignored diagnosis.expires_at = timezone.now() - datetime.timedelta(days=diagnosis.EXPIRATION_DELAY_MONTHS * 31 + 1)

Pourquoi on utilise EXPIRATION_DELAY_MONTHS ici ? Ce n'est pas considéré expiré si c'est un jour après le expires_at ?

xavfernandez · 2024-12-17T07:36:40Z

itou/utils/date.py

@@ -6,3 +6,15 @@
 def monday_of_the_week(value=None):
    the_day = timezone.localdate(value)
    return the_day - datetime.timedelta(days=the_day.weekday())
+
+
+def make_date_timezone_aware(value, tz=None):


Je ne suis pas certain de saisir l'utilité de cette fonction, on ne devrait avoir que des datetime avec timezone dans notre base de code et notre base de données.
Reste la conversion de date vers datetime: le mieux serait sûrement de ne pas avoir à les comparer et pour cela #5210 me semble une meilleure solution et devrait faciliter cette PR.

tonial

Je rejoins le commentaire de @xavfernandez sur le make_date_timezone_aware.

Pour le reste, par contre, c'est top 👍

tonial · 2024-12-17T05:34:47Z

itou/eligibility/models/iae.py

+        last_valid_diagnosis = self.last_considered_valid(job_seeker, for_siae=for_siae)
+        return job_seeker.has_valid_approval_for_date(on_date) or (
+            last_valid_diagnosis and last_valid_diagnosis.expires_at > make_date_timezone_aware(on_date)
+        )


C'est dommage : avec ton changement on fait les requêtes de last_considered_valid même si job_seeker.has_valid_approval_for_date(on_date) est True

Suggested change

last_valid_diagnosis = self.last_considered_valid(job_seeker, for_siae=for_siae)

return job_seeker.has_valid_approval_for_date(on_date) or (

last_valid_diagnosis and last_valid_diagnosis.expires_at > make_date_timezone_aware(on_date)

)

if job_seeker.has_valid_approval_for_date(on_date):

return True

last_valid_diagnosis = self.last_considered_valid(job_seeker, for_siae=for_siae)

return last_valide_diagnosis and last_valid_diagnosis.expires_at > make_date_timezone_aware(on_date)

)

tonial · 2024-12-17T05:37:00Z

itou/eligibility/models/iae.py

+        last_valid_diagnosis = self.last_considered_valid(job_seeker, for_siae=for_siae)
+        return job_seeker.has_valid_approval_for_date(on_date) or (
+            last_valid_diagnosis and last_valid_diagnosis.expires_at > make_date_timezone_aware(on_date)
+        )

    def last_considered_valid(self, job_seeker, for_siae=None, prefetch=None):


Pour moi, tu devrais ajouter un on_date= dans last_considered_valid() car c'est utilisé dans hire_confirmation()

tonial · 2024-12-17T07:15:03Z

itou/job_applications/models.py

-        return self.to_company.is_subject_to_eligibility_rules and not self.job_seeker.has_valid_diagnosis(
-            for_siae=self.to_company
+        return (
+            not self.hiring_without_approval


bien vu pour cet ajout

tonial · 2024-12-17T07:16:12Z

itou/users/models.py

@@ -511,6 +512,10 @@ def latest_common_approval(self):

        return self.latest_approval or self.latest_pe_approval

+    def has_valid_approval_for_date(self, on_date):
+        on_date = make_date_timezone_aware(on_date)


quel intérêt de rendre on_date et self.latest_approval.end_at aware pour les comparer.
Ce ne sont pas tous les deux des datetime.date ?

tonial · 2024-12-17T07:41:20Z

tests/eligibility/test_iae.py

+        diagnosis = IAEEligibilityDiagnosisFactory(
+            from_prescriber=True, job_seeker=self.job_seeker, expires_at=expires_at
+        )
+        has_considered_valid = EligibilityDiagnosis.objects.has_considered_valid(job_seeker=diagnosis.job_seeker)


pourquoi ne pas mettre les assert sans variable intermédiaire ?
assert EligibilityDiagnosis.objects.has_considered_valid(job_seeker=diagnosis.job_seeker)

tonial · 2024-12-17T07:42:24Z

tests/www/apply/__snapshots__/test_process.ambr

@@ -957,7 +810,7 @@
 # ---
 # name: TestProcessViews.test_details_for_company_from_approval[job application detail for company]
  dict({
-    'num_queries': 21,
+    'num_queries': 23,


ici 'et à d'autres endroits) on ajoute 2 queries, est-ce normal ?
je n'ai pas creusé, mais c'est sans doute le point que j'ai relevé plus haut sur la requête faite en plus

rsebille

Désolé je fait l'hélicoptère mais vu qu'on en a discuté (longuement) en atelier métier bah j'ai dû y mettre mon nez plus que je le voulais et le changement me semble bizarre par rapport au problème de base.

On se retrouve à faire des modifications profondes au niveau de la logique de validité des diagnostiques et des PASS IAE (et donc de si on en délivre un nouveau ou pas, toussa toussa) alors que le problème c'est de bloquer les employeurs d'accepter une candidature avec une date de début après la date de fin d'un PASS IAE et qui j'ai l'impression (mais je suis peut-être naif) pourrais se régler avec quelques lignes dans JobApplication.accept() :

if self.hiring_start_at > self.approval.end_at:
    raise xwf_models.AbortTransition(...)

et de faire grosso modo la même chose dans le formulaire pour afficher une erreur à l'utilisateur avant même d'arriver dans JobApplication.accept()

rsebille · 2024-12-18T11:27:24Z

itou/www/apply/views/process_views.py

+        if job_seeker.has_valid_diagnosis(for_siae=job_application.to_company, on_date=timezone.now()):
+            messages.error(request, "Le contrat doit débuter sur la période couverte par le PASS IAE.")


timezone.now() ? Ça devrais pas être self.hiring_start_at justement ?

Et le lien de causalité entre has_valid_diagnosis() et le message d'erreur n'est absolument pas clair, je dirais même qu'il n'y a aucun rapport car dans l'un on parle explicitement de "PASS IAE" et pas de "Diagnostique d'élégibilité".

rsebille · 2024-12-18T11:39:49Z

itou/job_applications/models.py

+                EligibilityDiagnosis.objects.for_job_seeker_and_siae(
+                    job_seeker=OuterRef("job_seeker"), siae=OuterRef("to_company")
+                )
+                .filter(expires_at__gt=OuterRef("hiring_start_at"))


hiring_start_at n'est renseignée qu'une fois que la candidature est acceptée par l'employeur donc la comparaison avec NULL sera toujours fausse donc aucun diagnostique ne sera renvoyé tant que le champ n'est pas remplis.

rsebille · 2024-12-18T11:46:14Z

tests/www/apply/test_process.py

+        # required assumptions for the test case
+        assert self.company.is_subject_to_eligibility_rules
+        assert self.job_seeker.has_valid_diagnosis(for_siae=self.company)  # valid today
+        # not valid on the day the job starts
+        assert (
+            not EligibilityDiagnosis.objects.for_job_seeker_and_siae(self.job_seeker, siae=self.company)
+            .valid(job_application.hiring_start_at)
+            .exists()
+        )
+        assert not self.job_seeker.has_valid_diagnosis(
+            for_siae=job_application.to_company, on_date=job_application.hiring_start_at
+        )


Dans les tests, les suppositions ne sont pas à faire avec des assert mais dans la mise en place.

Suggested change

# required assumptions for the test case

assert self.company.is_subject_to_eligibility_rules

assert self.job_seeker.has_valid_diagnosis(for_siae=self.company) # valid today

# not valid on the day the job starts

assert (

not EligibilityDiagnosis.objects.for_job_seeker_and_siae(self.job_seeker, siae=self.company)

.valid(job_application.hiring_start_at)

.exists()

)

assert not self.job_seeker.has_valid_diagnosis(

for_siae=job_application.to_company, on_date=job_application.hiring_start_at

)

assert self.job_seeker.has_valid_diagnosis(for_siae=self.company) # valid today

assert not self.job_seeker.has_valid_diagnosis(

for_siae=job_application.to_company, on_date=job_application.hiring_start_at

) # not valid on the day the job starts

rsebille · 2024-12-18T11:47:27Z

tests/www/apply/test_process.py

+        assert "Le contrat doit débuter sur la période couverte par le PASS IAE." == str(
+            list(response.context["messages"])[-1]
+        )


assertMessages() ?

rsebille · 2024-12-18T11:55:57Z

itou/eligibility/models/iae.py

+        on_date = on_date or timezone.now()
+        last_valid_diagnosis = self.last_considered_valid(job_seeker, for_siae=for_siae)
+        return job_seeker.has_valid_approval_for_date(on_date) or (
+            last_valid_diagnosis and last_valid_diagnosis.expires_at > make_date_timezone_aware(on_date)


A confirmer avec le métier mais pour les diagnostiques il me semble qu'on veux qu'il soit valide au moment où on accepte la candidature et on s'en fiche qu'il le soit encore le jour de l'embauche car il aura un PASS IAE à ce moment là.

calummackervoy added 2 commits December 10, 2024 20:35

Add make_date_timezone_aware date utility

2ab0dff

Useful when providing a DateField to a query on the DateTimeField of another field/model Refactor make_date_timezone_aware to use duck-typing

Job application considers hire_start_at while considering PASS IAE el…

d48a7e0

…igibility Extend tests for PASS IAE validity

calummackervoy added the modifié label Dec 11, 2024

calummackervoy requested a review from xavfernandez December 11, 2024 13:26

calummackervoy self-assigned this Dec 11, 2024

calummackervoy commented Dec 11, 2024

View reviewed changes

calummackervoy added the bug label Dec 11, 2024

francoisfreitag changed the title ~~PASS IAE: La date de début du contrat doit être est après la date de fin du PASS [GEN-2225]~~ PASS IAE: La date de début du contrat doit être avant la date de fin du PASS [GEN-2225] Dec 16, 2024

francoisfreitag self-requested a review December 16, 2024 06:49

xavfernandez reviewed Dec 17, 2024

View reviewed changes

tonial reviewed Dec 17, 2024

View reviewed changes

rsebille reviewed Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PASS IAE: La date de début du contrat doit être avant la date de fin du PASS [GEN-2225] #5248

PASS IAE: La date de début du contrat doit être avant la date de fin du PASS [GEN-2225] #5248

calummackervoy commented Dec 11, 2024

calummackervoy Dec 11, 2024

tonial Dec 17, 2024

calummackervoy Dec 11, 2024

xavfernandez Dec 17, 2024

tonial left a comment

tonial Dec 17, 2024

tonial Dec 17, 2024

tonial Dec 17, 2024

tonial Dec 17, 2024

tonial Dec 17, 2024

tonial Dec 17, 2024

rsebille left a comment

rsebille Dec 18, 2024

rsebille Dec 18, 2024

rsebille Dec 18, 2024

rsebille Dec 18, 2024

rsebille Dec 18, 2024

		if job_seeker.has_valid_diagnosis(for_siae=job_application.to_company, on_date=timezone.now()):
		messages.error(request, "Le contrat doit débuter sur la période couverte par le PASS IAE.")

PASS IAE: La date de début du contrat doit être avant la date de fin du PASS [GEN-2225] #5248

Are you sure you want to change the base?

PASS IAE: La date de début du contrat doit être avant la date de fin du PASS [GEN-2225] #5248

Conversation

calummackervoy commented Dec 11, 2024

🤔 Pourquoi ?

🍰 Comment ?

🚨 À vérifier

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonial left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsebille left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment