Dimension: normalize fk->, use normalized .mbql() in isEqual, fix render for...

Dimension: normalize fk->, use normalized .mbql() in isEqual, fix render for DateTime/Binned, add tests

frontend/src/metabase-lib/lib/Dimension.js

@@ -177,23 +177,8 @@ export default class Dimension {
     if (!otherDimension) {
       return false;
     }
-    // must be instace of the same class
-    if (this.constructor !== otherDimension.constructor) {
-      return false;
-    }
-    // must both or neither have a parent
-    if (!this._parent !== !otherDimension._parent) {
-      return false;
-    }
-    // parents must be equal
-    if (this._parent && !this._parent.isEqual(otherDimension._parent)) {
-      return false;
-    }
-    // args must be equal
-    if (!_.isEqual(this._args, otherDimension._args)) {
-      return false;
-    }
-    return true;
+    // assumes .mbql() returns canonical form
+    return _.isEqual(this.mbql(), other.mbql());
   }
 
   /**
@@ -283,7 +268,7 @@ export default class Dimension {
    * Renders a dimension to React
    */
   render(): ?React$Element<any> {
-    return [this.displayName()];
+    return this._parent ? this._parent.render() : [this.displayName()];
   }
 }
 
@@ -365,7 +350,7 @@ export class FKDimension extends FieldDimension {
       // $FlowFixMe
       const fkRef: ForeignFieldReference = mbql;
       const parent = Dimension.parseMBQL(fkRef[1], metadata);
-      return new FKDimension(parent, fkRef.slice(2));
+      return new FKDimension(parent, fkRef.slice(2), metadata);
     }
     return null;
   }
@@ -375,23 +360,24 @@ export class FKDimension extends FieldDimension {
       const field = parent.field();
       if (field.target && field.target.table) {
         return field.target.table.fields.map(
-          field => new FKDimension(parent, [field.id]),
+          field => new FKDimension(parent, [field.id], parent._metadata),
         );
       }
     }
     return [];
   }
 
+  constructor(parent: ?Dimension, args: any[], metadata?: Metadata): Dimension {
+    super(parent, args, metadata);
+    this._dest = Dimension.parseMBQL(args[0], metadata);
+  }
+
   mbql(): ForeignFieldReference {
-    // TODO: not sure `this._parent._args[0]` is the best way to handle this?
-    // we don't want the `["field-id", ...]` wrapper from the `this._parent.mbql()`
-    return ["fk->", this._parent._args[0], this._args[0]];
+    return ["fk->", this._parent.mbql(), this._dest.mbql()];
   }
 
   field() {
-    return (
-      (this._metadata && this._metadata.fields[this._args[0]]) || new Field()
-    );
+    return this._dest.field();
   }
 
   render() {

frontend/test/__support__/sample_dataset_fixture.js

@@ -23,6 +23,7 @@ export const ORDERS_TOTAL_FIELD_ID = 6;
 export const MAIN_METRIC_ID = 1;
 
 export const PRODUCT_CATEGORY_FIELD_ID = 21;
+export const PRODUCT_CREATED_AT_FIELD_ID = 22;
 export const PRODUCT_PK_FIELD_ID = 24;
 export const PRODUCT_TILE_FIELD_ID = 27;
 

frontend/test/metabase-lib/Dimension.unit.spec.js

+import ReactTestUtils from "react-dom/test-utils";
+
+import Dimension from "metabase-lib/lib/Dimension";
+import {
+  metadata,
+  ORDERS_CREATED_DATE_FIELD_ID,
+  ORDERS_PRODUCT_FK_FIELD_ID,
+  PRODUCT_CATEGORY_FIELD_ID,
+  PRODUCT_CREATED_AT_FIELD_ID,
+} from "__support__/sample_dataset_fixture";
+
+describe("Dimension", () => {
+  describe("parseMBQL", () => {
+    it("should parse (deprecated) bare field ID", () => {
+      const dimension = Dimension.parseMBQL(
+        ORDERS_PRODUCT_FK_FIELD_ID,
+        metadata,
+      );
+      expect(dimension.mbql()).toEqual([
+        "field-id",
+        ORDERS_PRODUCT_FK_FIELD_ID,
+      ]);
+      expect(dimension.render()).toEqual(["Product"]);
+    });
+    it("should parse field-id", () => {
+      const dimension = Dimension.parseMBQL(
+        ["field-id", ORDERS_PRODUCT_FK_FIELD_ID],
+        metadata,
+      );
+      expect(dimension.mbql()).toEqual([
+        "field-id",
+        ORDERS_PRODUCT_FK_FIELD_ID,
+      ]);
+      expect(dimension.render()).toEqual(["Product"]);
+    });
+    it("should parse fk-> with bare field IDs", () => {
+      const dimension = Dimension.parseMBQL(
+        ["fk->", ORDERS_PRODUCT_FK_FIELD_ID, PRODUCT_CATEGORY_FIELD_ID],
+        metadata,
+      );
+      expect(dimension.mbql()).toEqual([
+        "fk->",
+        ["field-id", ORDERS_PRODUCT_FK_FIELD_ID],
+        ["field-id", PRODUCT_CATEGORY_FIELD_ID],
+      ]);
+    });
+    it("should parse fk-> with field-id", () => {
+      const dimension = Dimension.parseMBQL(
+        [
+          "fk->",
+          ["field-id", ORDERS_PRODUCT_FK_FIELD_ID],
+          ["field-id", PRODUCT_CATEGORY_FIELD_ID],
+        ],
+        metadata,
+      );
+      expect(dimension.mbql()).toEqual([
+        "fk->",
+        ["field-id", ORDERS_PRODUCT_FK_FIELD_ID],
+        ["field-id", PRODUCT_CATEGORY_FIELD_ID],
+      ]);
+      const rendered = dimension.render();
+      expect(ReactTestUtils.isElement(rendered[1])).toBe(true); // Icon
+      rendered[1] = null;
+      expect(rendered).toEqual(["Product", null, "Category"]);
+    });
+
+    it("should parse datetime-field", () => {
+      const dimension = Dimension.parseMBQL(
+        ["datetime-field", ["field-id", PRODUCT_CREATED_AT_FIELD_ID], "hour"],
+        metadata,
+      );
+      expect(dimension.mbql()).toEqual([
+        "datetime-field",
+        ["field-id", PRODUCT_CREATED_AT_FIELD_ID],
+        "hour",
+      ]);
+      expect(dimension.render()).toEqual(["Created At", ": ", "Hour"]);
+    });
+
+    it("should parse datetime-field with fk->", () => {
+      const dimension = Dimension.parseMBQL(
+        [
+          "datetime-field",
+          ["fk->", ORDERS_PRODUCT_FK_FIELD_ID, PRODUCT_CREATED_AT_FIELD_ID],
+          "hour",
+        ],
+        metadata,
+      );
+      expect(dimension.mbql()).toEqual([
+        "datetime-field",
+        [
+          "fk->",
+          ["field-id", ORDERS_PRODUCT_FK_FIELD_ID],
+          ["field-id", PRODUCT_CREATED_AT_FIELD_ID],
+        ],
+
+        "hour",
+      ]);
+      const rendered = dimension.render();
+      expect(ReactTestUtils.isElement(rendered[1])).toBe(true); // Icon
+      rendered[1] = null;
+      expect(rendered).toEqual(["Product", null, "Created At", ": ", "Hour"]);
+    });
+  });
+});