Skip to content
Snippets Groups Projects
Unverified Commit c35f2503 authored by adam-james's avatar adam-james Committed by GitHub
Browse files

Export Formatting Correctness (#46306) 

* Export Formatting Correctness

WIP

Several bugs exist around the correctness of export formatting, this aims to fix them.

Fixes #27374

When a model has viz settings/metadata changed after save, the download won't incorporate said changes. At least it's
clear that scale doesn't work.

* number formatter merges settings from columns too

* column formatting on aggregated cols works in xlsx now too

* fixes 43040 by grabbing global-type-settings in csv formatter
parent b5ad29da
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/formatter.clj
+ 23
− 6
View file @ c35f2503
......@@ -8,6 +8,7 @@
[hiccup.util]
[metabase.formatter.datetime :as datetime]
[metabase.public-settings :as public-settings]
[metabase.query-processor.streaming.common :as common]
[metabase.shared.models.visualization-settings :as mb.viz]
[metabase.shared.util.currency :as currency]
[metabase.types :as types]
......@@ -83,19 +84,35 @@
figs (last (str/split val-string #"[\.0]+"))]
(count figs))))
(defn- qualify-keys
[m]
(update-keys m (fn [k] (keyword
"metabase.shared.models.visualization-settings"
(name k)))))
;; TODO: use `metabase.query-processor.streaming.common/viz-settings-for-col` here, it's
;; doing the same thing (unifying global settings, column settings, and viz-settings for the column.
;; perhaps that implementation needs to move here, or to a new `metabase.formatter.common` or something?
(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 [semantic_type effective_type base_type]
col-id :id field-ref :field_ref col-name :name :as _column}
col-id :id field-ref :field_ref col-name :name col-settings :settings :as col}
viz-settings]
(let [col-id (or col-id (second field-ref))
(let [global-type-settings (common/global-type-settings col viz-settings)
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])))
column-settings (or (get column-settings {::mb.viz/field-id col-id})
(get column-settings {::mb.viz/column-name col-name}))
global-settings (::mb.viz/global-column-settings viz-settings)
column-settings (merge
(or (get column-settings {::mb.viz/field-id col-id})
(get column-settings {::mb.viz/column-name col-name}))
(qualify-keys col-settings)
global-type-settings)
global-settings (merge
global-type-settings
(::mb.viz/global-column-settings viz-settings))
currency? (boolean (or (= (::mb.viz/number-style column-settings) "currency")
(= (::mb.viz/number-style viz-settings) "currency")
(and (nil? (::mb.viz/number-style column-settings))
(or
(::mb.viz/currency-style column-settings)
......@@ -145,7 +162,7 @@
(false? (::mb.viz/currency-in-header column-settings)))]
(str (when prefix prefix)
(when (and inline-currency? (or (nil? currency-style)
(= currency-style "symbol")))
(= currency-style "symbol")))
(get-in currency/currency [(keyword (or currency "USD")) :symbol]))
(when (and inline-currency? (= currency-style "code"))
(str (get-in currency/currency [(keyword (or currency "USD")) :code]) \space))
......
src/metabase/query_processor/streaming/common.clj
+ 25
− 19
View file @ c35f2503
......@@ -78,8 +78,11 @@
"Given the format settings for a currency column, returns the symbol, code or name for the
appropriate currency."
[format-settings]
(let [currency-code (::mb.viz/currency format-settings "USD")]
(condp = (::mb.viz/currency-style format-settings "symbol")
(let [currency-code (or (::mb.viz/currency format-settings)
(:currency format-settings "USD"))]
(condp = (or (::mb.viz/currency-style format-settings)
(:currency_style format-settings)
"symbol")
"symbol"
(if (currency/supports-symbol? currency-code)
(get-in currency/currency [(keyword currency-code) :symbol])
......@@ -105,9 +108,11 @@
(get col-settings' {::mb.viz/column-name (:name col)}))
is-currency? (or (isa? (:semantic_type col) :type/Currency)
(= (::mb.viz/number-style format-settings) "currency"))
merged-settings (if is-currency?
(merge-global-settings format-settings :type/Currency)
format-settings)
merged-settings (merge
(:settings col)
(if is-currency?
(merge-global-settings format-settings :type/Currency)
format-settings))
column-title (or (when format-rows? (::mb.viz/column-title merged-settings))
(:display_name col)
(:name col))]
......@@ -188,18 +193,19 @@
;; and not have any metadata. Since we don't know the metadata, we can never
;; match a key with metadata, even if we do have the correct name or id
(update-keys #(select-keys % [::mb.viz/field-id ::mb.viz/column-name])))
column-settings (or (all-cols-settings {::mb.viz/field-id field-id-or-name})
(all-cols-settings {::mb.viz/column-name (or field-id-or-name column-name)}))]
column-settings (or (get all-cols-settings {::mb.viz/field-id field-id-or-name})
(get all-cols-settings {::mb.viz/column-name column-name})
(get all-cols-settings {::mb.viz/column-name field-id-or-name}))]
(merge
;; The default global settings based on the type of the column
(global-type-settings col viz-settings)
;; Generally, we want to look up the default global settings based on semantic or effective type. However, if
;; a user has specified other settings, we should look up the base type of those settings and combine them.
(column-setting-defaults global-column-settings column-settings)
;; User defined metadata -- Note that this transformation should probably go in
;; `metabase.query-processor.middleware.results-metadata/merge-final-column-metadata
;; to prevent repetition
(mb.viz/db->norm-column-settings-entries metadata-column-settings)
;; Column settings coming from the user settings in the ui
;; (E.g. Click the ⚙️on the column)
column-settings)))
;; The default global settings based on the type of the column
(global-type-settings col viz-settings)
;; Generally, we want to look up the default global settings based on semantic or effective type. However, if
;; a user has specified other settings, we should look up the base type of those settings and combine them.
(column-setting-defaults global-column-settings column-settings)
;; User defined metadata -- Note that this transformation should probably go in
;; `metabase.query-processor.middleware.results-metadata/merge-final-column-metadata
;; to prevent repetition
(mb.viz/db->norm-column-settings-entries metadata-column-settings)
;; Column settings coming from the user settings in the ui
;; (E.g. Click the ⚙️ on the column)
column-settings)))
test/metabase/api/downloads_exports_test.clj
+ 107
− 2
View file @ c35f2503
......@@ -21,13 +21,30 @@
[metabase.query-processor.streaming.xlsx :as qp.xlsx]
[metabase.test :as mt])
(:import
(org.apache.poi.xssf.usermodel XSSFSheet)))
(org.apache.poi.xssf.usermodel XSSFSheet)
(org.apache.poi.ss.usermodel DataFormatter)))
(def ^:private cell-formatter (DataFormatter.))
(defn- read-cell-with-formatting
[c]
(.formatCellValue cell-formatter c))
(defn- read-xlsx
[result]
(with-open [in (io/input-stream result)]
(->> (spreadsheet/load-workbook in)
(spreadsheet/select-sheet "Query result")
(spreadsheet/row-seq)
(mapv (fn [r]
(->> (spreadsheet/cell-seq r)
(mapv read-cell-with-formatting)))))))
(defn- process-results
[export-format results]
(when (seq results)
(case export-format
:csv (csv/read-csv results))))
:csv (csv/read-csv results)
:xlsx (read-xlsx results))))
(defn- card-download
[{:keys [id] :as _card} export-format format-rows?]
......@@ -600,3 +617,91 @@
:subscription-attachment subscription-header}
{:alert-attachment (first alert-result)
:subscription-attachment (first subscription-result)}))))))))
(deftest ^:parallel model-viz-settings-downloads-test
(testing "A model's visualization settings are respected in downloads."
(testing "for csv"
(mt/dataset test-data
(mt/with-temp [:model/Card card {:display :table
:type :model
:dataset_query {:database (mt/id)
:type :query
:query {:source-table (mt/id :orders)
:limit 10}}
:visualization_settings {:table.cell_column "SUBTOTAL"}
:result_metadata [{:description
"The raw, pre-tax cost of the order."
:semantic_type :type/Currency
:coercion_strategy nil
:name "SUBTOTAL"
:settings {:currency_style "code"
:currency "CAD"
:scale 0.01}
:fk_target_field_id nil
:field_ref [:field (mt/id :orders :subtotal) nil]
:effective_type :type/Float
:id (mt/id :orders :subtotal)
:visibility_type :normal
:display_name "Subtotal"
:base_type :type/Float}]}]
(let [card-result (card-download card :csv true)
dashcard-result (dashcard-download card :csv true)]
(is (= {:card-download ["Subtotal (CAD)" "0.38"]
:dashcard-download ["Subtotal (CAD)" "0.38"]}
{:card-download (mapv #(nth % 3) (take 2 card-result))
:dashcard-download (mapv #(nth % 3) (take 2 dashcard-result))}))))))))
(deftest column-settings-on-aggregated-columns-test
(testing "Column settings on aggregated columns are applied"
(mt/dataset test-data
(mt/with-temp [:model/Card card {:display :table
:type :model
:dataset_query {:database (mt/id)
:type :query
:query {:source-table (mt/id :products)
:aggregation [[:sum [:field (mt/id :products :price) {:base-type :type/Float}]]]
:breakout [[:field (mt/id :products :category) {:base-type :type/Text}]]
:limit 10}}
:visualization_settings {:column_settings
{"[\"name\",\"sum\"]"
{:number_style "currency"
:currency "CAD"
:currency_style "name"
:currency_in_header false}}}}]
(testing "for csv"
(is (= "2,185.89 Canadian dollars"
(-> (card-download card :csv true) second second))))
(testing "for xlsx (#43039)"
(is (= "2,185.89 Canadian dollars"
(-> (card-download card :xlsx true) second second))))))))
(deftest table-metadata-affects-column-formatting-properly
(testing "A Table's configured metadata (eg. Semantic Type of currency) can affect column formatting"
(mt/dataset test-data
(mt/with-temp [:model/Card card {:display :table
:type :model
:dataset_query {:database (mt/id)
:type :query
:query {:source-table (mt/id :orders)
:filter [:not-null [:field (mt/id :orders :discount) {:base-type :type/Float}]]
:limit 1}}
:visualization_settings {:table.columns
[{:name "ID" :enabled false}
{:name "USER_ID" :enabled false}
{:name "PRODUCT_ID" :enabled false}
{:name "SUBTOTAL" :enabled false}
{:name "TAX" :enabled false}
{:name "TOTAL" :enabled false}
{:name "DISCOUNT" :enabled true}
{:name "CREATED_AT" :enabled false}
{:name "QUANTITY" :enabled false}]
:table.cell_column "SUBTOTAL"
:column_settings {(format "[\"ref\",[\"field\",%s,null]]" (mt/id :orders :discount))
{:currency_in_header false}}}}]
(testing "for csv"
(is (= [["Discount"] ["$6.42"]]
(-> (card-download card :csv true)))))
(testing "for xlsx"
;; the [$$] part will appear as $ when you open the Excel file in a spreadsheet app
(is (= [["Discount"] ["[$$]6.42"]]
(-> (card-download card :xlsx true)))))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment