Merge pull request #4918 from metabase/dont-analyze-hiden-tables

Don't do detailed analytics on hidden tables

src/metabase/api/table.clj

 (ns metabase.api.table
   "/api/table endpoints."
-  (:require [compojure.core :refer [GET PUT]]
+  (:require [clojure.tools.logging :as log]
+            [compojure.core :refer [GET PUT]]
+            [metabase
+             [sync-database :as sync-database]
+             [util :as u]]
             [metabase.api.common :as api]
             [metabase.models
              [field :refer [Field]]
@@ -37,6 +41,14 @@
   (-> (api/read-check Table id)
       (hydrate :db :pk_field)))
 
+(defn- visible-state?
+  "only the nil state is considered visible."
+  [state]
+  {:pre [(or (nil? state) (table/visibility-types state))]}
+  (if (nil? state)
+    :show
+    :hide))
+
 (api/defendpoint PUT "/:id"
   "Update `Table` with ID."
   [id :as {{:keys [display_name entity_type visibility_type description caveats points_of_interest show_in_getting_started]} :body}]
@@ -44,15 +56,25 @@
    entity_type     (s/maybe TableEntityType)
    visibility_type (s/maybe TableVisibilityType)}
   (api/write-check Table id)
-  (api/check-500 (db/update-non-nil-keys! Table id
-                   :display_name            display_name
-                   :caveats                 caveats
-                   :points_of_interest      points_of_interest
-                   :show_in_getting_started show_in_getting_started
-                   :entity_type             entity_type
-                   :description             description))
-  (api/check-500 (db/update! Table id, :visibility_type visibility_type))
-  (Table id))
+  (let [original-visibility-type (:visibility_type (Table :id id))]
+    (api/check-500 (db/update-non-nil-keys! Table id
+                     :display_name            display_name
+                     :caveats                 caveats
+                     :points_of_interest      points_of_interest
+                     :show_in_getting_started show_in_getting_started
+                     :entity_type             entity_type
+                     :description             description))
+    (api/check-500 (db/update! Table id, :visibility_type visibility_type))
+    (let [updated-table (Table id)
+          new-visibility (visible-state? (:visibility_type updated-table))
+          old-visibility (visible-state? original-visibility-type)
+          visibility-changed? (and (not= new-visibility
+                                         old-visibility)
+                                   (= :show new-visibility))]
+      (when visibility-changed?
+        (log/debug (u/format-color 'green "Table visibility changed, resyncing %s -> %s : %s") original-visibility-type visibility_type visibility-changed?)
+        (sync-database/sync-table! updated-table))
+      updated-table)))
 
 
 (api/defendpoint GET "/:id/query_metadata"

src/metabase/sync_database/analyze.clj

@@ -246,7 +246,7 @@
   (log/info (u/format-color 'blue "Analyzing data in %s database '%s' (this may take a while) ..." (name driver) (:name database)))
 
   (let [start-time-ns         (System/nanoTime)
-        tables                (db/select table/Table, :db_id database-id, :active true)
+        tables                (db/select table/Table, :db_id database-id, :active true, :visibility_type nil)
         tables-count          (count tables)
         finished-tables-count (atom 0)]
     (doseq [{table-name :name, :as table} tables]

test/metabase/api/table_test.clj

@@ -5,7 +5,8 @@
              [driver :as driver]
              [http-client :as http]
              [middleware :as middleware]
-             [util :as u]]
+             [util :as u]
+             [sync-database :as sync-database]]
             [metabase.models
              [database :refer [Database]]
              [field :refer [Field]]
@@ -375,6 +376,26 @@
       (dissoc ((user->client :crowberto) :get 200 (format "table/%d" (:id table)))
               :updated_at)))
 
+(tt/expect-with-temp [Table [table {:rows 15}]]
+  2
+  (let [original-sync-table! sync-database/sync-table!
+        called (atom 0)
+        test-fun (fn [state]
+                   (with-redefs [sync-database/sync-table! (fn [& args] (swap! called inc)
+                                                             (apply original-sync-table! args))]
+                     ((user->client :crowberto) :put 200 (format "table/%d" (:id table)) {:display_name    "Userz"
+                                                                                          :entity_type     "person"
+                                                                                          :visibility_type state
+                                                                                          :description     "What a nice table!"})))]
+    (do (test-fun "hidden")
+        (test-fun nil)
+        (test-fun "hidden")
+        (test-fun "cruft")
+        (test-fun "technical")
+        (test-fun nil)
+        (test-fun "technical")
+        @called)))
+
 
 ;; ## GET /api/table/:id/fks
 ;; We expect a single FK from CHECKINS.USER_ID -> USERS.ID