Fix `:convert-timezone` and `:get-week` schemas (#31054)

* Fix `:convert-timezone` and `:get-week` schemas

* Make sure moment-timezone stuff gets loaded in Cljs

src/metabase/lib/schema/common.cljc

@@ -10,7 +10,9 @@
 (mr/def ::non-blank-string
   [:and
    [:string {:min 1}]
-   [:fn (complement str/blank?)]])
+   [:fn
+    {:error/message "non-blank string"}
+    (complement str/blank?)]])
 
 ;;; Schema representing an integer than must also be greater than or equal to zero.
 (mr/def ::int-greater-than-or-equal-to-zero

src/metabase/lib/schema/expression/temporal.cljc

@@ -7,7 +7,19 @@
    [metabase.lib.schema.literal :as literal]
    [metabase.lib.schema.mbql-clause :as mbql-clause]
    [metabase.lib.schema.temporal-bucketing :as temporal-bucketing]
-   [metabase.util.malli.registry :as mr]))
+   [metabase.util.malli.registry :as mr])
+  #?@
+  (:clj
+   [(:import
+     (java.time ZoneId))]
+   :cljs
+   [(:require
+     ["moment" :as moment]
+     ["moment-timezone" :as mtz])]))
+
+#?(:cljs
+   ;; so the moment-timezone stuff gets loaded
+   (comment mtz/keep-me))
 
 (mbql-clause/define-tuple-mbql-clause :interval :- :type/Interval
   :int
@@ -34,19 +46,35 @@
                               :get-day :get-day-of-week
                               :get-month :get-quarter :get-year}]
   (mbql-clause/define-tuple-mbql-clause temporal-extract-op :- :type/Integer
-  #_:datetime [:schema [:ref ::expression/temporal]]))
-
-(mbql-clause/define-tuple-mbql-clause :get-week :- :type/Integer
-  #_:datetime [:schema [:ref ::expression/temporal]]
-  ;; TODO should this be in the options map?
-  #_:mode [:maybe [:enum :iso :us :instance]])
-
-(mbql-clause/define-tuple-mbql-clause :convert-timezone
-  #_:datetime [:schema [:ref ::expression/temporal]]
-  ;; TODO could be better specified - perhaps with a build time macro to inline the timezones?
-  ;; NOT expressions?
-  #_:target [:string]
-  #_:source [:maybe [:string]])
+    #_:datetime [:schema [:ref ::expression/temporal]]))
+
+(mr/def ::get-week-mode
+  [:enum :iso :us :instance])
+
+(mbql-clause/define-catn-mbql-clause :get-week :- :type/Integer
+  [:datetime [:schema [:ref ::expression/temporal]]]
+  ;; TODO : the mode should probably go in the options map in modern MBQL rather than have it be a separate positional
+  ;; argument. But we can't refactor everything in one go, so that will have to be a future refactor.
+  [:mode     [:? [:schema [:ref ::get-week-mode]]]])
+
+(mr/def ::timezone-id
+  [:and
+
+   ::common/non-blank-string
+   (into [:enum
+          {:error/message "valid timezone ID"
+           :error/fn      (fn [{:keys [value]} _]
+                            (str "invalid timezone ID: " (pr-str value)))}]
+         (sort
+          #?( ;; 600 timezones on java 17
+             :clj (ZoneId/getAvailableZoneIds)
+             ;; 596 timezones on moment-timezone 0.5.38
+             :cljs (.names (.-tz moment)))))])
+
+(mbql-clause/define-catn-mbql-clause :convert-timezone
+  [:datetime [:schema [:ref ::expression/temporal]]]
+  [:target   [:schema [:ref ::timezone-id]]]
+  [:source   [:? [:schema [:ref ::timezone-id]]]])
 
 (lib.hierarchy/derive :convert-timezone :lib.type-of/type-is-type-of-first-arg)
 

test/metabase/lib/schema/expression/temporal_test.cljc

@@ -4,7 +4,8 @@
    [malli.core :as mc]
    [malli.error :as me]
    [metabase.lib.schema]
-   [metabase.lib.schema.expression :as expression]))
+   [metabase.lib.schema.expression :as expression]
+   [metabase.lib.schema.expression.temporal :as temporal]))
 
 (comment metabase.lib.schema/keep-me)
 
@@ -68,3 +69,64 @@
     [:relative-datetime {:lib/uuid "00000000-0000-0000-0000-000000000000"} :current :day]
     [:relative-datetime {:lib/uuid "00000000-0000-0000-0000-000000000000"} :current :minute]
     [:relative-datetime {:lib/uuid "00000000-0000-0000-0000-000000000000"} :current]))
