From 903118417667a21fe687d56da2f4e2f5d0b5f1be Mon Sep 17 00:00:00 2001
From: Pablo Barton <pablo.barton@gmail.com>
Date: Sat, 17 Apr 2021 21:31:38 -0400
Subject: [PATCH] addressing some review

---
 bookwyrm/templates/lists/list.html            |  2 +-
 .../snippets/booklist-pagination.html         | 64 +++++++++----------
 bookwyrm/tests/views/test_list.py             | 19 +++---
 bookwyrm/views/list.py                        | 42 +++++++-----
 4 files changed, 69 insertions(+), 58 deletions(-)

diff --git a/bookwyrm/templates/lists/list.html b/bookwyrm/templates/lists/list.html
index 3d338097..c9464c85 100644
--- a/bookwyrm/templates/lists/list.html
+++ b/bookwyrm/templates/lists/list.html
@@ -13,7 +13,6 @@
 
 <div class="columns mt-3">
     <section class="column is-three-quarters">
-        {% include "snippets/booklist-pagination.html" with page=items %}
         {% if not items.object_list.exists %}
         <p>{% trans "This list is currently empty" %}</p>
         {% else %}
@@ -58,6 +57,7 @@
         {% endfor %}
         </ol>
         {% endif %}
+        {% include "snippets/booklist-pagination.html" with page=items %}
     </section>
 
     <section class="column is-one-quarter content">
diff --git a/bookwyrm/templates/snippets/booklist-pagination.html b/bookwyrm/templates/snippets/booklist-pagination.html
index 69fae52e..161c743c 100644
--- a/bookwyrm/templates/snippets/booklist-pagination.html
+++ b/bookwyrm/templates/snippets/booklist-pagination.html
@@ -1,6 +1,37 @@
 {% load i18n %}
 {% if page.has_other_pages %}
-<nav class="pagination is-small" aria-label="pagination">
+<nav class="pagination is-centered" aria-label="pagination">
+    <ul class="pagination-list">
+        {% if page.number == 2 %}
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number }}{{ anchor }}">{{ page.previous_page_number }}</a></li>
+        {% elif page.number == 3 %}
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number|add:-1 }}{{ anchor }}">{{ page.previous_page_number|add:-1 }}</a></li>
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number }}{{ anchor }}">{{ page.previous_page_number }}</a></li>
+        {% elif page.number == 4 %}
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number|add:-2 }}{{ anchor }}">{{ page.previous_page_number|add:-2 }}</a></li>
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number|add:-1 }}{{ anchor }}">{{ page.previous_page_number|add:-1 }}</a></li>
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number }}{{ anchor }}">{{ page.previous_page_number }}</a></li>
+        {% elif page.number > 4 %}
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page=1{{ anchor }}">1</a></li>
+            <span class="pagination-ellipsis">&hellip;</span>
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number }}{{ anchor }}">{{ page.previous_page_number }}</a></li>
+        {% endif %}
+    <li><a class="pagination-link is-link" disabled href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.number }}{{ anchor }}">{{ page.number }}</a></li>
+        {% if page.number|add:1 == page.paginator.num_pages %}
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number }}{{ anchor }}">{{ page.next_page_number }}</a></li>
+        {% elif page.number|add:2 == page.paginator.num_pages %}
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number }}{{ anchor }}">{{ page.next_page_number }}</a></li>
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number|add:1 }}{{ anchor }}">{{ page.next_page_number|add:1 }}</a></li>
+        {% elif page.number|add:3 == page.paginator.num_pages %}
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number }}{{ anchor }}">{{ page.next_page_number }}</a></li>
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number|add:1 }}{{ anchor }}">{{ page.next_page_number|add:1 }}</a></li>
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number|add:2 }}{{ anchor }}">{{ page.next_page_number|add:2 }}</a></li>
+        {% elif page.number|add:3 < page.paginator.num_pages %}
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number }}{{ anchor }}">{{ page.next_page_number }}</a></li>
+            <span class="pagination-ellipsis">&hellip;</span>
+            <li><a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ items.paginator.num_pages }}{{ anchor }}">{{ items.paginator.num_pages }}</a></li>
+        {% endif %}
+    </ul>
     <a
         class="pagination-previous {% if not page.has_previous %}is-disabled{% endif %}"
         {% if page.has_previous %}
@@ -23,36 +54,5 @@
         {% trans "Next" %}
         <span class="icon icon-arrow-right" aria-hidden="true"></span>
     </a>
