Address reviewer comments

- Use BackgroundMigration
- Style/readability
- Use pure function for Weight method
- use say in migration instead of logger
- make update query more concise

ee/changelogs/unreleased/4221-board-scope-shouldn-t-persist-1-when-one-selects-any-weight-or-any-milestone.yml → changelogs/unreleased/4221-board-scope-shouldn-t-persist-1-when-one-selects-any-weight-or-any-milestone.yml

 ---
 title: Properly persist -1 and null for board weight
-merge_request: 14180
+merge_request:
 author:
 type: changed

db/migrate/20190703001116_default_weight_to_nil.rb

 # frozen_string_literal: true
 
-# See http://doc.gitlab.com/ce/development/migration_style_guide.html
-# for more information on how to write migrations for GitLab.
-
 class DefaultWeightToNil < ActiveRecord::Migration[5.1]
   include Gitlab::Database::MigrationHelpers
 
-  # Set this constant to true if this migration requires downtime.
   DOWNTIME = false
 
-  # When a migration requires downtime you **must** uncomment the following
-  # constant and define a short and easy to understand explanation as to why the
-  # migration requires downtime.
-  # DOWNTIME_REASON = ''
-
-  # When using the methods "add_concurrent_index", "remove_concurrent_index" or
-  # "add_column_with_default" you must disable the use of transactions
-  # as these methods can not run in an existing transaction.
-  # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure
-  # that either of them is the _only_ method called in the migration,
-  # any other changes should go in a separate migration.
-  # This ensures that upon failure _only_ the index creation or removing fails
-  # and can be retried or reverted easily.
-  #
-  # To disable transactions uncomment the following line and remove these
-  # comments:
-  # disable_ddl_transaction!
+  disable_ddl_transaction!
 
-  def up
-    update_board_weights_from_none_to_any
-  end
+  class Board < ActiveRecord::Base
+    self.table_name = 'boards'
 
-  def down
-    update_board_weights_from_any_to_none
+    include ::EachBatch
   end
 
-  # up method
+  def up
+    Gitlab::BackgroundMigration.steal('UpdateBoardWeightsFromNoneToAny')
+
+    Board.where(weight: -1).each_batch(of: 50) do |batch|
+      range = batch.pluck('MIN(id)', 'MAX(id)').first
 
-  def update_board_weights_from_none_to_any
-    execute("UPDATE boards SET weight = null WHERE weight = -1")
-  end
+      say "Setting board weights from None to Any: #{range[0]} - #{range[1]}"
 
-  # down method
+      Gitlab::BackgroundMigration::UpdateBoardWeightsFromNoneToAny.new.perform(*range)
+    end
+  end
 
-  def update_board_weights_from_any_to_none
-    execute("UPDATE boards SET weight = -1 WHERE weight = null")
+  def down
+    # noop
   end
 end

ee/app/assets/javascripts/boards/components/weight_select.vue

@@ -2,14 +2,25 @@
 import WeightSelect from 'ee/weight_select';
 import { GlLoadingIcon } from '@gitlab/ui';
 
-const ANY_WEIGHT_LABEL = 'Any Weight';
-const NO_WEIGHT_LABEL = 'No Weight';
+const ANY_WEIGHT = {
+  LABEL: 'Any Weight',
+  STRING: 'Any',
+  VALUE: null,
+};
 
-const NO_WEIGHT_STR_VALUE = 'None';
-const ANY_WEIGHT_STR_VALUE = 'Any';
+const NO_WEIGHT = {
+  LABEL: 'No Weight',
+  STRING: 'None',
+  VALUE: -1,
+};
 
