diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index 3d92dd8a958003fcaa54e42d3a7d14ecb0db7905..8d6d68bfab471ce41632cf9972f4ec2b679e01b2 100644
--- a/src/metabase/api/card.clj
+++ b/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
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 7f736e64afbc373f3cca726944de9e324d0bf461..ebf23038f096a755976cda213f2141103c1c8aee 100644
--- a/test/metabase/api/card_test.clj
+++ b/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)))