Fiches salarié: Faciliter la saisie des nouvelles fiches salariés [GEN-749] by tonial · Pull Request #5469 · gip-inclusion/les-emplois

tonial · 2025-01-27T08:26:46Z

🤔 Pourquoi ?

Il manque :

Du code à affiner (voir le second commit)
Le get_form_kwarg() qui semble être appelé 20 fois (soit faire une mise en cache, soit comprendre pourquoi)
Les remarques faits sur Figma pour reprendre le wording avec la bénédiction des designers
Des tests écrire quand les autres points seront réglés :)

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

Mettre à jour le CHANGELOG_breaking_changes.md ?
Ajouter l'étiquette « Bug » ?

🏝️ Comment tester ?

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

rsebille · 2025-01-28T09:55:49Z

        return super().dispatch(request, *args, **kwargs)

    def get_form_kwargs(self, step=None):
+        # Why is it called 20 times ?


Oo, j'ai pas du tout souvenir de ce comportement 🙈.
A confirmer mais un @functools.cache() devrais fonctionner car normalement ni self.company ni self.get_cleaned_data_for_step("choose-employee")["employee"] ne devrait être modifié pour la même instance du formulaire mais on est pas à l'abri que ça soit le cas...

Le coupable est ce commit : 6f9e9d0
et la condition qui appelle le formulaire qui fait une requête, et cela en boucle via je ne sais quelle mystère que je n'ai pas trop creusé.

Cela explique aussi pourquoi on a étape 1/1 :

On "masque" l'étape suivante.

J'aimerai améliorer le fonctionnement de la vue, sauf que je ne sais pas quel est le bug que tu avais voulu corriger vu qu'on n'a plus l'historique sentry :/
Est-ce que par hazard tu t'en rappellerais ?
Le commit dit : www.employee_record: Prevent errors if a user go back to add last step
Est-ce que c'est s'il fait un get sur la dernière url (avec un précédent du navigateur) ?

J'aurais bien envie de bazarder la wizard view pour mettre un formulaire tout simple avec deux champs : le salarié, et le PASS (disabled au début, et dont les choix dépendent du salarié) mais ça fait du js non testé :(
(et ça correspond assez bien au message que tu avais mis : #3369 (comment))

En effet après un retour navigateur ça plante.
Je vais ajouter un test explicite sur ce comportement.

Après avoir bien creusé, j'ai quand même bien l'impression que cette lib a plein de soucis...
Cette histoire de retour arrière devrait se poser pour chacun de nos wizards.
Il faudrait systématiquement vérifier que le form de l'étape d'avant a un cleaned_data quand on essaye d'accéder à une étape particulière, mais ça revient à patcher au milieu de NamedUrlWizardView.get() ....

De mémoire l'erreur Sentry était une KeyError "choose-employee" car la session n'existe plus après l'étape done.
Et oui j'ai jamais compris pourquoi celui-ci avait ce comportement alors que celui pour les demandes de prolongations utilise aussi le condition_dict et y avais pas ce genre de problèmes, peut-être parce que le formulaire n'a que 2 vues dont 1 conditionnelles et donc les first, current, next, last sont un peu toujours les mêmes 🤷

Après avoir bien creusé, j'ai quand même bien l'impression que cette lib a plein de soucis...

Comme je disais dans mon autre commentaire, celui en POST à l'air plutôt OK mais celui avec les vues part vite en couille, mais c'est le seul formulaire qui posais problème donc j'étais tenté de dire que c'était notre utilisation qui était mauvaise :)

Dans les demandes de prolongation, on attend la fin de la première étape pour compter ^^
Du coup à la première étape, on a une barre de progression à 50% mais on ne marque pas étape 1/1
Et à la seconde étape, quand on sait s'il y aura une 3eme étape, on arrive à 66% avec 2/3 si c'est le cas.

notion-workspace · 2025-02-03T06:39:09Z

🌿 [Fiches salariés] ETQ qu’employeur , je peux savoir facilement quelle FS je dois créer

github-actions · 2025-02-04T13:03:57Z

