Merge branch 'ccr/49289_milestone_link' into 'master'

Add project milestone link

Closes #49289

See merge request gitlab-org/gitlab-ce!22552

app/assets/stylesheets/framework/issue_box.scss

@@ -45,9 +45,4 @@
   &.status-box-upcoming {
     background: $gl-text-color-secondary;
   }
-
-  &.status-box-milestone {
-    color: $gl-text-color;
-    background: $gray-darker;
-  }
 }

app/assets/stylesheets/pages/milestone.scss

+$status-box-line-height: 26px;
+
 .issues-sortable-list .str-truncated {
   max-width: 90%;
 }
@@ -38,6 +40,7 @@
       font-size: $tooltip-font-size;
       margin-top: 0;
       margin-right: $gl-padding-4;
+      line-height: $status-box-line-height;
 
       @include media-breakpoint-down(xs) {
         line-height: unset;

app/controllers/groups/milestones_controller.rb

@@ -43,14 +43,7 @@ class Groups::MilestonesController < Groups::ApplicationController
   def update
     # Keep this compatible with legacy group milestones where we have to update
     # all projects milestones states at once.
-    if @milestone.legacy_group_milestone?
-      update_params = milestone_params.select { |key| key == "state_event" }
-      milestones = @milestone.milestones
-    else
-      update_params = milestone_params
-      milestones = [@milestone]
-    end
-
+    milestones, update_params = get_milestones_for_update
     milestones.each do |milestone|
       Milestones::UpdateService.new(milestone.parent, current_user, update_params).execute(milestone)
     end
@@ -71,6 +64,14 @@ class Groups::MilestonesController < Groups::ApplicationController
 
   private
 
+  def get_milestones_for_update
+    if @milestone.legacy_group_milestone?
+      [@milestone.milestones, legacy_milestone_params]
+    else
+      [[@milestone], milestone_params]
+    end
+  end
+
   def authorize_admin_milestones!
     return render_404 unless can?(current_user, :admin_milestone, group)
   end
@@ -79,6 +80,10 @@ class Groups::MilestonesController < Groups::ApplicationController
     params.require(:milestone).permit(:title, :description, :start_date, :due_date, :state_event)
   end
 
+  def legacy_milestone_params
+    params.require(:milestone).permit(:state_event)
+  end
+
   def milestone_path
     if @milestone.legacy_group_milestone?
       group_milestone_path(group, @milestone.safe_title, title: @milestone.title)

app/helpers/milestones_helper.rb

@@ -231,12 +231,15 @@ module MilestonesHelper
     end
   end
 
-  def group_or_dashboard_milestone_path(milestone)
-    if milestone.group_milestone?
-      group_milestone_path(milestone.group, milestone.iid, milestone: { title: milestone.title })
-    else
-      dashboard_milestone_path(milestone.safe_title, title: milestone.title)
-    end
+  def group_or_project_milestone_path(milestone)
+    params =
+      if milestone.group_milestone?
+        { milestone: { title: milestone.title } }
+      else
+        { title: milestone.title }
+      end
+
+    milestone_path(milestone.milestone, params)
   end
 
   def can_admin_project_milestones?

app/models/concerns/milestoneish.rb

@@ -42,7 +42,7 @@ module Milestoneish
   def issues_visible_to_user(user)
     memoize_per_user(user, :issues_visible_to_user) do
       IssuesFinder.new(user, issues_finder_params)
-        .execute.preload(:assignees).where(milestone_id: milestoneish_ids)
+        .execute.preload(:assignees).where(milestone_id: milestoneish_id)
     end
   end
 

app/models/dashboard_group_milestone.rb

@@ -6,7 +6,7 @@ class DashboardGroupMilestone < GlobalMilestone
   attr_reader :group_name
 
   def initialize(milestone)
-    super(milestone.title, Array(milestone))
+    super
 
     @group_name = milestone.group.full_name
   end
@@ -18,22 +18,4 @@ class DashboardGroupMilestone < GlobalMilestone
              .active
              .map { |m| new(m) }
   end
-
-  override :group_milestone?
-  def group_milestone?
-    @first_milestone.group_milestone?
-  end
-
-  override :milestoneish_ids
-  def milestoneish_ids
-    milestones.map(&:id)
-  end
-
-  def group
-    @first_milestone.group
-  end
-
-  def iid
-    @first_milestone.iid
-  end
 end

app/models/dashboard_milestone.rb

 # frozen_string_literal: true
 
 class DashboardMilestone < GlobalMilestone
-  def issues_finder_params
-    { authorized_only: true }
+  attr_reader :project_name
+
+  def initialize(milestone)
+    super
+
+    @project_name = milestone.project.full_name
   end
 
-  def dashboard_milestone?
+  def project_milestone?
     true
   end
 end

app/models/global_milestone.rb

@@ -3,69 +3,78 @@
 class GlobalMilestone
   include Milestoneish
 
-  EPOCH = DateTime.parse('1970-01-01')
   STATE_COUNT_HASH = { opened: 0, closed: 0, all: 0 }.freeze
 
-  attr_accessor :title, :milestones
+  attr_reader :milestone
   alias_attribute :name, :title
 
+  delegate :title, :state, :due_date, :start_date, :participants, :project, :group, :expires_at, :closed?, :iid, :group_milestone?, :safe_title, :milestoneish_id, to: :milestone
+
+  def to_hash
+    {
+       name: title,
+       title: title,
+       group_name: group&.full_name,
+       project_name: project&.full_name
+    }
+  end
+
   def for_display
-    @first_milestone
+    @milestone
   end
 
   def self.build_collection(projects, params)
-    params =
-      { project_ids: projects.map(&:id), state: params[:state] }
-
-    child_milestones = MilestonesFinder.new(params).execute # rubocop: disable CodeReuse/Finder
-
-    milestones = child_milestones.select(:id, :title).group_by(&:title).map do |title, grouped|
-      milestones_relation = Milestone.where(id: grouped.map(&:id))
-      new(title, milestones_relation)
-    end
+    items = Milestone.of_projects(projects)
+                .reorder_by_due_date_asc
+                .order_by_name_asc
 
-    milestones.sort_by { |milestone| milestone.due_date || EPOCH }
+    Milestone.filter_by_state(items, params[:state]).map { |m| new(m) }
   end
 
+  # necessary for legacy milestones
   def self.build(projects, title)
-    child_milestones = Milestone.of_projects(projects).where(title: title)
-    return if child_milestones.blank?
+    milestones = Milestone.of_projects(projects).where(title: title)
+    return if milestones.blank?
 
-    new(title, child_milestones)
+    new(milestones.first)
   end
 
-  def self.count_by_state(milestones_by_state_and_title, state)
-    milestones_by_state_and_title.count do |(milestone_state, _), _|
-      milestone_state == state
+  def self.states_count(projects, group = nil)
+    legacy_group_milestones_count = legacy_group_milestone_states_count(projects)
+    group_milestones_count = group_milestones_states_count(group)
+
+    legacy_group_milestones_count.merge(group_milestones_count) do |k, legacy_group_milestones_count, group_milestones_count|
+      legacy_group_milestones_count + group_milestones_count
     end
   end
-  private_class_method :count_by_state
 
-  def initialize(title, milestones)
-    @title = title
-    @name = title
-    @milestones = milestones
-    @first_milestone = milestones.find {|m| m.description.present? } || milestones.first
-  end
+  def self.group_milestones_states_count(group)
+    return STATE_COUNT_HASH unless group
 
-  def milestoneish_ids
-    milestones.select(:id)
-  end
+    counts_by_state = Milestone.of_groups(group).count_by_state
 
-  def safe_title
-    @title.to_slug.normalize.to_s
+    {
+      opened: counts_by_state['active'] || 0,
+      closed: counts_by_state['closed'] || 0,
+      all: counts_by_state.values.sum
+    }
   end
 
-  def projects
-    @projects ||= Project.for_milestones(milestoneish_ids)
-  end
+  def self.legacy_group_milestone_states_count(projects)
+    return STATE_COUNT_HASH unless projects
 
-  def state
-    milestones.each do |milestone|
-      return 'active' if milestone.state != 'closed'
-    end
+    # We need to reorder(nil) on the projects, because the controller passes them in sorted.
+    relation = Milestone.of_projects(projects.reorder(nil)).count_by_state
 
-    'closed'
+    {
+      opened: relation['active'] || 0,
+      closed: relation['closed'] || 0,
+      all: relation.values.sum
+    }
+  end
+
+  def initialize(milestone)
+    @milestone = milestone
   end
 
   def active?
@@ -77,37 +86,14 @@ class GlobalMilestone
   end
 
   def issues
-    @issues ||= Issue.of_milestones(milestoneish_ids).includes(:project, :assignees, :labels)
+    @issues ||= Issue.of_milestones(milestone).includes(:project, :assignees, :labels)
   end
 
   def merge_requests
-    @merge_requests ||= MergeRequest.of_milestones(milestoneish_ids).includes(:target_project, :assignee, :labels)
-  end
-
-  def participants
-    @participants ||= milestones.map(&:participants).flatten.uniq
+    @merge_requests ||= MergeRequest.of_milestones(milestone).includes(:target_project, :assignee, :labels)
   end
 
   def labels
-    @labels ||= GlobalLabel.build_collection(milestones.includes(:labels).map(&:labels).flatten)
-                           .sort_by!(&:title)
-  end
-
-  def due_date
-    return @due_date if defined?(@due_date)
-
-    @due_date =
-      if @milestones.all? { |x| x.due_date == @milestones.first.due_date }
-        @milestones.first.due_date
-      end
-  end
-
-  def start_date
-    return @start_date if defined?(@start_date)
-
-    @start_date =
-      if @milestones.all? { |x| x.start_date == @milestones.first.start_date }
-        @milestones.first.start_date
-      end
+    @labels ||= GlobalLabel.build_collection(milestone.labels).sort_by!(&:title)
   end
 end

app/models/group_milestone.rb

 # frozen_string_literal: true
 # Group Milestones are milestones that can be shared among many projects within the same group
 class GroupMilestone < GlobalMilestone
-  attr_accessor :group
+  attr_reader :group, :milestones
 
   def self.build_collection(group, projects, params)
-    super(projects, params).each do |milestone|
-      milestone.group = group
+    params =
+      { state: params[:state] }
+
+    project_milestones = Milestone.of_projects(projects)
+    child_milestones = Milestone.filter_by_state(project_milestones, params[:state])
+    grouped_milestones = child_milestones.group_by(&:title)
+
+    grouped_milestones.map do |title, grouped|
+      new(title, grouped, group)
     end
   end
 
   def self.build(group, projects, title)
-    super(projects, title).tap do |milestone|
-      milestone&.group = group
-    end
+    child_milestones = Milestone.of_projects(projects).where(title: title)
+    return if child_milestones.blank?
+
+    new(title, child_milestones, group)
+  end
+
+  def initialize(title, milestones, group)
+    @milestones = milestones
+    @group = group
+  end
+
+  def milestone
+    @milestone ||= milestones.find { |m| m.description.present? } || milestones.first
   end
 
   def issues_finder_params

app/models/milestone.rb

@@ -94,6 +94,10 @@ class Milestone < ActiveRecord::Base
       end
     end
 
+    def count_by_state
+      reorder(nil).group(:state).count
+    end
+
     def predefined?(milestone)
       milestone == Any ||
         milestone == None ||
@@ -215,7 +219,7 @@ class Milestone < ActiveRecord::Base
     self.class.reference_prefix + self.title
   end
 
-  def milestoneish_ids
+  def milestoneish_id
     id
   end
 

app/views/dashboard/milestones/_milestone.html.haml

 = render 'shared/milestones/milestone',
-          milestone_path: group_or_dashboard_milestone_path(milestone),
+          milestone_path: group_or_project_milestone_path(milestone),
           issues_path: issues_dashboard_path(milestone_title: milestone.title),
           merge_requests_path: merge_requests_dashboard_path(milestone_title: milestone.title),
           milestone: milestone,

app/views/shared/milestones/_milestone.html.haml

 - dashboard = local_assigns[:dashboard]
-- custom_dom_id = dom_id(milestone.try(:milestones) ? milestone.milestones.first : milestone)
+- custom_dom_id = dom_id(milestone.try(:milestone) ? milestone.milestone : milestone)
 - milestone_type = milestone.group_milestone? ? 'Group Milestone' : 'Project Milestone'
 
 %li{ class: "milestone milestone-#{milestone.closed? ? 'closed' : 'open'}", id: custom_dom_id }
@@ -21,10 +21,12 @@
               = milestone.group.full_name
           - if milestone.legacy_group_milestone?
             .projects
-              - milestone.milestones.each do |milestone|
-                = link_to milestone_path(milestone) do
-                  %span.label-badge.label-badge-blue.d-inline-block.append-bottom-5
-                    = dashboard ? milestone.project.full_name : milestone.project.name
+              - link_to milestone_path(milestone.milestone) do
+                %span.label-badge.label-badge-blue.d-inline-block.append-bottom-5
+                  = dashboard ? milestone.project.full_name : milestone.project.name
+          - if milestone.project
+            .label-badge.label-badge-gray.d-inline-block
+              = milestone.project.full_name
 
     .col-sm-4.milestone-progress
       = milestone_progress_bar(milestone)
@@ -58,5 +60,5 @@
             - else
               = link_to 'Close Milestone', group_milestone_route(milestone, {state_event: :close }), method: :put, class: "btn btn-sm btn-grouped btn-close"
         - if dashboard
-          .status-box.status-box-milestone
+          .label-badge.label-badge-gray
             = milestone_type

app/views/shared/milestones/_top.html.haml

@@ -62,20 +62,19 @@
           %th Open issues
           %th State
           %th Due date
-      - milestone.milestones.each do |ms|
         %tr
           %td
-            - project_name = group ? ms.project.name : ms.project.full_name
-            = link_to project_name, project_milestone_path(ms.project, ms)
+            - project_name = group ? milestone.project.name : milestone.project.full_name
+            = link_to project_name, milestone_path(milestone.milestone)
           %td
-            = ms.issues_visible_to_user(current_user).opened.count
+            = milestone.milestone.issues_visible_to_user(current_user).opened.count
           %td
-            - if ms.closed?
+            - if milestone.closed?
               Closed
             - else
               Open
           %td
-            = ms.expires_at
+            = milestone.expires_at
 - elsif milestone.group_milestone?
   %br
   View

changelogs/unreleased/ccr-49289_milestone_link.yml

+---
+title: Add project milestone link
+merge_request: 22552
+author:
+type: added

spec/controllers/dashboard/milestones_controller_spec.rb

@@ -52,7 +52,7 @@ describe Dashboard::MilestonesController do
 
       expect(response).to have_gitlab_http_status(200)
       expect(json_response.size).to eq(2)
-      expect(json_response.map { |i| i["first_milestone"]["id"] }).to match_array([group_milestone.id, project_milestone.id])
+      expect(json_response.map { |i| i["name"] }).to match_array([group_milestone.name, project_milestone.name])
       expect(json_response.map { |i| i["group_name"] }.compact).to match_array(group.name)
     end
 

spec/controllers/groups/milestones_controller_spec.rb

@@ -64,7 +64,7 @@ describe Groups::MilestonesController do
 
     context 'when there is a title parameter' do
       it 'searches for a legacy group milestone' do
-        expect(GlobalMilestone).to receive(:build)
+        expect(GroupMilestone).to receive(:build)
         expect(Milestone).not_to receive(:find_by_iid)
 
         get :show, params: { group_id: group.to_param, id: title, title: milestone1.safe_title }

spec/features/groups/milestone_spec.rb

@@ -81,7 +81,7 @@ describe 'Group milestones' do
           description: 'Lorem Ipsum is simply dummy text'
         )
       end
