From e2e1441619389efb2f6059a246e7a0454ae0caa7 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Tue, 3 May 2022 14:06:38 +0700
Subject: [PATCH] Prevent channging visibility_type when updating table
properties (#22318)
* prevent channging visibility_type when updating table properties
* prevent sync when tables are hidden
---
src/metabase/api/table.clj | 15 +++++++--------
test/metabase/api/table_test.clj | 21 +++++++++++++++++++--
2 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/src/metabase/api/table.clj b/src/metabase/api/table.clj
index 07b6a33aec8..9db3be6bcd9 100644
--- a/src/metabase/api/table.clj
+++ b/src/metabase/api/table.clj
@@ -50,19 +50,18 @@
(defn- update-table!*
"Takes an existing table and the changes, updates in the database and optionally calls `table/update-field-positions!`
if field positions have changed."
- [{:keys [id] :as existing-table} {:keys [visibility_type] :as body}]
+ [{:keys [id] :as existing-table} body]
(api/check-500
(db/update! Table id
- (assoc (u/select-keys-when body
- :non-nil [:display_name :show_in_getting_started :entity_type :field_order]
- :present [:description :caveats :points_of_interest])
- :visibility_type visibility_type)))
+ (u/select-keys-when body
+ :non-nil [:display_name :show_in_getting_started :entity_type :field_order]
+ :present [:description :caveats :points_of_interest :visibility_type])))
(let [updated-table (Table id)
changed-field-order? (not= (:field_order updated-table) (:field_order existing-table))]
(if changed-field-order?
(do
- (table/update-field-positions! updated-table)
- (hydrate updated-table [:fields [:target :has_field_values] :dimensions :has_field_values]))
+ (table/update-field-positions! updated-table)
+ (hydrate updated-table [:fields [:target :has_field_values] :dimensions :has_field_values]))
updated-table)))
(defn- sync-unhidden-tables
@@ -85,7 +84,7 @@
(api/check-404 (= (count existing-tables) (count ids)))
(run! api/write-check existing-tables)
(let [updated-tables (db/transaction (mapv #(update-table!* % body) existing-tables))
- newly-unhidden (when (nil? visibility_type)
+ newly-unhidden (when (and (contains? body :visibility_type) (nil? visibility_type))
(into [] (filter (comp some? :visibility_type)) existing-tables))]
(sync-unhidden-tables newly-unhidden)
updated-tables)))
diff --git a/test/metabase/api/table_test.clj b/test/metabase/api/table_test.clj
index c0ed6452c98..ee5e239fae5 100644
--- a/test/metabase/api/table_test.clj
+++ b/test/metabase/api/table_test.clj
@@ -291,6 +291,13 @@
{property ""})
property))))))
+ (testing "Don't change visibility_type when updating properties (#22287)"
+ (doseq [property [:caveats :points_of_interest :description :display_name]]
+ (mt/with-temp Table [table {:visibility_type "hidden"}]
+ (mt/user-http-request :crowberto :put 200 (format "table/%d" (u/the-id table))
+ {property (mt/random-name)})
+ (is (= :hidden (db/select-one-field :visibility_type Table :id (:id table)))))))
+
(testing "A table can only be updated by a superuser"
(mt/with-temp Table [table]
(mt/user-http-request :rasta :put 403 (format "table/%d" (u/the-id table)) {:display_name "Userz"})))))
@@ -318,7 +325,12 @@
(mt/user-http-request :crowberto :put 200 (format "table/%d" (:id table))
{:display_name "Userz"
:visibility_type state
- :description "What a nice table!"}))]
+ :description "What a nice table!"}))
+ set-name (fn []
+ (mt/user-http-request :crowberto :put 200 (format "table/%d" (:id table))
+ {:display_name (mt/random-name)
+ :description "What a nice table!"}))]
+
(set-visibility "hidden")
(set-visibility nil) ; <- should get synced
(is (= 1
@@ -331,7 +343,12 @@
@called))
(set-visibility "technical")
(is (= 2
- @called))))))))
+ @called))
+ (testing "Update table's properties shouldn't trigger sync"
+ (set-name)
+ (is (= 2
+ @called)))))))))
+
(testing "Bulk updating visibility"
(let [unhidden-ids (atom #{})]
(mt/with-temp* [Table [{id-1 :id} {}]
--
GitLab