From 5d08a5a56aea744a8adff833379f0b90ba9427db Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Sat, 18 Apr 2015 18:44:46 -0400
Subject: [PATCH 01/11] Note's voting specs don't need to persist to the
 database

---
 spec/models/note_spec.rb | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 4a6bfdb2910..43bfd25a48f 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -34,48 +34,46 @@ describe Note do
     it { is_expected.to validate_presence_of(:project) }
   end
 
-  describe "Voting score" do
-    let(:project) { create(:project) }
-
-    it "recognizes a neutral note" do
-      note = create(:votable_note, note: "This is not a +1 note")
+  describe 'voting score' do
+    it 'recognizes a neutral note' do
+      note = build(:votable_note, note: 'This is not a +1 note')
       expect(note).not_to be_upvote
       expect(note).not_to be_downvote
     end
 
-    it "recognizes a neutral emoji note" do
+    it 'recognizes a neutral emoji note' do
       note = build(:votable_note, note: "I would :+1: this, but I don't want to")
       expect(note).not_to be_upvote
       expect(note).not_to be_downvote
     end
 
-    it "recognizes a +1 note" do
-      note = create(:votable_note, note: "+1 for this")
+    it 'recognizes a +1 note' do
+      note = build(:votable_note, note: '+1 for this')
       expect(note).to be_upvote
     end
 
-    it "recognizes a +1 emoji as a vote" do
-      note = build(:votable_note, note: ":+1: for this")
+    it 'recognizes a +1 emoji as a vote' do
+      note = build(:votable_note, note: ':+1: for this')
       expect(note).to be_upvote
     end
 
-    it "recognizes a thumbsup emoji as a vote" do
-      note = build(:votable_note, note: ":thumbsup: for this")
+    it 'recognizes a thumbsup emoji as a vote' do
+      note = build(:votable_note, note: ':thumbsup: for this')
       expect(note).to be_upvote
     end
 
-    it "recognizes a -1 note" do
-      note = create(:votable_note, note: "-1 for this")
+    it 'recognizes a -1 note' do
+      note = build(:votable_note, note: '-1 for this')
       expect(note).to be_downvote
     end
 
-    it "recognizes a -1 emoji as a vote" do
-      note = build(:votable_note, note: ":-1: for this")
+    it 'recognizes a -1 emoji as a vote' do
+      note = build(:votable_note, note: ':-1: for this')
       expect(note).to be_downvote
     end
 
-    it "recognizes a thumbsdown emoji as a vote" do
-      note = build(:votable_note, note: ":thumbsdown: for this")
+    it 'recognizes a thumbsdown emoji as a vote' do
+      note = build(:votable_note, note: ':thumbsdown: for this')
       expect(note).to be_downvote
     end
   end

From 0e89ff0fb05973bb3ff6930906d52f10517efa62 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Sat, 18 Apr 2015 18:45:12 -0400
Subject: [PATCH 02/11] Simplify `Note#upvote?` and `Note#downvote?`

---
 app/models/note.rb | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/app/models/note.rb b/app/models/note.rb
index cbce6786683..b32c32e2a20 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -449,16 +449,6 @@ class Note < ActiveRecord::Base
     @discussion_id ||= Note.build_discussion_id(noteable_type, noteable_id || commit_id, line_code)
   end
 
-  # Returns true if this is a downvote note,
-  # otherwise false is returned
-  def downvote?
-    votable? && (note.start_with?('-1') ||
-                 note.start_with?(':-1:') ||
-                 note.start_with?(':thumbsdown:') ||
-                 note.start_with?(':thumbs_down_sign:')
-                )
-  end
-
   def for_commit?
     noteable_type == "Commit"
   end
@@ -500,14 +490,18 @@ class Note < ActiveRecord::Base
     nil
   end
 
-  # Returns true if this is an upvote note,
-  # otherwise false is returned
+  DOWNVOTES = %w(-1 :-1: :thumbsdown: :thumbs_down_sign:)
+
+  # Check if the note is a downvote
+  def downvote?
+    votable? && note.start_with?(*DOWNVOTES)
+  end
+
+  UPVOTES = %w(+1 :+1: :thumbsup: :thumbs_up_sign:)
+
+  # Check if the note is an upvote
   def upvote?
