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

Don't sort official collections first in trash (#44322) 

Add a new query parameter to `/api/collection/:id/items` and
`/api/collection/root/items`. If `official_collections_first` is passed,
we'll sort official collections first, or not, as requested. If it isn't
passed, we'll default to `false` for the trash collection, or `true` for
normal collections.
parent 7f9e369c
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/collection.clj
+ 70
− 53
View file @ cb055ac9
...@@ -267,13 +267,14 @@ ...@@ -267,13 +267,14 @@
[:pinned-state {:optional true} [:maybe (into [:enum] (map keyword) valid-pinned-state-values)]] [:pinned-state {:optional true} [:maybe (into [:enum] (map keyword) valid-pinned-state-values)]]
;; when specified, only return results of this type. ;; when specified, only return results of this type.
[:models {:optional true} [:maybe [:set (into [:enum] (map keyword) valid-model-param-values)]]] [:models {:optional true} [:maybe [:set (into [:enum] (map keyword) valid-model-param-values)]]]
[:sort-info {:optional true} [:maybe [:tuple [:sort-info {:optional true} [:maybe [:map
(into [:enum {:error/message "sort-columns"}] [:sort-column (into [:enum {:error/message "sort-columns"}]
(map normalize-sort-choice) (map normalize-sort-choice)
valid-sort-columns) valid-sort-columns)]
(into [:enum {:error/message "sort-direction"}] [:sort-direction (into [:enum {:error/message "sort-direction"}]
(map normalize-sort-choice) (map normalize-sort-choice)
valid-sort-directions)]]]]) valid-sort-directions)]
[:official-collections-first? {:optional true} :boolean]]]]])
(defmulti ^:private collection-children-query (defmulti ^:private collection-children-query
"Query that will fetch the 'children' of a `collection`, for different types of objects. Possible options are listed "Query that will fetch the 'children' of a `collection`, for different types of objects. Possible options are listed
...@@ -815,19 +816,25 @@ ...@@ -815,19 +816,25 @@
(for [model [:card :metric :dataset :dashboard :snippet :pulse :collection :timeline]] (for [model [:card :metric :dataset :dashboard :snippet :pulse :collection :timeline]]
(:select (collection-children-query model {:id 1 :location "/"} nil))))) (:select (collection-children-query model {:id 1 :location "/"} nil)))))
(defn- official-collections-first-sort-clause [{:keys [official-collections-first?]}]
(when official-collections-first?
[[[:case [:= :authority_level "official"] 0 :else 1]] :asc]))
(def ^:private normal-collections-first-sort-clause
[[[:case
[:= :collection_type nil] 0
[:= :collection_type collection/trash-collection-type] 1
:else 2]]
:asc])
(defn children-sort-clause (defn children-sort-clause
"Given the client side sort-info, return sort clause to effect this. `db-type` is necessary due to complications from "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." treatment of nulls in the different app db types."
[sort-info db-type] [sort-info db-type]
;; always put "Metabase Analytics" last (->> (into [(official-collections-first-sort-clause sort-info)
(into [[[[:case [:= :authority_level "official"] 0 :else 1]] :asc] normal-collections-first-sort-clause]
[[[:case (case ((juxt :sort-column :sort-direction) sort-info)
[:= :collection_type nil] 0 [nil nil] [[:%lower.name :asc]]
[:= :collection_type collection/trash-collection-type] 1
:else 2]] :asc]]
(case sort-info
nil [[:%lower.name :asc]]
[:name :asc] [[:%lower.name :asc]] [:name :asc] [[:%lower.name :asc]]
[:name :desc] [[:%lower.name :desc]] [:name :desc] [[:%lower.name :desc]]
[:last-edited-at :asc] [(if (= db-type :mysql) [:last-edited-at :asc] [(if (= db-type :mysql)
...@@ -835,13 +842,12 @@ ...@@ -835,13 +842,12 @@
[:last_edit_timestamp :nulls-last]) [:last_edit_timestamp :nulls-last])
[:last_edit_timestamp :asc] [:last_edit_timestamp :asc]
[:%lower.name :asc]] [:%lower.name :asc]]
[:last-edited-at :desc] (remove nil? [:last-edited-at :desc] [(case db-type
[(case db-type :mysql [:%isnull.last_edit_timestamp]
:mysql [:%isnull.last_edit_timestamp] :postgres [:last_edit_timestamp :desc-nulls-last]
:postgres [:last_edit_timestamp :desc-nulls-last] :h2 nil)
:h2 nil) [:last_edit_timestamp :desc]
[:last_edit_timestamp :desc] [:%lower.name :asc]]
[:%lower.name :asc]])
[:last-edited-by :asc] [(if (= db-type :mysql) [:last-edited-by :asc] [(if (= db-type :mysql)
[:%isnull.last_edit_last_name] [:%isnull.last_edit_last_name]
[:last_edit_last_name :nulls-last]) [:last_edit_last_name :nulls-last])
...@@ -851,20 +857,21 @@ ...@@ -851,20 +857,21 @@
[:last_edit_first_name :nulls-last]) [:last_edit_first_name :nulls-last])
[:last_edit_first_name :asc] [:last_edit_first_name :asc]
[:%lower.name :asc]] [:%lower.name :asc]]
[:last-edited-by :desc] (remove nil? [:last-edited-by :desc] [(case db-type
[(case db-type :mysql [:%isnull.last_edit_last_name]
:mysql [:%isnull.last_edit_last_name] :postgres [:last_edit_last_name :desc-nulls-last]
:postgres [:last_edit_last_name :desc-nulls-last] :h2 nil)
:h2 nil) [:last_edit_last_name :desc]
[:last_edit_last_name :desc] (case db-type
(case db-type :mysql [:%isnull.last_edit_first_name]
:mysql [:%isnull.last_edit_first_name] :postgres [:last_edit_last_name :desc-nulls-last]
:postgres [:last_edit_last_name :desc-nulls-last] :h2 nil)
:h2 nil) [:last_edit_first_name :desc]
[:last_edit_first_name :desc] [:%lower.name :asc]]
[:%lower.name :asc]])
[:model :asc] [[:model_ranking :asc] [:%lower.name :asc]] [:model :asc] [[:model_ranking :asc] [:%lower.name :asc]]
[:model :desc] [[:model_ranking :desc] [:%lower.name :asc]]))) [:model :desc] [[:model_ranking :desc] [:%lower.name :asc]]))
(remove nil?)
(into [])))
(defn- collection-children* (defn- collection-children*
[collection models {:keys [sort-info] :as options}] [collection models {:keys [sort-info] :as options}]
...@@ -976,21 +983,27 @@ ...@@ -976,21 +983,27 @@
* `pinned_state` - when `is_pinned`, return pinned objects only. * `pinned_state` - when `is_pinned`, return pinned objects only.
when `is_not_pinned`, return non pinned objects only. when `is_not_pinned`, return non pinned objects only.
when `all`, return everything. By default returns everything" when `all`, return everything. By default returns everything"
[id models archived pinned_state sort_column sort_direction] [id models archived pinned_state sort_column sort_direction official_collections_first]
{id ms/PositiveInt {id ms/PositiveInt
models [:maybe Models] models [:maybe Models]
archived [:maybe ms/BooleanValue] archived [:maybe ms/BooleanValue]
pinned_state [:maybe (into [:enum] valid-pinned-state-values)] pinned_state [:maybe (into [:enum] valid-pinned-state-values)]
sort_column [:maybe (into [:enum] valid-sort-columns)] sort_column [:maybe (into [:enum] valid-sort-columns)]
sort_direction [:maybe (into [:enum] valid-sort-directions)]} sort_direction [:maybe (into [:enum] valid-sort-directions)]
official_collections_first [:maybe ms/MaybeBooleanValue]}
(let [model-kwds (set (map keyword (u/one-or-many models))) (let [model-kwds (set (map keyword (u/one-or-many models)))
collection (api/read-check Collection id)] collection (api/read-check Collection id)]
(u/prog1 (collection-children collection (u/prog1 (collection-children collection
{:models model-kwds {:models model-kwds
:archived? (or archived (:archived collection) (collection/is-trash? collection)) :archived? (or archived (:archived collection) (collection/is-trash? collection))
:pinned-state (keyword pinned_state) :pinned-state (keyword pinned_state)
:sort-info [(or (some-> sort_column normalize-sort-choice) :name) :sort-info {:sort-column (or (some-> sort_column normalize-sort-choice) :name)
(or (some-> sort_direction normalize-sort-choice) :asc)]}) :sort-direction (or (some-> sort_direction normalize-sort-choice) :asc)
;; default to sorting official collections first, except for the trash.
:official-collections-first? (if (and (nil? official_collections_first)
(not (collection/is-trash? collection)))
true
(boolean official_collections_first))}})
(events/publish-event! :event/collection-read {:object collection :user-id api/*current-user-id*})))) (events/publish-event! :event/collection-read {:object collection :user-id api/*current-user-id*}))))
...@@ -1033,13 +1046,14 @@ ...@@ -1033,13 +1046,14 @@
By default, this will show the 'normal' Collections namespace; to view a different Collections namespace, such as By default, this will show the 'normal' Collections namespace; to view a different Collections namespace, such as
`snippets`, you can pass the `?namespace=` parameter." `snippets`, you can pass the `?namespace=` parameter."
[models archived namespace pinned_state sort_column sort_direction] [models archived namespace pinned_state sort_column sort_direction official_collections_first]
{models [:maybe Models] {models [:maybe Models]
archived [:maybe ms/BooleanValue] archived [:maybe ms/BooleanValue]
namespace [:maybe ms/NonBlankString] namespace [:maybe ms/NonBlankString]
pinned_state [:maybe (into [:enum] valid-pinned-state-values)] pinned_state [:maybe (into [:enum] valid-pinned-state-values)]
sort_column [:maybe (into [:enum] valid-sort-columns)] sort_column [:maybe (into [:enum] valid-sort-columns)]
sort_direction [:maybe (into [:enum] valid-sort-directions)]} sort_direction [:maybe (into [:enum] valid-sort-directions)]
official_collections_first [:maybe ms/MaybeBooleanValue]}
;; Return collection contents, including Collections that have an effective location of being in the Root ;; Return collection contents, including Collections that have an effective location of being in the Root
;; Collection for the Current User. ;; Collection for the Current User.
(let [root-collection (assoc collection/root-collection :namespace namespace) (let [root-collection (assoc collection/root-collection :namespace namespace)
...@@ -1050,8 +1064,11 @@ ...@@ -1050,8 +1064,11 @@
{:archived? (boolean archived) {:archived? (boolean archived)
:models model-kwds :models model-kwds
:pinned-state (keyword pinned_state) :pinned-state (keyword pinned_state)
:sort-info [(or (some-> sort_column normalize-sort-choice) :name) :sort-info {:sort-column (or (some-> sort_column normalize-sort-choice) :name)
(or (some-> sort_direction normalize-sort-choice) :asc)]}))) :sort-direction (or (some-> sort_direction normalize-sort-choice) :asc)
;; default to sorting official collections first, but provide the option not to
:official-collections-first? (or (nil? official_collections_first)
(boolean official_collections_first))}})))
;;; ----------------------------------------- Creating/Editing a Collection ------------------------------------------ ;;; ----------------------------------------- Creating/Editing a Collection ------------------------------------------
......
test/metabase/api/collection_test.clj
+ 19
− 7
View file @ cb055ac9
...@@ -1111,7 +1111,7 @@ ...@@ -1111,7 +1111,7 @@
[:= :collection_type collection/trash-collection-type] 1 [:= :collection_type collection/trash-collection-type] 1
:else 2]] :asc] :else 2]] :asc]
[:%lower.name :asc]] [:%lower.name :asc]]
(api.collection/children-sort-clause nil app-db))))) (api.collection/children-sort-clause {:official-collections-first? true} app-db)))))
(testing "Sorting by last-edited-at" (testing "Sorting by last-edited-at"
(is (= [[[[:case [:= :authority_level "official"] 0 :else 1]] :asc] (is (= [[[[:case [:= :authority_level "official"] 0 :else 1]] :asc]
[[[:case [[[:case
...@@ -1121,7 +1121,9 @@ ...@@ -1121,7 +1121,9 @@
[:%isnull.last_edit_timestamp] [:%isnull.last_edit_timestamp]
[:last_edit_timestamp :asc] [:last_edit_timestamp :asc]
[:%lower.name :asc]] [:%lower.name :asc]]
(api.collection/children-sort-clause [:last-edited-at :asc] :mysql))) (api.collection/children-sort-clause {:sort-column :last-edited-at
:sort-direction :asc
:official-collections-first? true} :mysql)))
(is (= [[[[:case [:= :authority_level "official"] 0 :else 1]] :asc] (is (= [[[[:case [:= :authority_level "official"] 0 :else 1]] :asc]
[[[:case [[[:case
[:= :collection_type nil] 0 [:= :collection_type nil] 0
...@@ -1130,7 +1132,9 @@ ...@@ -1130,7 +1132,9 @@
[:last_edit_timestamp :nulls-last] [:last_edit_timestamp :nulls-last]
[:last_edit_timestamp :asc] [:last_edit_timestamp :asc]
[:%lower.name :asc]] [:%lower.name :asc]]
(api.collection/children-sort-clause [:last-edited-at :asc] :postgres)))) (api.collection/children-sort-clause {:sort-column :last-edited-at
:sort-direction :asc
:official-collections-first? true} :postgres))))
(testing "Sorting by last-edited-by" (testing "Sorting by last-edited-by"
(is (= [[[[:case [:= :authority_level "official"] 0 :else 1]] :asc] (is (= [[[[:case [:= :authority_level "official"] 0 :else 1]] :asc]
[[[:case [[[:case
...@@ -1142,7 +1146,9 @@ ...@@ -1142,7 +1146,9 @@
[:last_edit_first_name :nulls-last] [:last_edit_first_name :nulls-last]
[:last_edit_first_name :asc] [:last_edit_first_name :asc]
[:%lower.name :asc]] [:%lower.name :asc]]
(api.collection/children-sort-clause [:last-edited-by :asc] :postgres))) (api.collection/children-sort-clause {:sort-column :last-edited-by
:sort-direction :asc
:official-collections-first? true} :postgres)))
(is (= [[[[:case [:= :authority_level "official"] 0 :else 1]] :asc] (is (= [[[[:case [:= :authority_level "official"] 0 :else 1]] :asc]
[[[:case [[[:case
[:= :collection_type nil] 0 [:= :collection_type nil] 0
...@@ -1153,7 +1159,9 @@ ...@@ -1153,7 +1159,9 @@
[:%isnull.last_edit_first_name] [:%isnull.last_edit_first_name]
[:last_edit_first_name :asc] [:last_edit_first_name :asc]
[:%lower.name :asc]] [:%lower.name :asc]]
(api.collection/children-sort-clause [:last-edited-by :asc] :mysql)))) (api.collection/children-sort-clause {:sort-column :last-edited-by
:sort-direction :asc
:official-collections-first? true} :mysql))))
(testing "Sorting by model" (testing "Sorting by model"
(is (= [[[[:case [:= :authority_level "official"] 0 :else 1]] :asc] (is (= [[[[:case [:= :authority_level "official"] 0 :else 1]] :asc]
[[[:case [[[:case
...@@ -1162,7 +1170,9 @@ ...@@ -1162,7 +1170,9 @@
:else 2]] :asc] :else 2]] :asc]
[:model_ranking :asc] [:model_ranking :asc]
[:%lower.name :asc]] [:%lower.name :asc]]
(api.collection/children-sort-clause [:model :asc] :postgres))) (api.collection/children-sort-clause {:sort-column :model
:sort-direction :asc
:official-collections-first? true} :postgres)))
(is (= [[[[:case [:= :authority_level "official"] 0 :else 1]] :asc] (is (= [[[[:case [:= :authority_level "official"] 0 :else 1]] :asc]
[[[:case [[[:case
[:= :collection_type nil] 0 [:= :collection_type nil] 0
...@@ -1170,7 +1180,9 @@ ...@@ -1170,7 +1180,9 @@
:else 2]] :asc] :else 2]] :asc]
[:model_ranking :desc] [:model_ranking :desc]
[:%lower.name :asc]] [:%lower.name :asc]]
(api.collection/children-sort-clause [:model :desc] :mysql))))) (api.collection/children-sort-clause {:sort-column :model
:sort-direction :desc
:official-collections-first? true} :mysql)))))
(deftest snippet-collection-items-test (deftest snippet-collection-items-test
(testing "GET /api/collection/:id/items" (testing "GET /api/collection/:id/items"
......
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