Improve method names and add more specs

11 files changed, 63 insertions, 35 deletions

diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb
index 0f4d4d841ba..01599ce49c6 100644
--- a/app/models/concerns/milestoneish.rb
+++ b/app/models/concerns/milestoneish.rb
@@ -1,7 +1,7 @@
 module Milestoneish
   def closed_items_count(user)
     memoize_per_user(user, :closed_items_count) do
-      (count_issues_by_state(user)['closed']&.length || 0) + merge_requests.closed_and_merged.size
+      (count_issues_by_state(user)['closed'] || 0) + merge_requests.closed_and_merged.size
     end
   end
 
@@ -12,7 +12,7 @@ module Milestoneish
   end
 
   def total_issues_count(user)
-    issues_visible_to_user(user).length
+    count_issues_by_state(user).values.sum
   end
 
   def complete?(user)
@@ -44,6 +44,14 @@ module Milestoneish
     end
   end
 
+  def sorted_issues(user)
+    issues_visible_to_user(user).preload_associations.sort('label_priority')
+  end
+
+  def sorted_merge_requests
+    merge_requests.sort('label_priority')
+  end
+
   def upcoming?
     start_date && start_date.future?
   end
@@ -62,17 +70,11 @@ module Milestoneish
     due_date && due_date.past?
   end
 
-  def sorted_merge_requests
-    merge_requests.sort('label_priority')
-  end
-
   private
 
   def count_issues_by_state(user)
     memoize_per_user(user, :count_issues_by_state) do
-      # Need to group and count using ruby array to not break
-      # label ordering. Also it saves a SQL query.
-      issues_visible_to_user(user).to_a.group_by(&:state)
+      issues_visible_to_user(user).reorder(nil).group(:state).count
     end
   end
 
@@ -85,6 +87,6 @@ module Milestoneish
   # override in a class that includes this module to get a faster query
   # from IssuesFinder
   def issues_finder_params
-    { sort: 'label_priority' }
+    {}
   end
 end
diff --git a/app/models/dashboard_milestone.rb b/app/models/dashboard_milestone.rb
index e2af4040870..646c1e5ce1a 100644
--- a/app/models/dashboard_milestone.rb
+++ b/app/models/dashboard_milestone.rb
@@ -1,5 +1,5 @@
 class DashboardMilestone < GlobalMilestone
   def issues_finder_params
-    super.merge({ authorized_only: true })
+    { authorized_only: true }
   end
 end
diff --git a/app/models/group_milestone.rb b/app/models/group_milestone.rb
index 4db20470220..86d38e5468b 100644
--- a/app/models/group_milestone.rb
+++ b/app/models/group_milestone.rb
@@ -14,6 +14,6 @@ class GroupMilestone < GlobalMilestone
   end
 
   def issues_finder_params
-    super.merge({ group_id: group.id })
+    { group_id: group.id }
   end
 end
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 14647602c02..0a6fc064aec 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -187,6 +187,6 @@ class Milestone < ActiveRecord::Base
   end
 
   def issues_finder_params
-    super.merge({ project_id: project_id })
+    { project_id: project_id }
   end
 end
diff --git a/app/serializers/issuable_entity.rb b/app/serializers/issuable_entity.rb
index 65b204d4dd2..bd5211b8e58 100644
--- a/app/serializers/issuable_entity.rb
+++ b/app/serializers/issuable_entity.rb
@@ -5,7 +5,6 @@ class IssuableEntity < Grape::Entity
   expose :description
   expose :lock_version
   expose :milestone_id
-  expose :position
   expose :state
   expose :title
   expose :updated_by_id
diff --git a/app/views/shared/milestones/_sidebar.html.haml b/app/views/shared/milestones/_sidebar.html.haml
index e75edd750ff..9bb87640319 100644
--- a/app/views/shared/milestones/_sidebar.html.haml
+++ b/app/views/shared/milestones/_sidebar.html.haml
@@ -68,10 +68,10 @@
         .sidebar-collapsed-icon
           %strong
             = icon('hashtag', 'aria-hidden': 'true')
-          %span= milestone.issues_visible_to_user(current_user).length
+          %span= milestone.issues_visible_to_user(current_user).count
         .title.hide-collapsed
           Issues
-          %span.badge= milestone.issues_visible_to_user(current_user).length
+          %span.badge= milestone.issues_visible_to_user(current_user).count
           - if project && can?(current_user, :create_issue, project)
             = link_to new_namespace_project_issue_path(project.namespace, project, issue: { milestone_id: milestone.id }), class: "pull-right", title: "New Issue" do
               New issue
@@ -79,11 +79,11 @@
           %span.milestone-stat
             = link_to milestones_browse_issuables_path(milestone, type: :issues) do
               Open:
