Merge pull request #973 from metabase/revisions_via_events

Revisions via new events system

project.clj

@@ -86,7 +86,8 @@
              :expectations {:global-vars {*warn-on-reflection* false}
                             :injections [(require 'metabase.test-setup)]
                             :resource-paths ["test_resources"]
-                            :env {:mb-test-setting-1 "ABCDEFG"}
+                            :env {:mb-test-setting-1 "ABCDEFG"
+                                  :mb-run-mode "test"}
                             :jvm-opts ["-Dmb.db.in.memory=true"
                                        "-Dmb.jetty.join=false"
                                        "-Dmb.jetty.port=3010"

resources/migrations/015_add_revision_is_creation_field.yaml

+databaseChangeLog:
+  - changeSet:
+      id: 15
+      author: agilliland
+      changes:
+        - addColumn:
+            tableName: revision
+            columns:
+              - column:
+                  name: is_creation
+                  type: boolean
+                  defaultValueBoolean: false
+                  constraints:
+                    nullable: false

resources/migrations/liquibase.json

@@ -12,6 +12,7 @@
       {"include": {"file": "migrations/011_cleanup_dashboard_perms.yaml"}},
       {"include": {"file": "migrations/012_add_card_query_fields.yaml"}},
       {"include": {"file": "migrations/013_add_activity_table.yaml"}},
-      {"include": {"file": "migrations/014_add_view_log_table.yaml"}}
+      {"include": {"file": "migrations/014_add_view_log_table.yaml"}},
+      {"include": {"file": "migrations/015_add_revision_is_creation_field.yaml"}}
   ]
 }

src/metabase/api/card.clj

@@ -98,8 +98,7 @@
                     :name name
                     :public_perms public_perms
                     :visualization_settings visualization_settings)
-  (events/publish-event :card-update (assoc (sel :one Card :id id) :actor_id *current-user-id*))
-  (push-revision :entity Card, :object (Card id)))
+  (events/publish-event :card-update (assoc (sel :one Card :id id) :actor_id *current-user-id*)))
 
 (defendpoint DELETE "/:id"
   "Delete a `Card`."

src/metabase/api/dashboard.clj

@@ -57,8 +57,7 @@
                                :description description
                                :name name
                                :public_perms public_perms))
-  (events/publish-event :dashboard-update {:id id :actor_id *current-user-id*})
-  (push-revision :entity Dashboard, :object (Dashboard id)))
+  (events/publish-event :dashboard-update {:id id :actor_id *current-user-id*}))
 
 (defendpoint DELETE "/:id"
   "Delete a `Dashboard`."
@@ -78,7 +77,6 @@
   (check-400 (exists? Card :id cardId))
   (let [result (ins DashboardCard :card_id cardId :dashboard_id id)]
     (events/publish-event :dashboard-add-cards {:id id :actor_id *current-user-id* :dashcards [result]})
-    (push-revision :entity Dashboard, :object (Dashboard id))
     result))
 
 (defendpoint DELETE "/:id/cards"
@@ -90,7 +88,6 @@
   (let [dashcard (sel :one DashboardCard :id dashcardId)
         result (del DashboardCard :id dashcardId :dashboard_id id)]
     (events/publish-event :dashboard-remove-cards {:id id :actor_id *current-user-id* :dashcards [dashcard]})
-    (push-revision :entity Dashboard, :object (Dashboard id))
     result))
 
 (defendpoint POST "/:id/reposition"
@@ -108,8 +105,7 @@
     (let [dashcard (sel :one [DashboardCard :id] :id dashcard-id :dashboard_id id)]
       (when dashcard
         (upd DashboardCard dashcard-id :sizeX sizeX :sizeY sizeY :row row :col col))))
-  (events/publish-event :dashboard-reposition-cards {:id id :actor_id *current-user-id* :cards cards})
-  (push-revision :entity Dashboard, :object (Dashboard id))
+  (events/publish-event :dashboard-reposition-cards {:id id :actor_id *current-user-id* :dashcards cards})
   {:status :ok})
 
 (define-routes)

src/metabase/config.clj

@@ -6,6 +6,7 @@
 (def ^:const app-defaults
   "Global application defaults"
   {;; Database Configuration  (general options?  dburl?)
+   :mb-run-mode "prod"
    :mb-db-type "h2"
    ;:mb-db-dbname "postgres"
    ;:mb-db-host "localhost"
@@ -62,3 +63,6 @@
           (m/filter-keys (fn [k] (re-matches prefix-regex (str k))) app-defaults)
           (m/filter-keys (fn [k] (re-matches prefix-regex (str k))) environ/env))
       (m/map-keys (fn [k] (let [kstr (str k)] (keyword (subs kstr (+ 1 (count prefix))))))))))
+
+(defn ^Boolean is-prod? [] (= :prod (config-kw :mb-run-mode)))
+(defn ^Boolean is-test? [] (= :test (config-kw :mb-run-mode)))