-    votable? && (note.start_with?('+1') ||
-                 note.start_with?(':+1:') ||
-                 note.start_with?(':thumbsup:') ||
-                 note.start_with?(':thumbs_up_sign:')
-                )
+    votable? && note.start_with?(*UPVOTES)
   end
 
   def superceded?(notes)

From 48e6fb532a6655b98319403c5e8699f7daf0305e Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Thu, 30 Apr 2015 23:16:19 -0400
Subject: [PATCH 03/11] 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.
---
 app/models/concerns/mentionable.rb            |   2 +-
 app/models/note.rb                            | 215 +----------
 app/services/issuable_base_service.rb         |   6 +-
 app/services/issues/close_service.rb          |   2 +-
 app/services/issues/reopen_service.rb         |   2 +-
 app/services/merge_requests/base_service.rb   |   2 +-
 .../merge_requests/refresh_service.rb         |   5 +-
 app/services/system_note_service.rb           | 304 +++++++++++++++
 spec/factories/notes.rb                       |  15 +-
 spec/models/note_spec.rb                      | 256 -------------
 spec/services/system_note_service_spec.rb     | 353 ++++++++++++++++++
 11 files changed, 683 insertions(+), 479 deletions(-)
 create mode 100644 app/services/system_note_service.rb
 create mode 100644 spec/services/system_note_service_spec.rb

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

From cdb69d728c208ba48ccaede6e0ee086ea224d852 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Sat, 9 May 2015 14:12:43 -0400
Subject: [PATCH 04/11] Add migration to convert legacy system notes

---
 ...50509180749_convert_legacy_reference_notes.rb | 16 ++++++++++++++++
 db/schema.rb                                     |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)
 create mode 100644 db/migrate/20150509180749_convert_legacy_reference_notes.rb

diff --git a/db/migrate/20150509180749_convert_legacy_reference_notes.rb b/db/migrate/20150509180749_convert_legacy_reference_notes.rb
new file mode 100644
index 00000000000..b02605489be
--- /dev/null
+++ b/db/migrate/20150509180749_convert_legacy_reference_notes.rb
@@ -0,0 +1,16 @@
+# Convert legacy Markdown-emphasized notes to the current, non-emphasized format
+#
+#   _mentioned in 54f7727c850972f0401c1312a7c4a6a380de5666_
+#
+# becomes
+#
+#   mentioned in 54f7727c850972f0401c1312a7c4a6a380de5666
+class ConvertLegacyReferenceNotes < ActiveRecord::Migration
+  def up
+    execute %q{UPDATE notes SET note = trim(both '_' from note) WHERE system = true AND note LIKE '\_%\_'}
+  end
+
+  def down
+    # noop
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 04abf9bb9a6..199b959a8dc 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20150502064022) do
+ActiveRecord::Schema.define(version: 20150509180749) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"

From ff3a62aad171e4211dd9c5c6e762b1b32f9921ac Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Sat, 9 May 2015 18:09:34 -0400
Subject: [PATCH 05/11] Remove legacy special case for emphasized reference
 notes

---
 app/services/system_note_service.rb       | 22 ++++++--------
 spec/services/system_note_service_spec.rb | 36 -----------------------
 2 files changed, 9 insertions(+), 49 deletions(-)

diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 2486360b261..c4bc2339e7b 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -196,7 +196,7 @@ class SystemNoteService
     end
 
     gfm_reference = mentioner_gfm_ref(noteable, mentioner, true)
-    notes = notes.where('note like ?', cross_reference_note_pattern(gfm_reference))
+    notes = notes.where(note: cross_reference_note_content(gfm_reference))
 
     notes.count > 0
   end
@@ -207,17 +207,14 @@ class SystemNoteService
     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
+    # FIXME (rspeicher): This was breaking things.
+    # if mentioner.is_a?(Commit) && cross_reference
+    #   return mentioner.gfm_reference.sub('commit ', 'commit %')
+    # end
 
     full_gfm_reference(mentioner.project, noteable.project, mentioner)
   end
@@ -243,13 +240,12 @@ class SystemNoteService
     end
   end
 
-  def self.cross_reference_note_content(gfm_reference)
-    cross_reference_note_prefix + "#{gfm_reference}"
+  def self.cross_reference_note_prefix
+    'mentioned in '
   end
 
