From 06d1936ac99fe226b614f9fdd55901bcf770c827 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Thu, 4 Aug 2022 11:42:03 -0700 Subject: [PATCH 1/6] Fixes pagination of local search results --- bookwyrm/templates/search/book.html | 18 ++--- bookwyrm/templates/search/layout.html | 16 +++- bookwyrm/views/search.py | 102 ++++++++++++++------------ 3 files changed, 72 insertions(+), 64 deletions(-) diff --git a/bookwyrm/templates/search/book.html b/bookwyrm/templates/search/book.html index e7800a780..783b0b930 100644 --- a/bookwyrm/templates/search/book.html +++ b/bookwyrm/templates/search/book.html @@ -3,10 +3,9 @@ {% block panel %} -{% if results %} -{% with results|first as local_results %} +{% if results or remote_results %} -{% endwith %}
-{% for result_set in results|slice:"1:" %} +{% for result_set in remote_results %} {% if result_set.results %}
- {% if not result_set.connector.local %}
- {% endif %} - {% if not result_set.connector.local %} {% trans 'Results from' %} @@ -47,7 +42,6 @@ - {% endif %}
@@ -88,17 +82,15 @@
- {% if not result_set.connector.local %}
- {% endif %}
{% endif %} {% endfor %}
- - {% endif %} +{% endblock %} +{% block search_footer %}

{% if request.user.is_authenticated %} {% if not remote %} diff --git a/bookwyrm/templates/search/layout.html b/bookwyrm/templates/search/layout.html index 42729ebec..a2f64ad07 100644 --- a/bookwyrm/templates/search/layout.html +++ b/bookwyrm/templates/search/layout.html @@ -1,5 +1,6 @@ {% extends 'layout.html' %} {% load i18n %} +{% load humanize %} {% block title %}{% trans "Search" %}{% endblock %} @@ -53,17 +54,24 @@

- {% if not results %} -

+

+ {% if not results %} {% blocktrans %}No results found for "{{ query }}"{% endblocktrans %} + {% else %} + {% blocktrans trimmed count counter=results.paginator.count with result_count=results.paginator.count|intcomma %} + {{ result_count }} result found + {% plural %} + {{ result_count }} results found + {% endblocktrans %} + {% endif %}

