Skip to content
Snippets Groups Projects
Unverified Commit 918878eb authored by John Swanson's avatar John Swanson Committed by GitHub
Browse files

return collections in same order from /api/collections/tree and /api/collections/:id/items (#40044) 

There were two problems here:

- one endpoint was sorting in the database, but not putting 'analytics'
last.

- one endpoint was sorting in the database, but then sorting again in
clojure, which had subtly different behavior for special characters

Fixes #39965
parent 341e4b4c
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
enterprise/backend/test/metabase_enterprise/audit_app/api/collection_test.clj
+ 5
− 1
View file @ 918878eb
......@@ -48,7 +48,11 @@
[]
(if-not config/ee-available?
#{}
(let [colls (mapv #(select-keys % [:id :name :location :type]) (t2/select Collection :archived false))
(let [colls (->> (t2/select Collection :archived false)
(sort-by (fn [{coll-type :type coll-name :name coll-id :id}]
[coll-type ((fnil u/lower-case-en "") coll-name) coll-id]))
(mapv #(select-keys % [:id :name :location :type])))
id->coll (m/index-by :id colls)
collection-tree (collection/collections->tree {} colls)]
(->> (loop [[tree & coll-tree] collection-tree
......
src/metabase/api/collection.clj
+ 48
− 40
View file @ 918878eb
......@@ -532,6 +532,7 @@
:entity_id
:personal_owner_id
:location
[:type :collection_type]
[(h2x/literal "collection") :model]
:authority_level])
;; the nil indicates that collections are never pinned.
......@@ -576,15 +577,19 @@
:last_edit_first_name :first_name
:last_edit_email :email
:last_edit_timestamp :timestamp}]
(cond-> (apply dissoc row :model_ranking (keys mapping))
(cond-> (apply dissoc row (keys mapping))
;; don't use contains as they all have the key, we care about a value present
(:last_edit_user row) (assoc :last-edit-info (select-as row mapping))))))
(defn- remove-unwanted-keys [row]
(dissoc row :collection_type :model_ranking))
(defn- post-process-rows
"Post process any data. Have a chance to process all of the same type at once using
`post-process-collection-children`. Must respect the order passed in."
[rows]
(->> (map-indexed (fn [i row] (vary-meta row assoc ::index i)) rows) ;; keep db sort order
(map remove-unwanted-keys)
(group-by :model)
(into []
(comp (map (fn [[model rows]]
......@@ -623,6 +628,7 @@
:model :collection_position :authority_level [:personal_owner_id :integer] :location
:last_edit_email :last_edit_first_name :last_edit_last_name :moderated_status :icon
[:last_edit_user :integer] [:last_edit_timestamp :timestamp] [:database_id :integer]
:collection_type
;; for determining whether a model is based on a csv-uploaded table
[:table_id :integer] [:is_upload :boolean] :query_type])
......@@ -661,45 +667,47 @@
"Given the client side sort-info, return sort clause to effect this. `db-type` is necessary due to complications from
treatment of nulls in the different app db types."
[sort-info db-type]
(case sort-info
nil [[:%lower.name :asc]]
[:name :asc] [[:%lower.name :asc]]
[:name :desc] [[:%lower.name :desc]]
[:last-edited-at :asc] [(if (= db-type :mysql)
[:%isnull.last_edit_timestamp]
[:last_edit_timestamp :nulls-last])
[:last_edit_timestamp :asc]
[:%lower.name :asc]]
[:last-edited-at :desc] (remove nil?
[(case db-type
:mysql [:%isnull.last_edit_timestamp]
:postgres [:last_edit_timestamp :desc-nulls-last]
:h2 nil)
[:last_edit_timestamp :desc]
[:%lower.name :asc]])
[:last-edited-by :asc] [(if (= db-type :mysql)
[:%isnull.last_edit_last_name]
[:last_edit_last_name :nulls-last])
[:last_edit_last_name :asc]
(if (= db-type :mysql)
[:%isnull.last_edit_first_name]
[:last_edit_first_name :nulls-last])
[:last_edit_first_name :asc]
[:%lower.name :asc]]
[:last-edited-by :desc] (remove nil?
[(case db-type
:mysql [:%isnull.last_edit_last_name]
:postgres [:last_edit_last_name :desc-nulls-last]
:h2 nil)
[:last_edit_last_name :desc]
(case db-type
:mysql [:%isnull.last_edit_first_name]
:postgres [:last_edit_last_name :desc-nulls-last]
:h2 nil)
[:last_edit_first_name :desc]
[:%lower.name :asc]])
[:model :asc] [[:model_ranking :asc] [:%lower.name :asc]]
[:model :desc] [[:model_ranking :desc] [:%lower.name :asc]]))
;; always put "Metabase Analytics" last
(into [[[[:case [:= :collection_type nil] 0 :else 1]] :asc]]
(case sort-info
nil [[:%lower.name :asc]]
[:name :asc] [[:%lower.name :asc]]
[:name :desc] [[:%lower.name :desc]]
[:last-edited-at :asc] [(if (= db-type :mysql)
[:%isnull.last_edit_timestamp]
[:last_edit_timestamp :nulls-last])
[:last_edit_timestamp :asc]
[:%lower.name :asc]]
[:last-edited-at :desc] (remove nil?
[(case db-type
:mysql [:%isnull.last_edit_timestamp]
:postgres [:last_edit_timestamp :desc-nulls-last]
:h2 nil)
[:last_edit_timestamp :desc]
[:%lower.name :asc]])
[:last-edited-by :asc] [(if (= db-type :mysql)
[:%isnull.last_edit_last_name]
[:last_edit_last_name :nulls-last])
[:last_edit_last_name :asc]
(if (= db-type :mysql)
[:%isnull.last_edit_first_name]
[:last_edit_first_name :nulls-last])
[:last_edit_first_name :asc]
[:%lower.name :asc]]
[:last-edited-by :desc] (remove nil?
[(case db-type
:mysql [:%isnull.last_edit_last_name]
:postgres [:last_edit_last_name :desc-nulls-last]
:h2 nil)
[:last_edit_last_name :desc]
(case db-type
:mysql [:%isnull.last_edit_first_name]
:postgres [:last_edit_last_name :desc-nulls-last]
:h2 nil)
[:last_edit_first_name :desc]
[:%lower.name :asc]])
[:model :asc] [[:model_ranking :asc] [:%lower.name :asc]]
[:model :desc] [[:model_ranking :desc] [:%lower.name :asc]])))
(defn- collection-children*
[collection models {:keys [sort-info] :as options}]
......
src/metabase/models/collection.clj
+ 7
− 3
View file @ 918878eb
......@@ -1265,7 +1265,11 @@
:children [{:name \"G\"}]}]}]}
{:name \"H\"}]"
[coll-type-ids collections]
(let [all-visible-ids (set (map :id collections))]
(let [;; instead of attempting to re-sort like the database does, keep things consistent by just keeping things in
;; the same order they're already in.
original-position (into {} (map-indexed (fn [i {id :id}]
[id i]) collections))
all-visible-ids (set (map :id collections))]
(transduce
identity
(fn ->tree
......@@ -1298,8 +1302,8 @@
([m]
(->> (vals m)
(map #(update % :children ->tree))
(sort-by (fn [{coll-type :type, coll-name :name, coll-id :id}]
(sort-by (fn [{coll-id :id}]
;; coll-type is `nil` or "instance-analytics"
;; nil sorts first, so we get instance-analytics at the end, which is what we want
[coll-type ((fnil u/lower-case-en "") coll-name) coll-id])))))
(original-position coll-id))))))
(annotate-collections coll-type-ids collections))))
test/metabase/api/collection_test.clj
+ 15
− 7
View file @ 918878eb
......@@ -967,37 +967,45 @@
(into #{} (map :name) items))))))))
(deftest children-sort-clause-test
;; we always place "special" collection types (i.e. "Metabase Analytics") last
(testing "Default sort"
(doseq [app-db [:mysql :h2 :postgres]]
(is (= [[:%lower.name :asc]]
(is (= [[[[:case [:= :collection_type nil] 0 :else 1]] :asc]
[:%lower.name :asc]]
(api.collection/children-sort-clause nil app-db)))))
(testing "Sorting by last-edited-at"
(is (= [[:%isnull.last_edit_timestamp]
(is (= [[[[:case [:= :collection_type nil] 0 :else 1]] :asc]
[:%isnull.last_edit_timestamp]
[:last_edit_timestamp :asc]
[:%lower.name :asc]]
(api.collection/children-sort-clause [:last-edited-at :asc] :mysql)))
(is (= [[:last_edit_timestamp :nulls-last]
(is (= [[[[:case [:= :collection_type nil] 0 :else 1]] :asc]
[:last_edit_timestamp :nulls-last]
[:last_edit_timestamp :asc]
[:%lower.name :asc]]
(api.collection/children-sort-clause [:last-edited-at :asc] :postgres))))
(testing "Sorting by last-edited-by"
(is (= [[:last_edit_last_name :nulls-last]
(is (= [[[[:case [:= :collection_type nil] 0 :else 1]] :asc]
[:last_edit_last_name :nulls-last]
[:last_edit_last_name :asc]
[:last_edit_first_name :nulls-last]
[:last_edit_first_name :asc]
[:%lower.name :asc]]
(api.collection/children-sort-clause [:last-edited-by :asc] :postgres)))
(is (= [[:%isnull.last_edit_last_name]
(is (= [[[[:case [:= :collection_type nil] 0 :else 1]] :asc]
[:%isnull.last_edit_last_name]
[:last_edit_last_name :asc]
[:%isnull.last_edit_first_name]
[:last_edit_first_name :asc]
[:%lower.name :asc]]
(api.collection/children-sort-clause [:last-edited-by :asc] :mysql))))
(testing "Sortinb by model"
(is (= [[:model_ranking :asc]
(is (= [[[[:case [:= :collection_type nil] 0 :else 1]] :asc]
[:model_ranking :asc]
[:%lower.name :asc]]
(api.collection/children-sort-clause [:model :asc] :postgres)))
(is (= [[:model_ranking :desc]
(is (= [[[[:case [:= :collection_type nil] 0 :else 1]] :asc]
[:model_ranking :desc]
[:%lower.name :asc]]
(api.collection/children-sort-clause [:model :desc] :mysql)))))
......
test/metabase/models/collection_test.clj
+ 36
− 24
View file @ 918878eb
......@@ -1546,8 +1546,8 @@
:location "/1/3/"
:here #{:card}
:children [{:name "G", :id 7, :location "/1/3/6/", :children []}]}]}]}
{:name "aaa", :id 9, :location "/", :children [] :here #{:card}}
{:name "H", :id 8, :location "/", :children []}]
{:name "H", :id 8, :location "/", :children []}
{:name "aaa", :id 9, :location "/", :children [] :here #{:card}}]
(collection/collections->tree
{:dataset #{4 5} :card #{6 9}}
[{:name "A", :id 1, :location "/"}
......@@ -1596,28 +1596,40 @@
{:id 5, :name "a", :location "/3/1/2/"}
{:id 6, :name "a", :location "/3/"}])]
(testing (format "Permutation: %s" (pr-str (map :id collections)))
(is (= [{:id 3
:name "a"
:location "/"
:children [{:id 1
:name "a"
:location "/3/"
:children [{:id 2
:name "a"
:location "/3/1/"
:children [{:id 5
:name "a"
:location "/3/1/2/"
:children []}]}
{:id 4
:name "a"
:location "/3/1/"
:children []}]}
{:id 6
:name "a"
:location "/3/"
:children []}]}]
(collection/collections->tree {} collections)))))))
(let [id->idx (into {} (map-indexed
(fn [i c]
[(:id c) i])
collections))
correctly-order (fn [colls]
(sort-by (comp id->idx :id) colls))]
(testing "sanity check: correctly-order puts collections into the order they were passed in"
(is (= collections (correctly-order collections))))
(testing "A correct tree is generated, with children ordered as they were passed in"
(is (= [{:id 3
:name "a"
:location "/"
:children (correctly-order
[{:id 1
:name "a"
:location "/3/"
:children (correctly-order
[{:id 2
:name "a"
:location "/3/1/"
:children (correctly-order
[{:id 5
:name "a"
:location "/3/1/2/"
:children []}])}
{:id 4
:name "a"
:location "/3/1/"
:children []}])}
{:id 6
:name "a"
:location "/3/"
:children []}])}]
(collection/collections->tree {} collections)))))))))
(deftest ^:parallel annotate-collections-test
(let [collections [{:id 1, :name "a", :location "/"}
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment