Funnel Chart Static-Viz should handle row data more robustly (#40114) (#40537)

* Funnel Chart Static-Viz should handle row data more robustly

The row data coming into the funnel chart render method can evidently have a few slightly different shapes, and may
need to be 'coordinated' with the :funnel.rows key from viz-settings (names and order are specified there).

This is a WIP to make the method more robust to different row data shapes. For example:

[[1 100] [2 200]] should work as well as [["A" 100] ["B" 200]] and correctly handle the viz-settings

WIP because I need to add a test or two, try to get a handle on the exact schema that is allowed for 'row shape' and
the funnel.row viz key, and see if there are failing cases that I haven't considered yet.

* Funnel rows should now work more effectively with possible raw-rows

* Add a test that checks the success of the render and correct order

* Update src/metabase/pulse/render/body.clj



* Update src/metabase/pulse/render/body.clj



* Addressing feedback from the review

 - renamed build-funnel-rows to funnel-rows
 - used 'funnel-viz' as the binding for the {:key "asdf" :name "asdf" :enabled true} maps from the viz settings
 - added docstring to the function to try clarify its 2 branches (keys vs indices on the rows)

---------

Co-authored-by: adam-james <21064735+adam-james-v@users.noreply.github.com>
Co-authored-by: Chris Truter <crisptrutski@users.noreply.github.com>

src/metabase/pulse/render/body.clj

@@ -1016,23 +1016,62 @@
       [:img {:style (style/style {:display :block :width :100%})
              :src   (:image-src image-bundle)}]]}))
 
