From 9e21ae22bf5f72b79f4d766952d8fab9905db9ac Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Thu, 1 Jun 2023 17:55:52 +0700
Subject: [PATCH] Dashboard tab revision (#30898)
---
src/metabase/api/dashboard.clj | 213 ++++-----
src/metabase/events/revision.clj | 3 +
src/metabase/models/dashboard.clj | 86 ++--
src/metabase/models/dashboard_tab.clj | 81 +++-
src/metabase/util.cljc | 30 +-
test/metabase/api/dashboard_test.clj | 20 +-
test/metabase/api/revision_test.clj | 6 +-
test/metabase/events/revision_test.clj | 103 ++++-
test/metabase/models/dashboard_test.clj | 564 +++++++++++++++++-------
test/metabase/util_test.cljc | 9 +
10 files changed, 755 insertions(+), 360 deletions(-)
diff --git a/src/metabase/api/dashboard.clj b/src/metabase/api/dashboard.clj
index 8610a07408e..7bb90c31537 100644
--- a/src/metabase/api/dashboard.clj
+++ b/src/metabase/api/dashboard.clj
@@ -18,6 +18,7 @@
[metabase.models.collection :as collection]
[metabase.models.dashboard :as dashboard :refer [Dashboard]]
[metabase.models.dashboard-card :as dashboard-card :refer [DashboardCard]]
+ [metabase.models.dashboard-tab :as dashboard-tab]
[metabase.models.field :refer [Field]]
[metabase.models.interface :as mi]
[metabase.models.params :as params]
@@ -541,88 +542,6 @@
(assoc mapping :card-id (get dashcard-id->card-id dashcard-id)))]
(check-parameter-mapping-permissions new-mappings)))
-(mu/defn ^:private classify-changes :- [:map
- [:to-create [:sequential [:map [:id ms/NegativeInt]]]]
- [:to-update [:sequential [:map [:id ms/PositiveInt]]]]
- [:to-delete [:sequential [:map [:id ms/PositiveInt]]]]]
- "Given 2 lists of seq maps of changes, where each map an has an `id` key,
- return a map of 3 keys: `:to-create`, `:to-update`, `:to-delete`.
-
- Where:
- :to-create is a list of maps that has negative ids in `new-items`
- :to-update is a list of maps that has ids in both `current-items` and `new-items`
- :to delete is a list of maps that has ids only in `current-items`"
- [current-items :- [:sequential [:map [:id ms/PositiveInt]]]
- new-items :- [:sequential [:map [:id ms/Int]]]]
- (let [new-change-ids (set (map :id new-items))
- to-create (filter (comp neg? :id) new-items)
- ;; to-update items are new items with id in the current items
- to-update (filter (comp pos? :id) new-items)
- ;; to delete items in current but not new items
- to-delete (remove (comp new-change-ids :id) current-items)]
- {:to-update to-update
- :to-delete to-delete
- :to-create to-create}))
-
-(mu/defn ^:private create-tabs! :- [:map-of neg-int? pos-int?]
- "Create the new tabs and returned a mapping from temporary tab ID to the new tab ID."
- [dashboard :- map?
- new-tabs :- [:sequential [:map [:id neg-int?]]]]
- (let [new-tab-ids (t2/insert-returning-pks! :model/DashboardTab (->> new-tabs
- (map #(dissoc % :id))
- (map #(assoc % :dashboard_id (:id dashboard)))))]
-
- (zipmap (map :id new-tabs) new-tab-ids)))
-
-(mu/defn ^:private update-tabs! :- nil?
- [dashboard :- [:map [:id pos-int?]
- [:ordered_tabs [:maybe [:sequential map?]]]]
- new-tabs :- [:sequential [:map [:id ms/PositiveInt]]]]
- (let [current-tabs (:ordered_tabs dashboard)
- update-ks [:name :position]
- id->current-tab (group-by :id current-tabs)
- to-update-tabs (filter
- ;; filter out tabs that haven't changed
- (fn [new-tab]
- (let [current-tab (get id->current-tab (:id new-tab))]
- (not= (select-keys current-tab update-ks)
- (select-keys new-tab update-ks))))
-
- new-tabs)]
- (doseq [tab to-update-tabs]
- (t2/update! :model/DashboardTab (:id tab) (select-keys tab update-ks)))
- nil))
-
-(mu/defn ^:private delete-tabs! :- nil?
- [tab-ids :- [:sequential {:min 1} ms/PositiveInt]]
- (when (seq tab-ids)
- (t2/delete! :model/DashboardTab :id [:in tab-ids]))
- nil)
-
-(defn ^:private do-update-tabs!
- "Given current tabs and new tabs, do the necessary create/update/delete to apply new tab changes.
- Returns:
- - a map of temporary tab ID to the new real tab ID
- - a list of deleted tab ids"
- [dashboard current-tabs new-tabs]
- (when-not (= (range (count new-tabs))
- (sort (map :position new-tabs)))
- (throw (ex-info (tru "Tab positions must be sequential and start at 0")
- {:status-code 400})))
- (let [{:keys [to-create to-update to-delete]} (classify-changes current-tabs new-tabs)
- to-delete-ids (set (map :id to-delete))
- _ (when-let [to-delete-ids (seq to-delete-ids)]
- (delete-tabs! to-delete-ids))
- temp-tab-id->tab-id (when (seq to-create)
- (create-tabs! dashboard to-create))
- _ (when (seq to-update)
- (update-tabs! dashboard to-update))]
- {:temp->real-tab-ids temp-tab-id->tab-id
- :deleted-tab-ids to-delete-ids
- :num-tabs-created (count to-create)
- :num-tabs-deleted (count to-delete)
- :total-num-tabs (+ (count to-create) (count to-update))}))
-
(defn- create-dashcards!
[dashboard dashcards]
(doseq [{:keys [card_id]} dashcards
@@ -631,37 +550,29 @@
(check-parameter-mapping-permissions (for [{:keys [card_id parameter_mappings]} dashcards
mapping parameter_mappings]
(assoc mapping :card-id card_id)))
- (u/prog1 (api/check-500 (dashboard/add-dashcards! dashboard (map #(assoc % :creator_id @api/*current-user*) dashcards)))
- (events/publish-event! :dashboard-add-cards {:id (:id dashboard) :actor_id api/*current-user-id* :dashcards <>})
- (for [{:keys [card_id]} <>
- :when (pos-int? card_id)]
- (snowplow/track-event! ::snowplow/question-added-to-dashboard
- api/*current-user-id*
- {:dashboard-id (:id dashboard) :question-id card_id}))))
+ (api/check-500 (dashboard/add-dashcards! dashboard (map #(assoc % :creator_id @api/*current-user*) dashcards))))
(defn- update-dashcards! [dashboard dashcards]
(check-updated-parameter-mapping-permissions (:id dashboard) dashcards)
;; transform the dashcard data to the format of the DashboardCard model
;; so update-dashcards! can compare them with existing dashcards
(dashboard/update-dashcards! dashboard (map dashboard-card/from-parsed-json dashcards))
- ;; TODO this is potentially misleading, we don't know for sure here that the dashcards are repositioned
- (events/publish-event! :dashboard-reposition-cards {:id (:id dashboard) :actor_id api/*current-user-id* :dashcards dashcards}))
+ dashcards)
-(defn- delete-dashcards! [{dashboard-id :id :as _dashboard} dashcard-ids]
- (when (seq dashcard-ids)
- (let [dashboard-cards (t2/select DashboardCard :id [:in dashcard-ids])]
- (dashboard-card/delete-dashboard-cards! dashcard-ids)
- (events/publish-event! :dashboard-remove-cards {:id dashboard-id :actor_id api/*current-user-id* :dashcards dashboard-cards}))))
+(defn- delete-dashcards! [dashcard-ids]
+ (let [dashboard-cards (t2/select DashboardCard :id [:in dashcard-ids])]
+ (dashboard-card/delete-dashboard-cards! dashcard-ids)
+ dashboard-cards))
(defn- do-update-dashcards!
[dashboard current-cards new-cards]
- (let [{:keys [to-create to-update to-delete]} (classify-changes current-cards new-cards)]
- (when (seq to-delete)
- (delete-dashcards! dashboard (map :id to-delete)))
- (when (seq to-create)
- (create-dashcards! dashboard to-create))
- (when (seq to-update)
- (update-dashcards! dashboard to-update))))
+ (let [{:keys [to-create to-update to-delete]} (u/classify-changes current-cards new-cards)]
+ {:deleted-dashcards (when (seq to-delete)
+ (delete-dashcards! (map :id to-delete)))
+ :created-dashcards (when (seq to-create)
+ (create-dashcards! dashboard to-create))
+ :updated-dashcards (when (seq to-update)
+ (update-dashcards! dashboard to-update))}))
(def ^:private UpdatedDashboardCard
[:map
@@ -682,6 +593,47 @@
[:id ms/Int]
[:name ms/NonBlankString]])
+(defn- track-dashcard-and-tab-events!
+ [dashboard-id {:keys [created-dashcards deleted-dashcards updated-dashcards
+ created-tab-ids updated-tab-ids deleted-tab-ids total-num-tabs]}]
+ ;; Dashcard events
+ (when (seq deleted-dashcards)
+ (events/publish-event! :dashboard-remove-cards
+ {:id dashboard-id :actor_id api/*current-user-id* :dashcards deleted-dashcards}))
+ (when (seq created-dashcards)
+ (events/publish-event! :dashboard-add-cards
+ {:id dashboard-id :actor_id api/*current-user-id* :dashcards created-dashcards})
+ (for [{:keys [card_id]} created-dashcards
+ :when (pos-int? card_id)]
+ (snowplow/track-event! ::snowplow/question-added-to-dashboard
+ api/*current-user-id*
+ {:dashboard-id dashboard-id :question-id card_id})))
+ ;; TODO this is potentially misleading, we don't know for sure here that the dashcards are repositioned
+ (when (seq updated-dashcards)
+ (events/publish-event! :dashboard-reposition-cards
+ {:id dashboard-id :actor_id api/*current-user-id* :dashcards updated-dashcards}))
+
+ ;; Tabs events
+ (when (seq deleted-tab-ids)
+ (snowplow/track-event! ::snowplow/dashboard-tabs-deleted
+ api/*current-user-id*
+ {:dashboard-id dashboard-id
+ :num-tabs (count deleted-tab-ids)
+ :total-num-tabs total-num-tabs})
+ (events/publish-event! :dashboard-remove-tabs
+ {:id dashboard-id :actor_id api/*current-user-id* :tab-ids deleted-tab-ids}))
+ (when (seq created-tab-ids)
+ (snowplow/track-event! ::snowplow/dashboard-tabs-created
+ api/*current-user-id*
+ {:dashboard-id dashboard-id
+ :num-tabs (count created-tab-ids)
+ :total-num-tabs total-num-tabs})
+ (events/publish-event! :dashboard-add-tabs
+ {:id dashboard-id :actor_id api/*current-user-id* :tab-ids created-tab-ids}))
+ (when (seq updated-tab-ids)
+ (events/publish-event! :dashboard-update-tabs
+ {:id dashboard-id :actor_id api/*current-user-id* :tab-ids updated-tab-ids})))
+
(api/defendpoint PUT "/:id/cards"
"Update `Cards` and `Tabs` on a Dashboard. Request body should have the form:
@@ -711,40 +663,33 @@
(throw (ex-info (tru "This dashboard has tab, makes sure every card has a tab")
{:status-code 400})))
(api/check-500
- (let [tabs-stats-atom (atom nil)]
+ (let [changes-stats (atom nil)]
(t2/with-transaction [_conn]
- (let [{:keys [temp->real-tab-ids
+ (let [{:keys [old->new-tab-id
deleted-tab-ids]
- :as tab-stats} (do-update-tabs! dashboard (:ordered_tabs dashboard) new-tabs)
- current-cards (cond->> (:ordered_cards dashboard)
- (seq deleted-tab-ids)
- (remove (fn [card]
- (contains? deleted-tab-ids (:dashboard_tab_id card)))))
- new-cards (cond->> cards
- ;; fixup the temporary tab ids with the real ones
- (seq temp->real-tab-ids)
- (map (fn [card]
- (if-let [real-tab-id (get temp->real-tab-ids (:dashboard_tab_id card))]
- (assoc card :dashboard_tab_id real-tab-id)
- card))))]
- (do-update-dashcards! dashboard current-cards new-cards)
- (reset! tabs-stats-atom (select-keys tab-stats [:num-tabs-created :num-tabs-deleted :total-num-tabs]))))
- (let [{:keys [num-tabs-created num-tabs-deleted total-num-tabs]} @tabs-stats-atom]
- (when (pos-int? num-tabs-created)
- (snowplow/track-event! ::snowplow/dashboard-tabs-created
- api/*current-user-id*
- {:dashboard-id (:id dashboard)
- :num-tabs num-tabs-created
- :total-num-tabs total-num-tabs}))
- (when (pos-int? num-tabs-deleted)
- (snowplow/track-event! ::snowplow/dashboard-tabs-deleted
- api/*current-user-id*
- {:dashboard-id (:id dashboard)
- :num-tabs num-tabs-deleted
- :total-num-tabs total-num-tabs})))
+ :as tabs-changes-stats} (dashboard-tab/do-update-tabs! (:id dashboard) (:ordered_tabs dashboard) new-tabs)
+ deleted-tab-ids (set deleted-tab-ids)
+ current-cards (cond->> (:ordered_cards dashboard)
+ (seq deleted-tab-ids)
+ (remove (fn [card]
+ (contains? deleted-tab-ids (:dashboard_tab_id card)))))
+ new-cards (cond->> cards
+ ;; fixup the temporary tab ids with the real ones
+ (seq old->new-tab-id)
+ (map (fn [card]
+ (if-let [real-tab-id (get old->new-tab-id (:dashboard_tab_id card))]
+ (assoc card :dashboard_tab_id real-tab-id)
+ card))))
+ dashcards-changes-stats (do-update-dashcards! dashboard current-cards new-cards)]
+ (reset! changes-stats
+ (merge
+ (select-keys tabs-changes-stats [:created-tab-ids :updated-tab-ids :deleted-tab-ids :total-num-tabs])
+ (select-keys dashcards-changes-stats [:created-dashcards :deleted-dashcards :updated-dashcards])))))
+ ;; trigger events out of tx so rows are committed and visible from other threads
+ (track-dashcard-and-tab-events! id @changes-stats)
true))
- {:cards (t2/hydrate (dashboard/ordered-cards id) :series)
- :ordered_tabs (dashboard/ordered-tabs id)}))
+ {:cards (t2/hydrate (dashboard/ordered-cards id) :series)
+ :ordered_tabs (dashboard/ordered-tabs id)}))
(api/defendpoint GET "/:id/revisions"
"Fetch `Revisions` for Dashboard with ID."
diff --git a/src/metabase/events/revision.clj b/src/metabase/events/revision.clj
index 3673a76eb4e..43b2f1810fe 100644
--- a/src/metabase/events/revision.clj
+++ b/src/metabase/events/revision.clj
@@ -19,6 +19,9 @@
:dashboard-add-cards
:dashboard-remove-cards
:dashboard-reposition-cards
+ :dashboard-add-tabs
+ :dashboard-remove-tabs
+ :dashboard-update-tabs
:metric-create
:metric-update
:metric-delete
diff --git a/src/metabase/models/dashboard.clj b/src/metabase/models/dashboard.clj
index 23b1394190d..082d6cb1256 100644
--- a/src/metabase/models/dashboard.clj
+++ b/src/metabase/models/dashboard.clj
@@ -13,6 +13,7 @@
[metabase.models.dashboard-card
:as dashboard-card
:refer [DashboardCard]]
+ [metabase.models.dashboard-tab :as dashboard-tab]
[metabase.models.field-values :as field-values]
[metabase.models.interface :as mi]
[metabase.models.parameter-card :as parameter-card]
@@ -35,7 +36,8 @@
[methodical.core :as methodical]
[schema.core :as s]
[toucan.hydrate :refer [hydrate]]
- [toucan2.core :as t2]))
+ [toucan2.core :as t2]
+ [toucan2.realize :as t2.realize]))
(def Dashboard
"Used to be the toucan1 model name defined using [[toucan.models/defmodel]], not it's a reference to the toucan2 model name.
@@ -44,7 +46,6 @@
(methodical/defmethod t2/table-name :model/Dashboard [_model] :report_dashboard)
-
(doto :model/Dashboard
(derive :metabase/model)
(derive ::perms/use-parent-collection-perms)
@@ -183,36 +184,42 @@
(-> dashboard
(select-keys [:collection_id :description :name :cache_ttl :auto_apply_filters])
(assoc :cards (vec (for [dashboard-card (ordered-cards dashboard)]
- (-> (select-keys dashboard-card [:size_x :size_y :row :col :id :card_id])
- (assoc :series (mapv :id (dashboard-card/series dashboard-card)))))))))
+ (-> (select-keys dashboard-card [:dashboard_tab_id :size_x :size_y :row :col :id :card_id])
+ (assoc :series (mapv :id (dashboard-card/series dashboard-card)))))))
+ (assoc :tabs (map #(dissoc % :created_at :updated_at :entity_id) (ordered-tabs dashboard)))))
+
+(defn- revert-dashcards
+ [dashboard-id serialized-cards]
+ (let [current-cards (t2/select [DashboardCard :id :size_x :size_y :row :col :card_id :dashboard_id]
+ :dashboard_id dashboard-id)
+ id->current-card (zipmap (map :id current-cards) current-cards)
+ {:keys [to-create to-update to-delete]} (u/classify-changes current-cards serialized-cards)]
+ (when (seq to-delete)
+ (dashboard-card/delete-dashboard-cards! (map :id to-delete)))
+ (when (seq to-create)
+ (dashboard-card/create-dashboard-cards! (map #(assoc % :dashboard_id dashboard-id) to-create)))
+ (when (seq to-update)
+ (doseq [update-card to-update]
+ (dashboard-card/update-dashboard-card! update-card (id->current-card (:id update-card)))))))
(defmethod revision/revert-to-revision! :model/Dashboard
- [_model dashboard-id user-id serialized-dashboard]
+ [_model dashboard-id _user-id serialized-dashboard]
;; Update the dashboard description / name / permissions
- (t2/update! :model/Dashboard dashboard-id, (dissoc serialized-dashboard :cards))
- ;; Now update the cards as needed
- (let [serialized-cards (:cards serialized-dashboard)
- id->serialized-card (zipmap (map :id serialized-cards) serialized-cards)
- current-cards (t2/select [DashboardCard :size_x :size_y :row :col :id :card_id :dashboard_id]
- :dashboard_id dashboard-id)
- id->current-card (zipmap (map :id current-cards) current-cards)
- all-dashcard-ids (concat (map :id serialized-cards)
- (map :id current-cards))]
- (doseq [dashcard-id all-dashcard-ids]
- (let [serialized-card (id->serialized-card dashcard-id)
- current-card (id->current-card dashcard-id)]
- (cond
- ;; If card is in current-cards but not serialized-cards then we need to delete it
- (not serialized-card) (dashboard-card/delete-dashboard-cards! [(:id current-card)])
-
- ;; If card is in serialized-cards but not current-cards we need to add it
- (not current-card) (dashboard-card/create-dashboard-cards! [(assoc serialized-card
- :dashboard_id dashboard-id
- :creator_id user-id)])
-
- ;; If card is in both we need to update it to match serialized-card as needed
- :else (dashboard-card/update-dashboard-card! serialized-card current-card)))))
-
+ (t2/update! :model/Dashboard dashboard-id (dissoc serialized-dashboard :cards :tabs))
+ ;; Now update the tabs and cards as needed
+ (let [serialized-cards (:cards serialized-dashboard)
+ current-tabs (t2/select-fn-vec #(dissoc (t2.realize/realize %) :created_at :updated_at :entity_id :dashboard_id)
+ :model/DashboardTab :dashboard_id dashboard-id)
+ {:keys [old->new-tab-id]} (dashboard-tab/do-update-tabs! dashboard-id current-tabs (:tabs serialized-dashboard))
+ serialized-cards (cond->> serialized-cards
+ ;; in case reverting result in new tabs being created,
+ ;; we need to remap the tab-id
+ (seq old->new-tab-id)
+ (map (fn [card]
+ (if-let [new-tab-id (get old->new-tab-id (:dashboard_tab_id card))]
+ (assoc card :dashboard_tab_id new-tab-id)
+ card))))]
+ (revert-dashcards dashboard-id serialized-cards))
serialized-dashboard)
(defmethod revision/diff-strings :model/Dashboard
@@ -258,6 +265,27 @@
(= prev-card-ids new-card-ids)) (deferred-tru "rearranged the cards")
:else (deferred-tru "modified the cards"))))
+ (when (or (:tabs changes) (:tabs removals))
+ (let [prev-tabs (:tabs prev-dashboard)
+ new-tabs (:tabs dashboard)
+ prev-tab-ids (set (map :id prev-tabs))
+ num-prev-tabs (count prev-tab-ids)
+ new-tab-ids (set (map :id new-tabs))
+ num-new-tabs (count new-tab-ids)
+ num-tabs-diff (abs (- num-prev-tabs num-new-tabs))]
+ (cond
+ (and
+ (set/subset? prev-tab-ids new-tab-ids)
+ (< num-prev-tabs num-new-tabs)) (deferred-trun "added a tab" "added {0} tabs" num-tabs-diff)
+
+ (and
+ (set/subset? new-tab-ids prev-tab-ids)
+ (> num-prev-tabs num-new-tabs)) (deferred-trun "removed a tab" "removed {0} tabs" num-tabs-diff)
+
+ (= (set (map #(dissoc % :position) prev-tabs))
+ (set (map #(dissoc % :position) new-tabs))) (deferred-tru "rearranged the tabs")
+
+ :else (deferred-tru "modified the tabs"))))
(let [f (comp boolean :auto_apply_filters)]
(when (not= (f prev-dashboard) (f dashboard))
(deferred-tru "set auto apply filters to {0}" (str (f dashboard)))))]
diff --git a/src/metabase/models/dashboard_tab.clj b/src/metabase/models/dashboard_tab.clj
index 012779578ec..5e8e4903ba9 100644
--- a/src/metabase/models/dashboard_tab.clj
+++ b/src/metabase/models/dashboard_tab.clj
@@ -1,10 +1,12 @@
(ns metabase.models.dashboard-tab
(:require
- [metabase.models.dashboard :refer [Dashboard]]
- [metabase.models.dashboard-card :refer [DashboardCard]]
+ [medley.core :as m]
[metabase.models.interface :as mi]
[metabase.models.serialization :as serdes]
+ [metabase.util :as u]
[metabase.util.date-2 :as u.date]
+ [metabase.util.malli :as mu]
+ [metabase.util.malli.schema :as ms]
[methodical.core :as methodical]
[toucan2.core :as t2]
[toucan2.tools.hydrate :as t2.hydrate]))
@@ -40,7 +42,7 @@
(assert (= 1 (count (set (map :dashboard_id tabs)))), "All tabs must belong to the same dashboard")
(let [dashboard-id (:dashboard_id (first tabs))
tab-ids (map :id tabs)
- dashcards (t2/select DashboardCard :dashboard_id dashboard-id :dashboard_tab_id [:in tab-ids])
+ dashcards (t2/select :model/DashboardCard :dashboard_id dashboard-id :dashboard_tab_id [:in tab-ids])
tab-id->dashcards (-> (group-by :dashboard_tab_id dashcards)
(update-vals #(sort dashcard-comparator %)))
ordered-tabs (sort-by :position tabs)]
@@ -50,7 +52,7 @@
(defmethod mi/perms-objects-set :model/DashboardTab
[dashtab read-or-write]
(let [dashboard (or (:dashboard dashtab)
- (t2/select-one Dashboard :id (:dashboard_id dashtab)))]
+ (t2/select-one :model/Dashboard :id (:dashboard_id dashtab)))]
(mi/perms-objects-set dashboard read-or-write)))
@@ -59,19 +61,84 @@
[_dashboard-tab]
[:name
(comp serdes/identity-hash
- #(t2/select-one Dashboard :id %)
+ #(t2/select-one :model/Dashboard :id %)
:dashboard_id)
:position
:created_at])
;; DashboardTabs are not serialized as their own, separate entities. They are inlined onto their parent Dashboards.
(defmethod serdes/generate-path "DashboardTab" [_ dashcard]
- [(serdes/infer-self-path "Dashboard" (t2/select-one 'Dashboard :id (:dashboard_id dashcard)))
+ [(serdes/infer-self-path "Dashboard" (t2/select-one :model/Dashboard :id (:dashboard_id dashcard)))
(serdes/infer-self-path "DashboardTab" dashcard)])
(defmethod serdes/load-xform "DashboardTab"
[dashtab]
(-> dashtab
(dissoc :serdes/meta)
- (update :dashboard_id serdes/*import-fk* 'Dashboard)
+ (update :dashboard_id serdes/*import-fk* :model/Dashboard)
(update :created_at #(if (string? %) (u.date/parse %) %))))
+
+;;; -------------------------------------------------- CRUD fns ------------------------------------------------------
+
+(mu/defn create-tabs! :- [:map-of neg-int? pos-int?]
+ "Create the new tabs and returned a mapping from temporary tab ID to the new tab ID."
+ [dashboard-id :- ms/PositiveInt
+ new-tabs :- [:sequential [:map [:id neg-int?]]]]
+ (let [new-tab-ids (t2/insert-returning-pks! :model/DashboardTab (->> new-tabs
+ (map #(dissoc % :id))
+ (map #(assoc % :dashboard_id dashboard-id))))]
+ (zipmap (map :id new-tabs) new-tab-ids)))
+
+(mu/defn update-tabs! :- nil?
+ "Updates tabs of a dashboard if changed."
+ [current-tabs :- [:sequential [:map [:id ms/PositiveInt]]]
+ new-tabs :- [:sequential [:map [:id ms/PositiveInt]]]]
+ (let [update-ks [:name :position]
+ id->current-tab (m/index-by :id current-tabs)
+ to-update-tabs (filter
+ ;; filter out tabs that haven't changed
+ (fn [new-tab]
+ (let [current-tab (get id->current-tab (:id new-tab))]
+ (not= (select-keys current-tab update-ks)
+ (select-keys new-tab update-ks))))
+
+ new-tabs)]
+ (doseq [tab to-update-tabs]
+ (t2/update! :model/DashboardTab (:id tab) (select-keys tab update-ks)))
+ nil))
+
+(mu/defn delete-tabs! :- nil?
+ "Delete tabs of a Dashboard"
+ [tab-ids :- [:sequential {:min 1} ms/PositiveInt]]
+ (when (seq tab-ids)
+ (t2/delete! :model/DashboardTab :id [:in tab-ids]))
+ nil)
+
+(defn do-update-tabs!
+ "Given current tabs and new tabs, do the necessary create/update/delete to apply new tab changes.
+ Returns:
+ - `old->new-tab-id`: a map from tab IDs in `new-tabs` to newly created tab IDs
+ - `created-tab-ids`
+ - `updated-tab-ids`
+ - `deleted-tab-ids`
+ - `total-num-tabs`: the total number of active tabs after the operation."
+ [dashboard-id current-tabs new-tabs]
+ (let [{:keys [to-create
+ to-update
+ to-delete]} (u/classify-changes current-tabs new-tabs)
+ to-delete-ids (map :id to-delete)
+ _ (when-let [to-delete-ids (seq to-delete-ids)]
+ (delete-tabs! to-delete-ids))
+ old->new-tab-id (when (seq to-create)
+ (let [new-tab-ids (t2/insert-returning-pks! :model/DashboardTab
+ (->> to-create
+ (map #(dissoc % :id))
+ (map #(assoc % :dashboard_id dashboard-id))))]
+ (zipmap (map :id to-create) new-tab-ids)))]
+ (when (seq to-update)
+ (update-tabs! current-tabs to-update))
+ {:old->new-tab-id old->new-tab-id
+ :created-tab-ids (vals old->new-tab-id)
+ :updated-tab-ids (map :id to-update)
+ :deleted-tab-ids to-delete-ids
+ :total-num-tabs (reduce + (map count [to-create to-update]))}))
diff --git a/src/metabase/util.cljc b/src/metabase/util.cljc
index 1a769a45ec7..f2afcdddd50 100644
--- a/src/metabase/util.cljc
+++ b/src/metabase/util.cljc
@@ -2,6 +2,7 @@
"Common utility functions useful throughout the codebase."
(:require
[camel-snake-kebab.internals.macros :as csk.macros]
+ [clojure.data :refer [diff]]
[clojure.pprint :as pprint]
[clojure.set :as set]
[clojure.string :as str]
@@ -794,10 +795,25 @@
m
(assoc m k v)))
([m k v & kvs]
- (let [ret (assoc-default m k v)]
- (if kvs
- (if (next kvs)
- (recur ret (first kvs) (second kvs) (nnext kvs))
- (throw (ex-info "assoc-default expects an even number of key-values"
- {:kvs kvs})))
- ret))))
+ (let [ret (assoc-default m k v)]
+ (if kvs
+ (if (next kvs)
+ (recur ret (first kvs) (second kvs) (nnext kvs))
+ (throw (ex-info "assoc-default expects an even number of key-values"
+ {:kvs kvs})))
+ ret))))
+
+(defn classify-changes
+ "Given 2 lists of seq maps of changes, where each map an has an `id` key,
+ return a map of 3 keys: `:to-create`, `:to-update`, `:to-delete`.
+
+ Where:
+ :to-create is a list of maps that ids in `new-items`
+ :to-update is a list of maps that has ids in both `current-items` and `new-items`
+ :to delete is a list of maps that has ids only in `current-items`"
+ [current-items new-items]
+ (let [[delete-ids create-ids update-ids] (diff (set (map :id current-items))
+ (set (map :id new-items)))]
+ {:to-create (when (seq create-ids) (filter #(create-ids (:id %)) new-items))
+ :to-delete (when (seq delete-ids) (filter #(delete-ids (:id %)) current-items))
+ :to-update (when (seq update-ids) (filter #(update-ids (:id %)) new-items))}))
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index aa7736fd67e..9cf7b114e35 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -1663,14 +1663,14 @@
:name "New tab 2"}]
:cards []})
(is (= [{:data {"dashboard_id" dashboard-id
- "num_tabs" 2
+ "num_tabs" 1
"total_num_tabs" 4
- "event" "dashboard_tabs_created"}
+ "event" "dashboard_tabs_deleted"},
:user-id (str (mt/user->id :rasta))}
{:data {"dashboard_id" dashboard-id
- "num_tabs" 1
+ "num_tabs" 2
"total_num_tabs" 4
- "event" "dashboard_tabs_deleted"},
+ "event" "dashboard_tabs_created"}
:user-id (str (mt/user->id :rasta))}]
(take-last 2 (snowplow-test/pop-event-data-and-user-id!))))))
@@ -2052,7 +2052,7 @@
(is (= 1
(t2/count :model/DashboardTab :dashboard_id dashboard-id)))))))
(testing "prune"
- (with-simple-dashboard-with-tabs [{:keys [dashboard-id dashtab-id-2]}]
+ (with-simple-dashboard-with-tabs [{:keys [dashboard-id]}]
(testing "we have 2 tabs, each has 1 card to begin with"
(is (= 2
(t2/count DashboardCard, :dashboard_id dashboard-id)))
@@ -2063,7 +2063,7 @@
(mt/user-http-request :rasta :put 200
(format "dashboard/%d/cards" dashboard-id)
{:ordered_tabs []
- :cards (remove #(= (:dashboard_tab_id %) dashtab-id-2) (current-cards dashboard-id))})))
+ :cards []})))
(testing "dashboard should be empty"
(testing "0 card left"
(is (= 0
@@ -2072,14 +2072,6 @@
(is (= 0
(t2/count :model/DashboardTab :dashboard_id dashboard-id)))))))))
-(deftest classify-changes-test
- (testing "classify correctly"
- (is (= {:to-update [{:id 2 :name "c3"} {:id 4 :name "c4"}]
- :to-delete [{:id 1 :name "c1"} {:id 3 :name "c3"}]
- :to-create [{:id -1 :name "-c1"}]}
- (#'api.dashboard/classify-changes
- [{:id 1 :name "c1"} {:id 2 :name "c2"} {:id 3 :name "c3"} {:id 4 :name "c4"}]
- [{:id -1 :name "-c1"} {:id 2 :name "c3"} {:id 4 :name "c4"}])))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | GET /api/dashboard/:id/revisions |
diff --git a/test/metabase/api/revision_test.clj b/test/metabase/api/revision_test.clj
index f789f820325..155dc746b83 100644
--- a/test/metabase/api/revision_test.clj
+++ b/test/metabase/api/revision_test.clj
@@ -147,14 +147,14 @@
:message nil
:user @rasta-revision-info
:diff {:before {:cards nil}
- :after {:cards [{:size_x 4, :size_y 4, :row 0, :col 0, :card_id card-id, :series []}]}}
+ :after {:cards [{:size_x 4 :size_y 4 :row 0 :col 0 :card_id card-id :series [] :dashboard_tab_id nil}]}}
:has_multiple_changes false
:description "reverted to an earlier version."}
{:is_reversion false
:is_creation false
:message nil
:user @rasta-revision-info
- :diff {:before {:cards [{:size_x 4, :size_y 4, :row 0, :col 0, :card_id card-id, :series []}]}
+ :diff {:before {:cards [{:size_x 4 :size_y 4 :row 0 :col 0 :card_id card-id :series [] :dashboard_tab_id nil}]}
:after {:cards nil}}
:has_multiple_changes false
:description "removed a card."}
@@ -163,7 +163,7 @@
:message nil
:user @rasta-revision-info
:diff {:before {:cards nil}
- :after {:cards [{:size_x 4, :size_y 4, :row 0, :col 0, :card_id card-id, :series []}]}}
+ :after {:cards [{:size_x 4 :size_y 4 :row 0 :col 0 :card_id card-id :series [] :dashboard_tab_id nil}]}}
:has_multiple_changes false
:description "added a card."}
{:is_reversion false
diff --git a/test/metabase/events/revision_test.clj b/test/metabase/events/revision_test.clj
index d7e13e30dcf..c5137d9022f 100644
--- a/test/metabase/events/revision_test.clj
+++ b/test/metabase/events/revision_test.clj
@@ -6,7 +6,8 @@
:refer [Card Dashboard DashboardCard Database Metric Revision Segment Table]]
[metabase.test :as mt]
[metabase.util :as u]
- [toucan2.core :as t2]))
+ [toucan2.core :as t2]
+ [toucan2.tools.with-temp :as t2.with-temp]))
(defn- card-properties
"Some default properties for `Cards` for use in tests in this namespace."
@@ -48,7 +49,9 @@
:description nil
:cache_ttl nil
:auto_apply_filters true
- :name (:name dashboard)})
+ :name (:name dashboard)
+ :tabs []
+ :cards []})
(deftest card-create-test
(testing ":card-create"
@@ -106,7 +109,7 @@
(is (= {:model "Dashboard"
:model_id dashboard-id
:user_id (mt/user->id :rasta)
- :object (assoc (dashboard->revision-object dashboard) :cards [])
+ :object (dashboard->revision-object dashboard)
:is_reversion false
:is_creation false}
(mt/derecordize
@@ -126,7 +129,8 @@
(is (= {:model "Dashboard"
:model_id dashboard-id
:user_id (mt/user->id :rasta)
- :object (assoc (dashboard->revision-object dashboard) :cards [(assoc (select-keys dashcard [:id :card_id :size_x :size_y :row :col]) :series [])])
+ :object (assoc (dashboard->revision-object dashboard)
+ :cards [(assoc (select-keys dashcard [:id :card_id :size_x :size_y :row :col :dashboard_tab_id]) :series [])])
:is_reversion false
:is_creation false}
(mt/derecordize
@@ -168,13 +172,90 @@
(is (= {:model "Dashboard"
:model_id dashboard-id
:user_id (mt/user->id :crowberto)
- :object (assoc (dashboard->revision-object dashboard) :cards [{:id (:id dashcard)
- :card_id card-id
- :size_x 3
- :size_y 4
- :row 0
- :col 0
- :series []}])
+ :object (assoc (dashboard->revision-object dashboard) :cards [{:id (:id dashcard)
+ :card_id card-id
+ :size_x 3
+ :size_y 4
+ :row 0
+ :col 0
+ :series []
+ :dashboard_tab_id nil}])
+ :is_reversion false
+ :is_creation false}
+ (mt/derecordize
+ (t2/select-one [Revision :model :model_id :user_id :object :is_reversion :is_creation]
+ :model "Dashboard"
+ :model_id dashboard-id)))))))
+
+(deftest dashboard-add-tabs-test
+ (testing ":dashboard-add-tabs"
+ (t2.with-temp/with-temp
+ [:model/Dashboard {dashboard-id :id, :as dashboard} {:name "A dashboard"}
+ :model/DashboardTab {dashtab-id :id} {:name "First tab"
+ :position 0
+ :dashboard_id dashboard-id}]
+ (revision/process-revision-event! {:topic :dashboard-add-tabs
+ :item {:id dashboard-id
+ :actor_id (mt/user->id :rasta)
+ :tab_ids [dashtab-id]}})
+ (is (= {:model "Dashboard"
+ :model_id dashboard-id
+ :user_id (mt/user->id :rasta)
+ :object (assoc (dashboard->revision-object dashboard)
+ :tabs [{:id dashtab-id
+ :name "First tab"
+ :position 0
+ :dashboard_id dashboard-id}])
+ :is_reversion false
+ :is_creation false}
+ (mt/derecordize
+ (t2/select-one [Revision :model :model_id :user_id :object :is_reversion :is_creation]
+ :model "Dashboard"
+ :model_id dashboard-id)))))))
+
+(deftest dashboard-update-tabs-test
+ (testing ":dashboard-update-tabs"
+ (t2.with-temp/with-temp
+ [:model/Dashboard {dashboard-id :id, :as dashboard} {:name "A dashboard"}
+ :model/DashboardTab {dashtab-id :id} {:name "First tab"
+ :position 0
+ :dashboard_id dashboard-id}]
+ (t2/update! :model/DashboardTab dashtab-id {:name "New name"})
+ (revision/process-revision-event! {:topic :dashboard-update-tabs
+ :item {:id dashboard-id
+ :actor_id (mt/user->id :rasta)
+ :tab_ids [dashtab-id]}})
+ (is (= {:model "Dashboard"
+ :model_id dashboard-id
+ :user_id (mt/user->id :rasta)
+ :object (assoc (dashboard->revision-object dashboard)
+ :tabs [{:id dashtab-id
+ :name "New name"
+ :position 0
+ :dashboard_id dashboard-id}])
+ :is_reversion false
+ :is_creation false}
+ (mt/derecordize
+ (t2/select-one [Revision :model :model_id :user_id :object :is_reversion :is_creation]
+ :model "Dashboard"
+ :model_id dashboard-id)))))))
+
+(deftest dashboard-delete-tabs-test
+ (testing ":dashboard-delete-tabs"
+ (t2.with-temp/with-temp
+ [:model/Dashboard {dashboard-id :id, :as dashboard} {:name "A dashboard"}
+ :model/DashboardTab {dashtab-id :id} {:name "First tab"
+ :position 0
+ :dashboard_id dashboard-id}]
+ (t2/delete! :model/DashboardTab dashtab-id)
+ (revision/process-revision-event! {:topic :dashboard-delete-tabs
+ :item {:id dashboard-id
+ :actor_id (mt/user->id :rasta)
+ :tab_ids [dashtab-id]}})
+ (is (= {:model "Dashboard"
+ :model_id dashboard-id
+ :user_id (mt/user->id :rasta)
+ :object (dashboard->revision-object dashboard)
:is_reversion false
:is_creation false}
(mt/derecordize
diff --git a/test/metabase/models/dashboard_test.clj b/test/metabase/models/dashboard_test.clj
index 6ffe1f91abd..e06f4b0ac43 100644
--- a/test/metabase/models/dashboard_test.clj
+++ b/test/metabase/models/dashboard_test.clj
@@ -4,7 +4,7 @@
[metabase.api.common :as api]
[metabase.automagic-dashboards.core :as magic]
[metabase.models :refer [Card Collection Dashboard DashboardCard DashboardCardSeries
- Database Field Pulse PulseCard Table]]
+ Database Field Pulse PulseCard Revision Table]]
[metabase.models.collection :as collection]
[metabase.models.dashboard :as dashboard]
[metabase.models.dashboard-card :as dashboard-card]
@@ -18,6 +18,7 @@
[metabase.test.data.users :as test.users]
[metabase.test.util :as tu]
[metabase.util :as u]
+ [schema.core :as s]
[toucan.util.test :as tt]
[toucan2.core :as t2]
[toucan2.tools.with-temp :as t2.with-temp])
@@ -29,168 +30,213 @@
;; ## Dashboard Revisions
(deftest serialize-dashboard-test
- (tt/with-temp* [Dashboard [{dashboard-id :id :as dashboard} {:name "Test Dashboard"}]
- Card [{card-id :id}]
- Card [{series-id-1 :id}]
- Card [{series-id-2 :id}]
- DashboardCard [{dashcard-id :id} {:dashboard_id dashboard-id, :card_id card-id}]
- DashboardCardSeries [_ {:dashboardcard_id dashcard-id, :card_id series-id-1, :position 0}]
- DashboardCardSeries [_ {:dashboardcard_id dashcard-id, :card_id series-id-2, :position 1}]]
- (is (= {:name "Test Dashboard"
- :auto_apply_filters true
- :collection_id nil
- :description nil
- :cache_ttl nil
- :cards [{:size_x 4
- :size_y 4
- :row 0
- :col 0
- :id true
- :card_id true
- :series true}]}
- (update (revision/serialize-instance Dashboard (:id dashboard) dashboard)
- :cards
- (fn [[{:keys [id card_id series], :as card}]]
- [(assoc card
- :id (= dashcard-id id)
- :card_id (= card-id card_id)
- :series (= [series-id-1 series-id-2] series))]))))))
-
+ (testing "without tabs"
+ (tt/with-temp* [Dashboard [{dashboard-id :id :as dashboard} {:name "Test Dashboard"}]
+ Card [{card-id :id}]
+ Card [{series-id-1 :id}]
+ Card [{series-id-2 :id}]
+ DashboardCard [{dashcard-id :id} {:dashboard_id dashboard-id, :card_id card-id}]
+ DashboardCardSeries [_ {:dashboardcard_id dashcard-id, :card_id series-id-1, :position 0}]
+ DashboardCardSeries [_ {:dashboardcard_id dashcard-id, :card_id series-id-2, :position 1}]]
+ (is (= {:name "Test Dashboard"
+ :auto_apply_filters true
+ :collection_id nil
+ :description nil
+ :cache_ttl nil
+ :cards [{:size_x 4
+ :size_y 4
+ :row 0
+ :col 0
+ :id true
+ :card_id true
+ :series true
+ :dashboard_tab_id nil}]
+ :tabs []}
+ (update (revision/serialize-instance Dashboard (:id dashboard) dashboard)
+ :cards
+ (fn [[{:keys [id card_id series], :as card}]]
+ [(assoc card
+ :id (= dashcard-id id)
+ :card_id (= card-id card_id)
+ :series (= [series-id-1 series-id-2] series))]))))))
+
+ (testing "with tabs"
+ (mt/with-temp* [Dashboard [{dashboard-id :id :as dashboard} {:name "Test Dashboard"}]
+ :model/DashboardTab [{tab-id :id} {:dashboard_id dashboard-id :name "Test Tab" :position 0}]
+ DashboardCard [{dashcard-id :id} {:dashboard_id dashboard-id :dashboard_tab_id tab-id}]]
+ (is (=? {:name "Test Dashboard"
+ :auto_apply_filters true
+ :collection_id nil
+ :description nil
+ :cache_ttl nil
+ :cards [{:size_x 4
+ :size_y 4
+ :row 0
+ :col 0
+ :id dashcard-id
+ :dashboard_tab_id tab-id}]
+ :tabs [{:id tab-id
+ :name "Test Tab"
+ :position 0}]}
+ (revision/serialize-instance Dashboard (:id dashboard) dashboard))))))
(deftest diff-dashboards-str-test
- (is (= "added a description and renamed it from \"Diff Test\" to \"Diff Test Changed\"."
+ (testing "update general info ---"
+ (is (= "added a description and renamed it from \"Diff Test\" to \"Diff Test Changed\"."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:name "Diff Test"
+ :description nil
+ :cards []}
+ {:name "Diff Test Changed"
+ :description "foobar"
+ :cards []}))))
+
+ (is (= "renamed it from \"Apple\" to \"Next\" and modified the cards."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:name "Apple"
+ :cards [{:id 1} {:id 2}]}
+ {:name "Next"
+ :cards [{:id 1} {:id 3}]}))))
+
+ (is (= "set auto apply filters to false."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:name "Diff Test"
+ :auto_apply_filters true}
+ {:name "Diff Test"
+ :auto_apply_filters false}))))
+
+ ;; multiple changes
+ (is (= "changed the cache ttl from \"333\" to \"1,227\", rearranged the cards, modified the series on card 1 and added some series to card 2."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:name "Diff Test"
+ :description nil
+ :cache_ttl 333
+ :cards [{:size_x 4
+ :size_y 4
+ :row 0
+ :col 0
+ :id 1
+ :card_id 1
+ :series [5 6]}
+ {:size_x 4
+ :size_y 4
+ :row 0
+ :col 0
+ :id 2
+ :card_id 2
+ :series []}]}
+ {:name "Diff Test"
+ :description nil
+ :cache_ttl 1227
+ :cards [{:size_x 4
+ :size_y 4
+ :row 0
+ :col 0
+ :id 1
+ :card_id 1
+ :series [4 5]}
+ {:size_x 4
+ :size_y 4
+ :row 2
+ :col 0
+ :id 2
+ :card_id 2
+ :series [3 4 5]}]})))))
+
+ (testing "update cards ---"
+ (is (= "added a card."
(build-sentence
(revision/diff-strings
Dashboard
- {:name "Diff Test"
- :description nil
- :cards []}
- {:name "Diff Test Changed"
- :description "foobar"
- :cards []}))))
+ {:cards [{:id 1} {:id 2}]}
+ {:cards [{:id 1} {:id 2} {:id 3}]}))))
- (is (= "added a card."
+ (is (= "removed a card."
(build-sentence
(revision/diff-strings
Dashboard
- {:name "Diff Test"
- :description nil
- :cards []}
- {:name "Diff Test"
- :description nil
- :cards [{:size_x 4
- :size_y 4
- :row 0
- :col 0
- :id 1
- :card_id 1
- :series []}]}))))
-
- (is (= "set auto apply filters to false."
+ {:cards [{:id 1} {:id 2}]}
+ {:cards [{:id 1}]}))))
+
+ (is (= "rearranged the cards."
(build-sentence
(revision/diff-strings
Dashboard
- {:name "Diff Test"
- :auto_apply_filters true}
- {:name "Diff Test"
- :auto_apply_filters false}))))
+ {:cards [{:id 1 :row 0} {:id 2 :row 1}]}
+ {:cards [{:id 1 :row 1} {:id 2 :row 2}]}))))
- (is (= "changed the cache ttl from \"333\" to \"1,227\", rearranged the cards, modified the series on card 1 and added some series to card 2."
+ (is (= "modified the cards."
(build-sentence
(revision/diff-strings
Dashboard
- {:name "Diff Test"
- :description nil
- :cache_ttl 333
- :cards [{:size_x 4
- :size_y 4
- :row 0
- :col 0
- :id 1
- :card_id 1
- :series [5 6]}
- {:size_x 4
- :size_y 4
- :row 0
- :col 0
- :id 2
- :card_id 2
- :series []}]}
- {:name "Diff Test"
- :description nil
- :cache_ttl 1227
- :cards [{:size_x 4
- :size_y 4
- :row 0
- :col 0
- :id 1
- :card_id 1
- :series [4 5]}
- {:size_x 4
- :size_y 4
- :row 2
- :col 0
- :id 2
- :card_id 2
- :series [3 4 5]}]}))))
-
- (is (= "added a card."
- (build-sentence
- (revision/diff-strings
- Dashboard
- {:cards [{:id 1} {:id 2}]}
- {:cards [{:id 1} {:id 2} {:id 3}]}))))
-
- (is (= "removed a card."
- (build-sentence
- (revision/diff-strings
- Dashboard
- {:cards [{:id 1} {:id 2}]}
- {:cards [{:id 1}]}))))
-
- (is (= "rearranged the cards."
- (build-sentence
- (revision/diff-strings
- Dashboard
- {:cards [{:id 1 :row 0} {:id 2 :row 1}]}
- {:cards [{:id 1 :row 1} {:id 2 :row 2}]}))))
-
- (is (= "modified the cards."
- (build-sentence
- (revision/diff-strings
- Dashboard
- {:cards [{:id 1} {:id 2}]}
- {:cards [{:id 1} {:id 3}]}))))
-
- (is (= "renamed it from \"Apple\" to \"Next\" and modified the cards."
- (build-sentence
- (revision/diff-strings
- Dashboard
- {:name "Apple"
- :cards [{:id 1} {:id 2}]}
- {:name "Next"
- :cards [{:id 1} {:id 3}]}))))
- (t2.with-temp/with-temp
- [Collection {coll-id :id} {:name "New collection"}]
- (is (= "moved this Dashboard to New collection."
+ {:cards [{:id 1} {:id 2}]}
+ {:cards [{:id 1} {:id 3}]})))))
+
+ (testing "update collection ---"
+ (t2.with-temp/with-temp
+ [Collection {coll-id :id} {:name "New collection"}]
+ (is (= "moved this Dashboard to New collection."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:name "Apple"}
+ {:name "Apple"
+ :collection_id coll-id})))))
+ (t2.with-temp/with-temp
+ [Collection {coll-id-1 :id} {:name "Old collection"}
+ Collection {coll-id-2 :id} {:name "New collection"}]
+ (is (= "moved this Dashboard from Old collection to New collection."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:name "Apple"
+ :collection_id coll-id-1}
+ {:name "Apple"
+ :collection_id coll-id-2}))))))
+
+ (testing "update tabs"
+ (is (= "added a tab."
(build-sentence
(revision/diff-strings
Dashboard
- {:name "Apple"}
- {:name "Apple"
- :collection_id coll-id})))))
- (t2.with-temp/with-temp
- [Collection {coll-id-1 :id} {:name "Old collection"}
- Collection {coll-id-2 :id} {:name "New collection"}]
- (is (= "moved this Dashboard from Old collection to New collection."
+ {:tabs [{:id 0 :name "First tab" :position 0}]}
+ {:tabs [{:id 0 :name "First tab" :position 0}
+ {:id 1 :name "Second tab" :position 1}]}))))
+
+ (is (= "removed 2 tabs."
(build-sentence
(revision/diff-strings
Dashboard
- {:name "Apple"
- :collection_id coll-id-1}
- {:name "Apple"
- :collection_id coll-id-2}))))))
+ {:tabs [{:id 0 :name "First tab" :position 0}
+ {:id 1 :name "Second tab" :position 1}
+ {:id 2 :name "Third tab" :position 2}]}
+ {:tabs [{:id 0 :name "First tab" :position 0}]}))))
+ (is (= "rearranged the tabs."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:tabs [{:id 0 :name "First tab" :position 0}
+ {:id 1 :name "Second tab" :position 1}]}
+ {:tabs [{:id 1 :name "Second tab" :position 0}
+ {:id 0 :name "First tab" :position 1}]}))))
+
+ (is (= "modified the tabs."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:tabs [{:id 0 :name "Tab A" :position 0}
+ {:id 1 :name "Tab B" :position 1}]}
+ {:tabs [{:id 1 :name "Tab B new name and position" :position 0}
+ {:id 0 :name "Tab A new name and position" :position 1}]}))))))
(deftest revert-dashboard!-test
(tt/with-temp* [Dashboard [{dashboard-id :id, :as dashboard} {:name "Test Dashboard"}]
@@ -210,7 +256,8 @@
:auto_apply_filters true
:collection_id nil
:cache_ttl nil
- :cards []}
+ :cards []
+ :tabs []}
serialized-dashboard (revision/serialize-instance Dashboard (:id dashboard) dashboard)]
(testing "original state"
(is (= {:name "Test Dashboard"
@@ -218,13 +265,15 @@
:cache_ttl nil
:auto_apply_filters true
:collection_id nil
- :cards [{:size_x 4
- :size_y 4
- :row 0
- :col 0
- :id true
- :card_id true
- :series true}]}
+ :cards [{:size_x 4
+ :size_y 4
+ :row 0
+ :col 0
+ :id true
+ :card_id true
+ :series true
+ :dashboard_tab_id nil}]
+ :tabs []}
(update serialized-dashboard :cards check-ids))))
(testing "delete the dashcard and modify the dash attributes"
(dashboard-card/delete-dashboard-cards! [(:id dashboard-card)])
@@ -243,13 +292,15 @@
:cache_ttl nil
:auto_apply_filters true
:collection_id nil
- :cards [{:size_x 4
- :size_y 4
- :row 0
- :col 0
- :id false
- :card_id true
- :series true}]}
+ :cards [{:size_x 4
+ :size_y 4
+ :row 0
+ :col 0
+ :id false
+ :card_id true
+ :series true
+ :dashboard_tab_id nil}]
+ :tabs []}
(update (revision/serialize-instance Dashboard dashboard-id (t2/select-one Dashboard :id dashboard-id))
:cards check-ids))))
(testing "revert back to the empty state"
@@ -257,6 +308,209 @@
(is (= empty-dashboard
(revision/serialize-instance Dashboard dashboard-id (t2/select-one Dashboard :id dashboard-id))))))))
+(defn- create-dashboard-revision!
+ "Fetch the latest version of a Dashboard and save a revision entry for it. Returns the fetched Dashboard."
+ [dash-id is-creation?]
+ (revision/push-revision!
+ :object (t2/select-one Dashboard :id dash-id)
+ :entity Dashboard
+ :id dash-id
+ :user-id (mt/user->id :crowberto)
+ :is-creation? is-creation?))
+
+(defn- revert-to-previous-revision
+ "Revert to a previous revision for a model.
+ `n` is the number of revisions to revert back to.
+ `n=1` means do nothing (i.e. revert to the current revision).
+
+ To revert 1 action, you should have n=2."
+ [model model-id n]
+ (assert (> n 1), "n = 1 means revert to the current revision, which is a no-op.")
+ (let [ids (t2/select-pks-vec Revision :model (name model) :model_id model-id {:order-by [[:timestamp :desc]]
+ :limit n})]
+ (assert (= n (count ids)), "There are less revisions than required to revert")
+ (revision/revert! :entity model :id model-id :user-id (mt/user->id :crowberto) :revision-id (last ids))))
+
+(deftest revert-dashboard-with-tabs-basic-test
+ (testing "revert adding tabs"
+ (t2.with-temp/with-temp [:model/Dashboard {dashboard-id :id} {:name "A dashboard"}]
+ ;; 0. create a dashboard
+ (create-dashboard-revision! dashboard-id true)
+
+
+ ;; 1. add 2 tabs
+ (let [[tab-1-id tab-2-id] (t2/insert-returning-pks! :model/DashboardTab [{:name "Tab 1"
+ :position 0
+ :dashboard_id dashboard-id}
+ {:name "Tab 2"
+ :position 1
+ :dashboard_id dashboard-id}])
+ _ (create-dashboard-revision! dashboard-id false)
+
+ ;; 2. add another tab for revision
+ tab-3-id (first (t2/insert-returning-pks! :model/DashboardTab [{:name "Tab 3"
+ :position 2
+ :dashboard_id dashboard-id}]))]
+ (create-dashboard-revision! dashboard-id false)
+
+ ;; check to make sure we have everything setup before testing
+ (is (=? [{:id tab-1-id :name "Tab 1" :position 0}
+ {:id tab-2-id :name "Tab 2" :position 1}
+ {:id tab-3-id :name "Tab 3" :position 2}]
+ (t2/select :model/DashboardTab :dashboard_id dashboard-id {:order-by [[:position :asc]]})))
+ ;; revert
+ (revert-to-previous-revision Dashboard dashboard-id 2)
+ (is (=? [{:id tab-1-id :name "Tab 1" :position 0}
+ {:id tab-2-id :name "Tab 2" :position 1}]
+ (t2/select :model/DashboardTab :dashboard_id dashboard-id {:order-by [[:position :asc]]}))))))
+
+ (testing "revert renaming tabs"
+ (t2.with-temp/with-temp [:model/Dashboard {dashboard-id :id} {:name "A dashboard"}]
+ ;; 0. create a dashboard
+ (create-dashboard-revision! dashboard-id true)
+
+
+ ;; 1. add 2 tabs
+ (let [[tab-1-id tab-2-id] (t2/insert-returning-pks! :model/DashboardTab [{:name "Tab 1"
+ :position 0
+ :dashboard_id dashboard-id}
+ {:name "Tab 2"
+ :position 1
+ :dashboard_id dashboard-id}])]
+ (create-dashboard-revision! dashboard-id false)
+
+ ;; 2. update a tab name
+ (t2/update! :model/DashboardTab tab-1-id {:name "Tab 1 with new name"})
+ (create-dashboard-revision! dashboard-id false)
+
+ ;; check to make sure we have everything setup before testing
+ (is (=? [{:id tab-1-id :name "Tab 1 with new name" :position 0}
+ {:id tab-2-id :name "Tab 2" :position 1}]
+ (t2/select :model/DashboardTab :dashboard_id dashboard-id {:order-by [[:position :asc]]})))
+ ;; revert
+ (revert-to-previous-revision Dashboard dashboard-id 2)
+ (is (=? [{:id tab-1-id :name "Tab 1" :position 0}
+ {:id tab-2-id :name "Tab 2" :position 1}]
+ (t2/select :model/DashboardTab :dashboard_id dashboard-id {:order-by [[:position :asc]]}))))))
+
+ (testing "revert deleting tabs"
+ (t2.with-temp/with-temp [:model/Dashboard {dashboard-id :id} {:name "A dashboard"}]
+ ;; 0. create a dashboard
+ (create-dashboard-revision! dashboard-id true)
+
+
+ ;; 1. add 2 tabs
+ (let [[tab-1-id tab-2-id] (t2/insert-returning-pks! :model/DashboardTab [{:name "Tab 1"
+ :position 0
+ :dashboard_id dashboard-id}
+ {:name "Tab 2"
+ :position 1
+ :dashboard_id dashboard-id}])]
+ (create-dashboard-revision! dashboard-id false)
+
+ ;; 2. delete the 1st tab and re-position the second tab
+ (t2/delete! :model/DashboardTab tab-1-id)
+ (t2/update! :model/DashboardTab tab-2-id {:position 0})
+ (create-dashboard-revision! dashboard-id false)
+
+ ;; check to make sure we have everything setup before testing
+ (is (=? [{:id tab-2-id :name "Tab 2" :position 0}]
+ (t2/select :model/DashboardTab :dashboard_id dashboard-id {:order-by [[:position :asc]]})))
+ ;; revert
+ (revert-to-previous-revision Dashboard dashboard-id 2)
+ (is (=? [{:id #hawk/schema (s/pred pos-int?) :name "Tab 1" :position 0}
+ {:id tab-2-id :name "Tab 2" :position 1}]
+ (t2/select :model/DashboardTab :dashboard_id dashboard-id {:order-by [[:position :asc]]})))))))
+
+(deftest revert-dashboard-with-tabs-advanced-test
+ (t2.with-temp/with-temp [:model/Dashboard {dashboard-id :id} {:name "A dashboard"}]
+ ;; 0. create a dashboard
+ (create-dashboard-revision! dashboard-id true)
+
+ ;; 1. add 2 tabs, each with 2 cards
+ (let [[tab-1-id tab-2-id] (t2/insert-returning-pks! :model/DashboardTab [{:name "Tab 1"
+ :position 0
+ :dashboard_id dashboard-id}
+ {:name "Tab 2"
+ :position 1
+ :dashboard_id dashboard-id}])
+ [card-1-tab-1 card-2-tab-1] (t2/insert-returning-pks! :model/DashboardCard
+ [{:dashboard_id dashboard-id
+ :dashboard_tab_id tab-1-id
+ :row 0
+ :col 0
+ :size_x 4
+ :size_y 4}
+ {:dashboard_id dashboard-id
+ :dashboard_tab_id tab-1-id
+ :row 4
+ :col 4
+ :size_x 4
+ :size_y 4}
+ {:dashboard_id dashboard-id
+ :dashboard_tab_id tab-2-id
+ :row 0
+ :col 0
+ :size_x 4
+ :size_y 4}
+ {:dashboard_id dashboard-id
+ :dashboard_tab_id tab-2-id
+ :row 4
+ :col 4
+ :size_x 4
+ :size_y 4}])]
+ (create-dashboard-revision! dashboard-id false)
+
+ ;; 2.a: tab 1: delete the 2nd card, add 2 cards and update position of one card,
+ (t2/insert! :model/DashboardCard [{:dashboard_id dashboard-id
+ :dashboard_tab_id tab-1-id
+ :row 0
+ :col 0
+ :size_x 4
+ :size_y 4}
+ {:dashboard_id dashboard-id
+ :dashboard_tab_id tab-1-id
+ :row 0
+ :col 0
+ :size_x 4
+ :size_y 4}])
+ (t2/delete! :model/DashboardCard card-2-tab-1)
+ (t2/update! :model/DashboardCard card-1-tab-1 {:row 10 :col 10})
+
+ ;; 2.b: delete tab 2
+ (t2/delete! :model/DashboardTab tab-2-id)
+
+ ;; 2.c: create a new tab with 1 card
+ (let [new-tab-id (t2/insert-returning-pks! :model/DashboardTab {:name "Tab 3"
+ :position 1
+ :dashboard_id dashboard-id})]
+ (t2/insert! :model/DashboardCard {:dashboard_id dashboard-id
+ :dashboard_tab_id new-tab-id
+ :row 0
+ :col 0
+ :size_x 4
+ :size_y 4}))
+ (create-dashboard-revision! dashboard-id false)
+
+ ;; revert
+ (revert-to-previous-revision Dashboard dashboard-id 2)
+ (testing "tab 1 should have 2 cards"
+ (is (= 2 (t2/count :model/DashboardCard :dashboard_tab_id tab-1-id)))
+ (testing "and position of first card is (0,0)"
+ (is (=? {:row 0
+ :col 0}
+ (t2/select-one :model/DashboardCard card-1-tab-1)))))
+
+ (testing "tab \"Tab 2\" is restored"
+ (let [new-tab-2 (t2/select-one :model/DashboardTab :dashboard_id dashboard-id :name "Tab 2")]
+ (is (= 1 (:position new-tab-2)))
+
+ (testing "with its cards"
+ (is (= 2 (t2/count :model/DashboardCard :dashboard_id dashboard-id :dashboard_tab_id (:id new-tab-2)))))))
+
+ (testing "there are no \"Tab 3\""
+ (is (false? (t2/exists? :model/DashboardTab :dashboard_id dashboard-id :name "Tab 3")))))))
+
(deftest public-sharing-test
(testing "test that a Dashboard's :public_uuid comes back if public sharing is enabled..."
(tu/with-temporary-setting-values [enable-public-sharing true]
diff --git a/test/metabase/util_test.cljc b/test/metabase/util_test.cljc
index 90e32c81898..fce55178ffe 100644
--- a/test/metabase/util_test.cljc
+++ b/test/metabase/util_test.cljc
@@ -411,3 +411,12 @@
(testing "multiple defaults for the same key"
(is (= {:x nil, :y 1, :z 2}
(u/assoc-default {:x nil} :x 0, :y nil, :y 1, :z 2, :x 3, :z 4)))))
+
+(deftest classify-changes-test
+ (testing "classify correctly"
+ (is (= {:to-update [{:id 2 :name "c3"} {:id 4 :name "c4"}]
+ :to-delete [{:id 1 :name "c1"} {:id 3 :name "c3"}]
+ :to-create [{:id -1 :name "-c1"}]}
+ (u/classify-changes
+ [{:id 1 :name "c1"} {:id 2 :name "c2"} {:id 3 :name "c3"} {:id 4 :name "c4"}]
+ [{:id -1 :name "-c1"} {:id 2 :name "c3"} {:id 4 :name "c4"}])))))
--
GitLab