Retirer les CommuneFactory et CountryFactory #5591

francoisfreitag · 2025-02-11T10:44:49Z

🤔 Pourquoi ?

Être plus proche de la réalité, où nous utilisons le référentiel ASP sans créer ces objets.
Notamment, lors de la création d’un candidat, sa commune de naissance devrait être valide le jour de sa naissance. Au lieu d’apprendre aux factories comment créer ces communes, recherchons une commune existante.

tests/www/apply/test_process.py

xavfernandez · 2025-02-11T11:15:43Z

tests/utils/apis/test_api_particulier.py

    # Job seeker born outside of France
-    country = CountryOutsideEuropeFactory()
+    country = Country.objects.order_by("?").exclude(group=Country.Group.FRANCE).first()


Est-ce qu'il ne faudrait pas directement fournir un Trait born_outside_of_france ?

born_in_france=False avec un Maybe ? Me semble que ça devrais fonctionner

Je ne suis pas trop fan de lui donner forcément un pays de naissance, si c’est bien ta suggestion. Je suis parti sur la solution de Xavier : born_in_france, born_outside_france.

Ce que j'avais en tête :

born_in_france = factory.Maybe('born_in_france', yes_declaration=factory.Trait( jobseeker_profile__birth_place=factory.LazyAttribute( lambda instance: Commune.objects.order_by("?") .filter( start_date__lte=instance.birthdate, end_date__gte=instance.birthdate, ) .first(), ), jobseeker_profile__birth_country=factory.LazyAttribute(lambda _: Country.objects.get(name="FRANCE")), ), no_declaration=factory.Trait( jobseeker_profile__birth_country=factory.LazyAttribute( lambda _: Country.objects.order_by("?").exclude(group=Country.Group.FRANCE).first() ), ))

Mais ça ne fonctionne pas :(, a priori faut faire ça :

born_in_france = factory.Trait( jobseeker_profile__birth_place=factory.Maybe( "born_in_france", factory.LazyAttribute( lambda instance: Commune.objects.order_by("?") .filter( start_date__lte=instance.birthdate, end_date__gte=instance.birthdate, ) .first(), ), ), jobseeker_profile__birth_country=factory.Maybe( factory.LazyAttribute(lambda _: Country.objects.get(name="FRANCE")), factory.LazyAttribute( lambda _: Country.objects.order_by("?").exclude(group=Country.Group.FRANCE).first() ), ), )

Mais c'est vraiment pas ouf :´(.

rsebille · 2025-02-11T11:19:22Z

tests/users/test_models.py

-
-        # Europe and Commune filled
-        profile.birth_country = asp.CountryEuropeFactory()
+        profile.birth_country = not_france


Techniquement cette ligne n'est pas utile, mais si tu veux la garder pour être plus explicite alors je la mettrais après .birth_place = asp.CommuneFactory() pour avoir le même schéma dans les deux blocs et/ou un petit commentaire pour délimiter le cas testé.

Elle a été retirée, c’est du state invalide (un pays de naissance hors de France et une commune).

tests/users/factories.py

rsebille · 2025-02-11T11:22:31Z

tests/utils/apis/test_api_particulier.py

    # Job seeker born outside of France
-    country = CountryOutsideEuropeFactory()
+    country = Country.objects.order_by("?").exclude(group=Country.Group.FRANCE).first()


born_in_france=False avec un Maybe ? Me semble que ça devrais fonctionner

tests/www/apply/test_process.py

tests/utils/apis/test_api_particulier.py

celine-m-s · 2025-02-11T12:57:39Z

tests/api/applicants_api/tests.py

-        bob.jobseeker_profile.save()
-        dylan.jobseeker_profile.birth_place = CommuneFactory()
-        dylan.jobseeker_profile.birth_country = CountryFactory()
-        dylan.jobseeker_profile.save()


celine-m-s · 2025-02-11T13:00:01Z

tests/users/factories.py

@@ -172,7 +167,12 @@ class Params:
                )
                .first(),
            ),
-            jobseeker_profile__birth_country=factory.LazyAttribute(country_france),
+            jobseeker_profile__birth_country=factory.LazyAttribute(lambda _: Country.objects.get(name="FRANCE")),


Pourquoi ne pas avoir gardé le cache ?

Parce que je pense qu’il ne sert pas beaucoup (pas d’impact notable sur la durée des tests) et #5591 (comment).

celine-m-s

🤩

rsebille · 2025-02-12T09:07:51Z

tests/utils/apis/test_api_particulier.py

    # Job seeker born outside of France
-    country = CountryOutsideEuropeFactory()
+    country = Country.objects.order_by("?").exclude(group=Country.Group.FRANCE).first()


Ce que j'avais en tête :

born_in_france = factory.Maybe('born_in_france', yes_declaration=factory.Trait( jobseeker_profile__birth_place=factory.LazyAttribute( lambda instance: Commune.objects.order_by("?") .filter( start_date__lte=instance.birthdate, end_date__gte=instance.birthdate, ) .first(), ), jobseeker_profile__birth_country=factory.LazyAttribute(lambda _: Country.objects.get(name="FRANCE")), ), no_declaration=factory.Trait( jobseeker_profile__birth_country=factory.LazyAttribute( lambda _: Country.objects.order_by("?").exclude(group=Country.Group.FRANCE).first() ), ))

