per review, removing custom pagination for booklist and handling all pending books at the end of the list

bookwyrm/templates/lists/list.html

@@ -57,7 +57,7 @@
         {% endfor %}
         </ol>
         {% endif %}
-        {% include "snippets/booklist-pagination.html" with page=items %}
+        {% include "snippets/pagination.html" with page=items %}
     </section>
 
     <section class="column is-one-quarter content">

bookwyrm/templates/snippets/booklist-pagination.html

@@ -1,58 +0,0 @@
-{% load i18n %}
-{% if page.has_other_pages %}
-<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 %}
-        href="{{ path }}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.previous_page_number }}{{ anchor }}"
-        {% else %}
-        aria-hidden="true"
-        {% endif %}>
-
-        <span class="icon icon-arrow-left" aria-hidden="true"></span>
-        {% trans "Previous" %}
-    </a>
-    <a
-        class="pagination-next {% if not page.has_next %}is-disabled{% endif %}"
-        {% if page.has_next %}
-        href="{{ path }}?{% for k, v in request.GET.items %}{% if k != 'page' %}{{ k }}={{ v }}&{% endif %}{% endfor %}page={{ page.next_page_number }}{{ anchor }}"
-        {% else %}
-        aria-hidden="true"
-        {% endif %}>
-
-        {% trans "Next" %}
-        <span class="icon icon-arrow-right" aria-hidden="true"></span>
-    </a>
-</nav>
-{% endif %}

bookwyrm/tests/views/test_list.py

@@ -51,6 +51,13 @@ class ListViews(TestCase):
             remote_id="https://example.com/book/3",
             parent_work=work_three,
         )
