Merge branch 'eReGeBe/gitlab-ce-feature/milestone-md' into 'master'

Implement special GitLab markdown reference for milestones

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3453 by @eReGeBe, with two additions:

- Move changelog item to 8.8
- Fix cross-project milestone ref with invalid project, like https://gitlab.com/gitlab-org/gitlab-ce/commit/f7348cd348ad8f4a18d74dd668283a4e236f5790 did for labels

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/13829

See merge request !3897

CHANGELOG

 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.8.0 (unreleased)
+  - Implement GFM references for milestones (Alejandro Rodríguez)
   - Snippets tab under user profile. !4001 (Long Nguyen)
   - Fix error when using link to uploads in global snippets
   - Assign labels and milestone to target project when moving issue. !3934 (Long Nguyen)

app/assets/javascripts/gfm_auto_complete.js.coffee

@@ -18,6 +18,10 @@ GitLab.GfmAutoComplete =
   Issues:
     template: '<li><small>${id}</small> ${title}</li>'
 
+  # Milestones
+  Milestones:
+    template: '<li>${title}</li>'
+
   # Add GFM auto-completion to all input fields, that accept GFM input.
   setup: (wrap) ->
     @input = $('.js-gfm-input')
@@ -81,6 +85,19 @@ GitLab.GfmAutoComplete =
             title:  sanitize(i.title)
             search: "#{i.iid} #{i.title}"
 
+    @input.atwho
+      at: '%'
+      alias: 'milestones'
+      searchKey: 'search'
+      displayTpl: @Milestones.template
+      insertTpl: '${atwho-at}"${title}"'
+      callbacks:
+        beforeSave: (milestones) ->
+          $.map milestones, (m) ->
+            id:     m.iid
+            title:  sanitize(m.title)
+            search: "#{m.title}"
+
     @input.atwho
       at: '!'
       alias: 'mergerequests'
@@ -105,6 +122,8 @@ GitLab.GfmAutoComplete =
     @input.atwho 'load', '@', data.members
     # load issues
     @input.atwho 'load', 'issues', data.issues
+    # load milestones
+    @input.atwho 'load', 'milestones', data.milestones
     # load merge requests
     @input.atwho 'load', 'mergerequests', data.mergerequests
     # load emojis

app/controllers/projects_controller.rb

@@ -147,6 +147,7 @@ class ProjectsController < Projects::ApplicationController
     @suggestions = {
       emojis: AwardEmoji.urls,
       issues: autocomplete.issues,
+      milestones: autocomplete.milestones,
       mergerequests: autocomplete.merge_requests,
       members: participants
     }

app/models/milestone.rb

@@ -59,8 +59,27 @@ class Milestone < ActiveRecord::Base
     end
   end
 
+  def self.reference_prefix
+    '%'
+  end
+
   def self.reference_pattern
