Don't persist local undone follow (#194)

same deal but backwards this time

Co-authored-by: FloatingGhost <hannah@coffee-and-dreams.uk>
Reviewed-on: https://akkoma.dev/AkkomaGang/akkoma/pulls/194

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
     }

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
   """

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

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

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 Enum.count(Notification.for_user(followed_user)) == 1
-      assert [%{id: ^notification_id}] = Notification.for_user(followed_user)
     end
 
     test "dismisses the notification on follow request rejection" do

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)
+      follow_activity = Activity.get_by_id(follow_activity.id)
-      assert is_nil(activity)
+      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
 

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)

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"}} =
+      assert is_nil(Pleroma.Web.ActivityPub.Utils.fetch_latest_follow(follower, followed))
-               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"}} =
+      assert is_nil(Pleroma.Web.ActivityPub.Utils.fetch_latest_follow(follower, followed))
-               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