diff --git a/src/metabase/pulse.clj b/src/metabase/pulse.clj index 9c4beff7a00f12f11811e6cda19a38fbc2a4738d..1a6ea99cc0c53bcee5a784309198d67396574780 100644 --- a/src/metabase/pulse.clj +++ b/src/metabase/pulse.clj @@ -29,6 +29,18 @@ ;;; ------------------------------------------------- PULSE SENDING -------------------------------------------------- +(defn- merge-default-values + "For the specific case of Dashboard Subscriptions we should use `:default` parameter values as the actual `:value` for + the parameter if none is specified. Normally the FE client will take `:default` and pass it in as `:value` if it + wants to use it (see #20503 for more details) but this obviously isn't an option for Dashboard Subscriptions... so + go thru `parameters` and change `:default` to `:value` unless a `:value` is explicitly specified." + [parameters] + (for [{default-value :default, :as parameter} parameters] + (merge + (when default-value + {:value default-value}) + (dissoc parameter :default)))) + (defn- execute-dashboard-subscription-card [owner-id dashboard dashcard card-or-id parameters] (try @@ -41,7 +53,7 @@ :dashcard-id (u/the-id dashcard) :context :pulse ; TODO - we should support for `:dashboard-subscription` and use that to differentiate the two :export-format :api - :parameters parameters + :parameters (merge-default-values parameters) :middleware {:process-viz-settings? true :js-int-to-string? false} :run (fn [query info] diff --git a/src/metabase/query_processor/dashboard.clj b/src/metabase/query_processor/dashboard.clj index 3a3287b7e1777f01770fb182cb929874d862ad89..7f3e148c9fc4459914c2705557e9e740e981ec6e 100644 --- a/src/metabase/query_processor/dashboard.clj +++ b/src/metabase/query_processor/dashboard.clj @@ -112,6 +112,9 @@ request-params :- (s/maybe [su/Map])] (log/tracef "Resolving Dashboard %d Card %d query request parameters" dashboard-id card-id) (let [request-params (normalize/normalize-fragment [:parameters] request-params) + ;; ignore default values in request params as well. (#20516) + request-params (for [param request-params] + (dissoc param :default)) dashboard (api/check-404 (db/select-one Dashboard :id dashboard-id)) dashboard-param-id->param (into {} ;; remove the `:default` values from Dashboard params. We don't ACTUALLY want to diff --git a/test/metabase/dashboard_subscription_test.clj b/test/metabase/dashboard_subscription_test.clj index 8c234683a50fcabbcc0b8e226304925c229a3ead..b554514ed08eafc20829fbde9add9a40015374a2 100644 --- a/test/metabase/dashboard_subscription_test.clj +++ b/test/metabase/dashboard_subscription_test.clj @@ -367,3 +367,60 @@ :email (fn [_ _] (is (= {} (mt/summarize-multipart-email))))}})) + +(deftest use-default-values-test + (testing "Dashboard Subscriptions SHOULD use default values for Dashboard parameters when running (#20516)" + (mt/dataset sample-dataset + (mt/with-temp Dashboard [{dashboard-id :id, :as dashboard} {:name "20516 Dashboard" + :parameters [{:name "Category" + :slug "category" + :id "_MBQL_CATEGORY_" + :type :category + :default ["Doohickey"]} + {:name "SQL Category" + :slug "sql_category" + :id "_SQL_CATEGORY_" + :type :category + :default ["Gizmo"]}]}] + (testing "MBQL query" + (mt/with-temp* [Card [{mbql-card-id :id} {:name "Orders" + :dataset_query (mt/mbql-query products + {:fields [$id $title $category] + :order-by [[:asc $id]] + :limit 2})}] + DashboardCard [_ {:parameter_mappings [{:parameter_id "_MBQL_CATEGORY_" + :card_id mbql-card-id + :target [:dimension [:field (mt/id :products :category) nil]]}] + :card_id mbql-card-id + :dashboard_id dashboard-id}]] + (let [[mbql-results] (map :result (@#'pulse/execute-dashboard {:creator_id (mt/user->id :rasta)} dashboard))] + (is (= [[2 "Small Marble Shoes" "Doohickey"] + [3 "Synergistic Granite Chair" "Doohickey"]] + (mt/rows mbql-results)))))) + (testing "SQL Query" + (mt/with-temp* [Card [{sql-card-id :id} {:name "Products (SQL)" + :dataset_query (mt/native-query + {:query + (str "SELECT id, title, category\n" + "FROM products\n" + "WHERE {{category}}\n" + "ORDER BY id ASC\n" + "LIMIT 2") + + :template-tags + {"category" + {:id "_SQL_CATEGORY_TEMPLATE_TAG_" + :name "category" + :display-name "Category" + :type :dimension + :dimension [:field (mt/id :products :category) nil] + :widget-type :category}}})}] + DashboardCard [_ {:parameter_mappings [{:parameter_id "_SQL_CATEGORY_" + :card_id sql-card-id + :target [:dimension [:template-tag "category"]]}] + :card_id sql-card-id + :dashboard_id dashboard-id}]] + (let [[results] (map :result (@#'pulse/execute-dashboard {:creator_id (mt/user->id :rasta)} dashboard))] + (is (= [[1 "Rustic Paper Wallet" "Gizmo"] + [10 "Mediocre Wooden Table" "Gizmo"]] + (mt/rows results)))))))))) diff --git a/test/metabase/query_processor/dashboard_test.clj b/test/metabase/query_processor/dashboard_test.clj index c518ee1ce1ec419961f285d12b69e7841b625bf1..c1efbe9b432bf45b30a0bd6a3d71b42bc041e7ca 100644 --- a/test/metabase/query_processor/dashboard_test.clj +++ b/test/metabase/query_processor/dashboard_test.clj @@ -198,3 +198,39 @@ :dashboard_id dashboard-id}]] (is (= [[200]] (mt/rows (run-query-for-dashcard dashboard-id card-id dashcard-id))))))))) + +(deftest ignore-default-values-in-request-parameters-test + (testing "Parameters passed in from the request with only default values (but no actual values) should get ignored (#20516)" + (mt/dataset sample-dataset + (mt/with-temp* [Card [{card-id :id} {:name "Orders" + :dataset_query (mt/mbql-query products + {:fields [$id $title $category] + :order-by [[:asc $id]] + :limit 2})}] + Dashboard [{dashboard-id :id} {:name "20516 Dashboard" + :parameters [{:name "Category" + :slug "category" + :id "_CATEGORY_" + :type :category + :default ["Doohickey"]}]}] + DashboardCard [{dashcard-id :id} {:parameter_mappings [{:parameter_id "_CATEGORY_" + :card_id card-id + :target [:dimension [:field (mt/id :products :category) nil]]}] + :card_id card-id + :dashboard_id dashboard-id}]] + (testing "No parameters -- ignore Dashboard default (#20493, #20503)" + ;; [[metabase.query-processor.middleware.large-int-id]] middleware is converting the IDs to strings I guess + (is (= [["1" "Rustic Paper Wallet" "Gizmo"] + ["2" "Small Marble Shoes" "Doohickey"]] + (mt/rows + (run-query-for-dashcard dashboard-id card-id dashcard-id))))) + (testing "Request parameters with :default -- ignore these as well (#20516)" + (is (= [["1" "Rustic Paper Wallet" "Gizmo"] + ["2" "Small Marble Shoes" "Doohickey"]] + (mt/rows + (run-query-for-dashcard dashboard-id card-id dashcard-id + :parameters [{:name "Category" + :slug "category" + :id "_CATEGORY_" + :type :category + :default ["Gizmo"]}])))))))))