Merge branch 'create_dashboard' of github.com:metabase/metabase-init into create_dashboard

src/metabase/models/dashboard.clj

@@ -68,7 +68,7 @@
 
   serialized-dashboard)
 
-(defn- describe-diff [_ username dashboard₁ dashboard₂]
+(defn- describe-diff [_ dashboard₁ dashboard₂]
   (let [[removals changes] (diff dashboard₁ dashboard₂)]
     (->> [(when (:name changes)
             (format "renamed it from \"%s\" to \"%s\"" (:name dashboard₁) (:name dashboard₂)))
@@ -87,7 +87,6 @@
                 :else                     "rearranged the cards")))]
          (filter identity)
          build-sentence
-         (apply str username " ")
          (#(s/replace-first % "it " "this dashboard ")))))
 
 (extend DashboardEntity

src/metabase/models/revision.clj

@@ -21,7 +21,7 @@
     "Prepare an instance for serialization in a `Revision`.")
   (revert-to-revision [this id serialized-instance]
     "Return an object to the state recorded by SERIALIZED-INSTANCE.")
-  (describe-diff [this username object1 object2]
+  (describe-diff [this object1 object2]
     "Return a string describing the difference between OBJECT1 and OBJECT2."))
 
 ;;; ## Default Impl
@@ -34,8 +34,8 @@
          (m/filter-vals (complement delay?))))
   (revert-to-revision [entity id serialized-instance]
     (m/mapply upd entity id serialized-instance))
-  (describe-diff [entity username o1 o2]
-    (diff-str username (:name entity) o1 o2)))
+  (describe-diff [entity o1 o2]
+    (diff-str (:name entity) o1 o2)))
 
 
 ;;; # Revision Entity
@@ -64,12 +64,8 @@
   (loop [acc [], [r1 r2 & more] revisions]
     (if-not r2
       (conj acc (assoc r1 :description "First revision."))
-      (let [username (str (or (:common_name (:user r1))
-                              "An unknown user")
-                          (when (:is_reversion r1)
-                            " reverted to an earlier revision and"))]
-        (recur (conj acc (assoc r1 :description (describe-diff entity username (:object r2) (:object r1))))
-               (conj more r2))))))
+      (recur (conj acc (assoc r1 :description (describe-diff entity (:object r2) (:object r1))))
+             (conj more r2)))))
 
 (defn- add-details
   "Hydrate `user` and add `:description` to a sequence of REVISIONS."

src/metabase/models/revision/diff.clj

@@ -37,14 +37,12 @@
 
 (defn diff-str
   ([t o1 o2]
-   (let [[before after] (data/diff o1 o2)]
+   (let [[before after] (data/diff o1 o2)
+         reverted (when (:is_reversion o2) "reverted to an earlier revision and ")]
      (when before
        (let [ks (keys before)]
          (some-> (filter identity (for [k ks]
                                 (diff-str* t k (k before) (k after))))
                  build-sentence
-                 (s/replace-first #" it " (format " this %s " t)))))))
-  ([username t o1 o2]
-   (let [s (diff-str t o1 o2)]
-     (when (seq s)
-       (str username " " s)))))
+                 (s/replace-first #" it " (format " this %s " t))
+                 (->> (str (when reverted reverted)))))))))

test/metabase/models/revision/diff_test.clj

@@ -4,31 +4,37 @@
 
 
 ;; Check that pattern matching allows specialization and that string only reflects the keys that have changed
-(expect "Cam Saul renamed this card from \"Tips by State\" to \"Spots by State\"."
-  (diff-str "Cam Saul" "card"
+(expect "renamed this card from \"Tips by State\" to \"Spots by State\"."
+  (diff-str "card"
             {:name "Tips by State", :private false}
             {:name "Spots by State", :private false}))
 
-(expect "Cam Saul made this card private."
-  (diff-str "Cam Saul" "card"
+(expect "made this card private."
+  (diff-str "card"
             {:name "Spots by State", :private false}
             {:name "Spots by State", :private true}))
 
 
 ;; Check the fallback sentence fragment for key without specialized sentence fragment
-(expect "Cam Saul changed priority from \"Important\" to \"Regular\"."
-  (diff-str "Cam Saul" "card"
+(expect "changed priority from \"Important\" to \"Regular\"."
+  (diff-str "card"
             {:priority "Important"}
             {:priority "Regular"}))
 
 ;; Check that 2 changes are handled nicely
-(expect "Cam Saul made this card private and renamed it from \"Tips by State\" to \"Spots by State\"."
-  (diff-str "Cam Saul" "card"
+(expect "made this card private and renamed it from \"Tips by State\" to \"Spots by State\"."
+  (diff-str "card"
             {:name "Tips by State", :private false}
             {:name "Spots by State", :private true}))
 
 ;; Check that several changes are handled nicely
-(expect "Cam Saul changed priority from \"Important\" to \"Regular\", made this card private and renamed it from \"Tips by State\" to \"Spots by State\"."
-  (diff-str "Cam Saul" "card"
+(expect "changed priority from \"Important\" to \"Regular\", made this card private and renamed it from \"Tips by State\" to \"Spots by State\"."
+  (diff-str "card"
             {:name "Tips by State", :private false, :priority "Important"}
             {:name "Spots by State", :private true, :priority "Regular"}))
+
+;; Check that when the revision is a reversion from a previous revision that we update the string accordingly
+(expect "reverted to an earlier revision and renamed this card from \"Tips by State\" to \"Spots by State\"."
+        (diff-str "card"
+                  {:name "Tips by State", :private false}
+                  {:name "Spots by State", :private false, :is_reversion true}))