diff --git a/e2e/test/scenarios/admin/datamodel/datamodel.cy.spec.js b/e2e/test/scenarios/admin/datamodel/datamodel.cy.spec.js
index 9ee4d8f8f35c823aa8ba40bdb13619d52b245293..de985c4cb099700b8ce68692174fa9ccf2725415 100644
--- a/e2e/test/scenarios/admin/datamodel/datamodel.cy.spec.js
+++ b/e2e/test/scenarios/admin/datamodel/datamodel.cy.spec.js
@@ -67,8 +67,11 @@ describe("scenarios > admin > datamodel > field > field type", () => {
});
}
- function searchFieldType(type) {
- cy.findByPlaceholderText("Find...").type(type);
+ function searchFieldType(value) {
+ // .type() is flaky when used for ListSearchField - typed characters can
+ // sometimes get rearranged while typing.
+ // Unclear why. Possibly because it's rendered as a virtualized list item.
+ cy.findByPlaceholderText("Find...").invoke("val", value).trigger("blur");
}
function getFKTargetField(targetField) {
@@ -100,23 +103,19 @@ describe("scenarios > admin > datamodel > field > field type", () => {
cy.intercept("PUT", "/api/field/*").as("fieldUpdate");
});
- it(
- "should let you change the type to 'No semantic type'",
- { tags: "@flaky" },
- () => {
- visitAlias("@ORDERS_PRODUCT_ID_URL");
- cy.wait(["@metadata", "@metadata"]);
+ it("should let you change the type to 'No semantic type'", () => {
+ visitAlias("@ORDERS_PRODUCT_ID_URL");
+ cy.wait(["@metadata", "@metadata"]);
- setFieldType({ oldValue: "Foreign Key", newValue: "No semantic type" });
+ setFieldType({ oldValue: "Foreign Key", newValue: "No semantic type" });
- waitAndAssertOnResponse("fieldUpdate");
+ waitAndAssertOnResponse("fieldUpdate");
- cy.reload();
- cy.wait("@metadata");
+ cy.reload();
+ cy.wait("@metadata");
- getFieldType("No semantic type");
- },
- );
+ getFieldType("No semantic type");
+ });
it("should let you change the type to 'Foreign Key' and choose the target field", () => {
visitAlias("@ORDERS_QUANTITY_URL");
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/api.clj b/enterprise/backend/src/metabase_enterprise/serialization/api.clj
index d3ec36dd3fbe888035bb2589df36ae0637f1f0c5..97e71741ea23b10875f78830d5cabd4695520f60 100644
--- a/enterprise/backend/src/metabase_enterprise/serialization/api.clj
+++ b/enterprise/backend/src/metabase_enterprise/serialization/api.clj
@@ -125,13 +125,16 @@
{:additive *additive-logging*})]
(try ; try/catch inside logging to log errors
(log/infof "Serdes import, size %s" size)
- (let [cnt (u.compress/untgz file dst)
+ (let [cnt (try (u.compress/untgz file dst)
+ (catch Exception e
+ (throw (ex-info "Cannot unpack archive" {:status 422} e))))
path (find-serialization-dir dst)]
(when-not path
(throw (ex-info "No source dir detected. Please make sure the serialization files are in the top level dir."
- {:dst (.getPath dst)
- :count cnt
- :files (.listFiles dst)})))
+ {:status 400
+ :dst (.getPath dst)
+ :count cnt
+ :files (.listFiles dst)})))
(log/infof "In total %s entries unpacked, detected source dir: %s" cnt (.getName path))
(serdes/with-cache
(-> (v2.ingest/ingest-yaml (.getPath path))
@@ -142,6 +145,7 @@
(log/error e "Error during serialization")
(log/error (serdes/strip-error e "Error during serialization"))))))]
{:log-file log-file
+ :status (:status (ex-data @err))
:error-message (when @err
(serdes/strip-error @err nil))
:report report
@@ -253,6 +257,7 @@
(try
(let [start (System/nanoTime)
{:keys [log-file
+ status
error-message
report
callback]} (unpack&import (:tempfile file)
@@ -271,9 +276,13 @@
:error_count (count (:errors report))
:success (not error-message)
:error_message error-message})
- {:status 200
- :headers {"Content-Type" "text/plain"}
- :body (on-response! log-file callback)})
+ (if error-message
+ {:status (or status 500)
+ :headers {"Content-Type" "text/plain"}
+ :body (on-response! log-file callback)}
+ {:status 200
+ :headers {"Content-Type" "text/plain"}
+ :body (on-response! log-file callback)}))
(finally
(io/delete-file (:tempfile file)))))
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/api_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/api_test.clj
index 7c506aa00e858f26af238d1a6b27cd819709bf8c..cc306486f8e9be22300738cf5bc45c46f1a957c9 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/api_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/api_test.clj
@@ -217,7 +217,7 @@
(assoc :collection_id "DoesNotExist")))))))]
(testing "ERROR /api/ee/serialization/import"
(let [res (binding [api.serialization/*additive-logging* false]
- (mt/user-http-request :crowberto :post 200 "ee/serialization/import"
+ (mt/user-http-request :crowberto :post 500 "ee/serialization/import"
{:request-options {:headers {"content-type" "multipart/form-data"}}}
{:file ba}))
log (slurp (io/input-stream res))]
@@ -262,6 +262,13 @@
"models" "Collection,Dashboard"}
(-> (snowplow-test/pop-event-data-and-user-id!) last :data))))))))
+ (testing "Client error /api/ee/serialization/import"
+ (let [res (mt/user-http-request :crowberto :post 422 "ee/serialization/import"
+ {:request-options {:headers {"content-type" "multipart/form-data"}}}
+ {:file (.getBytes "not an archive" "UTF-8")})
+ log (slurp (io/input-stream res))]
+ (is (re-find #"Cannot unpack archive" log))))
+
(mt/with-dynamic-redefs [serdes/extract-one (extract-one-error (:entity_id card)
(mt/dynamic-value serdes/extract-one))]
(testing "ERROR /api/ee/serialization/export"
diff --git a/src/metabase/query_processor/pivot/postprocess.clj b/src/metabase/query_processor/pivot/postprocess.clj
index a70414848ed9fe041381cc4b3efacefd56743a56..f57c94dc2712141aad0251c6ee55dfc4c8c62593 100644
--- a/src/metabase/query_processor/pivot/postprocess.clj
+++ b/src/metabase/query_processor/pivot/postprocess.clj
@@ -7,6 +7,7 @@
(:require
[clojure.math.combinatorics :as math.combo]
[clojure.set :as set]
+ [clojure.string :as str]
[metabase.query-processor.streaming.common :as common]
[metabase.util.malli :as mu]
[metabase.util.malli.registry :as mr]))
@@ -103,6 +104,13 @@
[m k v]
(update m k conj v))
+(defn- default-agg-fn
+ [agg v]
+ (when v
+ (if (number? v)
+ (+ agg v)
+ v)))
+
(defn- update-aggregate
"Update the given `measure-aggregations` with `new-values` using the appropriate function in the `agg-fns` map.
@@ -116,7 +124,7 @@
(into {}
(map
(fn [[measure-key agg]]
- (let [agg-fn (get agg-fns measure-key +) ; default aggregation is just summation
+ (let [agg-fn (get agg-fns measure-key default-agg-fn)
new-v (get new-values measure-key)]
[measure-key (if new-v
(agg-fn agg new-v)
@@ -133,22 +141,27 @@
row-path (mapv row pivot-rows)
col-path (mapv row pivot-cols)
measure-vals (select-keys row pivot-measures)
- total-fn (fn [m path]
+ total-fn* (fn [m path]
(if (seq path)
(update-in m path
#(update-aggregate (or % (zipmap pivot-measures (repeat 0))) measure-vals measures))
- m))]
+ m))
+ total-fn (fn [m paths]
+ (reduce total-fn* m paths))]
(-> pivot
(update :row-count (fn [v] (if v (inc v) 0)))
(update :data update-in (concat row-path col-path)
#(update-aggregate (or % (zipmap pivot-measures (repeat 0))) measure-vals measures))
(update :totals (fn [totals]
(-> totals
- (total-fn [:grand-total])
- (total-fn row-path)
- (total-fn col-path)
- (total-fn [:section-totals (first row-path)])
- (total-fn (concat [:column-totals (first row-path)] col-path)))))
+ (total-fn [[:grand-total]])
+ (total-fn [row-path])
+ (total-fn [col-path])
+ (total-fn [[:section-totals (first row-path)]])
+ #_(total-fn [(concat [:column-totals (first row-path)] col-path)])
+ (total-fn (map (fn [part]
+ (concat [:column-totals] part col-path))
+ (rest (reductions conj [] row-path)))))))
(update :row-values #(reduce-kv update-set % (select-keys row pivot-rows)))
(update :column-values #(reduce-kv update-set % (select-keys row pivot-cols))))))
@@ -188,37 +201,43 @@
(defn- build-row
"Build a single row of the pivot table."
[row-combo col-combos pivot-measures data totals row-totals? ordered-formatters row-formatters]
- (let [row-path row-combo]
- (concat
- (when-not (seq row-formatters) (repeat (count pivot-measures) nil))
- (mapv fmt row-formatters row-combo)
- (concat
- (for [col-combo col-combos
- measure-key pivot-measures]
- (fmt (get ordered-formatters measure-key)
- (get-in data (concat row-path col-combo [measure-key]))))
- (when row-totals?
- (for [measure-key pivot-measures]
- (fmt (get ordered-formatters measure-key)
- (get-in totals (concat row-path [measure-key])))))))))
+ (let [row-path row-combo
+ measure-values (for [col-combo col-combos
+ measure-key pivot-measures]
+ (fmt (get ordered-formatters measure-key)
+ (get-in data (concat row-path col-combo [measure-key]))))]
+ (when (some #(and (some? %) (not= "" %)) measure-values)
+ (concat
+ (when-not (seq row-formatters) (repeat (count pivot-measures) nil))
+ row-combo
+ #_(mapv fmt row-formatters row-combo)
+ (concat
+ measure-values
+ (when row-totals?
+ (for [measure-key pivot-measures]
+ (fmt (get ordered-formatters measure-key)
+ (get-in totals (concat row-path [measure-key]))))))))))
(defn- build-column-totals
"Build column totals for a section."
- [section col-combos pivot-measures totals row-totals? ordered-formatters pivot-rows]
- (concat
- (cons (format "Totals for %s" (fmt (get ordered-formatters (first pivot-rows)) section))
- (repeat (dec (count pivot-rows)) nil))
- (for [col-combo col-combos
- measure-key pivot-measures]
- (fmt (get ordered-formatters measure-key)
- (get-in totals (concat
- [:column-totals section]
- col-combo
- [measure-key]))))
- (when row-totals?
- (for [measure-key pivot-measures]
- (fmt (get ordered-formatters measure-key)
- (get-in totals [:section-totals section measure-key]))))))
+ [section-path col-combos pivot-measures totals row-totals? ordered-formatters pivot-rows]
+ (let [totals-row (distinct (for [col-combo col-combos
+ measure-key pivot-measures]
+ (fmt (get ordered-formatters measure-key)
+ (get-in totals (concat
+ [:column-totals]
+ section-path
+ col-combo
+ [measure-key])))))]
+ (when (some #(and (some? %) (not= "" %)) totals-row)
+ (concat
+ (cons (format "Totals for %s" (fmt (get ordered-formatters (first pivot-rows)) (last section-path)))
+ (repeat (dec (count pivot-rows)) nil))
+ totals-row
+ (when row-totals?
+ (for [measure-key pivot-measures]
+ (fmt (get ordered-formatters measure-key)
+ (get-in totals (concat [:section-totals] section-path [measure-key])))))))))
(defn- build-grand-totals
"Build grand totals row."
@@ -236,29 +255,98 @@
(fmt (get ordered-formatters measure-key)
(get-in totals [:grand-total measure-key])))))
+(defn- append-totals-to-subsections
+ [pivot section col-combos ordered-formatters]
+ (let [{:keys [config
+ totals]} pivot
+ {:keys [pivot-rows
+ pivot-measures
+ row-totals?]} config]
+ (concat
+ (reduce
+ (fn [section pivot-row-idx]
+ (mapcat
+ (fn [[k rows]]
+ (let [partial-path (take pivot-row-idx (first rows))
+ subtotal-path (concat partial-path [k])
+ total-row (vec (build-column-totals
+ subtotal-path
+ col-combos
+ pivot-measures
+ totals
+ row-totals?
+ ordered-formatters pivot-rows))
+ ;; inside a subsection, we know that the 'parent' subsection values will all be the same
+ ;; so we can just grab it from the first row
+ next-subsection-value (nth (first rows) (dec pivot-row-idx))]
+ (vec (concat
+ rows
+ ;; assoc the next subsection's value into the row so it stays grouped in the next reduction
+ [(if (<= (dec pivot-row-idx) 0)
+ total-row
+ (assoc total-row (dec pivot-row-idx) next-subsection-value))]))))
+ (group-by (fn [r] (nth r pivot-row-idx)) section)))
+ section
+ (reverse (range 1 (dec (count pivot-rows)))))
+ [(vec (build-column-totals
+ [(ffirst section)]
+ col-combos
+ pivot-measures
+ totals
+ false #_row-totals?
+ ordered-formatters pivot-rows))])))
+
(defn build-pivot-output
"Arrange and format the aggregated `pivot` data."
[pivot ordered-formatters]
- (let [{:keys [config data totals row-values column-values]} pivot
- {:keys [pivot-rows pivot-cols pivot-measures column-titles row-totals? col-totals?]} config
- row-formatters (mapv #(get ordered-formatters %) pivot-rows)
- col-formatters (mapv #(get ordered-formatters %) pivot-cols)
- row-combos (apply math.combo/cartesian-product (map row-values pivot-rows))
- col-combos (apply math.combo/cartesian-product (map column-values pivot-cols))
- row-totals? (and row-totals? (boolean (seq pivot-cols)))
- column-headers (build-column-headers config col-combos col-formatters)
- headers (or (seq (build-headers column-headers config))
- [(concat
- (map #(get column-titles %) pivot-rows)
- (map #(get column-titles %) pivot-measures))])]
+ (let [{:keys [config
+ data
+ totals
+ row-values
+ column-values]} pivot
+ {:keys [pivot-rows
+ pivot-cols
+ pivot-measures
+ column-titles
+ row-totals?
+ col-totals?]} config
+ row-formatters (mapv #(get ordered-formatters %) pivot-rows)
+ col-formatters (mapv #(get ordered-formatters %) pivot-cols)
+ row-combos (apply math.combo/cartesian-product (map row-values pivot-rows))
+ col-combos (apply math.combo/cartesian-product (map column-values pivot-cols))
+ row-totals? (and row-totals? (boolean (seq pivot-cols)))
+ column-headers (build-column-headers config col-combos col-formatters)
+ headers (or (seq (build-headers column-headers config))
+ [(concat
+ (map #(get column-titles %) pivot-rows)
+ (map #(get column-titles %) pivot-measures))])]
(concat
headers
- (apply concat
- (for [section-row-combos (sort-by ffirst (vals (group-by first row-combos)))]
- (concat
- (for [row-combo (sort-by first section-row-combos)]
- (build-row row-combo col-combos pivot-measures data totals row-totals? ordered-formatters row-formatters))
- (when (and col-totals? (> (count pivot-rows) 1))
- [(build-column-totals (ffirst section-row-combos) col-combos pivot-measures totals row-totals? ordered-formatters pivot-rows)]))))
+ (filter seq
+ (apply concat
+ (let [sections-rows
+ (for [section-row-combos (sort-by ffirst (vals (group-by first row-combos)))]
+ (concat
+ (remove nil?
+ (for [row-combo (sort-by first section-row-combos)]
+ (build-row row-combo col-combos pivot-measures data totals row-totals? ordered-formatters row-formatters)))))]
+ (mapv
+ (fn [section-rows]
+ (->>
+ ;; section rows are either enriched with column-totals rows or left as is
+ (if col-totals?
+ (append-totals-to-subsections pivot section-rows col-combos ordered-formatters)
+ section-rows)
+ ;; then, we apply the row-formatters to the pivot-rows portion of each row,
+ ;; filtering out any rows that begin with "Totals ..."
+ (mapv
+ (fn [row]
+ (let [[row-part vals-part] (split-at (count pivot-rows) row)]
+ (if (or
+ (not (seq row-part))
+ (str/starts-with? (first row-part) "Totals"))
+ row
+ (vec (concat (map fmt row-formatters row-part) vals-part))))))))
+ sections-rows))))
(when col-totals?
[(build-grand-totals config col-combos totals row-totals? ordered-formatters)]))))
diff --git a/test/metabase/api/downloads_exports_test.clj b/test/metabase/api/downloads_exports_test.clj
index 2ab657537a7fd4f8509735ff4d791371d4ee8cb1..4ccd0dd7d412d251b1a4be90decda21b93df3f2a 100644
--- a/test/metabase/api/downloads_exports_test.clj
+++ b/test/metabase/api/downloads_exports_test.clj
@@ -279,9 +279,13 @@
(testing "formatted"
(is (= [[["Category" "2016" "2017" "2018" "2019" "Row totals"]
["Doohickey" "$632.14" "$854.19" "$496.43" "$203.13" "$2,185.89"]
+ ["Totals for Doohickey" "$632.14" "$854.19" "$496.43" "$203.13"]
["Gadget" "$679.83" "$1,059.11" "$844.51" "$435.75" "$3,019.20"]
+ ["Totals for Gadget" "$679.83" "$1,059.11" "$844.51" "$435.75"]
["Gizmo" "$529.70" "$1,080.18" "$997.94" "$227.06" "$2,834.88"]
+ ["Totals for Gizmo" "$529.70" "$1,080.18" "$997.94" "$227.06"]
["Widget" "$987.39" "$1,014.68" "$912.20" "$195.04" "$3,109.31"]
+ ["Totals for Widget" "$987.39" "$1,014.68" "$912.20" "$195.04"]
["Grand totals" "$2,829.06" "$4,008.16" "$3,251.08" "$1,060.98" "$11,149.28"]]
#{:unsaved-card-download :card-download :dashcard-download
:alert-attachment :subscription-attachment
@@ -298,9 +302,13 @@
"2019-01-01T00:00:00Z"
"Row totals"]
["Doohickey" "632.14" "854.19" "496.43" "203.13" "2185.89"]
+ ["Totals for Doohickey" "632.14" "854.19" "496.43" "203.13"]
["Gadget" "679.83" "1059.11" "844.51" "435.75" "3019.20"]
+ ["Totals for Gadget" "679.83" "1059.11" "844.51" "435.75"]
["Gizmo" "529.7" "1080.18" "997.94" "227.06" "2834.88"]
+ ["Totals for Gizmo" "529.7" "1080.18" "997.94" "227.06"]
["Widget" "987.39" "1014.68" "912.2" "195.04" "3109.31"]
+ ["Totals for Widget" "987.39" "1014.68" "912.2" "195.04"]
["Grand totals" "2829.06" "4008.16" "3251.08" "1060.98" "11149.28"]]
#{:unsaved-card-download :card-download :dashcard-download
:alert-attachment :subscription-attachment
@@ -512,12 +520,15 @@
:pivot_results true)
csv/read-csv)]
(is (= [["Created At" "Category" "Sum of Price"]
- ["April, 2016" "Doohickey" ""]
["April, 2016" "Gadget" "49.54"]
["April, 2016" "Gizmo" "87.29"]
- ["April, 2016" "Widget" ""]
- ["Totals for April, 2016" "" "136.83"]]
- (take 6 result))))))))
+ ["Totals for April, 2016" "" "136.83"]
+ ["May, 2016" "Doohickey" "144.12"]
+ ["May, 2016" "Gadget" "81.58"]
+ ["May, 2016" "Gizmo" "75.09"]
+ ["May, 2016" "Widget" "90.21"]
+ ["Totals for May, 2016" "" "391"]]
+ (take 9 result))))))))
(deftest ^:parallel zero-row-pivot-tables-test
(testing "Pivot tables with zero rows download correctly."
@@ -1154,3 +1165,43 @@
:dashcard-download expected-header
:public-dashcard-download expected-header}
(update-vals formatted-results first)))))))))
+
+(deftest pivot-non-numeric-values-in-aggregations
+ (testing "A pivot table with an aggegation that results in non-numeric values (eg. Dates) will still worl (#49353)."
+ (mt/dataset test-data
+ (mt/with-temp [:model/Card card {:display :pivot
+ :dataset_query {:database (mt/id)
+ :type :query
+ :query
+ {:source-table (mt/id :products)
+ :aggregation [[:count]
+ [:min
+ [:field (mt/id :products :created_at)
+ {:base-type :type/DateTime :temporal-unit :year}]]]
+ :breakout [[:field (mt/id :products :category) {:base-type :type/Text}]
+ [:field (mt/id :products :created_at)
+ {:base-type :type/DateTime :temporal-unit :year}]]}}
+ :visualization_settings {:pivot_table.column_split
+ {:rows [[:field (mt/id :products :created_at) {:base-type :type/DateTime :temporal-unit :year}]
+ [:field (mt/id :products :category) {:base-type :type/Text}]]
+ :columns []
+ :values [[:aggregation 0] [:aggregation 1]]}
+ :column_settings
+ {"[\"name\",\"count\"]" {:column_title "Count Renamed"}}}}]
+ (let [expected-header ["Created At" "Category" "Count" "Min of Created At: Year"]
+ formatted-results (all-downloads card {:export-format :csv :format-rows false :pivot true})]
+ (is (= {:unsaved-card-download expected-header
+ :card-download expected-header
+ :public-question-download expected-header
+ :dashcard-download expected-header
+ :public-dashcard-download expected-header}
+ (update-vals formatted-results first))))
+ (testing "The column title changes are used when format-rows is true"
+ (let [expected-header ["Created At" "Category" "Count Renamed" "Min of Created At: Year"]
+ formatted-results (all-downloads card {:export-format :csv :format-rows true :pivot true})]
+ (is (= {:unsaved-card-download expected-header
+ :card-download expected-header
+ :public-question-download expected-header
+ :dashcard-download expected-header
+ :public-dashcard-download expected-header}
+ (update-vals formatted-results first)))))))))