Merge branch '12726-preserve-issues-after-deleting-users' into 'master'

Deleting a user shouldn't delete associated issues.

Closes #12726

See merge request !7393

7 files changed, 180 insertions, 4 deletions

diff --git a/spec/factories/users.rb b/spec/factories/users.rb
index 1732b1a0081..249dabbaae8 100644
--- a/spec/factories/users.rb
+++ b/spec/factories/users.rb
@@ -26,6 +26,11 @@ FactoryGirl.define do
       two_factor_via_otp
     end
 
+    trait :ghost do
+      ghost true
+      after(:build) { |user, _| user.block! }
+    end
+
     trait :two_factor_via_otp do
       before(:create) do |user|
         user.otp_required_for_login = true
diff --git a/spec/features/profile_spec.rb b/spec/features/profile_spec.rb
index 7a562b5e03d..406d7cf791c 100644
--- a/spec/features/profile_spec.rb
+++ b/spec/features/profile_spec.rb
@@ -4,7 +4,7 @@ describe 'Profile account page', feature: true do
   let(:user) { create(:user) }
 
   before do
-    login_as :user
+    login_as(user)
   end
 
   describe 'when signup is enabled' do
@@ -16,7 +16,7 @@ describe 'Profile account page', feature: true do
     it { expect(page).to have_content('Remove account') }
 
     it 'deletes the account' do
-      expect { click_link 'Delete account' }.to change { User.count }.by(-1)
+      expect { click_link 'Delete account' }.to change { User.where(id: user.id).count }.by(-1)
       expect(current_path).to eq(new_user_session_path)
     end
   end
diff --git a/spec/models/abuse_report_spec.rb b/spec/models/abuse_report_spec.rb
index c4486a32082..4e71597521d 100644
--- a/spec/models/abuse_report_spec.rb
+++ b/spec/models/abuse_report_spec.rb
@@ -2,7 +2,7 @@ require 'rails_helper'
 
 RSpec.describe AbuseReport, type: :model do
   subject     { create(:abuse_report) }
-  let(:user)  { create(:user) }
+  let(:user)  { create(:admin) }
 
   it { expect(subject).to be_valid }
 
diff --git a/spec/models/concerns/uniquify_spec.rb b/spec/models/concerns/uniquify_spec.rb
new file mode 100644
index 00000000000..83187d732e4
--- /dev/null
+++ b/spec/models/concerns/uniquify_spec.rb
@@ -0,0 +1,33 @@
+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
+      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
+      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
+      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
+      result = uniquify.string(-> (counter) { "test_#{counter}_string" }) do |s|
+        s == 'test__string'
+      end
+
+      expect(result).to eq('test_1_string')
+    end
+  end
+end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index b69fd24c586..6356f8b6c92 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -22,7 +22,7 @@ describe User, models: true do
     it { is_expected.to have_many(:deploy_keys).dependent(:destroy) }
     it { is_expected.to have_many(:events).dependent(:destroy) }
     it { is_expected.to have_many(:recent_events).class_name('Event') }
-    it { is_expected.to have_many(:issues).dependent(:destroy) }
+    it { is_expected.to have_many(:issues).dependent(:restrict_with_exception) }
     it { is_expected.to have_many(:notes).dependent(:destroy) }
     it { is_expected.to have_many(:assigned_issues).dependent(:nullify) }
     it { is_expected.to have_many(:merge_requests).dependent(:destroy) }
@@ -208,6 +208,22 @@ describe User, models: true do
         end
       end
     end
+
+    describe 'ghost users' do
+      it 'does not allow a non-blocked ghost user' do
+        user = build(:user, :ghost)
+        user.state = 'active'
+
+        expect(user).to be_invalid
+      end
+
+      it 'allows a blocked ghost user' do
+        user = build(:user, :ghost)
+        user.state = 'blocked'
+
+        expect(user).to be_valid
+      end
+    end
   end
 
   describe "scopes" do
@@ -1490,4 +1506,41 @@ describe User, models: true do
       expect(user.admin).to be true
     end
   end
