Static Viz Formatting (#17957)

* Formatting

return a format string so it can be used over multiple rows. The rules
for formatting are quite annoying because there are defaults that are
assumed rather than present. IE, if you set a column as currency it
doesn't set the number style sometimes and defaults to USD without that
being in the settings. In the future would be far preferable to always
create a fully fleshed map of defaults rather than inferring them in
random places.

* appease our doc string supervisor

* Include column information for formatting numbers

really important for requiring two decimal places by default on doubles
and none on integers. Its close to correct ignoring it and using
"#,###.##" and this leaves non decimal places on integers and allows
only up to two on doubles, but things like 31.10 will format as 31.1 and
that is improper

* Remove unneeded metabase.test

* Update tests with nil column

* Get global viz settings in middleware, pass entire to formatting

Passing the entire viz settings to the formatting stuff makes the
calling code much simpler and the places where we merge global settings,
possibly get currency settings, etc, just in one place. We already had
the col information there so just use it.

* Remove public settings now that we get it from the metadata

src/metabase/pulse/render/body.clj

@@ -62,11 +62,32 @@
 ;;; --------------------------------------------------- Formatting ---------------------------------------------------
 
 (s/defn ^:private format-cell
-  [timezone-id :- (s/maybe s/Str) value col]
+  [timezone-id :- (s/maybe s/Str) value col visualization-settings]
   (cond
-    (types/temporal-field? col)                             (datetime/format-temporal-str timezone-id value col)
-    (and (number? value) (not (types/temporal-field? col))) (common/format-number value)
-    :else                                                   (str value)))
+    (types/temporal-field? col)
+    (datetime/format-temporal-str timezone-id value col)
+
+    (number? value)
+    (common/format-number value col visualization-settings)
+
+    :else
+    (str value)))
+
+(s/defn ^:private get-format
+  [timezone-id :- (s/maybe s/Str) col visualization-settings]
+  (cond
+    ;; for numbers, return a format function that has already computed the differences.
+    ;; todo: do the same for temporal strings
+    (types/temporal-field? col)
+    #(datetime/format-temporal-str timezone-id % col)
+
+    ;; todo integer columns with a unit
+    (or (isa? (:effective_type col) :type/Number)
+        (isa? (:base_type col) :type/Number))
+    (common/number-formatter col visualization-settings)
+
+    :else
+    str))
 
 ;;; --------------------------------------------------- Rendering ----------------------------------------------------
 
@@ -122,31 +143,36 @@
 
 (s/defn ^:private query-results->row-seq
   "Returns a seq of stringified formatted rows that can be rendered into HTML"
-  [timezone-id :- (s/maybe s/Str) remapping-lookup cols rows {:keys [bar-column min-value max-value]}]
-  (for [row rows]
-    {:bar-width (some-> (and bar-column (bar-column row))
-                        (normalize-bar-value min-value max-value))
-     :row (for [[maybe-remapped-col maybe-remapped-row-cell] (map vector cols row)
-                :when (and (not (:remapped_from maybe-remapped-col))
-                           (show-in-table? maybe-remapped-col))
-                :let [[col row-cell] (if (:remapped_to maybe-remapped-col)
-                                       [(nth cols (get remapping-lookup (:name maybe-remapped-col)))
-                                        (nth row (get remapping-lookup (:name maybe-remapped-col)))]
-                                       [maybe-remapped-col maybe-remapped-row-cell])]]
-            (format-cell timezone-id row-cell col))}))
+  [timezone-id :- (s/maybe s/Str) remapping-lookup cols rows viz-settings {:keys [bar-column min-value max-value]}]
+  (let [formatters (into [] (map #(get-format timezone-id % viz-settings)) cols)]
+    (for [row rows]
+      {:bar-width (some-> (and bar-column (bar-column row))
+                          (normalize-bar-value min-value max-value))
+       :row (for [[maybe-remapped-col maybe-remapped-row-cell fmt-fn] (map vector cols row formatters)
+                  :when (and (not (:remapped_from maybe-remapped-col))
+                             (show-in-table? maybe-remapped-col))
+                  :let [[formatter row-cell] (if (:remapped_to maybe-remapped-col)
+                                               (let [remapped-index (get remapping-lookup (:name maybe-remapped-col))]
+                                                [(nth formatters remapped-index)
+                                                 (nth row remapped-index)])
+                                               [fmt-fn maybe-remapped-row-cell])]]
+              (fmt-fn row-cell))})))
 
 (s/defn ^:private prep-for-html-rendering
   "Convert the query results (`cols` and `rows`) into a formatted seq of rows (list of strings) that can be rendered as
   HTML"
   ([timezone-id :- (s/maybe s/Str) card data column-limit]
    (prep-for-html-rendering timezone-id card data column-limit {}))
-  ([timezone-id :- (s/maybe s/Str) card {:keys [cols rows]} column-limit
+  ([timezone-id :- (s/maybe s/Str) card {:keys [cols rows viz-settings]} column-limit
     {:keys [bar-column min-value max-value] :as data-attributes}]
    (let [remapping-lookup (create-remapping-lookup cols)
          limited-cols (take column-limit cols)]
      (cons
       (query-results->header-row remapping-lookup card limited-cols bar-column)
-      (query-results->row-seq timezone-id remapping-lookup limited-cols (take rows-limit rows) data-attributes)))))
+      (query-results->row-seq timezone-id remapping-lookup limited-cols
+                              (take rows-limit rows)
+                              viz-settings
+                              data-attributes)))))
 
 (defn- strong-limit-text [number]
   [:strong {:style (style/style {:color style/color-gray-3})} (h (common/format-number number))])
