Merge remote-tracking branch 'bookwyrm/user-migration' into user-migration

bookwyrm/isbn/isbn.py

@@ -40,7 +40,12 @@ class IsbnHyphenator:
             self.__element_tree = ElementTree.parse(self.__range_file_path)
 
         gs1_prefix = isbn_13[:3]
-        reg_group = self.__find_reg_group(isbn_13, gs1_prefix)
+        try:
+            reg_group = self.__find_reg_group(isbn_13, gs1_prefix)
+        except ValueError:
+            # if the reg groups are invalid, just return the original isbn
+            return isbn_13
+
         if reg_group is None:
             return isbn_13  # failed to hyphenate
 

bookwyrm/migrations/0183_auto_20231105_1607.py

@@ -0,0 +1,18 @@
+# Generated by Django 3.2.20 on 2023-11-05 16:07
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("bookwyrm", "0182_auto_20231027_1122"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="user",
+            name="is_deleted",
+            field=models.BooleanField(default=False),
+        ),
+    ]

bookwyrm/migrations/0184_auto_20231106_0421.py

@@ -0,0 +1,49 @@
+# Generated by Django 3.2.20 on 2023-11-06 04:21
+
+from django.db import migrations
+from bookwyrm.models import User
+
+
+def update_deleted_users(apps, schema_editor):
+    """Find all the users who are deleted, not just inactive, and set deleted"""
+    users = apps.get_model("bookwyrm", "User")
+    db_alias = schema_editor.connection.alias
+    users.objects.using(db_alias).filter(
+        is_active=False,
+        deactivation_reason__in=[
+            "self_deletion",
+            "moderator_deletion",
+        ],
+    ).update(is_deleted=True)
+
+    # differente rules for remote users
+    users.objects.using(db_alias).filter(is_active=False, local=False,).exclude(
+        deactivation_reason="moderator_deactivation",
+    ).update(is_deleted=True)
+
+
+def erase_deleted_user_data(apps, schema_editor):
+    """Retroactively clear user data"""
+    for user in User.objects.filter(is_deleted=True):
+        user.erase_user_data()
+        user.save(
+            broadcast=False,
+            update_fields=["email", "avatar", "preview_image", "summary", "name"],
+        )
+        user.erase_user_statuses(broadcast=False)
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("bookwyrm", "0183_auto_20231105_1607"),
+    ]
+
+    operations = [
+        migrations.RunPython(
+            update_deleted_users, reverse_code=migrations.RunPython.noop
+        ),
+        migrations.RunPython(
+            erase_deleted_user_data, reverse_code=migrations.RunPython.noop
+        ),
+    ]

bookwyrm/models/__init__.py

@@ -36,7 +36,7 @@ from .site import PasswordReset, InviteRequest
 from .announcement import Announcement
 from .antispam import EmailBlocklist, IPBlocklist, AutoMod, automod_task
 
-from .notification import Notification
+from .notification import Notification, NotificationType
 
 from .hashtag import Hashtag
 

bookwyrm/models/antispam.py

@@ -10,6 +10,7 @@ from django.utils.translation import gettext_lazy as _
 
 from bookwyrm.tasks import app, MISC
 from .base_model import BookWyrmModel
+from .notification import NotificationType
 from .user import User
 
 
@@ -80,7 +81,7 @@ def automod_task():
     with transaction.atomic():
         for admin in admins:
             notification, _ = notification_model.objects.get_or_create(
-                user=admin, notification_type=notification_model.REPORT, read=False
+                user=admin, notification_type=NotificationType.REPORT, read=False
             )
             notification.related_reports.set(reports)
 

bookwyrm/models/book.py

@@ -366,9 +366,9 @@ class Edition(Book):
 
         # normalize isbn format
         if self.isbn_10:
-            self.isbn_10 = re.sub(r"[^0-9X]", "", self.isbn_10)
+            self.isbn_10 = normalize_isbn(self.isbn_10)
         if self.isbn_13:
-            self.isbn_13 = re.sub(r"[^0-9X]", "", self.isbn_13)
+            self.isbn_13 = normalize_isbn(self.isbn_13)
 
         # set rank
         self.edition_rank = self.get_rank()
@@ -463,6 +463,11 @@ def isbn_13_to_10(isbn_13):
     return converted + str(checkdigit)
 
 
+def normalize_isbn(isbn):
+    """Remove unexpected characters from ISBN 10 or 13"""
+    return re.sub(r"[^0-9X]", "", isbn)
+
+
 # pylint: disable=unused-argument
 @receiver(models.signals.post_save, sender=Edition)
 def preview_image(instance, *args, **kwargs):

bookwyrm/models/group.py

@@ -1,5 +1,4 @@
 """ do book related things with other users """
-from django.apps import apps
 from django.db import models, IntegrityError, transaction
 from django.db.models import Q
 from bookwyrm.settings import DOMAIN
@@ -143,26 +142,28 @@ class GroupMemberInvitation(models.Model):
     @transaction.atomic
     def accept(self):
         """turn this request into the real deal"""
+        # pylint: disable-next=import-outside-toplevel
+        from .notification import Notification, NotificationType  # circular dependency
+
         GroupMember.from_request(self)
 
-        model = apps.get_model("bookwyrm.Notification", require_ready=True)
         # tell the group owner