-      let!(:active_project_milestone2) { create(:milestone, project: other_project, state: 'active', title: 'v1.0') }
+      let!(:active_project_milestone2) { create(:milestone, project: other_project, state: 'active', title: 'v1.1') }
       let!(:closed_project_milestone1) { create(:milestone, project: project, state: 'closed', title: 'v2.0') }
       let!(:closed_project_milestone2) { create(:milestone, project: other_project, state: 'closed', title: 'v2.0') }
       let!(:active_group_milestone) { create(:milestone, group: group, state: 'active', title: 'GL-113') }
@@ -104,7 +104,7 @@ describe 'Group milestones' do
         legacy_milestone = GroupMilestone.build_collection(group, group.projects, { state: 'active' }).first
 
         expect(page).to have_selector("#milestone_#{active_group_milestone.id}", count: 1)
-        expect(page).to have_selector("#milestone_#{legacy_milestone.milestones.first.id}", count: 1)
+        expect(page).to have_selector("#milestone_#{legacy_milestone.milestone.id}", count: 1)
       end
 
       it 'shows milestone detail and supports its edit' do
@@ -121,6 +121,7 @@ describe 'Group milestones' do
 
       it 'renders milestones' do
         expect(page).to have_content('v1.0')
+        expect(page).to have_content('v1.1')
         expect(page).to have_content('GL-113')
         expect(page).to have_link(
           '1 Issue',

spec/features/groups/milestones_sorting_spec.rb

@@ -42,6 +42,7 @@ describe 'Milestones sorting', :js do
 
     expect(page).to have_button('Due later')
 
+    # assert descending sorting
     within '.milestones' do
       expect(page.all('ul.content-list > li').first.text).to include('v1.0')
       expect(page.all('ul.content-list > li')[1].text).to include('v3.0')

spec/models/global_milestone_spec.rb

@@ -65,56 +65,103 @@ describe GlobalMilestone do
       )
     end
 
