Merge branch 'group-milestone-references-system-notes' into 'master'

[EE] Support group milestone references

See merge request !2612

app/helpers/gitlab_routing_helper.rb

@@ -49,14 +49,6 @@ module GitlabRoutingHelper
     project_pipeline_path(pipeline.project, pipeline.id, *args)
   end
 
-  def milestone_path(entity, *args)
-    if entity.is_group_milestone?
-      group_milestone_path(entity.group, entity, *args)
-    elsif entity.is_project_milestone?
-      project_milestone_path(entity.project, entity, *args)
-    end
-  end
-
   def issue_url(entity, *args)
     project_issue_url(entity.project, entity, *args)
   end
@@ -69,14 +61,6 @@ module GitlabRoutingHelper
     project_pipeline_url(pipeline.project, pipeline.id, *args)
   end
 
-  def milestone_url(entity, *args)
-    if entity.is_group_milestone?
-      group_milestone_url(entity.group, entity, *args)
-    elsif entity.is_project_milestone?
-      project_milestone_url(entity.project, entity, *args)
-    end
-  end
-
   def pipeline_job_url(pipeline, build, *args)
     project_job_url(pipeline.project, build.id, *args)
   end

app/helpers/milestones_routing_helper.rb

+module MilestonesRoutingHelper
+  def milestone_path(milestone, *args)
+    if milestone.is_group_milestone?
+      group_milestone_path(milestone.group, milestone, *args)
+    elsif milestone.is_project_milestone?
+      project_milestone_path(milestone.project, milestone, *args)
+    end
+  end
+
+  def milestone_url(milestone, *args)
+    if milestone.is_group_milestone?
+      group_milestone_url(milestone.group, milestone, *args)
+    elsif milestone.is_project_milestone?
+      project_milestone_url(milestone.project, milestone, *args)
+    end
+  end
+end

app/models/milestone.rb

@@ -153,7 +153,9 @@ class Milestone < ActiveRecord::Base
   end
 
   ##
-  # Returns the String necessary to reference this Milestone in Markdown
+  # Returns the String necessary to reference this Milestone in Markdown. Group
+  # milestones only support name references, and do not support cross-project
+  # references.
   #
   # format - Symbol format to use (default: :iid, optional: :name)
   #
@@ -165,12 +167,16 @@ class Milestone < ActiveRecord::Base
   #   Milestone.first.to_reference(same_namespace_project)   # => "gitlab-ce%1"
   #
   def to_reference(from_project = nil, format: :iid, full: false)
-    return if is_group_milestone?
+    return if is_group_milestone? && format != :name
 
     format_reference = milestone_format_reference(format)
     reference = "#{self.class.reference_prefix}#{format_reference}"
 
+    if project
       "#{project.to_reference(from_project, full: full)}#{reference}"
+    else
+      reference
+    end
   end
 
   def reference_link_text(from_project = nil)

app/services/issuable_base_service.rb

@@ -4,11 +4,8 @@ class IssuableBaseService < BaseService
   private
 
   def create_milestone_note(issuable)
-    milestone = issuable.milestone
-    return if milestone && milestone.is_group_milestone?
-
     SystemNoteService.change_milestone(
-      issuable, issuable.project, current_user, milestone)
+      issuable, issuable.project, current_user, issuable.milestone)
   end
 
   def create_labels_note(issuable, old_labels)

app/services/projects/autocomplete_service.rb

@@ -5,7 +5,15 @@ module Projects
     end
 
     def milestones
-      @project.milestones.active.reorder(due_date: :asc, title: :asc).select([:iid, :title])
+      finder_params = {
+        project_ids: [@project.id],
+        state: :active,
+        order: { due_date: :asc, title: :asc }
+      }
+
+      finder_params[:group_ids] = [@project.group.id] if @project.group
+
+      MilestonesFinder.new(finder_params).execute.select([:iid, :title])
     end
 
     def merge_requests

app/services/system_note_service.rb

@@ -142,7 +142,8 @@ module SystemNoteService
   #
   # Returns the created Note object
   def change_milestone(noteable, project, author, milestone)
