Add test for scheduled sync tasks

resources/sample-content.edn

@@ -51,6 +51,42 @@
    :created_at #t "2024-04-11T12:41:25.429317Z",
    :public_uuid nil,
    :points_of_interest nil}),
+ :data_permissions
+ ({:id 1,
+   :group_id 1,
+   :perm_type "perms/view-data",
+   :db_id 1,
+   :schema_name nil,
+   :table_id nil,
+   :perm_value "unrestricted"}
+  {:id 2,
+   :group_id 1,
+   :perm_type "perms/create-queries",
+   :db_id 1,
+   :schema_name nil,
+   :table_id nil,
+   :perm_value "query-builder-and-native"}
+  {:id 3,
+   :group_id 1,
+   :perm_type "perms/download-results",
+   :db_id 1,
+   :schema_name nil,
+   :table_id nil,
+   :perm_value "one-million-rows"}
+  {:id 4,
+   :group_id 1,
+   :perm_type "perms/manage-table-metadata",
+   :db_id 1,
+   :schema_name nil,
+   :table_id nil,
+   :perm_value "no"}
+  {:id 5,
+   :group_id 1,
+   :perm_type "perms/manage-database",
+   :db_id 1,
+   :schema_name nil,
+   :table_id nil,
+   :perm_value "no"}),
  :metabase_table
  ({:description
    "Piespace does some anonymous analytics tracking on how users interact with their platform. They’ve only had time to implement a few events, but you know how it is. Pies come first.",

src/metabase/db/custom_migrations.clj

@@ -1199,12 +1199,15 @@
                                   :report_dashboard
                                   :dashboard_tab
                                   :report_dashboardcard
-                                  :dashboardcard_series]]
+                                  :dashboardcard_series
+                                  :permissions_group
+                                  :data_permissions]]
                 (when-let [values (seq (table-name->rows table-name))]
                   (t2/query {:insert-into table-name :values values})))
-              (t2/query {:insert-into :permissions
-                         :values      [{:object   (format "/collection/%s/" example-collection-id)
-                                        :group_id (:id (t2/query-one {:select :id :from :permissions_group :where [:= :name "All Users"]}))}]})
+              (let [group-id (:id (t2/query-one {:select :id :from :permissions_group :where [:= :name "All Users"]}))]
+                (t2/query {:insert-into :permissions
+                           :values      [{:object   (format "/collection/%s/" example-collection-id)
+                                          :group_id group-id}]}))
               (t2/query {:insert-into :setting
                          :values      [{:key   "example-dashboard-id"
                                         :value (str example-dashboard-id)}]})))))))