+
+(deftest ^:parallel timezone-id-test
+  (are [input error] (= error
+                        (me/humanize (mc/explain ::temporal/timezone-id input)))
+    "US/Pacific"  nil
+    "US/Specific" ["invalid timezone ID: \"US/Specific\""]
+    ""            ["should be at least 1 characters" "non-blank string" "invalid timezone ID: \"\""]
+    "  "          ["non-blank string" "invalid timezone ID: \"  \""]
+    nil           ["should be a string" "non-blank string" "invalid timezone ID: nil"]))
+
+(deftest ^:parallel convert-timezone-test
+  (are [clause error] (= error
+                         (me/humanize (mc/explain :mbql.clause/convert-timezone clause)))
+    ;; with both target and source timezone
+    [:convert-timezone
+     {:lib/uuid "00000000-0000-0000-0000-000000000000"}
+     [:field {:lib/uuid "00000000-0000-0000-0000-000000000000"} 1]
+     "Asia/Seoul"
+     "US/Pacific"]
+    nil
+
+    ;; with just the target timezone
+    [:convert-timezone
+     {:lib/uuid "00000000-0000-0000-0000-000000000000"}
+     [:field {:lib/uuid "00000000-0000-0000-0000-000000000000"} 1]
+     "Asia/Seoul"]
+    nil
+
+    ;; source cannot be nil
+    [:convert-timezone
+     {:lib/uuid "00000000-0000-0000-0000-000000000000"}
+     [:field {:lib/uuid "00000000-0000-0000-0000-000000000000"} 1]
+     "Asia/Seoul"
+     nil]
+    [nil nil nil nil ["should be a string" "non-blank string" "invalid timezone ID: nil" "Valid :convert-timezone clause"]]
+
+    ;; invalid timezone ID
+    [:convert-timezone
+     {:lib/uuid "00000000-0000-0000-0000-000000000000"}
+     [:field {:lib/uuid "00000000-0000-0000-0000-000000000000"} 1]
+     "US/Specific"]
+    [nil nil nil ["invalid timezone ID: \"US/Specific\""]]))
+
+(deftest ^:parallel get-week-test
+  (are [clause error] (= error
+                         (me/humanize (mc/explain :mbql.clause/get-week clause)))
+    ;; without mode
+    [:get-week {:lib/uuid "00000000-0000-0000-0000-000000000000"} "2023-05-25"]
+    nil
+
+    ;; with mode
+    [:get-week {:lib/uuid "00000000-0000-0000-0000-000000000000"} "2023-05-25" :iso]
+    nil
+
+    ;; invalid mode
+    [:get-week {:lib/uuid "00000000-0000-0000-0000-000000000000"} "2023-05-25" :isolation]
+    [nil nil nil ["should be either :iso, :us or :instance" "Valid :get-week clause"]]
+
+    ;; mode is not allowed to be nil
+    [:get-week {:lib/uuid "00000000-0000-0000-0000-000000000000"} "2023-05-25" nil]
+    [nil nil nil ["should be either :iso, :us or :instance" "Valid :get-week clause"]]))

test/metabase/query_processor_test/test_mlv2.clj

@@ -29,16 +29,6 @@
   place, [[metabase.models.query.permissions-test/invalid-queries-test]]."
   false)
 
-(defn- skip-conversion-tests?
-  "Whether to skip conversion tests against a `legacy-query`."
-  [legacy-query]
-  (or
-   *skip-conversion-tests*
-   ;; #29958: `:convert-timezone` with 2 args is broken
-   (mbql.u/match-one legacy-query
-     [:convert-timezone _expr _source-timezone]
-     "#29958")))
-
 (defn- skip-metadata-calculation-tests? [legacy-query]
   (or
    ;; #29907: wrong column name for joined columns in `:breakout`
@@ -62,7 +52,7 @@
 
 (defn- test-mlv2-metadata [original-query _qp-metadata]
   {:pre [(map? original-query)]}
-  (when-not (or (skip-conversion-tests? original-query)
+  (when-not (or *skip-conversion-tests*
                 (skip-metadata-calculation-tests? original-query))
     (do-with-legacy-query-testing-context
      original-query
@@ -82,7 +72,7 @@
               (is (any? mlv2-metadata)))))))))))
 
 (defn- test-mlv2-conversion [query]
-  (when-not (skip-conversion-tests? query)
+  (when-not *skip-conversion-tests*
     (do-with-legacy-query-testing-context
      query
      (^:once fn* []