-        model.notify(
+        Notification.notify(
             self.group.user,
             self.user,
             related_group=self.group,
-            notification_type=model.ACCEPT,
+            notification_type=NotificationType.ACCEPT,
         )
 
         # let the other members know about it
         for membership in self.group.memberships.all():
             member = membership.user
             if member not in (self.user, self.group.user):
-                model.notify(
+                Notification.notify(
                     member,
                     self.user,
                     related_group=self.group,
-                    notification_type=model.JOIN,
+                    notification_type=NotificationType.JOIN,
                 )
 
     def reject(self):

bookwyrm/models/move.py

@@ -6,7 +6,7 @@ from bookwyrm import activitypub
 from .activitypub_mixin import ActivityMixin
 from .base_model import BookWyrmModel
 from . import fields
-from .notification import Notification
+from .notification import Notification, NotificationType
 
 
 class Move(ActivityMixin, BookWyrmModel):
@@ -49,7 +49,6 @@ class MoveUser(Move):
 
         # only allow if the source is listed in the target's alsoKnownAs
         if self.user in self.target.also_known_as.all():
-
             self.user.also_known_as.add(self.target.id)
             self.user.update_active_date()
             self.user.moved_to = self.target.remote_id
@@ -65,7 +64,7 @@ class MoveUser(Move):
             for follower in self.user.followers.all():
                 if follower.local:
                     Notification.notify(
-                        follower, self.user, notification_type=Notification.MOVE
+                        follower, self.user, notification_type=NotificationType.MOVE
                     )
 
         else:

bookwyrm/models/notification.py

@@ -14,7 +14,7 @@ from . import (
 from . import ListItem, Report, Status, User, UserFollowRequest
 
 
-class Notification(BookWyrmModel):
+class NotificationType(models.TextChoices):
     """you've been tagged, liked, followed, etc"""
 
     # Status interactions
@@ -53,12 +53,10 @@ class Notification(BookWyrmModel):
     # Migrations
     MOVE = "MOVE"
 
-    # pylint: disable=line-too-long
-    NotificationType = models.TextChoices(
-        # there has got be a better way to do this
-        "NotificationType",
-        f"{FAVORITE} {REPLY} {MENTION} {TAG} {FOLLOW} {FOLLOW_REQUEST} {BOOST} {IMPORT} {USER_IMPORT} {USER_EXPORT} {ADD} {REPORT} {LINK_DOMAIN} {INVITE} {ACCEPT} {JOIN} {LEAVE} {REMOVE} {GROUP_PRIVACY} {GROUP_NAME} {GROUP_DESCRIPTION} {MOVE}",
-    )
+
+class Notification(BookWyrmModel):
+    """a notification object"""
+
 
     user = models.ForeignKey("User", on_delete=models.CASCADE)
     read = models.BooleanField(default=False)
@@ -106,11 +104,11 @@ class Notification(BookWyrmModel):
             user=user,
             related_users=related_user,
             related_list_items__book_list=list_item.book_list,
-            notification_type=Notification.ADD,
+            notification_type=NotificationType.ADD,
         ).first()
         if not notification:
             notification = cls.objects.create(
-                user=user, notification_type=Notification.ADD
+                user=user, notification_type=NotificationType.ADD
             )
             notification.related_users.add(related_user)
         notification.related_list_items.add(list_item)
@@ -137,7 +135,7 @@ def notify_on_fav(sender, instance, *args, **kwargs):
         instance.status.user,
         instance.user,
         related_status=instance.status,
-        notification_type=Notification.FAVORITE,
+        notification_type=NotificationType.FAVORITE,
     )
 
 
@@ -151,7 +149,7 @@ def notify_on_unfav(sender, instance, *args, **kwargs):
         instance.status.user,
         instance.user,
         related_status=instance.status,
-        notification_type=Notification.FAVORITE,
+        notification_type=NotificationType.FAVORITE,
     )
 
 
@@ -176,7 +174,7 @@ def notify_user_on_mention(sender, instance, *args, **kwargs):
             instance.reply_parent.user,
             instance.user,
             related_status=instance,
-            notification_type=Notification.REPLY,
+            notification_type=NotificationType.REPLY,
         )
 
     for mention_user in instance.mention_users.all():
@@ -188,7 +186,7 @@ def notify_user_on_mention(sender, instance, *args, **kwargs):
         Notification.notify(
             mention_user,
             instance.user,
-            notification_type=Notification.MENTION,
+            notification_type=NotificationType.MENTION,
             related_status=instance,
         )
 
@@ -207,7 +205,7 @@ def notify_user_on_boost(sender, instance, *args, **kwargs):
         instance.boosted_status.user,
         instance.user,
         related_status=instance.boosted_status,
-        notification_type=Notification.BOOST,
+        notification_type=NotificationType.BOOST,
     )
 
 
@@ -219,7 +217,7 @@ def notify_user_on_unboost(sender, instance, *args, **kwargs):
         instance.boosted_status.user,
         instance.user,
         related_status=instance.boosted_status,
-        notification_type=Notification.BOOST,
+        notification_type=NotificationType.BOOST,
     )
 
 
@@ -234,7 +232,7 @@ def notify_user_on_import_complete(
         return
     Notification.objects.get_or_create(
         user=instance.user,
-        notification_type=Notification.IMPORT,
+        notification_type=NotificationType.IMPORT,
         related_import=instance,
     )
 
@@ -283,7 +281,7 @@ def notify_admins_on_report(sender, instance, created, *args, **kwargs):
     for admin in admins:
         notification, _ = Notification.objects.get_or_create(
             user=admin,
-            notification_type=Notification.REPORT,
+            notification_type=NotificationType.REPORT,
             read=False,
         )
         notification.related_reports.add(instance)
@@ -303,7 +301,7 @@ def notify_admins_on_link_domain(sender, instance, created, *args, **kwargs):
     for admin in admins:
         notification, _ = Notification.objects.get_or_create(
             user=admin,
-            notification_type=Notification.LINK_DOMAIN,
+            notification_type=NotificationType.LINK_DOMAIN,
             read=False,
         )
         notification.related_link_domains.add(instance)
@@ -317,7 +315,7 @@ def notify_user_on_group_invite(sender, instance, *args, **kwargs):
         instance.user,
         instance.group.user,
         related_group=instance.group,
-        notification_type=Notification.INVITE,
+        notification_type=NotificationType.INVITE,
     )
 
 
