Merge pull request #1720 from bookwyrm-social/server-errors

Various fixes for server errors that popped up on bookwyrm.social

bookwyrm/importers/librarything_import.py

@@ -14,7 +14,8 @@ class LibrarythingImporter(Importer):
         """use the dataclass to create the formatted row of data"""
         remove_brackets = lambda v: re.sub(r"\[|\]", "", v) if v else None
         normalized = {k: remove_brackets(entry.get(v)) for k, v in mappings.items()}
-        isbn_13 = normalized["isbn_13"].split(", ")
+        isbn_13 = normalized.get("isbn_13")
+        isbn_13 = isbn_13.split(", ") if isbn_13 else []
         normalized["isbn_13"] = isbn_13[1] if len(isbn_13) > 0 else None
         return normalized
 

bookwyrm/models/base_model.py

@@ -84,6 +84,7 @@ class BookWyrmModel(models.Model):
         # you can see groups of which you are a member
         if (
             hasattr(self, "memberships")
+            and viewer.is_authenticated
             and self.memberships.filter(user=viewer).exists()
         ):
             return

bookwyrm/tests/data/librarything.tsv

@@ -1,4 +1,4 @@
 Book Id	Title	Sort Character	Primary Author	Primary Author Role	Secondary Author	Secondary Author Roles	Publication	Date	Review	Rating	Comment	Private Comment	Summary	Media	Physical Description	Weight	Height	Thickness	Length	Dimensions	Page Count	LCCN	Acquired	Date Started	Date Read	Barcode	BCID	Tags	Collections	Languages	Original Languages	LC Classification	ISBN	ISBNs	Subjects	Dewey Decimal	Dewey Wording	Other Call Number	Copies	Source	Entry Date	From Where	OCLC	Work id	Lending Patron	Lending Status	Lending Start	Lending End
 5498194	Marelle	1	Cortazar, Julio				Gallimard (1979), Poche	1979	chef d'oeuvre	4.5			Marelle by Julio Cortázar (1979)	Broché	590 p.; 7.24 inches	1.28 pounds	7.24 inches	1.26 inches	4.96 inches	7.24 x 4.96 x 1.26 inches	590 			[2007-04-16]	[2007-05-08]			roman, espagnol, expérimental, bohème, philosophie	Your library	French	Spanish	PQ7797 .C7145	[2070291340]	2070291340, 9782070291342	Cortâazar, Julio. Rayuela	863	Literature > Spanish And Portuguese > Spanish fiction		1	Amazon.fr	[2006-08-09]			57814				
 5015319	Le grand incendie de Londres: Récit, avec incises et bifurcations, 1985-1987 (Fiction & Cie)	1	Roubaud, Jacques				Seuil (1989), Unknown Binding	1989		5			Le grand incendie de Londres: Récit, avec incises et bifurcations, 1985-1987 (Fiction & Cie) by Jacques Roubaud (1989)	Broché	411 p.; 7.72 inches	0.88 pounds	7.72 inches	1.02 inches	5.43 inches	7.72 x 5.43 x 1.02 inches	411 								Your library	English		PQ2678 .O77	[2020104725]	2020104725, 9782020104722	Autobiographical fiction|Roubaud, Jacques > Fiction	813	American And Canadian > Fiction > Literature		1	Amazon.com	[2006-07-25]			478910				
