Skip updating website metadata on edit unless URL has changed (#318)

* Skip updating website metadata on edit unless URL has changed

* Prevent form fetching metadata when editing existing bookmark

bookmarks/models.py

@@ -101,6 +101,11 @@ class BookmarkForm(forms.ModelForm):
                             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
@@ -108,7 +113,17 @@ class BookmarkForm(forms.ModelForm):
 
     class Meta:
         model = Bookmark
-        fields = ['url', 'tag_string', 'title', 'description', 'unread', 'shared', 'auto_close']
+        fields = [
+            'url',
+            'tag_string',
+            'title',
+            'description',
+            'website_title',
+            'website_description',
+            'unread',
+            'shared',
+            'auto_close',
+        ]
 
 
 class BookmarkFilters:

bookmarks/services/bookmarks.py

@@ -5,7 +5,7 @@ from django.utils import timezone
 
 from bookmarks.models import Bookmark, parse_tag_string
 from bookmarks.services.tags import get_or_create_tags
-from bookmarks.services.website_loader import load_website_metadata
+from bookmarks.services import website_loader
 from bookmarks.services import tasks
 
 
@@ -38,16 +38,16 @@ def update_bookmark(bookmark: Bookmark, tag_string, current_user: User):
     # Detect URL change
     original_bookmark = Bookmark.objects.get(id=bookmark.id)
     has_url_changed = original_bookmark.url != bookmark.url
-    # Update website info
-    _update_website_metadata(bookmark)
     # Update tag list
     _update_bookmark_tags(bookmark, tag_string, current_user)
     # Update dates
     bookmark.date_modified = timezone.now()
     bookmark.save()
-    # Update web archive snapshot, if URL changed
     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)
 
     return bookmark
 
@@ -121,7 +121,7 @@ def _merge_bookmark_data(from_bookmark: Bookmark, to_bookmark: Bookmark):
 
 
 def _update_website_metadata(bookmark: Bookmark):
-    metadata = load_website_metadata(bookmark.url)
+    metadata = website_loader.load_website_metadata(bookmark.url)
     bookmark.website_title = metadata.title
     bookmark.website_description = metadata.description
 

bookmarks/templates/bookmarks/form.html

@@ -3,6 +3,8 @@
 
 <div class="bookmarks-form">
     {% csrf_token %}
+    {{ form.website_title }}
+    {{ form.website_description }}
     {{ form.auto_close|attr:"type:hidden" }}
     <div class="form-group {% if form.url.errors %}has-error{% endif %}">
         <label for="{{ form.url.id_for_label }}" class="form-label">URL</label>
@@ -126,6 +128,8 @@
             const urlInput = document.getElementById('{{ form.url.id_for_label }}');
             const titleInput = document.getElementById('{{ form.title.id_for_label }}');
             const descriptionInput = document.getElementById('{{ form.description.id_for_label }}');
+            const websiteTitleInput = document.getElementById('{{ form.website_title.id_for_label }}');
+            const websiteDescriptionInput = document.getElementById('{{ form.website_description.id_for_label }}');
             const editedBookmarkId = {{ bookmark_id }};
 
             function toggleLoadingIcon(input, show) {
@@ -182,8 +186,19 @@
                 });
             }
 
-            if (urlInput.value) checkUrl();
+            // Fetch initial website data if we have a URL, and we are not editing an existing bookmark
+            // For existing bookmarks we get the website metadata through hidden inputs
+            if (urlInput.value && !editedBookmarkId) {
+              checkUrl();
+            }
             urlInput.addEventListener('input', checkUrl);
+
+            // Set initial website title and description for edited bookmarks
+            if (editedBookmarkId) {
+              updatePlaceholder(titleInput, websiteTitleInput.value);
+              updatePlaceholder(descriptionInput, websiteDescriptionInput.value);
+            }
+
             setupEditAutoValueButton(titleInput);
             setupEditAutoValueButton(descriptionInput);
         })();

bookmarks/tests/test_bookmark_edit_view.py

