Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { Contribution } from "./type";
import Link from "../common/Link";
import BlueCard from "../common/BlueCard";
import { AgreementSearchForm } from "../convention-collective/AgreementSearch/AgreementSearchForm";
import { AccessibleAlert } from "../outils/common/components/AccessibleAlert";

type Props = {
onAgreementSelect: (agreement?: Agreement) => void;
Expand All @@ -37,9 +38,12 @@ export function ContributionGenericAgreementSearch({
const router = useRouter();
const { slug } = contribution;
const [isValid, setIsValid] = useState(false);
const [showMissingAgreementError, setShowMissingAgreementError] =
useState(false);

useEffect(() => {
setIsValid(isAgreementValid(contribution, selectedAgreement));
setShowMissingAgreementError(false);
}, [selectedAgreement]);

const selectedAgreementAlert = (agreement: Agreement) => {
Expand Down Expand Up @@ -125,24 +129,49 @@ export function ContributionGenericAgreementSearch({
}}
/>
{((contribution.isNoCDT && isValid) || !contribution.isNoCDT) && (
<Button
className={fr.cx("fr-mt-2w")}
type="button"
onClick={(event) => {
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();
}
}}
>
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
«&nbsp;Afficher les informations sans sélectionner une
convention collective&nbsp;» ci-dessous.
</p>
}
severity="error"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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
«&nbsp;Afficher les informations sans sélectionner une
convention collective&nbsp;» 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
«&nbsp;Afficher les informations sans sélectionner une
convention collective&nbsp;» ci-dessous.
</p>
}
severity="error"
className={["fr-mt-2w"]}
data-testid="missing-agreement-error"
/>
)}

className={["fr-mt-2w"]}
data-testid="missing-agreement-error"
/>
)}
<Button
className={fr.cx("fr-mt-2w")}
type="button"
onClick={(event) => {
if (!selectedAgreement) {
event.preventDefault();
setShowMissingAgreementError(true);
return;
}
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();
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[IMPORTANT] event.preventDefault() is unreachable dead code in the valid-agreement branch

  • After the early-return guard on line 148 (if (!selectedAgreement) { … return; }), selectedAgreement is guaranteed to be truthy for the rest of the handler.
  • Therefore the else { event.preventDefault() } branch (when isValid is false but selectedAgreement is set) is still reachable — but calling event.preventDefault() on a type="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 when selectedAgreement is truthy, and the useEffect already resets the flag whenever selectedAgreement changes. 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:

}}
>
Afficher les informations
</Button>
</>
)}
</div>
</BlueCard>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,54 @@ describe("<ContributionLayout />", () => {
expect(rendering.getByText("my content")).toBeInTheDocument();
});

it("should display an error and not navigate when clicking 'Afficher les informations' without selecting any agreement", async () => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[MINOR] New tests are declared async but contain no await — misleading signature

  • async functions that never await anything are functionally synchronous. This is misleading to future maintainers who may assume there is asynchronous work happening.
  • Either remove async from the two purely synchronous tests, or add the missing await if 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", () => {

fireEvent.click(ccUi.radio.agreementSearchOption.get());
expect(ui.generic.missingAgreementError.query()).not.toBeInTheDocument();

fireEvent.click(ccUi.buttonDisplayInfo.get());

expect(ui.generic.missingAgreementError.query()).toBeInTheDocument();
expect(pushMock).not.toHaveBeenCalled();
// The fallback link should still be available
expect(ui.generic.linkDisplayInfo.query()).toBeInTheDocument();
});

it("should hide the error when an agreement is selected after the error is shown", async () => {
(searchAgreement as jest.Mock).mockImplementation(() =>
Promise.resolve([
{
id: "0016",
num: 16,
url: "https://www.legifrance.gouv.fr/affichIDCC.do?idConvention=KALICONT000005635624",
shortTitle:
"Transports routiers et activités auxiliaires du transport",
slug: "16-transports-routiers-et-activites-auxiliaires-du-transport",
title:
"Convention collective nationale des transports routiers et activités auxiliaires du transport du 21 décembre 1950",
},
])
);
fireEvent.click(ccUi.radio.agreementSearchOption.get());
fireEvent.click(ccUi.buttonDisplayInfo.get());
expect(ui.generic.missingAgreementError.query()).toBeInTheDocument();

await userEvent.click(ccUi.searchByName.input.get());
await userEvent.type(ccUi.searchByName.input.get(), "16");
fireEvent.click(ccUi.searchByName.autocompleteLines.IDCC16.name.get());

expect(ui.generic.missingAgreementError.query()).not.toBeInTheDocument();
});

it("should still allow displaying generic content via the fallback link when no agreement is selected", async () => {
fireEvent.click(ccUi.radio.agreementSearchOption.get());
// The user clicks the fallback link directly without selecting any CC
fireEvent.click(ui.generic.linkDisplayInfo.get());
expect(ui.generic.missingAgreementError.query()).not.toBeInTheDocument();
expect(ui.generic.linkDisplayInfo.query()).not.toBeInTheDocument();
expect(rendering.getByText("my content")).toBeInTheDocument();
expect(pushMock).not.toHaveBeenCalled();
});

it("should display correctly when a treated agreement is selected", async () => {
(searchAgreement as jest.Mock).mockImplementation(() =>
Promise.resolve([
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { byText } from "testing-library-selector";
import { byTestId, byText } from "testing-library-selector";
import { searchAgreement } from "../../convention-collective";

export const ui = {
Expand All @@ -10,6 +10,7 @@ export const ui = {
nonTreatedInfo: byText(
/Cette réponse correspond à ce que prévoit le code du travail/
),
missingAgreementError: byTestId("missing-agreement-error"),
},
};
export const mockAgreementSearch = (idcc) =>
Expand Down
Loading