diff --git a/bookwyrm/isbn/isbn.py b/bookwyrm/isbn/isbn.py index 4cc7f47dd..56062ff7b 100644 --- a/bookwyrm/isbn/isbn.py +++ b/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 diff --git a/bookwyrm/migrations/0183_auto_20231105_1607.py b/bookwyrm/migrations/0183_auto_20231105_1607.py new file mode 100644 index 000000000..0c8376adc --- /dev/null +++ b/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), + ), + ] diff --git a/bookwyrm/migrations/0184_auto_20231106_0421.py b/bookwyrm/migrations/0184_auto_20231106_0421.py new file mode 100644 index 000000000..e8197dea1 --- /dev/null +++ b/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 + ), + ] diff --git a/bookwyrm/models/__init__.py b/bookwyrm/models/__init__.py index 4f86f2aa6..6bb99c7f2 100644 --- a/bookwyrm/models/__init__.py +++ b/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 diff --git a/bookwyrm/models/antispam.py b/bookwyrm/models/antispam.py index 94d978ec4..1067cbf1d 100644 --- a/bookwyrm/models/antispam.py +++ b/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) diff --git a/bookwyrm/models/book.py b/bookwyrm/models/book.py index 9e05c03af..e5941136f 100644 --- a/bookwyrm/models/book.py +++ b/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): diff --git a/bookwyrm/models/group.py b/bookwyrm/models/group.py index 003b23d02..d02b56ab1 100644 --- a/bookwyrm/models/group.py +++ b/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): diff --git a/bookwyrm/models/move.py b/bookwyrm/models/move.py index a5bf9d76d..d6d8ef78f 100644 --- a/bookwyrm/models/move.py +++ b/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: diff --git a/bookwyrm/models/notification.py b/bookwyrm/models/notification.py index f9cbee8d8..d4774cf54 100644 --- a/bookwyrm/models/notification.py +++ b/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, ) diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index 11646431b..cc44fe2bf 100644 --- a/bookwyrm/models/status.py +++ b/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): diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index c152cf445..75ca1d527 100644 --- a/bookwyrm/models/user.py +++ b/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""" diff --git a/bookwyrm/templates/guided_tour/home.html b/bookwyrm/templates/guided_tour/home.html index be8d095af..a464206ef 100644 --- a/bookwyrm/templates/guided_tour/home.html +++ b/bookwyrm/templates/guided_tour/home.html @@ -99,7 +99,7 @@ homeTour.addSteps([ ], }, { - text: "{% trans 'Use the Feed, Lists and Discover 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 Lists, Discover, and Your Books 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." %}

{% trans "Try selecting Profile from the drop down menu to continue the tour." %}

`, + text: `{% trans "Your profile, user directory, direct messages, and settings can be accessed by clicking on your name in the menu here." %}

{% trans "Try selecting Profile from the drop down menu to continue the tour." %}

`, title: "{% trans 'Profile and settings menu' %}", attachTo: { element: checkResponsiveState('#navbar-dropdown'), diff --git a/bookwyrm/templates/notifications/item.html b/bookwyrm/templates/notifications/item.html index 2175395ea..1f7adbf17 100644 --- a/bookwyrm/templates/notifications/item.html +++ b/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' %} diff --git a/bookwyrm/templates/settings/users/user_admin.html b/bookwyrm/templates/settings/users/user_admin.html index a1d93ddd0..cc5c51ba7 100644 --- a/bookwyrm/templates/settings/users/user_admin.html +++ b/bookwyrm/templates/settings/users/user_admin.html @@ -74,31 +74,7 @@ {{ user.created_date }} {{ user.last_active_date }} - {% if user.is_active %} - {% if user.moved_to %} - - {% trans "Moved" %} - {% else %} - - {% trans "Active" %} - {% endif %} - {% elif user.deactivation_reason == "moderator_deletion" or user.deactivation_reason == "self_deletion" %} - - {% trans "Deleted" %} - ({{ user.get_deactivation_reason_display }}) - {% else %} - - {% trans "Inactive" %} - ({{ user.get_deactivation_reason_display }}) - {% endif %} + {% include "snippets/user_active_tag.html" with user=user %} {% if status == "federated" %} diff --git a/bookwyrm/templates/settings/users/user_info.html b/bookwyrm/templates/settings/users/user_info.html index 368045a0d..f35c60db9 100644 --- a/bookwyrm/templates/settings/users/user_info.html +++ b/bookwyrm/templates/settings/users/user_info.html @@ -23,24 +23,7 @@

{% trans "Status" %}

- {% if user.is_active %} - {% if user.moved_to %} -

- {% trans "Moved" %} -

- {% else %} -

- {% trans "Active" %} -

- {% endif %} - {% else %} -

- {% trans "Inactive" %} - {% if user.deactivation_reason %} - ({% trans user.get_deactivation_reason_display %}) - {% endif %} -

- {% endif %} + {% include "snippets/user_active_tag.html" with large=True %}

{% if user.local %} {% trans "Local" %} diff --git a/bookwyrm/templates/snippets/user_active_tag.html b/bookwyrm/templates/snippets/user_active_tag.html new file mode 100644 index 000000000..1d85ae68b --- /dev/null +++ b/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 %} diff --git a/bookwyrm/templates/snippets/user_active_tag_item.html b/bookwyrm/templates/snippets/user_active_tag_item.html new file mode 100644 index 000000000..e722150f2 --- /dev/null +++ b/bookwyrm/templates/snippets/user_active_tag_item.html @@ -0,0 +1,19 @@ +{% if large %} + +

+ + {{ text }} + {% if deactivation_reason %} + ({{ deactivation_reason }}) + {% endif %} +

+ +{% else %} + + +{{ text }} + +{% endif %} + diff --git a/bookwyrm/tests/migrations/test_0184.py b/bookwyrm/tests/migrations/test_0184.py new file mode 100644 index 000000000..4bf1b66c9 --- /dev/null +++ b/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) diff --git a/bookwyrm/tests/models/test_activitypub_mixin.py b/bookwyrm/tests/models/test_activitypub_mixin.py index a465c2c12..645a6546b 100644 --- a/bookwyrm/tests/models/test_activitypub_mixin.py +++ b/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")) diff --git a/bookwyrm/tests/models/test_book_model.py b/bookwyrm/tests/models/test_book_model.py index 8122e9505..4347efcb6 100644 --- a/bookwyrm/tests/models/test_book_model.py +++ b/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") diff --git a/bookwyrm/tests/models/test_notification.py b/bookwyrm/tests/models/test_notification.py index 0e4fe91c7..1c412e1b4 100644 --- a/bookwyrm/tests/models/test_notification.py +++ b/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()) diff --git a/bookwyrm/tests/models/test_user_model.py b/bookwyrm/tests/models/test_user_model.py index 838dd2e49..30d7918c0 100644 --- a/bookwyrm/tests/models/test_user_model.py +++ b/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() diff --git a/bookwyrm/tests/test_isbn.py b/bookwyrm/tests/test_isbn.py index b528e9210..5486c7151 100644 --- a/bookwyrm/tests/test_isbn.py +++ b/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") diff --git a/bookwyrm/tests/views/inbox/test_inbox_delete.py b/bookwyrm/tests/views/inbox/test_inbox_delete.py index 0fb108e22..7b4c12564 100644 --- a/bookwyrm/tests/views/inbox/test_inbox_delete.py +++ b/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 = { diff --git a/bookwyrm/tests/views/test_status.py b/bookwyrm/tests/views/test_status.py index 33bd8b53a..424698130 100644 --- a/bookwyrm/tests/views/test_status.py +++ b/bookwyrm/tests/views/test_status.py @@ -420,21 +420,25 @@ http://www.fish.com/""" 'okay\n\nwww.fish.com/', ) - 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'(www.fish.com/)', - ) - 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'www.fish.com/.', - ) + """test many combinations of brackets, URLs, and punctuation""" + url = "https://bookwyrm.social" + html = f'bookwyrm.social' + 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'{url[8:]}' ) + 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" diff --git a/bookwyrm/views/group.py b/bookwyrm/views/group.py index 1ccfd6849..5ac4954e8 100644 --- a/bookwyrm/views/group.py +++ b/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 ) diff --git a/bookwyrm/views/status.py b/bookwyrm/views/status.py index 7a0517b01..34b62d0b4 100644 --- a/bookwyrm/views/status.py +++ b/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'{link}' + link = schema_re.sub("", potential_link) + split_content[i] = f'{prefix}{link}{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): diff --git a/bw-dev b/bw-dev index 57441f013..6769f4bcd 100755 --- a/bw-dev +++ b/bw-dev @@ -91,7 +91,7 @@ case "$CMD" in $DOCKER_COMPOSE run --rm --service-ports web ;; initdb) - initdb "@" + initdb "$@" ;; resetdb) prod_error