Update SystemNoteService method naming conventions

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

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

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

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)

app/services/merge_requests/refresh_service.rb

@@ -82,7 +82,7 @@ module MergeRequests
           mr_commit_ids.include?(commit.id)
         end
 
-        SystemNoteService.commit_add(merge_request, merge_request.project,
+        SystemNoteService.add_commits(merge_request, merge_request.project,
                                       @current_user, new_commits,
                                       existing_commits, @oldrev)
       end

app/services/system_note_service.rb

@@ -2,66 +2,48 @@
 #
 # 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 the assignee of a Noteable is changed or removed
+  # Called when commits are added to a Merge Request
   #
   # noteable         - Noteable object
   # project          - Project owning noteable
   # author           - User performing the change
-  # assignee - User being assigned, or nil
-  #
-  # Example Note text:
-  #
-  #   "Assignee removed"
+  # 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
   #
-  #   "Reassigned to @rspeicher"
+  # See new_commit_summary and existing_commit_summary.
   #
   # Returns the created Note object
-  def self.assignee_change(noteable, project, author, assignee)
-    body = assignee.nil? ? 'Assignee removed' : "Reassigned to @#{assignee.username}"
+  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 a Mentionable references a Noteable
+  # Called when the assignee of a Noteable is changed or removed
   #
-  # noteable  - Noteable object being referenced
-  # mentioner - Mentionable object
-  # author    - User performing the reference
+  # noteable - Noteable object
+  # project  - Project owning noteable
+  # author   - User performing the change
+  # assignee - User being assigned, or nil
   #
   # Example Note text:
   #
-  #   "Mentioned in #1"
-  #
-  #   "Mentioned in !2"
-  #
-  #   "Mentioned in 54f7727c"
+  #   "Assignee removed"
   #
-  # See cross_reference_note_content.
+  #   "Reassigned to @rspeicher"
   #
   # Returns the created Note object
-  def self.cross_reference(noteable, mentioner, author)
-    return if cross_reference_disallowed?(noteable, mentioner)
-
-    gfm_reference = mentioner_gfm_ref(noteable, mentioner)
-
-    note_options = {
-      project: noteable.project,
-      author:  author,
-      note:    cross_reference_note_content(gfm_reference)
-    }
-
-    if noteable.kind_of?(Commit)
-      note_options.merge!(noteable_type: 'Commit', commit_id: noteable.id)
-    else
-      note_options.merge!(noteable: noteable)
-    end
+  def self.change_assignee(noteable, project, author, assignee)
+    body = assignee.nil? ? 'Assignee removed' : "Reassigned to @#{assignee.username}"
 
-    create_note(note_options)
+    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
@@ -81,7 +63,7 @@ class SystemNoteService
   #   "Removed ~5 label"
   #
   # Returns the created Note object
-  def self.label_change(noteable, project, author, added_labels, removed_labels)
+  def self.change_label(noteable, project, author, added_labels, removed_labels)
     labels_count = added_labels.count + removed_labels.count
 
     references     = ->(label) { "~#{label.id}" }
@@ -119,36 +101,13 @@ class SystemNoteService
   #   "Miletone changed to 7.11"
   #
   # Returns the created Note object
-  def self.milestone_change(noteable, project, author, milestone)
+  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 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
@@ -164,13 +123,50 @@ class SystemNoteService
   #   "Status changed to closed by bc17db76"
   #
   # Returns the created Note object
-  def self.status_change(noteable, project, author, status, source)
+  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
+  # mentioner - Mentionable object
+  # author    - User performing the reference
+  #
+  # Example Note text:
+  #
+  #   "Mentioned in #1"
+  #
+  #   "Mentioned in !2"
+  #
+  #   "Mentioned in 54f7727c"
+  #
+  # See cross_reference_note_content.
+  #
+  # Returns the created Note object
+  def self.cross_reference(noteable, mentioner, author)
+    return if cross_reference_disallowed?(noteable, mentioner)
+
+    gfm_reference = mentioner_gfm_ref(noteable, mentioner)
+
+    note_options = {
+      project: noteable.project,
+      author:  author,
+      note:    cross_reference_note_content(gfm_reference)
+    }
+
+    if noteable.kind_of?(Commit)
+      note_options.merge!(noteable_type: 'Commit', commit_id: noteable.id)
+    else
+      note_options.merge!(noteable: noteable)
+    end
+
+    create_note(note_options)
+  end
+
   def self.cross_reference?(note_text)
     note_text.start_with?(cross_reference_note_prefix)
   end

spec/services/system_note_service_spec.rb

@@ -23,95 +23,110 @@ describe SystemNoteService do
     end
   end
 
-  describe '.assignee_change' do
-    let(:assignee) { create(:user) }
+  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.assignee_change(noteable, project, author, assignee) }
+    subject { described_class.add_commits(noteable, project, author, new_commits, old_commits, oldrev) }
 
     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