@@ -355,11 +353,12 @@ def notify_user_on_follow(sender, instance, created, *args, **kwargs):
         notification = Notification.objects.filter(
             user=instance.user_object,
             related_users=instance.user_subject,
-            notification_type=Notification.FOLLOW_REQUEST,
+            notification_type=NotificationType.FOLLOW_REQUEST,
         ).first()
         if not notification:
             notification = Notification.objects.create(
-                user=instance.user_object, notification_type=Notification.FOLLOW_REQUEST
+                user=instance.user_object,
+                notification_type=NotificationType.FOLLOW_REQUEST,
             )
         notification.related_users.set([instance.user_subject])
         notification.read = False
@@ -369,6 +368,6 @@ def notify_user_on_follow(sender, instance, created, *args, **kwargs):
         Notification.notify(
             instance.user_object,
             instance.user_subject,
-            notification_type=Notification.FOLLOW,
+            notification_type=NotificationType.FOLLOW,
             read=False,
         )

bookwyrm/models/status.py

@@ -102,7 +102,7 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel):
         if hasattr(self, "quotation"):
             self.quotation = None  # pylint: disable=attribute-defined-outside-init
         self.deleted_date = timezone.now()
-        self.save()
+        self.save(*args, **kwargs)
 
     @property
     def recipients(self):

bookwyrm/models/user.py

@@ -1,13 +1,14 @@
 """ database schema for user data """
 import re
 from urllib.parse import urlparse
+from uuid import uuid4
 
 from django.apps import apps
 from django.contrib.auth.models import AbstractUser
 from django.contrib.postgres.fields import ArrayField, CICharField
 from django.core.exceptions import PermissionDenied, ObjectDoesNotExist
 from django.dispatch import receiver
-from django.db import models, transaction
+from django.db import models, transaction, IntegrityError
 from django.utils import timezone
 from django.utils.translation import gettext_lazy as _
 from model_utils import FieldTracker
@@ -53,6 +54,7 @@ class User(OrderedCollectionPageMixin, AbstractUser):
 
     username = fields.UsernameField()
     email = models.EmailField(unique=True, null=True)
