-
Notifications
You must be signed in to change notification settings - Fork 24
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
Demandeur d’emploi : Ajout lieu de naissance dans le formulaire de création par un tiers [GEN-1944] #4732
Conversation
tests/www/apply/test_submit.py
Outdated
# This is the city matching with_ban_geoloc_address trait | ||
geispolsheim = create_city_geispolsheim() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'aurais pu le mettre dans une fixture, mais j'aimais que ce commentaire soit explicite (je pense c'est plus lisible)
86a4b42
to
db31518
Compare
itou/www/apply/forms.py
Outdated
@@ -194,6 +194,10 @@ def __init__(self, *args, **kwargs): | |||
) | |||
|
|||
|
|||
class CreateJobSeekerWithBirthFieldsStep1Form(BirthPlaceAndCountryMixin, CreateOrUpdateJobSeekerStep1Form): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je soupçonne que l'on souhaite également avoir ces champs dans les formulaires de modifications et donc tu pourrais directement rajouter le mixin sur CreateOrUpdateJobSeekerStep1Form
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top, merci 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cela implique ces deux vues :
apply/update_job_seeker/.../1
apply/hire/update_job_seeker/.../1
Mais je crois que ça va
db31518
to
834ea7a
Compare
itou/asp/forms.py
Outdated
# often used with JobProfileMixin. Added after super() init to prevent default widgets being generated | ||
self.PROFILE_FIELDS = list( | ||
set(getattr(self, "PROFILE_FIELDS", [])).union(set(["birth_place", "birth_country"])) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je serais plutôt pour que ce soit défini explicitement dans les classes/les formulaires utilisant ce mixin: cela permet d'y voir rapidement clair sur quel champs sont modifiés par le formulaire et cela devrait également permettre d'avoir les valeurs initiales correctement remplies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je voulais éviter que les défauts widgets soient générés (gaspillant, sur mon local ils ralentient la page) mais aussi d'éviter que l'ordre de déclaration de JobProfileMixin
et BirthPlaceAndCountryMixin
devient important
J'ai proposé une autre solution qui rajoute un peu de complexité au JobSeekerProfileFieldsMixin
mais qui enlève le besoin de ce petit hack
itou/asp/forms.py
Outdated
jobseeker_profile.birth_place = self.cleaned_data.get("birth_place") | ||
jobseeker_profile.birth_country = self.cleaned_data.get("birth_country") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sur les ModelForm de JobSeekerProfile
, ça devrait être fait via la déclaration Meta.fields
et pour ceux de User
via le JobSeekerProfileFieldsMixin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ce code est là pour accèder à jobseeker_profile._clean_birth_fields()
après - ce n'est pas pour lier ces champs au profil (je l'ai reformulé du code existant). Ce code devrait être superflu ?
En regardant le code c'est possible que ce cas n'est pas géré par JobSeekerProfileFieldsMixin._post_clean
, mais je proposerais qu'on corrige ça dans un autre PR si c'est le cas ? En plus je pense que c'est partie du travail de BirthPlaceAndCountryMixin
(qui devrait marcher en isolation)
Pour plus de contexte le test qui cible ce comportement est test_bad_birthplace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En plus je pense que c'est partie du travail de BirthPlaceAndCountryMixin (qui devrait marcher en isolation)
Le mieux serait je pense de sortir une fonction def _check_birth_location(birth_place, birth_country)
utilisée ici et dans _clean_birth_fields
pour mutualiser la vérification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah oui je vois, merci pour la clarification :)
834ea7a
to
3d82d07
Compare
itou/asp/models.py
Outdated
@classproperty | ||
def france_id(cls): | ||
if cls._ID_FRANCE is None: | ||
cls._ID_FRANCE = Country.objects.get(code=Country._CODE_FRANCE).pk | ||
return cls._ID_FRANCE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ajouté dans la dernière version. Ça sauve une réquête SQL sur les pages de création candidats et lors l'exécution des tests, mais ma motivation était de corriger un problème avec les assertNumQueries
qui pouvait varier dépendent des tests ciblés (et un champ lazy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je verrais bien cela dans une autre PR (car il me semble qu'il y a d'autres endroits où on vérifie le pays France)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'ai pensé sur ça mais en fait que ce PR touche tout les utilisations pertinent de Country si je me trompe pas ?
J'ai trouvé des utilisations du Country.code
que m'ont semblé valides, et d'ailleurs une partie du code qui utilise une groupe France (une abstraction que je trouvais assez propre)
🥁 La recette jetable est prête ! 👉 Je veux tester cette PR ! |
itou/asp/forms.py
Outdated
jobseeker_profile.birth_place = self.cleaned_data.get("birth_place") | ||
jobseeker_profile.birth_country = self.cleaned_data.get("birth_country") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En plus je pense que c'est partie du travail de BirthPlaceAndCountryMixin (qui devrait marcher en isolation)
Le mieux serait je pense de sortir une fonction def _check_birth_location(birth_place, birth_country)
utilisée ici et dans _clean_birth_fields
pour mutualiser la vérification.
itou/asp/models.py
Outdated
@classproperty | ||
def france_id(cls): | ||
if cls._ID_FRANCE is None: | ||
cls._ID_FRANCE = Country.objects.get(code=Country._CODE_FRANCE).pk | ||
return cls._ID_FRANCE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je verrais bien cela dans une autre PR (car il me semble qu'il y a d'autres endroits où on vérifie le pays France)
3d82d07
to
eac391b
Compare
@@ -463,7 +463,8 @@ class Country(PrettyPrintMixin, models.Model): | |||
Imported from ASP reference file: ref_grp_pays_v1, ref_insee_pays_v4.csv | |||
""" | |||
|
|||
_CODE_FRANCE = "100" | |||
INSEE_CODE_FRANCE = "100" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je ne vois pas pourquoi ce champ ne soit pas public ?
defines commonly used fields for jobseeker creation and hides the complexity of managing birth place, country and birthdate with INSEE records refactor(forms): further simplification
abd88c3
to
befc059
Compare
refactor(BirthPlaceAndCountryMixin): added comments fix: test refactor(BirthPlaceAndCountryMixin): reverse_lazy on autocomplete url fetch feat(modify jobseeker): using BirthPlaceAndCountryMixin refactor(BirthPlaceAndCountryMixin): avoid manipulation of PROFILE_FIELDS during __init__ feat(Country): cache france_id minor performance improvement for tests and for loading pages - but principally included to fix a flaky test refactor: solution reusability tests.www.apply: Use `assertSnapshotQueries()` in `test_submit.py` fix: snapshot requested changes added birth fields to profile_infos.html
befc059
to
0500419
Compare
🤔 Pourquoi ?
Les prescripteurs et les employeurs peuvent créer les demandeurs d'emploi et leurs candidatures sur les parcours de
apply/.../sender/...
etapply/.../hire/...
. Deux champs importants sur leur fichier sont la commune et la pays de naissance qui sont avec ce PR inclusent lors de ces parcours.🍰 Comment ?
Ce besoin a impliqué la logique un peu particulière du gestion de
birth_place
etbirth_country
, partagée maintenant entre trois formulaires (NewEmployeeRecordStep1Form
,CertifiedCriteriaInfoRequiredForm
etCreateJobSeekerWithBirthFieldsStep1Form
).Cette logique gère la création et la modification des profils du candidat sur les formulaires qui peuvent cible le
JobSeekerProfile
en directe ou par le clé surUser
. J'ai pu encacher cette complexité enBirthPlaceAndCountryMixin
, afin de simplifier les formulaires qui utilisent ses fonctionalités🚨 À vérifier
💻 Captures d'écran