diff --git a/frontend/src/metabase/dashboard/selectors.js b/frontend/src/metabase/dashboard/selectors.js
index 680cd0bea51db64e0b328cc26289f0c3f16d3075..e74666a5f889ddcee7bf93a85c1626857caf8db8 100644
--- a/frontend/src/metabase/dashboard/selectors.js
+++ b/frontend/src/metabase/dashboard/selectors.js
@@ -161,7 +161,7 @@ export const getParameters = createSelector(
.value();
return {
...parameter,
- field_id: fieldIds.length === 1 ? fieldIds[0] : null
+ field_ids: fieldIds
}
})
)
diff --git a/frontend/src/metabase/parameters/components/ParameterValueWidget.jsx b/frontend/src/metabase/parameters/components/ParameterValueWidget.jsx
index 8ab0733679ce63b76cc13566999d748c895947b6..19a9d0efac7a601c15216e4220ec53dcfa9b7415 100644
--- a/frontend/src/metabase/parameters/components/ParameterValueWidget.jsx
+++ b/frontend/src/metabase/parameters/components/ParameterValueWidget.jsx
@@ -31,17 +31,21 @@ const DATE_WIDGETS = {
}
import { fetchFieldValues } from "metabase/redux/metadata";
-import { getParameterFieldValues } from "metabase/selectors/metadata";
-
-const mapStateToProps = (state, props) => ({
- values: getParameterFieldValues(state, props),
-})
+import { makeGetMergedParameterFieldValues } from "metabase/selectors/metadata";
+
+const makeMapStateToProps = () => {
+ const getMergedParameterFieldValues = makeGetMergedParameterFieldValues();
+ const mapStateToProps = (state, props) => ({
+ values: getMergedParameterFieldValues(state, props),
+ })
+ return mapStateToProps;
+}
const mapDispatchToProps = {
fetchFieldValues
}
-@connect(mapStateToProps, mapDispatchToProps)
+@connect(makeMapStateToProps, mapDispatchToProps)
export default class ParameterValueWidget extends Component {
static propTypes = {
diff --git a/frontend/src/metabase/selectors/metadata.js b/frontend/src/metabase/selectors/metadata.js
index 1b6bda217a42d1feccf73be2e8a207faacc9aec5..426d5d5e75ebf26b9661579b7b9bfd4e158765ca 100644
--- a/frontend/src/metabase/selectors/metadata.js
+++ b/frontend/src/metabase/selectors/metadata.js
@@ -1,6 +1,6 @@
/* @flow weak */
-import { createSelector } from "reselect";
+import { createSelector, createSelectorCreator, defaultMemoize } from "reselect";
import Metadata from "metabase-lib/lib/metadata/Metadata";
import Database from "metabase-lib/lib/metadata/Database";
@@ -9,7 +9,8 @@ import Field from "metabase-lib/lib/metadata/Field";
import Metric from "metabase-lib/lib/metadata/Metric";
import Segment from "metabase-lib/lib/metadata/Segment";
-import { getIn } from "icepick";
+import _ from "underscore";
+import { shallowEqual } from "recompose";
import { getFieldValues } from "metabase/lib/query/field";
import {
@@ -17,6 +18,7 @@ import {
getBreakouts,
getAggregatorsWithFields
} from "metabase/lib/schema_metadata";
+import { getIn } from "icepick";
export const getNormalizedMetadata = state => state.metadata;
@@ -123,22 +125,75 @@ export const getSegments = createSelector(
({ segments }) => segments
);
-// MISC
-
-export const getParameterFieldValues = (state, props) => {
- const fieldValues = getFieldValues(getIn(state, ["metadata", "fields", props.parameter.field_id]));
+// FIELD VALUES FOR DASHBOARD FILTERS / SQL QUESTION PARAMETERS
+
+// Returns a dictionary of field id:s mapped to matching field values
+// Currently this assumes that you are passing the props of <ParameterValueWidget> which contain the
+// `field_ids` array inside `parameter` prop.
+const getParameterFieldValuesByFieldId = (state, props) => {
+ // NOTE Atte Keinänen 9/14/17: Reading the state directly instead of using `getFields` selector
+ // because `getMetadata` doesn't currently work with fields of public dashboards
+ return _.chain(getIn(state, ["metadata", "fields"]))
+ // SQL template tags provide `field_id` instead of `field_ids`
+ .pick(...(props.parameter.field_ids || [props.parameter.field_id]))
+ .mapObject(getFieldValues)
+ .value()
+}
- // HACK Atte Keinänen 7/27/17: Currently the field value analysis code only returns a single value for booleans,
- // this will be addressed in analysis sync refactor
+// Custom equality selector for checking if two field value dictionaries contain same fields and field values
+// Currently we simply check if fields match and the lengths of field value arrays are equal which makes the comparison fast
+// See https://github.com/reactjs/reselect#customize-equalitycheck-for-defaultmemoize
+const createFieldValuesEqualSelector = createSelectorCreator(defaultMemoize, (a, b) => {
+// TODO: Why can't we use plain shallowEqual, i.e. why the field value arrays change very often?
+ return shallowEqual(_.mapObject(a, (values) => values.length), _.mapObject(b, (values) => values.length));
+})
+
+// HACK Atte Keinänen 7/27/17: Currently the field value analysis code only returns a single value for booleans,
+// this will be addressed in analysis sync refactor
+const patchBooleanFieldValues_HACK = (valueArray) => {
const isBooleanFieldValues =
- fieldValues && fieldValues.length === 1 && fieldValues[0] && typeof(fieldValues[0][0]) === "boolean"
+ valueArray && valueArray.length === 1 && valueArray[0] && typeof(valueArray[0][0]) === "boolean"
+
if (isBooleanFieldValues) {
return [[true], [false]];
} else {
- return fieldValues;
+ return valueArray;
}
}
+// Merges the field values of fields linked to a parameter and removes duplicates
+// We want that we have a distinct selector for each field id combination, and for that reason
+// we export a method that creates a new selector; see
+// https://github.com/reactjs/reselect#sharing-selectors-with-props-across-multiple-components
+// TODO Atte Keinänen 7/20/17: Should we have any thresholds if the count of field values is high or we have many (>2?) fields?
+export const makeGetMergedParameterFieldValues = () => {
+ return createFieldValuesEqualSelector(getParameterFieldValuesByFieldId, (fieldValues) => {
+ const fieldIds = Object.keys(fieldValues)
+
+ if (fieldIds.length === 0) {
+ // If we have no fields for the parameter, don't return any field values
+ return [];
+ } else if (fieldIds.length === 1) {
+ // We have just a single field so we can return the field values almost as-is,
+ // only address the boolean bug for now
+ const singleFieldValues = fieldValues[fieldIds[0]]
+ return patchBooleanFieldValues_HACK(singleFieldValues);
+ } else {
+ // We have multiple fields, so let's merge their values to a single array
+ const sortedMergedValues = _.chain(Object.values(fieldValues))
+ .flatten(true)
+ .sortBy(fieldValue => {
+ const valueIsRemapped = fieldValue.length === 2
+ return valueIsRemapped ? fieldValue[1] : fieldValue[0]
+ })
+ .value()
+
+ // run the uniqueness comparision always against a non-remapped value
+ return _.uniq(sortedMergedValues, false, (fieldValue) => fieldValue[0]);
+ }
+ });
+}
+
// UTILS:
// clone each object in the provided mapping of objects
diff --git a/frontend/test/dashboard/dashboard.integ.spec.js b/frontend/test/dashboard/dashboard.integ.spec.js
index 9b000c24a0d0a999e9c49dc02b6f685ab2badd08..b43f2fc00caa641908dd6a391fb49b738c696e1a 100644
--- a/frontend/test/dashboard/dashboard.integ.spec.js
+++ b/frontend/test/dashboard/dashboard.integ.spec.js
@@ -9,7 +9,7 @@ import {
import { DashboardApi, PublicApi } from "metabase/services";
import * as Urls from "metabase/lib/urls";
-import { getParameterFieldValues } from "metabase/selectors/metadata";
+import { makeGetMergedParameterFieldValues } from "metabase/selectors/metadata";
import { ADD_PARAM_VALUES } from "metabase/redux/metadata";
import { mount } from "enzyme";
import {
@@ -87,14 +87,14 @@ describe("Dashboard", () => {
await store.dispatch(fetchDashboard('6e59cc97-3b6a-4bb6-9e7a-5efeee27e40f'));
await store.waitForActions(ADD_PARAM_VALUES)
- const fieldValues = await getParameterFieldValues(store.getState(), { parameter: { field_id: 21 }});
+ const getMergedParameterFieldValues = makeGetMergedParameterFieldValues();
+ const fieldValues = await getMergedParameterFieldValues(store.getState(), { parameter: { field_ids: [ 21 ] }});
expect(fieldValues).toEqual([["Doohickey"], ["Gadget"], ["Gizmo"], ["Widget"]]);
})
})
})
// Converted from Selenium E2E test
-
describe("dashboard page", () => {
let dashboardId = null;
diff --git a/frontend/test/dashboard/selectors.unit.spec.js b/frontend/test/dashboard/selectors.unit.spec.js
index f82406887e8e5009aae93c81e36b910ed05e6401..4f1b3ba5dc80fcaf1d61553cd31088cce79e0f50 100644
--- a/frontend/test/dashboard/selectors.unit.spec.js
+++ b/frontend/test/dashboard/selectors.unit.spec.js
@@ -42,7 +42,7 @@ describe("dashboard/selectors", () => {
.value();
expect(getParameters(state)).toEqual([{
id: 1,
- field_id: null
+ field_ids: []
}]);
})
it("should not include field id with one mapping, no field id", () => {
@@ -56,7 +56,7 @@ describe("dashboard/selectors", () => {
.value();
expect(getParameters(state)).toEqual([{
id: 1,
- field_id: null
+ field_ids: []
}]);
})
it("should include field id with one mappings, with field id", () => {
@@ -70,7 +70,7 @@ describe("dashboard/selectors", () => {
.value();
expect(getParameters(state)).toEqual([{
id: 1,
- field_id: 1
+ field_ids: [1]
}]);
})
it("should include field id with two mappings, with same field id", () => {
@@ -89,7 +89,7 @@ describe("dashboard/selectors", () => {
.value();
expect(getParameters(state)).toEqual([{
id: 1,
- field_id: 1
+ field_ids: [1]
}]);
})
it("should include field id with two mappings, one with field id, one without", () => {
@@ -108,10 +108,10 @@ describe("dashboard/selectors", () => {
.value();
expect(getParameters(state)).toEqual([{
id: 1,
- field_id: 1
+ field_ids: [1]
}]);
})
- it("should not include field id with two mappings, with different field ids", () => {
+ it("should include all field ids with two mappings, with different field ids", () => {
const state = chain(STATE)
.assocIn(["dashboard", "dashboards", 0, "parameters", 0], { id: 1 })
.assocIn(["dashboard", "dashcards", 0, "parameter_mappings", 0], {
@@ -127,7 +127,7 @@ describe("dashboard/selectors", () => {
.value();
expect(getParameters(state)).toEqual([{
id: 1,
- field_id: null
+ field_ids: [1, 2]
}]);
})
})
diff --git a/frontend/test/selectors/metadata.integ.spec.js b/frontend/test/selectors/metadata.integ.spec.js
new file mode 100644
index 0000000000000000000000000000000000000000..d2b8c43480ed863615d41c2e8342030b0ae0bdfb
--- /dev/null
+++ b/frontend/test/selectors/metadata.integ.spec.js
@@ -0,0 +1,76 @@
+import { createTestStore, login } from "__support__/integrated_tests";
+import {
+ deleteFieldDimension, fetchTableMetadata,
+ updateFieldDimension,
+ updateFieldValues,
+} from "metabase/redux/metadata";
+import { makeGetMergedParameterFieldValues } from "metabase/selectors/metadata";
+
+const REVIEW_RATING_ID = 33;
+const PRODUCT_CATEGORY_ID = 21;
+
+// NOTE Atte Keinänen 9/14/17: A hybrid of an integration test and a method unit test
+// I wanted to use a real state tree and have a realistic field remapping scenario
+
+describe('makeGetMergedParameterFieldValues', () => {
+ beforeAll(async () => {
+ await login();
+
+ // add remapping
+ const store = await createTestStore()
+
+ await store.dispatch(updateFieldDimension(REVIEW_RATING_ID, {
+ type: "internal",
+ name: "Rating Description",
+ human_readable_field_id: null
+ }));
+ await store.dispatch(updateFieldValues(REVIEW_RATING_ID, [
+ [1, 'Awful'], [2, 'Unpleasant'], [3, 'Meh'], [4, 'Enjoyable'], [5, 'Perfecto']
+ ]));
+ })
+
+ afterAll(async () => {
+ const store = await createTestStore()
+
+ await store.dispatch(deleteFieldDimension(REVIEW_RATING_ID));
+ await store.dispatch(updateFieldValues(REVIEW_RATING_ID, [
+ [1, '1'], [2, '2'], [3, '3'], [4, '4'], [5, '5']
+ ]));
+ })
+
+ it("should return empty array if no field ids", async () => {
+ const store = await createTestStore()
+ await store.dispatch(fetchTableMetadata(3))
+
+ const getMergedParameterFieldValues = makeGetMergedParameterFieldValues()
+ expect(
+ getMergedParameterFieldValues(store.getState(), { parameter: { field_ids: [] } })
+ ).toEqual([])
+
+ })
+
+ it("should return the original field values if a single field id", async () => {
+ const store = await createTestStore()
+ await store.dispatch(fetchTableMetadata(3))
+
+ const getMergedParameterFieldValues = makeGetMergedParameterFieldValues()
+ expect(
+ getMergedParameterFieldValues(store.getState(), { parameter: { field_ids: [PRODUCT_CATEGORY_ID] } })
+ ).toEqual([ [ 'Doohickey' ], [ 'Gadget' ], [ 'Gizmo' ], [ 'Widget' ] ])
+ })
+
+ it("should merge and sort field values if multiple field ids", async () => {
+ const store = await createTestStore()
+ await store.dispatch(fetchTableMetadata(3))
+ await store.dispatch(fetchTableMetadata(4))
+
+ const getMergedParameterFieldValues = makeGetMergedParameterFieldValues()
+ expect(
+ getMergedParameterFieldValues(store.getState(), { parameter: { field_ids: [PRODUCT_CATEGORY_ID, REVIEW_RATING_ID] } })
+ ).toEqual([
+ [1, 'Awful'], ['Doohickey'], [4, 'Enjoyable'], ['Gadget'],
+ ['Gizmo'], [3, 'Meh'], [5, 'Perfecto'], [2, 'Unpleasant'], ['Widget']
+ ])
+ })
+})
+