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.

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.");

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

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"))))))