Merge branch '1674-weight-desc-nullfix' into 'master'

Issue - fix order_weight_desc so null weights are placed at the end

Closes #1674

See merge request gitlab-org/gitlab-ee!4714

app/models/issue.rb

@@ -18,11 +18,6 @@ class Issue < ActiveRecord::Base
 
   ignore_column :assignee_id, :branch_name, :deleted_at
 
-  WEIGHT_RANGE = 1..9
-  WEIGHT_ALL = 'Everything'.freeze
-  WEIGHT_ANY = 'Any Weight'.freeze
-  WEIGHT_NONE = 'No Weight'.freeze
-
   DueDateStruct = Struct.new(:title, :name).freeze
   NoDueDate     = DueDateStruct.new('No Due Date', '0').freeze
   AnyDueDate    = DueDateStruct.new('Any Due Date', '').freeze
@@ -62,9 +57,6 @@ class Issue < ActiveRecord::Base
   scope :order_due_date_asc, -> { reorder('issues.due_date IS NULL, issues.due_date ASC') }
   scope :order_due_date_desc, -> { reorder('issues.due_date IS NULL, issues.due_date DESC') }
 
-  scope :order_weight_desc, -> { reorder('weight IS NOT NULL, weight DESC') }
-  scope :order_weight_asc, -> { reorder('weight ASC') }
-
   scope :preload_associations, -> { preload(:labels, project: :namespace) }
 
   scope :public_only, -> { where(confidential: false) }
@@ -128,9 +120,6 @@ class Issue < ActiveRecord::Base
     when 'due_date'      then order_due_date_asc
     when 'due_date_asc'  then order_due_date_asc
     when 'due_date_desc' then order_due_date_desc
-    when 'weight'        then order_weight_asc
-    when 'weight_asc'    then order_weight_asc
-    when 'weight_desc'   then order_weight_desc
     else
       super
     end
@@ -253,14 +242,6 @@ class Issue < ActiveRecord::Base
     end
   end
 
-  def self.weight_filter_options
-    WEIGHT_RANGE.to_a
-  end
-
-  def self.weight_options
-    [WEIGHT_NONE] + WEIGHT_RANGE.to_a
-  end
-
   def moved?
     !moved_to.nil?
   end

ee/app/models/ee/issue.rb

 module EE
   module Issue
+    extend ActiveSupport::Concern
     extend ::Gitlab::Utils::Override
 
+    prepended do
+      WEIGHT_RANGE = 1..9
+      WEIGHT_ALL = 'Everything'.freeze
+      WEIGHT_ANY = 'Any Weight'.freeze
+      WEIGHT_NONE = 'No Weight'.freeze
+
+      scope :order_weight_desc, -> { reorder ::Gitlab::Database.nulls_last_order('weight', 'DESC') }
+      scope :order_weight_asc, -> { reorder ::Gitlab::Database.nulls_last_order('weight') }
+    end
+
     # override
     def check_for_spam?
       author.support_bot? || super
@@ -56,5 +67,26 @@ module EE
     def supports_weight?
       project&.feature_available?(:issue_weights)
     end
+
+    module ClassMethods
+      # override
+      def sort(method, excluded_labels: [])
+        case method.to_s
+        when 'weight'        then order_weight_asc
+        when 'weight_asc'    then order_weight_asc
+        when 'weight_desc'   then order_weight_desc
+        else
+          super
+        end
+      end
+
+      def weight_filter_options
+        WEIGHT_RANGE.to_a
+      end
+
+      def weight_options
+        [WEIGHT_NONE] + WEIGHT_RANGE.to_a
+      end
+    end
   end
 end

ee/spec/models/issue_spec.rb

@@ -22,6 +22,27 @@ describe Issue do
     end
   end
 
+  describe '#sort' do
+    let(:project) { create(:project) }
+
+    context "by weight" do
+      let!(:issue)  { create(:issue, project: project) }
+      let!(:issue2) { create(:issue, weight: 1, project: project) }
+      let!(:issue3) { create(:issue, weight: 2, project: project) }
+      let!(:issue4) { create(:issue, weight: 3, project: project) }
+
+      it "sorts desc" do
+        issues = project.issues.sort('weight_desc')
+        expect(issues).to eq([issue4, issue3, issue2, issue])
+      end
+
+      it "sorts asc" do
+        issues = project.issues.sort('weight_asc')
+        expect(issues).to eq([issue2, issue3, issue4, issue])
+      end
+    end
+  end
+
   describe '#weight' do
     where(:license_value, :database_value, :expected) do
       true  | 5   | 5

spec/models/concerns/issuable_spec.rb

@@ -179,23 +179,6 @@ describe Issuable do
   describe "#sort" do
     let(:project) { create(:project) }
 
-    context "by weight" do
-      let!(:issue)  { create(:issue, project: project) }
-      let!(:issue2) { create(:issue, weight: 1, project: project) }
-      let!(:issue3) { create(:issue, weight: 2, project: project) }
-      let!(:issue4) { create(:issue, weight: 3, project: project) }
-
-      it "sorts desc" do
-        issues = project.issues.sort('weight_desc')
-        expect(issues).to match_array([issue4, issue3, issue2, issue])
-      end
-
-      it "sorts asc" do
-        issues = project.issues.sort('weight_asc')
-        expect(issues).to match_array([issue2, issue3, issue4, issue])
-      end
-    end
-
     context "by milestone due date" do
       # Correct order is:
       # Issues/MRs with milestones ordered by date