Merge pull request #2007 from viviicat/url-names

Add names of books/lists/authors/etc as slugs, redirect to slugified version of the page

bookwyrm/models/base_model.py

@@ -8,6 +8,7 @@ from django.db.models import Q
 from django.dispatch import receiver
 from django.http import Http404
 from django.utils.translation import gettext_lazy as _
+from django.utils.text import slugify
 
 from bookwyrm.settings import DOMAIN
 from .fields import RemoteIdField
@@ -35,10 +36,11 @@ class BookWyrmModel(models.Model):
     remote_id = RemoteIdField(null=True, activitypub_field="id")
 
     def get_remote_id(self):
-        """generate a url that resolves to the local object"""
+        """generate the url that resolves to the local object, without a slug"""
         base_path = f"https://{DOMAIN}"
         if hasattr(self, "user"):
             base_path = f"{base_path}{self.user.local_path}"
+
         model_name = type(self).__name__.lower()
         return f"{base_path}/{model_name}/{self.id}"
 
@@ -49,8 +51,20 @@ class BookWyrmModel(models.Model):
 
     @property
     def local_path(self):
-        """how to link to this object in the local app"""
+        """how to link to this object in the local app, with a slug"""
-        return self.get_remote_id().replace(f"https://{DOMAIN}", "")
+        local = self.get_remote_id().replace(f"https://{DOMAIN}", "")
+
+        name = None
+        if hasattr(self, "name_field"):
+            name = getattr(self, self.name_field)
+        elif hasattr(self, "name"):
+            name = self.name
+
+        if name:
+            slug = slugify(name)
+            local = f"{local}/s/{slug}"
+
+        return local
 
     def raise_visible_to_user(self, viewer):
         """is a user authorized to view an object?"""

bookwyrm/models/shelf.py

@@ -6,6 +6,7 @@ from django.db import models
 from django.utils import timezone
 
 from bookwyrm import activitypub
+from bookwyrm.settings import DOMAIN
 from .activitypub_mixin import CollectionItemMixin, OrderedCollectionMixin
 from .base_model import BookWyrmModel
 from . import fields
@@ -65,6 +66,11 @@ class Shelf(OrderedCollectionMixin, BookWyrmModel):
         identifier = self.identifier or self.get_identifier()
         return f"{base_path}/books/{identifier}"
 
+    @property
+    def local_path(self):
+        """No slugs"""
+        return self.get_remote_id().replace(f"https://{DOMAIN}", "")
+
     def raise_not_deletable(self, viewer):
         """don't let anyone delete a default shelf"""
         super().raise_not_deletable(viewer)

bookwyrm/templates/book/book.html

@@ -284,7 +284,7 @@
                 {% if user_statuses.review_count or user_statuses.comment_count or user_statuses.quotation_count %}
                 <nav class="tabs">
                     <ul>
-                        {% url 'book' book.id as tab_url %}
+                        {% url 'book' book.id book.name|slugify as tab_url %}
                         <li {% if tab_url == request.path %}class="is-active"{% endif %}>
                             <a href="{{ tab_url }}#reviews">{% trans "Reviews" %} ({{ review_count }})</a>
                         </li>

bookwyrm/templates/book/file_links/edit_links.html

@@ -15,7 +15,7 @@
 
     <nav class="breadcrumb subtitle" aria-label="breadcrumbs">
         <ul>
-            <li><a href="{% url 'book' book.id %}">{{ book|book_title }}</a></li>
+            <li><a href="{% url 'book' book.id book.name|slugify %}">{{ book|book_title }}</a></li>
             <li class="is-active">
                 <a href="#" aria-current="page">
                     {% trans "Edit links" %}

bookwyrm/templates/lists/curate.html

@@ -6,7 +6,7 @@
 <nav class="breadcrumb subtitle" aria-label="breadcrumbs">
     <ul>
         <li><a href="{% url 'lists' %}">{% trans "Lists" %}</a></li>
-        <li><a href="{% url 'list' list.id %}">{{ list.name|truncatechars:30 }}</a></li>
+        <li><a href="{% url 'list' list_id=list.id slug=list.name|slugify %}">{{ list.name|truncatechars:30 }}</a></li>
         <li class="is-active">
             <a href="#" aria-current="page">
                 {% trans "Curate" %}

bookwyrm/templates/lists/list.html

