diff --git a/resources/frontend_shared/static_viz_interface.js b/resources/frontend_shared/static_viz_interface.js index ad29af3598a4dc16818dd643edd3b0d8480a0cb0..606a004fd0bd91c782a9ee4ed998a4a1aefbceda 100644 --- a/resources/frontend_shared/static_viz_interface.js +++ b/resources/frontend_shared/static_viz_interface.js @@ -47,6 +47,15 @@ function timeseries_bar(data, labels, settings) { }); } +function combo_chart(series, settings, colors) { + // Thinking of combo as similar to multiple, although they're different in BE + return StaticViz.RenderChart("combo-chart", { + series: JSON.parse(series), + settings: JSON.parse(settings), + colors: JSON.parse(colors), + }); +} + function timeseries_waterfall(data, labels, settings) { return StaticViz.RenderChart("timeseries/waterfall", { data: toJSArray(data), diff --git a/src/metabase/api/pulse.clj b/src/metabase/api/pulse.clj index f0ac21013c3ce71c68a1bab92d95a906fdb48392..7231b7761cff9bf6b943af66bde3fb3316871090 100644 --- a/src/metabase/api/pulse.clj +++ b/src/metabase/api/pulse.clj @@ -172,7 +172,7 @@ (let [card (api/read-check Card id) result (pulse-card-query-results card) data (:data result) - card-type (render/detect-pulse-chart-type card data) + card-type (render/detect-pulse-chart-type card nil data) card-html (html (binding [render/*include-title* true] (render/render-pulse-card-for-display (p/defaulted-timezone card) card result)))] {:id id diff --git a/src/metabase/email/messages.clj b/src/metabase/email/messages.clj index f61a48fc7cea0d19c6d20522b343440c1481e1e1..504637ad6d921554642699eaf0e2f6df604d07b7 100644 --- a/src/metabase/email/messages.clj +++ b/src/metabase/email/messages.clj @@ -346,7 +346,7 @@ (some (complement render.body/show-in-table?) cols) (yes "some columns are not included in rendered results") - (not= :table (render/detect-pulse-chart-type card result-data)) + (not= :table (render/detect-pulse-chart-type card nil result-data)) (no "we've determined it should not be rendered as a table") (= (count (take render.body/cols-limit cols)) render.body/cols-limit) diff --git a/src/metabase/metabot/command.clj b/src/metabase/metabot/command.clj index d3800a11ac700ed30fef9b09ac80c6b85075dcdf..1db5bafaa980db63d7b26c073672b79cba1ebf26 100644 --- a/src/metabase/metabot/command.clj +++ b/src/metabase/metabot/command.clj @@ -12,6 +12,7 @@ [metabase.models.permissions :refer [Permissions]] [metabase.models.permissions-group :as perms-group] [metabase.pulse :as pulse] + [metabase.pulse.util :as pu] [metabase.util :as u] [metabase.util.i18n :refer [deferred-tru trs tru]] [metabase.util.urls :as urls] @@ -166,7 +167,7 @@ (metabot.slack/async (let [attachments (pulse/create-and-upload-slack-attachments! (pulse/create-slack-attachment-data - [(pulse/execute-card {} card-id, :context :metabot)]))] + [(pu/execute-card {} card-id, :context :metabot)]))] (metabot.slack/post-chat-message! nil attachments)))) (tru "Ok, just a second..."))) diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj index 36fdb97d84108dad89e516918f48f6b25dd3b3c0..23027fb1ff90b5462cab373a70bd2a9d3a746904 100644 --- a/src/metabase/models/card.clj +++ b/src/metabase/models/card.clj @@ -82,6 +82,8 @@ :Segment (extract-ids :segment inner-query)}))) + + ;;; --------------------------------------------------- Revisions ---------------------------------------------------- (defn serialize-instance diff --git a/src/metabase/models/dashboard_card.clj b/src/metabase/models/dashboard_card.clj index ce556a40d57c7dd9bba5ac581d19aec8bd674cf6..52f42b7540cb9a292c7efaadbf6259e52d1290b1 100644 --- a/src/metabase/models/dashboard_card.clj +++ b/src/metabase/models/dashboard_card.clj @@ -87,6 +87,29 @@ (-> (DashboardCard id) (hydrate :series))) +(defn dashcard->multi-cards + "Return the cards which are other cards with respect to this dashboard card + in multiple series display for dashboard + + Dashboard (and dashboard only) has this thing where you're displaying multiple cards entirely. + + This is actually completely different from the combo display, + which is a visualization type in visualization option. + + This is also actually completely different from having multiple series display + from the visualization with same type (line bar or whatever), + which is a separate option in line area or bar visualization" + [dashcard] + (db/query {:select [:newcard.*] + :from [[:report_dashboardcard :dashcard]] + :left-join [[:dashboardcard_series :dashcardseries] + [:= :dashcard.id :dashcardseries.dashboardcard_id] + [:report_card :newcard] + [:= :dashcardseries.card_id :newcard.id]] + :where [:and + [:= :newcard.archived false] + [:= :dashcard.id (:id dashcard)]]})) + (s/defn update-dashboard-card-series! "Update the DashboardCardSeries for a given DashboardCard. `card-ids` should be a definitive collection of *all* IDs of cards for the dashboard card in the desired order. diff --git a/src/metabase/pulse.clj b/src/metabase/pulse.clj index 852779630efad0cd28f4b8607b080b3038aef2b3..8d3c1337f79e1a608f1e6c65c9e14f274847c143 100644 --- a/src/metabase/pulse.clj +++ b/src/metabase/pulse.clj @@ -15,8 +15,8 @@ [metabase.pulse.markdown :as markdown] [metabase.pulse.parameters :as params] [metabase.pulse.render :as render] + [metabase.pulse.util :as pu] [metabase.query-processor :as qp] - [metabase.query-processor.middleware.permissions :as qp.perms] [metabase.query-processor.timezone :as qp.timezone] [metabase.server.middleware.session :as session] [metabase.util :as u] @@ -30,36 +30,6 @@ ;;; ------------------------------------------------- PULSE SENDING -------------------------------------------------- -;; TODO - this is probably something that could live somewhere else and just be reused -;; TODO - this should be done async -(defn execute-card - "Execute the query for a single Card. `options` are passed along to the Query Processor." - [{pulse-creator-id :creator_id} card-or-id & {:as options}] - ;; The Card must either be executed in the context of a User or by the MetaBot which itself is not a User - {:pre [(or (integer? pulse-creator-id) - (= (:context options) :metabot))]} - (let [card-id (u/the-id card-or-id)] - (try - (when-let [{query :dataset_query, :as card} (Card :id card-id, :archived false)] - (let [query (assoc query :async? false) - process-query (fn [] - (binding [qp.perms/*card-id* card-id] - (qp/process-query-and-save-with-max-results-constraints! - (assoc query :middleware {:process-viz-settings? true - :js-int-to-string? false}) - (merge {:executed-by pulse-creator-id - :context :pulse - :card-id card-id} - options)))) - result (if pulse-creator-id - (session/with-current-user pulse-creator-id - (process-query)) - (process-query))] - {:card card - :result result})) - (catch Throwable e - (log/warn e (trs "Error running query for Card {0}" card-id)))))) - (defn- execute-dashboard-subscription-card [owner-id dashboard dashcard card-or-id parameters] (try @@ -149,7 +119,7 @@ (if (and card result) {:title (or (-> dashcard :visualization_settings :card.title) card-name) - :rendered-info (render/render-pulse-card :inline (defaulted-timezone card) card nil result) + :rendered-info (render/render-pulse-card :inline (defaulted-timezone card) card dashcard result) :title_link (urls/card-url card-id) :attachment-name "image.png" :channel-id channel-id @@ -374,7 +344,7 @@ ;; send the cards instead (for [card cards ;; Pulse ID may be `nil` if the Pulse isn't saved yet - :let [result (execute-card pulse (u/the-id card), :pulse-id pulse-id)] + :let [result (pu/execute-card pulse (u/the-id card), :pulse-id pulse-id)] ;; some cards may return empty results, e.g. if the card has been archived :when result] result)))) diff --git a/src/metabase/pulse/render.clj b/src/metabase/pulse/render.clj index d74fd07b526ed209ed421cc9c75fdfd907d2a311..97197f417675ca434bf09514df96a55dfa8d0951 100644 --- a/src/metabase/pulse/render.clj +++ b/src/metabase/pulse/render.clj @@ -1,6 +1,7 @@ (ns metabase.pulse.render (:require [clojure.tools.logging :as log] [hiccup.core :refer [h]] + [metabase.models.dashboard-card :as dc-model] [metabase.pulse.render.body :as body] [metabase.pulse.render.common :as common] [metabase.pulse.render.image-bundle :as image-bundle] @@ -62,7 +63,7 @@ (defn detect-pulse-chart-type "Determine the pulse (visualization) type of a `card`, e.g. `:scalar` or `:bar`." - [{display-type :display, card-name :name, :as card} {:keys [cols rows insights], :as data}] + [{display-type :display, card-name :name, :as card} maybe-dashcard {:keys [cols rows insights], :as data}] (let [col-sample-count (delay (count (take 3 cols))) row-sample-count (delay (count (take 2 rows))) [col-1-rowfn col-2-rowfn] (common/graphing-column-row-fns card data) @@ -83,7 +84,7 @@ (#{:pin_map :state :country} display-type) (chart-type nil "display-type is %s" display-type) - (#{:funnel :progress :waterfall} display-type) + (#{:progress :waterfall :combo :funnel} display-type) (chart-type display-type "display-type is %s" display-type) (= @col-sample-count @row-sample-count 1) @@ -94,6 +95,11 @@ (seq insights)) (chart-type :smartscalar "result has two columns and insights") + (and (some? maybe-dashcard) + (> (count (dc-model/dashcard->multi-cards maybe-dashcard)) 0) + (not (#{:combo} display-type))) + (chart-type :multiple "result has multiple card semantics, a multiple chart") + (and (= @col-sample-count 2) (number-field? @col-2) (= display-type :bar)) @@ -118,19 +124,19 @@ ((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 {: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) results))) - (let [chart-type (or (detect-pulse-chart-type card data) + (let [chart-type (or (detect-pulse-chart-type card dashcard data) (when (is-attached? card) :attached) :unknown)] (log/debug (trs "Rendering pulse card with chart-type {0} and render-type {1}" chart-type render-type)) - (body/render chart-type render-type timezone-id card data)) + (body/render chart-type render-type timezone-id card dashcard data)) (catch Throwable e (log/error e (trs "Pulse card render error")) - (body/render :error nil nil nil nil)))) + (body/render :error nil nil nil nil nil)))) (defn- card-href [card] @@ -148,7 +154,7 @@ {description :content} (make-description-if-needed dashcard) {pulse-body :content body-attachments :attachments - text :render/text} (render-pulse-card-body render-type timezone-id card 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 7bcd69afb1e29fa8e3bf30422b3de699b78bcd9e..a83bfb591269c27ce50ec63eb31e61025b697e5d 100644 --- a/src/metabase/pulse/render/body.clj +++ b/src/metabase/pulse/render/body.clj @@ -12,10 +12,12 @@ [metabase.pulse.render.sparkline :as sparkline] [metabase.pulse.render.style :as style] [metabase.pulse.render.table :as table] + [metabase.pulse.util :as pu] [metabase.shared.models.visualization-settings :as mb.viz] [metabase.types :as types] [metabase.util :as u] [metabase.util.i18n :refer [trs tru]] + [metabase.util.ui-logic :as ui-logic] [schema.core :as s]) (:import [java.text DecimalFormat DecimalFormatSymbols])) @@ -225,11 +227,11 @@ (defmulti render "Render a Pulse as `chart-type` (e.g. `:bar`, `:scalar`, etc.) and `render-type` (either `:inline` or `:attachment`)." - {:arglists '([chart-type render-type timezone-id card data])} - (fn [chart-type _ _ _ _] chart-type)) + {:arglists '([chart-type render-type timezone-id card dashcard data])} + (fn [chart-type _ _ _ _ _] chart-type)) (s/defmethod render :table :- common/RenderedPulseCard - [_ render-type timezone-id :- (s/maybe s/Str) card {:keys [cols rows] :as data}] + [_ render-type timezone-id :- (s/maybe s/Str) card _ {:keys [cols rows] :as data}] (let [table-body [:div (table/render-table (color/make-color-selector data (:visualization_settings card)) @@ -265,6 +267,30 @@ "dddd, MMMM D, YYYY" {:week "MMMM D, YYYY" :month "MMMM, YYYY"}}) +(defn- update-date-style + [date-style unit] + (let [unit (or unit :default)] + (or (get-in override-date-styles [date-style unit]) + (get-in default-date-styles [unit]) + date-style))) + +(defn- backfill-currency + [{:keys [number_style currency] :as settings}] + (cond-> settings + (and (= number_style "currency") (nil? currency)) + (assoc :currency "USD"))) + +(defn- settings-from-column + [col column-settings] + (or (get column-settings {::mb.viz/field-id (:id col)}) + (get column-settings {::mb.viz/column-name (:name col)}))) + +(defn- update-col-for-js + [col-settings col] + (-> (m/map-keys (fn [k] (-> k name (str/replace #"-" "_") keyword)) col-settings) + (backfill-currency) + (u/update-when :date_style update-date-style (:unit col)))) + (defn- ->js-viz "Include viz settings for js. @@ -272,28 +298,47 @@ - chop off and underscore the nasty keys in our map - backfill currency to the default of USD if not present" [x-col y-col {::mb.viz/keys [column-settings] :as _viz-settings}] - (letfn [(settings [col] (or (get column-settings {::mb.viz/field-id (:id col)}) - (get column-settings {::mb.viz/column-name (:name col)}))) - (update-date-style [date-style unit] - (let [unit (or unit :default)] - (or (get-in override-date-styles [date-style unit]) - (get-in default-date-styles [unit]) - date-style))) - (backfill-currency [{:keys [number_style currency] :as settings}] - (cond-> settings - (and (= number_style "currency") (nil? currency)) - (assoc :currency "USD"))) - (for-js [col-settings col] - (-> (m/map-keys (fn [k] (-> k name (str/replace #"-" "_") keyword)) col-settings) - (backfill-currency) - (u/update-when :date_style update-date-style (:unit col))))] - (let [x-col-settings (settings x-col) - y-col-settings (settings y-col)] - (cond-> {:colors (public-settings/application-colors)} - x-col-settings - (assoc :x (for-js x-col-settings x-col)) - y-col-settings - (assoc :y (for-js y-col-settings y-col)))))) + (let [x-col-settings (settings-from-column x-col column-settings) + y-col-settings (settings-from-column y-col column-settings)] + (cond-> {:colors (public-settings/application-colors)} + x-col-settings + (assoc :x (update-col-for-js x-col-settings x-col)) + y-col-settings + (assoc :y (update-col-for-js y-col-settings y-col))))) + +(defn- ->ts-viz + "Include viz settings for the typed settings, initially in XY charts. + These are actually completely different than the previous settings format inasmuch: + 1. The labels are in the settings + 2. Colors are in the series, only the whitelabel colors are here + 3. Many fewer things are optional + 4. Must explicitly have yAxisPosition in all the series + + For further details look at frontend/src/metabase/static-viz/XYChart/types.ts" + [x-col y-col labels {::mb.viz/keys [column-settings] :as _viz-settings}] + (let [default-format {:number_style "decimal" + :decimals 0 + :currency "USD" + :currency_style "symbol"} + x-col-settings (or (settings-from-column x-col column-settings) {}) + y-col-settings (or (settings-from-column y-col column-settings) {}) + x-format (merge + (if (isa? (:effective_type x-col) :type/Temporal) + {:date_style "MMMM D, YYYY"} + default-format) + x-col-settings) + y-format (merge + default-format + y-col-settings) + default-x-type (if (isa? (:effective_type x-col) :type/Temporal) + "timeseries" + "ordinal")] + {:colors (public-settings/application-colors) + :x {:type (or (:graph.x_axis.scale _viz-settings) default-x-type) + :format x-format} + :y {:type (or (:graph.y_axis.scale _viz-settings) "linear") + :format y-format } + :labels labels})) (defn- x-and-y-axis-label-info "Generate the X and Y axis labels passed in as the `labels` argument @@ -305,8 +350,20 @@ :left (or (:graph.y_axis.title_text viz-settings) (:display_name y-col))}) +(defn- combo-label-info + "X and Y axis labels passed into the `labels` argument needs to be different + for combos specifically (as opposed to multiples)" + [x-cols y-cols viz-settings] + {:bottom (or (:graph.x_axis.title_text viz-settings) + (:display_name (first x-cols))) + :left (or (:graph.y_axis.title_text viz-settings) + (:display_name (first y-cols))) + :right (or (:graph.y_axis.title_text viz-settings) + (:display_name (second y-cols)) + "")}) + (s/defmethod render :bar :- common/RenderedPulseCard - [_ render-type _timezone-id :- (s/maybe s/Str) card {:keys [cols rows viz-settings] :as data}] + [_ render-type _timezone-id :- (s/maybe s/Str) card _ {:keys [cols rows 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/non-nil-rows x-axis-rowfn y-axis-rowfn rows)) @@ -367,7 +424,7 @@ (format-percentage (/ value total)))]))})) (s/defmethod render :categorical/donut :- common/RenderedPulseCard - [_ render-type _timezone-id :- (s/maybe s/Str) card {:keys [rows] :as data}] + [_ render-type _timezone-id :- (s/maybe s/Str) card _ {:keys [rows] :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/non-nil-rows x-axis-rowfn y-axis-rowfn rows)) @@ -399,7 +456,7 @@ (percentages label)]]))]})) (s/defmethod render :progress :- common/RenderedPulseCard - [_ render-type timezone-id card {:keys [cols rows viz-settings] :as data}] + [_ render-type timezone-id card _ {:keys [cols rows viz-settings] :as data}] (let [value (ffirst rows) goal (:progress.goal viz-settings) color (or (:progress.color viz-settings) (first colors)) @@ -420,8 +477,147 @@ [:img {:style (style/style {:display :block :width :100%}) :src (:image-src image-bundle)}]]})) +(def default-y-pos + "Default positions of the y-axes of multiple and combo graphs. + You kind of hope there's only two but here's for the eventuality" + (repeat "left")) + +(def default-combo-chart-types + "Default chart type seq of combo graphs (not multiple graphs)." + (conj (repeat "bar") + "line")) + +(defn- join-series + [names colors types row-seqs y-axis-positions] + ;;; gotta flatten i guess + (let [joined (map vector names colors types row-seqs y-axis-positions)] + (vec (for [[card-name card-color card-type rows y-axis-position] joined] + {:name card-name + :color card-color + :type card-type + :data rows + :yAxisPosition y-axis-position})))) + + +(s/defmethod render :multiple + [_ render-type timezone-id 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 + cards (cons card (map :card multi-res)) + multi-data (cons data (map #(get-in % [:result :data]) multi-res)) + rowfns (mapv common/graphing-column-row-fns cards multi-data) + row-seqs (map :rows multi-data) + row-seqs (for [[row-seq rowfnpair] (map vector row-seqs rowfns)] + (let [[x-rowfn y-rowfn] rowfnpair] + (map (juxt x-rowfn y-rowfn) + (common/non-nil-rows x-rowfn y-rowfn row-seq)))) + col-seqs (map :cols multi-data) + first-rowfns (first rowfns) + [x-col y-col] ((juxt (first first-rowfns) (second first-rowfns)) (first col-seqs)) + labels (x-and-y-axis-label-info x-col y-col viz-settings) + names (map :name cards) + colors (take (count multi-data) colors) + types (map :display cards) + settings (->ts-viz x-col y-col labels viz-settings) + y-pos (take (count names) default-y-pos) + series (join-series names colors types row-seqs y-pos) + image-bundle (image-bundle/make-image-bundle + render-type + (js-svg/combo-chart series settings))] + {:attachments + (when image-bundle + (image-bundle/image-bundle->attachment image-bundle)) + + :content + [:div + [:img {:style (style/style {:display :block + :width :100%}) + :src (:image-src image-bundle)}]]})) + +(defn- series-setting [viz-settings inner-key outer-key] + (get-in viz-settings [:series_settings (keyword outer-key) inner-key])) + +(defn- single-x-axis-combo-series + "This munges rows and columns into series in the format that we want for combo staticviz for literal combo displaytype, + for a single x-axis with multiple y-axis." + [joined-rows x-cols y-cols viz-settings] + (for [[idx y-col] (map-indexed vector y-cols)] + (let [y-col-key (keyword (:name y-col)) + card-name (or (series-setting viz-settings y-col-key :name) + (:display_name y-col)) + card-color (or (series-setting viz-settings y-col-key :color) + (nth colors idx)) + card-type (or (series-setting viz-settings y-col-key :display) + (nth default-combo-chart-types idx)) + selected-rows (sort-by first (map #(vector (ffirst %) (nth (second %) idx)) joined-rows)) + y-axis-pos (or (series-setting viz-settings y-col-key :axis) + (nth default-y-pos idx))] + {:name card-name + :color card-color + :type card-type + :data selected-rows + :yAxisPosition y-axis-pos}))) + +(defn- double-x-axis-combo-series + "This munges rows and columns into series in the format that we want for combo staticviz for literal combo displaytype, + for a double x-axis, which has pretty materially different semantics for that second dimension, with single y-axis only. + + This mimics default behavior in JS viz, which is to group by the second dimension and make every group-by-value a series. + This can have really high cardinality of series but the JS viz will complain about more than 100 already" + [joined-rows x-cols y-cols viz-settings] + (let [grouped-rows (group-by #(second (first %)) joined-rows) + groups (keys grouped-rows)] + (for [[idx group-key] (map-indexed vector groups)] + (let [row-group (get grouped-rows group-key) + selected-row-group (sort-by first (map #(vector (ffirst %) (first (second %))) row-group)) + card-name (or (series-setting viz-settings group-key :name) + group-key) + card-color (or (series-setting viz-settings group-key :color) + (nth colors idx)) + card-type (or (series-setting viz-settings group-key :display) + (nth default-combo-chart-types idx)) + y-axis-pos (or (series-setting viz-settings group-key :axis) + (nth default-y-pos idx))] + {:name card-name + :color card-color + :type card-type + :data selected-row-group + :yAxisPosition y-axis-pos})))) + +(s/defmethod render :combo :- common/RenderedPulseCard + [_ render-type _timezone-id :- (s/maybe s/Str) card _ {:keys [cols rows viz-settings] :as data}] + ;; Special axis-rowfns because we have more than 1 axis + (let [clean-rows (common/non-nil-combo-rows rows) + x-axis-rowfn (ui-logic/mult-x-axis-rowfn card (assoc data :rows clean-rows)) + y-axis-rowfn (ui-logic/mult-y-axis-rowfn card (assoc data :rows clean-rows)) + x-rows (map x-axis-rowfn clean-rows) + y-rows (map y-axis-rowfn clean-rows) + joined-rows (map vector x-rows y-rows) + [x-cols y-cols] ((juxt x-axis-rowfn y-axis-rowfn) cols) + + ;; NB: There's a hardcoded limit of arity 2 on x-axis, so there's only the 1-axis or 2-axis case + series (if (= (count x-cols) 1) + (single-x-axis-combo-series joined-rows x-cols y-cols viz-settings) + (double-x-axis-combo-series joined-rows x-cols y-cols viz-settings)) + + labels (combo-label-info x-cols y-cols viz-settings) + settings (->ts-viz (first x-cols) (first y-cols) labels viz-settings) + + image-bundle (image-bundle/make-image-bundle + render-type + (js-svg/combo-chart series settings))] + {:attachments + (when image-bundle + (image-bundle/image-bundle->attachment image-bundle)) + + :content + [:div + [:img {:style (style/style {:display :block :width :100%}) + :src (:image-src image-bundle)}]]})) + (s/defmethod render :scalar :- common/RenderedPulseCard - [_ _ timezone-id _card {:keys [cols rows viz-settings] :as data}] + [_ _ timezone-id _card _ {:keys [cols rows viz-settings] :as data}] (let [col (first cols) value (format-cell timezone-id (ffirst rows) (first cols) viz-settings)] {:attachments @@ -433,7 +629,7 @@ :render/text (str value)})) (s/defmethod render :smartscalar :- common/RenderedPulseCard - [_ _ timezone-id _card {:keys [cols _rows insights viz-settings]}] + [_ _ timezone-id _card _ {:keys [cols _rows 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))) @@ -466,7 +662,7 @@ @error-rendered-info)))) (s/defmethod render :sparkline :- common/RenderedPulseCard - [_ render-type timezone-id card {:keys [_rows cols viz-settings] :as data}] + [_ render-type timezone-id card _ {: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) @@ -516,7 +712,7 @@ (second labels)]]]]})) (s/defmethod render :waterfall :- common/RenderedPulseCard - [_ render-type timezone-id card {:keys [rows cols viz-settings] :as data}] + [_ render-type timezone-id card _ {: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) @@ -544,7 +740,7 @@ :src (:image-src image-bundle)}]]})) (s/defmethod render :funnel :- common/RenderedPulseCard - [_ render-type timezone-id card {:keys [rows cols viz-settings] :as data}] + [_ render-type timezone-id card _ {:keys [rows cols viz-settings] :as data}] ;; x-axis-rowfn is always first, y-axis-rowfn is always second (let [rows (common/non-nil-rows first second rows) [x-col y-col] cols @@ -565,7 +761,7 @@ (s/defmethod render :empty :- common/RenderedPulseCard - [_ render-type _ _ _] + [_ render-type _ _ _ _] (let [image-bundle (image-bundle/no-results-image-bundle render-type)] {:attachments (image-bundle/image-bundle->attachment image-bundle) @@ -581,7 +777,7 @@ (trs "No results")]]})) (s/defmethod render :attached :- common/RenderedPulseCard - [_ render-type _ _ _] + [_ render-type _ _ _ _] (let [image-bundle (image-bundle/attached-image-bundle render-type)] {:attachments (image-bundle/image-bundle->attachment image-bundle) @@ -597,7 +793,7 @@ (trs "This question has been included as a file attachment")]]})) (s/defmethod render :unknown :- common/RenderedPulseCard - [_ _ _ _ _] + [_ _ _ _ _ _] {:attachments nil @@ -611,5 +807,5 @@ (trs "Please view this card in Metabase.")]}) (s/defmethod render :error :- common/RenderedPulseCard - [_ _ _ _ _] + [_ _ _ _ _ _] @error-rendered-info) diff --git a/src/metabase/pulse/render/common.clj b/src/metabase/pulse/render/common.clj index b5cff2bd6aafa1a077d829ff63cf77d274077907..14786762344414de513860f776f1601dffcea415 100644 --- a/src/metabase/pulse/render/common.clj +++ b/src/metabase/pulse/render/common.clj @@ -98,3 +98,9 @@ "Remove any rows that have a nil value for the `x-axis-fn` OR `y-axis-fn`" [x-axis-fn y-axis-fn rows] (filter (every-pred x-axis-fn y-axis-fn) rows)) + +(defn non-nil-combo-rows + "Remove any rows that have a nil value for the entire row because + the row-function-generating functions themselves choke on nil values, for combo rowfuncs" + [rows] + (filter #(every? some? %) rows)) diff --git a/src/metabase/pulse/render/js_svg.clj b/src/metabase/pulse/render/js_svg.clj index 1ea691301b550c3d81c6cb8777e1976a3b852ae9..80921c761c4fe546ed4bddbc01dc4176b2527e31 100644 --- a/src/metabase/pulse/render/js_svg.clj +++ b/src/metabase/pulse/render/js_svg.clj @@ -135,6 +135,21 @@ (json/generate-string settings)))] (svg-string->bytes svg-string))) +(defn combo-chart + "Clojure entrypoint to render a combo or multiple chart. + These are different conceptions in the BE but being smushed together + because they're supposed to display similarly. + Series should be list of dicts of {rows: rows, cols: cols, type: type}, where types is 'line' or 'bar' or 'area'. + Rows should be tuples of [datetime numeric-value]. Labels is a + map of {:left \"left-label\" :botton \"bottom-label\"}. Returns a byte array of a png file." + [series settings] + (let [svg-string (.asString (js/execute-fn-name @context + "combo_chart" + (json/generate-string series) + (json/generate-string settings) + (json/generate-string (:colors settings))))] + (svg-string->bytes svg-string))) + (defn categorical-line "Clojure entrypoint to render a categorical line chart. Rows should be tuples of [stringable numeric-value]. Labels is a map of {:left \"left-label\" :botton \"bottom-label\". Returns a byte array of a png file." diff --git a/src/metabase/pulse/util.clj b/src/metabase/pulse/util.clj new file mode 100644 index 0000000000000000000000000000000000000000..b2d1688e171864b0ddcf64ac27294bead03481b6 --- /dev/null +++ b/src/metabase/pulse/util.clj @@ -0,0 +1,53 @@ +(ns metabase.pulse.util + "Utils for pulses." + (:require [clojure.tools.logging :as log] + [metabase.models.card :refer [Card]] + [metabase.models.dashboard-card :as dc-model :refer [DashboardCard]] + [metabase.query-processor :as qp] + [metabase.query-processor.middleware.permissions :as qp.perms] + [metabase.server.middleware.session :as session] + [metabase.util :as u] + [metabase.util.i18n :refer [trs]])) + +;; TODO - this should be done async +(defn execute-card + "Execute the query for a single Card. `options` are passed along to the Query Processor." + [{pulse-creator-id :creator_id} card-or-id & {:as options}] + ;; The Card must either be executed in the context of a User or by the MetaBot which itself is not a User + {:pre [(or (integer? pulse-creator-id) + (= (:context options) :metabot))]} + (let [card-id (u/the-id card-or-id)] + (try + (when-let [{query :dataset_query, :as card} (Card :id card-id, :archived false)] + (let [query (assoc query :async? false) + process-query (fn [] + (binding [qp.perms/*card-id* card-id] + (qp/process-query-and-save-with-max-results-constraints! + (assoc query :middleware {:process-viz-settings? true + :js-int-to-string? false}) + (merge {:executed-by pulse-creator-id + :context :pulse + :card-id card-id} + options)))) + result (if pulse-creator-id + (session/with-current-user pulse-creator-id + (process-query)) + (process-query))] + {:card card + :result result})) + (catch Throwable e + (log/warn e (trs "Error running query for Card {0}" card-id)))))) + +(defn execute-multi-card + "Multi series card is composed of multiple cards, all of which need to be executed. + + This is as opposed to combo cards and cards with visualizations with multiple series, + which are viz settings." + [card-or-id dashcard-or-id] + (let [card-id (u/the-id card-or-id) + dashcard-id (u/the-id dashcard-or-id) + card (Card :id card-id, :archived false) + dashcard (DashboardCard :id dashcard-id) + multi-cards (dc-model/dashcard->multi-cards dashcard)] + (for [multi-card multi-cards] + (execute-card {:creator_id (:creator_id card)} (:id multi-card))))) diff --git a/src/metabase/util/ui_logic.clj b/src/metabase/util/ui_logic.clj index 9cb318c469e114503c3781cf4c4261d2c128b21f..3f7ade648323f58e57ab6bde741b44641c174b2c 100644 --- a/src/metabase/util/ui_logic.clj +++ b/src/metabase/util/ui_logic.clj @@ -83,6 +83,30 @@ [card results] (graph-column-index :graph.dimensions card results)) +(defn mult-y-axis-rowfn + "This is used as the Y-axis column in the UI + when we have comboes, which have more than one y axis." + [card results] + (let [metrics (some-> card + (get-in [:visualization_settings :graph.metrics])) + col-indices (map #(column-name->index % results) metrics)] + (when (seq? col-indices) + (fn [row] + (vec (for [idx col-indices] + (get row idx))))))) + +(defn mult-x-axis-rowfn + "This is used as the X-axis column in the UI + when we have comboes, which have more than one x axis." + [card results] + (let [metrics (some-> card + (get-in [:visualization_settings :graph.dimensions])) + col-indices (map #(column-name->index % results) metrics)] + (when (seq? col-indices) + (fn [row] + (vec (for [idx col-indices] + (get row idx))))))) + (defn make-goal-comparison-rowfn "For a given resultset, return the index of the column that should be used for the goal comparison. This can come from the visualization settings if the column is specified, or from our default column logic" diff --git a/test/metabase/models/dashboard_card_test.clj b/test/metabase/models/dashboard_card_test.clj index fabe219a9c56cda9df27bf9e70c03fa323c3f36d..c83fecd5dbad6f536e3b7aa7cb604793dcdb07ce 100644 --- a/test/metabase/models/dashboard_card_test.clj +++ b/test/metabase/models/dashboard_card_test.clj @@ -65,6 +65,17 @@ :visualization_settings {}}]} (remove-ids-and-timestamps (dashboard-card/retrieve-dashboard-card dashcard-id))))))) +(deftest dashcard->multi-card-test + (testing "Check that the multi-cards are returned" + (mt/with-temp* [Card [card1] + Card [card2] + Dashboard [dashboard] + DashboardCard [dc-1 {:dashboard_id (u/the-id dashboard), :card_id (u/the-id card1)}] + DashboardCard [dc-2 {:dashboard_id (u/the-id dashboard), :card_id (u/the-id card2)}] + DashboardCardSeries [dcs-1 {:dashboardcard_id (u/the-id dc-1), :card_id (u/the-id card2)}]] + (testing "get multi-cards" + (is (= 1 (count (dashboard-card/dashcard->multi-cards dc-1)))))))) + (deftest update-dashboard-card-series!-test (mt/with-temp* [Dashboard [{dashboard-id :id} {:name "Test Dashboard" :creator_id (mt/user->id :rasta)}] diff --git a/test/metabase/pulse/render/body_test.clj b/test/metabase/pulse/render/body_test.clj index 2637f5830c5e37ab7eda2b04d5b6c6cf898bfc6d..0dbc9acb30e63414cbd6e36165389a07c09a7ceb 100644 --- a/test/metabase/pulse/render/body_test.clj +++ b/test/metabase/pulse/render/body_test.clj @@ -245,7 +245,7 @@ (count test-columns)))))) (defn- render-scalar-value [results] - (-> (body/render :scalar nil pacific-tz nil results) + (-> (body/render :scalar nil pacific-tz nil nil results) :content last)) @@ -286,13 +286,13 @@ :semantic_type nil}] :rows [["foo"]]}] (is (= "foo" - (:render/text (body/render :scalar nil pacific-tz nil results)))) + (:render/text (body/render :scalar nil pacific-tz nil nil results)))) (is (schema= {:attachments (s/eq nil) :content [(s/one (s/eq :div) "div tag") (s/one {:style s/Str} "style map") (s/one (s/eq "foo") "content")] :render/text (s/eq "foo")} - (body/render :scalar nil pacific-tz nil results))))) + (body/render :scalar nil pacific-tz nil nil results))))) (testing "for smartscalars" (let [results {:cols [{:name "value", :display_name "VALUE", @@ -310,11 +310,11 @@ :col "value" :last-value 40.0}]}] (is (= "40.00\nUp 133.33%. Was 30.00 last month" - (:render/text (body/render :smartscalar nil pacific-tz nil results)))) + (:render/text (body/render :smartscalar nil pacific-tz nil nil results)))) (is (schema= {:attachments (s/eq nil) :content (s/pred vector? "hiccup vector") :render/text (s/eq "40.00\nUp 133.33%. Was 30.00 last month")} - (body/render :smartscalar nil pacific-tz nil results))))))) + (body/render :smartscalar nil pacific-tz nil nil results))))))) (defn- replace-style-maps [hiccup-map] (walk/postwalk (fn [maybe-map] @@ -379,7 +379,7 @@ (tree-seq coll? seq html-data)) (defn- render-bar-graph [results] - (body/render :bar :inline pacific-tz render.tu/test-card results)) + (body/render :bar :inline pacific-tz render.tu/test-card nil results)) (def ^:private default-columns [{:name "Price", @@ -391,6 +391,20 @@ :base_type :type/BigInteger :semantic_type nil}]) +(def ^:private default-combo-columns + [{:name "Price", + :display_name "Price", + :base_type :type/BigInteger + :semantic_type nil} + {:name "NumPurchased", + :display_name "NumPurchased", + :base_type :type/BigInteger + :semantic_type nil} + {:name "NumKazoos", + :display_name "NumKazoos", + :base_type :type/BigInteger + :semantic_type nil}]) + (defn has-inline-image? [rendered] (some #{:img} (flatten-html-data rendered))) @@ -414,7 +428,7 @@ (defn- render-waterfall [results] - (body/render :waterfall :inline pacific-tz render.tu/test-card results)) + (body/render :waterfall :inline pacific-tz render.tu/test-card nil results)) (deftest render-waterfall-test (testing "Render a waterfall graph with non-nil values for the x and y axis" @@ -425,7 +439,7 @@ (is (has-inline-image? (render-waterfall {:cols default-columns :rows [[10.0 1] [5.0 10] [2.50 20] [1.25 nil]]})))) - (testing "Check to make sure we allow nil values for the y-axis" + (testing "Check to make sure we allow nil values for the x-axis" (is (has-inline-image? (render-waterfall {:cols default-columns :rows [[10.0 1] [5.0 10] [2.50 20] [nil 30]]})))) @@ -434,6 +448,26 @@ (render-waterfall {:cols default-columns :rows [[10.0 1] [5.0 10] [nil 20] [1.25 nil]]}))))) +(defn- render-combo [results] + (body/render :combo :inline pacific-tz render.tu/test-combo-card nil results)) + +(defn- render-combo-multi-x [results] + (body/render :combo :inline pacific-tz render.tu/test-combo-card-multi-x nil results)) + +(deftest render-combo-test + (testing "Render a combo graph with non-nil values for the x and y axis" + (is (has-inline-image? + (render-combo {:cols default-combo-columns + :rows [[10.0 1 123] [5.0 10 12] [2.50 20 1337] [1.25 30 -22]]})))) + (testing "Render a combo graph with multiple x axes" + (is (has-inline-image? + (render-combo-multi-x {:cols default-combo-columns + :rows [[10.0 "Bob" 123] [5.0 "Dobbs" 12] [2.50 "Robbs" 1337] [1.25 "Mobbs" -22]]})))) + (testing "Check to make sure we allow nil values for any axis" + (is (has-inline-image? + (render-combo {:cols default-combo-columns + :rows [[nil 1 1] [10.0 1 nil] [5.0 10 22] [2.50 nil 22] [1.25 nil nil]]}))))) + ;; Test rendering a sparkline ;; ;; Sparklines are a binary image either in-line or as an attachment, so there's not much introspection that we can do @@ -441,7 +475,7 @@ ;; attachment is included (defn- render-sparkline [results] - (body/render :sparkline :inline pacific-tz render.tu/test-card results)) + (body/render :sparkline :inline pacific-tz render.tu/test-card nil results)) (deftest render-sparkline-test (testing "Test that we can render a sparkline with all valid values" @@ -471,7 +505,7 @@ :rows [[10.0 1] [11.0 2] [nil 20] [1.25 nil]]}))))) (defn- render-funnel [results] - (body/render :funnel :inline pacific-tz render.tu/test-card results)) + (body/render :funnel :inline pacific-tz render.tu/test-card nil results)) (deftest render-funnel-test (testing "Test that we can render a funnel with all valid values" @@ -497,6 +531,7 @@ render (fn [rows] (body/render :categorical/donut :inline pacific-tz render.tu/test-card + nil {:cols columns :rows rows})) prune (fn prune [html-tree] (walk/prewalk (fn no-maps [x] @@ -523,6 +558,7 @@ render (fn [rows] (body/render :progress :inline pacific-tz render.tu/test-card + nil {:cols col :rows rows})) prune (fn prune [html-tree] (walk/prewalk (fn no-maps [x] diff --git a/test/metabase/pulse/render/js_svg_test.clj b/test/metabase/pulse/render/js_svg_test.clj index e358d38c7dd54237935cd1f5fbc540c043c70620..5a5fcfd5686ed9a6849ed65a473c93c0d9d2ff4a 100644 --- a/test/metabase/pulse/render/js_svg_test.clj +++ b/test/metabase/pulse/render/js_svg_test.clj @@ -160,6 +160,40 @@ (testing "it returns a valid svg string (no html in it)" (validate-svg-string :timelineseries-waterfall svg-string))))) +(deftest combo-test + (let [rows1 [[#t "1998-03-01T00:00:00Z" 2] + [#t "1999-03-01T00:00:00Z" 3]] + rows2 [[#t "2000-03-01T00:00:00Z" 3] + [#t "2002-03-01T00:00:00Z" 4]] + ;; this one needs more stuff because of stricter ts types + series [{:name "bob" + :color "#cccccc" + :type "area" + :data rows1 + :yAxisPosition "left"} + {:name "bob2" + :color "#cccccc" + :type "line" + :data rows2 + :yAxisPosition "right"}] + labels {:left "count" :bottom "year" :right "something"} + settings {:x {:type "timeseries" + :format {:date_style "YYYY"}} + :y {:type "linear" + :format {:number_style "decimal" :decimals 4}} + :colors {} + :labels labels}] + (testing "It returns bytes" + (let [svg-bytes (js-svg/combo-chart series settings)] + (is (bytes? svg-bytes)))) + (let [svg-string (.asString (js/execute-fn-name @context "combo_chart" + (json/generate-string series) + (json/generate-string settings) + (json/generate-string {}))) + svg-hiccup (-> svg-string parse-svg document-tag-hiccup)] + (testing "it returns a valid svg string (no html in it)" + (validate-svg-string :combo-chart svg-string))))) + (deftest categorical-donut-test (let [rows [["apples" 2] diff --git a/test/metabase/pulse/render/test_util.clj b/test/metabase/pulse/render/test_util.clj index 018450b99cc1dc55b45eaa3a1c03501e4ac14d52..ca861a5a9c368233cc89cd3ded53f09c1beebb0c 100644 --- a/test/metabase/pulse/render/test_util.clj +++ b/test/metabase/pulse/render/test_util.clj @@ -15,3 +15,13 @@ :max_type "custom" :max_value 9 :colors ["#00ff00" "#0000ff"]}]}}) + +(def test-combo-card + {:visualization_settings + {:graph.metrics ["NumPurchased", "NumKazoos"] + :graph.dimensions ["Price"]}}) + +(def test-combo-card-multi-x + {:visualization_settings + {:graph.metrics ["NumKazoos"] + :graph.dimensions ["Price" "NumPurchased"]}}) diff --git a/test/metabase/pulse/render_test.clj b/test/metabase/pulse/render_test.clj index 20aa9c3fe8b39ce26100d3fa862819678b4ec341..dce4112b765c4d059b4a292ee2dde6119fb5da78 100644 --- a/test/metabase/pulse/render_test.clj +++ b/test/metabase/pulse/render_test.clj @@ -1,11 +1,12 @@ (ns metabase.pulse.render-test (:require [clojure.test :refer :all] [metabase.mbql.util :as mbql.u] - [metabase.models.card :refer [Card]] + [metabase.models :refer [Card Dashboard DashboardCard DashboardCardSeries]] [metabase.pulse :as pulse] [metabase.pulse.render :as render] [metabase.query-processor :as qp] - [metabase.test :as mt])) + [metabase.test :as mt] + [metabase.util :as u])) ;; Let's make sure rendering Pulses actually works @@ -32,10 +33,12 @@ (deftest detect-pulse-chart-type-test (is (= :scalar (render/detect-pulse-chart-type {:display :anything} + {} {:cols [{:base_type :type/Number}] :rows [[6]]}))) (is (= :smartscalar (render/detect-pulse-chart-type {:display :smartscalar} + {} {:cols [{:base_type :type/Temporal :name "month"} {:base_type :type/Number @@ -48,12 +51,34 @@ :last-change 50.0}]}))) (is (= :bar (render/detect-pulse-chart-type {:display :bar} + {} {:cols [{:base_type :type/Text} {:base_type :type/Number}] :rows [["A" 2]]}))) + (is (= :combo + (render/detect-pulse-chart-type {:display :combo} + {} + {:cols [{:base_type :type/Temporal} + {:base_type :type/Number}] + :rows [[#t "2020" 2] + [#t "2021" 3]]}))) + + (is (= :multiple + (mt/with-temp* [Card [card1 {:display :something}] + Card [card2 {:display :whatever}] + Dashboard [dashboard] + DashboardCard [dc1 {:dashboard_id (u/the-id dashboard) :card_id (u/the-id card1)}] + DashboardCardSeries [dcs1 {:dashboardcard_id (u/the-id dc1) :card_id (u/the-id card2)}]] + (render/detect-pulse-chart-type card1 + dc1 + {:cols [{:base_type :type/Temporal} + {:base_type :type/Number}] + :rows [[#t "2020" 2] + [#t "2021" 3]]})))) (is (= :funnel (render/detect-pulse-chart-type {:display :funnel} + {} {:cols [{:base_type :type/Text} {:base_type :type/Number}] :rows [["A" 2]]}))) @@ -61,6 +86,7 @@ ;; timeseries line chart (is (= :sparkline (render/detect-pulse-chart-type {:display :line} + {} {:cols [{:base_type :type/Temporal} {:base_type :type/Number}] :rows [[#t "2020" 2] @@ -68,12 +94,14 @@ ;; Category line chart (is (= :sparkline (render/detect-pulse-chart-type {:display :line} + {} {:cols [{:base_type :type/Text} {:base_type :type/Number}] :rows [["Red" 2] ["Blue" 3]]}))) (is (= :categorical/donut (render/detect-pulse-chart-type {:display :pie} + {} {:cols [{:base_type :type/Text} {:base_type :type/Number}] :rows [["apple" 3] diff --git a/test/metabase/pulse_test.clj b/test/metabase/pulse_test.clj index 43672db5b05cef7acd3438c323735673dd89f57e..d1a1c5ad9ecac5b50acd0671c194a7793d11e9a4 100644 --- a/test/metabase/pulse_test.clj +++ b/test/metabase/pulse_test.clj @@ -11,6 +11,7 @@ [metabase.pulse.render :as render] [metabase.pulse.render.body :as render.body] [metabase.pulse.test-util :refer :all] + [metabase.pulse.util :as pu] [metabase.query-processor.middleware.constraints :as constraints] [metabase.test :as mt] [metabase.util :as u] @@ -750,7 +751,7 @@ :async? true}}] (is (schema= {:card (s/pred map?) :result (s/pred map?)} - (pulse/execute-card {:creator_id (mt/user->id :rasta)} card)))))) + (pu/execute-card {:creator_id (mt/user->id :rasta)} card)))))) (deftest pulse-permissions-test (testing "Pulses should be sent with the Permissions of the user that created them."