Consistently using iid when treating milestones as referrables

app/models/milestone.rb

@@ -83,10 +83,10 @@ class Milestone < ActiveRecord::Base
       (#{Project.reference_pattern})?
       #{Regexp.escape(reference_prefix)}
       (?:
-        (?<milestone_id>\d+) | # Integer-based milestone ID, or
+        (?<milestone_iid>\d+) | # Integer-based milestone iid, or
         (?<milestone_name>
-          [A-Za-z0-9_-]+ | # String-based single-word milestone title, or
-          "[^"]+"       # String-based multi-word milestone surrounded in quotes
+          [A-Za-z0-9_-]+ |      # String-based single-word milestone title, or
+          "[^"]+"               # String-based multi-word milestone surrounded in quotes
         )
       )
     }x
@@ -100,7 +100,18 @@ class Milestone < ActiveRecord::Base
     self.where('due_date > ?', Time.now).reorder(due_date: :asc).first
   end
 
-  def to_reference(from_project = nil, format: :id)
+  ##
+  # Returns the String necessary to reference this Milestone in Markdown
+  #
+  # format - Symbol format to use (default: :iid, optional: :name)
+  #
+  # Examples:
+  #
+  #   Milestone.first.to_reference                # => "%1"
+  #   Milestone.first.to_reference(format: :name) # => "%\"goal\""
+  #   Milestone.first.to_reference(project)       # => "gitlab-org/gitlab-ce%1"
+  #
+  def to_reference(from_project = nil, format: :iid)
     format_reference = milestone_format_reference(format)
     reference = "#{self.class.reference_prefix}#{format_reference}"
 
@@ -179,13 +190,13 @@ class Milestone < ActiveRecord::Base
 
   private
 
-  def milestone_format_reference(format = :id)
-    raise StandardError, 'Unknown format' unless [:id, :name].include?(format)
+  def milestone_format_reference(format = :iid)
+    raise StandardError, 'Unknown format' unless [:iid, :name].include?(format)
 
     if format == :name && !name.include?('"')
       %("#{name}")
     else
-      id
+      iid
     end
   end
 end

lib/banzai/filter/milestone_reference_filter.rb

@@ -7,17 +7,17 @@ module Banzai
       end
 
       def find_object(project, id)
-        project.milestones.find(id)
+        project.milestones.find_by(iid: id)
       end
 
       def references_in(text, pattern = Milestone.reference_pattern)
         text.gsub(pattern) do |match|
           project = project_from_ref($~[:project])
-          params = milestone_params($~[:milestone_id].to_i, $~[:milestone_name])
+          params = milestone_params($~[:milestone_iid].to_i, $~[:milestone_name])
           milestone = project.milestones.find_by(params)
 
           if milestone
-            yield match, milestone.id, $~[:project], $~
+            yield match, milestone.iid, $~[:project], $~
           else
             match
           end
@@ -30,11 +30,20 @@ module Banzai
                                         only_path: context[:only_path])
       end
 
-      def milestone_params(id, name)
+      def object_link_text(object, matches)
+        if context[:project] == object.project
+          super
+        else
+          "#{super} <i>in #{escape_once(object.project.name_with_namespace)}</i>".
+            html_safe
+        end
+      end
+
+      def milestone_params(iid, name)
         if name
           { name: name.tr('"', '') }
         else
-          { id: id }
+          { iid: iid }
         end
       end
     end

spec/lib/banzai/filter/milestone_reference_filter_spec.rb

@@ -3,8 +3,9 @@ require 'spec_helper'
 describe Banzai::Filter::MilestoneReferenceFilter, lib: true do
   include FilterSpecHelper
 
-  let(:project) { create(:project, :public) }
-  let(:milestone)   { create(:milestone, project: project) }
+  let(:project)   { create(:project, :public) }
+  let(:milestone) { create(:milestone, project: project) }
+  let(:reference) { milestone.to_reference }
 
   it 'requires project context' do
     expect { described_class.call('') }.to raise_error(ArgumentError, /:project/)
@@ -17,42 +18,126 @@ describe Banzai::Filter::MilestoneReferenceFilter, lib: true do
     end
   end
 
