Merge branch 'milestone-ref' into 'master'

Link to milestone in "Milestone changed" system note

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/4141

See merge request !2203

13 files changed, 172 insertions, 22 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 879d057fff0..5aeed6b1287 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -8,6 +8,7 @@ v 8.4.0 (unreleased)
   - Implement search inside emoji picker
   - Add API support for looking up a user by username (Stan Hu)
   - Add project permissions to all project API endpoints (Stan Hu)
+  - Link to milestone in "Milestone changed" system note
   - Only allow group/project members to mention `@all`
   - Expose Git's version in the admin area (Trey Davis)
   - Add "Frequently used" category to emoji picker
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index d8c7536cd31..550d14d4c39 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -22,6 +22,7 @@ class Milestone < ActiveRecord::Base
 
   include InternalId
   include Sortable
+  include Referable
   include StripAttribute
 
   belongs_to :project
@@ -61,6 +62,27 @@ class Milestone < ActiveRecord::Base
     end
   end
 
+  def self.reference_pattern
+    nil
+  end
+
+  def self.link_reference_pattern
+    super("milestones", /(?<milestone>\d+)/)
+  end
+
+  def to_reference(from_project = nil)
+    escaped_title = self.title.gsub("]", "\\]")
+
+    h = Gitlab::Application.routes.url_helpers
+    url = h.namespace_project_milestone_url(self.project.namespace, self.project, self)
+
+    "[#{escaped_title}](#{url})"
+  end
+
+  def reference_link_text(from_project = nil)
+    self.title
+  end
+
   def expired?
     if due_date
       due_date.past?
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 98a71cbf1ad..1083bcec054 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -41,7 +41,7 @@ class SystemNoteService
   #
   # Returns the created Note object
   def self.change_assignee(noteable, project, author, assignee)
-    body = assignee.nil? ? 'Assignee removed' : "Reassigned to @#{assignee.username}"
+    body = assignee.nil? ? 'Assignee removed' : "Reassigned to #{assignee.to_reference}"
 
     create_note(noteable: noteable, project: project, author: author, note: body)
   end
@@ -66,7 +66,7 @@ class SystemNoteService
   def self.change_label(noteable, project, author, added_labels, removed_labels)
     labels_count = added_labels.count + removed_labels.count
 
-    references     = ->(label) { "~#{label.id}" }
+    references     = ->(label) { label.to_reference(:id) }
     added_labels   = added_labels.map(&references).join(' ')
     removed_labels = removed_labels.map(&references).join(' ')
 
@@ -103,7 +103,7 @@ class SystemNoteService
   # Returns the created Note object
   def self.change_milestone(noteable, project, author, milestone)
     body = 'Milestone '
-    body += milestone.nil? ? 'removed' : "changed to #{milestone.title}"
+    body += milestone.nil? ? 'removed' : "changed to #{milestone.to_reference(project)}"
 
     create_note(noteable: noteable, project: project, author: author, note: body)
   end
diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb
index 230387c8383..b2db10e6864 100644
--- a/lib/banzai/filter/abstract_reference_filter.rb
+++ b/lib/banzai/filter/abstract_reference_filter.rb
@@ -70,27 +70,31 @@ module Banzai
       end
 
       def call
-        # `#123`
-        replace_text_nodes_matching(object_class.reference_pattern) do |content|
-          object_link_filter(content, object_class.reference_pattern)
-        end
+        if object_class.reference_pattern
+          # `#123`
+          replace_text_nodes_matching(object_class.reference_pattern) do |content|
+            object_link_filter(content, object_class.reference_pattern)
+          end
 
-        # `[Issue](#123)`, which is turned into
-        # `<a href="#123">Issue</a>`
-        replace_link_nodes_with_href(object_class.reference_pattern) do |link, text|
-          object_link_filter(link, object_class.reference_pattern, link_text: text)
+          # `[Issue](#123)`, which is turned into
+          # `<a href="#123">Issue</a>`
+          replace_link_nodes_with_href(object_class.reference_pattern) do |link, text|
+            object_link_filter(link, object_class.reference_pattern, link_text: text)
+          end
         end
 
