Merge pull request #5613 from metabase/pulse-tests

Split pulse HTML creation code from it's backing data structure

src/metabase/pulse/render.clj

@@ -89,8 +89,8 @@
 
 (defn- datetime-field?
   [field]
-  (or (isa? (:base_type field) :type/DateTime)
-      (isa? (:base_type field) :type/DateTime)))
+  (or (isa? (:base_type field)    :type/DateTime)
+      (isa? (:special_type field) :type/DateTime)))
 
 (defn- number-field?
   [field]
@@ -224,86 +224,115 @@
     (render-to-png html os width)
     (.toByteArray os)))
 
-(defn- create-remapping-lookup [cols col-indexes]
-  (into {}
-        (for [col-index col-indexes
-              :let [{:keys [remapped_from]} (nth cols col-index)]
-              :when remapped_from]
-          [remapped_from col-index])))
-
 (defn- render-table
-  [card rows cols col-indexes bar-column]
-  (let [max-value (if bar-column (apply max (map bar-column rows)))
-        remapping-lookup (create-remapping-lookup cols col-indexes)]
-    [:table {:style (style {:padding-bottom :8px, :border-bottom (str "4px solid " color-gray-1)})}
+  [header+rows]
+  [:table {:style (style {:padding-bottom :8px, :border-bottom (str "4px solid " color-gray-1)})}
+   (let [{header-row :row bar-width :bar-width} (first header+rows)]
      [:thead
       [:tr
-       (for [col-idx col-indexes
-             :let [col-at-index (nth cols col-idx)
-                   col (if (:remapped_to col-at-index)
-                         (nth cols (get remapping-lookup (:name col-at-index)))
-                         col-at-index)]
-             :when (not (:remapped_from col-at-index))]
+       (for [header-cell header-row]
          [:th {:style (style bar-td-style bar-th-style {:min-width :60px})}
-          (h (s/upper-case (name (or (:display_name col) (:name col)))))])
-       (when bar-column
-         [:th {:style (style bar-td-style bar-th-style {:width "99%"})}])]]
-     [:tbody
-      (map-indexed (fn [row-idx row]
-                     [:tr {:style (style {:color (if (odd? row-idx) color-gray-2 color-gray-3)})}
-                      (for [col-idx col-indexes
-                            :let [col (nth cols col-idx)]
-                            :when (not (:remapped_from col))]
-                        [:td {:style (style bar-td-style (when (and bar-column (= col-idx 1)) {:font-weight 700}))}
-                         (if-let [remapped-index (and (:remapped_to col)
-                                                      (get remapping-lookup (:name col)))]
-                           (-> row (nth remapped-index) (format-cell (nth cols remapped-index)) h)
-                           (-> row (nth col-idx) (format-cell col) h))])
-                      (when bar-column
-                        [:td {:style (style bar-td-style {:width :99%})}
-                         [:div {:style (style {:background-color color-purple
-                                               :max-height       :10px
-                                               :height           :10px
-                                               :border-radius    :2px
-                                               :width            (str (float (* 100 (/ (double (bar-column row)) max-value))) "%")})} ; cast to double to avoid "Non-terminating decimal expansion" errors
-                          " "]])])
-                   rows)]]))
+          (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)})}
+                    (map-indexed (fn [col-idx cell]
+                                   [:td {:style (style bar-td-style (when (and bar-width (= col-idx 1)) {:font-weight 700}))}
+                                    (h cell)])
+                                 row)
+                    (when bar-width
+                      [:td {:style (style bar-td-style {:width :99%})}
+                       [:div {:style (style {:background-color color-purple
+                                             :max-height       :10px
+                                             :height           :10px
+                                             :border-radius    :2px
+                                             :width            (str bar-width "%")})}
+                        " "]])])
+                 (rest header+rows))]])
+
+(defn- create-remapping-lookup
+  "Creates a map with from column names to a column index. This is
+  used to figure out what a given column name or value should be
+  replaced with"
+  [cols]
+  (into {}
+        (for [[col-idx {:keys [remapped_from]}] (map vector (range) cols)
+              :when remapped_from]
+          [remapped_from col-idx])))
+
+(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)]
+              ;; If this column is remapped from another, it's already
+              ;; in the output and should be skipped
+              :when (not (:remapped_from maybe-remapped-col))]
+          (s/upper-case (name (or (:display_name col) (:name col)))))
+   :bar-width (when include-bar? 99)})
+
+(defn- query-results->row-seq
+  "Returns a seq of stringified formatted rows that can be rendered into HTML"
+  [remapping-lookup cols rows bar-column max-value]
+  (for [row rows]
+    {:bar-width (when bar-column
+                  ;; 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))
+                :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)))]
+                                       [maybe-remapped-col maybe-remapped-row-cell])]]
+            (format-cell row-cell col))}))
+
+(defn- prep-for-html-rendering
+  "Convert the query results (`COLS` and `ROWS`) into a formatted seq
+  of rows (list of strings) that can be rendered as HTML"
+  [cols rows bar-column max-value column-limit]
+  (let [remapping-lookup (create-remapping-lookup cols)
+        limited-cols (take column-limit cols)]
+    (cons
+     (query-results->header-row remapping-lookup limited-cols bar-column)
+     (query-results->row-seq remapping-lookup limited-cols (take rows-limit rows) bar-column max-value))))
 
 (defn- render-truncation-warning
-  [card {:keys [cols rows]} rows-limit cols-limit]
-  (if (or (> (count rows) rows-limit)
-          (> (count cols) cols-limit))
+  [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
-       (> (count rows) rows-limit)
+       (> row-count row-limit)
        [:div {:style (style {:color color-gray-2
                              :padding-bottom :10px})}
-        "Showing " [:strong {:style (style {:color color-gray-3})} (format-number rows-limit)]
-        " of "     [:strong {:style (style {:color color-gray-3})} (format-number (count rows))]
+        "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."]
 
-       (> (count cols) cols-limit)
+       (> col-count col-limit)
        [:div {:style (style {:color          color-gray-2
                              :padding-bottom :10px})}
-        "Showing " [:strong {:style (style {:color color-gray-3})} (format-number cols-limit)]
-        " of "     [:strong {:style (style {:color color-gray-3})} (format-number (count cols))]
+        "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."])]))
 
 (defn- render:table
   [card {:keys [cols rows] :as data}]
-  (let [truncated-rows (take rows-limit rows)
-        truncated-cols (take cols-limit cols)
-        col-indexes    (map-indexed (fn [i _] i) truncated-cols)]
-    [:div
-     (render-table card truncated-rows truncated-cols col-indexes nil)
-     (render-truncation-warning card data rows-limit cols-limit)]))
+  [:div
+   (render-table (prep-for-html-rendering cols rows nil nil cols-limit))
+   (render-truncation-warning cols-limit (count cols) rows-limit (count rows))])
 
 (defn- render:bar
   [card {:keys [cols rows] :as data}]
-  (let [truncated-rows (take rows-limit rows)]
+  (let [max-value (apply max (map second rows))]
     [:div
-     (render-table card truncated-rows cols [0 1] second)
-     (render-truncation-warning card data rows-limit 2)]))
+     (render-table (prep-for-html-rendering cols rows second max-value 2))
+     (render-truncation-warning 2 (count cols) rows-limit (count rows))]))
 
 (defn- render:scalar
   [card {:keys [cols rows]}]

test/metabase/pulse/render_test.clj

+(ns metabase.pulse.render-test
+  (:require [clj-time.coerce :as c]
+            [expectations :refer :all]
+            [hiccup.core :refer [html]]
+            [metabase.pulse.render :refer :all]
+            [metabase.test.util :as tu]))
+
+(tu/resolve-private-vars metabase.pulse.render prep-for-html-rendering render-truncation-warning)
+
+(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}])
+
+(def ^:private test-data
+  [[1 34.0996 (c/from-string "2014-04-01T08:30:00.0000") "Stout Burgers & Beers"]
+   [2 34.0406 (c/from-string "2014-12-05T15:15:00.0000") "The Apple Pan"]
+   [3 34.0474 (c/from-string "2014-08-01T12:45:00.0000") "The Gorbals"]])
+
+;; Testing the format of headers
+(expect
+  {:row ["ID" "LATITUDE" "LAST LOGIN" "NAME"]
+   :bar-width nil}
+  (first (prep-for-html-rendering test-columns test-data nil nil (count test-columns))))
+
+;; When including a bar column, bar-width is 99%
+(expect
+  {:row ["ID" "LATITUDE" "LAST LOGIN" "NAME"]
+   :bar-width 99}
+  (first (prep-for-html-rendering test-columns test-data second 40.0 (count test-columns))))
+
+;; When there are too many columns, prep-for-html-rendering show narrow it
+(expect
+  {:row ["ID" "LATITUDE"]
+   :bar-width 99}
+  (first (prep-for-html-rendering test-columns test-data second 40.0 2)))
+
+;; 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"]}]
+  (rest (prep-for-html-rendering 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"]}]
+  (rest (prep-for-html-rendering test-columns test-data second 40 (count test-columns))))
+
+(defn- add-rating
+  "Injects `RATING-OR-COL` and `DESCRIPTION-OR-COL` into `COLUMNS-OR-ROW`"
+  [columns-or-row rating-or-col description-or-col]
+  (vec
+   (concat (subvec columns-or-row 0 2)
+           [rating-or-col]
+           (subvec columns-or-row 2)
+           [description-or-col])))
+
+(def ^:private test-columns-with-remapping
+  (add-rating test-columns
+              {:name         "rating"
+               :display_name "Rating"
+               :base_type    :type/Integer
+               :special_type :type/Category
+               :remapped_to  "rating_desc"}
+              {:name          "rating_desc"
+               :display_name  "Rating Desc"
+               :base_type     :type/Text
+               :special_type  nil
+               :remapped_from "rating"}))
+
+(def ^:private test-data-with-remapping
+  (mapv add-rating
+        test-data
+        [1 2 3]
+        ["Bad" "Ok" "Good"]))
+
+;; 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 (prep-for-html-rendering test-columns-with-remapping test-data-with-remapping nil nil (count test-columns-with-remapping))))
+
+;; 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"]]
+  (map :row (rest (prep-for-html-rendering 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
+(expect
+  ""
+  (html (render-truncation-warning 100 10 100 10)))
+
+;; When there are more rows than the limit, check to ensure a truncation warning is present
+(expect
+  [true false]
+  (let [html-output (html (render-truncation-warning 100 10 10 100))]
+    [(boolean (re-find #"Showing.*10.*of.*100.*rows" html-output))
+     (boolean (re-find #"Showing .* of .* columns" html-output))]))
+
+;; When there are more columns than the limit, check to ensure a truncation warning is present
+(expect
+  [true false]
+  (let [html-output (html (render-truncation-warning 10 100 100 10))]
+    [(boolean (re-find #"Showing.*10.*of.*100.*columns" html-output))
+     (boolean (re-find #"Showing .* of .* rows" html-output))]))
+
+(def ^:private test-columns-with-date-special-type
+  (update test-columns 2 merge {:base_type    :type/Text
+                                :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"]}]
+  (rest (prep-for-html-rendering test-columns-with-date-special-type test-data nil nil (count test-columns))))