From fe7ddbe6451a252c57222d0ed62cec4d6a8fafbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Sun, 22 Sep 2024 07:52:00 +0200 Subject: [PATCH] Allow bookmarks to have empty title and description (#843) * add migration for merging fields * remove usage of website title and description * keep empty website title and description in API for compatibility * restore scraping in API and add option for disabling it * document API scraping behavior * remove deprecated fields from API docs * improve form layout * cleanup migration * cleanup website loader * update tests --- bookmarks/api/routes.py | 20 ++- bookmarks/api/serializers.py | 32 ++++- .../e2e/e2e_test_bookmark_details_modal.py | 3 + bookmarks/e2e/e2e_test_bookmark_form.py | 106 ++++++++++++---- .../components/SearchAutoComplete.svelte | 2 +- bookmarks/migrations/0041_merge_metadata.py | 36 ++++++ bookmarks/models.py | 15 +-- bookmarks/queries.py | 6 - bookmarks/services/bookmarks.py | 21 +-- bookmarks/services/website_loader.py | 7 +- bookmarks/templates/bookmarks/form.html | 115 ++++------------- bookmarks/tests/helpers.py | 4 - .../tests/test_bookmark_details_modal.py | 21 +-- bookmarks/tests/test_bookmark_edit_view.py | 20 +-- bookmarks/tests/test_bookmark_new_view.py | 5 +- bookmarks/tests/test_bookmarks_api.py | 78 ++++++++++-- bookmarks/tests/test_bookmarks_model.py | 8 +- bookmarks/tests/test_bookmarks_service.py | 120 ++++++++++++------ bookmarks/tests/test_exporter.py | 2 - bookmarks/tests/test_feeds.py | 15 ++- bookmarks/tests/test_queries.py | 100 ++------------- docs/src/content/docs/api.md | 41 +++++- 22 files changed, 411 insertions(+), 366 deletions(-) create mode 100644 bookmarks/migrations/0041_merge_metadata.py diff --git a/bookmarks/api/routes.py b/bookmarks/api/routes.py index 8c1257b..332dc60 100644 --- a/bookmarks/api/routes.py +++ b/bookmarks/api/routes.py @@ -56,7 +56,12 @@ class BookmarkViewSet( return Bookmark.objects.all().filter(owner=user) def get_serializer_context(self): - return {"request": self.request, "user": self.request.user} + disable_scraping = "disable_scraping" in self.request.GET + return { + "request": self.request, + "user": self.request.user, + "disable_scraping": disable_scraping, + } @action(methods=["get"], detail=False) def archived(self, request): @@ -101,16 +106,7 @@ class BookmarkViewSet( self.get_serializer(bookmark).data if bookmark else None ) - # Either return metadata from existing bookmark, or scrape from URL - if bookmark: - metadata = WebsiteMetadata( - url, - bookmark.website_title, - bookmark.website_description, - None, - ) - else: - metadata = website_loader.load_website_metadata(url) + metadata = website_loader.load_website_metadata(url) # Return tags that would be automatically applied to the bookmark profile = request.user.profile @@ -120,7 +116,7 @@ class BookmarkViewSet( auto_tags = auto_tagging.get_tags(profile.auto_tagging_rules, url) except Exception as e: logger.error( - f"Failed to auto-tag bookmark. url={bookmark.url}", + f"Failed to auto-tag bookmark. url={url}", exc_info=e, ) diff --git a/bookmarks/api/serializers.py b/bookmarks/api/serializers.py index 38f19b9..88efa26 100644 --- a/bookmarks/api/serializers.py +++ b/bookmarks/api/serializers.py @@ -4,7 +4,11 @@ from rest_framework import serializers from rest_framework.serializers import ListSerializer from bookmarks.models import Bookmark, Tag, build_tag_string, UserProfile -from bookmarks.services.bookmarks import create_bookmark, update_bookmark +from bookmarks.services.bookmarks import ( + create_bookmark, + update_bookmark, + enhance_with_website_metadata, +) from bookmarks.services.tags import get_or_create_tag @@ -29,8 +33,6 @@ class BookmarkSerializer(serializers.ModelSerializer): "title", "description", "notes", - "website_title", - "website_description", "web_archive_snapshot_url", "favicon_url", "preview_image_url", @@ -40,15 +42,17 @@ class BookmarkSerializer(serializers.ModelSerializer): "tag_names", "date_added", "date_modified", - ] - read_only_fields = [ "website_title", "website_description", + ] + read_only_fields = [ "web_archive_snapshot_url", "favicon_url", "preview_image_url", "date_added", "date_modified", + "website_title", + "website_description", ] list_serializer_class = BookmarkListSerializer @@ -63,6 +67,9 @@ class BookmarkSerializer(serializers.ModelSerializer): tag_names = TagListField(required=False, default=[]) favicon_url = serializers.SerializerMethodField() preview_image_url = serializers.SerializerMethodField() + # Add dummy website title and description fields for backwards compatibility but keep them empty + website_title = serializers.SerializerMethodField() + website_description = serializers.SerializerMethodField() def get_favicon_url(self, obj: Bookmark): if not obj.favicon_file: @@ -80,6 +87,12 @@ class BookmarkSerializer(serializers.ModelSerializer): preview_image_url = request.build_absolute_uri(preview_image_file_path) return preview_image_url + def get_website_title(self, obj: Bookmark): + return None + + def get_website_description(self, obj: Bookmark): + return None + def create(self, validated_data): bookmark = Bookmark() bookmark.url = validated_data["url"] @@ -90,7 +103,14 @@ class BookmarkSerializer(serializers.ModelSerializer): bookmark.unread = validated_data["unread"] bookmark.shared = validated_data["shared"] tag_string = build_tag_string(validated_data["tag_names"]) - return create_bookmark(bookmark, tag_string, self.context["user"]) + + saved_bookmark = create_bookmark(bookmark, tag_string, self.context["user"]) + # Unless scraping is explicitly disabled, enhance bookmark with website + # metadata to preserve backwards compatibility with clients that expect + # title and description to be populated automatically when left empty + if not self.context.get("disable_scraping", False): + enhance_with_website_metadata(saved_bookmark) + return saved_bookmark def update(self, instance: Bookmark, validated_data): # Update fields if they were provided in the payload diff --git a/bookmarks/e2e/e2e_test_bookmark_details_modal.py b/bookmarks/e2e/e2e_test_bookmark_details_modal.py index 947a48f..93f03e4 100644 --- a/bookmarks/e2e/e2e_test_bookmark_details_modal.py +++ b/bookmarks/e2e/e2e_test_bookmark_details_modal.py @@ -135,6 +135,9 @@ class BookmarkDetailsModalE2ETestCase(LinkdingE2ETestCase): details_modal = self.open_details_modal(bookmark) + # Wait for confirm button to be initialized + self.page.wait_for_timeout(1000) + # Delete bookmark, verify return url with self.page.expect_navigation(url=self.live_server_url + url): details_modal.get_by_text("Delete...").click() diff --git a/bookmarks/e2e/e2e_test_bookmark_form.py b/bookmarks/e2e/e2e_test_bookmark_form.py index 8c031d5..e6236d6 100644 --- a/bookmarks/e2e/e2e_test_bookmark_form.py +++ b/bookmarks/e2e/e2e_test_bookmark_form.py @@ -1,26 +1,48 @@ +from unittest.mock import patch + from django.urls import reverse from playwright.sync_api import sync_playwright, expect from bookmarks.e2e.helpers import LinkdingE2ETestCase +from bookmarks.services import website_loader class BookmarkFormE2ETestCase(LinkdingE2ETestCase): + def test_create_enter_url_prefills_title_and_description(self): + with patch.object( + website_loader, "load_website_metadata" + ) as mock_load_website_metadata: + mock_load_website_metadata.return_value = website_loader.WebsiteMetadata( + url="https://example.com", + title="Example Domain", + description="This domain is for use in illustrative examples in documents. You may use this domain in literature without prior coordination or asking for permission.", + preview_image=None, + ) + + with sync_playwright() as p: + page = self.open(reverse("bookmarks:new"), p) + + page.get_by_label("URL").fill("https://example.com") + + title = page.get_by_label("Title") + description = page.get_by_label("Description") + expect(title).to_have_value("Example Domain") + expect(description).to_have_value( + "This domain is for use in illustrative examples in documents. You may use this domain in literature without prior coordination or asking for permission." + ) + def test_create_should_check_for_existing_bookmark(self): existing_bookmark = self.setup_bookmark( title="Existing title", description="Existing description", notes="Existing notes", tags=[self.setup_tag(name="tag1"), self.setup_tag(name="tag2")], - website_title="Existing website title", - website_description="Existing website description", unread=True, ) tag_names = " ".join(existing_bookmark.tag_names) with sync_playwright() as p: - browser = self.setup_browser(p) - page = browser.new_page() - page.goto(self.live_server_url + reverse("bookmarks:new")) + page = self.open(reverse("bookmarks:new"), p) # Enter bookmarked URL page.get_by_label("URL").fill(existing_bookmark.url) @@ -37,14 +59,6 @@ class BookmarkFormE2ETestCase(LinkdingE2ETestCase): self.assertEqual( existing_bookmark.notes, page.get_by_label("Notes").input_value() ) - self.assertEqual( - existing_bookmark.website_title, - page.get_by_label("Title").get_attribute("placeholder"), - ) - self.assertEqual( - existing_bookmark.website_description, - page.get_by_label("Description").get_attribute("placeholder"), - ) self.assertEqual(tag_names, page.get_by_label("Tags").input_value()) self.assertTrue(tag_names, page.get_by_label("Mark as unread").is_checked()) @@ -55,30 +69,66 @@ class BookmarkFormE2ETestCase(LinkdingE2ETestCase): state="hidden", timeout=2000 ) - browser.close() - def test_edit_should_not_check_for_existing_bookmark(self): bookmark = self.setup_bookmark() with sync_playwright() as p: - browser = self.setup_browser(p) - page = browser.new_page() - page.goto( - self.live_server_url + reverse("bookmarks:edit", args=[bookmark.id]) - ) + page = self.open(reverse("bookmarks:edit", args=[bookmark.id]), p) page.wait_for_timeout(timeout=1000) page.get_by_text("This URL is already bookmarked.").wait_for(state="hidden") + def test_edit_should_not_prefill_title_and_description(self): + bookmark = self.setup_bookmark() + with patch.object( + website_loader, "load_website_metadata" + ) as mock_load_website_metadata: + mock_load_website_metadata.return_value = website_loader.WebsiteMetadata( + url="https://example.com", + title="Example Domain", + description="This domain is for use in illustrative examples in documents. You may use this domain in literature without prior coordination or asking for permission.", + preview_image=None, + ) + + with sync_playwright() as p: + page = self.open(reverse("bookmarks:edit", args=[bookmark.id]), p) + page.wait_for_timeout(timeout=1000) + + title = page.get_by_label("Title") + description = page.get_by_label("Description") + expect(title).to_have_value(bookmark.title) + expect(description).to_have_value(bookmark.description) + + def test_edit_enter_url_should_not_prefill_title_and_description(self): + bookmark = self.setup_bookmark() + with patch.object( + website_loader, "load_website_metadata" + ) as mock_load_website_metadata: + mock_load_website_metadata.return_value = website_loader.WebsiteMetadata( + url="https://example.com", + title="Example Domain", + description="This domain is for use in illustrative examples in documents. You may use this domain in literature without prior coordination or asking for permission.", + preview_image=None, + ) + + with sync_playwright() as p: + page = self.open(reverse("bookmarks:edit", args=[bookmark.id]), p) + + page.get_by_label("URL").fill("https://example.com") + page.wait_for_timeout(timeout=1000) + + title = page.get_by_label("Title") + description = page.get_by_label("Description") + expect(title).to_have_value(bookmark.title) + expect(description).to_have_value(bookmark.description) + def test_enter_url_of_existing_bookmark_should_show_notes(self): bookmark = self.setup_bookmark( notes="Existing notes", description="Existing description" ) with sync_playwright() as p: - browser = self.setup_browser(p) - page = browser.new_page() - page.goto(self.live_server_url + reverse("bookmarks:new")) + page = self.open(reverse("bookmarks:new"), p) details = page.locator("details.notes") expect(details).not_to_have_attribute("open", value="") @@ -93,11 +143,11 @@ class BookmarkFormE2ETestCase(LinkdingE2ETestCase): with sync_playwright() as p: # Open page with URL that should have auto tags - browser = self.setup_browser(p) - page = browser.new_page() - url = self.live_server_url + reverse("bookmarks:new") - url += f"?url=https%3A%2F%2Fgithub.com%2Fsissbruecker%2Flinkding" - page.goto(url) + url = ( + reverse("bookmarks:new") + + "?url=https%3A%2F%2Fgithub.com%2Fsissbruecker%2Flinkding" + ) + page = self.open(url, p) auto_tags_hint = page.locator(".form-input-hint.auto-tags") expect(auto_tags_hint).to_be_visible() diff --git a/bookmarks/frontend/components/SearchAutoComplete.svelte b/bookmarks/frontend/components/SearchAutoComplete.svelte index 4a9cec9..64d8418 100644 --- a/bookmarks/frontend/components/SearchAutoComplete.svelte +++ b/bookmarks/frontend/components/SearchAutoComplete.svelte @@ -122,7 +122,7 @@ } const fetchedBookmarks = await api.listBookmarks(suggestionSearch, {limit: 5, offset: 0, path}) bookmarks = fetchedBookmarks.map(bookmark => { - const fullLabel = bookmark.title || bookmark.website_title || bookmark.url + const fullLabel = bookmark.title || bookmark.url const label = clampText(fullLabel, 60) return { type: 'bookmark', diff --git a/bookmarks/migrations/0041_merge_metadata.py b/bookmarks/migrations/0041_merge_metadata.py new file mode 100644 index 0000000..81e3ab9 --- /dev/null +++ b/bookmarks/migrations/0041_merge_metadata.py @@ -0,0 +1,36 @@ +# Generated by Django 5.1.1 on 2024-09-21 08:13 + +from django.db import migrations +from django.db.models import Q +from django.db.models.expressions import RawSQL + +from bookmarks.models import Bookmark + + +def forwards(apps, schema_editor): + Bookmark.objects.filter( + Q(title__isnull=True) | Q(title__exact=""), + ).extra( + where=["website_title IS NOT NULL"] + ).update(title=RawSQL("website_title", ())) + + Bookmark.objects.filter( + Q(description__isnull=True) | Q(description__exact=""), + ).extra(where=["website_description IS NOT NULL"]).update( + description=RawSQL("website_description", ()) + ) + + +def reverse(apps, schema_editor): + pass + + +class Migration(migrations.Migration): + + dependencies = [ + ("bookmarks", "0040_userprofile_items_per_page_and_more"), + ] + + operations = [ + migrations.RunPython(forwards, reverse), + ] diff --git a/bookmarks/models.py b/bookmarks/models.py index 7d8bc5a..e65c225 100644 --- a/bookmarks/models.py +++ b/bookmarks/models.py @@ -56,7 +56,9 @@ class Bookmark(models.Model): title = models.CharField(max_length=512, blank=True) description = models.TextField(blank=True) notes = models.TextField(blank=True) + # Obsolete field, kept to not remove column when generating migrations website_title = models.CharField(max_length=512, blank=True, null=True) + # Obsolete field, kept to not remove column when generating migrations website_description = models.TextField(blank=True, null=True) web_archive_snapshot_url = models.CharField(max_length=2048, blank=True) favicon_file = models.CharField(max_length=512, blank=True) @@ -74,14 +76,12 @@ class Bookmark(models.Model): def resolved_title(self): if self.title: return self.title - elif self.website_title: - return self.website_title else: return self.url @property def resolved_description(self): - return self.website_description if not self.description else self.description + return self.description @property def tag_names(self): @@ -141,14 +141,9 @@ class BookmarkForm(forms.ModelForm): # Use URLField for URL url = forms.CharField(validators=[BookmarkURLValidator()]) tag_string = forms.CharField(required=False) - # Do not require title and description in form as we fill these automatically if they are empty + # Do not require title and description as they may be empty title = forms.CharField(max_length=512, required=False) description = forms.CharField(required=False, widget=forms.Textarea()) - # Include website title and description as hidden field as they only provide info when editing bookmarks - website_title = forms.CharField( - max_length=512, required=False, widget=forms.HiddenInput() - ) - website_description = forms.CharField(required=False, widget=forms.HiddenInput()) unread = forms.BooleanField(required=False) shared = forms.BooleanField(required=False) # Hidden field that determines whether to close window/tab after saving the bookmark @@ -162,8 +157,6 @@ class BookmarkForm(forms.ModelForm): "title", "description", "notes", - "website_title", - "website_description", "unread", "shared", "auto_close", diff --git a/bookmarks/queries.py b/bookmarks/queries.py index 112f8df..8fd6a08 100644 --- a/bookmarks/queries.py +++ b/bookmarks/queries.py @@ -53,8 +53,6 @@ def _base_bookmarks_query( Q(title__icontains=term) | Q(description__icontains=term) | Q(notes__icontains=term) - | Q(website_title__icontains=term) - | Q(website_description__icontains=term) | Q(url__icontains=term) ) @@ -97,10 +95,6 @@ def _base_bookmarks_query( query_set = query_set.annotate( effective_title=Case( When(Q(title__isnull=False) & ~Q(title__exact=""), then=Lower("title")), - When( - Q(website_title__isnull=False) & ~Q(website_title__exact=""), - then=Lower("website_title"), - ), default=Lower("url"), output_field=CharField(), ) diff --git a/bookmarks/services/bookmarks.py b/bookmarks/services/bookmarks.py index 37d940b..034404f 100644 --- a/bookmarks/services/bookmarks.py +++ b/bookmarks/services/bookmarks.py @@ -26,8 +26,6 @@ def create_bookmark(bookmark: Bookmark, tag_string: str, current_user: User): _merge_bookmark_data(bookmark, existing_bookmark) return update_bookmark(existing_bookmark, tag_string, current_user) - # Update website info - _update_website_metadata(bookmark) # Set currently logged in user as owner bookmark.owner = current_user # Set dates @@ -67,13 +65,22 @@ def update_bookmark(bookmark: Bookmark, tag_string, current_user: User): if has_url_changed: # Update web archive snapshot, if URL changed tasks.create_web_archive_snapshot(current_user, bookmark, True) - # Only update website metadata if URL changed - _update_website_metadata(bookmark) bookmark.save() return bookmark +def enhance_with_website_metadata(bookmark: Bookmark): + metadata = website_loader.load_website_metadata(bookmark.url) + if not bookmark.title: + bookmark.title = metadata.title or "" + + if not bookmark.description: + bookmark.description = metadata.description or "" + + bookmark.save() + + def archive_bookmark(bookmark: Bookmark): bookmark.is_archived = True bookmark.date_modified = timezone.now() @@ -235,12 +242,6 @@ def _merge_bookmark_data(from_bookmark: Bookmark, to_bookmark: Bookmark): to_bookmark.shared = from_bookmark.shared -def _update_website_metadata(bookmark: Bookmark): - metadata = website_loader.load_website_metadata(bookmark.url) - bookmark.website_title = metadata.title - bookmark.website_description = metadata.description - - def _update_bookmark_tags(bookmark: Bookmark, tag_string: str, user: User): tag_names = parse_tag_string(tag_string) diff --git a/bookmarks/services/website_loader.py b/bookmarks/services/website_loader.py index 389e374..2134659 100644 --- a/bookmarks/services/website_loader.py +++ b/bookmarks/services/website_loader.py @@ -14,8 +14,8 @@ logger = logging.getLogger(__name__) @dataclass class WebsiteMetadata: url: str - title: str - description: str + title: str | None + description: str | None preview_image: str | None def to_dict(self): @@ -43,7 +43,8 @@ def load_website_metadata(url: str): start = timezone.now() soup = BeautifulSoup(page_text, "html.parser") - title = soup.title.string.strip() if soup.title is not None else None + if soup.title and soup.title.string: + title = soup.title.string.strip() description_tag = soup.find("meta", attrs={"name": "description"}) description = ( description_tag["content"].strip() diff --git a/bookmarks/templates/bookmarks/form.html b/bookmarks/templates/bookmarks/form.html index f35b32e..2223852 100644 --- a/bookmarks/templates/bookmarks/form.html +++ b/bookmarks/templates/bookmarks/form.html @@ -3,12 +3,13 @@
{% csrf_token %} - {{ form.website_title }} - {{ form.website_description }} {{ form.auto_close|attr:"type:hidden" }}
- {{ form.url|add_class:"form-input"|attr:"autofocus"|attr:"placeholder: " }} +
+ {{ form.url|add_class:"form-input"|attr:"autofocus"|attr:"placeholder: " }} + +
{% if form.url.errors %}
{{ form.url.errors }} @@ -29,44 +30,14 @@
{{ form.tag_string.errors }}
-
+
-
- {{ form.title|add_class:"form-input"|attr:"autocomplete:off" }} - - -
-
- Optional, leave empty to use title from website. -
+ {{ form.title|add_class:"form-input"|attr:"autocomplete:off" }} {{ form.title.errors }}
-
- {{ form.description|add_class:"form-input"|attr:"rows:2" }} - - -
-
- Optional, leave empty to use description from website. -
+ {{ form.description|add_class:"form-input"|attr:"rows:3" }} {{ form.description.errors }}
@@ -76,10 +47,10 @@ {{ form.notes|add_class:"form-input"|attr:"rows:8" }} +
+ Additional notes, supports Markdown. +
-
- Additional notes, supports Markdown. -
{{ form.notes.errors }}
@@ -119,9 +90,8 @@
diff --git a/bookmarks/tests/helpers.py b/bookmarks/tests/helpers.py index 4a5f016..ca3ea64 100644 --- a/bookmarks/tests/helpers.py +++ b/bookmarks/tests/helpers.py @@ -41,8 +41,6 @@ class BookmarkFactoryMixin: title: str = None, description: str = "", notes: str = "", - website_title: str = "", - website_description: str = "", web_archive_snapshot_url: str = "", favicon_file: str = "", preview_image_file: str = "", @@ -64,8 +62,6 @@ class BookmarkFactoryMixin: title=title, description=description, notes=notes, - website_title=website_title, - website_description=website_description, date_added=added, date_modified=timezone.now(), owner=user, diff --git a/bookmarks/tests/test_bookmark_details_modal.py b/bookmarks/tests/test_bookmark_details_modal.py index f924ae8..5a89739 100644 --- a/bookmarks/tests/test_bookmark_details_modal.py +++ b/bookmarks/tests/test_bookmark_details_modal.py @@ -150,16 +150,8 @@ class BookmarkDetailsModalTestCase(TestCase, BookmarkFactoryMixin, HtmlTestMixin self.assertIsNotNone(title) self.assertEqual(title.text.strip(), bookmark.title) - # with website title - bookmark = self.setup_bookmark(title="", website_title="Website title") - soup = self.get_index_details_modal(bookmark) - - title = soup.find("h2") - self.assertIsNotNone(title) - self.assertEqual(title.text.strip(), bookmark.website_title) - # with URL only - bookmark = self.setup_bookmark(title="", website_title="") + bookmark = self.setup_bookmark(title="") soup = self.get_index_details_modal(bookmark) title = soup.find("h2") @@ -478,7 +470,7 @@ class BookmarkDetailsModalTestCase(TestCase, BookmarkFactoryMixin, HtmlTestMixin def test_description(self): # without description - bookmark = self.setup_bookmark(description="", website_description="") + bookmark = self.setup_bookmark(description="") soup = self.get_index_details_modal(bookmark) section = self.find_section(soup, "Description") @@ -491,15 +483,6 @@ class BookmarkDetailsModalTestCase(TestCase, BookmarkFactoryMixin, HtmlTestMixin section = self.get_section(soup, "Description") self.assertEqual(section.text.strip(), bookmark.description) - # with website description - bookmark = self.setup_bookmark( - description="", website_description="Website description" - ) - soup = self.get_index_details_modal(bookmark) - - section = self.get_section(soup, "Description") - self.assertEqual(section.text.strip(), bookmark.website_description) - def test_notes(self): # without notes bookmark = self.setup_bookmark() diff --git a/bookmarks/tests/test_bookmark_edit_view.py b/bookmarks/tests/test_bookmark_edit_view.py index 68fbafb..6cc3094 100644 --- a/bookmarks/tests/test_bookmark_edit_view.py +++ b/bookmarks/tests/test_bookmark_edit_view.py @@ -80,8 +80,6 @@ class BookmarkEditViewTestCase(TestCase, BookmarkFactoryMixin): title="edited title", description="edited description", notes="edited notes", - website_title="website title", - website_description="website description", ) response = self.client.get(reverse("bookmarks:edit", args=[bookmark.id])) @@ -114,7 +112,7 @@ class BookmarkEditViewTestCase(TestCase, BookmarkFactoryMixin): self.assertInHTML( f""" - """, @@ -130,22 +128,6 @@ class BookmarkEditViewTestCase(TestCase, BookmarkFactoryMixin): html, ) - self.assertInHTML( - f""" - - """, - html, - ) - - self.assertInHTML( - f""" - - """, - html, - ) - def test_should_redirect_to_return_url(self): bookmark = self.setup_bookmark() form_data = self.create_form_data() diff --git a/bookmarks/tests/test_bookmark_new_view.py b/bookmarks/tests/test_bookmark_new_view.py index 02c76ce..cb18338 100644 --- a/bookmarks/tests/test_bookmark_new_view.py +++ b/bookmarks/tests/test_bookmark_new_view.py @@ -96,7 +96,7 @@ class BookmarkNewViewTestCase(TestCase, BookmarkFactoryMixin): self.assertInHTML( '', + 'rows="3" id="id_description">Example Site Description', html, ) @@ -115,6 +115,9 @@ class BookmarkNewViewTestCase(TestCase, BookmarkFactoryMixin): +
+ Additional notes, supports Markdown. +
""", html, diff --git a/bookmarks/tests/test_bookmarks_api.py b/bookmarks/tests/test_bookmarks_api.py index 68bd1a2..8794b8d 100644 --- a/bookmarks/tests/test_bookmarks_api.py +++ b/bookmarks/tests/test_bookmarks_api.py @@ -33,8 +33,6 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin): expectation["title"] = bookmark.title expectation["description"] = bookmark.description expectation["notes"] = bookmark.notes - expectation["website_title"] = bookmark.website_title - expectation["website_description"] = bookmark.website_description expectation["web_archive_snapshot_url"] = bookmark.web_archive_snapshot_url expectation["favicon_url"] = ( f"http://testserver/static/{bookmark.favicon_file}" @@ -56,6 +54,8 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin): expectation["date_modified"] = bookmark.date_modified.isoformat().replace( "+00:00", "Z" ) + expectation["website_title"] = None + expectation["website_description"] = None expectations.append(expectation) for data in data_list: @@ -87,6 +87,19 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin): ) self.assertBookmarkListEqual(response.data["results"], bookmarks) + def test_list_bookmarks_returns_none_for_website_title_and_description(self): + self.authenticate() + bookmark = self.setup_bookmark() + bookmark.website_title = "Website title" + bookmark.website_description = "Website description" + bookmark.save() + + response = self.get( + reverse("bookmarks:bookmark-list"), expected_status_code=status.HTTP_200_OK + ) + self.assertIsNone(response.data["results"][0]["website_title"]) + self.assertIsNone(response.data["results"][0]["website_description"]) + def test_list_bookmarks_does_not_return_archived_bookmarks(self): self.authenticate() bookmarks = self.setup_numbered_bookmarks(5) @@ -382,6 +395,44 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin): self.assertEqual(bookmark.tags.filter(name=data["tag_names"][0]).count(), 1) self.assertEqual(bookmark.tags.filter(name=data["tag_names"][1]).count(), 1) + def test_create_bookmark_enhances_with_metadata_by_default(self): + self.authenticate() + + data = {"url": "https://example.com/"} + with patch.object(website_loader, "load_website_metadata") as mock_load: + mock_load.return_value = WebsiteMetadata( + url="https://example.com/", + title="Website title", + description="Website description", + preview_image=None, + ) + self.post(reverse("bookmarks:bookmark-list"), data, status.HTTP_201_CREATED) + bookmark = Bookmark.objects.get(url=data["url"]) + self.assertEqual(bookmark.title, "Website title") + self.assertEqual(bookmark.description, "Website description") + + def test_create_bookmark_does_not_enhance_with_metadata_if_scraping_is_disabled( + self, + ): + self.authenticate() + + data = {"url": "https://example.com/"} + with patch.object(website_loader, "load_website_metadata") as mock_load: + mock_load.return_value = WebsiteMetadata( + url="https://example.com/", + title="Website title", + description="Website description", + preview_image=None, + ) + self.post( + reverse("bookmarks:bookmark-list") + "?disable_scraping", + data, + status.HTTP_201_CREATED, + ) + bookmark = Bookmark.objects.get(url=data["url"]) + self.assertEqual(bookmark.title, "") + self.assertEqual(bookmark.description, "") + def test_create_bookmark_with_same_url_updates_existing_bookmark(self): self.authenticate() @@ -775,18 +826,24 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin): "http://testserver/static/preview.png", bookmark_data["preview_image_url"] ) - def test_check_returns_existing_metadata_if_url_is_bookmarked(self): + def test_check_returns_scraped_metadata_if_url_is_bookmarked(self): self.authenticate() - bookmark = self.setup_bookmark( + self.setup_bookmark( url="https://example.com", - website_title="Existing title", - website_description="Existing description", ) with patch.object( website_loader, "load_website_metadata" ) as mock_load_website_metadata: + expected_metadata = WebsiteMetadata( + "https://example.com", + "Scraped metadata", + "Scraped description", + "https://example.com/preview.png", + ) + mock_load_website_metadata.return_value = expected_metadata + url = reverse("bookmarks:bookmark-check") check_url = urllib.parse.quote_plus("https://example.com") response = self.get( @@ -794,12 +851,11 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin): ) metadata = response.data["metadata"] - mock_load_website_metadata.assert_not_called() self.assertIsNotNone(metadata) - self.assertEqual(bookmark.url, metadata["url"]) - self.assertEqual(bookmark.website_title, metadata["title"]) - self.assertEqual(bookmark.website_description, metadata["description"]) - self.assertIsNone(metadata["preview_image"]) + self.assertEqual(expected_metadata.url, metadata["url"]) + self.assertEqual(expected_metadata.title, metadata["title"]) + self.assertEqual(expected_metadata.description, metadata["description"]) + self.assertEqual(expected_metadata.preview_image, metadata["preview_image"]) def test_check_returns_no_auto_tags_if_none_configured(self): self.authenticate() diff --git a/bookmarks/tests/test_bookmarks_model.py b/bookmarks/tests/test_bookmarks_model.py index f4c18b7..0331ce0 100644 --- a/bookmarks/tests/test_bookmarks_model.py +++ b/bookmarks/tests/test_bookmarks_model.py @@ -8,15 +8,9 @@ class BookmarkTestCase(TestCase): def test_bookmark_resolved_title(self): bookmark = Bookmark( title="Custom title", - website_title="Website title", url="https://example.com", ) self.assertEqual(bookmark.resolved_title, "Custom title") - bookmark = Bookmark( - title="", website_title="Website title", url="https://example.com" - ) - self.assertEqual(bookmark.resolved_title, "Website title") - - bookmark = Bookmark(title="", website_title="", url="https://example.com") + bookmark = Bookmark(title="", url="https://example.com") self.assertEqual(bookmark.resolved_title, "https://example.com") diff --git a/bookmarks/tests/test_bookmarks_service.py b/bookmarks/tests/test_bookmarks_service.py index 06aa80d..f6d096f 100644 --- a/bookmarks/tests/test_bookmarks_service.py +++ b/bookmarks/tests/test_bookmarks_service.py @@ -25,8 +25,8 @@ from bookmarks.services.bookmarks import ( share_bookmarks, unshare_bookmarks, upload_asset, + enhance_with_website_metadata, ) -from bookmarks.services.website_loader import WebsiteMetadata from bookmarks.tests.helpers import BookmarkFactoryMixin User = get_user_model() @@ -37,22 +37,14 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): def setUp(self) -> None: self.get_or_create_test_user() - def test_create_should_update_website_metadata(self): + def test_create_should_not_update_website_metadata(self): with patch.object( website_loader, "load_website_metadata" ) as mock_load_website_metadata: - expected_metadata = WebsiteMetadata( - "https://example.com", - "Website title", - "Website description", - "https://example.com/preview.png", - ) - mock_load_website_metadata.return_value = expected_metadata - bookmark_data = Bookmark( url="https://example.com", - title="Updated Title", - description="Updated description", + title="Initial Title", + description="Initial description", unread=True, shared=True, is_archived=True, @@ -62,10 +54,9 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): ) created_bookmark.refresh_from_db() - self.assertEqual(expected_metadata.title, created_bookmark.website_title) - self.assertEqual( - expected_metadata.description, created_bookmark.website_description - ) + self.assertEqual("Initial Title", created_bookmark.title) + self.assertEqual("Initial description", created_bookmark.description) + mock_load_website_metadata.assert_not_called() def test_create_should_update_existing_bookmark_with_same_url(self): original_bookmark = self.setup_bookmark( @@ -164,37 +155,28 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): mock_create_web_archive_snapshot.assert_not_called() - def test_update_should_update_website_metadata_if_url_did_change(self): - with patch.object( - website_loader, "load_website_metadata" - ) as mock_load_website_metadata: - expected_metadata = WebsiteMetadata( - "https://example.com/updated", - "Updated website title", - "Updated website description", - "https://example.com/preview.png", - ) - mock_load_website_metadata.return_value = expected_metadata - - bookmark = self.setup_bookmark() - bookmark.url = "https://example.com/updated" - update_bookmark(bookmark, "tag1,tag2", self.user) - - bookmark.refresh_from_db() - mock_load_website_metadata.assert_called_once() - self.assertEqual(expected_metadata.title, bookmark.website_title) - self.assertEqual( - expected_metadata.description, bookmark.website_description - ) - - def test_update_should_not_update_website_metadata_if_url_did_not_change(self): + def test_update_should_not_update_website_metadata(self): with patch.object( website_loader, "load_website_metadata" ) as mock_load_website_metadata: bookmark = self.setup_bookmark() bookmark.title = "updated title" update_bookmark(bookmark, "tag1,tag2", self.user) + bookmark.refresh_from_db() + self.assertEqual("updated title", bookmark.title) + mock_load_website_metadata.assert_not_called() + + def test_update_should_not_update_website_metadata_if_url_did_change(self): + with patch.object( + website_loader, "load_website_metadata" + ) as mock_load_website_metadata: + bookmark = self.setup_bookmark(title="initial title") + bookmark.url = "https://example.com/updated" + update_bookmark(bookmark, "tag1,tag2", self.user) + + bookmark.refresh_from_db() + self.assertEqual("initial title", bookmark.title) mock_load_website_metadata.assert_not_called() def test_update_should_update_favicon(self): @@ -914,3 +896,61 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): self.assertIsNone(asset.file_size) self.assertEqual(BookmarkAsset.STATUS_FAILURE, asset.status) self.assertEqual("", asset.file) + + def test_enhance_with_website_metadata(self): + bookmark = self.setup_bookmark(url="https://example.com") + with patch.object( + website_loader, "load_website_metadata" + ) as mock_load_website_metadata: + mock_load_website_metadata.return_value = website_loader.WebsiteMetadata( + url="https://example.com", + title="Website title", + description="Website description", + preview_image=None, + ) + + # missing title and description + bookmark.title = "" + bookmark.description = "" + bookmark.save() + enhance_with_website_metadata(bookmark) + bookmark.refresh_from_db() + + self.assertEqual("Website title", bookmark.title) + self.assertEqual("Website description", bookmark.description) + + # missing title only + bookmark.title = "" + bookmark.description = "Initial description" + bookmark.save() + enhance_with_website_metadata(bookmark) + bookmark.refresh_from_db() + + self.assertEqual("Website title", bookmark.title) + self.assertEqual("Initial description", bookmark.description) + + # missing description only + bookmark.title = "Initial title" + bookmark.description = "" + bookmark.save() + enhance_with_website_metadata(bookmark) + bookmark.refresh_from_db() + + self.assertEqual("Initial title", bookmark.title) + self.assertEqual("Website description", bookmark.description) + + # metadata returns None + mock_load_website_metadata.return_value = website_loader.WebsiteMetadata( + url="https://example.com", + title=None, + description=None, + preview_image=None, + ) + bookmark.title = "" + bookmark.description = "" + bookmark.save() + enhance_with_website_metadata(bookmark) + bookmark.refresh_from_db() + + self.assertEqual("", bookmark.title) + self.assertEqual("", bookmark.description) diff --git a/bookmarks/tests/test_exporter.py b/bookmarks/tests/test_exporter.py index 4045201..2457866 100644 --- a/bookmarks/tests/test_exporter.py +++ b/bookmarks/tests/test_exporter.py @@ -98,7 +98,5 @@ class ExporterTestCase(TestCase, BookmarkFactoryMixin): bookmark = self.setup_bookmark() bookmark.title = "" bookmark.description = "" - bookmark.website_title = None - bookmark.website_description = None bookmark.save() exporter.export_netscape_html([bookmark]) diff --git a/bookmarks/tests/test_feeds.py b/bookmarks/tests/test_feeds.py index 7b512c4..ffe3745 100644 --- a/bookmarks/tests/test_feeds.py +++ b/bookmarks/tests/test_feeds.py @@ -31,11 +31,18 @@ class FeedsTestCase(TestCase, BookmarkFactoryMixin): for tag in bookmark.tag_names: categories.append(f"{tag}") + if bookmark.resolved_description: + expected_description = ( + f"{bookmark.resolved_description}" + ) + else: + expected_description = "" + expected_item = ( "" f"{bookmark.resolved_title}" f"{bookmark.url}" - f"{bookmark.resolved_description}" + f"{expected_description}" f"{rfc2822_date(bookmark.date_added)}" f"{bookmark.url}" f"{''.join(categories)}" @@ -63,7 +70,7 @@ class FeedsTestCase(TestCase, BookmarkFactoryMixin): def test_all_returns_all_unarchived_bookmarks(self): bookmarks = [ self.setup_bookmark(description="test description"), - self.setup_bookmark(website_description="test website description"), + self.setup_bookmark(description=""), self.setup_bookmark(unread=True, description="test description"), ] self.setup_bookmark(is_archived=True) @@ -118,9 +125,7 @@ class FeedsTestCase(TestCase, BookmarkFactoryMixin): unread_bookmarks = [ self.setup_bookmark(unread=True, description="test description"), - self.setup_bookmark( - unread=True, website_description="test website description" - ), + self.setup_bookmark(unread=True, description=""), self.setup_bookmark(unread=True, description="test description"), ] diff --git a/bookmarks/tests/test_queries.py b/bookmarks/tests/test_queries.py index dc7158a..429cfdd 100644 --- a/bookmarks/tests/test_queries.py +++ b/bookmarks/tests/test_queries.py @@ -36,14 +36,6 @@ class QueriesTestCase(TestCase, BookmarkFactoryMixin): self.setup_bookmark(description=random_sentence(including_word="TERM1")), self.setup_bookmark(notes=random_sentence(including_word="term1")), self.setup_bookmark(notes=random_sentence(including_word="TERM1")), - self.setup_bookmark(website_title=random_sentence(including_word="term1")), - self.setup_bookmark(website_title=random_sentence(including_word="TERM1")), - self.setup_bookmark( - website_description=random_sentence(including_word="term1") - ), - self.setup_bookmark( - website_description=random_sentence(including_word="TERM1") - ), ] self.term1_term2_bookmarks = [ self.setup_bookmark(url="http://example.com/term1/term2"), @@ -55,30 +47,16 @@ class QueriesTestCase(TestCase, BookmarkFactoryMixin): description=random_sentence(including_word="term1"), title=random_sentence(including_word="term2"), ), - self.setup_bookmark( - website_title=random_sentence(including_word="term1"), - title=random_sentence(including_word="term2"), - ), - self.setup_bookmark( - website_description=random_sentence(including_word="term1"), - title=random_sentence(including_word="term2"), - ), ] self.tag1_bookmarks = [ self.setup_bookmark(tags=[tag1]), self.setup_bookmark(title=random_sentence(), tags=[tag1]), self.setup_bookmark(description=random_sentence(), tags=[tag1]), - self.setup_bookmark(website_title=random_sentence(), tags=[tag1]), - self.setup_bookmark(website_description=random_sentence(), tags=[tag1]), ] self.tag1_as_term_bookmarks = [ self.setup_bookmark(url="http://example.com/tag1"), self.setup_bookmark(title=random_sentence(including_word="tag1")), self.setup_bookmark(description=random_sentence(including_word="tag1")), - self.setup_bookmark(website_title=random_sentence(including_word="tag1")), - self.setup_bookmark( - website_description=random_sentence(including_word="tag1") - ), ] self.term1_tag1_bookmarks = [ self.setup_bookmark(url="http://example.com/term1", tags=[tag1]), @@ -88,12 +66,6 @@ class QueriesTestCase(TestCase, BookmarkFactoryMixin): self.setup_bookmark( description=random_sentence(including_word="term1"), tags=[tag1] ), - self.setup_bookmark( - website_title=random_sentence(including_word="term1"), tags=[tag1] - ), - self.setup_bookmark( - website_description=random_sentence(including_word="term1"), tags=[tag1] - ), ] self.tag2_bookmarks = [ self.setup_bookmark(tags=[tag2]), @@ -136,22 +108,6 @@ class QueriesTestCase(TestCase, BookmarkFactoryMixin): self.setup_bookmark( notes=random_sentence(including_word="TERM1"), tags=[self.setup_tag()] ), - self.setup_bookmark( - website_title=random_sentence(including_word="term1"), - tags=[self.setup_tag()], - ), - self.setup_bookmark( - website_title=random_sentence(including_word="TERM1"), - tags=[self.setup_tag()], - ), - self.setup_bookmark( - website_description=random_sentence(including_word="term1"), - tags=[self.setup_tag()], - ), - self.setup_bookmark( - website_description=random_sentence(including_word="TERM1"), - tags=[self.setup_tag()], - ), ] self.term1_term2_bookmarks = [ self.setup_bookmark( @@ -167,16 +123,6 @@ class QueriesTestCase(TestCase, BookmarkFactoryMixin): title=random_sentence(including_word="term2"), tags=[self.setup_tag()], ), - self.setup_bookmark( - website_title=random_sentence(including_word="term1"), - title=random_sentence(including_word="term2"), - tags=[self.setup_tag()], - ), - self.setup_bookmark( - website_description=random_sentence(including_word="term1"), - title=random_sentence(including_word="term2"), - tags=[self.setup_tag()], - ), ] self.tag1_bookmarks = [ self.setup_bookmark(tags=[tag1, self.setup_tag()]), @@ -184,21 +130,11 @@ class QueriesTestCase(TestCase, BookmarkFactoryMixin): self.setup_bookmark( description=random_sentence(), tags=[tag1, self.setup_tag()] ), - self.setup_bookmark( - website_title=random_sentence(), tags=[tag1, self.setup_tag()] - ), - self.setup_bookmark( - website_description=random_sentence(), tags=[tag1, self.setup_tag()] - ), ] self.tag1_as_term_bookmarks = [ self.setup_bookmark(url="http://example.com/tag1"), self.setup_bookmark(title=random_sentence(including_word="tag1")), self.setup_bookmark(description=random_sentence(including_word="tag1")), - self.setup_bookmark(website_title=random_sentence(including_word="tag1")), - self.setup_bookmark( - website_description=random_sentence(including_word="tag1") - ), ] self.term1_tag1_bookmarks = [ self.setup_bookmark( @@ -212,14 +148,6 @@ class QueriesTestCase(TestCase, BookmarkFactoryMixin): description=random_sentence(including_word="term1"), tags=[tag1, self.setup_tag()], ), - self.setup_bookmark( - website_title=random_sentence(including_word="term1"), - tags=[tag1, self.setup_tag()], - ), - self.setup_bookmark( - website_description=random_sentence(including_word="term1"), - tags=[tag1, self.setup_tag()], - ), ] self.tag2_bookmarks = [ self.setup_bookmark(tags=[tag2, self.setup_tag()]), @@ -1260,30 +1188,18 @@ class QueriesTestCase(TestCase, BookmarkFactoryMixin): self.setup_bookmark(title="A_1_2"), self.setup_bookmark(title="b_1_1"), self.setup_bookmark(title="B_1_2"), - self.setup_bookmark(title="", website_title="a_2_1"), - self.setup_bookmark(title="", website_title="A_2_2"), - self.setup_bookmark(title="", website_title="b_2_1"), - self.setup_bookmark(title="", website_title="B_2_2"), - self.setup_bookmark(title="", website_title="", url="a_3_1"), - self.setup_bookmark(title="", website_title="", url="A_3_2"), - self.setup_bookmark(title="", website_title="", url="b_3_1"), - self.setup_bookmark(title="", website_title="", url="B_3_2"), - self.setup_bookmark(title="a_4_1", website_title="0"), - self.setup_bookmark(title="A_4_2", website_title="0"), - self.setup_bookmark(title="b_4_1", website_title="0"), - self.setup_bookmark(title="B_4_2", website_title="0"), + self.setup_bookmark(title="", url="a_3_1"), + self.setup_bookmark(title="", url="A_3_2"), + self.setup_bookmark(title="", url="b_3_1"), + self.setup_bookmark(title="", url="B_3_2"), self.setup_bookmark(title="a_5_1", url="0"), self.setup_bookmark(title="A_5_2", url="0"), self.setup_bookmark(title="b_5_1", url="0"), self.setup_bookmark(title="B_5_2", url="0"), - self.setup_bookmark(title="", website_title="a_6_1", url="0"), - self.setup_bookmark(title="", website_title="A_6_2", url="0"), - self.setup_bookmark(title="", website_title="b_6_1", url="0"), - self.setup_bookmark(title="", website_title="B_6_2", url="0"), - self.setup_bookmark(title="a_7_1", website_title="0", url="0"), - self.setup_bookmark(title="A_7_2", website_title="0", url="0"), - self.setup_bookmark(title="b_7_1", website_title="0", url="0"), - self.setup_bookmark(title="B_7_2", website_title="0", url="0"), + self.setup_bookmark(title="", url="0"), + self.setup_bookmark(title="", url="0"), + self.setup_bookmark(title="", url="0"), + self.setup_bookmark(title="", url="0"), ] return bookmarks diff --git a/docs/src/content/docs/api.md b/docs/src/content/docs/api.md index 65bde9a..7a12bad 100644 --- a/docs/src/content/docs/api.md +++ b/docs/src/content/docs/api.md @@ -49,8 +49,6 @@ Example response: "title": "Example title", "description": "Example description", "notes": "Example notes", - "website_title": "Website title", - "website_description": "Website description", "web_archive_snapshot_url": "https://web.archive.org/web/20200926094623/https://example.com", "favicon_url": "http://127.0.0.1:8000/static/https_example_com.png", "preview_image_url": "http://127.0.0.1:8000/static/0ac5c53db923727765216a3a58e70522.jpg", @@ -87,6 +85,39 @@ GET /api/bookmarks// Retrieves a single bookmark by ID. +**Check** + +``` +GET /api/bookmarks/check/?url=https%3A%2F%2Fexample.com +``` + +Allows to check if a URL is already bookmarked. If the URL is already bookmarked, the `bookmark` property in the response holds the bookmark data, otherwise it is `null`. + +Also returns a `metadata` property that contains metadata scraped from the website. Finally, the `auto_tags` property contains the tag names that would be automatically added when creating a bookmark for that URL. + +Example response: + +```json +{ + "bookmark": { + "id": 1, + "url": "https://example.com", + "title": "Example title", + "description": "Example description", + ... + }, + "metadata": { + "title": "Scraped website title", + "description": "Scraped website description", + ... + }, + "auto_tags": [ + "tag1", + "tag2" + ] +} +``` + **Create** ``` @@ -96,6 +127,12 @@ 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: + +- 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. + Example payload: ```json