Add a SystemNoteService class

There's a lot of code in the Note model that only deals with creating
system notes, so we're going to split that into its own class.

11 files changed, 683 insertions, 479 deletions

diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb
index 3ef3e8b67d8..a5957391bb7 100644
--- a/app/models/concerns/mentionable.rb
+++ b/app/models/concerns/mentionable.rb
@@ -39,7 +39,7 @@ module Mentionable
   # Determine whether or not a cross-reference Note has already been created between this Mentionable and
   # the specified target.
   def has_mentioned?(target)
-    Note.cross_reference_exists?(target, local_reference)
+    SystemNoteService.cross_reference_exists?(target, local_reference)
   end
 
   def mentioned_users(current_user = nil, p = project)
diff --git a/app/models/note.rb b/app/models/note.rb
index b32c32e2a20..f2470e5f399 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -63,143 +63,9 @@ class Note < ActiveRecord::Base
   after_update :set_references
 
   class << self
-    def create_status_change_note(noteable, project, author, status, source)
-      body = "Status changed to #{status}#{' by ' + source.gfm_reference if source}"
-
-      create(
-        noteable: noteable,
-        project: project,
-        author: author,
-        note: body,
-        system: true
-      )
-    end
-
-    # +noteable+ was referenced from +mentioner+, by including GFM in either
-    # +mentioner+'s description or an associated Note.
-    # Create a system Note associated with +noteable+ with a GFM back-reference
-    # to +mentioner+.
-    def create_cross_reference_note(noteable, mentioner, author)
-      gfm_reference = mentioner_gfm_ref(noteable, mentioner)
-
-      note_options = {
-        project: noteable.project,
-        author: author,
-        note: cross_reference_note_content(gfm_reference),
-        system: true
-      }
-
-      if noteable.kind_of?(Commit)
-        note_options.merge!(noteable_type: 'Commit', commit_id: noteable.id)
-      else
-        note_options.merge!(noteable: noteable)
-      end
-
-      create(note_options) unless cross_reference_disallowed?(noteable, mentioner)
-    end
-
-    def create_milestone_change_note(noteable, project, author, milestone)
-      body = if milestone.nil?
-               'Milestone removed'
-             else
-               "Milestone changed to #{milestone.title}"
-             end
-
-      create(
-        noteable: noteable,
-        project: project,
-        author: author,
-        note: body,
-        system: true
-      )
-    end
-
-    def create_assignee_change_note(noteable, project, author, assignee)
-      body = assignee.nil? ? 'Assignee removed' : "Reassigned to @#{assignee.username}"
-
-      create({
-        noteable: noteable,
-        project: project,
-        author: author,
-        note: body,
-        system: true
-      })
-    end
-
-    def create_labels_change_note(noteable, project, author, added_labels, removed_labels)
-      labels_count = added_labels.count + removed_labels.count
-      added_labels = added_labels.map{ |label| "~#{label.id}" }.join(' ')
-      removed_labels = removed_labels.map{ |label| "~#{label.id}" }.join(' ')
-      message = ''
-
-      if added_labels.present?
-        message << "added #{added_labels}"
-      end
-
-      if added_labels.present? && removed_labels.present?
-        message << ' and '
-      end
-
-      if removed_labels.present?
-        message << "removed #{removed_labels}"
-      end
-
-      message << ' ' << 'label'.pluralize(labels_count)
-      body = "#{message.capitalize}"
-
-      create(
-        noteable: noteable,
-        project: project,
-        author: author,
-        note: body,
-        system: true
-      )
-    end
-
-    def create_new_commits_note(merge_request, project, author, new_commits, existing_commits = [], oldrev = nil)
-      total_count = new_commits.length + existing_commits.length
-      commits_text = ActionController::Base.helpers.pluralize(total_count, 'commit')
-      body = "Added #{commits_text}:\n\n"
-
-      if existing_commits.length > 0
-        commit_ids =
-          if existing_commits.length == 1
-            existing_commits.first.short_id
-          else
-            if oldrev
-              "#{Commit.truncate_sha(oldrev)}...#{existing_commits.last.short_id}"
-            else
-              "#{existing_commits.first.short_id}..#{existing_commits.last.short_id}"
-            end
-          end
-
-        commits_text = ActionController::Base.helpers.pluralize(existing_commits.length, 'commit')
-
-        branch =
-          if merge_request.for_fork?
-            "#{merge_request.target_project_namespace}:#{merge_request.target_branch}"
-          else
-            merge_request.target_branch
-          end
-
-        message = "* #{commit_ids} - #{commits_text} from branch `#{branch}`"
-        body << message
-        body << "\n"
-      end
-
-      new_commits.each do |commit|
-        message = "* #{commit.short_id} - #{commit.title}"
-        body << message
-        body << "\n"
-      end
-
-      create(
-        noteable: merge_request,
-        project: project,
-        author: author,
-        note: body,
-        system: true
-      )
+    # TODO (rspeicher): Update usages
+    def create_cross_reference_note(*args)
+      SystemNoteService.cross_reference(*args)
     end
 
     def discussions_from_notes(notes)
