From 910afcec40273d169d4c1b9e9a94233b3e4ac607 Mon Sep 17 00:00:00 2001
From: adam-james <21064735+adam-james-v@users.noreply.github.com>
Date: Tue, 6 Sep 2022 13:09:38 -0700
Subject: [PATCH] Fix condtional to properly render multi-line charts in
static-viz (#25216)
---
src/metabase/pulse/render.clj | 30 +++---
test/metabase/pulse/render_test.clj | 154 +++++++++++++++-------------
2 files changed, 92 insertions(+), 92 deletions(-)
diff --git a/src/metabase/pulse/render.clj b/src/metabase/pulse/render.clj
index 60e0909743e..129a79ef6ca 100644
--- a/src/metabase/pulse/render.clj
+++ b/src/metabase/pulse/render.clj
@@ -85,17 +85,9 @@
(#{:pin_map :state :country} display-type)
(chart-type nil "display-type is %s" display-type)
- (#{:area
- :bar
- :combo
- :funnel
- :progress
- :table
- :waterfall} display-type)
- (chart-type display-type "display-type is %s" display-type)
-
;; for scalar/smartscalar, the display-type might actually be :line, so we can't have line above
- (= @col-sample-count @row-sample-count 1)
+ (and (not= display-type :progress)
+ (= @col-sample-count @row-sample-count 1))
(chart-type :scalar "result has one row and one column")
(and (= display-type :smartscalar)
@@ -103,19 +95,21 @@
(seq insights))
(chart-type :smartscalar "result has two columns and insights")
+ (#{:line
+ :area
+ :bar
+ :combo
+ :funnel
+ :progress
+ :table
+ :waterfall} display-type)
+ (chart-type display-type "display-type is %s" display-type)
+
(and (some? maybe-dashcard)
(> (count (dashboard-card/dashcard->multi-cards maybe-dashcard)) 0)
(not (#{:combo} display-type)))
(chart-type :multiple "result has multiple card semantics, a multiple chart")
- ;; we have to check when display-type is :line that there are enough rows/cols to actually create a line chart
- ;; if there is only 1 row and 1 col, the chart should be considered scalar, actually.
- (and (= @col-sample-count 2)
- (> @row-sample-count 1)
- (number-field? @col-2)
- (not (#{:waterfall :pie :table :area} display-type)))
- (chart-type :line "result has 2 cols (%s and %s (number)) and > 1 row" (col-description @col-1) (col-description @col-2))
-
(and (= @col-sample-count 2)
(number-field? @col-2)
(= display-type :pie))
diff --git a/test/metabase/pulse/render_test.clj b/test/metabase/pulse/render_test.clj
index f38cb8b06b9..e62c8c763a5 100644
--- a/test/metabase/pulse/render_test.clj
+++ b/test/metabase/pulse/render_test.clj
@@ -38,81 +38,87 @@
"There was a problem with this question."))))
(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
- :name "apples"}]
- :rows [[#t "2020" 2]
- [#t "2021" 3]]
- :insights [{:name "apples"
- :last-value 3
- :previous-value 2
- :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 [_ {:dashboardcard_id (u/the-id dc1) :card_id (u/the-id card2)}]]
- (render/detect-pulse-chart-type card1
- dc1
- {:cols [{:base_type :type/Temporal}
+ (testing "Currently unsupported chart types for static-viz return `nil`."
+ (is (= [nil nil nil]
+ (map #(render/detect-pulse-chart-type {:display %}
+ {}
+ {:cols [{:base_type :type/Number}]
+ :rows [[2]]})
+ [:pin_map :state :country]))))
+ (testing "Queries resulting in no rows return `:empty`."
+ (is (= :empty
+ (render/detect-pulse-chart-type {:display :line}
+ {}
+ {:cols [{:base_type :type/Number}]
+ :rows [[nil]]}))))
+ (testing "Unrecognized display-types with otherwise valid results return `:table`."
+ (is (= :table
+ (render/detect-pulse-chart-type {:display :unrecognized}
+ {}
+ {:cols [{:base_type :type/Text}
{: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]]})))
-
- ;; timeseries line chart
- (is (= :line
- (render/detect-pulse-chart-type {:display :line}
- {}
- {:cols [{:base_type :type/Temporal}
- {:base_type :type/Number}]
- :rows [[#t "2020" 2]
- [#t "2021" 3]]})))
- ;; Category line chart
- (is (= :line
- (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]
- ["banana" 4]]}))))
+ :rows [["A" 2]
+ ["B" 4]]}))))
+ (testing "Scalar and Smartscalar charts are correctly identified"
+ (is (= :scalar
+ (render/detect-pulse-chart-type {:display :line}
+ {}
+ {:cols [{:base_type :type/Number}]
+ :rows [[3]]})))
+ (is (= :scalar
+ (render/detect-pulse-chart-type {:display :scalar}
+ {}
+ {: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
+ :name "apples"}]
+ :rows [[#t "2020" 2]
+ [#t "2021" 3]]
+ :insights [{:name "apples"
+ :last-value 3
+ :previous-value 2
+ :last-change 50.0}]}))))
+ (testing "Progress charts are correctly identified"
+ (is (= :progress
+ (render/detect-pulse-chart-type {:display :progress}
+ {}
+ {:cols [{:base_type :type/Number}]
+ :rows [[6]]}))))
+ (testing "Various Single-Series display-types return correct chart-types."
+ (mapv #(is (= %
+ (render/detect-pulse-chart-type {:display %}
+ {}
+ {:cols [{:base_type :type/Text}
+ {:base_type :type/Number}]
+ :rows [["A" 2]
+ ["B" 3]]})))
+ [:line :area :bar :combo :funnel :progress :table :waterfall]))
+ (testing "Pie charts are correctly identified and return `:categorical/donut`."
+ (is (= :categorical/donut
+ (render/detect-pulse-chart-type {:display :pie}
+ {}
+ {:cols [{:base_type :type/Text}
+ {:base_type :type/Number}]
+ :rows [["apple" 3]
+ ["banana" 4]]}))))
+ (testing "Dashboard Cards can return `:multiple`."
+ (is (= :multiple
+ (mt/with-temp* [Card [card1 {:display :pie}]
+ Card [card2 {:display :funnel}]
+ Dashboard [dashboard]
+ DashboardCard [dc1 {:dashboard_id (u/the-id dashboard) :card_id (u/the-id card1)}]
+ DashboardCardSeries [_ {: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]]}))))))
(deftest make-description-if-needed-test
(testing "Use Visualization Settings's description if it exists"
--
GitLab