-5015399	Le Maître et Marguerite	1	Boulgakov, Mikhaïl				Pocket (1994), Poche	1994					Le Maître et Marguerite by Mikhaïl Boulgakov (1994)	Broché	579 p.; 7.09 inches	0.66 pounds	7.09 inches	1.18 inches	4.33 inches	7.09 x 4.33 x 1.18 inches	579 								Your library	French		PG3476 .B78	[2266062328]	2266062328, 9782266062329	Allegories|Bulgakov|Good and evil > Fiction|Humanities|Jerusalem > Fiction|Jesus Christ > Fiction|Literature|Mental illness > Fiction|Moscow (Russia) > Fiction|Novel|Pilate, Pontius, 1st cent. > Fiction|Political fiction|Russia > Fiction|Russian fiction|Russian publications (Form Entry)|Soviet Union > History > 1925-1953 > Fiction|literature	891.7342	1917-1945 > 1917-1991 (USSR) > Literature > Literature of other Indo-European languages > Other Languages > Russian > Russian Fiction		1	Amazon.fr	[2006-07-25]			10151				
+5015399	Le Maître et Marguerite	1	Boulgakov, Mikhaïl				Pocket (1994), Poche	1994					Le Maître et Marguerite by Mikhaïl Boulgakov (1994)	Broché	579 p.; 7.09 inches	0.66 pounds	7.09 inches	1.18 inches	4.33 inches	7.09 x 4.33 x 1.18 inches	579 								Your library	French		PG3476 .B78	[2266062328]		Allegories|Bulgakov|Good and evil > Fiction|Humanities|Jerusalem > Fiction|Jesus Christ > Fiction|Literature|Mental illness > Fiction|Moscow (Russia) > Fiction|Novel|Pilate, Pontius, 1st cent. > Fiction|Political fiction|Russia > Fiction|Russian fiction|Russian publications (Form Entry)|Soviet Union > History > 1925-1953 > Fiction|literature	891.7342	1917-1945 > 1917-1991 (USSR) > Literature > Literature of other Indo-European languages > Other Languages > Russian > Russian Fiction		1	Amazon.fr	[2006-07-25]			10151				

bookwyrm/tests/importers/test_openlibrary_import.py

@@ -48,7 +48,9 @@ class OpenLibraryImport(TestCase):
             self.local_user, self.csv, False, "public"
         )
 
-        import_items = models.ImportItem.objects.filter(job=import_job).all()
+        import_items = (
+            models.ImportItem.objects.filter(job=import_job).order_by("index").all()
+        )
         self.assertEqual(len(import_items), 4)
         self.assertEqual(import_items[0].index, 0)
         self.assertEqual(import_items[0].data["Work Id"], "OL102749W")

bookwyrm/tests/views/shelf/test_shelf.py

@@ -60,6 +60,18 @@ class ShelfViews(TestCase):
         validate_html(result.render())
         self.assertEqual(result.status_code, 200)
 
+    def test_shelf_page_all_books_json(self, *_):
+        """there is no json view here"""
+        view = views.Shelf.as_view()
+        request = self.factory.get("")
+        request.user = self.local_user
+        with patch("bookwyrm.views.shelf.shelf.is_api_request") as is_api:
+            is_api.return_value = True
+            result = view(request, self.local_user.username)
+        self.assertIsInstance(result, TemplateResponse)
+        validate_html(result.render())
+        self.assertEqual(result.status_code, 200)
+
     def test_shelf_page_all_books_anonymous(self, *_):
         """there are so many views, this just makes sure it LOADS"""
         view = views.Shelf.as_view()

bookwyrm/tests/views/test_feed.py

@@ -36,6 +36,13 @@ class FeedViews(TestCase):
                 local=True,
                 localname="mouse",
             )
