Create collection together with the app (#24961)

Addresses #24951, part of #24861.

* Create collection together with the app
* Don't create an app collection on FE

Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com>

frontend/src/metabase/entities/data-apps/data-apps.ts

@@ -2,7 +2,7 @@ import { color } from "metabase/lib/colors";
 import { createEntity } from "metabase/lib/entities";
 
 import { DataAppSchema } from "metabase/schema";
-import { CollectionsApi, DataAppsApi } from "metabase/services";
+import { DataAppsApi } from "metabase/services";
 
 import { Collection, DataApp } from "metabase-types/api";
 
@@ -36,15 +36,13 @@ const DataApps = createEntity({
       description,
       ...dataAppProps
     }: CreateDataAppParams) => {
-      const collection = await CollectionsApi.create({
-        name,
-        description: description || null,
-        parent_id: null, // apps should always live in root collection
-        color: color(DEFAULT_COLLECTION_COLOR_ALIAS),
-      });
       return DataAppsApi.create({
         ...dataAppProps,
-        collection_id: collection.id,
+        collection: {
+          name,
+          description: description || null,
+          color: color(DEFAULT_COLLECTION_COLOR_ALIAS),
+        },
       });
     },
   },

src/metabase/api/app.clj

 (ns metabase.api.app
   (:require
     [compojure.core :refer [POST PUT]]
+    [metabase.api.collection :as api.collection]
     [metabase.api.common :as api]
     [metabase.models :refer [App Collection]]
     [metabase.models.collection :as collection]
@@ -14,16 +15,26 @@
 
 (api/defendpoint POST "/"
   "Endpoint to create an app"
-  [:as {{:keys [collection_id dashboard_id options nav_items] :as body} :body}]
-  {collection_id su/IntGreaterThanOrEqualToZero
-   dashboard_id (s/maybe su/IntGreaterThanOrEqualToZero)
-   options (s/maybe su/Map)
-   nav_items (s/maybe [(s/maybe su/Map)])}
-  (api/write-check Collection collection_id)
-  (api/check (not (db/select-one-id App :collection_id collection_id))
-    [400 "An App already exists on this Collection"])
-  (let [app (db/insert! App (select-keys body [:dashboard_id :collection_id :options :nav_items]))]
-    (hydrate-details app)))
+  [:as {{:keys [collection dashboard_id options nav_items]
+         {:keys [name color description parent_id namespace authority_level]} :collection
+         :as body} :body}]
+  {dashboard_id    (s/maybe su/IntGreaterThanOrEqualToZero)
+   options         (s/maybe su/Map)
+   nav_items       (s/maybe [(s/maybe su/Map)])
+   name            su/NonBlankString
+   color           collection/hex-color-regex
+   description     (s/maybe su/NonBlankString)
+   parent_id       (s/maybe su/IntGreaterThanZero)
+   namespace       (s/maybe su/NonBlankString)
+   authority_level collection/AuthorityLevel}
+  (db/transaction
+   (let [coll-params (select-keys collection [:name :color :description :parent_id :namespace :authority_level])
+         collection-instance (api.collection/create-collection! coll-params)
+         app-params (-> body
+                        (select-keys [:dashboard_id :options :nav_items])
+                        (assoc :collection_id (:id collection-instance)))
+         app (db/insert! App app-params)]
+     (hydrate-details app))))
 
 (api/defendpoint PUT "/:app-id"
   "Endpoint to change an app"
@@ -36,7 +47,6 @@
   (db/update! App app-id (select-keys body [:dashboard_id :options :nav_items]))
   (hydrate-details (db/select-one App :id app-id)))
 
-;; TODO handle personal collections, see collection/personal-collection-with-ui-details
 (api/defendpoint GET "/"
   "Fetch a list of all Apps that the current user has read permissions for.
 

src/metabase/api/collection.clj

@@ -723,15 +723,9 @@
                      (db/select-one Collection :id collection-id)
                      collection/root-collection)))
 
-(api/defendpoint POST "/"
-  "Create a new Collection."
-  [:as {{:keys [name color description parent_id namespace authority_level]} :body}]
-  {name            su/NonBlankString
-   color           collection/hex-color-regex
-   description     (s/maybe su/NonBlankString)
-   parent_id       (s/maybe su/IntGreaterThanZero)
-   namespace       (s/maybe su/NonBlankString)
-   authority_level collection/AuthorityLevel}
+(defn create-collection!
+  "Create a new collection."
+  [{:keys [name color description parent_id namespace authority_level]}]
   ;; To create a new collection, you need write perms for the location you are going to be putting it in...
   (write-check-collection-or-root-collection parent_id)
   ;; Now create the new Collection :)
@@ -747,6 +741,17 @@
      (when parent_id
        {:location (collection/children-location (db/select-one [Collection :location :id] :id parent_id))}))))
 
+(api/defendpoint POST "/"
+  "Create a new Collection."
+  [:as {{:keys [name color description parent_id namespace authority_level] :as body} :body}]
+  {name            su/NonBlankString
+   color           collection/hex-color-regex
+   description     (s/maybe su/NonBlankString)
+   parent_id       (s/maybe su/IntGreaterThanZero)
+   namespace       (s/maybe su/NonBlankString)
+   authority_level collection/AuthorityLevel}
+  (create-collection! body))
+
 ;; TODO - I'm not 100% sure it makes sense that moving a Collection requires a special call to `move-collection!`,
 ;; while archiving is handled automatically as part of the `pre-update` logic when you change a Collection's
 ;; `archived` value. They are both recursive operations; couldn't we just have moving happen automatically when you

