From fa75438070e6b9e800eafe76a82d8737f972278b Mon Sep 17 00:00:00 2001 From: Pablo Barton Date: Sun, 18 Apr 2021 14:46:28 -0400 Subject: [PATCH] per review, removing custom pagination for booklist and handling all pending books at the end of the list --- bookwyrm/templates/lists/list.html | 2 +- .../snippets/booklist-pagination.html | 58 --------- bookwyrm/tests/views/test_list.py | 118 ++++++++++++++++++ bookwyrm/views/list.py | 49 +++++--- 4 files changed, 154 insertions(+), 73 deletions(-) delete mode 100644 bookwyrm/templates/snippets/booklist-pagination.html diff --git a/bookwyrm/templates/lists/list.html b/bookwyrm/templates/lists/list.html index c9464c85b..2f0e7d678 100644 --- a/bookwyrm/templates/lists/list.html +++ b/bookwyrm/templates/lists/list.html @@ -57,7 +57,7 @@ {% endfor %} {% endif %} - {% include "snippets/booklist-pagination.html" with page=items %} + {% include "snippets/pagination.html" with page=items %}
diff --git a/bookwyrm/templates/snippets/booklist-pagination.html b/bookwyrm/templates/snippets/booklist-pagination.html deleted file mode 100644 index 161c743cb..000000000 --- a/bookwyrm/templates/snippets/booklist-pagination.html +++ /dev/null @@ -1,58 +0,0 @@ -{% load i18n %} -{% if page.has_other_pages %} - -{% endif %} diff --git a/bookwyrm/tests/views/test_list.py b/bookwyrm/tests/views/test_list.py index 02581939b..215f3e61e 100644 --- a/bookwyrm/tests/views/test_list.py +++ b/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 diff --git a/bookwyrm/views/list.py b/bookwyrm/views/list.py index 2f01410d2..7fb4979cb 100644 --- a/bookwyrm/views/list.py +++ b/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( - "-order" - ) + items = book_list.listitem_set.filter(order__gte=start) + 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()