Merge branch 'rz_fix_milestone_count' into 'master'

Fixing count on Milestones

Closes #50848

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

9 files changed, 77 insertions, 84 deletions

diff --git a/app/controllers/dashboard/milestones_controller.rb b/app/controllers/dashboard/milestones_controller.rb
index 6e17bc212e4..3802aa5f40f 100644
--- a/app/controllers/dashboard/milestones_controller.rb
+++ b/app/controllers/dashboard/milestones_controller.rb
@@ -4,12 +4,13 @@ class Dashboard::MilestonesController < Dashboard::ApplicationController
   include MilestoneActions
 
   before_action :projects
+  before_action :groups, only: :index
   before_action :milestone, only: [:show, :merge_requests, :participants, :labels]
 
   def index
     respond_to do |format|
       format.html do
-        @milestone_states = GlobalMilestone.states_count(@projects)
+        @milestone_states = Milestone.states_count(@projects.select(:id), @groups.select(:id))
         @milestones = Kaminari.paginate_array(milestones).page(params[:page])
       end
       format.json do
@@ -42,4 +43,8 @@ class Dashboard::MilestonesController < Dashboard::ApplicationController
     @milestone = DashboardMilestone.build(@projects, params[:title])
     render_404 unless @milestone
   end
+
+  def groups
+    @groups ||= GroupsFinder.new(current_user, state_all: true).execute
+  end
 end
diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb
index a7cee426cf1..b42116b0f36 100644
--- a/app/controllers/groups/milestones_controller.rb
+++ b/app/controllers/groups/milestones_controller.rb
@@ -10,7 +10,7 @@ class Groups::MilestonesController < Groups::ApplicationController
   def index
     respond_to do |format|
       format.html do
-        @milestone_states = GlobalMilestone.states_count(group_projects, group)
+        @milestone_states = Milestone.states_count(group_projects, [group])
         @milestones = Kaminari.paginate_array(milestones).page(params[:page])
       end
       format.json do
diff --git a/app/models/global_milestone.rb b/app/models/global_milestone.rb
index a6cebabe089..085ffd16c6a 100644
--- a/app/models/global_milestone.rb
+++ b/app/models/global_milestone.rb
@@ -34,50 +34,6 @@ class GlobalMilestone
     new(title, child_milestones)
   end
 
-  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
-
-  def self.group_milestones_states_count(group)
-    return STATE_COUNT_HASH unless group
-
-    params = { group_ids: [group.id], state: 'all' }
-
-    relation = MilestonesFinder.new(params).execute # rubocop: disable CodeReuse/Finder
-    grouped_by_state = relation.reorder(nil).group(:state).count
-
-    {
-      opened: grouped_by_state['active'] || 0,
-      closed: grouped_by_state['closed'] || 0,
-      all: grouped_by_state.values.sum
-    }
-  end
-
-  # Counts the legacy group milestones which must be grouped by title
-  def self.legacy_group_milestone_states_count(projects)
-    return STATE_COUNT_HASH unless projects
-
-    params = { project_ids: projects.map(&:id), state: 'all' }
-
-    relation = MilestonesFinder.new(params).execute # rubocop: disable CodeReuse/Finder
-    project_milestones_by_state_and_title = relation.reorder(nil).group(:state, :title).count
-
-    opened = count_by_state(project_milestones_by_state_and_title, 'active')
-    closed = count_by_state(project_milestones_by_state_and_title, 'closed')
-    all = project_milestones_by_state_and_title.map { |(_, title), _| title }.uniq.count
-
-    {
-      opened: opened,
-      closed: closed,
-      all: all
-    }
-  end
-
   def self.count_by_state(milestones_by_state_and_title, state)
     milestones_by_state_and_title.count do |(milestone_state, _), _|
       milestone_state == state
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 892a680f221..9f2c4efaa96 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -170,6 +170,22 @@ class Milestone < ActiveRecord::Base
     sorted.with_order_id_desc
   end
 
+  def self.states_count(projects, groups = nil)
+    return STATE_COUNT_HASH unless projects || groups
+
+    counts = Milestone
+               .for_projects_and_groups(projects&.map(&:id), groups&.map(&:id))
+               .reorder(nil)
+               .group(:state)
+               .count
+
+    {
+        opened: counts['active'] || 0,
+        closed: counts['closed'] || 0,
+        all: counts.values.sum
+    }
+  end
+
   ##
   # Returns the String necessary to reference this Milestone in Markdown. Group
   # milestones only support name references, and do not support cross-project
