Tech debt issue from group 4 (#39347)

Co-authored-by: Adam James <adam.vermeer2@gmail.com>
Co-authored-by: Noah Moss <noahbmoss@gmail.com>

src/metabase/driver/sql/query_processor.clj

@@ -726,7 +726,7 @@
   [driver [_ & args]]
   (into [:*]
         (map (partial ->honeysql driver))
-         args))
+        args))
 
 ;; for division we want to go ahead and convert any integer args to floats, because something like field / 2 will do
 ;; integer division and give us something like 1.0 where we would rather see something like 1.5
@@ -763,7 +763,7 @@
                                                           mbql-expr)))]
     (into [:/ (->float driver numerator)]
           (map safe-denominator)
-           denominators)))
+          denominators)))
 
 (defmethod ->honeysql [:sql :sum-where]
   [driver [_ arg pred]]

test/metabase/driver/sql/query_processor_test.clj

@@ -408,6 +408,7 @@
                                      ORDERS.CREATED_AT                  AS CREATED_AT
                                      ABS (0)                            AS pivot-grouping
                                      ;; TODO -- I'm not sure if the order here is deterministic
+                                     ;; Tech debt issue: #39396
                                      PRODUCTS__via__PRODUCT_ID.CATEGORY AS PRODUCTS__via__PRODUCT_ID__CATEGORY
                                      PEOPLE__via__USER_ID.SOURCE        AS PEOPLE__via__USER_ID__SOURCE
                                      PRODUCTS__via__PRODUCT_ID.ID       AS PRODUCTS__via__PRODUCT_ID__ID
@@ -1079,6 +1080,7 @@
 
       ;; String containing semicolon followed by double dash followed by THE _comment or semicolon or end of input_.
       ;; TODO: Enable when better sql parsing solution is found in the [[sql.qp/make-nestable-sql]]].
+      ;; Tech debt issue: #39401
       #_#_
       "SELECT 'string with \n ; -- ending on the same line';"
       "(SELECT 'string with \n ; -- ending on the same line')"

test/metabase/driver/util_test.clj

@@ -7,7 +7,6 @@
    [metabase.driver.util :as driver.u]
    [metabase.lib.test-metadata :as meta]
    [metabase.lib.test-util :as lib.tu]
-   [metabase.public-settings.premium-features :as premium-features]
    [metabase.query-processor.store :as qp.store]
    [metabase.test :as mt]
    [metabase.test.fixtures :as fixtures])