+
+  describe '.ghost' do
+    it "creates a ghost user if one isn't already present" do
+      ghost = User.ghost
+
+      expect(ghost).to be_ghost
+      expect(ghost).to be_persisted
+    end
+
+    it "does not create a second ghost user if one is already present" do
+      expect do
+        User.ghost
+        User.ghost
+      end.to change { User.count }.by(1)
+      expect(User.ghost).to eq(User.ghost)
+    end
+
+    context "when a regular user exists with the username 'ghost'" do
+      it "creates a ghost user with a non-conflicting username" do
+        create(:user, username: 'ghost')
+        ghost = User.ghost
+
+        expect(ghost).to be_persisted
+        expect(ghost.username).to eq('ghost1')
+      end
+    end
+
+    context "when a regular user exists with the email 'ghost@example.com'" do
+      it "creates a ghost user with a non-conflicting email" do
+        create(:user, email: 'ghost@example.com')
+        ghost = User.ghost
+
+        expect(ghost).to be_persisted
+        expect(ghost.email).to eq('ghost1@example.com')
+      end
+    end
+  end
 end
diff --git a/spec/policies/user_policy_spec.rb b/spec/policies/user_policy_spec.rb
new file mode 100644
index 00000000000..d5761390d39
--- /dev/null
+++ b/spec/policies/user_policy_spec.rb
@@ -0,0 +1,37 @@
+require 'spec_helper'
+
+describe UserPolicy, models: true do
+  let(:current_user) { create(:user) }
+  let(:user) { create(:user) }
+
+  subject { described_class.abilities(current_user, user).to_set }
+
+  describe "reading a user's information" do
+    it { is_expected.to include(:read_user) }
+  end
+
+  describe "destroying a user" do
+    context "when a regular user tries to destroy another regular user" do
+      it { is_expected.not_to include(:destroy_user) }
+    end
+
+    context "when a regular user tries to destroy themselves" do
+      let(:current_user) { user }
+
+      it { is_expected.to include(:destroy_user) }
+    end
+
+    context "when an admin user tries to destroy a regular user" do
+      let(:current_user) { create(:user, :admin) }
+
+      it { is_expected.to include(:destroy_user) }
+    end
+
+    context "when an admin user tries to destroy a ghost user" do
+      let(:current_user) { create(:user, :admin) }
+      let(:user) { create(:user, :ghost) }
+
+      it { is_expected.not_to include(:destroy_user) }
+    end
+  end
+end
diff --git a/spec/services/users/destroy_spec.rb b/spec/services/users/destroy_spec.rb
index c0bf27c698c..922e82445d0 100644
--- a/spec/services/users/destroy_spec.rb
+++ b/spec/services/users/destroy_spec.rb
@@ -24,6 +24,54 @@ describe Users::DestroyService, services: true do
       end
     end
 
+    context "a deleted user's issues" do
+      let(:project) { create :project }
+
+      before do
+        project.add_developer(user)
+      end
+
+      context "for an issue the user has created" do
+        let!(:issue) { create(:issue, project: project, author: user) }
+
+        before do
+          service.execute(user)
+        end
+
+        it 'does not delete the issue' do
+          expect(Issue.find_by_id(issue.id)).to be_present
+        end
+
+        it 'migrates the issue so that the "Ghost User" is the issue owner' do
+          migrated_issue = Issue.find_by_id(issue.id)
+
+          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
+        let!(:issue) { create(:issue, project: project, assignee: user) }
+
+        before do
+          service.execute(user)
+        end
+
+        it 'does not delete issues the user is assigned to' do
+          expect(Issue.find_by_id(issue.id)).to be_present
+        end
+
+        it 'migrates the issue so that it is "Unassigned"' do
+          migrated_issue = Issue.find_by_id(issue.id)
+
+          expect(migrated_issue.assignee).to be_nil
+        end
+      end
+    end
+
     context "solo owned groups present" do
       let(:solo_owned)  { create(:group) }
       let(:member)      { create(:group_member) }