-  def self.cross_reference_note_pattern(gfm_reference)
-    # Older cross reference notes contained underscores for emphasis
-    "%" + cross_reference_note_content(gfm_reference) + "%"
+  def self.cross_reference_note_content(gfm_reference)
+    "#{cross_reference_note_prefix}#{gfm_reference}"
   end
 
   # Build an Array of lines detailing each commit added in a merge request
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index cda4e7698f1..194687cbbd6 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -313,41 +313,5 @@ describe SystemNoteService do
           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

From 686f6855c25ecfc09fdf199cb7e3fb7dd787ed28 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Sat, 9 May 2015 18:18:50 -0400
Subject: [PATCH 06/11] Update SystemNoteService method naming conventions

Now the verb comes first, and there is no restriction on
singular/plural.
---
 app/services/issuable_base_service.rb         |   6 +-
 app/services/issues/close_service.rb          |   2 +-
 app/services/issues/reopen_service.rb         |   2 +-
 app/services/merge_requests/base_service.rb   |   2 +-
 .../merge_requests/refresh_service.rb         |   6 +-
 app/services/system_note_service.rb           | 220 +++++++------
 spec/services/system_note_service_spec.rb     | 293 +++++++++---------
 7 files changed, 264 insertions(+), 267 deletions(-)

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

From b94c6d9f5b48c9ea42cb3098c9b8de8c47fa76a2 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Sat, 9 May 2015 18:19:41 -0400
Subject: [PATCH 07/11] Check if `system` is truthy in `Note.cross_reference?`

---
 app/models/note.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/note.rb b/app/models/note.rb
index f2470e5f399..6939a7e73a0 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -99,7 +99,7 @@ class Note < ActiveRecord::Base
   end
 
   def cross_reference?
-    SystemNoteService.cross_reference?(note)
+    system && SystemNoteService.cross_reference?(note)
   end
 
   def max_attachment_size

From 19142f407990b7b8a8d74c68ba30bef066e01aa4 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Sun, 10 May 2015 22:57:44 -0400
Subject: [PATCH 08/11] Simplify Note model specs

---
 spec/factories/notes.rb  |   4 +-
 spec/models/note_spec.rb | 247 ++++++---------------------------------
 2 files changed, 36 insertions(+), 215 deletions(-)

diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index 1e4c0b03f7b..e1009d5916e 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -34,7 +34,7 @@ FactoryGirl.define do
     factory :system_note,                traits: [:system]
 
     trait :on_commit do
-      project factory: :project
+      project
       commit_id RepoHelpers.sample_commit.id
       noteable_type "Commit"
     end
@@ -44,7 +44,7 @@ FactoryGirl.define do
     end
 
     trait :on_merge_request do
-      project factory: :project
+      project
       noteable_id 1
       noteable_type "MergeRequest"
     end
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 4a769f36771..ddacba58261 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -20,20 +20,44 @@
 require 'spec_helper'
 
 describe Note do
-  describe "Associations" do
+  describe 'associations' do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:noteable) }
     it { is_expected.to belong_to(:author).class_name('User') }
   end
 
-  describe "Mass assignment" do
-  end
-
-  describe "Validation" do
+  describe 'validation' do
     it { is_expected.to validate_presence_of(:note) }
     it { is_expected.to validate_presence_of(:project) }
   end
 
+  describe '#votable?' do
+    it 'is true for issue notes' do
+      note = build(:note_on_issue)
+      expect(note).to be_votable
+    end
+
+    it 'is true for merge request notes' do
+      note = build(:note_on_merge_request)
+      expect(note).to be_votable
+    end
+
+    it 'is false for merge request diff notes' do
+      note = build(:note_on_merge_request_diff)
+      expect(note).not_to be_votable
+    end
+
+    it 'is false for commit notes' do
+      note = build(:note_on_commit)
+      expect(note).not_to be_votable
+    end
+
+    it 'is false for commit diff notes' do
+      note = build(:note_on_commit_diff)
+      expect(note).not_to be_votable
+    end
+  end
+
   describe 'voting score' do
     it 'recognizes a neutral note' do
       note = build(:votable_note, note: 'This is not a +1 note')
@@ -78,8 +102,6 @@ describe Note do
     end
   end
 
-  let(:project) { create(:project) }
-
   describe "Commit notes" do
     let!(:note) { create(:note_on_commit, note: "+1 from me") }
     let!(:commit) { note.noteable }