-const NO_WEIGHT_INT_VALUE = -1;
-const ANY_WEIGHT_INT_VALUE = null;
+function getWeightValue(weight) {
+  if (Number.isNaN(Number(weight))) {
+    return weight === NO_WEIGHT.STRING ? NO_WEIGHT.VALUE : ANY_WEIGHT.VALUE;
+  }
+
+  return parseInt(weight, 10);
+}
 
 export default {
   components: {
@@ -23,7 +34,7 @@ export default {
     value: {
       type: [Number, String],
       required: false,
-      default: ANY_WEIGHT_INT_VALUE,
+      default: ANY_WEIGHT.VALUE,
     },
     canEdit: {
       type: Boolean,
@@ -42,14 +53,14 @@ export default {
   },
   computed: {
     valueClass() {
-      if (this.value === ANY_WEIGHT_INT_VALUE) {
+      if (this.value === ANY_WEIGHT.VALUE) {
         return 'text-secondary';
       }
       return 'bold';
     },
     valueText() {
-      if (this.value === NO_WEIGHT_INT_VALUE) return NO_WEIGHT_LABEL;
-      if (this.value === ANY_WEIGHT_INT_VALUE) return ANY_WEIGHT_LABEL;
+      if (this.value === NO_WEIGHT.VALUE) return NO_WEIGHT.LABEL;
+      if (this.value === ANY_WEIGHT.VALUE) return ANY_WEIGHT.LABEL;
       return this.value;
     },
   },
@@ -62,29 +73,18 @@ export default {
   },
   methods: {
     isSelected(weight) {
-      if (this.value === NO_WEIGHT_INT_VALUE) {
-        return weight === NO_WEIGHT_STR_VALUE;
+      if (this.value === NO_WEIGHT.VALUE) {
+        return weight === NO_WEIGHT.STRING;
       }
 
-      if (this.value === ANY_WEIGHT_INT_VALUE) {
-        return weight === ANY_WEIGHT_STR_VALUE;
+      if (this.value === ANY_WEIGHT.VALUE) {
+        return weight === ANY_WEIGHT.STRING;
       }
 
       return this.value === weight;
     },
     selectWeight(weight) {
-      this.board.weight = this.weightInt(weight);
-    },
-    weightInt(weight) {
-      if (weight === NO_WEIGHT_STR_VALUE) {
-        return NO_WEIGHT_INT_VALUE;
-      }
-
-      if (weight === ANY_WEIGHT_STR_VALUE) {
-        return ANY_WEIGHT_INT_VALUE;
-      }
-
-      return weight;
+      this.board.weight = getWeightValue(weight);
     },
   },
 };

ee/app/finders/ee/issues_finder.rb

@@ -25,11 +25,10 @@ module EE
     # rubocop: disable CodeReuse/ActiveRecord
     def by_weight(items)
       return items unless weights?
+      return items if filter_by_any_weight?
 
       if filter_by_no_weight?
         items.where(weight: nil)
-      elsif filter_by_any_weight?
-        items
       else
         items.where(weight: params[:weight])
       end

ee/spec/finders/issues_finder_spec.rb

@@ -36,6 +36,14 @@ describe IssuesFinder do
             expect(issues).to contain_exactly(issue_with_weight_42)
           end
         end
+
+        context 'filter issues with a specific weight with no issues' do
+          let(:params) { { weight: 7 } }
+
+          it 'returns no issues' do
+            expect(issues).to be_empty
+          end
+        end
       end
 
       context 'filtering by assignee IDs' do

ee/spec/javascripts/boards/components/weight_select_spec.js

@@ -125,7 +125,7 @@ describe('WeightSelect', () => {
 
       setTimeout(() => {
         expect(activeDropdownItem()).toEqual('2');
-        expect(board.weight).toEqual('2');
+        expect(board.weight).toEqual(2);
         done();
       });
     });

lib/gitlab/background_migration/update_board_weights_from_none_to_any.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    class UpdateBoardWeightsFromNoneToAny
+      class Board < ActiveRecord::Base
+        self.table_name = 'boards'
+      end
+
+      def perform(start_id, stop_id)
+        Board.where(id: start_id..stop_id).update_all(milestone: nil)
+      end
+    end
+  end
+end