Fix sorting by label priorities

6 files changed, 47 insertions, 16 deletions

diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 76de927ceab..d726cb6b7aa 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -146,7 +146,8 @@ module Issuable
 
     def order_labels_priority(excluded_labels: [])
       condition_field = "#{table_name}.id"
-      highest_priority = highest_label_priority(name, condition_field, excluded_labels: excluded_labels).to_sql
+      project_field = "#{table_name}.project_id"
+      highest_priority = highest_label_priority(name, project_field, condition_field, excluded_labels: excluded_labels).to_sql
 
       select("#{table_name}.*, (#{highest_priority}) AS highest_priority").
         group(arel_table[:id]).
diff --git a/app/models/concerns/sortable.rb b/app/models/concerns/sortable.rb
index 1ebecd86af9..83e551fd152 100644
--- a/app/models/concerns/sortable.rb
+++ b/app/models/concerns/sortable.rb
@@ -38,10 +38,12 @@ module Sortable
 
     private
 
-    def highest_label_priority(object_types, condition_field, excluded_labels: [])
-      query = Label.select(Label.arel_table[:priority].minimum).
+    def highest_label_priority(object_types, project_field, condition_field, excluded_labels: [])
+      query = Label.select(LabelPriority.arel_table[:priority].minimum).
+        left_join_priorities.
         joins(:label_links).
         where(label_links: { target_type: object_types }).
+        where("label_priorities.project_id = #{project_field}").
         where("label_links.target_id = #{condition_field}").
         reorder(nil)
 
diff --git a/app/models/label.rb b/app/models/label.rb
index ea11d9d7864..1d775a83f96 100644
--- a/app/models/label.rb
+++ b/app/models/label.rb
@@ -42,6 +42,17 @@ class Label < ActiveRecord::Base
     where.not(id: prioritized(project).select(:id))
   end
 
+  def self.left_join_priorities
+    labels = Label.arel_table
+    priorities = LabelPriority.arel_table
+
+    label_priorities = labels.join(priorities, Arel::Nodes::OuterJoin).
+                              on(labels[:id].eq(priorities[:label_id])).
+                              join_sources
+
+    joins(label_priorities)
+  end
+
   alias_attribute :name, :title
 
   def self.reference_prefix
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 6ae9956ade5..fd90a893d2e 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -52,7 +52,7 @@ class Todo < ActiveRecord::Base
     # Todos with highest priority first then oldest todos
     # 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
-      highest_priority = highest_label_priority(["Issue", "MergeRequest"], "todos.target_id").to_sql
+      highest_priority = highest_label_priority(["Issue", "MergeRequest"], "todos.project_id", "todos.target_id").to_sql
 
       select("#{table_name}.*, (#{highest_priority}) AS highest_priority").
         order(Gitlab::Database.nulls_last_order('highest_priority', 'ASC')).
diff --git a/spec/controllers/projects/labels_controller_spec.rb b/spec/controllers/projects/labels_controller_spec.rb
index 29251f49810..622ab154493 100644
--- a/spec/controllers/projects/labels_controller_spec.rb
+++ b/spec/controllers/projects/labels_controller_spec.rb
@@ -15,21 +15,28 @@ describe Projects::LabelsController do
     let!(:label_1) { create(:label, project: project, priority: 1, title: 'Label 1') }
     let!(:label_2) { create(:label, project: project, priority: 3, title: 'Label 2') }
     let!(:label_3) { create(:label, project: project, priority: 1, title: 'Label 3') }
-    let!(:label_4) { create(:label, project: project, priority: nil, title: 'Label 4') }
-    let!(:label_5) { create(:label, project: project, priority: nil, title: 'Label 5') }
+    let!(:label_4) { create(:label, project: project, title: 'Label 4') }
+    let!(:label_5) { create(:label, project: project, title: 'Label 5') }
 
-    let!(:group_label_1) { create(:group_label, group: group, priority: 3, title: 'Group Label 1') }
-    let!(:group_label_2) { create(:group_label, group: group, priority: 1, title: 'Group Label 2') }
-    let!(:group_label_3) { create(:group_label, group: group, priority: nil, title: 'Group Label 3') }
-    let!(:group_label_4) { create(:group_label, group: group, priority: nil, title: 'Group Label 4') }
+    let!(:group_label_1) { create(:group_label, group: group, title: 'Group Label 1') }
+    let!(:group_label_2) { create(:group_label, group: group, title: 'Group Label 2') }
+    let!(:group_label_3) { create(:group_label, group: group, title: 'Group Label 3') }
+    let!(:group_label_4) { create(:group_label, group: group, title: 'Group Label 4') }
+
+    before do
+      create(:label_priority, project: project, label: group_label_1, priority: 3)
+      create(:label_priority, project: project, label: group_label_2, priority: 1)
+    end
 
     context '@prioritized_labels' do
       before do
         list_labels
       end
 
-      it 'contains only prioritized labels' do
-        expect(assigns(:prioritized_labels)).to all(have_attributes(priority: a_value > 0))
+      it 'does not include labels without priority' do
+        list_labels
+
+        expect(assigns(:prioritized_labels)).not_to include(group_label_3, group_label_4, label_4, label_5)
       end
 
       it 'is sorted by priority, then label title' do
@@ -38,16 +45,16 @@ describe Projects::LabelsController do
     end
 
     context '@labels' do
-      it 'contains only unprioritized labels' do
+      it 'is sorted by label title' do
         list_labels
 
-        expect(assigns(:labels)).to all(have_attributes(priority: nil))
+        expect(assigns(:labels)).to eq [group_label_3, group_label_4, label_4, label_5]
       end
 
-      it 'is sorted by label title' do
+      it 'does not include labels with priority' do
         list_labels
 
-        expect(assigns(:labels)).to eq [group_label_3, group_label_4, label_4, label_5]
+        expect(assigns(:labels)).not_to include(group_label_2, label_1, label_3, group_label_1, label_2)
       end
 
       it 'does not include group labels when project does not belong to a group' do
diff --git a/spec/factories/labels.rb b/spec/factories/labels.rb
index 5c789d72bac..3e8822faf97 100644
--- a/spec/factories/labels.rb
+++ b/spec/factories/labels.rb
@@ -3,6 +3,16 @@ FactoryGirl.define do
     sequence(:title) { |n| "label#{n}" }
     color "#990000"
     project
+
+    transient do
+      priority nil
+    end
+
+    after(:create) do |label, evaluator|
+      if evaluator.priority
+        label.priorities.create(project: label.project, priority: evaluator.priority)
+      end
+    end
   end
 
   factory :group_label, class: GroupLabel do