Merge branch 'issue_5603' into 'master'

Add milestones autocomplete for epics

Closes #5603

See merge request gitlab-org/gitlab-ee!8632

ee/app/controllers/groups/autocomplete_sources_controller.rb

@@ -19,6 +19,10 @@ class Groups::AutocompleteSourcesController < Groups::ApplicationController
     render json: @autocomplete_service.commands(target)
   end
 
+  def milestones
+    render json: @autocomplete_service.milestones
+  end
+
   private
 
   def load_autocomplete_service

ee/app/finders/parent_groups_finder.rb

+class ParentGroupsFinder
+  attr_accessor :user, :group
+
+  def initialize(user, group)
+    @group = group
+    @user = user
+  end
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def execute
+    group.self_and_ancestors.where(id: user&.authorized_groups)
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+end

ee/app/helpers/ee/application_helper.rb

@@ -76,7 +76,8 @@ module EE
           members: members_group_autocomplete_sources_path(object, type: noteable_type, type_id: params[:id]),
           labels: labels_group_autocomplete_sources_path(object),
           epics: epics_group_autocomplete_sources_path(object),
-          commands: commands_group_autocomplete_sources_path(object, type: noteable_type, type_id: params[:id])
+          commands: commands_group_autocomplete_sources_path(object, type: noteable_type, type_id: params[:id]),
+          milestones: milestones_group_autocomplete_sources_path(object)
         }
       elsif object.group&.feature_available?(:epics)
         { epics: epics_project_autocomplete_sources_path(object) }.merge(super)

ee/app/services/groups/autocomplete_service.rb

@@ -15,6 +15,13 @@ module Groups
       end
     end
 
+    def milestones
+      group_ids =
+        ParentGroupsFinder.new(current_user, group).execute.map(&:id)
+
+      MilestonesFinder.new(group_ids: group_ids).execute
+    end
+
     def labels_as_hash(target)
       super(target, group_id: group.id, only_group_labels: true)
     end

ee/changelogs/unreleased/issue_5603.yml

+---
+title: Add milestones autocomplete for epics
+merge_request: 8632
+author:
+type: added

ee/config/routes/group.rb

@@ -44,6 +44,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
         get 'labels'
         get 'epics'
         get 'commands'
+        get 'milestones'
       end
     end
 

ee/spec/controllers/groups/autocomplete_sources_controller_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 describe Groups::AutocompleteSourcesController do
   let(:user) { create(:user) }
-  let(:group) { create(:group) }
+  let(:group) { create(:group, :private) }
   let!(:epic) { create(:epic, group: group) }
 
   before do
@@ -30,6 +30,26 @@ describe Groups::AutocompleteSourcesController do
     end
   end
 
+  context '#milestones' do
+    it 'returns correct response' do
+      parent_group = create(:group, :private)
+      group.update!(parent: parent_group)
+      sub_group = create(:group, :private, parent: sub_group)
+      create(:milestone, group: parent_group)
+      create(:milestone, group: sub_group)
+      group_milestone = create(:milestone, group: group)
+
+      get :milestones, group_id: group
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response.count).to eq(1)
+      expect(response).to match_response_schema('public_api/v4/milestones')
+      expect(json_response.first).to include(
+        'id' => group_milestone.id, 'iid' => group_milestone.iid, 'title' => group_milestone.title
+      )
+    end
+  end
+
   context '#commands' do
     it 'returns 200 status' do
       get :commands, group_id: group, type: 'Epic', type_id: epic.iid

ee/spec/features/epics/gfm_autocomplete_spec.rb

@@ -23,7 +23,21 @@ describe 'GFM autocomplete', :js do
 
       # It should show all the epics on "&".
       type(note, '&')
-      expect_epics(shown: [epic, epic2])
+      expect_resources(shown: [epic, epic2])
+    end
+  end
+
+  context 'milestone' do
+    it 'shows group milestones' do
+      project = create(:project, namespace: group)
+      milestone_1 = create(:milestone, title: 'milestone_1', group: group)
+      milestone_2 = create(:milestone, title: 'milestone_2', group: group)
+      milestone_3 = create(:milestone, title: 'milestone_3', project: project)
+      note = find('#note-body')
+
+      type(note, '%')
+
+      expect_resources(shown: [milestone_1, milestone_2], not_shown: [milestone_3])
     end
   end
 