-    nil
+    # NOTE: The iid pattern only matches when all characters on the expression
+    # are digits, so it will match %2 but not %2.1 because that's probably a
+    # milestone name and we want it to be matched as such.
+    @reference_pattern ||= %r{
+      (#{Project.reference_pattern})?
+      #{Regexp.escape(reference_prefix)}
+      (?:
+        (?<milestone_iid>
+          \d+(?!\S\w)\b # Integer-based milestone iid, or
+        ) |
+        (?<milestone_name>
+          [^"\s]+\b |  # String-based single-word milestone title, or
+          "[^"]+"      # String-based multi-word milestone surrounded in quotes
+        )
+      )
+    }x
   end
 
   def self.link_reference_pattern
@@ -81,13 +100,26 @@ class Milestone < ActiveRecord::Base
     end
   end
 
-  def to_reference(from_project = nil)
-    escaped_title = self.title.gsub("]", "\\]")
-
-    h = Gitlab::Routing.url_helpers
-    url = h.namespace_project_milestone_url(self.project.namespace, self.project, self)
+  ##
+  # 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}"
 
-    "[#{escaped_title}](#{url})"
+    if cross_project_reference?(from_project)
+      project.to_reference + reference
+    else
+      reference
+    end
   end
 
   def reference_link_text(from_project = nil)
@@ -159,4 +191,16 @@ class Milestone < ActiveRecord::Base
     issues.where(id: ids).
       update_all(["position = CASE #{conditions} ELSE position END", *pairs])
   end
+
+  private
+
+  def milestone_format_reference(format = :iid)
+    raise ArgumentError, 'Unknown format' unless [:iid, :name].include?(format)
+
+    if format == :name && !name.include?('"')
+      %("#{name}")
+    else
+      iid
+    end
+  end
 end

app/services/projects/autocomplete_service.rb

@@ -4,6 +4,10 @@ module Projects
       @project.issues.visible_to_user(current_user).opened.select([:iid, :title])
     end
 
+    def milestones
+      @project.milestones.active.reorder(due_date: :asc, title: :asc).select([:iid, :title])
+    end
+
     def merge_requests
       @project.merge_requests.opened.select([:iid, :title])
     end

doc/markdown/markdown.md

@@ -185,20 +185,23 @@ GFM will turn that reference into a link so you can navigate between them easily
 
 GFM will recognize the following:
 
-| input                  | references                 |
-|:-----------------------|:---------------------------|
-| `@user_name`           | specific user              |
-| `@group_name`          | specific group             |
-| `@all`                 | entire team                |
-| `#123`                 | issue                      |
-| `!123`                 | merge request              |
-| `$123`                 | snippet                    |
-| `~123`                 | label by ID                |
-| `~bug`                 | one-word label by name     |
-| `~"feature request"`   | multi-word label by name   |
-| `9ba12248`             | specific commit            |
-| `9ba12248...b19a04f5`  | commit range comparison    |
-| `[README](doc/README)` | repository file references |
+| input                  | references                   |
+|:-----------------------|:---------------------------  |
+| `@user_name`           | specific user                |
+| `@group_name`          | specific group               |
+| `@all`                 | entire team                  |
+| `#123`                 | issue                        |
+| `!123`                 | merge request                |
+| `$123`                 | snippet                      |
+| `~123`                 | label by ID                  |
+| `~bug`                 | one-word label by name       |
+| `~"feature request"`   | multi-word label by name     |
+| `%123`                 | milestone by ID              |
+| `%v1.23`               | one-word milestone by name   |
+| `%"release candidate"` | multi-word milestone by name |
+| `9ba12248`             | specific commit              |
+| `9ba12248...b19a04f5`  | commit range comparison      |
+| `[README](doc/README)` | repository file references   |
 
 GFM also recognizes certain cross-project references:
 
@@ -206,6 +209,7 @@ GFM also recognizes certain cross-project references:
 |:----------------------------------------|:------------------------|
 | `namespace/project#123`                 | issue                   |
 | `namespace/project!123`                 | merge request           |
+| `namespace/project%123`                 | milestone               |
 | `namespace/project$123`                 | snippet                 |
 | `namespace/project@9ba12248`            | specific commit         |
 | `namespace/project@9ba12248...b19a04f5` | commit range comparison |

lib/banzai/filter/milestone_reference_filter.rb

@@ -10,11 +10,53 @@ module Banzai
         project.milestones.find_by(iid: id)
       end
 
-      def url_for_object(issue, project)
+      def references_in(text, pattern = Milestone.reference_pattern)
+        # We'll handle here the references that follow the `reference_pattern`.
+        # Other patterns (for example, the link pattern) are handled by the
+        # default implementation.
+        return super(text, pattern) if pattern != Milestone.reference_pattern
+
+        text.gsub(pattern) do |match|
+          milestone = find_milestone($~[:project], $~[:milestone_iid], $~[:milestone_name])
+
+          if milestone
+            yield match, milestone.iid, $~[:project], $~
+          else
+            match
+          end
+        end
+      end
+
+      def find_milestone(project_ref, milestone_id, milestone_name)
+        project = project_from_ref(project_ref)
+        return unless project
+
+        milestone_params = milestone_params(milestone_id, milestone_name)
+        project.milestones.find_by(milestone_params)
+      end
+
+      def milestone_params(iid, name)
+        if name
+          { name: name.tr('"', '') }
+        else
+          { iid: iid.to_i }
+        end
+      end
+
+      def url_for_object(milestone, project)
         h = Gitlab::Routing.url_helpers
         h.namespace_project_milestone_url(project.namespace, project, milestone,
                                         only_path: context[:only_path])
       end
+
+      def object_link_text(object, matches)
+        if context[:project] == object.project
+          super
+        else
+          "#{escape_once(super)} <i>in #{escape_once(object.project.path_with_namespace)}</i>".
+            html_safe
+        end
+      end
     end
   end
 end

spec/fixtures/markdown.md.erb

@@ -216,10 +216,14 @@ References should be parseable even inside _<%= merge_request.to_reference %>_ e
 
 #### MilestoneReferenceFilter
 
-- Milestone: <%= milestone.to_reference %>
+- Milestone by ID: <%= simple_milestone.to_reference %>
+- Milestone by name: <%= Milestone.reference_prefix %><%= simple_milestone.name %>
+- Milestone by name in quotes: <%= milestone.to_reference(format: :name) %>
 - 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) %>)