- {% endif %} {% block panel %} {% endblock %} -
+
{% include 'snippets/pagination.html' with page=results path=request.path %}
+ {% block search_footer %}{% endblock %}
{% endif %} diff --git a/bookwyrm/views/search.py b/bookwyrm/views/search.py index 2d7ef4f9f..b3abf4abf 100644 --- a/bookwyrm/views/search.py +++ b/bookwyrm/views/search.py @@ -23,22 +23,11 @@ class Search(View): def get(self, request): """that search bar up top""" - query = request.GET.get("q") - # check if query is isbn - query = isbn_check(query) - min_confidence = request.GET.get("min_confidence", 0) - search_type = request.GET.get("type") - search_remote = ( - request.GET.get("remote", False) and request.user.is_authenticated - ) - if is_api_request(request): - # only return local book results via json so we don't cascade - book_results = search(query, min_confidence=min_confidence) - return JsonResponse( - [format_search_result(r) for r in book_results], safe=False - ) + return api_book_search(request) + query = request.GET.get("q") + search_type = request.GET.get("type") if query and not search_type: search_type = "user" if "@" in query else "book" @@ -50,49 +39,64 @@ class Search(View): if not search_type in endpoints: search_type = "book" - data = { - "query": query or "", - "type": search_type, - "remote": search_remote, - } - if query: - results, search_remote = endpoints[search_type]( - query, request.user, min_confidence, search_remote - ) - if results: - paginated = Paginator(results, PAGE_LENGTH).get_page( - request.GET.get("page") - ) - data["results"] = paginated - data["remote"] = search_remote - - return TemplateResponse(request, f"search/{search_type}.html", data) + return endpoints[search_type](request) -def book_search(query, user, min_confidence, search_remote=False): +def api_book_search(request): + """Return books via API response""" + query = request.GET.get("q") + query = isbn_check(query) + min_confidence = request.GET.get("min_confidence", 0) + # only return local book results via json so we don't cascade + book_results = search(query, min_confidence=min_confidence) + return JsonResponse( + [format_search_result(r) for r in book_results], safe=False + ) + +def book_search(request): """the real business is elsewhere""" + query = request.GET.get("q") + # check if query is isbn + query = isbn_check(query) + min_confidence = request.GET.get("min_confidence", 0) + search_remote = ( + request.GET.get("remote", False) and request.user.is_authenticated + ) + # try a local-only search - results = [{"results": search(query, min_confidence=min_confidence)}] - if not user.is_authenticated or (results[0]["results"] and not search_remote): - return results, False - - # if there were no local results, or the request was for remote, search all sources - results += connector_manager.search(query, min_confidence=min_confidence) - return results, True + local_results = search(query, min_confidence=min_confidence) + paginated = Paginator(local_results, PAGE_LENGTH).get_page( + request.GET.get("page") + ) + data = { + "query": query, + "results": paginated, + "type": "book", + "remote": search_remote + } + # if a logged in user requested remote results or got no local results, try remote + if request.user.is_authenticated and (not local_results or search_remote): + data["remote_results"] = connector_manager.search( + query, min_confidence=min_confidence + ) + return TemplateResponse(request, "search/book.html", data) -def user_search(query, viewer, *_): +def user_search(request): """cool kids members only user search""" + viewer = request.user + query = request.GET.get("q") + data = {"type": "user", "query": query} # logged out viewers can't search users if not viewer.is_authenticated: - return models.User.objects.none(), None + return TemplateResponse(request, "search/user.html", data) # use webfinger for mastodon style account@domain.com username to load the user if # they don't exist locally (handle_remote_webfinger will check the db) if re.match(regex.FULL_USERNAME, query): handle_remote_webfinger(query) - return ( + data["results"] = ( models.User.viewer_aware_objects(viewer) .annotate( similarity=Greatest( @@ -104,14 +108,17 @@ def user_search(query, viewer, *_): similarity__gt=0.5, ) .order_by("-similarity") - ), None + ) + return TemplateResponse(request, "search/user.html", data) -def list_search(query, viewer, *_): +def list_search(request): """any relevent lists?""" - return ( + query = request.GET.get("q") + data = {"query": query, "type": "list"} + data["results"] = ( models.List.privacy_filter( - viewer, + request.user, privacy_levels=["public", "followers"], ) .annotate( @@ -124,7 +131,8 @@ def list_search(query, viewer, *_): similarity__gt=0.1, ) .order_by("-similarity") - ), None + ) + return TemplateResponse(request, "search/list.html", data) def isbn_check(query): From 48df5076e7654b2c773441cf8e1e0f8c5fbcefb1 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Thu, 4 Aug 2022 12:10:01 -0700 Subject: [PATCH 2/6] Use elided page range and paginate user and list results --- bookwyrm/views/search.py | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/bookwyrm/views/search.py b/bookwyrm/views/search.py index b3abf4abf..7d89bfab7 100644 --- a/bookwyrm/views/search.py +++ b/bookwyrm/views/search.py @@ -65,14 +65,16 @@ def book_search(request): # try a local-only search local_results = search(query, min_confidence=min_confidence) - paginated = Paginator(local_results, PAGE_LENGTH).get_page( - request.GET.get("page") - ) + paginated = Paginator(local_results, PAGE_LENGTH) + page = paginated.get_page(request.GET.get("page")) data = { "query": query, - "results": paginated, + "results": page, "type": "book", - "remote": search_remote + "remote": search_remote, + "page_range": paginated.get_elided_page_range( + page.number, on_each_side=2, on_ends=1 + ) } # if a logged in user requested remote results or got no local results, try remote if request.user.is_authenticated and (not local_results or search_remote): @@ -96,7 +98,7 @@ def user_search(request): if re.match(regex.FULL_USERNAME, query): handle_remote_webfinger(query) - data["results"] = ( + results = ( models.User.viewer_aware_objects(viewer) .annotate( similarity=Greatest( @@ -109,6 +111,12 @@ def user_search(request): ) .order_by("-similarity") ) + paginated = Paginator(results, PAGE_LENGTH) + page = paginated.get_page(request.GET.get("page")) + data["results"] = page + data["page_range"] = paginated.get_elided_page_range( + page.number, on_each_side=2, on_ends=1 + ) return TemplateResponse(request, "search/user.html", data) @@ -116,7 +124,7 @@ def list_search(request): """any relevent lists?""" query = request.GET.get("q") data = {"query": query, "type": "list"} - data["results"] = ( + results = ( models.List.privacy_filter( request.user, privacy_levels=["public", "followers"], @@ -132,6 +140,12 @@ def list_search(request): ) .order_by("-similarity") ) + paginated = Paginator(results, PAGE_LENGTH) + page = paginated.get_page(request.GET.get("page")) + data["results"] = page + data["page_range"] = paginated.get_elided_page_range( + page.number, on_each_side=2, on_ends=1 + ) return TemplateResponse(request, "search/list.html", data) From 77d1d7020734150b43f444fa9e09f4a4f1986537 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Thu, 4 Aug 2022 12:16:12 -0700 Subject: [PATCH 3/6] Much shorter search timeout --- .env.example | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.env.example b/.env.example index bb2d677ef..af1d6430c 100644 --- a/.env.example +++ b/.env.example @@ -56,7 +56,7 @@ EMAIL_SENDER_NAME=admin EMAIL_SENDER_DOMAIN= # Query timeouts -SEARCH_TIMEOUT=15 +SEARCH_TIMEOUT=5 QUERY_TIMEOUT=5 # Thumbnails Generation From ed71b791c9285fa612d38b9749924dc221896a39 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Thu, 4 Aug 2022 12:19:26 -0700 Subject: [PATCH 4/6] Python formatting --- bookwyrm/views/search.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/bookwyrm/views/search.py b/bookwyrm/views/search.py index 7d89bfab7..fc34cd915 100644 --- a/bookwyrm/views/search.py +++ b/bookwyrm/views/search.py @@ -49,9 +49,8 @@ def api_book_search(request): min_confidence = request.GET.get("min_confidence", 0) # only return local book results via json so we don't cascade book_results = search(query, min_confidence=min_confidence) - return JsonResponse( - [format_search_result(r) for r in book_results], safe=False - ) + return JsonResponse([format_search_result(r) for r in book_results], safe=False) + def book_search(request): """the real business is elsewhere""" @@ -59,9 +58,7 @@ def book_search(request): # check if query is isbn query = isbn_check(query) min_confidence = request.GET.get("min_confidence", 0) - search_remote = ( - request.GET.get("remote", False) and request.user.is_authenticated - ) + search_remote = request.GET.get("remote", False) and request.user.is_authenticated # try a local-only search local_results = search(query, min_confidence=min_confidence) @@ -74,7 +71,7 @@ def book_search(request): "remote": search_remote, "page_range": paginated.get_elided_page_range( page.number, on_each_side=2, on_ends=1 - ) + ), } # if a logged in user requested remote results or got no local results, try remote if request.user.is_authenticated and (not local_results or search_remote): From b4cfda058718b128da19e98e2bd2a3eb66a06feb Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Fri, 5 Aug 2022 08:56:24 -0700 Subject: [PATCH 5/6] Updates tests --- bookwyrm/tests/views/test_search.py | 38 ++++++++++++++++++++++------- bookwyrm/views/search.py | 3 +++ 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/bookwyrm/tests/views/test_search.py b/bookwyrm/tests/views/test_search.py index d6e00edb6..bf7bb2a5b 100644 --- a/bookwyrm/tests/views/test_search.py +++ b/bookwyrm/tests/views/test_search.py @@ -17,7 +17,7 @@ from bookwyrm.tests.validate_html import validate_html class Views(TestCase): """tag views""" - def setUp(self): + def setUp(self): # pylint: disable=invalid-name """we need basic test data and mocks""" self.factory = RequestFactory() with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch( @@ -90,13 +90,29 @@ class Views(TestCase): self.assertIsInstance(response, TemplateResponse) validate_html(response.render()) - connector_results = response.context_data["results"] - self.assertEqual(len(connector_results), 2) - self.assertEqual(connector_results[0]["results"][0].title, "Test Book") - self.assertEqual(connector_results[1]["results"][0].title, "Mock Book") - # don't search remote + local_results = response.context_data["results"] + self.assertEqual(local_results[0].title, "Test Book") + + connector_results = response.context_data["remote_results"] + self.assertEqual(connector_results[0]["results"][0].title, "Mock Book") + + def test_search_book_anonymous(self): + """Don't search remote for logged out user""" + view = views.Search.as_view() + + connector = models.Connector.objects.create( + identifier="example.com", + connector_file="openlibrary", + base_url="https://example.com", + books_url="https://example.com/books", + covers_url="https://example.com/covers", + search_url="https://example.com/search?q=", + ) + mock_result = SearchResult(title="Mock Book", connector=connector, key="hello") + request = self.factory.get("", {"q": "Test Book", "remote": True}) + anonymous_user = AnonymousUser anonymous_user.is_authenticated = False request.user = anonymous_user @@ -107,11 +123,15 @@ class Views(TestCase): {"results": [mock_result], "connector": connector} ] response = view(request) + self.assertIsInstance(response, TemplateResponse) validate_html(response.render()) - connector_results = response.context_data["results"] - self.assertEqual(len(connector_results), 1) - self.assertEqual(connector_results[0]["results"][0].title, "Test Book") + + local_results = response.context_data["results"] + self.assertEqual(local_results[0].title, "Test Book") + + connector_results = response.context_data.get("remote_results") + self.assertIsNone(connector_results) def test_search_users(self): """searches remote connectors""" diff --git a/bookwyrm/views/search.py b/bookwyrm/views/search.py index fc34cd915..997a5f841 100644 --- a/bookwyrm/views/search.py +++ b/bookwyrm/views/search.py @@ -27,6 +27,9 @@ class Search(View): return api_book_search(request) query = request.GET.get("q") + if not query: + return TemplateResponse(request, "search/book.html") + search_type = request.GET.get("type") if query and not search_type: search_type = "user" if "@" in query else "book" From 9fad5b5623c1de48316bc7932fb91c5eed59f5c2 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Fri, 5 Aug 2022 11:43:11 -0700 Subject: [PATCH 6/6] Fixes isbn view --- bookwyrm/tests/views/test_isbn.py | 5 +++-- bookwyrm/views/isbn.py | 13 ++++++++----- bookwyrm/views/search.py | 4 +++- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/bookwyrm/tests/views/test_isbn.py b/bookwyrm/tests/views/test_isbn.py index 7c18b4abd..e09379418 100644 --- a/bookwyrm/tests/views/test_isbn.py +++ b/bookwyrm/tests/views/test_isbn.py @@ -7,13 +7,14 @@ from django.test import TestCase from django.test.client import RequestFactory from bookwyrm import models, views +from bookwyrm.tests.validate_html import validate_html from bookwyrm.settings import DOMAIN class IsbnViews(TestCase): """tag views""" - def setUp(self): + def setUp(self): # pylint: disable=invalid-name """we need basic test data and mocks""" self.factory = RequestFactory() with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch( @@ -58,4 +59,4 @@ class IsbnViews(TestCase): is_api.return_value = False response = view(request, isbn="1234567890123") self.assertEqual(response.status_code, 200) - response.render() + validate_html(response.render()) diff --git a/bookwyrm/views/isbn.py b/bookwyrm/views/isbn.py index e5343488d..728936e1f 100644 --- a/bookwyrm/views/isbn.py +++ b/bookwyrm/views/isbn.py @@ -18,14 +18,17 @@ class Isbn(View): if is_api_request(request): return JsonResponse( - [book_search.format_search_result(r) for r in book_results], safe=False + [book_search.format_search_result(r) for r in book_results[:10]], + safe=False, ) - paginated = Paginator(book_results, PAGE_LENGTH).get_page( - request.GET.get("page") - ) + paginated = Paginator(book_results, PAGE_LENGTH) + page = paginated.get_page(request.GET.get("page")) data = { - "results": [{"results": paginated}], + "results": page, + "page_range": paginated.get_elided_page_range( + page.number, on_each_side=2, on_ends=1 + ), "query": isbn, "type": "book", } diff --git a/bookwyrm/views/search.py b/bookwyrm/views/search.py index 997a5f841..db33fd330 100644 --- a/bookwyrm/views/search.py +++ b/bookwyrm/views/search.py @@ -52,7 +52,9 @@ def api_book_search(request): min_confidence = request.GET.get("min_confidence", 0) # only return local book results via json so we don't cascade book_results = search(query, min_confidence=min_confidence) - return JsonResponse([format_search_result(r) for r in book_results], safe=False) + return JsonResponse( + [format_search_result(r) for r in book_results[:10]], safe=False + ) def book_search(request):