Format Pie Chart Legend Labels as Month, YYYY (#24890)

* Format Pie Chart Legend Labels as Month, YYYY

On the backend, the pie chart legend is not rendered in the javascript, it is instead re-created server side as a set
of html elements. This means "YYYY-MM-DD" formatted dates do not get passed through the frontend code responsible for
formatting date times (I believe this is done with a library called 'moment.js'). And, since the SVG cannot contain
html foreignObjects (the backend SVG renederer 'batik' ignores foreign objects), the legend is left as a
responsiblility for Clojure land.

This PR adds a `format-month` function to handle this case.

Current caveats with this approach:

- doesn't consider localization (yet)
- adjusted the html to use a table instead of a div full of spans and the formatting could use some tweaks
- assumes any string that is parse-able into a Jave DateTime object will be displayed in this Month, Year format,
- which might not be true in all cases

* Improve label formatting by using viz-settings transform fns

* Rework Static Pie Chart legend

Try to make it look a bit nicer:
- splits legends into 2 tables if there are more than 8 entries
- larger colored circle
- use tables to help lay things out more evenly (CSS flexbox doesn't seem to work)
- handle some legend label formatting cases beyond just timestamps (eg weeks of year, yearly quarters)

Also fixed the failing test

* Adding a month formatter.

Sometimes, the label values are not passed in as timestamps, but just numbers (as strings). In these cases, the
Datetime formatters won't be useful, so there are some cases built to handle these.

* Remove print statement

* Change temporal format fns to more closely follow viz-settings

Various changes are made to improve static-viz's use of viz-settings from the frontend.

- abbreviation is considered (eg. so "January" is rendered as "Jan" when setting is true)
- abbreviation also works for Days of week
- x-of-y cases are handled a bit better
- separator settings are properly reflected (eg. 2022/05 becomes 2022-05 if separator changed to "-")

* Remove unused key from the let

* Fix some failing tests

* Adding more tests

* Prevent non-temporal legend labels from being rendered via datetime

If a legend's label is some string, we want to pass it unmodified. If it's a number or a timestamp, we can pass it
through to the date time render function to format the string to match the viz-settings/column units

* Add missed docstring

* Don't lowercase 'D' format, pass correct lowercase 'd' before

'D' format = day of year, which in most human-readable formats is not what we want, but it IS what we want in a
:day-of-year case.

Instead of always lowercasing, just make sure the lowercase 'd' is in the defaults/overrides

* Better way to handle day of week and month of year

Also got rid of bad use of read-string by using parse-long

* Use a java lib. to format ordinal numbers, respecting site locale

src/metabase/pulse/render/body.clj

@@ -234,7 +234,7 @@
    :quarter "[Q]Q - YYYY"
    :minute-of-hour "m"
    :day-of-week "dddd"
-   :day-of-month "D"
+   :day-of-month "d"
    :day-of-year "DDD"
    :week-of-year "wo"
    :month-of-year "MMMM"
@@ -247,15 +247,19 @@
                :quarter "YYYY - [Q]Q"}
    "MMMM D, YYYY" {:month "MMMM, YYYY"}
    "D MMMM, YYYY" {:month "MMMM, YYYY"}
-   "dddd, MMMM D, YYYY" {:week "MMMM D, YYYY"
+   "dddd, MMMM D, YYYY" {:day "EEEE, MMMM d, YYYY"
+                         :week "MMMM d, YYYY"
                          :month "MMMM, YYYY"}})
 
 (defn- update-date-style
-  [date-style unit]
+  [date-style unit {::mb.viz/keys [date-abbreviate date-separator]}]
   (let [unit (or unit :default)]
-    (or (get-in override-date-styles [date-style unit])
-        (get-in default-date-styles [unit])
-        date-style)))
+    (cond-> (or (get-in override-date-styles [date-style unit])
+                (get-in default-date-styles [unit])
+                date-style)
+      (not= date-separator "/") (str/replace #"/" date-separator)
+      date-abbreviate (-> (str/replace #"MMMM" "MMM")
+                          (str/replace #"EEEE" "E")))))
 
 (defn- backfill-currency
   [{:keys [number_style currency] :as settings}]
@@ -272,7 +276,7 @@
   [col-settings col]
   (-> (m/map-keys (fn [k] (-> k name (str/replace #"-" "_") keyword)) col-settings)
       (backfill-currency)
-      (u/update-if-exists :date_style update-date-style (:unit col))))
+      (u/update-if-exists :date_style update-date-style (:unit col) col-settings)))
 
 (defn- ->js-viz
   "Include viz settings for js.
@@ -397,8 +401,38 @@
                                    (tru "N/A")
                                    (format-percentage (/ value total)))]))}))
 
+(defn- donut-legend
+  [legend-entries]
+  (letfn [(table-fn [entries]
+            (into [:table {:style (style/style {:color       "#4C5773"
+                                                :font-family "Lato, sans-serif"
+                                                :font-size   "24px"
+                                                :font-weight "bold"
+                                                :box-sizing  "border-box"
+                                                :white-space "nowrap"})}]
+                  (for [{:keys [label percentage color]} entries]
+                    [:tr {:style (style/style {:margin-right "12px"})}
+                     [:td {:style (style/style {:color         color
+                                                :padding-right "7px"
+                                                :line-height   "0"})}
+                      [:span {:style (style/style {:font-size   "2.875rem"
+                                                   :line-height "0"
+                                                   :position    "relative"
+                                                   :top         "-4px"})} "•"]]
+                     [:td {:style (style/style {:padding-right "30px"})}
+                      label]
+                   [:td percentage]])))]
+    (if (< (count legend-entries) 8)
+      (table-fn legend-entries)
+      [:table (into [:tr]
+                    (map (fn [some-entries]
+                           [:td {:style (style/style {:padding-right  "20px"
+                                                      :vertical-align "top"})}
+                            (table-fn some-entries)])
+                         (split-at (/ (count legend-entries) 2) legend-entries)))])))
+
 (s/defmethod render :categorical/donut :- common/RenderedPulseCard
-  [_ render-type _timezone-id :- (s/maybe s/Str) card dashcard {:keys [rows viz-settings] :as data}]
+  [_ render-type timezone-id :- (s/maybe s/Str) card dashcard {:keys [rows cols viz-settings] :as data}]
   (let [viz-settings                (merge viz-settings (:visualization_settings dashcard))
         [x-axis-rowfn y-axis-rowfn] (common/graphing-column-row-fns card data)
         rows                        (map (juxt (comp str x-axis-rowfn) y-axis-rowfn)
@@ -409,8 +443,9 @@
         legend-colors               (merge (zipmap (map first rows) (cycle colors))
                                            (update-keys (:pie.colors viz-settings) name))
         image-bundle                (image-bundle/make-image-bundle
-                                     render-type
-                                     (js-svg/categorical-donut rows legend-colors))]
+                                      render-type
+                                      (js-svg/categorical-donut rows legend-colors))
+        {label-viz-settings :x}     (->js-viz (x-axis-rowfn cols) (y-axis-rowfn cols) viz-settings)]
     {:attachments
      (when image-bundle
        (image-bundle/image-bundle->attachment image-bundle))
@@ -419,17 +454,20 @@
      [:div
       [:img {:style (style/style {:display :block :width :100%})
              :src   (:image-src image-bundle)}]
-      (into [:div {:style (style/style {:clear :both :width "540px" :color "#4C5773"})}]
-            (for [label (map first rows)]
-              [:div {:style (style/style {:float       :left :margin-right "12px"
-                                          :font-family "Lato, sans-serif"
-                                          :font-size   "16px"})}
-               [:span {:style (style/style {:color (legend-colors label)})}
-                "•"]
-               [:span {:style (style/style {:margin-left "6px"})}
-                label]
-               [:span {:style (style/style {:margin-left "6px"})}
-                (percentages label)]]))]}))
+      (donut-legend
+        (mapv (fn [row]
+                (let [label (first row)]
+                  {:percentage (percentages (first row))
+                   :color      (legend-colors (first row))
+                   :label      (if (or (datetime/temporal-string? label)
+                                       (boolean (parse-long label)))
+                                 (datetime/format-temporal-str
+                                   timezone-id
+                                   (first row)
+                                   (x-axis-rowfn cols)
+                                   label-viz-settings)
+                                 label)}))
+              rows))]}))
 
 (s/defmethod render :progress :- common/RenderedPulseCard
   [_ render-type _timezone-id _card dashcard {:keys [cols rows viz-settings] :as _data}]

src/metabase/pulse/render/datetime.clj

@@ -3,41 +3,93 @@
   (:require [clojure.string :as str]
             [clojure.tools.logging :as log]
             [java-time :as t]
+            [metabase.public-settings :as public-settings]
             [metabase.util.date-2 :as u.date]
             [metabase.util.i18n :refer [trs tru]]
             [metabase.util.schema :as su]
             [schema.core :as s])
-  (:import java.time.format.DateTimeFormatter
+  (:import com.ibm.icu.text.RuleBasedNumberFormat
+           java.time.format.DateTimeFormatter
            java.time.Period
-           java.time.temporal.Temporal))
+           java.time.temporal.Temporal
+           java.util.Locale))
+
+(defn temporal-string?
+  "Returns `true` if the string `s` is parseable as a datetime.
+
+  `(temporal-string? \"asdf\")` -> false
+  `(temporal-string? \"2020-02-02\")` -> true"
+  [s]
+  (boolean
+    (try
+      (u.date/parse s)
+      (catch Exception _e false))))
 
 (defn- reformat-temporal-str [timezone-id s new-format-string]
   (t/format new-format-string (u.date/parse s timezone-id)))
 