test/metabase/api/app_test.clj

@@ -7,30 +7,48 @@
     [metabase.test :as mt]))
 
 (deftest create-test
-  (mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
-    (mt/with-temp* [Collection [{collection_id :id}]]
-      (is (partial= {:collection_id collection_id}
-                    (mt/user-http-request :crowberto :post 200 "app" {:collection_id collection_id})))
-      (is (= "An App already exists on this Collection"
-             (mt/user-http-request :crowberto :post 400 "app" {:collection_id collection_id}))))
-    (testing "Collection permissions"
-      (mt/with-non-admin-groups-no-root-collection-perms
-        (mt/with-temp* [Collection [{collection_id :id}]]
-          (is (= "You don't have permissions to do that."
-                 (mt/user-http-request :rasta :post 403 "app" {:collection_id collection_id})))))
-      (mt/with-temp* [Collection [{collection_id :id}]]
-        (is (partial= {:collection_id collection_id}
-                      (mt/user-http-request :rasta :post 200 "app" {:collection_id collection_id})))))
-    (testing "With initial dashboard and nav_items"
-      (mt/with-temp* [Collection [{collection_id :id}]
-                      Dashboard [{dashboard_id :id}]]
-        (let [nav_items [{:options {:click_behavior {}}}]]
-          (is (partial= {:collection_id collection_id
-                         :dashboard_id dashboard_id
-                         :nav_items nav_items}
-                        (mt/user-http-request :crowberto :post 200 "app" {:collection_id collection_id
-                                                                          :dashboard_id dashboard_id
-                                                                          :nav_items nav_items}))))))))
+  (mt/with-model-cleanup [Collection]
+    (let [base-params {:name "App collection"
+                       :color "#123456"}]
+      (mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
+        (testing "Create app in non-root collection"
+          (mt/with-temp* [Collection [{collection-id :id}]]
+            (let [coll-params (assoc base-params :parent_id collection-id)
+                  response (mt/user-http-request :crowberto :post 200 "app" {:collection coll-params})]
+              (is (pos-int? (:id response)))
+              (is (pos-int? (:collection_id response)))
+              (is (partial= (assoc base-params :location (format "/%d/" collection-id))
+                            (:collection response))))))
+        (testing "Create aoo in the root"
+          (let [response (mt/user-http-request :crowberto :post 200 "app" {:collection base-params})]
+            (is (pos-int? (:id response)))
+            (is (pos-int? (:collection_id response)))
+            (is (partial= (assoc base-params :location "/")
+                          (:collection response)))))
+        (testing "Collection permissions"
+          (mt/with-non-admin-groups-no-root-collection-perms
+            (mt/with-temp* [Collection [{collection-id :id}]]
+              (let [coll-params (assoc base-params :parent_id collection-id)]
+                (is (= "You don't have permissions to do that."
+                       (mt/user-http-request :rasta :post 403 "app" {:collection coll-params}))))))
+          (mt/with-temp* [Collection [{collection-id :id}]]
+            (let [coll-params (assoc base-params :parent_id collection-id)
+                  response (mt/user-http-request :rasta :post 200 "app" {:collection coll-params})]
+              (is (pos-int? (:id response)))
+              (is (pos-int? (:collection_id response)))
+              (is (partial= (assoc base-params :location (format "/%d/" collection-id))
+                            (:collection response))))))
+        (testing "With initial dashboard and nav_items"
+          (mt/with-temp* [Collection [{collection-id :id}]
+                          Dashboard [{dashboard-id :id}]]
+            (let [coll-params (assoc base-params :parent_id collection-id)
+                  nav_items [{:options {:click_behavior {}}}]]
+              (is (partial= {:collection (assoc base-params :location (format "/%d/" collection-id))
+                             :dashboard_id dashboard-id
+                             :nav_items nav_items}
+                            (mt/user-http-request :crowberto :post 200 "app" {:collection coll-params
+                                                                              :dashboard_id dashboard-id
+                                                                              :nav_items nav_items}))))))))))
 
 (deftest update-test
   (mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
@@ -95,15 +113,15 @@
         (mt/with-temp* [Collection [collection-1 {:name "Collection 1"}]
                         Collection [collection-2 {:name "Collection 2" :archived true}]
                         Dashboard [{dashboard_id :id}]
-                        App [{app-1-id :id}   (assoc app-data :collection_id (:id collection-1) :dashboard_id dashboard_id)]
+                        App [{app-1-id :id} (assoc app-data :collection_id (:id collection-1) :dashboard_id dashboard_id)]
                         App [{app-2-id :id} (assoc app-data :collection_id (:id collection-2) :dashboard_id dashboard_id)]]
           (testing "listing normal apps"
             (let [expected (merge app-data {:id app-1-id
                                             :collection_id (:id collection-1)
                                             :dashboard_id dashboard_id
                                             :collection (assoc collection-1 :can_write true)})]
-             (is (partial= [expected]
-                           (mt/user-http-request :rasta :get 200 "app")))))
+              (is (partial= [expected]
+                            (mt/user-http-request :rasta :get 200 "app")))))
           (testing "listing archived"
             (let [expected (merge app-data {:id app-2-id
                                             :collection_id (:id collection-2)