Update unread flag when saving duplicate URL (#306)

Co-authored-by: Sascha Ißbrücker <sascha.issbruecker@gmail.com>

bookmarks/services/bookmarks.py

@@ -116,6 +116,7 @@ def untag_bookmarks(bookmark_ids: [Union[int, str]], tag_string: str, current_us
 def _merge_bookmark_data(from_bookmark: Bookmark, to_bookmark: Bookmark):
     to_bookmark.title = from_bookmark.title
     to_bookmark.description = from_bookmark.description
+    to_bookmark.unread = from_bookmark.unread
 
 
 def _update_website_metadata(bookmark: Bookmark):

bookmarks/tests/test_bookmarks_api.py

@@ -46,6 +46,24 @@ def assertBookmarkListEqual(self, data_list, bookmarks):
 
         self.assertCountEqual(data_list, expectations)
 
+    def test_list_bookmarks(self):
+        response = self.get(reverse('bookmarks:bookmark-list'), expected_status_code=status.HTTP_200_OK)
+        self.assertBookmarkListEqual(response.data['results'], [self.bookmark1, self.bookmark2, self.bookmark3])
+
+    def test_list_bookmarks_should_filter_by_query(self):
+        response = self.get(reverse('bookmarks:bookmark-list') + '?q=#' + self.tag1.name,
+                            expected_status_code=status.HTTP_200_OK)
+        self.assertBookmarkListEqual(response.data['results'], [self.bookmark1])
+
+    def test_list_archived_bookmarks_does_not_return_unarchived_bookmarks(self):
+        response = self.get(reverse('bookmarks:bookmark-archived'), expected_status_code=status.HTTP_200_OK)
+        self.assertBookmarkListEqual(response.data['results'], [self.archived_bookmark1, self.archived_bookmark2])
+
+    def test_list_archived_bookmarks_should_filter_by_query(self):
+        response = self.get(reverse('bookmarks:bookmark-archived') + '?q=#' + self.tag1.name,
+                            expected_status_code=status.HTTP_200_OK)
+        self.assertBookmarkListEqual(response.data['results'], [self.archived_bookmark1])
+
     def test_create_bookmark(self):
         data = {
             'url': 'https://example.com/',
@@ -66,6 +84,29 @@ def test_create_bookmark(self):
         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_with_same_url_updates_existing_bookmark(self):
+        original_bookmark = self.setup_bookmark()
+        data = {
+            'url': original_bookmark.url,
+            'title': 'Updated title',
+            'description': 'Updated description',
+            'unread': True,
+            'is_archived': True,
+            'tag_names': ['tag1', 'tag2']
+        }
+        self.post(reverse('bookmarks:bookmark-list'), data, status.HTTP_201_CREATED)
+        bookmark = Bookmark.objects.get(url=data['url'])
+        self.assertEqual(bookmark.id, original_bookmark.id)
+        self.assertEqual(bookmark.url, data['url'])
+        self.assertEqual(bookmark.title, data['title'])
+        self.assertEqual(bookmark.description, data['description'])
+        # Saving a duplicate bookmark should not modify archive flag - right?
+        self.assertFalse(bookmark.is_archived)
+        self.assertEqual(bookmark.unread, data['unread'])
+        self.assertEqual(bookmark.tags.count(), 2)
+        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_replaces_whitespace_in_tag_names(self):
         data = {
             'url': 'https://example.com/',
@@ -82,22 +123,6 @@ def test_create_bookmark_minimal_payload(self):
         data = {'url': 'https://example.com/'}
         self.post(reverse('bookmarks:bookmark-list'), data, status.HTTP_201_CREATED)
 
-    def test_list_bookmarks(self):
-        response = self.get(reverse('bookmarks:bookmark-list'), expected_status_code=status.HTTP_200_OK)
-        self.assertBookmarkListEqual(response.data['results'], [self.bookmark1, self.bookmark2, self.bookmark3])
-
-    def test_list_bookmarks_should_filter_by_query(self):
-        response = self.get(reverse('bookmarks:bookmark-list') + '?q=#' + self.tag1.name, expected_status_code=status.HTTP_200_OK)
-        self.assertBookmarkListEqual(response.data['results'], [self.bookmark1])
-
-    def test_list_archived_bookmarks_does_not_return_unarchived_bookmarks(self):
-        response = self.get(reverse('bookmarks:bookmark-archived'), expected_status_code=status.HTTP_200_OK)
-        self.assertBookmarkListEqual(response.data['results'], [self.archived_bookmark1, self.archived_bookmark2])
-
-    def test_list_archived_bookmarks_should_filter_by_query(self):
-        response = self.get(reverse('bookmarks:bookmark-archived') + '?q=#' + self.tag1.name, expected_status_code=status.HTTP_200_OK)
-        self.assertBookmarkListEqual(response.data['results'], [self.archived_bookmark1])
-
     def test_create_archived_bookmark(self):
         data = {
             'url': 'https://example.com/',
@@ -117,35 +142,22 @@ def test_create_archived_bookmark(self):
         self.assertEqual(bookmark.tags.filter(name=data['tag_names'][1]).count(), 1)
 
     def test_create_bookmark_is_not_archived_by_default(self):
-        data = {
-            'url': 'https://example.com/',
-        }
+        data = {'url': 'https://example.com/'}
         self.post(reverse('bookmarks:bookmark-list'), data, status.HTTP_201_CREATED)
         bookmark = Bookmark.objects.get(url=data['url'])
         self.assertFalse(bookmark.is_archived)
 
     def test_create_unread_bookmark(self):
-        data = {
-            'url': 'https://example.com/',
-            'unread': True,
-        }
+        data = {'url': 'https://example.com/', 'unread': True}
         self.post(reverse('bookmarks:bookmark-list'), data, status.HTTP_201_CREATED)
         bookmark = Bookmark.objects.get(url=data['url'])
         self.assertTrue(bookmark.unread)
 
     def test_create_bookmark_is_not_unread_by_default(self):
-        data = {
-            'url': 'https://example.com/',
-        }
-        self.post(reverse('bookmarks:bookmark-list'), data, status.HTTP_201_CREATED)
-        bookmark = Bookmark.objects.get(url=data['url'])
-        self.assertFalse(bookmark.unread)
-
-    def test_create_bookmark_minimal_payload_does_not_archive(self):
         data = {'url': 'https://example.com/'}
         self.post(reverse('bookmarks:bookmark-list'), data, status.HTTP_201_CREATED)
         bookmark = Bookmark.objects.get(url=data['url'])
-        self.assertFalse(bookmark.is_archived)
+        self.assertFalse(bookmark.unread)
 
     def test_get_bookmark(self):
         url = reverse('bookmarks:bookmark-detail', args=[self.bookmark1.id])
@@ -174,6 +186,13 @@ def test_update_bookmark_with_minimal_payload_clears_all_fields(self):
         self.assertEqual(updated_bookmark.description, '')
         self.assertEqual(updated_bookmark.tag_names, [])
 
+    def test_update_bookmark_unread_flag(self):
+        data = {'url': 'https://example.com/', 'unread': True}
+        url = reverse('bookmarks:bookmark-detail', args=[self.bookmark1.id])
+        self.put(url, data, expected_status_code=status.HTTP_200_OK)
+        updated_bookmark = Bookmark.objects.get(id=self.bookmark1.id)
+        self.assertEqual(updated_bookmark.unread, True)
+
     def test_patch_bookmark(self):
         data = {'url': 'https://example.com'}
         url = reverse('bookmarks:bookmark-detail', args=[self.bookmark1.id])

bookmarks/tests/test_bookmarks_service.py

@@ -18,6 +18,23 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin):
     def setUp(self) -> None:
         self.get_or_create_test_user()
 
+    def test_create_should_update_existing_bookmark_with_same_url(self):
+        original_bookmark = self.setup_bookmark(url='https://example.com', unread=False)
+        bookmark_data = Bookmark(url='https://example.com',
+                                 title='Updated Title',
+                                 description='Updated description',
+                                 unread=True,
+                                 is_archived=True)
+        updated_bookmark = create_bookmark(bookmark_data, '', self.get_or_create_test_user())
+
+        self.assertEqual(Bookmark.objects.count(), 1)
+        self.assertEqual(updated_bookmark.id, original_bookmark.id)
+        self.assertEqual(updated_bookmark.title, bookmark_data.title)
+        self.assertEqual(updated_bookmark.description, bookmark_data.description)
+        self.assertEqual(updated_bookmark.unread, bookmark_data.unread)
+        # Saving a duplicate bookmark should not modify archive flag - right?
+        self.assertFalse(updated_bookmark.is_archived)
+
     def test_create_should_create_web_archive_snapshot(self):
         with patch.object(tasks, 'create_web_archive_snapshot') as mock_create_web_archive_snapshot:
             bookmark_data = Bookmark(url='https://example.com')