Skip to content

Commit

Permalink
Adjust dependent dashboard cards (metabase#49308)
Browse files Browse the repository at this point in the history 
* [WIP] Adjust dependent dashboard cards

* Use transducer to make it less hairy (?)

* Looks better now

* Exception handling and log

* Throw away indices

* Add mapping deletion

* Avoid redundant db roundtrips

* Add test

* Update update-mapping

* Docstring

* Docstrings

* Format

* Comment

* comment

* Shutdown deletion

* Adjust update generationcode

* Add tests

* Comment

* Remove deletion completely for now

* Use transaction

* Update test

* Comment

* Update src/metabase/models/card.clj

Co-authored-by: Braden Shepherdson <[email protected]>

* Update src/metabase/models/card.clj

Co-authored-by: Braden Shepherdson <[email protected]>

* Update src/metabase/models/card.clj

Co-authored-by: Braden Shepherdson <[email protected]>

* Format and parens

* Address remaining remarks

* cljfmt

* Update src/metabase/models/card.clj

Co-authored-by: Braden Shepherdson <[email protected]>

* Address review remarks

---------

Co-authored-by: Braden Shepherdson <[email protected]>
  • Loading branch information
@lbrdnk @bshepherdson
lbrdnk and bshepherdson authored Oct 31, 2024
1 parent 6923ccb commit f40dc5f
Show file tree
Hide file tree
Showing 3 changed files with 199 additions and 1 deletion.
96 changes: 95 additions & 1 deletion src/metabase/models/card.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 [<action> & 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."
Expand All @@ -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)
Expand Down
52 changes: 52 additions & 0 deletions test/metabase/api/dashboard_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))))))
52 changes: 52 additions & 0 deletions test/metabase/models/card_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}]]}]}]]))

0 comments on commit f40dc5f

Please sign in to comment.