Merge branch 'milestones-finder-order-fix' into 'security-10-3'

Remove order param from the MilestoneFinder

See merge request gitlab/gitlabhq!2259

(cherry picked from commit 14408042e78f2ebc2644f956621b461dbfa3d36d)

155881e7 Remove order param from the MilestoneFinder

4 files changed, 18 insertions, 18 deletions

diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb
index f013d21275e..acf6aaf57f4 100644
--- a/app/controllers/groups/milestones_controller.rb
+++ b/app/controllers/groups/milestones_controller.rb
@@ -75,8 +75,6 @@ class Groups::MilestonesController < Groups::ApplicationController
   end
 
   def milestones
-    search_params = params.merge(group_ids: group.id)
-
     milestones = MilestonesFinder.new(search_params).execute
     legacy_milestones = GroupMilestone.build_collection(group, group_projects, params)
 
@@ -94,4 +92,8 @@ class Groups::MilestonesController < Groups::ApplicationController
 
     render_404 unless @milestone
   end
+
+  def search_params
+    params.permit(:state).merge(group_ids: group.id)
+  end
 end
diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb
index 980bbf699b6..0f70efbce40 100644
--- a/app/controllers/projects/milestones_controller.rb
+++ b/app/controllers/projects/milestones_controller.rb
@@ -92,12 +92,6 @@ class Projects::MilestonesController < Projects::ApplicationController
 
   def milestones
     @milestones ||= begin
-      if @project.group && can?(current_user, :read_group, @project.group)
-        group = @project.group
-      end
-
-      search_params = params.merge(project_ids: @project.id, group_ids: group&.id)
-
       MilestonesFinder.new(search_params).execute
     end
   end
@@ -113,4 +107,12 @@ class Projects::MilestonesController < Projects::ApplicationController
   def milestone_params
     params.require(:milestone).permit(:title, :description, :start_date, :due_date, :state_event)
   end
+
+  def search_params
+    if @project.group && can?(current_user, :read_group, @project.group)
+      group = @project.group
+    end
+
+    params.permit(:state).merge(project_ids: @project.id, group_ids: group&.id)
+  end
 end
diff --git a/app/finders/milestones_finder.rb b/app/finders/milestones_finder.rb
index 0a5a0ea2f35..b4605fca193 100644
--- a/app/finders/milestones_finder.rb
+++ b/app/finders/milestones_finder.rb
@@ -46,11 +46,7 @@ class MilestonesFinder
   end
 
   def order(items)
-    if params.has_key?(:order)
-      items.reorder(params[:order])
-    else
-      order_statement = Gitlab::Database.nulls_last_order('due_date', 'ASC')
-      items.reorder(order_statement)
-    end
+    order_statement = Gitlab::Database.nulls_last_order('due_date', 'ASC')
+    items.reorder(order_statement).order('title ASC')
   end
 end
diff --git a/app/models/global_milestone.rb b/app/models/global_milestone.rb
index c0864769314..dc2f6817190 100644
--- a/app/models/global_milestone.rb
+++ b/app/models/global_milestone.rb
@@ -44,10 +44,10 @@ class GlobalMilestone
   def self.group_milestones_states_count(group)
     return STATE_COUNT_HASH unless group
 
-    params = { group_ids: [group.id], state: 'all', order: nil }
+    params = { group_ids: [group.id], state: 'all' }
 
     relation = MilestonesFinder.new(params).execute
-    grouped_by_state = relation.group(:state).count
+    grouped_by_state = relation.reorder(nil).group(:state).count
 
     {
       opened: grouped_by_state['active'] || 0,
@@ -60,10 +60,10 @@ class GlobalMilestone
   def self.legacy_group_milestone_states_count(projects)
     return STATE_COUNT_HASH unless projects
 
-    params = { project_ids: projects.map(&:id), state: 'all', order: nil }
+    params = { project_ids: projects.map(&:id), state: 'all' }
 
     relation = MilestonesFinder.new(params).execute
-    project_milestones_by_state_and_title = relation.group(:state, :title).count
+    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')