diff --git a/dev/src/dev/render_png.clj b/dev/src/dev/render_png.clj
index a7fc1a0bc50d6f7cba3109bf1eddd524096d354f..892cdeb9274cc5e9ae3e5919a46378b85951f6e0 100644
--- a/dev/src/dev/render_png.clj
+++ b/dev/src/dev/render_png.clj
@@ -2,24 +2,26 @@
"Improve feedback loop for dealing with png rendering code. Will create images using the rendering that underpins
pulses and subscriptions and open those images without needing to send them to slack or email."
(:require
- [clojure.java.io :as io]
- [clojure.java.shell :as sh]
- [hiccup.core :as hiccup]
- [metabase.models.card :as card]
- [metabase.models.user :as user]
- [metabase.pulse :as pulse]
- [metabase.pulse.render :as render]
- [metabase.pulse.render.test-util :as render.tu]
- [metabase.query-processor :as qp]
- [metabase.query-processor.middleware.permissions :as qp.perms]))
+ [clojure.java.io :as io]
+ [clojure.java.shell :as sh]
+ [hiccup.core :as hiccup]
+ [metabase.models.card :as card]
+ [metabase.pulse :as pulse]
+ [metabase.pulse.render :as render]
+ [metabase.pulse.render.test-util :as render.tu]
+ [metabase.query-processor :as qp]
+ [toucan2.core :as t2])
+ (:import (java.io File)))
+
+(set! *warn-on-reflection* true)
;; taken from https://github.com/aysylu/loom/blob/master/src/loom/io.clj
(defn- os
"Returns :win, :mac, :unix, or nil"
[]
(condp
- #(<= 0 (.indexOf ^String %2 ^String %1))
- (.toLowerCase (System/getProperty "os.name"))
+ #(<= 0 (.indexOf ^String %2 ^String %1))
+ (.toLowerCase (System/getProperty "os.name"))
"win" :win
"mac" :mac
"nix" :unix
@@ -46,35 +48,44 @@
match what's rendered on another system, like a docker container."
[card-id]
(let [{:keys [dataset_query] :as card} (t2/select-one card/Card :id card-id)
- user (t2/select-one user/User)
- query-results (binding [qp.perms/*card-id* nil]
- (qp/process-query-and-save-execution!
- (-> dataset_query
- (assoc :async? false)
- (assoc-in [:middleware :process-viz-settings?] true))
- {:executed-by (:id user)
- :context :pulse
- :card-id card-id}))
+ query-results (qp/process-query dataset_query)
png-bytes (render/render-pulse-card-to-png (pulse/defaulted-timezone card)
card
query-results
1000)
- tmp-file (java.io.File/createTempFile "card-png" ".png")]
+ tmp-file (File/createTempFile "card-png" ".png")]
(with-open [w (java.io.FileOutputStream. tmp-file)]
(.write w ^bytes png-bytes))
(.deleteOnExit tmp-file)
(open tmp-file)))
-(defn open-hiccup-as-html [hiccup]
+(defn render-pulse-card
+ "Render a pulse card as a data structure"
+ [card-id]
+ (let [{:keys [dataset_query] :as card} (t2/select-one card/Card :id card-id)
+ query-results (qp/process-query dataset_query)]
+ (render/render-pulse-card
+ :inline (pulse/defaulted-timezone card)
+ card
+ nil
+ query-results)))
+
+(defn open-hiccup-as-html
+ "Take a hiccup data structure, render it as html, then open it in the browser."
+ [hiccup]
(let [html-str (hiccup/html hiccup)
- tmp-file (java.io.File/createTempFile "card-html" ".html")]
- (with-open [w (clojure.java.io/writer tmp-file)]
- (.write w html-str))
+ tmp-file (File/createTempFile "card-html" ".html")]
+ (with-open [w (io/writer tmp-file)]
+ (.write w ^String html-str))
(.deleteOnExit tmp-file)
(open tmp-file)))
(comment
(render-card-to-png 1)
+
+ (let [{:keys [content]} (render-pulse-card 1)]
+ (open-hiccup-as-html content))
+
;; open viz in your browser
(-> [["A" "B"]
[1 2]
@@ -92,4 +103,3 @@
:hidden-columns {:hide [0 2]}})
:viz-tree
open-hiccup-as-html))
-
diff --git a/src/metabase/pulse/render.clj b/src/metabase/pulse/render.clj
index b2c3cfaeee25ceb56408692e706558face60dbac..baecdd1b303d38efb62d50d8d667bce902fbec9b 100644
--- a/src/metabase/pulse/render.clj
+++ b/src/metabase/pulse/render.clj
@@ -119,7 +119,7 @@
((some-fn :include_csv :include_xls) card))
(s/defn ^:private render-pulse-card-body :- common/RenderedPulseCard
- [render-type timezone-id :- (s/maybe s/Str) card dashcard {:keys [data error], :as results}]
+ [render-type timezone-id :- (s/maybe s/Str) card dashcard {:keys [data error] :as results}]
(try
(when error
(throw (ex-info (tru "Card has errors: {0}" error) (assoc results :card-error true))))
diff --git a/src/metabase/pulse/render/body.clj b/src/metabase/pulse/render/body.clj
index 84227ff2961d37c7c328a77d14fc3f7d7f10e579..7c1487a1ece50fdd8bb20762d97e296db8a24801 100644
--- a/src/metabase/pulse/render/body.clj
+++ b/src/metabase/pulse/render/body.clj
@@ -151,8 +151,15 @@
(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 viz-settings {:keys [bar-column min-value max-value]}]
- (let [formatters (into [] (map #(get-format timezone-id % viz-settings)) cols)]
+ [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))
@@ -171,15 +178,21 @@
HTML"
([timezone-id :- (s/maybe s/Str) card data]
(prep-for-html-rendering timezone-id card data {}))
- ([timezone-id :- (s/maybe s/Str) card {:keys [cols rows viz-settings]}
+ ([timezone-id :- (s/maybe s/Str) {:keys [result_metadata] :as card} {:keys [cols rows viz-settings]}
{:keys [bar-column] :as data-attributes}]
- (let [remapping-lookup (create-remapping-lookup cols)]
+ (let [field-ref->curated-meta (zipmap (map :field_ref result_metadata) result_metadata)
+ ;; Add in user-curated metadata
+ cols (map (fn [{:keys [field_ref] :as col}] (into col (field-ref->curated-meta field_ref))) cols)
+ remapping-lookup (create-remapping-lookup cols)]
(cons
(query-results->header-row remapping-lookup card cols bar-column)
- (query-results->row-seq timezone-id remapping-lookup cols
- (take rows-limit rows)
- viz-settings
- data-attributes)))))
+ (query-results->row-seq
+ timezone-id
+ remapping-lookup
+ 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))])
@@ -204,7 +217,6 @@
:margin-bottom :16px})}
(trs "More results have been included as a file attachment")]))
-
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | render |
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -1001,7 +1013,6 @@
[:img {:style (style/style {:display :block :width :100%})
:src (:image-src image-bundle)}]]}))
-
(s/defmethod render :empty :- common/RenderedPulseCard
[_ render-type _ _ _ _]
(let [image-bundle (image-bundle/no-results-image-bundle render-type)]
diff --git a/src/metabase/pulse/render/common.clj b/src/metabase/pulse/render/common.clj
index bc964fc4390a86ec4655ed0ad2dda9344e47938a..8b19b43bb0cef8b26107bd3975c0f359f8e1c918 100644
--- a/src/metabase/pulse/render/common.clj
+++ b/src/metabase/pulse/render/common.clj
@@ -73,7 +73,9 @@
(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 field-ref :field_ref col-name :name :as _column} viz-settings]
+ [{:keys [semantic_type effective_type base_type]
+ col-id :id field-ref :field_ref col-name :name :as _column}
+ viz-settings]
(let [col-id (or col-id (second field-ref))
column-settings (-> (get viz-settings ::mb.viz/column-settings)
(update-keys #(select-keys % [::mb.viz/field-id ::mb.viz/column-name])))
@@ -92,7 +94,7 @@
(:type/Number global-settings)
column-settings)
integral? (isa? (or effective_type base_type) :type/Integer)
- percent? (= number-style "percent")
+ percent? (or (isa? semantic_type :type/Percentage) (= number-style "percent"))
scientific? (= number-style "scientific")
[decimal grouping] (or number-separators
(get-in (public-settings/custom-formatting) [:type/Number :number_separators])
@@ -106,14 +108,13 @@
(if (number? value)
(let [scaled-value (* value (or scale 1))
percent-scaled-value (* 100 scaled-value)
- decimals-in-value (digits-after-decimal (if (= number-style "percent")
- (* 100 scaled-value)
- scaled-value))
+ decimals-in-value (digits-after-decimal (if percent? (* 100 scaled-value) scaled-value))
decimal-digits (cond
decimals decimals ;; if user ever specifies # of decimals, use that
integral? 0
currency? (get-in currency/currency [(keyword (or currency "USD")) :decimal_digits])
(and percent? (> percent-scaled-value 100)) (min 2 decimals-in-value) ;; 5.5432 -> %554.32
+ ;; This needs configuration. I don't see why we don't keep more significant digits.
(and percent? (> 100 percent-scaled-value 1)) (min 0 decimals-in-value) ;; 0.2555 -> %26
:else (if (>= scaled-value 1)
(min 2 decimals-in-value) ;; values greater than 1 round to 2 decimal places
@@ -127,18 +128,18 @@
percent? (str "%"))
fmtr (doto (DecimalFormat. fmt-str symbols) (.setRoundingMode RoundingMode/HALF_UP))]
(map->NumericWrapper
- {:num-value value
- :num-str (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))
- (cond-> (.format fmtr scaled-value)
- (not decimals) (strip-trailing-zeroes decimal))
- (when (and currency? (= currency-style "name"))
- (str \space (get-in currency/currency [(keyword (or currency "USD")) :name_plural])))
- (when suffix suffix))}))
+ {:num-value value
+ :num-str (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))
+ (cond-> (.format fmtr scaled-value)
+ (not decimals) (strip-trailing-zeroes decimal))
+ (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
diff --git a/src/metabase/types.cljc b/src/metabase/types.cljc
index 51aad618a8cf6a5317b8f6fc74ae2dcead93f626..d1a98aaff2731db6b1a78ed0f6e67210cad4fbd2 100644
--- a/src/metabase/types.cljc
+++ b/src/metabase/types.cljc
@@ -88,6 +88,11 @@
(derive :type/Share :Semantic/*)
(derive :type/Share :type/Float)
+;; A percent value (generally 0-100)
+
+(derive :type/Percentage :Semantic/*)
+(derive :type/Percentage :type/Decimal)
+
;; `:type/Currency` -- an actual currency data type, for example Postgres `money`.
;; `:type/Currency` -- a column that should be interpreted as money.
;;
diff --git a/test/metabase/pulse/render_test.clj b/test/metabase/pulse/render_test.clj
index ddc721f37ca5cf163c55d8f536215eedcc269862..9e2f2d6ed6d8e8befb53dc04d494bc316f369d38 100644
--- a/test/metabase/pulse/render_test.clj
+++ b/test/metabase/pulse/render_test.clj
@@ -11,6 +11,8 @@
[metabase.util :as u]
[toucan2.tools.with-temp :as t2.with-temp]))
+(set! *warn-on-reflection* true)
+
;; Let's make sure rendering Pulses actually works
(defn- render-pulse-card
@@ -157,3 +159,42 @@
DashboardCard dc1 {:dashboard_id (:id dashboard) :card_id (:id card)}]
(binding [render/*include-description* true]
(is (= "<h1>Card description</h1>\n" (last (:content (#'render/make-description-if-needed dc1 card)))))))))
+
+(deftest table-rendering-of-percent-types-test
+ (testing "If a column is marked as a :type/Percentage semantic type it should render as a percent"
+ (mt/dataset sample-dataset
+ (mt/with-temp [Card {base-card-id :id} {:dataset_query {:database (mt/id)
+ :type :query
+ :query {:source-table (mt/id :orders)
+ :expressions {"Tax Rate" [:/
+ [:field (mt/id :orders :tax) {:base-type :type/Float}]
+ [:field (mt/id :orders :total) {:base-type :type/Float}]]},
+ :fields [[:field (mt/id :orders :tax) {:base-type :type/Float}]
+ [:field (mt/id :orders :total) {:base-type :type/Float}]
+ [:expression "Tax Rate"]]
+ :limit 10}}}
+ Card {:keys [dataset_query] :as card} {:dataset_query {:type :query
+ :database (mt/id)
+ :query {:source-table (format "card__%s" base-card-id)}}
+ :result_metadata [{:semantic_type :type/Percentage
+ :field_ref [:field "Tax Rate" {:base-type :type/Float}]}]}]
+ ;; NOTE -- The logic in metabase.pulse.render.common/number-formatter renders values between 1 and 100 as an
+ ;; integer value. IDK if this is what we want long term, but this captures the current logic. If we do extend
+ ;; the significant digits in the formatter, we'll need to modify this test as well.
+ (let [query-results (qp/process-query dataset_query)
+ expected (mapv (fn [row]
+ (format "%s%%" (Math/round ^float (* 100 (peek row)))))
+ (get-in query-results [:data :rows]))
+ rendered-card (render/render-pulse-card :inline (pulse/defaulted-timezone card) card nil query-results)
+ table (-> rendered-card
+ (get-in [:content 1 2 4 2])
+ first
+ second)
+ tax-col (->>
+ (rest (get-in table [2 1]))
+ (map-indexed (fn [i v] [i (last v)]))
+ (some (fn [[i v]] (when (= v "Tax Rate") i))))]
+ (testing "A column marked as semantic type :type/Percentage should be rendered with a percent sign"
+ (is (= expected
+ (->> (get-in table [3 1])
+ (map #(peek (get (vec (get % 2)) tax-col))))))))))))
diff --git a/test/metabase/sync/analyze/fingerprint_test.clj b/test/metabase/sync/analyze/fingerprint_test.clj
index 8dd82df2e3610836a144d5ceb9eff1aa9e4ec071..8675f1af09dc3b0e08721b1cfb2fadcd08e8a235 100644
--- a/test/metabase/sync/analyze/fingerprint_test.clj
+++ b/test/metabase/sync/analyze/fingerprint_test.clj
@@ -59,7 +59,7 @@
[:and
[:< :fingerprint_version 2]
[:in :base_type #{"type/Decimal" "type/Latitude" "type/Longitude" "type/Coordinate" "type/Currency" "type/Float"
- "type/Share" "type/Income" "type/Price" "type/Discount" "type/GrossMargin" "type/Cost"}]]
+ "type/Share" "type/Income" "type/Price" "type/Discount" "type/GrossMargin" "type/Cost" "type/Percentage"}]]
[:and
[:< :fingerprint_version 1]
[:in :base_type #{"type/ImageURL" "type/AvatarURL"}]]]]}
@@ -81,7 +81,7 @@
[:and
[:< :fingerprint_version 2]
[:in :base_type #{"type/Decimal" "type/Latitude" "type/Longitude" "type/Coordinate" "type/Currency" "type/Float"
- "type/Share" "type/Income" "type/Price" "type/Discount" "type/GrossMargin" "type/Cost"}]]
+ "type/Share" "type/Income" "type/Price" "type/Discount" "type/GrossMargin" "type/Cost" "type/Percentage"}]]
;; no type/Float stuff should be included for 1
[:and
[:< :fingerprint_version 1]
@@ -104,7 +104,7 @@
[:and
[:< :fingerprint_version 4]
[:in :base_type #{"type/Decimal" "type/Latitude" "type/Longitude" "type/Coordinate" "type/Currency" "type/Float"
- "type/Share" "type/Income" "type/Price" "type/Discount" "type/GrossMargin" "type/Cost"}]]
+ "type/Share" "type/Income" "type/Price" "type/Discount" "type/GrossMargin" "type/Cost" "type/Percentage"}]]
[:and
[:< :fingerprint_version 3]
[:in :base_type #{"type/URL" "type/ImageURL" "type/AvatarURL"}]]