-    <ul class="pagination-list">
-        {% if page.number == 2 %}
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number }}{{ anchor }}">{{ page.previous_page_number }}</a>
-        {% elif page.number == 3 %}
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number|add:-1 }}{{ anchor }}">{{ page.previous_page_number|add:-1 }}</a>
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number }}{{ anchor }}">{{ page.previous_page_number }}</a>
-        {% elif page.number == 4 %}
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number|add:-2 }}{{ anchor }}">{{ page.previous_page_number|add:-2 }}</a>
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number|add:-1 }}{{ anchor }}">{{ page.previous_page_number|add:-1 }}</a>
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number }}{{ anchor }}">{{ page.previous_page_number }}</a>
-        {% elif page.number > 4 %}
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page=1{{ anchor }}">1</a>
-            <span class="pagination-ellipsis">&hellip;</span>
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number }}{{ anchor }}">{{ page.previous_page_number }}</a>
-        {% endif %}
-    <a class="pagination-link is-link" disabled href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.number }}{{ anchor }}">{{ page.number }}</a>
-        {% if page.number|add:1 == page.paginator.num_pages %}
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number }}{{ anchor }}">{{ page.next_page_number }}</a>
-        {% elif page.number|add:2 == page.paginator.num_pages %}
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number }}{{ anchor }}">{{ page.next_page_number }}</a>
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number|add:1 }}{{ anchor }}">{{ page.next_page_number|add:1 }}</a>
-        {% elif page.number|add:3 == page.paginator.num_pages %}
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number }}{{ anchor }}">{{ page.next_page_number }}</a>
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number|add:1 }}{{ anchor }}">{{ page.next_page_number|add:1 }}</a>
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number|add:2 }}{{ anchor }}">{{ page.next_page_number|add:2 }}</a>
-        {% elif page.number|add:3 < page.paginator.num_pages %}
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number }}{{ anchor }}">{{ page.next_page_number }}</a>
-            <span class="pagination-ellipsis">&hellip;</span>
-            <a class="pagination-link" href="{% url 'list' list.id %}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ items.paginator.num_pages }}{{ anchor }}">{{ items.paginator.num_pages }}</a>
-        {% endif %}
-    </ul>
 </nav>
 {% endif %}
diff --git a/bookwyrm/tests/views/test_list.py b/bookwyrm/tests/views/test_list.py
index 0b4339fc..446169ab 100644
--- a/bookwyrm/tests/views/test_list.py
+++ b/bookwyrm/tests/views/test_list.py
@@ -304,8 +304,9 @@ class ListViews(TestCase):
             },
         )
         request_two.user = self.local_user
-        views.list.add_book(request_one)
-        views.list.add_book(request_two)
+        with patch("bookwyrm.models.activitypub_mixin.broadcast_task.delay"):
+            views.list.add_book(request_one)
+            views.list.add_book(request_two)
 
         items = self.list.listitem_set.order_by("order").all()
         self.assertEqual(items[0].book, self.book)
@@ -345,9 +346,10 @@ class ListViews(TestCase):
         )
         request_three.user = self.local_user
 
-        views.list.add_book(request_one)
-        views.list.add_book(request_two)
-        views.list.add_book(request_three)
+        with patch("bookwyrm.models.activitypub_mixin.broadcast_task.delay"):
+            views.list.add_book(request_one)
+            views.list.add_book(request_two)
+            views.list.add_book(request_three)
 
         items = self.list.listitem_set.order_by("order").all()
         self.assertEqual(items[0].book, self.book)
@@ -398,9 +400,10 @@ class ListViews(TestCase):
         )
         request_three.user = self.local_user
 
-        views.list.add_book(request_one)
-        views.list.add_book(request_two)
-        views.list.add_book(request_three)
+        with patch("bookwyrm.models.activitypub_mixin.broadcast_task.delay"):
+            views.list.add_book(request_one)
+            views.list.add_book(request_two)
+            views.list.add_book(request_three)
 
         items = self.list.listitem_set.order_by("order").all()
         self.assertEqual(items[0].book, self.book)
diff --git a/bookwyrm/views/list.py b/bookwyrm/views/list.py
index c7311bf1..9a32168a 100644
--- a/bookwyrm/views/list.py
+++ b/bookwyrm/views/list.py
@@ -114,29 +114,37 @@ class List(View):
 
         page = request.GET.get("page", 1)
 
+        internal_sort_by = {
+            "order": "order",
+            "title": "book__title",
+            "rating": "average_rating",
+        }
+        directional_sort_by = internal_sort_by[sort_by]
+        if direction == "descending":
+            directional_sort_by = "-" + directional_sort_by
+
         if sort_by == "order":
-            directional_sort_by = "order"
-            if direction == "descending":
-                directional_sort_by = "-" + directional_sort_by
-            items = book_list.listitem_set.filter(approved=True).order_by(
-                directional_sort_by
+            items = (
+                book_list.listitem_set
+                .filter(approved=True)
+                .order_by(directional_sort_by)
             )
         elif sort_by == "title":
-            directional_sort_by = "book__title"
-            if direction == "descending":
-                directional_sort_by = "-" + directional_sort_by
-            items = book_list.listitem_set.filter(approved=True).order_by(
-                directional_sort_by
+            items = (
+                book_list.listitem_set
+                .filter(approved=True)
+                .order_by(
+                directional_sort_by)
             )
         elif sort_by == "rating":
-            directional_sort_by = "average_rating"
-            if direction == "descending":
-                directional_sort_by = "-" + directional_sort_by
-            items = book_list.listitem_set.annotate(
-                average_rating=Avg(Coalesce("book__review__rating", 0))
-            ).order_by(directional_sort_by)
+            items = (
+                book_list.listitem_set
+                .annotate(average_rating=Avg(Coalesce("book__review__rating", 0)))
+                .filter(approved=True)
+                .order_by(directional_sort_by)
+            )
 
-        paginated = Paginator(items, 25)
+        paginated = Paginator(items, 2)
 
         if query and request.user.is_authenticated:
             # search for books