-    body = milestone.nil? ? 'removed milestone' : "changed milestone to #{milestone.to_reference(project)}"
+    format = milestone&.is_group_milestone? ? :name : :iid
+    body = milestone.nil? ? 'removed milestone' : "changed milestone to #{milestone.to_reference(project, format: format)}"
 
     create_note(NoteSummary.new(noteable, project, author, body, action: 'milestone'))
   end

changelogs/unreleased/group-milestone-references-system-notes.yml

+---
+title: Support Markdown references, autocomplete, and quick actions for group milestones
+merge_request:
+author:

config/application.rb

@@ -203,7 +203,11 @@ module Gitlab
         end
       end
 
+      # We add the MilestonesRoutingHelper because we know that this does not
+      # conflict with the methods defined in `project_url_helpers`, and we want
+      # these methods available in the same places.
       Gitlab::Routing.add_helpers(project_url_helpers)
+      Gitlab::Routing.add_helpers(MilestonesRoutingHelper)
     end
   end
 end

doc/user/markdown.md

@@ -248,7 +248,7 @@ GFM will recognize the following:
 | `~123`                     | label by ID                     |
 | `~bug`                     | one-word label by name          |
 | `~"feature request"`       | multi-word label by name        |
-| `%123`                     | milestone by ID                 |
+| `%123`                     | project milestone by ID         |
 | `%v1.23`                   | one-word milestone by name      |
 | `%"release candidate"`     | multi-word milestone by name    |
 | `9ba12248`                 | specific commit                 |
@@ -262,7 +262,7 @@ GFM also recognizes certain cross-project references:
 |:----------------------------------------|:------------------------|
 | `namespace/project#123`                 | issue                   |
 | `namespace/project!123`                 | merge request           |
-| `namespace/project%123`                 | milestone               |
+| `namespace/project%123`                 | project milestone       |
 | `namespace/project$123`                 | snippet                 |
 | `namespace/project@9ba12248`            | specific commit         |
 | `namespace/project@9ba12248...b19a04f5` | commit range comparison |
@@ -274,7 +274,7 @@ It also has a shorthand version to reference other projects from the same namesp
 |:------------------------------|:------------------------|
 | `project#123`                 | issue                   |
 | `project!123`                 | merge request           |
-| `project%123`                 | milestone               |
+| `project%123`                 | project milestone       |
 | `project$123`                 | snippet                 |
 | `project@9ba12248`            | specific commit         |
 | `project@9ba12248...b19a04f5` | commit range comparison |

doc/user/project/milestones/index.md

@@ -96,4 +96,5 @@ Burndown charts are only available for project milestones currently. They will b
 
 ## Quick actions
 
