addressing some review

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">

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 %}

bookwyrm/tests/views/test_list.py

@@ -304,6 +304,7 @@ class ListViews(TestCase):
             },
         )
         request_two.user = self.local_user
+        with patch("bookwyrm.models.activitypub_mixin.broadcast_task.delay"):
             views.list.add_book(request_one)
             views.list.add_book(request_two)
 
@@ -345,6 +346,7 @@ class ListViews(TestCase):
         )
         request_three.user = self.local_user
 
+        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)
@@ -398,6 +400,7 @@ class ListViews(TestCase):
         )
         request_three.user = self.local_user
 
+        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)

bookwyrm/views/list.py

@@ -114,29 +114,37 @@ class List(View):
 
         page = request.GET.get("page", 1)
 
-        if sort_by == "order":
+        internal_sort_by = {
-            directional_sort_by = "order"
+            "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
-            items = book_list.listitem_set.filter(approved=True).order_by(
+
-                directional_sort_by
+        if sort_by == "order":
+            items = (
+                book_list.listitem_set
+                .filter(approved=True)
+                .order_by(directional_sort_by)
             )
         elif sort_by == "title":
-            directional_sort_by = "book__title"
+            items = (
-            if direction == "descending":
+                book_list.listitem_set
-                directional_sort_by = "-" + directional_sort_by
+                .filter(approved=True)
-            items = book_list.listitem_set.filter(approved=True).order_by(
+                .order_by(
-                directional_sort_by
+                directional_sort_by)
             )
         elif sort_by == "rating":
-            directional_sort_by = "average_rating"
+            items = (
-            if direction == "descending":
+                book_list.listitem_set
-                directional_sort_by = "-" + directional_sort_by
+                .annotate(average_rating=Avg(Coalesce("book__review__rating", 0)))
-            items = book_list.listitem_set.annotate(
+                .filter(approved=True)
-                average_rating=Avg(Coalesce("book__review__rating", 0))
+                .order_by(directional_sort_by)
-            ).order_by(directional_sort_by)
+            )
 
-        paginated = Paginator(items, 25)
+        paginated = Paginator(items, 2)
 
         if query and request.user.is_authenticated:
             # search for books