Merge pull request #7886 from metabase/collection-position-updates

Add collection position reconcilliation to card/dashboard/pulse updates

frontend/src/metabase/components/CollectionLanding.jsx

@@ -189,10 +189,12 @@ class DefaultLanding extends React.Component {
                       <h4>{t`Pinned items`}</h4>
                     </Box>
                     <PinDropTarget
-                      pinIndex={1}
+                      pinIndex={
+                        pinned[pinned.length - 1].collection_position + 1
+                      }
+                      noDrop
                       marginLeft={8}
                       marginRight={8}
-                      noBorder
                     >
                       <Grid>
                         {pinned.map((item, index) => (
@@ -204,9 +206,12 @@ class DefaultLanding extends React.Component {
                                 item={item}
                                 collection={collection}
                               />
-                              <PinPositionDropTarget pinIndex={index} left />
                               <PinPositionDropTarget
-                                pinIndex={index + 1}
+                                pinIndex={item.collection_position}
+                                left
+                              />
+                              <PinPositionDropTarget
+                                pinIndex={item.collection_position + 1}
                                 right
                               />
                             </ItemDragSource>
@@ -214,7 +219,12 @@ class DefaultLanding extends React.Component {
                         ))}
                         {pinned.length % 2 === 1 ? (
                           <GridItem w={1 / 2} className="relative">
-                            <PinPositionDropTarget pinIndex={pinned.length} />
+                            <PinPositionDropTarget
+                              pinIndex={
+                                pinned[pinned.length - 1].collection_position +
+                                1
+                              }
+                            />
                           </GridItem>
                         ) : null}
                       </Grid>

frontend/src/metabase/containers/dnd/DropArea.jsx

@@ -5,7 +5,7 @@ import { normal } from "metabase/lib/colors";
 const DropTargetBackgroundAndBorder = ({
   highlighted,
   hovered,
-  noBorder = false,
+  noDrop = false,
   margin = 0,
   marginLeft = margin,
   marginRight = margin,
@@ -25,7 +25,7 @@ const DropTargetBackgroundAndBorder = ({
       zIndex: -1,
       boxSizing: "border-box",
       border: "2px solid transparent",
-      borderColor: hovered & !noBorder ? normal.blue : "transparent",
+      borderColor: hovered & !noDrop ? normal.blue : "transparent",
     }}
   />
 );

frontend/src/metabase/containers/dnd/PinDropTarget.jsx

@@ -7,7 +7,9 @@ const PinDropTarget = DropTarget(
   PinnableDragTypes,
   {
     drop(props, monitor, component) {
-      return { pinIndex: props.pinIndex };
+      if (!props.noDrop) {
+        return { pinIndex: props.pinIndex };
+      }
     },
     canDrop(props, monitor) {
       const { item } = monitor.getItem();

frontend/src/metabase/containers/dnd/PinPositionDropTarget.jsx

@@ -10,6 +10,20 @@ const PIN_DROP_TARGET_INDICATOR_WIDTH = 3;
   PinnableDragTypes,
   {
     drop(props, monitor, component) {
+      const { item } = monitor.getItem();
+      // no need to move to the same position
+      if (item.collection_position == props.pinIndex) {
+        return;
+      }
+      // no already pinned, so add it at the dropped position
+      if (item.collection_position == null) {
+        return { pinIndex: props.pinIndex };
+      }
+      // being moved to a later position, which will cause everything to be shifted down, so subtract one
+      if (item.collection_position < props.pinIndex) {
+        return { pinIndex: props.pinIndex - 1 };
+      }
+      // being moved to an earlier position, no need to account for shift
       return { pinIndex: props.pinIndex };
     },
   },

src/metabase/api/card.clj

@@ -224,16 +224,21 @@
   ;; check that we have permissions for the collection we're trying to save this card to, if applicable
   (collection/check-write-perms-for-collection collection_id)
   ;; everything is g2g, now save the card
-  (let [card (db/insert! Card
-               :creator_id             api/*current-user-id*
-               :dataset_query          dataset_query
-               :description            description
-               :display                display
-               :name                   name
-               :visualization_settings visualization_settings
-               :collection_id          collection_id
-               :collection_position    collection_position
-               :result_metadata        (result-metadata dataset_query result_metadata metadata_checksum))]
+  (let [card-data {:creator_id             api/*current-user-id*
+                   :dataset_query          dataset_query
+                   :description            description
+                   :display                display
+                   :name                   name
+                   :visualization_settings visualization_settings
+                   :collection_id          collection_id
+                   :collection_position    collection_position
+                   :result_metadata        (result-metadata dataset_query result_metadata metadata_checksum)}
+
+        card      (db/transaction
+                    ;; Adding a new card at `collection_position` could cause other cards in this
+                    ;; collection to change position, check that and fix it if needed
+                    (api/maybe-reconcile-collection-position! card-data)
+                    (db/insert! Card card-data))]
     (events/publish-event! :card-create card)
     ;; include same information returned by GET /api/card/:id since frontend replaces the Card it currently has
     ;; with returned one -- See #4283
@@ -402,14 +407,19 @@
     (let [card-updates (assoc card-updates
                          :result_metadata (result-metadata-for-updating card-before-update dataset_query
                                                                         result_metadata metadata_checksum))]
-      ;; ok, now save the Card
-      (db/update! Card id
-        ;; `collection_id` and `description` can be `nil` (in order to unset them). Other values should only be
-        ;; modified if they're passed in as non-nil
-        (u/select-keys-when card-updates
-          :present #{:collection_id :collection_position :description}
-          :non-nil #{:dataset_query :display :name :visualization_settings :archived :enable_embedding
-                     :embedding_params :result_metadata})))
+
+      ;; Setting up a transaction here so that we don't get a partially reconciled/updated card.
+      (db/transaction
+        (api/maybe-reconcile-collection-position! card-before-update card-updates)
+
+        ;; ok, now save the Card
+        (db/update! Card id
+          ;; `collection_id` and `description` can be `nil` (in order to unset them). Other values should only be
+          ;; modified if they're passed in as non-nil
+          (u/select-keys-when card-updates
+            :present #{:collection_id :collection_position :description}
+            :non-nil #{:dataset_query :display :name :visualization_settings :archived :enable_embedding
+                       :embedding_params :result_metadata}))))
     ;; Fetch the updated Card from the DB
     (let [card (Card id)]
       (delete-alerts-if-needed! card-before-update card)
@@ -454,26 +464,71 @@
 
 ;;; -------------------------------------------- Bulk Collections Update ---------------------------------------------
 
+(defn- update-collection-positions!
+  "For cards that have a position in the previous collection, add them to the end of the new collection, trying to
+  preseve the order from the original collections. Note it's possible for there to be multiple collections
+  (and thus duplicate collection positions) merged into this new collection. No special tie breaker logic for when
+  that's the case, just use the order the DB returned it in"
+  [new-collection-id-or-nil cards]
+  ;; Sorting by `:collection_position` to ensure lower position cards are appended first
+  (let [sorted-cards        (sort-by :collection_position cards)
+        max-position-result (db/select-one [Card [:%max.collection_position :max_position]]
+                              :collection_id new-collection-id-or-nil)
+        ;; collection_position for the next card in the collection
+        starting-position   (inc (get max-position-result :max_position 0))]
+
+    ;; This is using `map` but more like a `doseq` with multiple seqs. Wrapping this in a `doall` as we don't want it
+    ;; to be lazy and we're just going to discard the results
+    (doall
+     (map (fn [idx {:keys [collection_id collection_position] :as card}]
+            ;; We are removing this card from `collection_id` so we need to reconcile any
+            ;; `collection_position` entries left behind by this move
+            (api/reconcile-position-for-collection! collection_id collection_position nil)
+            ;; Now we can update the card with the new collection and a new calculated position
+            ;; that appended to the end
+            (db/update! Card (u/get-id card)
+              :collection_position idx
+              :collection_id       new-collection-id-or-nil))
+          ;; These are reversed because of the classic issue when removing an item from array. If we remove an
+          ;; item at index 1, everthing above index 1 will get decremented. By reversing our processing order we
+          ;; can avoid changing the index of cards we haven't yet updated
+          (reverse (range starting-position (+ (count sorted-cards) starting-position)))
+          (reverse sorted-cards)))))
+
 (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
     (api/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]
+    (let [cards (db/select [Card :id :collection_id :collection_position :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
+                                  (when new-collection-id-or-nil
+                                    [:= :collection_id nil])]]})] ; poisioned NULLs = ick
       ;; ...check that we have write permissions for it...
       (doseq [card cards]
         (api/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)))]
-        (api/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)))
+        (api/write-check Collection old-collection-id))
+
+      ;; Ensure all of the card updates occur in a transaction. Read commited (the default) really isn't what we want
+      ;; here. We are querying for the max card position for a given collection, then using that to base our position
+      ;; changes if the cards are moving to a different collection. Without repeatable read here, it's possible we'll
+      ;; get duplicates
+      (db/transaction
+        ;; If any of the cards have a `:collection_position`, we'll need to fixup the old collection now that the cards
+        ;; are gone and update the position in the new collection
+        (when-let [cards-with-position (seq (filter :collection_position cards))]
+          (update-collection-positions! new-collection-id-or-nil cards-with-position))
+
+        ;; ok, everything checks out. Set the new `collection_id` for all the Cards that haven't been updated already
+        (when-let [cards-without-position (seq (for [card cards
+                                                     :when (not (:collection_position card))]
+                                                 (u/get-id card)))]
+          (db/update-where! Card {:id [:in (set cards-without-position)]}
+            :collection_id new-collection-id-or-nil))))))
 
 (api/defendpoint POST "/collections"
   "Bulk update endpoint for Card Collections. Move a set of `Cards` with CARD_IDS into a `Collection` with

src/metabase/api/common.clj

@@ -7,6 +7,7 @@
             [clojure.string :as str]
             [clojure.tools.logging :as log]
             [compojure.core :as compojure]
+            [honeysql.types :as htypes]
             [medley.core :as m]
             [metabase
              [public-settings :as public-settings]
@@ -511,3 +512,77 @@
    (and (contains? object-updates k)
         (not= (get object-before-updates k)
               (get object-updates k)))))
+
+;;; ------------------------------------------ COLLECTION POSITION HELPER FNS ----------------------------------------
+
+(s/defn reconcile-position-for-collection!
+  "Compare `old-position` and `new-position` to determine what needs to be updated based on the position change. Used
+  for fixing card/dashboard/pulse changes that impact other instances in the collection"
+  [collection-id :- (s/maybe su/IntGreaterThanZero)
+   old-position :- (s/maybe su/IntGreaterThanZero)
+   new-position :- (s/maybe su/IntGreaterThanZero)]
+  (let [update-fn! (fn [plus-or-minus position-update-clause]
+                     (doseq [model '[Card Dashboard Pulse]]
+                       (db/update-where! model {:collection_id       collection-id
+                                                :collection_position position-update-clause}
+                         :collection_position (htypes/call plus-or-minus :collection_position 1))))]
+    (when (not= new-position old-position)
+      (cond
+        (and (nil? new-position)
+             old-position)
+        (update-fn! :-  [:> old-position])
+
+        (and new-position (nil? old-position))
+        (update-fn! :+ [:>= new-position])
+
+        (> new-position old-position)
+        (update-fn! :- [:between old-position new-position])
+
+        (< new-position old-position)
+        (update-fn! :+ [:between new-position old-position])))))
+
+(def ^:private ModelWithPosition
+  "Intended to cover Cards/Dashboards/Pulses, it only asserts collection id and position, allowing extra keys"
+  {:collection_id       (s/maybe su/IntGreaterThanZero)
+   :collection_position (s/maybe su/IntGreaterThanZero)
+   s/Any                s/Any})
+
+(def ^:private ModelWithOptionalPosition
+  "Intended to cover Cards/Dashboards/Pulses updates. Collection id and position are optional, if they are not
+  present, they didn't change. If they are present, they might have changed and we need to compare."
+  {(s/optional-key :collection_id)       (s/maybe su/IntGreaterThanZero)
+   (s/optional-key :collection_position) (s/maybe su/IntGreaterThanZero)
+   s/Any                                 s/Any})
+
+(s/defn maybe-reconcile-collection-position!
+  "Generic function for working on cards/dashboards/pulses. Checks the before and after changes to see if there is any
+  impact to the collection position of that model instance. If so, executes updates to fix the collection position
+  that goes with the change. The 2-arg version of this function is used for a new card/dashboard/pulse (i.e. not
+  updating an existing instance, but creating a new one)."
+  ([new-model-data :- ModelWithPosition]
+   (maybe-reconcile-collection-position! nil new-model-data))
+  ([{old-collection-id :collection_id, old-position :collection_position, :as before-update} :- (s/maybe ModelWithPosition)
+    {new-collection-id :collection_id, new-position :collection_position, :as model-updates} :- ModelWithOptionalPosition]
+   (let [updated-collection? (and (contains? model-updates :collection_id)
+                                  (not= old-collection-id new-collection-id))
+         updated-position?   (and (contains? model-updates :collection_position)
+                                  (not= old-position new-position))]
+     (cond
+       ;; If the collection hasn't changed, but we have a new collection position, we might need to reconcile
+       (and (not updated-collection?) updated-position?)
+       (reconcile-position-for-collection! old-collection-id old-position new-position)
+
+       ;; If we have a new collection id, but no new position, reconcile the old collection, then update the new
+       ;; collection with the existing position
+       (and updated-collection? (not updated-position?))
+       (do
+         (reconcile-position-for-collection! old-collection-id old-position nil)
+         (reconcile-position-for-collection! new-collection-id nil old-position))
+
+       ;; We have a new collection id AND and new collection position
+       ;; Update the old collection using the old position
+       ;; Update the new collection using the new position
+       (and updated-collection? updated-position?)
+       (do
+         (reconcile-position-for-collection! old-collection-id old-position nil)
+         (reconcile-position-for-collection! new-collection-id nil new-position))))))

src/metabase/api/dashboard.clj

@@ -67,16 +67,20 @@
    collection_position (s/maybe su/IntGreaterThanZero)}
   ;; if we're trying to save the new dashboard in a Collection make sure we have permissions to do that
   (collection/check-write-perms-for-collection collection_id)
-  ;; Ok, now save the Dashboard
-  (->> (db/insert! Dashboard
-         :name                name
-         :description         description
-         :parameters          (or parameters [])
-         :creator_id          api/*current-user-id*
-         :collection_id       collection_id
-         :collection_position collection_position)
-       ;; publish an event and return the newly created Dashboard
-       (events/publish-event! :dashboard-create)))
+  (let [dashboard-data {:name                name
+                        :description         description
+                        :parameters          (or parameters [])
+                        :creator_id          api/*current-user-id*
+                        :collection_id       collection_id
+                        :collection_position collection_position}]
+    (db/transaction
+      ;; Adding a new dashboard at `collection_position` could cause other dashboards in this collection to change
+      ;; position, check that and fix up if needed
+      (api/maybe-reconcile-collection-position! dashboard-data)
+      ;; Ok, now save the Dashboard
+      (->> (db/insert! Dashboard dashboard-data)
+           ;; publish an event and return the newly created Dashboard
+           (events/publish-event! :dashboard-create)))))
 
 
 ;;; -------------------------------------------- Hiding Unreadable Cards ---------------------------------------------
@@ -232,15 +236,21 @@
   (let [dash-before-update (api/write-check Dashboard id)]
     ;; Do various permissions checks as needed
     (collection/check-allowed-to-change-collection dash-before-update dash-updates)
-    (check-allowed-to-change-embedding dash-before-update dash-updates))
-  (api/check-500
-   (db/update! Dashboard id
-     ;; description, position, collection_id, and collection_position are allowed to be `nil`. Everything else must be
-     ;; non-nil
-     (u/select-keys-when dash-updates
-       :present #{:description :position :collection_id :collection_position}
-       :non-nil #{:name :parameters :caveats :points_of_interest :show_in_getting_started :enable_embedding
-                  :embedding_params :archived})))
+    (check-allowed-to-change-embedding dash-before-update dash-updates)
+    (api/check-500
+     (db/transaction
+
+       ;;If the dashboard has an updated position, or if the dashboard is moving to a new collection, we might need to
+       ;;adjust the collection position of other dashboards in the collection
+       (api/maybe-reconcile-collection-position! dash-before-update dash-updates)
+
+       (db/update! Dashboard id
+         ;; description, position, collection_id, and collection_position are allowed to be `nil`. Everything else must be
+         ;; non-nil
+         (u/select-keys-when dash-updates
+           :present #{:description :position :collection_id :collection_position}
+           :non-nil #{:name :parameters :caveats :points_of_interest :show_in_getting_started :enable_embedding
+                      :embedding_params :archived})))))
   ;; now publish an event and return the updated Dashboard
   (u/prog1 (Dashboard id)
     (events/publish-event! :dashboard-update (assoc <> :actor_id api/*current-user-id*))))

src/metabase/api/pulse.clj

@@ -55,14 +55,18 @@
   (check-card-read-permissions cards)
   ;; if we're trying to create this Pulse inside a Collection, make sure we have write permissions for that collection
   (collection/check-write-perms-for-collection collection_id)
-  ;; ok, now create the Pulse
-  (api/check-500
-   (pulse/create-pulse! (map pulse/card->ref cards) channels
-     {:name                name
-      :creator_id          api/*current-user-id*
-      :skip_if_empty       skip_if_empty
-      :collection_id       collection_id
-      :collection_position collection_position})))
+  (let [pulse-data {:name                name
+                    :creator_id          api/*current-user-id*
+                    :skip_if_empty       skip_if_empty
+                    :collection_id       collection_id
+                    :collection_position collection_position}]
+    (db/transaction
+      ;; Adding a new pulse at `collection_position` could cause other pulses in this collection to change position,
+      ;; check that and fix it if needed
+      (api/maybe-reconcile-collection-position! pulse-data)
+      ;; ok, now create the Pulse
+      (api/check-500
+       (pulse/create-pulse! (map pulse/card->ref cards) channels pulse-data)))))
 
 
 (api/defendpoint GET "/:id"
@@ -82,11 +86,16 @@
   ;; do various perms checks
   (let [pulse-before-update (api/write-check Pulse id)]
     (check-card-read-permissions cards)
-    (collection/check-allowed-to-change-collection pulse-before-update pulse-updates))
-  ;; ok, now update the Pulse
-  (pulse/update-pulse!
-   (assoc (select-keys pulse-updates [:name :cards :channels :skip_if_empty :collection_id :collection_position])
-     :id id))
+    (collection/check-allowed-to-change-collection pulse-before-update pulse-updates)
+
+    (db/transaction
+      ;; If the collection or position changed with this update, we might need to fixup the old and/or new collection,
+      ;; depending on what changed.
+      (api/maybe-reconcile-collection-position! pulse-before-update pulse-updates)
+      ;; ok, now update the Pulse
+      (pulse/update-pulse!
+       (assoc (select-keys pulse-updates [:name :cards :channels :skip_if_empty :collection_id :collection_position])
+         :id id))))
   ;; return updated Pulse
   (pulse/retrieve-pulse id))
 

test/metabase/api/card_test.clj

@@ -14,6 +14,7 @@
              [card-favorite :refer [CardFavorite]]
              [collection :refer [Collection]]
              [database :refer [Database]]
+             [dashboard :refer [Dashboard]]
              [permissions :as perms]
              [permissions-group :as perms-group]
              [pulse :as pulse :refer [Pulse]]
@@ -570,6 +571,260 @@
      {:collection_position nil})
     (db/select-one-field :collection_position Card :id (u/get-id card))))
 
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                      UPDATING THE POSITION OF A CARDS                                          |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defn- name->position [results]
+  (zipmap (map :name results)
+          (map :collection_position results)))
+
+(defn get-name->collection-position
+  "Call the collection endpoint for `collection-id` as `user-kwd`. Will return a map with the names of the items as
+  keys and their position as the value"
+  [user-kwd collection-or-collection-id]
+  (name->position ((user->client user-kwd) :get 200 (format "collection/%s/items" (u/get-id collection-or-collection-id)))))
+
+(defmacro with-ordered-items
+  "Macro for creating many sequetial collection_position model instances, putting each in `collection`"
+  [collection model-and-name-syms & body]
+  `(tt/with-temp* ~(vec (mapcat (fn [idx [model-instance name-sym]]
+                                  [model-instance [name-sym {:name (name name-sym)
+                                                             :collection_id `(u/get-id ~collection)
+                                                             :collection_position idx}]])
+                                (iterate inc 1)
+                                (partition-all 2 model-and-name-syms)))
+     ~@body))
+
+;; Check to make sure we can move a card in a collection of just cards
+(expect
+  {"c" 1
+   "a" 2
+   "b" 3
+   "d" 4}
+  (tt/with-temp Collection [collection]
+    (with-ordered-items collection [Card a
+                                    Card b
+                                    Card c
+                                    Card d]
+      (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "card/" (u/get-id c))
+       {:collection_position 1})
+      (get-name->collection-position :rasta collection))))
+
+;; Change the position of the 4th card to 1st, all other cards should inc their position
+(expect
+  {"d" 1
+   "a" 2
+   "b" 3
+   "c" 4}
+  (tt/with-temp Collection [collection]
+    (with-ordered-items collection [Dashboard a
+                                    Dashboard b
+                                    Pulse     c
+                                    Card      d]
+      (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "card/" (u/get-id d))
+       {:collection_position 1})
+      (get-name->collection-position :rasta collection))))
+
+;; Change the position of the 1st card to the 4th, all of the other items dec
+(expect
+  {"b" 1
+   "c" 2
+   "d" 3
+   "a" 4}
+  (tt/with-temp Collection [collection]
+    (with-ordered-items collection [Card      a
+                                    Dashboard b
+                                    Pulse     c
+                                    Dashboard d]
+      (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "card/" (u/get-id a))
+       {:collection_position 4})
+      (get-name->collection-position :rasta collection))))
+
+;; Change the position of a card from nil to 2nd, should adjust the existing items
+(expect
+  {"a" 1
+   "b" 2
+   "c" 3
+   "d" 4}
+  (tt/with-temp* [Collection [{coll-id :id :as collection}]
+                  Card       [_ {:name "a", :collection_id coll-id, :collection_position 1}]
+                  ;; Card b does not start with a collection_position
+                  Card       [b {:name "b", :collection_id coll-id}]
+                  Dashboard  [_ {:name "c", :collection_id coll-id, :collection_position 2}]
+                  Card       [_ {:name "d", :collection_id coll-id, :collection_position 3}]]
+    (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+    ((user->client :rasta) :put 200 (str "card/" (u/get-id b))
+     {:collection_position 2})
+    (get-name->collection-position :rasta coll-id)))
+
+;; Update an existing card to no longer have a position, should dec items after it's position
+(expect
+  {"a" 1
+   "b" nil
+   "c" 2
+   "d" 3}
+  (tt/with-temp Collection [collection]
+    (with-ordered-items collection [Card      a
+                                    Card      b
+                                    Dashboard c
+                                    Pulse     d]
+      (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "card/" (u/get-id b))
+       {:collection_position nil})
+      (get-name->collection-position :rasta collection))))
+
+;; Change the collection the card is in, leave the position, should cause old and new collection to have their
+;; positions updated
+(expect
+  [{"a" 1
+    "f" 2
+    "b" 3
+    "c" 4
+    "d" 5}
+   {"e" 1
+    "g" 2
+    "h" 3}]
+  (tt/with-temp* [Collection [collection-1]
+                  Collection [collection-2]]
+    (with-ordered-items collection-1 [Dashboard a
+                                      Card      b
+                                      Pulse     c
+                                      Dashboard d]
+      (with-ordered-items collection-2 [Pulse     e
+                                        Card      f
+                                        Card      g
+                                        Dashboard h]
+        (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-1)
+        (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-2)
+        ((user->client :rasta) :put 200 (str "card/" (u/get-id f))
+         {:collection_id (u/get-id collection-1)})
+        [(get-name->collection-position :rasta collection-1)
+         (get-name->collection-position :rasta collection-2)]))))
+
+;; Change the collection and the position, causing both collections and the updated card to have their order changed
+(expect
+  [{"h" 1
+    "a" 2
+    "b" 3
+    "c" 4
+    "d" 5}
+   {"e" 1
+    "f" 2
+    "g" 3}]
+  (tt/with-temp* [Collection [collection-1]
+                  Collection [collection-2]]
+    (with-ordered-items collection-1 [Pulse     a
+                                      Pulse     b
+                                      Dashboard c
+                                      Dashboard d]
+      (with-ordered-items collection-2 [Dashboard e
+                                        Dashboard f
+                                        Pulse     g
+                                        Card      h]
+        (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-1)
+        (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-2)
+        ((user->client :rasta) :put 200 (str "card/" (u/get-id h))
+         {:collection_position 1, :collection_id (u/get-id collection-1)})
+        [(get-name->collection-position :rasta collection-1)
+         (get-name->collection-position :rasta collection-2)]))))
+
+;; Add a new card to an existing collection at position 1, will cause all existing positions to increment by 1
+(expect
+  ;; Original collection, before adding the new card
+  [{"b" 1
+    "c" 2
+    "d" 3}
+   ;; Add new card at index 1
+   {"a" 1
+    "b" 2
+    "c" 3
+    "d" 4}]
+  (tt/with-temp Collection [collection]
+    (tu/with-model-cleanup [Card]
+      (with-ordered-items collection [Dashboard b
+                                      Pulse     c
+                                      Card      d]
+        (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+        [(get-name->collection-position :rasta collection)
+         (do
+           ((user->client :rasta) :post 200 "card"
+            (merge (card-with-name-and-query "a")
+                   {:collection_id       (u/get-id collection)
+                    :collection_position 1}))
+           (get-name->collection-position :rasta collection))]))))
+
+;; Add a new card to the end of an existing collection
+(expect
+  ;; Original collection, before adding the new card
+  [{"a" 1
+    "b" 2
+    "c" 3}
+   ;; Add new card at index 4
+   {"a" 1
+    "b" 2
+    "c" 3
+    "d" 4}]
+  (tt/with-temp Collection [collection]
+    (tu/with-model-cleanup [Card]
+      (with-ordered-items collection [Card      a
+                                      Dashboard b
+                                      Pulse     c]
+        (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+        [(get-name->collection-position :rasta collection)
+         (do
+           ((user->client :rasta) :post 200 "card"
+            (merge (card-with-name-and-query "d")
+                   {:collection_id (u/get-id collection)
+                    :collection_position 4}))
+           (get-name->collection-position :rasta collection))]))))
+
+;; When adding a new card to a collection that does not have a position, it should not change existing positions
+(expect
+  ;; Original collection, before adding the new card
+  [{"a" 1
+    "b" 2
+    "c" 3}
+   ;; Add new card without a position
+   {"a" 1
+    "b" 2
+    "c" 3
+    "d" nil}]
+  (tt/with-temp Collection [collection]
+    (tu/with-model-cleanup [Card]
+      (with-ordered-items collection [Pulse     a
+                                      Card      b
+                                      Dashboard c]
+        (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+        [(get-name->collection-position :rasta collection)
+         (do
+           ((user->client :rasta) :post 200 "card"
+            (merge (card-with-name-and-query "d")
+                   {:collection_id       (u/get-id collection)
+                    :collection_position nil}))
+           (get-name->collection-position :rasta collection))]))))
+
+(expect
+  {"d" 1
+   "a" 2
+   "b" 3
+   "c" 4
+   "e" 5
+   "f" 6}
+  (tt/with-temp Collection [collection]
+    (with-ordered-items collection [Dashboard a
+                                    Dashboard b
+                                    Card      c
+                                    Card      d
+                                    Pulse     e
+                                    Pulse     f]
+      (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "card/" (u/get-id d))
+       {:collection_position 1, :collection_id (u/get-id collection)})
+      (name->position ((user->client :rasta) :get 200 (format "collection/%s/items" (u/get-id collection)))))))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                        Card updates that impact alerts                                         |
@@ -1035,6 +1290,46 @@
     (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
     (POST-card-collections! :rasta 403 collection [card-1 card-2])))
 
+;; Test that we can bulk move some Cards from one collection to another, while updating the collection position of the
+;; old collection and the new collection
+(expect
+  [{:response    {:status "ok"}
+    :collections ["New Collection" "New Collection"]}
+   {"a" 4 ;-> Moved to the new collection, gets the first slot available
+    "b" 5
+    "c" 1 ;-> With a and b no longer in the collection, c is first
+    "d" 1 ;-> Existing cards in new collection are untouched and position unchanged
+    "e" 2
+    "f" 3}]
+  (tt/with-temp* [Collection [{coll-id-1 :id}      {:name "Old Collection"}]
+                  Collection [{coll-id-2 :id
+                               :as new-collection} {:name "New Collection"}]
+                  Card       [card-a               {:name "a", :collection_id coll-id-1, :collection_position 1}]
+                  Card       [card-b               {:name "b", :collection_id coll-id-1, :collection_position 2}]
+                  Card       [card-c               {:name "c", :collection_id coll-id-1, :collection_position 3}]
+                  Card       [card-d               {:name "d", :collection_id coll-id-2, :collection_position 1}]
+                  Card       [card-e               {:name "e", :collection_id coll-id-2, :collection_position 2}]
+                  Card       [card-f               {:name "f", :collection_id coll-id-2, :collection_position 3}]]
+    [(POST-card-collections! :crowberto 200 new-collection [card-a card-b])
+     (merge (name->position ((user->client :crowberto) :get 200 (format "collection/%s/items" coll-id-1)  :model "card" :archived "false"))
+            (name->position ((user->client :crowberto) :get 200 (format "collection/%s/items" coll-id-2)  :model "card" :archived "false")))]))
+
+;; Moving a card without a collection_position keeps the collection_position nil
+(expect
+  [{:response    {:status "ok"}
+    :collections ["New Collection" "New Collection"]}
+   {"a" nil
+    "b" 1
+    "c" 2}]
+  (tt/with-temp* [Collection [{coll-id-1 :id}      {:name "Old Collection"}]
+                  Collection [{coll-id-2 :id
+                               :as new-collection} {:name "New Collection"}]
+                  Card       [card-a               {:name "a", :collection_id coll-id-1}]
+                  Card       [card-b               {:name "b", :collection_id coll-id-2, :collection_position 1}]
+                  Card       [card-c               {:name "c", :collection_id coll-id-2, :collection_position 2}]]
+    [(POST-card-collections! :crowberto 200 new-collection [card-a card-b])
+     (merge (name->position ((user->client :crowberto) :get 200 (format "collection/%s/items" coll-id-1)  :model "card" :archived "false"))
+            (name->position ((user->client :crowberto) :get 200 (format "collection/%s/items" coll-id-2)  :model "card" :archived "false")))]))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                            PUBLIC SHARING ENDPOINTS                                            |

test/metabase/api/dashboard_test.clj

@@ -18,6 +18,7 @@
              [dashboard-test :as dashboard-test]
              [permissions :as perms]
              [permissions-group :as group]
+             [pulse :refer [Pulse]]
              [revision :refer [Revision]]]
             [metabase.test.data.users :refer :all]
             [metabase.test.util :as tu]
@@ -351,6 +352,164 @@
      {:collection_position nil})
     (db/select-one-field :collection_position Dashboard :id (u/get-id dashboard))))
 
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                              UPDATING DASHBOARD COLLECTION POSITIONS                                           |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+;; Check that we can update a dashboard's position in a collection of only dashboards
+(expect
+  {"a" 1
+   "c" 2
+   "d" 3
+   "b" 4}
+  (tt/with-temp Collection [collection]
+    (card-api-test/with-ordered-items collection [Dashboard a
+                                                  Dashboard b
+                                                  Dashboard c
+                                                  Dashboard d]
+      (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id b))
+       {:collection_position 4})
+      (card-api-test/get-name->collection-position :rasta collection))))
+
+;; Check that updating a dashboard at position 3 to position 1 will increment the positions before 3, not after
+(expect
+  {"c" 1
+   "a" 2
+   "b" 3
+   "d" 4}
+  (tt/with-temp Collection [collection]
+    (card-api-test/with-ordered-items collection [Card      a
+                                                  Pulse     b
+                                                  Dashboard c
+                                                  Dashboard d]
+      (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id c))
+       {:collection_position 1})
+      (card-api-test/get-name->collection-position :rasta collection))))
+
+;; Check that updating position 1 to 3 will cause b and c to be decremented
+(expect
+  {"b" 1
+   "c" 2
+   "a" 3
+   "d" 4}
+  (tt/with-temp Collection [collection]
+    (card-api-test/with-ordered-items collection [Dashboard a
+                                                  Card      b
+                                                  Pulse     c
+                                                  Dashboard d]
+      (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id a))
+       {:collection_position 3})
+      (card-api-test/get-name->collection-position :rasta collection))))
+
+;; Check that updating position 1 to 4 will cause a through c to be decremented
+(expect
+  {"b" 1
+   "c" 2
+   "d" 3
+   "a" 4}
+  (tt/with-temp Collection [collection]
+    (card-api-test/with-ordered-items collection [Dashboard a
+                                                  Card      b
+                                                  Pulse     c
+                                                  Pulse     d]
+      (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id a))
+       {:collection_position 4})
+      (card-api-test/get-name->collection-position :rasta collection))))
+
+;; Check that updating position 4 to 1 will cause a through c to be incremented
+(expect
+  {"d" 1
+   "a" 2
+   "b" 3
+   "c" 4}
+  (tt/with-temp Collection [collection]
+    (card-api-test/with-ordered-items collection [Card      a
+                                                  Pulse     b
+                                                  Card      c
+                                                  Dashboard d]
+      (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id d))
+       {:collection_position 1})
+      (card-api-test/get-name->collection-position :rasta collection))))
+
+;; Check that moving a dashboard to another collection will fixup both collections
+(expect
+  [{"b" 1
+    "c" 2
+    "d" 3}
+   {"a" 1
+    "e" 2
+    "f" 3
+    "g" 4
+    "h" 5}]
+  (tt/with-temp* [Collection [collection-1]
+                  Collection [collection-2]]
+    (card-api-test/with-ordered-items collection-1 [Dashboard a
+                                                    Card      b
+                                                    Card      c
+                                                    Pulse     d]
+      (card-api-test/with-ordered-items collection-2 [Pulse     e
+                                                      Pulse     f
+                                                      Dashboard g
+                                                      Card      h]
+        (perms/grant-collection-readwrite-permissions! (group/all-users) collection-1)
+        (perms/grant-collection-readwrite-permissions! (group/all-users) collection-2)
+        ;; Move the first dashboard in collection-1 to collection-1
+        ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id a))
+         {:collection_position 1, :collection_id (u/get-id collection-2)})
+        ;; "a" should now be gone from collection-1 and all the existing dashboards bumped down in position
+        [(card-api-test/get-name->collection-position :rasta collection-1)
+         ;; "a" is now first, all other dashboards in collection-2 bumped down 1
+         (card-api-test/get-name->collection-position :rasta collection-2)]))))
+
+;; Check that adding a new card at position 3 will cause the existing card at 3 to be incremented
+(expect
+  [{"a" 1
+    "b" 2
+    "d" 3}
+   {"a" 1
+    "b" 2
+    "c" 3
+    "d" 4}]
+  (tt/with-temp Collection [collection]
+    (tu/with-model-cleanup [Dashboard]
+      (card-api-test/with-ordered-items collection [Card  a
+                                                    Pulse b
+                                                    Card  d]
+        (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+        [(card-api-test/get-name->collection-position :rasta collection)
+         (do
+           ((user->client :rasta) :post 200 "dashboard" {:name "c"
+                                                         :parameters          [{}]
+                                                         :collection_id       (u/get-id collection)
+                                                         :collection_position 3})
+           (card-api-test/get-name->collection-position :rasta collection))]))))
+
+;; Check that adding a new card without a position, leaves the existing positions unchanged
+(expect
+  [{"a" 1
+    "b" 2
+    "d" 3}
+   {"a" 1
+    "b" 2
+    "c" nil
+    "d" 3}]
+  (tt/with-temp Collection [collection]
+    (tu/with-model-cleanup [Dashboard]
+      (card-api-test/with-ordered-items collection [Dashboard a
+                                                    Card      b
+                                                    Pulse     d]
+        (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+        [(card-api-test/get-name->collection-position :rasta collection)
+         (do
+           ((user->client :rasta) :post 200 "dashboard" {:name "c"
+                                                         :parameters          [{}]
+                                                         :collection_id       (u/get-id collection)})
+           (card-api-test/get-name->collection-position :rasta collection))]))))
 
 
 ;;; +----------------------------------------------------------------------------------------------------------------+

test/metabase/api/pulse_test.clj

@@ -10,6 +10,7 @@
             [metabase.models
              [card :refer [Card]]
              [collection :refer [Collection]]
+             [dashboard :refer [Dashboard]]
              [database :refer [Database]]
              [permissions :as perms]
              [permissions-group :as perms-group]
@@ -418,6 +419,200 @@
      {:collection_position nil})
     (db/select-one-field :collection_position Pulse :id (u/get-id pulse))))
 
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                   UPDATING PULSE COLLECTION POSITIONS                                          |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+;; Check that we can update a pulse's position in a collection only pulses
+(expect
+  {"d" 1
+   "a" 2
+   "b" 3
+   "c" 4}
+  (tt/with-temp Collection [{coll-id :id :as collection}]
+    (card-api-test/with-ordered-items collection [Pulse a
+                                                  Pulse b
+                                                  Pulse c
+                                                  Pulse d]
+      (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "pulse/" (u/get-id d))
+       {:collection_position 1})
+      (card-api-test/get-name->collection-position :rasta coll-id))))
+
+;; Change the position of b to 4, will dec c and d
+(expect
+  {"a" 1
+   "c" 2
+   "d" 3
+   "b" 4}
+  (tt/with-temp Collection [{coll-id :id :as collection}]
+    (card-api-test/with-ordered-items collection [Card      a
+                                                  Pulse     b
+                                                  Card      c
+                                                  Dashboard d]
+      (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "pulse/" (u/get-id b))
+       {:collection_position 4})
+      (card-api-test/get-name->collection-position :rasta coll-id))))
+
+;; Change the position of d to the 2, should inc b and c
+(expect
+  {"a" 1
+   "d" 2
+   "b" 3
+   "c" 4}
+  (tt/with-temp Collection [{coll-id :id :as collection}]
+    (card-api-test/with-ordered-items collection [Card      a
+                                                  Card      b
+                                                  Dashboard c
+                                                  Pulse     d]
+      (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "pulse/" (u/get-id d))
+       {:collection_position 2})
+      (card-api-test/get-name->collection-position :rasta coll-id))))
+
+;; Change the position of a to the 4th, will decrement all existing items
+(expect
+  {"b" 1
+   "c" 2
+   "d" 3
+   "a" 4}
+  (tt/with-temp Collection [{coll-id :id :as collection}]
+    (card-api-test/with-ordered-items collection [Pulse     a
+                                                  Dashboard b
+                                                  Card      c
+                                                  Pulse     d]
+      (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "pulse/" (u/get-id a))
+       {:collection_position 4})
+      (card-api-test/get-name->collection-position :rasta coll-id))))
+
+;; Change the position of the d to the 1st, will increment all existing items
+(expect
+  {"d" 1
+   "a" 2
+   "b" 3
+   "c" 4}
+  (tt/with-temp Collection [{coll-id :id :as collection}]
+    (card-api-test/with-ordered-items collection [Dashboard a
+                                                  Dashboard b
+                                                  Card      c
+                                                  Pulse     d]
+      (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+      ((user->client :rasta) :put 200 (str "pulse/" (u/get-id d))
+       {:collection_position 1})
+      (card-api-test/get-name->collection-position :rasta coll-id))))
+
+;; Check that no position change, but changing collections still triggers a fixup of both collections
+;; Moving `c` from collection-1 to collection-2, `c` is now at position 3 in collection 2
+(expect
+  [{"a" 1
+    "b" 2
+    "d" 3}
+   {"e" 1
+    "f" 2
+    "c" 3
+    "g" 4
+    "h" 5}]
+  (tt/with-temp* [Collection [collection-1]
+                  Collection [collection-2]]
+    (card-api-test/with-ordered-items collection-1 [Pulse     a
+                                                    Card      b
+                                                    Pulse     c
+                                                    Dashboard d]
+      (card-api-test/with-ordered-items collection-2 [Card      e
+                                                      Card      f
+                                                      Dashboard g
+                                                      Dashboard h]
+        (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-1)
+        (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-2)
+        ((user->client :rasta) :put 200 (str "pulse/" (u/get-id c))
+         {:collection_id (u/get-id collection-2)})
+        [(card-api-test/get-name->collection-position :rasta (u/get-id collection-1))
+         (card-api-test/get-name->collection-position :rasta (u/get-id collection-2))]))))
+
+;; Check that moving a pulse to another collection, with a changed position will fixup both collections
+;; Moving `b` to collection 2, giving it a position of 1
+(expect
+  [{"a" 1
+    "c" 2
+    "d" 3}
+   {"b" 1
+    "e" 2
+    "f" 3
+    "g" 4
+    "h" 5}]
+  (tt/with-temp* [Collection [collection-1]
+                  Collection [collection-2]]
+    (card-api-test/with-ordered-items collection-1 [Pulse     a
+                                                    Pulse     b
+                                                    Dashboard c
+                                                    Card      d]
+      (card-api-test/with-ordered-items collection-2 [Card      e
+                                                      Card      f
+                                                      Pulse     g
+                                                      Dashboard h]
+        (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-1)
+        (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-2)
+        ((user->client :rasta) :put 200 (str "pulse/" (u/get-id b))
+         {:collection_id (u/get-id collection-2), :collection_position 1})
+        [(card-api-test/get-name->collection-position :rasta (u/get-id collection-1))
+         (card-api-test/get-name->collection-position :rasta (u/get-id collection-2))]))))
+
+;; Add a new pulse at position 2, causing existing pulses to be incremented
+(expect
+  [{"a" 1
+    "c" 2
+    "d" 3}
+   {"a" 1
+    "b" 2
+    "c" 3
+    "d" 4}]
+  (tt/with-temp* [Collection [{coll-id :id :as collection}]
+                  Card       [card-1]]
+    (card-api-test/with-cards-in-readable-collection [card-1]
+      (card-api-test/with-ordered-items  collection [Card      a
+                                                     Dashboard c
+                                                     Pulse     d]
+        (tu/with-model-cleanup [Pulse]
+          (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+          [(card-api-test/get-name->collection-position :rasta coll-id)
+           (do ((user->client :rasta) :post 200 "pulse" {:name                "b"
+                                                         :collection_id       (u/get-id collection)
+                                                         :cards               [{:id          (u/get-id card-1)
+                                                                                :include_csv false
+                                                                                :include_xls false}]
+                                                         :channels            [daily-email-channel]
+                                                         :skip_if_empty       false
+                                                         :collection_position 2})
+               (card-api-test/get-name->collection-position :rasta coll-id))])))))
+
+;; Add a new pulse without a position, should leave existing positions unchanged
+(expect
+  [{"a" 1
+    "c" 2
+    "d" 3}
+   {"a" 1
+    "b" nil
+    "c" 2
+    "d" 3}]
+  (tt/with-temp* [Collection [{coll-id :id :as collection}]
+                  Card       [card-1]]
+    (card-api-test/with-cards-in-readable-collection [card-1]
+      (card-api-test/with-ordered-items collection [Pulse     a
+                                                    Card      c
+                                                    Dashboard d]
+        (tu/with-model-cleanup [Pulse]
+          (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+          [(card-api-test/get-name->collection-position :rasta coll-id)
+           (do ((user->client :rasta) :post 200 "pulse" {:name                "b"
+                                                         :collection_id       (u/get-id collection)
+                                                         :cards               [{:id          (u/get-id card-1)
+                                                                                :include_csv false
+                                                                                :include_xls false}]
+                                                         :channels            [daily-email-channel]
+                                                         :skip_if_empty       false})
+               (card-api-test/get-name->collection-position :rasta coll-id))])))))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                             DELETE /api/pulse/:id                                              |

test/metabase/api/search_test.clj

@@ -226,4 +226,7 @@
                                   :alert_first_only false
                                   :alert_above_goal nil
                                   :name             nil}]]
-      (filter #(= (u/get-id pulse) (:id %)) ((user->client :crowberto) :get 200 "search")))))
+      (filter (fn [{:keys [model id]}]
+                (and (= id (u/get-id pulse))
+                     (= "pulse" model)))
+              ((user->client :crowberto) :get 200 "search")))))