From 372cbd3e5d211937b84d02a85a498fa225012db8 Mon Sep 17 00:00:00 2001
From: Alexander Polyankin <alexander.polyankin@metabase.com>
Date: Mon, 28 Oct 2024 09:52:05 -0400
Subject: [PATCH] Invert exclude filter checkbox state (#49178)
---
...dashboard-filters-reproductions.cy.spec.js | 7 +-
.../DatePicker/DatePicker.unit.spec.tsx | 4 +-
.../pickers/DatePicker/ExcludeDatePicker.tsx | 25 ++--
.../ExcludeDatePicker.unit.spec.tsx | 119 ++++++++++++++----
.../parameters/utils/date-formatting.ts | 2 +-
.../ExcludeDatePicker/ExcludeDatePicker.tsx | 30 ++---
.../ExcludeDatePicker.unit.spec.tsx | 64 ++++++++--
7 files changed, 179 insertions(+), 72 deletions(-)
diff --git a/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js b/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js
index 9c897490c9a..1d8b7b88c59 100644
--- a/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js
+++ b/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js
@@ -1410,13 +1410,13 @@ describe("issue 24235", () => {
popover().within(() => {
cy.findByText("Exclude...").click();
cy.findByText("Days of the week...").click();
- cy.findByText("Select none...").click();
+ cy.findByText("Select all").click();
cy.findByText("Add filter").click();
});
filterWidget().click();
popover().within(() => {
- cy.findByText("Select all...").click();
+ cy.findByText("Select none").click();
cy.findByText("Update filter").click();
});
@@ -3013,9 +3013,10 @@ describe("issue 27579", () => {
popover().within(() => {
cy.findByText("Exclude...").click();
cy.findByText("Hours of the day...").click();
+ cy.findByText("Select all").click();
cy.findByLabelText("12 AM").should("be.checked");
- cy.findByText("Select none...").click();
+ cy.findByText("Select none").click();
cy.findByLabelText("12 AM").should("not.be.checked");
});
});
diff --git a/frontend/src/metabase/admin/datamodel/components/filters/pickers/DatePicker/DatePicker.unit.spec.tsx b/frontend/src/metabase/admin/datamodel/components/filters/pickers/DatePicker/DatePicker.unit.spec.tsx
index 94e9b0b2a71..ada68e5006f 100644
--- a/frontend/src/metabase/admin/datamodel/components/filters/pickers/DatePicker/DatePicker.unit.spec.tsx
+++ b/frontend/src/metabase/admin/datamodel/components/filters/pickers/DatePicker/DatePicker.unit.spec.tsx
@@ -419,7 +419,7 @@ describe("DatePicker", () => {
name: /12 AM/i,
});
- expect(midnightCheckbox).toBeChecked();
+ expect(midnightCheckbox).not.toBeChecked();
await userEvent.click(midnightCheckbox);
@@ -429,7 +429,7 @@ describe("DatePicker", () => {
0,
]);
- expect(midnightCheckbox).not.toBeChecked();
+ expect(midnightCheckbox).toBeChecked();
});
});
});
diff --git a/frontend/src/metabase/admin/datamodel/components/filters/pickers/DatePicker/ExcludeDatePicker.tsx b/frontend/src/metabase/admin/datamodel/components/filters/pickers/DatePicker/ExcludeDatePicker.tsx
index 91757b7019b..a0ae3cb377d 100644
--- a/frontend/src/metabase/admin/datamodel/components/filters/pickers/DatePicker/ExcludeDatePicker.tsx
+++ b/frontend/src/metabase/admin/datamodel/components/filters/pickers/DatePicker/ExcludeDatePicker.tsx
@@ -38,7 +38,7 @@ type Group = {
displayName: string;
init: (filter: Filter) => any[];
test: (filter: Filter) => boolean;
- getOptions: () => Option[][];
+ getOptionGroups: () => Option[][];
};
export const EXCLUDE_OPERATORS: Group[] = [
@@ -47,28 +47,28 @@ export const EXCLUDE_OPERATORS: Group[] = [
displayName: t`Days of the week...`,
test: filter => isDayOfWeekDateFilter(filter),
init: filter => getInitialDayOfWeekFilter(filter),
- getOptions: EXCLUDE_OPTIONS["day-of-week"],
+ getOptionGroups: EXCLUDE_OPTIONS["day-of-week"],
},
{
name: "months",
displayName: t`Months of the year...`,
test: filter => isMonthOfYearDateFilter(filter),
init: filter => getInitialMonthOfYearFilter(filter),
- getOptions: EXCLUDE_OPTIONS["month-of-year"],
+ getOptionGroups: EXCLUDE_OPTIONS["month-of-year"],
},
{
name: "quarters",
displayName: t`Quarters of the year...`,
test: filter => isQuarterofYearDateFilter(filter),
init: filter => getInitialQuarterOfYearFilter(filter),
- getOptions: EXCLUDE_OPTIONS["quarter-of-year"],
+ getOptionGroups: EXCLUDE_OPTIONS["quarter-of-year"],
},
{
name: "hours",
displayName: t`Hours of the day...`,
test: filter => isHourOfDayDateFilter(filter),
init: filter => getInitialHourOfDayFilter(filter),
- getOptions: EXCLUDE_OPTIONS["hour-of-day"],
+ getOptionGroups: EXCLUDE_OPTIONS["hour-of-day"],
},
];
@@ -142,12 +142,13 @@ export default function ExcludeDatePicker({
);
}
- const { getOptions } = temporalUnit;
- const options = getOptions();
+ const { getOptionGroups } = temporalUnit;
+ const optionGroups = getOptionGroups();
+ const options = optionGroups.flat();
const update = (values: string[]) =>
onFilterChange([operator, field, ...values]);
- const allSelected = values.length === 0;
- const selectAllLabel = allSelected ? t`Select none...` : t`Select all...`;
+ const allSelected = values.length === options.length;
+ const selectAllLabel = allSelected ? t`Select none` : t`Select all`;
return (
<div className={className}>
@@ -156,12 +157,12 @@ export default function ExcludeDatePicker({
checkedColor={primaryColor}
checked={allSelected}
onChange={() =>
- update(allSelected ? options.flat().map(({ value }) => value) : [])
+ update(allSelected ? [] : options.map(({ value }) => value))
}
/>
<Separator />
<ExcludeContainer>
- {options.map((inner, index) => (
+ {optionGroups.map((inner, index) => (
<ExcludeColumn key={index}>
{inner.map(({ displayName, value, test }) => {
const isValueExcluded = values.find(value => test(value)) != null;
@@ -169,7 +170,7 @@ export default function ExcludeDatePicker({
<ExcludeCheckBox
key={value}
label={<ExcludeLabel>{displayName}</ExcludeLabel>}
- checked={!isValueExcluded}
+ checked={isValueExcluded}
checkedColor={primaryColor}
onChange={() => {
if (!isValueExcluded) {
diff --git a/frontend/src/metabase/admin/datamodel/components/filters/pickers/DatePicker/ExcludeDatePicker.unit.spec.tsx b/frontend/src/metabase/admin/datamodel/components/filters/pickers/DatePicker/ExcludeDatePicker.unit.spec.tsx
index fa4f7784f95..a255e27d769 100644
--- a/frontend/src/metabase/admin/datamodel/components/filters/pickers/DatePicker/ExcludeDatePicker.unit.spec.tsx
+++ b/frontend/src/metabase/admin/datamodel/components/filters/pickers/DatePicker/ExcludeDatePicker.unit.spec.tsx
@@ -1,4 +1,5 @@
-import { fireEvent, render, screen } from "@testing-library/react";
+import { render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
import { createMockMetadata } from "__support__/metadata";
import { checkNotNull } from "metabase/lib/types";
@@ -14,48 +15,118 @@ import ExcludeDatePicker from "./ExcludeDatePicker";
const metadata = createMockMetadata({
databases: [createSampleDatabase()],
});
-
const ordersTable = checkNotNull(metadata.table(ORDERS_ID));
const query = ordersTable.legacyQuery({ useStructuredQuery: true });
-
const filter = new Filter(
[null, ["field", ORDERS.CREATED_AT, null]],
null,
query,
);
+const hours = Array.from({ length: 24 }, (_, i) => i);
+
+type SetupOpts = {
+ filter: Filter;
+};
+
+function setup({ filter }: SetupOpts) {
+ const onFilterChange = jest.fn();
+ const onCommit = jest.fn();
+
+ render(
+ <ExcludeDatePicker
+ filter={filter}
+ onFilterChange={onFilterChange}
+ onCommit={onCommit}
+ />,
+ );
+
+ return { onFilterChange, onCommit };
+}
describe("ExcludeDatePicker", () => {
- it("is empty option should exclude empty values by applying not-null filter", () => {
- const commitMock = jest.fn();
- render(
- <ExcludeDatePicker
- onFilterChange={jest.fn()}
- onCommit={commitMock}
- filter={filter}
- />,
+ beforeAll(() => {
+ jest.useFakeTimers({ advanceTimers: true });
+ jest.setSystemTime(new Date(2020, 0, 1));
+ });
+
+ it("should allow to exclude hours", async () => {
+ const filter = new Filter(
+ ["!=", ["field", ORDERS.CREATED_AT, { "temporal-unit": "hour-of-day" }]],
+ null,
+ query,
);
+ const { onFilterChange } = setup({ filter });
+ expect(screen.getByLabelText("Select all")).not.toBeChecked();
+ expect(screen.getByLabelText("8 AM")).not.toBeChecked();
+
+ await userEvent.click(screen.getByLabelText("8 AM"));
+ expect(onFilterChange).toHaveBeenCalledWith([
+ "!=",
+ ["field", ORDERS.CREATED_AT, { "temporal-unit": "hour-of-day" }],
+ 8,
+ ]);
+ });
- fireEvent.click(screen.getByText("Is empty"));
+ it("should allow to exclude all options", async () => {
+ const filter = new Filter(
+ [
+ "!=",
+ ["field", ORDERS.CREATED_AT, { "temporal-unit": "hour-of-day" }],
+ 8,
+ ],
+ null,
+ query,
+ );
+ const { onFilterChange } = setup({ filter });
+ expect(screen.getByLabelText("Select all")).not.toBeChecked();
+ expect(screen.getByLabelText("8 AM")).toBeChecked();
- expect(commitMock).toHaveBeenCalledWith([
+ await userEvent.click(screen.getByLabelText("Select all"));
+ expect(onFilterChange).toHaveBeenCalledWith([
+ "!=",
+ ["field", ORDERS.CREATED_AT, { "temporal-unit": "hour-of-day" }],
+ ...hours,
+ ]);
+ });
+
+ it("should allow to deselect all options", async () => {
+ const filter = new Filter(
+ [
+ "!=",
+ ["field", ORDERS.CREATED_AT, { "temporal-unit": "hour-of-day" }],
+ ...hours,
+ ],
+ null,
+ query,
+ );
+ const { onFilterChange } = setup({ filter });
+ expect(screen.getByLabelText("Select none")).toBeChecked();
+ expect(screen.getByLabelText("8 AM")).toBeChecked();
+
+ await userEvent.click(screen.getByLabelText("Select none"));
+ expect(onFilterChange).toHaveBeenCalledWith([
+ "!=",
+ ["field", ORDERS.CREATED_AT, { "temporal-unit": "hour-of-day" }],
+ ]);
+ });
+
+ it("is empty option should exclude empty values by applying not-null filter", async () => {
+ const { onCommit } = setup({ filter });
+
+ await userEvent.click(screen.getByText("Is empty"));
+
+ expect(onCommit).toHaveBeenCalledWith([
"not-null",
["field", ORDERS.CREATED_AT, null],
]);
});
- it("is not empty option should exclude non-empty values by applying is-null filter", () => {
- const commitMock = jest.fn();
- render(
- <ExcludeDatePicker
- onFilterChange={jest.fn()}
- onCommit={commitMock}
- filter={filter}
- />,
- );
+ it("is not empty option should exclude non-empty values by applying is-null filter", async () => {
+ const { onCommit } = setup({ filter });
- fireEvent.click(screen.getByText("Is not empty"));
+ await userEvent.click(screen.getByText("Is not empty"));
- expect(commitMock).toHaveBeenCalledWith([
+ expect(onCommit).toHaveBeenCalledWith([
"is-null",
["field", ORDERS.CREATED_AT, null],
]);
diff --git a/frontend/src/metabase/parameters/utils/date-formatting.ts b/frontend/src/metabase/parameters/utils/date-formatting.ts
index 11529baf3ca..69ca2f1f614 100644
--- a/frontend/src/metabase/parameters/utils/date-formatting.ts
+++ b/frontend/src/metabase/parameters/utils/date-formatting.ts
@@ -62,7 +62,7 @@ const serializersByOperatorName: Record<
return null;
}
const options = operator
- .getOptions()
+ .getOptionGroups()
.flat()
.filter(
({ test }) => _.find(values, (value: string) => test(value)) != null,
diff --git a/frontend/src/metabase/querying/filters/components/DatePicker/ExcludeDatePicker/ExcludeDatePicker.tsx b/frontend/src/metabase/querying/filters/components/DatePicker/ExcludeDatePicker/ExcludeDatePicker.tsx
index 65642dd607e..508fa89b0df 100644
--- a/frontend/src/metabase/querying/filters/components/DatePicker/ExcludeDatePicker/ExcludeDatePicker.tsx
+++ b/frontend/src/metabase/querying/filters/components/DatePicker/ExcludeDatePicker/ExcludeDatePicker.tsx
@@ -160,21 +160,17 @@ function ExcludeValuePicker({
onBack,
}: ExcludeValuePickerProps) {
const [values, setValues] = useState(initialValues);
- const isEmpty = values.length === 0;
-
- const option = useMemo(() => {
- return findExcludeUnitOption(unit);
- }, [unit]);
-
- const groups = useMemo(() => {
- return getExcludeValueOptionGroups(unit);
- }, [unit]);
+ const option = useMemo(() => findExcludeUnitOption(unit), [unit]);
+ const groups = useMemo(() => getExcludeValueOptionGroups(unit), [unit]);
+ const options = groups.flat();
+ const isAll = values.length === options.length;
+ const isNone = values.length === 0;
const handleToggleAll = (isChecked: boolean) => {
if (isChecked) {
- setValues([]);
- } else {
setValues(groups.flatMap(groups => groups.map(({ value }) => value)));
+ } else {
+ setValues([]);
}
};
@@ -183,9 +179,9 @@ function ExcludeValuePicker({
isChecked: boolean,
) => {
if (isChecked) {
- setValues(values.filter(value => value !== option.value));
- } else {
setValues([...values, option.value]);
+ } else {
+ setValues(values.filter(value => value !== option.value));
}
};
@@ -199,8 +195,8 @@ function ExcludeValuePicker({
<Divider />
<Stack p="md">
<Checkbox
- checked={isEmpty}
- label={isEmpty ? t`Select none…` : t`Select all…`}
+ checked={isAll}
+ label={isAll ? t`Select none` : t`Select all`}
onChange={event => handleToggleAll(event.target.checked)}
/>
<Divider />
@@ -211,7 +207,7 @@ function ExcludeValuePicker({
<Checkbox
key={optionIndex}
label={option.label}
- checked={!values.includes(option.value)}
+ checked={values.includes(option.value)}
onChange={event =>
handleToggleOption(option, event.target.checked)
}
@@ -223,7 +219,7 @@ function ExcludeValuePicker({
</Stack>
<Divider />
<Group p="sm" position="right">
- <Button variant="filled" disabled={isEmpty} onClick={handleSubmit}>
+ <Button variant="filled" disabled={isNone} onClick={handleSubmit}>
{isNew ? t`Add filter` : t`Update filter`}
</Button>
</Group>
diff --git a/frontend/src/metabase/querying/filters/components/DatePicker/ExcludeDatePicker/ExcludeDatePicker.unit.spec.tsx b/frontend/src/metabase/querying/filters/components/DatePicker/ExcludeDatePicker/ExcludeDatePicker.unit.spec.tsx
index 31351ae1aee..93420776c14 100644
--- a/frontend/src/metabase/querying/filters/components/DatePicker/ExcludeDatePicker/ExcludeDatePicker.unit.spec.tsx
+++ b/frontend/src/metabase/querying/filters/components/DatePicker/ExcludeDatePicker/ExcludeDatePicker.unit.spec.tsx
@@ -42,10 +42,13 @@ describe("ExcludeDatePicker", () => {
const { onChange } = setup({ isNew: true });
await userEvent.click(screen.getByText("Days of the week…"));
- await userEvent.click(screen.getByText("Monday"));
- await userEvent.click(screen.getByText("Sunday"));
- await userEvent.click(screen.getByText("Add filter"));
+ await userEvent.click(screen.getByLabelText("Monday"));
+ await userEvent.click(screen.getByLabelText("Sunday"));
+ expect(screen.getByLabelText("Monday")).toBeChecked();
+ expect(screen.getByLabelText("Sunday")).toBeChecked();
+ expect(screen.getByLabelText("Tuesday")).not.toBeChecked();
+ await userEvent.click(screen.getByRole("button", { name: "Add filter" }));
expect(onChange).toHaveBeenCalledWith({
type: "exclude",
operator: "!=",
@@ -54,13 +57,48 @@ describe("ExcludeDatePicker", () => {
});
});
+ it("should allow to exclude all options", async () => {
+ const { onChange } = setup({ isNew: true });
+
+ await userEvent.click(screen.getByText("Days of the week…"));
+ expect(screen.getByLabelText("Select all")).not.toBeChecked();
+ expect(screen.getByLabelText("Monday")).not.toBeChecked();
+ expect(screen.getByRole("button", { name: "Add filter" })).toBeDisabled();
+
+ await userEvent.click(screen.getByLabelText("Select all"));
+ expect(screen.getByLabelText("Select none")).toBeChecked();
+ expect(screen.getByLabelText("Monday")).toBeChecked();
+ expect(screen.getByRole("button", { name: "Add filter" })).toBeEnabled();
+
+ await userEvent.click(screen.getByRole("button", { name: "Add filter" }));
+ expect(onChange).toHaveBeenCalledWith({
+ type: "exclude",
+ operator: "!=",
+ unit: "day-of-week",
+ values: [1, 2, 3, 4, 5, 6, 7],
+ });
+ });
+
+ it("should allow to deselect all options", async () => {
+ const { onChange } = setup({ isNew: true });
+
+ await userEvent.click(screen.getByText("Days of the week…"));
+ await userEvent.click(screen.getByLabelText("Select all"));
+ await userEvent.click(screen.getByLabelText("Select none"));
+
+ expect(screen.getByLabelText("Select all")).not.toBeChecked();
+ expect(screen.getByLabelText("Monday")).not.toBeChecked();
+ expect(screen.getByRole("button", { name: "Add filter" })).toBeDisabled();
+ expect(onChange).not.toHaveBeenCalled();
+ });
+
it("should allow to exclude months", async () => {
const { onChange } = setup({ isNew: true });
await userEvent.click(screen.getByText("Months of the year…"));
- await userEvent.click(screen.getByText("January"));
- await userEvent.click(screen.getByText("December"));
- await userEvent.click(screen.getByText("Add filter"));
+ await userEvent.click(screen.getByLabelText("January"));
+ await userEvent.click(screen.getByLabelText("December"));
+ await userEvent.click(screen.getByRole("button", { name: "Add filter" }));
expect(onChange).toHaveBeenCalledWith({
type: "exclude",
@@ -74,9 +112,9 @@ describe("ExcludeDatePicker", () => {
const { onChange } = setup({ isNew: true });
await userEvent.click(screen.getByText("Quarters of the year…"));
- await userEvent.click(screen.getByText("1st"));
- await userEvent.click(screen.getByText("4th"));
- await userEvent.click(screen.getByText("Add filter"));
+ await userEvent.click(screen.getByLabelText("1st"));
+ await userEvent.click(screen.getByLabelText("4th"));
+ await userEvent.click(screen.getByRole("button", { name: "Add filter" }));
expect(onChange).toHaveBeenCalledWith({
type: "exclude",
@@ -90,10 +128,10 @@ describe("ExcludeDatePicker", () => {
const { onChange } = setup({ isNew: true });
await userEvent.click(screen.getByText("Hours of the day…"));
- await userEvent.click(screen.getByText("12 AM"));
- await userEvent.click(screen.getByText("2 AM"));
- await userEvent.click(screen.getByText("5 PM"));
- await userEvent.click(screen.getByText("Add filter"));
+ await userEvent.click(screen.getByLabelText("12 AM"));
+ await userEvent.click(screen.getByLabelText("2 AM"));
+ await userEvent.click(screen.getByLabelText("5 PM"));
+ await userEvent.click(screen.getByRole("button", { name: "Add filter" }));
expect(onChange).toHaveBeenCalledWith({
type: "exclude",
--
GitLab