From 3c2aaec1f2624ad4817e7ac52120985682afa448 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Fri, 14 Oct 2016 20:06:26 -0300 Subject: Fix sorting by label priorities --- app/models/concerns/issuable.rb | 3 ++- app/models/concerns/sortable.rb | 6 +++-- app/models/label.rb | 11 ++++++++ app/models/todo.rb | 2 +- .../controllers/projects/labels_controller_spec.rb | 31 +++++++++++++--------- spec/factories/labels.rb | 10 +++++++ 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 -- cgit v1.2.3