More SystemNoteService cleanuprs-to_reference

3 files changed, 22 insertions, 46 deletions

diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb
index 9b299889476..6f9f54d08cc 100644
--- a/app/models/concerns/mentionable.rb
+++ b/app/models/concerns/mentionable.rb
@@ -24,11 +24,11 @@ module Mentionable
   #
   # By default this will be the class name and the result of calling
   # `to_reference` on the object.
-  def gfm_reference
+  def gfm_reference(from_project = nil)
     # "MergeRequest" > "merge_request" > "Merge request" > "merge request"
     friendly_name = self.class.to_s.underscore.humanize.downcase
 
-    "#{friendly_name} #{to_reference}"
+    "#{friendly_name} #{to_reference(from_project)}"
   end
 
   # Construct a String that contains possible GFM references.
diff --git a/app/models/note.rb b/app/models/note.rb
index 6939a7e73a0..d5f716b3de0 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -326,8 +326,8 @@ class Note < ActiveRecord::Base
   end
 
   # Mentionable override.
-  def gfm_reference
-    noteable.gfm_reference
+  def gfm_reference(from_project = nil)
+    noteable.gfm_reference(from_project)
   end
 
   # Mentionable override.
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 0614f8689a4..3d57c35bc1d 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -10,7 +10,7 @@ class SystemNoteService
   # 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
+  # oldrev           - Optional String SHA of a previous Commit
   #
   # See new_commit_summary and existing_commit_summary.
   #
@@ -138,11 +138,11 @@ class SystemNoteService
   #
   # Example Note text:
   #
-  #   "Mentioned in #1"
+  #   "mentioned in #1"
   #
-  #   "Mentioned in !2"
+  #   "mentioned in !2"
   #
-  #   "Mentioned in 54f7727c"
+  #   "mentioned in 54f7727c"
   #
   # See cross_reference_note_content.
   #
@@ -150,7 +150,7 @@ class SystemNoteService
   def self.cross_reference(noteable, mentioner, author)
     return if cross_reference_disallowed?(noteable, mentioner)
 
-    gfm_reference = mentioner_gfm_ref(noteable, mentioner)
+    gfm_reference = mentioner.gfm_reference(noteable.project)
 
     note_options = {
       project: noteable.project,
@@ -181,12 +181,21 @@ class SystemNoteService
   #
   # Returns Boolean
   def self.cross_reference_disallowed?(noteable, mentioner)
-    return false unless MergeRequest === mentioner
-    return false unless Commit === noteable
+    return false unless mentioner.is_a?(MergeRequest)
+    return false unless noteable.is_a?(Commit)
 
     mentioner.commits.include?(noteable)
   end
 
+  # Check if a cross reference to a noteable from a mentioner already exists
+  #
+  # This method is used to prevent multiple notes being created for a mention
+  # when a issue is updated, for example.
+  #
+  # noteable  - Noteable object being referenced
+  # mentioner - Mentionable object
+  #
+  # Returns Boolean
   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)
@@ -198,7 +207,7 @@ class SystemNoteService
       notes = notes.where(noteable_id: noteable.id)
     end
 
-    gfm_reference = mentioner_gfm_ref(noteable, mentioner, true)
+    gfm_reference = mentioner.gfm_reference(noteable.project)
     notes = notes.where(note: cross_reference_note_content(gfm_reference))
 
     notes.count > 0
@@ -210,39 +219,6 @@ class SystemNoteService
     Note.create(args.merge(system: true))
   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)
-    # 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
-
-  # 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_prefix
     'mentioned in '
   end
@@ -267,7 +243,7 @@ class SystemNoteService
   #
   # 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.
+  # oldrev           - Optional String SHA of a previous Commit
   #
   # Examples:
   #