Merge branch 'revert-d58679e2' into 'master'

Revert weight filter behavior for "any"

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

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>
-import _ from 'underscore';
+/* eslint-disable vue/require-default-prop */
+
 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 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,
-      };
-  }
-}
+const ANY_WEIGHT = 'Any Weight';
+const NO_WEIGHT = 'No Weight';
 
 export default {
   components: {
@@ -67,7 +19,6 @@ export default {
     value: {
       type: [Number, String],
       required: false,
-      default: ANY_WEIGHT.value,
     },
     canEdit: {
       type: Boolean,
@@ -85,15 +36,16 @@ export default {
     };
   },
   computed: {
-    selectedWeight() {
-      return getWeightFromValue(this.value);
-    },
-
     valueClass() {
-      return this.selectedWeight.valueClass || 'bold';
+      if (this.valueText === ANY_WEIGHT) {
+        return 'text-secondary';
+      }
+      return 'bold';
     },
     valueText() {
-      return this.selectedWeight.label;
+      if (this.value > 0) return this.value;
+      if (this.value === 0) return NO_WEIGHT;
+      return ANY_WEIGHT;
     },
   },
   mounted() {
@@ -104,11 +56,17 @@ export default {
     });
   },
   methods: {
-    isSelected(weight) {
-      return this.selectedWeight.selectValue === weight;
-    },
     selectWeight(weight) {
-      this.board.weight = getWeightValueFromSelect(weight);
+      this.board.weight = this.weightInt(weight);
+    },
+    weightInt(weight) {
+      if (weight > 0) {
+        return weight;
+      }
+      if (weight === NO_WEIGHT) {
+        return 0;
+      }
+      return -1;
     },
   },
 };
@@ -140,7 +98,7 @@ export default {
           <div class="dropdown-content ">
             <ul>
               <li v-for="weight in weights" :key="weight">
-                <a :class="{ 'is-active': isSelected(weight) }" :data-id="weight" href="#">
+                <a :class="{ 'is-active': weight == valueText }" :data-id="weight" href="#">
                   {{ weight }}
                 </a>
               </li>

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

@@ -110,20 +110,13 @@ class BoardsStoreEE {
     }
 
     let { weight } = this.store.boardConfig;
-    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) {
+    if (weight !== -1) {
+      if (weight === 0) {
+        /* eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings */
+        weight = 'No+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-unused-vars, no-else-return, func-names */
+/* eslint-disable prefer-arrow-callback, one-var, no-var, object-shorthand, no-shadow, no-unused-vars, no-else-return, func-names */
 
 import $ from 'jquery';
 import '~/gl_dropdown';
@@ -49,11 +49,13 @@ function WeightSelect(els, options = {}) {
         }
       },
       clicked: function(glDropdownEvt) {
-        const { e, $el } = glDropdownEvt;
-        const selected = $el.data('id');
+        const { e } = glDropdownEvt;
+        let selected = glDropdownEvt.selectedObj;
+        const inputField = $dropdown.closest('.selectbox').find(`input[name='${fieldName}']`);
 
         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,18 +24,19 @@ module EE
 
     # rubocop: disable CodeReuse/ActiveRecord
     def by_weight(items)
-      return items unless filtered_by_weight?
-      return items if filter_by_any_weight?
+      return items unless weights?
 
       if filter_by_no_weight?
-        items.where(weight: nil)
+        items.where(weight: [-1, nil])
+      elsif filter_by_any_weight?
+        items.where.not(weight: [-1, nil])
       else
         items.where(weight: params[:weight])
       end
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
-    def filtered_by_weight?
+    def weights?
       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].freeze
+    EMPTY_SCOPE_STATE = [nil, -1].freeze
 
     prepended do
       belongs_to :milestone

ee/spec/features/boards/scoped_issue_board_spec.rb

@@ -151,7 +151,6 @@ 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)
@@ -171,12 +170,6 @@ 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,13 +11,12 @@ 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 with no weight' do
-            expect(issues).to contain_exactly(issue_with_no_weight, issue1, issue2, issue3, issue4)
+          it 'returns all issues' do
+            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
           end
         end
 
@@ -25,25 +24,17 @@ 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, issue_with_no_weight, issue1, issue2, issue3, issue4)
+            expect(issues).to contain_exactly(issue_with_weight_1, issue_with_weight_42)
           end
         end
 
         context 'filter issues with a specific weight' do
           let(:params) { { weight: 42 } }
 
-          it 'returns all issues with that weight' do
+          it 'returns all issues' 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,14 +4,7 @@ import IssuableContext from '~/issuable_context';
 
 let vm;
 let board;
-
-const expectedDropdownValues = {
-  anyWeight: 'Any',
-  noWeight: 'None',
-};
-
-// see ee/app/views/shared/boards/_switcher.html.haml
-const weights = ['Any', 'None', 0, 1, 2, 3];
+const weights = ['Any Weight', 'No Weight', 1, 2, 3];
 
 function getSelectedText() {
   return vm.$el.querySelector('.value').innerText.trim();
@@ -21,16 +14,12 @@ 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: -1,
+      weight: 0,
       labels: [],
     };
 
@@ -54,31 +43,23 @@ describe('WeightSelect', () => {
       expect(getSelectedText()).toBe('Any Weight');
     });
 
-    it('displays Any Weight for null', done => {
-      vm.value = null;
-      Vue.nextTick(() => {
-        expect(getSelectedText()).toEqual('Any Weight');
-        done();
-      });
-    });
-
-    it('displays No Weight for -1', done => {
+    it('displays Any Weight for value -1', done => {
       vm.value = -1;
       Vue.nextTick(() => {
-        expect(getSelectedText()).toEqual('No Weight');
+        expect(getSelectedText()).toEqual('Any Weight');
         done();
       });
     });
 
-    it('displays weight for 0', done => {
+    it('displays No Weight', done => {
       vm.value = 0;
       Vue.nextTick(() => {
-        expect(getSelectedText()).toEqual('0');
+        expect(getSelectedText()).toEqual('No Weight');
         done();
       });
     });
 
-    it('displays weight for 1', done => {
+    it('weight 1', done => {
       vm.value = 1;
       Vue.nextTick(() => {
         expect(getSelectedText()).toEqual('1');
@@ -92,17 +73,17 @@ describe('WeightSelect', () => {
       vm.$el.querySelector('.edit-link').click();
 
       setTimeout(() => {
-        expect(activeDropdownItem()).toEqual(expectedDropdownValues.anyWeight);
+        expect(activeDropdownItem()).toEqual('Any Weight');
         done();
       });
     });
 
     it('shows No Weight', done => {
-      vm.value = -1;
+      vm.value = 0;
       vm.$el.querySelector('.edit-link').click();
 
       setTimeout(() => {
-        expect(activeDropdownItem()).toEqual(expectedDropdownValues.noWeight);
+        expect(activeDropdownItem()).toEqual('No Weight');
         done();
       });
     });
@@ -123,12 +104,12 @@ describe('WeightSelect', () => {
       vm.$el.querySelector('.edit-link').click();
 
       setTimeout(() => {
-        findDropdownItem('2').click();
+        vm.$el.querySelectorAll('li a')[3].click();
       });
 
       setTimeout(() => {
         expect(activeDropdownItem()).toEqual('2');
-        expect(board.weight).toEqual(2);
+        expect(board.weight).toEqual('2');
         done();
       });
     });
@@ -138,26 +119,12 @@ describe('WeightSelect', () => {
       vm.$el.querySelector('.edit-link').click();
 
       setTimeout(() => {
-        findDropdownItem('Any').click();
+        vm.$el.querySelectorAll('li a')[0].click();
       });
 
       setTimeout(() => {
-        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);
+        expect(activeDropdownItem()).toEqual('Any Weight');
+        expect(board.weight).toEqual(-1);
         done();
       });
     });
@@ -167,26 +134,12 @@ describe('WeightSelect', () => {
       vm.$el.querySelector('.edit-link').click();
 
       setTimeout(() => {
-        findDropdownItem('None').click();
+        vm.$el.querySelectorAll('li a')[1].click();
       });
 
       setTimeout(() => {
-        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);
+        expect(activeDropdownItem()).toEqual('No Weight');
+        expect(board.weight).toEqual(0);
         done();
       });
     });

ee/spec/models/board_spec.rb

@@ -92,37 +92,19 @@ describe Board do
       stub_licensed_features(scoped_issue_board: true)
     end
 
-    it 'returns true when milestone is not "Any milestone" AND is not "No milestone"' do
+    it 'returns true when milestone is not nil' do
       milestone = create(:milestone)
       board = create(:board, milestone: milestone, weight: nil, labels: [], assignee: nil)
 
       expect(board).to be_scoped
     end
 
-    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
+    it 'returns true when weight is not nil' 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([issue.id, issue3.id, issue2.id, issue1.id])
+          expect_paginated_array_response([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 'none'" do
+        context 'when board weight is invalid' do
           it 'creates issue with nil weight' do
             board.update(weight: -1)
 
@@ -65,17 +65,6 @@ 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

@@ -1528,9 +1528,6 @@ msgstr ""
 msgid "Any Milestone"
 msgstr ""
 
-msgid "Any Weight"
-msgstr ""
-
 msgid "Any encrypted tokens"
 msgstr ""
 
@@ -9560,9 +9557,6 @@ msgstr ""
 msgid "No Tag"
 msgstr ""
 
-msgid "No Weight"
-msgstr ""
-
 msgid "No activities found"
 msgstr ""