Update to main #7

phildini · 2023-06-13T06:57:59Z

No description provided.

edited two files

Fixes #2801 Related to #2794 It is legitimate to use any url for the user's key id. We have been assuming this id is the user id plus a fragment (#key-id) but this is not always the case, notably in the case of GoToSocial it is at /key-id. This commit instead checks the remote user's information to see if the key id listed matches the key id of the message allegedly received from them. Whilst troubleshooting this it also became apparent that there is a mismatch between Bookwyrm users' keyId and the KeyId we claim to be using in signed requests (there is a forward slash missing). Since everything after the slash is a fragment, this usually slips through but we should be consistent so I updated that.

Previously the 'tag' value in an activitypub object was assumed to be a List (array). Some AP software sends 'tag' as a Dict (object) if there is only a single tag value. It's somewhat debatable whether this is spec compliant but we should aim to be robust. This commit puts an individual mention tag inside a list if necessary.

GoToSocial expects the 'name' value of a mention tag to have an initial '@' symbol. Mastodon doesn't seem to mind either way.

Bookwyrm keyIds are at `userpath/#main-key`, however when signing AP objects we have claimed in the headers that the keyId is at `userpath#main-key`. This is incorrect, and makes GoToSocial's strict checking break. Simply updating the signatures to use the correct KeyId breaks legacy Bookwyrm's signature checks, becuase it assumes that the keyId path is the same as the user path plus a fragment. This commit allows for either option, by sending the request a second time with the incorrect keyId if sending with the correct one causes an error.

GoToSocial sends 'tag' values as a single object if there is only one user mentioned, rather than an array with an object inside it. This causes Bookwyrm to reject the tag since it comes through as a dict rather than a list. This commit fixes this at the point the incoming AP object is transformed so that "mention" tags are turned into a mention_user.

Persist book.subjects and add_author when form validation fails. This does not resolve the problem of cover image uploads being dropped because this is a broader problem and is covered separately in #2760 (we should investigate the plugin ` django-file-resubmit`)

oops

…r instead of main page

Only trigger add_status_task when status is first created

Fixes for create-book

revert last commit because prettier was run with the wrong version This reverts commit 5d3883c.

Type annotations and related changes for connectors

Add copy button for ISBN

Adds management command to repair editions in bad state

Include Dutch locale

On readthrough, progressupdate or status delete return to previous page

Isbn hyphenation

Convert description from Markdown when importing from Open Library

Record report actions

Adds breadcrumbs and better titles to followers/following pages

phildini

Part 1

phildini · 2023-08-13T04:41:32Z

bookwyrm/connectors/openlibrary.py