@@ -39,19 +53,19 @@ describe 'GFM autocomplete', :js do
 
         # It should show all the labels on "~".
         type(note, '~')
-        expect_labels(shown: [backend, bug, feature_proposal])
+        expect_resources(shown: [backend, bug, feature_proposal])
 
         # It should show all the labels on "/label ~".
         type(note, '/label ~')
-        expect_labels(shown: [backend, bug, feature_proposal])
+        expect_resources(shown: [backend, bug, feature_proposal])
 
         # It should show all the labels on "/relabel ~".
         type(note, '/relabel ~')
-        expect_labels(shown: [backend, bug, feature_proposal])
+        expect_resources(shown: [backend, bug, feature_proposal])
 
         # It should show no labels on "/unlabel ~".
         type(note, '/unlabel ~')
-        expect_labels(not_shown: [backend, bug, feature_proposal])
+        expect_resources(not_shown: [backend, bug, feature_proposal])
       end
     end
 
@@ -65,19 +79,19 @@ describe 'GFM autocomplete', :js do
 
         # It should show all the labels on "~".
         type(note, '~')
-        expect_labels(shown: [backend, bug, feature_proposal])
+        expect_resources(shown: [backend, bug, feature_proposal])
 
         # It should show only unset labels on "/label ~".
         type(note, '/label ~')
-        expect_labels(shown: [bug, feature_proposal], not_shown: [backend])
+        expect_resources(shown: [bug, feature_proposal], not_shown: [backend])
 
         # It should show all the labels on "/relabel ~".
         type(note, '/relabel ~')
-        expect_labels(shown: [backend, bug, feature_proposal])
+        expect_resources(shown: [backend, bug, feature_proposal])
 
         # It should show only set labels on "/unlabel ~".
         type(note, '/unlabel ~')
-        expect_labels(shown: [backend], not_shown: [bug, feature_proposal])
+        expect_resources(shown: [backend], not_shown: [bug, feature_proposal])
       end
     end
 
@@ -91,19 +105,19 @@ describe 'GFM autocomplete', :js do
 
         # It should show all the labels on "~".
         type(note, '~')
-        expect_labels(shown: [backend, bug, feature_proposal])
+        expect_resources(shown: [backend, bug, feature_proposal])
 
         # It should show no labels on "/label ~".
         type(note, '/label ~')
-        expect_labels(not_shown: [backend, bug, feature_proposal])
+        expect_resources(not_shown: [backend, bug, feature_proposal])
 
         # It should show all the labels on "/relabel ~".
         type(note, '/relabel ~')
-        expect_labels(shown: [backend, bug, feature_proposal])
+        expect_resources(shown: [backend, bug, feature_proposal])
 
         # It should show all the labels on "/unlabel ~".
         type(note, '/unlabel ~')
-        expect_labels(shown: [backend, bug, feature_proposal])
+        expect_resources(shown: [backend, bug, feature_proposal])
       end
     end
   end
@@ -123,30 +137,16 @@ describe 'GFM autocomplete', :js do
     end
   end
 
-  def expect_labels(shown: nil, not_shown: nil)
-    page.within('.atwho-container') do
-      if shown
-        expect(page).to have_selector('.atwho-view li', count: shown.size)
-        shown.each { |label| expect(page).to have_content(label.title) }
-      end
-
-      if not_shown
-        expect(page).not_to have_selector('.atwho-view li') unless shown
-        not_shown.each { |label| expect(page).not_to have_content(label.title) }
-      end
-    end
-  end
-
-  def expect_epics(shown: nil, not_shown: nil)
+  def expect_resources(shown: nil, not_shown: nil)
     page.within('.atwho-container') do
       if shown
         expect(page).to have_selector('.atwho-view li', count: shown.size)
-        shown.each { |epic| expect(page).to have_content(epic.title) }
+        shown.each { |resource| expect(page).to have_content(resource.title) }
       end
 
       if not_shown
         expect(page).not_to have_selector('.atwho-view li') unless shown
