Merge pull request #2825 from metabase/fix-field-name

Cleanup and fix FieldName

frontend/src/metabase/lib/query.js

@@ -41,6 +41,9 @@ export function createQuery(type = "query", databaseId, tableId) {
     return dataset_query;
 }
 
+const mbqlCanonicalize = (a) => a.toLowerCase().replace(/_/g, "-");
+const mbqlCompare = (a, b) => mbqlCanonicalize(a) === mbqlCanonicalize(b)
+
 var Query = {
 
     isStructured(dataset_query) {
@@ -438,23 +441,23 @@ var Query = {
     },
 
     isLocalField(field) {
-        return Array.isArray(field) && field[0] === "field-id";
+        return Array.isArray(field) && mbqlCompare(field[0], "field-id");
     },
 
     isForeignKeyField(field) {
-        return Array.isArray(field) && field[0] === "fk->";
+        return Array.isArray(field) && mbqlCompare(field[0], "fk->");
     },
 
     isDatetimeField(field) {
-        return Array.isArray(field) && field[0] === "datetime_field";
+        return Array.isArray(field) && mbqlCompare(field[0], "datetime-field");
     },
 
     isExpressionField(field) {
-        return Array.isArray(field) && field.length === 2 && field[0] === "expression";
+        return Array.isArray(field) && field.length === 2 && mbqlCompare(field[0], "expression");
     },
 
     isAggregateField(field) {
-        return Array.isArray(field) && field[0] === "aggregation";
+        return Array.isArray(field) && mbqlCompare(field[0], "aggregation");
     },
 
     isValidField(field) {
@@ -462,7 +465,10 @@ var Query = {
             (Query.isRegularField(field)) ||
             (Query.isLocalField(field)) ||
             (Query.isForeignKeyField(field) && Query.isRegularField(field[1]) && Query.isRegularField(field[2])) ||
-            (Query.isDatetimeField(field)   && Query.isValidField(field[1]) && field[2] === "as" && typeof field[3] === "string") ||
+            (Query.isDatetimeField(field)   && Query.isValidField(field[1]) &&
+                (field.length === 4 ?
+                    (field[2] === "as" && typeof field[3] === "string") : // deprecated
+                    typeof field[2] === "string")) ||
             (Query.isExpressionField(field) && _.isString(field[1])) ||
             (Query.isAggregateField(field)  && typeof field[1] === "number")
         );
@@ -491,17 +497,20 @@ var Query = {
     },
 
     // gets the table and field definitions from from a raw, fk->, or datetime_field field
-    getFieldTarget: function(field, tableDef) {
+    getFieldTarget: function(field, tableDef, path = []) {
         if (Query.isRegularField(field)) {
-            return { table: tableDef, field: tableDef.fields_lookup && tableDef.fields_lookup[field] };
+            return { table: tableDef, field: tableDef.fields_lookup && tableDef.fields_lookup[field], path };
         } else if (Query.isLocalField(field)) {
-            return Query.getFieldTarget(field[1], tableDef);
+            return Query.getFieldTarget(field[1], tableDef, path);
         } else if (Query.isForeignKeyField(field)) {
             let fkFieldDef = tableDef.fields_lookup && tableDef.fields_lookup[field[1]];
             let targetTableDef = fkFieldDef && fkFieldDef.target.table;
-            return Query.getFieldTarget(field[2], targetTableDef);
+            return Query.getFieldTarget(field[2], targetTableDef, path.concat(fkFieldDef));
         } else if (Query.isDatetimeField(field)) {
-            return Query.getFieldTarget(field[1], tableDef);
+            return {
+                ...Query.getFieldTarget(field[1], tableDef, path),
+                unit: Query.getDatetimeUnit(field)
+            };
         } else if (Query.isExpressionField(field)) {
             // hmmm, since this is a dynamic field we'll need to build this here
             let fieldDef = {
@@ -525,13 +534,22 @@ var Query = {
 
             return {
                 table: tableDef,
-                field: fieldDef
+                field: fieldDef,
+                path: path
             }
         }
 
         console.warn("Unknown field type: ", field);
     },
 
+    getDatetimeUnit(field) {
+        if (field.length === 4) {
+            return field[3]; // deprecated
+        } else {
+            return field[2];
+        }
+    },
+
     getFieldOptions(fields, includeJoins = false, filterFn = (fields) => fields, usedFields = {}) {
         var results = {
             count: 0,

frontend/src/metabase/query_builder/FieldName.jsx

@@ -3,10 +3,9 @@ import React, { Component, PropTypes } from "react";
 import Icon from "metabase/components/Icon.jsx";
 
 import Query from "metabase/lib/query";
-import { formatBucketing, parseFieldBucketing, parseFieldTarget } from "metabase/lib/query_time";
+import { formatBucketing } from "metabase/lib/query_time";
 import { stripId } from "metabase/lib/formatting";
 
-import _ from "underscore";
 import cx from "classnames";
 
 export default class FieldName extends Component {
@@ -24,65 +23,32 @@ export default class FieldName extends Component {
     };
 
     render() {
-        let targetTitle, fkTitle, fkIcon, bucketingTitle;
-        let { field, fieldOptions } = this.props;
-
-        let bucketing = parseFieldBucketing(field);
-        field = parseFieldTarget(field);
-
-        let fieldDef;
-        if (Array.isArray(field) && field[0] === 'fk->') {
-            let fkDef = _.find(fieldOptions.fks, (fk) => _.isEqual(fk.field.id, field[1]));
-            if (fkDef) {
-                fkTitle = (<span>{stripId(fkDef.field.display_name)}</span>);
-                fieldDef = _.find(fkDef.fields, (f) => _.isEqual(f.id, field[2]));
-                if (fieldDef) {
-                    fkIcon = (<span className="px1"><Icon name="connections" width="10" height="10" /></span>);
-                }
-            }
-        } else if (Query.isExpressionField(field)) {
-            fieldDef = {
-                display_name: field[1]
-            };
-        } else {
-            fieldDef = _.find(fieldOptions.fields, (f) => _.isEqual(f.id, field));
-        }
-
-        if (fieldDef) {
-            targetTitle = (<span>{fieldDef.display_name}</span>);
-        }
-
-        // if this is a datetime field then add an extra bit of labeling about the time bucket
-        if (fieldDef && bucketing) {
-            bucketingTitle = ": " + formatBucketing(bucketing);
-        }
-
-        var titleElement;
-        if (fkTitle || targetTitle) {
-            titleElement = <span className="QueryOption">{fkTitle}{fkIcon}{targetTitle}{bucketingTitle}</span>;
-        } else {
-            titleElement = <span className="QueryOption">field</span>;
+        let { field, tableMetadata, className } = this.props;
+        let fieldTarget = Query.getFieldTarget(field, tableMetadata);
+
+        let parts = [];
+        // fk path
+        for (let [index, fkField] of Object.entries(fieldTarget.path)) {
+            parts.push(<span key={"fkName"+index}>{stripId(fkField.display_name)}</span>);
+            parts.push(<span key={"fkIcon"+index} className="px1"><Icon name="connections" width="10" height="10" /></span>);
         }
-
-        var classes = cx({
-            'selected': Query.isValidField(field)
-        });
-
-        var removeButton;
-        if (this.props.removeField) {
-            removeButton = (
-                <a className="text-grey-2 no-decoration pr1 flex align-center" onClick={this.props.removeField}>
-                    <Icon name='close' width="14px" height="14px" />
-                </a>
-            )
+        // target field itself
+        parts.push(<span key="field">{fieldTarget.field.display_name}</span>);
+        // datetime-field unit
+        if (fieldTarget.unit != null) {
+            parts.push(<span key="unit">{": " + formatBucketing(fieldTarget.unit)}</span>);
         }
 
         return (
             <div className="flex align-center">
-                <div className={this.props.className + " " + classes} onClick={this.props.onClick}>
-                    {titleElement}
+                <div className={cx(className, { selected: Query.isValidField(field) })} onClick={this.props.onClick}>
+                    <span className="QueryOption">{parts}</span>
                 </div>
-                {removeButton}
+                { this.props.removeField &&
+                    <a className="text-grey-2 no-decoration pr1 flex align-center" onClick={this.props.removeField}>
+                        <Icon name='close' width="14px" height="14px" />
+                    </a>
+                }
             </div>
         );
     }

frontend/test/unit/lib/query.spec.js

@@ -241,6 +241,83 @@ describe('Query', () => {
             expect(query.order_by).toBe(undefined);
         });
     });
+
+    describe('getFieldTarget', () => {
+        let field2 = {
+            display_name: "field2",
+        }
+        let table2 = {
+            display_name: "table2",
+            fields_lookup: {
+                2: field2
+            }
+        }
+        let field1 = {
+            display_name: "field1",
+            target: {
+                table: table2
+            }
+        }
+        let table1 = {
+            display_name: "table1",
+            fields_lookup: {
+                1: field1
+            }
+        }
+
+        it('should return field object for old-style local field', () => {
+            let target = Query.getFieldTarget(1, table1);
+            expect(target.table).toEqual(table1);
+            expect(target.field).toEqual(field1);
+            expect(target.path).toEqual([]);
+            expect(target.unit).toEqual(undefined);
+        });
+        it('should return field object for new-style local field', () => {
+            let target = Query.getFieldTarget(["field-id", 1], table1);
+            expect(target.table).toEqual(table1);
+            expect(target.field).toEqual(field1);
+            expect(target.path).toEqual([]);
+            expect(target.unit).toEqual(undefined);
+        });
+        it('should return unit object for old-style datetime_field', () => {
+            let target = Query.getFieldTarget(["datetime-field", 1, "as", "day"], table1);
+            expect(target.table).toEqual(table1);
+            expect(target.field).toEqual(field1);
+            expect(target.path).toEqual([]);
+            expect(target.unit).toEqual("day");
+        });
+        it('should return unit object for new-style datetime_field', () => {
+            let target = Query.getFieldTarget(["datetime-field", 1, "as", "day"], table1);
+            expect(target.table).toEqual(table1);
+            expect(target.field).toEqual(field1);
+            expect(target.path).toEqual([]);
+            expect(target.unit).toEqual("day");
+        });
+
+        it('should return field object and table for fk field', () => {
+            let target = Query.getFieldTarget(["fk->", 1, 2], table1);
+            expect(target.table).toEqual(table2);
+            expect(target.field).toEqual(field2);
+            expect(target.path).toEqual([field1]);
+            expect(target.unit).toEqual(undefined);
+        });
+
+        it('should return field object and table and unit for fk + datetime field', () => {
+            let target = Query.getFieldTarget(["datetime-field", ["fk->", 1, 2], "day"], table1);
+            expect(target.table).toEqual(table2);
+            expect(target.field).toEqual(field2);
+            expect(target.path).toEqual([field1]);
+            expect(target.unit).toEqual("day");
+        });
+
+        it('should return field object and table for expression', () => {
+            let target = Query.getFieldTarget(["expression", "foo"], table1);
+            expect(target.table).toEqual(table1);
+            expect(target.field.display_name).toEqual("foo");
+            expect(target.path).toEqual([]);
+            expect(target.unit).toEqual(undefined);
+        });
+    })
 });