Merge pull request #1469 from bookwyrm-social/user-shelf

Reorganize user/shelf/goal views code

bookwyrm/forms.py

@@ -268,7 +268,7 @@ class CreateInviteForm(CustomForm):
 class ShelfForm(CustomForm):
     class Meta:
         model = models.Shelf
-        fields = ["user", "name", "privacy"]
+        fields = ["user", "name", "privacy", "description"]
 
 
 class GoalForm(CustomForm):

bookwyrm/migrations/0100_shelf_description.py

@@ -0,0 +1,18 @@
+# Generated by Django 3.2.5 on 2021-09-28 23:20
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("bookwyrm", "0099_readthrough_is_active"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="shelf",
+            name="description",
+            field=models.TextField(blank=True, max_length=500, null=True),
+        ),
+    ]

bookwyrm/models/shelf.py

@@ -21,6 +21,7 @@ class Shelf(OrderedCollectionMixin, BookWyrmModel):
 
     name = fields.CharField(max_length=100)
     identifier = models.CharField(max_length=100)
+    description = models.TextField(blank=True, null=True, max_length=500)
     user = fields.ForeignKey(
         "User", on_delete=models.PROTECT, activitypub_field="owner"
     )
@@ -52,6 +53,11 @@ class Shelf(OrderedCollectionMixin, BookWyrmModel):
         """list of books for this shelf, overrides OrderedCollectionMixin"""
         return self.books.order_by("shelfbook")
 
+    @property
+    def deletable(self):
+        """can the shelf be safely deleted?"""
+        return self.editable and not self.shelfbook_set.exists()
+
     def get_remote_id(self):
         """shelf identifier instead of id"""
         base_path = self.user.remote_id
@@ -61,7 +67,7 @@ class Shelf(OrderedCollectionMixin, BookWyrmModel):
     def raise_not_deletable(self, viewer):
         """don't let anyone delete a default shelf"""
         super().raise_not_deletable(viewer)
-        if not self.editable:
+        if not self.deletable:
             raise PermissionDenied()
 
     class Meta:

bookwyrm/templates/shelf/create_shelf_form.html

@@ -0,0 +1,13 @@
+{% extends 'components/inline_form.html' %}
+{% load i18n %}
+
+{% block header %}
+{% trans "Create Shelf" %}
+{% endblock %}
+
+{% block form %}
+<form name="create-shelf" action="{% url 'shelf-create' %}" method="post">
+    {% include "shelf/form.html" with editable=shelf.editable form=create_form %}
+</form>
+{% endblock %}
+

bookwyrm/templates/shelf/edit_shelf_form.html

@@ -0,0 +1,13 @@
+{% extends 'components/inline_form.html' %}
+{% load i18n %}
+
+{% block header %}
+{% trans "Edit Shelf" %}
+{% endblock %}
+
+{% block form %}
+<form name="edit-shelf" action="{{ shelf.local_path }}" method="post">
+    {% include "shelf/form.html" with editable=shelf.editable form=edit_form privacy=shelf.privacy %}
+</form>
+{% endblock %}
+

bookwyrm/templates/shelf/form.html

@@ -0,0 +1,28 @@
+{% load i18n %}
+{% load utilities %}
+{% with 0|uuid as uuid %}
+{% csrf_token %}
+<input type="hidden" name="user" value="{{ request.user.id }}">
+
+{% if editable %}
+<div class="field">
+    <label class="label" for="id_name">{% trans "Name:" %}</label>
+    <input type="text" name="name" value="{{ form.name.value|default:'' }}" maxlength="100" class="input" required="" id="id_name">
+</div>
+{% else %}
+<input type="hidden" name="name" required="true" value="{{ shelf.name }}">
+{% endif %}
+
+<div class="field">
+    <label class="label" for="id_description_{{ uuid }}">{% trans "Description:" %}</label>
+    <textarea name="description" cols="40" rows="5" maxlength="500" class="textarea" id="id_description_{{ uuid }}">{{ form.description.value|default:'' }}</textarea>
+</div>
+<div class="field has-addons">
+    <div class="control">
+        {% include 'snippets/privacy_select.html' with current=privacy %}
+    </div>
+    <div class="control">
+        <button class="button is-primary" type="submit">{% trans "Save" %}</button>
+    </div>
+</div>
+{% endwith %}

