Skip to content

Fix champs bloquants pour les préavis d'accès aux services#4948

Draft
n0izn0iz wants to merge 5 commits intomasterfrom
norman/fix-4857-blocking-fields
Draft

Fix champs bloquants pour les préavis d'accès aux services#4948
n0izn0iz wants to merge 5 commits intomasterfrom
norman/fix-4857-blocking-fields

Conversation

@n0izn0iz
Copy link
Copy Markdown

@n0izn0iz n0izn0iz commented Mar 31, 2026

Fixes #4857 #4446

Notes:

  • J'ai commencé par retirer les champs dans le form de préavis quand purpose === LAN (frontend/src/features/PriorNotification/components/ManualPriorNotificationForm/Form.tsx)
  • La validation front ne passait plus car ces champs était requis du coup j'ai édité la validation frontend pour permettre d'avoir les champs cachés vide quand purpose === LAN. (frontend/src/features/PriorNotification/components/ManualPriorNotificationForm/constants.ts)
  • La validation backend ne passait plus, du coup j'ai aussi modifié les types en backend pour ne plus forcer à avoir ces champs et ajouté de la validation après parsing pour s'assurer que les champs soit set dans le cas débarquement et unset dans les autres cas. (backend/src/main/kotlin/fr/gouv/cnsp/monitorfish/domain/use_cases/prior_notification/CreateOrUpdateManualPriorNotification.kt)
  • Le template.ts était une antipattern (formating via string.replace), du coup j'ai remplacé par une vrai template string 01b77f3
  • Considerer l'utilisation d'une lib jinja js (https://www.npmjs.com/package/@huggingface/jinja par ex) pour utiliser le même template en front et dans le pipeline

@tristanrobert
Copy link
Copy Markdown

tristanrobert commented Mar 31, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Copy link
Copy Markdown
Collaborator

@louptheron louptheron left a comment

Choose a reason for hiding this comment

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

Je sais que c'est en draft, mais p'tit comment :)

@n0izn0iz n0izn0iz force-pushed the norman/fix-4857-blocking-fields branch 2 times, most recently from fbd5901 to 180e4ff Compare April 6, 2026 10:32
@n0izn0iz n0izn0iz marked this pull request as ready for review April 6, 2026 10:48
@n0izn0iz
Copy link
Copy Markdown
Author

n0izn0iz commented Apr 6, 2026

Je vois que les tests e2e sont instable sur master, es-ce que le test qui fail sur cet PR est lié a cette PR a priori?
(CI/CD Back & Front / Run E2E multi windows tests (push))

@n0izn0iz n0izn0iz self-assigned this Apr 6, 2026
@n0izn0iz
Copy link
Copy Markdown
Author

n0izn0iz commented Apr 6, 2026

Je viens aussi de voir que dans les templates de préavis, la date de débarque est affiché dans tout les cas il semble, je me demande si il faut aussi supprimer les champs la bas: /frontend/src/features/PriorNotification/components/shared/DownloadButton/template.ts par exemple.

?.logbookMessage
?.message as PNO?

val year =
Copy link
Copy Markdown
Collaborator

@louptheron louptheron Apr 8, 2026

Choose a reason for hiding this comment

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

Est-ce que tu peux rajouter un test unitaire dans CreateOrUpdateManualPriorNotificationUTests.kt pour tester ce case expectedLandingDate à null ?

Copy link
Copy Markdown
Author

@n0izn0iz n0izn0iz Apr 10, 2026

Choose a reason for hiding this comment

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

Test ajouté dans 2868b5d

Mais j'ai un gros doute, j'ai du changer le dernier assert qui verifie isBeingSent de true a false

assertThat(
                allValues
                    .first()
                    .logbookMessageAndValue.value.isBeingSent,
            ).isFalse

Es-ce que c'est normal dans le cas purpose != LAN?

Copy link
Copy Markdown
Collaborator

@louptheron louptheron Apr 15, 2026

Choose a reason for hiding this comment

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

Oui c'est normal que ça soit false car tu passes fishingCatches = listOf(),
Donc isPriorNotificationZero est à true :

val isPriorNotificationZero = fishingCatches.all { it.weight == null || it.weight == 0.0 }

        // If the prior notification is not in verification scope,
        // we pass `isBeingSent` as `true` in order to ask the workflow to send it.
        val isBeingSent = !isInVerificationScope && isPartOfControlUnitSubscriptions && !isPriorNotificationZero

@n0izn0iz n0izn0iz force-pushed the norman/fix-4857-blocking-fields branch from 180e4ff to 80b6837 Compare April 10, 2026 13:45
Norman added 2 commits April 10, 2026 16:15
Signed-off-by: Norman <norman@samourai.coop>
Signed-off-by: Norman <norman@samourai.coop>
@n0izn0iz n0izn0iz force-pushed the norman/fix-4857-blocking-fields branch 2 times, most recently from 5ee1909 to 2868b5d Compare April 10, 2026 14:16
@n0izn0iz
Copy link
Copy Markdown
Author

n0izn0iz commented Apr 10, 2026

Question sur le template.ts:

Dans template.ts, le template est une string normale avec des placeholder de variable du style {variable}. Cette string est passé dans la fonction suivante qui va remplacer les placeholder avec un string.replace:

function fillTemplate(html: string, data: TemplateData): string {
  return html.replace(/{(.*?)}/g, (_, key) => (data[key] !== null && data[key] !== undefined ? String(data[key]) : ''))
}

On dirait une grosse antipattern. Ça rend la maintenance compliquée parce-qu’il n'y a pas de type checking ni de logique imbriquée possible.

  1. Quelqu'un sait pourquoi on fait ça?
  2. Je peux remplacer ça par une template string javascript normale?
    Genre:
    function fillTemplate(data: TemplateData) => `<html>(...)<tr>${data.variable}</tr>(...)</html>`

@louptheron
Copy link
Copy Markdown
Collaborator

louptheron commented Apr 13, 2026

@n0izn0iz

Quelqu'un sait pourquoi on fait ça?

Je crois qu'à la base y'avait ce même template dans le code Python donc le but c'était de s'en approcher.

Je peux remplacer ça par une template string javascript normale?

Oui OK pour moi.

Norman added 3 commits April 14, 2026 11:37
Signed-off-by: Norman <norman@samourai.coop>
Signed-off-by: Norman <norman@samourai.coop>
Signed-off-by: Norman <norman@samourai.coop>
@sonarqubecloud
Copy link
Copy Markdown

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Préavis - champs bloquants pour les préavis d'accès aux services

3 participants