+    is_deleted = models.BooleanField(default=False)
 
     key_pair = fields.OneToOneField(
         "KeyPair",
@@ -394,9 +396,44 @@ class User(OrderedCollectionPageMixin, AbstractUser):
         """We don't actually delete the database entry"""
         # pylint: disable=attribute-defined-outside-init
         self.is_active = False
-        self.avatar = ""
+        self.allow_reactivation = False
+        self.is_deleted = True
+
+        self.erase_user_data()
+        self.erase_user_statuses()
+
         # skip the logic in this class's save()
-        super().save(*args, **kwargs)
+        super().save(
+            *args,
+            **kwargs,
+        )
+
+    def erase_user_data(self):
+        """Wipe a user's custom data"""
+        if not self.is_deleted:
+            raise IntegrityError(
+                "Trying to erase user data on user that is not deleted"
+            )
+
+        # mangle email address
+        self.email = f"{uuid4()}@deleted.user"
+
+        # erase data fields
+        self.avatar = ""
+        self.preview_image = ""
+        self.summary = None
+        self.name = None
+        self.favorites.set([])
+
+    def erase_user_statuses(self, broadcast=True):
+        """Wipe the data on all the user's statuses"""
+        if not self.is_deleted:
+            raise IntegrityError(
+                "Trying to erase user data on user that is not deleted"
+            )
+
+        for status in self.status_set.all():
+            status.delete(broadcast=broadcast)
 
     def deactivate(self):
         """Disable the user but allow them to reactivate"""

bookwyrm/templates/guided_tour/home.html

@@ -99,7 +99,7 @@ homeTour.addSteps([
         ],
     },
     {
-        text: "{% trans 'Use the <strong>Feed</strong>, <strong>Lists</strong> and <strong>Discover</strong> links to discover the latest news from your feed, lists of books by topic, and the latest happenings on this Bookwyrm server!' %}",
+        text: "{% trans 'Use the <strong>Lists</strong>, <strong>Discover</strong>, and <strong>Your Books</strong> links to discover reading suggestions and the latest happenings on this server, or to see your catalogued books!' %}",
         title: "{% trans 'Navigation Bar' %}",
         attachTo: {
             element: checkResponsiveState('#tour-navbar-start'),
@@ -197,7 +197,7 @@ homeTour.addSteps([
         ],
     },
     {
-        text: `{% trans "Your profile, books, direct messages, and settings can be accessed by clicking on your name in the menu here." %} <p class="notification is-warning is-light mt-3">{% trans "Try selecting <strong>Profile</strong> from the drop down menu to continue the tour." %}</p>`,
+        text: `{% trans "Your profile, user directory, direct messages, and settings can be accessed by clicking on your name in the menu here." %} <p class="notification is-warning is-light mt-3">{% trans "Try selecting <strong>Profile</strong> from the drop down menu to continue the tour." %}</p>`,
         title: "{% trans 'Profile and settings menu' %}",
         attachTo: {
             element: checkResponsiveState('#navbar-dropdown'),

bookwyrm/templates/notifications/item.html

@@ -10,7 +10,9 @@
 {% elif notification.notification_type == 'FOLLOW' %}
     {% include 'notifications/items/follow.html' %}
 {% elif notification.notification_type == 'FOLLOW_REQUEST' %}
-    {% include 'notifications/items/follow_request.html' %}
+    {% if notification.related_users.0.is_active %}
+        {% include 'notifications/items/follow_request.html' %}
+    {% endif %}
 {% elif notification.notification_type == 'IMPORT' %}
     {% include 'notifications/items/import.html' %}
 {% elif notification.notification_type == 'USER_IMPORT' %}

bookwyrm/templates/settings/users/user_admin.html

@@ -74,31 +74,7 @@
             <td>{{ user.created_date }}</td>
             <td>{{ user.last_active_date }}</td>
             <td>
-                {% if user.is_active %}
-                    {% if user.moved_to %}
-                        <span class="tag is-info" aria-hidden="true">
-                            <span class="icon icon-x"></span>
-                        </span>
-                        {% trans "Moved" %}
-                    {% else %}
-                        <span class="tag is-success" aria-hidden="true">
-                            <span class="icon icon-check"></span>
-                        </span>
-                        {% trans "Active" %}
-                    {% endif %}
-                {% elif user.deactivation_reason == "moderator_deletion" or user.deactivation_reason == "self_deletion" %}
-                    <span class="tag is-danger" aria-hidden="true">
-                        <span class="icon icon-x"></span>
-                    </span>
-                    {% trans "Deleted" %}
-                    <span class="help">({{ user.get_deactivation_reason_display }})</span>
-                {% else %}
-                    <span class="tag is-warning" aria-hidden="true">
-                        <span class="icon icon-x"></span>
-                    </span>
-                    {% trans "Inactive" %}
-                    <span class="help">({{ user.get_deactivation_reason_display }})</span>
-                {% endif %}
+                {% include "snippets/user_active_tag.html" with user=user %}
             </td>
             {% if status == "federated" %}
             <td>

bookwyrm/templates/settings/users/user_info.html

@@ -23,24 +23,7 @@
     <div class="column is-flex is-flex-direction-column is-4">
         <h4 class="title is-4">{% trans "Status" %}</h4>
         <div class="box is-flex-grow-1 has-text-weight-bold">
-            {% if user.is_active %}
-                {% if user.moved_to %}
-                <p class="notification is-info">
-                    {% trans "Moved" %}
-                </p>
-                {% else %}
-                <p class="notification is-success">
-                    {% trans "Active" %}
-                </p>
-                {% endif %}
-            {% else %}
-            <p class="notification is-warning">
-                {% trans "Inactive" %}
-                {% if user.deactivation_reason %}
-                <span class="help">({% trans user.get_deactivation_reason_display %})</span>
-                {% endif %}
-            </p>
-            {% endif %}
+            {% include "snippets/user_active_tag.html" with large=True %}
             <p class="notification">
                 {% if user.local %}
                 {% trans "Local" %}

bookwyrm/templates/snippets/user_active_tag.html

@@ -0,0 +1,17 @@
+{% load i18n %}
+
+{% if user.is_active %}
+    {% if user.moved_to %}
+        {% trans "Moved" as text %}
+        {% include "snippets/user_active_tag_item.html" with icon="x" text=text level="info" %}
+    {% else %}
+        {% trans "Active" as text %}
+        {% include "snippets/user_active_tag_item.html" with icon="check" text=text level="success" %}
+    {% endif %}
+{% elif user.is_deleted %}
+    {% trans "Deleted" as text %}
+    {% include "snippets/user_active_tag_item.html" with icon="x" text=text level="danger" deactivation_reason=user.get_deactivation_reason_display %}
+{% else %}
+    {% trans "Inactive" as text %}
+    {% include "snippets/user_active_tag_item.html" with icon="x" text=text level="warning" deactivation_reason=user.get_deactivation_reason_display %}
+{% endif %}

bookwyrm/templates/snippets/user_active_tag_item.html

@@ -0,0 +1,19 @@
+{% if large %}
+
+<p class="notification is-{{ level }}">
+<span class="icon icon-{{ icon }}" aria-hidden="true"></span>
+    {{ text }}
+    {% if deactivation_reason %}
+    <span class="help">({{ deactivation_reason }})</span>
+    {% endif %}
+</p>
+
+{% else %}
+
+<span class="tag is-{{ level }}" aria-hidden="true">
+    <span class="icon icon-{{ icon }}"></span>
+</span>
+{{ text }}
+
+{% endif %}
+

bookwyrm/tests/migrations/test_0184.py

@@ -0,0 +1,121 @@
+""" testing migrations """
+from unittest.mock import patch
+
+from django.test import TestCase
+from django.db.migrations.executor import MigrationExecutor
+from django.db import connection
+
+from bookwyrm import models
+from bookwyrm.management.commands import initdb
+from bookwyrm.settings import DOMAIN
+
+# pylint: disable=missing-class-docstring
+# pylint: disable=missing-function-docstring
+class EraseDeletedUserDataMigration(TestCase):
+
+    migrate_from = "0183_auto_20231105_1607"
+    migrate_to = "0184_auto_20231106_0421"
+
+    # pylint: disable=invalid-name
+    def setUp(self):
+        with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch(
+            "bookwyrm.activitystreams.populate_stream_task.delay"
+        ), patch("bookwyrm.lists_stream.populate_lists_task.delay"):
+            self.active_user = models.User.objects.create_user(
+                f"activeuser@{DOMAIN}",
+                "activeuser@activeuser.activeuser",
+                "activeuserword",
+                local=True,
+                localname="active",
+                name="a name",
+            )
+            self.inactive_user = models.User.objects.create_user(
+                f"inactiveuser@{DOMAIN}",
+                "inactiveuser@inactiveuser.inactiveuser",
+                "inactiveuserword",
+                local=True,
+                localname="inactive",
+                is_active=False,
+                deactivation_reason="self_deactivation",
+                name="name name",
+            )
+            self.deleted_user = models.User.objects.create_user(
+                f"deleteduser@{DOMAIN}",
+                "deleteduser@deleteduser.deleteduser",
+                "deleteduserword",
+                local=True,
+                localname="deleted",
+                is_active=False,
+                deactivation_reason="self_deletion",
+                name="cool name",
+            )
+        with patch(
+            "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"
+        ), patch("bookwyrm.activitystreams.add_status_task.delay"):
+            self.active_status = models.Status.objects.create(
+                user=self.active_user, content="don't delete me"
+            )
+            self.inactive_status = models.Status.objects.create(
+                user=self.inactive_user, content="also don't delete me"
+            )
+            self.deleted_status = models.Status.objects.create(
+                user=self.deleted_user, content="yes, delete me"
+            )
+
+        initdb.init_groups()
+        initdb.init_permissions()
+
+        self.migrate_from = [("bookwyrm", self.migrate_from)]
+        self.migrate_to = [("bookwyrm", self.migrate_to)]
+        executor = MigrationExecutor(connection)
+        old_apps = executor.loader.project_state(self.migrate_from).apps
+
+        # Reverse to the original migration
+        executor.migrate(self.migrate_from)
+
+        self.setUpBeforeMigration(old_apps)
+
+        # Run the migration to test
+        executor = MigrationExecutor(connection)
+        executor.loader.build_graph()  # reload.
+        with patch("bookwyrm.activitystreams.remove_status_task.delay"):
+            executor.migrate(self.migrate_to)
+
+        self.apps = executor.loader.project_state(self.migrate_to).apps
+
+    def setUpBeforeMigration(self, apps):
+        pass
+
+    def test_user_data_deleted(self):
+        """Make sure that only the right data was deleted"""
+        self.active_user.refresh_from_db()
+        self.inactive_user.refresh_from_db()
+        self.deleted_user.refresh_from_db()
+        self.active_status.refresh_from_db()
+        self.inactive_status.refresh_from_db()
+        self.deleted_status.refresh_from_db()
+
+        self.assertTrue(self.active_user.is_active)
+        self.assertFalse(self.active_user.is_deleted)
+        self.assertEqual(self.active_user.name, "a name")
+        self.assertNotEqual(self.deleted_user.email, "activeuser@activeuser.activeuser")
+        self.assertFalse(self.active_status.deleted)
+        self.assertEqual(self.active_status.content, "don't delete me")
+
+        self.assertFalse(self.inactive_user.is_active)
+        self.assertFalse(self.inactive_user.is_deleted)
+        self.assertEqual(self.inactive_user.name, "name name")
+        self.assertNotEqual(
+            self.deleted_user.email, "inactiveuser@inactiveuser.inactiveuser"
+        )
+        self.assertFalse(self.inactive_status.deleted)
+        self.assertEqual(self.inactive_status.content, "also don't delete me")
+
+        self.assertFalse(self.deleted_user.is_active)
+        self.assertTrue(self.deleted_user.is_deleted)
+        self.assertIsNone(self.deleted_user.name)
+        self.assertNotEqual(
+            self.deleted_user.email, "deleteduser@deleteduser.deleteduser"
+        )
+        self.assertTrue(self.deleted_status.deleted)
+        self.assertIsNone(self.deleted_status.content)

bookwyrm/tests/models/test_activitypub_mixin.py

@@ -119,6 +119,25 @@ class ActivitypubMixins(TestCase):
         result = models.Edition.find_existing({"openlibraryKey": "OL1234"})
         self.assertEqual(result, book)
 
+    def test_find_existing_with_id(self, *_):
+        """make sure that an "id" field won't produce a match"""
+        book = models.Edition.objects.create(title="Test edition")
+
+        result = models.Edition.find_existing({"id": book.id})
+        self.assertIsNone(result)
+
+    def test_find_existing_with_id_and_match(self, *_):
+        """make sure that an "id" field won't produce a match"""
+        book = models.Edition.objects.create(title="Test edition")
+        matching_book = models.Edition.objects.create(
+            title="Another test edition", openlibrary_key="OL1234"
+        )
+
+        result = models.Edition.find_existing(
+            {"id": book.id, "openlibraryKey": "OL1234"}
+        )
+        self.assertEqual(result, matching_book)
+
     def test_get_recipients_public_object(self, *_):
         """determines the recipients for an object's broadcast"""
         MockSelf = namedtuple("Self", ("privacy"))

bookwyrm/tests/models/test_book_model.py

@@ -11,7 +11,7 @@ from django.test import TestCase
 from django.utils import timezone
 
 from bookwyrm import models, settings
-from bookwyrm.models.book import isbn_10_to_13, isbn_13_to_10
+from bookwyrm.models.book import isbn_10_to_13, isbn_13_to_10, normalize_isbn
 from bookwyrm.settings import ENABLE_THUMBNAIL_GENERATION
 
 
@@ -72,6 +72,10 @@ class Book(TestCase):
         isbn_10 = isbn_13_to_10(isbn_13)
         self.assertEqual(isbn_10, "178816167X")
 
+    def test_normalize_isbn(self):
+        """Remove misc characters from ISBNs"""
+        self.assertEqual(normalize_isbn("978-0-4633461-1-2"), "9780463346112")
+
     def test_get_edition_info(self):
         """text slug about an edition"""
         book = models.Edition.objects.create(title="Test Edition")

bookwyrm/tests/models/test_notification.py

@@ -43,7 +43,7 @@ class Notification(TestCase):
     def test_notification(self):
         """New notifications are unread"""
         notification = models.Notification.objects.create(
-            user=self.local_user, notification_type=models.Notification.FAVORITE
+            user=self.local_user, notification_type=models.NotificationType.FAVORITE
         )
         self.assertFalse(notification.read)
 
@@ -52,7 +52,7 @@ class Notification(TestCase):
         models.Notification.notify(
             self.local_user,
             self.remote_user,
-            notification_type=models.Notification.FAVORITE,
+            notification_type=models.NotificationType.FAVORITE,
         )
         self.assertTrue(models.Notification.objects.exists())
 
@@ -61,7 +61,7 @@ class Notification(TestCase):
         models.Notification.notify(
             self.local_user,
             self.remote_user,
-            notification_type=models.Notification.FAVORITE,
+            notification_type=models.NotificationType.FAVORITE,
         )
         self.assertEqual(models.Notification.objects.count(), 1)
         notification = models.Notification.objects.get()
@@ -70,7 +70,7 @@ class Notification(TestCase):
         models.Notification.notify(
             self.local_user,
             self.another_user,
-            notification_type=models.Notification.FAVORITE,
+            notification_type=models.NotificationType.FAVORITE,
         )
         self.assertEqual(models.Notification.objects.count(), 1)
         notification.refresh_from_db()
@@ -92,7 +92,7 @@ class Notification(TestCase):
         models.Notification.notify(
             self.remote_user,
             self.local_user,
-            notification_type=models.Notification.FAVORITE,
+            notification_type=models.NotificationType.FAVORITE,
         )
         self.assertFalse(models.Notification.objects.exists())
 
@@ -101,7 +101,7 @@ class Notification(TestCase):
         models.Notification.notify(
             self.local_user,
             self.local_user,
-            notification_type=models.Notification.FAVORITE,
+            notification_type=models.NotificationType.FAVORITE,
         )
         self.assertFalse(models.Notification.objects.exists())
 
@@ -154,14 +154,14 @@ class Notification(TestCase):
         models.Notification.notify(
             self.local_user,
             self.remote_user,
-            notification_type=models.Notification.FAVORITE,
+            notification_type=models.NotificationType.FAVORITE,
         )
         self.assertTrue(models.Notification.objects.exists())
 
         models.Notification.unnotify(
             self.local_user,
             self.remote_user,
-            notification_type=models.Notification.FAVORITE,
+            notification_type=models.NotificationType.FAVORITE,
         )
         self.assertFalse(models.Notification.objects.exists())
 
@@ -170,25 +170,25 @@ class Notification(TestCase):
         models.Notification.notify(
             self.local_user,
             self.remote_user,
-            notification_type=models.Notification.FAVORITE,
+            notification_type=models.NotificationType.FAVORITE,
         )
         models.Notification.notify(
             self.local_user,
             self.another_user,
-            notification_type=models.Notification.FAVORITE,
+            notification_type=models.NotificationType.FAVORITE,
         )
         self.assertTrue(models.Notification.objects.exists())
 
         models.Notification.unnotify(
             self.local_user,
             self.remote_user,
-            notification_type=models.Notification.FAVORITE,
+            notification_type=models.NotificationType.FAVORITE,
         )
         self.assertTrue(models.Notification.objects.exists())
 
         models.Notification.unnotify(
             self.local_user,
             self.another_user,
-            notification_type=models.Notification.FAVORITE,
+            notification_type=models.NotificationType.FAVORITE,
         )
         self.assertFalse(models.Notification.objects.exists())

bookwyrm/tests/models/test_user_model.py

@@ -1,7 +1,9 @@
 """ testing models """
 import json
+
 from unittest.mock import patch
 from django.contrib.auth.models import Group
+from django.db import IntegrityError
 from django.test import TestCase
 import responses
 
@@ -9,9 +11,11 @@ from bookwyrm import models
 from bookwyrm.management.commands import initdb
 from bookwyrm.settings import USE_HTTPS, DOMAIN
 
+
 # pylint: disable=missing-class-docstring
 # pylint: disable=missing-function-docstring
 class User(TestCase):
+
     protocol = "https://" if USE_HTTPS else "http://"
 
     # pylint: disable=invalid-name
@@ -26,6 +30,7 @@ class User(TestCase):
                 local=True,
                 localname="mouse",
                 name="hi",
+                summary="a summary",
                 bookwyrm_user=False,
             )
             self.another_user = models.User.objects.create_user(
@@ -218,19 +223,71 @@ class User(TestCase):
 
     @patch("bookwyrm.suggested_users.remove_user_task.delay")
     def test_delete_user(self, _):
-        """deactivate a user"""
+        """permanently delete a user"""
         self.assertTrue(self.user.is_active)
+        self.assertEqual(self.user.name, "hi")
+        self.assertEqual(self.user.summary, "a summary")
+        self.assertEqual(self.user.email, "mouse@mouse.mouse")
         with patch(
             "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"
-        ) as broadcast_mock:
+        ) as broadcast_mock, patch(
+            "bookwyrm.models.user.User.erase_user_statuses"
+        ) as erase_statuses_mock:
             self.user.delete()
 
+        self.assertEqual(erase_statuses_mock.call_count, 1)
+
+        # make sure the deletion is broadcast
         self.assertEqual(broadcast_mock.call_count, 1)
         activity = json.loads(broadcast_mock.call_args[1]["args"][1])
         self.assertEqual(activity["type"], "Delete")
         self.assertEqual(activity["object"], self.user.remote_id)
+
+        self.user.refresh_from_db()
+
+        # the user's account data should be deleted
+        self.assertIsNone(self.user.name)
+        self.assertIsNone(self.user.summary)
+        self.assertNotEqual(self.user.email, "mouse@mouse.mouse")
         self.assertFalse(self.user.is_active)
 
+    @patch("bookwyrm.suggested_users.remove_user_task.delay")
+    @patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async")
+    @patch("bookwyrm.activitystreams.add_status_task.delay")
+    @patch("bookwyrm.activitystreams.remove_status_task.delay")
+    def test_delete_user_erase_statuses(self, *_):
+        """erase user statuses when user is deleted"""
+        status = models.Status.objects.create(user=self.user, content="hello")
+        self.assertFalse(status.deleted)
+        self.assertIsNotNone(status.content)
+        self.assertIsNone(status.deleted_date)
+
+        self.user.delete()
+        status.refresh_from_db()
+
+        self.assertTrue(status.deleted)
+        self.assertIsNone(status.content)
+        self.assertIsNotNone(status.deleted_date)
+
+    @patch("bookwyrm.suggested_users.remove_user_task.delay")
+    @patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async")
+    @patch("bookwyrm.activitystreams.add_status_task.delay")
+    def test_delete_user_erase_statuses_invalid(self, *_):
+        """erase user statuses when user is deleted"""
+        status = models.Status.objects.create(user=self.user, content="hello")
+        self.assertFalse(status.deleted)
+        self.assertIsNotNone(status.content)
+        self.assertIsNone(status.deleted_date)
+
+        self.user.deactivate()
+        with self.assertRaises(IntegrityError):
+            self.user.erase_user_statuses()
+
+        status.refresh_from_db()
+        self.assertFalse(status.deleted)
+        self.assertIsNotNone(status.content)
+        self.assertIsNone(status.deleted_date)
+
     def test_admins_no_admins(self):
         """list of admins"""
         result = models.User.admins()

bookwyrm/tests/test_isbn.py

@@ -29,3 +29,10 @@ class TestISBN(TestCase):
         self.assertEqual(hyphenator.hyphenate("9786769533251"), "9786769533251")
         # 979-8 (United States) 2300000-3499999 (unassigned)
         self.assertEqual(hyphenator.hyphenate("9798311111111"), "9798311111111")
+
+    def test_isbn_hyphenation_invalid_data(self):
+        """Make sure not to throw an error when a bad ISBN is found"""
+        # no action taken
+        self.assertEqual(hyphenator.hyphenate("978-0-4633461-1-2"), "978-0-4633461-1-2")
+        self.assertEqual(hyphenator.hyphenate("9-0-4633461-1-2"), "9-0-4633461-1-2")
+        self.assertEqual(hyphenator.hyphenate("90463346112"), "90463346112")

bookwyrm/tests/views/inbox/test_inbox_delete.py

@@ -11,6 +11,7 @@ from bookwyrm import models, views
 class InboxActivities(TestCase):
     """inbox tests"""
 
+    # pylint: disable=invalid-name
     def setUp(self):
         """basic user and book data"""
         with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch(
@@ -97,7 +98,8 @@ class InboxActivities(TestCase):
         self.assertEqual(models.Notification.objects.get(), notif)
 
     @patch("bookwyrm.suggested_users.remove_user_task.delay")
-    def test_delete_user(self, _):
+    @patch("bookwyrm.activitystreams.remove_status_task.delay")
+    def test_delete_user(self, *_):
         """delete a user"""
         self.assertTrue(models.User.objects.get(username="rat@example.com").is_active)
         activity = {

bookwyrm/tests/views/test_status.py

@@ -420,21 +420,25 @@ http://www.fish.com/"""
             'okay\n\n<a href="http://www.fish.com/">www.fish.com/</a>',
         )
 
-    def test_format_links_parens(self, *_):
-        """find and format urls into a tags"""
-        url = "http://www.fish.com/"
-        self.assertEqual(
-            views.status.format_links(f"({url})"),
-            f'(<a href="{url}">www.fish.com/</a>)',
-        )
-
     def test_format_links_punctuation(self, *_):
-        """don’t take trailing punctuation into account pls"""
-        url = "http://www.fish.com/"
-        self.assertEqual(
-            views.status.format_links(f"{url}."),
-            f'<a href="{url}">www.fish.com/</a>.',
-        )
+        """test many combinations of brackets, URLs, and punctuation"""
+        url = "https://bookwyrm.social"
+        html = f'<a href="{url}">bookwyrm.social</a>'
+        test_table = [
+            ("punct", f"text and {url}.", f"text and {html}."),
+            ("multi_punct", f"text, then {url}?...", f"text, then {html}?..."),
+            ("bracket_punct", f"here ({url}).", f"here ({html})."),
+            ("punct_bracket", f"there [{url}?]", f"there [{html}?]"),
+            ("punct_bracket_punct", f"not here? ({url}!).", f"not here? ({html}!)."),
+            (
+                "multi_punct_bracket",
+                f"not there ({url}...);",
+                f"not there ({html}...);",
+            ),
+        ]
+        for desc, text, output in test_table:
+            with self.subTest(desc=desc):
+                self.assertEqual(views.status.format_links(text), output)
 
     def test_format_links_special_chars(self, *_):
         """find and format urls into a tags"""
@@ -464,6 +468,13 @@ http://www.fish.com/"""
             views.status.format_links(url), f'<a href="{url}">{url[8:]}</a>'
         )
 
+    def test_format_links_ignore_non_urls(self, *_):
+        """formating links should leave plain text untouced"""
+        text_elision = "> “The distinction is significant.” [...]"  # bookwyrm#2993
+        text_quoteparens = "some kind of gene-editing technology (?)"  # bookwyrm#3049
+        self.assertEqual(views.status.format_links(text_elision), text_elision)
+        self.assertEqual(views.status.format_links(text_quoteparens), text_quoteparens)
+
     def test_format_mentions_with_at_symbol_links(self, *_):
         """A link with an @username shouldn't treat the username as a mention"""
         content = "a link to https://example.com/user/@mouse"

bookwyrm/views/group.py

@@ -13,9 +13,11 @@ from django.contrib.postgres.search import TrigramSimilarity
 from django.db.models.functions import Greatest
 
 from bookwyrm import forms, models
+from bookwyrm.models import NotificationType
 from bookwyrm.suggested_users import suggested_users
 from .helpers import get_user_from_username, maybe_redirect_local_path
 
+
 # pylint: disable=no-self-use
 class Group(View):
     """group page"""
@@ -59,11 +61,11 @@ class Group(View):
         model = apps.get_model("bookwyrm.Notification", require_ready=True)
         for field in form.changed_data:
             notification_type = (
-                model.GROUP_PRIVACY
+                NotificationType.GROUP_PRIVACY
                 if field == "privacy"
-                else model.GROUP_NAME
+                else NotificationType.GROUP_NAME
                 if field == "name"
-                else model.GROUP_DESCRIPTION
+                else NotificationType.GROUP_DESCRIPTION
                 if field == "description"
                 else None
             )
@@ -251,7 +253,9 @@ def remove_member(request):
 
         memberships = models.GroupMember.objects.filter(group=group)
         model = apps.get_model("bookwyrm.Notification", require_ready=True)
-        notification_type = model.LEAVE if user == request.user else model.REMOVE
+        notification_type = (
+            NotificationType.LEAVE if user == request.user else NotificationType.REMOVE
+        )
         # let the other members know about it
         for membership in memberships:
             member = membership.user
@@ -264,7 +268,7 @@ def remove_member(request):
                 )
 
         # let the user (now ex-member) know as well, if they were removed
-        if notification_type == model.REMOVE:
+        if notification_type == NotificationType.REMOVE:
             model.notify(
                 user, None, related_group=group, notification_type=notification_type
             )

bookwyrm/views/status.py

@@ -1,7 +1,6 @@
 """ what are we here for if not for posting """
 import re
 import logging
-from urllib.parse import urlparse
 
 from django.contrib.auth.decorators import login_required
 from django.core.validators import URLValidator
@@ -297,65 +296,51 @@ def find_or_create_hashtags(content):
 
 def format_links(content):
     """detect and format links"""
-    validator = URLValidator()
-    formatted_content = ""
+    validator = URLValidator(["http", "https"])
+    schema_re = re.compile(r"\bhttps?://")
     split_content = re.split(r"(\s+)", content)
 
-    for potential_link in split_content:
-        if not potential_link:
+    for i, potential_link in enumerate(split_content):
+        if not schema_re.search(potential_link):
             continue
-        wrapped = _wrapped(potential_link)
-        if wrapped:
-            wrapper_close = potential_link[-1]
-            formatted_content += potential_link[0]
-            potential_link = potential_link[1:-1]
-
-        ends_with_punctuation = _ends_with_punctuation(potential_link)
-        if ends_with_punctuation:
-            punctuation_glyph = potential_link[-1]
-            potential_link = potential_link[0:-1]
 
+        # Strip surrounding brackets and trailing punctuation.
+        prefix, potential_link, suffix = _unwrap(potential_link)
         try:
             # raises an error on anything that's not a valid link
             validator(potential_link)
 
             # use everything but the scheme in the presentation of the link
-            url = urlparse(potential_link)
-            link = url.netloc + url.path + url.params
-            if url.query != "":
-                link += "?" + url.query
-            if url.fragment != "":
-                link += "#" + url.fragment
-
-            formatted_content += f'<a href="{potential_link}">{link}</a>'
+            link = schema_re.sub("", potential_link)
+            split_content[i] = f'{prefix}<a href="{potential_link}">{link}</a>{suffix}'
         except (ValidationError, UnicodeError):
-            formatted_content += potential_link
+            pass
 
-        if wrapped:
-            formatted_content += wrapper_close
-
-        if ends_with_punctuation:
-            formatted_content += punctuation_glyph
-
-    return formatted_content
+    return "".join(split_content)
 
 
-def _wrapped(text):
-    """check if a line of text is wrapped"""
-    wrappers = [("(", ")"), ("[", "]"), ("{", "}")]
-    for wrapper in wrappers:
+def _unwrap(text):
+    """split surrounding brackets and trailing punctuation from a string of text"""
+    punct = re.compile(r'([.,;:!?"’”»]+)$')
+    prefix = suffix = ""
+
+    if punct.search(text):
+        # Move punctuation to suffix segment.
+        text, suffix, _ = punct.split(text)
+
+    for wrapper in ("()", "[]", "{}"):
         if text[0] == wrapper[0] and text[-1] == wrapper[-1]:
-            return True
-    return False
+            # Split out wrapping chars.
+            suffix = text[-1] + suffix
+            prefix, text = text[:1], text[1:-1]
+            break  # Nested wrappers not supported atm.
 
+    if punct.search(text):
+        # Move inner punctuation to suffix segment.
+        text, inner_punct, _ = punct.split(text)
+        suffix = inner_punct + suffix
 
-def _ends_with_punctuation(text):
-    """check if a line of text ends with a punctuation glyph"""
-    glyphs = [".", ",", ";", ":", "!", "?", "”", "’", '"', "»"]
-    for glyph in glyphs:
-        if text[-1] == glyph:
-            return True
-    return False
+    return prefix, text, suffix
 
 
 def to_markdown(content):

bw-dev

@@ -91,7 +91,7 @@ case "$CMD" in
         $DOCKER_COMPOSE run --rm --service-ports web
         ;;
     initdb)
-        initdb "@"
+        initdb "$@"
         ;;
     resetdb)
         prod_error