From 8e4de118c1f108437c43c2bdbca67d1084d69ba5 Mon Sep 17 00:00:00 2001 From: floatingghost Date: Wed, 31 Aug 2022 18:00:36 +0000 Subject: [PATCH] Don't persist local undone follow (#194) same deal but backwards this time Co-authored-by: FloatingGhost Reviewed-on: https://akkoma.dev/AkkomaGang/akkoma/pulls/194 --- lib/pleroma/web/activity_pub/activity_pub.ex | 8 +++--- lib/pleroma/web/activity_pub/utils.ex | 12 --------- ...1170605_remove_local_cancelled_follows.exs | 22 ++++++++++++++++ test/mix/tasks/pleroma/relay_test.exs | 6 ++--- test/pleroma/notification_test.exs | 5 ++-- .../web/activity_pub/activity_pub_test.exs | 26 +++++++++++++++++-- test/pleroma/web/activity_pub/utils_test.exs | 23 ---------------- test/pleroma/web/common_api_test.exs | 16 +++++------- 8 files changed, 61 insertions(+), 57 deletions(-) create mode 100644 priv/repo/migrations/20220831170605_remove_local_cancelled_follows.exs diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 03e72be58..20acdf86e 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -331,9 +331,9 @@ defp do_unfollow(follower, followed, activity_id, local) defp do_unfollow(follower, followed, activity_id, local) when local == true do with %Activity{} = follow_activity <- fetch_latest_follow(follower, followed), - {:ok, follow_activity} <- update_follow_state(follow_activity, "cancelled"), unfollow_data <- make_unfollow_data(follower, followed, follow_activity, activity_id), {:ok, activity} <- insert(unfollow_data, local), + {:ok, _activity} <- Repo.delete(follow_activity), _ <- notify_and_stream(activity), :ok <- maybe_federate(activity) do {:ok, activity} @@ -349,7 +349,7 @@ defp do_unfollow(follower, followed, activity_id, false) do with %Activity{} = follow_activity <- fetch_latest_follow(follower, followed), {:ok, _activity} <- Repo.delete(follow_activity), unfollow_data <- make_unfollow_data(follower, followed, follow_activity, activity_id), - unfollow_activity <- remote_unfollow_data(unfollow_data), + unfollow_activity <- make_unfollow_activity(unfollow_data, false), _ <- notify_and_stream(unfollow_activity) do {:ok, unfollow_activity} else @@ -358,12 +358,12 @@ defp do_unfollow(follower, followed, activity_id, false) do end end - defp remote_unfollow_data(data) do + defp make_unfollow_activity(data, local) do {recipients, _, _} = get_recipients(data) %Activity{ data: data, - local: false, + local: local, actor: data["actor"], recipients: recipients } diff --git a/lib/pleroma/web/activity_pub/utils.ex b/lib/pleroma/web/activity_pub/utils.ex index 5e5df4888..b920e8c1d 100644 --- a/lib/pleroma/web/activity_pub/utils.ex +++ b/lib/pleroma/web/activity_pub/utils.ex @@ -472,18 +472,6 @@ def update_follow_state_for_all( {:ok, activity} end - def update_follow_state( - %Activity{} = activity, - state - ) do - new_data = Map.put(activity.data, "state", state) - changeset = Changeset.change(activity, data: new_data) - - with {:ok, activity} <- Repo.update(changeset) do - {:ok, activity} - end - end - @doc """ Makes a follow activity data for the given follower and followed """ diff --git a/priv/repo/migrations/20220831170605_remove_local_cancelled_follows.exs b/priv/repo/migrations/20220831170605_remove_local_cancelled_follows.exs new file mode 100644 index 000000000..16597f848 --- /dev/null +++ b/priv/repo/migrations/20220831170605_remove_local_cancelled_follows.exs @@ -0,0 +1,22 @@ +defmodule Pleroma.Repo.Migrations.RemoveLocalCancelledFollows do + use Ecto.Migration + + def up do + statement = """ + DELETE FROM + activities + WHERE + (data->>'type') = 'Follow' + AND + (data->>'state') = 'cancelled' + AND + local = true; + """ + + execute(statement) + end + + def down do + :ok + end +end diff --git a/test/mix/tasks/pleroma/relay_test.exs b/test/mix/tasks/pleroma/relay_test.exs index db75b3630..d45690418 100644 --- a/test/mix/tasks/pleroma/relay_test.exs +++ b/test/mix/tasks/pleroma/relay_test.exs @@ -65,7 +65,7 @@ test "relay is unfollowed" do Mix.Tasks.Pleroma.Relay.run(["unfollow", target_instance]) cancelled_activity = Activity.get_by_ap_id(follow_activity.data["id"]) - assert cancelled_activity.data["state"] == "cancelled" + assert is_nil(cancelled_activity) [undo_activity] = ActivityPub.fetch_activities([], %{ @@ -78,7 +78,6 @@ test "relay is unfollowed" do assert undo_activity.data["type"] == "Undo" assert undo_activity.data["actor"] == local_user.ap_id - assert undo_activity.data["object"]["id"] == cancelled_activity.data["id"] refute "#{target_instance}/followers" in User.following(local_user) end @@ -142,7 +141,7 @@ test "force unfollow when relay is dead" do Mix.Tasks.Pleroma.Relay.run(["unfollow", target_instance, "--force"]) cancelled_activity = Activity.get_by_ap_id(follow_activity.data["id"]) - assert cancelled_activity.data["state"] == "cancelled" + assert is_nil(cancelled_activity) [undo_activity] = ActivityPub.fetch_activities( @@ -152,7 +151,6 @@ test "force unfollow when relay is dead" do assert undo_activity.data["type"] == "Undo" assert undo_activity.data["actor"] == local_user.ap_id - assert undo_activity.data["object"]["id"] == cancelled_activity.data["id"] refute "#{target_instance}/followers" in User.following(local_user) end end diff --git a/test/pleroma/notification_test.exs b/test/pleroma/notification_test.exs index 4354dd2b6..8db208878 100644 --- a/test/pleroma/notification_test.exs +++ b/test/pleroma/notification_test.exs @@ -427,13 +427,12 @@ test "it doesn't create a notification for follow-unfollow-follow chains" do {:ok, _, _, _activity} = CommonAPI.follow(user, followed_user) assert FollowingRelationship.following?(user, followed_user) - assert [notification] = Notification.for_user(followed_user) + assert [_notification] = Notification.for_user(followed_user) CommonAPI.unfollow(user, followed_user) {:ok, _, _, _activity_dupe} = CommonAPI.follow(user, followed_user) - notification_id = notification.id - assert [%{id: ^notification_id}] = Notification.for_user(followed_user) + assert Enum.count(Notification.for_user(followed_user)) == 1 end test "dismisses the notification on follow request rejection" do diff --git a/test/pleroma/web/activity_pub/activity_pub_test.exs b/test/pleroma/web/activity_pub/activity_pub_test.exs index 50eff9431..ec562ac7b 100644 --- a/test/pleroma/web/activity_pub/activity_pub_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_test.exs @@ -1373,6 +1373,25 @@ test "creates an undo activity for a pending follow request" do assert embedded_object["id"] == follow_activity.data["id"] end + test "it removes the follow activity if it was local" do + follower = insert(:user, local: true) + followed = insert(:user) + + {:ok, _, _, follow_activity} = CommonAPI.follow(follower, followed) + {:ok, activity} = ActivityPub.unfollow(follower, followed, nil, true) + + assert activity.data["type"] == "Undo" + assert activity.data["actor"] == follower.ap_id + + follow_activity = Activity.get_by_id(follow_activity.id) + assert is_nil(follow_activity) + assert is_nil(Utils.fetch_latest_follow(follower, followed)) + + # We need to keep our own undo + undo_activity = Activity.get_by_ap_id(activity.data["id"]) + refute is_nil(undo_activity) + end + test "it removes the follow activity if it was remote" do follower = insert(:user, local: false) followed = insert(:user) @@ -1383,9 +1402,12 @@ test "it removes the follow activity if it was remote" do assert activity.data["type"] == "Undo" assert activity.data["actor"] == follower.ap_id - activity = Activity.get_by_id(follow_activity.id) - assert is_nil(activity) + follow_activity = Activity.get_by_id(follow_activity.id) + assert is_nil(follow_activity) assert is_nil(Utils.fetch_latest_follow(follower, followed)) + + undo_activity = Activity.get_by_ap_id(activity.data["id"]) + assert is_nil(undo_activity) end end diff --git a/test/pleroma/web/activity_pub/utils_test.exs b/test/pleroma/web/activity_pub/utils_test.exs index 0d88303e3..e45af3aec 100644 --- a/test/pleroma/web/activity_pub/utils_test.exs +++ b/test/pleroma/web/activity_pub/utils_test.exs @@ -229,29 +229,6 @@ test "also updates the state of accepted follows" do end end - describe "update_follow_state/2" do - test "updates the state of the given follow activity" do - user = insert(:user, is_locked: true) - follower = insert(:user) - - {:ok, _, _, follow_activity} = CommonAPI.follow(follower, user) - {:ok, _, _, follow_activity_two} = CommonAPI.follow(follower, user) - - data = - follow_activity_two.data - |> Map.put("state", "accept") - - cng = Ecto.Changeset.change(follow_activity_two, data: data) - - {:ok, follow_activity_two} = Repo.update(cng) - - {:ok, follow_activity_two} = Utils.update_follow_state(follow_activity_two, "reject") - - assert refresh_record(follow_activity).data["state"] == "pending" - assert refresh_record(follow_activity_two).data["state"] == "reject" - end - end - describe "update_element_in_object/3" do test "updates likes" do user = insert(:user) diff --git a/test/pleroma/web/common_api_test.exs b/test/pleroma/web/common_api_test.exs index fa751bf60..840d74d2f 100644 --- a/test/pleroma/web/common_api_test.exs +++ b/test/pleroma/web/common_api_test.exs @@ -1058,24 +1058,23 @@ test "also unsubscribes a user" do refute User.subscribed_to?(follower, followed) end - test "cancels a pending follow for a local user" do + test "removes a pending follow for a local user" do follower = insert(:user) followed = insert(:user, is_locked: true) - assert {:ok, follower, followed, %{id: activity_id, data: %{"state" => "pending"}}} = + assert {:ok, follower, followed, %{id: _activity_id, data: %{"state" => "pending"}}} = CommonAPI.follow(follower, followed) assert User.get_follow_state(follower, followed) == :follow_pending assert {:ok, follower} = CommonAPI.unfollow(follower, followed) assert User.get_follow_state(follower, followed) == nil - assert %{id: ^activity_id, data: %{"state" => "cancelled"}} = - Pleroma.Web.ActivityPub.Utils.fetch_latest_follow(follower, followed) + assert is_nil(Pleroma.Web.ActivityPub.Utils.fetch_latest_follow(follower, followed)) assert %{ data: %{ "type" => "Undo", - "object" => %{"type" => "Follow", "state" => "cancelled"} + "object" => %{"type" => "Follow"} } } = Pleroma.Web.ActivityPub.Utils.fetch_latest_undo(follower) end @@ -1084,20 +1083,19 @@ test "cancels a pending follow for a remote user" do follower = insert(:user) followed = insert(:user, is_locked: true, local: false, ap_enabled: true) - assert {:ok, follower, followed, %{id: activity_id, data: %{"state" => "pending"}}} = + assert {:ok, follower, followed, %{id: _activity_id, data: %{"state" => "pending"}}} = CommonAPI.follow(follower, followed) assert User.get_follow_state(follower, followed) == :follow_pending assert {:ok, follower} = CommonAPI.unfollow(follower, followed) assert User.get_follow_state(follower, followed) == nil - assert %{id: ^activity_id, data: %{"state" => "cancelled"}} = - Pleroma.Web.ActivityPub.Utils.fetch_latest_follow(follower, followed) + assert is_nil(Pleroma.Web.ActivityPub.Utils.fetch_latest_follow(follower, followed)) assert %{ data: %{ "type" => "Undo", - "object" => %{"type" => "Follow", "state" => "cancelled"} + "object" => %{"type" => "Follow"} } } = Pleroma.Web.ActivityPub.Utils.fetch_latest_undo(follower) end