From d5bd7cac1fad8c243324d0455f43c95462f42844 Mon Sep 17 00:00:00 2001
From: Uladzimir Havenchyk <125459446+uladzimirdev@users.noreply.github.com>
Date: Fri, 17 May 2024 12:37:14 +0300
Subject: [PATCH] Maintain user-level state in dashboards for filters (#39813)
---
.../dashboard-cards/click-behavior.cy.spec.js | 5 +
...ters-sql-required-simple-filter.cy.spec.js | 4 +-
.../dashboard-filters/parameters.cy.spec.js | 122 ++++++++++++++++--
...flter-cc-dropped-on-second-edit.cy.spec.js | 3 +-
...ully-deal-with-corrupted-filter.cy.spec.js | 9 +-
.../v1/parameters/utils/parameter-values.js | 7 +-
.../utils/parameter-values.unit.spec.js | 53 ++++++++
frontend/src/metabase-types/api/dashboard.ts | 4 +
.../src/metabase-types/api/mocks/dashboard.ts | 1 +
.../dashboard/actions/data-fetching-typed.ts | 8 +-
.../metabase/dashboard/actions/parameters.ts | 11 +-
.../components/DashboardTabs/test-utils.ts | 1 +
.../parameters/components/ParametersList.jsx | 3 +-
.../parameters/utils/parameter-values.ts | 16 ++-
.../utils/parameter-values.unit.spec.js | 24 ++++
15 files changed, 250 insertions(+), 21 deletions(-)
diff --git a/e2e/test/scenarios/dashboard-cards/click-behavior.cy.spec.js b/e2e/test/scenarios/dashboard-cards/click-behavior.cy.spec.js
index a5e3fb59e87..fd88ff4e7c1 100644
--- a/e2e/test/scenarios/dashboard-cards/click-behavior.cy.spec.js
+++ b/e2e/test/scenarios/dashboard-cards/click-behavior.cy.spec.js
@@ -27,6 +27,7 @@ import {
visitEmbeddedPage,
visitIframe,
entityPickerModal,
+ filterWidget,
} from "e2e/support/helpers";
import { b64hash_to_utf8 } from "metabase/lib/encoding";
import {
@@ -964,6 +965,10 @@ describe("scenarios > dashboard > dashboard cards > click behavior", () => {
});
});
+ cy.log("reset filter state");
+
+ filterWidget().icon("close").click();
+
testChangingBackToDefaultBehavior();
});
diff --git a/e2e/test/scenarios/dashboard-filters/dashboard-filters-sql-required-simple-filter.cy.spec.js b/e2e/test/scenarios/dashboard-filters/dashboard-filters-sql-required-simple-filter.cy.spec.js
index ed5231ab489..76e86d71bff 100644
--- a/e2e/test/scenarios/dashboard-filters/dashboard-filters-sql-required-simple-filter.cy.spec.js
+++ b/e2e/test/scenarios/dashboard-filters/dashboard-filters-sql-required-simple-filter.cy.spec.js
@@ -113,8 +113,8 @@ describe("scenarios > dashboard > filters > SQL > simple filter > required ", ()
saveDashboard();
- // The URL query params should include the parameter with an empty value
- cy.location("search").should("eq", "?text=");
+ // The URL query params should include the last used parameter value
+ cy.location("search").should("eq", "?text=Bar");
});
});
diff --git a/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js b/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js
index acd4d3b8414..4089163fa9b 100644
--- a/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js
+++ b/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js
@@ -29,9 +29,17 @@ import {
spyRequestFinished,
entityPickerModal,
} from "e2e/support/helpers";
-
-const { ORDERS_ID, ORDERS, PRODUCTS, PRODUCTS_ID, REVIEWS_ID } =
- SAMPLE_DATABASE;
+import { createMockParameter } from "metabase-types/api/mocks";
+
+const {
+ ORDERS_ID,
+ ORDERS,
+ PRODUCTS,
+ PRODUCTS_ID,
+ REVIEWS_ID,
+ PEOPLE,
+ PEOPLE_ID,
+} = SAMPLE_DATABASE;
describe("scenarios > dashboard > parameters", () => {
const cards = [
@@ -246,14 +254,15 @@ describe("scenarios > dashboard > parameters", () => {
cy.findByText("Remove").click();
cy.location("search").should("eq", `?${endsWith.slug}=zmo`);
- cy.button("Save").click();
+ saveDashboard();
+
+ cy.log(
+ "There should only be one filter remaining and its value is preserved",
+ );
- cy.log("There should only be one filter remaining and its value cleared");
filterWidget().contains(new RegExp(`${endsWith.name}`, "i"));
- cy.location("search").should("eq", `?${endsWith.slug}=`);
- // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
- cy.findByText("37.65");
+ cy.location("search").should("eq", `?${endsWith.slug}=zmo`);
});
it("should handle mismatch between filter types (metabase#9299, metabase#16181)", () => {
@@ -1126,6 +1135,103 @@ describe("scenarios > dashboard > parameters", () => {
});
});
});
+
+ describe("preserve last used value", () => {
+ beforeEach(() => {
+ const textFilter = createMockParameter({
+ name: "Text",
+ slug: "string",
+ id: "5aefc726",
+ type: "string/=",
+ sectionId: "string",
+ });
+
+ const peopleQuestionDetails = {
+ query: { "source-table": PEOPLE_ID, limit: 5 },
+ };
+
+ cy.createDashboardWithQuestions({
+ dashboardDetails: {
+ parameters: [textFilter],
+ },
+ questions: [peopleQuestionDetails],
+ }).then(({ dashboard, questions: cards }) => {
+ const [peopleCard] = cards;
+
+ updateDashboardCards({
+ dashboard_id: dashboard.id,
+ cards: [
+ {
+ card_id: peopleCard.id,
+ parameter_mappings: [
+ {
+ parameter_id: textFilter.id,
+ card_id: peopleCard.id,
+ target: ["dimension", ["field", PEOPLE.NAME, null]],
+ },
+ ],
+ },
+ ],
+ });
+
+ visitDashboard(dashboard.id);
+
+ cy.wrap(dashboard.id).as("dashboardId");
+ });
+ });
+
+ it("should retain the last used value for a dashboard filter", () => {
+ cy.intercept("GET", "/api/**/items?pinned_state*").as("getPinnedItems");
+
+ filterWidget().click();
+
+ popover().within(() => {
+ cy.findByRole("textbox").type("Antwan Fisher");
+ cy.button("Add filter").click();
+ });
+
+ getDashboardCard().findByText("7750 Michalik Lane").should("be.visible");
+
+ cy.visit("/collection/root");
+ cy.wait("@getPinnedItems");
+
+ cy.get("@dashboardId").then(dashboardId => visitDashboard(dashboardId));
+
+ filterWidget()
+ .findByRole("listitem")
+ .should("have.text", "Antwan Fisher");
+
+ cy.log("verify filter resetting works");
+
+ filterWidget().icon("close").click();
+ getDashboardCard().findByText("761 Fish Hill Road").should("be.visible");
+ });
+
+ it("should allow resetting last used value", () => {
+ filterWidget().click();
+
+ popover().within(() => {
+ cy.findByRole("textbox").type("Antwan Fisher");
+ cy.button("Add filter").click();
+ });
+
+ getDashboardCard().findByText("7750 Michalik Lane").should("be.visible");
+
+ cy.log("reset filter values from url by visiting dashboard by id");
+
+ cy.get("@dashboardId").then(dashboardId => visitDashboard(dashboardId));
+
+ filterWidget().icon("close").click();
+
+ getDashboardCard().findByText("761 Fish Hill Road").should("be.visible");
+
+ cy.log("verify filter value is not specified after reload");
+
+ cy.get("@dashboardId").then(dashboardId => visitDashboard(dashboardId));
+
+ getDashboardCard().findByText("761 Fish Hill Road").should("be.visible");
+ });
+ });
});
function isFilterSelected(filter, bool) {
diff --git a/e2e/test/scenarios/dashboard-filters/reproductions/22788-flter-cc-dropped-on-second-edit.cy.spec.js b/e2e/test/scenarios/dashboard-filters/reproductions/22788-flter-cc-dropped-on-second-edit.cy.spec.js
index d382eb9afb2..e385cdf27b1 100644
--- a/e2e/test/scenarios/dashboard-filters/reproductions/22788-flter-cc-dropped-on-second-edit.cy.spec.js
+++ b/e2e/test/scenarios/dashboard-filters/reproductions/22788-flter-cc-dropped-on-second-edit.cy.spec.js
@@ -90,7 +90,8 @@ describe("issue 22788", () => {
saveDashboard();
- addFilterAndAssert();
+ cy.findAllByText("Gizmo");
+ cy.findAllByText("Doohickey").should("not.exist");
});
});
diff --git a/e2e/test/scenarios/dashboard-filters/reproductions/24500-gracefully-deal-with-corrupted-filter.cy.spec.js b/e2e/test/scenarios/dashboard-filters/reproductions/24500-gracefully-deal-with-corrupted-filter.cy.spec.js
index 5681b002adc..61a307e2e59 100644
--- a/e2e/test/scenarios/dashboard-filters/reproductions/24500-gracefully-deal-with-corrupted-filter.cy.spec.js
+++ b/e2e/test/scenarios/dashboard-filters/reproductions/24500-gracefully-deal-with-corrupted-filter.cy.spec.js
@@ -119,11 +119,16 @@ describe("issues 15279 and 24500", () => {
saveDashboard();
// Check the list filter again
- filterWidget().contains("List").click();
+ filterWidget().contains("List").parent().click();
cy.wait("@values");
- // Check that the search filter works
+ cy.log("Check that the search filter works");
+
+ // reset filter value
+ filterWidget().contains("Search").parent().icon("close").click();
+
filterWidget().contains("Search").click();
+
cy.findByPlaceholderText("Search by Name").type("Lora Cronin");
cy.button("Add filter").click();
diff --git a/frontend/src/metabase-lib/v1/parameters/utils/parameter-values.js b/frontend/src/metabase-lib/v1/parameters/utils/parameter-values.js
index 221e57100b5..78c6140e8f2 100644
--- a/frontend/src/metabase-lib/v1/parameters/utils/parameter-values.js
+++ b/frontend/src/metabase-lib/v1/parameters/utils/parameter-values.js
@@ -18,10 +18,14 @@ export function getParameterValue({
parameter,
values = {},
defaultRequired = false,
+ lastUsedParameterValue = null,
}) {
const value = values?.[parameter.id];
const useDefault = defaultRequired && parameter.required;
- return value ?? (useDefault ? parameter.default : null);
+
+ return (
+ lastUsedParameterValue ?? value ?? (useDefault ? parameter.default : null)
+ );
}
/**
@@ -133,6 +137,7 @@ export function normalizeParameterValue(type, value) {
export function getParameterValuesBySlug(parameters, parameterValuesById) {
parameters = parameters ?? [];
parameterValuesById = parameterValuesById ?? {};
+
return Object.fromEntries(
parameters.map(parameter => [
parameter.slug,
diff --git a/frontend/src/metabase-lib/v1/parameters/utils/parameter-values.unit.spec.js b/frontend/src/metabase-lib/v1/parameters/utils/parameter-values.unit.spec.js
index bf95dcd2b38..37f0f9f6de5 100644
--- a/frontend/src/metabase-lib/v1/parameters/utils/parameter-values.unit.spec.js
+++ b/frontend/src/metabase-lib/v1/parameters/utils/parameter-values.unit.spec.js
@@ -2,7 +2,9 @@ import {
getValuePopulatedParameters,
getParameterValuesBySlug,
normalizeParameterValue,
+ getParameterValue,
} from "metabase-lib/v1/parameters/utils/parameter-values";
+import { createMockParameter } from "metabase-types/api/mocks";
describe("parameters/utils/parameter-values", () => {
let field1;
@@ -196,6 +198,57 @@ describe("parameters/utils/parameter-values", () => {
});
});
+ describe("getParameterValue", () => {
+ const parameter = createMockParameter({
+ id: 333,
+ slug: "baz",
+ default: "parameter default value",
+ fields: [field1],
+ required: true,
+ });
+
+ it("should return last_used_param_value if there is any", () => {
+ expect(
+ getParameterValue({
+ parameter,
+ values: { [parameter.id]: "parameter value" },
+ defaultRequired: true,
+ lastUsedParameterValue: "last used value",
+ }),
+ ).toBe("last used value");
+ });
+
+ it("should return value when there is no last_used_param_value", () => {
+ expect(
+ getParameterValue({
+ parameter,
+ values: { [parameter.id]: "parameter value" },
+ defaultRequired: true,
+ }),
+ ).toBe("parameter value");
+ });
+
+ it("should return default value if parameter is required and default is required", () => {
+ expect(
+ getParameterValue({
+ parameter,
+ values: {},
+ defaultRequired: true,
+ }),
+ ).toBe("parameter default value");
+ });
+
+ it("should return null if no default value required", () => {
+ expect(
+ getParameterValue({
+ parameter,
+ values: {},
+ defaultRequired: false,
+ }),
+ ).toBe(null);
+ });
+ });
+
describe("normalizeParameterValue", () => {
it("should return same value for location/category parameters", () => {
expect(normalizeParameterValue("category", "foo")).toEqual("foo");
diff --git a/frontend/src/metabase-types/api/dashboard.ts b/frontend/src/metabase-types/api/dashboard.ts
index 60f31b85c11..a334d962ad8 100644
--- a/frontend/src/metabase-types/api/dashboard.ts
+++ b/frontend/src/metabase-types/api/dashboard.ts
@@ -52,6 +52,10 @@ export interface Dashboard {
last_name: string;
timestamp: string;
};
+ last_used_param_values: Record<
+ ParameterId,
+ string | number | boolean | null | string[] | number[]
+ >;
auto_apply_filters: boolean;
archived: boolean;
public_uuid: string | null;
diff --git a/frontend/src/metabase-types/api/mocks/dashboard.ts b/frontend/src/metabase-types/api/mocks/dashboard.ts
index 5e95ad14e73..4e2e0c2a841 100644
--- a/frontend/src/metabase-types/api/mocks/dashboard.ts
+++ b/frontend/src/metabase-types/api/mocks/dashboard.ts
@@ -26,6 +26,7 @@ export const createMockDashboard = (opts?: Partial<Dashboard>): Dashboard => ({
last_name: "Doe",
timestamp: "2018-01-01",
},
+ last_used_param_values: {},
auto_apply_filters: true,
archived: false,
public_uuid: null,
diff --git a/frontend/src/metabase/dashboard/actions/data-fetching-typed.ts b/frontend/src/metabase/dashboard/actions/data-fetching-typed.ts
index 17f3a47f83b..f052e723d1c 100644
--- a/frontend/src/metabase/dashboard/actions/data-fetching-typed.ts
+++ b/frontend/src/metabase/dashboard/actions/data-fetching-typed.ts
@@ -169,9 +169,15 @@ export const fetchDashboard = createAsyncThunk(
questions,
);
+ const lastUsedParametersValues = result["last_used_param_values"] ?? {};
+
const parameterValuesById = preserveParameters
? getParameterValues(getState())
- : getParameterValuesByIdFromQueryParams(parameters, queryParams);
+ : getParameterValuesByIdFromQueryParams(
+ parameters,
+ queryParams,
+ lastUsedParametersValues,
+ );
entities = entities ?? normalize(result, dashboard).entities;
diff --git a/frontend/src/metabase/dashboard/actions/parameters.ts b/frontend/src/metabase/dashboard/actions/parameters.ts
index bdc1a89c7a5..c16e77f30a5 100644
--- a/frontend/src/metabase/dashboard/actions/parameters.ts
+++ b/frontend/src/metabase/dashboard/actions/parameters.ts
@@ -147,6 +147,7 @@ export const removeParameter = createThunkAction(
updateParameters(dispatch, getState, parameters =>
parameters.filter(p => p.id !== parameterId),
);
+
return { id: parameterId };
},
);
@@ -267,6 +268,7 @@ export const setParameterName = createThunkAction(
updateParameter(dispatch, getState, parameterId, parameter =>
setParamName(parameter, name),
);
+
return { id: parameterId, name };
},
);
@@ -372,6 +374,7 @@ export const setParameterFilteringParameters = createThunkAction(
...parameter,
filteringParameters,
}));
+
return { id: parameterId, filteringParameters };
},
);
@@ -379,12 +382,13 @@ export const setParameterFilteringParameters = createThunkAction(
export const SET_PARAMETER_VALUE = "metabase/dashboard/SET_PARAMETER_VALUE";
export const setParameterValue = createThunkAction(
SET_PARAMETER_VALUE,
- (parameterId: ParameterId, value: any) => (_dispatch, getState) => {
+ (parameterId: ParameterId, value: unknown) => (_dispatch, getState) => {
const isSettingDraftParameterValues = !getIsAutoApplyFilters(getState());
+ const isValueEmpty = isParameterValueEmpty(value);
return {
id: parameterId,
- value: isParameterValueEmpty(value) ? PULSE_PARAM_EMPTY : value,
+ value: isValueEmpty ? PULSE_PARAM_EMPTY : value,
isDraft: isSettingDraftParameterValues,
};
},
@@ -466,6 +470,7 @@ export const setParameterIsMultiSelect = createThunkAction(
...parameter,
isMultiSelect: isMultiSelect,
}));
+
return { id: parameterId, isMultiSelect };
},
);
@@ -480,6 +485,7 @@ export const setParameterQueryType = createThunkAction(
...parameter,
values_query_type: queryType,
}));
+
return { id: parameterId, queryType };
},
);
@@ -494,6 +500,7 @@ export const setParameterSourceType = createThunkAction(
...parameter,
values_source_type: sourceType,
}));
+
return { id: parameterId, sourceType };
},
);
diff --git a/frontend/src/metabase/dashboard/components/DashboardTabs/test-utils.ts b/frontend/src/metabase/dashboard/components/DashboardTabs/test-utils.ts
index d4e5ba7f4b8..e1bb757e665 100644
--- a/frontend/src/metabase/dashboard/components/DashboardTabs/test-utils.ts
+++ b/frontend/src/metabase/dashboard/components/DashboardTabs/test-utils.ts
@@ -40,6 +40,7 @@ export const TEST_DASHBOARD_STATE: DashboardState = {
last_name: "",
timestamp: "",
},
+ last_used_param_values: {},
dashcards: [1, 2],
tabs: [
getDefaultTab({ tabId: 1, dashId: 1, name: "Tab 1" }),
diff --git a/frontend/src/metabase/parameters/components/ParametersList.jsx b/frontend/src/metabase/parameters/components/ParametersList.jsx
index dbb1b9b4ce3..59b6204b742 100644
--- a/frontend/src/metabase/parameters/components/ParametersList.jsx
+++ b/frontend/src/metabase/parameters/components/ParametersList.jsx
@@ -66,7 +66,8 @@ function ParametersList({
setEditingParameter={setEditingParameter}
setValue={
setParameterValue &&
- (value => setParameterValue(valuePopulatedParameter.id, value))
+ (value =>
+ setParameterValue(valuePopulatedParameter.id, value, dashboard?.id))
}
setParameterValueToDefault={setParameterValueToDefault}
enableParameterRequiredBehavior={enableParameterRequiredBehavior}
diff --git a/frontend/src/metabase/parameters/utils/parameter-values.ts b/frontend/src/metabase/parameters/utils/parameter-values.ts
index 72292618696..1ad8a7ea0a2 100644
--- a/frontend/src/metabase/parameters/utils/parameter-values.ts
+++ b/frontend/src/metabase/parameters/utils/parameter-values.ts
@@ -3,6 +3,7 @@ import type { FieldFilterUiParameter } from "metabase-lib/v1/parameters/types";
import { getParameterType } from "metabase-lib/v1/parameters/utils/parameter-type";
import type {
Parameter,
+ ParameterId,
ParameterValue,
ParameterValueOrArray,
} from "metabase-types/api";
@@ -10,8 +11,10 @@ import type {
export function getParameterValueFromQueryParams(
parameter: Parameter,
queryParams: Record<string, string | string[] | null | undefined>,
+ lastUsedParametersValues?: Record<ParameterId, unknown>,
) {
queryParams = queryParams || {};
+ lastUsedParametersValues = lastUsedParametersValues || {};
const maybeParameterValue = queryParams[parameter.slug || parameter.id];
@@ -19,9 +22,11 @@ export function getParameterValueFromQueryParams(
if (maybeParameterValue === "") {
return null;
} else if (maybeParameterValue == null) {
- // try to use the default if the parameter is not present in the query params
- return parameter.default ?? null;
+ // first try to use last used parameter value then try to use the default if
+ // the parameter is not present in the query params
+ return lastUsedParametersValues[parameter.id] ?? parameter.default ?? null;
}
+
const parsedValue = parseParameterValue(maybeParameterValue, parameter);
return normalizeParameterValueForWidget(parsedValue, parameter);
}
@@ -100,11 +105,16 @@ function normalizeParameterValueForWidget(
export function getParameterValuesByIdFromQueryParams(
parameters: Parameter[],
queryParams: Record<string, string | string[] | null | undefined>,
+ lastUsedParametersValues?: Record<ParameterId, unknown>,
) {
return Object.fromEntries(
parameters.map(parameter => [
parameter.id,
- getParameterValueFromQueryParams(parameter, queryParams),
+ getParameterValueFromQueryParams(
+ parameter,
+ queryParams,
+ lastUsedParametersValues,
+ ),
]),
);
}
diff --git a/frontend/src/metabase/parameters/utils/parameter-values.unit.spec.js b/frontend/src/metabase/parameters/utils/parameter-values.unit.spec.js
index 784d8018cd0..3ec2a18c552 100644
--- a/frontend/src/metabase/parameters/utils/parameter-values.unit.spec.js
+++ b/frontend/src/metabase/parameters/utils/parameter-values.unit.spec.js
@@ -266,6 +266,30 @@ describe("parameters/utils/parameter-values", () => {
);
});
+ describe("last used param value", () => {
+ it("should use query parameter over last used param value", () => {
+ expect(
+ getParameterValueFromQueryParams(
+ parameter2,
+ {
+ [parameter2.slug]: "parameter 2 value",
+ },
+ { [parameter2.id]: "last used value" },
+ ),
+ ).toEqual(["parameter 2 value"]);
+ });
+
+ it("should use last used param value when query parameter is empty", () => {
+ expect(
+ getParameterValueFromQueryParams(
+ parameter2,
+ {},
+ { [parameter2.id]: "last used value" },
+ ),
+ ).toEqual("last used value");
+ });
+ });
+
describe("for number filter type", () => {
const numberParameter = {
id: 111,
--
GitLab