diff --git a/frontend/src/metabase/pulse/components/PulseCardPreview.jsx b/frontend/src/metabase/pulse/components/PulseCardPreview.jsx
index 1ccbf48d936a779ca700f917905fff83f251353e..3db924022146e883bd9e8233665e394ba8b571a2 100644
--- a/frontend/src/metabase/pulse/components/PulseCardPreview.jsx
+++ b/frontend/src/metabase/pulse/components/PulseCardPreview.jsx
@@ -67,7 +67,12 @@ export default class PulseCardPreview extends Component {
cardPreview &&
cardPreview.pulse_card_type == null;
return (
- <div className="flex relative flex-full">
+ <div
+ className="flex relative flex-full"
+ style={{
+ maxWidth: 379,
+ }}
+ >
<div
className="absolute p2 text-grey-2"
style={{
diff --git a/frontend/src/metabase/pulse/components/PulseEditCards.jsx b/frontend/src/metabase/pulse/components/PulseEditCards.jsx
index b6f721492135ff52351885d1572f7b5f1a075ac1..df98dfd4a23f3e9cb6d2a2174712d9f3e76a8d8e 100644
--- a/frontend/src/metabase/pulse/components/PulseEditCards.jsx
+++ b/frontend/src/metabase/pulse/components/PulseEditCards.jsx
@@ -11,6 +11,17 @@ import MetabaseAnalytics from "metabase/lib/analytics";
const SOFT_LIMIT = 10;
const HARD_LIMIT = 25;
+const TABLE_MAX_ROWS = 20;
+const TABLE_MAX_COLS = 10;
+
+function isAutoAttached(cardPreview) {
+ return (
+ cardPreview &&
+ cardPreview.pulse_card_type === "table" &&
+ (cardPreview.row_count > TABLE_MAX_ROWS ||
+ cardPreview.col_cound > TABLE_MAX_COLS)
+ );
+}
export default class PulseEditCards extends Component {
constructor(props) {
@@ -69,9 +80,10 @@ export default class PulseEditCards extends Component {
const showSoftLimitWarning = index === SOFT_LIMIT;
let notices = [];
const hasAttachment =
- this.props.attachmentsEnabled &&
- card &&
- (card.include_csv || card.include_xls);
+ isAutoAttached(cardPreview) ||
+ (this.props.attachmentsEnabled &&
+ card &&
+ (card.include_csv || card.include_xls));
if (hasAttachment) {
notices.push({
head: t`Attachment`,
@@ -85,6 +97,13 @@ export default class PulseEditCards extends Component {
});
}
if (cardPreview) {
+ if (isAutoAttached(cardPreview)) {
+ notices.push({
+ type: "warning",
+ head: t`Heads up`,
+ body: t`We'll show the first 10 columns and 20 rows of this table in your Pulse. If you email this, we'll add a file attachment with all columns and up to 2,000 rows.`,
+ });
+ }
if (cardPreview.pulse_card_type == null && !hasAttachment) {
notices.push({
type: "warning",
@@ -164,7 +183,10 @@ export default class PulseEditCards extends Component {
onChange={this.setCard.bind(this, index)}
onRemove={this.removeCard.bind(this, index)}
fetchPulseCardPreview={this.props.fetchPulseCardPreview}
- attachmentsEnabled={this.props.attachmentsEnabled}
+ attachmentsEnabled={
+ this.props.attachmentsEnabled &&
+ !isAutoAttached(cardPreviews[card.id])
+ }
trackPulseEvent={this.trackPulseEvent}
/>
) : (
diff --git a/frontend/test/pulse/pulse.integ.spec.js b/frontend/test/pulse/pulse.integ.spec.js
index b6255cf420b49c45a4d03792c54acae85e0d803a..756e0b2e4ee9cc7f97f6b705a08e12ed1f2b96f7 100644
--- a/frontend/test/pulse/pulse.integ.spec.js
+++ b/frontend/test/pulse/pulse.integ.spec.js
@@ -133,8 +133,8 @@ describe("Pulse", () => {
// NOTE: check text content since enzyme doesn't doesn't seem to work well with dangerouslySetInnerHTML
expect(previews.at(0).text()).toBe("count12,805");
expect(previews.at(0).find(".Icon-attachment").length).toBe(1);
- expect(previews.at(1).text()).toBe(
- "tableThis question will be added as a file attachment",
+ expect(previews.at(1).text()).toEqual(
+ expect.stringContaining("Showing 20 of 12,805 rows"),
);
expect(previews.at(1).find(".Icon-attachment").length).toBe(0);
@@ -144,8 +144,8 @@ describe("Pulse", () => {
previews = app.find(PulseCardPreview);
expect(previews.at(0).text()).toBe("count12,805");
expect(previews.at(0).find(".Icon-attachment").length).toBe(0);
- expect(previews.at(1).text()).toBe(
- "tableThis question won't be included in your Pulse",
+ expect(previews.at(1).text()).toEqual(
+ expect.stringContaining("Showing 20 of 12,805 rows"),
);
expect(previews.at(1).find(".Icon-attachment").length).toBe(0);
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index 181ab8214394d21ce490a12785c82fc48369cae6..45814d0c2c7138dbc3713146ab8f97fec17bf55a 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -594,7 +594,7 @@
"Run the query for Card with PARAMETERS and CONSTRAINTS, and return results in the usual format."
{:style/indent 1}
[card-id & {:keys [parameters constraints context dashboard-id]
- :or {constraints dataset-api/default-query-constraints
+ :or {constraints qp/default-query-constraints
context :question}}]
{:pre [(u/maybe? sequential? parameters)]}
(let [card (api/read-check (hydrate (Card card-id) :in_public_dashboard))
diff --git a/src/metabase/api/dashboard.clj b/src/metabase/api/dashboard.clj
index 0b7a49f856f99dec95a807c0c7183ff386b6c431..02e994e4769acc3b55cb6d181d69d56af5cce236 100644
--- a/src/metabase/api/dashboard.clj
+++ b/src/metabase/api/dashboard.clj
@@ -4,6 +4,7 @@
[compojure.core :refer [DELETE GET POST PUT]]
[metabase
[events :as events]
+ [query-processor :as qp]
[util :as u]]
[metabase.api
[common :as api]
@@ -123,7 +124,7 @@
[{:keys [dataset_query]}]
(u/ignore-exceptions
[(qp-util/query-hash dataset_query)
- (qp-util/query-hash (assoc dataset_query :constraints dataset/default-query-constraints))]))
+ (qp-util/query-hash (assoc dataset_query :constraints qp/default-query-constraints))]))
(defn- dashcard->query-hashes
"Return a sequence of all the query hashes for this DASHCARD, including the top-level Card and any Series."
diff --git a/src/metabase/api/dataset.clj b/src/metabase/api/dataset.clj
index be2c68d17e734f5ed42d2cbe6935e1cd6ff20a5d..e7999d5b4a95e821a2dcae0de5ffca0a05eed1a8 100644
--- a/src/metabase/api/dataset.clj
+++ b/src/metabase/api/dataset.clj
@@ -21,22 +21,6 @@
[schema :as su]]
[schema.core :as s]))
-;;; --------------------------------------------------- Constants ----------------------------------------------------
-
-(def ^:private ^:const max-results-bare-rows
- "Maximum number of rows to return specifically on :rows type queries via the API."
- 2000)
-
-(def ^:private ^:const max-results
- "General maximum number of rows to return from an API query."
- 10000)
-
-(def ^:const default-query-constraints
- "Default map of constraints that we apply on dataset queries executed by the api."
- {:max-results max-results
- :max-results-bare-rows max-results-bare-rows})
-
-
;;; -------------------------------------------- Running a Query Normally --------------------------------------------
(defn- query->source-card-id
@@ -61,8 +45,8 @@
(let [source-card-id (query->source-card-id query)]
(api/cancellable-json-response
(fn []
- (qp/process-query-and-save-execution! (assoc query :constraints default-query-constraints)
- {:executed-by api/*current-user-id*, :context :ad-hoc, :card-id source-card-id, :nested? (boolean source-card-id)})))))
+ (qp/process-query-and-save-with-max! query {:executed-by api/*current-user-id*, :context :ad-hoc,
+ :card-id source-card-id, :nested? (boolean source-card-id)})))))
;;; ----------------------------------- Downloading Query Results in Other Formats -----------------------------------
@@ -151,7 +135,7 @@
;; try calculating the average for the query as it was given to us, otherwise with the default constraints if
;; there's no data there. If we still can't find relevant info, just default to 0
{:average (or (query/average-execution-time-ms (qputil/query-hash query))
- (query/average-execution-time-ms (qputil/query-hash (assoc query :constraints default-query-constraints)))
+ (query/average-execution-time-ms (qputil/query-hash (assoc query :constraints qp/default-query-constraints)))
0)})
(api/define-routes)
diff --git a/src/metabase/api/pulse.clj b/src/metabase/api/pulse.clj
index f6106dcdad95141c591c1388597025dc47d73a50..324216bd28d74f6426bbfdb8adb02a983781c9b8 100644
--- a/src/metabase/api/pulse.clj
+++ b/src/metabase/api/pulse.clj
@@ -133,7 +133,8 @@
:pulse_card_html card-html
:pulse_card_name (:name card)
:pulse_card_url (urls/card-url (:id card))
- :row_count (:row_count result)}))
+ :row_count (:row_count result)
+ :col_count (count (:cols (:data result)))}))
(api/defendpoint GET "/preview_card_png/:id"
"Get PNG rendering of a `Card` with ID."
diff --git a/src/metabase/email/_footer_pulse.mustache b/src/metabase/email/_footer_pulse.mustache
new file mode 100644
index 0000000000000000000000000000000000000000..725cae61054372fdcf844edacc1ec9ee8eb705f7
--- /dev/null
+++ b/src/metabase/email/_footer_pulse.mustache
@@ -0,0 +1,11 @@
+ </div>
+ {{#quotation}}
+ <div style="padding-top: 2em; padding-bottom: 1em; text-align: left; color: #CCCCCC; font-size: small;">"{{quotation}}"<br/>- {{quotationAuthor}}</div>
+ {{/quotation}}
+ {{#logoFooter}}
+ <div style="padding-bottom: 2em; padding-top: 1em; text-align: left;">
+ <img width="32" height="40" src="http://static.metabase.com/email_logo.png"/>
+ </div>
+ {{/logoFooter}}
+</body>
+</html>
diff --git a/src/metabase/email/_header.mustache b/src/metabase/email/_header.mustache
index 9541f783cac427b737057b370af6a5eae331689a..85c926ad42a85155f06ec1d9f7f7e93a55e1297f 100644
--- a/src/metabase/email/_header.mustache
+++ b/src/metabase/email/_header.mustache
@@ -14,4 +14,4 @@
<img width="47" height="60" src="http://static.metabase.com/email_logo.png"/>
</div>
{{/logoHeader}}
- <div class="container" style="margin: 0 auto; padding: 0 0 2em 0; max-width: 500px; font-size: 16px; line-height: 24px; color: #616D75;">
+ <div class="container" style="margin: 0; padding: 0 0 2em 0; max-width: 100%; font-size: 16px; line-height: 24px; color: #616D75;">
diff --git a/src/metabase/email/messages.clj b/src/metabase/email/messages.clj
index 6464251693070ddbf104a522ee23078c706c1a86..ffd585129da6bc978780f286cb05f72c820bb0ce 100644
--- a/src/metabase/email/messages.clj
+++ b/src/metabase/email/messages.clj
@@ -204,19 +204,21 @@
:content-type content-type
:file-name (format "%s.%s" card-name ext)
:content (-> attachment-file .toURI .toURL)
- :description (format "Full results for '%s'" card-name)}))
+ :description (format "More results for '%s'" card-name)}))
(defn- result-attachments [results]
(remove nil?
(apply concat
- (for [{{card-name :name, csv? :include_csv, xls? :include_xls} :card :as result} results
- :when (and (or csv? xls?)
- (seq (get-in result [:result :data :rows])))]
- [(when-let [temp-file (and csv? (create-temp-file "csv"))]
+ (for [{{card-name :name, :as card} :card :as result} results
+ :let [{:keys [rows] :as result-data} (get-in result [:result :data])]
+ :when (seq rows)]
+ [(when-let [temp-file (and (render/include-csv-attachment? card result-data)
+ (create-temp-file "csv"))]
(export/export-to-csv-writer temp-file result)
(create-result-attachment-map "csv" card-name temp-file))
- (when-let [temp-file (and xls? (create-temp-file "xlsx"))]
+ (when-let [temp-file (and (render/include-xls-attachment? card result-data)
+ (create-temp-file "xlsx"))]
(export/export-to-xlsx-file temp-file result)
(create-result-attachment-map "xlsx" card-name temp-file))]))))
diff --git a/src/metabase/email/pulse.mustache b/src/metabase/email/pulse.mustache
index d2f5e05cc7a2e1518f250bae9bdee722dfff6a20..e024ababd09f0a304f9949164db62eaea1515d6f 100644
--- a/src/metabase/email/pulse.mustache
+++ b/src/metabase/email/pulse.mustache
@@ -1,8 +1,8 @@
{{> metabase/email/_header}}
{{#pulseName}}
- <h1 style="{{sectionStyle}} margin: 16px; color: {{colorGrey4}};">
+ <h1 style="{{sectionStyle}} margin: 16px; color: {{color-brand}};">
{{pulseName}}
</h1>
{{/pulseName}}
{{{pulse}}}
-{{> metabase/email/_footer}}
+{{> metabase/email/_footer_pulse}}
diff --git a/src/metabase/pulse.clj b/src/metabase/pulse.clj
index b2e718de90a7c3fc41da02e79f99e24f191ddb79..41ab09ac923d04a59d8ce76bf4bf7fc4c64e0088 100644
--- a/src/metabase/pulse.clj
+++ b/src/metabase/pulse.clj
@@ -34,9 +34,10 @@
(let [{:keys [creator_id dataset_query]} card]
(try
{:card card
- :result (qp/process-query-and-save-execution! dataset_query
- (merge {:executed-by creator_id, :context :pulse, :card-id card-id}
- options))}
+ :result (qp/process-query-and-save-with-max! dataset_query (merge {:executed-by creator_id,
+ :context :pulse,
+ :card-id card-id}
+ options))}
(catch Throwable t
(log/warn (format "Error running card query (%n)" card-id) t))))))
diff --git a/src/metabase/pulse/render.clj b/src/metabase/pulse/render.clj
index 2c02ae9fb86bb0fe07af09f1388765566a48dd22..d9bbd926f1aac471821230a87a6f5a9d64a4e673 100644
--- a/src/metabase/pulse/render.clj
+++ b/src/metabase/pulse/render.clj
@@ -8,7 +8,9 @@
[string :as str]]
[clojure.java.io :as io]
[clojure.tools.logging :as log]
- [hiccup.core :refer [h html]]
+ [hiccup
+ [core :refer [h html]]
+ [util :as hutil]]
[metabase.util :as u]
[metabase.util
[ui-logic :as ui-logic]
@@ -35,21 +37,24 @@
;;; # ------------------------------------------------------------ STYLES ------------------------------------------------------------
(def ^:private ^:const card-width 400)
-(def ^:private ^:const rows-limit 10)
-(def ^:private ^:const cols-limit 3)
+(def ^:private ^:const rows-limit 20)
+(def ^:private ^:const cols-limit 10)
(def ^:private ^:const sparkline-dot-radius 6)
(def ^:private ^:const sparkline-thickness 3)
(def ^:private ^:const sparkline-pad 8)
;;; ## STYLES
-(def ^:private ^:const color-brand "rgb(45,134,212)")
-(def ^:private ^:const color-purple "rgb(135,93,175)")
-(def ^:private ^:const color-gold "#F9D45C")
-(def ^:private ^:const color-error "#EF8C8C")
-(def ^:private ^:const color-gray-1 "rgb(248,248,248)")
-(def ^:private ^:const color-gray-2 "rgb(189,193,191)")
-(def ^:private ^:const color-gray-3 "rgb(124,131,129)")
+(def ^:private ^:const color-brand "rgb(45,134,212)")
+(def ^:private ^:const color-purple "rgb(135,93,175)")
+(def ^:private ^:const color-gold "#F9D45C")
+(def ^:private ^:const color-error "#EF8C8C")
+(def ^:private ^:const color-gray-1 "rgb(248,248,248)")
+(def ^:private ^:const color-gray-2 "rgb(189,193,191)")
+(def ^:private ^:const color-gray-3 "rgb(124,131,129)")
(def ^:const color-gray-4 "A ~25% Gray color." "rgb(57,67,64)")
+(def ^:private ^:const color-dark-gray "#616D75")
+(def ^:private ^:const color-row-border "#EDF0F1")
+
(def ^:private ^:const font-style {:font-family "Lato, \"Helvetica Neue\", Helvetica, Arial, sans-serif"})
(def ^:const section-style
@@ -68,19 +73,47 @@
:color color-brand}))
(def ^:private ^:const bar-th-style
- (merge font-style {:font-size :10px
- :font-weight 400
- :color color-gray-4
- :border-bottom (str "4px solid " color-gray-1)
- :padding-top :0px
- :padding-bottom :10px}))
+ (merge font-style {:font-size :14.22px
+ :font-weight 700
+ :color color-gray-4
+ :border-bottom (str "1px solid " color-row-border)
+ :padding-top :20px
+ :padding-bottom :5px}))
+
+;; TO-DO for @senior: apply this style to headings of numeric columns
+(def ^:private ^:const bar-th-numeric-style
+ (merge font-style {:text-align :right
+ :font-size :14.22px
+ :font-weight 700
+ :color color-gray-4
+ :border-bottom (str "1px solid " color-row-border)
+ :padding-top :20px
+ :padding-bottom :5px}))
(def ^:private ^:const bar-td-style
- (merge font-style {:font-size :16px
- :font-weight 400
- :text-align :left
- :padding-right :1em
- :padding-top :8px}))
+ (merge font-style {:font-size :14.22px
+ :font-weight 400
+ :color color-dark-gray
+ :text-align :left
+ :padding-right :1em
+ :padding-top :2px
+ :padding-bottom :1px
+ :max-width :500px
+ :overflow :hidden
+ :text-overflow :ellipsis
+ :border-bottom (str "1px solid " color-row-border)}))
+
+;; TO-DO for @senior: apply this style to numeric cells
+(def ^:private ^:const bar-td-style-numeric
+ (merge font-style {:font-size :14.22px
+ :font-weight 400
+ :color color-dark-gray
+ :text-align :right
+ :padding-right :1em
+ :padding-top :2px
+ :padding-bottom :1px
+ :font-family "Courier, Monospace"
+ :border-bottom (str "1px solid " color-row-border)}))
(def ^:private RenderedPulseCard
"Schema used for functions that operate on pulse card contents and their attachments"
@@ -98,6 +131,11 @@
:let [v (if (keyword? v) (name v) v)]]
(str (name k) ": " v ";"))))
+(defn- graphing-columns [card {:keys [cols] :as data}]
+ [(or (ui-logic/x-axis-rowfn card data)
+ first)
+ (or (ui-logic/y-axis-rowfn card data)
+ second)])
(defn- datetime-field?
[field]
@@ -109,12 +147,53 @@
(or (isa? (:base_type field) :type/Number)
(isa? (:special_type field) :type/Number)))
+(defn detect-pulse-card-type
+ "Determine the pulse (visualization) type of a CARD, e.g. `:scalar` or `:bar`."
+ [card data]
+ (let [col-count (-> data :cols count)
+ row-count (-> data :rows count)
+ [col-1-rowfn col-2-rowfn] (graphing-columns card data)
+ col-1 (col-1-rowfn (:cols data))
+ col-2 (col-2-rowfn (:cols data))
+ aggregation (-> card :dataset_query :query :aggregation first)]
+ (cond
+ (or (zero? row-count)
+ ;; Many aggregations result in [[nil]] if there are no rows to aggregate after filters
+ (= [[nil]] (-> data :rows))) :empty
+ (contains? #{:pin_map :state :country} (:display card)) nil
+ (and (= col-count 1)
+ (= row-count 1)) :scalar
+ (and (= col-count 2)
+ (> row-count 1)
+ (datetime-field? col-1)
+ (number-field? col-2)) :sparkline
+ (and (= col-count 2)
+ (number-field? col-2)) :bar
+ :else :table)))
+
+(defn include-csv-attachment?
+ "Returns true if this card and resultset should include a CSV attachment"
+ [{:keys [include_csv] :as card} {:keys [cols rows] :as result-data}]
+ (or (:include_csv card)
+ (and (= :table (detect-pulse-card-type card result-data))
+ (or (< cols-limit (count cols))
+ (< rows-limit (count rows))))))
+
+(defn include-xls-attachment?
+ "Returns true if this card and resultset should include an XLS attachment"
+ [{:keys [include_csv] :as card} result-data]
+ (:include_xls card))
+
;;; # ------------------------------------------------------------ FORMATTING ------------------------------------------------------------
+(defrecord NumericWrapper [num-str]
+ hutil/ToString
+ (to-str [_] num-str))
+
(defn- format-number
[n]
- (cl-format nil (if (integer? n) "~:d" "~,2f") n))
+ (NumericWrapper. (cl-format nil (if (integer? n) "~:d" "~,2f") n)))
(defn- reformat-timestamp [timezone old-format-timestamp new-format-string]
(f/unparse (f/with-zone (f/formatter new-format-string)
@@ -251,22 +330,35 @@
(render-to-png html os width)
(.toByteArray os)))
+
+(defn- heading-style-for-type
+ [cell]
+ (if (instance? NumericWrapper cell)
+ bar-th-numeric-style
+ bar-th-style))
+
+(defn- row-style-for-type
+ [cell]
+ (if (instance? NumericWrapper cell)
+ bar-td-style-numeric
+ bar-td-style))
+
(defn- render-table
[header+rows]
- [:table {:style (style {:padding-bottom :8px, :border-bottom (str "4px solid " color-gray-1)})}
+ [:table {:style (style {:max-width (str "100%"), :white-space :nowrap, :padding-bottom :8px, :border-collapse :collapse})}
(let [{header-row :row bar-width :bar-width} (first header+rows)]
[:thead
[:tr
(for [header-cell header-row]
- [:th {:style (style bar-td-style bar-th-style {:min-width :60px})}
+ [:th {:style (style (row-style-for-type header-cell) (heading-style-for-type header-cell) {:min-width :60px})}
(h header-cell)])
(when bar-width
[:th {:style (style bar-td-style bar-th-style {:width (str bar-width "%")})}])]])
[:tbody
(map-indexed (fn [row-idx {:keys [row bar-width]}]
- [:tr {:style (style {:color (if (odd? row-idx) color-gray-2 color-gray-3)})}
+ [:tr {:style (style {:color color-gray-3})}
(map-indexed (fn [col-idx cell]
- [:td {:style (style bar-td-style (when (and bar-width (= col-idx 1)) {:font-weight 700}))}
+ [:td {:style (style (row-style-for-type cell) (when (and bar-width (= col-idx 1)) {:font-weight 700}))}
(h cell)])
row)
(when bar-width
@@ -289,18 +381,27 @@
:when remapped_from]
[remapped_from col-idx])))
+(defn- show-in-table? [{:keys [special_type visibility_type] :as column}]
+ (and (not (isa? special_type :type/Description))
+ (not (contains? #{:details-only :retired :sensitive} visibility_type))))
+
(defn- query-results->header-row
"Returns a row structure with header info from `COLS`. These values
are strings that are ready to be rendered as HTML"
[remapping-lookup cols include-bar?]
{:row (for [maybe-remapped-col cols
- :let [col (if (:remapped_to maybe-remapped-col)
- (nth cols (get remapping-lookup (:name maybe-remapped-col)))
- maybe-remapped-col)]
+ :when (show-in-table? maybe-remapped-col)
+ :let [{:keys [base_type special_type] :as col} (if (:remapped_to maybe-remapped-col)
+ (nth cols (get remapping-lookup (:name maybe-remapped-col)))
+ maybe-remapped-col)
+ column-name (name (or (:display_name col) (:name col)))]
;; If this column is remapped from another, it's already
;; in the output and should be skipped
:when (not (:remapped_from maybe-remapped-col))]
- (str/upper-case (name (or (:display_name col) (:name col)))))
+ (if (or (isa? base_type :type/Number)
+ (isa? special_type :type/Number))
+ (NumericWrapper. column-name)
+ column-name))
:bar-width (when include-bar? 99)})
(defn- query-results->row-seq
@@ -311,7 +412,8 @@
;; cast to double to avoid "Non-terminating decimal expansion" errors
(float (* 100 (/ (double (bar-column row)) max-value))))
:row (for [[maybe-remapped-col maybe-remapped-row-cell] (map vector cols row)
- :when (not (:remapped_from maybe-remapped-col))
+ :when (and (not (:remapped_from maybe-remapped-col))
+ (show-in-table? maybe-remapped-col))
:let [[col row-cell] (if (:remapped_to maybe-remapped-col)
[(nth cols (get remapping-lookup (:name maybe-remapped-col)))
(nth row (get remapping-lookup (:name maybe-remapped-col)))]
@@ -328,38 +430,58 @@
(query-results->header-row remapping-lookup limited-cols bar-column)
(query-results->row-seq timezone remapping-lookup limited-cols (take rows-limit rows) bar-column max-value))))
+(defn- strong-limit-text [number]
+ [:strong {:style (style {:color color-gray-3})} (h (format-number number))])
+
(defn- render-truncation-warning
[col-limit col-count row-limit row-count]
- (if (or (> row-count row-limit)
- (> col-count col-limit))
- [:div {:style (style {:padding-top :16px})}
- (cond
- (> row-count row-limit)
- [:div {:style (style {:color color-gray-2
- :padding-bottom :10px})}
- "Showing " [:strong {:style (style {:color color-gray-3})} (format-number row-limit)]
- " of " [:strong {:style (style {:color color-gray-3})} (format-number row-count)]
- " rows."]
-
- (> col-count col-limit)
- [:div {:style (style {:color color-gray-2
- :padding-bottom :10px})}
- "Showing " [:strong {:style (style {:color color-gray-3})} (format-number col-limit)]
- " of " [:strong {:style (style {:color color-gray-3})} (format-number col-count)]
- " columns."])]))
+ (let [over-row-limit (> row-count row-limit)
+ over-col-limit (> col-count col-limit)]
+ (when (or over-row-limit over-col-limit)
+ [:div {:style (style {:padding-top :16px})}
+ (cond
+
+ (and over-row-limit over-col-limit)
+ [:div {:style (style {:color color-gray-2
+ :padding-bottom :10px})}
+ "Showing " (strong-limit-text row-limit)
+ " of " (strong-limit-text row-count)
+ " rows and " (strong-limit-text col-limit)
+ " of " (strong-limit-text col-count)
+ " columns."]
+
+ over-row-limit
+ [:div {:style (style {:color color-gray-2
+ :padding-bottom :10px})}
+ "Showing " (strong-limit-text row-limit)
+ " of " (strong-limit-text row-count)
+ " rows."]
+
+ over-col-limit
+ [:div {:style (style {:color color-gray-2
+ :padding-bottom :10px})}
+ "Showing " (strong-limit-text col-limit)
+ " of " (strong-limit-text col-count)
+ " columns."])])))
+
+(defn- attached-results-text
+ "Returns hiccup structures to indicate truncated results are available as an attachment"
+ [render-type cols cols-limit rows rows-limit]
+ (when (and (not= :inline render-type)
+ (or (< cols-limit (count cols))
+ (< rows-limit (count rows))))
+ [:div {:style (style {:color color-gray-2})}
+ "More results have been included as a file attachment"]))
(s/defn ^:private render:table :- RenderedPulseCard
- [timezone card {:keys [cols rows] :as data}]
- {:attachments nil
- :content [:div
- (render-table (prep-for-html-rendering timezone cols rows nil nil cols-limit))
- (render-truncation-warning cols-limit (count cols) rows-limit (count rows))]})
-
-(defn- graphing-columns [card {:keys [cols] :as data}]
- [(or (ui-logic/x-axis-rowfn card data)
- first)
- (or (ui-logic/y-axis-rowfn card data)
- second)])
+ [render-type timezone card {:keys [cols rows] :as data}]
+ (let [table-body [:div
+ (render-table (prep-for-html-rendering timezone cols rows nil nil cols-limit))
+ (render-truncation-warning cols-limit (count cols) rows-limit (count rows))]]
+ {:attachments nil
+ :content (if-let [results-attached (attached-results-text render-type cols cols-limit rows rows-limit)]
+ (list results-attached table-body)
+ (list table-body))}))
(s/defn ^:private render:bar :- RenderedPulseCard
[timezone card {:keys [cols rows] :as data}]
@@ -599,31 +721,6 @@
:padding :16px})}
"An error occurred while displaying this card."]})
-(defn detect-pulse-card-type
- "Determine the pulse (visualization) type of a CARD, e.g. `:scalar` or `:bar`."
- [card data]
- (let [col-count (-> data :cols count)
- row-count (-> data :rows count)
- [col-1-rowfn col-2-rowfn] (graphing-columns card data)
- col-1 (col-1-rowfn (:cols data))
- col-2 (col-2-rowfn (:cols data))
- aggregation (-> card :dataset_query :query :aggregation first)]
- (cond
- (or (zero? row-count)
- ;; Many aggregations result in [[nil]] if there are no rows to aggregate after filters
- (= [[nil]] (-> data :rows))) :empty
- (or (> col-count 3)
- (contains? #{:pin_map :state :country} (:display card))) nil
- (and (= col-count 1)
- (= row-count 1)) :scalar
- (and (= col-count 2)
- (> row-count 1)
- (datetime-field? col-1)
- (number-field? col-2)) :sparkline
- (and (= col-count 2)
- (number-field? col-2)) :bar
- :else :table)))
-
(s/defn ^:private make-title-if-needed :- (s/maybe RenderedPulseCard)
[render-type card]
(when *include-title*
@@ -659,7 +756,7 @@
:scalar (render:scalar timezone card data)
:sparkline (render:sparkline render-type timezone card data)
:bar (render:bar timezone card data)
- :table (render:table timezone card data)
+ :table (render:table render-type timezone card data)
(if (is-attached? card)
(render:attached render-type card data)
(render:unknown card data)))
@@ -691,16 +788,20 @@
(s/defn render-pulse-section :- RenderedPulseCard
"Render a specific section of a Pulse, i.e. a single Card, to Hiccup HTML."
- [timezone {:keys [card result]}]
+ [timezone {card :card {:keys [data] :as result} :result}]
(let [{:keys [attachments content]} (binding [*include-title* true]
(render-pulse-card :attachment timezone card result))]
{:attachments attachments
- :content [:div {:style (style {:margin-top :10px
- :margin-bottom :20px
- :border "1px solid #dddddd"
- :border-radius :2px
- :background-color :white
- :box-shadow "0 1px 2px rgba(0, 0, 0, .08)"})}
+ :content [:div {:style (style (merge {:margin-top :10px
+ :margin-bottom :20px}
+ ;; Don't include the border on cards rendered with a table as the table
+ ;; will be to larger and overrun the border
+ (when-not (= :table (detect-pulse-card-type card data))
+ {:border "1px solid #dddddd"
+ :border-radius :2px
+ :background-color :white
+ :width "500px !important"
+ :box-shadow "0 1px 2px rgba(0, 0, 0, .08)"})))}
content]}))
(defn render-pulse-card-to-png
diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj
index b87f5e8d1dedb2cea8dd30cc7d6e4ee4a00cf12e..bf203e52134c805cc1b50c1c44965bd27a2c6d4a 100644
--- a/src/metabase/query_processor.clj
+++ b/src/metabase/query_processor.clj
@@ -281,3 +281,22 @@
(run-and-save-query! (assoc query :info (assoc options
:query-hash (qputil/query-hash query)
:query-type (if (qputil/mbql-query? query) "MBQL" "native")))))
+
+(def ^:private ^:const max-results-bare-rows
+ "Maximum number of rows to return specifically on :rows type queries via the API."
+ 2000)
+
+(def ^:private ^:const max-results
+ "General maximum number of rows to return from an API query."
+ 10000)
+
+(def default-query-constraints
+ "Default map of constraints that we apply on dataset queries executed by the api."
+ {:max-results max-results
+ :max-results-bare-rows max-results-bare-rows})
+
+(s/defn process-query-and-save-with-max!
+ "Same as `process-query-and-save-execution!` but will include the default max rows returned as a constraint"
+ {:style/indent 1}
+ [query, options :- DatasetQueryOptions]
+ (process-query-and-save-execution! (assoc query :constraints default-query-constraints) options))
diff --git a/test/metabase/api/dataset_test.clj b/test/metabase/api/dataset_test.clj
index e4915aaf7245d672bac6676e8b6da67f05307cde..2ce781577505b97402bc2b1c7876c638279e4f54 100644
--- a/test/metabase/api/dataset_test.clj
+++ b/test/metabase/api/dataset_test.clj
@@ -8,10 +8,10 @@
[dk.ative.docjure.spreadsheet :as spreadsheet]
[expectations :refer :all]
[medley.core :as m]
- [metabase.api.dataset :refer [default-query-constraints]]
[metabase.models
[database :refer [Database]]
[query-execution :refer [QueryExecution]]]
+ [metabase.query-processor :as qp]
[metabase.query-processor.middleware.expand :as ql]
[metabase.sync :as sync]
[metabase.test
@@ -75,7 +75,7 @@
(ql/aggregation (ql/count))))
(assoc :type "query")
(assoc-in [:query :aggregation] [{:aggregation-type "count", :custom-name nil}])
- (assoc :constraints default-query-constraints))
+ (assoc :constraints qp/default-query-constraints))
:started_at true
:running_time true
:average_execution_time nil}
@@ -113,7 +113,7 @@
:json_query {:database (id)
:type "native"
:native {:query "foobar"}
- :constraints default-query-constraints}
+ :constraints qp/default-query-constraints}
:started_at true
:running_time true}
;; QueryExecution entry in the DB
diff --git a/test/metabase/api/session_test.clj b/test/metabase/api/session_test.clj
index 3412c041c24f61d11048b0072ab23c5b90f4bbc9..4a8c49e7fb8fd7f52e675037959a852f8bb8a5de 100644
--- a/test/metabase/api/session_test.clj
+++ b/test/metabase/api/session_test.clj
@@ -2,6 +2,7 @@
"Tests for /api/session"
(:require [expectations :refer :all]
[metabase
+ [email-test :as et]
[http-client :refer :all]
[public-settings :as public-settings]
[util :as u]]
@@ -68,16 +69,17 @@
;; ## POST /api/session/forgot_password
;; Test that we can initiate password reset
(expect
- (let [reset-fields-set? (fn []
- (let [{:keys [reset_token reset_triggered]} (db/select-one [User :reset_token :reset_triggered], :id (user->id :rasta))]
- (boolean (and reset_token reset_triggered))))]
- ;; make sure user is starting with no values
- (db/update! User (user->id :rasta), :reset_token nil, :reset_triggered nil)
- (assert (not (reset-fields-set?)))
- ;; issue reset request (token & timestamp should be saved)
- ((user->client :rasta) :post 200 "session/forgot_password" {:email (:username (user->credentials :rasta))})
- ;; TODO - how can we test email sent here?
- (reset-fields-set?)))
+ (et/with-fake-inbox
+ (let [reset-fields-set? (fn []
+ (let [{:keys [reset_token reset_triggered]} (db/select-one [User :reset_token :reset_triggered], :id (user->id :rasta))]
+ (boolean (and reset_token reset_triggered))))]
+ ;; make sure user is starting with no values
+ (db/update! User (user->id :rasta), :reset_token nil, :reset_triggered nil)
+ (assert (not (reset-fields-set?)))
+ ;; issue reset request (token & timestamp should be saved)
+ ((user->client :rasta) :post 200 "session/forgot_password" {:email (:username (user->credentials :rasta))})
+ ;; TODO - how can we test email sent here?
+ (reset-fields-set?))))
;; Test that email is required
(expect {:errors {:email "value must be a valid email address."}}
@@ -94,39 +96,41 @@
(expect
{:reset_token nil
:reset_triggered nil}
- (let [password {:old "password"
- :new "whateverUP12!!"}]
- (tt/with-temp User [{:keys [email id]} {:password (:old password), :reset_triggered (System/currentTimeMillis)}]
- (let [token (u/prog1 (str id "_" (java.util.UUID/randomUUID))
- (db/update! User id, :reset_token <>))
- creds {:old {:password (:old password)
- :username email}
- :new {:password (:new password)
- :username email}}]
- ;; Check that creds work
- (client :post 200 "session" (:old creds))
-
- ;; Call reset password endpoint to change the PW
- (client :post 200 "session/reset_password" {:token token
- :password (:new password)})
- ;; Old creds should no longer work
- (assert (= (client :post 400 "session" (:old creds))
- {:errors {:password "did not match stored password"}}))
- ;; New creds *should* work
- (client :post 200 "session" (:new creds))
- ;; Double check that reset token was cleared
- (db/select-one [User :reset_token :reset_triggered], :id id)))))
+ (et/with-fake-inbox
+ (let [password {:old "password"
+ :new "whateverUP12!!"}]
+ (tt/with-temp User [{:keys [email id]} {:password (:old password), :reset_triggered (System/currentTimeMillis)}]
+ (let [token (u/prog1 (str id "_" (java.util.UUID/randomUUID))
+ (db/update! User id, :reset_token <>))
+ creds {:old {:password (:old password)
+ :username email}
+ :new {:password (:new password)
+ :username email}}]
+ ;; Check that creds work
+ (client :post 200 "session" (:old creds))
+
+ ;; Call reset password endpoint to change the PW
+ (client :post 200 "session/reset_password" {:token token
+ :password (:new password)})
+ ;; Old creds should no longer work
+ (assert (= (client :post 400 "session" (:old creds))
+ {:errors {:password "did not match stored password"}}))
+ ;; New creds *should* work
+ (client :post 200 "session" (:new creds))
+ ;; Double check that reset token was cleared
+ (db/select-one [User :reset_token :reset_triggered], :id id))))))
;; Check that password reset returns a valid session token
(expect
{:success true
:session_id true}
- (tt/with-temp User [{:keys [id]} {:reset_triggered (System/currentTimeMillis)}]
- (let [token (u/prog1 (str id "_" (java.util.UUID/randomUUID))
- (db/update! User id, :reset_token <>))]
- (-> (client :post 200 "session/reset_password" {:token token
- :password "whateverUP12!!"})
- (update :session_id tu/is-uuid-string?)))))
+ (et/with-fake-inbox
+ (tt/with-temp User [{:keys [id]} {:reset_triggered (System/currentTimeMillis)}]
+ (let [token (u/prog1 (str id "_" (java.util.UUID/randomUUID))
+ (db/update! User id, :reset_token <>))]
+ (-> (client :post 200 "session/reset_password" {:token token
+ :password "whateverUP12!!"})
+ (update :session_id tu/is-uuid-string?))))))
;; Test that token and password are required
(expect {:errors {:token "value must be a non-blank string."}}
@@ -217,11 +221,12 @@
;; should totally work if the email domains match up
(expect
{:first_name "Rasta", :last_name "Toucan", :email "rasta@sf-toucannery.com"}
- (tu/with-temporary-setting-values [google-auth-auto-create-accounts-domain "sf-toucannery.com"
- admin-email "rasta@toucans.com"]
- (select-keys (u/prog1 (#'session-api/google-auth-create-new-user! "Rasta" "Toucan" "rasta@sf-toucannery.com")
- (db/delete! User :id (:id <>))) ; make sure we clean up after ourselves !
- [:first_name :last_name :email])))
+ (et/with-fake-inbox
+ (tu/with-temporary-setting-values [google-auth-auto-create-accounts-domain "sf-toucannery.com"
+ admin-email "rasta@toucans.com"]
+ (select-keys (u/prog1 (#'session-api/google-auth-create-new-user! "Rasta" "Toucan" "rasta@sf-toucannery.com")
+ (db/delete! User :id (:id <>))) ; make sure we clean up after ourselves !
+ [:first_name :last_name :email]))))
;;; tests for google-auth-fetch-or-create-user!
@@ -248,10 +253,11 @@
;; test that a user that doesn't exist with the *same* domain as the auto-create accounts domain means a new user gets
;; created
(expect
- (tu/with-temporary-setting-values [google-auth-auto-create-accounts-domain "sf-toucannery.com"
- admin-email "rasta@toucans.com"]
- (u/prog1 (is-session? (#'session-api/google-auth-fetch-or-create-user! "Rasta" "Toucan" "rasta@sf-toucannery.com"))
- (db/delete! User :email "rasta@sf-toucannery.com")))) ; clean up after ourselves
+ (et/with-fake-inbox
+ (tu/with-temporary-setting-values [google-auth-auto-create-accounts-domain "sf-toucannery.com"
+ admin-email "rasta@toucans.com"]
+ (u/prog1 (is-session? (#'session-api/google-auth-fetch-or-create-user! "Rasta" "Toucan" "rasta@sf-toucannery.com"))
+ (db/delete! User :email "rasta@sf-toucannery.com"))))) ; clean up after ourselves
;;; ------------------------------------------- TESTS FOR LDAP AUTH STUFF --------------------------------------------
diff --git a/test/metabase/api/user_test.clj b/test/metabase/api/user_test.clj
index 8e82e371e6890feb60e58ffeb2798ab0fe10a31c..04f58fa3e8c73443d339ee49a2da72fe0b961eb9 100644
--- a/test/metabase/api/user_test.clj
+++ b/test/metabase/api/user_test.clj
@@ -2,6 +2,7 @@
"Tests for /api/user endpoints."
(:require [expectations :refer :all]
[metabase
+ [email-test :as et]
[http-client :as http]
[middleware :as middleware]
[util :as u]]
@@ -73,13 +74,14 @@
:common_name (str user-name " " user-name)
:is_superuser false
:is_qbnewb true}
- (do ((user->client :crowberto) :post 200 "user" {:first_name user-name
- :last_name user-name
- :email email})
- (u/prog1 (db/select-one [User :email :first_name :last_name :is_superuser :is_qbnewb]
- :email email)
- ;; clean up after ourselves
- (db/delete! User :email email)))))
+ (et/with-fake-inbox
+ ((user->client :crowberto) :post 200 "user" {:first_name user-name
+ :last_name user-name
+ :email email})
+ (u/prog1 (db/select-one [User :email :first_name :last_name :is_superuser :is_qbnewb]
+ :email email)
+ ;; clean up after ourselves
+ (db/delete! User :email email)))))
;; Test that reactivating a disabled account works
diff --git a/test/metabase/pulse/render_test.clj b/test/metabase/pulse/render_test.clj
index 43e01a9cf5ac0c26855ec811bf6f0b051c8ed3dc..2f5e95915630407467e54ee74ef4a7f0e2df6e82 100644
--- a/test/metabase/pulse/render_test.clj
+++ b/test/metabase/pulse/render_test.clj
@@ -1,5 +1,6 @@
(ns metabase.pulse.render-test
- (:require [expectations :refer :all]
+ (:require [clojure.walk :as walk]
+ [expectations :refer :all]
[hiccup.core :refer [html]]
[metabase.pulse.render :as render :refer :all])
(:import java.util.TimeZone))
@@ -7,58 +8,120 @@
(def ^:private pacific-tz (TimeZone/getTimeZone "America/Los_Angeles"))
(def ^:private test-columns
- [{:name "ID",
- :display_name "ID",
- :base_type :type/BigInteger
- :special_type nil}
- {:name "latitude"
- :display_name "Latitude"
- :base-type :type/Float
- :special-type :type/Latitude}
- {:name "last_login"
- :display_name "Last Login"
- :base_type :type/DateTime
- :special_type nil}
- {:name "name"
- :display_name "Name"
- :base-type :type/Text
- :special_type nil}])
+ [{:name "ID",
+ :display_name "ID",
+ :base_type :type/BigInteger
+ :special_type nil
+ :visibility_type :normal}
+ {:name "latitude"
+ :display_name "Latitude"
+ :base_type :type/Float
+ :special_type :type/Latitude
+ :visibility_type :normal}
+ {:name "last_login"
+ :display_name "Last Login"
+ :base_type :type/DateTime
+ :special_type nil
+ :visibility_type :normal}
+ {:name "name"
+ :display_name "Name"
+ :base_type :type/Text
+ :special_type nil
+ :visibility_type :normal}])
(def ^:private test-data
[[1 34.0996 "2014-04-01T08:30:00.0000" "Stout Burgers & Beers"]
[2 34.0406 "2014-12-05T15:15:00.0000" "The Apple Pan"]
[3 34.0474 "2014-08-01T12:45:00.0000" "The Gorbals"]])
+(defn- col-counts [results]
+ (set (map (comp count :row) results)))
+
+(defn- number [x]
+ (#'render/map->NumericWrapper {:num-str x}))
+
+(def ^:private default-header-result
+ [{:row [(number "ID") (number "Latitude") "Last Login" "Name"]
+ :bar-width nil}
+ #{4}])
+
+(defn- prep-for-html-rendering'
+ [cols rows bar-column max-value]
+ (let [results (#'render/prep-for-html-rendering pacific-tz cols rows bar-column max-value (count cols))]
+ [(first results)
+ (col-counts results)]))
+
+
+
;; Testing the format of headers
(expect
- {:row ["ID" "LATITUDE" "LAST LOGIN" "NAME"]
- :bar-width nil}
- (first (#'render/prep-for-html-rendering pacific-tz test-columns test-data nil nil (count test-columns))))
+ default-header-result
+ (prep-for-html-rendering' test-columns test-data nil nil))
+
+(expect
+ default-header-result
+ (let [cols-with-desc (conj test-columns {:name "desc_col"
+ :display_name "Description Column"
+ :base_type :type/Text
+ :special_type :type/Description
+ :visibility_type :normal})
+ data-with-desc (mapv #(conj % "Desc") test-data)]
+ (prep-for-html-rendering' cols-with-desc data-with-desc nil nil)))
+
+(expect
+ default-header-result
+ (let [cols-with-details (conj test-columns {:name "detail_col"
+ :display_name "Details Column"
+ :base_type :type/Text
+ :special_type nil
+ :visibility_type :details-only})
+ data-with-details (mapv #(conj % "Details") test-data)]
+ (prep-for-html-rendering' cols-with-details data-with-details nil nil)))
+
+(expect
+ default-header-result
+ (let [cols-with-sensitive (conj test-columns {:name "sensitive_col"
+ :display_name "Sensitive Column"
+ :base_type :type/Text
+ :special_type nil
+ :visibility_type :sensitive})
+ data-with-sensitive (mapv #(conj % "Sensitive") test-data)]
+ (prep-for-html-rendering' cols-with-sensitive data-with-sensitive nil nil)))
+
+(expect
+ default-header-result
+ (let [columns-with-retired (conj test-columns {:name "retired_col"
+ :display_name "Retired Column"
+ :base_type :type/Text
+ :special_type nil
+ :visibility_type :retired})
+ data-with-retired (mapv #(conj % "Retired") test-data)]
+ (prep-for-html-rendering' columns-with-retired data-with-retired nil nil)))
;; When including a bar column, bar-width is 99%
(expect
- {:row ["ID" "LATITUDE" "LAST LOGIN" "NAME"]
- :bar-width 99}
- (first (#'render/prep-for-html-rendering pacific-tz test-columns test-data second 40.0 (count test-columns))))
+ (assoc-in default-header-result [0 :bar-width] 99)
+ (prep-for-html-rendering' test-columns test-data second 40.0))
;; When there are too many columns, #'render/prep-for-html-rendering show narrow it
(expect
- {:row ["ID" "LATITUDE"]
- :bar-width 99}
- (first (#'render/prep-for-html-rendering pacific-tz test-columns test-data second 40.0 2)))
+ [{:row [(number "ID") (number "Latitude")]
+ :bar-width 99}
+ #{2}]
+ (prep-for-html-rendering' (subvec test-columns 0 2) test-data second 40.0 ))
;; Basic test that result rows are formatted correctly (dates, floating point numbers etc)
(expect
- [{:bar-width nil, :row ["1" "34.10" "Apr 1, 2014" "Stout Burgers & Beers"]}
- {:bar-width nil, :row ["2" "34.04" "Dec 5, 2014" "The Apple Pan"]}
- {:bar-width nil, :row ["3" "34.05" "Aug 1, 2014" "The Gorbals"]}]
+ [{:bar-width nil, :row [(number "1") (number "34.10") "Apr 1, 2014" "Stout Burgers & Beers"]}
+ {:bar-width nil, :row [(number "2") (number "34.04") "Dec 5, 2014" "The Apple Pan"]}
+ {:bar-width nil, :row [(number "3") (number "34.05") "Aug 1, 2014" "The Gorbals"]}]
(rest (#'render/prep-for-html-rendering pacific-tz test-columns test-data nil nil (count test-columns))))
;; Testing the bar-column, which is the % of this row relative to the max of that column
(expect
- [{:bar-width (float 85.249), :row ["1" "34.10" "Apr 1, 2014" "Stout Burgers & Beers"]}
- {:bar-width (float 85.1015), :row ["2" "34.04" "Dec 5, 2014" "The Apple Pan"]}
- {:bar-width (float 85.1185), :row ["3" "34.05" "Aug 1, 2014" "The Gorbals"]}]
+ [{:bar-width (float 85.249), :row [(number "1") (number "34.10") "Apr 1, 2014" "Stout Burgers & Beers"]}
+ {:bar-width (float 85.1015), :row [(number "2") (number "34.04") "Dec 5, 2014" "The Apple Pan"]}
+ {:bar-width (float 85.1185), :row [(number "3") (number "34.05") "Aug 1, 2014" "The Gorbals"]}]
(rest (#'render/prep-for-html-rendering pacific-tz test-columns test-data second 40 (count test-columns))))
(defn- add-rating
@@ -91,15 +154,16 @@
;; With a remapped column, the header should contain the name of the remapped column (not the original)
(expect
- {:row ["ID" "LATITUDE" "RATING DESC" "LAST LOGIN" "NAME"]
- :bar-width nil}
- (first (#'render/prep-for-html-rendering pacific-tz test-columns-with-remapping test-data-with-remapping nil nil (count test-columns-with-remapping))))
+ [{:row [(number "ID") (number "Latitude") "Rating Desc" "Last Login" "Name"]
+ :bar-width nil}
+ #{5}]
+ (prep-for-html-rendering' test-columns-with-remapping test-data-with-remapping nil nil))
;; Result rows should include only the remapped column value, not the original
(expect
- [["1" "34.10" "Bad" "Apr 1, 2014" "Stout Burgers & Beers"]
- ["2" "34.04" "Ok" "Dec 5, 2014" "The Apple Pan"]
- ["3" "34.05" "Good" "Aug 1, 2014" "The Gorbals"]]
+ [[(number "1") (number "34.10") "Bad" "Apr 1, 2014" "Stout Burgers & Beers"]
+ [(number "2") (number "34.04") "Ok" "Dec 5, 2014" "The Apple Pan"]
+ [(number "3") (number "34.05") "Good" "Aug 1, 2014" "The Gorbals"]]
(map :row (rest (#'render/prep-for-html-rendering pacific-tz test-columns-with-remapping test-data-with-remapping nil nil (count test-columns-with-remapping)))))
;; There should be no truncation warning if the number of rows/cols is fewer than the row/column limit
@@ -126,9 +190,9 @@
:special_type :type/DateTime}))
(expect
- [{:bar-width nil, :row ["1" "34.10" "Apr 1, 2014" "Stout Burgers & Beers"]}
- {:bar-width nil, :row ["2" "34.04" "Dec 5, 2014" "The Apple Pan"]}
- {:bar-width nil, :row ["3" "34.05" "Aug 1, 2014" "The Gorbals"]}]
+ [{:bar-width nil, :row [(number "1") (number "34.10") "Apr 1, 2014" "Stout Burgers & Beers"]}
+ {:bar-width nil, :row [(number "2") (number "34.04") "Dec 5, 2014" "The Apple Pan"]}
+ {:bar-width nil, :row [(number "3") (number "34.05") "Aug 1, 2014" "The Gorbals"]}]
(rest (#'render/prep-for-html-rendering pacific-tz test-columns-with-date-special-type test-data nil nil (count test-columns))))
(defn- render-scalar-value [results]
@@ -166,3 +230,47 @@
:base_type :type/DateTime
:special_type nil}]
:rows [["2014-04-01T08:30:00.0000"]]}))
+
+(defn- replace-style-maps [hiccup-map]
+ (walk/postwalk (fn [maybe-map]
+ (if (and (map? maybe-map)
+ (contains? maybe-map :style))
+ :style-map
+ maybe-map)) hiccup-map))
+
+(def ^:private render-truncation-warning'
+ (comp replace-style-maps #'render/render-truncation-warning))
+
+(expect
+ nil
+ (render-truncation-warning' 10 5 20 10))
+
+(expect
+ [:div :style-map
+ [:div :style-map
+ "Showing " [:strong :style-map "10"] " of "
+ [:strong :style-map "11"] " columns."]]
+ (render-truncation-warning' 10 11 20 10))
+
+(expect
+ [:div
+ :style-map
+ [:div :style-map "Showing "
+ [:strong :style-map "20"] " of " [:strong :style-map "21"] " rows."]]
+ (render-truncation-warning' 10 5 20 21))
+
+(expect
+ [:div
+ :style-map
+ [:div
+ :style-map
+ "Showing "
+ [:strong :style-map "20"]
+ " of "
+ [:strong :style-map "21"]
+ " rows and "
+ [:strong :style-map "10"]
+ " of "
+ [:strong :style-map "11"]
+ " columns."]]
+ (render-truncation-warning' 10 11 20 21))
diff --git a/test/metabase/pulse_test.clj b/test/metabase/pulse_test.clj
index 7c4b06ecfaeddabe54957b1af6a5cd3758befd7e..2bce6e85ef8a39936523b961fbfe7061d3dbf9a3 100644
--- a/test/metabase/pulse_test.clj
+++ b/test/metabase/pulse_test.clj
@@ -1,17 +1,20 @@
(ns metabase.pulse-test
- (:require [clojure.walk :as walk]
+ (:require [clojure.string :as str]
+ [clojure.walk :as walk]
[expectations :refer :all]
[medley.core :as m]
[metabase.integrations.slack :as slack]
[metabase
[email-test :as et]
- [pulse :refer :all]]
+ [pulse :refer :all]
+ [query-processor :as qp]]
[metabase.models
[card :refer [Card]]
[pulse :refer [Pulse retrieve-pulse retrieve-pulse-or-alert]]
[pulse-card :refer [PulseCard]]
[pulse-channel :refer [PulseChannel]]
[pulse-channel-recipient :refer [PulseChannelRecipient]]]
+ [metabase.pulse.render :as render]
[metabase.test
[data :as data]
[util :as tu]]
@@ -73,6 +76,15 @@
png-attachment]}
email)))
+(def ^:private csv-attachment
+ {:type :attachment, :content-type "text/csv", :file-name "Test card.csv",
+ :content java.net.URL, :description "More results for 'Test card'", :content-id false})
+
+(def ^:private xls-attachment
+ {:type :attachment, :file-name "Test card.xlsx",
+ :content-type "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet",
+ :content java.net.URL, :description "More results for 'Test card'", :content-id false})
+
;; Basic test, 1 card, 1 recipient
(expect
(rasta-pulse-email)
@@ -89,6 +101,72 @@
(send-pulse! (retrieve-pulse pulse-id))
(et/summarize-multipart-email #"Pulse Name"))))
+;; Basic test, 1 card, 1 recipient, 21 results results in a CSV being attached and a table being sent
+(expect
+ (rasta-pulse-email {:body [{"Pulse Name" true
+ "More results have been included" true
+ "ID</th>" true},
+ csv-attachment]})
+ (tt/with-temp* [Card [{card-id :id} (checkins-query {:aggregation nil
+ :limit 21})]
+ Pulse [{pulse-id :id} {:name "Pulse Name"
+ :skip_if_empty false}]
+ PulseCard [_ {:pulse_id pulse-id
+ :card_id card-id
+ :position 0}]
+ PulseChannel [{pc-id :id} {:pulse_id pulse-id}]
+ PulseChannelRecipient [_ {:user_id (rasta-id)
+ :pulse_channel_id pc-id}]]
+ (email-test-setup
+ (send-pulse! (retrieve-pulse pulse-id))
+ (et/summarize-multipart-email #"Pulse Name" #"More results have been included" #"ID</th>"))))
+
+;; Validate pulse queries are limited by qp/default-query-constraints
+(expect
+ 31 ;; Should return 30 results (the redef'd limit) plus the header row
+ (tt/with-temp* [Card [{card-id :id} (checkins-query {:aggregation nil})]
+ Pulse [{pulse-id :id} {:name "Pulse Name"
+ :skip_if_empty false}]
+ PulseCard [_ {:pulse_id pulse-id
+ :card_id card-id
+ :position 0}]
+ PulseChannel [{pc-id :id} {:pulse_id pulse-id}]
+ PulseChannelRecipient [_ {:user_id (rasta-id)
+ :pulse_channel_id pc-id}]]
+ (email-test-setup
+ (with-redefs [qp/default-query-constraints {:max-results 10000
+ :max-results-bare-rows 30}]
+ (send-pulse! (retrieve-pulse pulse-id))
+ ;; Slurp in the generated CSV and count the lines found in the file
+ (-> @et/inbox
+ vals
+ ffirst
+ :body
+ last
+ :content
+ slurp
+ str/split-lines
+ count)))))
+
+;; Basic test, 1 card, 1 recipient, 19 results, so no attachment
+(expect
+ (rasta-pulse-email {:body [{"Pulse Name" true
+ "More results have been included" false
+ "ID</th>" true}]})
+ (tt/with-temp* [Card [{card-id :id} (checkins-query {:aggregation nil
+ :limit 19})]
+ Pulse [{pulse-id :id} {:name "Pulse Name"
+ :skip_if_empty false}]
+ PulseCard [_ {:pulse_id pulse-id
+ :card_id card-id
+ :position 0}]
+ PulseChannel [{pc-id :id} {:pulse_id pulse-id}]
+ PulseChannelRecipient [_ {:user_id (rasta-id)
+ :pulse_channel_id pc-id}]]
+ (email-test-setup
+ (send-pulse! (retrieve-pulse pulse-id))
+ (et/summarize-multipart-email #"Pulse Name" #"More results have been included" #"ID</th>"))))
+
;; Pulse should be sent to two recipients
(expect
(into {} (map (fn [user-kwd]
@@ -193,7 +271,8 @@
;; Rows alert with data
(expect
(rasta-alert-email "Metabase alert: Test card has results"
- [{"Test card.*has results for you to see" true}, png-attachment])
+ [{"Test card.*has results for you to see" true
+ "More results have been included" false}, png-attachment])
(tt/with-temp* [Card [{card-id :id} (checkins-query {:breakout [["datetime-field" (data/id :checkins :date) "hour"]]})]
Pulse [{pulse-id :id} {:alert_condition "rows"
:alert_first_only false}]
@@ -205,7 +284,28 @@
:pulse_channel_id pc-id}]]
(email-test-setup
(send-pulse! (retrieve-pulse-or-alert pulse-id))
- (et/summarize-multipart-email #"Test card.*has results for you to see"))))
+ (et/summarize-multipart-email #"Test card.*has results for you to see" #"More results have been included"))))
+
+;; Rows alert with too much data will attach as CSV and include a table
+(expect
+ (rasta-alert-email "Metabase alert: Test card has results"
+ [{"Test card.*has results for you to see" true
+ "More results have been included" true
+ "ID</th>" true},
+ csv-attachment])
+ (tt/with-temp* [Card [{card-id :id} (checkins-query {:limit 21
+ :aggregation nil})]
+ Pulse [{pulse-id :id} {:alert_condition "rows"
+ :alert_first_only false}]
+ PulseCard [_ {:pulse_id pulse-id
+ :card_id card-id
+ :position 0}]
+ PulseChannel [{pc-id :id} {:pulse_id pulse-id}]
+ PulseChannelRecipient [_ {:user_id (rasta-id)
+ :pulse_channel_id pc-id}]]
+ (email-test-setup
+ (send-pulse! (retrieve-pulse-or-alert pulse-id))
+ (et/summarize-multipart-email #"Test card.*has results for you to see" #"More results have been included" #"ID</th>"))))
;; Above goal alert with data
(expect
@@ -322,6 +422,42 @@
(assoc result :attachments (for [attachment-info attachments]
(update attachment-info :attachment-bytes-thunk fn?))))
+(defprotocol WrappedFunction
+ (input [_])
+ (output [_]))
+
+(defn- invoke-with-wrapping
+ "Apply `args` to `func`, capturing the arguments of the invocation and the result of the invocation. Store the arguments in
+ `input-atom` and the result in `output-atom`."
+ [input-atom output-atom func args]
+ (swap! input-atom conj args)
+ (let [result (apply func args)]
+ (swap! output-atom conj result)
+ result))
+
+(defn- wrap-function
+ "Return a function that wraps `func`, not interfering with it but recording it's input and output, which is
+ available via the `input` function and `output`function that can be used directly on this object"
+ [func]
+ (let [input (atom nil)
+ output (atom nil)]
+ (reify WrappedFunction
+ (input [_] @input)
+ (output [_] @output)
+ clojure.lang.IFn
+ (invoke [_ x1]
+ (invoke-with-wrapping input output func [x1]))
+ (invoke [_ x1 x2]
+ (invoke-with-wrapping input output func [x1 x2]))
+ (invoke [_ x1 x2 x3]
+ (invoke-with-wrapping input output func [x1 x2 x3]))
+ (invoke [_ x1 x2 x3 x4]
+ (invoke-with-wrapping input output func [x1 x2 x3 x4]))
+ (invoke [_ x1 x2 x3 x4 x5]
+ (invoke-with-wrapping input output func [x1 x2 x3 x4 x5]))
+ (invoke [_ x1 x2 x3 x4 x5 x6]
+ (invoke-with-wrapping input output func [x1 x2 x3 x4 x5 x6])))))
+
;; Basic slack test, 1 card, 1 recipient channel
(tt/expect-with-temp [Card [{card-id :id} (checkins-query {:breakout [["datetime-field" (data/id :checkins :date) "hour"]]})]
Pulse [{pulse-id :id} {:name "Pulse Name"
@@ -346,6 +482,47 @@
first
thunk->boolean)))
+(defn- force-bytes-thunk
+ "Grabs the thunk that produces the image byte array and invokes it"
+ [results]
+ ((-> results
+ :attachments
+ first
+ :attachment-bytes-thunk)))
+
+;; Basic slack test, 1 card, 1 recipient channel, verifies that "more results in attachment" text is not present for
+;; slack pulses
+(tt/expect-with-temp [Card [{card-id :id} (checkins-query {:aggregation nil
+ :limit 25})]
+ Pulse [{pulse-id :id} {:name "Pulse Name"
+ :skip_if_empty false}]
+ PulseCard [_ {:pulse_id pulse-id
+ :card_id card-id
+ :position 0}]
+ PulseChannel [{pc-id :id} {:pulse_id pulse-id
+ :channel_type "slack"
+ :details {:channel "#general"}}]]
+ [{:channel-id "#general",
+ :message "Pulse: Pulse Name",
+ :attachments
+ [{:title "Test card",
+ :attachment-bytes-thunk true
+ :title_link (str "https://metabase.com/testmb/question/" card-id),
+ :attachment-name "image.png",
+ :channel-id "FOO",
+ :fallback "Test card"}]}
+ 1 ;; -> attached-results-text should be invoked exactly once
+ [nil] ;; -> attached-results-text should return nil since it's a slack message
+ ]
+ (slack-test-setup
+ (with-redefs [render/attached-results-text (wrap-function (var-get #'render/attached-results-text))]
+ (let [[pulse-results] (send-pulse! (retrieve-pulse pulse-id))]
+ ;; If we don't force the thunk, the rendering code will never execute and attached-results-text won't be called
+ (force-bytes-thunk pulse-results)
+ [(thunk->boolean pulse-results)
+ (count (input (var-get #'render/attached-results-text)))
+ (output (var-get #'render/attached-results-text))]))))
+
(defn- produces-bytes? [{:keys [attachment-bytes-thunk]}]
(< 0 (alength (attachment-bytes-thunk))))
@@ -542,15 +719,6 @@
[@et/inbox
(db/exists? Pulse :id pulse-id)])))
-(def ^:private csv-attachment
- {:type :attachment, :content-type "text/csv", :file-name "Test card.csv",
- :content java.net.URL, :description "Full results for 'Test card'", :content-id false})
-
-(def ^:private xls-attachment
- {:type :attachment, :file-name "Test card.xlsx",
- :content-type "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet",
- :content java.net.URL, :description "Full results for 'Test card'", :content-id false})
-
(defn- add-rasta-attachment
"Append `ATTACHMENT` to the first email found for Rasta"
[email attachment]