From 8857cabca42fc21d3063862c9954e079e7efd275 Mon Sep 17 00:00:00 2001
From: Matt Jankowski <mjankowski@thoughtbot.com>
Date: Wed, 26 Apr 2017 14:09:01 -0400
Subject: [PATCH] Domain block service cleanup (#2490)

* Add coverage for domain block service with silence

* Get rid of warning about find_each and order

* Move domain_block to attr_reader

* Move optional clear_media into silence_accounts method

* Use blocked_domain method to reduce passed vars

* Extract blocked_domain_accounts method to find accounts on the domain

* Extract media_from_blocked_domain method to find relevant attachments

* Separate destruction of account images and account attachments
---
 app/services/block_domain_service.rb       | 66 +++++++++++++++-------
 spec/services/block_domain_service_spec.rb | 47 +++++++++++----
 2 files changed, 82 insertions(+), 31 deletions(-)

diff --git a/app/services/block_domain_service.rb b/app/services/block_domain_service.rb
index 97d2ebcd76..5d13cfc0e6 100644
--- a/app/services/block_domain_service.rb
+++ b/app/services/block_domain_service.rb
@@ -1,36 +1,62 @@
 # frozen_string_literal: true
 
 class BlockDomainService < BaseService
+  attr_reader :domain_block
+
   def call(domain_block)
-    if domain_block.silence?
-      silence_accounts!(domain_block.domain)
-      clear_media!(domain_block.domain) if domain_block.reject_media?
-    else
-      suspend_accounts!(domain_block.domain)
-    end
+    @domain_block = domain_block
+    process_domain_block
   end
 
   private
 
-  def silence_accounts!(domain)
-    Account.where(domain: domain).update_all(silenced: true)
-  end
-
-  def clear_media!(domain)
-    Account.where(domain: domain).find_each do |account|
-      account.avatar.destroy
-      account.header.destroy
-    end
-
-    MediaAttachment.where(account: Account.where(domain: domain)).find_each do |attachment|
-      attachment.file.destroy
+  def process_domain_block
+    if domain_block.silence?
+      silence_accounts!
+    else
+      suspend_accounts!
     end
   end
 
-  def suspend_accounts!(domain)
-    Account.where(domain: domain).where(suspended: false).find_each do |account|
+  def silence_accounts!
+    blocked_domain_accounts.update_all(silenced: true)
+    clear_media! if domain_block.reject_media?
+  end
+
+  def clear_media!
+    clear_account_images
+    clear_account_attachments
+  end
+
+  def suspend_accounts!
+    blocked_domain_accounts.where(suspended: false).find_each do |account|
       account.subscription(api_subscription_url(account.id)).unsubscribe if account.subscribed?
       SuspendAccountService.new.call(account)
     end
   end
+
+  def clear_account_images
+    blocked_domain_accounts.find_each do |account|
+      account.avatar.destroy
+      account.header.destroy
+    end
+  end
+
+  def clear_account_attachments
+    media_from_blocked_domain.find_each do |attachment|
+      attachment.file.destroy
+    end
+  end
+
+  def blocked_domain
+    domain_block.domain
+  end
+
+  def blocked_domain_accounts
+    Account.where(domain: blocked_domain)
+  end
+
+  def media_from_blocked_domain
+    MediaAttachment.where(account: blocked_domain_accounts).reorder(nil)
+  end
 end
diff --git a/spec/services/block_domain_service_spec.rb b/spec/services/block_domain_service_spec.rb
index 8e71d45429..5c2cfc8c70 100644
--- a/spec/services/block_domain_service_spec.rb
+++ b/spec/services/block_domain_service_spec.rb
@@ -13,21 +13,46 @@ RSpec.describe BlockDomainService do
     bad_status1
     bad_status2
     bad_attachment
-
-    subject.call(DomainBlock.create!(domain: 'evil.org', severity: :suspend))
   end
 
-  it 'creates a domain block' do
-    expect(DomainBlock.blocked?('evil.org')).to be true
+  describe 'for a suspension' do
+    before do
+      subject.call(DomainBlock.create!(domain: 'evil.org', severity: :suspend))
+    end
+
+    it 'creates a domain block' do
+      expect(DomainBlock.blocked?('evil.org')).to be true
+    end
+
+    it 'removes remote accounts from that domain' do
+      expect(Account.find_remote('badguy666', 'evil.org').suspended?).to be true
+    end
+
+    it 'removes the remote accounts\'s statuses and media attachments' do
+      expect { bad_status1.reload }.to raise_exception ActiveRecord::RecordNotFound
+      expect { bad_status2.reload }.to raise_exception ActiveRecord::RecordNotFound
+      expect { bad_attachment.reload }.to raise_exception ActiveRecord::RecordNotFound
+    end
   end
 
-  it 'removes remote accounts from that domain' do
-    expect(Account.find_remote('badguy666', 'evil.org').suspended?).to be true
-  end
+  describe 'for a silence with reject media' do
+    before do
+      subject.call(DomainBlock.create!(domain: 'evil.org', severity: :silence, reject_media: true))
+    end
 
-  it 'removes the remote accounts\'s statuses and media attachments' do
-    expect { bad_status1.reload }.to raise_exception ActiveRecord::RecordNotFound
-    expect { bad_status2.reload }.to raise_exception ActiveRecord::RecordNotFound
-    expect { bad_attachment.reload }.to raise_exception ActiveRecord::RecordNotFound
+    it 'does not create a domain block' do
+      expect(DomainBlock.blocked?('evil.org')).to be false
+    end
+
+    it 'silences remote accounts from that domain' do
+      expect(Account.find_remote('badguy666', 'evil.org').silenced?).to be true
+    end
+
+    it 'leaves the domains status and attachements, but clears media' do
+      expect { bad_status1.reload }.not_to raise_error
+      expect { bad_status2.reload }.not_to raise_error
+      expect { bad_attachment.reload }.not_to raise_error
+      expect(bad_attachment.file.exists?).to be false
+    end
   end
 end