Refactor global and group milestones logic

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

19 files changed, 145 insertions, 165 deletions

diff --git a/app/controllers/concerns/global_milestones.rb b/app/controllers/concerns/global_milestones.rb
new file mode 100644
index 00000000000..b428249acd3
--- /dev/null
+++ b/app/controllers/concerns/global_milestones.rb
@@ -0,0 +1,19 @@
+module GlobalMilestones
+  extend ActiveSupport::Concern
+
+  def milestones
+    @milestones = MilestonesFinder.new.execute(@projects, params)
+    @milestones = GlobalMilestone.build_collection(@milestones)
+    @milestones = Kaminari.paginate_array(@milestones).page(params[:page]).per(ApplicationController::PER_PAGE)
+  end
+
+  def milestone
+    milestones = Milestone.of_projects(@projects).where(title: params[:title])
+
+    if milestones.present?
+      @milestone = GlobalMilestone.new(params[:title], milestones)
+    else
+      render_404
+    end
+  end
+end
diff --git a/app/controllers/dashboard/milestones_controller.rb b/app/controllers/dashboard/milestones_controller.rb
index 53896d4f2c7..2bdce0f8a00 100644
--- a/app/controllers/dashboard/milestones_controller.rb
+++ b/app/controllers/dashboard/milestones_controller.rb
@@ -1,34 +1,19 @@
 class Dashboard::MilestonesController < Dashboard::ApplicationController
-  before_action :load_projects
+  include GlobalMilestones
+
+  before_action :projects
+  before_action :milestones, only: [:index]
+  before_action :milestone, only: [:show]
 
   def index
-    project_milestones = case params[:state]
-                         when 'all'; state
-                         when 'closed'; state('closed')
-                         else state('active')
-                         end
-    @dashboard_milestones = Milestones::GroupService.new(project_milestones).execute
-    @dashboard_milestones = Kaminari.paginate_array(@dashboard_milestones).page(params[:page]).per(PER_PAGE)
   end
 
   def show
-    project_milestones = Milestone.where(project_id: @projects).order("due_date ASC")
-    @dashboard_milestone = Milestones::GroupService.new(project_milestones).milestone(title)
   end
 
   private
 
-  def load_projects
-    @projects = current_user.authorized_projects.sorted_by_activity.non_archived
-  end
-
-  def title
-    params[:title]
-  end
-
-  def state(state = nil)
-    conditions = { project_id: @projects }
-    conditions.reverse_merge!(state: state) if state
-    Milestone.where(conditions).order("title ASC")
+  def projects
+    @projects ||= current_user.authorized_projects.sorted_by_activity.non_archived
   end
 end
diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb
index 6878d4bc07e..be801858eaf 100644
--- a/app/controllers/groups/application_controller.rb
+++ b/app/controllers/groups/application_controller.rb
@@ -1,8 +1,13 @@
 class Groups::ApplicationController < ApplicationController
   layout 'group'
+  before_action :group
 
   private
-  
+
+  def group
+    @group ||= Group.find_by(path: params[:group_id])
+  end
+
   def authorize_read_group!
     unless @group and can?(current_user, :read_group, @group)
       if current_user.nil?
@@ -12,13 +17,13 @@ class Groups::ApplicationController < ApplicationController
       end
     end
   end
-  
+
   def authorize_admin_group!
     unless can?(current_user, :admin_group, group)
       return render_404
     end
   end
-  
+
   def authorize_admin_group_member!
     unless can?(current_user, :admin_group_member, group)
       return render_403
diff --git a/app/controllers/groups/avatars_controller.rb b/app/controllers/groups/avatars_controller.rb
index 6aa64222f77..f390705dc6a 100644
--- a/app/controllers/groups/avatars_controller.rb
+++ b/app/controllers/groups/avatars_controller.rb
@@ -1,8 +1,6 @@
 class Groups::AvatarsController < ApplicationController
   def destroy
-    @group = Group.find_by(path: params[:group_id])
     @group.remove_avatar!
-
     @group.save
 
     redirect_to edit_group_path(@group)
diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb
index 91518c44a98..b25957a06e2 100644
--- a/app/controllers/groups/group_members_controller.rb
+++ b/app/controllers/groups/group_members_controller.rb
@@ -1,6 +1,5 @@
 class Groups::GroupMembersController < Groups::ApplicationController
   skip_before_action :authenticate_user!, only: [:index]
-  before_action :group
 
   # Authorize
   before_action :authorize_read_group!
@@ -80,10 +79,6 @@ class Groups::GroupMembersController < Groups::ApplicationController
 
   protected
 
-  def group
-    @group ||= Group.find_by(path: params[:group_id])
-  end
-
   def member_params
     params.require(:group_member).permit(:access_level, :user_id)
   end
diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb
index 8779376d93c..6833a550c9e 100644
--- a/app/controllers/groups/milestones_controller.rb
+++ b/app/controllers/groups/milestones_controller.rb
@@ -1,21 +1,16 @@
 class Groups::MilestonesController < Groups::ApplicationController
-  before_action :authorize_group_milestone!, only: :update
-  before_action :group
+  include GlobalMilestones
 
-  def index
-    project_milestones =
-      case params[:state]
-      when 'all'; state
-      when 'closed'; state('closed')
-      else state('active')
-      end
+  before_action :projects
+  before_action :milestones, only: [:index]
+  before_action :milestone, only: [:show, :update]
+  before_action :authorize_group_milestone!, only: [:create, :update]
 
-    @group_milestones = Milestones::GroupService.new(project_milestones).execute
-    @group_milestones = Kaminari.paginate_array(@group_milestones).page(params[:page]).per(PER_PAGE)
+  def index
   end
 
   def new
-    @group_milestone = OpenStruct.new(title: nil, description: nil)
+    @milestone = Milestone.new
   end
 
   def create
@@ -26,55 +21,35 @@ class Groups::MilestonesController < Groups::ApplicationController
       Milestones::CreateService.new(project, current_user, milestone_params).execute
     end
 
-    redirect_to group_milestone_path(@group, title.parameterize, title: title)
+    redirect_to milestone_path(title)
   end
 
   def show
-    project_milestones = Milestone.where(project_id: group.projects).order("due_date ASC")
-    @group_milestone = Milestones::GroupService.new(project_milestones).milestone(title)
   end
 
   def update
-    project_milestones = Milestone.where(project_id: group.projects).order("due_date ASC")
-    @group_milestones = Milestones::GroupService.new(project_milestones).milestone(title)
-
-    @group_milestones.milestones.each do |milestone|
-      Milestones::UpdateService.new(milestone.project, current_user, params[:milestone]).execute(milestone)
+    @milestone.milestones.each do |milestone|
+      Milestones::UpdateService.new(milestone.project, current_user, milestone_params).execute(milestone)
     end
 
-    respond_to do |format|
-      format.js
-      format.html do
-        redirect_to group_milestones_path(group)
-      end
-    end
+    redirect_back_or_default(default: milestone_path(@milestone.title))
   end
 
   private
 
-  def group
-    @group ||= Group.find_by(path: params[:group_id])
-  end
-
-  def title
-    params[:title]
+  def authorize_group_milestone!
+    return render_404 unless can?(current_user, :admin_group, group)
   end
 
-  def state(state = nil)
-    conditions = { project_id: group.projects }
-    conditions.reverse_merge!(state: state) if state
-    Milestone.where(conditions).order("title ASC")
+  def milestone_params
+    params.require(:milestone).permit(:title, :description, :due_date, :state_event)
   end
 
-  def authorize_group_milestone!
-    return render_404 unless can?(current_user, :admin_group, group)
+  def milestone_path(title)
+    group_milestone_path(@group, title.parameterize, title: title)
   end
 
-  def milestone_params
-    params.require(:milestone).permit(
-      :title,
-      :description,
-      :due_date
-    )
+  def projects
+    @projects ||= @group.projects
   end
 end
