From 0520ee44985528d3076df1208bda7c6c7ff8ec79 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Fri, 9 Jun 2017 15:53:32 -0300 Subject: Improve method names and add more specs --- app/models/concerns/milestoneish.rb | 22 ++++++++------ app/models/dashboard_milestone.rb | 2 +- app/models/group_milestone.rb | 2 +- app/models/milestone.rb | 2 +- app/serializers/issuable_entity.rb | 1 - app/views/shared/milestones/_sidebar.html.haml | 8 ++--- app/views/shared/milestones/_tabs.html.haml | 2 +- app/views/shared/milestones/_top.html.haml | 2 +- spec/models/concerns/milestoneish_spec.rb | 14 +++------ spec/models/group_milestone_spec.rb | 1 - spec/requests/api/milestones_spec.rb | 42 +++++++++++++++++++++++--- 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) -- cgit v1.2.1