Demandeur d’emploi : Correction de la sélection de la commune de naissance pour les personnes nées le dernier jour de validité de la commune by rsebille · Pull Request #5227 · gip-inclusion/les-emplois

rsebille · 2024-12-09T12:47:20Z

🤔 Pourquoi ?

Test bagottant : https://github.com/gip-inclusion/les-emplois/actions/runs/12235030153/attempts/1
Reproductible avec pytest --randomly-seed=3864335593 -vv tests/www/employee_record_views/test_create.py::TestCreateEmployeeRecordStep2::test_access_granted

Les valeurs choisies était "1999-12-31" pour la date de naissance alors que la ville était :

  id   | start_date |  end_date  | code  |     name     |          created_at           | city_id 
-------+------------+------------+-------+--------------+-------------------------------+---------
 57089 | 1900-01-01 | 1999-12-31 | 67152 | GEISPOLSHEIM | 2023-10-02 08:19:11.118299+00 |  [NULL]

francoisfreitag

❤️

francoisfreitag · 2024-12-10T10:43:04Z

itou/asp/models.py

        "Lookup a Commune object by INSEE code and valid at the given period"
        return (
            self.filter(code=insee_code, start_date__lte=period)
-            .filter(Q(end_date=None) | Q(end_date__gt=period))


À quand l’utilisation de InclusiveDateRange ?

Un jour. Peut-être. 😁

francoisfreitag · 2024-12-10T10:43:51Z

tests/asp/test_communes.py


-        result = Commune.objects.by_insee_code_and_period(99999, datetime.datetime(2022, 11, 28))
-        assert new_commune == result
+        for period in [new_commune.start_date, datetime.date(2022, 11, 28), timezone.now()]:


Suggested change

for period in [new_commune.start_date, datetime.date(2022, 11, 28), timezone.now()]:

for period in [new_commune.start_date, datetime.date(2022, 11, 28), timezone.localdate()]:

rsebille

@francoisfreitag La correction pour le test en échec au moment de ta relecture est assez velue donc je veux bien un zieutage pour être sûr que c'est compréhensible :).

rsebille · 2024-12-10T11:31:37Z

itou/asp/models.py

        "Lookup a Commune object by INSEE code and valid at the given period"
        return (
            self.filter(code=insee_code, start_date__lte=period)
-            .filter(Q(end_date=None) | Q(end_date__gt=period))


Un jour. Peut-être. 😁

francoisfreitag · 2024-12-10T14:34:44Z

Je n’étais pas satisfait par ta proposition, je viens de pousser un fixup pour clarifier. Effectivement c’est velu.

tests/www/apply/test_process.py

francoisfreitag · 2024-12-12T08:46:59Z

Romain, je te laisse merger si ça te convient ?

xavfernandez · 2024-12-12T10:10:07Z

Ça mériterait peut-être un changelog au final: https://itou-inclusion.slack.com/archives/C01181Y04LT/p1733996418452109 🙈

tests/www/apply/test_process.py

francoisfreitag · 2024-12-12T12:03:18Z

J’ai l’impression qu’on s’est télescopé ?

rsebille · 2024-12-12T12:05:55Z

J’ai l’impression qu’on s’est télescopé ?

Je pense pas, j'ai rien push en tout cas.
Mais j'ai pas compris pourquoi il a vu autant de commit différent par contre 🤷

francoisfreitag · 2024-12-12T12:07:49Z

C’est moi qui utilisait une remote pas à jour dont la branche master avait déviée.

francoisfreitag · 2024-12-12T12:34:14Z

tests/www/apply/test_forms.py

        )
        assert form.is_valid()
-
-    def test_submit_commune_invalid_commune_lookup(self):


Ce test avait le même problème et c’est un doublon moins bien intégré de test_accept_updated_birthdate_invalidating_birth_place. => 🗑️

Je préférerais presque garder celui-ci et virer sa logique de l'autre mais oui 1 seul est suffisant ;).

Je préfère garder les tests d’intégration, qui couvrent mieux la façon dont les données arrivent au formulaire et limitent le risque de couvrir des cas impossible en réalité. 🤷

code | start_date | end_date -------+------------+------------ 01001 | 1900-01-01 | 1999-12-31 01001 | 2000-01-01 | [NULL]

The content is already verified in the accept views integration, and getting the data right is tricky: ``` birth_place = ( Commune.objects.filter( # The birthdate must be >= 1900-01-01, and we’re removing 1 day from start_date. Q(start_date__gt=datetime.date(1900, 1, 2)), # Must be a valid choice for the user current birthdate. Q(start_date__lte=birthdate), Q(end_date__gte=birthdate) | Q(end_date=None), ) .exclude( Exists( # The same code must not exists at the early_date. Commune.objects.exclude(pk=OuterRef("pk")).filter( code=OuterRef("code"), start_date__lt=OuterRef("start_date"), ) ) ) .first() ) ```

rsebille added the no-changelog Ne doit pas figurer dans le journal des changements. label Dec 9, 2024

rsebille requested a review from francoisfreitag December 9, 2024 12:47

rsebille self-assigned this Dec 9, 2024

francoisfreitag approved these changes Dec 10, 2024

View reviewed changes

rsebille force-pushed the rsebille/er-flaky-test branch 2 times, most recently from d4b2993 to 89478a6 Compare December 10, 2024 11:33

rsebille commented Dec 10, 2024

View reviewed changes

rsebille requested a review from francoisfreitag December 10, 2024 11:34

francoisfreitag force-pushed the rsebille/er-flaky-test branch from 9b0aebe to e9e2350 Compare December 10, 2024 14:37

xavfernandez reviewed Dec 11, 2024

View reviewed changes

tests/www/apply/test_process.py Outdated Show resolved Hide resolved

francoisfreitag force-pushed the rsebille/er-flaky-test branch from e9e2350 to 36a63b5 Compare December 12, 2024 08:25

francoisfreitag reviewed Dec 12, 2024

View reviewed changes

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

xavfernandez approved these changes Dec 12, 2024

View reviewed changes

francoisfreitag added the bug label Dec 12, 2024

rsebille commented Dec 12, 2024

View reviewed changes

tests/www/apply/test_process.py Outdated Show resolved Hide resolved

tests/www/apply/test_process.py Outdated Show resolved Hide resolved

rsebille changed the title ~~Tech : Corrections liés au lieu de naissance~~ Demandeur d’emploi : Correction de la sélection de la commune de naissance pour les personnes nées le dernier jours de celle-ci Dec 12, 2024

rsebille added modifié Modifié dans le changelog. and removed no-changelog Ne doit pas figurer dans le journal des changements. labels Dec 12, 2024

francoisfreitag force-pushed the rsebille/er-flaky-test branch from 0f24b23 to 645b129 Compare December 12, 2024 12:02

francoisfreitag enabled auto-merge December 12, 2024 12:08

francoisfreitag added this pull request to the merge queue Dec 12, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 12, 2024

francoisfreitag reviewed Dec 12, 2024

View reviewed changes

rsebille and others added 3 commits December 12, 2024 16:25

tests: Don't exclude 48.1% of valid possibilities for users' birthplaces

db136fc

asp.models: Commune.end_date is inclusive

e853183

code | start_date | end_date -------+------------+------------ 01001 | 1900-01-01 | 1999-12-31 01001 | 2000-01-01 | [NULL]

rsebille force-pushed the rsebille/er-flaky-test branch from 5a4e6af to 5d85773 Compare December 12, 2024 15:26

rsebille enabled auto-merge December 12, 2024 15:26

rsebille added this pull request to the merge queue Dec 12, 2024

Merged via the queue into master with commit 6adb772 Dec 12, 2024

rsebille deleted the rsebille/er-flaky-test branch December 12, 2024 15:45

	for period in [new_commune.start_date, datetime.date(2022, 11, 28), timezone.now()]:
	for period in [new_commune.start_date, datetime.date(2022, 11, 28), timezone.localdate()]:

Conversation

rsebille commented Dec 9, 2024

🤔 Pourquoi ?

Uh oh!

francoisfreitag left a comment

Choose a reason for hiding this comment

Uh oh!

francoisfreitag Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

rsebille Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

francoisfreitag Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

rsebille left a comment

Choose a reason for hiding this comment

Uh oh!

rsebille Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

francoisfreitag commented Dec 10, 2024

Uh oh!

Uh oh!

Uh oh!

francoisfreitag commented Dec 12, 2024

Uh oh!

xavfernandez commented Dec 12, 2024

Uh oh!

Uh oh!

Uh oh!

francoisfreitag commented Dec 12, 2024

Uh oh!

rsebille commented Dec 12, 2024

Uh oh!

francoisfreitag commented Dec 12, 2024

Uh oh!

Uh oh!

francoisfreitag Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

rsebille Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

francoisfreitag Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants