fix(contribution): bloquer "Afficher les informations" sans CC sélectionnée#7232
Open
fix(contribution): bloquer "Afficher les informations" sans CC sélectionnée#7232
Conversation
…ionnée Sur les pages contribution, cliquer sur le bouton « Afficher les informations » sans avoir sélectionné de convention collective n'avait aucun retour visuel pour l'utilisateur. Le clic est désormais bloqué et un message d'erreur invite à sélectionner une convention collective ou à utiliser le bouton « Afficher les informations sans sélectionner une convention collective » situé en dessous. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
revu-bot
reviewed
Apr 11, 2026
Collaborator
revu-bot
left a comment
There was a problem hiding this comment.
Overall Assessment
The PR correctly addresses issue #7231 by showing a validation error when the user clicks "Afficher les informations" without selecting a convention collective. The implementation is clean and the test coverage is solid. A few issues worth addressing:
| File | Lines | Severity | Issue |
|---|---|---|---|
ContributionGenericAgreementSearch.tsx |
155–165 | IMPORTANT | event.preventDefault() called after router.push() — dead code in the valid-agreement branch |
ContributionGenericAgreementSearch.tsx |
148–149 | IMPORTANT | setShowMissingAgreementError(false) is redundant after the early-return guard |
ContributionGenericAgreementSearch.tsx |
133–145 | MINOR | Error alert rendered above the button it describes — consider placing it after for better reading order |
contributions.test.tsx |
127–148 | MINOR | New tests marked async but contain no await — misleading signature |
revu-bot
reviewed
Apr 11, 2026
Comment on lines
+159
to
+169
| setShowMissingAgreementError(false); | ||
| onDisplayClick(isValid && !!selectedAgreement); | ||
| if (isValid && selectedAgreement) { | ||
| router.push( | ||
| slug === "les-conges-pour-evenements-familiaux" | ||
| ? `/contribution/${slug}/${selectedAgreement?.slug || selectedAgreement?.num}` | ||
| : `/contribution/${selectedAgreement?.num}-${slug}` | ||
| ); | ||
| } else { | ||
| event.preventDefault(); | ||
| } |
Collaborator
There was a problem hiding this comment.
[IMPORTANT] event.preventDefault() is unreachable dead code in the valid-agreement branch
- After the early-return guard on line 148 (
if (!selectedAgreement) { … return; }),selectedAgreementis guaranteed to be truthy for the rest of the handler. - Therefore the
else { event.preventDefault() }branch (whenisValidis false butselectedAgreementis set) is still reachable — but callingevent.preventDefault()on atype="button"has no effect anyway, since buttons don't have a default browser action. - More importantly,
setShowMissingAgreementError(false)on line 149 is now redundant: the only way to reach it is whenselectedAgreementis truthy, and theuseEffectalready resets the flag wheneverselectedAgreementchanges. Keeping it creates a false impression that it is needed here.
- setShowMissingAgreementError(false);
onDisplayClick(isValid && !!selectedAgreement);
if (isValid && selectedAgreement) {
router.push(
slug === "les-conges-pour-evenements-familiaux"
? `/contribution/${slug}/${selectedAgreement?.slug || selectedAgreement?.num}`
: `/contribution/${selectedAgreement?.num}-${slug}`
);
- } else {
- event.preventDefault();
}Suggested clean-up:
revu-bot
reviewed
Apr 11, 2026
Comment on lines
+133
to
+145
| {showMissingAgreementError && ( | ||
| <AccessibleAlert | ||
| title="Veuillez sélectionner une convention collective" | ||
| description={ | ||
| <p> | ||
| Pour afficher des informations personnalisées, vous devez | ||
| d’abord sélectionner une convention collective. Si vous ne | ||
| souhaitez pas en sélectionner, vous pouvez cliquer sur | ||
| « Afficher les informations sans sélectionner une | ||
| convention collective » ci-dessous. | ||
| </p> | ||
| } | ||
| severity="error" |
Collaborator
There was a problem hiding this comment.
[IMPORTANT] Error alert is rendered above the button it describes — reading order concern
- Screen readers and keyboard users encounter the error message before the button that triggered it. This is counter-intuitive: the user activates the button, then has to scroll/tab back up to read the error.
- DSFR convention for inline form errors is to place the alert immediately after the triggering control, not before it.
- Moving the
<AccessibleAlert>below the<Button>keeps the DOM order consistent with the visual interaction flow and improves the experience for assistive-technology users.
Suggested change
| {showMissingAgreementError && ( | |
| <AccessibleAlert | |
| title="Veuillez sélectionner une convention collective" | |
| description={ | |
| <p> | |
| Pour afficher des informations personnalisées, vous devez | |
| d’abord sélectionner une convention collective. Si vous ne | |
| souhaitez pas en sélectionner, vous pouvez cliquer sur | |
| « Afficher les informations sans sélectionner une | |
| convention collective » ci-dessous. | |
| </p> | |
| } | |
| severity="error" | |
| <Button | |
| className={fr.cx("fr-mt-2w")} | |
| type="button" | |
| onClick={(event) => { | |
| if (!selectedAgreement) { | |
| event.preventDefault(); | |
| setShowMissingAgreementError(true); | |
| return; | |
| } | |
| onDisplayClick(isValid && !!selectedAgreement); | |
| if (isValid) { | |
| router.push( | |
| slug === "les-conges-pour-evenements-familiaux" | |
| ? `/contribution/${slug}/${selectedAgreement.slug || selectedAgreement.num}` | |
| : `/contribution/${selectedAgreement.num}-${slug}` | |
| ); | |
| } | |
| }} | |
| > | |
| Afficher les informations | |
| </Button> | |
| {showMissingAgreementError && ( | |
| <AccessibleAlert | |
| title="Veuillez sélectionner une convention collective" | |
| description={ | |
| <p> | |
| Pour afficher des informations personnalisées, vous devez | |
| d'abord sélectionner une convention collective. Si vous ne | |
| souhaitez pas en sélectionner, vous pouvez cliquer sur | |
| « Afficher les informations sans sélectionner une | |
| convention collective » ci-dessous. | |
| </p> | |
| } | |
| severity="error" | |
| className={["fr-mt-2w"]} | |
| data-testid="missing-agreement-error" | |
| /> | |
| )} |
revu-bot
reviewed
Apr 11, 2026
| expect(rendering.getByText("my content")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("should display an error and not navigate when clicking 'Afficher les informations' without selecting any agreement", async () => { |
Collaborator
There was a problem hiding this comment.
[MINOR] New tests are declared async but contain no await — misleading signature
asyncfunctions that neverawaitanything are functionally synchronous. This is misleading to future maintainers who may assume there is asynchronous work happening.- Either remove
asyncfrom the two purely synchronous tests, or add the missingawaitif async assertions were intended.
Affected tests:
"should display an error and not navigate when clicking 'Afficher les informations' without selecting any agreement"(line 127)"should still allow displaying generic content via the fallback link when no agreement is selected"(line 163)
Suggested change
| it("should display an error and not navigate when clicking 'Afficher les informations' without selecting any agreement", async () => { | |
| it("should display an error and not navigate when clicking 'Afficher les informations' without selecting any agreement", () => { |
…ns CC Le test «afficher les infos - sans CC» vérifiait l'ancien comportement buggé où cliquer sur le bouton principal sans convention collective émettait l'évènement de tracking «click_afficher_les_informations_sans_CC». Désormais, ce clic est bloqué et un message d'erreur est affiché : le test vérifie l'absence d'évènement et la présence du message d'erreur. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
L'ancien bouton « Afficher les informations sans sélectionner une convention collective » a été remplacé par un radio « Je ne souhaite pas renseigner ma convention collective. » suivi du bouton « Afficher les informations ». Le test e2e suit désormais ce parcours. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…d'information Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
🎉 Deployment for commit 8ef96d7 : IngressesDocker images
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



fix #7231