-        # `http://gitlab.example.com/namespace/project/issues/123`, which is turned into
-        # `<a href="http://gitlab.example.com/namespace/project/issues/123">http://gitlab.example.com/namespace/project/issues/123</a>`
-        replace_link_nodes_with_text(object_class.link_reference_pattern) do |text|
-          object_link_filter(text, object_class.link_reference_pattern)
-        end
+        if object_class.link_reference_pattern
+          # `http://gitlab.example.com/namespace/project/issues/123`, which is turned into
+          # `<a href="http://gitlab.example.com/namespace/project/issues/123">http://gitlab.example.com/namespace/project/issues/123</a>`
+          replace_link_nodes_with_text(object_class.link_reference_pattern) do |text|
+            object_link_filter(text, object_class.link_reference_pattern)
+          end
 
-        # `[Issue](http://gitlab.example.com/namespace/project/issues/123)`, which is turned into
-        # `<a href="http://gitlab.example.com/namespace/project/issues/123">Issue</a>`
-        replace_link_nodes_with_href(object_class.link_reference_pattern) do |link, text|
-          object_link_filter(link, object_class.link_reference_pattern, link_text: text)
+          # `[Issue](http://gitlab.example.com/namespace/project/issues/123)`, which is turned into
+          # `<a href="http://gitlab.example.com/namespace/project/issues/123">Issue</a>`
+          replace_link_nodes_with_href(object_class.link_reference_pattern) do |link, text|
+            object_link_filter(link, object_class.link_reference_pattern, link_text: text)
+          end
         end
       end
 
diff --git a/lib/banzai/filter/milestone_reference_filter.rb b/lib/banzai/filter/milestone_reference_filter.rb
new file mode 100644
index 00000000000..e88b27c1fae
--- /dev/null
+++ b/lib/banzai/filter/milestone_reference_filter.rb
@@ -0,0 +1,22 @@
+require 'banzai'
+
+module Banzai
+  module Filter
+    # HTML filter that replaces milestone references with links.
+    class MilestoneReferenceFilter < AbstractReferenceFilter
+      def self.object_class
+        Milestone
+      end
+
+      def find_object(project, id)
+        project.milestones.find_by(iid: id)
+      end
+
+      def url_for_object(issue, project)
+        h = Gitlab::Application.routes.url_helpers
+        h.namespace_project_milestone_url(project.namespace, project, milestone,
+                                        only_path: context[:only_path])
+      end
+    end
+  end
+end
diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb
index 38750b55ec7..838155e8831 100644
--- a/lib/banzai/pipeline/gfm_pipeline.rb
+++ b/lib/banzai/pipeline/gfm_pipeline.rb
@@ -22,6 +22,7 @@ module Banzai
           Filter::CommitRangeReferenceFilter,
           Filter::CommitReferenceFilter,
           Filter::LabelReferenceFilter,
+          Filter::MilestoneReferenceFilter,
 
           Filter::TaskListFilter
         ]
diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb
index be795649e59..4164e998dd1 100644
--- a/lib/gitlab/reference_extractor.rb
+++ b/lib/gitlab/reference_extractor.rb
@@ -19,7 +19,7 @@ module Gitlab
       super(text, context.merge(project: project))
     end
 
-    %i(user label merge_request snippet commit commit_range).each do |type|
+    %i(user label milestone merge_request snippet commit commit_range).each do |type|
       define_method("#{type}s") do
         @references[type] ||= references(type, reference_context)
       end
diff --git a/spec/features/markdown_spec.rb b/spec/features/markdown_spec.rb
index fdd8cf07b12..e836d81c40b 100644
--- a/spec/features/markdown_spec.rb
+++ b/spec/features/markdown_spec.rb
@@ -212,6 +212,7 @@ describe 'GitLab Markdown', feature: true do
         expect(doc).to reference_commit_ranges
         expect(doc).to reference_commits
         expect(doc).to reference_labels
+        expect(doc).to reference_milestones
       end
     end
 