@@ -145,8 +144,7 @@
                                       :visible-if           {:use-keystore true}}]
                                     true]]]
       (testing (str " with is-hosted? " is-hosted?)
-        ;; TODO: create capability to temporarily override token-features for testing
-        (with-redefs [premium-features/is-hosted? (constantly is-hosted?)]
+        (mt/with-premium-features (if is-hosted? #{:hosting} #{})
           (let [client-conn-props (-> (driver.u/available-drivers-info) ; this calls connection-props-server->client
                                       :secret-test-driver
                                       :details-fields)]

test/metabase/events/audit_log_test.clj

@@ -436,7 +436,6 @@
 
 (deftest user-joined-event-test
   (testing :user-joined
-    ;; TODO - what's the difference between `user-login` / `user-joined`?
     (is (= {:user-id (mt/user->id :rasta)}
            (events/publish-event! :event/user-joined {:user-id (mt/user->id :rasta)})))
     (is (= {:topic       :user-joined

test/metabase/lib/drill_thru/column_filter_test.cljc

@@ -263,26 +263,27 @@
       (is (some? (:column colfilter))))))
 
 ;; TODO: Bring back this test. It doesn't work in CLJ due to the inconsistencies noted in #38558.
-#_(deftest ^:parallel leaky-model-ref-test
-  (testing "input `:column-ref` must be used for the drill, in case a model leaks metadata like `:join-alias` (#38034)"
-    (let [query      (lib/query lib.tu/metadata-provider-with-mock-cards (lib.tu/mock-cards :model/products-and-reviews))
-          retcols    (lib/returned-columns query)
-          by-id      (m/index-by :id retcols)
-          reviews-id (by-id (meta/id :reviews :id))
-          _ (is (some? reviews-id))
-          context    {:column reviews-id
-                      :value  nil
-                      :column-ref (-> reviews-id
-                                      lib/ref
-                                      ((fn [r] (prn r) r))
-                                      (lib.options/update-options select-keys [:lib/uuid :base-type]))}
-          drills     (lib/available-drill-thrus query -1 context)]
-      (lib.drill-thru.tu/test-returns-drill
-        {:drill-type   :drill-thru/column-filter
-         :click-type   :header
-         :query-type   :unaggregated
-         :column-name  "ID_2"
-         :custom-query query
-         :expected     {:type       :drill-thru/column-filter
-                        :initial-op {:short :=}
-                        :column     {:lib/type :metadata/column}}}))))
+#_
+(deftest ^:parallel leaky-model-ref-test
+ (testing "input `:column-ref` must be used for the drill, in case a model leaks metadata like `:join-alias` (#38034)"
+   (let [query      (lib/query lib.tu/metadata-provider-with-mock-cards (lib.tu/mock-cards :model/products-and-reviews))
+         retcols    (lib/returned-columns query)
+         by-id      (m/index-by :id retcols)
+         reviews-id (by-id (meta/id :reviews :id))
+         _ (is (some? reviews-id))
+         context    {:column reviews-id
+                     :value  nil
+                     :column-ref (-> reviews-id
+                                     lib/ref
+                                     ((fn [r] (prn r) r))
+                                     (lib.options/update-options select-keys [:lib/uuid :base-type]))}
+         drills     (lib/available-drill-thrus query -1 context)]
+     (lib.drill-thru.tu/test-returns-drill
+       {:drill-type   :drill-thru/column-filter
+        :click-type   :header
+        :query-type   :unaggregated
+        :column-name  "ID_2"
+        :custom-query query
+        :expected     {:type       :drill-thru/column-filter
+                       :initial-op {:short :=}
+                       :column     {:lib/type :metadata/column}}}))))

test/metabase/lib/drill_thru/fk_details_test.cljc

@@ -187,6 +187,7 @@
                             :column          (m/find-first #(= (:name %) "PRODUCT_ID") (lib/returned-columns query))
                             :object-id       venue-id
                             ;; TODO: This field actually refers to the source table, not the target one. Is that right?
+                            ;; Tech Debt Issue: #39409
                             :many-pks?       false}
            :expected-query {:stages [{:filters [[:= {} [:field {} (meta/id :checkins :venue-id)] venue-id]]}]}}))
 

test/metabase/lib/drill_thru/summarize_column_by_time_test.cljc

@@ -78,3 +78,4 @@
 ;; TODO: Bring the fingerprint-based unit selection logic from
 ;; https://github.com/metabase/metabase/blob/0624d8d0933f577cc70c03948f4b57f73fe13ada/frontend/src/metabase-lib/metadata/Field.ts#L397
 ;; into this drill. Currently it always chooses the default date unit of months.
+;; Tech Debt Issue: #39382

test/metabase/lib/drill_thru/underlying_records_test.cljc

@@ -31,6 +31,7 @@
         ;;   contains all the breakouts (not exactly 2 as claimed in the spec).
         ;; That all makes sense to me (Braden) and I think this is a bug in the docs, but it also might be a bug in the FE
         ;; code that should be setting the aggregation :value for cell clicks?
+        ;; Tech debt issue: #39380
         (and (#{:cell #_:pivot :legend} click)
              (seq (:dimensions context)))))))
 

test/metabase/lib/drill_thru_test.cljc

@@ -576,7 +576,9 @@
         (test-drill-applications query context)))))
 
 ;; TODO: Restore this test once zoom-in and underlying-records are checked properly.
-#_(deftest ^:parallel histogram-available-drill-thrus-test
+;; Tech debt issue: #39373
+#_
+(deftest ^:parallel histogram-available-drill-thrus-test
   (testing "histogram breakout view"
     (testing "broken out by state - click a state - underlying, zoom in, pivot (non-location), automatic insights, quick filter"
       (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :people))

test/metabase/lib/expression_test.cljc

@@ -104,7 +104,7 @@
                                  :lib/expression-name "prev_month"}
                                 (lib.tu/field-clause :users :last-login)
                                 [:interval {:lib/uuid (str (random-uuid))} -1 :month]]]
