From 02eb6982d2c7bb1f29bffec9e51654232e5d6e75 Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Tue, 23 Aug 2022 22:13:10 +0300
Subject: [PATCH] Hydrate app_id for collections with apps attached (#24950)
* Hydrate app_id for collections with apps attached
Addresses #24941, part of #24861
---
src/metabase/api/collection.clj | 27 +++---
src/metabase/models/app.clj | 20 +++++
test/metabase/api/collection_test.clj | 121 +++++++++++++++-----------
3 files changed, 102 insertions(+), 66 deletions(-)
diff --git a/src/metabase/api/collection.clj b/src/metabase/api/collection.clj
index cd169f17282..7b84d1cf0c5 100644
--- a/src/metabase/api/collection.clj
+++ b/src/metabase/api/collection.clj
@@ -65,7 +65,7 @@
(cond->> collections
(mi/can-read? root)
(cons root))))
- (hydrate collections :can_write)
+ (hydrate collections :can_write :app_id)
;; remove the :metabase.models.collection.root/is-root? tag since FE doesn't need it
;; and for personal collections we translate the name to user's locale
(for [collection collections]
@@ -104,17 +104,18 @@
(db/reducible-query {:select [:collection_id :dataset]
:modifiers [:distinct]
:from [:report_card]
- :where [:= :archived false]}))]
- (->> (db/select Collection
- {:where [:and
- (when exclude-archived
- [:= :archived false])
- [:= :namespace namespace]
- (collection/visible-collection-ids->honeysql-filter-clause
- :id
- (collection/permissions-set->visible-collection-ids @api/*current-user-permissions-set*))]})
- (map collection/personal-collection-with-ui-details)
- (collection/collections->tree coll-type-ids))))
+ :where [:= :archived false]}))
+ colls (db/select Collection
+ {:where [:and
+ (when exclude-archived
+ [:= :archived false])
+ [:= :namespace namespace]
+ (collection/visible-collection-ids->honeysql-filter-clause
+ :id
+ (collection/permissions-set->visible-collection-ids @api/*current-user-permissions-set*))]})
+ colls (map collection/personal-collection-with-ui-details colls)
+ colls (hydrate colls :app_id)]
+ (collection/collections->tree coll-type-ids colls)))
;;; --------------------------------- Fetching a single Collection & its 'children' ----------------------------------
@@ -604,7 +605,7 @@
[collection :- collection/CollectionWithLocationAndIDOrRoot]
(-> collection
collection/personal-collection-with-ui-details
- (hydrate :parent_id :effective_location [:effective_ancestors :can_write] :can_write)))
+ (hydrate :parent_id :effective_location [:effective_ancestors :can_write] :can_write :app_id)))
(api/defendpoint GET "/:id"
"Fetch a specific Collection with standard details added"
diff --git a/src/metabase/models/app.clj b/src/metabase/models/app.clj
index 6bdd4d01979..68239422951 100644
--- a/src/metabase/models/app.clj
+++ b/src/metabase/models/app.clj
@@ -3,6 +3,7 @@
[metabase.models.permissions :as perms]
[metabase.models.serialization.hash :as serdes.hash]
[metabase.util :as u]
+ [toucan.db :as db]
[toucan.models :as models]))
(models/defmodel App :app)
@@ -23,3 +24,22 @@
;; necessary to satisfy metabase-enterprise.models.entity-id-test/comprehensive-identity-hash-test.
serdes.hash/IdentityHashable
{:identity-hash-fields (constantly [:entity_id])})
+
+(defn add-app-id
+ "Add `app_id` to Collections that are linked with an App."
+ {:batched-hydrate :app_id}
+ [collections]
+ (if-let [coll-ids (seq (into #{}
+ (comp (map :id)
+ ;; The ID "root" breaks the query.
+ (filter int?))
+ collections))]
+ (let [coll-id->app-id (into {}
+ (map (juxt :collection_id :id))
+ (db/select [App :collection_id :id]
+ :collection_id [:in coll-ids]))]
+ (for [coll collections]
+ (let [app-id (coll-id->app-id (:id coll))]
+ (cond-> coll
+ app-id (assoc :app_id app-id)))))
+ collections))
diff --git a/test/metabase/api/collection_test.clj b/test/metabase/api/collection_test.clj
index 26f6432b56e..9eb357391bd 100644
--- a/test/metabase/api/collection_test.clj
+++ b/test/metabase/api/collection_test.clj
@@ -4,7 +4,7 @@
[clojure.test :refer :all]
[honeysql.core :as hsql]
[metabase.api.collection :as api.collection]
- [metabase.models :refer [Card Collection Dashboard DashboardCard ModerationReview NativeQuerySnippet
+ [metabase.models :refer [App Card Collection Dashboard DashboardCard ModerationReview NativeQuerySnippet
PermissionsGroup PermissionsGroupMembership Pulse PulseCard PulseChannel
PulseChannelRecipient Revision Timeline TimelineEvent User]]
[metabase.models.collection :as collection]
@@ -110,26 +110,28 @@
(str/includes? collection-name "Personal Collection"))))
(map :name)))))))
- (mt/with-temp* [Collection [_ {:name "Archived Collection", :archived true}]
- Collection [_ {:name "Regular Collection"}]]
+ (mt/with-temp* [Collection [{coll1-id :id} {:name "Archived Collection", :archived true}]
+ Collection [{coll2-id :id} {:name "Regular Collection"}]
+ App [{app1-id :id} {:collection_id coll1-id}]
+ App [{app2-id :id} {:collection_id coll2-id}]]
(letfn [(remove-other-collections [collections]
(filter (fn [{collection-name :name}]
(or (#{"Our analytics" "Archived Collection" "Regular Collection"} collection-name)
(str/includes? collection-name "Personal Collection")))
collections))]
(testing "check that we don't see collections if they're archived"
- (is (= ["Our analytics"
- "Rasta Toucan's Personal Collection"
- "Regular Collection"]
+ (is (= [["Our analytics" nil]
+ ["Rasta Toucan's Personal Collection" nil]
+ ["Regular Collection" app2-id]]
(->> (mt/user-http-request :rasta :get 200 "collection")
remove-other-collections
- (map :name)))))
+ (map (juxt :name :app_id))))))
(testing "Check that if we pass `?archived=true` we instead see archived Collections"
- (is (= ["Archived Collection"]
+ (is (= [["Archived Collection" app1-id]]
(->> (mt/user-http-request :rasta :get 200 "collection" :archived :true)
remove-other-collections
- (map :name)))))))
+ (map (juxt :name :app_id))))))))
(testing "?namespace= parameter"
(mt/with-temp* [Collection [{normal-id :id} {:name "Normal Collection"}]
@@ -163,14 +165,18 @@
(cond-> collection
(:children collection) (update :children (partial collection-tree-transform xform))))))
-(defn- collection-tree-names-only
- "Keep just the names of Collections in `collection-ids-to-keep` in the response returned by the Collection tree
- endpoint."
- [collection-ids-to-keep collections]
- (collection-tree-transform (fn [collection]
- (when (contains? (set collection-ids-to-keep) (:id collection))
- (select-keys collection [:name :children])))
- collections))
+(defn- collection-tree-view
+ "Keep just the fields specified by `fields-to-keep` of Collections in `collection-ids-to-keep` in the response
+ returned by the Collection tree endpoint. If `fields-to-keep` is not specified, only the names are kept."
+ ([collection-ids-to-keep collections]
+ (collection-tree-view collection-ids-to-keep [:name] collections))
+ ([collection-ids-to-keep fields-to-keep collections]
+ (let [selection (conj fields-to-keep :children)
+ ids-to-keep (set collection-ids-to-keep)]
+ (collection-tree-transform (fn [collection]
+ (when (contains? ids-to-keep (:id collection))
+ (select-keys collection selection)))
+ collections))))
(deftest collection-tree-test
(testing "GET /api/collection/tree"
@@ -178,34 +184,41 @@
(testing "sanity check"
(is (some? personal-collection)))
(with-collection-hierarchy [a b c d e f g]
- (let [ids (set (map :id (cons personal-collection [a b c d e f g])))
- response (mt/user-http-request :rasta :get 200 "collection/tree")]
- (testing "Make sure overall tree shape of the response is as is expected"
- (is (= [{:name "A"
- :children [{:name "B", :children []}
- {:name "C"
- :children [{:name "D"
- :children [{:name "E", :children []}]}
- {:name "F"
- :children [{:name "G", :children []}]}]}]}
- {:name "Rasta Toucan's Personal Collection", :children []}]
- (collection-tree-names-only ids response))))
- (testing "Make sure each Collection comes back with the expected keys"
- (is (= {:description nil
- :archived false
- :entity_id (:entity_id personal-collection)
- :slug "rasta_toucan_s_personal_collection"
- :color "#31698A"
- :name "Rasta Toucan's Personal Collection"
- :personal_owner_id (mt/user->id :rasta)
- :id (:id (collection/user->personal-collection (mt/user->id :rasta)))
- :location "/"
- :namespace nil
- :children []
- :authority_level nil}
- (some #(when (= (:id %) (:id (collection/user->personal-collection (mt/user->id :rasta))))
- %)
- response))))))
+ (mt/with-temp* [App [{app-a-id :id} {:collection_id (:id a)}]
+ App [{app-c-id :id} {:collection_id (:id c)}]
+ App [{app-g-id :id} {:collection_id (:id g)}]]
+ (let [ids (set (map :id (cons personal-collection [a b c d e f g])))
+ response (mt/user-http-request :rasta :get 200 "collection/tree")]
+ (testing "Make sure overall tree shape of the response is as is expected"
+ (is (= [{:name "A"
+ :app_id app-a-id
+ :children [{:name "B", :children []}
+ {:name "C"
+ :app_id app-c-id
+ :children [{:name "D"
+ :children [{:name "E", :children []}]}
+ {:name "F"
+ :children [{:name "G"
+ :app_id app-g-id
+ :children []}]}]}]}
+ {:name "Rasta Toucan's Personal Collection", :children []}]
+ (collection-tree-view ids [:name :app_id] response))))
+ (testing "Make sure each Collection comes back with the expected keys"
+ (is (= {:description nil
+ :archived false
+ :entity_id (:entity_id personal-collection)
+ :slug "rasta_toucan_s_personal_collection"
+ :color "#31698A"
+ :name "Rasta Toucan's Personal Collection"
+ :personal_owner_id (mt/user->id :rasta)
+ :id (:id (collection/user->personal-collection (mt/user->id :rasta)))
+ :location "/"
+ :namespace nil
+ :children []
+ :authority_level nil}
+ (some #(when (= (:id %) (:id (collection/user->personal-collection (mt/user->id :rasta))))
+ %)
+ response)))))))
(testing "Excludes archived collections (#19603)"
(mt/with-temp* [Collection [a {:name "A"}]
Collection [b {:name "B archived"
@@ -217,7 +230,7 @@
response (mt/user-http-request :rasta :get 200
"collection/tree?exclude-archived=true")]
(is (= [{:name "A" :children []}]
- (collection-tree-names-only ids response)))))))
+ (collection-tree-view ids response)))))))
(testing "for personal collections, it should return name and slug in user's locale"
(with-french-user-and-personal-collection user collection
@@ -250,7 +263,7 @@
(perms/revoke-collection-permissions! (perms-group/all-users) parent-collection)
(perms/grant-collection-readwrite-permissions! (perms-group/all-users) child-collection)
(is (= [{:name "Child", :children []}]
- (collection-tree-names-only (map :id [parent-collection child-collection])
+ (collection-tree-view (map :id [parent-collection child-collection])
(mt/user-http-request :rasta :get 200 "collection/tree")))))))
(testing "Namespace parameter"
@@ -259,17 +272,17 @@
(let [ids [normal-id coins-id]]
(testing "shouldn't show Collections of a different `:namespace` by default"
(is (= [{:name "Normal Collection", :children []}]
- (collection-tree-names-only ids (mt/user-http-request :rasta :get 200 "collection/tree")))))
+ (collection-tree-view ids (mt/user-http-request :rasta :get 200 "collection/tree")))))
(perms/grant-collection-read-permissions! (perms-group/all-users) coins-id)
(testing "By passing `:namespace` we should be able to see Collections of that `:namespace`"
(testing "?namespace=currency"
(is (= [{:name "Coin Collection", :children []}]
- (collection-tree-names-only ids (mt/user-http-request :rasta :get 200 "collection/tree?namespace=currency")))))
+ (collection-tree-view ids (mt/user-http-request :rasta :get 200 "collection/tree?namespace=currency")))))
(testing "?namespace=stamps"
(is (= []
- (collection-tree-names-only ids (mt/user-http-request :rasta :get 200 "collection/tree?namespace=stamps")))))))))
+ (collection-tree-view ids (mt/user-http-request :rasta :get 200 "collection/tree?namespace=stamps")))))))))
(testing "Tree should elide Collections for which we have no permissions (#14280)"
;; Create hierarchy like
@@ -311,10 +324,12 @@
(deftest fetch-collection-test
(testing "GET /api/collection/:id"
(testing "check that we can see collection details"
- (mt/with-temp Collection [collection {:name "Coin Collection"}]
+ (mt/with-temp* [Collection [collection {:name "Coin Collection"}]
+ App [{app-id :id} {:collection_id (:id collection)}]]
(perms/grant-collection-read-permissions! (perms-group/all-users) collection)
- (is (= "Coin Collection"
- (:name (mt/user-http-request :rasta :get 200 (str "collection/" (u/the-id collection))))))))
+ (is (= ["Coin Collection" app-id]
+ ((juxt :name :app_id)
+ (mt/user-http-request :rasta :get 200 (str "collection/" (u/the-id collection))))))))
(testing "check that collections detail properly checks permissions"
(mt/with-non-admin-groups-no-root-collection-perms
--
GitLab