@@ -73,32 +73,43 @@ class BookmarkEditViewTestCase(TestCase, BookmarkFactoryMixin):
     def test_should_prefill_bookmark_form_fields(self):
         tag1 = self.setup_tag()
         tag2 = self.setup_tag()
-        bookmark = self.setup_bookmark(tags=[tag1, tag2], title='edited title', description='edited description')
+        bookmark = self.setup_bookmark(tags=[tag1, tag2], title='edited title', description='edited description',
+                                       website_title='website title', website_description='website description')
 
         response = self.client.get(reverse('bookmarks:edit', args=[bookmark.id]))
         html = response.content.decode()
 
-        self.assertInHTML('<input type="text" name="url" '
+        self.assertInHTML(f'''
-                          'value="{0}" placeholder=" " '
+            <input type="text" name="url" value="{bookmark.url}" placeholder=" "
-                          'autofocus class="form-input" required '
+                    autofocus class="form-input" required id="id_url">   
-                          'id="id_url">'.format(bookmark.url),
+        ''', html)
-                          html)
 
         tag_string = build_tag_string(bookmark.tag_names, ' ')
-        self.assertInHTML('<input type="text" name="tag_string" '
+        self.assertInHTML(f'''
-                          'value="{0}" autocomplete="off" '
+            <input type="text" name="tag_string" value="{tag_string}" 
-                          'class="form-input" '
+                    autocomplete="off" class="form-input" id="id_tag_string">
-                          'id="id_tag_string">'.format(tag_string),
+        ''', html)
-                          html)
 
-        self.assertInHTML('<input type="text" name="title" maxlength="512" '
+        self.assertInHTML(f'''
-                          'autocomplete="off" class="form-input" '
+            <input type="text" name="title" value="{bookmark.title}" maxlength="512" autocomplete="off" 
-                          'value="{0}" id="id_title">'.format(bookmark.title),
+                    class="form-input" id="id_title">
-                          html)
+        ''', html)
 
-        self.assertInHTML('<textarea name="description" cols="40" rows="2" class="form-input" id="id_description">{0}'
+        self.assertInHTML(f'''
-                          '</textarea>'.format(bookmark.description),
+            <textarea name="description" cols="40" rows="2" class="form-input" id="id_description">
-                          html)
+                {bookmark.description}
+            </textarea>
+        ''', html)
+
+        self.assertInHTML(f'''
+            <input type="hidden" name="website_title"  id="id_website_title"
+                    value="{bookmark.website_title}">
+        ''', html)
+
+        self.assertInHTML(f'''
+            <input type="hidden" name="website_description"  id="id_website_description"
+                    value="{bookmark.website_description}">
+        ''', html)
 
     def test_should_redirect_to_return_url(self):
         bookmark = self.setup_bookmark()
@@ -173,4 +184,3 @@ class BookmarkEditViewTestCase(TestCase, BookmarkFactoryMixin):
               <span>Share</span>
             </label>            
         ''', html, count=1)
-

bookmarks/tests/test_bookmarks_service.py

@@ -7,6 +7,7 @@ from django.utils import timezone
 from bookmarks.models import Bookmark, Tag
 from bookmarks.services.bookmarks import create_bookmark, update_bookmark, archive_bookmark, archive_bookmarks, \
     unarchive_bookmark, unarchive_bookmarks, delete_bookmarks, tag_bookmarks, untag_bookmarks
+from bookmarks.services import website_loader
 from bookmarks.tests.helpers import BookmarkFactoryMixin
 from bookmarks.services import tasks
 
@@ -60,6 +61,22 @@ 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:
+            bookmark = self.setup_bookmark()
+            bookmark.url = 'https://example.com/updated'
+            update_bookmark(bookmark, 'tag1,tag2', self.user)
+
+            mock_load_website_metadata.assert_called_once()
+
+    def test_update_should_not_update_website_metadata_if_url_did_not_change(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)
+
+            mock_load_website_metadata.assert_not_called()
+
     def test_archive_bookmark(self):
         bookmark = Bookmark(
             url='https://example.com',