bookwyrm/templates/shelf/shelf.html

@@ -5,7 +5,7 @@
 {% load i18n %}
 
 {% block title %}
-{% include 'user/shelf/books_header.html' %}
+{% include 'user/books_header.html' %}
 {% endblock %}
 
 {% block opengraph_images %}
@@ -15,7 +15,7 @@
 {% block content %}
 <header class="block">
     <h1 class="title">
-        {% include 'user/shelf/books_header.html' %}
+        {% include 'user/books_header.html' %}
     </h1>
 </header>
 
@@ -60,9 +60,10 @@
 </div>
 
 <div class="block">
-    {% include 'user/shelf/create_shelf_form.html' with controls_text='create_shelf_form' %}
+    {% include 'shelf/create_shelf_form.html' with controls_text='create_shelf_form' %}
 </div>
 
+<div>
     <div class="block columns is-mobile">
         <div class="column">
             <h2 class="title is-3">
@@ -91,14 +92,30 @@
         </div>
         {% if is_self and shelf.id %}
         <div class="column is-narrow">
+            <div class="is-flex">
                 {% trans "Edit shelf" as button_text %}
                 {% include 'snippets/toggle/open_button.html' with text=button_text icon_with_text="pencil" controls_text="edit_shelf_form" focus="edit_shelf_form_header" %}
+
+                {% if shelf.deletable %}
+                <form class="ml-1" name="delete-shelf" action="/delete-shelf/{{ shelf.id }}" method="post">
+                    {% csrf_token %}
+                    <input type="hidden" name="user" value="{{ request.user.id }}">
+                    <button class="button is-danger is-light" type="submit">
+                        {% trans "Delete shelf" %}
+                    </button>
+                </form>
+                {% endif %}
             </div>
+        </div>
+        {% endif %}
+    </div>
+    {% if shelf.description %}
+    <p>{{ shelf.description }}</p>
     {% endif %}
 </div>
 
 <div class="block">
-    {% include 'user/shelf/edit_shelf_form.html' with controls_text="edit_shelf_form" %}
+    {% include 'shelf/edit_shelf_form.html' with controls_text="edit_shelf_form" %}
 </div>
 
 <div class="block">
@@ -167,17 +184,7 @@
             </tbody>
         </table>
         {% else %}
-        <p>{% trans "This shelf is empty." %}</p>
-        {% if shelf.id and shelf.editable %}
-        <form name="delete-shelf" action="/delete-shelf/{{ shelf.id }}" method="post">
-            {% csrf_token %}
-            <input type="hidden" name="user" value="{{ request.user.id }}">
-            <button class="button is-danger is-light" type="submit">
-                {% trans "Delete shelf" %}
-            </button>
-        </form>
-        {% endif %}
-
+        <p><em>{% trans "This shelf is empty." %}</em></p>
         {% endif %}
 
     </div>

bookwyrm/templates/snippets/privacy_select.html

@@ -1,7 +1,7 @@
 {% load i18n %}
 {% load utilities %}
 <div class="select {{ class }}">
-    {% with 0|uuid as uuid %}
+    {% firstof uuid 0|uuid as uuid %}
     {% if not no_label %}
     <label class="is-sr-only" for="privacy_{{ uuid }}">{% trans "Post privacy" %}</label>
     {% endif %}
@@ -20,6 +20,5 @@
             {% trans "Private" %}
         </option>
     </select>
-    {% endwith %}
 </div>
 

bookwyrm/templates/user/relationships/followers.html

@@ -9,6 +9,6 @@
 
 {% block nullstate %}
 <div>
-    {% blocktrans with username=user.display_name %}{{ username }} has no followers{% endblocktrans %}
+    <em>{% blocktrans with username=user.display_name %}{{ username }} has no followers{% endblocktrans %}</em>
 </div>
 {% endblock %}

bookwyrm/templates/user/relationships/following.html

@@ -9,7 +9,7 @@
 
 {% block nullstate %}
 <div>
-    {% blocktrans with username=user.display_name %}{{ username }} isn't following any users{% endblocktrans %}
+    <em>{% blocktrans with username=user.display_name %}{{ username }} isn't following any users{% endblocktrans %}</em>
 </div>
 {% endblock %}
 

