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 <jonp@hauris.org>
This commit is contained in:
Sascha Ißbrücker
2022-08-04 20:31:24 +02:00
committed by GitHub
parent fec966f687
commit dd5e65ecd7
14 changed files with 271 additions and 66 deletions

View File

@@ -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 = ''

View File

@@ -91,9 +91,19 @@ ul.bookmark-list {
.tag-cloud {
.selected-tags {
margin-bottom: 0.8rem;
a, a:visited:hover {
color: $error-color;
}
}
.unselected-tags {
a, a:visited:hover {
color: $alternative-color;
}
}
.group {
margin-bottom: 0.4rem;

View File

@@ -36,7 +36,7 @@
<div class="content-area-header">
<h2>Tags</h2>
</div>
{% tag_cloud tags %}
{% tag_cloud tags selected_tags %}
</section>
</div>

View File

@@ -15,7 +15,7 @@
{% if bookmark.tag_names %}
<span>
{% for tag_name in bookmark.tag_names %}
<a href="?{% append_query_param q=tag_name|hash_tag %}">{{ tag_name|hash_tag }}</a>
<a href="?{% append_to_query_param q=tag_name|hash_tag %}">{{ tag_name|hash_tag }}</a>
{% endfor %}
</span>
{% endif %}

View File

@@ -36,7 +36,7 @@
<div class="content-area-header">
<h2>Tags</h2>
</div>
{% tag_cloud tags %}
{% tag_cloud tags selected_tags %}
</section>
</div>

View File

@@ -39,7 +39,7 @@
<div class="content-area-header">
<h2>Tags</h2>
</div>
{% tag_cloud tags %}
{% tag_cloud tags selected_tags %}
</section>
</div>

View File

@@ -1,18 +1,29 @@
{% load shared %}
<div class="tag-cloud">
{% if has_selected_tags %}
<p class="selected-tags">
{% for tag in selected_tags %}
<a href="?{% remove_from_query_param q=tag.name|hash_tag %}"
class="text-bold mr-2">
<span>-{{ tag.name }}</span>
</a>
{% endfor %}
</p>
{% endif %}
<div class="unselected-tags">
{% for group in groups %}
<p class="group">
{% for tag in group.tags %}
{# Highlight first char of first tag in group #}
{% if forloop.counter == 1 %}
<a href="?{% append_query_param q=tag.name|hash_tag %}"
<a href="?{% append_to_query_param q=tag.name|hash_tag %}"
class="mr-2" data-is-tag-item>
<span class="highlight-char">{{ tag.name|first_char }}</span><span>{{ tag.name|remaining_chars:1 }}</span>
</a>
{% else %}
{# Render remaining tags normally #}
<a href="?{% append_query_param q=tag.name|hash_tag %}"
<a href="?{% append_to_query_param q=tag.name|hash_tag %}"
class="mr-2" data-is-tag-item>
<span>{{ tag.name }}</span>
</a>
@@ -20,4 +31,5 @@
{% endfor %}
</p>
{% endfor %}
</div>
</div>

View File

@@ -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,
}

View File

@@ -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()

View File

@@ -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)

View File

@@ -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),

View File

@@ -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(),

View File

@@ -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('''
<a href="?q=%23tag2"
class="text-bold mr-2">
<span>-tag1</span>
</a>
''', rendered_template)
self.assertInHTML('''
<a href="?q=%23tag1"
class="text-bold mr-2">
<span>-tag2</span>
</a>
''', 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('''
<a href="?q="
class="text-bold mr-2">
<span>-TEST</span>
</a>
''', 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('''
<a href="?q="
class="text-bold mr-2">
<span>-shared</span>
</a>
''', 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('''
<a href="?q=term1+term2+%21untagged"
class="text-bold mr-2">
<span>-tag1</span>
</a>
''', 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']
])

View File

@@ -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,