+            self.another_user = models.User.objects.create_user(
+                "nutria@local.com",
+                "nutria@nutria.nutria",
+                "password",
+                local=True,
+                localname="nutria",
+            )
         self.book = models.Edition.objects.create(
             parent_work=models.Work.objects.create(title="hi"),
             title="Example Edition",
@@ -171,6 +178,17 @@ class FeedViews(TestCase):
         result.render()
         self.assertEqual(result.status_code, 200)
 
+    def test_direct_messages_page_user(self, *_):
+        """there are so many views, this just makes sure it LOADS"""
+        view = views.DirectMessage.as_view()
+        request = self.factory.get("")
+        request.user = self.local_user
+        result = view(request, "nutria")
+        self.assertIsInstance(result, TemplateResponse)
+        result.render()
+        self.assertEqual(result.status_code, 200)
+        self.assertEqual(result.context_data["partner"], self.another_user)
+
     @patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async")
     @patch("bookwyrm.activitystreams.add_book_statuses_task.delay")
     def test_get_suggested_book(self, *_):

bookwyrm/tests/views/test_group.py

@@ -1,6 +1,8 @@
 """ test for app action functionality """
 from unittest.mock import patch
 
+from django.contrib.auth.models import AnonymousUser
+from django.http import Http404
 from django.template.response import TemplateResponse
 from django.test import TestCase
 from django.test.client import RequestFactory
@@ -43,6 +45,8 @@ class GroupViews(TestCase):
         self.membership = models.GroupMember.objects.create(
             group=self.testgroup, user=self.local_user
         )
+        self.anonymous_user = AnonymousUser
+        self.anonymous_user.is_authenticated = False
 
         models.SiteSettings.objects.create()
 
@@ -56,6 +60,17 @@ class GroupViews(TestCase):
         validate_html(result.render())
         self.assertEqual(result.status_code, 200)
 
+    def test_group_get_anonymous(self, _):
+        """there are so many views, this just makes sure it LOADS"""
+        self.testgroup.privacy = "followers"
+        self.testgroup.save()
+
+        view = views.Group.as_view()
+        request = self.factory.get("")
+        request.user = self.anonymous_user
+        with self.assertRaises(Http404):
+            view(request, group_id=self.testgroup.id)
+
     def test_usergroups_get(self, _):
         """there are so many views, this just makes sure it LOADS"""
         view = views.UserGroups.as_view()

bookwyrm/tests/views/test_user.py

@@ -80,6 +80,29 @@ class UserViews(TestCase):
         self.assertIsInstance(result, ActivitypubResponse)
         self.assertEqual(result.status_code, 200)
 
+    def test_user_page_domain(self):
+        """when the user domain has dashes in it"""
+        with patch("bookwyrm.models.user.set_remote_server"):
+            self.remote_user = models.User.objects.create_user(
+                "nutria",
+                "",
+                "nutriaword",
+                local=False,
+                remote_id="https://ex--ample.co----m/users/nutria",
+                inbox="https://ex--ample.co----m/users/nutria/inbox",
+                outbox="https://ex--ample.co----m/users/nutria/outbox",
+            )
+
+        view = views.User.as_view()
+        request = self.factory.get("")
+        request.user = self.local_user
+        with patch("bookwyrm.views.user.is_api_request") as is_api:
+            is_api.return_value = False
+            result = view(request, "nutria@ex--ample.co----m")
+        self.assertIsInstance(result, TemplateResponse)
+        validate_html(result.render())
+        self.assertEqual(result.status_code, 200)
+
     def test_user_page_blocked(self):
         """there are so many views, this just makes sure it LOADS"""
         view = views.User.as_view()

bookwyrm/urls.py

@@ -50,7 +50,7 @@ urlpatterns = [
     re_path("^api/updates/stream/(?P<stream>[a-z]+)/?$", views.get_unread_status_count),
     # authentication
     re_path(r"^login/?$", views.Login.as_view(), name="login"),
-    re_path(r"^login/(?P<confirmed>confirmed)?$", views.Login.as_view(), name="login"),
+    re_path(r"^login/(?P<confirmed>confirmed)/?$", views.Login.as_view(), name="login"),
     re_path(r"^register/?$", views.Register.as_view()),
     re_path(r"confirm-email/?$", views.ConfirmEmail.as_view(), name="confirm-email"),
     re_path(
@@ -112,12 +112,12 @@ urlpatterns = [
         name="settings-federated-server",
     ),
     re_path(
-        r"^settings/federation/(?P<server>\d+)/block?$",
+        r"^settings/federation/(?P<server>\d+)/block/?$",
         views.block_server,
         name="settings-federated-server-block",
     ),
     re_path(
-        r"^settings/federation/(?P<server>\d+)/unblock?$",
+        r"^settings/federation/(?P<server>\d+)/unblock/?$",
         views.unblock_server,
         name="settings-federated-server-unblock",
     ),
@@ -140,7 +140,7 @@ urlpatterns = [
         name="settings-invite-requests",
     ),
     re_path(
-        r"^settings/requests/ignore?$",
+        r"^settings/requests/ignore/?$",
         views.ignore_invite_request,
         name="settings-invite-requests-ignore",
     ),
@@ -229,7 +229,7 @@ urlpatterns = [
         r"^direct-messages/?$", views.DirectMessage.as_view(), name="direct-messages"
     ),
     re_path(
-        rf"^direct-messages/(?P<username>{regex.USERNAME})?$",
+        rf"^direct-messages/(?P<username>{regex.USERNAME})/?$",
         views.DirectMessage.as_view(),
         name="direct-messages-user",
     ),
@@ -339,7 +339,7 @@ urlpatterns = [
     re_path(r"^save-list/(?P<list_id>\d+)/?$", views.save_list, name="list-save"),
     re_path(r"^unsave-list/(?P<list_id>\d+)/?$", views.unsave_list, name="list-unsave"),
     re_path(
-        r"^list/(?P<list_id>\d+)/embed/(?P<list_key>[0-9a-f]+)?$",
+        r"^list/(?P<list_id>\d+)/embed/(?P<list_key>[0-9a-f]+)/?$",
         views.unsafe_embed_list,
         name="embed-list",
     ),
@@ -356,7 +356,7 @@ urlpatterns = [
         name="shelf",
     ),
     re_path(r"^create-shelf/?$", views.create_shelf, name="shelf-create"),
-    re_path(r"^delete-shelf/(?P<shelf_id>\d+)?$", views.delete_shelf),
+    re_path(r"^delete-shelf/(?P<shelf_id>\d+)/?$", views.delete_shelf),
     re_path(r"^shelve/?$", views.shelve),
     re_path(r"^unshelve/?$", views.unshelve),
     # goals
@@ -423,7 +423,7 @@ urlpatterns = [
     re_path(rf"{BOOK_PATH}/edit/?$", views.EditBook.as_view(), name="edit-book"),
     re_path(rf"{BOOK_PATH}/confirm/?$", views.ConfirmEditBook.as_view()),
     re_path(r"^create-book/?$", views.EditBook.as_view(), name="create-book"),
-    re_path(r"^create-book/confirm?$", views.ConfirmEditBook.as_view()),
+    re_path(r"^create-book/confirm/?$", views.ConfirmEditBook.as_view()),
     re_path(rf"{BOOK_PATH}/editions(.json)?/?$", views.Editions.as_view()),
     re_path(
         r"^upload-cover/(?P<book_id>\d+)/?$", views.upload_cover, name="upload-cover"

bookwyrm/utils/regex.py

@@ -1,6 +1,6 @@
 """ defining regexes for regularly used concepts """
 
-DOMAIN = r"[\w_\-\.]+\.[a-z]{2,}"
+DOMAIN = r"[\w_\-\.]+\.[a-z\-]{2,}"
 LOCALNAME = r"@?[a-zA-Z_\-\.0-9]+"
 STRICT_LOCALNAME = r"@[a-zA-Z_\-\.0-9]+"
 USERNAME = rf"{LOCALNAME}(@{DOMAIN})?"

bookwyrm/views/landing/login.py

@@ -39,7 +39,8 @@ class Login(View):
             return redirect("/")
         login_form = forms.LoginForm(request.POST)
 
-        localname = login_form.data["localname"]
+        localname = login_form.data.get("localname")
+
         if "@" in localname:  # looks like an email address to me
             try:
                 username = models.User.objects.get(email=localname).username
@@ -47,7 +48,7 @@ class Login(View):
                 username = localname
         else:
             username = f"{localname}@{DOMAIN}"
-        password = login_form.data["password"]
+        password = login_form.data.get("password")
 
         # perform authentication
         user = authenticate(request, username=username, password=password)

bookwyrm/views/shelf/shelf.py

@@ -52,7 +52,7 @@ class Shelf(View):
             )
             shelf = FakeShelf("all", _("All books"), user, books, "public")
 
-        if is_api_request(request):
+        if is_api_request(request) and shelf_identifier:
             return ActivitypubResponse(shelf.to_activity(**request.GET))
 
         reviews = models.Review.objects