From a8623d11ef4bce4508e1016bbfdbed61b57a6989 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Tue, 1 Jul 2025 07:09:02 +0200 Subject: [PATCH] Update order when deleting bundle (#1114) --- bookmarks/api/routes.py | 5 ++- bookmarks/api/serializers.py | 16 +++------ bookmarks/services/bundles.py | 37 ++++++++++++++++++++ bookmarks/tests/test_bundles_api.py | 18 ++++++++++ bookmarks/tests/test_bundles_index_view.py | 12 +++++++ bookmarks/tests/test_settings_export_view.py | 8 +++-- bookmarks/views/bundles.py | 30 +++++----------- 7 files changed, 89 insertions(+), 37 deletions(-) create mode 100644 bookmarks/services/bundles.py diff --git a/bookmarks/api/routes.py b/bookmarks/api/routes.py index e20603d..dd87363 100644 --- a/bookmarks/api/routes.py +++ b/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//assets// # Instead create separate routers for each view set and manually register them in urls.py diff --git a/bookmarks/api/serializers.py b/bookmarks/api/serializers.py index 7acca98..ecc9b03 100644 --- a/bookmarks/api/serializers.py +++ b/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 - validated_data["owner"] = 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) + bundle = BookmarkBundle(**validated_data) + bundle.order = validated_data["order"] if "order" in validated_data else None + return bundles.create_bundle(bundle, self.context["user"]) class BookmarkSerializer(serializers.ModelSerializer): diff --git a/bookmarks/services/bundles.py b/bookmarks/services/bundles.py new file mode 100644 index 0000000..402723e --- /dev/null +++ b/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"]) diff --git a/bookmarks/tests/test_bundles_api.py b/bookmarks/tests/test_bundles_api.py index 3a14b01..2446f27 100644 --- a/bookmarks/tests/test_bundles_api.py +++ b/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() diff --git a/bookmarks/tests/test_bundles_index_view.py b/bookmarks/tests/test_bundles_index_view.py index 57d19f3..e4dff8e 100644 --- a/bookmarks/tests/test_bundles_index_view.py +++ b/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) diff --git a/bookmarks/tests/test_settings_export_view.py b/bookmarks/tests/test_settings_export_view.py index 4837025..9593bd9 100644 --- a/bookmarks/tests/test_settings_export_view.py +++ b/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) diff --git a/bookmarks/views/bundles.py b/bookmarks/views/bundles.py index 062db8d..52ae2a4 100644 --- a/bookmarks/views/bundles.py +++ b/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( - BookmarkBundle.objects.filter(owner=request.user).order_by("order") - ) - - 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"]) + move_position = int(request.POST.get("move_position")) + bundles.move_bundle(bundle_to_move, move_position) 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 - max_order_result = BookmarkBundle.objects.filter( - owner=request.user - ).aggregate(Max("order", default=-1)) - instance.order = max_order_result["order__max"] + 1 + if bundle is None: + instance.order = None + bundles.create_bundle(instance, request.user) + else: + instance.save() - instance.save() messages.success(request, "Bundle saved successfully.") return HttpResponseRedirect(reverse("linkding:bundles.index"))