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

@@ -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)))

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} {}]