diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj index 732fdd814f157..a4e00be52ab1f 100644 --- a/src/metabase/models/card.clj +++ b/src/metabase/models/card.clj @@ -767,6 +767,92 @@ :query_type ;; these first three may not even be changeable :dataset_query}) +(defn- breakout-->identifier->refs + "Generate mapping of of _ref identifier_ -> #{_ref..._}. + + _ref identifier_ is a vector of first 2 elements of ref, eg. [:expression \"xix\"] or [:field 10]" + [breakouts] + (-> (group-by #(subvec % 0 2) breakouts) + (update-vals set))) + +(defn- action-for-identifier+refs + "Generate _action_ for combination of args. + + _Action_ is to be performed on _parameter mapping_ of a _dashcard_. For more info see + the [[update-associated-parameters!]]'s docstring. + + _Action_ has a form of [ & args]." + [after--identifier->refs identifier before--refs] + (let [after--refs (get after--identifier->refs identifier #{})] + (when (and (= 1 (count before--refs) (count after--refs)) + (not= before--refs after--refs)) + [:update (first after--refs)]))) + +(defn- breakouts-->identifier->action + "Generate mapping of _identifier_ -> _action_. + + _identifier_ is is a vector of first 2 elements of ref, eg. [:expression \"xix\"] or [:field 10]. Action is generated + in [[action-for-identifier+refs]] and performed later in [[update-mapping]]." + [breakout-before-update breakout-after-update] + (let [before--identifier->refs (breakout-->identifier->refs breakout-before-update) + after--identifier->refs (breakout-->identifier->refs breakout-after-update)] + ;; Remove no-ops to avoid redundant db calls in [[update-associated-parameters!]]. + (->> before--identifier->refs + (m/map-kv-vals #(action-for-identifier+refs after--identifier->refs %1 %2)) + (m/filter-vals some?) + not-empty))) + +(defn eligible-mapping? + "Decide whether parameter mapping has strucuture so it can be updated presumably using [[update-mapping]]." + [{[dim [ref-kind]] :target :as _mapping}] + (and (= dim :dimension) + (#{:field :expression} ref-kind))) + +(defn- update-mapping + "Return modifed mapping according to action." + [identifier->action {[_dim ref] :target :as mapping}] + (let [identifier (subvec ref 0 2) + [action arg] (get identifier->action identifier)] + (case action + :update (assoc-in mapping [:target 1] arg) + mapping))) + +(defn- updates-for-dashcards + [identifier->action dashcards] + (not-empty (for [{:keys [id parameter_mappings]} dashcards + :let [updated (into [] (map #(if (eligible-mapping? %) + (update-mapping identifier->action %) + %)) + parameter_mappings)] + :when (not= parameter_mappings updated)] + [id {:parameter_mappings updated}]))) + +(defn- update-associated-parameters! + "Update _parameter mappings_ of _dashcards_ that target modified _card_, to reflect the modification. + + This function handles only modifications to breakout. + + Context. Card can have multiple multiple breakout elements referencing same field or expression, having different + _temporal unit_. Those refs can be targeted by dashboard _temporal unit parameter_. If refs change, and card is saved, + _parameter mappings_ have to be updated to target new, modified refs. This function takes care of that. + + First mappings of _identifier_ -> _action_ are generated. _identifier_ is described + eg. in [[breakouts-->identifier->action]] docstring. Then, dashcards are fetched and updates are generated + by [[updates-for-dashcards]]. Updates are then executed." + [card-before card-after] + (let [card->breakout #(-> % :dataset_query mbql.normalize/normalize :query :breakout) + breakout-before (card->breakout card-before) + breakout-after (card->breakout card-after)] + (when-some [identifier->action (breakouts-->identifier->action breakout-before breakout-after)] + (let [dashcards (t2/select :model/DashboardCard :card_id (some :id [card-after card-before])) + updates (updates-for-dashcards identifier->action dashcards)] + ;; Beware. This can have negative impact on card update performance as queries are fired in sequence. I'm not + ;; aware of more reasonable way. + (when (seq updates) + (t2/with-transaction [_conn] + (doseq [[id update] updates] + (t2/update! :model/DashboardCard :id id update)))))))) + (defn update-card! "Update a Card. Metadata is fetched asynchronously. If it is ready before [[metadata-sync-wait-ms]] elapses it will be included, otherwise the metadata will be saved to the database asynchronously." @@ -792,7 +878,15 @@ :present #{:collection_id :collection_position :description :cache_ttl :archived_directly} :non-nil #{:dataset_query :display :name :visualization_settings :archived :enable_embedding :type :parameters :parameter_mappings :embedding_params - :result_metadata :collection_preview :verified-result-metadata?}))) + :result_metadata :collection_preview :verified-result-metadata?})) + ;; ok, now update dependent dashcard parameters + (try + (update-associated-parameters! card-before-update card-updates) + (catch Throwable e + (log/error "Update of dependent card parameters failed!") + (log/debug e + "`card-before-update`:" (pr-str card-before-update) + "`card-updates`:" (pr-str card-updates))))) ;; Fetch the updated Card from the DB (let [card (t2/select-one Card :id (:id card-before-update))] (delete-alerts-if-needed! :old-card card-before-update, :new-card card, :actor actor) diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj index 0ccf486fe634f..83ec9d454aa38 100644 --- a/test/metabase/api/dashboard_test.clj +++ b/test/metabase/api/dashboard_test.clj @@ -4864,3 +4864,55 @@ ;; dashcards and parameters for each dashcard, linked to a single card. Following is the proof ;; of things working as described. (is (= 1 @call-count)))))))) + +;; Exception during scheduled (grouper) update of UserParameterValue is thrown. It is not relevant in context +;; of tested functionality. +;; TODO: Address the exception! +(deftest dependent-dashcard-parameters-test + (mt/with-temp [:model/Card {card-id :id} {:name "c1" + :dataset_query (mt/mbql-query + orders + {:aggregation [[:count]] + :breakout + [!day.$created_at]})} + :model/Dashboard {dashboard-id :id} {:name "d1" + :parameters []} + :model/DashboardCard {dashcard-id :id} {:card_id card-id + :dashboard_id dashboard-id + :parameter_mappings []}] + (t2/update! :model/Dashboard :id dashboard-id {:parameters [{:name "TIME Gr" + :slug "tgr" + :id "30d7efb0" + :type :temporal-unit + :sectionId "temporal-unit"}]}) + (t2/update! :model/DashboardCard :id dashcard-id {:parameter_mappings [{:parameter_id "30d7efb0" + :type :temporal-unit + :card_id card-id + :target [:dimension + (mt/$ids orders !day.$created_at)]}]}) + (testing "Baseline" + (is (=? [["2016-04-01T00:00:00Z" 1] + ["2016-05-01T00:00:00Z" 19]] + (->> (mt/user-http-request + :crowberto :post 202 + (format "dashboard/%d/dashcard/%d/card/%d/query" dashboard-id dashcard-id card-id) + {:parameters [{:id "30d7efb0" + :type "temporal-unit" + :value "month" + :target + [:dimension + (mt/$ids orders !day.$created_at)]}]}) + mt/rows + (take 2))))) + (mt/user-http-request + :crowberto :put 200 + (format "card/%d" card-id) + {:dataset_query (mt/mbql-query + orders + {:aggregation [[:count]] + :breakout + [!year.$created_at]})}) + (testing "Mapping is adjusted to new target (#49202)" + (is (= (mt/$ids orders !year.$created_at) + (t2/select-one-fn #(get-in % [:parameter_mappings 0 :target 1]) + :model/DashboardCard :id dashcard-id)))))) diff --git a/test/metabase/models/card_test.clj b/test/metabase/models/card_test.clj index dacc5199bb637..eee2dc0903cc0 100644 --- a/test/metabase/models/card_test.clj +++ b/test/metabase/models/card_test.clj @@ -1037,3 +1037,55 @@ (mt/with-test-user :crowberto (is (false? (mi/can-read? card))) (is (false? (mi/can-write? card))))))))) + +(deftest breakouts-->identifier->action-fn-test + (are [b1 b2 expected--identifier->action] (= expected--identifier->action + (#'card/breakouts-->identifier->action b1 b2)) + [[:field 10 {:temporal-unit :day}]] + nil + nil + + [[:expression "x" {:temporal-unit :day}]] + nil + nil + + [[:expression "x" {:temporal-unit :day}]] + [[:expression "x" {:temporal-unit :month}]] + {[:expression "x"] [:update [:expression "x" {:temporal-unit :month}]]} + + [[:expression "x" {:temporal-unit :day}]] + [[:expression "x" {:temporal-unit :day}]] + nil + + [[:field 10 {:temporal-unit :day}] [:expression "x" {:temporal-unit :day}]] + [[:expression "x" {:temporal-unit :day}] [:field 10 {:temporal-unit :month}]] + {[:field 10] [:update [:field 10 {:temporal-unit :month}]]} + + [[:field 10 {:temporal-unit :year}] [:field 10 {:temporal-unit :day-of-week}]] + [[:field 10 {:temporal-unit :year}]] + nil)) + +(deftest update-for-dashcard-fn-test + (are [indetifier->action quasi-dashcards expected-quasi-dashcards] + (= expected-quasi-dashcards + (#'card/updates-for-dashcards indetifier->action quasi-dashcards)) + + {[:field 10] [:update [:field 10 {:temporal-unit :month}]]} + [{:parameter_mappings []}] + nil + + {[:field 10] [:update [:field 10 {:temporal-unit :month}]]} + [{:id 1 :parameter_mappings [{:target [:dimension [:field 10 nil]]}]}] + [[1 {:parameter_mappings [{:target [:dimension [:field 10 {:temporal-unit :month}]]}]}]] + + {[:field 10] [:noop]} + [{:id 1 :parameter_mappings [{:target [:dimension [:field 10 nil]]}]}] + nil + + {[:field 10] [:update [:field 10 {:temporal-unit :month}]]} + [{:id 1 :parameter_mappings [{:target [:dimension [:field 10 {:temporal-unit :year}]]} + {:target [:dimension [:field 33 {:temporal-unit :month}]]} + {:target [:dimension [:field 10 {:temporal-unit :day}]]}]}] + [[1 {:parameter_mappings [{:target [:dimension [:field 10 {:temporal-unit :month}]]} + {:target [:dimension [:field 33 {:temporal-unit :month}]]} + {:target [:dimension [:field 10 {:temporal-unit :month}]]}]}]]))