Fix revision failed to fetch and returns null pointer (#31352)

src/metabase/models/revision.clj

@@ -98,12 +98,21 @@
   (cond
     (:is_creation revision)  [(deferred-tru "created this")]
     (:is_reversion revision) [(deferred-tru "reverted to an earlier version")]
+    ;; We only keep [[revision/max-revisions]] number of revision per entity.
+    ;; prev-revision can be nil when we generate description for oldest revision
+    (nil? prev-revision)     [(deferred-tru "modified this")]
     :else                    (diff-strings model (:object prev-revision) (:object revision))))
 
 (defn- revision-description-info
   [model prev-revision revision]
   (let [changes (revision-changes model prev-revision revision)]
-    {:description          (build-sentence changes)
+    {:description          (if (seq changes)
+                             (build-sentence changes)
+                             ;; HACK: before #30285 we record revision even when there is nothing changed,
+                             ;; so there are cases when revision can comeback as `nil`.
+                             ;; This is a safe guard for us to not display "Crowberto null" as
+                             ;; description on UI
+                             (deferred-tru "created a revision with no change."))
      ;; this is used on FE
      :has_multiple_changes (> (count changes) 1)}))
 
@@ -142,7 +151,8 @@
   (when-let [old-revisions (seq (drop max-revisions (map :id (t2/select [Revision :id]
                                                                :model    (name model)
                                                                :model_id id
-                                                               {:order-by [[:timestamp :desc]]}))))]
+                                                               {:order-by [[:timestamp :desc]
+                                                                           [:id :desc]]}))))]
     (t2/delete! Revision :id [:in old-revisions])))
 
 (defn push-revision!

test/metabase/api/revision_test.clj

@@ -52,7 +52,7 @@
 (deftest no-revisions-test
   (testing "Loading revisions, where there are no revisions, should work"
     (t2.with-temp/with-temp [Card {:keys [id]}]
-      (is (= [{:user {}, :diff nil, :description nil, :has_multiple_changes false}]
+      (is (= [{:user {}, :diff nil, :description "modified this.", :has_multiple_changes false}]
              (get-revisions :card id))))))
 
 ;; case with single creation revision
@@ -69,6 +69,30 @@
                :description          "created this."}]
              (get-revisions :card id))))))
 