+(defn- day-of-week
+  [n abbreviate]
+  (let [fmtr (java.time.format.DateTimeFormatter/ofPattern (if abbreviate "EEE" "EEEE"))]
+    (.format fmtr (java.time.DayOfWeek/of n))))
+
+(defn- month-of-year
+  [n abbreviate]
+  (let [fmtr (java.time.format.DateTimeFormatter/ofPattern (if abbreviate "MMM" "MMMM"))]
+    (.format fmtr (java.time.Month/of n))))
+
+(defn- x-of-y
+  "Format an integer as x-th of y, for example, 2nd week of year."
+  [n]
+  (let [nf (RuleBasedNumberFormat. (Locale. (public-settings/site-locale)) RuleBasedNumberFormat/ORDINAL)]
+    (.format nf n)))
+
+(defn- hour-of-day
+  [s time-style]
+  (let [n  (parse-long s)
+        ts (u.date/parse "2022-01-01-00:00:00")]
+    (u.date/format time-style (t/plus ts (t/hours n)))))
+
 (defn format-temporal-str
   "Reformat a temporal literal string `s` (i.e., an ISO-8601 string) with a human-friendly format based on the
   column `:unit`."
-  [timezone-id s col]
-  (cond (str/blank? s) ""
-
-        (isa? (or (:effective_type col) (:base_type col)) :type/Time)
-        (t/format DateTimeFormatter/ISO_LOCAL_TIME (u.date/parse s timezone-id))
-
-        :else
-        (case (:unit col)
-          ;; these types have special formatting
-          :hour    (reformat-temporal-str timezone-id s "h a - MMM yyyy")
-          :week    (str "Week " (reformat-temporal-str timezone-id s "w - YYYY"))
-          :month   (reformat-temporal-str timezone-id s "MMMM yyyy")
-          :quarter (reformat-temporal-str timezone-id s "QQQ - yyyy")
-
-          ;; no special formatting here : return as ISO-8601
-          ;; TODO: probably shouldn't even be showing sparkline for x-of-y groupings?
-          (:year :hour-of-day :day-of-week :week-of-year :month-of-year)
-          s
-
-          ;; for everything else return in this format
-          (reformat-temporal-str timezone-id s "MMM d, yyyy"))))
+  ([timezone-id s col] (format-temporal-str timezone-id s col {}))
+  ([timezone-id s col col-viz-settings]
+   (Locale/setDefault (Locale. (public-settings/site-locale)))
+   (let [{date-style :date_style
+          abbreviate :date_abbreviate
+          time-style :time_style} col-viz-settings]
+     (cond (str/blank? s) ""
+
+           (isa? (or (:effective_type col) (:base_type col)) :type/Time)
+           (t/format DateTimeFormatter/ISO_LOCAL_TIME (u.date/parse s timezone-id))
+
+           :else
+           (case (:unit col)
+             ;; these types have special formatting
+             :minute  (reformat-temporal-str timezone-id s
+                                             (str (or date-style "MMMM, yyyy") ", "
+                                                  (str/replace (or time-style "h:mm a") #"A" "a")))
+             :hour    (reformat-temporal-str timezone-id s
+                                             (str (or date-style "MMMM, yyyy") ", "
+                                                  (str/replace (or time-style "h a") #"A" "a")))
+             :day     (reformat-temporal-str timezone-id s (or date-style "EEEE, MMMM d, YYYY"))
+             :week    (str (tru "Week ") (reformat-temporal-str timezone-id s "w - YYYY"))
+             :month   (reformat-temporal-str timezone-id s (or date-style "MMMM, yyyy"))
+             :quarter (reformat-temporal-str timezone-id s "QQQ - yyyy")
+             :year    (reformat-temporal-str timezone-id s "YYYY")
+
+             ;; s is just a number as a string here
+             :day-of-week     (day-of-week (parse-long s) abbreviate)
+             :month-of-year   (month-of-year (parse-long s) abbreviate)
+             :quarter-of-year (format "Q%s" s)
+             :hour-of-day     (hour-of-day s (str/replace (or time-style "h a") #"A" "a"))
+
+             (:week-of-year :minute-of-hour :day-of-month :day-of-year) (x-of-y (parse-long s))
+             ;; TODO: probably shouldn't even be showing sparkline for x-of-y groupings?
+
+             ;; for everything else return in this format
+             (reformat-temporal-str timezone-id s "MMM d, yyyy"))))))
 
 (def ^:private RenderableInterval
   {:interval-start     Temporal

test/metabase/pulse/render/body_test.clj

@@ -619,9 +619,9 @@
     (testing "Includes percentages"
       (is (= [:div
               [:img]
-              [:div
-               [:div [:span "•"] [:span "Doohickey"] [:span "75%"]]
-               [:div [:span "•"] [:span "Widget"] [:span "25%"]]]]
+              [:table
+               [:tr [:td [:span "•"]] [:td "Doohickey"] [:td "75%"]]
+               [:tr [:td [:span "•"]] [:td "Widget"] [:td "25%"]]]]
              (prune (:content (render [["Doohickey" 75] ["Widget" 25]]))))))))
 
 (deftest render-progress

test/metabase/pulse/render/datetime_test.clj

@@ -17,7 +17,7 @@
            (format-temporal-string-pair :day "2020-07-15T18:04:00Z" nil)))
     (is (= ["Today" "Previous day"]
            (format-temporal-string-pair :day now nil)))
-    (is (= ["Jul 18, 2020" "Jul 20, 2020"]
+    (is (= ["Saturday, July 18, 2020" "Monday, July 20, 2020"]
            (format-temporal-string-pair :day "2020-07-18T18:04:00Z" "2020-07-20T18:04:00Z")))
     (is (= ["Last week" "Previous week"]
            (format-temporal-string-pair :week "2020-07-09T18:04:00Z" nil)))
@@ -29,7 +29,7 @@
            (format-temporal-string-pair :month "2020-07-16T18:04:00Z" nil)))
     (is (= ["This month" "Previous month"]
            (format-temporal-string-pair :month now nil)))
-    (is (= ["July 2021" "July 2022"]
+    (is (= ["July, 2021" "July, 2022"]
            (format-temporal-string-pair :month "2021-07-16T18:04:00Z" "2022-07-16T18:04:00Z")))
     (is (= ["Last quarter" "Previous quarter"]
            (format-temporal-string-pair :quarter "2020-05-16T18:04:00Z" nil)))
@@ -41,17 +41,60 @@
            (format-temporal-string-pair :year "2019-07-16T18:04:00Z" nil)))
     (is (= ["This year" "Previous year"]
            (format-temporal-string-pair :year now nil)))
-    (testing "No special formatting for year? :shrug:"
-      (is (= ["2018-07-16T18:04:00Z" "2021-07-16T18:04:00Z"]
-             (format-temporal-string-pair :year "2018-07-16T18:04:00Z" "2021-07-16T18:04:00Z"))))))
+    (is (= ["2018" "2021"]
+           (format-temporal-string-pair :year "2018-07-16T18:04:00Z" "2021-07-16T18:04:00Z")))))
 
 (deftest format-temporal-str-test
   (testing "Null values do not blow up"
     (is (= ""
            (datetime/format-temporal-str "UTC" nil :now))))
-  (testing "Not-null values work"
-    (is (= "Jul 16, 2020"
-           (datetime/format-temporal-str "UTC" now :day))))
+  (testing "Temporal Units are formatted"
+    (testing :minute
+      (is (= "July, 2020, 6:04 PM"
+             (datetime/format-temporal-str "UTC" now {:unit :minute}))))
+    (testing :hour
+      (is (= "July, 2020, 6 PM"
+             (datetime/format-temporal-str "UTC" now {:unit :hour}))))
+    (testing :day
+      (is (= "Thursday, July 16, 2020"
+             (datetime/format-temporal-str "UTC" now {:unit :day}))))
+    (testing :week
+      (is (= "Week 29 - 2020"
+             (datetime/format-temporal-str "UTC" now {:unit :week}))))
+    (testing :month
+      (is (= "July, 2020"
+             (datetime/format-temporal-str "UTC" now {:unit :month}))))
+    (testing :quarter
+      (is (= "Q3 - 2020"
+             (datetime/format-temporal-str "UTC" now {:unit :quarter}))))
+    (testing :year
+      (is (= "2020"
+             (datetime/format-temporal-str "UTC" now {:unit :year})))))
+  (testing "x-of-y Temporal Units are formatted"
+    (testing :minute-of-hour
+      (is (= "1st"
+             (datetime/format-temporal-str "UTC" "1" {:unit :minute-of-hour}))))
+    (testing :day-of-month
+      (is (= "2nd"
+             (datetime/format-temporal-str "UTC" "2" {:unit :day-of-month}))))
+    (testing :day-of-year
+      (is (= "203rd"
+             (datetime/format-temporal-str "UTC" "203" {:unit :day-of-year}))))
+    (testing :week-of-year
+      (is (= "44th"
+             (datetime/format-temporal-str "UTC" "44" {:unit :week-of-year}))))
+    (testing :day-of-week
+      (is (= "Thursday"
+             (datetime/format-temporal-str "UTC" "4" {:unit :day-of-week}))))
+    (testing :month-of-year
+      (is (= "May"
+             (datetime/format-temporal-str "UTC" "5" {:unit :month-of-year}))))
+    (testing :quarter-of-year
+      (is (= "Q3"
+             (datetime/format-temporal-str "UTC" "3" {:unit :quarter-of-year}))))
+    (testing :hour-of-day
+      (is (= "4 AM"
+             (datetime/format-temporal-str "UTC" "4" {:unit :hour-of-day})))))
   (testing "Can render time types (#15146)"
     (is (= "08:05:06"
            (datetime/format-temporal-str "UTC" "08:05:06Z"

test/metabase/pulse/render_test.clj

@@ -28,7 +28,7 @@
                                   (mt/mbql-query checkins
                                     {:aggregation [[:count]]
                                      :breakout    [!month.date]}))
-                                 [:td _ "November 2015"])))))
+                                 [:td _ "November, 2015"])))))
 
 (deftest render-error-test
   (testing "gives us a proper error if we have erroring card"