@@ -227,88 +93,19 @@ class Note < ActiveRecord::Base
       [:discussion, type.try(:underscore), id, line_code].join("-").to_sym
     end
 
-    # Determine if cross reference note should be created.
-    # eg. mentioning a commit in MR comments which exists inside a MR
-    # should not create "mentioned in" note.
-    def cross_reference_disallowed?(noteable, mentioner)
-      if mentioner.kind_of?(MergeRequest)
-        mentioner.commits.map(&:id).include? noteable.id
-      end
-    end
-
-    # Determine whether or not a cross-reference note already exists.
-    def cross_reference_exists?(noteable, mentioner)
-      gfm_reference = mentioner_gfm_ref(noteable, mentioner, true)
-      notes = if noteable.is_a?(Commit)
-                where(commit_id: noteable.id, noteable_type: 'Commit')
-              else
-                where(noteable_id: noteable.id, noteable_type: noteable.class)
-              end
-
-      notes.where('note like ?', cross_reference_note_pattern(gfm_reference)).
-        system.any?
-    end
-
     def search(query)
       where("note like :query", query: "%#{query}%")
     end
+  end
 
-    def cross_reference_note_prefix
-      'mentioned in '
-    end
-
-    private
-
-    def cross_reference_note_content(gfm_reference)
-      cross_reference_note_prefix + "#{gfm_reference}"
-    end
-
-    def cross_reference_note_pattern(gfm_reference)
-      # Older cross reference notes contained underscores for emphasis
-      "%" + cross_reference_note_content(gfm_reference) + "%"
-    end
-
-    # Prepend the mentioner's namespaced project path to the GFM reference for
-    # cross-project references.  For same-project references, return the
-    # unmodified GFM reference.
-    def mentioner_gfm_ref(noteable, mentioner, cross_reference = false)
-      if mentioner.is_a?(Commit) && cross_reference
-        return mentioner.gfm_reference.sub('commit ', 'commit %')
-      end
-      
-      full_gfm_reference(mentioner.project, noteable.project, mentioner)
-    end
-
-    # Return the +mentioner+ GFM reference.  If the mentioner and noteable
-    # projects are not the same, add the mentioning project's path to the
-    # returned value.
-    def full_gfm_reference(mentioning_project, noteable_project, mentioner)
-      if mentioning_project == noteable_project
-        mentioner.gfm_reference
-      else
-        if mentioner.is_a?(Commit)
-          mentioner.gfm_reference.sub(
-            /(commit )/,
-            "\\1#{mentioning_project.path_with_namespace}@"
-          )
-        else
-          mentioner.gfm_reference.sub(
-            /(issue |merge request )/,
-            "\\1#{mentioning_project.path_with_namespace}"
-          )
-        end
-      end
-    end
+  def cross_reference?
+    SystemNoteService.cross_reference?(note)
   end
 
   def max_attachment_size
     current_application_settings.max_attachment_size.megabytes.to_i
   end
 
