From 6a319625ccdb253d8fedae7141896134dc1f7b2f Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Wed, 11 Jan 2023 18:13:05 +0700
Subject: [PATCH] Add default keys for dashboard.parameters that are used for
custom-list (#27609)
* add default values keys for dashboard.parmaeters
* fix tests and add tests
---
shared/src/metabase/mbql/normalize.cljc | 10 +++--
src/metabase/models/dashboard.clj | 12 +++++-
test/metabase/api/dashboard_test.clj | 13 +++---
test/metabase/api/embed_test.clj | 4 +-
test/metabase/api/preview_embed_test.clj | 14 +++---
test/metabase/models/dashboard_test.clj | 54 +++++++++++++++++++++---
6 files changed, 82 insertions(+), 25 deletions(-)
diff --git a/shared/src/metabase/mbql/normalize.cljc b/shared/src/metabase/mbql/normalize.cljc
index 062cb8d7321..e6ae89b6394 100644
--- a/shared/src/metabase/mbql/normalize.cljc
+++ b/shared/src/metabase/mbql/normalize.cljc
@@ -287,12 +287,14 @@
(defn normalize-query-parameter
"Normalize a parameter in the query `:parameters` list."
- [{:keys [type target id], :as param}]
+ [{:keys [type target id values_source_config], :as param}]
(cond-> param
- id (update :id mbql.u/qualified-name)
+ id (update :id mbql.u/qualified-name)
;; some things that get ran thru here, like dashcard param targets, do not have :type
- type (update :type maybe-normalize-token)
- target (update :target #(normalize-tokens % :ignore-path))))
+ type (update :type maybe-normalize-token)
+ target (update :target #(normalize-tokens % :ignore-path))
+ values_source_config (update-in [:values_source_config :label_field] #(normalize-tokens % :ignore-path))
+ values_source_config (update-in [:values_source_config :value_field] #(normalize-tokens % :ignore-path))))
(defn- normalize-source-query [source-query]
(let [{native? :native, :as source-query} (m/map-keys maybe-normalize-token source-query)]
diff --git a/src/metabase/models/dashboard.clj b/src/metabase/models/dashboard.clj
index 0475c47bf21..00f4bf8d23d 100644
--- a/src/metabase/models/dashboard.clj
+++ b/src/metabase/models/dashboard.clj
@@ -140,6 +140,16 @@
:collection_id (:collection_id dashboard))
(pulse-card/bulk-create! new-pulse-cards)))))))
+(defn- with-default-parameters-value
+ [{:keys [parameters] :as dashboard}]
+ (cond-> dashboard
+ (seq parameters)
+ (update :parameters (fn [parameters]
+ (map #(merge {:values_query_type "list"
+ :values_source_type nil
+ :values_source_config {}}
+ %) parameters)))))
+
(defn- post-update
[dashboard]
(update-dashboard-subscription-pulses! dashboard))
@@ -154,7 +164,7 @@
:post-insert post-insert
:pre-update pre-update
:post-update post-update
- :post-select public-settings/remove-public-uuid-if-public-sharing-is-disabled})
+ :post-select (comp public-settings/remove-public-uuid-if-public-sharing-is-disabled with-default-parameters-value)})
(defmethod serdes.hash/identity-hash-fields Dashboard
[_dashboard]
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index 2a7b7245fcc..c9dec97da9e 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -171,7 +171,8 @@
dashboard-defaults
{:name test-dashboard-name
:creator_id (mt/user->id :rasta)
- :parameters [{:id "abc123", :name "test", :type "date"}]
+ :parameters [{:id "abc123", :name "test", :type "date"
+ :values_query_type "list", :values_source_type nil, :values_source_config {}}]
:updated_at true
:created_at true
:collection_id true
@@ -1039,11 +1040,11 @@
(try
(is (= 2
(count (db/select-ids DashboardCard, :dashboard_id copy-id))))
- (is (= [{:name "Category ID" :slug "category_id" :id "_CATEGORY_ID_" :type :category}]
+ (is (=? [{:name "Category ID" :slug "category_id" :id "_CATEGORY_ID_" :type :category}]
(db/select-one-field :parameters Dashboard :id copy-id)))
- (is (= [{:parameter_id "random-id"
- :card_id card-id
- :target [:dimension [:field (mt/id :venues :name) nil]]}]
+ (is (=? [{:parameter_id "random-id"
+ :card_id card-id
+ :target [:dimension [:field (mt/id :venues :name) nil]]}]
(db/select-one-field :parameter_mappings DashboardCard :id dashcard-id)))
(finally
(db/delete! Dashboard :id copy-id))))))))
@@ -1874,12 +1875,14 @@
:slug "static_category"
:id "_STATIC_CATEGORY_",
:type "category",
+ :values_query_type "search"
:values_source_type "static-list"
:values_source_config {"values" ["BBQ" "Bakery" "Bar"]}}]})]
(is (= [{:name "Static Category",
:slug "static_category"
:id "_STATIC_CATEGORY_",
:type "category",
+ :values_query_type "search"
:values_source_type "static-list"
:values_source_config {:values ["BBQ" "Bakery" "Bar"]}}]
(:parameters dashboard))))))
diff --git a/test/metabase/api/embed_test.clj b/test/metabase/api/embed_test.clj
index 85dbcfc8f8a..cc996646346 100644
--- a/test/metabase/api/embed_test.clj
+++ b/test/metabase/api/embed_test.clj
@@ -425,8 +425,8 @@
{:id "_b", :slug "b", :name "b", :type "date"}
{:id "_c", :slug "c", :name "c", :type "date"}
{:id "_d", :slug "d", :name "d", :type "date"}]}]
- (is (= [{:id "_d", :slug "d", :name "d", :type "date"}]
- (:parameters (client/client :get 200 (dashboard-url dash {:params {:c 100}})))))))))
+ (is (=? [{:id "_d", :slug "d", :name "d", :type "date"}]
+ (:parameters (client/client :get 200 (dashboard-url dash {:params {:c 100}})))))))))
(deftest locked-params-are-substituted-into-text-cards
(testing "check that locked params are substituted into text cards with mapped variables on the backend"
diff --git a/test/metabase/api/preview_embed_test.clj b/test/metabase/api/preview_embed_test.clj
index 19738257f63..8e85aa846cc 100644
--- a/test/metabase/api/preview_embed_test.clj
+++ b/test/metabase/api/preview_embed_test.clj
@@ -201,13 +201,13 @@
{:id "_b", :slug "b", :name "b", :type "date"}
{:id "_c", :slug "c", :name "c", :type "date"}
{:id "_d", :slug "d", :name "d", :type "date"}]}]
- (is (= [{:id "_d", :slug "d", :name "d", :type "date"}]
- (:parameters (mt/user-http-request :crowberto :get 200 (dashboard-url dash
- {:params {:c 100}
- :_embedding_params {:a "locked"
- :b "disabled"
- :c "enabled"
- :d "enabled"}})))))))))
+ (is (=? [{:id "_d", :slug "d", :name "d", :type "date"}]
+ (:parameters (mt/user-http-request :crowberto :get 200 (dashboard-url dash
+ {:params {:c 100}
+ :_embedding_params {:a "locked"
+ :b "disabled"
+ :c "enabled"
+ :d "enabled"}})))))))))
;;; ------------------ GET /api/preview_embed/dashboard/:token/dashcard/:dashcard-id/card/:card-id -------------------
diff --git a/test/metabase/models/dashboard_test.clj b/test/metabase/models/dashboard_test.clj
index b6bf59de301..c73b5eeac8b 100644
--- a/test/metabase/models/dashboard_test.clj
+++ b/test/metabase/models/dashboard_test.clj
@@ -346,18 +346,60 @@
(doseq [[target expected] {[:dimension [:field-id 1000]] [:dimension [:field 1000 nil]]
[:field-id 1000] [:field 1000 nil]}]
(testing (format "target = %s" (pr-str target))
- (mt/with-temp Dashboard [{dashboard-id :id} {:parameters [{:name "Category Name"
- :slug "category_name"
- :id "_CATEGORY_NAME_"
- :type "category"
- :target target}]}]
+ (mt/with-temp* [Card [{card-id :id}]
+ Dashboard [{dashboard-id :id} {:parameters [{:name "Category Name"
+ :slug "category_name"
+ :id "_CATEGORY_NAME_"
+ :type "category"
+ :values_query_type "list"
+ :values_source_type "card"
+ :values_source_config {:card_id card-id
+ :value_field [:field 2 nil]}
+ :target target}]}]]
(is (= [{:name "Category Name"
:slug "category_name"
:id "_CATEGORY_NAME_"
:type :category
- :target expected}]
+ :target expected
+ :values_query_type "list",
+ :values_source_type "card",
+ :values_source_config {:card_id card-id, :value_field [:field 2 nil]}}]
(db/select-one-field :parameters Dashboard :id dashboard-id))))))))
+(deftest should-add-default-values-source-test
+ (testing "shoudld add default if not exists"
+ (mt/with-temp Dashboard [{dashboard-id :id} {:parameters [{:name "Category Name"
+ :slug "category_name"
+ :id "_CATEGORY_NAME_"
+ :type "category"}]}]
+ (is (=? [{:name "Category Name"
+ :slug "category_name"
+ :id "_CATEGORY_NAME_"
+ :type :category
+ :values_query_type "list",
+ :values_source_type nil,
+ :values_source_config {}}]
+ (db/select-one-field :parameters Dashboard :id dashboard-id)))))
+
+ (testing "shoudld not override if existsed "
+ (mt/with-temp* [Card [{card-id :id}]
+ Dashboard [{dashboard-id :id} {:parameters [{:name "Category Name"
+ :slug "category_name"
+ :id "_CATEGORY_NAME_"
+ :type "category"
+ :values_query_type "list"
+ :values_source_type "card"
+ :values_source_config {:card_id card-id
+ :value_field [:field 2 nil]}}]}]]
+ (is (=? [{:name "Category Name"
+ :slug "category_name"
+ :id "_CATEGORY_NAME_"
+ :type :category
+ :values_query_type "list",
+ :values_source_type "card",
+ :values_source_config {:card_id card-id, :value_field [:field 2 nil]}}]
+ (db/select-one-field :parameters Dashboard :id dashboard-id))))))
+
(deftest identity-hash-test
(testing "Dashboard hashes are composed of the name and parent collection's hash"
(let [now (LocalDateTime/of 2022 9 1 12 34 56)]
--
GitLab