-    before do
-      projects = [
+    let!(:projects) do
+      [
         project1,
         project2,
         project3
       ]
-
-      @global_milestones = described_class.build_collection(projects, {})
     end
 
-    it 'has all project milestones' do
-      expect(@global_milestones.count).to eq(2)
+    let!(:global_milestones) { described_class.build_collection(projects, {}) }
+
+    context 'when building a collection of milestones' do
+      it 'has all project milestones' do
+        expect(global_milestones.count).to eq(6)
+      end
+
+      it 'has all project milestones titles' do
+        expect(global_milestones.map(&:title)).to match_array(['Milestone v1.2', 'Milestone v1.2', 'Milestone v1.2', 'VD-123', 'VD-123', 'VD-123'])
+      end
+
+      it 'has all project milestones' do
+        expect(global_milestones.size).to eq(6)
+      end
+
+      it 'sorts collection by due date' do
+        expect(global_milestones.map(&:due_date)).to eq [milestone1_due_date, milestone1_due_date, milestone1_due_date, nil, nil, nil]
+      end
     end
 
-    it 'has all project milestones titles' do
-      expect(@global_milestones.map(&:title)).to match_array(['Milestone v1.2', 'VD-123'])
+    context 'when adding new milestones' do
+      it 'does not add more queries' do
+        control_count = ActiveRecord::QueryRecorder.new do
+          described_class.build_collection(projects, {})
+        end.count
+
+        create_list(:milestone, 3, project: project3)
+
+        expect do
+          described_class.build_collection(projects, {})
+        end.not_to exceed_all_query_limit(control_count)
+      end
     end
+  end
+
+  describe '.states_count' do
+    context 'when the projects have milestones' do
+      before do
+        create(:closed_milestone, title: 'Active Group Milestone', project: project3)
+        create(:active_milestone, title: 'Active Group Milestone', project: project1)
+        create(:active_milestone, title: 'Active Group Milestone', project: project2)
+        create(:closed_milestone, title: 'Closed Group Milestone', project: project1)
+        create(:closed_milestone, title: 'Closed Group Milestone', project: project2)
+        create(:closed_milestone, title: 'Closed Group Milestone', project: project3)
+        create(:closed_milestone, title: 'Closed Group Milestone 4', group: group)
+      end
+
+      it 'returns the quantity of global milestones and group milestones in each possible state' do
+        expected_count = { opened: 2, closed: 5, all: 7 }
 
-    it 'has all project milestones' do
-      expect(@global_milestones.map { |group_milestone| group_milestone.milestones.count }.sum).to eq(6)
+        count = described_class.states_count(Project.all, group)
+
+        expect(count).to eq(expected_count)
+      end
+
+      it 'returns the quantity of global milestones in each possible state' do
+        expected_count = { opened: 2, closed: 4, all: 6 }
+
+        count = described_class.states_count(Project.all)
+
+        expect(count).to eq(expected_count)
+      end
     end
 
-    it 'sorts collection by due date' do
-      expect(@global_milestones.map(&:due_date)).to eq [nil, milestone1_due_date]
+    context 'when the projects do not have milestones' do
+      before do
+        project1
+      end
+
+      it 'returns 0 as the quantity of global milestones in each state' do
+        expected_count = { opened: 0, closed: 0, all: 0 }
+
+        count = described_class.states_count(Project.all)
+
+        expect(count).to eq(expected_count)
+      end
     end
   end
 
   describe '#initialize' do
     let(:milestone1_project1) { create(:milestone, title: "Milestone v1.2", project: project1) }
-    let(:milestone1_project2) { create(:milestone, title: "Milestone v1.2", project: project2) }
-    let(:milestone1_project3) { create(:milestone, title: "Milestone v1.2", project: project3) }
-
-    before do
-      milestones =
-        [
-          milestone1_project1,
-          milestone1_project2,
-          milestone1_project3
-        ]
-      milestones_relation = Milestone.where(id: milestones.map(&:id))
-
-      @global_milestone = described_class.new(milestone1_project1.title, milestones_relation)
-    end
+    subject(:global_milestone) { described_class.new(milestone1_project1) }
 
     it 'has exactly one group milestone' do
-      expect(@global_milestone.title).to eq('Milestone v1.2')
+      expect(global_milestone.title).to eq('Milestone v1.2')
     end
 
     it 'has all project milestones with the same title' do
-      expect(@global_milestone.milestones.count).to eq(3)
+      expect(global_milestone.milestone).to eq(milestone1_project1)
     end
   end
 
@@ -122,7 +169,7 @@ describe GlobalMilestone do
     let(:milestone) { create(:milestone, title: "git / test", project: project1) }
 
     it 'strips out slashes and spaces' do
-      global_milestone = described_class.new(milestone.title, Milestone.where(id: milestone.id))
+      global_milestone = described_class.new(milestone)
 
       expect(global_milestone.safe_title).to eq('git-test')
     end
@@ -132,11 +179,8 @@ describe GlobalMilestone do
     context 'when at least one milestone is active' do
       it 'returns active' do
         title = 'Active Group Milestone'
-        milestones = [
-          create(:active_milestone, title: title),
-          create(:closed_milestone, title: title)
-        ]
-        global_milestone = described_class.new(title, milestones)
+
+        global_milestone = described_class.new(create(:active_milestone, title: title))
 
         expect(global_milestone.state).to eq('active')
       end
@@ -145,11 +189,8 @@ describe GlobalMilestone do
     context 'when all milestones are closed' do
       it 'returns closed' do
         title = 'Closed Group Milestone'
-        milestones = [
-          create(:closed_milestone, title: title),
-          create(:closed_milestone, title: title)
-        ]
-        global_milestone = described_class.new(title, milestones)
+
+        global_milestone = described_class.new(create(:closed_milestone, title: title))
 
         expect(global_milestone.state).to eq('closed')
       end

spec/models/group_milestone_spec.rb

@@ -20,13 +20,36 @@ describe GroupMilestone do
   end
 
   describe '.build_collection' do
-    before do
-      project_milestone
+    let(:group) { create(:group) }
+    let(:project1) { create(:project, group: group) }
+    let(:project2) { create(:project, path: 'gitlab-ci', group: group) }
+    let(:project3) { create(:project, path: 'cookbook-gitlab', group: group) }
+
+    let!(:projects) do
+      [
+          project1,
+          project2,
+          project3
+      ]
     end
 
     it 'returns array of milestones, each with group assigned' do
       milestones = described_class.build_collection(group, [project], {})
       expect(milestones).to all(have_attributes(group: group))
     end
+
+    context 'when adding new milestones' do
+      it 'does not add more queries' do
+        control_count = ActiveRecord::QueryRecorder.new do
+          described_class.build_collection(group, projects, {})
+        end.count
+
+        create(:milestone, title: 'This title', project: project1)
+
+        expect do
+          described_class.build_collection(group, projects, {})
+        end.not_to exceed_all_query_limit(control_count)
+      end
+    end
   end
 end