Merge branch '34930-fix-edited-by' into 'master'

Fix issue detail if user who last edited an issue was deleted

Closes #34930

See merge request !12933

app/models/concerns/editable.rb

@@ -4,4 +4,8 @@ module Editable
   def is_edited?
     last_edited_at.present? && last_edited_at != created_at
   end
+
+  def last_edited_by
+    super || User.ghost
+  end
 end

app/models/user.rb

@@ -385,9 +385,11 @@ class User < ActiveRecord::Base
     # Return (create if necessary) the ghost user. The ghost user
     # owns records previously belonging to deleted users.
     def ghost
-      unique_internal(where(ghost: true), 'ghost', 'ghost%s@example.com') do |u|
+      email = 'ghost%s@example.com'
+      unique_internal(where(ghost: true), 'ghost', email) do |u|
         u.bio = 'This is a "Ghost User", created to hold all issues authored by users that have since been deleted. This user cannot be removed.'
         u.name = 'Ghost User'
+        u.notification_email = email
       end
     end
   end

app/services/users/migrate_to_ghost_user_service.rb

@@ -50,10 +50,12 @@ module Users
 
     def migrate_issues
       user.issues.update_all(author_id: ghost_user.id)
+      Issue.where(last_edited_by_id: user.id).update_all(last_edited_by_id: ghost_user.id)
     end
 
     def migrate_merge_requests
       user.merge_requests.update_all(author_id: ghost_user.id)
+      MergeRequest.where(merge_user_id: user.id).update_all(merge_user_id: ghost_user.id)
     end
 
     def migrate_notes

changelogs/unreleased/34930-fix-edited-by.yml

+---
+title: Use Ghost user for last_edited_by and merge_user when original user is deleted
+merge_request: 12933
+author:

spec/controllers/projects/issues_controller_spec.rb

@@ -516,6 +516,36 @@ describe Projects::IssuesController do
       end
     end
 
+    describe 'GET #realtime_changes' do
+      it_behaves_like 'restricted action', success: 200
+
+      def go(id:)
+        get :realtime_changes,
+          namespace_id: project.namespace.to_param,
+          project_id: project,
+          id: id
+      end
+
+      context 'when an issue was edited by a deleted user' do
+        let(:deleted_user) { create(:user) }
+
+        before do
+          project.team << [user, :developer]
+
+          issue.update!(last_edited_by: deleted_user, last_edited_at: Time.now)
+
+          deleted_user.destroy
+          sign_in(user)
+        end
+
+        it 'returns 200' do
+          go(id: issue.iid)
+
+          expect(response).to have_http_status(200)
+        end
+      end
+    end
+
     describe 'GET #edit' do
       it_behaves_like 'restricted action', success: 200
 

spec/features/issues/issue_detail_spec.rb

+require 'rails_helper'
+
+feature 'Issue Detail', js: true, feature: true do
+  let(:user)      { create(:user) }
+  let(:project)   { create(:project, :public) }
+  let(:issue)     { create(:issue, project: project, author: user) }
+
+  context 'when user displays the issue' do
+    before do
+      visit project_issue_path(project, issue)
+      wait_for_requests
+    end
+
+    it 'shows the issue' do
+      page.within('.issuable-details') do
+        expect(find('h2')).to have_content(issue.title)
+      end
+    end
+  end
+
+  context 'when edited by a user who is later deleted' do
+    before do
+      sign_in(user)
+      visit project_issue_path(project, issue)
+      wait_for_requests
+
+      click_link 'Edit'
+      fill_in 'issue-title', with: 'issue title'
+      click_button 'Save'
+
+      visit profile_account_path
+      click_link 'Delete account'
+
+      visit project_issue_path(project, issue)
+    end
+
+    it 'shows the issue' do
+      page.within('.issuable-details') do
+        expect(find('h2')).to have_content(issue.reload.title)
+      end
+    end
+  end
+end

spec/helpers/issuables_helper_spec.rb

@@ -244,5 +244,25 @@ describe IssuablesHelper do
 
     it { expect(helper.updated_at_by(unedited_issuable)).to eq({}) }
     it { expect(helper.updated_at_by(edited_issuable)).to eq(edited_updated_at_by) }
+
+    context 'when updated by a deleted user' do
+      let(:edited_updated_at_by) do
+        {
+          updatedAt: edited_issuable.updated_at.to_time.iso8601,
+          updatedBy: {
+            name: User.ghost.name,
+            path: user_path(User.ghost)
+          }
+        }
+      end
+
+      before do
+        user.destroy
+      end
+
+      it 'returns "Ghost user" as edited_by' do
+        expect(helper.updated_at_by(edited_issuable.reload)).to eq(edited_updated_at_by)
+      end
+    end
   end
 end

