tests for the revision API endpoints and dashboards

src/metabase/models/dashboard.clj

@@ -71,27 +71,26 @@
   serialized-dashboard)
 
 (defn- describe-diff [_ username dashboard₁ dashboard₂]
-  (let [[_ changes] (diff dashboard₁ dashboard₂)]
-    (println "CHANGES -> " changes)
-    (str (->> [(when (:name changes)
-                 (format "renamed it from \"%s\" to \"%s\"" (:name dashboard₁) (:name dashboard₂)))
-               (when (:description changes)
-                 (format "changed the description from \"%s\" to \"%s\"" (:description dashboard₁) (:description dashboard₂)))
-               (when (:public_perms changes)
-                 (if (zero? (:public_perms dashboard₂))
-                   "made it private"
-                   "made it public")) ; TODO - are both 1 and 2 "public" now ?
-               (when (:cards changes)
-                 (let [num-cards₁ (count (:cards dashboard₁))
-                       num-cards₂ (count (:cards dashboard₂))]
-                   (cond
-                     (> num-cards₁ num-cards₂) "removed a card"
-                     (< num-cards₁ num-cards₂) "added a card"
-                     :else                     "repositioned the cards")))]
-              (filter identity)
-              build-sentence
-              (apply str username " ")
-              (#(s/replace-first % "it " "this dashboard "))) \.)))
+  (let [[removals changes] (diff dashboard₁ dashboard₂)]
+    (->> [(when (:name changes)
+            (format "renamed it from \"%s\" to \"%s\"" (:name dashboard₁) (:name dashboard₂)))
+          (when (:description changes)
+            (format "changed the description from \"%s\" to \"%s\"" (:description dashboard₁) (:description dashboard₂)))
+          (when (:public_perms changes)
+            (if (zero? (:public_perms dashboard₂))
+              "made it private"
+              "made it public")) ; TODO - are both 1 and 2 "public" now ?
+          (when (or (:cards changes) (:cards removals))
+            (let [num-cards₁ (count (:cards dashboard₁))
+                  num-cards₂ (count (:cards dashboard₂))]
+              (cond
+                (< num-cards₁ num-cards₂) "added a card"
+                (> num-cards₁ num-cards₂) "removed a card"
+                :else                     "rearranged the cards")))]
+         (filter identity)
+         build-sentence
+         (apply str username " ")
+         (#(s/replace-first % "it " "this dashboard ")))))
 
 (extend DashboardEntity
   IRevisioned

test/metabase/api/revision_test.clj

+(ns metabase.api.revision-test
+  (:require [expectations :refer :all]
+            [korma.core :as k]
+            [medley.core :as m]
+            [metabase.api.revision :refer :all]
+            [metabase.db :as db]
+            (metabase.models [dashboard :refer [Dashboard]]
+                             [dashboard-card :refer [DashboardCard]]
+                             [revision-test :refer [with-fake-card]])
+            [metabase.test.data.users :refer :all]))
+
+
+(defn x []
+  (with-fake-card [{card-id₁ :id}]
+    (with-fake-card [{card-id₂ :id}]
+      ((user->client :rasta) :get 200 "revision", :entity :card, :id card-id₁))))
+
+(defn y []
+  (with-fake-card [{card-id :id}]
+    ((user->client :rasta) :get 200 "revision", :entity :card, :id card-id)))
+
+(defn- fake-dashboard [& {:as kwargs}]
+  (m/mapply db/ins Dashboard (merge {:name         (str (java.util.UUID/randomUUID))
+                                     :public_perms 0
+                                     :creator_id   (user->id :rasta)}
+                                    kwargs)))
+
+(defmacro with-fake-dashboard [[binding & {:as kwargs}] & body]
+  `(let [dash# (fake-dashboard ~@kwargs)
+         ~binding dash#]
+     (try ~@body
+          (finally
+            (db/cascade-delete Dashboard :id (:id dash#))))))
+
+(def ^:private rasta-revision-info
+  (delay {:id (user->id :rasta) :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"}))
+
+;;; # TESTS FOR GET /api/revision
+(expect [{:description "First revision.", :user {}}]
+  (with-fake-card [{card-id :id}]
+    ((user->client :rasta) :get 200 "revision", :entity :card, :id card-id)))
+
+(expect [{:is_reversion false, :user @rasta-revision-info, :description "First revision."}]
+  (with-fake-dashboard [{dash-id :id}]
+    (with-fake-card [{card-id :id}]
+      ((user->client :rasta) :post 200 (format "dash/%d/cards" dash-id), {:cardId card-id})
+      (->> ((user->client :rasta) :get 200 "revision", :entity :dashboard, :id dash-id)
+           (mapv #(dissoc % :timestamp :id))))))
+
+(expect [{:is_reversion false, :user @rasta-revision-info, :description "Rasta Toucan added a card."}
+         {:is_reversion false, :user @rasta-revision-info, :description "First revision."}]
+  (with-fake-dashboard [{dash-id :id}]
+    (with-fake-card [{card-id₁ :id}]
+      (with-fake-card [{card-id₂ :id}]
+        ((user->client :rasta) :post 200 (format "dash/%d/cards" dash-id), {:cardId card-id₁})
+        ((user->client :rasta) :post 200 (format "dash/%d/cards" dash-id), {:cardId card-id₂})
+        (->> ((user->client :rasta) :get 200 "revision", :entity :dashboard, :id dash-id)
+             (mapv #(dissoc % :timestamp :id)))))))
+
+(expect [{:is_reversion false, :user @rasta-revision-info, :description "Rasta Toucan removed a card."}
+         {:is_reversion false, :user @rasta-revision-info, :description "Rasta Toucan added a card."}
+         {:is_reversion false, :user @rasta-revision-info, :description "First revision."}]
+  (with-fake-dashboard [{dash-id :id}]
+    (with-fake-card [{card-id₁ :id}]
+      (with-fake-card [{card-id₂ :id}]
+        ((user->client :rasta) :post 200   (format "dash/%d/cards" dash-id), {:cardId card-id₁})
+        ((user->client :rasta) :post 200   (format "dash/%d/cards" dash-id), {:cardId card-id₂})
+        ((user->client :rasta) :delete 204 (format "dash/%d/cards" dash-id), :dashcardId (db/sel :one :id DashboardCard (k/order :id :desc)))
+        (->> ((user->client :rasta) :get 200 "revision", :entity :dashboard, :id dash-id)
+             (mapv #(dissoc % :timestamp :id)))))))
+
+;;; # TESTS FOR POST /api/revision/revert
+(expect [2
+         [{:is_reversion true,  :user @rasta-revision-info, :description "Rasta Toucan reverted to an earlier revision and added a card."}
+          {:is_reversion false, :user @rasta-revision-info, :description "Rasta Toucan removed a card."}
+          {:is_reversion false, :user @rasta-revision-info, :description "Rasta Toucan added a card."}
+          {:is_reversion false, :user @rasta-revision-info, :description "First revision."}]]
+  (with-fake-dashboard [{dash-id :id}]
+    (with-fake-card [{card-id₁ :id}]
+      (with-fake-card [{card-id₂ :id}]
+        ((user->client :rasta) :post 200   (format "dash/%d/cards" dash-id), {:cardId card-id₁})
+        ((user->client :rasta) :post 200   (format "dash/%d/cards" dash-id), {:cardId card-id₂})
+        ((user->client :rasta) :delete 204 (format "dash/%d/cards" dash-id), :dashcardId (db/sel :one :id DashboardCard (k/order :id :desc)))
+        (let [[_ {previous-revision-id :id}] (metabase.models.revision/revisions Dashboard dash-id)]
+          ;; Revert to the previous revision
+          ((user->client :rasta) :post 200 "revision/revert", {:entity :dashboard, :id dash-id, :revision_id previous-revision-id})
+          [ ;; [1] There should be 2 cards again
+           (count @(:ordered_cards (Dashboard dash-id)))
+           ;; [2] A new revision recording the reversion should have been pushed
+           (->> ((user->client :rasta) :get 200 "revision", :entity :dashboard, :id dash-id)
+                (mapv #(dissoc % :timestamp :id)))])))))

test/metabase/models/revision_test.clj

@@ -9,7 +9,7 @@
             [metabase.test.data.users :refer :all]
             [metabase.util :as u]))
 
-(defn- fake-card [& {:as kwargs}]
+(defn fake-card [& {:as kwargs}]
   (m/mapply db/ins Card (merge {:name (str (java.util.UUID/randomUUID))
                                 :display                :table
                                 :public_perms           0