BE: Set initial_sync_status="incomplete" during sync for any previously inactive tables (#31607)

frontend/src/metabase/browse/components/TableBrowser/TableBrowser.jsx

@@ -55,12 +55,10 @@ const TableBrowser = ({
       <Grid>
         {tables.map(table => (
           <TableGridItem key={table.id}>
-            <TableCard hoverable={!isTableLoading(table, database)}>
+            <TableCard hoverable={!isSyncInProgress(table)}>
               <TableLink
                 to={
-                  !isTableLoading(table, database)
-                    ? getTableUrl(table, metadata)
-                    : ""
+                  !isSyncInProgress(table) ? getTableUrl(table, metadata) : ""
                 }
                 data-metabase-event={`${ANALYTICS_CONTEXT};Table Click`}
               >
@@ -90,7 +88,7 @@ const itemPropTypes = {
 
 const TableBrowserItem = ({ database, table, dbId, xraysEnabled }) => {
   const isVirtual = isVirtualCardId(table.id);
-  const isLoading = isTableLoading(table, database);
+  const isLoading = isSyncInProgress(table);
 
   return (
     <EntityItem
@@ -143,10 +141,6 @@ const TableBrowserItemButtons = ({ tableId, dbId, xraysEnabled }) => {
 
 TableBrowserItemButtons.propTypes = itemButtonsPropTypes;
 
-const isTableLoading = (table, database) => {
-  return database && isSyncInProgress(database) && isSyncInProgress(table);
-};
-
 const getDatabaseCrumbs = dbId => {
   if (dbId === SAVED_QUESTIONS_VIRTUAL_DB_ID) {
     return {

frontend/src/metabase/browse/components/TableBrowser/TableBrowser.unit.spec.js

@@ -24,28 +24,31 @@ describe("TableBrowser", () => {
     expect(screen.queryByTestId("loading-spinner")).not.toBeInTheDocument();
   });
 
-  it("should render syncing tables", () => {
-    const database = getDatabase({
-      initial_sync_status: "incomplete",
-    });
-
-    const tables = [
-      getTable({ id: 1, name: "Orders", initial_sync_status: "incomplete" }),
-    ];
-
-    render(
-      <TableBrowser
-        database={database}
-        tables={tables}
-        getTableUrl={getTableUrl}
-        xraysEnabled={true}
-      />,
-    );
-
-    expect(screen.getByText("Orders")).toBeInTheDocument();
-    expect(screen.queryByLabelText("bolt_filled icon")).not.toBeInTheDocument();
-    expect(screen.getByTestId("loading-spinner")).toBeInTheDocument();
-  });
+  it.each(["incomplete", "complete"])(
+    "should render syncing tables, regardless of the database's initial_sync_status",
+    initial_sync_status => {
+      const database = getDatabase({ initial_sync_status });
+
+      const tables = [
+        getTable({ id: 1, name: "Orders", initial_sync_status: "incomplete" }),
+      ];
+
+      render(
+        <TableBrowser
+          database={database}
+          tables={tables}
+          getTableUrl={getTableUrl}
+          xraysEnabled={true}
+        />,
+      );
+
+      expect(screen.getByText("Orders")).toBeInTheDocument();
+      expect(
+        screen.queryByLabelText("bolt_filled icon"),
+      ).not.toBeInTheDocument();
+      expect(screen.getByTestId("loading-spinner")).toBeInTheDocument();
+    },
+  );
 
   it("should render tables with a sync error", () => {
     const database = getDatabase({

frontend/src/metabase/browse/containers/TableBrowser/TableBrowser.jsx

@@ -31,12 +31,8 @@ const getSchemaName = props => {
   return props.schemaName || props.params.schemaName;
 };
 
-const getReloadInterval = (state, { database }, tables = []) => {
-  if (
-    database &&
-    isSyncInProgress(database) &&
-    tables.some(t => isSyncInProgress(t))
-  ) {
+const getReloadInterval = (state, _props, tables = []) => {
+  if (tables.some(t => isSyncInProgress(t))) {
     return RELOAD_INTERVAL;
   } else {
     return 0;

src/metabase/models/table.clj

@@ -57,9 +57,8 @@
 
 (t2/define-before-insert :model/Table
   [table]
-  (let [defaults {:display_name        (humanization/name->human-readable-name (:name table))
-                  :field_order         (driver/default-field-order (t2/select-one-fn :engine Database :id (:db_id table)))
-                  :initial_sync_status "incomplete"}]
+  (let [defaults {:display_name (humanization/name->human-readable-name (:name table))
+                  :field_order  (driver/default-field-order (t2/select-one-fn :engine Database :id (:db_id table)))}]
     (merge defaults table)))
 
 (t2/define-before-delete :model/Table

src/metabase/sync/sync_metadata/tables.clj

@@ -96,25 +96,29 @@
 (defn create-or-reactivate-table!
   "Create a single new table in the database, or mark it as active if it already exists."
   [database {schema :schema, table-name :name, :as table}]
-  (if-let [existing-id (t2/select-one-pk Table
-                         :db_id (u/the-id database)
-                         :schema schema
-                         :name table-name
-                         :active false)]
-    ;; if the table already exists but is marked *inactive*, mark it as *active*
-    (t2/update! Table existing-id
-      {:active true})
-    ;; otherwise create a new Table
-    (let [is-crufty? (is-crufty-table? table)]
+  (let [;; if this is a crufty table, mark initial sync as complete since we'll skip the subsequent sync steps
+        is-crufty?          (is-crufty-table? table)
+        initial-sync-status (if is-crufty? "complete" "incomplete")
+        visibility-type     (when is-crufty? :cruft)]
+    (if-let [existing-id (t2/select-one-pk Table
+                                           :db_id (u/the-id database)
+                                           :schema schema
+                                           :name table-name
+                                           :active false)]
+      ;; if the table already exists but is marked *inactive*, mark it as *active*
+      (t2/update! Table existing-id
+                  {:active              true
+                   :visibility_type     visibility-type
+                   :initial_sync_status initial-sync-status})
+      ;; otherwise create a new Table
       (first (t2/insert-returning-instances! Table
                                              :db_id (u/the-id database)
                                              :schema schema
                                              :name table-name
                                              :display_name (humanization/name->human-readable-name table-name)
                                              :active true
-                                             :visibility_type (when is-crufty? :cruft)
-                                             ;; if this is a crufty table, mark initial sync as complete since we'll skip the subsequent sync steps
-                                             :initial_sync_status (if is-crufty? "complete" "incomplete"))))))
+                                             :visibility_type visibility-type
+                                             :initial_sync_status initial-sync-status)))))
 
 ;; TODO - should we make this logic case-insensitive like it is for fields?
 

test/metabase/api/search_test.clj

@@ -592,7 +592,7 @@
     :model               "table"
     :database_id         true
     :pk_ref              nil
-    :initial_sync_status "incomplete"}))
+    :initial_sync_status "complete"}))
 
 (defmacro ^:private do-test-users {:style/indent 1} [[user-binding users] & body]
   `(doseq [user# ~users

test/metabase/sync/util_test.clj

 (ns ^:mb/once metabase.sync.util-test
   "Tests for the utility functions shared by all parts of sync, such as the duplicate ops guard."
   (:require
+   [clojure.core.async :as a]
    [clojure.string :as str]
    [clojure.test :refer :all]
    [java-time :as t]
@@ -11,6 +12,7 @@
    [metabase.models.task-history :refer [TaskHistory]]
    [metabase.sync :as sync]
    [metabase.sync.sync-metadata :as sync-metadata]
+   [metabase.sync.sync-metadata.fields :as sync-fields]
    [metabase.sync.util :as sync-util]
    [metabase.test :as mt]
    [metabase.test.util :as tu]
@@ -299,3 +301,34 @@
             db (t2/select-one Database :id (:id (mt/db)))]
         (sync/sync-database! db)
         (is (= "complete" (t2/select-one-fn :initial_sync_status Database :id (:id db))))))))
+
+(deftest initial-sync-status-table-only-test
+  ;; Test that if a database is already completed sync'ing, then the sync is started again, it should initially be marked as
+  ;; incomplete, but then marked as complete after the sync is finished.
+  (mt/dataset sample-dataset
+    (testing "If `initial-sync-status` on a DB is already `complete`"
+      (let [[active-table inactive-table] (t2/select Table :db_id (mt/id))
+            get-active-table #(t2/select-one Table :id (:id active-table))
+            get-inactive-table #(t2/select-one Table :id (:id inactive-table))]
+        (t2/update! Table (:id active-table) {:initial_sync_status "complete" :active true})
+        (t2/update! Table (:id inactive-table) {:initial_sync_status "complete" :active false})
+        (let [syncing-chan   (a/chan)
+              completed-chan (a/chan)]
+          (let [sync-fields! sync-fields/sync-fields!]
+            (with-redefs [sync-fields/sync-fields! (fn [database]
+                                                     (a/>!! syncing-chan ::syncing)
+                                                     (sync-fields! database))]
+              (future
+                (sync/sync-database! (mt/db))
+                (a/>!! completed-chan ::sync-completed))
+              (a/<!! syncing-chan)
+              (testing "for existing tables initial_sync_status is complete while sync is running"
+                (is (= "complete"   (:initial_sync_status (get-active-table)))))
+              (testing "for new or previously inactive tables, initial_sync_status is incomplete while sync is running"
+                (is (= "incomplete" (:initial_sync_status (get-inactive-table)))))
+              (a/<!! completed-chan)
+              (testing "initial_sync_status is complete after the sync is finished"
+                (is (= "complete"   (:initial_sync_status (get-active-table))))
+                (is (= "complete"   (:initial_sync_status (get-inactive-table)))))))
+          (a/close! syncing-chan)
+          (a/close! completed-chan))))))