-
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
Candidats: Ajout des champs commune et pays de naissance dans « Mon profil » #5126
base: master
Are you sure you want to change the base?
Conversation
Instead of a datetime.
Make sure users are able to fill in that information in order to have their IAE eligibility certified. The Country.france_id is now preloaded to stabilize queries count across the test suite. Before, depending on previous tests, the class would have the id cached or not.
🥁 La recette jetable est prête ! 👉 Je veux tester cette PR ! |
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.
Rien à redire sur les changements en tant que tels, mais la validation ne me convainc pas vraiment :
Le champ birth_country
n'est pas indiqué requis à l'utilisateur (astérisque) :
Il n'est pas dans la liste des requis car il est complété automatiquement si la commune est française :
Lines 56 to 65 in efae4e1
if not birth_country: | |
# Selecting a birth place sets the birth country field to France and disables it. | |
# However, disabled fields are ignored by Django. | |
# That's also why we can't make it mandatory. | |
# See utils.js > toggleDisableAndSetValue | |
if birth_place: | |
self.cleaned_data["birth_country"] = Country.objects.get(code=Country.INSEE_CODE_FRANCE) | |
else: | |
# Display the error above the field instead of top of page. | |
self.add_error("birth_country", "Le pays de naissance est obligatoire.") |
Également, si on ne saisit pas de manière linéaire ou que l'on remplit le pays à France sans saisir de ville on a ce message d'erreur incohérent :
Je pense que les deux messages ont été intervertis ici, au regard des conditions :
les-emplois/itou/utils/validators.py
Lines 95 to 102 in efae4e1
def validate_birth_location(birth_country, birth_place): | |
# If birth country is France, then birth place must be provided | |
if birth_country and birth_country.code == Country.INSEE_CODE_FRANCE and not birth_place: | |
raise ValidationError("Il n'est pas possible de saisir une commune de naissance hors de France.") | |
# If birth country is not France, do not fill a birth place (no ref file) | |
if birth_country and birth_country.code != Country.INSEE_CODE_FRANCE and birth_place: | |
raise ValidationError("Si le pays de naissance est la France, la commune de naissance est obligatoire.") |
Bref, BirthPlaceAndCountryMixin
est aussi utilisé ailleurs, je n'ai pas en tête tous les tenants, mais d'emblée j'aurais plutôt procédé ainsi :
- Placer le champ country en
required=True
en premier, éditable dans tous les cas, quitte a réordonner les valeurs pour placer la France en tête de liste - Placer le champ ville en 2e puisqu'il dépend du pays, quitte à conditionner son affichage si le pays de naissance est la France
À voir avec les UX ?
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 des points remontés par Léo (et qui devraient idéalement être corrigés dans une autre PR vu que ça concerne des formulaires existant), je m'attendais à revoir les tests propres au mixin BirthPlaceAndCountryMixin
(commune française avec pays hors de France & commune française sans pays France...)
Tu as raison, ce sera corrigé avec #5173. |
🤔 Pourquoi ?
Collecter ces informations pour certifier la situation administrative des candidats (Bénéficiaire RSA, …).
🏝️ Comment tester
En tant que candidat
En tant que prescripteur/orienteur ou employeur
💻 Captures d'écran