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

Some Email/Attachment Improvements and Fixes (#45691) 

* fix conditional formatting color lookup with correct col names

The column names for color lookup are expected to be the same as the :name key of each column, and applying column
formatting broke this lookup. This fixes it.

* move overflow-x into pulse body div to exlude card title from scroll

* add a test for conditional formatting w/ a hidden row.

This still fails at the moment, working on a fix yet.

* apply conditional formatting in static viz even when col is hidden

Conditional formatting that depends on a hidden column was not being rendered in the static viz, but now we pass all
columns into the color selector fn, thus when rendering the table with the ordered/hidden columns, all other columns
will still use their appropriate conditional formatting

* I missed a linter warning

* fix tab rendering

* fix test

* fix up tests with some better structural searching
parent 77619a50
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 187 additions and 44 deletions
src/metabase/models/dashboard.clj
+ 3
− 2
View file @ 7efce7df
......@@ -386,9 +386,10 @@
(->> (filter identity)))))
(defn has-tabs?
"Check if a dashboard has tabs."
"Check if a dashboard has more than 1 tab.
We don't need to render the tab title if only 1 exists (issue #45123)."
[dashboard-or-id]
(t2/exists? :model/DashboardTab :dashboard_id (u/the-id dashboard-or-id)))
(< 1 (t2/count :model/DashboardTab :dashboard_id (u/the-id dashboard-or-id))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | OTHER CRUD FNS |
......
src/metabase/pulse.clj
+ 8
− 3
View file @ 7efce7df
......@@ -155,9 +155,14 @@
(let [dashboard-id (u/the-id dashboard)]
(mw.session/with-current-user pulse-creator-id
(let [parts (if (dashboard/has-tabs? dashboard)
(let [tabs-with-cards (t2/hydrate (t2/select :model/DashboardTab :dashboard_id dashboard-id) :tab-cards)]
(doall (flatten (for [{:keys [cards] :as tab} tabs-with-cards]
(concat [(tab->part tab)] (dashcards->part cards pulse dashboard))))))
(let [tabs (t2/hydrate (t2/select :model/DashboardTab :dashboard_id dashboard-id) :tab-cards)
tabs-with-cards (filter #(seq (:cards %)) tabs)]
(doall (flatten (for [{:keys [cards] :as tab} tabs-with-cards
:when (seq cards)]
(concat
(when (< 1 (count tabs-with-cards))
[(tab->part tab)])
(dashcards->part cards pulse dashboard))))))
(dashcards->part (t2/select :model/DashboardCard :dashboard_id dashboard-id) pulse dashboard))]
(if skip_if_empty
;; Remove cards that have no results when empty results aren't wanted
......
src/metabase/pulse/render.clj
+ 3
− 2
View file @ 7efce7df
......@@ -173,7 +173,7 @@
:content [:p
;; Provide a horizontal scrollbar for tables that overflow container width.
;; Surrounding <p> element prevents buggy behavior when dragging scrollbar.
[:div {:style (style/style {:overflow-x :auto})}
[:div
[:a {:href (card-href card)
:target "_blank"
:rel "noopener noreferrer"
......@@ -184,7 +184,8 @@
title
description
[:div {:class "pulse-body"
:style (style/style {:display :block
:style (style/style {:overflow-x :auto
:display :block
:margin :16px})}
(if-let [more-results-message (body/attached-results-text render-type card)]
(conj more-results-message (list pulse-body))
......
src/metabase/pulse/render/body.clj
+ 6
− 5
View file @ 7efce7df
......@@ -226,15 +226,16 @@
timezone-id :- [:maybe :string]
card
_dashcard
{:keys [rows viz-settings format-rows?] :as data}]
(let [[ordered-cols ordered-rows] (order-data data viz-settings)
data (-> data
{:keys [rows viz-settings format-rows?] :as unordered-data}]
(let [[ordered-cols ordered-rows] (order-data unordered-data viz-settings)
data (-> unordered-data
(assoc :rows ordered-rows)
(assoc :cols ordered-cols))
table-body [:div
(table/render-table
(color/make-color-selector data viz-settings)
(common/column-titles ordered-cols (::mb.viz/column-settings viz-settings) format-rows?)
(color/make-color-selector unordered-data viz-settings)
{:cols-for-color-lookup (mapv :name ordered-cols)
:col-names (common/column-titles ordered-cols (::mb.viz/column-settings viz-settings) format-rows?)}
(prep-for-html-rendering timezone-id card data))
(render-truncation-warning (public-settings/attachment-table-row-limit) (count rows))]]
{:attachments
......
src/metabase/pulse/render/table.clj
+ 5
− 5
View file @ 7efce7df
......@@ -144,10 +144,10 @@
background color for a given cell. `column-names` is different from the header in `header+rows` as the header is the
display_name (i.e. human friendly. `header+rows` includes the text contents of the table we're about ready to
create. If `normalized-zero` is set (defaults to 0), render values less than it as negative"
([color-selector column-names contents]
(render-table color-selector 0 column-names contents))
([color-selector column-names-map contents]
(render-table color-selector 0 column-names-map contents))
([color-selector normalized-zero column-names [header & rows]]
([color-selector normalized-zero {:keys [col-names cols-for-color-lookup]} [header & rows]]
[:table {:style (style/style {:max-width "100%"
:white-space :nowrap
:border (str "1px solid " style/color-border)
......@@ -155,5 +155,5 @@
:width "1%"})
:cellpadding "0"
:cellspacing "0"}
(render-table-head (vec column-names) header)
(render-table-body (partial color/get-background-color color-selector) normalized-zero column-names rows)]))
(render-table-head (vec col-names) header)
(render-table-body (partial color/get-background-color color-selector) normalized-zero cols-for-color-lookup rows)]))
test/metabase/dashboard_subscription_test.clj
+ 46
− 0
View file @ 7efce7df
......@@ -812,6 +812,52 @@
{:text "Card 2 tab-2", :type :text}]
(@#'metabase.pulse/execute-dashboard {:creator_id (mt/user->id :rasta)} dashboard))))))
(deftest execute-dashboard-with-empty-tabs-test
(testing "Dashboard with one tab."
(t2.with-temp/with-temp
[Dashboard {dashboard-id :id
:as dashboard} {:name "Dashboard"}
:model/DashboardTab {} {:name "The second tab"
:position 1
:dashboard_id dashboard-id}
:model/DashboardTab {tab-id-1 :id} {:name "The first tab"
:position 0
:dashboard_id dashboard-id}
DashboardCard _ {:dashboard_id dashboard-id
:dashboard_tab_id tab-id-1
:row 2
:visualization_settings {:text "Card 2 tab-1"}}
DashboardCard _ {:dashboard_id dashboard-id
:dashboard_tab_id tab-id-1
:row 1
:visualization_settings {:text "Card 1 tab-1"}}]
(testing "Tab title is omitted (#45123)"
(is (= [{:text "Card 1 tab-1", :type :text}
{:text "Card 2 tab-1", :type :text}]
(@#'metabase.pulse/execute-dashboard {:creator_id (mt/user->id :rasta)} dashboard))))))
(testing "Dashboard with multiple tabs"
(t2.with-temp/with-temp
[Dashboard {dashboard-id :id
:as dashboard} {:name "Dashboard"}
:model/DashboardTab {} {:name "The second tab"
:position 1
:dashboard_id dashboard-id}
:model/DashboardTab {tab-id-1 :id} {:name "The first tab"
:position 0
:dashboard_id dashboard-id}
DashboardCard _ {:dashboard_id dashboard-id
:dashboard_tab_id tab-id-1
:row 2
:visualization_settings {:text "Card 2 tab-1"}}
DashboardCard _ {:dashboard_id dashboard-id
:dashboard_tab_id tab-id-1
:row 1
:visualization_settings {:text "Card 1 tab-1"}}]
(testing "Tab title is omitted when only 1 tab contains cards."
(is (= [{:text "Card 1 tab-1", :type :text}
{:text "Card 2 tab-1", :type :text}]
(@#'metabase.pulse/execute-dashboard {:creator_id (mt/user->id :rasta)} dashboard)))))))
(deftest render-dashboard-with-tabs-test
(tests! {:pulse {:skip_if_empty false}
:dashboard pulse.test-util/test-dashboard}
......
test/metabase/pulse/render/body_test.clj
+ 77
− 0
View file @ 7efce7df
......@@ -894,3 +894,80 @@
:dashcard dashcard-header}
{:card (mapcat :content card-header-els)
:dashcard (mapcat :content dash-header-els)}))))))))
(deftest table-renders-respect-conditional-formatting
(testing "Rendered Tables respect the conditional formatting on a card."
(let [ids-to-colour [1 2 3 5 8 13]]
(mt/dataset test-data
(mt/with-temp [:model/Card {card-id :id} {:display :table
:dataset_query {:database (mt/id)
:type :query
:query {:source-table (mt/id :orders)}}
:visualization_settings
{:table.column_formatting
(into []
(map-indexed
(fn [idx id-to-colour]
{:columns ["ID"]
:type "single"
:operator "="
:value id-to-colour
:color "#A989C5"
:highlight_row false
:id idx})
ids-to-colour))}}]
(mt/with-current-user (mt/user->id :rasta)
(let [card-doc (render.tu/render-card-as-hickory card-id)
card-row-els (hik.s/select (hik.s/tag :tr) card-doc)]
(is (= (mapv str ids-to-colour)
(keep
(fn [{:keys [attrs] :as el}]
(let [style-str (:style attrs)]
(when (str/includes? style-str "background-color")
(-> el :content first))))
(mapcat :content (take 20 card-row-els))))))))))))
(deftest table-renders-conditional-formatting-even-with-hidden-column
(testing "Rendered Tables respect the conditional formatting on a card."
(let [ids-to-colour [1 2 3 5 8 13]]
(mt/dataset test-data
(mt/with-temp [:model/Card {card-id :id} {:display :table
:dataset_query {:database (mt/id)
:type :query
:query {:source-table (mt/id :orders)}}
:visualization_settings
{:table.columns
[{:name "ID" :enabled false}
{:name "TOTAL" :enabled true}
{:name "TAX" :enabled true}
{:name "USER_ID" :enabled true}
{:name "CREATED_AT" :enabled true}
{:name "QUANTITY" :enabled true}
{:name "SUBTOTAL" :enabled true}
{:name "PRODUCT_ID" :enabled true}
{:name "DISCOUNT" :enabled true}]
:table.column_formatting
(into []
(map-indexed
(fn [idx id-to-colour]
{:columns ["ID"]
:type "single"
:operator "="
:value id-to-colour
:color "#A989C5"
:highlight_row true
:id idx})
ids-to-colour))}}]
(mt/with-current-user (mt/user->id :rasta)
(let [card-doc (render.tu/render-card-as-hickory card-id)
card-row-els (hik.s/select (hik.s/tag :tr) card-doc)]
(is (= ids-to-colour
(keep
(fn [[id row-els]]
(let [{:keys [attrs]} (first row-els)
style-str (:style attrs)]
(when (str/includes? style-str "background-color")
id)))
(map vector
(range)
(map :content (take 20 card-row-els)))))))))))))
test/metabase/pulse/render/table_test.clj
+ 2
− 1
View file @ 7efce7df
......@@ -82,7 +82,8 @@
"1.001,5" "rgba(0, 0, 255, 0.75)"
"1,001.5" "rgba(0, 0, 255, 0.75)"}
(-> (color/make-color-selector query-results (:visualization_settings render.tu/test-card))
(#'table/render-table 0 ["a" "b" "c"] (query-results->header+rows query-results))
(#'table/render-table 0 {:col-names ["a" "b" "c"]
:cols-for-color-lookup ["a" "b" "c"]} (query-results->header+rows query-results))
find-table-body
cell-value->background-color)))))
......
test/metabase/pulse/render_test.clj
+ 37
− 26
View file @ 7efce7df
(ns metabase.pulse.render-test
(:require
[clojure.test :refer :all]
[hiccup.core :as hiccup]
[hickory.core :as hik]
[hickory.select :as hik.s]
[metabase.lib.util.match :as lib.util.match]
[metabase.models
:refer [Card Dashboard DashboardCard DashboardCardSeries]]
[metabase.pulse :as pulse]
[metabase.pulse.render :as render]
[metabase.pulse.render.test-util :as render.tu]
[metabase.query-processor :as qp]
[metabase.test :as mt]
[metabase.util :as u]
......@@ -22,6 +26,13 @@
([card results]
(render/render-pulse-card-for-display (pulse/defaulted-timezone card) card results)))
(defn- hiccup->hickory
[content]
(-> content
hiccup/html
hik/parse
hik/as-hickory))
(deftest render-test
(testing "If the pulse renders correctly, it will have an img tag."
(let [query {:database (mt/id)
......@@ -40,10 +51,11 @@
(deftest render-error-test
(testing "gives us a proper error if we have erroring card"
(is (= (get-in (render/render-pulse-card-for-display
nil nil
{:error "some error"}) [1 2 4 2 2])
"There was a problem with this question."))))
(let [rendered-card (render/render-pulse-card-for-display nil nil {:error "some error"})]
(is (= "There was a problem with this question."
(-> (render.tu/nodes-with-text rendered-card "There was a problem with this question.")
first
last))))))
(deftest detect-pulse-chart-type-test
(testing "Currently unsupported chart types for static-viz return `nil`."
......@@ -189,20 +201,20 @@
model-query :dataset_query
model-metadata :result_metadata
:as model-card} {:type :model
:dataset_query {:type :query
:database (mt/id)
:query {:source-table (format "card__%s" base-card-id)}}
:result_metadata [{:name "TAX"
:display_name "Tax"
:base_type :type/Float}
{:name "TOTAL"
:display_name "Total"
:base_type :type/Float}
{:name "Tax Rate"
:display_name "Tax Rate"
:base_type :type/Float
:semantic_type :type/Percentage
:field_ref [:field "Tax Rate" {:base-type :type/Float}]}]}
:dataset_query {:type :query
:database (mt/id)
:query {:source-table (format "card__%s" base-card-id)}}
:result_metadata [{:name "TAX"
:display_name "Tax"
:base_type :type/Float}
{:name "TOTAL"
:display_name "Total"
:base_type :type/Float}
{:name "Tax Rate"
:display_name "Tax Rate"
:base_type :type/Float
:semantic_type :type/Percentage
:field_ref [:field "Tax Rate" {:base-type :type/Float}]}]}
Card {question-query :dataset_query
:as question-card} {:dataset_query {:type :query
:database (mt/id)
......@@ -215,17 +227,16 @@
(format "%.2f%%" (* 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 (get-in rendered-card [:content 1 2 4 2 1])
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))))]
doc (hiccup->hickory (:content rendered-card))
rows (hik.s/select (hik.s/tag :tr) doc)
tax-rate-col 2]
{:expected expected
:actual (->> (get-in table [3 1])
(map #(peek (get (vec (get % 2)) tax-col))))}))]
:actual (mapcat (fn [row]
(:content (nth row tax-rate-col)))
(map :content (rest rows)))}))]
(testing "To apply the custom metadata to a model, you must explicitly pass the result metadata"
(let [query-results (qp/process-query
(assoc-in model-query [:info :metadata/model-metadata] model-metadata))
(assoc-in model-query [:info :metadata/model-metadata] model-metadata))
{:keys [expected actual]} (create-comparison-results query-results model-card)]
(is (= expected actual))))
(testing "A question based on a model will use the underlying model's metadata"
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment