From dd5e65ecd7e9537500d524cf72fa403c722bbed3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Thu, 4 Aug 2022 20:31:24 +0200 Subject: [PATCH] Display selected tags in tag cloud (#307) * Add links to remove tags from current query * Display selected tags in tag cloud * Add tag cloud tests * Fix tag cloud in archive * Add tests for bookmark views * Expose parse query string * Improve tag cloud tests * Cleanup * Fix rebase issues * Ignore casing when removing tags from query Co-authored-by: Jon Hauris --- bookmarks/queries.py | 4 +- bookmarks/styles/bookmarks.scss | 14 ++- bookmarks/templates/bookmarks/archive.html | 2 +- .../templates/bookmarks/bookmark_list.html | 2 +- bookmarks/templates/bookmarks/index.html | 2 +- bookmarks/templates/bookmarks/shared.html | 2 +- bookmarks/templates/bookmarks/tag_cloud.html | 46 ++++--- bookmarks/templatetags/bookmarks.py | 26 ++-- bookmarks/templatetags/shared.py | 18 ++- bookmarks/tests/helpers.py | 6 + .../tests/test_bookmark_archived_view.py | 41 +++++-- bookmarks/tests/test_bookmark_index_view.py | 40 ++++-- bookmarks/tests/test_tag_cloud_tag.py | 116 +++++++++++++++--- bookmarks/views/bookmarks.py | 18 ++- 14 files changed, 271 insertions(+), 66 deletions(-) diff --git a/bookmarks/queries.py b/bookmarks/queries.py index 2dd8568..0a72607 100644 --- a/bookmarks/queries.py +++ b/bookmarks/queries.py @@ -47,7 +47,7 @@ def _base_bookmarks_query(user: Optional[User], query_string: str) -> QuerySet: query_set = query_set.filter(owner=user) # Split query into search terms and tags - query = _parse_query_string(query_string) + query = parse_query_string(query_string) # Filter for search terms and tags for term in query['search_terms']: @@ -117,7 +117,7 @@ def get_user_tags(user: User): return Tag.objects.filter(owner=user).all() -def _parse_query_string(query_string): +def parse_query_string(query_string): # Sanitize query params if not query_string: query_string = '' diff --git a/bookmarks/styles/bookmarks.scss b/bookmarks/styles/bookmarks.scss index 252125e..8574d49 100644 --- a/bookmarks/styles/bookmarks.scss +++ b/bookmarks/styles/bookmarks.scss @@ -91,8 +91,18 @@ ul.bookmark-list { .tag-cloud { - a, a:visited:hover { - color: $alternative-color; + .selected-tags { + margin-bottom: 0.8rem; + + a, a:visited:hover { + color: $error-color; + } + } + + .unselected-tags { + a, a:visited:hover { + color: $alternative-color; + } } .group { diff --git a/bookmarks/templates/bookmarks/archive.html b/bookmarks/templates/bookmarks/archive.html index c0405ce..0c8b359 100644 --- a/bookmarks/templates/bookmarks/archive.html +++ b/bookmarks/templates/bookmarks/archive.html @@ -36,7 +36,7 @@

Tags

- {% tag_cloud tags %} + {% tag_cloud tags selected_tags %} diff --git a/bookmarks/templates/bookmarks/bookmark_list.html b/bookmarks/templates/bookmarks/bookmark_list.html index c32f193..261a3e2 100644 --- a/bookmarks/templates/bookmarks/bookmark_list.html +++ b/bookmarks/templates/bookmarks/bookmark_list.html @@ -15,7 +15,7 @@ {% if bookmark.tag_names %} {% for tag_name in bookmark.tag_names %} - {{ tag_name|hash_tag }} + {{ tag_name|hash_tag }} {% endfor %} {% endif %} diff --git a/bookmarks/templates/bookmarks/index.html b/bookmarks/templates/bookmarks/index.html index 487b040..60696b5 100644 --- a/bookmarks/templates/bookmarks/index.html +++ b/bookmarks/templates/bookmarks/index.html @@ -36,7 +36,7 @@

Tags

- {% tag_cloud tags %} + {% tag_cloud tags selected_tags %} diff --git a/bookmarks/templates/bookmarks/shared.html b/bookmarks/templates/bookmarks/shared.html index 578a827..3fa0d4c 100644 --- a/bookmarks/templates/bookmarks/shared.html +++ b/bookmarks/templates/bookmarks/shared.html @@ -39,7 +39,7 @@

Tags

- {% tag_cloud tags %} + {% tag_cloud tags selected_tags %} diff --git a/bookmarks/templates/bookmarks/tag_cloud.html b/bookmarks/templates/bookmarks/tag_cloud.html index e48ffe7..579d2b5 100644 --- a/bookmarks/templates/bookmarks/tag_cloud.html +++ b/bookmarks/templates/bookmarks/tag_cloud.html @@ -1,23 +1,35 @@ {% load shared %}
- {% for group in groups %} -

- {% for tag in group.tags %} - {# Highlight first char of first tag in group #} - {% if forloop.counter == 1 %} - - {{ tag.name|first_char }}{{ tag.name|remaining_chars:1 }} - - {% else %} - {# Render remaining tags normally #} - - {{ tag.name }} - - {% endif %} + {% if has_selected_tags %} +

+ {% for tag in selected_tags %} + + -{{ tag.name }} + {% endfor %}

- {% endfor %} + {% endif %} +
+ {% for group in groups %} +

+ {% for tag in group.tags %} + {# Highlight first char of first tag in group #} + {% if forloop.counter == 1 %} + + {{ tag.name|first_char }}{{ tag.name|remaining_chars:1 }} + + {% else %} + {# Render remaining tags normally #} + + {{ tag.name }} + + {% endif %} + {% endfor %} +

+ {% endfor %} +
diff --git a/bookmarks/templatetags/bookmarks.py b/bookmarks/templatetags/bookmarks.py index b5dc3c6..793c520 100644 --- a/bookmarks/templatetags/bookmarks.py +++ b/bookmarks/templatetags/bookmarks.py @@ -1,4 +1,4 @@ -from typing import List +from typing import List, Set from django import template from django.core.paginator import Page @@ -26,14 +26,9 @@ class TagGroup: self.char = char -def create_tag_groups(tags: List[Tag]): - # Only display each tag name once, ignoring casing - # This covers cases where the tag cloud contains shared tags with duplicate names - # Also means that the cloud can not make assumptions that it will necessarily contain - # all tags of the current user - unique_tags = unique(tags, key=lambda x: str.lower(x.name)) +def create_tag_groups(tags: Set[Tag]): # Ensure groups, as well as tags within groups, are ordered alphabetically - sorted_tags = sorted(unique_tags, key=lambda x: str.lower(x.name)) + sorted_tags = sorted(tags, key=lambda x: str.lower(x.name)) group = None groups = [] @@ -51,10 +46,21 @@ def create_tag_groups(tags: List[Tag]): @register.inclusion_tag('bookmarks/tag_cloud.html', name='tag_cloud', takes_context=True) -def tag_cloud(context, tags: List[Tag]): - groups = create_tag_groups(tags) +def tag_cloud(context, tags: List[Tag], selected_tags: List[Tag]): + # Only display each tag name once, ignoring casing + # This covers cases where the tag cloud contains shared tags with duplicate names + # Also means that the cloud can not make assumptions that it will necessarily contain + # all tags of the current user + unique_tags = unique(tags, key=lambda x: str.lower(x.name)) + unique_selected_tags = unique(selected_tags, key=lambda x: str.lower(x.name)) + + has_selected_tags = len(unique_selected_tags) > 0 + unselected_tags = set(unique_tags).symmetric_difference(unique_selected_tags) + groups = create_tag_groups(unselected_tags) return { 'groups': groups, + 'selected_tags': unique_selected_tags, + 'has_selected_tags': has_selected_tags, } diff --git a/bookmarks/templatetags/shared.py b/bookmarks/templatetags/shared.py index 65aea28..9e136db 100644 --- a/bookmarks/templatetags/shared.py +++ b/bookmarks/templatetags/shared.py @@ -17,7 +17,7 @@ def update_query_string(context, **kwargs): @register.simple_tag(takes_context=True) -def append_query_param(context, **kwargs): +def append_to_query_param(context, **kwargs): query = context.request.GET.copy() # Append to or create query param @@ -32,6 +32,22 @@ def append_query_param(context, **kwargs): return query.urlencode() +@register.simple_tag(takes_context=True) +def remove_from_query_param(context, **kwargs): + query = context.request.GET.copy() + + # Remove item from query param + for key in kwargs: + if query.__contains__(key): + value = query.__getitem__(key) + parts = value.split() + part_to_remove = kwargs[key] + updated_parts = [part for part in parts if str.lower(part) != str.lower(part_to_remove)] + updated_value = ' '.join(updated_parts) + query.__setitem__(key, updated_value) + + return query.urlencode() + @register.simple_tag(takes_context=True) def replace_query_param(context, **kwargs): query = context.request.GET.copy() diff --git a/bookmarks/tests/helpers.py b/bookmarks/tests/helpers.py index 058c2fc..ef57ed1 100644 --- a/bookmarks/tests/helpers.py +++ b/bookmarks/tests/helpers.py @@ -2,6 +2,7 @@ import random import logging from typing import List +from bs4 import BeautifulSoup from django.contrib.auth.models import User from django.utils import timezone from django.utils.crypto import get_random_string @@ -80,6 +81,11 @@ class BookmarkFactoryMixin: return user +class HtmlTestMixin: + def make_soup(self, html: str): + return BeautifulSoup(html, features="html.parser") + + class LinkdingApiTestCase(APITestCase): def get(self, url, expected_status_code=status.HTTP_200_OK): response = self.client.get(url) diff --git a/bookmarks/tests/test_bookmark_archived_view.py b/bookmarks/tests/test_bookmark_archived_view.py index 12ff174..6c02f63 100644 --- a/bookmarks/tests/test_bookmark_archived_view.py +++ b/bookmarks/tests/test_bookmark_archived_view.py @@ -1,18 +1,20 @@ +from typing import List + from django.contrib.auth.models import User from django.test import TestCase from django.urls import reverse from bookmarks.models import Bookmark, Tag, UserProfile -from bookmarks.tests.helpers import BookmarkFactoryMixin +from bookmarks.tests.helpers import BookmarkFactoryMixin, HtmlTestMixin -class BookmarkArchivedViewTestCase(TestCase, BookmarkFactoryMixin): +class BookmarkArchivedViewTestCase(TestCase, BookmarkFactoryMixin, HtmlTestMixin): def setUp(self) -> None: user = self.get_or_create_test_user() self.client.force_login(user) - def assertVisibleBookmarks(self, response, bookmarks: [Bookmark], link_target: str = '_blank'): + def assertVisibleBookmarks(self, response, bookmarks: List[Bookmark], link_target: str = '_blank'): html = response.content.decode() self.assertContains(response, 'data-is-bookmark-item', count=len(bookmarks)) @@ -22,7 +24,7 @@ class BookmarkArchivedViewTestCase(TestCase, BookmarkFactoryMixin): html ) - def assertInvisibleBookmarks(self, response, bookmarks: [Bookmark], link_target: str = '_blank'): + def assertInvisibleBookmarks(self, response, bookmarks: List[Bookmark], link_target: str = '_blank'): html = response.content.decode() for bookmark in bookmarks: @@ -32,16 +34,27 @@ class BookmarkArchivedViewTestCase(TestCase, BookmarkFactoryMixin): count=0 ) - def assertVisibleTags(self, response, tags: [Tag]): + def assertVisibleTags(self, response, tags: List[Tag]): self.assertContains(response, 'data-is-tag-item', count=len(tags)) for tag in tags: self.assertContains(response, tag.name) - def assertInvisibleTags(self, response, tags: [Tag]): + def assertInvisibleTags(self, response, tags: List[Tag]): for tag in tags: self.assertNotContains(response, tag.name) + def assertSelectedTags(self, response, tags: List[Tag]): + soup = self.make_soup(response.content.decode()) + selected_tags = soup.select('p.selected-tags')[0] + self.assertIsNotNone(selected_tags) + + tag_list = selected_tags.select('a') + self.assertEqual(len(tag_list), len(tags)) + + for tag in tags: + self.assertTrue(tag.name in selected_tags.text, msg=f'Selected tags do not contain: {tag.name}') + def test_should_list_archived_and_user_owned_bookmarks(self): other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123') visible_bookmarks = [ @@ -119,7 +132,7 @@ class BookmarkArchivedViewTestCase(TestCase, BookmarkFactoryMixin): self.setup_tag(), ] - self.setup_bookmark(is_archived=True, tags=[visible_tags[0]], title='searchvalue'), + self.setup_bookmark(is_archived=True, tags=[visible_tags[0]], title='searchvalue') self.setup_bookmark(is_archived=True, tags=[visible_tags[1]], title='searchvalue') self.setup_bookmark(is_archived=True, tags=[visible_tags[2]], title='searchvalue') self.setup_bookmark(is_archived=True, tags=[invisible_tags[0]]) @@ -131,6 +144,20 @@ class BookmarkArchivedViewTestCase(TestCase, BookmarkFactoryMixin): self.assertVisibleTags(response, visible_tags) self.assertInvisibleTags(response, invisible_tags) + def test_should_display_selected_tags_from_query(self): + tags = [ + self.setup_tag(), + self.setup_tag(), + self.setup_tag(), + self.setup_tag(), + self.setup_tag(), + ] + self.setup_bookmark(is_archived=True, tags=tags) + + response = self.client.get(reverse('bookmarks:archived') + f'?q=%23{tags[0].name}+%23{tags[1].name}') + + self.assertSelectedTags(response, [tags[0], tags[1]]) + def test_should_open_bookmarks_in_new_page_by_default(self): visible_bookmarks = [ self.setup_bookmark(is_archived=True), diff --git a/bookmarks/tests/test_bookmark_index_view.py b/bookmarks/tests/test_bookmark_index_view.py index b07bf4f..c5cb916 100644 --- a/bookmarks/tests/test_bookmark_index_view.py +++ b/bookmarks/tests/test_bookmark_index_view.py @@ -1,3 +1,4 @@ +from typing import List import urllib.parse from django.contrib.auth.models import User @@ -5,16 +6,16 @@ from django.test import TestCase from django.urls import reverse from bookmarks.models import Bookmark, Tag, UserProfile -from bookmarks.tests.helpers import BookmarkFactoryMixin +from bookmarks.tests.helpers import BookmarkFactoryMixin, HtmlTestMixin -class BookmarkIndexViewTestCase(TestCase, BookmarkFactoryMixin): +class BookmarkIndexViewTestCase(TestCase, BookmarkFactoryMixin, HtmlTestMixin): def setUp(self) -> None: user = self.get_or_create_test_user() self.client.force_login(user) - def assertVisibleBookmarks(self, response, bookmarks: [Bookmark], link_target: str = '_blank'): + def assertVisibleBookmarks(self, response, bookmarks: List[Bookmark], link_target: str = '_blank'): html = response.content.decode() self.assertContains(response, 'data-is-bookmark-item', count=len(bookmarks)) @@ -24,7 +25,7 @@ class BookmarkIndexViewTestCase(TestCase, BookmarkFactoryMixin): html ) - def assertInvisibleBookmarks(self, response, bookmarks: [Bookmark], link_target: str = '_blank'): + def assertInvisibleBookmarks(self, response, bookmarks: List[Bookmark], link_target: str = '_blank'): html = response.content.decode() for bookmark in bookmarks: @@ -34,16 +35,27 @@ class BookmarkIndexViewTestCase(TestCase, BookmarkFactoryMixin): count=0 ) - def assertVisibleTags(self, response, tags: [Tag]): + def assertVisibleTags(self, response, tags: List[Tag]): self.assertContains(response, 'data-is-tag-item', count=len(tags)) for tag in tags: self.assertContains(response, tag.name) - def assertInvisibleTags(self, response, tags: [Tag]): + def assertInvisibleTags(self, response, tags: List[Tag]): for tag in tags: self.assertNotContains(response, tag.name) + def assertSelectedTags(self, response, tags: List[Tag]): + soup = self.make_soup(response.content.decode()) + selected_tags = soup.select('p.selected-tags')[0] + self.assertIsNotNone(selected_tags) + + tag_list = selected_tags.select('a') + self.assertEqual(len(tag_list), len(tags)) + + for tag in tags: + self.assertTrue(tag.name in selected_tags.text, msg=f'Selected tags do not contain: {tag.name}') + def test_should_list_unarchived_and_user_owned_bookmarks(self): other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123') visible_bookmarks = [ @@ -121,7 +133,7 @@ class BookmarkIndexViewTestCase(TestCase, BookmarkFactoryMixin): self.setup_tag(), ] - self.setup_bookmark(tags=[visible_tags[0]], title='searchvalue'), + self.setup_bookmark(tags=[visible_tags[0]], title='searchvalue') self.setup_bookmark(tags=[visible_tags[1]], title='searchvalue') self.setup_bookmark(tags=[visible_tags[2]], title='searchvalue') self.setup_bookmark(tags=[invisible_tags[0]]) @@ -133,6 +145,20 @@ class BookmarkIndexViewTestCase(TestCase, BookmarkFactoryMixin): self.assertVisibleTags(response, visible_tags) self.assertInvisibleTags(response, invisible_tags) + def test_should_display_selected_tags_from_query(self): + tags = [ + self.setup_tag(), + self.setup_tag(), + self.setup_tag(), + self.setup_tag(), + self.setup_tag(), + ] + self.setup_bookmark(tags=tags) + + response = self.client.get(reverse('bookmarks:index') + f'?q=%23{tags[0].name}+%23{tags[1].name}') + + self.assertSelectedTags(response, [tags[0], tags[1]]) + def test_should_open_bookmarks_in_new_page_by_default(self): visible_bookmarks = [ self.setup_bookmark(), diff --git a/bookmarks/tests/test_tag_cloud_tag.py b/bookmarks/tests/test_tag_cloud_tag.py index 25b8014..a0133c9 100644 --- a/bookmarks/tests/test_tag_cloud_tag.py +++ b/bookmarks/tests/test_tag_cloud_tag.py @@ -1,27 +1,27 @@ from typing import List -from bs4 import BeautifulSoup from django.template import Template, RequestContext from django.test import TestCase, RequestFactory -from bookmarks.models import Tag, User -from bookmarks.tests.helpers import BookmarkFactoryMixin +from bookmarks.models import Tag +from bookmarks.tests.helpers import BookmarkFactoryMixin, HtmlTestMixin -class TagCloudTagTest(TestCase, BookmarkFactoryMixin): - def make_soup(self, html: str): - return BeautifulSoup(html, features="html.parser") +class TagCloudTagTest(TestCase, BookmarkFactoryMixin, HtmlTestMixin): + def render_template(self, tags: List[Tag], selected_tags: List[Tag] = None, url: str = '/test'): + if not selected_tags: + selected_tags = [] - def render_template(self, tags: List[Tag], url: str = '/test'): rf = RequestFactory() request = rf.get(url) context = RequestContext(request, { 'request': request, 'tags': tags, + 'selected_tags': selected_tags, }) template_to_render = Template( '{% load bookmarks %}' - '{% tag_cloud tags %}' + '{% tag_cloud tags selected_tags %}' ) return template_to_render.render(context) @@ -41,6 +41,11 @@ class TagCloudTagTest(TestCase, BookmarkFactoryMixin): link_element = link_elements[tag_index] self.assertEqual(link_element.text.strip(), tag) + def assertNumSelectedTags(self, rendered_template: str, count: int): + soup = self.make_soup(rendered_template) + link_elements = soup.select('p.selected-tags a') + self.assertEqual(len(link_elements), count) + def test_group_alphabetically(self): tags = [ self.setup_tag(name='Cockatoo'), @@ -75,14 +80,10 @@ class TagCloudTagTest(TestCase, BookmarkFactoryMixin): ]) def test_no_duplicate_tag_names(self): - user1 = User.objects.create_user('user1', 'user1@example.com', 'password123') - user2 = User.objects.create_user('user2', 'user2@example.com', 'password123') - user3 = User.objects.create_user('user3', 'user3@example.com', 'password123') - tags = [ - self.setup_tag(name='shared', user=user1), - self.setup_tag(name='shared', user=user2), - self.setup_tag(name='shared', user=user3), + self.setup_tag(name='shared', user=self.setup_user()), + self.setup_tag(name='shared', user=self.setup_user()), + self.setup_tag(name='shared', user=self.setup_user()), ] rendered_template = self.render_template(tags) @@ -92,3 +93,88 @@ class TagCloudTagTest(TestCase, BookmarkFactoryMixin): 'shared', ], ]) + + def test_selected_tags(self): + tags = [ + self.setup_tag(name='tag1'), + self.setup_tag(name='tag2'), + ] + + rendered_template = self.render_template(tags, tags, url='/test?q=%23tag1 %23tag2') + + self.assertNumSelectedTags(rendered_template, 2) + + self.assertInHTML(''' + + -tag1 + + ''', rendered_template) + + self.assertInHTML(''' + + -tag2 + + ''', rendered_template) + + def test_selected_tags_ignore_casing_when_removing_query_part(self): + tags = [ + self.setup_tag(name='TEST'), + ] + + rendered_template = self.render_template(tags, tags, url='/test?q=%23test') + + self.assertInHTML(''' + + -TEST + + ''', rendered_template) + + def test_no_duplicate_selected_tags(self): + tags = [ + self.setup_tag(name='shared', user=self.setup_user()), + self.setup_tag(name='shared', user=self.setup_user()), + self.setup_tag(name='shared', user=self.setup_user()), + ] + + rendered_template = self.render_template(tags, tags, url='/test?q=%23shared') + + self.assertInHTML(''' + + -shared + + ''', rendered_template, count=1) + + def test_selected_tag_url_keeps_other_search_terms(self): + tag = self.setup_tag(name='tag1') + + rendered_template = self.render_template([tag], [tag], url='/test?q=term1 %23tag1 term2 %21untagged') + + self.assertInHTML(''' + + -tag1 + + ''', rendered_template) + + def test_selected_tags_are_excluded_from_groups(self): + tags = [ + self.setup_tag(name='tag1'), + self.setup_tag(name='tag2'), + self.setup_tag(name='tag3'), + self.setup_tag(name='tag4'), + self.setup_tag(name='tag5'), + ] + selected_tags = [ + tags[0], + tags[1], + ] + + rendered_template = self.render_template(tags, selected_tags) + + self.assertTagGroups(rendered_template, [ + ['tag3', 'tag4', 'tag5'] + ]) diff --git a/bookmarks/views/bookmarks.py b/bookmarks/views/bookmarks.py index 3f1662e..84de4a1 100644 --- a/bookmarks/views/bookmarks.py +++ b/bookmarks/views/bookmarks.py @@ -3,7 +3,7 @@ import urllib.parse from django.contrib.auth.decorators import login_required from django.core.handlers.wsgi import WSGIRequest from django.core.paginator import Paginator -from django.db.models import QuerySet, prefetch_related_objects +from django.db.models import QuerySet, Q, prefetch_related_objects from django.http import HttpResponseRedirect, Http404 from django.shortcuts import render from django.urls import reverse @@ -50,6 +50,20 @@ def shared(request): return render(request, 'bookmarks/shared.html', context) +def _get_selected_tags(tags: QuerySet[Tag], query_string: str): + parsed_query = queries.parse_query_string(query_string) + tag_names = parsed_query['tag_names'] + + if len(tag_names) == 0: + return [] + + condition = Q() + for tag_name in parsed_query['tag_names']: + condition = condition | Q(name__iexact=tag_name) + + return list(tags.filter(condition)) + + def get_bookmark_view_context(request: WSGIRequest, filters: BookmarkFilters, query_set: QuerySet[Bookmark], @@ -58,6 +72,7 @@ def get_bookmark_view_context(request: WSGIRequest, page = request.GET.get('page') paginator = Paginator(query_set, _default_page_size) bookmarks = paginator.get_page(page) + selected_tags = _get_selected_tags(tags, filters.query) # Prefetch owner relation, this avoids n+1 queries when using the owner in templates prefetch_related_objects(bookmarks.object_list, 'owner') return_url = generate_return_url(base_url, page, filters) @@ -71,6 +86,7 @@ def get_bookmark_view_context(request: WSGIRequest, return { 'bookmarks': bookmarks, 'tags': tags, + 'selected_tags': selected_tags, 'filters': filters, 'empty': paginator.count == 0, 'return_url': return_url,