From 91275d74baf5269a5188cd23af0ba89291ad7d38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Wed, 18 Sep 2024 08:10:55 +0100 Subject: [PATCH] fix: Move "invalid graph" warning to `MapFieldInput` (#3689) --- .../@planx/components/List/Public/index.tsx | 44 +--------- .../components/MapAndLabel/Public/index.tsx | 22 +---- .../PlanningConstraints/Public.test.tsx | 74 +++++++++-------- .../components/PlanningConstraints/Public.tsx | 30 +------ .../PropertyInformation/Public.test.tsx | 12 ++- .../components/PropertyInformation/Public.tsx | 23 ++---- .../Schema/InputFields/MapFieldInput.tsx | 11 +++ .../{ => Error}/ErrorFallback.stories.tsx | 0 .../components/{ => Error}/ErrorFallback.tsx | 15 ++-- .../src/components/Error/GraphError.test.tsx | 81 +++++++++++++++++++ .../src/components/Error/GraphError.tsx | 43 ++++++++++ .../Settings/Submissions/EventsLog.tsx | 2 +- .../FlowEditor/components/forms/FormModal.tsx | 2 +- .../src/pages/Preview/Questions.tsx | 2 +- .../src/pages/layout/FlowEditorLayout.tsx | 2 +- .../src/pages/layout/PublicLayout.tsx | 2 +- 16 files changed, 212 insertions(+), 153 deletions(-) rename editor.planx.uk/src/components/{ => Error}/ErrorFallback.stories.tsx (100%) rename editor.planx.uk/src/components/{ => Error}/ErrorFallback.tsx (70%) create mode 100644 editor.planx.uk/src/components/Error/GraphError.test.tsx create mode 100644 editor.planx.uk/src/components/Error/GraphError.tsx diff --git a/editor.planx.uk/src/@planx/components/List/Public/index.tsx b/editor.planx.uk/src/@planx/components/List/Public/index.tsx index aef0b30a2c..6b7f1f6888 100644 --- a/editor.planx.uk/src/@planx/components/List/Public/index.tsx +++ b/editor.planx.uk/src/@planx/components/List/Public/index.tsx @@ -8,11 +8,8 @@ import TableBody from "@mui/material/TableBody"; import TableCell from "@mui/material/TableCell"; import TableRow from "@mui/material/TableRow"; import Typography from "@mui/material/Typography"; -import { SiteAddress } from "@planx/components/FindProperty/model"; -import { ErrorSummaryContainer } from "@planx/components/shared/Preview/ErrorSummaryContainer"; import { SchemaFields } from "@planx/components/shared/Schema/SchemaFields"; import { PublicProps } from "@planx/components/ui"; -import { useStore } from "pages/FlowEditor/lib/store"; import React, { useEffect, useRef } from "react"; import { FONT_WEIGHT_SEMI_BOLD } from "theme"; import FullWidthWrapper from "ui/public/FullWidthWrapper"; @@ -56,14 +53,8 @@ const InactiveListCardLayout = styled(Box)(({ theme }) => ({ const ActiveListCard: React.FC<{ index: number; }> = ({ index: i }) => { - const { - schema, - saveItem, - cancelEditItem, - errors, - formik, - activeIndex, - } = useListContext(); + const { schema, saveItem, cancelEditItem, errors, formik, activeIndex } = + useListContext(); const ref = useRef(null); useEffect(() => { @@ -119,8 +110,7 @@ const ActiveListCard: React.FC<{ const InactiveListCard: React.FC<{ index: number; }> = ({ index: i }) => { - const { schema, formik, removeItem, editItem } = - useListContext(); + const { schema, formik, removeItem, editItem } = useListContext(); const mapPreview = schema.fields.find((field) => field.type === "map"); @@ -189,8 +179,7 @@ const Root = () => { listProps, } = useListContext(); - const { title, description, info, policyRef, howMeasured, handleSubmit } = - listProps; + const { title, description, info, policyRef, howMeasured } = listProps; const rootError: string = (errors.min && `You must provide at least ${schema.min} response(s)`) || @@ -201,32 +190,7 @@ const Root = () => { const shouldShowAddAnotherButton = schema.max !== 1 || formik.values.schemaData.length < 1; - // If the selected schema has a "map" field, ensure there's a FindProperty component preceding it (eg address data in state to position map view) const hasMapField = schema.fields.some((field) => field.type === "map"); - const { longitude, latitude } = useStore( - (state) => - (state.computePassport()?.data?.["_address"] as SiteAddress) || {}, - ); - - if (hasMapField && (!longitude || !latitude)) { - return ( - - - - - Invalid graph - - - Edit this flow so that "List" is positioned after "FindProperty"; an - address is required for schemas that include a "map" field. - - - - ); - } const listContent = ( diff --git a/editor.planx.uk/src/@planx/components/MapAndLabel/Public/index.tsx b/editor.planx.uk/src/@planx/components/MapAndLabel/Public/index.tsx index 4b260271a7..f55921d07f 100644 --- a/editor.planx.uk/src/@planx/components/MapAndLabel/Public/index.tsx +++ b/editor.planx.uk/src/@planx/components/MapAndLabel/Public/index.tsx @@ -8,8 +8,8 @@ import Tab, { tabClasses, TabProps } from "@mui/material/Tab"; import Tabs from "@mui/material/Tabs"; import Typography from "@mui/material/Typography"; import { SiteAddress } from "@planx/components/FindProperty/model"; -import { ErrorSummaryContainer } from "@planx/components/shared/Preview/ErrorSummaryContainer"; import { SchemaFields } from "@planx/components/shared/Schema/SchemaFields"; +import { GraphError } from "components/Error/GraphError"; import { GeoJsonObject } from "geojson"; import sortBy from "lodash/sortBy"; import { useStore } from "pages/FlowEditor/lib/store"; @@ -305,24 +305,6 @@ export const Presentational: React.FC = (props) => ( ); -const GraphError = (props: Props) => ( - - - - - Invalid graph - - - Edit this flow so that "MapAndLabel" is positioned after "FindProperty"; - an initial address is required to correctly display the map. - - - -); - function MapAndLabelComponent(props: Props) { const teamSettings = useStore.getState().teamSettings; const passport = useStore((state) => state.computePassport()); @@ -330,7 +312,7 @@ function MapAndLabelComponent(props: Props) { (passport?.data?._address as SiteAddress) || {}; if (!latitude || !longitude) { - return ; + throw new GraphError("nodeMustFollowFindProperty"); } return ( diff --git a/editor.planx.uk/src/@planx/components/PlanningConstraints/Public.test.tsx b/editor.planx.uk/src/@planx/components/PlanningConstraints/Public.test.tsx index 2eefae94fe..256dff7063 100644 --- a/editor.planx.uk/src/@planx/components/PlanningConstraints/Public.test.tsx +++ b/editor.planx.uk/src/@planx/components/PlanningConstraints/Public.test.tsx @@ -1,5 +1,6 @@ -import { screen } from "@testing-library/react"; +import ErrorFallback from "components/Error/ErrorFallback"; import React from "react"; +import { ErrorBoundary } from "react-error-boundary"; import { setup } from "testUtils"; import { vi } from "vitest"; import { axe } from "vitest-axe"; @@ -9,7 +10,7 @@ import digitalLandResponseMock from "./mocks/digitalLandResponseMock"; import PlanningConstraints from "./Public"; vi.mock("swr", () => ({ - default: vi.fn((url: any) => { + default: vi.fn((url: () => string) => { const isGISRequest = url()?.startsWith( `${import.meta.env.VITE_APP_API_URL}/gis/`, ); @@ -17,47 +18,54 @@ vi.mock("swr", () => ({ `${import.meta.env.VITE_APP_API_URL}/roads/`, ); - if (isGISRequest) return { data: digitalLandResponseMock } as any; - if (isRoadsRequest) return { data: classifiedRoadsResponseMock } as any; + if (isGISRequest) return { data: digitalLandResponseMock }; + if (isRoadsRequest) return { data: classifiedRoadsResponseMock }; return { data: null }; }), })); -it("renders correctly", async () => { - const handleSubmit = vi.fn(); +describe("error state", () => { + it("renders an error if no addres is present in the passport", async () => { + const handleSubmit = vi.fn(); - const { user } = setup( - , - ); - - expect(screen.getByText("Planning constraints")).toBeInTheDocument(); + const { getByRole, getByTestId } = setup( + + + , + , + ); - // TODO mock passport _address so that SWR request is actually triggered to return mock response - expect(screen.getByTestId("error-summary-invalid-graph")).toBeInTheDocument(); + expect(getByTestId("error-summary-invalid-graph")).toBeInTheDocument(); + expect(getByRole("heading", { name: "Invalid graph" })).toBeInTheDocument(); + }); - await user.click(screen.getByTestId("continue-button")); - expect(handleSubmit).toHaveBeenCalledTimes(1); + it("should not have any accessibility violations", async () => { + const { container } = setup( + + + , + , + ); + const results = await axe(container); + expect(results).toHaveNoViolations(); + }); }); -it("should not have any accessibility violations", async () => { - const { container } = setup( - , - ); - const results = await axe(container); - expect(results).toHaveNoViolations(); -}); +it.todo("renders correctly"); + +it.todo("should not have any accessibility violations"); it.todo("fetches classified roads only when we have a siteBoundary"); // using expect(spy).toHaveBeenCalled() ?? diff --git a/editor.planx.uk/src/@planx/components/PlanningConstraints/Public.tsx b/editor.planx.uk/src/@planx/components/PlanningConstraints/Public.tsx index f93b0a1edc..513961d632 100644 --- a/editor.planx.uk/src/@planx/components/PlanningConstraints/Public.tsx +++ b/editor.planx.uk/src/@planx/components/PlanningConstraints/Public.tsx @@ -9,6 +9,7 @@ import Card from "@planx/components/shared/Preview/Card"; import CardHeader from "@planx/components/shared/Preview/CardHeader"; import type { PublicProps } from "@planx/components/ui"; import DelayedLoadingIndicator from "components/DelayedLoadingIndicator"; +import { GraphError } from "components/Error/GraphError"; import capitalize from "lodash/capitalize"; import { useStore } from "pages/FlowEditor/lib/store"; import { HandleSubmit } from "pages/Preview/Node"; @@ -63,6 +64,8 @@ function Component(props: Props) { // PlanningConstraints must come after at least a FindProperty in the graph const showGraphError = !x || !y || !longitude || !latitude; + if (showGraphError) + throw new GraphError("mapInputFieldMustFollowFindProperty"); // Even though this component will fetch fresh GIS data when coming "back", // still prepopulate any previously marked inaccurateConstraints @@ -145,8 +148,6 @@ function Component(props: Props) { ...roads?.metadata, }; - if (showGraphError) return ; - const isLoading = isValidating || isValidatingRoads; if (isLoading) return ( @@ -396,28 +397,3 @@ const ConstraintsFetchError = (props: ConstraintsFetchErrorProps) => ( ); - -interface ConstraintsGraphErrorProps { - title: string; - description: string; - handleSubmit?: HandleSubmit; -} - -const ConstraintsGraphError = (props: ConstraintsGraphErrorProps) => ( - - - - - Invalid graph - - - Edit this flow so that "Planning constraints" is positioned after "Find - property"; an address or site boundary drawing is required to fetch - data. - - - -); diff --git a/editor.planx.uk/src/@planx/components/PropertyInformation/Public.test.tsx b/editor.planx.uk/src/@planx/components/PropertyInformation/Public.test.tsx index 318aaf37f2..2e35f47189 100644 --- a/editor.planx.uk/src/@planx/components/PropertyInformation/Public.test.tsx +++ b/editor.planx.uk/src/@planx/components/PropertyInformation/Public.test.tsx @@ -1,6 +1,8 @@ import { MockedProvider } from "@apollo/client/testing"; import { screen } from "@testing-library/react"; +import ErrorFallback from "components/Error/ErrorFallback"; import React from "react"; +import { ErrorBoundary } from "react-error-boundary"; import { setup } from "testUtils"; import { vi } from "vitest"; @@ -18,10 +20,12 @@ const defaultPresentationalProps: PresentationalProps = { test("renders a warning for editors if address data is not in state", async () => { setup( - + + + , ); diff --git a/editor.planx.uk/src/@planx/components/PropertyInformation/Public.tsx b/editor.planx.uk/src/@planx/components/PropertyInformation/Public.tsx index 5502fc42aa..6b6bfc17d9 100644 --- a/editor.planx.uk/src/@planx/components/PropertyInformation/Public.tsx +++ b/editor.planx.uk/src/@planx/components/PropertyInformation/Public.tsx @@ -1,12 +1,12 @@ import { useQuery } from "@apollo/client"; import Box from "@mui/material/Box"; import Link from "@mui/material/Link"; -import Typography from "@mui/material/Typography"; import { visuallyHidden } from "@mui/utils"; import Card from "@planx/components/shared/Preview/Card"; import CardHeader from "@planx/components/shared/Preview/CardHeader"; import { SummaryListTable } from "@planx/components/shared/Preview/SummaryList"; import type { PublicProps } from "@planx/components/ui"; +import { GraphError } from "components/Error/GraphError"; import { Feature } from "geojson"; import { publicClient } from "lib/graphql"; import find from "lodash/find"; @@ -17,7 +17,6 @@ import React from "react"; import type { SiteAddress } from "../FindProperty/model"; import { FETCH_BLPU_CODES } from "../FindProperty/Public"; -import { ErrorSummaryContainer } from "../shared/Preview/ErrorSummaryContainer"; import { MapContainer } from "../shared/Preview/MapContainer"; import type { PropertyInformation } from "./model"; @@ -32,7 +31,10 @@ function Component(props: PublicProps) { client: publicClient, }); - return passport.data?._address ? ( + if (!passport.data?._address) + throw new GraphError("nodeMustFollowFindProperty"); + + return ( ) { }); }} /> - ) : ( - - - - Invalid graph - - - Edit this flow so that "Property information" is positioned after - "Find property"; an address is required to render. - - - ); } diff --git a/editor.planx.uk/src/@planx/components/shared/Schema/InputFields/MapFieldInput.tsx b/editor.planx.uk/src/@planx/components/shared/Schema/InputFields/MapFieldInput.tsx index 765ef75489..7ddc054bc9 100644 --- a/editor.planx.uk/src/@planx/components/shared/Schema/InputFields/MapFieldInput.tsx +++ b/editor.planx.uk/src/@planx/components/shared/Schema/InputFields/MapFieldInput.tsx @@ -1,6 +1,8 @@ import Box from "@mui/material/Box"; +import { SiteAddress } from "@opensystemslab/planx-core/types"; import { MapContainer } from "@planx/components/shared/Preview/MapContainer"; import type { MapField } from "@planx/components/shared/Schema/model"; +import { GraphError } from "components/Error/GraphError"; import { Feature } from "geojson"; import { useStore } from "pages/FlowEditor/lib/store"; import React, { useEffect, useState } from "react"; @@ -11,6 +13,15 @@ import { getFieldProps, Props } from "."; import { FieldInputDescription } from "./shared"; export const MapFieldInput: React.FC> = (props) => { + // Ensure there's a FindProperty component preceding this field (eg address data in state to position map view) + const { longitude, latitude } = useStore( + (state) => + (state.computePassport()?.data?.["_address"] as SiteAddress) || {}, + ); + + if (!longitude || !latitude) + throw new GraphError("mapInputFieldMustFollowFindProperty"); + const { formik, data: { title, description, mapOptions }, diff --git a/editor.planx.uk/src/components/ErrorFallback.stories.tsx b/editor.planx.uk/src/components/Error/ErrorFallback.stories.tsx similarity index 100% rename from editor.planx.uk/src/components/ErrorFallback.stories.tsx rename to editor.planx.uk/src/components/Error/ErrorFallback.stories.tsx diff --git a/editor.planx.uk/src/components/ErrorFallback.tsx b/editor.planx.uk/src/components/Error/ErrorFallback.tsx similarity index 70% rename from editor.planx.uk/src/components/ErrorFallback.tsx rename to editor.planx.uk/src/components/Error/ErrorFallback.tsx index 68a85ff170..ad9e822d91 100644 --- a/editor.planx.uk/src/components/ErrorFallback.tsx +++ b/editor.planx.uk/src/components/Error/ErrorFallback.tsx @@ -1,12 +1,15 @@ import Typography from "@mui/material/Typography"; import Card from "@planx/components/shared/Preview/Card"; import { ErrorSummaryContainer } from "@planx/components/shared/Preview/ErrorSummaryContainer"; +import { logger } from "airbrake"; import React from "react"; -import { logger } from "../airbrake"; +import { GraphErrorComponent, isGraphError } from "./GraphError"; -function ErrorFallback(props: { error: Error }) { - logger.notify(props.error); +const ErrorFallback: React.FC<{ error: Error }> = ({ error }) => { + if (isGraphError(error)) return ; + + logger.notify(error); return ( @@ -15,9 +18,9 @@ function ErrorFallback(props: { error: Error }) { Something went wrong - {props.error?.message && ( + {error.message && ( 
-              {props.error.message}
+              {error.message}
)} @@ -27,6 +30,6 @@ function ErrorFallback(props: { error: Error }) { ); -} +}; export default ErrorFallback; diff --git a/editor.planx.uk/src/components/Error/GraphError.test.tsx b/editor.planx.uk/src/components/Error/GraphError.test.tsx new file mode 100644 index 0000000000..ec79de8dfc --- /dev/null +++ b/editor.planx.uk/src/components/Error/GraphError.test.tsx @@ -0,0 +1,81 @@ +import { logger } from "airbrake"; +import ErrorFallback from "components/Error/ErrorFallback"; +import React from "react"; +import { ErrorBoundary } from "react-error-boundary"; +import { setup } from "testUtils"; +import { vi } from "vitest"; +import { axe } from "vitest-axe"; + +import { GraphError } from "./GraphError"; + +vi.mock("airbrake", () => ({ + logger: { + notify: vi.fn(), + }, +})); + +const ThrowError: React.FC = () => { + throw new Error("Something broke"); +}; + +const ThrowGraphError: React.FC = () => { + throw new GraphError("nodeMustFollowFindProperty"); +}; + +it("does not render if a child does not throw an error", () => { + const { queryByRole } = setup( + +

No error

+ , + ); + expect( + queryByRole("heading", { name: /Invalid graph/ }), + ).not.toBeInTheDocument(); +}); + +it("does not render if a child throws a non-Graph error", () => { + const { queryByRole, getByText } = setup( + + + , + ); + // ErrorFallback displays... + expect(getByText(/Something went wrong/)).toBeInTheDocument(); + // ...but does not show a GraphError + expect( + queryByRole("heading", { name: /Invalid graph/ }), + ).not.toBeInTheDocument(); +}); + +it("renders if a child throws an error", () => { + const { queryByText, getByRole } = setup( + + + , + ); + + expect(queryByText(/Something went wrong/)).not.toBeInTheDocument(); + expect(getByRole("heading", { name: /Invalid graph/ })).toBeInTheDocument(); +}); + +it("does not call Airbrake", () => { + const loggerSpy = vi.spyOn(logger, "notify"); + + setup( + + + , + ); + + expect(loggerSpy).not.toHaveBeenCalled(); +}); + +it("should not have accessability violations", async () => { + const { container } = setup( + + + , + ); + const results = await axe(container); + expect(results).toHaveNoViolations(); +}); diff --git a/editor.planx.uk/src/components/Error/GraphError.tsx b/editor.planx.uk/src/components/Error/GraphError.tsx new file mode 100644 index 0000000000..2b86e7aaef --- /dev/null +++ b/editor.planx.uk/src/components/Error/GraphError.tsx @@ -0,0 +1,43 @@ +import Typography from "@mui/material/Typography"; +import Card from "@planx/components/shared/Preview/Card"; +import { ErrorSummaryContainer } from "@planx/components/shared/Preview/ErrorSummaryContainer"; +import React from "react"; + +type GraphErrorType = + | "nodeMustFollowFindProperty" + | "mapInputFieldMustFollowFindProperty"; + +const GRAPH_ERROR_MESSAGES: Record = { + nodeMustFollowFindProperty: + 'Edit this flow so that this node is positioned after "Find property"; an address or site boundary drawing is required to fetch data', + mapInputFieldMustFollowFindProperty: + 'Edit this flow so that this component is positioned after "FindProperty"; an address is required for schemas that include a "map" field.', +}; + +export class GraphError extends Error { + constructor(public type: GraphErrorType) { + super(); + this.type = type; + } +} + +export const isGraphError = (error: unknown): error is GraphError => + error instanceof GraphError; + +export const GraphErrorComponent: React.FC<{ error: GraphError }> = ({ + error, +}) => ( + + + + Invalid graph + + + {GRAPH_ERROR_MESSAGES[error.type]} + + + +); diff --git a/editor.planx.uk/src/pages/FlowEditor/components/Settings/Submissions/EventsLog.tsx b/editor.planx.uk/src/pages/FlowEditor/components/Settings/Submissions/EventsLog.tsx index 4de7b76aac..29d1994471 100644 --- a/editor.planx.uk/src/pages/FlowEditor/components/Settings/Submissions/EventsLog.tsx +++ b/editor.planx.uk/src/pages/FlowEditor/components/Settings/Submissions/EventsLog.tsx @@ -15,7 +15,7 @@ import TableHead from "@mui/material/TableHead"; import TableRow from "@mui/material/TableRow"; import Typography from "@mui/material/Typography"; import DelayedLoadingIndicator from "components/DelayedLoadingIndicator"; -import ErrorFallback from "components/ErrorFallback"; +import ErrorFallback from "components/Error/ErrorFallback"; import { format } from "date-fns"; import React, { useState } from "react"; import ErrorSummary from "ui/shared/ErrorSummary"; diff --git a/editor.planx.uk/src/pages/FlowEditor/components/forms/FormModal.tsx b/editor.planx.uk/src/pages/FlowEditor/components/forms/FormModal.tsx index 9e400b8267..a937b8d259 100644 --- a/editor.planx.uk/src/pages/FlowEditor/components/forms/FormModal.tsx +++ b/editor.planx.uk/src/pages/FlowEditor/components/forms/FormModal.tsx @@ -9,7 +9,7 @@ import IconButton from "@mui/material/IconButton"; import { styled } from "@mui/material/styles"; import { ComponentType as TYPES } from "@opensystemslab/planx-core/types"; import { parseFormValues } from "@planx/components/shared"; -import ErrorFallback from "components/ErrorFallback"; +import ErrorFallback from "components/Error/ErrorFallback"; import { hasFeatureFlag } from "lib/featureFlags"; import React from "react"; import { ErrorBoundary } from "react-error-boundary"; diff --git a/editor.planx.uk/src/pages/Preview/Questions.tsx b/editor.planx.uk/src/pages/Preview/Questions.tsx index f287939af9..1819922734 100644 --- a/editor.planx.uk/src/pages/Preview/Questions.tsx +++ b/editor.planx.uk/src/pages/Preview/Questions.tsx @@ -12,7 +12,7 @@ import React, { useCallback, useEffect, useMemo, useState } from "react"; import { ErrorBoundary } from "react-error-boundary"; import { ApplicationPath, Session } from "types"; -import ErrorFallback from "../../components/ErrorFallback"; +import ErrorFallback from "../../components/Error/ErrorFallback"; import { useStore } from "../FlowEditor/lib/store"; import Node, { HandleSubmit } from "./Node"; diff --git a/editor.planx.uk/src/pages/layout/FlowEditorLayout.tsx b/editor.planx.uk/src/pages/layout/FlowEditorLayout.tsx index 327d8da975..d1d9e6eada 100644 --- a/editor.planx.uk/src/pages/layout/FlowEditorLayout.tsx +++ b/editor.planx.uk/src/pages/layout/FlowEditorLayout.tsx @@ -1,4 +1,4 @@ -import ErrorFallback from "components/ErrorFallback"; +import ErrorFallback from "components/Error/ErrorFallback"; import FlowEditor from "pages/FlowEditor"; import React, { PropsWithChildren } from "react"; import { ErrorBoundary } from "react-error-boundary"; diff --git a/editor.planx.uk/src/pages/layout/PublicLayout.tsx b/editor.planx.uk/src/pages/layout/PublicLayout.tsx index 23df5c9232..f7b21a23b1 100644 --- a/editor.planx.uk/src/pages/layout/PublicLayout.tsx +++ b/editor.planx.uk/src/pages/layout/PublicLayout.tsx @@ -6,7 +6,7 @@ import { ThemeProvider, } from "@mui/material/styles"; import Typography from "@mui/material/Typography"; -import ErrorFallback from "components/ErrorFallback"; +import ErrorFallback from "components/Error/ErrorFallback"; import Feedback from "components/Feedback"; import { useStore } from "pages/FlowEditor/lib/store"; import React, { PropsWithChildren } from "react";