-        not_shown.each { |epic| expect(page).not_to have_content(epic.title) }
+        not_shown.each { |resource| expect(page).not_to have_content(resource.title) }
       end
     end
   end

ee/spec/helpers/application_helper_spec.rb

@@ -62,7 +62,7 @@ describe ApplicationHelper do
       let(:noteable_type) { Epic }
 
       it 'returns paths for autocomplete_sources_controller' do
-        expect_autocomplete_data_sources(object, noteable_type, [:members, :labels, :epics, :commands])
+        expect_autocomplete_data_sources(object, noteable_type, [:members, :labels, :epics, :commands, :milestones])
       end
     end
 

ee/spec/services/groups/autocomplete_service_spec.rb

 require 'spec_helper'
 
 describe Groups::AutocompleteService do
-  let!(:group) { create(:group, :nested, avatar: fixture_file_upload('spec/fixtures/dk.png')) }
-  let!(:sub_group) { create(:group, parent: group) }
+  let!(:group) { create(:group, :nested, :private, avatar: fixture_file_upload('spec/fixtures/dk.png')) }
+  let!(:sub_group) { create(:group, :private, parent: group) }
   let(:user) { create(:user) }
   let!(:epic) { create(:epic, group: group, author: user) }
 
@@ -82,4 +82,47 @@ describe Groups::AutocompleteService do
       end
     end
   end
+
+  describe '#milestones' do
+    let!(:group_milestone) { create(:milestone, group: group) }
+    let!(:subgroup_milestone) { create(:milestone, group: sub_group) }
+
+    before do
+      sub_group.add_guest(user)
+    end
+
+    context 'when group is public' do
+      it 'returns milestones from groups', :nested_groups do
+        group = create(:group, :public)
+        subgroup = create(:group, :public, parent: group)
+        group_milestone = create(:milestone, group: group)
+        subgroup_milestone = create(:milestone, group: subgroup)
+        subgroup.add_guest(user)
+        group.add_guest(user)
+
+        subject = described_class.new(subgroup, user)
+
+        expect(subject.milestones).to match_array([group_milestone, subgroup_milestone])
+      end
+    end
+
+    it 'returns milestones from group' do
+      expect(subject.milestones).to include(group_milestone)
+    end
+
+    it 'returns milestones from groups and subgroups', :nested_groups do
+      milestones = described_class.new(sub_group, user).milestones
+
+      expect(milestones).to include(group_milestone, subgroup_milestone)
+    end
+
+    it 'returns only milestones that user can read', :nested_groups do
+      user = create(:user)
+      sub_group.add_guest(user)
+
+      milestones = described_class.new(sub_group, user).milestones
+
+      expect(milestones).to match_array([subgroup_milestone])
+    end
+  end
 end

lib/banzai/filter/milestone_reference_filter.rb

@@ -4,6 +4,8 @@ module Banzai
   module Filter
     # HTML filter that replaces milestone references with links.
     class MilestoneReferenceFilter < AbstractReferenceFilter
+      include Gitlab::Utils::StrongMemoize
+
       self.reference_type = :milestone
 
       def self.object_class
@@ -13,16 +15,34 @@ module Banzai
       # 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)
-        return unless project.is_a?(Project)
+      def find_object(parent, id)
+        return unless valid_context?(parent)
 
-        find_milestone_with_finder(project, id: id)
+        find_milestone_with_finder(parent, id: id)
       end
 
-      def find_object_from_link(project, iid)
-        return unless project.is_a?(Project)
+      def find_object_from_link(parent, iid)
+        return unless valid_context?(parent)
+
+        find_milestone_with_finder(parent, iid: iid)
+      end
+
+      def valid_context?(parent)
+        strong_memoize(:valid_context) do
+          group_context?(parent) || project_context?(parent)
+        end
+      end
+
+      def group_context?(parent)
+        strong_memoize(:group_context) do
+          parent.is_a?(Group)
+        end
+      end
 
-        find_milestone_with_finder(project, iid: iid)
+      def project_context?(parent)
+        strong_memoize(:project_context) do
+          parent.is_a?(Project)
+        end
       end
 
       def references_in(text, pattern = Milestone.reference_pattern)