diff --git a/spec/fixtures/markdown.md.erb b/spec/fixtures/markdown.md.erb
index e8dfc5c0eb1..0620096d689 100644
--- a/spec/fixtures/markdown.md.erb
+++ b/spec/fixtures/markdown.md.erb
@@ -214,6 +214,13 @@ References should be parseable even inside _<%= merge_request.to_reference %>_ e
 - Ignored in links: [Link to <%= simple_label.to_reference %>](#label-link)
 - Link to label by reference: [Label](<%= label.to_reference %>)
 
+#### MilestoneReferenceFilter
+
+- Milestone: <%= milestone.to_reference %>
+- Milestone in another project: <%= xmilestone.to_reference(project) %>
+- Ignored in code: `<%= milestone.to_reference %>`
+- Link to milestone by URL: [Milestone](<%= urls.namespace_project_milestone_url(milestone.project.namespace, milestone.project, milestone) %>)
+
 ### Task Lists
 
 - [ ] Incomplete task 1
diff --git a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb
new file mode 100644
index 00000000000..ebf3d7489b5
--- /dev/null
+++ b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb
@@ -0,0 +1,75 @@
+require 'spec_helper'
+
+describe Banzai::Filter::MilestoneReferenceFilter, lib: true do
+  include FilterSpecHelper
+
+  let(:project) { create(:project, :public) }
+  let(:milestone)   { create(:milestone, project: project) }
+
+  it 'requires project context' do
+    expect { described_class.call('') }.to raise_error(ArgumentError, /:project/)
+  end
+
+  %w(pre code a style).each do |elem|
+    it "ignores valid references contained inside '#{elem}' element" do
+      exp = act = "<#{elem}>milestone #{milestone.to_reference}</#{elem}>"
+      expect(reference_filter(act).to_html).to eq exp
+    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 '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(/\(<a.+>#{Regexp.escape(milestone.title)}<\/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}"
+    end
+
+    it 'escapes the title attribute' do
+      milestone.update_attribute(:title, %{"></a>whatever<a title="})
+
+      doc = reference_filter("milestone #{reference}")
+      expect(doc.text).to eq "milestone #{milestone.title}"
+    end
+
+    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 'adds to the results hash' do
+      result = reference_pipeline_result("milestone #{reference}")
+      expect(result[:references][:milestone]).to eq [milestone]
+    end
+  end
+end
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index c9f828ae2f7..d3364a71022 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -171,7 +171,7 @@ describe SystemNoteService, services: true do
 
     context 'when milestone added' do
       it 'sets the note text' do
-        expect(subject.note).to eq "Milestone changed to #{milestone.title}"
+        expect(subject.note).to eq "Milestone changed to #{milestone.to_reference}"
       end
     end
 
diff --git a/spec/support/markdown_feature.rb b/spec/support/markdown_feature.rb
index d6d3062a197..5d97fdd4882 100644
--- a/spec/support/markdown_feature.rb
+++ b/spec/support/markdown_feature.rb
@@ -59,6 +59,10 @@ class MarkdownFeature
     @label ||= create(:label, name: 'awaiting feedback', project: project)
   end
 
+  def milestone
+    @milestone ||= create(:milestone, project: project)
+  end
+
   # Cross-references -----------------------------------------------------------
 
   def xproject
@@ -93,6 +97,10 @@ class MarkdownFeature
     end
   end
 
+  def xmilestone
+    @xmilestone ||= create(:milestone, project: xproject)
+  end
+
   def urls
     Gitlab::Application.routes.url_helpers
   end
diff --git a/spec/support/matchers/markdown_matchers.rb b/spec/support/matchers/markdown_matchers.rb
index 7eadcd58c1f..b251e7f8f23 100644
--- a/spec/support/matchers/markdown_matchers.rb
+++ b/spec/support/matchers/markdown_matchers.rb
@@ -130,6 +130,15 @@ module MarkdownMatchers
     end
   end
 
+  # MilestoneReferenceFilter
+  matcher :reference_milestones do
+    set_default_markdown_messages
+
+    match do |actual|
+      expect(actual).to have_selector('a.gfm.gfm-milestone', count: 3)
+    end
+  end
+
   # TaskListFilter
   matcher :parse_task_lists do
     set_default_markdown_messages