From e06e507c8d3296d9e639ff0764c7d4135c53369a Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 8 Sep 2021 11:14:41 -0700 Subject: [PATCH 1/5] Cleans up suggested users logic --- bookwyrm/suggested_users.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/bookwyrm/suggested_users.py b/bookwyrm/suggested_users.py index 92902938..6fbdee88 100644 --- a/bookwyrm/suggested_users.py +++ b/bookwyrm/suggested_users.py @@ -43,7 +43,6 @@ class SuggestedUsers(RedisStore): ~Q(id=user.id), ~Q(followers=user), ~Q(follower_requests=user), - bookwyrm_user=True, ) def get_stores_for_object(self, obj): @@ -103,7 +102,9 @@ class SuggestedUsers(RedisStore): def get_annotated_users(viewer, *args, **kwargs): """Users, annotated with things they have in common""" return ( - models.User.objects.filter(discoverable=True, is_active=True, *args, **kwargs) + models.User.objects.filter( + discoverable=True, is_active=True, bookwyrm_user=True, *args, **kwargs + ) .exclude(Q(id__in=viewer.blocks.all()) | Q(blocks=viewer)) .annotate( mutuals=Count( @@ -178,15 +179,21 @@ def update_suggestions_on_unfollow(sender, instance, **kwargs): @receiver(signals.post_save, sender=models.User) # pylint: disable=unused-argument, too-many-arguments -def add_new_user(sender, instance, created, update_fields=None, **kwargs): - """a new user, wow how cool""" +def updated_user(sender, instance, created, update_fields=None, **kwargs): + """an updated user, neat""" # a new user is found, create suggestions for them if created and instance.local: rerank_suggestions_task.delay(instance.id) + # we know what fields were updated and discoverability didn't change if update_fields and not "discoverable" in update_fields: return + # deleted the user + if not instance.is_active: + remove_user_task.delay(instance.id) + return + # this happens on every save, not just when discoverability changes, annoyingly if instance.discoverable: rerank_user_task.delay(instance.id, update_only=False) From 2e2ee723339a434698b3f9910ec54d4d4fd386f7 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 8 Sep 2021 11:18:55 -0700 Subject: [PATCH 2/5] Fixes inactive or mastodon users showing up in suggestions They shouldn't be there, but just to be safe --- bookwyrm/suggested_users.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/bookwyrm/suggested_users.py b/bookwyrm/suggested_users.py index 6fbdee88..cbd33659 100644 --- a/bookwyrm/suggested_users.py +++ b/bookwyrm/suggested_users.py @@ -85,10 +85,12 @@ class SuggestedUsers(RedisStore): values = self.get_store(self.store_id(user), withscores=True) results = [] # annotate users with mutuals and shared book counts - for user_id, rank in values[:5]: + for user_id, rank in values: counts = self.get_counts_from_rank(rank) try: - user = models.User.objects.get(id=user_id) + user = models.User.objects.get( + id=user_id, is_active=True, bookwyrm_user=True + ) except models.User.DoesNotExist as err: # if this happens, the suggestions are janked way up logger.exception(err) @@ -96,6 +98,8 @@ class SuggestedUsers(RedisStore): user.mutuals = counts["mutuals"] # user.shared_books = counts["shared_books"] results.append(user) + if len(results) >= 5: + break return results @@ -179,14 +183,16 @@ def update_suggestions_on_unfollow(sender, instance, **kwargs): @receiver(signals.post_save, sender=models.User) # pylint: disable=unused-argument, too-many-arguments -def updated_user(sender, instance, created, update_fields=None, **kwargs): +def update_user(sender, instance, created, update_fields=None, **kwargs): """an updated user, neat""" # a new user is found, create suggestions for them if created and instance.local: rerank_suggestions_task.delay(instance.id) # we know what fields were updated and discoverability didn't change - if update_fields and not "discoverable" in update_fields: + if not instance.bookwyrm_user or ( + update_fields and not "discoverable" in update_fields + ): return # deleted the user @@ -201,6 +207,9 @@ def updated_user(sender, instance, created, update_fields=None, **kwargs): remove_user_task.delay(instance.id) +# ------------------- TASKS + + @app.task(queue="low_priority") def rerank_suggestions_task(user_id): """do the hard work in celery""" From d3b3dd6d996620643d4dd9a27427b1a4344440f1 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 8 Sep 2021 11:38:22 -0700 Subject: [PATCH 3/5] Update suggestions on domain block --- bookwyrm/models/federated_server.py | 4 ++-- bookwyrm/suggested_users.py | 27 +++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/bookwyrm/models/federated_server.py b/bookwyrm/models/federated_server.py index e297c46c..114f387b 100644 --- a/bookwyrm/models/federated_server.py +++ b/bookwyrm/models/federated_server.py @@ -28,7 +28,7 @@ class FederatedServer(BookWyrmModel): def block(self): """block a server""" self.status = "blocked" - self.save() + self.save(update_fields="status") # deactivate all associated users self.user_set.filter(is_active=True).update( @@ -45,7 +45,7 @@ class FederatedServer(BookWyrmModel): def unblock(self): """unblock a server""" self.status = "federated" - self.save() + self.save(update_fields="status") self.user_set.filter(deactivation_reason="domain_block").update( is_active=True, deactivation_reason=None diff --git a/bookwyrm/suggested_users.py b/bookwyrm/suggested_users.py index cbd33659..953945d1 100644 --- a/bookwyrm/suggested_users.py +++ b/bookwyrm/suggested_users.py @@ -207,6 +207,19 @@ def update_user(sender, instance, created, update_fields=None, **kwargs): remove_user_task.delay(instance.id) +@receiver(signals.post_save, sender=models.FederatedServer) +def domain_level_update(sender, instance, created, update_fields=None, **kwargs): + """remove users on a domain block""" + if not update_fields or "status" not in update_fields: + return + + userset = instance.user_set.values_list("id", flat=True) + if instance.status == "blocked": + bulk_remove_users_task.delay(userset) + return + bulk_add_users_task.delay(userset) + + # ------------------- TASKS @@ -235,3 +248,17 @@ def remove_suggestion_task(user_id, suggested_user_id): """remove a specific user from a specific user's suggestions""" suggested_user = models.User.objects.get(id=suggested_user_id) suggested_users.remove_suggestion(user_id, suggested_user) + + +@app.task(queue="low_priority") +def bulk_remove_users_task(user_ids): + """remove a bunch of users from recs""" + for user in models.User.objects.filter(id__in=user_ids): + suggested_users.remove_object_from_related_stores(user) + + +@app.task(queue="low_priority") +def bulk_add_users_task(user_ids): + """remove a bunch of users from recs""" + for user in models.User.objects.filter(id__in=user_ids): + suggested_users.rerank_obj(user, update_only=False) From 88a65b0b883aa0e0b5f9912a7d09467222759e8b Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 8 Sep 2021 11:47:36 -0700 Subject: [PATCH 4/5] Only re-rank on bookwyrm instances --- bookwyrm/models/federated_server.py | 4 ++-- bookwyrm/suggested_users.py | 19 +++++++++++-------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/bookwyrm/models/federated_server.py b/bookwyrm/models/federated_server.py index 114f387b..2be13b38 100644 --- a/bookwyrm/models/federated_server.py +++ b/bookwyrm/models/federated_server.py @@ -28,7 +28,7 @@ class FederatedServer(BookWyrmModel): def block(self): """block a server""" self.status = "blocked" - self.save(update_fields="status") + self.save(update_fields=["status"]) # deactivate all associated users self.user_set.filter(is_active=True).update( @@ -45,7 +45,7 @@ class FederatedServer(BookWyrmModel): def unblock(self): """unblock a server""" self.status = "federated" - self.save(update_fields="status") + self.save(update_fields=["status"]) self.user_set.filter(deactivation_reason="domain_block").update( is_active=True, deactivation_reason=None diff --git a/bookwyrm/suggested_users.py b/bookwyrm/suggested_users.py index 953945d1..c9307734 100644 --- a/bookwyrm/suggested_users.py +++ b/bookwyrm/suggested_users.py @@ -210,14 +210,17 @@ def update_user(sender, instance, created, update_fields=None, **kwargs): @receiver(signals.post_save, sender=models.FederatedServer) def domain_level_update(sender, instance, created, update_fields=None, **kwargs): """remove users on a domain block""" - if not update_fields or "status" not in update_fields: + if ( + not update_fields + or "status" not in update_fields + or instance.application_type != "bookwyrm" + ): return - userset = instance.user_set.values_list("id", flat=True) if instance.status == "blocked": - bulk_remove_users_task.delay(userset) + bulk_remove_instance_task.delay(instance.id) return - bulk_add_users_task.delay(userset) + bulk_add_instance_task.delay(instance.id) # ------------------- TASKS @@ -251,14 +254,14 @@ def remove_suggestion_task(user_id, suggested_user_id): @app.task(queue="low_priority") -def bulk_remove_users_task(user_ids): +def bulk_remove_instance_task(instance_id): """remove a bunch of users from recs""" - for user in models.User.objects.filter(id__in=user_ids): + for user in models.User.objects.filter(federated_server__id=instance_id): suggested_users.remove_object_from_related_stores(user) @app.task(queue="low_priority") -def bulk_add_users_task(user_ids): +def bulk_add_instance_task(instance_id): """remove a bunch of users from recs""" - for user in models.User.objects.filter(id__in=user_ids): + for user in models.User.objects.filter(federated_server__id=instance_id): suggested_users.rerank_obj(user, update_only=False) From 4db5677509286c19795a05b61d19f324efb33ae6 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 8 Sep 2021 12:06:23 -0700 Subject: [PATCH 5/5] Fixes filters --- bookwyrm/suggested_users.py | 7 +++---- bookwyrm/tests/views/test_federation.py | 10 ++++++++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/bookwyrm/suggested_users.py b/bookwyrm/suggested_users.py index c9307734..88343061 100644 --- a/bookwyrm/suggested_users.py +++ b/bookwyrm/suggested_users.py @@ -43,6 +43,7 @@ class SuggestedUsers(RedisStore): ~Q(id=user.id), ~Q(followers=user), ~Q(follower_requests=user), + bookwyrm_user=True, ) def get_stores_for_object(self, obj): @@ -106,9 +107,7 @@ class SuggestedUsers(RedisStore): def get_annotated_users(viewer, *args, **kwargs): """Users, annotated with things they have in common""" return ( - models.User.objects.filter( - discoverable=True, is_active=True, bookwyrm_user=True, *args, **kwargs - ) + models.User.objects.filter(discoverable=True, is_active=True, *args, **kwargs) .exclude(Q(id__in=viewer.blocks.all()) | Q(blocks=viewer)) .annotate( mutuals=Count( @@ -196,7 +195,7 @@ def update_user(sender, instance, created, update_fields=None, **kwargs): return # deleted the user - if not instance.is_active: + if not created and not instance.is_active: remove_user_task.delay(instance.id) return diff --git a/bookwyrm/tests/views/test_federation.py b/bookwyrm/tests/views/test_federation.py index ce5de7e3..493aaf93 100644 --- a/bookwyrm/tests/views/test_federation.py +++ b/bookwyrm/tests/views/test_federation.py @@ -80,7 +80,9 @@ class FederationViews(TestCase): request.user = self.local_user request.user.is_superuser = True - view(request, server.id) + with patch("bookwyrm.suggested_users.bulk_remove_instance_task.delay") as mock: + view(request, server.id) + self.assertEqual(mock.call_count, 1) server.refresh_from_db() self.remote_user.refresh_from_db() @@ -118,7 +120,11 @@ class FederationViews(TestCase): request.user = self.local_user request.user.is_superuser = True - views.federation.unblock_server(request, server.id) + with patch("bookwyrm.suggested_users.bulk_add_instance_task.delay") as mock: + views.federation.unblock_server(request, server.id) + self.assertEqual(mock.call_count, 1) + self.assertEqual(mock.call_args[0][0], server.id) + server.refresh_from_db() self.remote_user.refresh_from_db() self.assertEqual(server.status, "federated")