From 2ba0e3d7ff786fac6d52005a7c725bae33093dd7 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 19 Nov 2023 20:03:48 +1100 Subject: [PATCH 1/4] Allow removing followers and fix follow rejections * adds the ability to remove a user from your followers list * fixes verbs.Reject to process reject activities for previously accepted follows in both directions fixes #2635 --- bookwyrm/activitypub/verbs.py | 32 +++++++++++++++++-- bookwyrm/models/relationship.py | 28 ++++++++++++---- bookwyrm/static/js/bookwyrm.js | 32 +++++++++++++------ .../templates/snippets/follow_button.html | 12 ++++++- .../user/relationships/followers.html | 5 +++ .../templates/user/relationships/layout.html | 2 +- bookwyrm/urls.py | 1 + bookwyrm/views/__init__.py | 1 + bookwyrm/views/follow.py | 28 ++++++++++++++++ 9 files changed, 120 insertions(+), 21 deletions(-) diff --git a/bookwyrm/activitypub/verbs.py b/bookwyrm/activitypub/verbs.py index 00c9524fe..2753104b2 100644 --- a/bookwyrm/activitypub/verbs.py +++ b/bookwyrm/activitypub/verbs.py @@ -171,8 +171,36 @@ class Reject(Verb): type: str = "Reject" def action(self, allow_external_connections=True): - """reject a follow request""" - obj = self.object.to_model(save=False, allow_create=False) + """reject a follow or follow request""" + + if self.object.type == "Follow": + model = apps.get_model("bookwyrm.UserFollowRequest") + obj = self.object.to_model( + model=model, + save=False, + allow_create=False, + allow_external_connections=allow_external_connections, + ) + if not obj: + # This is a deletion (soft-block) of an accepted follow + model = apps.get_model("bookwyrm.UserFollows") + obj = self.object.to_model( + model=model, + save=False, + allow_create=False, + allow_external_connections=allow_external_connections, + ) + else: + # it's something else + obj = self.object.to_model( + model=model, + save=False, + allow_create=False, + allow_external_connections=allow_external_connections, + ) + if not obj: + # if we don't have the object, we can't reject it. + return obj.reject() diff --git a/bookwyrm/models/relationship.py b/bookwyrm/models/relationship.py index 7af6ad5ab..3386a02dc 100644 --- a/bookwyrm/models/relationship.py +++ b/bookwyrm/models/relationship.py @@ -65,6 +65,13 @@ class UserRelationship(BookWyrmModel): base_path = self.user_subject.remote_id return f"{base_path}#follows/{self.id}" + def get_accept_reject_id(self, status): + """get id for sending an accept or reject of a local user""" + + base_path = self.user_object.remote_id + status_id = self.id or 0 + return f"{base_path}#{status}/{status_id}" + class UserFollows(ActivityMixin, UserRelationship): """Following a user""" @@ -105,6 +112,20 @@ class UserFollows(ActivityMixin, UserRelationship): ) return obj + def reject(self): + """generate a Reject for this follow. This would normally happen + when a user deletes a follow they previously accepted""" + + if self.user_object.local: + activity = activitypub.Reject( + id=self.get_accept_reject_id(status="rejects"), + actor=self.user_object.remote_id, + object=self.to_activity(), + ).serialize() + self.broadcast(activity, self.user_object) + + self.delete() + class UserFollowRequest(ActivitypubMixin, UserRelationship): """following a user requires manual or automatic confirmation""" @@ -148,13 +169,6 @@ class UserFollowRequest(ActivitypubMixin, UserRelationship): if not manually_approves: self.accept() - def get_accept_reject_id(self, status): - """get id for sending an accept or reject of a local user""" - - base_path = self.user_object.remote_id - status_id = self.id or 0 - return f"{base_path}#{status}/{status_id}" - def accept(self, broadcast_only=False): """turn this request into the real deal""" user = self.user_object diff --git a/bookwyrm/static/js/bookwyrm.js b/bookwyrm/static/js/bookwyrm.js index a2351a98c..7093219e5 100644 --- a/bookwyrm/static/js/bookwyrm.js +++ b/bookwyrm/static/js/bookwyrm.js @@ -330,18 +330,30 @@ let BookWyrm = new (class { const bookwyrm = this; const form = event.currentTarget; + const formAction = event.submitter.getAttribute("formaction") || form.action; const relatedforms = document.querySelectorAll(`.${form.dataset.id}`); // Toggle class on all related forms. - relatedforms.forEach((relatedForm) => - bookwyrm.addRemoveClass( - relatedForm, - "is-hidden", - relatedForm.className.indexOf("is-hidden") == -1 - ) - ); + if (formAction == "/remove-follow") { + // Remove ALL follow/unfollow/remote buttons + relatedforms.forEach((relatedForm) => relatedForm.classList.add("is-hidden")); - this.ajaxPost(form).catch((error) => { + // Remove orphaned user-options dropdown + const parent = form.parentElement; + const next = parent.nextElementSibling; + + next.classList.add("is-hidden"); + } else { + relatedforms.forEach((relatedForm) => + bookwyrm.addRemoveClass( + relatedForm, + "is-hidden", + relatedForm.className.indexOf("is-hidden") == -1 + ) + ); + } + + this.ajaxPost(formAction, form).catch((error) => { // @todo Display a notification in the UI instead. console.warn("Request failed:", error); }); @@ -353,8 +365,8 @@ let BookWyrm = new (class { * @param {object} form - Form to be submitted * @return {Promise} */ - ajaxPost(form) { - return fetch(form.action, { + ajaxPost(target, form) { + return fetch(target, { method: "POST", body: new FormData(form), headers: { diff --git a/bookwyrm/templates/snippets/follow_button.html b/bookwyrm/templates/snippets/follow_button.html index 2bde47f58..5c0839065 100644 --- a/bookwyrm/templates/snippets/follow_button.html +++ b/bookwyrm/templates/snippets/follow_button.html @@ -12,6 +12,7 @@
+ {% if not followers_page %} -
{% endfor %} diff --git a/bookwyrm/urls.py b/bookwyrm/urls.py index 8541f4fb6..ab1dca378 100644 --- a/bookwyrm/urls.py +++ b/bookwyrm/urls.py @@ -763,6 +763,7 @@ urlpatterns = [ # following re_path(r"^follow/?$", views.follow, name="follow"), re_path(r"^unfollow/?$", views.unfollow, name="unfollow"), + re_path(r"^remove-follow/?$", views.remove_follow, name="remove-follow"), re_path(r"^accept-follow-request/?$", views.accept_follow_request), re_path(r"^delete-follow-request/?$", views.delete_follow_request), re_path(r"^ostatus_follow/?$", views.remote_follow, name="remote-follow"), diff --git a/bookwyrm/views/__init__.py b/bookwyrm/views/__init__.py index 2d2e97f52..0d7af9d68 100644 --- a/bookwyrm/views/__init__.py +++ b/bookwyrm/views/__init__.py @@ -113,6 +113,7 @@ from .feed import DirectMessage, Feed, Replies, Status from .follow import ( follow, unfollow, + remove_follow, ostatus_follow_request, ostatus_follow_success, remote_follow, diff --git a/bookwyrm/views/follow.py b/bookwyrm/views/follow.py index 0090cbe32..f9a09e2c9 100644 --- a/bookwyrm/views/follow.py +++ b/bookwyrm/views/follow.py @@ -69,6 +69,34 @@ def unfollow(request): return redirect("/") +@login_required +@require_POST +def remove_follow(request): + """remove a previously approved follower without blocking them""" + + username = request.POST["user"] + to_remove = get_user_from_username(request.user, username) + + try: + models.UserFollows.objects.get( + user_subject=to_remove, user_object=request.user + ).reject() + except models.UserFollows.DoesNotExist: + clear_cache(to_remove, request.user) + + try: + models.UserFollowRequest.objects.get( + user_subject=to_remove, user_object=request.user + ).reject() + except models.UserFollowRequest.DoesNotExist: + clear_cache(to_remove, request.user) + + if is_api_request(request): + return HttpResponse() + # this is handled with ajax so it shouldn't really matter + return redirect("/") + + @login_required @require_POST def accept_follow_request(request): From 2c9ebba5d72adf4fef99291fc07ebe13cb1b36f4 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Tue, 21 Nov 2023 20:13:56 +1100 Subject: [PATCH 2/4] fix reject PR - rationalise activitypub.Reject and fix model being undefined - fix not being able to follow users from followers page: 'delete' option now in user_options dropdown - revert bookwyrm.js - fix delete_follow_request deleting instead of rejecting - add user id to 'remove-follow' path --- bookwyrm/activitypub/verbs.py | 32 ++++-------------- bookwyrm/static/js/bookwyrm.js | 33 ++++++------------- .../templates/snippets/follow_button.html | 14 ++------ .../snippets/remove_follower_button.html | 5 +++ bookwyrm/templates/snippets/user_options.html | 5 +++ bookwyrm/tests/views/test_follow.py | 12 +++++++ bookwyrm/urls.py | 4 ++- bookwyrm/views/follow.py | 11 +++---- 8 files changed, 49 insertions(+), 67 deletions(-) create mode 100644 bookwyrm/templates/snippets/remove_follower_button.html diff --git a/bookwyrm/activitypub/verbs.py b/bookwyrm/activitypub/verbs.py index 2753104b2..a365f4cc0 100644 --- a/bookwyrm/activitypub/verbs.py +++ b/bookwyrm/activitypub/verbs.py @@ -173,35 +173,17 @@ class Reject(Verb): def action(self, allow_external_connections=True): """reject a follow or follow request""" - if self.object.type == "Follow": - model = apps.get_model("bookwyrm.UserFollowRequest") - obj = self.object.to_model( + for model_name in ["UserFollowRequest", "UserFollows", None]: + model = apps.get_model(f"bookwyrm.{model_name}") if model_name else None + if obj := self.object.to_model( model=model, save=False, allow_create=False, allow_external_connections=allow_external_connections, - ) - if not obj: - # This is a deletion (soft-block) of an accepted follow - model = apps.get_model("bookwyrm.UserFollows") - obj = self.object.to_model( - model=model, - save=False, - allow_create=False, - allow_external_connections=allow_external_connections, - ) - else: - # it's something else - obj = self.object.to_model( - model=model, - save=False, - allow_create=False, - allow_external_connections=allow_external_connections, - ) - if not obj: - # if we don't have the object, we can't reject it. - return - obj.reject() + ): + # Reject the first model that can be built. + obj.reject() + break @dataclass(init=False) diff --git a/bookwyrm/static/js/bookwyrm.js b/bookwyrm/static/js/bookwyrm.js index 7093219e5..dcde9cc72 100644 --- a/bookwyrm/static/js/bookwyrm.js +++ b/bookwyrm/static/js/bookwyrm.js @@ -330,30 +330,17 @@ let BookWyrm = new (class { const bookwyrm = this; const form = event.currentTarget; - const formAction = event.submitter.getAttribute("formaction") || form.action; const relatedforms = document.querySelectorAll(`.${form.dataset.id}`); - // Toggle class on all related forms. - if (formAction == "/remove-follow") { - // Remove ALL follow/unfollow/remote buttons - relatedforms.forEach((relatedForm) => relatedForm.classList.add("is-hidden")); + relatedforms.forEach((relatedForm) => + bookwyrm.addRemoveClass( + relatedForm, + "is-hidden", + relatedForm.className.indexOf("is-hidden") == -1 + ) + ); - // Remove orphaned user-options dropdown - const parent = form.parentElement; - const next = parent.nextElementSibling; - - next.classList.add("is-hidden"); - } else { - relatedforms.forEach((relatedForm) => - bookwyrm.addRemoveClass( - relatedForm, - "is-hidden", - relatedForm.className.indexOf("is-hidden") == -1 - ) - ); - } - - this.ajaxPost(formAction, form).catch((error) => { + this.ajaxPost(form).catch((error) => { // @todo Display a notification in the UI instead. console.warn("Request failed:", error); }); @@ -365,8 +352,8 @@ let BookWyrm = new (class { * @param {object} form - Form to be submitted * @return {Promise} */ - ajaxPost(target, form) { - return fetch(target, { + ajaxPost(form) { + return fetch(form.action, { method: "POST", body: new FormData(form), headers: { diff --git a/bookwyrm/templates/snippets/follow_button.html b/bookwyrm/templates/snippets/follow_button.html index 5c0839065..28b979987 100644 --- a/bookwyrm/templates/snippets/follow_button.html +++ b/bookwyrm/templates/snippets/follow_button.html @@ -12,7 +12,6 @@
- {% if not followers_page %} {% csrf_token %} @@ -24,22 +23,13 @@ {% endif %} - {% endif %} -
{% if not minimal %}
- {% include 'snippets/user_options.html' with user=user class="is-small" %} + {% include 'snippets/user_options.html' with user=user followers_page=followers_page class="is-small" %}
{% endif %}
diff --git a/bookwyrm/templates/snippets/remove_follower_button.html b/bookwyrm/templates/snippets/remove_follower_button.html new file mode 100644 index 000000000..28bef6842 --- /dev/null +++ b/bookwyrm/templates/snippets/remove_follower_button.html @@ -0,0 +1,5 @@ +{% load i18n %} + + {% csrf_token %} + + diff --git a/bookwyrm/templates/snippets/user_options.html b/bookwyrm/templates/snippets/user_options.html index 35abc98c2..ab028a494 100644 --- a/bookwyrm/templates/snippets/user_options.html +++ b/bookwyrm/templates/snippets/user_options.html @@ -20,4 +20,9 @@
  • {% include 'snippets/block_button.html' with user=user class="is-fullwidth" blocks=False %}
    • +{% if followers_page %} +
  • + {% include 'snippets/remove_follower_button.html' with user=user class="is-fullwidth" blocks=False %} +
    • +{% endif %} {% endblock %} diff --git a/bookwyrm/tests/views/test_follow.py b/bookwyrm/tests/views/test_follow.py index d18e24f89..8d73a666c 100644 --- a/bookwyrm/tests/views/test_follow.py +++ b/bookwyrm/tests/views/test_follow.py @@ -180,6 +180,18 @@ class FollowViews(TestCase): # follow relationship should not exist self.assertEqual(models.UserFollows.objects.filter(id=rel.id).count(), 0) + def test_handle_reject_existing(self, *_): + """reject a follow previously approved""" + request = self.factory.post("", {"user": self.remote_user.username}) + request.user = self.local_user + rel = models.UserFollows.objects.create( + user_subject=self.remote_user, user_object=self.local_user + ) + with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + views.remove_follow(request, self.remote_user.id) + # follow relationship should not exist + self.assertEqual(models.UserFollows.objects.filter(id=rel.id).count(), 0) + def test_ostatus_follow_request(self, *_): """check ostatus subscribe template loads""" request = self.factory.get( diff --git a/bookwyrm/urls.py b/bookwyrm/urls.py index ab1dca378..41eff3b8c 100644 --- a/bookwyrm/urls.py +++ b/bookwyrm/urls.py @@ -763,7 +763,9 @@ urlpatterns = [ # following re_path(r"^follow/?$", views.follow, name="follow"), re_path(r"^unfollow/?$", views.unfollow, name="unfollow"), - re_path(r"^remove-follow/?$", views.remove_follow, name="remove-follow"), + re_path( + r"^remove-follow/(?P\d+)/?$", views.remove_follow, name="remove-follow" + ), re_path(r"^accept-follow-request/?$", views.accept_follow_request), re_path(r"^delete-follow-request/?$", views.delete_follow_request), re_path(r"^ostatus_follow/?$", views.remote_follow, name="remote-follow"), diff --git a/bookwyrm/views/follow.py b/bookwyrm/views/follow.py index f9a09e2c9..dcb1c695c 100644 --- a/bookwyrm/views/follow.py +++ b/bookwyrm/views/follow.py @@ -71,11 +71,10 @@ def unfollow(request): @login_required @require_POST -def remove_follow(request): +def remove_follow(request, user_id): """remove a previously approved follower without blocking them""" - username = request.POST["user"] - to_remove = get_user_from_username(request.user, username) + to_remove = get_object_or_404(models.User, id=user_id) try: models.UserFollows.objects.get( @@ -93,8 +92,8 @@ def remove_follow(request): if is_api_request(request): return HttpResponse() - # this is handled with ajax so it shouldn't really matter - return redirect("/") + + return redirect(f"{request.user.local_path}/followers") @login_required @@ -128,7 +127,7 @@ def delete_follow_request(request): ) follow_request.raise_not_deletable(request.user) - follow_request.delete() + follow_request.reject() return redirect(f"/user/{request.user.localname}") From 8ed4a997f8f81a4a5be020151fba7f741c8a0b3a Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Tue, 21 Nov 2023 20:20:11 +1100 Subject: [PATCH 3/4] add comment back to bookwyrm.js --- bookwyrm/static/js/bookwyrm.js | 1 + 1 file changed, 1 insertion(+) diff --git a/bookwyrm/static/js/bookwyrm.js b/bookwyrm/static/js/bookwyrm.js index dcde9cc72..a2351a98c 100644 --- a/bookwyrm/static/js/bookwyrm.js +++ b/bookwyrm/static/js/bookwyrm.js @@ -332,6 +332,7 @@ let BookWyrm = new (class { const form = event.currentTarget; const relatedforms = document.querySelectorAll(`.${form.dataset.id}`); + // Toggle class on all related forms. relatedforms.forEach((relatedForm) => bookwyrm.addRemoveClass( relatedForm, From 6ba74181214e253924f022b2e537e7772eaadfe1 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Wed, 22 Nov 2023 20:04:17 +1100 Subject: [PATCH 4/4] improve tests and minor cleanup --- bookwyrm/templates/snippets/user_options.html | 2 +- bookwyrm/tests/views/test_follow.py | 18 ++++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/bookwyrm/templates/snippets/user_options.html b/bookwyrm/templates/snippets/user_options.html index ab028a494..0e15e413a 100644 --- a/bookwyrm/templates/snippets/user_options.html +++ b/bookwyrm/templates/snippets/user_options.html @@ -22,7 +22,7 @@ {% if followers_page %}
  • - {% include 'snippets/remove_follower_button.html' with user=user class="is-fullwidth" blocks=False %} + {% include 'snippets/remove_follower_button.html' with user=user class="is-fullwidth" %}
    • {% endif %} {% endblock %} diff --git a/bookwyrm/tests/views/test_follow.py b/bookwyrm/tests/views/test_follow.py index 8d73a666c..d88a42210 100644 --- a/bookwyrm/tests/views/test_follow.py +++ b/bookwyrm/tests/views/test_follow.py @@ -173,8 +173,15 @@ class FollowViews(TestCase): user_subject=self.remote_user, user_object=self.local_user ) - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ) as broadcast_mock: views.delete_follow_request(request) + # did we send the reject activity? + activity = json.loads(broadcast_mock.call_args[1]["args"][1]) + self.assertEqual(activity["actor"], self.local_user.remote_id) + self.assertEqual(activity["object"]["object"], rel.user_object.remote_id) + self.assertEqual(activity["type"], "Reject") # request should be deleted self.assertEqual(models.UserFollowRequest.objects.filter(id=rel.id).count(), 0) # follow relationship should not exist @@ -187,8 +194,15 @@ class FollowViews(TestCase): rel = models.UserFollows.objects.create( user_subject=self.remote_user, user_object=self.local_user ) - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ) as broadcast_mock: views.remove_follow(request, self.remote_user.id) + # did we send the reject activity? + activity = json.loads(broadcast_mock.call_args[1]["args"][1]) + self.assertEqual(activity["actor"], self.local_user.remote_id) + self.assertEqual(activity["object"]["object"], rel.user_object.remote_id) + self.assertEqual(activity["type"], "Reject") # follow relationship should not exist self.assertEqual(models.UserFollows.objects.filter(id=rel.id).count(), 0)