-  def cross_reference?
-    note.start_with?(self.class.cross_reference_note_prefix)
-  end
-
   def find_diff
     return nil unless noteable && noteable.diffs.present?
 
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 5e1906ad2ae..4d251cb30db 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)
-    Note.create_assignee_change_note(
+    SystemNoteService.assignee_change(
       issuable, issuable.project, current_user, issuable.assignee)
   end
 
   def create_milestone_note(issuable)
-    Note.create_milestone_change_note(
+    SystemNoteService.milestone_change(
       issuable, issuable.project, current_user, issuable.milestone)
   end
 
   def create_labels_note(issuable, added_labels, removed_labels)
-    Note.create_labels_change_note(
+    SystemNoteService.label_change(
       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 f670019cc63..bdc13685fd0 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)
-      Note.create_status_change_note(issue, issue.project, current_user, issue.state, current_commit)
+      SystemNoteService.status_change(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 1e5c398516d..fbbf573e5a3 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)
-      Note.create_status_change_note(issue, issue.project, current_user, issue.state, nil)
+      SystemNoteService.status_change(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 f6e1ae6f283..9b470880adf 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)
-      Note.create_status_change_note(merge_request, merge_request.target_project, current_user, merge_request.state, nil)
+      SystemNoteService.status_change(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 e9b526d1fb7..1d57fd11de7 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -82,8 +82,9 @@ module MergeRequests
           mr_commit_ids.include?(commit.id)
         end
 
-        Note.create_new_commits_note(merge_request, merge_request.project,
-                                     @current_user, new_commits, existing_commits, @oldrev)
+        SystemNoteService.commit_add(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
new file mode 100644
index 00000000000..2486360b261
--- /dev/null
+++ b/app/services/system_note_service.rb
@@ -0,0 +1,304 @@
+# SystemNoteService
+#
+# 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
+  #
+  # noteable - Noteable object
+  # project  - Project owning noteable
+  # author   - User performing the change
+  # assignee - User being assigned, or nil
+  #
+  # Example Note text:
+  #
+  #   "Assignee removed"
+  #
+  #   "Reassigned to @rspeicher"
+  #
+  # Returns the created Note object
+  def self.assignee_change(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 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
+
+  # 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
+
+  # Determine if cross reference note should be created.
+  # eg. mentioning a commit in MR comments which exists inside a MR
+  # should not create "mentioned in" note.
+  def self.cross_reference_disallowed?(noteable, mentioner)
+    if mentioner.kind_of?(MergeRequest)
+      mentioner.commits.map(&:id).include? noteable.id
+    end
+  end
+
+  def self.cross_reference_exists?(noteable, mentioner)
+    # Initial scope should be system notes of this noteable type
+    notes = Note.system.where(noteable_type: noteable.class)
+
+    if noteable.is_a?(Commit)
+      # Commits have non-integer IDs, so they're stored in `commit_id`
+      notes = notes.where(commit_id: noteable.id)
+    else
+      notes = notes.where(noteable_id: noteable.id)
+    end
+
+    gfm_reference = mentioner_gfm_ref(noteable, mentioner, true)
+    notes = notes.where('note like ?', cross_reference_note_pattern(gfm_reference))
+
+    notes.count > 0
+  end
+
+  private
+
+  def self.create_note(args = {})
+    Note.create(args.merge(system: true))
+  end
+
+  def self.cross_reference_note_prefix
+    'mentioned in '
+  end
+
+  # Prepend the mentioner's namespaced project path to the GFM reference for
+  # cross-project references.  For same-project references, return the
+  # unmodified GFM reference.
+  def self.mentioner_gfm_ref(noteable, mentioner, cross_reference = false)
+    if mentioner.is_a?(Commit) && cross_reference
+      return mentioner.gfm_reference.sub('commit ', 'commit %')
+    end
+
+    full_gfm_reference(mentioner.project, noteable.project, mentioner)
+  end
+
+  # Return the +mentioner+ GFM reference.  If the mentioner and noteable
+  # projects are not the same, add the mentioning project's path to the
+  # returned value.
+  def self.full_gfm_reference(mentioning_project, noteable_project, mentioner)
+    if mentioning_project == noteable_project
+      mentioner.gfm_reference
+    else
+      if mentioner.is_a?(Commit)
+        mentioner.gfm_reference.sub(
+          /(commit )/,
+          "\\1#{mentioning_project.path_with_namespace}@"
+        )
+      else
+        mentioner.gfm_reference.sub(
+          /(issue |merge request )/,
+          "\\1#{mentioning_project.path_with_namespace}"
+        )
+      end
+    end
+  end
+
+  def self.cross_reference_note_content(gfm_reference)
+    cross_reference_note_prefix + "#{gfm_reference}"
+  end
+
+  def self.cross_reference_note_pattern(gfm_reference)
+    # Older cross reference notes contained underscores for emphasis
+    "%" + cross_reference_note_content(gfm_reference) + "%"
+  end
+
+  # Build an Array of lines detailing each commit added in a merge request
+  #
+  # new_commits - Array of new Commit objects
+  #
+  # Returns an Array of Strings
+  def self.new_commit_summary(new_commits)
+    new_commits.collect do |commit|
+      "* #{commit.short_id} - #{commit.title}"
+    end
+  end
+
+  # Build a single line summarizing existing commits being added in a merge
+  # request
+  #
+  # noteable         - MergeRequest object
+  # existing_commits - Array of existing Commit objects
+  # oldrev           - Optional String SHA of ... TODO (rspeicher): I have no idea what this actually does.
+  #
+  # Examples:
+  #
+  #   "* ea0f8418...2f4426b7 - 24 commits from branch `master`"
+  #
+  #   "* ea0f8418..4188f0ea - 15 commits from branch `fork:master`"
+  #
+  #   "* ea0f8418 - 1 commit from branch `feature`"
+  #
+  # Returns a newline-terminated String
+  def self.existing_commit_summary(noteable, existing_commits, oldrev = nil)
+    return '' if existing_commits.empty?
+
+    count = existing_commits.size
+
+    commit_ids = if count == 1
+                   existing_commits.first.short_id
+                 else
+                   if oldrev
+                     "#{Commit.truncate_sha(oldrev)}...#{existing_commits.last.short_id}"
+                   else
+                     "#{existing_commits.first.short_id}..#{existing_commits.last.short_id}"
+                   end
+                 end
+
+    commits_text = "#{count} commit".pluralize(count)
+
+    branch = noteable.target_branch
+    branch = "#{noteable.target_project_namespace}:#{branch}" if noteable.for_fork?
+
+    "* #{commit_ids} - #{commits_text} from branch `#{branch}`\n"
+  end
+end
diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index f1c33461b55..1e4c0b03f7b 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -25,12 +25,13 @@ FactoryGirl.define do
     note "Note"
     author
 
-    factory :note_on_commit, traits: [:on_commit]
-    factory :note_on_commit_diff, traits: [:on_commit, :on_diff]
-    factory :note_on_issue, traits: [:on_issue], aliases: [:votable_note]
-    factory :note_on_merge_request, traits: [:on_merge_request]
+    factory :note_on_commit,             traits: [:on_commit]
+    factory :note_on_commit_diff,        traits: [:on_commit, :on_diff]
+    factory :note_on_issue,              traits: [:on_issue], aliases: [:votable_note]
+    factory :note_on_merge_request,      traits: [:on_merge_request]
     factory :note_on_merge_request_diff, traits: [:on_merge_request, :on_diff]
-    factory :note_on_project_snippet, traits: [:on_project_snippet]
+    factory :note_on_project_snippet,    traits: [:on_project_snippet]
+    factory :system_note,                traits: [:system]
 
     trait :on_commit do
       project factory: :project
@@ -58,6 +59,10 @@ FactoryGirl.define do
       noteable_type "Snippet"
     end
 
+    trait :system do
+      system true
+    end
+
     trait :with_attachment do
       attachment { fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "`/png") }
     end
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 43bfd25a48f..4a769f36771 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -150,178 +150,6 @@ describe Note do
     end
   end
 
-  describe '#create_status_change_note' do
-    let(:project) { create(:project) }
-    let(:thing) { create(:issue, project: project) }
-    let(:author) { create(:user) }
-    let(:status) { 'new_status' }
-
-    subject { Note.create_status_change_note(thing, project, author, status, nil) }
-
-    it 'creates and saves a Note' do
-      is_expected.to be_a Note
-      expect(subject.id).not_to be_nil
-    end
-
-    describe '#noteable' do
-      subject { super().noteable }
-      it { is_expected.to eq(thing) }
-    end
-
-    describe '#project' do
-      subject { super().project }
-      it { is_expected.to eq(thing.project) }
-    end
-
-    describe '#author' do
-      subject { super().author }
-      it { is_expected.to eq(author) }
-    end
-
-    describe '#note' do
-      subject { super().note }
-      it { is_expected.to eq("Status changed to #{status}") }
-    end
-
-    it 'appends a back-reference if a closing mentionable is supplied' do
-      commit = double('commit', gfm_reference: 'commit 123456')
-      n = Note.create_status_change_note(thing, project, author, status, commit)
-
-      expect(n.note).to eq("Status changed to #{status} by commit 123456")
-    end
-  end
-
-  describe '#create_assignee_change_note' do
-    let(:project) { create(:project) }
-    let(:thing) { create(:issue, project: project) }
-    let(:author) { create(:user) }
-    let(:assignee) { create(:user, username: "assigned_user") }
-
-    subject { Note.create_assignee_change_note(thing, project, author, assignee) }
-
-    context 'creates and saves a Note' do
-      it { is_expected.to be_a Note }
-
-      describe '#id' do
-        subject { super().id }
-        it { is_expected.not_to be_nil }
-      end
-    end
-
-    describe '#noteable' do
-      subject { super().noteable }
-      it { is_expected.to eq(thing) }
-    end
-
-    describe '#project' do
-      subject { super().project }
-      it { is_expected.to eq(thing.project) }
-    end
-
-    describe '#author' do
-      subject { super().author }
-      it { is_expected.to eq(author) }
-    end
-
-    describe '#note' do
-      subject { super().note }
-      it { is_expected.to eq('Reassigned to @assigned_user') }
-    end
-
-    context 'assignee is removed' do
-      let(:assignee) { nil }
-
-      describe '#note' do
-        subject { super().note }
-        it { is_expected.to eq('Assignee removed') }
-      end
-    end
-  end
-
-  describe '#create_labels_change_note' do
-    let(:project) { create(:project) }
-    let(:thing) { create(:issue, project: project) }
-    let(:author) { create(:user) }
-    let(:label1) { create(:label) }
-    let(:label2) { create(:label) }
-    let(:added_labels) { [label1, label2] }
-    let(:removed_labels) { [] }
-
-    subject { Note.create_labels_change_note(thing, project, author, added_labels, removed_labels) }
-
-    context 'creates and saves a Note' do
-      it { is_expected.to be_a Note }
-
-      describe '#id' do
-        subject { super().id }
-        it { is_expected.not_to be_nil }
-      end
-    end
-
-    describe '#noteable' do
-      subject { super().noteable }
-      it { is_expected.to eq(thing) }
-    end
-
-    describe '#project' do
-      subject { super().project }
-      it { is_expected.to eq(thing.project) }
-    end
-
-    describe '#author' do
-      subject { super().author }
-      it { is_expected.to eq(author) }
-    end
-
-    describe '#note' do
-      subject { super().note }
-      it { is_expected.to eq("Added ~#{label1.id} ~#{label2.id} labels") }
-    end
-
-    context 'label is removed' do
-      let(:added_labels) { [label1]  }
-      let(:removed_labels) { [label2] }
-
-      describe '#note' do
-        subject { super().note }
-        it { is_expected.to eq("Added ~#{label1.id} and removed ~#{label2.id} labels") }
-      end
-    end
-  end
-
-  describe '#create_milestone_change_note' do
-    let(:project) { create(:project) }
-    let(:thing) { create(:issue, project: project) }
-    let(:milestone) { create(:milestone, project: project, title: "first_milestone") }
-    let(:author) { create(:user) }
-
-    subject { Note.create_milestone_change_note(thing, project, author, milestone) }
-
-    context 'creates and saves a Note' do
-      it { is_expected.to be_a Note }
-
-      describe '#id' do
-        subject { super().id }
-        it { is_expected.not_to be_nil }
-      end
-    end
-
-    describe '#project' do
-      subject { super().project }
-      it { is_expected.to eq(thing.project) }
-    end
-
-    describe '#author' do
-      subject { super().author }
-      it { is_expected.to eq(author) }
-    end
-
-    describe '#note' do
-      subject { super().note }
-      it { is_expected.to eq("Milestone changed to first_milestone") }
-    end
-  end
-
   describe '#create_cross_reference_note' do
     let(:project)    { create(:project) }
     let(:author)     { create(:user) }
@@ -476,70 +304,6 @@ describe Note do
     end
   end
 
-  describe '#cross_reference_exists?' do
-    let(:project) { create :project }
-    let(:author) { create :user }
-    let(:issue) { create :issue }
-    let(:commit0) { project.commit }
-    let(:commit1) { project.commit('HEAD~2') }
-
-    before do
-      Note.create_cross_reference_note(issue, commit0, author)
-    end
-
-    it 'detects if a mentionable has already been mentioned' do
-      expect(Note.cross_reference_exists?(issue, commit0)).to be_truthy
-    end
-
-    it 'detects if a mentionable has not already been mentioned' do
-      expect(Note.cross_reference_exists?(issue, commit1)).to be_falsey
-    end
-
-    context 'commit on commit' do
-      before do
-        Note.create_cross_reference_note(commit0, commit1, author)
-      end
-
-      it { expect(Note.cross_reference_exists?(commit0, commit1)).to be_truthy }
-      it { expect(Note.cross_reference_exists?(commit1, commit0)).to be_falsey }
-    end
-
-    context 'legacy note with Markdown emphasis' do
-      let(:issue2) { create :issue, project: project }
-      let!(:note) do
-        create :note, system: true, noteable_id: issue2.id,
-                      noteable_type: "Issue", note: "_mentioned in issue " \
-                      "#{issue.project.path_with_namespace}##{issue.iid}_"
-      end
-
-      it 'detects if a mentionable with emphasis has been mentioned' do
-        expect(Note.cross_reference_exists?(issue2, issue)).to be_truthy
-      end
-    end
-  end
-
-  describe '#cross_references_with_underscores?' do
-    let(:project) { create :project, path: "first_project" }
-    let(:second_project) { create :project, path: "second_project" }
-
-    let(:author) { create :user }
-    let(:issue0) { create :issue, project: project }
-    let(:issue1) { create :issue, project: second_project }
-    let!(:note) { Note.create_cross_reference_note(issue0, issue1, author) }
-
-    it 'detects if a mentionable has already been mentioned' do
-      expect(Note.cross_reference_exists?(issue0, issue1)).to be_truthy
-    end
-
-    it 'detects if a mentionable has not already been mentioned' do
-      expect(Note.cross_reference_exists?(issue1, issue0)).to be_falsey
-    end
-
-    it 'detects that text has underscores' do
-      expect(note.note).to eq("mentioned in issue #{second_project.path_with_namespace}##{issue1.iid}")
-    end
-  end
-
   describe '#system?' do
     let(:project) { create(:project) }
     let(:issue)   { create(:issue, project: project) }
@@ -554,30 +318,10 @@ describe Note do
       expect(@note).not_to be_system
     end
 
-    it 'should identify status-change notes as system notes' do
-      @note = Note.create_status_change_note(issue, project, author, 'closed', nil)
-      expect(@note).to be_system
-    end
-
     it 'should identify cross-reference notes as system notes' do
       @note = Note.create_cross_reference_note(issue, other, author)
       expect(@note).to be_system
     end
-
-    it 'should identify assignee-change notes as system notes' do
-      @note = Note.create_assignee_change_note(issue, project, author, assignee)
-      expect(@note).to be_system
-    end
-
-    it 'should identify label-change notes as system notes' do
-      @note = Note.create_labels_change_note(issue, project, author, [label], [])
-      expect(@note).to be_system
-    end
-
-    it 'should identify milestone-change notes as system notes' do
-      @note = Note.create_milestone_change_note(issue, project, author, milestone)
-      expect(@note).to be_system
-    end
   end
 
   describe :authorization do
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
new file mode 100644
index 00000000000..cda4e7698f1
--- /dev/null
+++ b/spec/services/system_note_service_spec.rb
@@ -0,0 +1,353 @@
+require 'spec_helper'
+
+describe SystemNoteService do
+  let(:project)  { create(:project) }
+  let(:author)   { create(:user) }
+  let(:noteable) { create(:issue, project: project) }
+
+  shared_examples_for 'a system note' do
+    it 'sets the noteable model' do
+      expect(subject.noteable).to eq noteable
+    end
+
+    it 'sets the project' do
+      expect(subject.project).to eq project
+    end
+
+    it 'sets the author' do
+      expect(subject.author).to eq author
+    end
+
+    it 'is a system note' do
+      expect(subject).to be_system
+    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
+    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) }
+
+    it_behaves_like 'a system note'
+
+    describe 'note body' do
+      let(:note_lines) { subject.note.split("\n").reject(&:blank?) }
+
+      context 'without existing commits' do
+        it 'adds a message header' do
+          expect(note_lines[0]).to eq "Added #{new_commits.size} commits:"
+        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}"
+          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
+
+        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 'includes a commit count' do
+              expect(summary_line).to end_with " - 2 commits from branch `feature`"
+            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}"
+            end
+
+            it 'includes a commit count' do
+              expect(summary_line).to end_with " - 2 commits from branch `feature`"
+            end
+          end
+
+          context 'on a fork' do
+            before do
+              expect(noteable).to receive(:for_fork?).and_return(true)
+            end
+
+            it 'includes the project namespace' do
+              expect(summary_line).to end_with "`#{noteable.target_project_namespace}:feature`"
+            end
+          end
+        end
+      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') }
+
+      it 'sets the note text' do
+        expect(subject.note).to eq "Status changed to #{status} by commit 123456"
+      end
+    end
+
+    context 'without a source' do
+      it 'sets the note text' do
+        expect(subject.note).to eq "Status changed to #{status}"
+      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
+    end
+
+    it 'is falsey when text does not begin with expected text' do
+      expect(described_class.cross_reference?('this is a note')).to be_falsey
+    end
+  end
+
+  describe '.cross_reference_disallowed?'
+
+  describe '.cross_reference_exists?' do
+    let(:commit0) { project.commit }
+    let(:commit1) { project.commit('HEAD~2') }
+
+    context 'issue from commit' do
+      before do
+        # Mention issue (noteable) from commit0
+        described_class.cross_reference(noteable, commit0, author)
+      end
+
+      it 'is truthy when already mentioned' do
+        expect(described_class.cross_reference_exists?(noteable, commit0)).
+          to be_truthy
+      end
+
+      it 'is falsey when not already mentioned' do
+        expect(described_class.cross_reference_exists?(noteable, commit1)).
+          to be_falsey
+      end
+    end
+
+    context 'commit from commit' do
+      before do
+        # Mention commit1 from commit0
+        described_class.cross_reference(commit0, commit1, author)
+      end
+
+      it 'is truthy when already mentioned' do
+        expect(described_class.cross_reference_exists?(commit0, commit1)).
+          to be_truthy
+      end
+
+      it 'is falsey when not already mentioned' do
+        expect(described_class.cross_reference_exists?(commit1, commit0)).
+          to be_falsey
+      end
+    end
+
+    context 'legacy note with Markdown emphasis' do
+      let(:mentioner) { create(:issue, project: project) }
+
+      before do
+        note = "_mentioned in issue ##{mentioner.iid}_"
+        create(:system_note, noteable: noteable, note: note, project: project)
+      end
+
+      it 'detects if a mentionable with emphasis has been mentioned' do
+        expect(described_class.cross_reference_exists?(noteable, mentioner)).
+          to be_truthy
+      end
+
+      context 'when referenced project has underscores' do
+        let(:project)  { create(:empty_project, path: 'first_project') }
+        let(:project2) { create(:empty_project, path: 'second_project') }
+
+        let(:issue)  { mentioner }
+        let(:issue2) { create(:issue, project: project2) }
+
+        before do
+          described_class.cross_reference(issue, issue2, author)
+        end
+
+        it 'is truthy when already mentioned' do
+          expect(described_class.cross_reference_exists?(issue, issue2)).
+            to be_truthy
+        end
+
+        it 'is falsey when not already mentioned' do
+          expect(described_class.cross_reference_exists?(issue2, issue)).
+            to be_falsey
+        end
+      end
+    end
+  end
+end