Update the keys we consider to unverify a card (#18734)

* Update the keys we consider to unverify a card

Previously when almost anything changed we would unverify a card. But
this was way too aggresive. We really only want to unverify when a query
changes. Far simpler to specify the keys we do want to consider rather
than the keys to ignore.

Was prompted because when we marked a question as a dataset it would
unverify because the `:display` was changed and also `:dataset`. Neither
of these matter for verification purposes

* Update testing context string [ci skip]

* Empty commit to trigger CI

had skipped ci since it was a docstring change but that prevents merging
sad face :(

src/metabase/api/card.clj

@@ -425,38 +425,37 @@
   (-> card :moderation_reviews first :status #{"verified"} boolean))
 
 (defn- changed?
-  "Return whether there were any changes other than ignored values.
+  "Return whether there were any changes in the objects at the keys for `consider`.
 
   returns false because changes to collection_id are ignored:
-  (changed? {:collection_id 1 :description \"foo\"}
-            {:collection_id 2}
-            {:ignore #{:collection_id}})
+  (changed? #{:description}
+            {:collection_id 1 :description \"foo\"}
+            {:collection_id 2 :description \"foo\"})
 
   returns true:
-  (changed? {:collection_id 1 :description \"foo\"}
-            {:collection_id 2 :description \"diff\"}
-            {:ignore #{:collection_id}})"
-  [card-before updates {:keys [ignore]}]
+  (changed? #{:description}
+            {:collection_id 1 :description \"foo\"}
+            {:collection_id 2 :description \"diff\"})"
+  [consider card-before updates]
   ;; have to ignore keyword vs strings over api. `{:type :query}` vs `{:type "query"}`
   (let [prepare              (fn prepare [card] (walk/prewalk (fn [x] (if (keyword? x)
                                                                         (name x)
                                                                         x))
                                                               card))
-        before               (apply dissoc card-before ignore)
-        after                (apply dissoc updates ignore)
-        [_ changes-in-after] (data/diff (prepare before) (prepare after))]
+        before               (prepare (select-keys card-before consider))
+        after                (prepare (select-keys updates consider))
+        [_ changes-in-after] (data/diff before after)]
     (boolean (seq changes-in-after))))
 
 
 
-(def card-compare-ignores
-  "Keys to ignore when comparing card changes to the existing card."
-  #{:result_metadata ;; not interested in changes here
-    ;; ignore collection changes, pinning, and public changes
-    :collection_id :collection_position
-    :public_uuid :made_public_by_id
-    ;; compares java.time objects vs strings over api
-    :created_at :updated_at})
+(def card-compare-keys
+  "When comparing a card to possibly unverify, only consider these keys as changing something 'important' about the
+  query."
+  #{:table_id
+    :database_id
+    :query_type ;; these first three may not even be changeable
+    :dataset_query})
 
 (defn- update-card-async!
   "Update a Card asynchronously. Returns a `core.async` promise channel that will return updated Card."
@@ -468,8 +467,7 @@
       (api/maybe-reconcile-collection-position! card-before-update card-updates)
 
       (when (and (card-is-verified? card-before-update)
-                 (changed? card-before-update card-updates
-                           {:ignore card-compare-ignores}))
+                 (changed? card-compare-keys card-before-update card-updates))
         ;; this is an enterprise feature but we don't care if enterprise is enabled here. If there is a review we need
         ;; to remove it regardless if enterprise edition is present at the moment.
         (moderation-review/create-review! {:moderated_item_id   id

test/metabase/api/card_test.clj

@@ -1525,21 +1525,23 @@
 
 (deftest changed?-test
   (letfn [(changed? [before after]
-            (#'card-api/changed? before after {:ignore card-api/card-compare-ignores}))]
+            (#'card-api/changed? card-api/card-compare-keys before after))]
    (testing "Ignores keyword/string"
      (is (false? (changed? {:dataset_query {:type :query}} {:dataset_query {:type "query"}}))))
-   (testing "Ignores properties passed in `card-api/card-compare-ignores"
+   (testing "Ignores properties not in `card-api/card-compare-keys"
      (is (false? (changed? {:collection_id 1
                             :collection_position 0}
                            {:collection_id 2
                             :collection_position 1}))))
    (testing "Sees changes"
-     (is (true? (changed? {:description "foo"} {:description "diff"})))
-     (testing "But only when they are different in the after"
-       (is (false? (changed? {:description "foo" :title "something" :collection_id 1}
+     (is (true? (changed? {:dataset_query {:type :query}}
+                          {:dataset_query {:type :query
+                                           :query {}}})))
+     (testing "But only when they are different in the after, not just omitted"
+       (is (false? (changed? {:dataset_query {} :collection_id 1}
                              {:collection_id 1})))
-       (is (true? (changed? {:description "foo" :title "something" :collection_id 1}
-                            {:description nil :title nil :collection_id 1})))))))
+       (is (true? (changed? {:dataset_query {} :collection_id 1}
+                            {:dataset_query nil :collection_id 1})))))))
 
 (deftest update-verified-card-test
   (tools.macro/macrolet
@@ -1571,7 +1573,7 @@
         (testing "Changing core attributes un-verifies the card"
           (with-card :verified
             (is (verified? card))
-            (update-card card {:description "a new description"})
+            (update-card card (update-in card [:dataset_query :query :source-table] inc))
             (is (not (verified? card)))
             (testing "The unverification edit has explanatory text"
               (is (= "Unverified due to edit"
@@ -1591,7 +1593,22 @@
             (testing "making public"
               (remains-verified
                (update-card card {:made_public_by_id (mt/user->id :rasta)
-                                  :public_uuid (UUID/randomUUID)})))))
+                                  :public_uuid (UUID/randomUUID)})))
+            (testing "Changing description"
+              (remains-verified
+               (update-card card {:description "foo"})))
+            (testing "Changing name"
+              (remains-verified
+               (update-card card {:name "foo"})))
+            (testing "Changing archived"
+              (remains-verified
+               (update-card card {:archived true})))
+            (testing "Changing display"
+              (remains-verified
+               (update-card card {:display :line})))
+            (testing "Changing visualization settings"
+              (remains-verified
+               (update-card card {:visualization_settings {:table.cell_column "FOO"}})))))
         (testing "Does not add a new nil moderation review when not verified"
           (with-card :not-verified
             (is (empty? (reviews card)))