🥁 La recette jetable est prête ! 👉 Je veux tester cette PR !

rsebille · 2025-02-11T10:00:35Z

+            JobApplication.objects.eligible_as_employee_record(siae)
+            .filter(
+                hiring_start_at__gte=timezone.localdate() - relativedelta(months=4),
+                hiring_start_at__lte=timezone.localdate(),


Je suis presque sûr que des employeurs vont arriver et dire "Je comprend pas, j'ai embauché untel et ça ne s'affiche pas", et a priori on peux tomber dans un cas bizarre :

L'employeur embauche a une date ultérieur

Il n'a pas d'autres FS en attente

On lui affiche le bouton "Créer une fiche salarié"

Il retrouve son embauche dans la liste

Mais gros 👍 pour ajouter cette limite (y a pas un ticket d'ailleurs ?) mais a mon avis faut aussi le faire dans la vue de création et adapter le wording.

La limite est indiquée dans le ticket -> https://www.notion.so/gip-inclusion/Fiches-salari-s-ETQ-qu-employeur-je-peux-savoir-facilement-quelle-FS-je-dois-cr-er-18a5f321b60481d08f09c4b01528ca55?pvs=4*
Par contre il est aussi indiqué que dans la page de création on les veut tous.

Tu changerais quoi comme wording ?

J'aurais dû préciser, je ne parle pas de la limite à 4 mois (car effectivement on aurais eu le message tout le temps, comme au temps des FS hologrammes...) mais de la limite de ne pas prendre les embauches dans le futur, c'est une demande récurrente de bloquer l'envoi afin d'éviter des manipulations ensuite au support (en faite la personne est pas venue, etc) donc je pensais que tu avais embarqué ça dedans mais j'ai pas l'impression :).
Pour le wording je pensais juste à préciser qu'on affiche que les embauches "commencées" mais je ne trouve pas de bon adjectif pour ajouter à "Cette liste se base sur les embauches que vous avez déclarées sur le site des Emplois de l’inclusion." :/.

Cette liste se base sur les embauches que vous avez déclarées sur le site des Emplois de l’inclusion et qui ont déjà démarrée. ?

En effet, j'ai hiring_start_at__lte=timezone.localdate(), dans eligible_as_employee_record() pour que ce soit pris en compte partout et ton wording @xavfernandez

rsebille · 2025-02-11T10:07:47Z

+            employees = []
+            # Add job seekers in order, whithout duplicates
+            for job_app in hiring_of_the_company.eligible_as_employee_record(self.company).select_related(
+                "job_seeker"
+            ):
+                if job_app.job_seeker not in employees:
+                    employees.append(job_app.job_seeker)


Cette partie me fait me poser la question de si le tri ne devrais pas être fait au niveau des vues plutôt que dans la méthode du queryset en fait 🤔.

Le soucis c'est de retrouver le tri une fois que tu as ta liste de candidats.
Je vais repasser dessus dans un second temps en utilisant le Wizard de @xavfernandez comme base à la place de deux de formtools, et j'arriverais sans doute à faire un truc plus propre :)

rsebille · 2025-02-11T10:25:44Z

Si tu supprimes les deux fonctions test_done_ alors faut aussi supprimer le code mort associé

les-emplois/itou/www/employee_record_views/views.py

Lines 147 to 163 in 2da7cb5

else:

if employee_record.status == Status.NEW: # Should be filled, send to the creation tunnel

return HttpResponseRedirect(

reverse(

"employee_record_views:create",

kwargs={"job_application_id": employee_record.job_application.pk},

)

+ "?back_url="

+ reverse("employee_record_views:add", kwargs={"step": "choose-employee"})

)

else: # An employee record exists, show the summary

return HttpResponseRedirect(

add_url_params(

reverse("employee_record_views:summary", kwargs={"employee_record_id": employee_record.pk}),

{"back_url": reverse("employee_record_views:list")},

)

)

En effet, On ne peut plus y arriver maintenant qu'on filtre les salariés à l'étape 2, je vais nettoyer le code.

J'ai aussi ajouté une étape au test pour vérifier ce qui se passe dans ce cas (et mis un fixme pour quand ce sera plus simple de gérer : sans formtools)

tonial · 2025-02-11T12:14:19Z

@merci pour la relecture @rsebille !

rsebille

2 petits trucs à corriger mais je tamponne pour pas refaire un aller-retour derrière :).
Pour l'histoire de la limite sur les embauches futures, le comportement actuel ne me semble pas gênant donc a toi de voir ce que tu veux en faire.

