diff options
| -rw-r--r-- | app/assets/javascripts/LabelManager.js.coffee | 21 | ||||
| -rw-r--r-- | app/controllers/projects/labels_controller.rb | 15 | ||||
| -rw-r--r-- | app/finders/issuable_finder.rb | 2 | ||||
| -rw-r--r-- | app/models/concerns/issuable.rb | 30 | ||||
| -rw-r--r-- | app/models/issue.rb | 4 | ||||
| -rw-r--r-- | app/models/label.rb | 25 | ||||
| -rw-r--r-- | app/views/projects/labels/_label.html.haml | 2 | ||||
| -rw-r--r-- | config/initializers/nulls_last.rb | 15 | ||||
| -rw-r--r-- | config/routes.rb | 2 | ||||
| -rw-r--r-- | lib/gitlab/database.rb | 14 | ||||
| -rw-r--r-- | spec/lib/gitlab/database_spec.rb | 16 |
11 files changed, 85 insertions, 61 deletions
diff --git a/app/assets/javascripts/LabelManager.js.coffee b/app/assets/javascripts/LabelManager.js.coffee index 056a03651b5..ad2acb9d0b9 100644 --- a/app/assets/javascripts/LabelManager.js.coffee +++ b/app/assets/javascripts/LabelManager.js.coffee @@ -25,7 +25,7 @@ class @LabelManager action = if $btn.parents('.js-prioritized-labels').length then 'remove' else 'add' _this.toggleLabelPriority($label, action) - toggleLabelPriority: ($label, action, pasive = false) -> + toggleLabelPriority: ($label, action, persistState = false) -> _this = @ url = $label.find('.js-toggle-priority').data 'url' @@ -46,16 +46,19 @@ class @LabelManager $label.detach().appendTo($target) # Return if we are not persisting state - return if pasive + return if persistState - xhr = $.post url + if action is 'remove' + xhr = $.ajax url: url, type: 'DELETE' - # If request fails, put label back to Other labels group - xhr.fail -> - _this.toggleLabelPriority($label, 'remove', true) + # If request fails, put label back to Other labels group + xhr.fail -> + _this.toggleLabelPriority($label, 'remove', true) - # Show a message - new Flash('Unable to update label prioritization at this time' , 'alert') + # Show a message + new Flash('Unable to update label prioritization at this time' , 'alert') + else + @savePrioritySort() onPrioritySortUpdate: -> @savePrioritySort() @@ -76,4 +79,4 @@ class @LabelManager sortedIds = [] @prioritizedLabels.find('li').each -> sortedIds.push $(@).data 'id' - sortedIds
\ No newline at end of file + sortedIds diff --git a/app/controllers/projects/labels_controller.rb b/app/controllers/projects/labels_controller.rb index 0a60a802430..bd46a81ff10 100644 --- a/app/controllers/projects/labels_controller.rb +++ b/app/controllers/projects/labels_controller.rb @@ -72,11 +72,9 @@ class Projects::LabelsController < Projects::ApplicationController end end - def toggle_priority - priority = label.priority - + def remove_priority respond_to do |format| - if label.update_attributes(priority: !priority) + if label.update_attribute(:priority, nil) format.json { render json: label } else message = label.errors.full_messages.uniq.join('. ') @@ -86,8 +84,15 @@ class Projects::LabelsController < Projects::ApplicationController end def set_sorting + Label.transaction do + params[:label_ids].each_with_index do |label_id, index| + label = @project.labels.find_by_id(label_id) + label.update_attribute(:priority, index) if label + end + end + respond_to do |format| - format.json { render json: {message: 'success'}} + format.json { render json: { message: 'success' } } end end diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 68ab6e87684..a0932712bd0 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -224,7 +224,7 @@ class IssuableFinder def sort(items) # Ensure we always have an explicit sort order (instead of inheriting # multiple orders when combining ActiveRecord::Relation objects). - params[:sort] ? items.sort(params[:sort], label_names) : items.reorder(id: :desc) + params[:sort] ? items.sort(params[:sort], excluded_labels: label_names) : items.reorder(id: :desc) end def by_assignee(items) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 8871cb8a6cd..92526a99147 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -48,12 +48,6 @@ module Issuable scope :non_archived, -> { join_project.where(projects: { archived: false }) } - def self.order_priority(labels) - select("#{table_name}.*, (#{Label.high_priority(name, table_name, labels).to_sql}) AS highest_priority") - .group("#{table_name}.id") - .reorder(nulls_last('highest_priority', 'ASC')) - end - delegate :name, :email, to: :author, @@ -111,18 +105,24 @@ module Issuable where(t[:title].matches(pattern).or(t[:description].matches(pattern))) end - def sort(method, labels = []) + def sort(method, excluded_labels: []) case method.to_s when 'milestone_due_asc' then order_milestone_due_asc when 'milestone_due_desc' then order_milestone_due_desc when 'downvotes_desc' then order_downvotes_desc when 'upvotes_desc' then order_upvotes_desc - when 'priority' then order_priority(labels) + when 'priority' then order_labels_priority(excluded_labels: excluded_labels) else order_by(method) end end + def order_labels_priority(excluded_labels: []) + select("#{table_name}.*, (#{highest_label_priority(excluded_labels).to_sql}) AS highest_priority"). + group(arel_table[:id]). + reorder(Gitlab::Database.nulls_last_order('highest_priority', 'ASC')) + end + def with_label(title, sort = nil) if title.is_a?(Array) && title.size > 1 joins(:labels).where(labels: { title: title }).group(*grouping_columns(sort)).having("COUNT(DISTINCT labels.title) = #{title.size}") @@ -146,6 +146,20 @@ module Issuable grouping_columns end + + private + + def highest_label_priority(excluded_labels) + query = Label.select(Label.arel_table[:priority].minimum). + joins(:label_links). + where(label_links: { target_type: name }). + where("label_links.target_id = #{table_name}.id"). + reorder(nil) + + query.where.not(title: excluded_labels) if excluded_labels.present? + + query + end end def today? diff --git a/app/models/issue.rb b/app/models/issue.rb index bd0fbc96d18..235922710ad 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -75,10 +75,10 @@ class Issue < ActiveRecord::Base @link_reference_pattern ||= super("issues", /(?<issue>\d+)/) end - def self.sort(method) + def self.sort(method, excluded_labels: []) case method.to_s when 'due_date_asc' then order_due_date_asc - when 'due_date_desc' then order_due_date_desc + when 'due_date_desc' then order_due_date_desc else super end diff --git a/app/models/label.rb b/app/models/label.rb index 4437ca393ed..7fd77880558 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -19,7 +19,6 @@ class Label < ActiveRecord::Base validates :color, color: true, allow_blank: false validates :project, presence: true, unless: Proc.new { |service| service.template? } - validates :priority, presence: false, default: false # Don't allow '?', '&', and ',' for label titles validates :title, @@ -32,21 +31,11 @@ class Label < ActiveRecord::Base default_scope { order(title: :asc) } scope :templates, -> { where(template: true) } - scope :prioritized, ->(value = true) { where(priority: value) } - - def self.high_priority(name, table_name, labels) - unfiltered = unscoped - .select("MIN(labels.priority)") - .joins("INNER JOIN label_links ON label_links.label_id = labels.id") - .where("label_links.target_type = '#{name}'") - .where("label_links.target_id = #{table_name}.id") - .where("labels.project_id = #{table_name}.project_id") - - if labels.empty? - unfiltered - else - unfiltered.where("labels.title NOT IN (?)", labels) - end + + def self.prioritized(bool = true) + query = bool ? where.not(priority: nil) : where(priority: nil) + + query.reorder(Gitlab::Database.nulls_last_order(:priority), :title) end alias_attribute :name, :title @@ -139,8 +128,6 @@ class Label < ActiveRecord::Base end def nillify_priority - unless self.priority.present? - self.priority = nil - end + self.priority = nil if priority.blank? end end diff --git a/app/views/projects/labels/_label.html.haml b/app/views/projects/labels/_label.html.haml index eda75b64e79..a22dee1a626 100644 --- a/app/views/projects/labels/_label.html.haml +++ b/app/views/projects/labels/_label.html.haml @@ -1,7 +1,7 @@ - label_css_id = dom_id(label) %li{id: label_css_id, :"data-id" => label.id} %a.js-toggle-priority{:href => "#", - :"data-url" => toggle_priority_namespace_project_label_path(@project.namespace, @project, label), + :"data-url" => remove_priority_namespace_project_label_path(@project.namespace, @project, label), :"data-dom-id" => "#{label_css_id}" } %span.add-priority (+) diff --git a/config/initializers/nulls_last.rb b/config/initializers/nulls_last.rb deleted file mode 100644 index 47b7b0bb3d2..00000000000 --- a/config/initializers/nulls_last.rb +++ /dev/null @@ -1,15 +0,0 @@ -module ActiveRecord - class Base - def self.nulls_last(field, direction = 'ASC') - if Gitlab::Database.postgresql? - "#{field} #{direction} NULLS LAST" - else - if direction.upcase == 'ASC' - "-#{field} DESC" - else - "#{field} DESC" - end - end - end - end -end diff --git a/config/routes.rb b/config/routes.rb index 5aa8a0fe8a5..93ff825d7b2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -724,7 +724,7 @@ Rails.application.routes.draw do member do post :toggle_subscription - post :toggle_priority + delete :remove_priority end end diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index 42bec913a45..04fa6a3a5de 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -16,6 +16,20 @@ module Gitlab database_version.match(/\A(?:PostgreSQL |)([^\s]+).*\z/)[1] end + def self.nulls_last_order(field, direction = 'ASC') + order = "#{field} #{direction}" + + if Gitlab::Database.postgresql? + order << ' NULLS LAST' + else + # `field IS NULL` will be `0` for non-NULL columns and `1` for NULL + # columns. In the (default) ascending order, `0` comes first. + order.prepend("#{field} IS NULL, ") if direction == 'ASC' + end + + order + end + def true_value if Gitlab::Database.postgresql? "'t'" diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb index d0a447753b7..3031559c613 100644 --- a/spec/lib/gitlab/database_spec.rb +++ b/spec/lib/gitlab/database_spec.rb @@ -39,6 +39,22 @@ describe Gitlab::Database, lib: true do end end + describe '.nulls_last_order' do + context 'when using PostgreSQL' do + before { expect(described_class).to receive(:postgresql?).and_return(true) } + + it { expect(described_class.nulls_last_order('column', 'ASC')).to eq 'column ASC NULLS LAST'} + it { expect(described_class.nulls_last_order('column', 'DESC')).to eq 'column DESC NULLS LAST'} + end + + context 'when using MySQL' do + before { expect(described_class).to receive(:postgresql?).and_return(false) } + + it { expect(described_class.nulls_last_order('column', 'ASC')).to eq 'column IS NULL, column ASC'} + it { expect(described_class.nulls_last_order('column', 'DESC')).to eq 'column DESC'} + end + end + describe '#true_value' do it 'returns correct value for PostgreSQL' do expect(described_class).to receive(:postgresql?).and_return(true) |