From d1f81fee0e06700f5a9da46e55dff9717cc789aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Tue, 24 Sep 2024 13:13:32 +0200 Subject: [PATCH] Prevent duplicates when editing (#853) * prevent creating duplicate URLs on edit * Prevent duplicates when editing --- bookmarks/api/serializers.py | 18 ++++++++++++ bookmarks/models.py | 18 ++++++++++++ bookmarks/tests/test_bookmark_edit_view.py | 34 ++++++++++++++++++++++ bookmarks/tests/test_bookmarks_api.py | 23 +++++++++++++++ docs/src/content/docs/api.md | 8 ++--- 5 files changed, 97 insertions(+), 4 deletions(-) diff --git a/bookmarks/api/serializers.py b/bookmarks/api/serializers.py index 37a7bb0..153341e 100644 --- a/bookmarks/api/serializers.py +++ b/bookmarks/api/serializers.py @@ -111,6 +111,24 @@ class BookmarkSerializer(serializers.ModelSerializer): return update_bookmark(instance, tag_string, self.context["user"]) + def validate(self, attrs): + # When creating a bookmark, the service logic prevents duplicate URLs by + # updating the existing bookmark instead. When editing a bookmark, + # there is no assumption that it would update a different bookmark if + # the URL is a duplicate, so raise a validation error in that case. + if self.instance and "url" in attrs: + is_duplicate = ( + Bookmark.objects.filter(owner=self.instance.owner, url=attrs["url"]) + .exclude(pk=self.instance.pk) + .exists() + ) + if is_duplicate: + raise serializers.ValidationError( + {"url": "A bookmark with this URL already exists."} + ) + + return attrs + class TagSerializer(serializers.ModelSerializer): class Meta: diff --git a/bookmarks/models.py b/bookmarks/models.py index e65c225..2b94cfa 100644 --- a/bookmarks/models.py +++ b/bookmarks/models.py @@ -168,6 +168,24 @@ class BookmarkForm(forms.ModelForm): self.instance and self.instance.notes ) + def clean_url(self): + # When creating a bookmark, the service logic prevents duplicate URLs by + # updating the existing bookmark instead, which is also communicated in + # the form's UI. When editing a bookmark, there is no assumption that + # it would update a different bookmark if the URL is a duplicate, so + # raise a validation error in that case. + url = self.cleaned_data["url"] + if self.instance.pk: + is_duplicate = ( + Bookmark.objects.filter(owner=self.instance.owner, url=url) + .exclude(pk=self.instance.pk) + .exists() + ) + if is_duplicate: + raise forms.ValidationError("A bookmark with this URL already exists.") + + return url + class BookmarkSearch: SORT_ADDED_ASC = "added_asc" diff --git a/bookmarks/tests/test_bookmark_edit_view.py b/bookmarks/tests/test_bookmark_edit_view.py index 18b04ec..46e305d 100644 --- a/bookmarks/tests/test_bookmark_edit_view.py +++ b/bookmarks/tests/test_bookmark_edit_view.py @@ -141,6 +141,40 @@ class BookmarkEditViewTestCase(TestCase, BookmarkFactoryMixin): html, ) + def test_should_prevent_duplicate_urls(self): + edited_bookmark = self.setup_bookmark(url="http://example.com/edited") + existing_bookmark = self.setup_bookmark(url="http://example.com/existing") + other_user_bookmark = self.setup_bookmark( + url="http://example.com/other-user", user=User.objects.create_user("other") + ) + + # if the URL isn't modified it's not a duplicate + form_data = self.create_form_data({"url": edited_bookmark.url}) + response = self.client.post( + reverse("bookmarks:edit", args=[edited_bookmark.id]), form_data + ) + self.assertEqual(response.status_code, 302) + + # if the URL is already bookmarked by another user, it's not a duplicate + form_data = self.create_form_data({"url": other_user_bookmark.url}) + response = self.client.post( + reverse("bookmarks:edit", args=[edited_bookmark.id]), form_data + ) + self.assertEqual(response.status_code, 302) + + # if the URL is already bookmarked by the same user, it's a duplicate + form_data = self.create_form_data({"url": existing_bookmark.url}) + response = self.client.post( + reverse("bookmarks:edit", args=[edited_bookmark.id]), form_data + ) + self.assertEqual(response.status_code, 422) + self.assertInHTML( + "
  • A bookmark with this URL already exists.
    • ", + response.content.decode(), + ) + edited_bookmark.refresh_from_db() + self.assertNotEqual(edited_bookmark.url, existing_bookmark.url) + def test_should_redirect_to_return_url(self): bookmark = self.setup_bookmark() form_data = self.create_form_data() diff --git a/bookmarks/tests/test_bookmarks_api.py b/bookmarks/tests/test_bookmarks_api.py index 4bbe5a2..2166773 100644 --- a/bookmarks/tests/test_bookmarks_api.py +++ b/bookmarks/tests/test_bookmarks_api.py @@ -685,6 +685,29 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin): updated_bookmark = Bookmark.objects.get(id=bookmark.id) self.assertCountEqual(updated_bookmark.tags.all(), [tag1, tag2]) + def test_update_bookmark_should_prevent_duplicate_urls(self): + self.authenticate() + edited_bookmark = self.setup_bookmark(url="https://example.com/edited") + existing_bookmark = self.setup_bookmark(url="https://example.com/existing") + other_user_bookmark = self.setup_bookmark( + url="https://example.com/other", user=self.setup_user() + ) + + # if the URL isn't modified it's not a duplicate + data = {"url": edited_bookmark.url} + url = reverse("bookmarks:bookmark-detail", args=[edited_bookmark.id]) + self.put(url, data, expected_status_code=status.HTTP_200_OK) + + # if the URL is already bookmarked by another user, it's not a duplicate + data = {"url": other_user_bookmark.url} + url = reverse("bookmarks:bookmark-detail", args=[edited_bookmark.id]) + self.put(url, data, expected_status_code=status.HTTP_200_OK) + + # if the URL is already bookmarked by the same user, it's a duplicate + data = {"url": existing_bookmark.url} + url = reverse("bookmarks:bookmark-detail", args=[edited_bookmark.id]) + self.put(url, data, expected_status_code=status.HTTP_400_BAD_REQUEST) + def test_patch_bookmark(self): self.authenticate() bookmark = self.setup_bookmark() diff --git a/docs/src/content/docs/api.md b/docs/src/content/docs/api.md index f23fe33..06f640a 100644 --- a/docs/src/content/docs/api.md +++ b/docs/src/content/docs/api.md @@ -127,11 +127,9 @@ POST /api/bookmarks/ Creates a new bookmark. Tags are simply assigned using their names. Including `is_archived: true` saves a bookmark directly to the archive. -If the title and description are not provided or empty, the application automatically tries to scrape them from the bookmarked website. This behavior can be disabled by adding the `disable_scraping` query parameter to the API request. If you have an application where you want to keep using scraped metadata, but also allow users to leave the title or description empty, you should: +If the provided URL is already bookmarked, this silently updates the existing bookmark instead of creating a new one. If you are implementing a user interface, consider notifying users about this behavior. You can use the `/check` endpoint to check if a URL is already bookmarked and at the same time get the existing bookmark data. This behavior may change in the future to return an error instead. -- Fetch the scraped title and description using the `/check` endpoint. -- Prefill the title and description fields in your app with the fetched values and allow users to clear those values. -- Add the `disable_scraping` query parameter to prevent the API from adding them back again. +If the title and description are not provided or empty, the application automatically tries to scrape them from the bookmarked website. This behavior can be disabled by adding the `disable_scraping` query parameter to the API request. Example payload: @@ -164,6 +162,8 @@ When using `PATCH`, only the fields that should be updated need to be provided. Regardless which method is used, any field that is not provided is not modified. Tags are simply assigned using their names. +If the provided URL is already bookmarked this returns an error. + Example payload: ```json