-              = milestone.issues_visible_to_user(current_user).opened.length
+              = milestone.issues_visible_to_user(current_user).opened.count
           %span.milestone-stat
             = link_to milestones_browse_issuables_path(milestone, type: :issues, state: 'closed') do
               Closed:
-              = milestone.issues_visible_to_user(current_user).closed.length
+              = milestone.issues_visible_to_user(current_user).closed.count
 
     .block.merge-requests
       .sidebar-collapsed-icon
diff --git a/app/views/shared/milestones/_tabs.html.haml b/app/views/shared/milestones/_tabs.html.haml
index b1d24578daf..4de8a6cb15f 100644
--- a/app/views/shared/milestones/_tabs.html.haml
+++ b/app/views/shared/milestones/_tabs.html.haml
@@ -31,7 +31,7 @@
 .tab-content.milestone-content
   - if milestone.is_a?(GlobalMilestone) || can?(current_user, :read_issue, @project)
     .tab-pane.active#tab-issues{ data: { sort_endpoint: (sort_issues_namespace_project_milestone_path(@project.namespace, @project, @milestone) if @project && current_user) } }
-      = render 'shared/milestones/issues_tab', issues: milestone.issues_visible_to_user(current_user).preload_associations, show_project_name: show_project_name, show_full_project_name: show_full_project_name
+      = render 'shared/milestones/issues_tab', issues: milestone.sorted_issues(current_user), show_project_name: show_project_name, show_full_project_name: show_full_project_name
     .tab-pane#tab-merge-requests
       -# loaded async
       = render "shared/milestones/tab_loading"
diff --git a/app/views/shared/milestones/_top.html.haml b/app/views/shared/milestones/_top.html.haml
index 36c2969a555..2562f085338 100644
--- a/app/views/shared/milestones/_top.html.haml
+++ b/app/views/shared/milestones/_top.html.haml
@@ -50,7 +50,7 @@
           - project_name = group ? ms.project.name : ms.project.name_with_namespace
           = link_to project_name, namespace_project_milestone_path(ms.project.namespace, ms.project, ms)
         %td
-          = ms.issues_visible_to_user(current_user).opened.length
+          = ms.issues_visible_to_user(current_user).opened.count
         %td
           - if ms.closed?
             Closed
diff --git a/spec/models/concerns/milestoneish_spec.rb b/spec/models/concerns/milestoneish_spec.rb
index 7616123b486..cefe7fb6fea 100644
--- a/spec/models/concerns/milestoneish_spec.rb
+++ b/spec/models/concerns/milestoneish_spec.rb
@@ -28,13 +28,13 @@ describe Milestone, 'Milestoneish' do
     project.team << [guest, :guest]
   end
 
-  describe '#issues_visible_to_user' do
+  describe '#sorted_issues' do
     it 'sorts issues by label priority' do
       issue.labels << label_1
       security_issue_1.labels << label_2
       closed_issue_1.labels << label_3
 
-      issues = milestone.issues_visible_to_user(member)
+      issues = milestone.sorted_issues(member)
 
       expect(issues.first).to eq(issue)
       expect(issues.second).to eq(security_issue_1)
@@ -43,14 +43,10 @@ describe Milestone, 'Milestoneish' do
   end
 
   describe '#sorted_merge_requests' do
-    let(:merge_request_1) { create(:merge_request, :simple, source_project: project, source_branch: 'branch_1', milestone: milestone) }
-    let(:merge_request_2) { create(:merge_request, :simple, source_project: project, source_branch: 'branch_2', milestone: milestone) }
-    let(:merge_request_3) { create(:merge_request, :simple, source_project: project, source_branch: 'branch_3', milestone: milestone) }
-
     it 'sorts merge requests by label priority' do
-      merge_request_2.labels << label_1
-      merge_request_1.labels << label_2
-      merge_request_3.labels << label_3
+      merge_request_1 = create(:labeled_merge_request, labels: [label_2], source_project: project, source_branch: 'branch_1', milestone: milestone)
+      merge_request_2 = create(:labeled_merge_request, labels: [label_1], source_project: project, source_branch: 'branch_2', milestone: milestone)
+      merge_request_3 = create(:labeled_merge_request, labels: [label_3], source_project: project, source_branch: 'branch_3', milestone: milestone)
 
       merge_requests = milestone.sorted_merge_requests
 
diff --git a/spec/models/group_milestone_spec.rb b/spec/models/group_milestone_spec.rb
index b81d985b15c..916afb7aaf5 100644
--- a/spec/models/group_milestone_spec.rb
+++ b/spec/models/group_milestone_spec.rb
@@ -3,7 +3,6 @@ require 'spec_helper'
 describe GroupMilestone, models: true do
   let(:group) { create(:group) }
   let(:project) { create(:empty_project, group: group) }
-  let(:project_2) { create(:empty_project, group: group) }
   let(:project_milestone) do
     create(:milestone, title: "Milestone v1.2", project: project)
   end
