diff --git a/app/services/unfollow_service.rb b/app/services/unfollow_service.rb index 1ea8af6992..385aa0c7b1 100644 --- a/app/services/unfollow_service.rb +++ b/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] target_account Which to unfollow + # @param [Account] follower Where to unfollow from + # @param [Account] followee Which to unfollow # @param [Hash] options # @option [Boolean] :skip_unmerge - def call(source_account, target_account, options = {}) - @source_account = source_account - @target_account = target_account - @options = options + def call(follower, followee, options = {}) + @follower = follower + @followee = followee + @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? - create_reject_notification(follow) if @target_account.local? && !@source_account.local? && @source_account.activitypub? + if @followee.local? && @follower.remote? && @follower.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.push_bulk(@source_account.owned_lists.with_list_account(@target_account).pluck(:list_id)) do |list_id| - [@target_account.id, list_id, 'list'] + UnmergeWorker.perform_async(@followee.id, @follower.id, 'home') + UnmergeWorker.push_bulk(list_ids) do |list_id| + [@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 diff --git a/spec/services/unfollow_service_spec.rb b/spec/services/unfollow_service_spec.rb index 6cf24ca5e1..365468e432 100644 --- a/spec/services/unfollow_service_spec.rb +++ b/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 - let(:bob) { Fabricate(:account, username: 'bob') } + before do + 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 - subject.call(sender, bob) + before do + list.accounts << followee + end - expect(sender) - .to_not be_following(bob) + it 'schedules removal of posts from this user from the list' do + expect { subject.call(follower, followee) } + .to enqueue_sidekiq_job(UnmergeWorker).with(followee.id, list.id, 'list') end end - describe 'remote ActivityPub', :inline_jobs do - let(:bob) { Fabricate(:account, username: 'bob', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') } - - before do - sender.follow!(bob) - stub_request(:post, 'http://example.com/inbox').to_return(status: 200) + describe 'a local user unfollowing another local user' do + it 'destroys the following relation and unmerge from home' 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') end - it 'destroys the following relation and sends unfollow activity' do - 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 + it_behaves_like 'when the followee is in a list' end - describe 'remote ActivityPub (reverse)', :inline_jobs do - let(:bob) { Fabricate(:account, username: 'bob', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') } + describe 'a local user unfollowing a remote ActivityPub user' do + let(:followee) { Fabricate(:account, username: 'bob', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') } - before do - bob.follow!(sender) - stub_request(:post, 'http://example.com/inbox').to_return(status: 200) + it 'destroys the following relation, unmerge from home and sends undo 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: 'Undo'), follower.id, followee.inbox_url) end - it 'destroys the following relation and sends a reject activity' do - subject.call(bob, sender) + it_behaves_like 'when the followee is in a list' + end - expect(sender) - .to_not be_following(bob) - expect(a_request(:post, 'http://example.com/inbox')) - .to have_been_made.once + describe 'a remote ActivityPub user unfollowing a local user' do + let(:follower) { Fabricate(:account, username: 'bob', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') } + + 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