Mais ça ne fonctionne pas :(, a priori faut faire ça :

born_in_france = factory.Trait( jobseeker_profile__birth_place=factory.Maybe( "born_in_france", factory.LazyAttribute( lambda instance: Commune.objects.order_by("?") .filter( start_date__lte=instance.birthdate, end_date__gte=instance.birthdate, ) .first(), ), ), jobseeker_profile__birth_country=factory.Maybe( factory.LazyAttribute(lambda _: Country.objects.get(name="FRANCE")), factory.LazyAttribute( lambda _: Country.objects.order_by("?").exclude(group=Country.Group.FRANCE).first() ), ), )

Mais c'est vraiment pas ouf :´(.

xavfernandez · 2025-02-12T12:55:58Z

tests/www/dashboard/test_edit_user_info.py

        post_data = {
            "email": "[email protected]",
            "title": "M",
            "first_name": "Bob",
            "last_name": "Saint Clar",
            "birthdate": birthdate.isoformat(),
-            "birth_country": birth_country.pk,
+            "birth_country": user.jobseeker_profile.birth_country_id,


Tu pourrais le laisser inchangé non ?

En effet. Initialement, je pensais dégager complètement la variable birth_country, mais elle est utilisée dans le dernier assert du test pour vérifier que le pays de naissance est toujours le même pays.
Je ne suis pas convaincu de la pertinence de cet assert, mais c’est pas le but de cette PR ;)

The JobSeekerFactory and JobSeekerProfileFactory are in use in the application at large. The note about the fixtures is now documented with conftest.py (always load the fixture, for the whole session), so the developer shouldn’t have to worry about it anymore.

The object just got created by the factory, no need to load it from the database.

Ensure the birth_place start_date and end_date attribute are around the birthdate, so that data is realistic. Updated the factory to use actual objects from the ASP and not a fakes, as the application doesn’t manage Commune and Country objects. Remove the with_birth_place trait, as it would setup an incomplete object (birth country isn’t France). It was only used once and was merged into the born_in_france trait.

Use the ASP reference instead to be more realistic, as the application does not create Country objects. Removed tests for impossible or unused scenario: - european country vs world country, as it makes no difference to the application. - filling commune and selecting a world country, that’s invalid state

Use the ASP reference instead to be more realistic, as the application does not create Commune objects.

francoisfreitag added the no-changelog Ne doit pas figurer dans le journal des changements. label Feb 11, 2025

francoisfreitag self-assigned this Feb 11, 2025

francoisfreitag requested review from celine-m-s and rsebille February 11, 2025 10:44

francoisfreitag force-pushed the ff/factories branch from 371a2f6 to 1617c90 Compare February 11, 2025 10:50

xavfernandez reviewed Feb 11, 2025

View reviewed changes

rsebille reviewed Feb 11, 2025

View reviewed changes

francoisfreitag force-pushed the ff/factories branch from 1617c90 to e4b178a Compare February 11, 2025 12:00

celine-m-s reviewed Feb 11, 2025

View reviewed changes

tests/utils/apis/test_api_particulier.py Show resolved Hide resolved

celine-m-s reviewed Feb 11, 2025

View reviewed changes

celine-m-s approved these changes Feb 11, 2025

View reviewed changes

rsebille approved these changes Feb 12, 2025

View reviewed changes

xavfernandez approved these changes Feb 12, 2025

View reviewed changes

francoisfreitag added 3 commits February 13, 2025 10:49

Remove extraneous request from test_api_particulier

47fab9d

The object just got created by the factory, no need to load it from the database.

factories: Remove unused trait with_birth_country

640e35a

francoisfreitag force-pushed the ff/factories branch from e4b178a to 3eaa555 Compare February 13, 2025 10:13

francoisfreitag added 3 commits February 13, 2025 11:16

Remove CommuneFactory

6e58145

Use the ASP reference instead to be more realistic, as the application does not create Commune objects.

francoisfreitag force-pushed the ff/factories branch from 3eaa555 to 6e58145 Compare February 13, 2025 10:16

francoisfreitag added this pull request to the merge queue Feb 13, 2025

Merged via the queue into master with commit 6f6b233 Feb 13, 2025
11 checks passed

francoisfreitag deleted the ff/factories branch February 13, 2025 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retirer les CommuneFactory et CountryFactory #5591

Retirer les CommuneFactory et CountryFactory #5591

francoisfreitag commented Feb 11, 2025 •

edited

Loading

xavfernandez Feb 11, 2025

rsebille Feb 11, 2025

francoisfreitag Feb 11, 2025

francoisfreitag Feb 11, 2025

rsebille Feb 12, 2025

rsebille Feb 11, 2025

francoisfreitag Feb 11, 2025

rsebille Feb 11, 2025

celine-m-s Feb 11, 2025

celine-m-s Feb 11, 2025

francoisfreitag Feb 11, 2025

celine-m-s left a comment

rsebille Feb 12, 2025

xavfernandez Feb 12, 2025

francoisfreitag Feb 13, 2025

Retirer les CommuneFactory et CountryFactory #5591

Retirer les CommuneFactory et CountryFactory #5591

Conversation

francoisfreitag commented Feb 11, 2025 • edited Loading

🤔 Pourquoi ?

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

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

celine-m-s 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

francoisfreitag commented Feb 11, 2025 •

edited

Loading