From 82b4268a263c500f460eb936b3d2965178f23b49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Sun, 12 Dec 2021 22:54:22 +0100 Subject: [PATCH] Ensure tag names don't contain spaces (#184) --- bookmarks/api/serializers.py | 4 ++-- bookmarks/models.py | 10 +++++++++- bookmarks/services/bookmarks.py | 6 +++--- bookmarks/tests/test_bookmarks_api.py | 12 ++++++++++++ bookmarks/tests/test_bookmarks_service.py | 24 +++++++++++------------ bookmarks/tests/test_importer.py | 13 ++++++++++++ bookmarks/tests/test_tags_model.py | 6 ++++++ bookmarks/views/bookmarks.py | 16 +++++++++++---- 8 files changed, 69 insertions(+), 22 deletions(-) diff --git a/bookmarks/api/serializers.py b/bookmarks/api/serializers.py index 7f8f65d..f52ee60 100644 --- a/bookmarks/api/serializers.py +++ b/bookmarks/api/serializers.py @@ -41,14 +41,14 @@ class BookmarkSerializer(serializers.ModelSerializer): bookmark.url = validated_data['url'] bookmark.title = validated_data['title'] bookmark.description = validated_data['description'] - tag_string = build_tag_string(validated_data['tag_names'], ' ') + tag_string = build_tag_string(validated_data['tag_names']) return create_bookmark(bookmark, tag_string, self.context['user']) def update(self, instance: Bookmark, validated_data): instance.url = validated_data['url'] instance.title = validated_data['title'] instance.description = validated_data['description'] - tag_string = build_tag_string(validated_data['tag_names'], ' ') + tag_string = build_tag_string(validated_data['tag_names']) return update_bookmark(instance, tag_string, self.context['user']) diff --git a/bookmarks/models.py b/bookmarks/models.py index 8527e06..7e978c0 100644 --- a/bookmarks/models.py +++ b/bookmarks/models.py @@ -20,11 +20,19 @@ class Tag(models.Model): return self.name +def sanitize_tag_name(tag_name: str): + # strip leading/trailing spaces + # replace inner spaces with replacement char + return tag_name.strip().replace(' ', '-') + + def parse_tag_string(tag_string: str, delimiter: str = ','): if not tag_string: return [] names = tag_string.strip().split(delimiter) - names = [name.strip() for name in names if name] + # remove empty names, sanitize remaining names + names = [sanitize_tag_name(name) for name in names if name] + # remove duplicates names = unique(names, str.lower) names.sort(key=str.lower) diff --git a/bookmarks/services/bookmarks.py b/bookmarks/services/bookmarks.py index 5171016..3408fd5 100644 --- a/bookmarks/services/bookmarks.py +++ b/bookmarks/services/bookmarks.py @@ -90,7 +90,7 @@ def delete_bookmarks(bookmark_ids: [Union[int, str]], current_user: User): def tag_bookmarks(bookmark_ids: [Union[int, str]], tag_string: str, current_user: User): sanitized_bookmark_ids = _sanitize_id_list(bookmark_ids) bookmarks = Bookmark.objects.filter(owner=current_user, id__in=sanitized_bookmark_ids) - tag_names = parse_tag_string(tag_string, ' ') + tag_names = parse_tag_string(tag_string) tags = get_or_create_tags(tag_names, current_user) for bookmark in bookmarks: @@ -103,7 +103,7 @@ def tag_bookmarks(bookmark_ids: [Union[int, str]], tag_string: str, current_user def untag_bookmarks(bookmark_ids: [Union[int, str]], tag_string: str, current_user: User): sanitized_bookmark_ids = _sanitize_id_list(bookmark_ids) bookmarks = Bookmark.objects.filter(owner=current_user, id__in=sanitized_bookmark_ids) - tag_names = parse_tag_string(tag_string, ' ') + tag_names = parse_tag_string(tag_string) tags = get_or_create_tags(tag_names, current_user) for bookmark in bookmarks: @@ -125,7 +125,7 @@ def _update_website_metadata(bookmark: Bookmark): def _update_bookmark_tags(bookmark: Bookmark, tag_string: str, user: User): - tag_names = parse_tag_string(tag_string, ' ') + tag_names = parse_tag_string(tag_string) tags = get_or_create_tags(tag_names, user) bookmark.tags.set(tags) diff --git a/bookmarks/tests/test_bookmarks_api.py b/bookmarks/tests/test_bookmarks_api.py index 83d839a..4bbd05d 100644 --- a/bookmarks/tests/test_bookmarks_api.py +++ b/bookmarks/tests/test_bookmarks_api.py @@ -60,6 +60,18 @@ 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_replaces_whitespace_in_tag_names(self): + data = { + 'url': 'https://example.com/', + 'title': 'Test title', + 'description': 'Test description', + 'tag_names': ['tag 1', 'tag 2'] + } + self.post(reverse('bookmarks:bookmark-list'), data, status.HTTP_201_CREATED) + bookmark = Bookmark.objects.get(url=data['url']) + tag_names = [tag.name for tag in bookmark.tags.all()] + self.assertListEqual(tag_names, ['tag-1', 'tag-2']) + def test_create_bookmark_minimal_payload(self): data = {'url': 'https://example.com/'} self.post(reverse('bookmarks:bookmark-list'), data, status.HTTP_201_CREATED) diff --git a/bookmarks/tests/test_bookmarks_service.py b/bookmarks/tests/test_bookmarks_service.py index 6219e39..d438491 100644 --- a/bookmarks/tests/test_bookmarks_service.py +++ b/bookmarks/tests/test_bookmarks_service.py @@ -21,7 +21,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): 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') - bookmark = create_bookmark(bookmark_data, 'tag1 tag2', self.user) + bookmark = create_bookmark(bookmark_data, 'tag1,tag2', self.user) mock_create_web_archive_snapshot.assert_called_once_with(bookmark.id, False) @@ -29,7 +29,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): with patch.object(tasks, 'create_web_archive_snapshot') as mock_create_web_archive_snapshot: bookmark = self.setup_bookmark() bookmark.url = 'https://example.com/updated' - update_bookmark(bookmark, 'tag1 tag2', self.user) + update_bookmark(bookmark, 'tag1,tag2', self.user) mock_create_web_archive_snapshot.assert_called_once_with(bookmark.id, True) @@ -37,7 +37,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): with patch.object(tasks, 'create_web_archive_snapshot') as mock_create_web_archive_snapshot: bookmark = self.setup_bookmark() bookmark.title = 'updated title' - update_bookmark(bookmark, 'tag1 tag2', self.user) + update_bookmark(bookmark, 'tag1,tag2', self.user) mock_create_web_archive_snapshot.assert_not_called() @@ -216,7 +216,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): tag1 = self.setup_tag() tag2 = self.setup_tag() - tag_bookmarks([bookmark1.id, bookmark2.id, bookmark3.id], f'{tag1.name} {tag2.name}', + tag_bookmarks([bookmark1.id, bookmark2.id, bookmark3.id], f'{tag1.name},{tag2.name}', self.get_or_create_test_user()) bookmark1.refresh_from_db() @@ -232,7 +232,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): bookmark2 = self.setup_bookmark() bookmark3 = self.setup_bookmark() - tag_bookmarks([bookmark1.id, bookmark2.id, bookmark3.id], 'tag1 tag2', self.get_or_create_test_user()) + tag_bookmarks([bookmark1.id, bookmark2.id, bookmark3.id], 'tag1,tag2', self.get_or_create_test_user()) bookmark1.refresh_from_db() bookmark2.refresh_from_db() @@ -257,7 +257,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): tag1 = self.setup_tag() tag2 = self.setup_tag() - tag_bookmarks([bookmark1.id, bookmark3.id], f'{tag1.name} {tag2.name}', self.get_or_create_test_user()) + tag_bookmarks([bookmark1.id, bookmark3.id], f'{tag1.name},{tag2.name}', self.get_or_create_test_user()) bookmark1.refresh_from_db() bookmark2.refresh_from_db() @@ -275,7 +275,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): tag1 = self.setup_tag() tag2 = self.setup_tag() - tag_bookmarks([bookmark1.id, bookmark2.id, inaccessible_bookmark.id], f'{tag1.name} {tag2.name}', + tag_bookmarks([bookmark1.id, bookmark2.id, inaccessible_bookmark.id], f'{tag1.name},{tag2.name}', self.get_or_create_test_user()) bookmark1.refresh_from_db() @@ -293,7 +293,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): tag1 = self.setup_tag() tag2 = self.setup_tag() - tag_bookmarks([str(bookmark1.id), bookmark2.id, str(bookmark3.id)], f'{tag1.name} {tag2.name}', + tag_bookmarks([str(bookmark1.id), bookmark2.id, str(bookmark3.id)], f'{tag1.name},{tag2.name}', self.get_or_create_test_user()) self.assertCountEqual(bookmark1.tags.all(), [tag1, tag2]) @@ -307,7 +307,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): bookmark2 = self.setup_bookmark(tags=[tag1, tag2]) bookmark3 = self.setup_bookmark(tags=[tag1, tag2]) - untag_bookmarks([bookmark1.id, bookmark2.id, bookmark3.id], f'{tag1.name} {tag2.name}', + untag_bookmarks([bookmark1.id, bookmark2.id, bookmark3.id], f'{tag1.name},{tag2.name}', self.get_or_create_test_user()) bookmark1.refresh_from_db() @@ -325,7 +325,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): bookmark2 = self.setup_bookmark(tags=[tag1, tag2]) bookmark3 = self.setup_bookmark(tags=[tag1, tag2]) - untag_bookmarks([bookmark1.id, bookmark3.id], f'{tag1.name} {tag2.name}', self.get_or_create_test_user()) + untag_bookmarks([bookmark1.id, bookmark3.id], f'{tag1.name},{tag2.name}', self.get_or_create_test_user()) bookmark1.refresh_from_db() bookmark2.refresh_from_db() @@ -343,7 +343,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): bookmark2 = self.setup_bookmark(tags=[tag1, tag2]) inaccessible_bookmark = self.setup_bookmark(user=other_user, tags=[tag1, tag2]) - untag_bookmarks([bookmark1.id, bookmark2.id, inaccessible_bookmark.id], f'{tag1.name} {tag2.name}', + untag_bookmarks([bookmark1.id, bookmark2.id, inaccessible_bookmark.id], f'{tag1.name},{tag2.name}', self.get_or_create_test_user()) bookmark1.refresh_from_db() @@ -361,7 +361,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin): bookmark2 = self.setup_bookmark(tags=[tag1, tag2]) bookmark3 = self.setup_bookmark(tags=[tag1, tag2]) - untag_bookmarks([str(bookmark1.id), bookmark2.id, str(bookmark3.id)], f'{tag1.name} {tag2.name}', + untag_bookmarks([str(bookmark1.id), bookmark2.id, str(bookmark3.id)], f'{tag1.name},{tag2.name}', self.get_or_create_test_user()) self.assertCountEqual(bookmark1.tags.all(), []) diff --git a/bookmarks/tests/test_importer.py b/bookmarks/tests/test_importer.py index d67638e..bc6934c 100644 --- a/bookmarks/tests/test_importer.py +++ b/bookmarks/tests/test_importer.py @@ -2,6 +2,7 @@ from unittest.mock import patch from django.test import TestCase +from bookmarks.models import Tag from bookmarks.services import tasks from bookmarks.services.importer import import_netscape_html from bookmarks.tests.helpers import BookmarkFactoryMixin, disable_logging @@ -20,6 +21,18 @@ class ImporterTestCase(TestCase, BookmarkFactoryMixin):

''' + def test_replace_whitespace_in_tag_names(self): + test_html = self.create_import_html(f''' +

Example.com +
Example.com + ''') + import_netscape_html(test_html, self.get_or_create_test_user()) + + tags = Tag.objects.all() + tag_names = [tag.name for tag in tags] + + self.assertListEqual(tag_names, ['tag-1', 'tag-2', 'tag-3']) + @disable_logging def test_validate_empty_or_missing_bookmark_url(self): test_html = self.create_import_html(f''' diff --git a/bookmarks/tests/test_tags_model.py b/bookmarks/tests/test_tags_model.py index a41e70f..3bd38f4 100644 --- a/bookmarks/tests/test_tags_model.py +++ b/bookmarks/tests/test_tags_model.py @@ -25,3 +25,9 @@ class TagTestCase(TestCase): def test_parse_tag_string_deduplicates_tag_names(self): self.assertEqual(len(parse_tag_string('book,book,Book,BOOK')), 1) + def test_parse_tag_string_handles_duplicate_separators(self): + self.assertCountEqual(parse_tag_string('book,,movie,,,album'), ['album', 'book', 'movie']) + + def test_parse_tag_string_replaces_whitespace_within_names(self): + self.assertCountEqual(parse_tag_string('travel guide, book recommendations'), + ['travel-guide', 'book-recommendations']) diff --git a/bookmarks/views/bookmarks.py b/bookmarks/views/bookmarks.py index ea0cae9..d665fe2 100644 --- a/bookmarks/views/bookmarks.py +++ b/bookmarks/views/bookmarks.py @@ -68,6 +68,12 @@ def generate_return_url(base_url, page, query_string): return urllib.parse.quote_plus(return_url) +def convert_tag_string(tag_string: str): + # Tag strings coming from inputs are space-separated, however services.bookmarks functions expect comma-separated + # strings + return tag_string.replace(' ', ',') + + @login_required def new(request): initial_url = request.GET.get('url') @@ -78,7 +84,8 @@ def new(request): auto_close = form.data['auto_close'] if form.is_valid(): current_user = request.user - create_bookmark(form.save(commit=False), form.data['tag_string'], current_user) + tag_string = convert_tag_string(form.data['tag_string']) + create_bookmark(form.save(commit=False), tag_string, current_user) if auto_close: return HttpResponseRedirect(reverse('bookmarks:close')) else: @@ -107,7 +114,8 @@ def edit(request, bookmark_id: int): form = BookmarkForm(request.POST, instance=bookmark) return_url = form.data['return_url'] if form.is_valid(): - update_bookmark(form.save(commit=False), form.data['tag_string'], request.user) + tag_string = convert_tag_string(form.data['tag_string']) + update_bookmark(form.save(commit=False), tag_string, request.user) return HttpResponseRedirect(return_url) else: return_url = request.GET.get('return_url') @@ -166,10 +174,10 @@ def bulk_edit(request): if 'bulk_delete' in request.POST: delete_bookmarks(bookmark_ids, request.user) if 'bulk_tag' in request.POST: - tag_string = request.POST['bulk_tag_string'] + tag_string = convert_tag_string(request.POST['bulk_tag_string']) tag_bookmarks(bookmark_ids, tag_string, request.user) if 'bulk_untag' in request.POST: - tag_string = request.POST['bulk_tag_string'] + tag_string = convert_tag_string(request.POST['bulk_tag_string']) untag_bookmarks(bookmark_ids, tag_string, request.user) return_url = request.GET.get('return_url')