+    else:
+        description = markdown(description_blob)
+
+    if (


Wow I'm not a fan of this at all

Is there an alternative you'd prefer?

phildini · 2023-08-13T04:49:31Z

bookwyrm/isbn/isbn.py

+from bookwyrm import settings
+
+
+class IsbnHyphenator:


This whole thing makes me sad, but I'll deal with it later

This is one where we should likely submit our proposed change to upstream and then merge down from there, rather than just changing for our fork.

phildini · 2023-08-13T04:53:02Z

bookwyrm/management/commands/repair_editions.py

+    help = "Repairs an edition that is in a broken state"
+
+    # pylint: disable=unused-argument
+    def handle(self, *args, **options):


This is neat we should run this

I don't think we've seen any broken editions yet, but it doesn't seem like it would hurt!

phildini · 2023-08-13T04:57:50Z

bookwyrm/models/activitypub_mixin.py

@@ -529,7 +537,7 @@ async def async_broadcast(recipients: List[str], sender, data: str):


 async def sign_and_send(


Review this again soon.

phildini · 2023-08-13T04:59:15Z

bookwyrm/models/book.py

@@ -363,8 +372,34 @@ def save(self, *args, **kwargs):
            for author_id in self.authors.values_list("id", flat=True):
                cache.delete(f"author-books-{author_id}")

+        # Create sort title by removing articles from title
+        if self.sort_title in [None, ""]:


Hmm... this is kiiiinda weird? if it's well-tested probably fine

MaggieFero · 2023-08-13T05:38:10Z

I've started reviewing this, but I'm pausing for the night at 126/141 files viewed. Leaving this comment mostly so I can see if there's anything I need to look at again because of changes before I finish.

MaggieFero · 2023-08-16T04:03:27Z

I've started reviewing this, but I'm pausing for the night at 126/141 files viewed. Leaving this comment mostly so I can see if there's anything I need to look at again because of changes before I finish.

No new commits since then! Picking up my review where I left off.

MaggieFero

Overall, there are a couple things I don't love in here, but nothing we should block a merge over. There are a few security changes we've discussed OOB that I'd like us to test after merging but before deploying this, and if they work out for us we can also propose them upstream.

Overall, let's merge this!

MaggieFero · 2023-08-13T04:09:39Z

.github/workflows/prettier.yaml

@@ -17,7 +17,7 @@ jobs:
      - uses: actions/checkout@v3

      - name: Install modules
-        run: npm install prettier
+        run: npm install prettier@2.5.1


We should consider pinning a newer version; current is 3.0.1. I'm guessing this pinning was triggered by a bad version accidentally published of 2.5.7, but there are some pretty nice improvements since then, and it was fixed.

MaggieFero · 2023-08-13T04:48:51Z

bookwyrm/settings.py

@@ -312,6 +313,9 @@
    ("zh-hant", _("繁體中文 (Traditional Chinese)")),
 ]

+LANGUAGE_ARTICLES = {


We should probably commit the equivalent articles for the other languages our server uses frequently to upstream directly; I think Spanish and Dutch are the biggest ones we see, but maybe also French would be worth adding?

MaggieFero · 2023-08-13T04:59:46Z

bookwyrm/templates/book/editions/search_filter.html

@@ -4,7 +4,7 @@
 {% block filter %}
 <div class="control">
    <label class="label" for="id_search">{% trans "Search editions" %}</label>
-    <input type="text" class="input" name="q" value="{{ request.GET.q|default:'' }}" id="id_search">
+    <input type="text" class="input" name="q" value="{{ request.GET.q|default:'' }}" id="id_search" spellcheck="false">


Feels like actually spellcheck might be useful here???

MaggieFero · 2023-08-13T04:59:57Z

bookwyrm/templates/get_started/books.html

@@ -6,7 +6,7 @@
    <h2 class="title is-4">{% trans "What are you reading?" %}</h2>
    <form class="field has-addons" method="get" action="{% url 'get-started-books' %}">
        <div class="control">
-            <input type="text" name="query" value="{{ request.GET.query }}" class="input" placeholder="{% trans 'Search for a book' %}" aria-label="{% trans 'Search for a book' %}">
+            <input type="text" name="query" value="{{ request.GET.query }}" class="input" placeholder="{% trans 'Search for a book' %}" aria-label="{% trans 'Search for a book' %}" spellcheck="false">


As mentioned on previous: Feels like actually spellcheck might be useful here???

MaggieFero · 2023-08-13T05:11:31Z

bookwyrm/templates/layout.html

@@ -38,7 +38,7 @@
                        {% else %}
                            {% trans "Search for a book" as search_placeholder %}
                        {% endif %}
-                        <input aria-label="{{ search_placeholder }}" id="tour-search" class="input" type="text" name="q" placeholder="{{ search_placeholder }}" value="{{ query }}">
+                        <input aria-label="{{ search_placeholder }}" id="tour-search" class="input" type="text" name="q" placeholder="{{ search_placeholder }}" value="{{ query }}" spellcheck="false">


Another place spellcheck seems potentially nice?

MaggieFero · 2023-08-13T05:14:54Z

bookwyrm/templates/lists/list.html

@@ -210,7 +210,7 @@ <h2 class="title is-5 mt-6">
        <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 }}">
+                    <input aria-label="{% trans 'Search for a book' %}" class="input" type="text" name="q" placeholder="{% trans 'Search for a book' %}" value="{{ query }}" spellcheck="false">


Spellchecking human names is bad, but spellchecking book titles seems convenient?

MaggieFero · 2023-08-13T05:32:23Z

celerywyrm/settings.py

@@ -6,7 +6,7 @@
 QUERY_TIMEOUT = env.int("CELERY_QUERY_TIMEOUT", env.int("QUERY_TIMEOUT", 30))

 # pylint: disable=line-too-long
-REDIS_BROKER_PASSWORD = requests.utils.quote(env("REDIS_BROKER_PASSWORD", ""))
+REDIS_BROKER_PASSWORD = requests.compat.quote(env("REDIS_BROKER_PASSWORD", ""))


@phildini I'm not familiar with this, but it has the general vibe that I should maybe have feelings about it. Is this change likely to have security implications, or is requests.compat equally fine for passwords?

MaggieFero · 2023-08-13T05:38:42Z

bookwyrm/connectors/openlibrary.py

