Update order when deleting bundle (#1114)

bookmarks/api/routes.py

@@ -26,7 +26,7 @@ from bookmarks.models import (
     User,
     BookmarkBundle,
 )
-from bookmarks.services import assets, bookmarks, auto_tagging, website_loader
+from bookmarks.services import assets, bookmarks, bundles, auto_tagging, website_loader
 from bookmarks.type_defs import HttpRequest
 from bookmarks.views import access
 
@@ -290,6 +290,9 @@ class BookmarkBundleViewSet(
     def get_serializer_context(self):
         return {"user": self.request.user}
 
+    def perform_destroy(self, instance):
+        bundles.delete_bundle(instance)
+
 
 # DRF routers do not support nested view sets such as /bookmarks/<id>/assets/<id>/
 # Instead create separate routers for each view set and manually register them in urls.py

bookmarks/api/serializers.py

@@ -11,7 +11,7 @@ from bookmarks.models import (
     UserProfile,
     BookmarkBundle,
 )
-from bookmarks.services import bookmarks
+from bookmarks.services import bookmarks, bundles
 from bookmarks.services.tags import get_or_create_tag
 from bookmarks.services.wayback import generate_fallback_webarchive_url
 from bookmarks.utils import app_version
@@ -55,17 +55,9 @@ class BookmarkBundleSerializer(serializers.ModelSerializer):
         ]
 
     def create(self, validated_data):
-        # Set owner to the authenticated user
+        bundle = BookmarkBundle(**validated_data)
-        validated_data["owner"] = self.context["user"]
+        bundle.order = validated_data["order"] if "order" in validated_data else None
-
+        return bundles.create_bundle(bundle, self.context["user"])
-        # Set order to the next available position if not provided
-        if "order" not in validated_data:
-            max_order = BookmarkBundle.objects.filter(
-                owner=self.context["user"]
-            ).aggregate(Max("order", default=-1))["order__max"]
-            validated_data["order"] = max_order + 1
-
-        return super().create(validated_data)
 
 
 class BookmarkSerializer(serializers.ModelSerializer):

bookmarks/services/bundles.py

@@ -0,0 +1,37 @@
+from django.db.models import Max
+
+from bookmarks.models import BookmarkBundle, User
+
+
+def create_bundle(bundle: BookmarkBundle, current_user: User):
+    bundle.owner = current_user
+    if bundle.order is None:
+        max_order_result = BookmarkBundle.objects.filter(owner=current_user).aggregate(
+            Max("order", default=-1)
+        )
+        bundle.order = max_order_result["order__max"] + 1
+    bundle.save()
+    return bundle
+
+
+def move_bundle(bundle_to_move: BookmarkBundle, new_order: int):
+    user_bundles = list(
+        BookmarkBundle.objects.filter(owner=bundle_to_move.owner).order_by("order")
+    )
+
+    if new_order != user_bundles.index(bundle_to_move):
+        user_bundles.remove(bundle_to_move)
+        user_bundles.insert(new_order, bundle_to_move)
+        for bundle_index, bundle in enumerate(user_bundles):
+            bundle.order = bundle_index
+
+        BookmarkBundle.objects.bulk_update(user_bundles, ["order"])
+
+
+def delete_bundle(bundle: BookmarkBundle):
+    bundle.delete()
+
+    user_bundles = BookmarkBundle.objects.filter(owner=bundle.owner).order_by("order")
+    for index, user_bundle in enumerate(user_bundles):
+        user_bundle.order = index
+    BookmarkBundle.objects.bulk_update(user_bundles, ["order"])

bookmarks/tests/test_bundles_api.py

@@ -269,6 +269,24 @@ class BundlesApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin):
 
         self.assertFalse(BookmarkBundle.objects.filter(id=bundle.id).exists())
 
+    def test_delete_bundle_updates_order(self):
+        self.authenticate()
+
+        bundle1 = self.setup_bundle(name="Bundle 1", order=0)
+        bundle2 = self.setup_bundle(name="Bundle 2", order=1)
+        bundle3 = self.setup_bundle(name="Bundle 3", order=2)
+
+        url = reverse("linkding:bundle-detail", kwargs={"pk": bundle2.id})
+        self.delete(url, expected_status_code=status.HTTP_204_NO_CONTENT)
+
+        self.assertFalse(BookmarkBundle.objects.filter(id=bundle2.id).exists())
+
+        # Check that the remaining bundles have updated orders
+        bundle1.refresh_from_db()
+        bundle3.refresh_from_db()
+        self.assertEqual(bundle1.order, 0)
+        self.assertEqual(bundle3.order, 1)
+
     def test_delete_bundle_only_allows_own_bundles(self):
         self.authenticate()
 

