From ce812466c749c6abb45e57efe24b01e0f511e0b5 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sun, 11 Jun 2017 17:13:43 +0200 Subject: [PATCH] Fix removal of status sending the original status to mentioned users instead of delete Salmon (#3672) * Fix removal of status sending the original status to mentioned users instead of delete Salmon, add test * Create remove_status_service_spec.rb --- app/services/remove_status_service.rb | 75 +++++++++++---------- spec/services/remove_status_service_spec.rb | 40 +++++++++++ 2 files changed, 78 insertions(+), 37 deletions(-) create mode 100644 spec/services/remove_status_service_spec.rb diff --git a/app/services/remove_status_service.rb b/app/services/remove_status_service.rb index 50bb7fc97e8..92077415615 100644 --- a/app/services/remove_status_service.rb +++ b/app/services/remove_status_service.rb @@ -4,56 +4,57 @@ class RemoveStatusService < BaseService include StreamEntryRenderer def call(status) - @payload = Oj.dump(event: :delete, payload: status.id) + @payload = Oj.dump(event: :delete, payload: status.id) + @status = status + @account = status.account + @tags = status.tags.pluck(:name).to_a + @mentions = status.mentions.includes(:account).to_a + @reblogs = status.reblogs.to_a + @stream_entry = status.stream_entry - remove_from_self(status) if status.account.local? - remove_from_followers(status) - remove_from_mentioned(status) - remove_reblogs(status) - remove_from_hashtags(status) - remove_from_public(status) + remove_from_self if status.account.local? + remove_from_followers + remove_reblogs + remove_from_hashtags + remove_from_public - status.destroy! + @status.destroy! - return unless status.account.local? + return unless @account.local? - Pubsubhubbub::DistributionWorker.perform_async(status.stream_entry.id) + remove_from_mentioned(@stream_entry.reload) + Pubsubhubbub::DistributionWorker.perform_async(@stream_entry.id) end private - def remove_from_self(status) - unpush(:home, status.account, status) + def remove_from_self + unpush(:home, @account, @status) end - def remove_from_followers(status) - status.account.followers.where(domain: nil).each do |follower| - unpush(:home, follower, status) + def remove_from_followers + redis.pipelined do + @account.followers.local.find_each do |follower| + unpush(:home, follower, @status) + end end end - def remove_from_mentioned(status) - return unless status.local? - notified_domains = [] + def remove_from_mentioned(stream_entry) + salmon_xml = stream_entry_to_xml(stream_entry) + target_accounts = @mentions.map(&:account).reject(&:local?).uniq(&:domain) - status.mentions.each do |mention| - mentioned_account = mention.account - - next if mentioned_account.local? - next if notified_domains.include?(mentioned_account.domain) - - notified_domains << mentioned_account.domain - send_delete_salmon(mentioned_account, status) + NotificationWorker.push_bulk(target_accounts) do |target_account| + [salmon_xml, stream_entry.account_id, target_account.id] end end - def send_delete_salmon(account, status) - return unless status.local? - NotificationWorker.perform_async(stream_entry_to_xml(status.stream_entry), status.account_id, account.id) - end + def remove_reblogs + # We delete reblogs of the status before the original status, + # because once original status is gone, reblogs will disappear + # without us being able to do all the fancy stuff - def remove_reblogs(status) - status.reblogs.each do |reblog| + @reblogs.each do |reblog| RemoveStatusService.new.call(reblog) end end @@ -68,16 +69,16 @@ class RemoveStatusService < BaseService Redis.current.publish("timeline:#{receiver.id}", @payload) end - def remove_from_hashtags(status) - status.tags.pluck(:name) do |hashtag| + def remove_from_hashtags + @tags.each do |hashtag| Redis.current.publish("timeline:hashtag:#{hashtag}", @payload) - Redis.current.publish("timeline:hashtag:#{hashtag}:local", @payload) if status.local? + Redis.current.publish("timeline:hashtag:#{hashtag}:local", @payload) if @status.local? end end - def remove_from_public(status) + def remove_from_public Redis.current.publish('timeline:public', @payload) - Redis.current.publish('timeline:public:local', @payload) if status.local? + Redis.current.publish('timeline:public:local', @payload) if @status.local? end def redis diff --git a/spec/services/remove_status_service_spec.rb b/spec/services/remove_status_service_spec.rb new file mode 100644 index 00000000000..a3bce761339 --- /dev/null +++ b/spec/services/remove_status_service_spec.rb @@ -0,0 +1,40 @@ +require 'rails_helper' + +RSpec.describe RemoveStatusService do + subject { RemoveStatusService.new } + + let!(:alice) { Fabricate(:account) } + let!(:bob) { Fabricate(:account, username: 'bob', domain: 'example.com', salmon_url: 'http://example.com/salmon') } + let!(:jeff) { Fabricate(:account) } + + before do + stub_request(:post, 'http://example.com/push').to_return(status: 200, body: '', headers: {}) + stub_request(:post, 'http://example.com/salmon').to_return(status: 200, body: '', headers: {}) + + Fabricate(:subscription, account: alice, callback_url: 'http://example.com/push', confirmed: true, expires_at: 30.days.from_now) + jeff.follow!(alice) + @status = PostStatusService.new.call(alice, 'Hello @bob@example.com') + subject.call(@status) + end + + it 'removes status from author\'s home feed' do + expect(Feed.new(:home, alice).get(10)).to_not include(@status.id) + end + + it 'removes status from local follower\'s home feed' do + expect(Feed.new(:home, jeff).get(10)).to_not include(@status.id) + end + + it 'sends PuSH update to PuSH subscribers' do + expect(a_request(:post, 'http://example.com/push').with { |req| + req.body.match(TagManager::VERBS[:delete]) + }).to have_been_made + end + + it 'sends Salmon slap to previously mentioned users' do + expect(a_request(:post, "http://example.com/salmon").with { |req| + xml = OStatus2::Salmon.new.unpack(req.body) + xml.match(TagManager::VERBS[:delete]) + }).to have_been_made.once + end +end