diff --git a/spec/requests/api/milestones_spec.rb b/spec/requests/api/milestones_spec.rb
index 40934c25afc..ab5ea3e8f2c 100644
--- a/spec/requests/api/milestones_spec.rb
+++ b/spec/requests/api/milestones_spec.rb
@@ -5,6 +5,9 @@ describe API::Milestones do
   let!(:project) { create(:empty_project, namespace: user.namespace ) }
   let!(:closed_milestone) { create(:closed_milestone, project: project, title: 'version1', description: 'closed milestone') }
   let!(:milestone) { create(:milestone, project: project, title: 'version2', description: 'open milestone') }
+  let(:label_1) { create(:label, title: 'label_1', project: project, priority: 1) }
+  let(:label_2) { create(:label, title: 'label_2', project: project, priority: 2) }
+  let(:label_3) { create(:label, title: 'label_3', project: project) }
 
   before do
     project.team << [user, :developer]
@@ -228,6 +231,18 @@ describe API::Milestones do
       expect(json_response.first['milestone']['title']).to eq(milestone.title)
     end
 
+    it 'returns project issues sorted by label priority' do
+      issue_1 = create(:labeled_issue, project: project, milestone: milestone, labels: [label_3])
+      issue_2 = create(:labeled_issue, project: project, milestone: milestone, labels: [label_1])
+      issue_3 = create(:labeled_issue, project: project, milestone: milestone, labels: [label_2])
+
+      get api("/projects/#{project.id}/milestones/#{milestone.id}/issues", user)
+
+      expect(json_response.first['id']).to eq(issue_2.id)
+      expect(json_response.second['id']).to eq(issue_3.id)
+      expect(json_response.third['id']).to eq(issue_1.id)
+    end
+
     it 'matches V4 response schema for a list of issues' do
       get api("/projects/#{project.id}/milestones/#{milestone.id}/issues", user)
 
@@ -244,8 +259,8 @@ describe API::Milestones do
     describe 'confidential issues' do
       let(:public_project) { create(:empty_project, :public) }
       let(:milestone) { create(:milestone, project: public_project) }
-      let(:issue) { create(:issue, project: public_project, position: 2) }
-      let(:confidential_issue) { create(:issue, confidential: true, project: public_project, position: 1) }
+      let(:issue) { create(:issue, project: public_project) }
+      let(:confidential_issue) { create(:issue, confidential: true, project: public_project) }
 
       before do
         public_project.team << [user, :developer]
@@ -285,7 +300,10 @@ describe API::Milestones do
         expect(json_response.map { |issue| issue['id'] }).to include(issue.id)
       end
 
-      it 'returns issues ordered by position asc' do
+      it 'returns issues ordered by label priority' do
+        issue.labels << label_2
+        confidential_issue.labels << label_1
+
         get api("/projects/#{public_project.id}/milestones/#{milestone.id}/issues", user)
 
         expect(response).to have_http_status(200)
@@ -299,8 +317,8 @@ describe API::Milestones do
   end
 
   describe 'GET /projects/:id/milestones/:milestone_id/merge_requests' do
-    let(:merge_request) { create(:merge_request, source_project: project, position: 2) }
-    let(:another_merge_request) { create(:merge_request, :simple, source_project: project, position: 1) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+    let(:another_merge_request) { create(:merge_request, :simple, source_project: project) }
 
     before do
       milestone.merge_requests << merge_request
@@ -318,6 +336,18 @@ describe API::Milestones do
       expect(json_response.first['milestone']['title']).to eq(milestone.title)
     end
 
+    it 'returns project merge_requests sorted by label priority' do
+      merge_request_1 = create(:labeled_merge_request, source_branch: 'branch_1', source_project: project, milestone: milestone, labels: [label_2])
+      merge_request_2 = create(:labeled_merge_request, source_branch: 'branch_2', source_project: project, milestone: milestone, labels: [label_1])
+      merge_request_3 = create(:labeled_merge_request, source_branch: 'branch_3', source_project: project, milestone: milestone, labels: [label_3])
+
+      get api("/projects/#{project.id}/milestones/#{milestone.id}/merge_requests", user)
+
+      expect(json_response.first['id']).to eq(merge_request_2.id)
+      expect(json_response.second['id']).to eq(merge_request_1.id)
+      expect(json_response.third['id']).to eq(merge_request_3.id)
+    end
+
     it 'returns a 404 error if milestone id not found' do
       get api("/projects/#{project.id}/milestones/1234/merge_requests", user)
 
@@ -339,6 +369,8 @@ describe API::Milestones do
 
     it 'returns merge_requests ordered by position asc' do
       milestone.merge_requests << another_merge_request
+      another_merge_request.labels << label_1
+      merge_request.labels << label_2
 
       get api("/projects/#{project.id}/milestones/#{milestone.id}/merge_requests", user)