Update positions of other items affected by moving/adding/removing

[IvanJelicSF]

Summary

Bulk patches on /content_lists/<id>/items now keep positions contiguous and unique when items are added, moved, or removed. Previously, only the directly-touched item's position was updated, which could leave gaps after deletes or produce duplicate positions on inserts/moves.

The new behavior in apps/content_lists/service.py:

• Add at position p: shift every non-sticky item with position >= p up by 1 before inserting.
• Move from old to new: items strictly between the two endpoints shift by 1 in the opposite direction; the moved item itself is excluded from the shift.
• Delete at position p: shift every non-sticky item with position > p down by 1.
• Sticky items (sticky: true) are excluded from all reordering.
• Items with position: None are handled — add/delete still works, and a move from/to None degrades to a one-sided shift.

Test plan

Four new behave scenarios in features/content_lists.feature exercise each path end-to-end via the bulk-patch endpoint and assert the final ordering:

• Move an item to an occupied position — other item shifts to keep positions unique
• Add an item at an occupied position — existing items shift down
• Move an item forward across several positions — items in between shift back by one
• Delete an item — items after it shift up to close the gap

Fixes: SDESK-7928

[Copilot]

Pull request overview

This PR updates the /content_lists/<id>/items bulk-patch behavior so that when items are added, moved, or deleted, the positions of other affected (non-sticky) items are adjusted to keep positions unique and to close gaps after deletes.

Changes:

• Add position-shifting logic to the content list items bulk patch service for add/move/delete operations (excluding sticky items).
• Introduce helper methods to shift positions up/down and to handle range shifts on moves.
• Add Behave scenarios covering add-at-position, move-to-occupied, move-forward, and delete-gap-closing behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
features/content_lists.feature	Adds end-to-end Behave scenarios asserting reordered positions after bulk-patch operations.
apps/content_lists/service.py	Implements MongoDB update_many-based position shifting during bulk patch add/move/delete.

Comments suppressed due to low confidence (2)

apps/content_lists/service.py:66

• In the add action, positions are shifted before attempting create(). If create() fails (e.g. content relation validation fails, or the unique (list_id, content) index rejects a duplicate), the list will be left with all later items shifted even though nothing was inserted. Consider creating first and then shifting other items (excluding the inserted _id), or running the shift+insert in a transaction / compensating rollback on error.

This issue also appears on line 67 of the same file.

            if action == "add":
                new_position = item_data.get("position")
                if new_position is not None:
                    await self._shift_positions_up(list_id, new_position)
                await self.create(
                    [
                        {
                            "list_id": list_id,
                            "content": content_id,
                            "position": new_position,
                            "sticky": item_data.get("sticky", False),
                            "sticky_position": item_data.get("stickyPosition"),
                            "enabled": True,
                        }
                    ]
                )

apps/content_lists/service.py:72

• In the move action, _shift_positions_for_move(...) is called before update(). If the subsequent update() fails validation (e.g. invalid position type) or raises a DB error, other items will already have been shifted, leaving the list in an inconsistent state. To avoid partial updates, consider updating the moved item first and then shifting others (excluding the moved _id), or wrapping both operations in a MongoDB transaction.

            elif action == "move":
                existing = await self.find_one(req=None, list_id=list_id, content=content_id)
                if existing:
                    new_position = item_data.get("position")
                    await self._shift_positions_for_move(list_id, existing.id, existing.position, new_position)
                    await self.update(existing.id, {"position": new_position})

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[eos87]

Looks good in general, just two concerns:

1. Atomicity: each action does a shift + a create/update/delete as two separate Mongo calls. If the second call fails after the shift succeeds, the list ends up in a broken state?

2. Sticky items: I see they are being excluded intentionally but, we need to make sure the sticky items get their position value properly set. Like: when an item becomes sticky, set its position to None. When an item becomes non-sticky, it needs a real position assigned (probably appended to the end, or supplied by the client). Thoughts?

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

apps/content_lists/service.py:121

• _renumber always rewrites non-sticky item positions into a contiguous 0..N-1 sequence. This changes the externally observable meaning of requests that set positions beyond the current list length (e.g. moving a single item to position 5 will be collapsed back to 0). If the API is expected to preserve the requested position (even if it creates gaps), this behavior is breaking; if the new behavior is intended, the API/tests/docs should be updated to reflect that out-of-range positions are clamped/normalized.

        ops = [
            UpdateOne({"_id": doc["_id"]}, {"$set": {"position": i}})
            for i, doc in enumerate(docs)
            if doc.get("position") != i
        ]
        if ops:
            await self.mongo_async.bulk_write(ops, ordered=False)