+        work_four = models.Work.objects.create(title="Travailler")
+        self.book_four = models.Edition.objects.create(
+            title="Example Edition 4",
+            remote_id="https://example.com/book/4",
+            parent_work=work_four,
+        )
+
         with patch("bookwyrm.models.activitypub_mixin.broadcast_task.delay"):
             self.list = models.List.objects.create(
                 name="Test List", user=self.local_user
@@ -369,6 +376,117 @@ class ListViews(TestCase):
         self.assertEqual(items[0].order, 1)
         self.assertEqual(items[1].order, 2)
 
+    def test_adding_book_with_a_pending_book(self):
+        """
+        When a list contains any pending books, the pending books should have
+        be at the end of the list by order. If a book is added while a book is
+        pending, its order should precede the pending books.
+        """
+        request = self.factory.post(
+            "",
+            {
+                "book": self.book_three.id,
+                "list": self.list.id,
+            },
+        )
+        request.user = self.local_user
+        with patch("bookwyrm.models.activitypub_mixin.broadcast_task.delay"):
+            models.ListItem.objects.create(
+                book_list=self.list,
+                user=self.local_user,
+                book=self.book,
+                approved=True,
+                order=1,
+            )
+            models.ListItem.objects.create(
+                book_list=self.list,
+                user=self.rat,
+                book=self.book_two,
+                approved=False,
+                order=2,
+            )
+            views.list.add_book(request)
+
+        items = self.list.listitem_set.order_by("order").all()
+        self.assertEqual(items[0].book, self.book)
+        self.assertEqual(items[0].order, 1)
+        self.assertTrue(items[0].approved)
+
+        self.assertEqual(items[1].book, self.book_three)
+        self.assertEqual(items[1].order, 2)
+        self.assertTrue(items[1].approved)
+
+        self.assertEqual(items[2].book, self.book_two)
+        self.assertEqual(items[2].order, 3)
+        self.assertFalse(items[2].approved)
+
+    def test_approving_one_pending_book_from_multiple(self):
+        """
+        When a list contains any pending books, the pending books should have
+        be at the end of the list by order. If a pending book is approved, then
+        its order should be at the end of the approved books and before the
+        remaining pending books.
+        """
+        with patch("bookwyrm.models.activitypub_mixin.broadcast_task.delay"):
+            models.ListItem.objects.create(
+                book_list=self.list,
+                user=self.local_user,
+                book=self.book,
+                approved=True,
+                order=1,
+            )
+            models.ListItem.objects.create(
+                book_list=self.list,
+                user=self.local_user,
+                book=self.book_two,
+                approved=True,
+                order=2,
+            )
+            models.ListItem.objects.create(
+                book_list=self.list,
+                user=self.rat,
+                book=self.book_three,
+                approved=False,
+                order=3,
+            )
+            to_be_approved = models.ListItem.objects.create(
+                book_list=self.list,
+                user=self.rat,
+                book=self.book_four,
+                approved=False,
+                order=4,
+            )
+
+        view = views.Curate.as_view()
+        request = self.factory.post(
+            "",
+            {
+                "item": to_be_approved.id,
+                "approved": "true",
+            },
+        )
+        request.user = self.local_user
+
+        with patch("bookwyrm.models.activitypub_mixin.broadcast_task.delay"):
+            view(request, self.list.id)
+
+        items = self.list.listitem_set.order_by("order").all()
+        self.assertEqual(items[0].book, self.book)
+        self.assertEqual(items[0].order, 1)
+        self.assertTrue(items[0].approved)
+
+        self.assertEqual(items[1].book, self.book_two)
+        self.assertEqual(items[1].order, 2)
+        self.assertTrue(items[1].approved)
+
+        self.assertEqual(items[2].book, self.book_four)
+        self.assertEqual(items[2].order, 3)
+        self.assertTrue(items[2].approved)
+
+        self.assertEqual(items[3].book, self.book_three)
+        self.assertEqual(items[3].order, 4)
+        self.assertFalse(items[3].approved)
+
     def test_add_three_books_and_move_last_to_first(self):
         """
         Put three books on the list and move the last book to the first

bookwyrm/views/list.py

@@ -1,4 +1,6 @@
 """ book list views"""
+from typing import Optional
+
 from django.contrib.auth.decorators import login_required
 from django.core.paginator import Paginator
 from django.db import IntegrityError, transaction
@@ -140,7 +142,7 @@ class List(View):
                 .order_by(directional_sort_by)
             )
 
-        paginated = Paginator(items, 25)
+        paginated = Paginator(items, 12)
 
         if query and request.user.is_authenticated:
             # search for books
@@ -210,14 +212,17 @@ class Curate(View):
         suggestion = get_object_or_404(models.ListItem, id=request.POST.get("item"))
         approved = request.POST.get("approved") == "true"
         if approved:
+            # update the book and set it to be the last in the order of approved books, before any pending books
             suggestion.approved = True
-            current_order = suggestion.order
             order_max = (
-                book_list.listitem_set.aggregate(Max("order"))["order__max"] or 0
+                book_list.listitem_set.filter(approved=True).aggregate(Max("order"))[
+                    "order__max"
+                ]
+                or 0
             ) + 1
             suggestion.order = order_max
+            increment_order_in_reverse(book_list.id, order_max)
             suggestion.save()
-            normalize_book_list_ordering(book_list.id, start=current_order)
         else:
             deleted_order = suggestion.order
             suggestion.delete(broadcast=False)
@@ -232,13 +237,17 @@ def add_book(request):
     if not book_list.visible_to_user(request.user):
         return HttpResponseNotFound()
 
-    order_max = book_list.listitem_set.aggregate(Max("order"))["order__max"] or 0
-
     book = get_object_or_404(models.Edition, id=request.POST.get("book"))
     # do you have permission to add to the list?
     try:
         if request.user == book_list.user or book_list.curation == "open":
-            # go ahead and add it
+            # add the book at the latest order of approved books, before any pending books
+            order_max = (
+                book_list.listitem_set.filter(approved=True).aggregate(Max("order"))[
+                    "order__max"
+                ]
+            ) or 0
+            increment_order_in_reverse(book_list.id, order_max + 1)
             models.ListItem.objects.create(
                 book=book,
                 book_list=book_list,
@@ -246,7 +255,10 @@ def add_book(request):
                 order=order_max + 1,
             )
         elif book_list.curation == "curated":
-            # make a pending entry
+            # make a pending entry at the end of the list
+            order_max = (
+                book_list.listitem_set.aggregate(Max("order"))["order__max"]
+            ) or 0
             models.ListItem.objects.create(
                 approved=False,
                 book=book,
@@ -283,7 +295,8 @@ def remove_book(request, list_id):
 @require_POST
 def set_book_position(request, list_item_id):
     """
-    Action for when the list user manually specifies a list position, takes special care with the unique ordering per list
+    Action for when the list user manually specifies a list position, takes
+    special care with the unique ordering per list.
     """
     with transaction.atomic():
         list_item = get_object_or_404(models.ListItem, id=list_item_id)
@@ -298,7 +311,12 @@ def set_book_position(request, list_item_id):
             return HttpResponseBadRequest("position cannot be less than 1")
 
         book_list = list_item.book_list
-        order_max = book_list.listitem_set.aggregate(Max("order"))["order__max"]
+
+        # the max position to which a book may be set is the highest order for
+        # books which are approved
+        order_max = book_list.listitem_set.filter(approved=True).aggregate(
+            Max("order")
+        )["order__max"]
 
         if int_position > order_max:
             int_position = order_max
@@ -325,14 +343,17 @@ def set_book_position(request, list_item_id):
 
 
 @transaction.atomic
-def increment_order_in_reverse(book_list_id, start, end):
+def increment_order_in_reverse(
+    book_list_id: int, start: int, end: Optional[int] = None
+):
     try:
         book_list = models.List.objects.get(id=book_list_id)
     except models.List.DoesNotExist:
         return
-    items = book_list.listitem_set.filter(order__gte=start, order__lt=end).order_by(
+    items = book_list.listitem_set.filter(order__gte=start)
-        "-order"
+    if end is not None:
-    )
+        items = items.filter(order__lt=end)
+    items = items.order_by("-order")
     for item in items:
         item.order += 1
         item.save()