spec/services/users/migrate_to_ghost_user_service_spec.rb

@@ -7,16 +7,32 @@ describe Users::MigrateToGhostUserService, services: true do
 
   context "migrating a user's associated records to the ghost user" do
     context 'issues'  do
-      include_examples "migrating a deleted user's associated records to the ghost user", Issue do
-        let(:created_record) { create(:issue, project: project, author: user) }
-        let(:assigned_record) { create(:issue, project: project, assignee: user) }
+      context 'deleted user is present as both author and edited_user' do
+        include_examples "migrating a deleted user's associated records to the ghost user", Issue, [:author, :last_edited_by] do
+          let(:created_record) do
+            create(:issue, project: project, author: user, last_edited_by: user)
+          end
+        end
+      end
+
+      context 'deleted user is present only as edited_user' do
+        include_examples "migrating a deleted user's associated records to the ghost user", Issue, [:last_edited_by] do
+          let(:created_record) { create(:issue, project: project, author: create(:user), last_edited_by: user) }
+        end
       end
     end
 
     context 'merge requests' do
-      include_examples "migrating a deleted user's associated records to the ghost user", MergeRequest do
-        let(:created_record) { create(:merge_request, source_project: project, author: user, target_branch: "first") }
-        let(:assigned_record) { create(:merge_request, source_project: project, assignee: user, target_branch: 'second') }
+      context 'deleted user is present as both author and merge_user' do
+        include_examples "migrating a deleted user's associated records to the ghost user", MergeRequest, [:author, :merge_user] do
+          let(:created_record) { create(:merge_request, source_project: project, author: user, merge_user: user, target_branch: "first") }
+        end
+      end
+
+      context 'deleted user is present only as both merge_user' do
+        include_examples "migrating a deleted user's associated records to the ghost user", MergeRequest, [:merge_user] do
+          let(:created_record) { create(:merge_request, source_project: project, merge_user: user, target_branch: "first") }
+        end
       end
     end
 
@@ -33,9 +49,8 @@ describe Users::MigrateToGhostUserService, services: true do
     end
 
     context 'award emoji' do
-      include_examples "migrating a deleted user's associated records to the ghost user", AwardEmoji do
+      include_examples "migrating a deleted user's associated records to the ghost user", AwardEmoji, [:user] do
         let(:created_record) { create(:award_emoji, user: user) }
-        let(:author_alias) { :user }
 
         context "when the awardable already has an award emoji of the same name assigned to the ghost user" do
           let(:awardable) { create(:issue) }

spec/support/services/migrate_to_ghost_user_service_shared_examples.rb

 require "spec_helper"
 
-shared_examples "migrating a deleted user's associated records to the ghost user" do |record_class|
+shared_examples "migrating a deleted user's associated records to the ghost user" do |record_class, fields|
   record_class_name = record_class.to_s.titleize.downcase
 
   let(:project) { create(:project) }
@@ -11,6 +11,7 @@ shared_examples "migrating a deleted user's associated records to the ghost user
 
   context "for a #{record_class_name} the user has created" do
     let!(:record) { created_record }
+    let(:migrated_fields) { fields || [:author] }
 
     it "does not delete the #{record_class_name}" do
       service.execute
@@ -18,22 +19,20 @@ shared_examples "migrating a deleted user's associated records to the ghost user
       expect(record_class.find_by_id(record.id)).to be_present
     end
 
-    it "migrates the #{record_class_name} so that the 'Ghost User' is the #{record_class_name} owner" do
+    it "blocks the user before migrating #{record_class_name}s to the 'Ghost User'" do
       service.execute
 
-      migrated_record = record_class.find_by_id(record.id)
-
-      if migrated_record.respond_to?(:author)
-        expect(migrated_record.author).to eq(User.ghost)
-      else
-        expect(migrated_record.send(author_alias)).to eq(User.ghost)
-      end
+      expect(user).to be_blocked
     end
 
-    it "blocks the user before migrating #{record_class_name}s to the 'Ghost User'" do
+    it 'migrates all associated fields to te "Ghost user"' do
       service.execute
 
-      expect(user).to be_blocked
+      migrated_record = record_class.find_by_id(record.id)
+
+      migrated_fields.each do |field|
+        expect(migrated_record.public_send(field)).to eq(User.ghost)
+      end
     end
 
     context "race conditions" do