+    else:
+        description = markdown(description_blob)
+
+    if (


Is there an alternative you'd prefer?

MaggieFero · 2023-08-13T05:39:28Z

bookwyrm/isbn/isbn.py

+from bookwyrm import settings
+
+
+class IsbnHyphenator:


This is one where we should likely submit our proposed change to upstream and then merge down from there, rather than just changing for our fork.

MaggieFero · 2023-08-13T05:40:03Z

bookwyrm/management/commands/repair_editions.py

+    help = "Repairs an edition that is in a broken state"
+
+    # pylint: disable=unused-argument
+    def handle(self, *args, **options):


I don't think we've seen any broken editions yet, but it doesn't seem like it would hurt!

phildini · 2023-08-16T04:43:08Z

bookwyrm/views/admin/user_admin.py

@@ -93,6 +95,10 @@ def post(self, request, user):
            form = forms.UserGroupForm(request.POST, instance=user)
            if form.is_valid():
                form.save(request)
+
+        if report_id:


It maybe sucks you can save the formgroup without recording the action.

rritik772 and others added 30 commits January 28, 2023 12:10

fixed singularisation/pluralisation

fc599f8

edited two files

Splited stirng into two then fixed pluralization error.

e5e9e80

Merge remote-tracking branch 'origin/main' into pluralization-error

a12dc69

Fixed Syntax of pluralisation

adcf931

formatting fixes

49758f2

clean up troubleshooting comment

e112718

Include initial '@' in mention tag name

c9dcd4f

GoToSocial expects the 'name' value of a mention tag to have an initial '@' symbol. Mastodon doesn't seem to mind either way.

add comment

279fa38

update comment to identify bug

c450947

formatting

a667671

make get_legacy_key more DRY

c7adb62

pylint fixes

56a062d

fix tests and formatting

123628c

fix tests and make pylint happier

8a8af4e

oops black

9872658

Merge branch 'main' into gts

a0b7112

Fixes #2571

fb3cb22

Persist book.subjects and add_author when form validation fails. This does not resolve the problem of cover image uploads being dropped because this is a broader problem and is covered separately in #2760 (we should investigate the plugin ` django-file-resubmit`)

Merge branch 'main' into 2571

74f0832

fix ensure_transient_values_persist call

6f025af

Actually fix ensure_transient_values_persist call

5f5886e

oops

add support for title sort to ignore initial article

a94a473

updating shelf view

21d9cb5

updating list view

a3013c6

Adding test for sort_title population

6b39052

responding to review comments

575e1ba

Merge branch 'main' into 2678

1985c2d

axiomizer and others added 24 commits August 1, 2023 09:45

Fix pylint issues and failing unit tests

8565367

When deleting readthrough, progressupdate or status, return to refere…

d1bad52

…r instead of main page

Merge pull request #2934 from bookwyrm-social/reduce-status-tasks

211b60b

Only trigger add_status_task when status is first created

Merge pull request #2937 from jderuiter/create-book-fixes

173d0b7

Fixes for create-book

Make copy button screen reader friendly

3ca36fe

revert last commit because prettier was run with the wrong version This reverts commit 5d3883c.

Merge pull request #2925 from jderuiter/mypy-connectors

acafa0b

Type annotations and related changes for connectors

Fixes typo and outdated comment

455b0c8

Merge pull request #2915 from axiomizer/main

73f1484

Add copy button for ISBN

Merge pull request #2905 from bookwyrm-social/broken-editions

9d69f2f

Adds management command to repair editions in bad state

Merge branch 'main' into isbn-hyphenation

0be5cf3

Merge branch 'main' into markdown-import

f4a4b59

Sanitise description from Open Library

ae5c27f

Change import of clean

a901014

Revert change to redirect when deleting status

2dddb2e

Make hyphenated ISBN a property on the book model

83ad456

Merge pull request #2918 from jderuiter/dutch

013c726

Include Dutch locale

Adds breadcrumbs and better titles to followers/following pages

0e43cc4

Merge pull request #2938 from jderuiter/redirect-referer

b7e7867

On readthrough, progressupdate or status delete return to previous page

Merge pull request #2921 from axiomizer/isbn-hyphenation

e76b44f

Isbn hyphenation

Merge branch 'main' into report-actions

a166af9

Adds merge migration

518f87e

Merge pull request #2935 from jderuiter/markdown-import

861d3b1

Convert description from Markdown when importing from Open Library

Merge pull request #2854 from bookwyrm-social/report-actions

15e82ec

Record report actions

Merge pull request #2946 from bookwyrm-social/followers-page-breadcrumbs

53c8085

Adds breadcrumbs and better titles to followers/following pages

phildini commented Aug 13, 2023

View reviewed changes

MaggieFero approved these changes Aug 16, 2023

View reviewed changes

phildini commented Aug 16, 2023

View reviewed changes

phildini merged commit 2826e18 into phildini:main Aug 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to main #7

Update to main #7

phildini commented Jun 13, 2023

phildini left a comment

phildini Aug 13, 2023

MaggieFero Aug 13, 2023

phildini Aug 13, 2023

MaggieFero Aug 13, 2023

phildini Aug 13, 2023

MaggieFero Aug 13, 2023

phildini Aug 13, 2023

phildini Aug 13, 2023

MaggieFero commented Aug 13, 2023

MaggieFero commented Aug 16, 2023

MaggieFero left a comment

MaggieFero Aug 13, 2023

MaggieFero Aug 13, 2023

MaggieFero Aug 13, 2023

MaggieFero Aug 13, 2023

MaggieFero Aug 13, 2023

MaggieFero Aug 13, 2023

MaggieFero Aug 13, 2023

MaggieFero Aug 13, 2023

MaggieFero Aug 13, 2023

MaggieFero Aug 13, 2023

phildini Aug 16, 2023

		@@ -529,7 +537,7 @@ async def async_broadcast(recipients: List[str], sender, data: str):


		async def sign_and_send(

Update to main #7

Update to main #7

Conversation

phildini commented Jun 13, 2023

phildini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaggieFero commented Aug 13, 2023

MaggieFero commented Aug 16, 2023

MaggieFero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment