Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

feat: Ajout d'une section "Temps passé" dans le formulaire d'édition d'un service #446

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jbuget
Copy link
Contributor

@jbuget jbuget commented Aug 30, 2024

🍣 Problème

Il s'agit de la partie front de la PR dora-back#369

🦄 Solution

On ajoute une section dans le formulaire d'édition d'un service.

Cette nouvelle section permet de définir 2 champs :

  • Nombre d'heures totales passées par le bénéficiaires service (Number)
  • Un champs de précision de la répartition des heures (String)

Côté implémentation, on utilise, modifie et enrichit le composant <BasicInputField/> pour qu'il supporte correctement le type "number".

Jusqu'à présent, on l'utilisait avec des types ["text", "tel", "email"] tous basés sur une valeur de type String. Il a fallu l'adapter pour supporter aussi le type Number. Conséquence : nécessite quelques .toString() à certains endroits.

Sur le modèle du type "tel", il a fallu définir des handlers spécifiques pour s'assurer que le format Number reste valide.

Côté utilisateur, on empêche l'utilisateur de saisir autre chose que des chiffres. On fait en sorte qu'il puisse quand même effacer la valeur et enregistrer (sans erreur) le formulaire.

🍒 Bonus

1/ Test de composants

Cette PR ajoute le support de tests de composant (rendu et comportement).

Pour cela on ajoute la bibliothèque Testing Library, compatible et préconisée avec Svelte(kit) / Vitest.

Les plus grosses difficultés rencontrées :

  • se rendre compte que le rendu du composant est dépendant du schéma du formulaire qui embarque le composant
  • parvenir à renseigner et injecter le schéma dans le test (finalement, c'est tout simple)

2/ Vite UI

Par ailleurs, cette PR permet aussi de lancer les tests via l'éditeur graphique.

Cela se fait avec la commande : npm run test:ui

Aucune difficulté ni rien de notable (à part que c'est assez cool).

3/ Ignore le répertoire .history

Il s'agit d'un répertoire utilisé par certaines extensions d'IDE (comme VSCode).

Par souci de se prémunir d'ajout intempestif qui pourrait perturber Git ou ESLint, on les ajoute aux fichiers à ignorer (.gitignore et .eslintignore).

4/ Stabiliser la CI

J'ai rencontré un souci récurrent de CI, peut-être parce que je suis sous Mac (mais pas sûr). Apparemment, il s'agit d'un problème connu de Vite depuis des mois, cf. cette issue GitHub.

La solution proposée par de multiples contributeur a fonctionné pour moi. Je propose de la conserver sur le projet.

👓 Revue de code

On peut y aller commit-par-commit 🧘‍♂️.

Copy link

socket-security bot commented Aug 30, 2024

@jbuget jbuget force-pushed the feat/temps-passe-par-service branch 3 times, most recently from 3caccc4 to 601fc9a Compare August 30, 2024 22:21
Added a nnew section "Beneficiary participation" with spending_time_* fields (total hours and operating details)

Needed to enrich / fix the basic-input-field component in order to correctly support numeric value.

In order to do this, we needed to add type `number` to variable `value`. Thus, we had to force `value.toString()` in some places in the component. Not very satisfying, but I did not have clever idea.
* Add testing-library framework for Svelte (with jsdom)
* Fix TypeScript config
* Add an example of component testing with BasicInputField

Good-to-konw:
In order to render, <BasicInputField/> requires that the given ID to be included (defined) in the associated form schema. It is why the tests use `spendingTimeTotalHours`.

This mechanism is based on a Svelte $store object (so the '$' character in the template). I thought I had to defined "context" (in `render()` method) but they are two separate concepts. The solution was simplest, just importing and changing value of the form schema.
It seems that there is a well known problem since few months, see vitejs/vite#15532

Lots of people recommend to add this optional dependency. Don't understand where, when and by whom it is required in the project.
Added .history to both .gitignore and .eslintignore to prevent temporary files created by the VSCode extension from being tracked by Git or linted by ESLint.
@jbuget jbuget force-pushed the feat/temps-passe-par-service branch from 601fc9a to cba9961 Compare September 3, 2024 09:20
Copy link
Collaborator

@ggounot ggounot left a comment

Choose a reason for hiding this comment

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

Super travail et merci pour l'ajout des tests de composants !

Je n'ai fait que relire le code pour le moment car je ne pourrai pas tester avant demain.

Comment on lines -102 to -103
on:blur={onBlur}
on:change={onChange}
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: en plus des conversions, ne devrait-on pas également toujours appeler le onBlur() et le onChange() du FieldWrapper afin que la validation du schéma se fasse ?

@@ -365,6 +365,18 @@ export const serviceSchema: v.Schema = {
default: false,
rules: [v.isBool()],
},
spendingTimeTotalHours: {
label: "Temps passé",
default: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: Je mettrais plutôt "" comme valeur par défaut sachant que le champ de saisie sous-jacent gère une chaîne de caractères.

Comment on lines +180 to +181
{:else}
currentSchema is not defined
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: l'utilisateur peut voir ça si le champ n'est pas trouvé dans le schéma

question: est-ce que dans le cas où le champ n'est pas dans le schéma, le champ ne doit tout simplement pas être affiché (cas actuel) ou devrait lever une erreur (au développeur) ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants