Address reviewer comments

- Filter by no weight
- Modify 'board scoped' method
- Use JS objects rather than a series of constants
- Prevent toggling behaviour that leaves dropdown in a weird state

db/migrate/20190703001116_default_weight_to_nil.rb

 # frozen_string_literal: true
 
 class DefaultWeightToNil < ActiveRecord::Migration[5.1]
-  include Gitlab::Database::MigrationHelpers
-
   DOWNTIME = false
 
-  disable_ddl_transaction!
-
-  class Board < ActiveRecord::Base
-    self.table_name = 'boards'
-
-    include ::EachBatch
-  end
-
   def up
-    Gitlab::BackgroundMigration.steal('UpdateBoardWeightsFromNoneToAny')
-
-    Board.where(weight: -1).each_batch(of: 50) do |batch|
-      range = batch.pluck('MIN(id)', 'MAX(id)').first
-
-      say "Setting board weights from None to Any: #{range[0]} - #{range[1]}"
-
-      Gitlab::BackgroundMigration::UpdateBoardWeightsFromNoneToAny.new.perform(*range)
-    end
+    execute(update_board_weights_query)
   end
 
   def down
-    # noop
+    # no-op
+  end
+
+  private
+
+  # Only 288 records to update, as of 2019/07/18
+  def update_board_weights_query
+    <<~HEREDOC
+   UPDATE boards
+     SET weight = NULL
+   WHERE boards.weight = -1
+    HEREDOC
   end
 end

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

 <script>
+import _ from 'underscore';
 import WeightSelect from 'ee/weight_select';
 import { GlLoadingIcon } from '@gitlab/ui';
+import { __ } from '~/locale';
 
 const ANY_WEIGHT = {
-  LABEL: 'Any Weight',
-  STRING: 'Any',
-  VALUE: null,
+  label: __('Any Weight'),
+  selectValue: 'Any',
+  value: null,
+  valueClass: 'text-secondary',
 };
 
 const NO_WEIGHT = {
-  LABEL: 'No Weight',
-  STRING: 'None',
-  VALUE: -1,
+  label: __('No Weight'),
+  selectValue: 'None',
+  value: -1,
 };
 
-function getWeightValue(weight) {
-  if (Number.isNaN(Number(weight))) {
-    return weight === NO_WEIGHT.STRING ? NO_WEIGHT.VALUE : ANY_WEIGHT.VALUE;
+function unstringifyValue(value) {
+  if (!_.isString(value)) {
+    return value;
   }
+  const numValue = Number(value);
+  return Number.isNaN(numValue) ? null : numValue;
+}
+
+function getWeightValueFromSelect(selectValue) {
+  switch (selectValue) {
+    case ANY_WEIGHT.selectValue:
+      return ANY_WEIGHT.value;
+    case NO_WEIGHT.selectValue:
+      return NO_WEIGHT.value;
+    case null:
+    case undefined:
+      return ANY_WEIGHT.value;
+    default:
+      return Number(selectValue);
+  }
+}
 
-  return parseInt(weight, 10);
+function getWeightFromValue(strValue) {
+  const value = unstringifyValue(strValue);
+  switch (value) {
+    case ANY_WEIGHT.value:
+      return ANY_WEIGHT;
+    case NO_WEIGHT.value:
+      return NO_WEIGHT;
+    default:
+      return {
+        label: String(value),
+        selectValue: value,
+        value,
+      };
+  }
 }
 
 export default {
@@ -34,7 +67,7 @@ export default {
     value: {
       type: [Number, String],
       required: false,
-      default: ANY_WEIGHT.VALUE,
+      default: ANY_WEIGHT.value,
     },
     canEdit: {
       type: Boolean,
@@ -52,16 +85,15 @@ export default {
     };
   },
   computed: {
+    selectedWeight() {
+      return getWeightFromValue(this.value);
+    },
+
     valueClass() {
-      if (this.value === ANY_WEIGHT.VALUE) {
-        return 'text-secondary';
-      }
-      return 'bold';
+      return this.selectedWeight.valueClass || 'bold';
     },
     valueText() {
-      if (this.value === NO_WEIGHT.VALUE) return NO_WEIGHT.LABEL;
-      if (this.value === ANY_WEIGHT.VALUE) return ANY_WEIGHT.LABEL;
-      return this.value;
+      return this.selectedWeight.label;
     },
   },
   mounted() {
@@ -73,18 +105,10 @@ export default {
   },
   methods: {
     isSelected(weight) {
-      if (this.value === NO_WEIGHT.VALUE) {
-        return weight === NO_WEIGHT.STRING;
-      }
-
-      if (this.value === ANY_WEIGHT.VALUE) {
-        return weight === ANY_WEIGHT.STRING;
-      }
-
-      return this.value === weight;
+      return this.selectedWeight.selectValue === weight;
     },
     selectWeight(weight) {
-      this.board.weight = getWeightValue(weight);
+      this.board.weight = getWeightValueFromSelect(weight);
     },
   },
 };

ee/app/assets/javascripts/boards/stores/boards_store_ee.js

@@ -110,13 +110,20 @@ class BoardsStoreEE {
     }
 
     let { weight } = this.store.boardConfig;
-    if (weight !== -1) {
-      if (weight === 0) {
+    if (weight === null) {
       /* eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings */
-        weight = 'No+Weight';
+      weight = 'Any';
+    } else if (weight === -1) {
+      /* eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings */
+      weight = 'None';
+    } else if (weight === 0) {
+      weight = '0';
     }
+    if (weight) {
       updateFilterPath('weight', weight);
+      this.store.cantEdit.push('weight');
     }
+
     updateFilterPath('assignee_username', this.store.boardConfig.assigneeUsername);
     if (this.store.boardConfig.assigneeUsername) {
       this.store.cantEdit.push('assignee');

ee/app/assets/javascripts/weight_select.js

-/* eslint-disable prefer-arrow-callback, one-var, no-var, object-shorthand, no-shadow, no-unused-vars, no-else-return, func-names */
+/* eslint-disable prefer-arrow-callback, one-var, no-var, object-shorthand, no-unused-vars, no-else-return, func-names */
 
 import $ from 'jquery';
 import '~/gl_dropdown';
@@ -49,13 +49,11 @@ function WeightSelect(els, options = {}) {
         }
       },
       clicked: function(glDropdownEvt) {
-        const { e } = glDropdownEvt;
-        let selected = glDropdownEvt.selectedObj;
-        const inputField = $dropdown.closest('.selectbox').find(`input[name='${fieldName}']`);
+        const { e, $el } = glDropdownEvt;
+        const selected = $el.data('id');
 
         if (options.handleClick) {
           e.preventDefault();
-          selected = inputField.val();
           options.handleClick(selected);
         } else if ($dropdown.is('.js-issuable-form-weight')) {
           e.preventDefault();

ee/app/finders/ee/issues_finder.rb

@@ -24,7 +24,7 @@ module EE
 
     # rubocop: disable CodeReuse/ActiveRecord
     def by_weight(items)
-      return items unless weights?
+      return items unless filtered_by_weight?
       return items if filter_by_any_weight?
 
       if filter_by_no_weight?
@@ -35,7 +35,7 @@ module EE
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
-    def weights?
+    def filtered_by_weight?
       params[:weight].present? && params[:weight] != ::Issue::WEIGHT_ALL
     end
 

ee/app/models/ee/board.rb

@@ -6,7 +6,7 @@ module EE
     extend ::Gitlab::Utils::Override
 
     # Empty state for milestones and weights.
-    EMPTY_SCOPE_STATE = [nil, -1].freeze
+    EMPTY_SCOPE_STATE = [nil].freeze
 
     prepended do
       belongs_to :milestone

ee/spec/features/boards/scoped_issue_board_spec.rb

@@ -177,7 +177,7 @@ describe 'Scoped issue boards', :js do
         it 'creates board filtering by "None" weight' do
           create_board_weight('None')
 
-          expect(page).to have_selector('.board-card', count: 5)
+          expect(page).to have_selector('.board-card', count: 4)
         end
 
         it 'displays dot highlight and tooltip' do

ee/spec/finders/issues_finder_spec.rb

@@ -16,7 +16,7 @@ describe IssuesFinder do
         context 'filter issues with no weight' do
           let(:params) { { weight: Issue::WEIGHT_NONE } }
 
-          it 'returns all issues with nil weight' do
+          it 'returns all issues with no weight' do
             expect(issues).to contain_exactly(issue_with_no_weight, issue1, issue2, issue3, issue4)
           end
         end

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

@@ -21,12 +21,16 @@ function activeDropdownItem() {
   return vm.$el.querySelector('.is-active').innerText.trim();
 }
 
+function findDropdownItem(text) {
+  return Array.from(vm.$el.querySelectorAll('li a')).find(({ innerText }) => innerText === text);
+}
+
 describe('WeightSelect', () => {
   beforeEach(done => {
     setFixtures('<div class="test-container"></div>');
 
     board = {
-      weight: 0,
+      weight: -1,
       labels: [],
     };
 
@@ -118,10 +122,9 @@ describe('WeightSelect', () => {
     it('sets value', done => {
       vm.$el.querySelector('.edit-link').click();
 
-      const dropdownItem = vm.$el.querySelectorAll('li a')[4];
-
-      expect(dropdownItem.innerText).toBe('2');
-      dropdownItem.click();
+      setTimeout(() => {
+        findDropdownItem('2').click();
+      });
 
       setTimeout(() => {
         expect(activeDropdownItem()).toEqual('2');
@@ -135,7 +138,7 @@ describe('WeightSelect', () => {
       vm.$el.querySelector('.edit-link').click();
 
       setTimeout(() => {
-        vm.$el.querySelectorAll('li a')[0].click();
+        findDropdownItem('Any').click();
       });
 
       setTimeout(() => {
@@ -145,12 +148,26 @@ describe('WeightSelect', () => {
       });
     });
 
+    it('sets Any Weight if it is already selected', done => {
+      vm.value = null;
+      vm.$el.querySelector('.edit-link').click();
+
+      setTimeout(() => {
+        findDropdownItem('Any').click();
+      });
+
+      setTimeout(() => {
+        expect(board.weight).toEqual(null);
+        done();
+      });
+    });
+
     it('sets No Weight', done => {
       vm.value = 2;
       vm.$el.querySelector('.edit-link').click();
 
       setTimeout(() => {
-        vm.$el.querySelectorAll('li a')[1].click();
+        findDropdownItem('None').click();
       });
 
       setTimeout(() => {
@@ -159,5 +176,19 @@ describe('WeightSelect', () => {
         done();
       });
     });
+
+    it('sets No Weight if it is already selected', done => {
+      vm.value = -1;
+      vm.$el.querySelector('.edit-link').click();
+
+      setTimeout(() => {
+        findDropdownItem('None').click();
+      });
+
+      setTimeout(() => {
+        expect(board.weight).toEqual(-1);
+        done();
+      });
+    });
   });
 });

ee/spec/models/board_spec.rb

@@ -92,19 +92,37 @@ describe Board do
       stub_licensed_features(scoped_issue_board: true)
     end
 
-    it 'returns true when milestone is not nil' do
+    it 'returns true when milestone is not "Any milestone" AND is not "No milestone"' do
       milestone = create(:milestone)
       board = create(:board, milestone: milestone, weight: nil, labels: [], assignee: nil)
 
       expect(board).to be_scoped
     end
 
-    it 'returns true when weight is not nil' do
+    it "returns true when milestone is 'No milestone'" do
+      board = create(:board, milestone_id: -1, weight: nil, labels: [], assignee: nil)
+
+      expect(board).to be_scoped
+    end
+
+    it 'returns true when weight is 0 weight' do
+      board = create(:board, milestone: nil, weight: 0, labels: [], assignee: nil)
+
+      expect(board).to be_scoped
+    end
+
+    it 'returns true when weight is not "Any weight" AND is not "No weight"' do
       board = create(:board, milestone: nil, weight: 2, labels: [], assignee: nil)
 
       expect(board).to be_scoped
     end
 
+    it 'returns true when weight is "No weight"' do
+      board = create(:board, milestone: nil, weight: -1, labels: [], assignee: nil)
+
+      expect(board).to be_scoped
+    end
+
     it 'returns true when any label exists' do
       board = create(:board, milestone: nil, weight: nil, assignee: nil)
       board.labels.create!(title: 'foo')

ee/spec/services/ee/boards/issues/create_service_spec.rb

@@ -55,7 +55,7 @@ describe Boards::Issues::CreateService do
           expect(issue).to be_valid
         end
 
-        context 'when board weight is invalid' do
+        context "when board weight is 'none'" do
           it 'creates issue with nil weight' do
             board.update(weight: -1)
 
@@ -65,6 +65,17 @@ describe Boards::Issues::CreateService do
             expect(issue).to be_valid
           end
         end
+
+        context "when board weight is invalid" do
+          it 'creates issue with nil weight' do
+            board.update(weight: -6)
+
+            issue = service.execute
+
+            expect(issue.weight).to be_nil
+            expect(issue).to be_valid
+          end
+        end
       end
     end
 

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

locale/gitlab.pot

@@ -1501,6 +1501,9 @@ msgstr ""
 msgid "Any Milestone"
 msgstr ""
 
+msgid "Any Weight"
+msgstr ""
+
 msgid "Any encrypted tokens"
 msgstr ""
 
@@ -9447,6 +9450,9 @@ msgstr ""
 msgid "No Tag"
 msgstr ""
 
+msgid "No Weight"
+msgstr ""
+
 msgid "No activities found"
 msgstr ""