Merge pull request #1981 from bookwyrm-social/list-duplicate-books

Show meaningful message when you try to add a duplicate book to a list

bookwyrm/templates/book/book.html

@@ -352,7 +352,7 @@
                 {% endfor %}
                 </ul>
 
-                {% if request.user.list_set.exists %}
+                {% if list_options.exists %}
                 <form name="list-add" method="post" action="{% url 'list-add-book' %}">
                     {% csrf_token %}
                     <input type="hidden" name="book" value="{{ book.id }}">
@@ -361,7 +361,7 @@
                     <div class="field has-addons">
                         <div class="select control is-clipped">
                             <select name="book_list" id="id_list">
-                                {% for list in user.list_set.all %}
+                                {% for list in list_options %}
                                 <option value="{{ list.id }}">{{ list.name }}</option>
                                 {% endfor %}
                             </select>

bookwyrm/templates/lists/list.html

@@ -30,13 +30,23 @@
 
 <div class="columns mt-3">
     <section class="column is-three-quarters">
-        {% if request.GET.updated %}
+        {% if add_failed %}
-        <div class="notification is-primary">
+        <div class="notification is-danger is-light">
-            {% if list.curation != "open" and request.user != list.user and not list.group|is_member:request.user %}
+            <span class="icon icon-x" aria-hidden="true"></span>
-            {% trans "You successfully suggested a book for this list!" %}
+            <span>
-            {% else %}
+                {% trans "That book is already on this list." %}
-            {% trans "You successfully added a book to this list!" %}
+            </span>
-            {% endif %}
+        </div>
+        {% elif add_succeeded %}
+        <div class="notification is-success is-light">
+            <span class="icon icon-check" aria-hidden="true"></span>
+            <span>
+                {% if list.curation != "open" and request.user != list.user and not list.group|is_member:request.user %}
+                    {% trans "You successfully suggested a book for this list!" %}
+                {% else %}
+                    {% trans "You successfully added a book to this list!" %}
+                {% endif %}
+            </span>
         </div>
         {% endif %}
 

bookwyrm/views/books/books.py

@@ -83,6 +83,7 @@ class Book(View):
         }
 
         if request.user.is_authenticated:
+            data["list_options"] = request.user.list_set.exclude(id__in=data["lists"])
             data["file_link_form"] = forms.FileLinkForm()
             readthroughs = models.ReadThrough.objects.filter(
                 user=request.user,

bookwyrm/views/list/list.py

@@ -1,11 +1,10 @@
 """ book list views"""
 from typing import Optional
-from urllib.parse import urlencode
 
 from django.contrib.auth.decorators import login_required
 from django.core.exceptions import PermissionDenied
 from django.core.paginator import Paginator
-from django.db import IntegrityError, transaction
+from django.db import transaction
 from django.db.models import Avg, DecimalField, Q, Max
 from django.db.models.functions import Coalesce
 from django.http import HttpResponseBadRequest, HttpResponse
@@ -26,7 +25,7 @@ from bookwyrm.views.helpers import is_api_request
 class List(View):
     """book list page"""
 
-    def get(self, request, list_id):
+    def get(self, request, list_id, add_failed=False, add_succeeded=False):
         """display a book list"""
         book_list = get_object_or_404(models.List, id=list_id)
         book_list.raise_visible_to_user(request.user)
@@ -37,33 +36,10 @@ class List(View):
         query = request.GET.get("q")
         suggestions = None
 
-        # sort_by shall be "order" unless a valid alternative is given
+        items = book_list.listitem_set.filter(approved=True).prefetch_related(
-        sort_by = request.GET.get("sort_by", "order")
+            "user", "book", "book__authors"
-        if sort_by not in ("order", "title", "rating"):
+        )
-            sort_by = "order"
+        items = sort_list(request, items)
-
-        # direction shall be "ascending" unless a valid alternative is given
-        direction = request.GET.get("direction", "ascending")
-        if direction not in ("ascending", "descending"):
-            direction = "ascending"
-
-        directional_sort_by = {
-            "order": "order",
-            "title": "book__title",
-            "rating": "average_rating",
-        }[sort_by]
-        if direction == "descending":
-            directional_sort_by = "-" + directional_sort_by
-
-        items = book_list.listitem_set.prefetch_related("user", "book", "book__authors")
-        if sort_by == "rating":
-            items = items.annotate(
-                average_rating=Avg(
-                    Coalesce("book__review__rating", 0.0),
-                    output_field=DecimalField(),
-                )
-            )
-        items = items.filter(approved=True).order_by(directional_sort_by)
 
         paginated = Paginator(items, PAGE_LENGTH)
 
@@ -106,10 +82,10 @@ class List(View):
             "suggested_books": suggestions,
             "list_form": forms.ListForm(instance=book_list),
             "query": query or "",
-            "sort_form": forms.SortListForm(
+            "sort_form": forms.SortListForm(request.GET),
-                {"direction": direction, "sort_by": sort_by}
-            ),
             "embed_url": embed_url,
+            "add_failed": add_failed,
+            "add_succeeded": add_succeeded,
         }
         return TemplateResponse(request, "lists/list.html", data)
 
@@ -131,6 +107,36 @@ class List(View):
         return redirect(book_list.local_path)
 
 
+def sort_list(request, items):
+    """helper to handle the surprisngly involved sorting"""
+    # sort_by shall be "order" unless a valid alternative is given
+    sort_by = request.GET.get("sort_by", "order")
+    if sort_by not in ("order", "title", "rating"):
+        sort_by = "order"
+
+    # direction shall be "ascending" unless a valid alternative is given
+    direction = request.GET.get("direction", "ascending")
+    if direction not in ("ascending", "descending"):
+        direction = "ascending"
+
+    directional_sort_by = {
+        "order": "order",
+        "title": "book__title",
+        "rating": "average_rating",
+    }[sort_by]
+    if direction == "descending":
+        directional_sort_by = "-" + directional_sort_by
+
+    if sort_by == "rating":
+        items = items.annotate(
+            average_rating=Avg(
+                Coalesce("book__review__rating", 0.0),
+                output_field=DecimalField(),
+            )
+        )
+    return items.order_by(directional_sort_by)
+
+
 @require_POST
 @login_required
 def save_list(request, list_id):
@@ -179,8 +185,8 @@ def add_book(request):
 
     form = forms.ListItemForm(request.POST)
     if not form.is_valid():
-        # this shouldn't happen, there aren't validated fields
+        return List().get(request, book_list.id, add_failed=True)
-        raise Exception(form.errors)
+
     item = form.save(commit=False)
 
     if book_list.curation == "curated":
@@ -196,17 +202,9 @@ def add_book(request):
         ) or 0
         increment_order_in_reverse(book_list.id, order_max + 1)
     item.order = order_max + 1
+    item.save()
 
-    try:
+    return List().get(request, book_list.id, add_succeeded=True)
-        item.save()
-    except IntegrityError:
-        # if the book is already on the list, don't flip out
-        pass
-
-    path = reverse("list", args=[book_list.id])
-    params = request.GET.copy()
-    params["updated"] = True
-    return redirect(f"{path}?{urlencode(params)}")
 
 
 @require_POST