diff --git a/app/finders/milestones_finder.rb b/app/finders/milestones_finder.rb
new file mode 100644
index 00000000000..71f207ca030
--- /dev/null
+++ b/app/finders/milestones_finder.rb
@@ -0,0 +1,13 @@
+class MilestonesFinder
+  def execute(projects, params)
+    milestones = Milestone.of_projects(projects)
+    milestones = milestones.order("due_date ASC")
+
+    case params[:state]
+    when 'closed' then milestones.closed
+    when 'all' then milestones
+    else milestones.active
+    end
+  end
+end
+
diff --git a/app/helpers/labels_helper.rb b/app/helpers/labels_helper.rb
index ee04ace35d0..795fb439f25 100644
--- a/app/helpers/labels_helper.rb
+++ b/app/helpers/labels_helper.rb
@@ -100,7 +100,7 @@ module LabelsHelper
         Label.where(project_id: @projects)
       end
 
-    grouped_labels = Labels::GroupService.new(labels).execute
+    grouped_labels = GlobalLabel.build_collection(labels)
     grouped_labels.unshift(Label::None)
     grouped_labels.unshift(Label::Any)
 
diff --git a/app/helpers/milestones_helper.rb b/app/helpers/milestones_helper.rb
index 37a5b58cce8..ad43892b639 100644
--- a/app/helpers/milestones_helper.rb
+++ b/app/helpers/milestones_helper.rb
@@ -28,7 +28,7 @@ module MilestonesHelper
         Milestone.where(project_id: @projects)
       end.active
 
-    grouped_milestones = Milestones::GroupService.new(milestones).execute
+    grouped_milestones = GlobalMilestone.build_collection(milestones)
     grouped_milestones.unshift(Milestone::None)
     grouped_milestones.unshift(Milestone::Any)
 
diff --git a/app/models/global_label.rb b/app/models/global_label.rb
new file mode 100644
index 00000000000..0171f7d54b7
--- /dev/null
+++ b/app/models/global_label.rb
@@ -0,0 +1,17 @@
+class GlobalLabel
+  attr_accessor :title, :labels
+  alias_attribute :name, :title
+
+  def self.build_collection(labels)
+    labels = labels.group_by(&:title)
+
+    labels.map do |title, label|
+      new(title, label)
+    end
+  end
+
+  def initialize(title, labels)
+    @title = title
+    @labels = labels
+  end
+end
diff --git a/app/models/group_milestone.rb b/app/models/global_milestone.rb
index 91844da62e2..f96e9d41c94 100644
--- a/app/models/group_milestone.rb
+++ b/app/models/global_milestone.rb
@@ -1,7 +1,15 @@
-class GroupMilestone
+class GlobalMilestone
   attr_accessor :title, :milestones
   alias_attribute :name, :title
 
+  def self.build_collection(milestones)
+    milestones = milestones.group_by(&:title)
+
+    milestones.map do |title, milestones|
+      new(title, milestones)
+    end
+  end
+
   def initialize(title, milestones)
     @title = title
     @milestones = milestones
@@ -10,7 +18,7 @@ class GroupMilestone
   def safe_title
     @title.parameterize
   end
-  
+
   def projects
     milestones.map { |milestone| milestone.project }
   end
@@ -60,15 +68,15 @@ class GroupMilestone
   end
 
   def issues
-    @group_issues ||= milestones.map(&:issues).flatten.group_by(&:state)
+    @issues ||= milestones.map(&:issues).flatten.group_by(&:state)
   end
 
   def merge_requests
-    @group_merge_requests ||= milestones.map(&:merge_requests).flatten.group_by(&:state)
+    @merge_requests ||= milestones.map(&:merge_requests).flatten.group_by(&:state)
   end
 
   def participants
-    @group_participants ||= milestones.map(&:participants).flatten.compact.uniq
+    @participants ||= milestones.map(&:participants).flatten.compact.uniq
   end
 
   def opened_issues
