Merge branch...

Merge branch '4221-board-scope-shouldn-t-persist-1-when-one-selects-any-weight-or-any-milestone' into 'master'

Properly persist "-1" and "null" for board weight

Closes #11529 and #4221

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

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:
+author:
+type: changed

db/migrate/20190703001116_default_weight_to_nil.rb

+# frozen_string_literal: true
+
+class DefaultWeightToNil < ActiveRecord::Migration[5.1]
+  DOWNTIME = false
+
+  def up
+    execute(update_board_weights_query)
+  end
+
+  def down
+    # 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>
-/* eslint-disable vue/require-default-prop */
-
+import _ from 'underscore';
 import WeightSelect from 'ee/weight_select';
 import { GlLoadingIcon } from '@gitlab/ui';
+import { __ } from '~/locale';
+
+const ANY_WEIGHT = {
+  label: __('Any Weight'),
+  selectValue: 'Any',
+  value: null,
+  valueClass: 'text-secondary',
+};
 
-const ANY_WEIGHT = 'Any Weight';
-const NO_WEIGHT = 'No Weight';
+const NO_WEIGHT = {
+  label: __('No Weight'),
+  selectValue: 'None',
+  value: -1,
+};
+
+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);
+  }
+}
+
+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 {
   components: {
@@ -19,6 +67,7 @@ export default {
     value: {
       type: [Number, String],
       required: false,
+      default: ANY_WEIGHT.value,
     },
     canEdit: {
       type: Boolean,
@@ -36,16 +85,15 @@ export default {
     };
   },
   computed: {
+    selectedWeight() {
+      return getWeightFromValue(this.value);
+    },
+
     valueClass() {
-      if (this.valueText === ANY_WEIGHT) {
-        return 'text-secondary';
-      }
-      return 'bold';
+      return this.selectedWeight.valueClass || 'bold';
     },
     valueText() {
-      if (this.value > 0) return this.value;
-      if (this.value === 0) return NO_WEIGHT;
-      return ANY_WEIGHT;
+      return this.selectedWeight.label;
     },
   },
   mounted() {
@@ -56,17 +104,11 @@ export default {
     });
   },
   methods: {
-    selectWeight(weight) {
-      this.board.weight = this.weightInt(weight);
+    isSelected(weight) {
+      return this.selectedWeight.selectValue === weight;
     },
-    weightInt(weight) {
-      if (weight > 0) {
-        return weight;
-      }
-      if (weight === NO_WEIGHT) {
-        return 0;
-      }
-      return -1;
+    selectWeight(weight) {
+      this.board.weight = getWeightValueFromSelect(weight);
     },
   },
 };
@@ -96,7 +138,7 @@ export default {
           <div class="dropdown-content ">
             <ul>
               <li v-for="weight in weights" :key="weight">
-                <a :class="{ 'is-active': weight == valueText }" :data-id="weight" href="#">
+                <a :class="{ 'is-active': isSelected(weight) }" :data-id="weight" href="#">
                   {{ weight }}
                 </a>
               </li>

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) {
-        /* eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings */
-        weight = 'No+Weight';
-      }
+    if (weight === null) {
+      /* eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings */
+      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,19 +24,18 @@ 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?
-        items.where(weight: [-1, nil])
-      elsif filter_by_any_weight?
-        items.where.not(weight: [-1, nil])
+        items.where(weight: nil)
       else
         items.where(weight: params[:weight])
       end
     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

@@ -151,6 +151,7 @@ describe 'Scoped issue boards', :js do
 
       context 'weight' do
         let!(:issue_weight_1) { create(:issue, project: project, weight: 1) }
+        let!(:issue_weight_none) { create(:issue, project: project, weight: nil) }
 
         it 'creates board filtering by weight' do
           create_board_weight(1)
@@ -170,6 +171,12 @@ describe 'Scoped issue boards', :js do
         it 'creates board filtering by "Any" weight' do
           create_board_weight('Any')
 
+          expect(page).to have_selector('.board-card', count: 5)
+        end
+
+        it 'creates board filtering by "None" weight' do
+          create_board_weight('None')
+
           expect(page).to have_selector('.board-card', count: 4)
         end
 

ee/spec/finders/issues_finder_spec.rb

@@ -11,12 +11,13 @@ describe IssuesFinder do
       describe 'filter by weight' do
         set(:issue_with_weight_1) { create(:issue, project: project3, weight: 1) }
         set(:issue_with_weight_42) { create(:issue, project: project3, weight: 42) }
+        set(:issue_with_no_weight) { create(:issue, project: project3, weight: nil) }
 
         context 'filter issues with no weight' do
           let(:params) { { weight: Issue::WEIGHT_NONE } }
 
-          it 'returns all issues' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
+          it 'returns all issues with no weight' do
+            expect(issues).to contain_exactly(issue_with_no_weight, issue1, issue2, issue3, issue4)
           end
         end
 
@@ -24,17 +25,25 @@ describe IssuesFinder do
           let(:params) { { weight: Issue::WEIGHT_ANY } }
 
           it 'returns all issues' do
-            expect(issues).to contain_exactly(issue_with_weight_1, issue_with_weight_42)
+            expect(issues).to contain_exactly(issue_with_weight_1, issue_with_weight_42, issue_with_no_weight, issue1, issue2, issue3, issue4)
           end
         end
 
         context 'filter issues with a specific weight' do
           let(:params) { { weight: 42 } }
 
-          it 'returns all issues' do
+          it 'returns all issues with that weight' 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

@@ -4,7 +4,14 @@ import IssuableContext from '~/issuable_context';
 
 let vm;
 let board;
-const weights = ['Any Weight', 'No Weight', 1, 2, 3];
+
+const expectedDropdownValues = {
+  anyWeight: 'Any',
+  noWeight: 'None',
+};
+
+// see ee/app/views/shared/boards/_switcher.html.haml
+const weights = ['Any', 'None', 0, 1, 2, 3];
 
 function getSelectedText() {
   return vm.$el.querySelector('.value').innerText.trim();
@@ -14,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: [],
     };
 
@@ -43,23 +54,31 @@ describe('WeightSelect', () => {
       expect(getSelectedText()).toBe('Any Weight');
     });
 
-    it('displays Any Weight for value -1', done => {
-      vm.value = -1;
+    it('displays Any Weight for null', done => {
+      vm.value = null;
       Vue.nextTick(() => {
         expect(getSelectedText()).toEqual('Any Weight');
         done();
       });
     });
 
-    it('displays No Weight', done => {
-      vm.value = 0;
+    it('displays No Weight for -1', done => {
+      vm.value = -1;
       Vue.nextTick(() => {
         expect(getSelectedText()).toEqual('No Weight');
         done();
       });
     });
 
-    it('weight 1', done => {
+    it('displays weight for 0', done => {
+      vm.value = 0;
+      Vue.nextTick(() => {
+        expect(getSelectedText()).toEqual('0');
+        done();
+      });
+    });
+
+    it('displays weight for 1', done => {
       vm.value = 1;
       Vue.nextTick(() => {
         expect(getSelectedText()).toEqual('1');
@@ -73,17 +92,17 @@ describe('WeightSelect', () => {
       vm.$el.querySelector('.edit-link').click();
 
       setTimeout(() => {
-        expect(activeDropdownItem()).toEqual('Any Weight');
+        expect(activeDropdownItem()).toEqual(expectedDropdownValues.anyWeight);
         done();
       });
     });
 
     it('shows No Weight', done => {
-      vm.value = 0;
+      vm.value = -1;
       vm.$el.querySelector('.edit-link').click();
 
       setTimeout(() => {
-        expect(activeDropdownItem()).toEqual('No Weight');
+        expect(activeDropdownItem()).toEqual(expectedDropdownValues.noWeight);
         done();
       });
     });
@@ -104,12 +123,12 @@ describe('WeightSelect', () => {
       vm.$el.querySelector('.edit-link').click();
 
       setTimeout(() => {
-        vm.$el.querySelectorAll('li a')[3].click();
+        findDropdownItem('2').click();
       });
 
       setTimeout(() => {
         expect(activeDropdownItem()).toEqual('2');
-        expect(board.weight).toEqual('2');
+        expect(board.weight).toEqual(2);
         done();
       });
     });
@@ -119,12 +138,26 @@ describe('WeightSelect', () => {
       vm.$el.querySelector('.edit-link').click();
 
       setTimeout(() => {
-        vm.$el.querySelectorAll('li a')[0].click();
+        findDropdownItem('Any').click();
       });
 
       setTimeout(() => {
-        expect(activeDropdownItem()).toEqual('Any Weight');
-        expect(board.weight).toEqual(-1);
+        expect(activeDropdownItem()).toEqual(expectedDropdownValues.anyWeight);
+        expect(board.weight).toEqual(null);
+        done();
+      });
+    });
+
+    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();
       });
     });
@@ -134,12 +167,26 @@ describe('WeightSelect', () => {
       vm.$el.querySelector('.edit-link').click();
 
       setTimeout(() => {
-        vm.$el.querySelectorAll('li a')[1].click();
+        findDropdownItem('None').click();
       });
 
       setTimeout(() => {
-        expect(activeDropdownItem()).toEqual('No Weight');
-        expect(board.weight).toEqual(0);
+        expect(activeDropdownItem()).toEqual(expectedDropdownValues.noWeight);
+        expect(board.weight).toEqual(-1);
+        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/requests/api/issues_spec.rb

@@ -57,7 +57,7 @@ describe API::Issues, :mailer do
         it 'returns issues with any weight' do
           get api('/issues', user), params: { weight: 'Any', scope: 'all' }
 
-          expect_paginated_array_response([issue3.id, issue2.id, issue1.id])
+          expect_paginated_array_response([issue.id, issue3.id, issue2.id, issue1.id])
         end
       end
 

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
 

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