Filter group milestones by state

8 files changed, 72 insertions, 27 deletions

diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb
index 43382886031..420ac51f11e 100644
--- a/app/controllers/groups/milestones_controller.rb
+++ b/app/controllers/groups/milestones_controller.rb
@@ -64,11 +64,10 @@ class Groups::MilestonesController < Groups::ApplicationController
   end
 
   def milestones
-    @group_milestones = GroupMilestone.all
-    @project_milestones = Milestone.where(project_id: group.projects.pluck(:id))
+    @group_milestones = GroupMilestonesFinder.new(group, params).execute
+    @project_milestones = ProjectMilestonesFinder.new(@projects, params).execute
 
     @group_milestones + @project_milestones
-    #@milestones = GroupMilestone.build_collection(@group, @projects, params)
   end
 
   def milestone
diff --git a/app/finders/group_milestones_finder.rb b/app/finders/group_milestones_finder.rb
new file mode 100644
index 00000000000..d5bf810c41b
--- /dev/null
+++ b/app/finders/group_milestones_finder.rb
@@ -0,0 +1,12 @@
+class GroupMilestonesFinder
+  attr_reader :group, :params
+
+  def initialize(group, params)
+    @group = group
+    @params = params
+  end
+
+  def execute
+    GroupMilestone.filter_by_state(group.milestones, params[:state])
+  end
+end
diff --git a/app/finders/milestones_finder.rb b/app/finders/milestones_finder.rb
deleted file mode 100644
index 259d8d40d98..00000000000
--- a/app/finders/milestones_finder.rb
+++ /dev/null
@@ -1,15 +0,0 @@
-class MilestonesFinder
-  def execute(projects, group = nil, params)
-    milestones = group ? group.milestones : Milestone.of_projects(projects)
-
-    filter_by_state(milestones, params[:state])
-  end
-
-  def filter_by_state(milestones, state)
-    case state
-    when 'closed' then milestones.closed
-    when 'all' then milestones
-    else milestones.active
-    end
-  end
-end
diff --git a/app/finders/project_milestones_finder.rb b/app/finders/project_milestones_finder.rb
new file mode 100644
index 00000000000..a9ffbc9de55
--- /dev/null
+++ b/app/finders/project_milestones_finder.rb
@@ -0,0 +1,14 @@
+class ProjectMilestonesFinder
+  attr_reader :projects, :params
+
+  def initialize(projects, params)
+    @projects = projects
+    @params = params
+  end
+
+  def execute
+    milestones = Milestone.of_projects(projects)
+
+    Milestone.filter_by_state(milestones, params[:state])
+  end
+end
diff --git a/app/models/concerns/milestone_model_properties.rb b/app/models/concerns/shared_milestone_properties.rb
index d7d3bd25018..abb6cb34f05 100644
--- a/app/models/concerns/milestone_model_properties.rb
+++ b/app/models/concerns/shared_milestone_properties.rb
@@ -1,4 +1,4 @@
-module MilestoneModelProperties
+module SharedMilestoneProperties
   extend ActiveSupport::Concern
 
   include StripAttribute
@@ -39,6 +39,16 @@ module MilestoneModelProperties
     cache_markdown_field :description
   end
 
+  module ClassMethods
+    def filter_by_state(milestones, state)
+      case state
+      when 'closed' then milestones.closed
+      when 'all' then milestones
+      else milestones.active
+      end
+    end
+  end
+
   def start_date_should_be_less_than_due_date
     if due_date <= start_date
       errors.add(:start_date, "Can't be greater than due date")
diff --git a/app/models/global_milestone.rb b/app/models/global_milestone.rb
index 5d3209156cc..afe680a7202 100644
--- a/app/models/global_milestone.rb
+++ b/app/models/global_milestone.rb
@@ -2,6 +2,7 @@ class GlobalMilestone
   include Milestoneish
 
   EPOCH = DateTime.parse('1970-01-01')
+  STATE_COUNT_HASH = { opened: 0, closed: 0, all: 0 }
 
   attr_accessor :title, :milestones
   alias_attribute :name, :title
@@ -11,7 +12,7 @@ class GlobalMilestone
   end
 
   def self.build_collection(projects, params)
-    child_milestones = MilestonesFinder.new.execute(projects, params)
+    child_milestones = ProjectMilestonesFinder.new.execute(projects, params)
 
     milestones = child_milestones.select(:id, :title).group_by(&:title).map do |title, grouped|
       milestones_relation = Milestone.where(id: grouped.map(&:id))
@@ -29,12 +30,36 @@ class GlobalMilestone
   end
 
   def self.states_count(projects, group = nil)
-    relation = MilestonesFinder.new.execute(projects, state: 'all')
-    milestones_by_state_and_title = relation.reorder(nil).group(:state, :title).count
+    projects_milestones_count = legacy_group_milestone_states_count(projects)
+    group_milestones_count = group_milestones_states_count(group)
 
-    opened = count_by_state(milestones_by_state_and_title, 'active')
-    closed = count_by_state(milestones_by_state_and_title, 'closed')
-    all = milestones_by_state_and_title.map { |(_, title), _| title }.uniq.count
+    projects_milestones_count.merge(group_milestones_count) do |k, project_milestones_count, group_milestones_count|
+      project_milestones_count + group_milestones_count
+    end
+  end
+
+  def self.group_milestones_states_count(group)
+    return STATE_COUNT_HASH unless group
+
+    relation = GroupMilestonesFinder.new(group, state: 'all').execute
+    grouped_by_state = relation.reorder(nil).group(:state).count
+
+    {
+      opened: grouped_by_state['active'] || 0,
+      closed: grouped_by_state['closed'] || 0,
+      all: relation.count
+    }
+  end
+
+  def self.legacy_group_milestone_states_count(projects)
+    return STATE_COUNT_HASH unless projects
+
+    relation = ProjectMilestonesFinder.new(projects, state: 'all').execute
+    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,
diff --git a/app/models/group_milestone.rb b/app/models/group_milestone.rb
index 87c422f33db..1dce39f65c4 100644
--- a/app/models/group_milestone.rb
+++ b/app/models/group_milestone.rb
@@ -1,5 +1,5 @@
 class GroupMilestone < ActiveRecord::Base
-  include MilestoneModelProperties
+  include SharedMilestoneProperties
   include Milestoneish
   include CacheMarkdownField
 
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 73dd64ca784..40655c286d1 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -7,7 +7,7 @@ class Milestone < ActiveRecord::Base
   Upcoming = MilestoneStruct.new('Upcoming', '#upcoming', -2)
   Started = MilestoneStruct.new('Started', '#started', -3)
 
-  include MilestoneModelProperties
+  include SharedMilestoneProperties
   include InternalId
   include Sortable
   include Referable