+- Ignored in code: `<%= simple_milestone.to_reference %>`
+- Ignored in links: [Link to <%= simple_milestone.to_reference %>](#milestone-link)
+- Milestone by URL: <%= urls.namespace_project_milestone_url(milestone.project.namespace, milestone.project, milestone) %>
+- Link to milestone by URL: [Milestone](<%= milestone.to_reference %>)
 
 ### Task Lists
 

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,11 +18,42 @@ 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}")
 
@@ -30,29 +62,82 @@ describe Banzai::Filter::MilestoneReferenceFilter, lib: true do
     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 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 'escapes the title attribute' do
-      milestone.update_attribute(:title, %{"></a>whatever<a title="})
+    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
 
-      doc = reference_filter("milestone #{reference}")
-      expect(doc.text).to eq "milestone \">whatever"
+    it 'links with adjacent text' do
+      doc = reference_filter("Milestone (#{reference}.)")
+      expect(doc.to_html).to match(%r(\(<a.+>#{milestone.name}</a>\.\)))
     end
 
-    it 'includes default classes' do
-      doc = reference_filter("milestone #{reference}")
-      expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-milestone'
+    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(%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
+
+  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}")
+
+      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</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,34 @@ 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_path) { another_project.path_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_path}"
+    end
+
+    it 'escapes the name attribute' do
+      allow_any_instance_of(Milestone).to receive(:title).and_return(%{"></a>whatever<a title="})
+      doc = reference_filter("See #{reference}")
+      expect(doc.css('a').first.text).to eq "#{milestone.name} in #{project_path}"
+    end
+  end
 end

spec/support/markdown_feature.rb

@@ -63,8 +63,12 @@ class MarkdownFeature
     @label ||= create(:label, name: 'awaiting feedback', project: project)
   end
 
+  def simple_milestone
+    @simple_milestone ||= create(:milestone, name: 'gfm-milestone', project: project)
+  end
+
   def milestone
-    @milestone ||= create(:milestone, project: project)
+    @milestone ||= create(:milestone, name: 'next goal', project: project)
   end
 
   # Cross-references -----------------------------------------------------------

spec/support/matchers/markdown_matchers.rb

@@ -154,7 +154,7 @@ module MarkdownMatchers
     set_default_markdown_messages
 
     match do |actual|
-      expect(actual).to have_selector('a.gfm.gfm-milestone', count: 3)
+      expect(actual).to have_selector('a.gfm.gfm-milestone', count: 6)
     end
   end