diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 4d251cb30db..8960235b093 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -2,17 +2,17 @@ class IssuableBaseService < BaseService private def create_assignee_note(issuable) - SystemNoteService.assignee_change( + SystemNoteService.change_assignee( issuable, issuable.project, current_user, issuable.assignee) end def create_milestone_note(issuable) - SystemNoteService.milestone_change( + SystemNoteService.change_milestone( issuable, issuable.project, current_user, issuable.milestone) end def create_labels_note(issuable, added_labels, removed_labels) - SystemNoteService.label_change( + SystemNoteService.change_label( issuable, issuable.project, current_user, added_labels, removed_labels) end end diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb index bdc13685fd0..138465859ce 100644 --- a/app/services/issues/close_service.rb +++ b/app/services/issues/close_service.rb @@ -14,7 +14,7 @@ module Issues private def create_note(issue, current_commit) - SystemNoteService.status_change(issue, issue.project, current_user, issue.state, current_commit) + SystemNoteService.change_status(issue, issue.project, current_user, issue.state, current_commit) end end end diff --git a/app/services/issues/reopen_service.rb b/app/services/issues/reopen_service.rb index fbbf573e5a3..e48ca359f4f 100644 --- a/app/services/issues/reopen_service.rb +++ b/app/services/issues/reopen_service.rb @@ -14,7 +14,7 @@ module Issues private def create_note(issue) - SystemNoteService.status_change(issue, issue.project, current_user, issue.state, nil) + SystemNoteService.change_status(issue, issue.project, current_user, issue.state, nil) end end end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 9b470880adf..e455fe95791 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -2,7 +2,7 @@ module MergeRequests class BaseService < ::IssuableBaseService def create_note(merge_request) - SystemNoteService.status_change(merge_request, merge_request.target_project, current_user, merge_request.state, nil) + SystemNoteService.change_status(merge_request, merge_request.target_project, current_user, merge_request.state, nil) end def hook_data(merge_request, action) diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 1d57fd11de7..66610a08a44 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -82,9 +82,9 @@ module MergeRequests mr_commit_ids.include?(commit.id) end - SystemNoteService.commit_add(merge_request, merge_request.project, - @current_user, new_commits, - existing_commits, @oldrev) + SystemNoteService.add_commits(merge_request, merge_request.project, + @current_user, new_commits, + existing_commits, @oldrev) end end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index c4bc2339e7b..a1de050ca5a 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -2,11 +2,30 @@ # # Used for creating system notes (e.g., when a user references a merge request # from an issue, an issue's assignee changes, an issue is closed, etc.) -# -# All methods creating notes should be named using a singular noun and -# present-tense verb (e.g., "assignee_change" not "assignee_changed", -# "label_change" not "labels_change"). class SystemNoteService + # Called when commits are added to a Merge Request + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # new_commits - Array of Commits added since last push + # existing_commits - Array of Commits added in a previous push + # oldrev - TODO (rspeicher): I have no idea what this actually does + # + # See new_commit_summary and existing_commit_summary. + # + # Returns the created Note object + def self.add_commits(noteable, project, author, new_commits, existing_commits = [], oldrev = nil) + total_count = new_commits.length + existing_commits.length + commits_text = "#{total_count} commit".pluralize(total_count) + + body = "Added #{commits_text}:\n\n" + body << existing_commit_summary(noteable, existing_commits, oldrev) + body << new_commit_summary(new_commits).join("\n") + + create_note(noteable: noteable, project: project, author: author, note: body) + end + # Called when the assignee of a Noteable is changed or removed # # noteable - Noteable object @@ -21,12 +40,96 @@ class SystemNoteService # "Reassigned to @rspeicher" # # Returns the created Note object - def self.assignee_change(noteable, project, author, assignee) + def self.change_assignee(noteable, project, author, assignee) body = assignee.nil? ? 'Assignee removed' : "Reassigned to @#{assignee.username}" create_note(noteable: noteable, project: project, author: author, note: body) end + # Called when one or more labels on a Noteable are added and/or removed + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # added_labels - Array of Labels added + # removed_labels - Array of Labels removed + # + # Example Note text: + # + # "Added ~1 and removed ~2 ~3 labels" + # + # "Added ~4 label" + # + # "Removed ~5 label" + # + # Returns the created Note object + def self.change_label(noteable, project, author, added_labels, removed_labels) + labels_count = added_labels.count + removed_labels.count + + references = ->(label) { "~#{label.id}" } + added_labels = added_labels.map(&references).join(' ') + removed_labels = removed_labels.map(&references).join(' ') + + body = '' + + if added_labels.present? + body << "added #{added_labels}" + body << ' and ' if removed_labels.present? + end + + if removed_labels.present? + body << "removed #{removed_labels}" + end + + body << ' ' << 'label'.pluralize(labels_count) + body = "#{body.capitalize}" + + create_note(noteable: noteable, project: project, author: author, note: body) + end + + # Called when the milestone of a Noteable is changed + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # milestone - Milestone being assigned, or nil + # + # Example Note text: + # + # "Milestone removed" + # + # "Miletone changed to 7.11" + # + # Returns the created Note object + def self.change_milestone(noteable, project, author, milestone) + body = 'Milestone ' + body += milestone.nil? ? 'removed' : "changed to #{milestone.title}" + + create_note(noteable: noteable, project: project, author: author, note: body) + end + + # Called when the status of a Noteable is changed + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # status - String status + # source - Mentionable performing the change, or nil + # + # Example Note text: + # + # "Status changed to merged" + # + # "Status changed to closed by bc17db76" + # + # Returns the created Note object + def self.change_status(noteable, project, author, status, source) + body = "Status changed to #{status}" + body += " by #{source.gfm_reference}" if source + + create_note(noteable: noteable, project: project, author: author, note: body) + end + # Called when a Mentionable references a Noteable # # noteable - Noteable object being referenced @@ -64,113 +167,6 @@ class SystemNoteService create_note(note_options) end - # Called when one or more labels on a Noteable are added and/or removed - # - # noteable - Noteable object - # project - Project owning noteable - # author - User performing the change - # added_labels - Array of Labels added - # removed_labels - Array of Labels removed - # - # Example Note text: - # - # "Added ~1 and removed ~2 ~3 labels" - # - # "Added ~4 label" - # - # "Removed ~5 label" - # - # Returns the created Note object - def self.label_change(noteable, project, author, added_labels, removed_labels) - labels_count = added_labels.count + removed_labels.count - - references = ->(label) { "~#{label.id}" } - added_labels = added_labels.map(&references).join(' ') - removed_labels = removed_labels.map(&references).join(' ') - - body = '' - - if added_labels.present? - body << "added #{added_labels}" - body << ' and ' if removed_labels.present? - end - - if removed_labels.present? - body << "removed #{removed_labels}" - end - - body << ' ' << 'label'.pluralize(labels_count) - body = "#{body.capitalize}" - - create_note(noteable: noteable, project: project, author: author, note: body) - end - - # Called when the milestone of a Noteable is changed - # - # noteable - Noteable object - # project - Project owning noteable - # author - User performing the change - # milestone - Milestone being assigned, or nil - # - # Example Note text: - # - # "Milestone removed" - # - # "Miletone changed to 7.11" - # - # Returns the created Note object - def self.milestone_change(noteable, project, author, milestone) - body = 'Milestone ' - body += milestone.nil? ? 'removed' : "changed to #{milestone.title}" - - create_note(noteable: noteable, project: project, author: author, note: body) - end - - # Called when commits are added to a Merge Request - # - # noteable - Noteable object - # project - Project owning noteable - # author - User performing the change - # new_commits - Array of Commits added since last push - # existing_commits - Array of Commits added in a previous push - # oldrev - TODO (rspeicher): I have no idea what this actually does - # - # See new_commit_summary and existing_commit_summary. - # - # Returns the created Note object - def self.commit_add(noteable, project, author, new_commits, existing_commits = [], oldrev = nil) - total_count = new_commits.length + existing_commits.length - commits_text = "#{total_count} commit".pluralize(total_count) - - body = "Added #{commits_text}:\n\n" - body << existing_commit_summary(noteable, existing_commits, oldrev) - body << new_commit_summary(new_commits).join("\n") - - create_note(noteable: noteable, project: project, author: author, note: body) - end - - # Called when the status of a Noteable is changed - # - # noteable - Noteable object - # project - Project owning noteable - # author - User performing the change - # status - String status - # source - Mentionable performing the change, or nil - # - # Example Note text: - # - # "Status changed to merged" - # - # "Status changed to closed by bc17db76" - # - # Returns the created Note object - def self.status_change(noteable, project, author, status, source) - body = "Status changed to #{status}" - body += " by #{source.gfm_reference}" if source - - create_note(noteable: noteable, project: project, author: author, note: body) - end - def self.cross_reference?(note_text) note_text.start_with?(cross_reference_note_prefix) end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 194687cbbd6..3e9528a83d0 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -23,155 +23,13 @@ describe SystemNoteService do end end - describe '.assignee_change' do - let(:assignee) { create(:user) } - - subject { described_class.assignee_change(noteable, project, author, assignee) } - - it_behaves_like 'a system note' - - context 'when assignee added' do - it 'sets the note text' do - expect(subject.note).to eq "Reassigned to @#{assignee.username}" - end - end - - context 'when assignee removed' do - let(:assignee) { nil } - - it 'sets the note text' do - expect(subject.note).to eq 'Assignee removed' - end - end - end - - describe '.cross_reference' do - let(:mentioner) { create(:issue, project: project) } - - subject { described_class.cross_reference(noteable, mentioner, author) } - - it_behaves_like 'a system note' - - context 'when cross-reference disallowed' do - before do - expect(described_class).to receive(:cross_reference_disallowed?).and_return(true) - end - - it 'returns nil' do - expect(subject).to be_nil - end - end - - context 'when cross-reference allowed' do - before do - expect(described_class).to receive(:cross_reference_disallowed?).and_return(false) - end - - describe 'note_body' do - context 'cross-project' do - let(:project2) { create(:project) } - let(:mentioner) { create(:issue, project: project2) } - - context 'from Commit' do - let(:mentioner) { project2.repository.commit } - - it 'references the mentioning commit' do - expect(subject.note).to eq "mentioned in commit #{project2.path_with_namespace}@#{mentioner.id}" - end - end - - context 'from non-Commit' do - it 'references the mentioning object' do - expect(subject.note).to eq "mentioned in issue #{project2.path_with_namespace}##{mentioner.iid}" - end - end - end - - context 'same project' do - context 'from Commit' do - let(:mentioner) { project.repository.commit } - - it 'references the mentioning commit' do - expect(subject.note).to eq "mentioned in commit #{mentioner.id}" - end - end - - context 'from non-Commit' do - it 'references the mentioning object' do - expect(subject.note).to eq "mentioned in issue ##{mentioner.iid}" - end - end - end - end - end - end - - describe '.label_change' do - let(:labels) { create_list(:label, 2) } - let(:added) { [] } - let(:removed) { [] } - - subject { described_class.label_change(noteable, project, author, added, removed) } - - it_behaves_like 'a system note' - - context 'with added labels' do - let(:added) { labels } - let(:removed) { [] } - - it 'sets the note text' do - expect(subject.note).to eq "Added ~#{labels[0].id} ~#{labels[1].id} labels" - end - end - - context 'with removed labels' do - let(:added) { [] } - let(:removed) { labels } - - it 'sets the note text' do - expect(subject.note).to eq "Removed ~#{labels[0].id} ~#{labels[1].id} labels" - end - end - - context 'with added and removed labels' do - let(:added) { [labels[0]] } - let(:removed) { [labels[1]] } - - it 'sets the note text' do - expect(subject.note).to eq "Added ~#{labels[0].id} and removed ~#{labels[1].id} labels" - end - end - end - - describe '.milestone_change' do - let(:milestone) { create(:milestone, project: project) } - - subject { described_class.milestone_change(noteable, project, author, milestone) } - - it_behaves_like 'a system note' - - context 'when milestone added' do - it 'sets the note text' do - expect(subject.note).to eq "Milestone changed to #{milestone.title}" - end - end - - context 'when milestone removed' do - let(:milestone) { nil } - - it 'sets the note text' do - expect(subject.note).to eq 'Milestone removed' - end - end - end - - describe '.commit_add' do + describe '.add_commits' do let(:noteable) { create(:merge_request, source_project: project) } let(:new_commits) { noteable.commits } let(:old_commits) { [] } let(:oldrev) { nil } - subject { described_class.commit_add(noteable, project, author, new_commits, old_commits, oldrev) } + subject { described_class.add_commits(noteable, project, author, new_commits, old_commits, oldrev) } it_behaves_like 'a system note' @@ -241,11 +99,92 @@ describe SystemNoteService do end end - describe '.status_change' do + describe '.change_assignee' do + let(:assignee) { create(:user) } + + subject { described_class.change_assignee(noteable, project, author, assignee) } + + it_behaves_like 'a system note' + + context 'when assignee added' do + it 'sets the note text' do + expect(subject.note).to eq "Reassigned to @#{assignee.username}" + end + end + + context 'when assignee removed' do + let(:assignee) { nil } + + it 'sets the note text' do + expect(subject.note).to eq 'Assignee removed' + end + end + end + + describe '.change_label' do + let(:labels) { create_list(:label, 2) } + let(:added) { [] } + let(:removed) { [] } + + subject { described_class.change_label(noteable, project, author, added, removed) } + + it_behaves_like 'a system note' + + context 'with added labels' do + let(:added) { labels } + let(:removed) { [] } + + it 'sets the note text' do + expect(subject.note).to eq "Added ~#{labels[0].id} ~#{labels[1].id} labels" + end + end + + context 'with removed labels' do + let(:added) { [] } + let(:removed) { labels } + + it 'sets the note text' do + expect(subject.note).to eq "Removed ~#{labels[0].id} ~#{labels[1].id} labels" + end + end + + context 'with added and removed labels' do + let(:added) { [labels[0]] } + let(:removed) { [labels[1]] } + + it 'sets the note text' do + expect(subject.note).to eq "Added ~#{labels[0].id} and removed ~#{labels[1].id} labels" + end + end + end + + describe '.change_milestone' do + let(:milestone) { create(:milestone, project: project) } + + subject { described_class.change_milestone(noteable, project, author, milestone) } + + it_behaves_like 'a system note' + + context 'when milestone added' do + it 'sets the note text' do + expect(subject.note).to eq "Milestone changed to #{milestone.title}" + end + end + + context 'when milestone removed' do + let(:milestone) { nil } + + it 'sets the note text' do + expect(subject.note).to eq 'Milestone removed' + end + end + end + + describe '.change_status' do let(:status) { 'new_status' } let(:source) { nil } - subject { described_class.status_change(noteable, project, author, status, source) } + subject { described_class.change_status(noteable, project, author, status, source) } it_behaves_like 'a system note' @@ -264,6 +203,67 @@ describe SystemNoteService do end end + describe '.cross_reference' do + let(:mentioner) { create(:issue, project: project) } + + subject { described_class.cross_reference(noteable, mentioner, author) } + + it_behaves_like 'a system note' + + context 'when cross-reference disallowed' do + before do + expect(described_class).to receive(:cross_reference_disallowed?).and_return(true) + end + + it 'returns nil' do + expect(subject).to be_nil + end + end + + context 'when cross-reference allowed' do + before do + expect(described_class).to receive(:cross_reference_disallowed?).and_return(false) + end + + describe 'note_body' do + context 'cross-project' do + let(:project2) { create(:project) } + let(:mentioner) { create(:issue, project: project2) } + + context 'from Commit' do + let(:mentioner) { project2.repository.commit } + + it 'references the mentioning commit' do + expect(subject.note).to eq "mentioned in commit #{project2.path_with_namespace}@#{mentioner.id}" + end + end + + context 'from non-Commit' do + it 'references the mentioning object' do + expect(subject.note).to eq "mentioned in issue #{project2.path_with_namespace}##{mentioner.iid}" + end + end + end + + context 'same project' do + context 'from Commit' do + let(:mentioner) { project.repository.commit } + + it 'references the mentioning commit' do + expect(subject.note).to eq "mentioned in commit #{mentioner.id}" + end + end + + context 'from non-Commit' do + it 'references the mentioning object' do + expect(subject.note).to eq "mentioned in issue ##{mentioner.iid}" + end + end + end + end + end + end + describe '.cross_reference?' do it 'is truthy when text begins with expected text' do expect(described_class.cross_reference?('mentioned in issue #1')).to be_truthy @@ -274,6 +274,7 @@ describe SystemNoteService do end end + # TODO (rspeicher) describe '.cross_reference_disallowed?' describe '.cross_reference_exists?' do