rsebille · 2025-02-12T09:28:43Z

+            JobApplication.objects.eligible_as_employee_record(siae)
+            .filter(
+                hiring_start_at__gte=timezone.localdate() - relativedelta(months=4),
+                hiring_start_at__lte=timezone.localdate(),


J'aurais dû préciser, je ne parle pas de la limite à 4 mois (car effectivement on aurais eu le message tout le temps, comme au temps des FS hologrammes...) mais de la limite de ne pas prendre les embauches dans le futur, c'est une demande récurrente de bloquer l'envoi afin d'éviter des manipulations ensuite au support (en faite la personne est pas venue, etc) donc je pensais que tu avais embarqué ça dedans mais j'ai pas l'impression :).
Pour le wording je pensais juste à préciser qu'on affiche que les embauches "commencées" mais je ne trouve pas de bon adjectif pour ajouter à "Cette liste se base sur les embauches que vous avez déclarées sur le site des Emplois de l’inclusion." :/.

xavfernandez · 2025-02-12T13:16:36Z

+            JobApplication.objects.eligible_as_employee_record(siae)
+            .filter(
+                hiring_start_at__gte=timezone.localdate() - relativedelta(months=4),
+                hiring_start_at__lte=timezone.localdate(),


Cette liste se base sur les embauches que vous avez déclarées sur le site des Emplois de l’inclusion et qui ont déjà démarrée. ?

xavfernandez · 2025-02-12T13:21:04Z

+                hiring_start_at__gte=timezone.localdate() - relativedelta(months=4),
+                hiring_start_at__lte=timezone.localdate(),
+            )
+            .values_list("job_seeker", flat=True)


Suggested change

.values_list("job_seeker", flat=True)

.values_list("job_seeker_id", flat=True)

Je me suis rendu compte que ça faisait pareil en fait, mais c'est vrai que le *_id est plus explicite sur ce qu'on récupère

The name case is already handled in the get_full_name method

To be used in a future commit

Only display employee without record, and in -hiring_start_at order Adapt steps wording accordingly

tonial requested a review from rsebille January 27, 2025 08:26

tonial self-assigned this Jan 27, 2025

tonial added the modifié Modifié dans le changelog. label Jan 27, 2025

tonial force-pushed the alaurent/missing_employee_record branch 2 times, most recently from d8889a5 to 26575df Compare January 27, 2025 10:39

tonial commented Jan 28, 2025

View reviewed changes

Comment thread itou/job_applications/models.py Outdated

rsebille reviewed Jan 28, 2025

View reviewed changes

tonial force-pushed the alaurent/missing_employee_record branch 2 times, most recently from df7b372 to ab1a9d7 Compare February 3, 2025 06:29

tonial changed the title ~~Fiches salarié: Faciliter la saisie des nouvelles fiches salariés~~ Fiches salarié: Faciliter la saisie des nouvelles fiches salariés [GEN-749] Feb 3, 2025

tonial mentioned this pull request Feb 3, 2025

Technique: Amélioration du code de création de fiches salariées #5541

Merged

2 tasks

tonial force-pushed the alaurent/missing_employee_record branch 2 times, most recently from c8e9fe1 to d47f648 Compare February 4, 2025 10:07

tonial added the 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC label Feb 4, 2025

tonial assigned xavfernandez Feb 4, 2025

tonial force-pushed the alaurent/missing_employee_record branch from d47f648 to c987d72 Compare February 4, 2025 12:45

tonial added 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC and removed 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC labels Feb 4, 2025

tonial marked this pull request as ready for review February 4, 2025 12:46

