Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Move "invalid graph" warning to MapFieldInput #3689

Merged
merged 4 commits into from
Sep 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
44 changes: 4 additions & 40 deletions editor.planx.uk/src/@planx/components/List/Public/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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<HTMLDivElement>(null);
useEffect(() => {
Expand Down Expand Up @@ -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");

Expand Down Expand Up @@ -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)`) ||
Expand All @@ -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 (
<Card handleSubmit={handleSubmit} isValid>
<CardHeader title={title} description={description} />
<ErrorSummaryContainer
role="status"
data-testid="error-summary-invalid-graph"
>
<Typography variant="h4" component="h2" gutterBottom>
Invalid graph
</Typography>
<Typography variant="body2">
Edit this flow so that "List" is positioned after "FindProperty"; an
address is required for schemas that include a "map" field.
</Typography>
</ErrorSummaryContainer>
</Card>
);
}

const listContent = (
<ErrorWrapper error={rootError}>
Expand Down
22 changes: 2 additions & 20 deletions editor.planx.uk/src/@planx/components/MapAndLabel/Public/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -305,32 +305,14 @@ export const Presentational: React.FC<PresentationalProps> = (props) => (
</MapAndLabelProvider>
);

const GraphError = (props: Props) => (
<Card handleSubmit={props.handleSubmit} isValid>
<CardHeader title={props.title} description={props.description} />
<ErrorSummaryContainer
role="status"
data-testid="error-summary-invalid-graph"
>
<Typography variant="h4" component="h2" gutterBottom>
Invalid graph
</Typography>
<Typography variant="body2">
Edit this flow so that "MapAndLabel" is positioned after "FindProperty";
an initial address is required to correctly display the map.
</Typography>
</ErrorSummaryContainer>
</Card>
);

function MapAndLabelComponent(props: Props) {
const teamSettings = useStore.getState().teamSettings;
const passport = useStore((state) => state.computePassport());
const { latitude, longitude } =
(passport?.data?._address as SiteAddress) || {};

if (!latitude || !longitude) {
return <GraphError {...props} />;
throw new GraphError("nodeMustFollowFindProperty");
}

return (
Expand Down
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's some basic tests here currently missing, I'll see if I can set this up in a follow up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -9,55 +10,62 @@ 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/`,
);
const isRoadsRequest = url()?.startsWith(
`${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(
<PlanningConstraints
title="Planning constraints"
description="Things that might affect your project"
fn="property.constraints.planning"
disclaimer="This page does not include information about historic planning conditions that may apply to this property."
handleSubmit={handleSubmit}
/>,
);

expect(screen.getByText("Planning constraints")).toBeInTheDocument();
const { getByRole, getByTestId } = setup(
<ErrorBoundary FallbackComponent={ErrorFallback}>
<PlanningConstraints
title="Planning constraints"
description="Things that might affect your project"
fn="property.constraints.planning"
disclaimer="This page does not include information about historic planning conditions that may apply to this property."
handleSubmit={handleSubmit}
/>
,
</ErrorBoundary>,
);

// 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(
<ErrorBoundary FallbackComponent={ErrorFallback}>
<PlanningConstraints
title="Planning constraints"
description="Things that might affect your project"
fn="property.constraints.planning"
disclaimer="This page does not include information about historic planning conditions that may apply to this property."
/>
,
</ErrorBoundary>,
);
const results = await axe(container);
expect(results).toHaveNoViolations();
});
});

it("should not have any accessibility violations", async () => {
const { container } = setup(
<PlanningConstraints
title="Planning constraints"
description="Things that might affect your project"
fn="property.constraints.planning"
disclaimer="This page does not include information about historic planning conditions that may apply to this property."
/>,
);
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() ??

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -145,8 +148,6 @@ function Component(props: Props) {
...roads?.metadata,
};

if (showGraphError) return <ConstraintsGraphError {...props} />;

const isLoading = isValidating || isValidatingRoads;
if (isLoading)
return (
Expand Down Expand Up @@ -396,28 +397,3 @@ const ConstraintsFetchError = (props: ConstraintsFetchErrorProps) => (
</ErrorSummaryContainer>
</Card>
);

interface ConstraintsGraphErrorProps {
title: string;
description: string;
handleSubmit?: HandleSubmit;
}

const ConstraintsGraphError = (props: ConstraintsGraphErrorProps) => (
<Card handleSubmit={props.handleSubmit} isValid>
<CardHeader title={props.title} description={props.description} />
<ErrorSummaryContainer
role="status"
data-testid="error-summary-invalid-graph"
>
<Typography variant="h4" component="h2" gutterBottom>
Invalid graph
</Typography>
<Typography variant="body2">
Edit this flow so that "Planning constraints" is positioned after "Find
property"; an address or site boundary drawing is required to fetch
data.
</Typography>
</ErrorSummaryContainer>
</Card>
);
Original file line number Diff line number Diff line change
@@ -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";

Expand All @@ -18,10 +20,12 @@ const defaultPresentationalProps: PresentationalProps = {
test("renders a warning for editors if address data is not in state", async () => {
setup(
<MockedProvider>
<PropertyInformation
title="About the property"
description="This is the information we currently have about the property"
/>
<ErrorBoundary FallbackComponent={ErrorFallback}>
<PropertyInformation
title="About the property"
description="This is the information we currently have about the property"
/>
</ErrorBoundary>
</MockedProvider>,
);

Expand Down
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -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";

Expand All @@ -32,7 +31,10 @@ function Component(props: PublicProps<PropertyInformation>) {
client: publicClient,
});

return passport.data?._address ? (
if (!passport.data?._address)
throw new GraphError("nodeMustFollowFindProperty");

return (
<Presentational
title={props.title}
description={props.description}
Expand Down Expand Up @@ -60,21 +62,6 @@ function Component(props: PublicProps<PropertyInformation>) {
});
}}
/>
) : (
<Card>
<ErrorSummaryContainer
role="status"
data-testid="error-summary-invalid-graph"
>
<Typography variant="h4" component="h2" gutterBottom>
Invalid graph
</Typography>
<Typography variant="body2">
Edit this flow so that "Property information" is positioned after
"Find property"; an address is required to render.
</Typography>
</ErrorSummaryContainer>
</Card>
);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -11,6 +13,15 @@ import { getFieldProps, Props } from ".";
import { FieldInputDescription } from "./shared";

export const MapFieldInput: React.FC<Props<MapField>> = (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 },
Expand Down
Loading
Loading