Implement final review comments from @DouweM and @rymai

- Have `Uniquify` take a block instead of a Proc/function. This is more
  idiomatic than passing around a function in Ruby.

- Block a user before moving their issues to the ghost user. This avoids a data
  race where an issue is created after the issues are migrated to the ghost user,
  and before the destroy takes place.

- No need to migrate issues (to the ghost user) in a transaction, because
  we're using `update_all`

- Other minor changes

2 files changed, 12 insertions, 15 deletions

diff --git a/spec/models/concerns/uniquify_spec.rb b/spec/models/concerns/uniquify_spec.rb
index 4e06a1f4c31..83187d732e4 100644
--- a/spec/models/concerns/uniquify_spec.rb
+++ b/spec/models/concerns/uniquify_spec.rb
@@ -1,38 +1,31 @@
 require 'spec_helper'
 
 describe Uniquify, models: true do
+  let(:uniquify) { described_class.new }
+
   describe "#string" do
     it 'returns the given string if it does not exist' do
-      uniquify = Uniquify.new
-
-      result = uniquify.string('test_string', -> (s) { false })
+      result = uniquify.string('test_string') { |s| false }
 
       expect(result).to eq('test_string')
     end
 
     it 'returns the given string with a counter attached if the string exists' do
-      uniquify = Uniquify.new
-
-      result = uniquify.string('test_string', -> (s) { true if s == 'test_string' })
+      result = uniquify.string('test_string') { |s| s == 'test_string' }
 
       expect(result).to eq('test_string1')
     end
 
     it 'increments the counter for each candidate string that also exists' do
-      uniquify = Uniquify.new
-
-      result = uniquify.string('test_string', -> (s) { true if s == 'test_string' || s == 'test_string1' })
+      result = uniquify.string('test_string') { |s| s == 'test_string' || s == 'test_string1' }
 
       expect(result).to eq('test_string2')
     end
 
     it 'allows passing in a base function that defines the location of the counter' do
-      uniquify = Uniquify.new
-
-      result = uniquify.string(
-        -> (counter) { "test_#{counter}_string" },
-        -> (s) { true if s == 'test__string' }
-      )
+      result = uniquify.string(-> (counter) { "test_#{counter}_string" }) do |s|
+        s == 'test__string'
+      end
 
       expect(result).to eq('test_1_string')
     end
diff --git a/spec/services/users/destroy_spec.rb b/spec/services/users/destroy_spec.rb
index 4f0834064e5..922e82445d0 100644
--- a/spec/services/users/destroy_spec.rb
+++ b/spec/services/users/destroy_spec.rb
@@ -47,6 +47,10 @@ describe Users::DestroyService, services: true do
 
           expect(migrated_issue.author).to eq(User.ghost)
         end
+
+        it 'blocks the user before migrating issues to the "Ghost User' do
+          expect(user).to be_blocked
+        end
       end
 
       context "for an issue the user was assigned to" do