Fix the revision history of an edited transform test (#83818)

enterprise/backend/test/metabase_enterprise/transform_testing/revision_test.clj

@@ -6,6 +6,7 @@
    [metabase-enterprise.transform-testing.run-tracking :as transform-testing.run-tracking]
    [metabase.revisions.core :as revisions]
    [metabase.revisions.events]
+   [metabase.revisions.models.revision.diff :as revision.diff]
    [metabase.test :as mt]
    [toucan2.core :as t2]))
 
@@ -82,3 +83,39 @@
           (is (zero? (t2/count :model/Revision :model "TransformTestRun" :model_id run-id)))
           (finally
             (t2/delete! :model/TransformTestRun :id run-id)))))))
+
+(deftest transform-test-revision-history-api-test
+  (testing "GET /api/revision/transform-test/:id describes a rename"
+    (mt/with-premium-features #{:transforms-basic :transforms-testing}
+      (mt/with-temporary-raw-setting-values [transforms-enabled "true"]
+        (mt/with-current-user (mt/user->id :crowberto)
+          (mt/with-temp [:model/Transform     {transform-id :id} {}
+                         :model/TransformTest {transform-test-id :id} {:transform_id transform-id
+                                                                       :name         "Original"}]
+            (t2/update! :model/TransformTest transform-test-id {:name "Renamed"})
+            (is (=? [{:is_creation false
+                      :description "renamed this Transform test from \"Original\" to \"Renamed\"."}
+                     {:is_creation true
+                      :description "created this."}]
+                    (mt/user-http-request :crowberto :get 200 (str "revision/transform-test/" transform-test-id))))))))))
+
+(deftest transform-test-revision-history-without-display-name-api-test
+  (testing "GET /api/revision/transform-test/:id loads when the model has no display name"
+    (mt/with-premium-features #{:transforms-basic :transforms-testing}
+      (mt/with-temporary-raw-setting-values [transforms-enabled "true"]
+        (mt/with-current-user (mt/user->id :crowberto)
+          (mt/with-temp [:model/Transform     {transform-id :id} {}
+                         :model/TransformTest {transform-test-id :id} {:transform_id transform-id
+                                                                       :name         "Original"}]
+            (t2/update! :model/TransformTest transform-test-id {:name "Renamed"})
+            (mt/with-dynamic-fn-redefs [revision.diff/model-str->i18n-str
+                                        (fn [model-str]
+                                          (throw (IllegalArgumentException. (str "No matching clause: " model-str))))]
+              (mt/with-log-messages-for-level [messages [metabase.revisions.models.revision.diff :warn]]
+                (is (=? [{:is_creation false
+                          :description "renamed this TransformTest from \"Original\" to \"Renamed\"."}
+                         {:is_creation true
+                          :description "created this."}]
+                        (mt/user-http-request :crowberto :get 200 (str "revision/transform-test/" transform-test-id))))
+                (is (=? [{:level :warn, :message #".*TransformTest.*"}]
+                        (messages)))))))))))

src/metabase/revisions/models/revision/diff.clj

@@ -4,6 +4,7 @@
    [metabase.models.interface :as mi]
    [metabase.revisions.db :as revisions.db]
    [metabase.util.i18n :refer [deferred-tru]]
+   [metabase.util.log :as log]
    [metabase.util.match :as match]))
 
 (defn- readable-name
@@ -150,19 +151,26 @@
 (defn ^:private model-str->i18n-str
   [model-str]
   (case model-str
-    "Dashboard" (deferred-tru "Dashboard")
+    "Dashboard"     (deferred-tru "Dashboard")
-    "Card"      (deferred-tru "Card")
+    "Card"          (deferred-tru "Card")
-    "Segment"   (deferred-tru "Segment")
+    "Segment"       (deferred-tru "Segment")
-    "Measure"   (deferred-tru "Measure")
+    "Measure"       (deferred-tru "Measure")
-    "Document"  (deferred-tru "Document")
+    "Document"      (deferred-tru "Document")
-    "Transform" (deferred-tru "Transform")))
+    "Exploration"   (deferred-tru "Exploration")
+    "Transform"     (deferred-tru "Transform")
+    "TransformTest" (deferred-tru "Transform test")))
 
 (defn diff-strings*
   "Create a seq of string describing how `o1` is different from `o2`.
   The directionality of the statement should indicate that `o1` changed into `o2`."
   [model o1 o2]
   (when-let [[before after] (data/diff o1 o2)]
-    (let [model-name (model-str->i18n-str model)
+    (let [model-name (try
+                       (model-str->i18n-str model)
+                       ;; A missing display name costs one untranslated word, not the whole history read.
+                       (catch Exception e
+                         (log/warnf e "No display name for model %s; the revision description uses the model string" model)
+                         model))
           ;; ignore collection_id as part of diff if the dashboard_id has changed
           ;; so that the final diff string doesn't contain two messages about moving
           ks         (cond->> (keys (or after before))

test/metabase/revisions/models/revision_test.clj

@@ -115,6 +115,36 @@
              {:private true :collection_unknown_field nil}
              {:private false :collection_unknown_field 1}))))))
 
+(deftest ^:parallel every-revisioned-model-has-a-diff-description-test
+  (testing "Each revisioned model gets a description of a rename"
+    (let [model->display-name (cond-> {:model/Card        "Card"
+                                       :model/Dashboard   "Dashboard"
+                                       :model/Document    "Document"
+                                       :model/Exploration "Exploration"
+                                       :model/Measure     "Measure"
+                                       :model/Segment     "Segment"
+                                       :model/Transform   "Transform"}
+                                config/ee-available? (assoc :model/TransformTest "Transform test"))]
+      ;; `push-revision!` calls `serialize-instance`, which has no default: a model with revisions has a method.
+      (is (= (set (filter #(= "model" (namespace %)) (keys (methods revision/serialize-instance))))
+             (set (keys model->display-name))))
+      (doseq [[model display-name] model->display-name]
+        (testing model
+          (mt/with-log-messages-for-level [messages [metabase.revisions.models.revision.diff :warn]]
+            (is (= (str "renamed this " display-name " from \"A\" to \"B\".")
+                   (u/build-sentence (revision/diff-strings model {:name "A"} {:name "B"}))))
+            ;; A missing display name falls back to the model string, which equals the display name for most models.
+            (testing "with no warning of a missing display name"
+              (is (= [] (messages))))))))))
+
+(deftest ^:parallel model-with-no-display-name-gets-a-description-test
+  (testing "A model with no display name gets a description with the model string, and a warning that names the model"
+    (mt/with-log-messages-for-level [messages [metabase.revisions.models.revision.diff :warn]]
+      (is (= "renamed this NoSuchModel from \"A\" to \"B\"."
+             (u/build-sentence (revision.diff/diff-strings* "NoSuchModel" {:name "A"} {:name "B"}))))
+      (is (=? [{:level :warn, :message #".*NoSuchModel.*"}]
+              (messages))))))
+
 ;;; # REVISIONS + PUSH-REVISION!
 
 (deftest new-object-no-revisions-test