diff --git a/app/models/group_label.rb b/app/models/group_label.rb
deleted file mode 100644
index 0fc39cb8771..00000000000
--- a/app/models/group_label.rb
+++ /dev/null
@@ -1,9 +0,0 @@
-class GroupLabel
-  attr_accessor :title, :labels
-  alias_attribute :name, :title
-
-  def initialize(title, labels)
-    @title = title
-    @labels = labels
-  end
-end
diff --git a/app/services/labels/group_service.rb b/app/services/labels/group_service.rb
deleted file mode 100644
index b26cee24d56..00000000000
--- a/app/services/labels/group_service.rb
+++ /dev/null
@@ -1,26 +0,0 @@
-module Labels
-  class GroupService < ::BaseService
-    def initialize(project_labels)
-      @project_labels = project_labels.group_by(&:title)
-    end
-
-    def execute
-      build(@project_labels)
-    end
-
-    def label(title)
-      if title
-        group_label = @project_labels[title].group_by(&:title)
-        build(group_label).first
-      else
-        nil
-      end
-    end
-
-    private
-
-    def build(label)
-      label.map { |title, labels| GroupLabel.new(title, labels) }
-    end
-  end
-end
diff --git a/app/services/milestones/group_service.rb b/app/services/milestones/collection_service.rb
index 11d702f1e7b..2eefec99e7b 100644
--- a/app/services/milestones/group_service.rb
+++ b/app/services/milestones/collection_service.rb
@@ -1,5 +1,5 @@
 module Milestones
-  class GroupService < Milestones::BaseService
+  class CollectionService < Milestones::BaseService
     def initialize(project_milestones)
       @project_milestones = project_milestones.group_by(&:title)
     end
diff --git a/app/views/dashboard/milestones/index.html.haml b/app/views/dashboard/milestones/index.html.haml
index 21b25c3986e..635251e2374 100644
--- a/app/views/dashboard/milestones/index.html.haml
+++ b/app/views/dashboard/milestones/index.html.haml
@@ -10,10 +10,10 @@
 
 .milestones
   %ul.content-list
-    - if @dashboard_milestones.blank?
+    - if @milestones.blank?
       %li
         .nothing-here-block No milestones to show
     - else
-      - @dashboard_milestones.each do |milestone|
+      - @milestones.each do |milestone|
         = render 'milestone', milestone: milestone
-  = paginate @dashboard_milestones, theme: "gitlab"
+  = paginate @milestones, theme: "gitlab"
diff --git a/app/views/dashboard/milestones/show.html.haml b/app/views/dashboard/milestones/show.html.haml
index 2fe14c6388c..580db613ed4 100644
--- a/app/views/dashboard/milestones/show.html.haml
+++ b/app/views/dashboard/milestones/show.html.haml
@@ -1,14 +1,14 @@
-- page_title @dashboard_milestone.title, "Milestones"
+- page_title @milestone.title, "Milestones"
 %h4.page-title
-  .issue-box{ class: "issue-box-#{@dashboard_milestone.closed? ? 'closed' : 'open'}" }
-    - if @dashboard_milestone.closed?
+  .issue-box{ class: "issue-box-#{@milestone.closed? ? 'closed' : 'open'}" }
+    - if @milestone.closed?
       Closed
     - else
       Open
-  Milestone #{@dashboard_milestone.title}
+  Milestone #{@milestone.title}
 
 %hr
-- if (@dashboard_milestone.total_items_count == @dashboard_milestone.closed_items_count) && @dashboard_milestone.active?
+- if (@milestone.total_items_count == @milestone.closed_items_count) && @milestone.active?
   .alert.alert-success
     %span All issues for this milestone are closed. You may close the milestone now.
 
@@ -22,7 +22,7 @@
         %th Open issues
         %th State
         %th Due date
-    - @dashboard_milestone.milestones.each do |milestone|
+    - @milestone.milestones.each do |milestone|
       %tr
         %td
           = link_to "#{milestone.project.name_with_namespace}", namespace_project_milestone_path(milestone.project.namespace, milestone.project, milestone)
@@ -39,46 +39,46 @@
 .context
   %p.lead
     Progress:
-    #{@dashboard_milestone.closed_items_count} closed
+    #{@milestone.closed_items_count} closed
     &ndash;
-    #{@dashboard_milestone.open_items_count} open
-  = milestone_progress_bar(@dashboard_milestone)
+    #{@milestone.open_items_count} open
+  = milestone_progress_bar(@milestone)
 
 %ul.nav.nav-tabs
   %li.active
     = link_to '#tab-issues', 'data-toggle' => 'tab' do
       Issues
-      %span.badge= @dashboard_milestone.issue_count
+      %span.badge= @milestone.issue_count
   %li
     = link_to '#tab-merge-requests', 'data-toggle' => 'tab' do
       Merge Requests