-                :fields      [[:expression {:base-type :type/DateTime, :lib/uuid (str (random-uuid))} "prev_month"]]})]
+                 :fields      [[:expression {:base-type :type/DateTime, :lib/uuid (str (random-uuid))} "prev_month"]]})]
     (is (=? [{:name         "prev_month"
               :display-name "prev_month"
               :base-type    :type/DateTime
@@ -351,7 +351,7 @@
           rating (m/find-first #(= (:id %) (meta/id :products :rating)) cols)
           query  (lib/expression base "bad_product" (lib/< rating 3))
           join   (first (lib/joins query))]
-      ;; TODO: There should probably be a (lib/join-alias join) ;=> "Products" function.
+      ;; TODO: There should probably be a (lib/join-alias join) ;=> "Products" function. (#39368)
       (is (=? [[:< {:lib/expression-name "bad_product"}
                 [:field {:join-alias (:alias join)} (meta/id :products :rating)]
                 3]]

test/metabase/lib/native_test.cljc

@@ -28,6 +28,7 @@
     #{"snippet:   foo  *#&@"} "SELECT * FROM table WHERE {{snippet:   foo  *#&@}}"
     ;; TODO: This logic should trim the whitespace and unify these two snippet names.
     ;; I think this is a bug in the original code but am aiming to reproduce it exactly for now.
+    ;; Tech debt issue: #39378
     #{"snippet: foo" "snippet:foo"} "SELECT * FROM table WHERE {{snippet: foo}} AND {{snippet:foo}}"))
 
 (deftest ^:parallel card-tag-test
@@ -35,6 +36,7 @@
     #{"#123"} "SELECT * FROM table WHERE {{ #123 }} AND some_field IS NOT NULL"
     ;; TODO: This logic should trim the whitespace and unify these two card tags.
     ;; I think this is a bug in the original code but am aiming to reproduce it exactly for now.
+    ;; Tech debt issue: #39378
     #{"#123" "#123-with-slug"} "SELECT * FROM table WHERE {{ #123 }} AND {{  #123-with-slug  }}"
     #{"#123"} "SELECT * FROM table WHERE {{ #not-this }} AND {{#123}}"
     #{} "{{ #123foo }}"))
@@ -55,6 +57,7 @@
                              :snippet-name "foo"
                              :id           string?}}
             ;; TODO: This should probably be considered a bug - whitespace matters for the name.
+            ;; Tech debt issue: #39378
             (lib.native/extract-template-tags "SELECT * FROM {{snippet: foo}} WHERE {{snippet:foo}}"))))
 
   (testing "renaming a variable"
@@ -273,7 +276,7 @@
           (lib/native-query (lib.tu/mock-metadata-provider
                               meta/metadata-provider
                               {:database (merge (lib.metadata/database meta/metadata-provider) {:native-permissions :write})})
-                                                    "select * from x;")))
+                            "select * from x;")))
     (is (not (lib/has-write-permission
                (lib/native-query (lib.tu/mock-metadata-provider
                                    meta/metadata-provider

test/metabase/lib/query_test.cljc

@@ -92,6 +92,7 @@
                                                 [:expression
                                                  {}
                                                  ;; TODO Fill these in?
+                                                 ;; tech debt issue: #39376
                                                  #_{:base-type :type/Integer}
                                                  "CC"]]]}]}]}
 
@@ -106,21 +107,21 @@
                   (lib/aggregate (lib/count)))]
     (is (=? {:stages [{:lib/stage-metadata {:columns [{:field-ref [:expression "BirthMonth" {:base-type :type/Integer}]} {}]}} {}]}
           (lib/query meta/metadata-provider (assoc-in (lib.convert/->pMBQL (lib.convert/->legacy-MBQL query))
-                                                        [:stages 0 :lib/stage-metadata]
-                                                        {:columns [{:base-type :type/Float,
-                                                                    :display-name "BirthMonth",
-                                                                    :field-ref [:expression
-                                                                                "BirthMonth"
-                                                                                {:base-type :type/Integer}],
-                                                                    :name "BirthMonth",
-                                                                    :lib/type :metadata/column}
-                                                                   {:base-type :type/Integer,
-                                                                    :display-name "Count",
-                                                                    :field-ref [:aggregation 0],
-                                                                    :name "count",
-                                                                    :semantic-type :type/Quantity,
-                                                                    :lib/type :metadata/column}],
-                                                         :lib/type :metadata/results}))))))
+                                                      [:stages 0 :lib/stage-metadata]
+                                                      {:columns [{:base-type :type/Float,
+                                                                  :display-name "BirthMonth",
+                                                                  :field-ref [:expression
+                                                                              "BirthMonth"
+                                                                              {:base-type :type/Integer}],
+                                                                  :name "BirthMonth",
+                                                                  :lib/type :metadata/column}
+                                                                 {:base-type :type/Integer,
+                                                                  :display-name "Count",
+                                                                  :field-ref [:aggregation 0],
+                                                                  :name "count",
+                                                                  :semantic-type :type/Quantity,
+                                                                  :lib/type :metadata/column}],
+                                                       :lib/type :metadata/results}))))))
 
 (deftest ^:parallel stage-count-test
   (is (= 1 (lib/stage-count lib.tu/venues-query)))

test/metabase/test/data/datasets.clj

@@ -11,7 +11,8 @@
   `DRIVERS` env var.
 
   TODO - this namespace name really doesn't make a lot of sense. How about `metabase.test.driver` or something like
-  that?"
+  that?
+  Tech debt issue: #39348"
   (:require
    [clojure.test :as t]
    [colorize.core :as colorize]

test/metabase/test/data/interface.clj

@@ -4,7 +4,8 @@
   Drivers with test extensions know how to load a `DatabaseDefinition` into an actual physical database. This
   functionality allows us to easily test with multiple datasets.
 
-  TODO - We should rename this namespace to `metabase.driver.test-extensions` or something like that."
+  TODO - We should rename this namespace to `metabase.driver.test-extensions` or something like that.
+  Tech debt issue: #39363"
   (:require
    [clojure.string :as str]
    [clojure.tools.reader.edn :as edn]
@@ -91,6 +92,7 @@
    (ms/InstanceOfClass DatabaseDefinition)])
 
 ;; TODO - this should probably be a protocol instead
+;; Tech debt issue: #39350
 (defmulti ^DatabaseDefinition get-dataset-definition
   "Return a definition of a dataset, so a test database can be created from it. Returns a map matching
   the [[ValidDatabaseDefinition]] schema."
@@ -466,6 +468,7 @@
    [:sequential [:sequential :any]]])
 
 ;; TODO - not sure everything below belongs in this namespace
