From 5156d881f6dbec14b3e817f1111f47f013c8628f Mon Sep 17 00:00:00 2001
From: Howon Lee <hlee.howon@gmail.com>
Date: Fri, 18 Jun 2021 11:10:39 -0700
Subject: [PATCH] Fix issue with the collections appearing in the list page
(#16562)
I thought the collections appearing in the list page problem was solved, but this turned out to be a bug with the ttest. Upon actual inspection the lists remained.
To fix this without changing all sorts of crap that depended upon collections api contract (we futzed with the output but the futzing of the output is transparent to the frontend because of the entity thing they got going on), needed a way to elicit null response from collections endpoint. shoved one in. then renamed it because the first name, "dummy", makes it all sound kind of jank, so 'no-model' for explicitly asking for no model without handing in an empty set, which turns into the set of all models.
This is an 80% unjankening. Full unjankening would entail whacking this special case.
---
.../collections/collections.cy.spec.js | 2 +-
src/metabase/api/collection.clj | 30 +++++++++++++++----
test/metabase/api/collection_test.clj | 3 ++
3 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/frontend/test/metabase/scenarios/collections/collections.cy.spec.js b/frontend/test/metabase/scenarios/collections/collections.cy.spec.js
index fd630839966..acd1e962bec 100644
--- a/frontend/test/metabase/scenarios/collections/collections.cy.spec.js
+++ b/frontend/test/metabase/scenarios/collections/collections.cy.spec.js
@@ -277,7 +277,7 @@ describe("scenarios > collection_defaults", () => {
cy.signIn("nocollection");
});
- it.skip("should not render collections in items list if user doesn't have collection access (metabase#16555)", () => {
+ it("should not render collections in items list if user doesn't have collection access (metabase#16555)", () => {
cy.visit("/collection/root");
// Since this user doesn't have access rights to the root collection, it should render empty
cy.findByText("Nothing to see yet.");
diff --git a/src/metabase/api/collection.clj b/src/metabase/api/collection.clj
index 22fb2bac69f..6630f8af91c 100644
--- a/src/metabase/api/collection.clj
+++ b/src/metabase/api/collection.clj
@@ -85,8 +85,9 @@
;;; --------------------------------- Fetching a single Collection & its 'children' ----------------------------------
(def ^:private valid-model-param-values
- "Valid values for the `?model=` param accepted by endpoints in this namespace."
- #{"card" "collection" "dashboard" "pulse" "snippet"})
+ "Valid values for the `?model=` param accepted by endpoints in this namespace.
+ `no_models` is for nilling out the set because a nil model set is actually the total model set"
+ #{"card" "collection" "dashboard" "pulse" "snippet" "no_models"})
(def ^:private ModelString
(apply s/enum valid-model-param-values))
@@ -426,7 +427,13 @@
:when (or (= model-kw :collection)
(contains? allowed-namespaces (keyword collection-namespace)))]
model-kw)]
- (collection-children* collection valid-models (assoc options :collection-namespace collection-namespace))))
+ (if (seq valid-models)
+ (collection-children* collection valid-models (assoc options :collection-namespace collection-namespace))
+ {:total 0
+ :data []
+ :limit offset-paging/*limit*
+ :offset offset-paging/*offset*
+ :models valid-models})))
(s/defn ^:private collection-detail
"Add a standard set of details to `collection`, including things like `effective_location`.
@@ -474,6 +481,18 @@
{namespace (s/maybe su/NonBlankString)}
(dissoc (root-collection namespace) ::collection.root/is-root?))
+(defn- visible-model-kwds
+ "If you pass in explicitly keywords that you can't see, you can't see them.
+ But there is an exception for the collections,
+ because you might not be able to see the top-level collections
+ but be able to see, children of those invisible top-level collections."
+ [root-collection model-set]
+ (if (mi/can-read? root-collection)
+ model-set
+ (if (or (empty? model-set) (contains? model-set :collection))
+ #{:collection}
+ #{:no_models})))
+
(api/defendpoint GET "/root/items"
"Fetch objects that the current user should see at their root level. As mentioned elsewhere, the 'Root' Collection
doesn't actually exist as a row in the application DB: it's simply a virtual Collection where things with no
@@ -498,9 +517,8 @@
;; Return collection contents, including Collections that have an effective location of being in the Root
;; Collection for the Current User.
(let [root-collection (assoc collection/root-collection :namespace namespace)
- model-kwds (if (mi/can-read? root-collection)
- (set (map keyword (u/one-or-many models)))
- #{:collection})]
+ model-set (set (map keyword (u/one-or-many models)))
+ model-kwds (visible-model-kwds root-collection model-set)]
(collection-children
root-collection
{:models model-kwds
diff --git a/test/metabase/api/collection_test.clj b/test/metabase/api/collection_test.clj
index e640ce3fce0..192405abeaa 100644
--- a/test/metabase/api/collection_test.clj
+++ b/test/metabase/api/collection_test.clj
@@ -363,6 +363,9 @@
(mt/with-temp Collection [collection {:name "Art Collection"}]
(perms/grant-collection-read-permissions! (group/all-users) collection)
(with-some-children-of-collection collection
+ (is (= ()
+ (mt/boolean-ids-and-timestamps
+ (:data (mt/user-http-request :rasta :get 200 (str "collection/" (u/the-id collection) "/items?models=no_models"))))))
(is (= [(default-item {:name "Dine & Dashboard", :description nil, :favorite false, :model "dashboard"})]
(mt/boolean-ids-and-timestamps
(:data (mt/user-http-request :rasta :get 200 (str "collection/" (u/the-id collection) "/items?models=dashboard"))))))
--
GitLab