@@ -180,7 +180,7 @@
         <h2 class="title is-5">
             {% trans "Sort List" %}
         </h2>
-        <form name="sort" action="{% url 'list' list.id %}" method="GET" class="block">
+        <form name="sort" action="{% url 'list' list_id=list.id slug=list.name|slugify %}" method="GET" class="block">
             <div class="field">
                 <label class="label" for="id_sort_by">{% trans "Sort By" %}</label>
                 <div class="select is-fullwidth">
@@ -207,7 +207,7 @@
                 {% trans "Suggest Books" %}
             {% endif %}
         </h2>
-        <form name="search" action="{% url 'list' list.id %}" method="GET" class="block">
+        <form name="search" action="{% url 'list' list_id=list.id slug=list.name|slugify %}" method="GET" class="block">
             <div class="field has-addons">
                 <div class="control">
                     <input aria-label="{% trans 'Search for a book' %}" class="input" type="text" name="q" placeholder="{% trans 'Search for a book' %}" value="{{ query }}">
@@ -221,7 +221,7 @@
                 </div>
             </div>
             {% if query %}
-            <p class="help"><a href="{% url 'list' list.id %}">{% trans "Clear search" %}</a></p>
+            <p class="help"><a href="{% url 'list' list_id=list.id slug=list.name|slugify %}">{% trans "Clear search" %}</a></p>
             {% endif %}
         </form>
         {% if not suggested_books %}

bookwyrm/tests/connectors/test_abstract_connector.py

@@ -101,6 +101,7 @@ class AbstractConnector(TestCase):
         result = self.connector.get_or_create_book(
             f"https://{DOMAIN}/book/{self.book.id}"
         )
+
         self.assertEqual(models.Book.objects.count(), 1)
         self.assertEqual(result, self.book)
 

bookwyrm/urls.py