+;; Tech debt issue: #39363
 
 (mu/defn ^:private dataset-field-definition :- ValidFieldDefinition
   "Parse a Field definition (from a `defdatset` form or EDN file) and return a FieldDefinition instance for
@@ -617,6 +620,7 @@
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
 ;; TODO - maybe this should go in a different namespace
+;; Tech debt issue: #39363
 
 (mu/defn ^:private tabledef-with-name :- ValidTableDefinition
   "Return `TableDefinition` with `table-name` in `dbdef`."

test/metabase/test/data/mysql.clj

@@ -52,6 +52,7 @@
       {:base_type :type/Decimal}))))
 
 ;; TODO - we might be able to do SQL all at once by setting `allowMultiQueries=true` on the connection string
+;; Tech debt issue: #39343
 (defmethod execute/execute-sql! :mysql
   [& args]
   (apply execute/sequentially-execute-sql! args))

test/metabase/test/data/sql.clj

@@ -35,6 +35,7 @@
 (defmethod pk-field-name :sql/test-extensions [_] "id")
 
 ;; TODO - WHAT ABOUT SCHEMA NAME???
+;; Tech debt issue - #39356
 (defmulti qualified-name-components
   "Return a vector of String names that can be used to refer to a Database, Table, or Field. This is provided so drivers
   have the opportunity to inject things like schema names or even modify the names themselves.

test/metabase/test/data/sql_jdbc/load_data.clj

@@ -192,6 +192,7 @@
         (log/tracef "[insert] %s" (pr-str sql-args))
         (try
           ;; TODO - why don't we use [[execute/execute-sql!]] here like we do below?
+          ;; Tech Debt Issue: #39375
           (jdbc/execute! spec sql-args {:set-parameters (fn [stmt params]
                                                           (sql-jdbc.execute/set-parameters! driver stmt params))})
           (catch Throwable e

test/metabase/test/mock/moviedb.clj

@@ -5,6 +5,7 @@
 
 ;; TODO - this whole fake driver is used in exactly one test. Can definitely remove a lot of the stuff here since it's
 ;; not used.
+;; Tech debt issue: #39338
 
 (def ^:private moviedb-tables
   {"movies"

test/metabase/test/util.clj

@@ -233,6 +233,7 @@
               :table_id    (data/id :checkins)}))
 
    ;; TODO - `with-temp` doesn't return `Sessions`, probably because their ID is a string?
+   ;; Tech debt issue: #39329
 
    :model/Table
    (fn [_] (default-timestamped
@@ -278,9 +279,6 @@
 
 (defn- set-with-temp-defaults! []
   (doseq [[model defaults-fn] with-temp-defaults-fns]
-    ;; TODO -- we shouldn't need to ignore this, but it's a product of the custom hook defined for Methodical
-    ;; `defmethod`. Fix the hook upstream
-    #_{:clj-kondo/ignore [:redundant-fn-wrapper]}
     (methodical/defmethod t2.with-temp/with-temp-defaults model
       [model]
       (defaults-fn model))))
@@ -789,7 +787,6 @@
                          :moderated_item_id card-id
                          :moderated_item_type "card"))))))
 
-;; TODO - not 100% sure I understand
 (defn call-with-paused-query
   "This is a function to make testing query cancellation eaiser as it can be complex handling the multiple threads
   needed to orchestrate a query cancellation.

test/metabase/test/util/log.clj

@@ -237,6 +237,7 @@
            (logs#)))))))
 
 ;; TODO -- this macro should probably just take a binding for the `logs` function so you can eval when needed
+;; Tech debt issue: #39335
 (defmacro with-log-messages-for-level [ns+level & body]
   (macros/case
     :clj  `(with-log-messages-for-level-clj ~ns+level ~@body)