tonial requested a review from xavfernandez February 4, 2025 12:47

tonial unassigned xavfernandez Feb 4, 2025

tonial force-pushed the alaurent/missing_employee_record branch from c987d72 to 5cd26df Compare February 4, 2025 12:48

tonial force-pushed the alaurent/missing_employee_record branch from 5cd26df to b720627 Compare February 4, 2025 14:39

tonial requested a review from rsebille February 5, 2025 05:44

tonial force-pushed the alaurent/missing_employee_record branch 2 times, most recently from 8090516 to f828bd9 Compare February 7, 2025 13:30

xavfernandez reviewed Feb 7, 2025

View reviewed changes

Comment thread itou/templates/employee_record/list.html Outdated

Comment thread itou/templates/employee_record/list.html

Comment thread itou/www/employee_record_views/views.py

tonial force-pushed the alaurent/missing_employee_record branch from f828bd9 to 0af7222 Compare February 8, 2025 20:37

tonial force-pushed the alaurent/missing_employee_record branch 2 times, most recently from 5fbd20a to 37767b1 Compare February 10, 2025 09:58

hellodeloo self-requested a review February 10, 2025 10:09

tonial force-pushed the alaurent/missing_employee_record branch from 37767b1 to e79e8c0 Compare February 10, 2025 10:10

hellodeloo reviewed Feb 10, 2025

View reviewed changes

Comment thread itou/templates/employee_record/list.html Outdated

tonial force-pushed the alaurent/missing_employee_record branch 3 times, most recently from c67bd79 to 2da7cb5 Compare February 10, 2025 20:49

rsebille reviewed Feb 11, 2025

View reviewed changes

tonial force-pushed the alaurent/missing_employee_record branch from 2da7cb5 to a24c541 Compare February 11, 2025 12:13

tonial requested a review from rsebille February 11, 2025 12:13

tonial force-pushed the alaurent/missing_employee_record branch 2 times, most recently from 0e6a7f1 to cff23d3 Compare February 11, 2025 14:05

rsebille approved these changes Feb 12, 2025

View reviewed changes

xavfernandez reviewed Feb 12, 2025

View reviewed changes

tonial added 4 commits February 13, 2025 06:17

employee_records_views: Don't title the job seeker full name

278dd62

The name case is already handled in the get_full_name method

job_applications: Put back code to find missing EmployeeRecords

3872362

To be used in a future commit

employee_record: Add missing record alert on list

4f95bd6

employee_record_views: Improve AddView

c733c8b

Only display employee without record, and in -hiring_start_at order Adapt steps wording accordingly

tonial force-pushed the alaurent/missing_employee_record branch from cff23d3 to c733c8b Compare February 13, 2025 05:31

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

Merged via the queue into master with commit ae72d94 Feb 13, 2025

tonial deleted the alaurent/missing_employee_record branch February 13, 2025 05:51

	else:
	if employee_record.status == Status.NEW: # Should be filled, send to the creation tunnel
	return HttpResponseRedirect(
	reverse(
	"employee_record_views:create",
	kwargs={"job_application_id": employee_record.job_application.pk},
	)
	+ "?back_url="
	+ reverse("employee_record_views:add", kwargs={"step": "choose-employee"})
	)
	else: # An employee record exists, show the summary
	return HttpResponseRedirect(
	add_url_params(
	reverse("employee_record_views:summary", kwargs={"employee_record_id": employee_record.pk}),
	{"back_url": reverse("employee_record_views:list")},
	)
	)

	.values_list("job_seeker", flat=True)
	.values_list("job_seeker_id", flat=True)

Uh oh!

Conversation

tonial commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤔 Pourquoi ?

🍰 Comment ?

🚨 À vérifier

🏝️ Comment tester ?

💻 Captures d'écran

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

notion-workspace Bot commented Feb 3, 2025

Uh oh!

github-actions Bot commented Feb 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonial Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonial commented Feb 11, 2025

Uh oh!

rsebille left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

tonial commented Jan 27, 2025 •

edited

Loading

tonial Feb 13, 2025 •

edited

Loading