From 5acedcb85c51a1bf26c8f4102f3ede3f881bd7d5 Mon Sep 17 00:00:00 2001
From: Dalton <daltojohnso@users.noreply.github.com>
Date: Wed, 1 Jun 2022 10:08:13 -0700
Subject: [PATCH] Simplify FieldValuesWidget fetching/option-handling logic
(#22990)
* Move caching logic out of FieldValuesWidget
* Try to clean up/centralize field value fetching
* Move dup logic to fetchValues
* Alwyas rely on option in component state
* Simplify hasList
* Make unit tests async
* Fix test import
* Fix outdated prop reference
* fix remappings
* Fix dashboard action unit tests
---
.../metabase/components/FieldValuesWidget.jsx | 256 +++++++-----------
frontend/src/metabase/dashboard/actions.js | 20 +-
.../metabase/dashboard/actions.unit.spec.js | 26 +-
frontend/src/metabase/dashboard/reducers.js | 4 +-
.../components/FieldValuesWidget.unit.spec.js | 41 +--
5 files changed, 156 insertions(+), 191 deletions(-)
diff --git a/frontend/src/metabase/components/FieldValuesWidget.jsx b/frontend/src/metabase/components/FieldValuesWidget.jsx
index 0a899345fbf..68960850ac9 100644
--- a/frontend/src/metabase/components/FieldValuesWidget.jsx
+++ b/frontend/src/metabase/components/FieldValuesWidget.jsx
@@ -20,7 +20,6 @@ import { addRemappings, fetchFieldValues } from "metabase/redux/metadata";
import { defer } from "metabase/lib/promise";
import { stripId } from "metabase/lib/formatting";
import { fetchDashboardParameterValues } from "metabase/dashboard/actions";
-import { getDashboardParameterValuesCache } from "metabase/dashboard/selectors";
import Fields from "metabase/entities/fields";
@@ -42,9 +41,7 @@ const mapDispatchToProps = {
};
function mapStateToProps(state, { fields = [] }) {
- // try and use the selected fields, but fall back to the ones passed
return {
- dashboardParameterValuesCache: getDashboardParameterValuesCache(state),
fields: fields.map(
field =>
Fields.selectors.getObject(state, { entityId: field.id }) || field,
@@ -52,6 +49,30 @@ function mapStateToProps(state, { fields = [] }) {
};
}
+async function searchFieldValues(
+ { fields, value, disablePKRemappingForSearch, maxResults },
+ cancelled,
+) {
+ let options = dedupeValues(
+ await Promise.all(
+ fields.map(field =>
+ MetabaseApi.field_search(
+ {
+ value,
+ fieldId: field.id,
+ searchFieldId: searchField(field, disablePKRemappingForSearch).id,
+ limit: maxResults,
+ },
+ { cancelled },
+ ),
+ ),
+ ),
+ );
+
+ options = options.map(result => [].concat(result));
+ return options;
+}
+
class FieldValuesWidgetInner extends Component {
constructor(props) {
super(props);
@@ -74,39 +95,85 @@ class FieldValuesWidgetInner extends Component {
componentDidMount() {
if (shouldList(this.props.fields, this.props.disableSearch)) {
- if (usesChainFilterEndpoints(this.props.dashboard)) {
- this.fetchDashboardParamValues();
- } else {
- const { fields, fetchFieldValues } = this.props;
- fields.forEach(field => fetchFieldValues(field.id));
- }
+ this.fetchValues();
}
}
- fetchDashboardParamValues = async () => {
+ async fetchValues(query) {
this.setState({
loadingState: "LOADING",
options: [],
});
- let options;
+ let options = [];
try {
- const { dashboard, parameter, parameters } = this.props;
- const args = {
- dashboardId: dashboard?.id,
- parameter,
- parameters,
- };
- await this.props.fetchDashboardParameterValues(args);
- options = this.props.dashboardParameterValuesCache.get(args);
+ if (usesChainFilterEndpoints(this.props.dashboard)) {
+ options = await this.fetchDashboardParamValues(query);
+ } else {
+ options = await this.fetchFieldValues(query);
+ }
} finally {
+ this.updateRemappings(options);
this.setState({
loadingState: "LOADED",
options,
});
}
+ }
+
+ fetchFieldValues = async query => {
+ if (query == null) {
+ const { fields, fetchFieldValues } = this.props;
+ await Promise.all(fields.map(field => fetchFieldValues(field.id)));
+ return dedupeValues(this.props.fields.map(field => field.values));
+ } else {
+ const { fields } = this.props;
+ const cancelDeferred = defer();
+ const cancelled = cancelDeferred.promise;
+ this._cancel = () => {
+ this._cancel = null;
+ cancelDeferred.resolve();
+ };
+
+ const options = await searchFieldValues(
+ {
+ value: query,
+ fields,
+ disablePKRemappingForSearch: this.props.disablePKRemappingForSearch,
+ maxResults: this.props.maxResults,
+ },
+ cancelled,
+ );
+
+ this._cancel = null;
+ return options;
+ }
};
+ fetchDashboardParamValues = async query => {
+ const { dashboard, parameter, parameters } = this.props;
+ const args = {
+ dashboardId: dashboard?.id,
+ parameter,
+ parameters,
+ query,
+ };
+ return this.props.fetchDashboardParameterValues(args);
+ };
+
+ updateRemappings(options) {
+ const { fields } = this.props;
+ if (showRemapping(fields)) {
+ const [field] = fields;
+ if (
+ field.remappedField() ===
+ searchField(field, this.props.disablePKRemappingForSearch)
+ ) {
+ this.props.addRemappings(field.id, options);
+ }
+ }
+ }
+
componentWillUnmount() {
if (this._cancel) {
this._cancel();
@@ -126,59 +193,17 @@ class FieldValuesWidgetInner extends Component {
return value;
};
- search = async (value, cancelled) => {
+ search = _.debounce(async value => {
if (!value) {
return;
}
- const { fields } = this.props;
-
- let results;
- if (usesChainFilterEndpoints(this.props.dashboard)) {
- const { dashboard, parameter, parameters } = this.props;
- const args = {
- dashboardId: dashboard?.id,
- parameter,
- parameters,
- query: value,
- };
- await this.props.fetchDashboardParameterValues(args);
- results = this.props.dashboardParameterValuesCache.get(args);
- } else {
- results = dedupeValues(
- await Promise.all(
- fields.map(field =>
- MetabaseApi.field_search(
- {
- value,
- fieldId: field.id,
- searchFieldId: searchField(
- field,
- this.props.disablePKRemappingForSearch,
- ).id,
- limit: this.props.maxResults,
- },
- { cancelled },
- ),
- ),
- ),
- );
-
- results = results.map(result => [].concat(result));
- }
+ await this.fetchValues(value);
- if (showRemapping(fields)) {
- const [field] = fields;
- if (
- field.remappedField() ===
- searchField(field, this.props.disablePKRemappingForSearch)
- ) {
- this.props.addRemappings(field.id, results);
- }
- }
-
- return results;
- };
+ this.setState({
+ lastValue: value,
+ });
+ }, 500);
_search = value => {
const { lastValue, options } = this.state;
@@ -194,51 +219,15 @@ class FieldValuesWidgetInner extends Component {
return;
}
- this.setState({
- loadingState: "LOADING",
- });
-
if (this._cancel) {
this._cancel();
}
- this._searchDebounced(value);
- };
-
- _searchDebounced = _.debounce(async value => {
this.setState({
loadingState: "LOADING",
});
-
- const cancelDeferred = defer();
- this._cancel = () => {
- this._cancel = null;
- cancelDeferred.resolve();
- };
-
- let results;
- try {
- results = await this.search(value, cancelDeferred.promise);
- } catch (e) {
- console.warn(e);
- }
-
- this._cancel = null;
-
- if (results) {
- this.setState({
- loadingState: "LOADED",
- options: results,
- lastValue: value,
- });
- } else {
- this.setState({
- loadingState: "INIT",
- options: [],
- lastValue: value,
- });
- }
- }, 500);
+ this.search(value);
+ };
render() {
const {
@@ -253,54 +242,28 @@ class FieldValuesWidgetInner extends Component {
parameter,
prefix,
disableSearch,
- dashboard,
disablePKRemappingForSearch,
formatOptions,
placeholder,
} = this.props;
- const { loadingState, options: stateOptions } = this.state;
+ const { loadingState, options = [] } = this.state;
const tokenFieldPlaceholder = getTokenFieldPlaceholder({
fields,
disableSearch,
- dashboard,
placeholder,
disablePKRemappingForSearch,
loadingState,
- options: stateOptions,
+ options,
});
- let options = [];
- if (
- hasList({
- fields,
- disableSearch,
- dashboard,
- loadingState,
- options: stateOptions,
- }) &&
- !usesChainFilterEndpoints(this.props.dashboard)
- ) {
- options = dedupeValues(fields.map(field => field.values));
- } else if (
- loadingState === "LOADED" &&
- (isSearchable(fields, disableSearch, disablePKRemappingForSearch) ||
- usesChainFilterEndpoints(this.props.dashboard))
- ) {
- options = this.state.options;
- } else {
- options = [];
- }
-
const isLoading = loadingState === "LOADING";
const isFetchingList =
shouldList(this.props.fields, this.props.disableSearch) && isLoading;
const hasListData = hasList({
fields,
disableSearch,
- dashboard,
- loadingState,
- options: stateOptions,
+ options,
});
return (
@@ -349,9 +312,11 @@ class FieldValuesWidgetInner extends Component {
compact: false,
})
}
- optionRenderer={option =>
- renderValue(fields, formatOptions, option[0], { autoLoad: false })
- }
+ optionRenderer={option => {
+ return renderValue(fields, formatOptions, option[0], {
+ autoLoad: false,
+ });
+ }}
layoutRenderer={layoutProps => (
<div>
{layoutProps.valuesList}
@@ -504,14 +469,8 @@ function getSearchableTokenFieldPlaceholder(
return placeholder;
}
-function hasList({ fields, disableSearch, dashboard, loadingState, options }) {
- const nonEmptyArray = a => a && a.length > 0;
- return (
- shouldList(fields, disableSearch) &&
- (usesChainFilterEndpoints(dashboard)
- ? loadingState === "LOADED" && nonEmptyArray(options)
- : fields.every(field => nonEmptyArray(field.values)))
- );
+function hasList({ fields, disableSearch, options }) {
+ return shouldList(fields, disableSearch) && !_.isEmpty(options);
}
function isSearchable(fields, disableSearch, disablePKRemappingForSearch) {
@@ -532,7 +491,6 @@ function isSearchable(fields, disableSearch, disablePKRemappingForSearch) {
function getTokenFieldPlaceholder({
fields,
disableSearch,
- dashboard,
placeholder,
disablePKRemappingForSearch,
loadingState,
@@ -548,9 +506,6 @@ function getTokenFieldPlaceholder({
hasList({
fields,
disableSearch,
- disablePKRemappingForSearch,
- dashboard,
- loadingState,
options,
})
) {
@@ -575,7 +530,6 @@ function renderOptions(
alwaysShowOptions,
fields,
disableSearch,
- dashboard,
disablePKRemappingForSearch,
} = props;
const { loadingState, options } = state;
@@ -587,8 +541,6 @@ function renderOptions(
hasList({
fields,
disableSearch,
- dashboard,
- loadingState,
options,
})
) {
diff --git a/frontend/src/metabase/dashboard/actions.js b/frontend/src/metabase/dashboard/actions.js
index c2ab2b00892..cd138a7b383 100644
--- a/frontend/src/metabase/dashboard/actions.js
+++ b/frontend/src/metabase/dashboard/actions.js
@@ -59,6 +59,7 @@ import {
getParameterValues,
getDashboardParameterValuesSearchCache,
getLoadingDashCards,
+ getDashboardParameterValuesCache,
} from "./selectors";
import { getMetadata } from "metabase/selectors/metadata";
import { getCardAfterVisualizationClick } from "metabase/visualizations/lib/utils";
@@ -136,8 +137,8 @@ export const SHOW_ADD_PARAMETER_POPOVER =
export const HIDE_ADD_PARAMETER_POPOVER =
"metabase/dashboard/HIDE_ADD_PARAMETER_POPOVER";
-export const FETCH_DASHBOARD_PARAMETER_FIELD_VALUES =
- "metabase/dashboard/FETCH_DASHBOARD_PARAMETER_FIELD_VALUES";
+export const FETCH_DASHBOARD_PARAMETER_FIELD_VALUES_WITH_CACHE =
+ "metabase/dashboard/FETCH_DASHBOARD_PARAMETER_FIELD_VALUES_WITH_CACHE";
export const SET_SIDEBAR = "metabase/dashboard/SET_SIDEBAR";
export const CLOSE_SIDEBAR = "metabase/dashboard/CLOSE_SIDEBAR";
@@ -1098,8 +1099,8 @@ const loadMetadataForDashboard = dashCards => (dispatch, getState) => {
);
};
-export const fetchDashboardParameterValues = createThunkAction(
- FETCH_DASHBOARD_PARAMETER_FIELD_VALUES,
+export const fetchDashboardParameterValuesWithCache = createThunkAction(
+ FETCH_DASHBOARD_PARAMETER_FIELD_VALUES_WITH_CACHE,
({ dashboardId, parameter, parameters, query }) => async (
dispatch,
getState,
@@ -1138,3 +1139,14 @@ export const fetchDashboardParameterValues = createThunkAction(
};
},
);
+
+export const fetchDashboardParameterValues = args => async (
+ dispatch,
+ getState,
+) => {
+ await dispatch(fetchDashboardParameterValuesWithCache(args));
+ const dashboardParameterValuesCache = getDashboardParameterValuesCache(
+ getState(),
+ );
+ return dashboardParameterValuesCache.get(args) || [];
+};
diff --git a/frontend/src/metabase/dashboard/actions.unit.spec.js b/frontend/src/metabase/dashboard/actions.unit.spec.js
index b5a2a3991b3..262a6046710 100644
--- a/frontend/src/metabase/dashboard/actions.unit.spec.js
+++ b/frontend/src/metabase/dashboard/actions.unit.spec.js
@@ -11,8 +11,8 @@ import {
openAddQuestionSidebar,
removeParameter,
SET_DASHBOARD_ATTRIBUTES,
- fetchDashboardParameterValues,
- FETCH_DASHBOARD_PARAMETER_FIELD_VALUES,
+ fetchDashboardParameterValuesWithCache,
+ FETCH_DASHBOARD_PARAMETER_FIELD_VALUES_WITH_CACHE,
} from "./actions";
import { SIDEBAR_NAME } from "./constants";
@@ -154,7 +154,7 @@ describe("dashboard actions", () => {
});
});
- describe("fetchDashboardParameterValues", () => {
+ describe("fetchDashboardParameterValuesWithCache", () => {
const dashboardId = 1;
const parameter = { id: "a" };
const parameterWithFilteringParameters = {
@@ -183,7 +183,7 @@ describe("dashboard actions", () => {
});
it("should fetch parameter values using the given query string", async () => {
- const action = await fetchDashboardParameterValues({
+ const action = await fetchDashboardParameterValuesWithCache({
dashboardId,
parameter,
parameters,
@@ -201,12 +201,12 @@ describe("dashboard actions", () => {
"dashboardId: 1, parameterId: a, query: foo, filteringParameterValues: []",
results: [[1], [2], [3]],
},
- type: FETCH_DASHBOARD_PARAMETER_FIELD_VALUES,
+ type: FETCH_DASHBOARD_PARAMETER_FIELD_VALUES_WITH_CACHE,
});
});
it("should fetch parameter values without a query string", async () => {
- const action = await fetchDashboardParameterValues({
+ const action = await fetchDashboardParameterValuesWithCache({
dashboardId,
parameter,
parameters,
@@ -223,12 +223,12 @@ describe("dashboard actions", () => {
"dashboardId: 1, parameterId: a, query: null, filteringParameterValues: []",
results: [[4], [5], [6]],
},
- type: FETCH_DASHBOARD_PARAMETER_FIELD_VALUES,
+ type: FETCH_DASHBOARD_PARAMETER_FIELD_VALUES_WITH_CACHE,
});
});
it("should fetch parameter values using a query string and filtering parameters", async () => {
- const action = await fetchDashboardParameterValues({
+ const action = await fetchDashboardParameterValuesWithCache({
dashboardId,
parameter: parameterWithFilteringParameters,
parameters,
@@ -247,12 +247,12 @@ describe("dashboard actions", () => {
'dashboardId: 1, parameterId: a, query: bar, filteringParameterValues: [["b","bbb"]]',
results: [[1], [2], [3]],
},
- type: FETCH_DASHBOARD_PARAMETER_FIELD_VALUES,
+ type: FETCH_DASHBOARD_PARAMETER_FIELD_VALUES_WITH_CACHE,
});
});
it("should fetch parameter values without a query string but with filtering parameters", async () => {
- const action = await fetchDashboardParameterValues({
+ const action = await fetchDashboardParameterValuesWithCache({
dashboardId,
parameter: parameterWithFilteringParameters,
parameters,
@@ -270,7 +270,7 @@ describe("dashboard actions", () => {
'dashboardId: 1, parameterId: a, query: null, filteringParameterValues: [["b","bbb"]]',
results: [[4], [5], [6]],
},
- type: FETCH_DASHBOARD_PARAMETER_FIELD_VALUES,
+ type: FETCH_DASHBOARD_PARAMETER_FIELD_VALUES_WITH_CACHE,
});
});
@@ -281,7 +281,7 @@ describe("dashboard actions", () => {
[cacheKey]: [[1], [2], [3]],
};
- const action = await fetchDashboardParameterValues({
+ const action = await fetchDashboardParameterValuesWithCache({
dashboardId,
parameter: parameterWithFilteringParameters,
parameters,
@@ -293,7 +293,7 @@ describe("dashboard actions", () => {
expect(action).toEqual({
payload: undefined,
- type: FETCH_DASHBOARD_PARAMETER_FIELD_VALUES,
+ type: FETCH_DASHBOARD_PARAMETER_FIELD_VALUES_WITH_CACHE,
});
});
});
diff --git a/frontend/src/metabase/dashboard/reducers.js b/frontend/src/metabase/dashboard/reducers.js
index be8cca1b11b..3ee94cca59a 100644
--- a/frontend/src/metabase/dashboard/reducers.js
+++ b/frontend/src/metabase/dashboard/reducers.js
@@ -30,7 +30,7 @@ import {
HIDE_ADD_PARAMETER_POPOVER,
SET_SIDEBAR,
CLOSE_SIDEBAR,
- FETCH_DASHBOARD_PARAMETER_FIELD_VALUES,
+ FETCH_DASHBOARD_PARAMETER_FIELD_VALUES_WITH_CACHE,
SAVE_DASHBOARD_AND_CARDS,
SET_DOCUMENT_TITLE,
SET_SHOW_LOADING_COMPLETE_FAVICON,
@@ -284,7 +284,7 @@ const parameterValuesSearchCache = handleActions(
[SAVE_DASHBOARD_AND_CARDS]: {
next: () => ({}),
},
- [FETCH_DASHBOARD_PARAMETER_FIELD_VALUES]: {
+ [FETCH_DASHBOARD_PARAMETER_FIELD_VALUES_WITH_CACHE]: {
next: (state, { payload }) =>
payload ? assoc(state, payload.cacheKey, payload.results) : state,
},
diff --git a/frontend/test/metabase/components/FieldValuesWidget.unit.spec.js b/frontend/test/metabase/components/FieldValuesWidget.unit.spec.js
index 449c30812c9..041c451457e 100644
--- a/frontend/test/metabase/components/FieldValuesWidget.unit.spec.js
+++ b/frontend/test/metabase/components/FieldValuesWidget.unit.spec.js
@@ -14,6 +14,7 @@ const renderFieldValuesWidget = props =>
value={[]}
onChange={() => {}}
fetchFieldValues={() => {}}
+ addRemappings={() => {}}
{...props}
/>,
);
@@ -31,9 +32,9 @@ describe("FieldValuesWidget", () => {
expect(fetchFieldValues).not.toHaveBeenCalled();
});
- it("should have 'Enter some text' as the placeholder text", () => {
+ it("should have 'Enter some text' as the placeholder text", async () => {
renderFieldValuesWidget({ ...props });
- screen.getByPlaceholderText("Enter some text");
+ await screen.findByPlaceholderText("Enter some text");
});
});
describe("has_field_values = list", () => {
@@ -55,11 +56,11 @@ describe("FieldValuesWidget", () => {
).toBeNull();
});
- it("should have 'Enter some text' as the placeholder text for fields with more than 10 values", () => {
+ it("should have 'Enter some text' as the placeholder text for fields with more than 10 values", async () => {
renderFieldValuesWidget({
fields: [mock(PRODUCTS.TITLE, { has_field_values: "list" })],
});
- screen.getByPlaceholderText("Enter some text");
+ await screen.findByPlaceholderText("Enter some text");
});
});
@@ -74,25 +75,25 @@ describe("FieldValuesWidget", () => {
expect(fetchFieldValues).not.toHaveBeenCalled();
});
- it("should have 'Search by Category' as the placeholder text", () => {
+ it("should have 'Search by Category' as the placeholder text", async () => {
renderFieldValuesWidget({ ...props });
- screen.getByPlaceholderText("Search by Category");
+ await screen.findByPlaceholderText("Search by Category");
});
});
});
describe("id field", () => {
describe("has_field_values = none", () => {
- it("should have 'Enter an ID' as the placeholder text", () => {
+ it("should have 'Enter an ID' as the placeholder text", async () => {
renderFieldValuesWidget({
fields: [mock(ORDERS.PRODUCT_ID, { has_field_values: "none" })],
});
- screen.getByPlaceholderText("Enter an ID");
+ await screen.findByPlaceholderText("Enter an ID");
});
});
describe("has_field_values = list", () => {
- it("should have 'Search the list' as the placeholder text", () => {
+ it("should have 'Search the list' as the placeholder text", async () => {
renderFieldValuesWidget({
fields: [
mock(ORDERS.PRODUCT_ID, {
@@ -101,12 +102,12 @@ describe("FieldValuesWidget", () => {
}),
],
});
- screen.getByPlaceholderText("Search the list");
+ await screen.findByPlaceholderText("Search the list");
});
});
describe("has_field_values = search", () => {
- it("should have 'Search by Category or enter an ID' as the placeholder text", () => {
+ it("should have 'Search by Category or enter an ID' as the placeholder text", async () => {
renderFieldValuesWidget({
fields: [
mock(ORDERS.PRODUCT_ID, {
@@ -115,7 +116,7 @@ describe("FieldValuesWidget", () => {
}),
],
});
- screen.getByPlaceholderText("Search by Category or enter an ID");
+ await screen.findByPlaceholderText("Search by Category or enter an ID");
});
it("should not duplicate 'ID' in placeholder when ID itself is searchable", async () => {
@@ -126,7 +127,7 @@ describe("FieldValuesWidget", () => {
}),
];
renderFieldValuesWidget({ fields });
- screen.getByPlaceholderText("Search by Product");
+ await screen.findByPlaceholderText("Search by Product");
});
});
});
@@ -138,31 +139,31 @@ describe("FieldValuesWidget", () => {
mock(PEOPLE.STATE, { has_field_values: "list" }),
];
renderFieldValuesWidget({ fields });
- screen.getByPlaceholderText("Search the list");
+ await screen.findByPlaceholderText("Search the list");
- screen.getByText("AZ");
- screen.getByText("Facebook");
+ await screen.findByText("AZ");
+ await screen.findByText("Facebook");
});
- it("search if any field is a search", () => {
+ it("search if any field is a search", async () => {
const fields = [
mock(PEOPLE.SOURCE, { has_field_values: "search" }),
mock(PEOPLE.STATE, { has_field_values: "list" }),
];
renderFieldValuesWidget({ fields });
- screen.getByPlaceholderText("Search");
+ await screen.findByPlaceholderText("Search");
expect(screen.queryByText("AZ")).toBeNull();
expect(screen.queryByText("Facebook")).toBeNull();
});
- it("don't list any values if any is set to 'plain input box'", () => {
+ it("don't list any values if any is set to 'plain input box'", async () => {
const fields = [
mock(PEOPLE.SOURCE, { has_field_values: "none" }),
mock(PEOPLE.STATE, { has_field_values: "list" }),
];
renderFieldValuesWidget({ fields });
- screen.getByPlaceholderText("Enter some text");
+ await screen.findByPlaceholderText("Enter some text");
expect(screen.queryByText("AZ")).toBeNull();
expect(screen.queryByText("Facebook")).toBeNull();
--
GitLab