Deleting a user shouldn't delete associated issues.

- "Associated" issues are issues the user has created + issues that the
  user is assigned to.

- Issues that a user owns are transferred to a "Ghost User" (just a
  regular user with `state = 'ghost'` that is created when
  `User.ghost` is called).

- Issues that a user is assigned to are moved to the "Unassigned" state.

- Fix a spec failure in `profile_spec` — a spec was asserting that when a user
  is deleted, `User.count` decreases by 1. After this change, deleting a user
  creates (potentially) a ghost user, causing `User.count` not to change. The
  spec has been updated to look for the relevant user in the assertion.

5 files changed, 129 insertions, 4 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index fada0e567f0..23beaa6c7c8 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -81,7 +81,6 @@ class User < ActiveRecord::Base
   has_many :authorized_projects, through: :project_authorizations, source: :project
 
   has_many :snippets,                 dependent: :destroy, foreign_key: :author_id
-  has_many :issues,                   dependent: :destroy, foreign_key: :author_id
   has_many :notes,                    dependent: :destroy, foreign_key: :author_id
   has_many :merge_requests,           dependent: :destroy, foreign_key: :author_id
   has_many :events,                   dependent: :destroy, foreign_key: :author_id
@@ -99,6 +98,11 @@ class User < ActiveRecord::Base
   has_many :assigned_issues,          dependent: :nullify, foreign_key: :assignee_id, class_name: "Issue"
   has_many :assigned_merge_requests,  dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest"
 
+  # Issues that a user owns are expected to be moved to the "ghost" user before
+  # the user is destroyed. If the user owns any issues during deletion, this
+  # should be treated as an exceptional condition.
+  has_many :issues,                   dependent: :restrict_with_exception, foreign_key: :author_id
+
   #
   # Validations
   #
@@ -181,6 +185,8 @@ class User < ActiveRecord::Base
           "administrator if you think this is an error."
       end
     end
+
+    state :ghost
   end
 
   mount_uploader :avatar, AvatarUploader
@@ -337,6 +343,30 @@ class User < ActiveRecord::Base
         (?<user>#{Gitlab::Regex::NAMESPACE_REF_REGEX_STR})
       }x
     end
+
+    # Return (create if necessary) the ghost user. The ghost user
+    # owns records previously belonging to deleted users.
+    def ghost
+      ghost_user = User.with_state(:ghost).first
+
+      ghost_user ||
+        begin
+          users = Enumerator.new do |y|
+            n = nil
+            loop do
+              user = User.new(
+                username: "ghost#{n}", password: Devise.friendly_token,
+                email: "ghost#{n}@example.com", name: "Ghost User", state: :ghost
+              )
+
+              y.yield(user)
+              n = n ? n.next : 0
+            end
+          end
+
+          users.lazy.select { |user| user.valid? }.first.tap(&:save!)
+        end
+    end
   end
 
   #
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb
index bc0653cb634..d88b81c04e2 100644
--- a/app/services/users/destroy_service.rb
+++ b/app/services/users/destroy_service.rb
@@ -26,6 +26,8 @@ module Users
         ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute
       end
 
+      move_issues_to_ghost_user(user)
+
       # Destroy the namespace after destroying the user since certain methods may depend on the namespace existing
       namespace = user.namespace
       user_data = user.destroy
@@ -33,5 +35,17 @@ module Users
 
       user_data
     end
+
+    private
+
+    def move_issues_to_ghost_user(user)
+      ghost_user = User.ghost
+
+      Issue.transaction do
+        user.issues.each { |issue| issue.update!(author: ghost_user) }
+      end
+
+      user.reload
+    end
   end
 end
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/user_spec.rb b/spec/models/user_spec.rb
index b69fd24c586..00e7927bf90 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) }
@@ -1490,4 +1490,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('ghost0')
+      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('ghost0@example.com')
+      end
+    end
+  end
 end
diff --git a/spec/services/users/destroy_spec.rb b/spec/services/users/destroy_spec.rb
index c0bf27c698c..4f0834064e5 100644
--- a/spec/services/users/destroy_spec.rb
+++ b/spec/services/users/destroy_spec.rb
@@ -24,6 +24,50 @@ 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
+      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) }