-      %span.badge= @dashboard_milestone.merge_requests_count
+      %span.badge= @milestone.merge_requests_count
   %li
     = link_to '#tab-participants', 'data-toggle' => 'tab' do
       Participants
-      %span.badge= @dashboard_milestone.participants.count
+      %span.badge= @milestone.participants.count
 
   .pull-right
-    = link_to 'Browse Issues', issues_dashboard_path(milestone_title: @dashboard_milestone.title), class: "btn  edit-milestone-link btn-grouped"
+    = link_to 'Browse Issues', issues_dashboard_path(milestone_title: @milestone.title), class: "btn  edit-milestone-link btn-grouped"
 
 .tab-content
   .tab-pane.active#tab-issues
     .row
       .col-md-6
-        = render 'issues', title: "Open", issues: @dashboard_milestone.opened_issues
+        = render 'issues', title: "Open", issues: @milestone.opened_issues
       .col-md-6
-        = render 'issues', title: "Closed", issues: @dashboard_milestone.closed_issues
+        = render 'issues', title: "Closed", issues: @milestone.closed_issues
 
   .tab-pane#tab-merge-requests
     .row
       .col-md-6
-        = render 'merge_requests', title: "Open", merge_requests: @dashboard_milestone.opened_merge_requests
+        = render 'merge_requests', title: "Open", merge_requests: @milestone.opened_merge_requests
       .col-md-6
-        = render 'merge_requests', title: "Closed", merge_requests: @dashboard_milestone.closed_merge_requests
+        = render 'merge_requests', title: "Closed", merge_requests: @milestone.closed_merge_requests
 
   .tab-pane#tab-participants
     %ul.bordered-list
-      - @dashboard_milestone.participants.each do |user|
+      - @milestone.participants.each do |user|
         %li
           = link_to user, title: user.name, class: "darken" do
             = image_tag avatar_icon(user, 32), class: "avatar s32"
diff --git a/app/views/groups/milestones/index.html.haml b/app/views/groups/milestones/index.html.haml
index ded4f3713f6..ffd7dd3fc0b 100644
--- a/app/views/groups/milestones/index.html.haml
+++ b/app/views/groups/milestones/index.html.haml
@@ -14,10 +14,10 @@
     group are listed here.
 .milestones
   %ul.content-list
-    - if @group_milestones.blank?
+    - if @milestones.blank?
       %li
         .nothing-here-block No milestones to show
     - else
-      - @group_milestones.each do |milestone|
+      - @milestones.each do |milestone|
         = render 'milestone', milestone: milestone
-  = paginate @group_milestones, theme: "gitlab"
+  = paginate @milestones, theme: "gitlab"
diff --git a/app/views/groups/milestones/new.html.haml b/app/views/groups/milestones/new.html.haml
index 287f89a7074..4c490d8ccb3 100644
--- a/app/views/groups/milestones/new.html.haml
+++ b/app/views/groups/milestones/new.html.haml
@@ -5,7 +5,7 @@
   This will create milestone in every selected project
 %hr
 
-= form_for @group_milestone, as: :milestone, url: group_milestones_path(@group), html: { class: 'form-horizontal milestone-form gfm-form js-requires-input' }  do |f|
+= form_for @milestone, url: group_milestones_path(@group), html: { class: 'form-horizontal milestone-form gfm-form js-requires-input' }  do |f|
   .row
     .col-md-6
       .form-group
diff --git a/app/views/groups/milestones/show.html.haml b/app/views/groups/milestones/show.html.haml
index a92ad5d751b..e609abca08e 100644
--- a/app/views/groups/milestones/show.html.haml
+++ b/app/views/groups/milestones/show.html.haml
@@ -1,22 +1,22 @@
-- page_title @group_milestone.title, "Milestones"
+- page_title @milestone.title, "Milestones"
 = render "header_title"
 
 %h4.page-title
-  .issue-box{ class: "issue-box-#{@group_milestone.closed? ? 'closed' : 'open'}" }
-    - if @group_milestone.closed?
+  .issue-box{ class: "issue-box-#{@milestone.closed? ? 'closed' : 'open'}" }
+    - if @milestone.closed?
       Closed
     - else
       Open
-  Milestone #{@group_milestone.title}
+  Milestone #{@milestone.title}
   .pull-right
     - if can?(current_user, :admin_group, @group)
