diff options
| -rw-r--r-- | app/models/concerns/editable.rb | 4 | ||||
| -rw-r--r-- | app/models/user.rb | 4 | ||||
| -rw-r--r-- | app/services/users/migrate_to_ghost_user_service.rb | 2 | ||||
| -rw-r--r-- | changelogs/unreleased/34930-fix-edited-by.yml | 4 | ||||
| -rw-r--r-- | spec/controllers/projects/issues_controller_spec.rb | 30 | ||||
| -rw-r--r-- | spec/features/issues/issue_detail_spec.rb | 43 | ||||
| -rw-r--r-- | spec/helpers/issuables_helper_spec.rb | 20 | ||||
| -rw-r--r-- | spec/services/users/migrate_to_ghost_user_service_spec.rb | 31 | ||||
| -rw-r--r-- | spec/support/services/migrate_to_ghost_user_service_shared_examples.rb | 21 |
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 |