bookwyrm/templates/user/shelf/create_shelf_form.html

@@ -1,27 +0,0 @@
-{% extends 'components/inline_form.html' %}
-{% load i18n %}
-
-{% block header %}
-{% trans "Create Shelf" %}
-{% endblock %}
-
-{% block form %}
-<form name="create-shelf" action="{% url 'shelf-create' %}" method="post">
-    {% csrf_token %}
-    <input type="hidden" name="user" value="{{ request.user.id }}">
-    <div class="field">
-        <label class="label" for="id_name_create">{% trans "Name:" %}</label>
-        <input type="text" name="name" maxlength="100" class="input" required="true" id="id_name_create">
-    </div>
-
-    <div class="field has-addons">
-        <div class="control">
-            {% include 'snippets/privacy_select.html' %}
-        </div>
-        <div class="control">
-            <button class="button is-primary" type="submit">{% trans "Create Shelf" %}</button>
-        </div>
-    </div>
-</form>
-{% endblock %}
-

bookwyrm/templates/user/shelf/edit_shelf_form.html

@@ -1,31 +0,0 @@
-{% extends 'components/inline_form.html' %}
-{% load i18n %}
-
-{% block header %}
-{% trans "Edit Shelf" %}
-{% endblock %}
-
-{% block form %}
-<form name="edit-shelf" action="{{ shelf.local_path }}" method="post">
-    {% csrf_token %}
-    <input type="hidden" name="user" value="{{ request.user.id }}">
-    {% if shelf.editable %}
-    <div class="field">
-        <label class="label" for="id_name">{% trans "Name:" %}</label>
-        <input type="text" name="name" maxlength="100" class="input" required="true" value="{{ shelf.name }}" id="id_name">
-    </div>
-    {% else %}
-    <input type="hidden" name="name" required="true" value="{{ shelf.name }}">
-    {% endif %}
-
-    <div class="field has-addons">
-        <div class="control">
-            {% include 'snippets/privacy_select.html' with current=shelf.privacy %}
-        </div>
-        <div class="control">
-            <button class="button is-primary" type="submit">{% trans "Update shelf" %}</button>
-        </div>
-    </div>
-</form>
-{% endblock %}
-

bookwyrm/templates/user/user.html

@@ -24,7 +24,7 @@
 {% if user.bookwyrm_user %}
 <div class="block">
     <h2 class="title">
-        {% include 'user/shelf/books_header.html' %}
+        {% include 'user/books_header.html' %}
     </h2>
     <div class="columns is-mobile scroll-x">
         {% for shelf in shelves %}

bookwyrm/tests/views/test_shelf.py

@@ -1,11 +1,14 @@
 """ test for app action functionality """
 import json
 from unittest.mock import patch
+from tidylib import tidy_document
+
+from django.core.exceptions import PermissionDenied
 from django.template.response import TemplateResponse
 from django.test import TestCase
 from django.test.client import RequestFactory
 
-from bookwyrm import models, views
+from bookwyrm import forms, models, views
 from bookwyrm.activitypub import ActivitypubResponse
 
 
@@ -53,7 +56,16 @@ class ShelfViews(TestCase):
             is_api.return_value = False
             result = view(request, self.local_user.username, shelf.identifier)
         self.assertIsInstance(result, TemplateResponse)
-        result.render()
+        html = result.render()
+        _, errors = tidy_document(
+            html.content,
+            options={
+                "drop-empty-elements": False,
+                "warn-proprietary-attributes": False,
+            },
+        )
+        if errors:
+            raise Exception(errors)
         self.assertEqual(result.status_code, 200)
 
         with patch("bookwyrm.views.shelf.is_api_request") as is_api:
@@ -122,7 +134,7 @@ class ShelfViews(TestCase):
 
         self.assertEqual(shelf.name, "To Read")
 