+    describe 'note body' do
+      let(:note_lines) { subject.note.split("\n").reject(&:blank?) }
 
-    context 'when assignee removed' do
-      let(:assignee) { nil }
+      context 'without existing commits' do
+        it 'adds a message header' do
+          expect(note_lines[0]).to eq "Added #{new_commits.size} commits:"
+        end
 
-      it 'sets the note text' do
-        expect(subject.note).to eq 'Assignee removed'
+        it 'adds a message line for each commit' do
+          new_commits.each_with_index do |commit, i|
+            # Skip the header
+            expect(note_lines[i + 1]).to eq "* #{commit.short_id} - #{commit.title}"
           end
         end
       end
 
-  describe '.cross_reference' do
-    let(:mentioner) { create(:issue, project: project) }
+      describe 'summary line for existing commits' do
+        let(:summary_line) { note_lines[1] }
 
-    subject { described_class.cross_reference(noteable, mentioner, author) }
+        context 'with one existing commit' do
+          let(:old_commits) { [noteable.commits.last] }
 
-    it_behaves_like 'a system note'
+          it 'includes the existing commit' do
+            expect(summary_line).to eq "* #{old_commits.first.short_id} - 1 commit from branch `feature`"
+          end
+        end
 
-    context 'when cross-reference disallowed' do
-      before do
-        expect(described_class).to receive(:cross_reference_disallowed?).and_return(true)
+        context 'with multiple existing commits' do
+          let(:old_commits) { noteable.commits[3..-1] }
+
+          context 'with oldrev' do
+            let(:oldrev) { noteable.commits[2].id }
+
+            it 'includes a commit range' do
+              expect(summary_line).to start_with "* #{Commit.truncate_sha(oldrev)}...#{old_commits.last.short_id}"
             end
 
-      it 'returns nil' do
-        expect(subject).to be_nil
+            it 'includes a commit count' do
+              expect(summary_line).to end_with " - 2 commits from branch `feature`"
             end
           end
 
-    context 'when cross-reference allowed' do
-      before do
-        expect(described_class).to receive(:cross_reference_disallowed?).and_return(false)
+          context 'without oldrev' do
+            it 'includes a commit range' do
+              expect(summary_line).to start_with "* #{old_commits[0].short_id}..#{old_commits[-1].short_id}"
             end
 
-      describe 'note_body' do
-        context 'cross-project' do
-          let(:project2)  { create(:project) }
-          let(:mentioner) { create(:issue, project: project2) }
+            it 'includes a commit count' do
+              expect(summary_line).to end_with " - 2 commits from branch `feature`"
+            end
+          end
 
-          context 'from Commit' do
-            let(:mentioner) { project2.repository.commit }
+          context 'on a fork' do
+            before do
+              expect(noteable).to receive(:for_fork?).and_return(true)
+            end
 
-            it 'references the mentioning commit' do
-              expect(subject.note).to eq "mentioned in commit #{project2.path_with_namespace}@#{mentioner.id}"
+            it 'includes the project namespace' do
+              expect(summary_line).to end_with "`#{noteable.target_project_namespace}:feature`"
+            end
           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 }
+  describe '.change_assignee' do
+    let(:assignee) { create(:user) }
 
-            it 'references the mentioning commit' do
-              expect(subject.note).to eq "mentioned in commit #{mentioner.id}"
-            end
-          end
+    subject { described_class.change_assignee(noteable, project, author, assignee) }
 
-          context 'from non-Commit' do
-            it 'references the mentioning object' do
-              expect(subject.note).to eq "mentioned in issue ##{mentioner.iid}"
-            end
+    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 '.label_change' do
+  describe '.change_label' do
     let(:labels)  { create_list(:label, 2) }
     let(:added)   { [] }
     let(:removed) { [] }
 
-    subject { described_class.label_change(noteable, project, author, added, removed) }
+    subject { described_class.change_label(noteable, project, author, added, removed) }
 
     it_behaves_like 'a system note'
 
@@ -143,10 +158,10 @@ describe SystemNoteService do
     end
   end
 
-  describe '.milestone_change' do
+  describe '.change_milestone' do
     let(:milestone) { create(:milestone, project: project) }
 
-    subject { described_class.milestone_change(noteable, project, author, milestone) }
+    subject { described_class.change_milestone(noteable, project, author, milestone) }
 
     it_behaves_like 'a system note'
 
@@ -165,101 +180,86 @@ describe SystemNoteService do
     end
   end
 