@@ -44,13 +64,15 @@ module Banzai
 
       def find_milestone(project_ref, namespace_ref, milestone_id, milestone_name)
         project_path = full_project_path(namespace_ref, project_ref)
-        project = parent_from_ref(project_path)
 
-        return unless project && project.is_a?(Project)
+        # Returns group if project is not found by path
+        parent = parent_from_ref(project_path)
+
+        return unless parent
 
         milestone_params = milestone_params(milestone_id, milestone_name)
 
-        find_milestone_with_finder(project, milestone_params)
+        find_milestone_with_finder(parent, milestone_params)
       end
 
       def milestone_params(iid, name)
@@ -61,16 +83,28 @@ module Banzai
         end
       end
 
-      def find_milestone_with_finder(project, params)
-        finder_params = { project_ids: [project.id], order: nil, state: 'all' }
+      def find_milestone_with_finder(parent, params)
+        finder_params = milestone_finder_params(parent, params[:iid].present?)
+
+        MilestonesFinder.new(finder_params).find_by(params)
+      end
 
-        # 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.self_and_ancestors_ids
+      def milestone_finder_params(parent, find_by_iid)
+        { order: nil, state: 'all' }.tap do |params|
+          params[:project_ids] = parent.id if project_context?(parent)
+
+          # We don't support IID lookups because IIDs can clash between
+          # group/project milestones and group/subgroup milestones.
+          params[:group_ids] = self_and_ancestors_ids(parent) unless find_by_iid
         end
+      end
 
-        MilestonesFinder.new(finder_params).find_by(params)
+      def self_and_ancestors_ids(parent)
+        if group_context?(parent)
+          parent.self_and_ancestors_ids
+        elsif project_context?(parent)
+          parent.group&.self_and_ancestors_ids
+        end
       end
 
       def url_for_object(milestone, project)

spec/lib/banzai/filter/milestone_reference_filter_spec.rb

@@ -351,21 +351,50 @@ describe Banzai::Filter::MilestoneReferenceFilter do
   end
 
   context 'group context' do
-    let(:context) { { project: nil, group: create(:group) } }
-    let(:milestone) { create(:milestone, project: project) }
+    let(:group) { create(:group) }
+    let(:context) { { project: nil, group: group } }
 
-    it 'links to a valid reference' do
-      reference = "#{project.full_path}%#{milestone.iid}"
+    context 'when project milestone' do
+      let(:milestone) { create(:milestone, project: project) }
 
-      result = reference_filter("See #{reference}", context)
+      it 'links to a valid reference' do
+        reference = "#{project.full_path}%#{milestone.iid}"
 
-      expect(result.css('a').first.attr('href')).to eq(urls.milestone_url(milestone))
+        result = reference_filter("See #{reference}", context)
+
+        expect(result.css('a').first.attr('href')).to eq(urls.milestone_url(milestone))
+      end
+
+      it 'ignores internal references' do
+        exp = act = "See %#{milestone.iid}"
+
+        expect(reference_filter(act, context).to_html).to eq exp
+      end
     end
 
-    it 'ignores internal references' do
-      exp = act = "See %#{milestone.iid}"
+    context 'when group milestone' do
+      let(:group_milestone) { create(:milestone, title: 'group_milestone', group: group) }
 
-      expect(reference_filter(act, context).to_html).to eq exp
+      context 'for subgroups', :nested_groups do
+        let(:sub_group) { create(:group, parent: group) }
+        let(:sub_group_milestone) { create(:milestone, title: 'sub_group_milestone', group: sub_group) }
+
+        it 'links to a valid reference of subgroup and group milestones' do
+          [group_milestone, sub_group_milestone].each do |milestone|
+            reference = "%#{milestone.title}"
+
+            result = reference_filter("See #{reference}", { project: nil, group: sub_group })
+
+            expect(result.css('a').first.attr('href')).to eq(urls.milestone_url(milestone))
+          end
+        end
+      end
+
+      it 'ignores internal references' do
+        exp = act = "See %#{group_milestone.iid}"
+
+        expect(reference_filter(act, context).to_html).to eq exp
+      end
     end
   end