diff --git a/changelogs/unreleased/rz_fix_milestone_count.yml b/changelogs/unreleased/rz_fix_milestone_count.yml
new file mode 100644
index 00000000000..1013b88e0bc
--- /dev/null
+++ b/changelogs/unreleased/rz_fix_milestone_count.yml
@@ -0,0 +1,5 @@
+---
+title: Fixing count on Milestones
+merge_request: 21446
+author:
+type: fixed
diff --git a/spec/controllers/dashboard/milestones_controller_spec.rb b/spec/controllers/dashboard/milestones_controller_spec.rb
index 56047c0c8d2..278b980b6d8 100644
--- a/spec/controllers/dashboard/milestones_controller_spec.rb
+++ b/spec/controllers/dashboard/milestones_controller_spec.rb
@@ -45,6 +45,8 @@ describe Dashboard::MilestonesController do
   end
 
   describe "#index" do
+    render_views
+
     it 'returns group and project milestones to which the user belongs' do
       get :index, format: :json
 
@@ -53,5 +55,12 @@ describe Dashboard::MilestonesController do
       expect(json_response.map { |i| i["first_milestone"]["id"] }).to match_array([group_milestone.id, project_milestone.id])
       expect(json_response.map { |i| i["group_name"] }.compact).to match_array(group.name)
     end
+
+    it 'should contain group and project milestones to which the user belongs to' do
+      get :index
+
+      expect(response.body).to include("Open\n<span class=\"badge badge-pill\">3</span>")
+      expect(response.body).to include("Closed\n<span class=\"badge badge-pill\">0</span>")
+    end
   end
 end
diff --git a/spec/features/groups/milestone_spec.rb b/spec/features/groups/milestone_spec.rb
index e8ca6a6714f..174840794ed 100644
--- a/spec/features/groups/milestone_spec.rb
+++ b/spec/features/groups/milestone_spec.rb
@@ -95,9 +95,9 @@ describe 'Group milestones' do
       end
 
       it 'counts milestones correctly' do
-        expect(find('.top-area .active .badge').text).to eq("2")
-        expect(find('.top-area .closed .badge').text).to eq("2")
-        expect(find('.top-area .all .badge').text).to eq("4")
+        expect(find('.top-area .active .badge').text).to eq("3")
+        expect(find('.top-area .closed .badge').text).to eq("3")
+        expect(find('.top-area .all .badge').text).to eq("6")
       end
 
       it 'lists legacy group milestones and group milestones' do
diff --git a/spec/models/global_milestone_spec.rb b/spec/models/global_milestone_spec.rb
index ab58f5c5021..b6355455c1d 100644
--- a/spec/models/global_milestone_spec.rb
+++ b/spec/models/global_milestone_spec.rb
@@ -92,41 +92,6 @@ describe GlobalMilestone do
     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)
-      end
-
-      it 'returns the quantity of global milestones in each possible state' do
-        expected_count = { opened: 1, closed: 2, all: 2 }
-
-        count = described_class.states_count(Project.all)
-
-        expect(count).to eq(expected_count)
-      end
-    end
-
-    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) }
diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb
index 27d4e622710..d11eb46159e 100644
--- a/spec/models/milestone_spec.rb
+++ b/spec/models/milestone_spec.rb
@@ -348,4 +348,41 @@ describe Milestone do
       end
     end
   end
+
+  describe '.states_count' do
+    context 'when the projects have milestones' do
+      before do
+        project_1 = create(:project)
+        project_2 = create(:project)
+        group_1 = create(:group)
+        group_2 = create(:group)
+
+        create(:active_milestone, title: 'Active Group Milestone', project: project_1)
+        create(:closed_milestone, title: 'Closed Group Milestone', project: project_1)
+        create(:active_milestone, title: 'Active Group Milestone', project: project_2)
+        create(:closed_milestone, title: 'Closed Group Milestone', project: project_2)
+        create(:closed_milestone, title: 'Active Group Milestone', group: group_1)
+        create(:closed_milestone, title: 'Closed Group Milestone', group: group_1)
+        create(:closed_milestone, title: 'Active Group Milestone', group: group_2)
+        create(:closed_milestone, title: 'Closed Group Milestone', group: group_2)
+      end
+
+      it 'returns the quantity of milestones in each possible state' do
+        expected_count = { opened: 5, closed: 6, all: 11 }
+
+        count = described_class.states_count(Project.all, Group.all)
+        expect(count).to eq(expected_count)
+      end
+    end
+
+    context 'when the projects do not have milestones' do
+      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])
+
+        expect(count).to eq(expected_count)
+      end
+    end
+  end
 end