bookmarks/tests/test_bundles_index_view.py

@@ -100,6 +100,18 @@ class BundleIndexViewTestCase(TestCase, BookmarkFactoryMixin):
 
         self.assertFalse(BookmarkBundle.objects.filter(id=bundle.id).exists())
 
+    def test_remove_bundle_updates_order(self):
+        bundle1 = self.setup_bundle(name="Bundle 1", order=0)
+        bundle2 = self.setup_bundle(name="Bundle 2", order=1)
+        bundle3 = self.setup_bundle(name="Bundle 3", order=2)
+
+        self.client.post(
+            reverse("linkding:bundles.action"),
+            {"remove_bundle": str(bundle2.id)},
+        )
+
+        self.assertBundleOrder([bundle1, bundle3])
+
     def test_remove_other_user_bundle(self):
         other_user = self.setup_user(name="otheruser")
         other_user_bundle = self.setup_bundle(name="Other User Bundle", user=other_user)

bookmarks/tests/test_settings_export_view.py

@@ -79,10 +79,12 @@ class SettingsExportViewTestCase(TestCase, BookmarkFactoryMixin):
 
     def test_filename_includes_date_and_time(self):
         self.setup_bookmark()
-        
+
         # Mock timezone.now to return a fixed datetime for predictable filename
-        fixed_time = datetime.datetime(2023, 5, 15, 14, 30, 45, tzinfo=datetime.timezone.utc)
+        fixed_time = datetime.datetime(
-        
+            2023, 5, 15, 14, 30, 45, tzinfo=datetime.timezone.utc
+        )
+
         with patch("bookmarks.views.settings.timezone.now", return_value=fixed_time):
             response = self.client.get(reverse("linkding:settings.export"), follow=True)
 

bookmarks/views/bundles.py

@@ -1,11 +1,11 @@
 from django.contrib import messages
 from django.contrib.auth.decorators import login_required
-from django.db.models import Max
 from django.http import HttpRequest, HttpResponseRedirect
 from django.shortcuts import render
 from django.urls import reverse
 
 from bookmarks.models import BookmarkBundle, BookmarkBundleForm, BookmarkSearch
+from bookmarks.services import bundles
 from bookmarks.views import access
 from bookmarks.views.contexts import ActiveBookmarkListContext
 
@@ -23,24 +23,14 @@ def action(request: HttpRequest):
         remove_bundle_id = request.POST.get("remove_bundle")
         bundle = access.bundle_write(request, remove_bundle_id)
         bundle_name = bundle.name
-        bundle.delete()
+        bundles.delete_bundle(bundle)
         messages.success(request, f"Bundle '{bundle_name}' removed successfully.")
 
     elif "move_bundle" in request.POST:
         bundle_id = request.POST.get("move_bundle")
-        move_position = int(request.POST.get("move_position"))
         bundle_to_move = access.bundle_write(request, bundle_id)
-        user_bundles = list(
+        move_position = int(request.POST.get("move_position"))
-            BookmarkBundle.objects.filter(owner=request.user).order_by("order")
+        bundles.move_bundle(bundle_to_move, move_position)
-        )
-
-        if move_position != user_bundles.index(bundle_to_move):
-            user_bundles.remove(bundle_to_move)
-            user_bundles.insert(move_position, bundle_to_move)
-            for bundle_index, bundle in enumerate(user_bundles):
-                bundle.order = bundle_index
-
-            BookmarkBundle.objects.bulk_update(user_bundles, ["order"])
 
     return HttpResponseRedirect(reverse("linkding:bundles.index"))
 
@@ -52,15 +42,13 @@ def _handle_edit(request: HttpRequest, template: str, bundle: BookmarkBundle = N
     if request.method == "POST":
         if form.is_valid():
             instance = form.save(commit=False)
-            instance.owner = request.user
 
-            if bundle is None:  # New bundle
+            if bundle is None:
-                max_order_result = BookmarkBundle.objects.filter(
+                instance.order = None
-                    owner=request.user
+                bundles.create_bundle(instance, request.user)
-                ).aggregate(Max("order", default=-1))
+            else:
-                instance.order = max_order_result["order__max"] + 1
+                instance.save()
 
-            instance.save()
             messages.success(request, "Bundle saved successfully.")
             return HttpResponseRedirect(reverse("linkding:bundles.index"))