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

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

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