From 3cd511733b5b646becfdf72e36062b863dfbcf20 Mon Sep 17 00:00:00 2001 From: Ronald van Zon Date: Thu, 30 Aug 2018 14:17:36 +0000 Subject: Fixing count on Milestones By adding groups to milestones we can now include them in the count of Open and Closed. --- app/controllers/dashboard/application_controller.rb | 4 ++++ app/controllers/dashboard/milestones_controller.rb | 3 ++- app/models/global_milestone.rb | 20 ++++++++++++++++++-- changelogs/unreleased/rz_fix_milestone_count.yml | 5 +++++ 4 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/rz_fix_milestone_count.yml diff --git a/app/controllers/dashboard/application_controller.rb b/app/controllers/dashboard/application_controller.rb index cee0753a021..1c9a5917da5 100644 --- a/app/controllers/dashboard/application_controller.rb +++ b/app/controllers/dashboard/application_controller.rb @@ -12,4 +12,8 @@ class Dashboard::ApplicationController < ApplicationController def projects @projects ||= current_user.authorized_projects.sorted_by_activity.non_archived end + + def groups + @groups ||= GroupsFinder.new(current_user, state_all: true).execute + end end diff --git a/app/controllers/dashboard/milestones_controller.rb b/app/controllers/dashboard/milestones_controller.rb index 6e17bc212e4..ddc1a66d11d 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 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 = GlobalMilestone.states_count(@projects, @groups) @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..d07ef43c97e 100644 --- a/app/models/global_milestone.rb +++ b/app/models/global_milestone.rb @@ -34,15 +34,31 @@ class GlobalMilestone new(title, child_milestones) end - def self.states_count(projects, group = nil) + def self.states_count(projects, groups = nil) legacy_group_milestones_count = legacy_group_milestone_states_count(projects) - group_milestones_count = group_milestones_states_count(group) + group_milestones_count = groups_milestone_state_count(groups) 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.groups_milestone_state_count(groups) + return STATE_COUNT_HASH unless groups + return self.group_milestones_states_count(groups) unless groups.respond_to?(:each) + + milestone_states = STATE_COUNT_HASH + + groups.each do |group| + group_milestones_count = self.group_milestones_states_count(group) + milestone_states = milestone_states.merge(group_milestones_count) do |k, milestone_state, group_milestones_count| + milestone_state + group_milestones_count + end + end + + milestone_states + end + def self.group_milestones_states_count(group) return STATE_COUNT_HASH unless group diff --git a/changelogs/unreleased/rz_fix_milestone_count.yml b/changelogs/unreleased/rz_fix_milestone_count.yml new file mode 100644 index 00000000000..f85cfe48b2d --- /dev/null +++ b/changelogs/unreleased/rz_fix_milestone_count.yml @@ -0,0 +1,5 @@ +--- +title: Fixing count on Milestones +merge_request: 21446 +author: eagllus +type: fixed -- cgit v1.2.3 From 43f32c6bf966a85d6d5e1d8933a1cea67ec89ba4 Mon Sep 17 00:00:00 2001 From: Eagllus Date: Wed, 3 Oct 2018 15:19:13 +0200 Subject: Add test for the milestone count --- spec/controllers/dashboard/milestones_controller_spec.rb | 9 +++++++++ 1 file changed, 9 insertions(+) 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\n3") + expect(response.body).to include("Closed\n0") + end end end -- cgit v1.2.3 From 074fafe9e09935cd53cf286ad00b9d53240a9413 Mon Sep 17 00:00:00 2001 From: Eagllus Date: Tue, 16 Oct 2018 14:52:08 +0200 Subject: Update code according comment recommendations --- app/controllers/dashboard/application_controller.rb | 4 ---- app/controllers/dashboard/milestones_controller.rb | 8 ++++++-- app/controllers/groups/milestones_controller.rb | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/dashboard/application_controller.rb b/app/controllers/dashboard/application_controller.rb index 1c9a5917da5..cee0753a021 100644 --- a/app/controllers/dashboard/application_controller.rb +++ b/app/controllers/dashboard/application_controller.rb @@ -12,8 +12,4 @@ class Dashboard::ApplicationController < ApplicationController def projects @projects ||= current_user.authorized_projects.sorted_by_activity.non_archived end - - def groups - @groups ||= GroupsFinder.new(current_user, state_all: true).execute - end end diff --git a/app/controllers/dashboard/milestones_controller.rb b/app/controllers/dashboard/milestones_controller.rb index ddc1a66d11d..8252a0f2511 100644 --- a/app/controllers/dashboard/milestones_controller.rb +++ b/app/controllers/dashboard/milestones_controller.rb @@ -4,13 +4,13 @@ class Dashboard::MilestonesController < Dashboard::ApplicationController include MilestoneActions before_action :projects - before_action :groups + 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, @groups) + @milestone_states = Milestone.states_count(@projects, @groups) @milestones = Kaminari.paginate_array(milestones).page(params[:page]) end format.json do @@ -43,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 -- cgit v1.2.3 From d96585f5739f4cb83fd00fa402192a15d6958881 Mon Sep 17 00:00:00 2001 From: Eagllus Date: Tue, 16 Oct 2018 15:18:25 +0200 Subject: Moving state_count to Milestone model and related tests By moving and improving state_count the functions in GlobalMilestone are no longer used. --- app/models/global_milestone.rb | 60 ------------------------------------ app/models/milestone.rb | 16 ++++++++++ spec/models/global_milestone_spec.rb | 35 --------------------- spec/models/milestone_spec.rb | 35 +++++++++++++++++++++ 4 files changed, 51 insertions(+), 95 deletions(-) diff --git a/app/models/global_milestone.rb b/app/models/global_milestone.rb index d07ef43c97e..085ffd16c6a 100644 --- a/app/models/global_milestone.rb +++ b/app/models/global_milestone.rb @@ -34,66 +34,6 @@ class GlobalMilestone new(title, child_milestones) end - def self.states_count(projects, groups = nil) - legacy_group_milestones_count = legacy_group_milestone_states_count(projects) - group_milestones_count = groups_milestone_state_count(groups) - - 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.groups_milestone_state_count(groups) - return STATE_COUNT_HASH unless groups - return self.group_milestones_states_count(groups) unless groups.respond_to?(:each) - - milestone_states = STATE_COUNT_HASH - - groups.each do |group| - group_milestones_count = self.group_milestones_states_count(group) - milestone_states = milestone_states.merge(group_milestones_count) do |k, milestone_state, group_milestones_count| - milestone_state + group_milestones_count - end - end - - milestone_states - 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/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..b6aad769e3b 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -348,4 +348,39 @@ describe Milestone do end end end + + describe '.states_count' do + context 'when the projects have milestones' do + let(:project_1) { create(:project) } + let(:project_2) { create(:project) } + let(:project_3) { create(:project) } + + before do + create(:closed_milestone, title: 'Active Group Milestone', project: project_3) + create(:active_milestone, title: 'Active Group Milestone', project: project_1) + create(:active_milestone, title: 'Active Group Milestone', project: project_2) + create(:closed_milestone, title: 'Closed Group Milestone', project: project_1) + create(:closed_milestone, title: 'Closed Group Milestone', project: project_2) + create(:closed_milestone, title: 'Closed Group Milestone', project: project_3) + end + + it 'returns the quantity of milestones in each possible state' do + expected_count = { opened: 5, closed: 4, all: 9 } + + count = described_class.states_count(Project.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 -- cgit v1.2.3 From c42f15c03f3e4f72aa0bd5d978cc4ee1cf2a75de Mon Sep 17 00:00:00 2001 From: Eagllus Date: Thu, 18 Oct 2018 09:13:49 +0200 Subject: Update related tests --- spec/features/groups/milestone_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/features/groups/milestone_spec.rb b/spec/features/groups/milestone_spec.rb index 80df0618a6a..a7ef215df5f 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 -- cgit v1.2.3 From 473138f1151905b86527c2664d822c056f57b5cd Mon Sep 17 00:00:00 2001 From: Eagllus Date: Mon, 22 Oct 2018 13:56:08 +0200 Subject: Update related tests based on comment --- spec/models/milestone_spec.rb | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index b6aad769e3b..617f0151f33 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -353,21 +353,26 @@ describe Milestone do context 'when the projects have milestones' do let(:project_1) { create(:project) } let(:project_2) { create(:project) } - let(:project_3) { create(:project) } + let(:group_1) { create(:group) } + let(:group_2) { create(:group) } before do - create(:closed_milestone, title: 'Active Group Milestone', project: project_3) create(:active_milestone, title: 'Active Group Milestone', project: project_1) - create(:active_milestone, title: 'Active Group Milestone', project: project_2) 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: 'Closed Group Milestone', project: project_3) + 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: 4, all: 9 } + expected_count = { opened: 5, closed: 6, all: 11 } + + count = described_class.states_count(Project.all, Group.all) - count = described_class.states_count(Project.all) + p Project.all, Group.all expect(count).to eq(expected_count) end -- cgit v1.2.3 From 0ff5b0f45361cf9fe622cc3265b943a999c14d0e Mon Sep 17 00:00:00 2001 From: Ronald van Zon Date: Thu, 25 Oct 2018 09:52:26 +0000 Subject: Removed the print in test --- spec/models/milestone_spec.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 617f0151f33..651f0540824 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -371,9 +371,6 @@ describe Milestone do expected_count = { opened: 5, closed: 6, all: 11 } count = described_class.states_count(Project.all, Group.all) - - p Project.all, Group.all - expect(count).to eq(expected_count) end end -- cgit v1.2.3 From 3b70cf69188b9906098df5bd984e9337f16e1080 Mon Sep 17 00:00:00 2001 From: Eagllus Date: Fri, 26 Oct 2018 13:43:28 +0200 Subject: Update MR based on Sean's feedback --- app/controllers/dashboard/milestones_controller.rb | 2 +- changelogs/unreleased/rz_fix_milestone_count.yml | 2 +- spec/models/milestone_spec.rb | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/dashboard/milestones_controller.rb b/app/controllers/dashboard/milestones_controller.rb index 8252a0f2511..3802aa5f40f 100644 --- a/app/controllers/dashboard/milestones_controller.rb +++ b/app/controllers/dashboard/milestones_controller.rb @@ -10,7 +10,7 @@ class Dashboard::MilestonesController < Dashboard::ApplicationController def index respond_to do |format| format.html do - @milestone_states = Milestone.states_count(@projects, @groups) + @milestone_states = Milestone.states_count(@projects.select(:id), @groups.select(:id)) @milestones = Kaminari.paginate_array(milestones).page(params[:page]) end format.json do diff --git a/changelogs/unreleased/rz_fix_milestone_count.yml b/changelogs/unreleased/rz_fix_milestone_count.yml index f85cfe48b2d..1013b88e0bc 100644 --- a/changelogs/unreleased/rz_fix_milestone_count.yml +++ b/changelogs/unreleased/rz_fix_milestone_count.yml @@ -1,5 +1,5 @@ --- title: Fixing count on Milestones merge_request: 21446 -author: eagllus +author: type: fixed diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 651f0540824..d11eb46159e 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -351,12 +351,12 @@ describe Milestone do describe '.states_count' do context 'when the projects have milestones' do - let(:project_1) { create(:project) } - let(:project_2) { create(:project) } - let(:group_1) { create(:group) } - let(:group_2) { create(:group) } - 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) -- cgit v1.2.3