From fac827fab93037d810d4dced55eadc9119ccb7c5 Mon Sep 17 00:00:00 2001
From: Jerry Huang <34140255+qwef@users.noreply.github.com>
Date: Wed, 16 Aug 2023 19:31:05 -0700
Subject: [PATCH] Hide columns from SQL Query such that they will take effect
in dashboard subscription email/slack (#32991)
* initial columns
* fix tests
* only filter when exists
* add test
* add whitespace
* add noah code
* fix test
* add column reordering
* fix issues with column ordering
* rename test
* Update src/metabase/pulse/render/body.clj
Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com>
---------
Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com>
---
src/metabase/pulse/render.clj | 13 +++-
src/metabase/pulse/render/body.clj | 90 +++++++++++-----------
src/metabase/query_processor/streaming.clj | 2 +-
test/metabase/pulse/render/table_test.clj | 57 ++++++++++++++
4 files changed, 114 insertions(+), 48 deletions(-)
diff --git a/src/metabase/pulse/render.clj b/src/metabase/pulse/render.clj
index 32001ce9e38..b2c3cfaeee2 100644
--- a/src/metabase/pulse/render.clj
+++ b/src/metabase/pulse/render.clj
@@ -8,6 +8,7 @@
[metabase.pulse.render.image-bundle :as image-bundle]
[metabase.pulse.render.png :as png]
[metabase.pulse.render.style :as style]
+ [metabase.shared.models.visualization-settings :as mb.viz]
[metabase.util.i18n :refer [trs tru]]
[metabase.util.log :as log]
[metabase.util.urls :as urls]
@@ -149,11 +150,17 @@
- render/text : raw text suitable for substituting on clients when text is preferable. (Currently slack uses this for
scalar results where text is preferable to an image of a div of a single result."
[render-type timezone-id :- (s/maybe s/Str) card dashcard results]
- (let [{title :content, title-attachments :attachments} (make-title-if-needed render-type card dashcard)
- {description :content} (make-description-if-needed dashcard card)
+ (let [{title :content
+ title-attachments :attachments} (make-title-if-needed render-type card dashcard)
+ {description :content} (make-description-if-needed dashcard card)
+ results (update-in results
+ [:data :viz-settings]
+ (fn [viz-settings]
+ (merge viz-settings (mb.viz/db->norm
+ (:visualization_settings dashcard)))))
{pulse-body :content
body-attachments :attachments
- text :render/text} (render-pulse-card-body render-type timezone-id card dashcard results)]
+ text :render/text} (render-pulse-card-body render-type timezone-id card dashcard results)]
(cond-> {:attachments (merge title-attachments body-attachments)
:content [:p
;; Provide a horizontal scrollbar for tables that overflow container width.
diff --git a/src/metabase/pulse/render/body.clj b/src/metabase/pulse/render/body.clj
index 6ba87d85889..cb0ec3186ec 100644
--- a/src/metabase/pulse/render/body.clj
+++ b/src/metabase/pulse/render/body.clj
@@ -12,6 +12,7 @@
[metabase.pulse.render.style :as style]
[metabase.pulse.render.table :as table]
[metabase.pulse.util :as pu]
+ [metabase.query-processor.streaming :as qp.streaming]
[metabase.shared.models.visualization-settings :as mb.viz]
[metabase.types :as types]
[metabase.util :as u]
@@ -213,15 +214,27 @@
{:arglists '([chart-type render-type timezone-id card dashcard data])}
(fn [chart-type _ _ _ _ _] chart-type))
+(defn- filter-rows [data table-columns rows]
+ (let [column-order (qp.streaming/export-column-order (:cols data) table-columns)
+ keep-filtered-idx (fn [col] (map #(nth col %) column-order))
+ filtered-rows (map #(update % :row keep-filtered-idx) rows)]
+ filtered-rows))
+
(s/defmethod render :table :- common/RenderedPulseCard
- [_ render-type timezone-id :- (s/maybe s/Str) card dashcard {:keys [rows viz-settings] :as data}]
- (let [viz-settings (merge viz-settings (:visualization_settings dashcard))
- table-body [:div
- (table/render-table
- (color/make-color-selector data viz-settings)
- (mapv :name (:cols data))
- (prep-for-html-rendering timezone-id card data))
- (render-truncation-warning rows-limit (count rows))]]
+ [_ render-type timezone-id :- (s/maybe s/Str) card _dashcard {:keys [rows viz-settings] :as data}]
+ (let [table-columns (::mb.viz/table-columns viz-settings)
+ filter-columns? (some? table-columns)
+ filtered-columns-names (if filter-columns?
+ (mapv ::mb.viz/table-column-name (filter ::mb.viz/table-column-enabled table-columns))
+ (mapv :name (:cols data)))
+ filtered-rows (cond->> (prep-for-html-rendering timezone-id card data)
+ filter-columns? (filter-rows data table-columns))
+ table-body [:div
+ (table/render-table
+ (color/make-color-selector data viz-settings)
+ filtered-columns-names
+ filtered-rows)
+ (render-truncation-warning rows-limit (count rows))]]
{:attachments
nil
@@ -460,12 +473,10 @@
rows))
(s/defmethod render :categorical/donut :- common/RenderedPulseCard
- [_ render-type timezone-id :- (s/maybe s/Str) card dashcard {:keys [rows cols viz-settings] :as data}]
- (let [rows (replace-nils rows)
- viz-settings (merge viz-settings (:visualization_settings dashcard))
- [x-axis-rowfn y-axis-rowfn] (common/graphing-column-row-fns card data)
+ [_ render-type timezone-id :- (s/maybe s/Str) card _dashcard {:keys [rows cols viz-settings] :as data}]
+ (let [[x-axis-rowfn y-axis-rowfn] (common/graphing-column-row-fns card data)
rows (map (juxt (comp str x-axis-rowfn) y-axis-rowfn)
- (common/row-preprocess x-axis-rowfn y-axis-rowfn rows))
+ (common/row-preprocess x-axis-rowfn y-axis-rowfn (replace-nils rows)))
slice-threshold (or (get viz-settings :pie.slice_threshold)
2.5)
{:keys [rows percentages]} (donut-info slice-threshold rows)
@@ -500,9 +511,8 @@
rows))]}))
(s/defmethod render :progress :- common/RenderedPulseCard
- [_ render-type _timezone-id _card dashcard {:keys [cols rows viz-settings] :as _data}]
- (let [viz-settings (merge viz-settings (:visualization_settings dashcard))
- value (ffirst rows)
+ [_ render-type _timezone-id _card _dashcard {:keys [cols rows viz-settings] :as _data}]
+ (let [value (ffirst rows)
goal (:progress.goal viz-settings)
color (:progress.color viz-settings)
settings (assoc
@@ -640,8 +650,7 @@
(defn- render-multiple-scalars
"When multiple scalar cards are combined, they render as a bar chart"
[render-type card dashcard {:keys [viz-settings] :as data}]
- (let [viz-settings (merge viz-settings (:visualization_settings dashcard))
- multi-res (pu/execute-multi-card card dashcard)
+ (let [multi-res (pu/execute-multi-card card dashcard)
cards (cons card (map :card multi-res))
multi-data (cons data (map #(get-in % [:result :data]) multi-res))
x-rows (map :name cards) ;; Bar labels
@@ -779,10 +788,8 @@
(defn- render-multiple-lab-chart
"When multiple non-scalar cards are combined, render them as a line, area, or bar chart"
- [render-type card dashcard {:keys [viz-settings]
- :as data}]
- (let [viz-settings (merge viz-settings (:visualization_settings dashcard))
- multi-res (pu/execute-multi-card card dashcard)
+ [render-type card dashcard {:keys [viz-settings] :as data}]
+ (let [multi-res (pu/execute-multi-card card dashcard)
;; multi-res gets the other results from the set of multis.
;; we shove cards and data here all together below for uniformity's sake
viz-settings (set-default-stacked viz-settings card)
@@ -799,9 +806,8 @@
"Generate an image-bundle for a Line Area Bar chart (LAB)
Use the combo charts for every chart-type in line area bar because we get multiple chart series for cheaper this way."
- [chart-type render-type _timezone-id card dashcard {:keys [cols rows viz-settings] :as data}]
+ [chart-type render-type _timezone-id card {:keys [cols rows viz-settings] :as data}]
(let [rows (replace-nils rows)
- viz-settings (merge viz-settings (:visualization_settings dashcard))
x-axis-rowfn (or (ui-logic/mult-x-axis-rowfn card data) #(vector (first %)))
y-axis-rowfn (or (ui-logic/mult-y-axis-rowfn card data) #(vector (second %)))
x-rows (filter some? (map x-axis-rowfn rows))
@@ -831,20 +837,20 @@
render-type card dashcard data))
(s/defmethod render :line :- common/RenderedPulseCard
- [_ render-type timezone-id card dashcard data]
- (attach-image-bundle (lab-image-bundle :line render-type timezone-id card dashcard data)))
+ [_ render-type timezone-id card _dashcard data]
+ (attach-image-bundle (lab-image-bundle :line render-type timezone-id card data)))
(s/defmethod render :area :- common/RenderedPulseCard
- [_ render-type timezone-id card dashcard data]
- (attach-image-bundle (lab-image-bundle :area render-type timezone-id card dashcard data)))
+ [_ render-type timezone-id card _dashcard data]
+ (attach-image-bundle (lab-image-bundle :area render-type timezone-id card data)))
(s/defmethod render :bar :- common/RenderedPulseCard
- [_chart-type render-type timezone-id :- (s/maybe s/Str) card dashcard data]
- (attach-image-bundle (lab-image-bundle :bar render-type timezone-id card dashcard data)))
+ [_chart-type render-type timezone-id :- (s/maybe s/Str) card _dashcard data]
+ (attach-image-bundle (lab-image-bundle :bar render-type timezone-id card data)))
(s/defmethod render :combo :- common/RenderedPulseCard
- [_chart-type render-type timezone-id :- (s/maybe s/Str) card dashcard data]
- (attach-image-bundle (lab-image-bundle :combo render-type timezone-id card dashcard data)))
+ [_chart-type render-type timezone-id :- (s/maybe s/Str) card _dashcard data]
+ (attach-image-bundle (lab-image-bundle :combo render-type timezone-id card data)))
(s/defmethod render :gauge :- common/RenderedPulseCard
[_chart-type render-type _timezone-id :- (s/maybe s/Str) card _dashcard data]
@@ -878,9 +884,8 @@
:src (:image-src image-bundle)}]]}))
(s/defmethod render :scalar :- common/RenderedPulseCard
- [_chart-type _render-type timezone-id _card dashcard {:keys [cols rows viz-settings]}]
- (let [viz-settings (merge viz-settings (:visualization_settings dashcard))
- value (format-cell timezone-id (ffirst rows) (first cols) viz-settings)]
+ [_chart-type _render-type timezone-id _card _dashcard {:keys [cols rows viz-settings]}]
+ (let [value (format-cell timezone-id (ffirst rows) (first cols) viz-settings)]
{:attachments
nil
@@ -890,7 +895,7 @@
:render/text (str value)}))
(s/defmethod render :smartscalar :- common/RenderedPulseCard
- [_chart-type _render-type timezone-id _card dashcard {:keys [cols insights viz-settings]}]
+ [_chart-type _render-type timezone-id _card _dashcard {:keys [cols 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)))
@@ -899,8 +904,7 @@
(format-percentage arg)
" - "))
(format-unit [unit] (str/replace (name unit) "-" " "))]
- (let [viz-settings (merge viz-settings (:visualization_settings dashcard))
- [_time-col metric-col] (if (col-of-type :type/Temporal (first cols)) cols (reverse cols))
+ (let [[_time-col metric-col] (if (col-of-type :type/Temporal (first cols)) cols (reverse cols))
{:keys [last-value previous-value unit last-change] :as _insight}
(where (comp #{(:name metric-col)} :col) insights)]
@@ -938,9 +942,8 @@
"\n" (trs "Nothing to compare to."))}))))
(s/defmethod render :waterfall :- common/RenderedPulseCard
- [_ render-type _timezone-id card dashcard {:keys [rows cols viz-settings] :as data}]
- (let [viz-settings (merge viz-settings (:visualization_settings dashcard))
- [x-axis-rowfn
+ [_ render-type _timezone-id card _dashcard {:keys [rows cols viz-settings] :as data}]
+ (let [[x-axis-rowfn
y-axis-rowfn] (common/graphing-column-row-fns card data)
[x-col y-col] ((juxt x-axis-rowfn y-axis-rowfn) cols)
rows (map (juxt x-axis-rowfn y-axis-rowfn)
@@ -975,9 +978,8 @@
:src (:image-src image-bundle)}]]}))
(s/defmethod render :funnel :- common/RenderedPulseCard
- [_ render-type _timezone-id card dashcard {:keys [rows cols viz-settings] :as data}]
- (let [viz-settings (merge viz-settings (:visualization_settings dashcard))
- [x-axis-rowfn
+ [_ render-type _timezone-id card _dashcard {:keys [rows cols viz-settings] :as data}]
+ (let [[x-axis-rowfn
y-axis-rowfn] (common/graphing-column-row-fns card data)
rows (map (juxt x-axis-rowfn y-axis-rowfn)
(common/row-preprocess x-axis-rowfn y-axis-rowfn rows))
diff --git a/src/metabase/query_processor/streaming.clj b/src/metabase/query_processor/streaming.clj
index 74d2d97a3bf..373e70c1854 100644
--- a/src/metabase/query_processor/streaming.clj
+++ b/src/metabase/query_processor/streaming.clj
@@ -50,7 +50,7 @@
table-columns)
table-columns)))
-(defn- export-column-order
+(defn export-column-order
"For each entry in `table-columns` that is enabled, finds the index of the corresponding
entry in `cols` by name or id. If a col has been remapped, uses the index of the new column.
diff --git a/test/metabase/pulse/render/table_test.clj b/test/metabase/pulse/render/table_test.clj
index c3d9af6683c..81e71dec307 100644
--- a/test/metabase/pulse/render/table_test.clj
+++ b/test/metabase/pulse/render/table_test.clj
@@ -1,6 +1,7 @@
(ns metabase.pulse.render.table-test
(:require
[clojure.test :refer :all]
+ [metabase.pulse.render :as render]
[metabase.pulse.render.color :as color]
[metabase.pulse.render.common :as common]
[metabase.pulse.render.table :as table]
@@ -155,3 +156,59 @@
render.tu/remove-attrs
(render.tu/nodes-with-tag :td)
(->> (map second)))))))))
+
+(defn- render-table [dashcard results]
+ (render/render-pulse-card :attachment "America/Los_Angeles" render.tu/test-card dashcard results))
+
+(deftest render-table-columns-test
+ (testing "Disabling a column has the same effect as not having the column at all."
+ (is (=
+ (render-table
+ {:visualization_settings {:table.columns
+ [{:name "b" :enabled true}]}}
+ {:data {:cols [{:name "b",
+ :display_name "b",
+ :base_type :type/BigInteger
+ :semantic_type nil}]
+ :rows [[2] [4]]}})
+ (render-table
+ {:visualization_settings {:table.columns
+ [{:name "a" :enabled false}
+ {:name "b" :enabled true}]}}
+ {:data {:cols [{:name "a",
+ :display_name "a",
+ :base_type :type/BigInteger
+ :semantic_type nil}
+ {:name "b",
+ :display_name "b",
+ :base_type :type/BigInteger
+ :semantic_type nil}]
+ :rows [[1 2] [3 4]]}}))))
+ (testing "Column order in table.columns is respected."
+ (is (=
+ (render-table
+ {:visualization_settings {:table.columns
+ [{:name "b" :enabled true}
+ {:name "a" :enabled true}]}}
+ {:data {:cols [{:name "a",
+ :display_name "a",
+ :base_type :type/BigInteger
+ :semantic_type nil}
+ {:name "b",
+ :display_name "b",
+ :base_type :type/BigInteger
+ :semantic_type nil}]
+ :rows [[1 2] [3 4]]}})
+ (render-table
+ {:visualization_settings {:table.columns
+ [{:name "b" :enabled true}
+ {:name "a" :enabled true}]}}
+ {:data {:cols [{:name "b",
+ :display_name "b",
+ :base_type :type/BigInteger
+ :semantic_type nil}
+ {:name "a",
+ :display_name "a",
+ :base_type :type/BigInteger
+ :semantic_type nil}]
+ :rows [[2 1] [4 3]]}})))))
--
GitLab