POST /api/card/collections (bulk update) endpoint

docs/api-documentation.md

@@ -93,6 +93,7 @@ Favorite a Card.
 ## `POST /api/card/:card-id/labels`
 
 Update the set of `Labels` that apply to a `Card`.
+   (This endpoint is considered DEPRECATED as Labels will be removed in a future version of Metabase.)
 
 ##### PARAMS:
 
@@ -134,6 +135,18 @@ Run the query associated with a Card, and return its results as JSON. Note that
 *  **`parameters`** value may be nil, or if non-nil, value must be a valid JSON string.
 
 
+## `POST /api/card/collections`
+
+Bulk update endpoint for Card Collections. Move a set of `Cards` with CARD_IDS into a `Collection` with COLLECTION_ID,
+   or remove them from any Collections by passing a `null` COLLECTION_ID.
+
+##### PARAMS:
+
+*  **`card_ids`** value must be an array. Each value must be an integer greater than zero.
+
+*  **`collection_id`** value may be nil, or if non-nil, value must be an integer greater than zero.
+
+
 ## `PUT /api/card/:id`
 
 Update a `Card`.

src/metabase/api/card.clj

@@ -207,6 +207,10 @@
       (->> (events/publish-event! :card-read))
       (dissoc :actor_id)))
 
+(defn- check-permissions-for-collection
+  "Check that we have permissions to add or remove cards from `Collection` with COLLECTION-ID."
+  [collection-id]
+  (check-403 (perms/set-has-full-permissions? @*current-user-permissions-set* (perms/collection-readwrite-path collection-id))))
 
 (defendpoint PUT "/:id"
   "Update a `Card`."
@@ -220,7 +224,7 @@
   (let [card (write-check Card id)]
     ;; if we're changing the `collection_id` of the Card, make sure we have write permissions for the new group
     (when (not= (:collection_id card) collection_id)
-      (check-403 (perms/set-has-full-permissions? @*current-user-permissions-set* (perms/collection-readwrite-path collection_id))))
+      (check-permissions-for-collection collection_id))
     ;; ok, now save the Card
     (db/update! Card id
       (merge {:collection_id collection_id}
@@ -270,7 +274,8 @@
 
 
 (defendpoint POST "/:card-id/labels"
-  "Update the set of `Labels` that apply to a `Card`."
+  "Update the set of `Labels` that apply to a `Card`.
+   (This endpoint is considered DEPRECATED as Labels will be removed in a future version of Metabase.)"
   [card-id :as {{:keys [label_ids]} :body}]
   {label_ids [su/IntGreaterThanZero]}
   (write-check Card card-id)
@@ -283,6 +288,38 @@
   {:status :ok})
 
 
+;;; ------------------------------------------------------------ Bulk Collections Update ------------------------------------------------------------
+
+(defn- move-cards-to-collection! [new-collection-id-or-nil card-ids]
+  ;; if moving to a collection, make sure we have write perms for it
+  (when new-collection-id-or-nil
+    (write-check Collection new-collection-id-or-nil))
+  ;; for each affected card...
+  (when (seq card-ids)
+    (let [cards (db/select [Card :id :collection_id :dataset_query]
+                  {:where [:and [:in :id (set card-ids)]
+                                [:or [:not= :collection_id new-collection-id-or-nil]
+                                     (when new-collection-id-or-nil
+                                       [:= :collection_id nil])]]})] ; poisioned NULLs = ick
+      ;; ...check that we have write permissions for it...
+      (doseq [card cards]
+        (write-check card))
+      ;; ...and check that we have write permissions for the old collections if applicable
+      (doseq [old-collection-id (set (filter identity (map :collection_id cards)))]
+        (write-check Collection old-collection-id)))
+    ;; ok, everything checks out. Set the new `collection_id` for all the Cards
+    (db/update-where! Card {:id [:in (set card-ids)]}
+      :collection_id new-collection-id-or-nil)))
+
+(defendpoint POST "/collections"
+  "Bulk update endpoint for Card Collections. Move a set of `Cards` with CARD_IDS into a `Collection` with COLLECTION_ID,
+   or remove them from any Collections by passing a `null` COLLECTION_ID."
+  [:as {{:keys [card_ids collection_id]} :body}]
+  {card_ids [su/IntGreaterThanZero], collection_id (s/maybe su/IntGreaterThanZero)}
+  (move-cards-to-collection! collection_id card_ids)
+  {:status :ok})
+
+
 ;;; ------------------------------------------------------------ Running a Query ------------------------------------------------------------
 
 (defn- run-query-for-card [card-id parameters]

test/metabase/api/card_test.clj

@@ -132,6 +132,10 @@
   [card-2-id]
   (map :id ((user->client :rasta) :get 200 "card", :label "more_toucans")))                 ; filtering is done by slug
 
+(defn- mbql-count-query [database-id table-id]
+  {:database database-id
+   :type     "query"
+   :query    {:source-table table-id, :aggregation {:aggregation-type "count"}}})
 
 ;; ## POST /api/card
 ;; Test that we can make a card
@@ -141,9 +145,7 @@
     {:description            nil
      :name                   card-name
      :creator_id             (user->id :rasta)
-     :dataset_query          {:database database-id
-                              :type     "query"
-                              :query    {:source-table table-id, :aggregation {:aggregation-type "count"}}}
+     :dataset_query          (mbql-count-query database-id table-id)
      :display                "scalar"
      :visualization_settings {:global {:title nil}}
      :database_id            database-id ; these should be inferred automatically
@@ -154,9 +156,7 @@
     ;; make sure we clean up after ourselves as well and delete the Card we create
     (dissoc (u/prog1 ((user->client :rasta) :post 200 "card" {:name                   card-name
                                                               :display                "scalar"
-                                                              :dataset_query          {:database database-id
-                                                                                       :type     :query
-                                                                                       :query    {:source-table table-id, :aggregation {:aggregation-type :count}}}
+                                                              :dataset_query          (mbql-count-query database-id table-id)
                                                               :visualization_settings {:global {:title nil}}})
               (db/cascade-delete! Card :id (u/get-id <>)))
             :created_at :updated_at :id)))
@@ -165,9 +165,7 @@
 ;; Test that we can fetch a card
 (expect-with-temp [Database  [{database-id :id}]
                    Table     [{table-id :id}   {:db_id database-id}]
-                   Card      [card             {:dataset_query {:database database-id
-                                                                :type     :query
-                                                                :query    {:source-table table-id, :aggregation {:aggregation-type :count}}}}]]
+                   Card      [card             {:dataset_query (mbql-count-query database-id table-id)}]]
   (match-$ card
     {:description            nil
      :dashboard_count        0
@@ -200,9 +198,7 @@
 ;; Check that a user without permissions isn't allowed to fetch the card
 (expect-with-temp [Database  [{database-id :id}]
                    Table     [{table-id :id}   {:db_id database-id}]
-                   Card      [card             {:dataset_query {:database database-id
-                                                                :type     :query
-                                                                :query    {:source-table table-id, :aggregation {:aggregation-type :count}}}}]]
+                   Card      [card             {:dataset_query (mbql-count-query database-id table-id)}]]
   "You don't have permissions to do that."
   (do
     ;; revoke permissions for default group to this database
@@ -405,9 +401,7 @@
     (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
     (let [{card-id :id} ((user->client :rasta) :post 200 "card" {:name                   "My Cool Card"
                                                                  :display                "scalar"
-                                                                 :dataset_query          {:database (id)
-                                                                                          :type     :query
-                                                                                          :query    {:source-table (id :venues), :aggregation {:aggregation-type :count}}}
+                                                                 :dataset_query          (mbql-count-query (id) (id :venues))
                                                                  :visualization_settings {:global {:title nil}}
                                                                  :collection_id          (u/get-id collection)})]
       ;; make sure we clean up after ourselves and delete the newly created Card
@@ -419,9 +413,7 @@
   "You don't have permissions to do that."
   ((user->client :rasta) :post 403 "card" {:name                   "My Cool Card"
                                            :display                "scalar"
-                                           :dataset_query          {:database (id)
-                                                                    :type     :query
-                                                                    :query    {:source-table (id :venues), :aggregation {:aggregation-type :count}}}
+                                           :dataset_query          (mbql-count-query (id) (id :venues))
                                            :visualization_settings {:global {:title nil}}
                                            :collection_id          (u/get-id collection)}))
 
@@ -491,3 +483,74 @@
 (expect
   "Not found."
   ((user->client :rasta) :get 404 "card/" :collection :some_fake_collection_slug))
+
+
+;;; ------------------------------------------------------------ Bulk Collections Update (POST /api/card/collections) ------------------------------------------------------------
+
+(defn- collection-ids [cards-or-card-ids]
+  (map :collection_id (db/select [Card :collection_id]
+                        :id [:in (map u/get-id cards-or-card-ids)])))
+
+(defn- POST-card-collections!
+  "Update the Collection of CARDS-OR-CARD-IDS via the `POST /api/card/collections` endpoint using USERNAME;
+   return the response of this API request and the latest Collection IDs from the database."
+  [username expected-status-code collection-or-collection-id-or-nil cards-or-card-ids]
+  [((user->client username) :post expected-status-code "card/collections"
+     {:collection_id (when collection-or-collection-id-or-nil
+                       (u/get-id collection-or-collection-id-or-nil))
+      :card_ids      (map u/get-id cards-or-card-ids)})
+   (collection-ids cards-or-card-ids)])
+
+;; Test that we can bulk move some Cards with no collection into a collection
+(tu/expect-with-temp [Collection [collection]
+                      Card       [card-1]
+                      Card       [card-2]]
+  [{:status "ok"}
+   [(u/get-id collection) (u/get-id collection)]]
+  (POST-card-collections! :crowberto 200 collection [card-1 card-2]))
+
+;; Test that we can bulk move some Cards from one collection to another
+(tu/expect-with-temp [Collection [old-collection]
+                      Collection [new-collection]
+                      Card       [card-1         {:collection_id (u/get-id old-collection)}]
+                      Card       [card-2         {:collection_id (u/get-id old-collection)}]]
+  [{:status "ok"}
+   [(u/get-id new-collection) (u/get-id new-collection)]]
+  (POST-card-collections! :crowberto 200 new-collection [card-1 card-2]))
+
+;; Test that we can bulk remove some Cards from a collection
+(tu/expect-with-temp [Collection [collection]
+                      Card       [card-1     {:collection_id (u/get-id collection)}]
+                      Card       [card-2     {:collection_id (u/get-id collection)}]]
+  [{:status "ok"}
+   [nil nil]]
+  (POST-card-collections! :crowberto 200 nil [card-1 card-2]))
+
+;; Check that we aren't allowed to move Cards if we don't have permissions for destination collection
+(tu/expect-with-temp [Collection [collection]
+                      Card       [card-1]
+                      Card       [card-2]]
+  ["You don't have permissions to do that."
+   [nil nil]]
+  (POST-card-collections! :rasta 403 collection [card-1 card-2]))
+
+;; Check that we aren't allowed to move Cards if we don't have permissions for source collection
+(tu/expect-with-temp [Collection [collection]
+                      Card       [card-1     {:collection_id (u/get-id collection)}]
+                      Card       [card-2     {:collection_id (u/get-id collection)}]]
+  ["You don't have permissions to do that."
+   [(u/get-id collection) (u/get-id collection)]]
+  (POST-card-collections! :rasta 403 nil [card-1 card-2]))
+
+;; Check that we aren't allowed to move Cards if we don't have permissions for the Card
+(tu/expect-with-temp [Collection [collection]
+                      Database   [database]
+                      Table      [table      {:db_id (u/get-id database)}]
+                      Card       [card-1     {:dataset_query (mbql-count-query (u/get-id database) (u/get-id table))}]
+                      Card       [card-2     {:dataset_query (mbql-count-query (u/get-id database) (u/get-id table))}]]
+  ["You don't have permissions to do that."
+   [nil nil]]
+  (do
+    (perms/revoke-permissions! (perms-group/all-users) (u/get-id database))
+    (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+    (POST-card-collections! :rasta 403 collection [card-1 card-2])))