@@ -391,6 +391,9 @@ urlpatterns = [
     re_path(
         r"^group/(?P<group_id>\d+)(.json)?/?$", views.Group.as_view(), name="group"
     ),
+    re_path(
+        rf"^group/(?P<group_id>\d+){regex.SLUG}/?$", views.Group.as_view(), name="group"
+    ),
     re_path(
         r"^group/delete/(?P<group_id>\d+)/?$", views.delete_group, name="delete-group"
     ),
@@ -417,7 +420,10 @@ urlpatterns = [
     re_path(rf"{USER_PATH}/lists/?$", views.UserLists.as_view(), name="user-lists"),
     re_path(r"^list/?$", views.Lists.as_view(), name="lists"),
     re_path(r"^list/saved/?$", views.SavedLists.as_view(), name="saved-lists"),
-    re_path(r"^list/(?P<list_id>\d+)(.json)?/?$", views.List.as_view(), name="list"),
+    re_path(r"^list/(?P<list_id>\d+)(\.json)?/?$", views.List.as_view(), name="list"),
+    re_path(
+        rf"^list/(?P<list_id>\d+){regex.SLUG}/?$", views.List.as_view(), name="list"
+    ),
     re_path(
         r"^list/(?P<list_id>\d+)/item/(?P<list_item>\d+)/?$",
         views.ListItem.as_view(),
@@ -487,6 +493,7 @@ urlpatterns = [
     re_path(r"^unblock/(?P<user_id>\d+)/?$", views.unblock),
     # statuses
     re_path(rf"{STATUS_PATH}(.json)?/?$", views.Status.as_view(), name="status"),
+    re_path(rf"{STATUS_PATH}{regex.SLUG}/?$", views.Status.as_view(), name="status"),
     re_path(rf"{STATUS_PATH}/activity/?$", views.Status.as_view(), name="status"),
     re_path(
         rf"{STATUS_PATH}/replies(.json)?/?$", views.Replies.as_view(), name="replies"
@@ -523,6 +530,7 @@ urlpatterns = [
     re_path(r"^unboost/(?P<status_id>\d+)/?$", views.Unboost.as_view()),
     # books
     re_path(rf"{BOOK_PATH}(.json)?/?$", views.Book.as_view(), name="book"),
+    re_path(rf"{BOOK_PATH}{regex.SLUG}/?$", views.Book.as_view(), name="book"),
     re_path(
         rf"{BOOK_PATH}/(?P<user_statuses>review|comment|quote)/?$",
         views.Book.as_view(),
@@ -580,6 +588,11 @@ urlpatterns = [
     re_path(
         r"^author/(?P<author_id>\d+)(.json)?/?$", views.Author.as_view(), name="author"
     ),
+    re_path(
+        rf"^author/(?P<author_id>\d+){regex.SLUG}/?$",
+        views.Author.as_view(),
+        name="author",
+    ),
     re_path(
         r"^author/(?P<author_id>\d+)/edit/?$",
         views.EditAuthor.as_view(),

bookwyrm/utils/regex.py

@@ -6,5 +6,6 @@ STRICT_LOCALNAME = r"@[a-zA-Z_\-\.0-9]+"
 USERNAME = rf"{LOCALNAME}(@{DOMAIN})?"
 STRICT_USERNAME = rf"\B{STRICT_LOCALNAME}(@{DOMAIN})?\b"
 FULL_USERNAME = rf"{LOCALNAME}@{DOMAIN}\b"
+SLUG = r"/s/(?P<slug>[-_a-z0-9]*)"
 # should match (BookWyrm/1.0.0; or (BookWyrm/99.1.2;
 BOOKWYRM_USER_AGENT = r"\(BookWyrm/[0-9]+\.[0-9]+\.[0-9]+;"

bookwyrm/views/author.py

@@ -11,20 +11,24 @@ from bookwyrm import forms, models
 from bookwyrm.activitypub import ActivitypubResponse
 from bookwyrm.connectors import connector_manager
 from bookwyrm.settings import PAGE_LENGTH
-from bookwyrm.views.helpers import is_api_request
+from bookwyrm.views.helpers import is_api_request, maybe_redirect_local_path
 
 
 # pylint: disable= no-self-use
 class Author(View):
     """this person wrote a book"""
 
-    def get(self, request, author_id):
+    # pylint: disable=unused-argument
+    def get(self, request, author_id, slug=None):
         """landing page for an author"""
         author = get_object_or_404(models.Author, id=author_id)
 
         if is_api_request(request):
             return ActivitypubResponse(author.to_activity())
 
+        if redirect_local_path := maybe_redirect_local_path(request, author):
+            return redirect_local_path
+
         books = (
             models.Work.objects.filter(editions__authors=author)
             .order_by("created_date")

bookwyrm/views/books/books.py

@@ -15,14 +15,14 @@ from bookwyrm.activitypub import ActivitypubResponse
 from bookwyrm.connectors import connector_manager, ConnectorException
 from bookwyrm.connectors.abstract_connector import get_image
 from bookwyrm.settings import PAGE_LENGTH
-from bookwyrm.views.helpers import is_api_request
+from bookwyrm.views.helpers import is_api_request, maybe_redirect_local_path
 
 
 # pylint: disable=no-self-use
 class Book(View):
     """a book! this is the stuff"""
 
-    def get(self, request, book_id, user_statuses=False, update_error=False):
+    def get(self, request, book_id, **kwargs):
         """info about a book"""
         if is_api_request(request):
             book = get_object_or_404(
@@ -30,7 +30,11 @@ class Book(View):
             )
             return ActivitypubResponse(book.to_activity())
 
-        user_statuses = user_statuses if request.user.is_authenticated else False
+        user_statuses = (
+            kwargs.get("user_statuses", False)
+            if request.user.is_authenticated
+            else False
+        )
 
         # it's safe to use this OR because edition and work and subclasses of the same
         # table, so they never have clashing IDs
@@ -46,6 +50,11 @@ class Book(View):
         if not book or not book.parent_work:
             raise Http404()
 
+        if redirect_local_path := not user_statuses and maybe_redirect_local_path(
+            request, book
+        ):
+            return redirect_local_path
+
         # all reviews for all editions of the book
         reviews = models.Review.privacy_filter(request.user).filter(
             book__parent_work__editions=book
@@ -80,7 +89,7 @@ class Book(View):
             else None,
             "rating": reviews.aggregate(Avg("rating"))["rating__avg"],
             "lists": lists,
-            "update_error": update_error,
+            "update_error": kwargs.get("update_error", False),
         }
 
         if request.user.is_authenticated:

bookwyrm/views/feed.py

@@ -15,7 +15,7 @@ from bookwyrm.activitypub import ActivitypubResponse
 from bookwyrm.settings import PAGE_LENGTH, STREAMS
 from bookwyrm.suggested_users import suggested_users
 from .helpers import filter_stream_by_status_type, get_user_from_username
-from .helpers import is_api_request, is_bookwyrm_request
+from .helpers import is_api_request, is_bookwyrm_request, maybe_redirect_local_path
 from .annual_summary import get_annual_summary_year
 
 
@@ -113,7 +113,8 @@ class DirectMessage(View):
 class Status(View):
     """get posting"""
 
-    def get(self, request, username, status_id):
+    # pylint: disable=unused-argument
+    def get(self, request, username, status_id, slug=None):
         """display a particular status (and replies, etc)"""
         user = get_user_from_username(request.user, username)
         status = get_object_or_404(
@@ -130,6 +131,9 @@ class Status(View):
                 status.to_activity(pure=not is_bookwyrm_request(request))
             )
 
+        if redirect_local_path := maybe_redirect_local_path(request, status):
+            return redirect_local_path
+
         visible_thread = (
             models.Status.privacy_filter(request.user)
             .filter(thread_id=status.thread_id)

bookwyrm/views/group.py

@@ -14,17 +14,22 @@ from django.db.models.functions import Greatest
 
 from bookwyrm import forms, models
 from bookwyrm.suggested_users import suggested_users
-from .helpers import get_user_from_username
+from .helpers import get_user_from_username, maybe_redirect_local_path
 
 # pylint: disable=no-self-use
 class Group(View):
     """group page"""
 
-    def get(self, request, group_id):
+    # pylint: disable=unused-argument
+    def get(self, request, group_id, slug=None):
         """display a group"""
 
         group = get_object_or_404(models.Group, id=group_id)
         group.raise_visible_to_user(request.user)
+
+        if redirect_local_path := maybe_redirect_local_path(request, group):
+            return redirect_local_path
+
         lists = (
             models.List.privacy_filter(request.user)
             .filter(group=group)
@@ -80,7 +85,8 @@ class Group(View):
 class UserGroups(View):
     """a user's groups page"""
 
-    def get(self, request, username):
+    # pylint: disable=unused-argument
+    def get(self, request, username, slug=None):
         """display a group"""
         user = get_user_from_username(request.user, username)
         groups = (

bookwyrm/views/helpers.py

@@ -8,6 +8,7 @@ from dateutil.parser import ParserError
 from requests import HTTPError
 from django.db.models import Q
 from django.conf import settings as django_settings
+from django.shortcuts import redirect
 from django.http import Http404
 from django.utils import translation
 
@@ -201,3 +202,21 @@ def filter_stream_by_status_type(activities, allowed_types=None):
         )
 
     return activities
+
+
+def maybe_redirect_local_path(request, model):
+    """
+    if the request had an invalid path, return a permanent redirect response to the
+    correct one, including a slug if any.
+    if path is valid, returns False.
+    """
+
+    # don't redirect empty path for unit tests which currently have this
+    if request.path in ("/", model.local_path):
+        return False
+
+    new_path = model.local_path
+    if len(request.GET) > 0:
+        new_path = f"{model.local_path}?{request.GET.urlencode()}"
+
+    return redirect(new_path, permanent=True)

bookwyrm/views/list/list.py

@@ -18,21 +18,27 @@ from django.views.decorators.http import require_POST
 from bookwyrm import book_search, forms, models
 from bookwyrm.activitypub import ActivitypubResponse
 from bookwyrm.settings import PAGE_LENGTH
-from bookwyrm.views.helpers import is_api_request
+from bookwyrm.views.helpers import is_api_request, maybe_redirect_local_path
 
 
 # pylint: disable=no-self-use
 class List(View):
     """book list page"""
 
-    def get(self, request, list_id, add_failed=False, add_succeeded=False):
+    def get(self, request, list_id, **kwargs):
         """display a book list"""
+        add_failed = kwargs.get("add_failed", False)
+        add_succeeded = kwargs.get("add_succeeded", False)
+
         book_list = get_object_or_404(models.List, id=list_id)
         book_list.raise_visible_to_user(request.user)
 
         if is_api_request(request):
             return ActivitypubResponse(book_list.to_activity(**request.GET))
 
+        if r := maybe_redirect_local_path(request, book_list):
+            return r
+
         query = request.GET.get("q")
         suggestions = None