From fb4bfa2896b3471b531940da4389e5aac09b1a45 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Wed, 18 Oct 2023 10:40:25 -0700
Subject: [PATCH] Drill thru fixes: #34439, #34440, #34441 (#34760)
---
frontend/src/metabase-lib/drills.unit.spec.ts | 255 +++++++++---------
src/metabase/lib/drill_thru.cljc | 6 +-
src/metabase/lib/drill_thru/fk_details.cljc | 39 +--
src/metabase/lib/drill_thru/foreign_key.cljc | 6 +-
.../lib/drill_thru/fk_details_test.cljc | 74 ++++-
.../lib/drill_thru/foreign_key_test.cljc | 27 +-
test/metabase/lib/drill_thru/sort_test.cljc | 28 ++
test/metabase/lib/drill_thru/test_util.cljc | 10 +-
.../drill_thru/underlying_records_test.cljc | 27 +-
.../drill_thru/zoom_in_timeseries_test.cljc | 33 +--
test/metabase/lib/drill_thru_test.cljc | 46 ++--
11 files changed, 342 insertions(+), 209 deletions(-)
diff --git a/frontend/src/metabase-lib/drills.unit.spec.ts b/frontend/src/metabase-lib/drills.unit.spec.ts
index 7fff19c797b..092df539f7a 100644
--- a/frontend/src/metabase-lib/drills.unit.spec.ts
+++ b/frontend/src/metabase-lib/drills.unit.spec.ts
@@ -19,6 +19,8 @@ import {
createProductsVendorDatasetColumn,
ORDERS,
ORDERS_ID,
+ PEOPLE,
+ PEOPLE_ID,
PRODUCTS,
PRODUCTS_ID,
SAMPLE_DB_ID,
@@ -449,49 +451,48 @@ describe("availableDrillThrus", () => {
},
],
},
- // FIXME: fk-filter gets returned for non-fk column (metabase#34440), fk-details gets returned for non-fk colum (metabase#34441), underlying-records drill gets shown two times for aggregated query (metabase#34439)
- // {
- // clickType: "cell",
- // queryType: "aggregated",
- // columnName: "count",
- // expectedDrills: [
- // {
- // type: "drill-thru/quick-filter",
- // operators: ["<", ">", "=", "≠"],
- // },
- // {
- // type: "drill-thru/underlying-records",
- // rowCount: 2, // FIXME: (metabase#32108) this should return real count of rows
- // tableName: "Orders",
- // },
- // {
- // displayName: "See this month by week",
- // type: "drill-thru/zoom-in.timeseries",
- // },
- // ],
- // },
- // FIXME: fk-filter gets returned for non-fk column (metabase#34440), fk-details gets returned for non-fk colum (metabase#34441), underlying-records drill gets shown two times for aggregated query (metabase#34439)
- // {
- // clickType: "cell",
- // queryType: "aggregated",
- // columnName: "max",
- // expectedDrills: [
- // {
- // type: "drill-thru/quick-filter",
- // operators: ["=", "≠"],
- // },
- // {
- // type: "drill-thru/underlying-records",
- // rowCount: 2, // FIXME: (metabase#32108) this should return real count of rows
- // tableName: "Orders",
- // },
- //
- // {
- // type: "drill-thru/zoom-in.timeseries",
- // displayName: "See this month by week",
- // },
- // ],
- // },
+ // fk-filter gets returned for non-fk column (metabase#34440), fk-details gets returned for non-fk colum (metabase#34441), underlying-records drill gets shown two times for aggregated query (metabase#34439)
+ {
+ clickType: "cell",
+ queryType: "aggregated",
+ columnName: "count",
+ expectedDrills: [
+ {
+ type: "drill-thru/quick-filter",
+ operators: ["<", ">", "=", "≠"],
+ },
+ {
+ type: "drill-thru/underlying-records",
+ rowCount: 77, // FIXME: (metabase#32108) this should return real count of rows
+ tableName: "Orders",
+ },
+ {
+ displayName: "See this month by week",
+ type: "drill-thru/zoom-in.timeseries",
+ },
+ ],
+ },
+ // fk-filter gets returned for non-fk column (metabase#34440), fk-details gets returned for non-fk colum (metabase#34441), underlying-records drill gets shown two times for aggregated query (metabase#34439)
+ {
+ clickType: "cell",
+ queryType: "aggregated",
+ columnName: "max",
+ expectedDrills: [
+ {
+ type: "drill-thru/quick-filter",
+ operators: ["=", "≠"],
+ },
+ {
+ type: "drill-thru/underlying-records",
+ rowCount: 2, // FIXME: (metabase#32108) this should return real count of rows
+ tableName: "Orders",
+ },
+ {
+ type: "drill-thru/zoom-in.timeseries",
+ displayName: "See this month by week",
+ },
+ ],
+ },
// FIXME: quick-filter gets returned for non-metric column (metabase#34443)
// {
// clickType: "cell",
@@ -981,16 +982,16 @@ describe("availableDrillThrus", () => {
type: "drill-thru/fk-filter",
},
},
- // FIXME: `fk-filter` doesn't get returned for fk column that was used as breakout (metabase#34440)
- // {
- // drillType: "drill-thru/fk-filter",
- // clickType: "cell",
- // queryType: "aggregated",
- // columnName: "PRODUCT_ID",
- // expectedParameters: {
- // type: "drill-thru/fk-filter",
- // },
- // },
+ // `fk-filter` doesn't get returned for fk column that was used as breakout (metabase#34440)
+ {
+ drillType: "drill-thru/fk-filter",
+ clickType: "cell",
+ queryType: "aggregated",
+ columnName: "PRODUCT_ID",
+ expectedParameters: {
+ type: "drill-thru/fk-filter",
+ },
+ },
// endregion
// region --- drill-thru/quick-filter
@@ -1085,7 +1086,7 @@ describe("availableDrillThrus", () => {
columnName: "count",
expectedParameters: {
type: "drill-thru/underlying-records",
- rowCount: 3, // FIXME: (metabase#32108) this should return real count of rows
+ rowCount: 77, // FIXME: (metabase#32108) this should return real count of rows
tableName: "Orders",
},
},
@@ -1096,7 +1097,7 @@ describe("availableDrillThrus", () => {
columnName: "sum",
expectedParameters: {
type: "drill-thru/underlying-records",
- rowCount: 3, // FIXME: (metabase#32108) this should return real count of rows
+ rowCount: 1, // FIXME: (metabase#32108) this should return real count of rows
tableName: "Orders",
},
},
@@ -1107,7 +1108,7 @@ describe("availableDrillThrus", () => {
columnName: "max",
expectedParameters: {
type: "drill-thru/underlying-records",
- rowCount: 3, // FIXME: (metabase#32108) this should return real count of rows
+ rowCount: 2, // FIXME: (metabase#32108) this should return real count of rows
tableName: "Orders",
},
},
@@ -1144,34 +1145,37 @@ describe("availableDrillThrus", () => {
// endregion
// region --- drill-thru/zoom-in.timeseries
- // FIXME: "zoom-in.timeseries" should be returned for aggregated query metric click (metabase#33811)
- // {
- // drillType: "drill-thru/zoom-in.timeseries",
- // clickType: "header",
- // queryType: "aggregated",
- // columnName: "count",
- // expectedParameters: {
- // type: "drill-thru/zoom-in.timeseries",
- // },
- // },
- // {
- // drillType: "drill-thru/zoom-in.timeseries",
- // clickType: "header",
- // queryType: "aggregated",
- // columnName: "max",
- // expectedParameters: {
- // type: "drill-thru/zoom-in.timeseries",
- // },
- // },
- // {
- // drillType: "drill-thru/zoom-in.timeseries",
- // clickType: "header",
- // queryType: "aggregated",
- // columnName: "sum",
- // expectedParameters: {
- // type: "drill-thru/zoom-in.timeseries",
- // },
- // },
+ // "zoom-in.timeseries" should be returned for aggregated query metric click (metabase#33811)
+ {
+ drillType: "drill-thru/zoom-in.timeseries",
+ clickType: "cell",
+ queryType: "aggregated",
+ columnName: "count",
+ expectedParameters: {
+ displayName: "See this month by week",
+ type: "drill-thru/zoom-in.timeseries",
+ },
+ },
+ {
+ drillType: "drill-thru/zoom-in.timeseries",
+ clickType: "cell",
+ queryType: "aggregated",
+ columnName: "max",
+ expectedParameters: {
+ displayName: "See this month by week",
+ type: "drill-thru/zoom-in.timeseries",
+ },
+ },
+ {
+ drillType: "drill-thru/zoom-in.timeseries",
+ clickType: "cell",
+ queryType: "aggregated",
+ columnName: "sum",
+ expectedParameters: {
+ displayName: "See this month by week",
+ type: "drill-thru/zoom-in.timeseries",
+ },
+ },
// endregion
// region --- drill-thru/zoom
@@ -2030,31 +2034,35 @@ describe("drillThru", () => {
// },
// },
- // FIXME: fk-details doesn't create a query for fk target table (metabase#34383)
- // {
- // drillType: "drill-thru/fk-details",
- // clickType: "cell",
- // columnName: "PRODUCT_ID",
- // queryType: "unaggregated",
- // expectedQuery: {
- // filter: [
- // "=",
- // ["field", PRODUCTS.ID, null],
- // ORDERS_ROW_VALUES.PRODUCT_ID,
- // ],
- // "source-table": PRODUCTS_ID,
- // },
- // },
- // {
- // drillType: "drill-thru/fk-details",
- // clickType: "cell",
- // columnName: "USER_ID",
- // queryType: "unaggregated",
- // expectedQuery: {
- // filter: ["=", ["field", PEOPLE.ID, null], ORDERS_ROW_VALUES.USER_ID],
- // "source-table": PEOPLE_ID,
- // },
- // },
+ // fk-details should create a query for fk target table (metabase#34383)
+ {
+ drillType: "drill-thru/fk-details",
+ clickType: "cell",
+ columnName: "PRODUCT_ID",
+ queryType: "unaggregated",
+ expectedQuery: {
+ filter: [
+ "=",
+ ["field", PRODUCTS.ID, { "base-type": "type/BigInteger" }],
+ ORDERS_ROW_VALUES.PRODUCT_ID,
+ ],
+ "source-table": PRODUCTS_ID,
+ },
+ },
+ {
+ drillType: "drill-thru/fk-details",
+ clickType: "cell",
+ columnName: "USER_ID",
+ queryType: "unaggregated",
+ expectedQuery: {
+ filter: [
+ "=",
+ ["field", PEOPLE.ID, { "base-type": "type/BigInteger" }],
+ ORDERS_ROW_VALUES.USER_ID,
+ ],
+ "source-table": PEOPLE_ID,
+ },
+ },
])(
'should return correct result on "$drillType" drill apply to $columnName on $clickType in $queryType query',
({
@@ -2176,18 +2184,23 @@ describe("drillThru", () => {
"order-by": [["asc", ["aggregation", 3]]],
},
},
- // FIXME: should support sorting for custom column without table relation (metabase#34499)
- // {
- // // should support sorting for custom column without table relation
- // drillType: "drill-thru/sort",
- // clickType: "header",
- // columnName: "CustomColumn",
- // drillArgs: ["asc"],
- // expectedQuery: {
- // ...ORDERS_WITH_CUSTOM_COLUMN_DATASET_QUERY.query,
- // "order-by": [["asc", ["expression", "CustomColumn"]]],
- // },
- // },
+ // should support sorting for custom column without table relation (metabase#34499)
+ {
+ // should support sorting for custom column without table relation
+ drillType: "drill-thru/sort",
+ clickType: "header",
+ columnName: "CustomColumn",
+ drillArgs: ["asc"],
+ expectedQuery: {
+ ...ORDERS_WITH_CUSTOM_COLUMN_DATASET_QUERY.query,
+ "order-by": [
+ [
+ "asc",
+ ["expression", "CustomColumn", { "base-type": "type/Integer" }],
+ ],
+ ],
+ },
+ },
])(
'should return correct result on "$drillType" drill apply to $columnName on $clickType in query with custom column',
({ columnName, clickType, drillArgs, expectedQuery, drillType }) => {
diff --git a/src/metabase/lib/drill_thru.cljc b/src/metabase/lib/drill_thru.cljc
index 99be59180ae..356799c9e9d 100644
--- a/src/metabase/lib/drill_thru.cljc
+++ b/src/metabase/lib/drill_thru.cljc
@@ -68,14 +68,14 @@
`:return-drills-for-dimensions?` specifies which type we have."
[{:f #'lib.drill-thru.distribution/distribution-drill, :return-drills-for-dimensions? true}
{:f #'lib.drill-thru.column-filter/column-filter-drill, :return-drills-for-dimensions? true}
- {:f #'lib.drill-thru.foreign-key/foreign-key-drill, :return-drills-for-dimensions? true}
- {:f #'lib.drill-thru.object-details/object-detail-drill, :return-drills-for-dimensions? true}
+ {:f #'lib.drill-thru.foreign-key/foreign-key-drill, :return-drills-for-dimensions? false}
+ {:f #'lib.drill-thru.object-details/object-detail-drill, :return-drills-for-dimensions? false}
{:f #'lib.drill-thru.pivot/pivot-drill, :return-drills-for-dimensions? true}
{:f #'lib.drill-thru.quick-filter/quick-filter-drill, :return-drills-for-dimensions? false}
{:f #'lib.drill-thru.sort/sort-drill, :return-drills-for-dimensions? true}
{:f #'lib.drill-thru.summarize-column/summarize-column-drill, :return-drills-for-dimensions? true}
{:f #'lib.drill-thru.summarize-column-by-time/summarize-column-by-time-drill, :return-drills-for-dimensions? true}
- {:f #'lib.drill-thru.underlying-records/underlying-records-drill, :return-drills-for-dimensions? true}
+ {:f #'lib.drill-thru.underlying-records/underlying-records-drill, :return-drills-for-dimensions? false}
{:f #'lib.drill-thru.zoom-in-timeseries/zoom-in-timeseries-drill, :return-drills-for-dimensions? true}])
(mu/defn ^:private dimension-contexts :- [:maybe [:sequential {:min 1} ::lib.schema.drill-thru/context]]
diff --git a/src/metabase/lib/drill_thru/fk_details.cljc b/src/metabase/lib/drill_thru/fk_details.cljc
index 6a2a208bc1f..d0bd6ade19c 100644
--- a/src/metabase/lib/drill_thru/fk_details.cljc
+++ b/src/metabase/lib/drill_thru/fk_details.cljc
@@ -1,40 +1,25 @@
(ns metabase.lib.drill-thru.fk-details
- "[[metabase.lib.drill-thru.object-details/object-detail-drill]] has the logic for determining whether to return this
+ "An FK details drill is one where you click a foreign key value in a table view e.g. ORDERS.USER_ID and choose the
+ 'View details' option, then it shows you the PEOPLE record in question (e.g. Person 5 if USER_ID was 5).
+
+ [[metabase.lib.drill-thru.object-details/object-detail-drill]] has the logic for determining whether to return this
drill as an option or not."
(:require
[metabase.lib.drill-thru.common :as lib.drill-thru.common]
[metabase.lib.filter :as lib.filter]
[metabase.lib.metadata :as lib.metadata]
- [metabase.lib.metadata.calculation :as lib.metadata.calculation]
- [metabase.lib.types.isa :as lib.types.isa]
- [metabase.lib.util :as lib.util]))
+ [metabase.lib.query :as lib.query]))
(defmethod lib.drill-thru.common/drill-thru-info-method :drill-thru/fk-details
[_query _stage-number drill-thru]
(select-keys drill-thru [:many-pks? :object-id :type]))
-(defn- field-id [x]
- (cond
- (int? x) x
- (string? x) x
- (and (vector? x)
- (= :field (first x))) (field-id (nth x 2))
- (map? x) (:id x)))
-
(defmethod lib.drill-thru.common/drill-thru-method :drill-thru/fk-details
- [query stage-number {:keys [column object-id]} & _]
+ [query _stage-number {:keys [column object-id]} & _]
+ ;; generate a NEW query against the FK target table and column, e.g. if the original query was
+ ;; ORDERS/ORDERS.USER_ID, the new query should by PEOPLE/PEOPLE.ID.
(let [fk-column-id (:fk-target-field-id column)
- fk-column (lib.metadata/field query fk-column-id)
- fk-filter (lib.filter/= fk-column object-id)
- ;; Only filters which specify other PKs of the table are allowed to remain.
- other-pk? (fn [[op _opts lhs :as _old-filter]]
- (and lhs
- (not= (field-id lhs) fk-column-id)
- (= op :=)
- (when-let [filter-field (lib.metadata.calculation/metadata query stage-number lhs)]
- (and (lib.types.isa/primary-key? filter-field)
- (= (:table-id fk-column) (:table-id filter-field))))))
- other-pk-filters (filter other-pk? (lib.filter/filters query stage-number))]
- (reduce #(lib.filter/filter %1 stage-number %2)
- (lib.util/update-query-stage query stage-number dissoc :filters)
- (concat [fk-filter] other-pk-filters))))
+ fk-column (some->> fk-column-id (lib.metadata/field query))
+ fk-column-table (some->> (:table-id fk-column) (lib.metadata/table query))]
+ (-> (lib.query/query query fk-column-table)
+ (lib.filter/filter (lib.filter/= fk-column object-id)))))
diff --git a/src/metabase/lib/drill_thru/foreign_key.cljc b/src/metabase/lib/drill_thru/foreign_key.cljc
index 68c71d2b314..904abd799ca 100644
--- a/src/metabase/lib/drill_thru/foreign_key.cljc
+++ b/src/metabase/lib/drill_thru/foreign_key.cljc
@@ -15,9 +15,9 @@
This has the same effect as the `=` filter on a generic field (ie. not a key), but renders differently.
Contrast [[object-detail-drill]], which shows the details of the foreign object."
- [query :- ::lib.schema/query
- stage-number :- :int
- {:keys [column value]} :- ::lib.schema.drill-thru/context]
+ [query :- ::lib.schema/query
+ stage-number :- :int
+ {:keys [column value], :as _context} :- ::lib.schema.drill-thru/context]
(when (and (lib.drill-thru.common/mbql-stage? query stage-number)
column
(some? value)
diff --git a/test/metabase/lib/drill_thru/fk_details_test.cljc b/test/metabase/lib/drill_thru/fk_details_test.cljc
index 3028e6bb24a..648407b40fd 100644
--- a/test/metabase/lib/drill_thru/fk_details_test.cljc
+++ b/test/metabase/lib/drill_thru/fk_details_test.cljc
@@ -1,7 +1,14 @@
(ns metabase.lib.drill-thru.fk-details-test
(:require
- [clojure.test :refer [deftest]]
- [metabase.lib.drill-thru.test-util :as lib.drill-thru.tu]))
+ [clojure.test :refer [deftest is testing]]
+ [medley.core :as m]
+ [metabase.lib.core :as lib]
+ [metabase.lib.drill-thru.test-util :as lib.drill-thru.tu]
+ [metabase.lib.test-metadata :as meta]
+ [metabase.util :as u]
+ #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))
+
+#?(:cljs (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me))
(deftest ^:parallel returns-fk-details-test-1
(lib.drill-thru.tu/test-returns-drill
@@ -22,3 +29,66 @@
:expected {:type :drill-thru/fk-details
:object-id (get-in lib.drill-thru.tu/test-queries ["ORDERS" :unaggregated :row "USER_ID"])
:many-pks? false}}))
+
+(deftest ^:parallel do-not-return-fk-details-for-non-fk-column-test
+ (testing "fk-details should not get returned for non-fk column (#34441)"
+ (let [test-case {:click-type :cell
+ :query-type :aggregated
+ :column-name "max"}
+ {:keys [query row]} (lib.drill-thru.tu/query-and-row-for-test-case test-case)
+ context (lib.drill-thru.tu/test-case-context query row test-case)]
+ (testing (str "\nQuery = \n" (u/pprint-to-str query)
+ "\nContext =\n" (u/pprint-to-str context))
+ (let [drills (into #{}
+ (map :type)
+ (lib/available-drill-thrus query context))]
+ (testing (str "\nAvailable drills =\n" (u/pprint-to-str drills))
+ (is (not (contains? drills :drill-thru/fk-details)))))))))
+
+(deftest ^:parallel apply-fk-details-test
+ (testing "fk-details should create a correct query for fk target table (#34383)"
+ (let [test-case {:click-type :cell
+ :query-type :unaggregated
+ :column-name "PRODUCT_ID"}
+ {:keys [query row]} (lib.drill-thru.tu/query-and-row-for-test-case test-case)
+ context (lib.drill-thru.tu/test-case-context query row test-case)]
+ (is (get row "PRODUCT_ID"))
+ (testing (str "\nQuery = \n" (u/pprint-to-str query)
+ "\nContext =\n" (u/pprint-to-str context))
+ (let [drill (m/find-first #(= (:type %) :drill-thru/fk-details)
+ (lib/available-drill-thrus query context))]
+ (is (=? {:lib/type :metabase.lib.drill-thru/drill-thru
+ :type :drill-thru/fk-details,
+ :column {:name "PRODUCT_ID"}
+ :object-id (get row "PRODUCT_ID")
+ :many-pks? false}
+ drill))
+ (is (=? {:stages [{:source-table (meta/id :products)
+ :filters [[:= {}
+ [:field {} (meta/id :products :id)]
+ (get row "PRODUCT_ID")]]}]}
+ (lib/drill-thru query -1 drill))))))))
+
+(deftest ^:parallel apply-fk-details-test-2
+ (testing "fk-details should create a correct query for fk target table (#34383)"
+ (let [test-case {:click-type :cell
+ :query-type :unaggregated
+ :column-name "USER_ID"}
+ {:keys [query row]} (lib.drill-thru.tu/query-and-row-for-test-case test-case)
+ context (lib.drill-thru.tu/test-case-context query row test-case)]
+ (is (get row "USER_ID"))
+ (testing (str "\nQuery = \n" (u/pprint-to-str query)
+ "\nContext =\n" (u/pprint-to-str context))
+ (let [drill (m/find-first #(= (:type %) :drill-thru/fk-details)
+ (lib/available-drill-thrus query context))]
+ (is (=? {:lib/type :metabase.lib.drill-thru/drill-thru
+ :type :drill-thru/fk-details,
+ :column {:name "USER_ID"}
+ :object-id (get row "USER_ID")
+ :many-pks? false}
+ drill))
+ (is (=? {:stages [{:source-table (meta/id :people)
+ :filters [[:= {}
+ [:field {} (meta/id :people :id)]
+ (get row "USER_ID")]]}]}
+ (lib/drill-thru query -1 drill))))))))
diff --git a/test/metabase/lib/drill_thru/foreign_key_test.cljc b/test/metabase/lib/drill_thru/foreign_key_test.cljc
index a4651d63cba..8c3e1743524 100644
--- a/test/metabase/lib/drill_thru/foreign_key_test.cljc
+++ b/test/metabase/lib/drill_thru/foreign_key_test.cljc
@@ -1,7 +1,9 @@
(ns metabase.lib.drill-thru.foreign-key-test
(:require
- [clojure.test :refer [deftest]]
- [metabase.lib.drill-thru.test-util :as lib.drill-thru.tu]))
+ [clojure.test :refer [deftest is testing]]
+ [metabase.lib.core :as lib]
+ [metabase.lib.drill-thru.test-util :as lib.drill-thru.tu]
+ [metabase.util :as u]))
(deftest ^:parallel returns-fk-filter-test-1
(lib.drill-thru.tu/test-returns-drill
@@ -19,11 +21,26 @@
:column-name "PRODUCT_ID"
:expected {:type :drill-thru/fk-filter}}))
-;;; FIXME `fk-filter` doesn't get returned for fk column that was used as breakout (#34440)
(deftest ^:parallel returns-fk-filter-test-3
- #_(lib.drill-thru.tu/test-returns-drill
+ (testing "`fk-filter` should get returned for fk column that was used as breakout (#34440)"
+ (lib.drill-thru.tu/test-returns-drill
{:drill-type :drill-thru/fk-filter
:click-type :cell
:query-type :aggregated
:column-name "PRODUCT_ID"
- :expected {:type :drill-thru/fk-filter}}))
+ :expected {:type :drill-thru/fk-filter}})))
+
+(deftest ^:parallel do-not-return-fk-filter-for-non-fk-column-test
+ (testing "fk-filter should not get returned for non-fk column (#34440)"
+ (let [test-case {:click-type :cell
+ :query-type :aggregated
+ :column-name "max"}
+ {:keys [query row]} (lib.drill-thru.tu/query-and-row-for-test-case test-case)
+ context (lib.drill-thru.tu/test-case-context query row test-case)]
+ (testing (str "\nQuery = \n" (u/pprint-to-str query)
+ "\nContext =\n" (u/pprint-to-str context))
+ (let [drills (into #{}
+ (map :type)
+ (lib/available-drill-thrus query context))]
+ (testing (str "\nAvailable drills =\n" (u/pprint-to-str drills))
+ (is (not (contains? drills :drill-thru/fk-filter)))))))))
diff --git a/test/metabase/lib/drill_thru/sort_test.cljc b/test/metabase/lib/drill_thru/sort_test.cljc
index 47a97743f9e..d9e60b21b06 100644
--- a/test/metabase/lib/drill_thru/sort_test.cljc
+++ b/test/metabase/lib/drill_thru/sort_test.cljc
@@ -201,3 +201,31 @@
(get-in lib.drill-thru.tu/test-queries ["ORDERS" :aggregated :query])
(lib/order-by (meta/field-metadata :orders :created-at) :asc))
:expected {:type :drill-thru/sort, :sort-directions [:desc]}}))
+
+(deftest ^:parallel custom-column-test
+ (testing "should support sorting for custom column without table relation (metabase#34499)"
+ (let [{:keys [query row]} (get-in lib.drill-thru.tu/test-queries ["ORDERS" :aggregated])
+ query (as-> query query
+ (lib/expression query "CustomColumn" (lib/+ 1 1))
+ (lib/expression query "CustomTax" (lib/+ (meta/field-metadata :orders :tax) 2))
+ (lib/aggregate query (lib/avg (lib/expression-ref query "CustomTax")))
+ (lib/breakout query (lib/expression-ref query "CustomColumn")))
+
+ row (merge row
+ {"CustomColumn" 2
+ "avg" 13.2})
+ context (lib.drill-thru.tu/test-case-context query
+ row
+ {:column-name "CustomColumn"
+ :click-type :header
+ :query-type :aggregated})
+ drill (m/find-first
+ #(= (:type %) :drill-thru/sort)
+ (lib/available-drill-thrus query -1 context))]
+ (is (=? {:type :drill-thru/sort
+ :column {:name "CustomColumn"}
+ :sort-directions [:asc :desc]}
+ drill))
+ (let [query' (lib/drill-thru query drill)]
+ (is (=? [[:asc {} [:expression {} "CustomColumn"]]]
+ (lib/order-bys query')))))))
diff --git a/test/metabase/lib/drill_thru/test_util.cljc b/test/metabase/lib/drill_thru/test_util.cljc
index 81d935a7f50..19e13232e8d 100644
--- a/test/metabase/lib/drill_thru/test_util.cljc
+++ b/test/metabase/lib/drill_thru/test_util.cljc
@@ -60,22 +60,22 @@
[:query-type [:enum :aggregated :unaggregated]]
[:column-name :string]
;; defaults to "ORDERS"
- [:query-table {:optional true} [:enum "ORDERS" #_"PRODUCTS"]]
+ [:query-table {:optional true} [:enum "ORDERS" "PRODUCTS"]]
[:custom-query {:optional true} ::lib.schema/query]])
(def ^:private Row
[:map-of :string :any])
-(mu/defn ^:private query-and-row-for-test-case :- [:map
- [:query ::lib.schema/query]
- [:row Row]]
+(mu/defn query-and-row-for-test-case :- [:map
+ [:query ::lib.schema/query]
+ [:row Row]]
[{:keys [query-table query-type]
:or {query-table "ORDERS"}
:as test-case} :- TestCase]
(or (get-in test-queries [query-table query-type])
(throw (ex-info "Invalid query-table/query-:type no matching test query" {:test-case test-case}))))
-(mu/defn ^:private test-case-context :- ::lib.schema.drill-thru/context
+(mu/defn test-case-context :- ::lib.schema.drill-thru/context
[query :- ::lib.schema/query
row :- Row
{:keys [column-name click-type query-type], :as _test-case} :- TestCase]
diff --git a/test/metabase/lib/drill_thru/underlying_records_test.cljc b/test/metabase/lib/drill_thru/underlying_records_test.cljc
index 48967cfb4ba..8b4ba1ac0c4 100644
--- a/test/metabase/lib/drill_thru/underlying_records_test.cljc
+++ b/test/metabase/lib/drill_thru/underlying_records_test.cljc
@@ -1,7 +1,9 @@
(ns metabase.lib.drill-thru.underlying-records-test
(:require
- [clojure.test :refer [deftest]]
- [metabase.lib.drill-thru.test-util :as lib.drill-thru.tu]))
+ [clojure.test :refer [deftest is testing]]
+ [metabase.lib.core :as lib]
+ [metabase.lib.drill-thru.test-util :as lib.drill-thru.tu]
+ [metabase.util :as u]))
(deftest ^:parallel returns-underlying-records-test-1
(lib.drill-thru.tu/test-returns-drill
@@ -9,7 +11,7 @@
:click-type :cell
:query-type :aggregated
:column-name "count"
- :expected {:type :drill-thru/underlying-records, :row-count 3, :table-name "Orders"}}))
+ :expected {:type :drill-thru/underlying-records, :row-count 77, :table-name "Orders"}}))
(deftest ^:parallel returns-underlying-records-test-2
(lib.drill-thru.tu/test-returns-drill
@@ -17,7 +19,7 @@
:click-type :cell
:query-type :aggregated
:column-name "sum"
- :expected {:type :drill-thru/underlying-records, :row-count 3, :table-name "Orders"}}))
+ :expected {:type :drill-thru/underlying-records, :row-count 1, :table-name "Orders"}}))
(deftest ^:parallel returns-underlying-records-test-3
(lib.drill-thru.tu/test-returns-drill
@@ -25,4 +27,19 @@
:click-type :cell
:query-type :aggregated
:column-name "max"
- :expected {:type :drill-thru/underlying-records, :row-count 3, :table-name "Orders"}}))
+ :expected {:type :drill-thru/underlying-records, :row-count 2, :table-name "Orders"}}))
+
+(deftest ^:parallel do-not-return-fk-filter-for-non-fk-column-test
+ (testing "underlying-records should only get shown once for aggregated query (#34439)"
+ (let [test-case {:click-type :cell
+ :query-type :aggregated
+ :column-name "max"}
+ {:keys [query row]} (lib.drill-thru.tu/query-and-row-for-test-case test-case)
+ context (lib.drill-thru.tu/test-case-context query row test-case)]
+ (testing (str "\nQuery = \n" (u/pprint-to-str query)
+ "\nContext =\n" (u/pprint-to-str context))
+ (let [drills (lib/available-drill-thrus query context)]
+ (testing (str "\nAvailable drills =\n" (u/pprint-to-str drills))
+ (is (= 1
+ (count (filter #(= (:type %) :drill-thru/underlying-records)
+ drills))))))))))
diff --git a/test/metabase/lib/drill_thru/zoom_in_timeseries_test.cljc b/test/metabase/lib/drill_thru/zoom_in_timeseries_test.cljc
index fe95140e4a0..e9d1b37ec49 100644
--- a/test/metabase/lib/drill_thru/zoom_in_timeseries_test.cljc
+++ b/test/metabase/lib/drill_thru/zoom_in_timeseries_test.cljc
@@ -1,8 +1,9 @@
(ns metabase.lib.drill-thru.zoom-in-timeseries-test
(:require
- [clojure.test :refer [deftest is]]
+ [clojure.test :refer [deftest is testing]]
[medley.core :as m]
[metabase.lib.core :as lib]
+ [metabase.lib.drill-thru.test-util :as lib.drill-thru.tu]
[metabase.lib.drill-thru.zoom-in-timeseries :as lib.drill-thru.zoom-in-timeseries]
[metabase.lib.temporal-bucket :as lib.temporal-bucket]
[metabase.lib.test-metadata :as meta]
@@ -79,29 +80,29 @@
"2022-04-01T00:00:00"]]}]}
query''))))))))
-;;; FIXME zoom-in.timeseries should be returned for aggregated query metric click (#33811)
(deftest ^:parallel returns-zoom-in-timeseries-test-1
- #_(lib.drill-thru.tu/test-returns-drill
+ (testing "zoom-in.timeseries should be returned for aggregated query metric click (#33811)"
+ (lib.drill-thru.tu/test-returns-drill
{:drill-type :drill-thru/zoom-in.timeseries
- :click-type :header
+ :click-type :cell
:query-type :aggregated
:column-name "count"
- :expected {:type :drill-thru/zoom-in.timeseries}}))
+ :expected {:type :drill-thru/zoom-in.timeseries}})))
-;;; FIXME zoom-in.timeseries should be returned for aggregated query metric click (#33811)
(deftest ^:parallel returns-zoom-in-timeseries-test-2
- #_(lib.drill-thru.tu/test-returns-drill
- {:drill-type :drill-thru/zoom-in.timeseries
- :click-type :header
- :query-type :aggregated
- :column-name "max"
- :expected {:type :drill-thru/zoom-in.timeseries}}))
+ (testing "zoom-in.timeseries should be returned for aggregated query metric click (#33811)"
+ (lib.drill-thru.tu/test-returns-drill
+ {:drill-type :drill-thru/zoom-in.timeseries
+ :click-type :cell
+ :query-type :aggregated
+ :column-name "max"
+ :expected {:type :drill-thru/zoom-in.timeseries}})))
-;;; FIXME zoom-in.timeseries should be returned for aggregated query metric click (#33811)
(deftest ^:parallel returns-zoom-in-timeseries-test-3
- #_(lib.drill-thru.tu/test-returns-drill
+ (testing "zoom-in.timeseries should be returned for aggregated query metric click (#33811)"
+ (lib.drill-thru.tu/test-returns-drill
{:drill-type :drill-thru/zoom-in.timeseries
- :click-type :header
+ :click-type :cell
:query-type :aggregated
:column-name "sum"
- :expected {:type :drill-thru/zoom-in.timeseries}}))
+ :expected {:type :drill-thru/zoom-in.timeseries}})))
diff --git a/test/metabase/lib/drill_thru_test.cljc b/test/metabase/lib/drill_thru_test.cljc
index 4814bdfe15f..547ba40b227 100644
--- a/test/metabase/lib/drill_thru_test.cljc
+++ b/test/metabase/lib/drill_thru_test.cljc
@@ -379,7 +379,7 @@
{:name "≠"}]}
:underlying-records {:lib/type :metabase.lib.drill-thru/drill-thru
:type :drill-thru/underlying-records
- :row-count 2
+ :row-count 457
:table-name "Orders"}
:zoom-in.timeseries {:lib/type :metabase.lib.drill-thru/drill-thru
:display-name "See this month by week"
@@ -618,35 +618,37 @@
{:type :drill-thru/sort, :sort-directions [:asc :desc]}
{:type :drill-thru/summarize-column, :aggregations [:distinct]}]}))
-;; FIXME: fk-filter gets returned for non-fk column (#34440), fk-details gets returned for non-fk
-;; column (#34441), underlying-records drill gets shown two times for aggregated query (#34439)
(deftest ^:parallel available-drill-thrus-test-9
- #_(lib.drill-thru.tu/test-available-drill-thrus
- {:click-type :cell
- :query-type :aggregated
- :column-name "count"
- :expected [{:type :drill-thru/quick-filter
- :operators [{:name "<"}
- {:name ">"}
- {:name "="}
- {:name "≠"}]}
- {:type :drill-thru/underlying-records
- :row-count 2
- :table-name "Orders"}
- {:display-name "See this month by week"
- :type :drill-thru/zoom-in.timeseries}]}))
-
-;; FIXME: fk-filter gets returned for non-fk column (#34440), fk-details gets returned for non-fk
-;; column (#34441), underlying-records drill gets shown two times for aggregated query (#34439)
+ (testing (str "fk-filter should not get returned for non-fk column (#34440) "
+ "fk-details should not get returned for non-fk column (#34441) "
+ "underlying-records should only get shown once for aggregated query (#34439)"))
+ (lib.drill-thru.tu/test-available-drill-thrus
+ {:click-type :cell
+ :query-type :aggregated
+ :column-name "count"
+ :expected [{:type :drill-thru/quick-filter
+ :operators [{:name "<"}
+ {:name ">"}
+ {:name "="}
+ {:name "≠"}]}
+ {:type :drill-thru/underlying-records
+ :row-count 77
+ :table-name "Orders"}
+ {:display-name "See this month by week"
+ :type :drill-thru/zoom-in.timeseries}]}))
+
(deftest ^:parallel available-drill-thrus-test-10
- #_(lib.drill-thru.tu/test-available-drill-thrus
+ (testing (str "fk-filter should not get returned for non-fk column (#34440) "
+ "fk-details should not get returned for non-fk column (#34441) "
+ "underlying-records should only get shown once for aggregated query (#34439)")
+ (lib.drill-thru.tu/test-available-drill-thrus
{:click-type :cell
:query-type :aggregated
:column-name "max"
:expected [{:type :drill-thru/quick-filter, :operators [{:name "="}
{:name "≠"}]}
{:type :drill-thru/underlying-records, :row-count 2, :table-name "Orders"}
- {:type :drill-thru/zoom-in.timeseries, :display-name "See this month by week"}]}))
+ {:type :drill-thru/zoom-in.timeseries, :display-name "See this month by week"}]})))
;; FIXME: quick-filter gets returned for non-metric column (#34443)
(deftest ^:parallel available-drill-thrus-test-11
--
GitLab