Skip to content

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

Merged
tonial merged 2 commits intomasterfrom
alaurent/add_employee_records
Feb 4, 2025
Merged

Technique: Amélioration du code de création de fiches salariées#5541
tonial merged 2 commits intomasterfrom
alaurent/add_employee_records

Conversation

@tonial
Copy link
Contributor

@tonial tonial commented Feb 3, 2025

🤔 Pourquoi ?

Voir la discussion : #5469 (comment)

condition_dict "masquait" la dernière étape quand la première n'avait pas été complétée, ce qui fait qu'on voyait étape 1/1
La lib se retrouve également a appeler plein de fois get_form_kwargs() ce qui fait plein de requêtes.

On pourrait mettre un cache sur get_form_kwargs() mais cela ne résout pas la question du 1/1

Voici une approche un peu crado qui permet de rediriger au bon endroit quand on détecte un soucis de retour arrière (inspirée de ce qu'on fait dans JobApplicationRefuseView).
Mais bon, j'ai l'impression que la lib a un gros défaut : je ne comprends pas qu'elle permette de "sauter" vers une étape plus loin sans rien dire....

🍰 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

@tonial tonial added the tech label Feb 3, 2025
@tonial tonial self-assigned this Feb 3, 2025
@tonial tonial force-pushed the alaurent/add_employee_records branch from 3996aba to ca34de9 Compare February 4, 2025 05:48
Copy link
Contributor

@rsebille rsebille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'aurais bien voulu dire "ça m'ennuie de pas comprendre pourquoi celui-ci à ce comportement et pas les autres" mais franchement autant WizardView() me semble OK que NamedUrlWizardView() c'est bizarre depuis le début, bref...
Merci pour l'amélioration 🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On pourrais pas juste faire ça ?

Suggested change
if step_url == self.steps.last and self.steps.current == self.steps.first:
if step_url != self.steps.current:

Quoique j'ai l'impression que le step n'est pas encore mis à ce moment car ça l'air d'être fait dans NamedUrlWizardView.get() :/.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ça plante pour l'étape done.
Donc il faut ajouter step_url != self.steps.current and step_url != "done"
mais c'est une bonne idée, ça force à l'étape en cours, c'est plus simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(et les steps sont créés dans le dispatch, donc c'est bon)

Using condition_dict would make the user think there is only one step
It also make a lot of calls to get_form_kwargs(), and plenty of
queries...
@tonial tonial force-pushed the alaurent/add_employee_records branch from ca34de9 to 0b862e0 Compare February 4, 2025 08:52
@tonial tonial enabled auto-merge February 4, 2025 08:53
@tonial tonial added this pull request to the merge queue Feb 4, 2025
Comment on lines +101 to +102
step_url = kwargs.get("step", None)
if step_url != self.steps.current and step_url != "done":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est vraiment des "url" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non, c'est plus step_in_url mais c'est comme ça que c'est appelé dans NamedUrlWizardView donc j'ai voulu garder le même nom

Merged via the queue into master with commit c54c4e7 Feb 4, 2025
11 checks passed
@tonial tonial deleted the alaurent/add_employee_records branch February 4, 2025 09:09
@sentry
Copy link

sentry bot commented Feb 6, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ TypeError: 'NoneType' object is not subscriptable /employee_record/add/{step} View Issue
  • ‼️ SuspiciousOperation: Les données du formulaire ManagementForm sont manquantes ou ont été manipulées. /employee_record/add/{step} View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments