Fix existing posts not being removed from lists when a list member is unfollowed (#38048)

app/services/unfollow_service.rb

@@ -6,16 +6,16 @@ class UnfollowService < BaseService
   include Lockable
 
   # Unfollow and notify the remote user
-  # @param [Account] source_account Where to unfollow from
+  # @param [Account] follower Where to unfollow from
-  # @param [Account] target_account Which to unfollow
+  # @param [Account] followee Which to unfollow
   # @param [Hash] options
   # @option [Boolean] :skip_unmerge
-  def call(source_account, target_account, options = {})
+  def call(follower, followee, options = {})
-    @source_account = source_account
+    @follower = follower
-    @target_account = target_account
+    @followee = followee
-    @options        = options
+    @options  = options
 
-    with_redis_lock("relationship:#{[source_account.id, target_account.id].sort.join(':')}") do
+    with_redis_lock("relationship:#{[follower.id, followee.id].sort.join(':')}") do
       unfollow! || undo_follow_request!
     end
   end
@@ -23,19 +23,25 @@ class UnfollowService < BaseService
   private
 
   def unfollow!
-    follow = Follow.find_by(account: @source_account, target_account: @target_account)
+    follow = Follow.find_by(account: @follower, target_account: @followee)
-
     return unless follow
 
+    # List members are removed immediately with the follow relationship removal,
+    # so we need to fetch the list IDs first
+    list_ids = @follower.owned_lists.with_list_account(@followee).pluck(:list_id) unless @options[:skip_unmerge]
+
     follow.destroy!
 
-    create_notification(follow) if !@target_account.local? && @target_account.activitypub?
+    if @followee.local? && @follower.remote? && @follower.activitypub?
-    create_reject_notification(follow) if @target_account.local? && !@source_account.local? && @source_account.activitypub?
+      send_reject_follow(follow)
+    elsif @followee.remote? && @followee.activitypub?
+      send_undo_follow(follow)
+    end
 
     unless @options[:skip_unmerge]
-      UnmergeWorker.perform_async(@target_account.id, @source_account.id, 'home')
+      UnmergeWorker.perform_async(@followee.id, @follower.id, 'home')
-      UnmergeWorker.push_bulk(@source_account.owned_lists.with_list_account(@target_account).pluck(:list_id)) do |list_id|
+      UnmergeWorker.push_bulk(list_ids) do |list_id|
-        [@target_account.id, list_id, 'list']
+        [@followee.id, list_id, 'list']
       end
     end
 
@@ -43,22 +49,21 @@ class UnfollowService < BaseService
   end
 
   def undo_follow_request!
-    follow_request = FollowRequest.find_by(account: @source_account, target_account: @target_account)
+    follow_request = FollowRequest.find_by(account: @follower, target_account: @followee)
-
     return unless follow_request
 
     follow_request.destroy!
 
-    create_notification(follow_request) unless @target_account.local?
+    send_undo_follow(follow_request) unless @followee.local?
 
     follow_request
   end
 
-  def create_notification(follow)
+  def send_undo_follow(follow)
     ActivityPub::DeliveryWorker.perform_async(build_json(follow), follow.account_id, follow.target_account.inbox_url)
   end
 
-  def create_reject_notification(follow)
+  def send_reject_follow(follow)
     ActivityPub::DeliveryWorker.perform_async(build_reject_json(follow), follow.target_account_id, follow.account.inbox_url)
   end
 

spec/services/unfollow_service_spec.rb

@@ -5,54 +5,57 @@ require 'rails_helper'
 RSpec.describe UnfollowService do
   subject { described_class.new }
 
-  let(:sender) { Fabricate(:account, username: 'alice') }
+  let(:follower) { Fabricate(:account) }
+  let(:followee) { Fabricate(:account) }
 
-  describe 'local' do
+  before do
-    let(:bob) { Fabricate(:account, username: 'bob') }
+    follower.follow!(followee)
+  end
 
-    before { sender.follow!(bob) }
+  shared_examples 'when the followee is in a list' do
+    let(:list) { Fabricate(:list, account: follower) }
 
-    it 'destroys the following relation' do
+    before do
-      subject.call(sender, bob)
+      list.accounts << followee
+    end
 
-      expect(sender)
+    it 'schedules removal of posts from this user from the list' do
-        .to_not be_following(bob)
+      expect { subject.call(follower, followee) }
+        .to enqueue_sidekiq_job(UnmergeWorker).with(followee.id, list.id, 'list')
     end
   end
 
-  describe 'remote ActivityPub', :inline_jobs do
+  describe 'a local user unfollowing another local user' do
-    let(:bob) { Fabricate(:account, username: 'bob', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') }
+    it 'destroys the following relation and unmerge from home' do
-
+      expect { subject.call(follower, followee) }
-    before do
+        .to change { follower.following?(followee) }.from(true).to(false)
-      sender.follow!(bob)
+        .and enqueue_sidekiq_job(UnmergeWorker).with(followee.id, follower.id, 'home')
-      stub_request(:post, 'http://example.com/inbox').to_return(status: 200)
     end
 
-    it 'destroys the following relation and sends unfollow activity' do
+    it_behaves_like 'when the followee is in a list'
-      subject.call(sender, bob)
-
-      expect(sender)
-        .to_not be_following(bob)
-      expect(a_request(:post, 'http://example.com/inbox'))
-        .to have_been_made.once
-    end
   end
 
-  describe 'remote ActivityPub (reverse)', :inline_jobs do
+  describe 'a local user unfollowing a remote ActivityPub user' do
-    let(:bob) { Fabricate(:account, username: 'bob', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') }
+    let(:followee) { Fabricate(:account, username: 'bob', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') }
 
-    before do
+    it 'destroys the following relation, unmerge from home and sends undo activity' do
-      bob.follow!(sender)
+      expect { subject.call(follower, followee) }
-      stub_request(:post, 'http://example.com/inbox').to_return(status: 200)
+        .to change { follower.following?(followee) }.from(true).to(false)
+        .and enqueue_sidekiq_job(UnmergeWorker).with(followee.id, follower.id, 'home')
+        .and enqueue_sidekiq_job(ActivityPub::DeliveryWorker).with(match_json_values(type: 'Undo'), follower.id, followee.inbox_url)
     end
 
-    it 'destroys the following relation and sends a reject activity' do
+    it_behaves_like 'when the followee is in a list'
-      subject.call(bob, sender)
+  end
 
-      expect(sender)
+  describe 'a remote ActivityPub user unfollowing a local user' do
-        .to_not be_following(bob)
+    let(:follower) { Fabricate(:account, username: 'bob', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') }
-      expect(a_request(:post, 'http://example.com/inbox'))
+
-        .to have_been_made.once
+    it 'destroys the following relation, unmerge from home and sends a reject activity' do
+      expect { subject.call(follower, followee) }
+        .to change { follower.following?(followee) }.from(true).to(false)
+        .and enqueue_sidekiq_job(UnmergeWorker).with(followee.id, follower.id, 'home')
+        .and enqueue_sidekiq_job(ActivityPub::DeliveryWorker).with(match_json_values(type: 'Reject'), followee.id, follower.inbox_url)
     end
   end
 end