@@ -98,10 +120,6 @@ describe Note do
     it "should be recognized by #for_commit?" do
       expect(note).to be_for_commit
     end
-
-    it "should not be votable" do
-      expect(note).not_to be_votable
-    end
   end
 
   describe "Commit diff line notes" do
@@ -126,205 +144,7 @@ describe Note do
     end
   end
 
-  describe "Issue notes" do
-    let!(:note) { create(:note_on_issue, note: "+1 from me") }
-
-    it "should not be votable" do
-      expect(note).to be_votable
-    end
-  end
-
-  describe "Merge request notes" do
-    let!(:note) { create(:note_on_merge_request, note: "+1 from me") }
-
-    it "should be votable" do
-      expect(note).to be_votable
-    end
-  end
-
-  describe "Merge request diff line notes" do
-    let!(:note) { create(:note_on_merge_request_diff, note: "+1 from me") }
-
-    it "should not be votable" do
-      expect(note).not_to be_votable
-    end
-  end
-
-  describe '#create_cross_reference_note' do
-    let(:project)    { create(:project) }
-    let(:author)     { create(:user) }
-    let(:issue)      { create(:issue, project: project) }
-    let(:mergereq)   { create(:merge_request, :simple, target_project: project, source_project: project) }
-    let(:commit)     { project.commit }
-
-    # Test all of {issue, merge request, commit} in both the referenced and referencing
-    # roles, to ensure that the correct information can be inferred from any argument.
-
-    context 'issue from a merge request' do
-      subject { Note.create_cross_reference_note(issue, mergereq, author) }
-
-      it { is_expected.to be_valid }
-
-      describe '#noteable' do
-        subject { super().noteable }
-        it { is_expected.to eq(issue) }
-      end
-
-      describe '#project' do
-        subject { super().project }
-        it { is_expected.to eq(issue.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("mentioned in merge request !#{mergereq.iid}") }
-      end
-    end
-
-    context 'issue from a commit' do
-      subject { Note.create_cross_reference_note(issue, commit, author) }
-
-      it { is_expected.to be_valid }
-
-      describe '#noteable' do
-        subject { super().noteable }
-        it { is_expected.to eq(issue) }
-      end
-
-      describe '#note' do
-        subject { super().note }
-        it { is_expected.to eq("mentioned in commit #{commit.sha}") }
-      end
-    end
-
-    context 'merge request from an issue' do
-      subject { Note.create_cross_reference_note(mergereq, issue, author) }
-
-      it { is_expected.to be_valid }
-
-      describe '#noteable' do
-        subject { super().noteable }
-        it { is_expected.to eq(mergereq) }
-      end
-
-      describe '#project' do
-        subject { super().project }
-        it { is_expected.to eq(mergereq.project) }
-      end
-
-      describe '#note' do
-        subject { super().note }
-        it { is_expected.to eq("mentioned in issue ##{issue.iid}") }
-      end
-    end
-
-    context 'commit from a merge request' do
-      subject { Note.create_cross_reference_note(commit, mergereq, author) }
-
-      it { is_expected.to be_valid }
-
-      describe '#noteable' do
-        subject { super().noteable }
-        it { is_expected.to eq(commit) }
-      end
-
-      describe '#project' do
-        subject { super().project }
-        it { is_expected.to eq(project) }
-      end
-
-      describe '#note' do
-        subject { super().note }
-        it { is_expected.to eq("mentioned in merge request !#{mergereq.iid}") }
-      end
-    end
-
-    context 'commit contained in a merge request' do
-      subject { Note.create_cross_reference_note(mergereq.commits.first, mergereq, author) }
-
-      it { is_expected.to be_nil }
-    end
-
-    context 'commit from issue' do
-      subject { Note.create_cross_reference_note(commit, issue, author) }
-
-      it { is_expected.to be_valid }
-
-      describe '#noteable_type' do
-        subject { super().noteable_type }
-        it { is_expected.to eq("Commit") }
-      end
-
-      describe '#noteable_id' do
-        subject { super().noteable_id }
-        it { is_expected.to be_nil }
-      end
-
-      describe '#commit_id' do
-        subject { super().commit_id }
-        it { is_expected.to eq(commit.id) }
-      end
-
-      describe '#note' do
-        subject { super().note }
-        it { is_expected.to eq("mentioned in issue ##{issue.iid}") }
-      end
-    end
-
-    context 'commit from commit' do
-      let(:parent_commit) { commit.parents.first }
-      subject { Note.create_cross_reference_note(commit, parent_commit, author) }
-
-      it { is_expected.to be_valid }
-
-      describe '#noteable_type' do
-        subject { super().noteable_type }
-        it { is_expected.to eq("Commit") }
-      end
-
-      describe '#noteable_id' do
-        subject { super().noteable_id }
-        it { is_expected.to be_nil }
-      end
-
-      describe '#commit_id' do
-        subject { super().commit_id }
-        it { is_expected.to eq(commit.id) }
-      end
-
-      describe '#note' do
-        subject { super().note }
-        it { is_expected.to eq("mentioned in commit #{parent_commit.id}") }
-      end
-    end
-  end
-
-  describe '#system?' do
-    let(:project) { create(:project) }
-    let(:issue)   { create(:issue, project: project) }
-    let(:other)   { create(:issue, project: project) }
-    let(:author)  { create(:user) }
-    let(:assignee) { create(:user) }
-    let(:label) { create(:label) }
-    let(:milestone) { create(:milestone) }
-
-    it 'should recognize user-supplied notes as non-system' do
-      @note = create(:note_on_issue)
-      expect(@note).not_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
-  end
-
-  describe :authorization do
+  describe 'authorization' do
     before do
       @p1 = create(:project)
       @p2 = create(:project)
@@ -335,7 +155,7 @@ describe Note do
       @abilities << Ability
     end
 
-    describe :read do
+    describe 'read' do
       before do
         @p1.project_members.create(user: @u2, access_level: ProjectMember::GUEST)
         @p2.project_members.create(user: @u3, access_level: ProjectMember::GUEST)
@@ -346,7 +166,7 @@ describe Note do
       it { expect(@abilities.allowed?(@u3, :read_note, @p1)).to be_falsey }
     end
 
-    describe :write do
+    describe 'write' do
       before do
         @p1.project_members.create(user: @u2, access_level: ProjectMember::DEVELOPER)
         @p2.project_members.create(user: @u3, access_level: ProjectMember::DEVELOPER)
@@ -357,7 +177,7 @@ describe Note do
       it { expect(@abilities.allowed?(@u3, :write_note, @p1)).to be_falsey }
     end
 
-    describe :admin do
+    describe 'admin' do
       before do
         @p1.project_members.create(user: @u1, access_level: ProjectMember::REPORTER)
         @p1.project_members.create(user: @u2, access_level: ProjectMember::MASTER)
@@ -373,6 +193,7 @@ describe Note do
   it_behaves_like 'an editable mentionable' do
     subject { create :note, noteable: issue, project: project }
 
+    let(:project) { create(:project) }
     let(:issue) { create :issue, project: project }
     let(:backref_text) { issue.gfm_reference }
     let(:set_mentionable_text) { ->(txt) { subject.note = txt } }

From 83904275831511f6b17b33064255b669604e0e74 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Sun, 10 May 2015 23:51:49 -0400
Subject: [PATCH 09/11] Spec SystemNoteService.cross_reference_disallowed?

---
 spec/services/system_note_service_spec.rb | 28 +++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 3e9528a83d0..6ddec8e67b1 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -274,8 +274,32 @@ describe SystemNoteService do
     end
   end
 
-  # TODO (rspeicher)
-  describe '.cross_reference_disallowed?'
+  describe '.cross_reference_disallowed?' do
+    context 'when mentioner is not a MergeRequest' do
+      it 'is falsey' do
+        mentioner = noteable.dup
+        expect(described_class.cross_reference_disallowed?(noteable, mentioner)).
+          to be_falsey
+      end
+    end
+
+    context 'when mentioner is a MergeRequest' do
+      let(:mentioner) { create(:merge_request, :simple, source_project: project) }
+      let(:noteable)  { project.commit }
+
+      it 'is truthy when noteable is in commits' do
+        expect(mentioner).to receive(:commits).and_return([noteable])
+        expect(described_class.cross_reference_disallowed?(noteable, mentioner)).
+          to be_truthy
+      end
+
+      it 'is falsey when noteable is not in commits' do
+        expect(mentioner).to receive(:commits).and_return([])
+        expect(described_class.cross_reference_disallowed?(noteable, mentioner)).
+          to be_falsey
+      end
+    end
+  end
 
   describe '.cross_reference_exists?' do
     let(:commit0) { project.commit }

From 3e8efce360e3dadac69554d0bdebdcfde918f6e7 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Sun, 10 May 2015 23:52:03 -0400
Subject: [PATCH 10/11] Refactor SystemNoteService.cross_reference_disallowed?

---
 app/services/system_note_service.rb | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index a1de050ca5a..0614f8689a4 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -171,13 +171,20 @@ class SystemNoteService
     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.
+  # Check if a cross-reference is disallowed
+  #
+  # This method prevents adding a "mentioned in !1" note on every single commit
+  # in a merge request.
+  #
+  # noteable  - Noteable object being referenced
+  # mentioner - Mentionable object
+  #
+  # Returns Boolean
   def self.cross_reference_disallowed?(noteable, mentioner)
-    if mentioner.kind_of?(MergeRequest)
-      mentioner.commits.map(&:id).include? noteable.id
-    end
+    return false unless MergeRequest === mentioner
+    return false unless Commit === noteable
+
+    mentioner.commits.include?(noteable)
   end
 
   def self.cross_reference_exists?(noteable, mentioner)

From 54db527b4c44b102b81f4614bee6b06d2b742f6a Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Sun, 10 May 2015 23:52:46 -0400
Subject: [PATCH 11/11] Reorganize SystemNoteService spec a bit

---
 spec/services/system_note_service_spec.rb | 30 +++++++++++++----------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 6ddec8e67b1..4e4cb6d19ed 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -6,6 +6,10 @@ describe SystemNoteService do
   let(:noteable) { create(:issue, project: project) }
 
   shared_examples_for 'a system note' do
+    it 'is valid' do
+      expect(subject).to be_valid
+    end
+
     it 'sets the noteable model' do
       expect(subject.noteable).to eq noteable
     end
@@ -24,13 +28,13 @@ describe SystemNoteService do
   end
 
   describe '.add_commits' do
+    subject { described_class.add_commits(noteable, project, author, new_commits, old_commits, oldrev) }
+
     let(:noteable)    { create(:merge_request, source_project: project) }
     let(:new_commits) { noteable.commits }
     let(:old_commits) { [] }
     let(:oldrev)      { nil }
 
-    subject { described_class.add_commits(noteable, project, author, new_commits, old_commits, oldrev) }
-
     it_behaves_like 'a system note'
 
     describe 'note body' do
@@ -100,10 +104,10 @@ describe SystemNoteService do
   end
 
   describe '.change_assignee' do
-    let(:assignee) { create(:user) }
-
     subject { described_class.change_assignee(noteable, project, author, assignee) }
 
+    let(:assignee) { create(:user) }
+
     it_behaves_like 'a system note'
 
     context 'when assignee added' do
@@ -122,12 +126,12 @@ describe SystemNoteService do
   end
 
   describe '.change_label' do
+    subject { described_class.change_label(noteable, project, author, added, removed) }
+
     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
@@ -159,10 +163,10 @@ describe SystemNoteService do
   end
 
   describe '.change_milestone' do
-    let(:milestone) { create(:milestone, project: project) }
-
     subject { described_class.change_milestone(noteable, project, author, milestone) }
 
+    let(:milestone) { create(:milestone, project: project) }
+
     it_behaves_like 'a system note'
 
     context 'when milestone added' do
@@ -181,11 +185,11 @@ describe SystemNoteService do
   end
 
   describe '.change_status' do
+    subject { described_class.change_status(noteable, project, author, status, source) }
+
     let(:status) { 'new_status' }
     let(:source) { nil }
 
-    subject { described_class.change_status(noteable, project, author, status, source) }
-
     it_behaves_like 'a system note'
 
     context 'with a source' do
@@ -204,10 +208,10 @@ describe SystemNoteService do
   end
 
   describe '.cross_reference' do
-    let(:mentioner) { create(:issue, project: project) }
-
     subject { described_class.cross_reference(noteable, mentioner, author) }
 
+    let(:mentioner) { create(:issue, project: project) }
+
     it_behaves_like 'a system note'
 
     context 'when cross-reference disallowed' do
@@ -245,7 +249,7 @@ describe SystemNoteService do
           end
         end
 
-        context 'same project' do
+        context 'within the same project' do
           context 'from Commit' do
             let(:mentioner) { project.repository.commit }