diff --git a/enterprise/backend/test/metabase_enterprise/audit_app/api/collection_test.clj b/enterprise/backend/test/metabase_enterprise/audit_app/api/collection_test.clj
index 86f27664f2e976dbdaf2bacb5c197c418e96bb99..e6ad954d0a51ed319eb4b13162d219ed2d63650a 100644
--- a/enterprise/backend/test/metabase_enterprise/audit_app/api/collection_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/audit_app/api/collection_test.clj
@@ -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
diff --git a/src/metabase/api/collection.clj b/src/metabase/api/collection.clj
index 3e5e77dc09bd1d82773ad257fa05c775c6b88fbf..346161e50df18bafdd1f50fe6ba953c74b7a6b23 100644
--- a/src/metabase/api/collection.clj
+++ b/src/metabase/api/collection.clj
@@ -521,6 +521,7 @@
:description
:entity_id
:personal_owner_id
+ [:type :collection_type]
[(h2x/literal "collection") :model]
:authority_level])
;; the nil indicates that collections are never pinned.
@@ -556,15 +557,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]]
@@ -603,6 +608,7 @@
:model :collection_position :authority_level [:personal_owner_id :integer]
: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])
@@ -641,45 +647,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}]
diff --git a/src/metabase/models/collection.clj b/src/metabase/models/collection.clj
index aa6f3c3fed108ede6219aec06b1552f64249c317..741507e08de608fe06d3109a53d50b5d0daedc8e 100644
--- a/src/metabase/models/collection.clj
+++ b/src/metabase/models/collection.clj
@@ -1272,7 +1272,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
@@ -1305,8 +1309,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))))
diff --git a/test/metabase/api/collection_test.clj b/test/metabase/api/collection_test.clj
index 332ab257f704c0b0aa54aff02f5b902dc25d6428..bb391a8a2a02495e09194e0778e96bfea18027c3 100644
--- a/test/metabase/api/collection_test.clj
+++ b/test/metabase/api/collection_test.clj
@@ -921,37 +921,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)))))
diff --git a/test/metabase/models/collection_test.clj b/test/metabase/models/collection_test.clj
index 206015a39062130aa122eb5e63bcc23af7bc685d..8e94360122dba7ef8d41fc042603d357336b7897 100644
--- a/test/metabase/models/collection_test.clj
+++ b/test/metabase/models/collection_test.clj
@@ -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 "/"}