+(defn- all-unique?
+  [funnel-rows]
+  (let [ks (into #{} (map :key) funnel-rows)]
+    (= (count ks) (count funnel-rows))))
+
+(defn- funnel-rows
+  "Creates the expected row form for the javascript side of our funnel rendering.
+  If funnel-viz exists, we want to use the value in :key as the first elem in the row.
+
+  Eg. funnel-viz -> [{:key \"asdf\" :name \"Asdf\" :enabled true}
+                     {:key \"wasd\" :name \"Wasd\" :enabled true}]
+        raw-rows -> [[1 234] [2 5678]]
+
+  Should become: [[\"asdf\" 234]
+                  [\"wasd\" 5678]]
+
+  Additionally, raw-rows can come in with strings already. In this case we want simply to re-order
+  The raw-rows based on the order of the funnel-rows.
+
+  Eg. funnel-viz -> [{:key \"wasd\" :name \"Wasd\" :enabled true}
+                     {:key \"asdf\" :name \"Asdf\" :enabled true}]
+        raw-rows ->  [[\"asdf\" 234] [\"wasd\" 5678]]
+
+  Should become: [[\"wasd\" 5678]
+                  [\"asdf\" 234]]"
+  [funnel-viz raw-rows]
+  (if (string? (ffirst raw-rows))
+    ;; re-create the rows with the order/visibility specified in the funnel-viz
+    (let [rows (into {} (vec raw-rows))]
+      (for [{k        :key
+             enabled? :enabled} funnel-viz
+            :when               enabled?]
+        [k (get rows k)]))
+    ;; re-create the rows with the label/visibilty specified in the funnel-viz
+    (let [rows-data (map (fn [{k :key enabled? :enabled} [_ value]]
+                           (when enabled?
+                             [k value])) funnel-viz raw-rows)]
+      (remove nil? rows-data))))
+
 (s/defmethod render :funnel :- formatter/RenderedPulseCard
-  [_ render-type _timezone-id card _dashcard {:keys [rows cols viz-settings] :as data}]
+  [_chart-type render-type _timezone-id card _dashcard {:keys [rows cols viz-settings] :as data}]
   (let [[x-axis-rowfn
          y-axis-rowfn] (formatter/graphing-column-row-fns card data)
-        funnel-rows    (:funnel.rows viz-settings)
+        funnel-viz    (:funnel.rows viz-settings)
         raw-rows       (map (juxt x-axis-rowfn y-axis-rowfn)
                             (formatter/row-preprocess x-axis-rowfn y-axis-rowfn rows))
-        rows           (cond->> raw-rows
-                         funnel-rows (mapv (fn [[idx val]]
-                                             [(get-in funnel-rows [(dec idx) :key]) val])))
-        [x-col y-col]  cols
-        settings       (as-> (->js-viz x-col y-col viz-settings) jsviz-settings
-                         (assoc jsviz-settings :step    {:name   (:display_name x-col)
-                                                         :format (:x jsviz-settings)}
-                                :measure {:format (:y jsviz-settings)}))
-        svg            (js-svg/funnel rows settings)
-        image-bundle   (image-bundle/make-image-bundle render-type svg)]
+        rows          (if (and funnel-viz (all-unique? funnel-viz))
+                         (funnel-rows funnel-viz raw-rows)
+                         raw-rows)
+        [x-col y-col] cols
+        settings      (as-> (->js-viz x-col y-col viz-settings) jsviz-settings
+                        (assoc jsviz-settings :step    {:name   (:display_name x-col)
+                                                        :format (:x jsviz-settings)}
+                               :measure {:format (:y jsviz-settings)}))
+        svg           (js-svg/funnel rows settings)
+        image-bundle  (image-bundle/make-image-bundle render-type svg)]
     {:attachments
      (image-bundle/image-bundle->attachment image-bundle)
 

test/metabase/pulse/render/body_test.clj

@@ -11,6 +11,7 @@
    [metabase.pulse.render.test-util :as render.tu]
    [metabase.query-processor :as qp]
    [metabase.test :as mt]
+   [metabase.test.data.interface :as tx]
    [toucan2.core :as t2]))
 
 (use-fixtures :each
@@ -602,6 +603,50 @@
                             doc)]
             (is (not (render-error? pulse-body)))))))))
 
+(def ^:private funnel-rows
+  [["cart" 1500]
+   ["checkout" 450]
+   ["homepage" 10000]
+   ["product_page" 5000]
+   ["purchase" 225]])
+
+(tx/defdataset funnel-data
+  [["stages"
+    [{:field-name "stage", :base-type :type/Text}
+     {:field-name "count", :base-type :type/Quantity}]
+    funnel-rows]])
+
+(deftest render-funnel-with-row-keys-test
+  (testing "Static-viz Funnel Chart with text keys in viz-settings and text in returned
+            rows renders without error and in the order specified by the viz-settings (#39743)."
+    (mt/dataset funnel-data
+      (let [funnel-query {:database (mt/id)
+                          :type     :query
+                          :query
+                          {:source-table (mt/id :stages)
+                           ;; we explicitly select the 2 columns because if we don't the query returns the ID as well.
+                           ;; this is done here to construct the failing case resulting from the reproduction steps in issue #39743
+                           :fields       [[:field (mt/id :stages :stage)]
+                                          [:field (mt/id :stages :count)]]}}
+            funnel-card  {:display       :funnel
+                          :dataset_query funnel-query
+                          :visualization_settings
+                          {:funnel.rows
+                           [{:key "homepage" :name "homepage" :enabled true}
+                            {:key "product_page" :name "product_page" :enabled true}
+                            {:key "cart" :name "cart" :enabled true}
+                            {:key "checkout" :name "checkout" :enabled true}
+                            {:key "purchase" :name "purchase" :enabled true}]}}]
+        (mt/with-temp [:model/Card {card-id :id} funnel-card]
+          (let [row-names      (into #{} (map first funnel-rows))
+                doc            (render.tu/render-card-as-hickory card-id)
+                section-labels (->> doc
+                                    (hik.s/select (hik.s/tag :tspan))
+                                    (mapv (comp first :content))
+                                    (filter row-names))]
+            (is (= (map :key (get-in funnel-card [:visualization_settings :funnel.rows]))
+                   section-labels))))))))
+
 (deftest render-categorical-donut-test
   (let [columns [{:name          "category",
                   :display_name  "Category",