Hide columns from SQL Query such that they will take effect in dashboard...

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

@@ -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.

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))

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.
 

test/metabase/pulse/render/table_test.clj

 (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]]}})))))