Merge pull request #5543 from metabase/fix-4909

Show field values in category filter if it is linked to multiple fields [WIP]

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
             }
         })
 )

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 = {

frontend/src/metabase/selectors/metadata.js

 /* @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

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;
 

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]
             }]);
         })
     })

frontend/test/selectors/metadata.integ.spec.js

+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']
+        ])
+    })
+})
+