+(deftest get-revision-for-entity-with-revision-exceeds-max-revision-test
+  (t2.with-temp/with-temp [Card {:keys [id] :as card} {:name "A card"}]
+    (create-card-revision! (:id card) true :rasta)
+    (doseq [i (range (inc revision/max-revisions))]
+      (t2/update! :model/Card (:id card) {:name (format "New name %d" i)})
+      (create-card-revision! (:id card) false :rasta))
+
+    (is (= ["renamed this Card from \"New name 14\" to \"New name 15\"."
+            "renamed this Card from \"New name 13\" to \"New name 14\"."
+            "renamed this Card from \"New name 12\" to \"New name 13\"."
+            "renamed this Card from \"New name 11\" to \"New name 12\"."
+            "renamed this Card from \"New name 10\" to \"New name 11\"."
+            "renamed this Card from \"New name 9\" to \"New name 10\"."
+            "renamed this Card from \"New name 8\" to \"New name 9\"."
+            "renamed this Card from \"New name 7\" to \"New name 8\"."
+            "renamed this Card from \"New name 6\" to \"New name 7\"."
+            "renamed this Card from \"New name 5\" to \"New name 6\"."
+            "renamed this Card from \"New name 4\" to \"New name 5\"."
+            "renamed this Card from \"New name 3\" to \"New name 4\"."
+            "renamed this Card from \"New name 2\" to \"New name 3\"."
+            "renamed this Card from \"New name 1\" to \"New name 2\"."
+            "modified this."]
+           (map :description (get-revisions :card id))))))
+
 ;; case with multiple revisions, including reversion
 (deftest multiple-revisions-with-reversion-test
   (testing "Creating multiple revisions, with a reversion, works"
@@ -116,16 +140,16 @@
   (mapv #(dissoc % :id) objects))
 
 (def ^:private default-revision-card
- {:size_x                 4
-  :size_y                 4
-  :row                    0
-  :col                    0
-  :card_id                nil
-  :series                 []
-  :dashboard_tab_id       nil
-  :action_id              nil
-  :parameter_mappings     []
-  :visualization_settings {}})
+  {:size_x                 4
+   :size_y                 4
+   :row                    0
+   :col                    0
+   :card_id                nil
+   :series                 []
+   :dashboard_tab_id       nil
+   :action_id              nil
+   :parameter_mappings     []
+   :visualization_settings {}})
 
 (deftest revert-test
   (testing "Reverting through API works"
@@ -380,10 +404,10 @@
 
       ;; 1. add 2 cards
       (t2/insert-returning-pks! DashboardCard [{:dashboard_id dashboard-id
-                                                                   :size_x       4
-                                                                   :size_y       4
-                                                                   :col          1
-                                                                   :row          1}
+                                                :size_x       4
+                                                :size_y       4
+                                                :col          1
+                                                :row          1}
                                                {:dashboard_id dashboard-id
                                                 :size_x       4
                                                 :size_y       4

test/metabase/models/card_test.clj

@@ -640,71 +640,53 @@
 
 ;;; -------------------------------------------- Revision tests  --------------------------------------------
 
-(deftest diff-cards-str-test
-  (testing "update general info ---"
-    (is (= "added a description and renamed it from \"Diff Test\" to \"Diff Test Changed\"."
-           (build-sentence
-             (revision/diff-strings
-               :model/Card
-               {:name        "Diff Test"
-                :description nil}
-               {:name        "Diff Test Changed"
-                :description "foobar"}))))
-
-    (is (= "renamed this Card from \"Apple\" to \"Next\"."
-           (build-sentence
-             (revision/diff-strings
-               :model/Card
-               {:name "Apple"}
-               {:name "Next"}))))
-
-    ;; multiple changes
-    (is (= "added a description and renamed it from \"Diff Test\" to \"Diff Test changed\"."
-           (build-sentence
-             (revision/diff-strings
-               :model/Card
-               {:name        "Diff Test"
-                :description nil}
-               {:name        "Diff Test changed"
-                :description "New description"})))))
-
- (testing "update collection ---"
-   (is (= "moved this Card to Our analytics."
-          (build-sentence
-            (revision/diff-strings
-              :model/Card
-              {:name "Apple"}
-              {:name          "Apple"
-               :collection_id nil}))))
-  (t2.with-temp/with-temp
-    [Collection {coll-id :id} {:name "New collection"}]
-    (is (= "moved this Card to New collection."
-           (build-sentence
-             (revision/diff-strings
-               :model/Card
-               {:name "Apple"}
-               {:name          "Apple"
-                :collection_id coll-id}))))
-    (is (= "moved this Card from New collection to Our analytics."
-           (build-sentence
-             (revision/diff-strings
-               :model/Card
-               {:name "Apple"
-                :collection_id coll-id}
-               {:name          "Apple"
-                :collection_id nil})))))
-
-  (t2.with-temp/with-temp
-    [Collection {coll-id-1 :id} {:name "Old collection"}
-     Collection {coll-id-2 :id} {:name "New collection"}]
-    (is (= "moved this Card from Old collection to New collection."
-           (build-sentence
-             (revision/diff-strings
-               :model/Card
-               {:name          "Apple"
-                :collection_id coll-id-1}
-               {:name          "Apple"
-                :collection_id coll-id-2})))))))
+(deftest ^:parallel diff-cards-str-test
+  (are [x y expected] (= expected
+                       (build-sentence (revision/diff-strings :model/Card x y)))
+    {:name        "Diff Test"
+     :description nil}
+    {:name        "Diff Test Changed"
+     :description "foobar"}
+    "added a description and renamed it from \"Diff Test\" to \"Diff Test Changed\"."
+
+    {:name "Apple"}
+    {:name "Next"}
+    "renamed this Card from \"Apple\" to \"Next\"."
+
+    {:display :table}
+    {:display :pie}
+    "changed the display from table to pie."
+
+    {:name        "Diff Test"
+     :description nil}
+    {:name        "Diff Test changed"
+     :description "New description"}
+    "added a description and renamed it from \"Diff Test\" to \"Diff Test changed\"."))
+
+
+(deftest diff-cards-str-update-collection--test
+ (t2.with-temp/with-temp
+     [Collection {coll-id-1 :id} {:name "Old collection"}
+      Collection {coll-id-2 :id} {:name "New collection"}]
+     (are [x y expected] (= expected
+                          (build-sentence (revision/diff-strings :model/Card x y)))
+
+       {:name "Apple"}
+       {:name          "Apple"
+        :collection_id coll-id-2}
+       "moved this Card to New collection."
+
+       {:name        "Diff Test"
+        :description nil}
+       {:name        "Diff Test changed"
+        :description "New description"}
+       "added a description and renamed it from \"Diff Test\" to \"Diff Test changed\"."
+
+       {:name          "Apple"
+        :collection_id coll-id-1}
+       {:name          "Apple"
+        :collection_id coll-id-2}
+       "moved this Card from Old collection to New collection.")))
 
 (defn- create-card-revision!
   "Fetch the latest version of a Dashboard and save a revision entry for it. Returns the fetched Dashboard."

test/metabase/models/revision_test.clj

@@ -188,7 +188,15 @@
                (assert (= 2 (count revisions)))
                (-> (revision/add-revision-details FakedCard (first revisions) (last revisions))
                    (dissoc :timestamp :id :model_id)
-                   mt/derecordize)))))))
+                   mt/derecordize))))))
+
+  (testing "test that we return a description even when there is no change between revision"
+    (is (= "created a revision with no change."
+           (str (:description (revision/add-revision-details FakedCard {:name "Apple"} {:name "Apple"}))))))
+
+  (testing "that we return a descrtiopn when there is no previous revision"
+    (is (= "modified this."
+           (str (:description (revision/add-revision-details FakedCard {:name "Apple"} nil)))))))
 
 (deftest revisions+details-test
   (testing "Check that revisions+details pulls in user info and adds description"
@@ -203,9 +211,10 @@
                 :diff                 {:o1 nil
                                        :o2 {:name "Tips Created by Day", :serialized true}}
                 :has_multiple_changes false
-                :description          nil})]
+                :description          "modified this."})]
              (->> (revision/revisions+details FakedCard card-id)
-                  (map #(dissoc % :timestamp :id :model_id))))))))
+                  (map #(dissoc % :timestamp :id :model_id))
+                  (map #(update % :description str))))))))
 
 (deftest defer-to-describe-diff-test
   (testing "Check that revisions properly defer to describe-diff"
@@ -232,9 +241,10 @@
                 :diff                 {:o1 nil
                                        :o2 {:name "Tips Created by Day", :serialized true}}
                 :has_multiple_changes false
-                :description          nil})]
+                :description          "modified this."})]
              (->> (revision/revisions+details FakedCard card-id)
-                  (map #(dissoc % :timestamp :id :model_id))))))))
+                  (map #(dissoc % :timestamp :id :model_id))
+                  (map #(update % :description str))))))))
 
 ;;; # REVERT