summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/models/concerns/editable.rb4
-rw-r--r--app/models/user.rb4
-rw-r--r--app/services/users/migrate_to_ghost_user_service.rb2
-rw-r--r--changelogs/unreleased/34930-fix-edited-by.yml4
-rw-r--r--spec/controllers/projects/issues_controller_spec.rb30
-rw-r--r--spec/features/issues/issue_detail_spec.rb43
-rw-r--r--spec/helpers/issuables_helper_spec.rb20
-rw-r--r--spec/services/users/migrate_to_ghost_user_service_spec.rb31
-rw-r--r--spec/support/services/migrate_to_ghost_user_service_shared_examples.rb21
9 files changed, 139 insertions, 20 deletions
diff --git a/app/models/concerns/editable.rb b/app/models/concerns/editable.rb
index c62c7e1e936..28623d257a6 100644
--- a/app/models/concerns/editable.rb
+++ b/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
diff --git a/app/models/user.rb b/app/models/user.rb
index 8f40af24e20..c26be6d05a2 100644
--- a/app/models/user.rb
+++ b/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
diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb
index 4628c4c6f6e..3a9c151cf9b 100644
--- a/app/services/users/migrate_to_ghost_user_service.rb
+++ b/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
diff --git a/changelogs/unreleased/34930-fix-edited-by.yml b/changelogs/unreleased/34930-fix-edited-by.yml
new file mode 100644
index 00000000000..f133dfab0c2
--- /dev/null
+++ b/changelogs/unreleased/34930-fix-edited-by.yml
@@ -0,0 +1,4 @@
+---
+title: Use Ghost user for last_edited_by and merge_user when original user is deleted
+merge_request: 12933
+author:
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 1f9ca765233..18d0be3c103 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/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
diff --git a/spec/features/issues/issue_detail_spec.rb b/spec/features/issues/issue_detail_spec.rb
new file mode 100644
index 00000000000..e1c55d246ab
--- /dev/null
+++ b/spec/features/issues/issue_detail_spec.rb
@@ -0,0 +1,43 @@
+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
diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb
index b423a09873b..7789cfa3554 100644
--- a/spec/helpers/issuables_helper_spec.rb
+++ b/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
diff --git a/spec/services/users/migrate_to_ghost_user_service_spec.rb b/spec/services/users/migrate_to_ghost_user_service_spec.rb
index 9e1edf1ac30..e52ecd6d614 100644
--- a/spec/services/users/migrate_to_ghost_user_service_spec.rb
+++ b/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) }
diff --git a/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb b/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb
index dcc562c684b..855051921f0 100644
--- a/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb
+++ b/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb
@@ -1,6 +1,6 @@
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