diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1028dff3712842b5d6b346330d4c6412f67d33cf..5bcbd309f19d5494ab99abffae2ca3e87a8279d5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -36,6 +36,7 @@ Please view this file on the master branch, on stable branches it's out of date.
## 8.13.2
- Fix builds dropdown overlapping bug !7124
+ - Fix and improve `Sortable.highest_label_priority`
## 8.13.1 (2016-10-25)
- Fix branch protection API. !6215
diff --git a/app/models/concerns/sortable.rb b/app/models/concerns/sortable.rb
index 12b23f00769c525bd28bed9d089b781adf250532..7edb0acd56c75d9f48187e3aa6ee2339b835c3a3 100644
--- a/app/models/concerns/sortable.rb
+++ b/app/models/concerns/sortable.rb
@@ -38,16 +38,21 @@ module Sortable
private
- def highest_label_priority(target_type:, target_column:, project_column:, excluded_labels: [])
+ def highest_label_priority(target_type_column: nil, target_type: nil, target_column:, project_column:, excluded_labels: [])
query = Label.select(LabelPriority.arel_table[:priority].minimum).
left_join_priorities.
joins(:label_links).
where("label_priorities.project_id = #{project_column}").
- where(label_links: { target_type: target_type }).
where("label_links.target_id = #{target_column}").
reorder(nil)
- query.where.not(title: excluded_labels) if excluded_labels.present?
+ if target_type_column
+ query = query.where("label_links.target_type = #{target_type_column}")
+ else
+ query = query.where(label_links: { target_type: target_type })
+ end
+
+ query = query.where.not(title: excluded_labels) if excluded_labels.present?
query
end
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 11c072dd00080c1c8ff3d20a2e3b186fd0f9d8c1..f5ade1cc293cb0175ec157e6d29826d60d606261 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -53,7 +53,7 @@ class Todo < ActiveRecord::Base
# Need to order by created_at last because of differences on Mysql and Postgres when joining by type "Merge_request/Issue"
def order_by_labels_priority
params = {
- target_type: ['Issue', 'MergeRequest'],
+ target_type_column: "todos.target_type",
target_column: "todos.target_id",
project_column: "todos.project_id"
}
diff --git a/spec/features/todos/todos_sorting_spec.rb b/spec/features/todos/todos_sorting_spec.rb
index e74a51acede8b52e189aab15020a3533e27183d9..fec28c55d30e80fad4e52867f79097c6429827bb 100644
--- a/spec/features/todos/todos_sorting_spec.rb
+++ b/spec/features/todos/todos_sorting_spec.rb
@@ -8,60 +8,90 @@ describe "Dashboard > User sorts todos", feature: true do
let(:label_2) { create(:label, title: 'label_2', project: project, priority: 2) }
let(:label_3) { create(:label, title: 'label_3', project: project, priority: 3) }
- let(:issue_1) { create(:issue, title: 'issue_1', project: project) }
- let(:issue_2) { create(:issue, title: 'issue_2', project: project) }
- let(:issue_3) { create(:issue, title: 'issue_3', project: project) }
- let(:issue_4) { create(:issue, title: 'issue_4', project: project) }
-
- let!(:merge_request_1) { create(:merge_request, source_project: project, title: "merge_request_1") }
-
- before do
- create(:todo, user: user, project: project, target: issue_4, created_at: 5.hours.ago)
- create(:todo, user: user, project: project, target: issue_2, created_at: 4.hours.ago)
- create(:todo, user: user, project: project, target: issue_3, created_at: 3.hours.ago)
- create(:todo, user: user, project: project, target: issue_1, created_at: 2.hours.ago)
- create(:todo, user: user, project: project, target: merge_request_1, created_at: 1.hour.ago)
-
- merge_request_1.labels << label_1
- issue_3.labels << label_1
- issue_2.labels << label_3
- issue_1.labels << label_2
-
- project.team << [user, :developer]
- login_as(user)
- visit dashboard_todos_path
- end
+ before { project.team << [user, :developer] }
- it "sorts with oldest created todos first" do
- click_link "Last created"
+ context 'sort options' do
+ let(:issue_1) { create(:issue, title: 'issue_1', project: project) }
+ let(:issue_2) { create(:issue, title: 'issue_2', project: project) }
+ let(:issue_3) { create(:issue, title: 'issue_3', project: project) }
+ let(:issue_4) { create(:issue, title: 'issue_4', project: project) }
- results_list = page.find('.todos-list')
- expect(results_list.all('p')[0]).to have_content("merge_request_1")
- expect(results_list.all('p')[1]).to have_content("issue_1")
- expect(results_list.all('p')[2]).to have_content("issue_3")
- expect(results_list.all('p')[3]).to have_content("issue_2")
- expect(results_list.all('p')[4]).to have_content("issue_4")
- end
+ let!(:merge_request_1) { create(:merge_request, source_project: project, title: "merge_request_1") }
+
+ before do
+ create(:todo, user: user, project: project, target: issue_4, created_at: 5.hours.ago)
+ create(:todo, user: user, project: project, target: issue_2, created_at: 4.hours.ago)
+ create(:todo, user: user, project: project, target: issue_3, created_at: 3.hours.ago)
+ create(:todo, user: user, project: project, target: issue_1, created_at: 2.hours.ago)
+ create(:todo, user: user, project: project, target: merge_request_1, created_at: 1.hour.ago)
+
+ merge_request_1.labels << label_1
+ issue_3.labels << label_1
+ issue_2.labels << label_3
+ issue_1.labels << label_2
+
+ login_as(user)
+ visit dashboard_todos_path
+ end
+
+ it "sorts with oldest created todos first" do
+ click_link "Last created"
+
+ results_list = page.find('.todos-list')
+ expect(results_list.all('p')[0]).to have_content("merge_request_1")
+ expect(results_list.all('p')[1]).to have_content("issue_1")
+ expect(results_list.all('p')[2]).to have_content("issue_3")
+ expect(results_list.all('p')[3]).to have_content("issue_2")
+ expect(results_list.all('p')[4]).to have_content("issue_4")
+ end
- it "sorts with newest created todos first" do
- click_link "Oldest created"
+ it "sorts with newest created todos first" do
+ click_link "Oldest created"
- results_list = page.find('.todos-list')
- expect(results_list.all('p')[0]).to have_content("issue_4")
- expect(results_list.all('p')[1]).to have_content("issue_2")
- expect(results_list.all('p')[2]).to have_content("issue_3")
- expect(results_list.all('p')[3]).to have_content("issue_1")
- expect(results_list.all('p')[4]).to have_content("merge_request_1")
+ results_list = page.find('.todos-list')
+ expect(results_list.all('p')[0]).to have_content("issue_4")
+ expect(results_list.all('p')[1]).to have_content("issue_2")
+ expect(results_list.all('p')[2]).to have_content("issue_3")
+ expect(results_list.all('p')[3]).to have_content("issue_1")
+ expect(results_list.all('p')[4]).to have_content("merge_request_1")
+ end
+
+ it "sorts by priority" do
+ click_link "Priority"
+
+ results_list = page.find('.todos-list')
+ expect(results_list.all('p')[0]).to have_content("issue_3")
+ expect(results_list.all('p')[1]).to have_content("merge_request_1")
+ expect(results_list.all('p')[2]).to have_content("issue_1")
+ expect(results_list.all('p')[3]).to have_content("issue_2")
+ expect(results_list.all('p')[4]).to have_content("issue_4")
+ end
end
- it "sorts by priority" do
- click_link "Priority"
+ context 'issues and merge requests' do
+ let(:issue_1) { create(:issue, id: 10000, title: 'issue_1', project: project) }
+ let(:issue_2) { create(:issue, id: 10001, title: 'issue_2', project: project) }
+ let(:merge_request_1) { create(:merge_request, id: 10000, title: 'merge_request_1', source_project: project) }
+
+ before do
+ issue_1.labels << label_1
+ issue_2.labels << label_2
+
+ create(:todo, user: user, project: project, target: issue_1)
+ create(:todo, user: user, project: project, target: issue_2)
+ create(:todo, user: user, project: project, target: merge_request_1)
+
+ login_as(user)
+ visit dashboard_todos_path
+ end
+
+ it "doesn't mix issues and merge requests priorities" do
+ click_link "Priority"
- results_list = page.find('.todos-list')
- expect(results_list.all('p')[0]).to have_content("issue_3")
- expect(results_list.all('p')[1]).to have_content("merge_request_1")
- expect(results_list.all('p')[2]).to have_content("issue_1")
- expect(results_list.all('p')[3]).to have_content("issue_2")
- expect(results_list.all('p')[4]).to have_content("issue_4")
+ results_list = page.find('.todos-list')
+ expect(results_list.all('p')[0]).to have_content("issue_1")
+ expect(results_list.all('p')[1]).to have_content("issue_2")
+ expect(results_list.all('p')[2]).to have_content("merge_request_1")
+ end
end
end
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 60e4bbc85647025e44aafaf370a0f8315b8e37a3..a59d30687f6ecc915a0ac875433419d935f81687 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -298,6 +298,20 @@ describe Issue, "Issuable" do
end
end
+ describe '.order_labels_priority' do
+ let(:label_1) { create(:label, title: 'label_1', project: issue.project, priority: 1) }
+ let(:label_2) { create(:label, title: 'label_2', project: issue.project, priority: 2) }
+
+ subject { Issue.order_labels_priority(excluded_labels: ['label_1']).first.highest_priority }
+
+ before do
+ issue.labels << label_1
+ issue.labels << label_2
+ end
+
+ it { is_expected.to eq(2) }
+ end
+
describe ".with_label" do
let(:project) { create(:project, :public) }
let(:bug) { create(:label, project: project, title: 'bug') }