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

feat: Filters support choosing any flagset category #3797

Merged
merged 7 commits into from
Oct 11, 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
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ test.describe("Flow creation, publish and preview", () => {
await expect(editor.nodeList).toContainText([
"Find property",
"an internal portalEdit Portal",
"(Flags Filter)ImmuneMissing informationPermission neededPrior approvalNoticePermitted developmentNot development(No Result)",
"Filter - Planning permissionImmuneMissing informationPermission neededPrior approvalNoticePermitted developmentNot developmentNo flag result",
"Upload and label",
"Confirm your location plan",
"Planning constraints",
Expand Down
141 changes: 141 additions & 0 deletions editor.planx.uk/src/@planx/components/Filter/Editor.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
import {
ComponentType as TYPES,
DEFAULT_FLAG_CATEGORY,
flatFlags,
} from "@opensystemslab/planx-core/types";
import { fireEvent, screen, waitFor } from "@testing-library/react";
import React from "react";
import { setup } from "testUtils";
import { vi } from "vitest";

import Filter from "./Editor";

test("Adding a filter without explicit props uses the default flagset", async () => {
const handleSubmit = vi.fn();

setup(<Filter handleSubmit={handleSubmit} />);

expect(screen.getByTestId("flagset-category-select")).toHaveValue(
DEFAULT_FLAG_CATEGORY,
);

fireEvent.submit(screen.getByTestId("filter-component-form"));

await waitFor(() =>
expect(handleSubmit).toHaveBeenCalledWith(
{
type: TYPES.Filter,
data: {
fn: "flag",
category: DEFAULT_FLAG_CATEGORY,
},
},
mockDefaultFlagOptions,
),
);
});

test("Adding a filter and selecting a flagset category", async () => {
const handleSubmit = vi.fn();

setup(<Filter handleSubmit={handleSubmit} />);

expect(screen.getByTestId("flagset-category-select")).toHaveValue(
DEFAULT_FLAG_CATEGORY,
);

fireEvent.change(screen.getByTestId("flagset-category-select"), {
target: { value: "Community infrastructure levy" },
Copy link
Contributor

Choose a reason for hiding this comment

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

question: why would we use fireEvent here rather than userEvent?

Does it make selecting from a <select> easier?
(rather than going through user.click, find the options, user.click again...)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using fireEvent throughout this file because the modal submit buttons like "Create filter" / "Update filter" aren't easily accessible to user.click() - therefore fireEvent.submit() is handy to have (this is commonly used throughout other "Editor" test files as well).

I think the select interaction specifically could be migrated to userEvent going forward - which would mean we'd be testing the interaction more realistically to how the user would experience it in the browser.

});

fireEvent.submit(screen.getByTestId("filter-component-form"));

await waitFor(() =>
expect(handleSubmit).toHaveBeenCalledWith(
{
type: TYPES.Filter,
data: {
fn: "flag",
category: "Community infrastructure levy",
},
},
mockCILFlagOptions,
),
);
});

test("Updating an existing filter to another category", async () => {
const handleSubmit = vi.fn();

setup(
<Filter
id="filterNodeId"
node={mockExistingFilterNode}
handleSubmit={handleSubmit}
/>,
);

expect(screen.getByTestId("flagset-category-select")).toHaveValue(
"Listed building consent",
);

fireEvent.change(screen.getByTestId("flagset-category-select"), {
target: { value: "Community infrastructure levy" },
});

fireEvent.submit(screen.getByTestId("filter-component-form"));

await waitFor(() =>
expect(handleSubmit).toHaveBeenCalledWith(
{
type: TYPES.Filter,
data: {
fn: "flag",
category: "Community infrastructure levy",
},
},
mockCILFlagOptions,
),
);
});

const mockExistingFilterNode = {
data: {
fn: "flag",
category: "Listed building consent",
},
type: 500,
edges: ["flag1", "flag2", "flag3", "flag4", "blankFlag"],
};

const mockDefaultFlagOptions = [
...flatFlags.filter((flag) => flag.category === DEFAULT_FLAG_CATEGORY),
{
category: DEFAULT_FLAG_CATEGORY,
text: "No flag result",
value: "",
},
].map((flag) => ({
type: TYPES.Answer,
data: {
text: flag.text,
val: flag.value,
},
}));

const mockCILFlagOptions = [
...flatFlags.filter(
(flag) => flag.category === "Community infrastructure levy",
),
{
category: "Community infrastructure levy",
text: "No flag result",
value: "",
},
].map((flag) => ({
type: TYPES.Answer,
data: {
text: flag.text,
val: flag.value,
},
}));
90 changes: 61 additions & 29 deletions editor.planx.uk/src/@planx/components/Filter/Editor.tsx
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd expect the Editor model to be similar to the Result component, where I can see the filter options before updating. I am an uneducated user, so it is feasible that this is redundant for people who know what to expect from the filters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filter options are not editable, they are hard-coded only still - therefore in my opinion there's no reason to display them in the modal here (they'd have to be disabled fields?).

In the future, when we may have customisable filter options (aka flags and flagsets), then I imagine it will make sense to expose them directly here rather select from a pre-existing/pre-configured category ! But that's out of scope of these changes.

Original file line number Diff line number Diff line change
@@ -1,52 +1,84 @@
import Typography from "@mui/material/Typography";
import {
ComponentType as TYPES,
DEFAULT_FLAG_CATEGORY,
flatFlags,
} from "@opensystemslab/planx-core/types";
import { ComponentType as TYPES } from "@opensystemslab/planx-core/types";
import { useFormik } from "formik";
import React from "react";
import ModalSection from "ui/editor/ModalSection";
import ModalSectionContent from "ui/editor/ModalSectionContent";

import { ICONS } from "../ui";

export interface Props {
id?: string;
handleSubmit?: (d: any, c?: any) => void;
handleSubmit?: (data: any, children?: any) => void;
node?: any;
}

const Filter: React.FC<Props> = (props) => {
const formik = useFormik({
initialValues: {},
initialValues: {
fn: "flag",
category: props?.node?.data?.category || DEFAULT_FLAG_CATEGORY,
},
onSubmit: (newValues) => {
if (props.handleSubmit) {
const children = props.id
? undefined
: [
...flatFlags,
{
category: DEFAULT_FLAG_CATEGORY,
text: "(No Result)",
value: "",
},
]
.filter((f) => f.category === DEFAULT_FLAG_CATEGORY)
.map((f) => ({
type: TYPES.Answer,
data: {
text: f.text,
val: f.value,
},
}));
if (props?.handleSubmit) {
const children = [
...flatFlags,
{
category: formik.values.category,
text: "No flag result",
value: "",
},
]
.filter((f) => f.category === formik.values.category)
.map((f) => ({
type: TYPES.Answer,
data: {
text: f.text,
val: f.value,
},
}));

props.handleSubmit(
{ type: TYPES.Filter, data: { newValues, fn: "flag" } },
children,
);
props.handleSubmit({ type: TYPES.Filter, data: newValues }, children);
}
},
validate: () => {},
});

const categories = new Set(flatFlags.map((flag) => flag.category));

return (
<form onSubmit={formik.handleSubmit} id="modal">
<h1>Filter Component</h1>
<form
onSubmit={formik.handleSubmit}
id="modal"
data-testid="filter-component-form"
>
<ModalSection>
<ModalSectionContent title="Filter" Icon={ICONS[TYPES.Filter]}>
<Typography variant="body2">
Filters automatically sort based on collected flags. Flags within a
category are ordered heirarchically and the filter will route
through the left-most matching flag option only.
</Typography>
</ModalSectionContent>
<ModalSectionContent title="Pick a flagset category (coming soon)">
<select
data-testid="flagset-category-select"
name="category"
value={formik.values.category}
onChange={formik.handleChange}
disabled
>
Copy link
Member Author

Choose a reason for hiding this comment

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

My current thinking is that we can merge this to main with a simple disabled prop added here?

Then I can rebase into #3764, correctly adjust filter automations, and un-disable? Does this sound solid, anything I might be forgetting?

Please test flows on this pizza with existing filter components to confirm they work correctly before any type of node "Update" !

{Array.from(categories).map((category) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

First time seeing this, its so slick with the new Set() above!

<option key={category} value={category}>
{category}
</option>
))}
</select>
</ModalSectionContent>
</ModalSection>
</form>
);
};
Expand Down
5 changes: 0 additions & 5 deletions editor.planx.uk/src/@planx/components/Filter/model.ts

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { ComponentType as TYPES } from "@opensystemslab/planx-core/types";
import {
ComponentType as TYPES,
DEFAULT_FLAG_CATEGORY,
} from "@opensystemslab/planx-core/types";
import React from "react";
import { ErrorBoundary } from "react-error-boundary";
import { exhaustiveCheck } from "utils";
Expand Down Expand Up @@ -63,7 +66,12 @@ const Node: React.FC<any> = (props) => {
/>
);
case TYPES.Filter:
return <Filter {...allProps} text="(Flags Filter)" />;
return (
<Filter
{...allProps}
text={`Filter - ${node?.data?.category || DEFAULT_FLAG_CATEGORY}`}
/>
);
case TYPES.FindProperty:
return <Question {...allProps} text="Find property" />;
case TYPES.List:
Expand Down
Loading