From 04bfc7b2a06b71e187b6714b339ea559a3aac8c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Mon, 23 Sep 2024 23:20:42 +0200 Subject: [PATCH 1/2] prevent creating duplicate URLs on edit --- bookmarks/models.py | 16 ++++++++++ bookmarks/tests/test_bookmark_edit_view.py | 36 ++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/bookmarks/models.py b/bookmarks/models.py index e65c2251..333c1b1d 100644 --- a/bookmarks/models.py +++ b/bookmarks/models.py @@ -168,6 +168,22 @@ def has_notes(self): self.instance and self.instance.notes ) + def clean(self): + cleaned_data = super().clean() + url = cleaned_data.get("url") + + if self.instance.pk and url: + # Ensure there is no existing Bookmark with the same URL + existing_bookmark = ( + Bookmark.objects.filter(url=url, owner=self.instance.owner) + .exclude(pk=self.instance.pk) + .first() + ) + if existing_bookmark: + self.add_error("url", "A bookmark with this URL already exists.") + + return cleaned_data + 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 18b04ec9..1ca7dba4 100644 --- a/bookmarks/tests/test_bookmark_edit_view.py +++ b/bookmarks/tests/test_bookmark_edit_view.py @@ -141,6 +141,42 @@ def test_should_prefill_bookmark_form_fields(self): html, ) + def test_should_prevent_changing_url_to_existing_url_for_same_owner(self): + edited_bookmark = self.setup_bookmark(url="http://example.com/edited") + existing_bookmark = self.setup_bookmark(url="http://example.com/existing") + + form_data = self.create_form_data( + {"id": edited_bookmark.id, "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_allow_changing_url_to_existing_url_for_different_owner(self): + edited_bookmark = self.setup_bookmark(url="http://example.com/edited") + existing_bookmark = self.setup_bookmark( + url="http://example.com/existing", user=User.objects.create_user("other") + ) + + form_data = self.create_form_data( + {"id": edited_bookmark.id, "url": existing_bookmark.url} + ) + response = self.client.post( + reverse("bookmarks:edit", args=[edited_bookmark.id]), form_data + ) + + self.assertEqual(response.status_code, 302) + edited_bookmark.refresh_from_db() + self.assertEqual(edited_bookmark.url, existing_bookmark.url) + def test_should_redirect_to_return_url(self): bookmark = self.setup_bookmark() form_data = self.create_form_data() From 74f8ca4452d181ea1e9cdc6b8b651b6582704251 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Tue, 24 Sep 2024 13:05:26 +0200 Subject: [PATCH 2/2] Prevent duplicates when editing --- bookmarks/api/serializers.py | 18 ++++++++++ bookmarks/models.py | 26 ++++++++------- bookmarks/tests/test_bookmark_edit_view.py | 38 ++++++++++------------ bookmarks/tests/test_bookmarks_api.py | 23 +++++++++++++ docs/src/content/docs/api.md | 8 ++--- 5 files changed, 77 insertions(+), 36 deletions(-) diff --git a/bookmarks/api/serializers.py b/bookmarks/api/serializers.py index 37a7bb0c..153341e8 100644 --- a/bookmarks/api/serializers.py +++ b/bookmarks/api/serializers.py @@ -111,6 +111,24 @@ def update(self, instance: Bookmark, validated_data): 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 333c1b1d..2b94cfac 100644 --- a/bookmarks/models.py +++ b/bookmarks/models.py @@ -168,21 +168,23 @@ def has_notes(self): self.instance and self.instance.notes ) - def clean(self): - cleaned_data = super().clean() - url = cleaned_data.get("url") - - if self.instance.pk and url: - # Ensure there is no existing Bookmark with the same URL - existing_bookmark = ( - Bookmark.objects.filter(url=url, owner=self.instance.owner) + 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) - .first() + .exists() ) - if existing_bookmark: - self.add_error("url", "A bookmark with this URL already exists.") + if is_duplicate: + raise forms.ValidationError("A bookmark with this URL already exists.") - return cleaned_data + return url class BookmarkSearch: diff --git a/bookmarks/tests/test_bookmark_edit_view.py b/bookmarks/tests/test_bookmark_edit_view.py index 1ca7dba4..46e305d6 100644 --- a/bookmarks/tests/test_bookmark_edit_view.py +++ b/bookmarks/tests/test_bookmark_edit_view.py @@ -141,17 +141,32 @@ def test_should_prefill_bookmark_form_fields(self): html, ) - def test_should_prevent_changing_url_to_existing_url_for_same_owner(self): + 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") + ) - form_data = self.create_form_data( - {"id": edited_bookmark.id, "url": existing_bookmark.url} + # 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.
    • ", @@ -160,23 +175,6 @@ def test_should_prevent_changing_url_to_existing_url_for_same_owner(self): edited_bookmark.refresh_from_db() self.assertNotEqual(edited_bookmark.url, existing_bookmark.url) - def test_should_allow_changing_url_to_existing_url_for_different_owner(self): - edited_bookmark = self.setup_bookmark(url="http://example.com/edited") - existing_bookmark = self.setup_bookmark( - url="http://example.com/existing", user=User.objects.create_user("other") - ) - - form_data = self.create_form_data( - {"id": edited_bookmark.id, "url": existing_bookmark.url} - ) - response = self.client.post( - reverse("bookmarks:edit", args=[edited_bookmark.id]), form_data - ) - - self.assertEqual(response.status_code, 302) - edited_bookmark.refresh_from_db() - self.assertEqual(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 4bbe5a2c..21667732 100644 --- a/bookmarks/tests/test_bookmarks_api.py +++ b/bookmarks/tests/test_bookmarks_api.py @@ -685,6 +685,29 @@ def test_update_bookmark_adds_tags_from_auto_tagging(self): 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 f23fe339..06f640a4 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