src/metabase/events.clj

@@ -91,3 +91,26 @@
       (catch Exception e
         (log/error "Unexpected error listening on events" e)))
     (recur)))
+
+
+;;; ## ---------------------------------------- HELPER FUNCTIONS ----------------------------------------
+
+
+(defn topic->model
+  "Determine a valid `model` identifier for the given `topic`."
+  [topic]
+  ;; just take the first part of the topic name after splitting on dashes.
+  (first (clojure.string/split (name topic) #"-")))
+
+(defn object->model-id
+  "Determine the appropriate `model_id` (if possible) for a given `object`."
+  [topic object]
+  (if (contains? (set (keys object)) :id)
+    (:id object)
+    (let [model (topic->model topic)]
+      (get object (keyword (format "%s_id" model))))))
+
+(defn object->user-id
+  "Determine the appropriate `user_id` (if possible) for a given `object`."
+  [object]
+  (or (:actor_id object) (:user_id object) (:creator_id object)))

src/metabase/events/activity_feed.clj

@@ -2,6 +2,7 @@
   (:require [clojure.core.async :as async]
             [clojure.tools.logging :as log]
             [metabase.db :as db]
+            [metabase.config :as config]
             [metabase.events :as events]
             (metabase.models [activity :refer [Activity]]
                              [dashboard :refer [Dashboard]]
@@ -22,11 +23,6 @@
     :install
     :user-login})
 
-(defn valid-activity-topic?
-  "Predicate function that checks if a topic is in `activity-feed-topics`. true if included, false otherwise."
-  [topic]
-  (contains? activity-feed-topics (keyword topic)))
-
 (def ^:private activity-feed-channel
   "Channel for receiving event notifications we want to subscribe to for the activity feed."
   (async/chan))
@@ -35,25 +31,6 @@
 ;;; ## ---------------------------------------- EVENT PROCESSING ----------------------------------------
 
 
-(defn- topic->model
-  "Determine a valid `model` identifier for the given `topic`."
-  [topic]
-  ;; just take the first part of the topic name after splitting on dashes.
-  (first (clojure.string/split (name topic) #"-")))
-
-(defn- object->model-id
-  "Determine the appropriate `model_id` (if possible) for a given `object`."
-  [topic object]
-  (if (contains? (set (keys object)) :id)
-    (:id object)
-    (let [model (topic->model topic)]
-      (get object (keyword (format "%s_id" model))))))
-
-(defn- object->user-id
-  "Determine the appropriate `user_id` (if possible) for a given `object`."
-  [object]
-  (or (:actor_id object) (:user_id object) (:creator_id object)))
-
 (defn- record-activity
   "Simple base function for recording activity using defaults.
   Allows caller to specify a custom serialization function to apply to `object` to generate the activity `:details`."
@@ -62,9 +39,9 @@
                                           (database-table-fn object))]
      (db/ins Activity
           :topic topic
-          :user_id (object->user-id object)
-          :model (topic->model topic)
-          :model_id (object->model-id topic object)
+          :user_id (events/object->user-id object)
+          :model (events/topic->model topic)
+          :model_id (events/object->model-id topic object)
           :database_id database-id
           :table_id table-id
           :custom_id (:custom_id object)
@@ -88,7 +65,7 @@
         add-remove-card-details (fn [{:keys [dashcards] :as obj}]
                                   ;; we expect that the object has just a dashboard :id at the top level
                                   ;; plus a `:dashcards` attribute which is a vector of the cards added/removed
-                                  (-> (db/sel :one Dashboard :id (object->model-id topic obj))
+                                  (-> (db/sel :one Dashboard :id (events/object->model-id topic obj))
                                       (select-keys [:description :name :public_perms])
                                       (assoc :dashcards (for [{:keys [id card_id card]} dashcards]
                                                           (-> @card
@@ -105,7 +82,7 @@
   (let [database-details-fn (fn [obj] (-> obj
                                           (assoc :status "started")
                                           (dissoc :database_id :custom_id)))
-        database-table-fn (fn [obj] {:database-id (object->model-id topic obj)})]
+        database-table-fn (fn [obj] {:database-id (events/object->model-id topic obj)})]
     (case topic
       :database-sync-begin (record-activity :database-sync object database-details-fn database-table-fn)
       :database-sync-end   (let [{activity-id :id} (db/sel :one Activity :custom_id (:custom_id object))]
@@ -121,7 +98,7 @@
     (db/ins Activity
       :topic    :user-joined
       :user_id  (:user_id object)
-      :model    (topic->model topic)
+      :model    (events/topic->model topic)
       :model_id (:user_id object))))
 
 (defn process-activity-event
@@ -130,7 +107,7 @@
   ;; try/catch here to prevent individual topic processing exceptions from bubbling up.  better to handle them here.
   (try
     (when-let [{topic :topic object :item} activity-event]
-      (case (topic->model topic)
+      (case (events/topic->model topic)
         "card"      (process-card-activity topic object)
         "dashboard" (process-dashboard-activity topic object)
         "database"  (process-database-activity topic object)
@@ -146,4 +123,6 @@
 
 
 ;; this is what actually kicks off our listener for events
-(events/start-event-listener activity-feed-topics activity-feed-channel process-activity-event)
+(when (config/is-prod?)
+  (log/info "Starting activity-feed events listener")
+  (events/start-event-listener activity-feed-topics activity-feed-channel process-activity-event))

src/metabase/events/revision.clj

+(ns metabase.events.revision
+  (:require [clojure.core.async :as async]
+            [clojure.tools.logging :as log]
+            [metabase.config :as config]
+            [metabase.events :as events]
+            (metabase.models [card :refer [Card]]
+                             [dashboard :refer [Dashboard]]
+                             [revision :refer [push-revision]])))
+
+
+(def revisions-topics
+  "The `Set` of event topics which are subscribed to for use in revision tracking."
+  #{:card-create
+    :card-update
+    :dashboard-create
+    :dashboard-update
+    :dashboard-add-cards
+    :dashboard-remove-cards
+    :dashboard-reposition-cards})
+
+(def ^:private revisions-channel
+  "Channel for receiving event notifications we want to subscribe to for revision events."
+  (async/chan))
+
+
+;;; ## ---------------------------------------- EVENT PROCESSING ----------------------------------------
+
+
+(defn process-revision-event
+  "Handle processing for a single event notification received on the revisions-channel"
+  [revision-event]
+  ;; try/catch here to prevent individual topic processing exceptions from bubbling up.  better to handle them here.
+  (try
+    (when-let [{topic :topic object :item} revision-event]
+      (let [model   (events/topic->model topic)
+            id      (events/object->model-id topic object)
+            user-id (events/object->user-id object)]
+        (case model
+          "card"      (push-revision :entity       Card,
+                                     :id           id,
+                                     :object       (Card id),
+                                     :user-id      user-id,
+                                     :is-creation? (= :card-create topic))
+          "dashboard" (push-revision :entity       Dashboard,
+                                     :id           id,
+                                     :object       (Dashboard id),
+                                     :user-id      user-id,
+                                     :is-creation? (= :dashboard-create topic)))))
+    (catch Throwable e
+      (log/warn (format "Failed to process revision event. %s" (:topic revision-event)) e))))
+
+
+
+;;; ## ---------------------------------------- LIFECYLE ----------------------------------------
+
+
+;; this is what actually kicks off our listener for events
+(when (config/is-prod?)
+  (log/info "Starting revision events listener")
+  (events/start-event-listener revisions-topics revisions-channel process-revision-event))

src/metabase/events/view_log.clj

 (ns metabase.events.view-log
   (:require [clojure.core.async :as async]
             [clojure.tools.logging :as log]
+            [metabase.config :as config]
             [metabase.db :as db]
             [metabase.events :as events]
             [metabase.models.view-log :refer [ViewLog]]))
@@ -29,26 +30,6 @@
     :model    model
     :model_id model-id))
 
-(defn- topic->model
-  "Determine a valid `model` identifier for the given `topic`."
-  [topic]
-  {:pre [(contains? view-counts-topics topic)]}
-  ;; just take the first part of the topic name after splitting on dashes.
-  (first (clojure.string/split (name topic) #"-")))
-
-(defn- object->model-id
-  "Determine the appropriate `model_id` (if possible) for a given `object`."
-  [topic object]
-  (if (contains? (set (keys object)) :id)
-    (:id object)
-    (let [model (topic->model topic)]
-      (get object (keyword (format "%s_id" model))))))
-
-(defn- object->user-id
-  "Determine the appropriate `user_id` (if possible) for a given `object`."
-  [object]
-  (or (:actor_id object) (:user_id object) (:creator_id object)))
-
 (defn process-view-count-event
   "Handle processing for a single event notification received on the view-counts-channel"
   [event]
@@ -56,9 +37,9 @@
   (try
     (when-let [{topic :topic object :item} event]
       (record-view
-        (topic->model topic)
-        (object->model-id topic object)
-        (object->user-id object)))
+        (events/topic->model topic)
+        (events/object->model-id topic object)
+        (events/object->user-id object)))
     (catch Throwable e
       (log/warn (format "Failed to process activity event. %s" (:topic event)) e))))
 
@@ -67,4 +48,6 @@
 
 
 ;; this is what actually kicks off our listener for events
-(events/start-event-listener view-counts-topics view-counts-channel process-view-count-event)
+(when (config/is-prod?)
+  (log/info "Starting view-log events listener")
+  (events/start-event-listener view-counts-topics view-counts-channel process-view-count-event))

src/metabase/models/activity.clj

 (ns metabase.models.activity
   (:require [korma.core :refer :all, :exclude [defentity update]]
-            [metabase.api.common :refer [*current-user-id*]]
-            [metabase.db :refer :all]
+            [metabase.db :refer [exists?]]
             [metabase.events :as events]
             (metabase.models [card :refer [Card]]
                              [dashboard :refer [Dashboard]]

src/metabase/models/card.clj

 (ns metabase.models.card
   (:require [korma.core :refer :all, :exclude [defentity update]]
+            [medley.core :as m]
             [metabase.db :refer :all]
             (metabase.models [interface :refer :all]
+                             [revision :as revision]
                              [user :refer [User]])))
 
 (def ^:const display-types
@@ -60,3 +62,18 @@
     (cascade-delete 'CardFavorite :card_id id)))
 
 (extend-ICanReadWrite CardEntity :read :public-perms, :write :public-perms)
+
+
+;;; ## ---------------------------------------- REVISIONS ----------------------------------------
+
+
+(defn- serialize-instance [_ _ instance]
+  (->> (dissoc instance :created_at :updated_at)
+       (into {})                                 ; if it's a record type like CardInstance we need to convert it to a regular map or filter-vals won't work
+       (m/filter-vals (complement delay?))))
+
+(extend CardEntity
+  revision/IRevisioned
+  {:serialize-instance serialize-instance
+   :revert-to-revision revision/default-revert-to-revision
+   :describe-diff      revision/default-describe-diff})

src/metabase/models/dashboard.clj

@@ -36,6 +36,10 @@
 
 (extend-ICanReadWrite DashboardEntity :read :public-perms, :write :public-perms)
 
+
+;;; ## ---------------------------------------- REVISIONS ----------------------------------------
+
+
 (defn- serialize-instance [_ id {:keys [ordered_cards], :as dashboard}]
   (-> dashboard
       (select-keys [:description :name :public_perms])
@@ -44,7 +48,7 @@
 
 (defn- revert-to-revision [_ dashboard-id serialized-dashboard]
   ;; Update the dashboard description / name / permissions
-
+  (m/mapply upd 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)

src/metabase/models/revision.clj

@@ -24,18 +24,20 @@
   (describe-diff [this object1 object2]
     "Return a string describing the difference between OBJECT1 and OBJECT2."))
 
-;;; ## Default Impl
 
-(extend-protocol IRevisioned
-  Object
-  (serialize-instance [_ _ instance]
-    (->> (dissoc instance :created_at :updated_at)
-         (into {})                                 ; if it's a record type like CardInstance we need to convert it to a regular map or filter-vals won't work
-         (m/filter-vals (complement delay?))))
-  (revert-to-revision [entity id serialized-instance]
-    (m/mapply upd entity id serialized-instance))
-  (describe-diff [entity o1 o2]
-    (diff-str (:name entity) o1 o2)))
+;;; # Reusable Base Implementations for IRevisioned functions
+
+;; NOTE that we do not provide a base implementation for `serialize-instance`, that should be done per entity.
+
+(defn default-revert-to-revision
+  "Default implementation of `revert-to-revision` which simply does an update using the values from `serialized-instance`."
+  [entity id serialized-instance]
+  (m/mapply upd entity id serialized-instance))
+
+(defn default-describe-diff
+  "Default implementation of `describe-diff` which calls `diff-str` on the 2 objects."
+  [entity o1 o2]
+  (diff-str (:name entity) o1 o2))
 
 
 ;;; # Revision Entity
@@ -103,8 +105,10 @@
 (defn push-revision
   "Record a new `Revision` for ENTITY with ID.
    Returns OBJECT."
-  {:arglists '([& {:keys [object entity id user-id skip-serialization? is-reversion?]}])}
-  [& {object :object, :keys [entity id user-id skip-serialization? is-reversion?], :or {user-id *current-user-id*, id (:id object), skip-serialization? false, is-reversion? false}}]
+  {:arglists '([& {:keys [object entity id user-id is-creation? skip-serialization? is-reversion?]}])}
+  [& {object :object,
+      :keys [entity id user-id is-creation? skip-serialization? is-reversion?],
+      :or {id (:id object), is-creation? false, skip-serialization? false, is-reversion? false}}]
   {:pre [(metabase-entity? entity)
          (integer? user-id)
          (db/exists? User :id user-id)
@@ -114,7 +118,7 @@
   (let [object (if skip-serialization? object
                    (serialize-instance entity id object))]
     (assert (map? object))
-    (ins Revision :model (:name entity) :model_id id, :user_id user-id, :object object, :is_reversion is-reversion?))
+    (ins Revision :model (:name entity) :model_id id, :user_id user-id, :object object, :is_creation is-creation?, :is_reversion is-reversion?))
   (delete-old-revisions entity id)
   object)
 

test/metabase/api/activity_test.clj

 (ns metabase.api.activity-test
   "Tests for /api/activity endpoints."
   (:require [expectations :refer :all]
+            [korma.core :as k]
             [metabase.db :as db]
             [metabase.http-client :refer :all]
             (metabase.models [activity :refer [Activity]]
@@ -19,7 +20,8 @@
 ;  2. :user and :model_exists are hydrated
 
 ; NOTE: timestamp matching was being a real PITA so I cheated a bit.  ideally we'd fix that
-(expect-let [activity1 (db/ins Activity
+(expect-let [_         (k/delete Activity)
+             activity1 (db/ins Activity
                          :topic     "install"
                          :details   {}
                          :timestamp (u/parse-iso8601 "2015-09-09T12:13:14.888Z"))
@@ -97,9 +99,7 @@
       :custom_id    nil
       :details      $})]
   (->> ((user->client :crowberto) :get 200 "activity")
-       (map #(dissoc % :timestamp))
-       ;; a little hacky, but we limit the possible results to just the test activity we manually generated
-       (filter #(contains? #{(:id activity1) (:id activity2) (:id activity3)} (:id %)))))
+       (map #(dissoc % :timestamp))))
 
 
 ;; GET /recent_views
@@ -158,7 +158,8 @@
                         :model    model
                         :model_id model-id
                         :timestamp (u/new-sql-timestamp))
-                      (Thread/sleep 25))]
+                      ;; we sleep a few milliseconds to ensure no events have the same timestamp
+                      (Thread/sleep 5))]
     (do
       (create-view (user->id :crowberto) "card" (:id card2))
       (create-view (user->id :crowberto) "dashboard" (:id dash1))

test/metabase/api/revision_test.clj

@@ -4,84 +4,130 @@
             [medley.core :as m]
             [metabase.api.revision :refer :all]
             [metabase.db :as db]
-            (metabase.models [dashboard :refer [Dashboard]]
+            (metabase.models [card :refer [Card]]
+                             [dashboard :refer [Dashboard]]
                              [dashboard-card :refer [DashboardCard]]
+                             [revision :refer [push-revision revisions]]
                              [revision-test :refer [with-fake-card]])
-            [metabase.test.data.users :refer :all]))
-
-(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#))))))
+            [metabase.test.data :refer :all]
+            [metabase.test.data.users :refer :all]
+            [metabase.test.util :refer [expect-eval-actual-first with-temp random-name]]))
 
 (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)))
-
-(defn- get-dashboard-revisions [dashboard-id]
-  (->> ((user->client :rasta) :get 200 "revision", :entity :dashboard, :id dashboard-id)
+(defn- get-revisions [entity object-id]
+  (->> ((user->client :rasta) :get 200 "revision", :entity entity, :id object-id)
        (mapv #(dissoc % :timestamp :id))))
 
-(defn- post-dashcard [dash-id card-id]
-  ((user->client :rasta) :post 200 (format "dashboard/%d/cards" dash-id), {:cardId card-id}))
-
-(defn- delete-dashcard [dash-id card-id]
-  ((user->client :rasta) :delete 204 (format "dashboard/%d/cards" dash-id), :dashcardId (db/sel :one :id DashboardCard :dashboard_id dash-id, :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}]
-      (post-dashcard dash-id card-id)
-      (get-dashboard-revisions dash-id))))
-
-(expect [{:is_reversion false, :user @rasta-revision-info, :description "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}]
-        (post-dashcard dash-id card-id₁)
-        (post-dashcard dash-id card-id₂)
-        (get-dashboard-revisions dash-id)))))
-
-(expect [{:is_reversion false, :user @rasta-revision-info, :description "removed a card."}
-         {:is_reversion false, :user @rasta-revision-info, :description "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}]
-        (post-dashcard   dash-id card-id₁)
-        (post-dashcard   dash-id card-id₂)
-        (delete-dashcard dash-id card-id₂)
-        (get-dashboard-revisions dash-id)))))
-
-;;; # TESTS FOR POST /api/revision/revert
-(expect [2
-         [{:is_reversion true,  :user @rasta-revision-info, :description "reverted to an earlier revision and added a card."}
-          {:is_reversion false, :user @rasta-revision-info, :description "removed a card."}
-          {:is_reversion false, :user @rasta-revision-info, :description "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}]
-        (post-dashcard   dash-id card-id₁)
-        (post-dashcard   dash-id card-id₂)
-        (delete-dashcard dash-id card-id₂)
-        (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
-           (get-dashboard-revisions dash-id)])))))
+(defn- create-test-card []
+  (let [rand-name (random-name)]
+    (db/ins Card
+      :name                   rand-name
+      :description            rand-name
+      :public_perms           2
+      :display                "table"
+      :dataset_query          {:database (db-id)
+                               :type     "query"
+                               :query    {:aggregation ["rows"]
+                                          :source_table (id :categories)}}
+      :visualization_settings {}
+      :creator_id             (user->id :rasta))))
+
+(defn- create-card-revision [card is-creation? is-reversion?]
+  (push-revision
+    :object        card
+    :entity        Card
+    :id            (:id card)
+    :user-id       (user->id :rasta)
+    :is-creation?  is-creation?
+    :is-reversion? is-reversion?))
+
+(defn- create-test-dashboard []
+  (let [rand-name (random-name)]
+    (db/ins Dashboard
+      :name                   rand-name
+      :description            rand-name
+      :public_perms           2
+      :creator_id             (user->id :rasta))))
+
+(defn- create-dashboard-revision [dash is-creation? is-reversion?]
+  (push-revision
+    :object        (Dashboard (:id dash))
+    :entity        Dashboard
+    :id            (:id dash)
+    :user-id       (user->id :rasta)
+    :is-creation?  is-creation?
+    :is-reversion? is-reversion?))
+
+
+;;; # GET /revision
+
+; Things we are testing for:
+;  1. ordered by timestamp DESC
+;  2. :user is hydrated
+;  3. :description is calculated
+
+;; case with no revisions
+(expect-let [{:keys [id] :as card} (create-test-card)]
+  [{:user {}, :description "First revision."}]
+  (get-revisions :card id))
+
+;; case with single creation revision
+(expect-let [{:keys [id] :as card} (create-test-card)]
+  [{:is_reversion false, :is_creation true, :user @rasta-revision-info, :description "First revision."}]
+  (do
+    (create-card-revision card true false)
+    (get-revisions :card id)))
+
+;; case with multiple revisions, including reversion
+(expect-let [{:keys [id name] :as card} (create-test-card)]
+  [{:is_reversion true, :is_creation false, :user @rasta-revision-info, :description (format "reverted to an earlier revision and renamed this Card from \"something else\" to \"%s\"." name)}
+   {:is_reversion false, :is_creation false, :user @rasta-revision-info, :description (format "renamed this Card from \"%s\" to \"something else\"." name)}
+   {:is_reversion false, :is_creation true, :user @rasta-revision-info, :description "First revision."}]
+  (do
+    (create-card-revision card true false)
+    (create-card-revision (assoc card :name "something else") false false)
+    (create-card-revision card false true)
+    (get-revisions :card id)))
+
+;; dashboard with single revision
+(expect-let [{:keys [id] :as dash} (create-test-dashboard)]
+  [{:is_reversion false, :is_creation true, :user @rasta-revision-info, :description "First revision."}]
+  (do
+    (create-dashboard-revision dash true false)
+    (get-revisions :dashboard id)))
+
+;; dashboard with card add then delete
+(expect-let [{:keys [id] :as dash} (create-test-dashboard)
+             card                  (create-test-card)]
+  [{:is_reversion false, :is_creation false, :user @rasta-revision-info, :description "removed a card."}
+   {:is_reversion false, :is_creation false, :user @rasta-revision-info, :description "added a card."}
+   {:is_reversion false, :is_creation true, :user @rasta-revision-info, :description "First revision."}]
+  (do
+    (create-dashboard-revision dash true false)
+    (let [dashcard (db/ins DashboardCard :dashboard_id id :card_id (:id card))]
+      (create-dashboard-revision dash false false)
+      (db/del DashboardCard :id (:id dashcard)))
+    (create-dashboard-revision dash false false)
+    (get-revisions :dashboard id)))
+
+
+;;; # POST /revision/revert
+
+(expect-let [{:keys [id] :as dash} (create-test-dashboard)
+             card                  (create-test-card)]
+  [{:is_reversion true,  :is_creation false, :user @rasta-revision-info, :description "reverted to an earlier revision and added a card."}
+   {:is_reversion false, :is_creation false, :user @rasta-revision-info, :description "removed a card."}
+   {:is_reversion false, :is_creation false, :user @rasta-revision-info, :description "added a card."}
+   {:is_reversion false, :is_creation true, :user @rasta-revision-info, :description "First revision."}]
+  (do
+    (create-dashboard-revision dash true false)
+    (let [dashcard (db/ins DashboardCard :dashboard_id id :card_id (:id card))]
+      (create-dashboard-revision dash false false)
+      (db/del DashboardCard :id (:id dashcard)))
+    (create-dashboard-revision dash false false)
+    (let [[_ {previous-revision-id :id}] (revisions Dashboard id)]
+      ;; Revert to the previous revision
+      ((user->client :rasta) :post 200 "revision/revert", {:entity :dashboard, :id id, :revision_id previous-revision-id}))
+    (get-revisions :dashboard id)))

test/metabase/events/revision_test.clj

+(ns metabase.events.revision-test
+  (:require [expectations :refer :all]
+            [korma.core :as k]
+            [metabase.db :as db]
+            [metabase.events.revision :refer :all]
+            (metabase.models [card :refer [Card]]
+                             [dashboard :refer [Dashboard]]
+                             [dashboard-card :refer [DashboardCard]]
+                             [revision :refer [Revision revisions]]
+                             [revision-test :refer [with-fake-card]])
+            [metabase.test.data :refer :all]
+            [metabase.test.data.users :refer :all]
+            [metabase.test.util :refer [expect-eval-actual-first with-temp random-name]]
+            [metabase.test-setup :refer :all]))
+
+(defn- create-test-card []
+  (let [rand-name (random-name)]
+    (db/ins Card
+      :name                   rand-name
+      :description            rand-name
+      :public_perms           2
+      :display                "table"
+      :dataset_query          {:database (db-id)
+                               :type     "query"
+                               :query    {:aggregation ["rows"]
+                                          :source_table (id :categories)}}
+      :visualization_settings {}
+      :creator_id             (user->id :crowberto))))
+
+(defn- test-card-object [card]
+  {:description (:name card),
+   :table_id (id :categories),
+   :database_id (db-id),
+   :organization_id nil,
+   :query_type "query",
+   :name (:name card),
+   :creator_id (user->id :crowberto),
+   :dataset_query (:dataset_query card),
+   :id (:id card),
+   :display "table",
+   :visualization_settings {},
+   :public_perms 2})
+
+(defn- create-test-dashboard []
+  (let [rand-name (random-name)]
+    (db/ins Dashboard
+      :name                   rand-name
+      :description            rand-name
+      :public_perms           2
+      :creator_id             (user->id :crowberto))))
+
+(defn- test-dashboard-object [dashboard]
+  {:description (:name dashboard),
+   :name (:name dashboard),
+   :public_perms 2})
+
+
+;; :card-create
+(expect-let [{card-id :id :as card} (create-test-card)]
+  {:model        "Card"
+   :model_id     card-id
+   :user_id      (user->id :crowberto)
+   :object       (test-card-object card)
+   :is_reversion false
+   :is_creation  true}
+  (do
+    (process-revision-event {:topic :card-create
+                             :item  card})
+    (-> (db/sel :one Revision :model "Card" :model_id card-id)
+        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))
+
+;; :card-update
+(expect-let [{card-id :id :as card} (create-test-card)]
+  {:model        "Card"
+   :model_id     card-id
+   :user_id      (user->id :crowberto)
+   :object       (test-card-object card)
+   :is_reversion false
+   :is_creation  false}
+  (do
+    (process-revision-event {:topic :card-update
+                             :item  card})
+    (-> (db/sel :one Revision :model "Card" :model_id card-id)
+        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))
+
+;; :dashboard-create
+(expect-let [{dashboard-id :id :as dashboard} (create-test-dashboard)]
+  {:model        "Dashboard"
+   :model_id     dashboard-id
+   :user_id      (user->id :crowberto)
+   :object       (assoc (test-dashboard-object dashboard) :cards [])
+   :is_reversion false
+   :is_creation  true}
+  (do
+    (process-revision-event {:topic :dashboard-create
+                             :item  dashboard})
+    (-> (db/sel :one Revision :model "Dashboard" :model_id dashboard-id)
+        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))
+
+;; :dashboard-update
+(expect-let [{dashboard-id :id :as dashboard} (create-test-dashboard)]
+  {:model        "Dashboard"
+   :model_id     dashboard-id
+   :user_id      (user->id :crowberto)
+   :object       (assoc (test-dashboard-object dashboard) :cards [])
+   :is_reversion false
+   :is_creation  false}
+  (do
+    (process-revision-event {:topic :dashboard-update
+                             :item  dashboard})
+    (-> (db/sel :one Revision :model "Dashboard" :model_id dashboard-id)
+        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))
+
+;; :dashboard-add-cards
+(expect-let [{dashboard-id :id :as dashboard} (create-test-dashboard)
+             {card-id :id}                    (create-test-card)
+             dashcard                         (db/ins DashboardCard :card_id card-id :dashboard_id dashboard-id)]
+  {:model        "Dashboard"
+   :model_id     dashboard-id
+   :user_id      (user->id :crowberto)
+   :object       (assoc (test-dashboard-object dashboard) :cards [(select-keys dashcard [:id :card_id :sizeX :sizeY :row :col])])
+   :is_reversion false
+   :is_creation  false}
+  (do
+    (process-revision-event {:topic :dashboard-add-cards
+                             :item  {:id       dashboard-id
+                                     :actor_id (user->id :crowberto)
+                                     :dashcards [dashcard]}})
+    (-> (db/sel :one Revision :model "Dashboard" :model_id dashboard-id)
+        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))
+
+;; :dashboard-remove-cards
+(expect-let [{dashboard-id :id :as dashboard} (create-test-dashboard)
+             {card-id :id}                    (create-test-card)
+             dashcard                         (db/ins DashboardCard :card_id card-id :dashboard_id dashboard-id)
+             _                                (db/del DashboardCard :id (:id dashcard))]
+  {:model        "Dashboard"
+   :model_id     dashboard-id
+   :user_id      (user->id :crowberto)
+   :object       (assoc (test-dashboard-object dashboard) :cards [])
+   :is_reversion false
+   :is_creation  false}
+  (do
+    (process-revision-event {:topic :dashboard-remove-cards
+                             :item  {:id       dashboard-id
+                                     :actor_id (user->id :crowberto)
+                                     :dashcards [dashcard]}})
+    (-> (db/sel :one Revision :model "Dashboard" :model_id dashboard-id)
+        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))
+
+;; :dashboard-reposition-cards
+(expect-let [{dashboard-id :id :as dashboard} (create-test-dashboard)
+             {card-id :id}                    (create-test-card)
+             dashcard                         (db/ins DashboardCard :card_id card-id :dashboard_id dashboard-id)
+             _                                (db/upd DashboardCard (:id dashcard) :sizeX 4)]
+  {:model        "Dashboard"
+   :model_id     dashboard-id
+   :user_id      (user->id :crowberto)
+   :object       (assoc (test-dashboard-object dashboard) :cards [{:id      (:id dashcard)
+                                                                   :card_id card-id
+                                                                   :sizeX   4
+                                                                   :sizeY   2
+                                                                   :row     nil
+                                                                   :col     nil}])
+   :is_reversion false
+   :is_creation  false}
+  (do
+    (process-revision-event {:topic :dashboard-reeposition-cards
+                             :item  {:id       dashboard-id
+                                     :actor_id (user->id :crowberto)
+                                     :dashcards [(assoc dashcard :sizeX 4)]}})
+    (-> (db/sel :one Revision :model "Dashboard" :model_id dashboard-id)
+        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))

test/metabase/models/revision_test.clj

@@ -52,11 +52,12 @@
   (with-fake-card [{card-id :id}]
     (revisions FakedCard card-id)))
 
-;; Test that when we can add a revision
+;; Test that we can add a revision
 (expect [{:model        "FakedCard"
           :user_id      (user->id :rasta)
           :object       {:name "Tips Created by Day", :serialized true}
-          :is_reversion false}]
+          :is_reversion false
+          :is_creation  false}]
   (with-fake-card [{card-id :id}]
     (push-fake-revision card-id, :name "Tips Created by Day")
     (->> (revisions FakedCard card-id)
@@ -66,11 +67,13 @@
 (expect [{:model        "FakedCard"
           :user_id      (user->id :rasta)
           :object       {:name "Spots Created by Day", :serialized true}
-          :is_reversion false}
+          :is_reversion false
+          :is_creation  false}
          {:model        "FakedCard"
           :user_id      (user->id :rasta)
           :object       {:name "Tips Created by Day", :serialized true}
-          :is_reversion false}]
+          :is_reversion false
+          :is_creation  false}]
   (with-fake-card [{card-id :id}]
     (push-fake-revision card-id, :name "Tips Created by Day")
     (push-fake-revision card-id, :name "Spots Created by Day")
@@ -89,6 +92,7 @@
 
 ;; Check that revisions+details pulls in user info and adds description
 (expect [{:is_reversion false,
+          :is_creation  false,
           :user         {:id (user->id :rasta), :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"},
           :description  "First revision."}]
   (with-fake-card [{card-id :id}]
@@ -98,9 +102,11 @@
 
 ;; Check that revisions properly defer to describe-diff
 (expect [{:is_reversion false,
+          :is_creation  false,
           :user         {:id (user->id :rasta), :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"},
           :description  "BEFORE={:name \"Tips Created by Day\", :serialized true},AFTER={:name \"Spots Created by Day\", :serialized true}"}
          {:is_reversion false,
+          :is_creation  false,
           :user         {:id (user->id :rasta), :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"},
           :description  "First revision."}]
   (with-fake-card [{card-id :id}]
@@ -134,15 +140,18 @@
 (expect [{:model        "FakedCard"
           :user_id      (user->id :rasta)
           :object       {:name "Tips Created by Day", :serialized true}
-          :is_reversion true}
+          :is_reversion true
+          :is_creation  false}
          {:model        "FakedCard",
           :user_id      (user->id :rasta)
           :object       {:name "Spots Created by Day", :serialized true}
-          :is_reversion false}
+          :is_reversion false
+          :is_creation  false}
          {:model        "FakedCard",
           :user_id      (user->id :rasta)
           :object       {:name "Tips Created by Day", :serialized true}
-          :is_reversion false}]
+          :is_reversion false
+          :is_creation  false}]
   (with-fake-card [{card-id :id}]
     (push-fake-revision card-id, :name "Tips Created by Day")
     (push-fake-revision card-id, :name "Spots Created by Day")