@@ -364,8 +390,9 @@
                 (percentages label)]]))]}))
 
 (s/defmethod render :scalar :- common/RenderedPulseCard
-  [_ _ timezone-id card {:keys [cols rows]}]
-  (let [value (format-cell timezone-id (ffirst rows) (first cols))]
+  [_ _ timezone-id _card {:keys [cols rows viz-settings] :as data}]
+  (let [col             (first cols)
+        value           (format-cell timezone-id (ffirst rows) (first cols) viz-settings)]
     {:attachments
      nil
 
@@ -375,7 +402,7 @@
      :render/text (str value)}))
 
 (s/defmethod render :smartscalar :- common/RenderedPulseCard
-  [_ _ timezone-id _card {:keys [cols rows insights]}]
+  [_ _ timezone-id _card {:keys [cols _rows insights viz-settings]}]
   (letfn [(col-of-type [t c] (or (isa? (:effective_type c) t)
                                  ;; computed and agg columns don't have an effective type
                                  (isa? (:base_type c) t)))
@@ -389,9 +416,9 @@
           {:keys [last-value previous-value unit last-change] :as _insight}
           (where (comp #{(:name metric-col)} :col) insights)]
       (if (and last-value previous-value unit last-change)
-        (let [value    (format-cell timezone-id last-value metric-col)
-              previous (format-cell timezone-id previous-value metric-col)
-              adj      (if (pos? last-change) (tru "Up") (tru "Down"))]
+        (let [value           (format-cell timezone-id last-value metric-col viz-settings)
+              previous        (format-cell timezone-id previous-value metric-col viz-settings)
+              adj             (if (pos? last-change) (tru "Up") (tru "Down"))]
           {:attachments nil
            :content     [:div
                          [:div {:style (style/style (style/scalar-style))}

src/metabase/pulse/render/common.clj

 (ns metabase.pulse.render.common
   (:require [clojure.pprint :refer [cl-format]]
             [hiccup.util :as hutil]
+            [metabase.shared.models.visualization-settings :as mb.viz]
+            [metabase.shared.util.currency :as currency]
             [metabase.util.ui-logic :as ui-logic]
             [potemkin.types :as p.types]
             [schema.core :as s])
-  (:import java.net.URL))
+  (:import java.net.URL
+           (java.text DecimalFormat DecimalFormatSymbols)))
 
 ;; Fool Eastwood into thinking this namespace is used
 (comment hutil/keep-me)
@@ -22,11 +25,63 @@
   Object
   (toString [_] num-str))
 
+(defn number-formatter
+  "Return a function that will take a number and format it according to its column viz settings. Useful to compute the
+  format string once and then apply it over many values."
+  [{:keys [effective_type base_type] col-id :id col-name :name  :as _column} viz-settings]
+  (let [column-settings    (or (get-in viz-settings [::mb.viz/column-settings {::mb.viz/field-id col-id}])
+                               (get-in viz-settings [::mb.viz/column-settings {::mb.viz/column-name col-name}]))
+        global-settings    (::mb.viz/global-column-settings viz-settings)
+        currency?          (boolean (or (= (::mb.viz/number-style column-settings) "currency")
+                                        (and (nil? (::mb.viz/number-style column-settings))
+                                             (or
+                                              (::mb.viz/currency-style column-settings)
+                                              (::mb.viz/currency column-settings)))))
+
+        {::mb.viz/keys [number-separators decimals scale number-style
+                        prefix suffix currency-style currency]} (merge
+                                                                 (when currency?
+                                                                   (:type/Currency global-settings))
+                                                                 (:type/Number global-settings)
+                                                                 column-settings)
+
+
+        [decimal grouping] (or number-separators ".,")
+        symbols            (doto (DecimalFormatSymbols.)
+                             (cond-> decimal (.setDecimalSeparator decimal))
+                             (cond-> grouping (.setGroupingSeparator grouping)))
+        base               (if (= number-style "scientific") "0" "#,###")
+        integral?          (isa? (or effective_type base_type) :type/Integer)
+        fmt-str            (cond-> (cond
+                                     decimals  (apply str base "." (repeat decimals "0"))
+                                     integral? base
+                                     :else     (str base ".00"))
+                             (= number-style "scientific") (str "E0")
+                             (= number-style "percent")    (str "%"))
+        fmtr               (DecimalFormat. fmt-str symbols)]
+    (fn [value]
+      (if (number? value)
+        (NumericWrapper.
+         (str (when prefix prefix)
+              (when (and currency? (or (nil? currency-style)
+                                       (= currency-style "symbol")))
+                (get-in currency/currency [(keyword (or currency "USD")) :symbol]))
+              (when (and currency? (= currency-style "code"))
+                (str (get-in currency/currency [(keyword (or currency "USD")) :code]) \space))
+              (.format fmtr (* value (or scale 1)))
+              (when (and currency? (= currency-style "name"))
+                (str \space (get-in currency/currency [(keyword (or currency "USD")) :name_plural])))
+              (when suffix suffix)))
+        value))))
+
 (s/defn format-number :- NumericWrapper
   "Format a number `n` and return it as a NumericWrapper; this type is used to do special formatting in other
   `pulse.render` namespaces."
-  [n :- s/Num]
-  (NumericWrapper. (cl-format nil (if (integer? n) "~:d" "~,2f") n)))
+  ([n :- s/Num]
+   (NumericWrapper. (cl-format nil (if (integer? n) "~:d" "~,2f") n)))
+  ([value column viz-settings]
+   (let [fmttr (number-formatter column viz-settings)]
+     (fmttr value))))
 
 (defn graphing-column-row-fns
   "Return a pair of `[get-x-axis get-y-axis]` functions that can be used to get the x-axis and y-axis values in a row,

src/metabase/query_processor/middleware/visualization_settings.clj

 (ns metabase.query-processor.middleware.visualization-settings
-  (:require [metabase.models.card :refer [Card]]
+  (:require [medley.core :as m]
+            [metabase.models.card :refer [Card]]
+            [metabase.public-settings :as public-settings]
             [metabase.query-processor.store :as qp.store]
             [metabase.shared.models.visualization-settings :as mb.viz]
             [toucan.db :as db]))
@@ -50,7 +52,11 @@
             updated-column-viz-settings (if (= (:type query) :query)
                                           (update-card-viz-settings column-viz-settings field-ids)
                                           column-viz-settings)
-            updated-card-viz-settings   (assoc card-viz-settings ::mb.viz/column-settings updated-column-viz-settings)
+            global-settings             (m/map-vals mb.viz/db->norm-column-settings-entries
+                                                    (public-settings/custom-formatting))
+            updated-card-viz-settings   (-> card-viz-settings
+                                            (assoc ::mb.viz/column-settings updated-column-viz-settings)
+                                            (assoc ::mb.viz/global-column-settings global-settings))
             query'                      (dissoc query :viz-settings)
             rff'                        (fn [metadata] (rff (assoc metadata :viz-settings updated-card-viz-settings)))]
         (qp query' rff' context))

test/metabase/pulse/render/common_test.clj

+(ns metabase.pulse.render.common-test
+  (:refer-clojure :exclude [format])
+  (:require [clojure.test :refer :all]
+            [metabase.pulse.render.common :as common]
+            [metabase.shared.models.visualization-settings :as mb.viz]))
+
+(defn format [value viz]
+  (str ((common/number-formatter {:id 1}
+                                 {::mb.viz/column-settings
+                                  {{::mb.viz/field-id 1} viz}})
+        value)))
+
+(deftest number-formatting-test
+  (let [value 12345.5432
+        fmt (partial format value)]
+    (testing "Regular Number formatting"
+      (is (= "12,345.54" (fmt nil)))
+      (is (= "12*345^54" (fmt {::mb.viz/number-separators "^*"})))
+      (is (= "prefix12,345.54suffix" (fmt {::mb.viz/prefix "prefix"
+                                           ::mb.viz/suffix "suffix"})))
+      (is (= "12,345.5432000" (fmt {::mb.viz/decimals 7}))))
+    (testing "Currency"
+      (testing "defaults to USD and two decimal places and symbol"
+        (is (= "$12,345.54" (fmt {::mb.viz/number-style "currency"}))))
+      (testing "Defaults to currency when there is a currency style"
+        (is (= "$12,345.54" (fmt {::mb.viz/currency-style "symbol"}))))
+      (testing "Defaults to currency when there is a currency"
+        (is (= "$12,345.54" (fmt {::mb.viz/currency "USD"}))))
+      (testing "Other currencies"
+        (is (= "AED12,345.54" (fmt {::mb.viz/currency "AED"})))
+        (is (= "₡12,345.54" (fmt {::mb.viz/currency "CRC"})))
+        (is (= "12,345.54 Cape Verdean escudos"
+               (fmt {::mb.viz/currency "CVE"
+                     ::mb.viz/currency-style "name"}))))
+      (testing "Understands name, code, and symbol"
+        (doseq [[style expected] [["name" "12,345.54 Czech Republic korunas"]
+                                  ["symbol" "Kč12,345.54"]
+                                  ["code" "CZK 12,345.54"]]]
+          (is (= expected (fmt {::mb.viz/currency "CZK"
+                                ::mb.viz/currency-style style}))
+              style))))
+    (testing "scientific notation"
+      (is (= "1.23E4" (fmt {::mb.viz/number-style "scientific"})))
+      (is (= "1.2346E4" (fmt {::mb.viz/number-style "scientific"
+                              ::mb.viz/decimals 4})))
+      (is (= "1.E4" (fmt {::mb.viz/number-style "scientific"
+                          ::mb.viz/decimals 0}))))
+    (testing "Percentage"
+      (is (= "1,234,554.32%" (fmt {::mb.viz/number-style "percent"})))
+      (is (= "1.234.554,3200%"
+             (fmt {::mb.viz/number-style "percent"
+                   ::mb.viz/decimals 4
+                   ::mb.viz/number-separators ",."}))))
+    (testing "Column Settings"
+      (letfn [(fmt-with-type [type value]
+                (let [fmt-fn (common/number-formatter {:id 1 :effective_type type}
+                                                      {::mb.viz/column-settings
+                                                       {{::mb.viz/column-id 1}
+                                                        {:effective_type type}}})]
+                  (str (fmt-fn value))))]
+        (is (= "3" (fmt-with-type :type/Integer 3)))
+        (is (= "3.00" (fmt-with-type :type/Decimal 3)))
+        (is (= "3.10" (fmt-with-type :type/Decimal 3.1)))))
+    (testing "Does not throw on nils"
+        (is (nil?
+             ((common/number-formatter {:id 1}
+                                       {::mb.viz/column-settings
+                                        {{::mb.viz/column-id 1}
+                                         {::mb.viz/number-style "percent"}}})
+              nil))))
+    (testing "Does not throw on non-numeric types"
+        (is (= "bob"
+               ((common/number-formatter {:id 1}
+                                       {::mb.viz/column-settings
+                                        {{::mb.viz/column-id 1}
+                                         {::mb.viz/number-style "percent"}}})
+                "bob"))))))

test/metabase/query_processor/middleware/visualization_settings_test.clj

@@ -6,11 +6,16 @@
             [metabase.shared.models.visualization-settings :as mb.viz]
             [metabase.test :as mt]))
 
-(defn- update-viz-settings [query]
-  (-> (mt/test-qp-middleware viz-settings/update-viz-settings query)
-      :metadata
-      :data
-      :viz-settings))
+(defn- update-viz-settings
+  ([query] (update-viz-settings query true))
+  ([query remove-global?]
+   (mt/with-everything-store
+     (cond-> (-> (mt/test-qp-middleware viz-settings/update-viz-settings query)
+                 :metadata
+                 :data
+                 :viz-settings)
+       remove-global?
+       (dissoc ::mb.viz/global-column-settings)))))
 
 (defn- field-id->db-column-ref
   [field-id]
@@ -115,3 +120,15 @@
     (let [query  (test-query [] nil test-native-query-viz-settings :native)
           result (update-viz-settings query)]
       (is (= test-native-query-viz-settings result)))))
+
+(deftest includes-global-settings
+  (testing "Viz settings include global viz settings"
+    (mt/with-temp* [Field [{field-id-1 :id}]
+                    Field [{field-id-2 :id}]]
+      (let [query    (test-query [field-id-1 field-id-2] nil nil)
+            result   (update-viz-settings query false)
+            expected (assoc (processed-viz-settings field-id-1 field-id-2)
+                            ::mb.viz/global-column-settings
+                            #:type{:Number {::mb.viz/number_separators ".,"}
+                                   :Currency {::mb.viz/currency "BIF"
+                                              ::mb.viz/currency_style "code"}})]))))