@@ -1212,7 +1215,7 @@
 (comment
   ;; How to create `resources/sample-content.edn` used in `CreateSampleContent`
   ;; -----------------------------------------------------------------------------
-  ;; Start a fresh metabase instance without the :ee alias
+  ;; Start a fresh metabase instance without the :ee alias so instance analytics stuff is not created.
   ;; 1. create a collection with dashboards, or import one with (metabase.cmd/import "<path>")
   ;; 2. execute the following to spit out the collection to an EDN file:
   (let [pretty-spit (fn [file-name data]
@@ -1230,7 +1233,8 @@
                                      :report_card
                                      :parameter_card
                                      :dashboard_tab
-                                     :dashboardcard_series]
+                                     :dashboardcard_series
+                                     :data_permissions]
                          :let [query (cond-> {:select [:*] :from table-name}
                                        (= table-name :collection) (assoc :where [:and
                                                                                  [:= :namespace nil] ; excludes the analytics namespace

test/metabase/api/database_test.clj

@@ -28,6 +28,7 @@
    [metabase.sync.sync-metadata :as sync-metadata]
    [metabase.task :as task]
    [metabase.task.sync-databases :as task.sync-databases]
+   [metabase.task.sync-databases-test :as task.sync-databases-test]
    [metabase.test :as mt]
    [metabase.test.data.impl :as data.impl]
    [metabase.test.data.interface :as tx]
@@ -43,7 +44,7 @@
    [toucan2.tools.with-temp :as t2.with-temp])
   (:import
    (java.sql Connection)
-   (org.quartz TriggerKey)))
+   (org.quartz JobDetail TriggerKey)))
 
 (set! *warn-on-reflection* true)
 
@@ -269,26 +270,6 @@
 
 (def ^:private monthly-schedule {:schedule_type "monthly" :schedule_day "fri" :schedule_frame "last"})
 
-(defn- all-db-sync-triggers-name
-  "Returns the name of trigger for DB.
-  This is all trigger names that a DB SHOULD have."
-  [db]
-  (set (map #(.getName ^TriggerKey (#'task.sync-databases/trigger-key (t2/instance :model/Database db) %))
-            @#'task.sync-databases/all-tasks)))
-
-(defn- query-all-db-sync-triggers-name
-  "Find the all triggers for DB \"db\".
-  This is all triger names that a DB HAVE in the scheduler."
-  [db]
-  (let [db (t2/instance :model/Database db)]
-    (assert (some? (#'task/scheduler)) "makes sure the scheduler is initialized!")
-    (->> (for [task-info @#'task.sync-databases/all-tasks]
-           (keep #(when (= (.getName ^TriggerKey (#'task.sync-databases/trigger-key db task-info)) (:key %))
-                    (:key %))
-                 (:triggers (task/job-info (#'task.sync-databases/job-key task-info)))))
-         flatten
-         set)))
-
 (defn- sync-and-analyze-trigger-name
   [db]
   (.getName ^TriggerKey (#'task.sync-databases/trigger-key db @#'task.sync-databases/sync-analyze-task-info)))
@@ -305,8 +286,8 @@
   `(mt/with-temp-scheduler
      (#'task.sync-databases/job-init)
      (u/prog1 ~@body
-       (qs/delete-job (#'task/scheduler) (.getKey @#'task.sync-databases/sync-analyze-job))
-       (qs/delete-job (#'task/scheduler) (.getKey @#'task.sync-databases/field-values-job)))))
+       (qs/delete-job (#'task/scheduler) (.getKey ^JobDetail @#'task.sync-databases/sync-analyze-job))
+       (qs/delete-job (#'task/scheduler) (.getKey ^JobDetail @#'task.sync-databases/field-values-job)))))
 
 (deftest create-db-default-schedule-test
   (testing "POST /api/database"
@@ -332,8 +313,8 @@
                           [:features   [:= (driver.u/features ::test-driver (mt/db))]]
                           [:creator_id [:= (mt/user->id :crowberto)]]]]
                         db))
-            (is (= (all-db-sync-triggers-name db)
-                   (query-all-db-sync-triggers-name db)))))))))
+            (is (= (task.sync-databases-test/all-db-sync-triggers-name db)
+                   (task.sync-databases-test/query-all-db-sync-triggers-name db)))))))))
 
 (deftest create-db-no-full-sync-test
   (testing "POST /api/database"
@@ -1127,8 +1108,8 @@
                    (:metadata_sync_schedule db)))
             (is (= (u.cron/schedule-map->cron-string schedule-map-for-last-friday-at-11pm)
                    (:cache_field_values_schedule db)))
-            (is (= (all-db-sync-triggers-name db)
-                   (query-all-db-sync-triggers-name db)))))))))
+            (is (= (task.sync-databases-test/all-db-sync-triggers-name db)
+                   (task.sync-databases-test/query-all-db-sync-triggers-name db)))))))))
 
 (deftest create-db-never-scan-field-values-test
   (testing "POST /api/database"
@@ -1149,7 +1130,7 @@
                    (:metadata_sync_schedule db)))
             (is (nil? (:cache_field_values_schedule db)))
             (is (= #{(sync-and-analyze-trigger-name db)}
-                   (query-all-db-sync-triggers-name db)))))))))
+                   (task.sync-databases-test/query-all-db-sync-triggers-name db)))))))))
 
 (deftest create-db-on-demand-scan-field-values-test
   (testing "POST /api/database"
@@ -1170,7 +1151,7 @@
                    (:metadata_sync_schedule db)))
             (is (nil? (:cache_field_values_schedule db)))
             (is (= #{(sync-and-analyze-trigger-name db)}
-                   (query-all-db-sync-triggers-name db)))))))))
+                   (task.sync-databases-test/query-all-db-sync-triggers-name db)))))))))
 
 (deftest update-db-to-sync-on-custom-schedule-test
   (with-db-scheduler-setup
@@ -1197,8 +1178,8 @@
                                                :cache_field_values schedule-map-for-last-friday-at-11pm}
                                  :is_full_sync true
                                  :is_on_demand false})
-          (is (= (all-db-sync-triggers-name db)
-                 (query-all-db-sync-triggers-name db)))
+          (is (= (task.sync-databases-test/all-db-sync-triggers-name db)
+                 (task.sync-databases-test/query-all-db-sync-triggers-name db)))
           (let [db (t2/select-one :model/Database (:id db))]
             (is (= (u.cron/schedule-map->cron-string schedule-map-for-weekly)
                    (:metadata_sync_schedule db)))
@@ -1213,7 +1194,7 @@
                                 :is_full_sync false
                                 :is_on_demand false})
          (is (= #{(sync-and-analyze-trigger-name db)}
-                (query-all-db-sync-triggers-name db)))
+                (task.sync-databases-test/query-all-db-sync-triggers-name db)))
          (let [db (t2/select-one :model/Database (:id db))]
            (is (= (u.cron/schedule-map->cron-string schedule-map-for-weekly)
                   (:metadata_sync_schedule db)))
@@ -1226,8 +1207,8 @@
                                               :cache_field_values schedule-map-for-last-friday-at-11pm}
                                 :is_full_sync true
                                 :is_on_demand false})
-         (is (= (all-db-sync-triggers-name db)
-                (query-all-db-sync-triggers-name db)))
+         (is (= (task.sync-databases-test/all-db-sync-triggers-name db)
+                (task.sync-databases-test/query-all-db-sync-triggers-name db)))
          (let [db (t2/select-one :model/Database (:id db))]
            ;; make sure the new schedule is randomized, not from the payload
            (is (not= (-> schedule-map-for-weekly u.cron/schedule-map->cron-string)
@@ -1242,8 +1223,8 @@
         [:model/Database db {}]
         (testing "update db setting to never scan should remove scan field values trigger"
           (testing "sanity check that it has all triggers to begin with"
-            (is (= (all-db-sync-triggers-name db)
-                   (query-all-db-sync-triggers-name db))))
+            (is (= (task.sync-databases-test/all-db-sync-triggers-name db)
+                   (task.sync-databases-test/query-all-db-sync-triggers-name db))))
           (mt/user-http-request :crowberto :put 200 (format "/database/%d" (:id db))
                                 {:details     {:let-user-control-scheduling true}
                                  :schedules   {:metadata_sync      schedule-map-for-weekly
@@ -1251,7 +1232,7 @@
                                  :is_full_sync false
                                  :is_on_demand false})
           (is (= #{(sync-and-analyze-trigger-name db)}
-                 (query-all-db-sync-triggers-name db)))
+                 (task.sync-databases-test/query-all-db-sync-triggers-name db)))
           (let [db (t2/select-one :model/Database (:id db))]
             (is (= (u.cron/schedule-map->cron-string schedule-map-for-weekly)
                    (:metadata_sync_schedule db)))
@@ -1270,7 +1251,7 @@
                                  :is_full_sync false
                                  :is_on_demand true})
           (is (= #{(sync-and-analyze-trigger-name db)}
-                 (query-all-db-sync-triggers-name db)))
+                 (task.sync-databases-test/query-all-db-sync-triggers-name db)))
           (let [db (t2/select-one :model/Database (:id db))]
             (is (= (u.cron/schedule-map->cron-string schedule-map-for-weekly)
                    (:metadata_sync_schedule db)))

test/metabase/db/custom_migrations_test.clj

@@ -24,6 +24,7 @@
    [metabase.models.setting :as setting]
    [metabase.native-query-analyzer :as query-analyzer]
    [metabase.task :as task]
+   [metabase.task.sync-databases-test :as task.sync-databases-test]
    [metabase.test :as mt]
    [metabase.test.fixtures :as fixtures]
    [metabase.util :as u]
@@ -1658,19 +1659,19 @@
           (doseq [db (concat db-with-scan-fv db-without-scan-fv)]
             (#'database/check-and-schedule-tasks-for-db! (t2/instance :model/Database db))
             (testing "sanity check that the schedule exists"
-              (is (= (#'api.database-test/all-db-sync-triggers-name db)
-                     (#'api.database-test/query-all-db-sync-triggers-name db)))))
+              (is (= (#'task.sync-databases-test/all-db-sync-triggers-name db)
+                     (#'task.sync-databases-test/query-all-db-sync-triggers-name db)))))
 
           (migrate!)
           (testing "default options and scan with manual schedules should have scan field values"
             (doseq [db db-with-scan-fv]
-              (is (= (#'api.database-test/all-db-sync-triggers-name db)
-                     (#'api.database-test/query-all-db-sync-triggers-name db)))))
+              (is (= (#'task.sync-databases-test/all-db-sync-triggers-name db)
+                     (#'task.sync-databases-test/query-all-db-sync-triggers-name db)))))
 
           (testing "never scan and on demand should not have scan field values"
             (doseq [db (t2/select :model/Database :id [:in (map :id db-without-scan-fv)])]
               (is (= #{(#'api.database-test/sync-and-analyze-trigger-name db)}
-                     (#'api.database-test/query-all-db-sync-triggers-name db)))
+                     (#'task.sync-databases-test/query-all-db-sync-triggers-name db)))
               (is (nil? (:cache_field_values_schedule db))))))))))
 
 (deftest backfill-query-field-test

test/metabase/sample_data_test.clj

@@ -5,11 +5,14 @@
    [clojure.java.jdbc :as jdbc]
    [clojure.string :as str]
    [clojure.test :refer :all]
+   [metabase.api.database-test :as api.database-test]
+   [metabase.db :as mdb]
    [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
    [metabase.models :refer [Database Field Table]]
    [metabase.plugins :as plugins]
    [metabase.sample-data :as sample-data]
    [metabase.sync :as sync]
+   [metabase.task.sync-databases-test :as task.sync-databases-test]
    [metabase.test :as mt]
    [metabase.util :as u]
    [metabase.util.files :as u.files]
@@ -178,3 +181,18 @@
              (testing "drop table"
                  (jdbc/execute! conn-spec "DROP TABLE NEW_TABLE;")
                  (is (not (contains? (get-tables) "NEW_TABLE"))))))))))))
+
+(deftest sample-database-schedule-sync-test
+  (testing "Check that the sample database has scheduled sync jobs, just like a newly created database"
+    (mt/with-temp-empty-app-db [_conn :h2]
+      (api.database-test/with-db-scheduler-setup
+        (mdb/setup-db! :create-sample-content? true)
+        (sample-data/extract-and-sync-sample-database!)
+        (testing "Sense check: a newly created database should have sync jobs scheduled"
+          (mt/with-temp [:model/Database db {}]
+            (is (= (task.sync-databases-test/all-db-sync-triggers-name db)
+                   (task.sync-databases-test/query-all-db-sync-triggers-name db)))))
+        (testing "The sample database should also have sync jobs scheduled"
+          (let [sample-db (t2/select-one :model/Database :is_sample true)]
+            (is (= (task.sync-databases-test/all-db-sync-triggers-name sample-db)
+                   (task.sync-databases-test/query-all-db-sync-triggers-name sample-db)))))))))

test/metabase/setup_test.clj

@@ -57,13 +57,7 @@
     (mdb/setup-db! :create-sample-content? true)
     (testing "The example-dashboard-id setting should be set if the example content is loaded"
       (is (= 1
-             (public-settings/example-dashboard-id))))
-    (testing "Rasta (as a member of 'All Users') should have sufficient privileges to edit the example content"
-      (mt/with-current-user (mt/user->id :rasta)
-        (let [dashboard (t2/select-one :model/Dashboard (public-settings/example-dashboard-id))
-              collection (t2/select-one :model/Collection (:collection_id dashboard))]
-          (mi/can-write? dashboard)
-          (mi/can-write? collection)))))
+             (public-settings/example-dashboard-id)))))
   (testing "The example-dashboard-id setting should be nil if the example content isn't loaded"
     (mt/with-temp-empty-app-db [_conn :h2]
       (mdb/setup-db! :create-sample-content? false)
@@ -75,3 +69,31 @@
              (public-settings/example-dashboard-id)))
       (t2/update! :model/Dashboard 1 {:archived true})
       (is (nil? (public-settings/example-dashboard-id))))))
+
+(deftest sample-content-permissions-test
+  (mt/with-temp-empty-app-db [_conn :h2]
+    (mdb/setup-db! :create-sample-content? true)
+    (let [dashboard  (t2/select-one :model/Dashboard :creator_id config/internal-mb-user-id)
+          collection (t2/select-one :model/Collection (:collection_id dashboard))
+          card       (t2/select-one :model/Card :creator_id config/internal-mb-user-id)]
+      (testing "Rasta (as a member of 'All Users') should have sufficient privileges to edit the example content"
+        (mt/with-current-user (mt/user->id :rasta)
+          (is (true? (mi/can-write? dashboard)))
+          (is (true? (mi/can-write? card)))
+          (is (true? (mi/can-write? collection))))))
+    (let [sample-db       (t2/select-one :model/Database :is_sample true)
+          sample-db-table (t2/select-one :model/Table :db_id (:id sample-db))
+          sample-db-field (t2/select-one :model/Field :table_id (:id sample-db-table))]
+      (testing "Rasta (as a member of 'All Users') should have read but not write privileges to the sample database"
+        (mt/with-current-user (mt/user->id :rasta)
+          (is (true? (mi/can-read? sample-db)))
+          (is (true? (mi/can-read? sample-db-table)))
+          (is (true? (mi/can-read? sample-db-field)))
+          (is (false? (mi/can-write? sample-db)))
+          (is (false? (mi/can-write? sample-db-table)))
+          (is (false? (mi/can-write? sample-db-field)))))
+      (testing "Crowberto (as an admin member of 'All Users') should have write privileges to the sample database"
+        (mt/with-current-user (mt/user->id :crowberto)
+          (is (true? (mi/can-write? sample-db)))
+          (is (true? (mi/can-write? sample-db-table)))
+          (is (true? (mi/can-write? sample-db-field))))))))

test/metabase/task/sync_databases_test.clj

@@ -9,13 +9,16 @@
    [java-time.api :as t]
    [metabase.models.database :refer [Database]]
    [metabase.sync.schedules :as sync.schedules]
+   [metabase.task :as task]
    [metabase.task.sync-databases :as task.sync-databases]
    [metabase.test :as mt]
    [metabase.test.util :as tu]
    [metabase.util :as u]
    [toucan2.core :as t2]
    [toucan2.tools.with-temp :as t2.with-temp])
-  (:import [metabase.task.sync_databases SyncAndAnalyzeDatabase UpdateFieldValues]))
+  (:import
+   (metabase.task.sync_databases SyncAndAnalyzeDatabase UpdateFieldValues)
+   (org.quartz TriggerKey)))
 
 (set! *warn-on-reflection* true)
 
@@ -38,6 +41,26 @@
                                             (update    :key  replace-trailing-id-with-<id>)
                                             (update-in [:data "db-id"] replace-trailing-id-with-<id>))))))))))
 
+(defn all-db-sync-triggers-name
+  "Returns the name of trigger for DB.
+  This is all trigger names that a DB SHOULD have."
+  [db]
+  (set (map #(.getName ^TriggerKey (#'task.sync-databases/trigger-key (t2/instance :model/Database db) %))
+            @#'task.sync-databases/all-tasks)))
+
+(defn query-all-db-sync-triggers-name
+  "Find the all triggers for DB \"db\".
+  This is all triger names that a DB HAVE in the scheduler."
+  [db]
+  (let [db (t2/instance :model/Database db)]
+    (assert (some? (#'task/scheduler)) "makes sure the scheduler is initialized!")
+    (->> (for [task-info @#'task.sync-databases/all-tasks]
+           (keep #(when (= (.getName ^TriggerKey (#'task.sync-databases/trigger-key db task-info)) (:key %))
+                    (:key %))
+                 (:triggers (task/job-info (#'task.sync-databases/job-key task-info)))))
+         flatten
+         set)))
+
 (defn- current-tasks-for-db [db-or-id]
   (replace-ids-with-<id>
    (for [job   (tu/scheduler-current-tasks)