-      - if @group_milestone.active?
-        = link_to 'Close Milestone', group_milestone_path(@group, @group_milestone.safe_title, title: @group_milestone.title, milestone: {state_event: :close }), method: :put, class: "btn btn-sm btn-close"
+      - if @milestone.active?
+        = link_to 'Close Milestone', group_milestone_path(@group, @milestone.safe_title, title: @milestone.title, milestone: {state_event: :close }), method: :put, class: "btn btn-sm btn-close"
       - else
-        = link_to 'Reopen Milestone', group_milestone_path(@group, @group_milestone.safe_title, title: @group_milestone.title, milestone: {state_event: :activate }), method: :put, class: "btn btn-sm btn-grouped btn-reopen"
+        = link_to 'Reopen Milestone', group_milestone_path(@group, @milestone.safe_title, title: @milestone.title, milestone: {state_event: :activate }), method: :put, class: "btn btn-sm btn-grouped btn-reopen"
 
 %hr
-- if (@group_milestone.total_items_count == @group_milestone.closed_items_count) && @group_milestone.active?
+- if (@milestone.total_items_count == @milestone.closed_items_count) && @milestone.active?
   .alert.alert-success
     %span All issues for this milestone are closed. You may close the milestone now.
 
@@ -30,7 +30,7 @@
         %th Open issues
         %th State
         %th Due date
-    - @group_milestone.milestones.each do |milestone|
+    - @milestone.milestones.each do |milestone|
       %tr
         %td
           = link_to "#{milestone.project.name}", namespace_project_milestone_path(milestone.project.namespace, milestone.project, milestone)
@@ -47,46 +47,46 @@
 .context
   %p.lead
     Progress:
-    #{@group_milestone.closed_items_count} closed
+    #{@milestone.closed_items_count} closed
     &ndash;
-    #{@group_milestone.open_items_count} open
-  = milestone_progress_bar(@group_milestone)
+    #{@milestone.open_items_count} open
+  = milestone_progress_bar(@milestone)
 
 %ul.nav.nav-tabs
   %li.active
     = link_to '#tab-issues', 'data-toggle' => 'tab' do
       Issues
-      %span.badge= @group_milestone.issue_count
+      %span.badge= @milestone.issue_count
   %li
     = link_to '#tab-merge-requests', 'data-toggle' => 'tab' do
       Merge Requests
-      %span.badge= @group_milestone.merge_requests_count
+      %span.badge= @milestone.merge_requests_count
   %li
     = link_to '#tab-participants', 'data-toggle' => 'tab' do
       Participants
-      %span.badge= @group_milestone.participants.count
+      %span.badge= @milestone.participants.count
 
   .pull-right
-    = link_to 'Browse Issues', issues_group_path(@group, milestone_title: @group_milestone.title), class: "btn  edit-milestone-link btn-grouped"
+    = link_to 'Browse Issues', issues_group_path(@group, milestone_title: @milestone.title), class: "btn  edit-milestone-link btn-grouped"
 
 .tab-content
   .tab-pane.active#tab-issues
     .row
       .col-md-6
-        = render 'issues', title: "Open", issues: @group_milestone.opened_issues
+        = render 'issues', title: "Open", issues: @milestone.opened_issues
       .col-md-6
-        = render 'issues', title: "Closed", issues: @group_milestone.closed_issues
+        = render 'issues', title: "Closed", issues: @milestone.closed_issues
 
   .tab-pane#tab-merge-requests
     .row
       .col-md-6
-        = render 'merge_requests', title: "Open", merge_requests: @group_milestone.opened_merge_requests
+        = render 'merge_requests', title: "Open", merge_requests: @milestone.opened_merge_requests
       .col-md-6
-        = render 'merge_requests', title: "Closed", merge_requests: @group_milestone.closed_merge_requests
+        = render 'merge_requests', title: "Closed", merge_requests: @milestone.closed_merge_requests
 
   .tab-pane#tab-participants
     %ul.bordered-list
-      - @group_milestone.participants.each do |user|
+      - @milestone.participants.each do |user|
         %li
           = link_to user, title: user.name, class: "darken" do
             = image_tag avatar_icon(user, 32), class: "avatar s32"