-  context 'internal reference' do
-    # Convert the Markdown link to only the URL, since these tests aren't run through the regular Markdown pipeline.
-    # Milestone reference behavior in the full Markdown pipeline is tested elsewhere.
-    let(:reference) { milestone.to_reference.gsub(/\[([^\]]+)\]\(([^)]+)\)/, '\2') }
+  it 'includes default classes' do
+    doc = reference_filter("Milestone #{reference}")
+    expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-milestone'
+  end
+
+  it 'includes a data-project attribute' do
+    doc = reference_filter("Milestone #{reference}")
+    link = doc.css('a').first
+
+    expect(link).to have_attribute('data-project')
+    expect(link.attr('data-project')).to eq project.id.to_s
+  end
+
+  it 'includes a data-milestone attribute' do
+    doc = reference_filter("See #{reference}")
+    link = doc.css('a').first
+
+    expect(link).to have_attribute('data-milestone')
+    expect(link.attr('data-milestone')).to eq milestone.id.to_s
+  end
+
+  it 'supports an :only_path context' do
+    doc = reference_filter("Milestone #{reference}", only_path: true)
+    link = doc.css('a').first.attr('href')
+
+    expect(link).not_to match %r(https?://)
+    expect(link).to eq urls.
+      namespace_project_milestone_path(project.namespace, project, milestone)
+  end
+
+  it 'adds to the results hash' do
+    result = reference_pipeline_result("Milestone #{reference}")
+    expect(result[:references][:milestone]).to eq [milestone]
+  end
+
+  context 'Integer-based references' do
+    it 'links to a valid reference' do
+      doc = reference_filter("See #{reference}")
+
+      expect(doc.css('a').first.attr('href')).to eq urls.
+        namespace_project_milestone_url(project.namespace, project, milestone)
+    end
+
+    it 'links with adjacent text' do
+      doc = reference_filter("Milestone (#{reference}.)")
+      expect(doc.to_html).to match(%r(\(<a.+>#{milestone.name}</a>\.\)))
+    end
+
+    it 'ignores invalid milestone IIDs' do
+      exp = act = "Milestone #{invalidate_reference(reference)}"
+
+      expect(reference_filter(act).to_html).to eq exp
+    end
+  end
+
+  context 'String-based single-word references' do
+    let(:milestone) { create(:milestone, name: 'gfm', project: project) }
+    let(:reference) { "#{Milestone.reference_prefix}#{milestone.name}" }
+
+    it 'links to a valid reference' do
+      doc = reference_filter("See #{reference}")
+
+      expect(doc.css('a').first.attr('href')).to eq urls.
+        namespace_project_milestone_url(project.namespace, project, milestone)
+      expect(doc.text).to eq 'See gfm'
+    end
+
+    it 'links with adjacent text' do
+      doc = reference_filter("Milestone (#{reference}.)")
+      expect(doc.to_html).to match(%r(\(<a.+>#{milestone.name}</a>\.\)))
+    end
+
+    it 'ignores invalid milestone names' do
+      exp = act = "Milestone #{Milestone.reference_prefix}#{milestone.name.reverse}"
+
+      expect(reference_filter(act).to_html).to eq exp
+    end
+  end
+
+  context 'String-based multi-word references in quotes' do
+    let(:milestone) { create(:milestone, name: 'gfm references', project: project) }
+    let(:reference) { milestone.to_reference(format: :name) }
 
     it 'links to a valid reference' do
       doc = reference_filter("See #{reference}")
 
       expect(doc.css('a').first.attr('href')).to eq urls.
         namespace_project_milestone_url(project.namespace, project, milestone)
+      expect(doc.text).to eq 'See gfm references'
     end
 
     it 'links with adjacent text' do
-      doc = reference_filter("milestone (#{reference}.)")
-      expect(doc.to_html).to match(/\(<a.+>#{Regexp.escape(milestone.title)}<\/a>\.\)/)
+      doc = reference_filter("Milestone (#{reference}.)")
+      expect(doc.to_html).to match(%r(\(<a.+>#{milestone.name}</a>\.\)))
     end
 
-    it 'includes a title attribute' do
-      doc = reference_filter("milestone #{reference}")
-      expect(doc.css('a').first.attr('title')).to eq "Milestone: #{milestone.title}"
+    it 'ignores invalid milestone names' do
+      exp = act = %(Milestone #{Milestone.reference_prefix}"#{milestone.name.reverse}")
+
+      expect(reference_filter(act).to_html).to eq exp
     end
+  end
 
-    it 'escapes the title attribute' do
-      milestone.update_attribute(:title, %{"></a>whatever<a title="})
+  describe 'referencing a milestone in a link href' do
+    let(:reference) { %Q{<a href="#{milestone.to_reference}">Milestone</a>} }
+
+    it 'links to a valid reference' do
+      doc = reference_filter("See #{reference}")
 
-      doc = reference_filter("milestone #{reference}")
-      expect(doc.text).to eq "milestone #{milestone.title}"
+      expect(doc.css('a').first.attr('href')).to eq urls.
+        namespace_project_milestone_url(project.namespace, project, milestone)
     end
 
-    it 'includes default classes' do
-      doc = reference_filter("milestone #{reference}")
-      expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-milestone'
+    it 'links with adjacent text' do
+      doc = reference_filter("Milestone (#{reference}.)")
+      expect(doc.to_html).to match(%r(\(<a.+>Milestone</a>\.\)))
     end
 
     it 'includes a data-project attribute' do
-      doc = reference_filter("milestone #{reference}")
+      doc = reference_filter("Milestone #{reference}")
       link = doc.css('a').first
 
       expect(link).to have_attribute('data-project')
@@ -68,8 +153,28 @@ describe Banzai::Filter::MilestoneReferenceFilter, lib: true do
     end
 
     it 'adds to the results hash' do
-      result = reference_pipeline_result("milestone #{reference}")
+      result = reference_pipeline_result("Milestone #{reference}")
       expect(result[:references][:milestone]).to eq [milestone]
     end
   end
+
+  describe 'cross project milestone references' do
+    let(:another_project)  { create(:empty_project, :public) }
+    let(:project_name) { another_project.name_with_namespace }
+    let(:milestone) { create(:milestone, project: another_project) }
+    let(:reference) { milestone.to_reference(project) }
+
+    let!(:result) { reference_filter("See #{reference}") }
+
+    it 'points to referenced project milestone page' do
+      expect(result.css('a').first.attr('href')).to eq urls.
+        namespace_project_milestone_url(another_project.namespace,
+                                        another_project,
+                                        milestone)
+    end
+
+    it 'contains cross project content' do
+      expect(result.css('a').first.text).to eq "#{milestone.name} in #{project_name}"
+    end
+  end
 end