Improve Issuable.order_labels_priority

Signed-off-by: Rémy Coutable <remy@rymai.me>

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,9 +46,10 @@ 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 ->
@@ -56,6 +57,8 @@ class @LabelManager
 
         # Show a message
         new Flash('Unable to update label prioritization at this time' , 'alert')
+    else
+      @savePrioritySort()
 
   onPrioritySortUpdate: ->
     @savePrioritySort()

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
 

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)

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?

app/models/issue.rb

@@ -75,7 +75,7 @@ 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

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

app/views/projects/labels/_label.html.haml

 - 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
       (+)

config/initializers/nulls_last.rb

-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

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
 

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'"

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)