-[Quick actions](../quick_actions.md) are available for assigning and removing project milestones only. [In the future](https://gitlab.com/gitlab-org/gitlab-ce/issues/34778), this will also apply to group milestones.
+[Quick actions](../quick_actions.md) are available for assigning and removing
+project and group milestones.

lib/banzai/filter/abstract_reference_filter.rb

@@ -54,42 +54,42 @@ module Banzai
         self.class.references_in(*args, &block)
       end
 
-      def find_object(project, id)
       # Implement in child class
       # Example: project.merge_requests.find
+      def find_object(project, id)
       end
 
-      def find_object_cached(project, id)
-        if RequestStore.active?
-          cache = find_objects_cache[object_class][project.id]
-
-          get_or_set_cache(cache, id) { find_object(project, id) }
-        else
+      # Override if the link reference pattern produces a different ID (global
+      # ID vs internal ID, for instance) to the regular reference pattern.
+      def find_object_from_link(project, id)
         find_object(project, id)
       end
+
+      # Implement in child class
+      # Example: project_merge_request_url
+      def url_for_object(object, project)
       end
 
-      def project_from_ref_cached(ref)
-        if RequestStore.active?
-          cache = project_refs_cache
+      def find_object_cached(project, id)
+        cached_call(:banzai_find_object, id, path: [object_class, project.id]) do
+          find_object(project, id)
+        end
+      end
 
-          get_or_set_cache(cache, ref) { project_from_ref(ref) }
-        else
-          project_from_ref(ref)
+      def find_object_from_link_cached(project, id)
+        cached_call(:banzai_find_object_from_link, id, path: [object_class, project.id]) do
+          find_object_from_link(project, id)
         end
       end
 
-      def url_for_object(object, project)
-        # Implement in child class
-        # Example: project_merge_request_url
+      def project_from_ref_cached(ref)
+        cached_call(:banzai_project_refs, ref) do
+          project_from_ref(ref)
+        end
       end
 
       def url_for_object_cached(object, project)
-        if RequestStore.active?
-          cache = url_for_object_cache[object_class][project.id]
-
-          get_or_set_cache(cache, object) { url_for_object(object, project) }
-        else
+        cached_call(:banzai_url_for_object, object, path: [object_class, project.id]) do
           url_for_object(object, project)
         end
       end
@@ -120,7 +120,7 @@ module Banzai
 
               if link == inner_html && inner_html =~ /\A#{link_pattern}/
                 replace_link_node_with_text(node, link) do
-                  object_link_filter(inner_html, link_pattern)
+                  object_link_filter(inner_html, link_pattern, link_reference: true)
                 end
 
                 next
@@ -128,7 +128,7 @@ module Banzai
 
               if link =~ /\A#{link_pattern}\z/
                 replace_link_node_with_href(node, link) do
-                  object_link_filter(link, link_pattern, link_content: inner_html)
+                  object_link_filter(link, link_pattern, link_content: inner_html, link_reference: true)
                 end
 
                 next
@@ -146,15 +146,26 @@ module Banzai
       # text - String text to replace references in.
       # pattern - Reference pattern to match against.
       # link_content - Original content of the link being replaced.
+      # link_reference - True if this was using the link reference pattern,
+      #                  false otherwise.
       #
       # Returns a String with references replaced with links. All links
       # have `gfm` and `gfm-OBJECT_NAME` class names attached for styling.
-      def object_link_filter(text, pattern, link_content: nil)
+      def object_link_filter(text, pattern, link_content: nil, link_reference: false)
         references_in(text, pattern) do |match, id, project_ref, namespace_ref, matches|
           project_path = full_project_path(namespace_ref, project_ref)
           project = project_from_ref_cached(project_path)
 
-          if project && object = find_object_cached(project, id)
+          if project
+            object =
+              if link_reference
+                find_object_from_link_cached(project, id)
+              else
+                find_object_cached(project, id)
+              end
+          end
+
+          if object
             title = object_link_title(object)
             klass = reference_class(object_sym)
 
@@ -297,15 +308,17 @@ module Banzai
         RequestStore[:banzai_project_refs] ||= {}
       end
 
-      def find_objects_cache
-        RequestStore[:banzai_find_objects_cache] ||= Hash.new do |hash, key|
+      def cached_call(request_store_key, cache_key, path: [])
+        if RequestStore.active?
+          cache = RequestStore[request_store_key] ||= Hash.new do |hash, key|
             hash[key] = Hash.new { |h, k| h[k] = {} }
           end
-      end
 
-      def url_for_object_cache
-        RequestStore[:banzai_url_for_object] ||= Hash.new do |hash, key|
-          hash[key] = Hash.new { |h, k| h[k] = {} }
+          cache = cache.dig(*path) if path.any?
+
+          get_or_set_cache(cache, cache_key) { yield }
+        else
+          yield
         end
       end
 

lib/banzai/filter/milestone_reference_filter.rb

@@ -8,8 +8,15 @@ module Banzai
         Milestone
       end
 
+      # Links to project milestones contain the IID, but when we're handling
+      # 'regular' references, we need to use the global ID to disambiguate
+      # between group and project milestones.
       def find_object(project, id)
-        project.milestones.find_by(iid: id)
+        find_milestone_with_finder(project, id: id)
+      end
+
+      def find_object_from_link(project, iid)
+        find_milestone_with_finder(project, iid: iid)
       end
 
       def references_in(text, pattern = Milestone.reference_pattern)
@@ -22,7 +29,7 @@ module Banzai
           milestone = find_milestone($~[:project], $~[:namespace], $~[:milestone_iid], $~[:milestone_name])
 
           if milestone
-            yield match, milestone.iid, $~[:project], $~[:namespace], $~
+            yield match, milestone.id, $~[:project], $~[:namespace], $~
           else
             match
           end
@@ -36,7 +43,8 @@ module Banzai
         return unless project
 
         milestone_params = milestone_params(milestone_id, milestone_name)
-        project.milestones.find_by(milestone_params)
+
+        find_milestone_with_finder(project, milestone_params)
       end
 
       def milestone_params(iid, name)
@@ -47,15 +55,27 @@ module Banzai
         end
       end
 
+      def find_milestone_with_finder(project, params)
+        finder_params = { project_ids: [project.id], order: nil }
+
+        # We don't support IID lookups for group milestones, because IIDs can
+        # clash between group and project milestones.
+        if project.group && !params[:iid]
+          finder_params[:group_ids] = [project.group.id]
+        end
+
+        MilestonesFinder.new(finder_params).execute.find_by(params)
+      end
+
       def url_for_object(milestone, project)
-        h = Gitlab::Routing.url_helpers
-        h.project_milestone_url(project, milestone,
-                                        only_path: context[:only_path])
+        Gitlab::Routing
+          .url_helpers
+          .milestone_url(milestone, only_path: context[:only_path])
       end
 
       def object_link_text(object, matches)
         milestone_link = escape_once(super)
-        reference = object.project.to_reference(project)
+        reference = object.project&.to_reference(project)
 
         if reference.present?
           "#{milestone_link} <i>in #{reference}</i>".html_safe

spec/fixtures/markdown.md.erb

@@ -227,8 +227,11 @@ References should be parseable even inside _<%= merge_request.to_reference %>_ e
 - Milestone in another project: <%= xmilestone.to_reference(project) %>
 - Ignored in code: `<%= simple_milestone.to_reference %>`
 - Ignored in links: [Link to <%= simple_milestone.to_reference %>](#milestone-link)
-- Milestone by URL: <%= urls.project_milestone_url(milestone.project, milestone) %>
+- Milestone by URL: <%= urls.milestone_url(milestone) %>
 - Link to milestone by URL: [Milestone](<%= milestone.to_reference %>)
+- Group milestone by name: <%= Milestone.reference_prefix %><%= group_milestone.name %>
+- Group milestone by name in quotes: <%= group_milestone.to_reference(format: :name) %>
+- Group milestone by URL is ignore: <%= urls.milestone_url(group_milestone) %>
 
 ### Task Lists
 

spec/helpers/gitlab_routing_helper_spec.rb

@@ -63,44 +63,4 @@ describe GitlabRoutingHelper do
       it { expect(resend_invite_group_member_path(group_member)).to eq resend_invite_group_group_member_path(group_member.source, group_member) }
     end
   end
-
-  describe '#milestone_path' do
-    context 'for a group milestone' do
-      let(:milestone) { build_stubbed(:milestone, group: group, iid: 1) }
-
-      it 'links to the group milestone page' do
-        expect(milestone_path(milestone))
-          .to eq(group_milestone_path(group, milestone))
-      end
-    end
-
-    context 'for a project milestone' do
-      let(:milestone) { build_stubbed(:milestone, project: project, iid: 1) }
-
-      it 'links to the project milestone page' do
-        expect(milestone_path(milestone))
-          .to eq(project_milestone_path(project, milestone))
-      end
-    end
-  end
-
-  describe '#milestone_url' do
-    context 'for a group milestone' do
-      let(:milestone) { build_stubbed(:milestone, group: group, iid: 1) }
-
-      it 'links to the group milestone page' do
-        expect(milestone_url(milestone))
-          .to eq(group_milestone_url(group, milestone))
-      end
-    end
-
-    context 'for a project milestone' do
-      let(:milestone) { build_stubbed(:milestone, project: project, iid: 1) }
-
-      it 'links to the project milestone page' do
-        expect(milestone_url(milestone))
-          .to eq(project_milestone_url(project, milestone))
-      end
-    end
-  end
 end

spec/helpers/milestones_routing_helper_spec.rb

+require 'spec_helper'
+
+describe MilestonesRoutingHelper do
+  let(:project) { build_stubbed(:project) }
+  let(:group) { build_stubbed(:group) }
+
+  describe '#milestone_path' do
+    context 'for a group milestone' do
+      let(:milestone) { build_stubbed(:milestone, group: group, iid: 1) }
+
+      it 'links to the group milestone page' do
+        expect(milestone_path(milestone))
+          .to eq(group_milestone_path(group, milestone))
+      end
+    end
+
+    context 'for a project milestone' do
+      let(:milestone) { build_stubbed(:milestone, project: project, iid: 1) }
+
+      it 'links to the project milestone page' do
+        expect(milestone_path(milestone))
+          .to eq(project_milestone_path(project, milestone))
+      end
+    end
+  end
+
+  describe '#milestone_url' do
+    context 'for a group milestone' do
+      let(:milestone) { build_stubbed(:milestone, group: group, iid: 1) }
+
+      it 'links to the group milestone page' do
+        expect(milestone_url(milestone))
+          .to eq(group_milestone_url(group, milestone))
+      end
+    end
+
+    context 'for a project milestone' do
+      let(:milestone) { build_stubbed(:milestone, project: project, iid: 1) }
+
+      it 'links to the project milestone page' do
+        expect(milestone_url(milestone))
+          .to eq(project_milestone_url(project, milestone))
+      end
+    end
+  end
+end

spec/lib/banzai/filter/milestone_reference_filter_spec.rb

@@ -3,23 +3,24 @@ require 'spec_helper'
 describe Banzai::Filter::MilestoneReferenceFilter do
   include FilterSpecHelper
 
-  let(:project)   { create(:project, :public) }
-  let(:milestone) { create(:milestone, project: project) }
-  let(:reference) { milestone.to_reference }
+  let(:group) { create(:group, :public) }
+  let(:project) { create(:project, :public, group: group) }
 
   it 'requires project context' do
     expect { described_class.call('') }.to raise_error(ArgumentError, /:project/)
   end
 
+  shared_examples 'reference parsing' do
     %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}>"
+        exp = act = "<#{elem}>milestone #{reference}</#{elem}>"
         expect(reference_filter(act).to_html).to eq exp
       end
     end
 
     it 'includes default classes' do
       doc = reference_filter("Milestone #{reference}")
+
       expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-milestone has-tooltip'
     end
 
@@ -44,16 +45,15 @@ describe Banzai::Filter::MilestoneReferenceFilter do
       link = doc.css('a').first.attr('href')
 
       expect(link).not_to match %r(https?://)
-    expect(link).to eq urls
-      .project_milestone_path(project, milestone)
+      expect(link).to eq urls.milestone_path(milestone)
+    end
   end
 
-  context 'Integer-based references' do
+  shared_examples '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
-        .project_milestone_url(project, milestone)
+      expect(doc.css('a').first.attr('href')).to eq urls.milestone_url(milestone)
     end
 
     it 'links with adjacent text' do
@@ -68,15 +68,17 @@ describe Banzai::Filter::MilestoneReferenceFilter do
     end
   end
 
-  context 'String-based single-word references' do
-    let(:milestone) { create(:milestone, name: 'gfm', project: project) }
+  shared_examples 'String-based single-word references' do
     let(:reference) { "#{Milestone.reference_prefix}#{milestone.name}" }
 
+    before do
+      milestone.update!(name: 'gfm')
+    end
+
     it 'links to a valid reference' do
       doc = reference_filter("See #{reference}")
 
-      expect(doc.css('a').first.attr('href')).to eq urls
-        .project_milestone_url(project, milestone)
+      expect(doc.css('a').first.attr('href')).to eq urls.milestone_url(milestone)
       expect(doc.text).to eq 'See gfm'
     end
 
@@ -92,15 +94,17 @@ describe Banzai::Filter::MilestoneReferenceFilter do
     end
   end
 
-  context 'String-based multi-word references in quotes' do
-    let(:milestone) { create(:milestone, name: 'gfm references', project: project) }
+  shared_examples 'String-based multi-word references in quotes' do
     let(:reference) { milestone.to_reference(format: :name) }
 
+    before do
+      milestone.update!(name: 'gfm references')
+    end
+
     it 'links to a valid reference' do
       doc = reference_filter("See #{reference}")
 
-      expect(doc.css('a').first.attr('href')).to eq urls
-        .project_milestone_url(project, milestone)
+      expect(doc.css('a').first.attr('href')).to eq urls.milestone_url(milestone)
       expect(doc.text).to eq 'See gfm references'
     end
 
@@ -116,23 +120,27 @@ describe Banzai::Filter::MilestoneReferenceFilter do
     end
   end
 
-  describe 'referencing a milestone in a link href' do
-    let(:reference) { %Q{<a href="#{milestone.to_reference}">Milestone</a>} }
+  shared_examples 'referencing a milestone in a link href' do
+    let(:unquoted_reference) { "#{Milestone.reference_prefix}#{milestone.name}" }
+    let(:link_reference) { %Q{<a href="#{unquoted_reference}">Milestone</a>} }
+
+    before do
+      milestone.update!(name: 'gfm')
+    end
 
     it 'links to a valid reference' do
-      doc = reference_filter("See #{reference}")
+      doc = reference_filter("See #{link_reference}")
 
-      expect(doc.css('a').first.attr('href')).to eq urls
-        .project_milestone_url(project, milestone)
+      expect(doc.css('a').first.attr('href')).to eq urls.milestone_url(milestone)
     end
 
     it 'links with adjacent text' do
-      doc = reference_filter("Milestone (#{reference}.)")
+      doc = reference_filter("Milestone (#{link_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 #{link_reference}")
       link = doc.css('a').first
 
       expect(link).to have_attribute('data-project')
@@ -140,7 +148,35 @@ describe Banzai::Filter::MilestoneReferenceFilter do
     end
 
     it 'includes a data-milestone attribute' do
-      doc = reference_filter("See #{reference}")
+      doc = reference_filter("See #{link_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
+  end
+
+  shared_examples 'linking to a milestone as the entire link' do
+    let(:unquoted_reference) { "#{Milestone.reference_prefix}#{milestone.name}" }
+    let(:link) { urls.milestone_url(milestone) }
+    let(:link_reference) { %Q{<a href="#{link}">#{link}</a>} }
+
+    it 'replaces the link text with the milestone reference' do
+      doc = reference_filter("See #{link}")
+
+      expect(doc.css('a').first.text).to eq(unquoted_reference)
+    end
+
+    it 'includes a data-project attribute' do
+      doc = reference_filter("Milestone #{link_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 #{link_reference}")
       link = doc.css('a').first
 
       expect(link).to have_attribute('data-milestone')
@@ -148,7 +184,7 @@ describe Banzai::Filter::MilestoneReferenceFilter do
     end
   end
 
-  describe 'cross-project / cross-namespace complete reference' do
+  shared_examples 'cross-project / cross-namespace complete reference' do
     let(:namespace)       { create(:namespace) }
     let(:another_project) { create(:project, :public, namespace: namespace) }
     let(:milestone)       { create(:milestone, project: another_project) }
@@ -184,7 +220,7 @@ describe Banzai::Filter::MilestoneReferenceFilter do
     end
   end
 
-  describe 'cross-project / same-namespace complete reference' do
+  shared_examples 'cross-project / same-namespace complete reference' do
     let(:namespace)       { create(:namespace) }
     let(:project)         { create(:project, :public, namespace: namespace) }
     let(:another_project) { create(:project, :public, namespace: namespace) }
@@ -221,7 +257,7 @@ describe Banzai::Filter::MilestoneReferenceFilter do
     end
   end
 
-  describe 'cross project shorthand reference' do
+  shared_examples 'cross project shorthand reference' do
     let(:namespace)       { create(:namespace) }
     let(:project)         { create(:project, :public, namespace: namespace) }
     let(:another_project) { create(:project, :public, namespace: namespace) }
@@ -258,27 +294,53 @@ describe Banzai::Filter::MilestoneReferenceFilter do
     end
   end
 
-  describe 'cross project milestone references' do
-    let(:another_project)  { create(:project, :public) }
-    let(:project_path) { another_project.full_path }
-    let(:milestone) { create(:milestone, project: another_project) }
-    let(:reference) { milestone.to_reference(project) }
+  context 'project milestones' do
+    let(:milestone) { create(:milestone, project: project) }
+    let(:reference) { milestone.to_reference }
 
-    let!(:result) { reference_filter("See #{reference}") }
+    include_examples 'reference parsing'
 
-    it 'points to referenced project milestone page' do
-      expect(result.css('a').first.attr('href')).to eq urls
-        .project_milestone_url(another_project, milestone)
+    it_behaves_like 'Integer-based references'
+    it_behaves_like 'String-based single-word references'
+    it_behaves_like 'String-based multi-word references in quotes'
+    it_behaves_like 'referencing a milestone in a link href'
+    it_behaves_like 'cross-project / cross-namespace complete reference'
+    it_behaves_like 'cross-project / same-namespace complete reference'
+    it_behaves_like 'cross project shorthand reference'
   end
 
-    it 'contains cross project content' do
-      expect(result.css('a').first.text).to eq "#{milestone.name} in #{project_path}"
+  context 'group milestones' do
+    let(:milestone) { create(:milestone, group: group) }
+    let(:reference) { milestone.to_reference(format: :name) }
+
+    include_examples 'reference parsing'
+
+    it_behaves_like 'String-based single-word references'
+    it_behaves_like 'String-based multi-word references in quotes'
+    it_behaves_like 'referencing a milestone in a link href'
+
+    it 'does not support references by IID' do
+      doc = reference_filter("See #{Milestone.reference_prefix}#{milestone.iid}")
+
+      expect(doc.css('a')).to be_empty
     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}"
+    it 'does not support references by link' do
+      doc = reference_filter("See #{urls.milestone_url(milestone)}")
+
+      expect(doc.css('a').first.text).to eq(urls.milestone_url(milestone))
+    end
+
+    it 'does not support cross-project references' do
+      another_group = create(:group)
+      another_project = create(:project, :public, group: group)
+      project_reference = another_project.to_reference(project)
+
+      milestone.update!(group: another_group)
+
+      doc = reference_filter("See #{project_reference}#{reference}")
+
+      expect(doc.css('a')).to be_empty
     end
   end
 end

spec/models/milestone_spec.rb

@@ -232,16 +232,40 @@ describe Milestone do
   end
 
   describe '#to_reference' do
-    let(:project) { build(:project, name: 'sample-project') }
-    let(:milestone) { build(:milestone, iid: 1, project: project) }
+    let(:group) { build_stubbed(:group) }
+    let(:project) { build_stubbed(:project, name: 'sample-project') }
+    let(:another_project) { build_stubbed(:project, name: 'another-project', namespace: project.namespace) }
+
+    context 'for a project milestone' do
+      let(:milestone) { build_stubbed(:milestone, iid: 1, project: project, name: 'milestone') }
 
       it 'returns a String reference to the object' do
-      expect(milestone.to_reference).to eq "%1"
+        expect(milestone.to_reference).to eq '%1'
+      end
+
+      it 'returns a reference by name when the format is set to :name' do
+        expect(milestone.to_reference(format: :name)).to eq '%"milestone"'
       end
 
       it 'supports a cross-project reference' do
-      another_project = build(:project, name: 'another-project', namespace: project.namespace)
-      expect(milestone.to_reference(another_project)).to eq "sample-project%1"
+        expect(milestone.to_reference(another_project)).to eq 'sample-project%1'
+      end
+    end
+
+    context 'for a group milestone' do
+      let(:milestone) { build_stubbed(:milestone, iid: 1, group: group, name: 'milestone') }
+
+      it 'returns nil with the default format' do
+        expect(milestone.to_reference).to be_nil
+      end
+
+      it 'returns a reference by name when the format is set to :name' do
+        expect(milestone.to_reference(format: :name)).to eq '%"milestone"'
+      end
+
+      it 'does not supports cross-project references' do
+        expect(milestone.to_reference(another_project, format: :name)).to eq '%"milestone"'
+      end
     end
   end
 

spec/services/projects/autocomplete_service_spec.rb

@@ -88,4 +88,31 @@ describe Projects::AutocompleteService do
       end
     end
   end
+
+  describe '#milestones' do
+    let(:user) { create(:user) }
+    let(:group) { create(:group) }
+    let(:project) { create(:project, group: group) }
+    let!(:group_milestone) { create(:milestone, group: group) }
+    let!(:project_milestone) { create(:milestone, project: project) }
+
+    let(:milestone_titles) { described_class.new(project, user).milestones.map(&:title) }
+
+    it 'includes project and group milestones' do
+      expect(milestone_titles).to eq([group_milestone.title, project_milestone.title])
+    end
+
+    it 'does not include closed milestones' do
+      group_milestone.close
+
+      expect(milestone_titles).to eq([project_milestone.title])
+    end
+
+    it 'does not include milestones from other projects in the group' do
+      other_project = create(:project, group: group)
+      project_milestone.update!(project: other_project)
+
+      expect(milestone_titles).to eq([group_milestone.title])
+    end
+  end
 end

spec/services/system_note_service_spec.rb

@@ -3,7 +3,8 @@ require 'spec_helper'
 describe SystemNoteService do
   include Gitlab::Routing
 
-  let(:project)  { create(:project) }
+  let(:group)    { create(:group) }
+  let(:project)  { create(:project, group: group) }
   let(:author)   { create(:user) }
   let(:noteable) { create(:issue, project: project) }
   let(:issue)    { noteable }
@@ -242,6 +243,7 @@ describe SystemNoteService do
   end
 
   describe '.change_milestone' do
+    context 'for a project milestone' do
       subject { described_class.change_milestone(noteable, project, author, milestone) }
 
       let(:milestone) { create(:milestone, project: project) }
@@ -265,6 +267,31 @@ describe SystemNoteService do
       end
     end
 
+    context 'for a group milestone' do
+      subject { described_class.change_milestone(noteable, project, author, milestone) }
+
+      let(:milestone) { create(:milestone, group: group) }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'milestone' }
+      end
+
+      context 'when milestone added' do
+        it 'sets the note text to use the milestone name' do
+          expect(subject.note).to eq "changed milestone to #{milestone.to_reference(format: :name)}"
+        end
+      end
+
+      context 'when milestone removed' do
+        let(:milestone) { nil }
+
+        it 'sets the note text' do
+          expect(subject.note).to eq 'removed milestone'
+        end
+      end
+    end
+  end
+
   describe '.change_status' do
     subject { described_class.change_status(noteable, project, author, status, source) }
 

spec/support/issuable_shared_examples.rb

@@ -21,15 +21,15 @@ shared_examples 'system notes for milestones' do
       create(:group_member, group: group, user: user)
     end
 
-    it 'does not create system note' do
+    it 'creates a system note' do
       expect do
         update_issuable(milestone: group_milestone)
-      end.not_to change { Note.system.count }
+      end.to change { Note.system.count }.by(1)
     end
   end
 
   context 'project milestones' do
-    it 'creates system note' do
+    it 'creates a system note' do
       expect do
         update_issuable(milestone: create(:milestone))
       end.to change { Note.system.count }.by(1)

spec/support/markdown_feature.rb

@@ -23,7 +23,7 @@ class MarkdownFeature
   # Direct references ----------------------------------------------------------
 
   def project
-    @project ||= create(:project, :repository).tap do |project|
+    @project ||= create(:project, :repository, group: group).tap do |project|
       project.team << [user, :master]
     end
   end
@@ -75,6 +75,10 @@ class MarkdownFeature
     @milestone ||= create(:milestone, name: 'next goal', project: project)
   end
 
+  def group_milestone
+    @group_milestone ||= create(:milestone, name: 'group-milestone', group: group)
+  end
+
   # Cross-references -----------------------------------------------------------
 
   def xproject

spec/support/matchers/markdown_matchers.rb

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