From 3dfcb3d5613a69cd34076f51da382b5991cee076 Mon Sep 17 00:00:00 2001
From: adam-james <21064735+adam-james-v@users.noreply.github.com>
Date: Fri, 27 Sep 2024 16:38:53 +0200
Subject: [PATCH] Pivot Options are Properly Calculated for Questions based on
Models with Joins (#47830)
Fixes 46575
Creating a Pivot Table Question that is based off of a model that has at least one column derived from a join failed
to display row totals.
This is because the pivot-options map was being mis-calculated; not all column indices were correctly found/passed in
to the :pivot-rows or :pivot-cols keys, causing the pivot query not to compute all necessary data.
Here, I just modify the :lib/source key of the columns whose source is a card (as determined by the existence of
:lib/card-id). The columns being checked will all have :source/breakout, which caused, in the issue's repro example,
the "NAME" column to be missed. If it instead has :lib/source :source/card, the logic inside
`lib/find-matching-column` works.
---
src/metabase/query_processor/pivot.clj | 7 +++-
test/metabase/api/card_test.clj | 46 ++++++++++++++++++++++++++
2 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/src/metabase/query_processor/pivot.clj b/src/metabase/query_processor/pivot.clj
index fcc51208709..3e0d6bbafd8 100644
--- a/src/metabase/query_processor/pivot.clj
+++ b/src/metabase/query_processor/pivot.clj
@@ -359,7 +359,12 @@
mlv2-query (lib/query metadata-provider query)
breakouts (into []
(map-indexed (fn [i col]
- (assoc col ::i i)))
+ (cond-> col
+ true (assoc ::i i)
+ ;; if the col has a card-id, we swap the :lib/source to say source/card
+ ;; this allows `lib/find-matching-column` to properly match a column that has a join-alias
+ ;; but whose source is a model
+ (contains? col :lib/card-id) (assoc :lib/source :source/card))))
(lib/breakouts-metadata mlv2-query))]
(fn [legacy-ref]
(try
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 91e6c1bd0d6..21d043a8292 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -3847,3 +3847,49 @@
false
(apply original-can-read? args)))]
(is (map? (mt/user-http-request :crowberto :get 200 (format "card/%d/query_metadata" (:id card))))))))))
+
+(deftest pivot-tables-with-model-sources-show-row-totals
+ (testing "Pivot Tables with a model source will return row totals (#46575)"
+ (mt/with-temp [:model/Card {model-id :id} {:type :model
+ :dataset_query
+ {:database (mt/id)
+ :type :query
+ :query
+ {:source-table (mt/id :orders)
+ :joins
+ [{:fields :all
+ :strategy :left-join
+ :alias "People - User"
+ :condition
+ [:=
+ [:field (mt/id :orders :user_id) {:base-type :type/Integer}]
+ [:field (mt/id :people :id) {:base-type :type/BigInteger :join-alias "People - User"}]]
+ :source-table (mt/id :people)}]}}}
+ :model/Card {pivot-id :id} {:display :pivot
+ :dataset_query
+ {:database (mt/id)
+ :type :query
+ :query
+ {:aggregation [[:sum [:field "TOTAL" {:base-type :type/Float}]]]
+ :breakout
+ [[:field "CREATED_AT" {:base-type :type/DateTime, :temporal-unit :month}]
+ [:field "NAME" {:base-type :type/Text}]
+ [:field (mt/id :products :category) {:base-type :type/Text
+ :source-field (mt/id :orders :product_id)}]]
+ :source-table (format "card__%s" model-id)}}
+ :visualization_settings
+ {:pivot_table.column_split
+ {:rows
+ [[:field "NAME" {:base-type :type/Text}]
+ [:field "CREATED_AT" {:base-type :type/DateTime, :temporal-unit :month}]]
+ :columns [[:field (mt/id :products :category) {:base-type :type/Text
+ :source-field (mt/id :orders :product_id)}]]
+ :values [[:aggregation 0]]}}}]
+ ;; pivot row totals have a pivot-grouping of 1 (the second-last column in these results)
+ ;; before fixing issue #46575, these rows would not be returned given the model + card setup
+ (is (= [nil "Abbey Satterfield" "Doohickey" 1 347.91]
+ (let [result (mt/user-http-request :rasta :post 202 (format "card/pivot/%d/query" pivot-id))
+ totals (filter (fn [row]
+ (< 0 (second (reverse row))))
+ (get-in result [:data :rows]))]
+ (first totals)))))))
--
GitLab