-    def test_handle_shelve(self, *_):
+    def test_shelve(self, *_):
         """shelve a book"""
         request = self.factory.post(
             "", {"book": self.book.id, "shelf": self.shelf.identifier}
@@ -140,7 +152,7 @@ class ShelfViews(TestCase):
         # make sure the book is on the shelf
         self.assertEqual(self.shelf.books.get(), self.book)
 
-    def test_handle_shelve_to_read(self, *_):
+    def test_shelve_to_read(self, *_):
         """special behavior for the to-read shelf"""
         shelf = models.Shelf.objects.get(identifier="to-read")
         request = self.factory.post(
@@ -153,7 +165,7 @@ class ShelfViews(TestCase):
         # make sure the book is on the shelf
         self.assertEqual(shelf.books.get(), self.book)
 
-    def test_handle_shelve_reading(self, *_):
+    def test_shelve_reading(self, *_):
         """special behavior for the reading shelf"""
         shelf = models.Shelf.objects.get(identifier="reading")
         request = self.factory.post(
@@ -166,7 +178,7 @@ class ShelfViews(TestCase):
         # make sure the book is on the shelf
         self.assertEqual(shelf.books.get(), self.book)
 
-    def test_handle_shelve_read(self, *_):
+    def test_shelve_read(self, *_):
         """special behavior for the read shelf"""
         shelf = models.Shelf.objects.get(identifier="read")
         request = self.factory.post(
@@ -179,7 +191,7 @@ class ShelfViews(TestCase):
         # make sure the book is on the shelf
         self.assertEqual(shelf.books.get(), self.book)
 
-    def test_handle_unshelve(self, *_):
+    def test_unshelve(self, *_):
         """remove a book from a shelf"""
         with patch("bookwyrm.models.activitypub_mixin.broadcast_task.delay"):
             models.ShelfBook.objects.create(
@@ -197,3 +209,76 @@ class ShelfViews(TestCase):
         self.assertEqual(activity["type"], "Remove")
         self.assertEqual(activity["object"]["id"], item.remote_id)
         self.assertEqual(self.shelf.books.count(), 0)
+
+    def test_create_shelf(self, *_):
+        """a brand new custom shelf"""
+        form = forms.ShelfForm()
+        form.data["user"] = self.local_user.id
+        form.data["name"] = "new shelf name"
+        form.data["description"] = "desc"
+        form.data["privacy"] = "unlisted"
+        request = self.factory.post("", form.data)
+        request.user = self.local_user
+
+        views.create_shelf(request)
+
+        shelf = models.Shelf.objects.get(name="new shelf name")
+        self.assertEqual(shelf.privacy, "unlisted")
+        self.assertEqual(shelf.description, "desc")
+        self.assertEqual(shelf.user, self.local_user)
+
+    def test_delete_shelf(self, *_):
+        """delete a brand new custom shelf"""
+        request = self.factory.post("")
+        request.user = self.local_user
+        shelf_id = self.shelf.id
+
+        views.delete_shelf(request, shelf_id)
+
+        self.assertFalse(models.Shelf.objects.filter(id=shelf_id).exists())
+
+    def test_delete_shelf_unauthorized(self, *_):
+        """delete a brand new custom shelf"""
+        with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch(
+            "bookwyrm.activitystreams.populate_stream_task.delay"
+        ):
+            rat = models.User.objects.create_user(
+                "rat@local.com",
+                "rat@mouse.mouse",
+                "password",
+                local=True,
+                localname="rat",
+            )
+        request = self.factory.post("")
+        request.user = rat
+
+        with self.assertRaises(PermissionDenied):
+            views.delete_shelf(request, self.shelf.id)
+
+        self.assertTrue(models.Shelf.objects.filter(id=self.shelf.id).exists())
+
+    def test_delete_shelf_has_book(self, *_):
+        """delete a brand new custom shelf"""
+        with patch("bookwyrm.models.activitypub_mixin.broadcast_task.delay"):
+            models.ShelfBook.objects.create(
+                book=self.book, user=self.local_user, shelf=self.shelf
+            )
+        request = self.factory.post("")
+        request.user = self.local_user
+
+        with self.assertRaises(PermissionDenied):
+            views.delete_shelf(request, self.shelf.id)
+
+        self.assertTrue(models.Shelf.objects.filter(id=self.shelf.id).exists())
+
+    def test_delete_shelf_not_editable(self, *_):
+        """delete a brand new custom shelf"""
+        shelf = self.local_user.shelf_set.first()
+        self.assertFalse(shelf.editable)
+        request = self.factory.post("")
+        request.user = self.local_user
+
+        with self.assertRaises(PermissionDenied):
+            views.delete_shelf(request, shelf.id)
+
+        self.assertTrue(models.Shelf.objects.filter(id=shelf.id).exists())

bookwyrm/tests/views/test_user.py

@@ -1,5 +1,6 @@
 """ test for app action functionality """
 from unittest.mock import patch
+from tidylib import tidy_document
 
 from django.contrib.auth.models import AnonymousUser
 from django.http.response import Http404
@@ -55,7 +56,16 @@ class UserViews(TestCase):
             is_api.return_value = False
             result = view(request, "mouse")
         self.assertIsInstance(result, TemplateResponse)
-        result.render()
+        html = result.render()
+        _, errors = tidy_document(
+            html.content,
+            options={
+                "drop-empty-elements": False,
+                "warn-proprietary-attributes": False,
+            },
+        )
+        if errors:
+            raise Exception(errors)
         self.assertEqual(result.status_code, 200)
 
         request.user = self.anonymous_user
@@ -63,7 +73,16 @@ class UserViews(TestCase):
             is_api.return_value = False
             result = view(request, "mouse")
         self.assertIsInstance(result, TemplateResponse)
-        result.render()
+        html = result.render()
+        _, errors = tidy_document(
+            html.content,
+            options={
+                "drop-empty-elements": False,
+                "warn-proprietary-attributes": False,
+            },
+        )
+        if errors:
+            raise Exception(errors)
         self.assertEqual(result.status_code, 200)
 
         with patch("bookwyrm.views.user.is_api_request") as is_api:
@@ -92,7 +111,16 @@ class UserViews(TestCase):
             is_api.return_value = False
             result = view(request, "mouse")
         self.assertIsInstance(result, TemplateResponse)
-        result.render()
+        html = result.render()
+        _, errors = tidy_document(
+            html.content,
+            options={
+                "drop-empty-elements": False,
+                "warn-proprietary-attributes": False,
+            },
+        )
+        if errors:
+            raise Exception(errors)
         self.assertEqual(result.status_code, 200)
 
         with patch("bookwyrm.views.user.is_api_request") as is_api:
@@ -123,7 +151,16 @@ class UserViews(TestCase):
             is_api.return_value = False
             result = view(request, "mouse")
         self.assertIsInstance(result, TemplateResponse)
-        result.render()
+        html = result.render()
+        _, errors = tidy_document(
+            html.content,
+            options={
+                "drop-empty-elements": False,
+                "warn-proprietary-attributes": False,
+            },
+        )
+        if errors:
+            raise Exception(errors)
         self.assertEqual(result.status_code, 200)
 
         with patch("bookwyrm.views.user.is_api_request") as is_api:

bookwyrm/views/goal.py

@@ -41,7 +41,7 @@ class Goal(View):
             "year": year,
             "is_self": request.user == user,
         }
-        return TemplateResponse(request, "goal.html", data)
+        return TemplateResponse(request, "user/goal.html", data)
 
     def post(self, request, username, year):
         """update or create an annual goal"""
@@ -58,7 +58,7 @@ class Goal(View):
                 "goal": goal,
                 "year": year,
             }
-            return TemplateResponse(request, "goal.html", data)
+            return TemplateResponse(request, "user/goal.html", data)
         goal = form.save()
 
         if request.POST.get("post-status"):

bookwyrm/views/shelf.py

@@ -85,12 +85,14 @@ class Shelf(View):
             "shelves": shelves,
             "shelf": shelf,
             "books": page,
+            "edit_form": forms.ShelfForm(instance=shelf if shelf_identifier else None),
+            "create_form": forms.ShelfForm(),
             "page_range": paginated.get_elided_page_range(
                 page.number, on_each_side=2, on_ends=1
             ),
         }
 
-        return TemplateResponse(request, "user/shelf/shelf.html", data)
+        return TemplateResponse(request, "shelf/shelf.html", data)
 
     @method_decorator(login_required, name="dispatch")
     # pylint: disable=unused-argument
@@ -128,7 +130,7 @@ def create_shelf(request):
 def delete_shelf(request, shelf_id):
     """user generated shelves"""
     shelf = get_object_or_404(models.Shelf, id=shelf_id)
-    shelf.raise_not_deletable()
+    shelf.raise_not_deletable(request.user)
 
     shelf.delete()
     return redirect("user-shelves", request.user.localname)