-  describe '.commit_add' do
-    let(:noteable)    { create(:merge_request, source_project: project) }
-    let(:new_commits) { noteable.commits }
-    let(:old_commits) { [] }
-    let(:oldrev)      { nil }
+  describe '.change_status' do
+    let(:status) { 'new_status' }
+    let(:source) { nil }
 
-    subject { described_class.commit_add(noteable, project, author, new_commits, old_commits, oldrev) }
+    subject { described_class.change_status(noteable, project, author, status, source) }
 
     it_behaves_like 'a system note'
 
-    describe 'note body' do
-      let(:note_lines) { subject.note.split("\n").reject(&:blank?) }
+    context 'with a source' do
+      let(:source) { double('commit', gfm_reference: 'commit 123456') }
 
-      context 'without existing commits' do
-        it 'adds a message header' do
-          expect(note_lines[0]).to eq "Added #{new_commits.size} commits:"
+      it 'sets the note text' do
+        expect(subject.note).to eq "Status changed to #{status} by commit 123456"
+      end
     end
 
-        it 'adds a message line for each commit' do
-          new_commits.each_with_index do |commit, i|
-            # Skip the header
-            expect(note_lines[i + 1]).to eq "* #{commit.short_id} - #{commit.title}"
+    context 'without a source' do
+      it 'sets the note text' do
+        expect(subject.note).to eq "Status changed to #{status}"
       end
     end
   end
 
-      describe 'summary line for existing commits' do
-        let(:summary_line) { note_lines[1] }
-
-        context 'with one existing commit' do
-          let(:old_commits) { [noteable.commits.last] }
-
-          it 'includes the existing commit' do
-            expect(summary_line).to eq "* #{old_commits.first.short_id} - 1 commit from branch `feature`"
-          end
-        end
+  describe '.cross_reference' do
+    let(:mentioner) { create(:issue, project: project) }
 
-        context 'with multiple existing commits' do
-          let(:old_commits) { noteable.commits[3..-1] }
+    subject { described_class.cross_reference(noteable, mentioner, author) }
 
-          context 'with oldrev' do
-            let(:oldrev) { noteable.commits[2].id }
+    it_behaves_like 'a system note'
 
-            it 'includes a commit range' do
-              expect(summary_line).to start_with "* #{Commit.truncate_sha(oldrev)}...#{old_commits.last.short_id}"
+    context 'when cross-reference disallowed' do
+      before do
+        expect(described_class).to receive(:cross_reference_disallowed?).and_return(true)
       end
 
-            it 'includes a commit count' do
-              expect(summary_line).to end_with " - 2 commits from branch `feature`"
+      it 'returns nil' do
+        expect(subject).to be_nil
       end
     end
 
-          context 'without oldrev' do
-            it 'includes a commit range' do
-              expect(summary_line).to start_with "* #{old_commits[0].short_id}..#{old_commits[-1].short_id}"
+    context 'when cross-reference allowed' do
+      before do
+        expect(described_class).to receive(:cross_reference_disallowed?).and_return(false)
       end
 
-            it 'includes a commit count' do
-              expect(summary_line).to end_with " - 2 commits from branch `feature`"
-            end
-          end
+      describe 'note_body' do
+        context 'cross-project' do
+          let(:project2)  { create(:project) }
+          let(:mentioner) { create(:issue, project: project2) }
 
-          context 'on a fork' do
-            before do
-              expect(noteable).to receive(:for_fork?).and_return(true)
-            end
+          context 'from Commit' do
+            let(:mentioner) { project2.repository.commit }
 
-            it 'includes the project namespace' do
-              expect(summary_line).to end_with "`#{noteable.target_project_namespace}:feature`"
-            end
+            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
 
-  describe '.status_change' do
-    let(:status) { 'new_status' }
-    let(:source) { nil }
-
-    subject { described_class.status_change(noteable, project, author, status, source) }
-
-    it_behaves_like 'a system note'
-
-    context 'with a source' do
-      let(:source) { double('commit', gfm_reference: 'commit 123456') }
+        context 'same project' do
+          context 'from Commit' do
+            let(:mentioner) { project.repository.commit }
 
-      it 'sets the note text' do
-        expect(subject.note).to eq "Status changed to #{status} by commit 123456"
+            it 'references the mentioning commit' do
+              expect(subject.note).to eq "mentioned in commit #{mentioner.id}"
             end
           end
 
-    context 'without a source' do
-      it 'sets the note text' do
-        expect(subject.note).to eq "Status changed to #{status}"
+          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
@@ -274,6 +274,7 @@ describe SystemNoteService do
     end
   end
 
+  # TODO (rspeicher)
   describe '.cross_reference_disallowed?'
 
   describe '.cross_reference_exists?' do