diff --git a/changelogs/unreleased/4221-board-scope-shouldn-t-persist-1-when-one-selects-any-weight-or-any-milestone.yml b/changelogs/unreleased/4221-board-scope-shouldn-t-persist-1-when-one-selects-any-weight-or-any-milestone.yml deleted file mode 100644 index 54b47e19a2dae07a5e3556487119d9ec9eaa991b..0000000000000000000000000000000000000000 --- a/changelogs/unreleased/4221-board-scope-shouldn-t-persist-1-when-one-selects-any-weight-or-any-milestone.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Properly persist -1 and null for board weight -merge_request: -author: -type: changed diff --git a/db/migrate/20190703001116_default_weight_to_nil.rb b/db/migrate/20190703001116_default_weight_to_nil.rb deleted file mode 100644 index 89e7f089468926aa1d549d6dea837355b2c342e3..0000000000000000000000000000000000000000 --- a/db/migrate/20190703001116_default_weight_to_nil.rb +++ /dev/null @@ -1,24 +0,0 @@ -# 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 diff --git a/ee/app/assets/javascripts/boards/components/weight_select.vue b/ee/app/assets/javascripts/boards/components/weight_select.vue index 99441eab387ce38a4d65bf4677a452109626499f..2644755b98bddd357b3acde0afecacc17cb67cf1 100644 --- a/ee/app/assets/javascripts/boards/components/weight_select.vue +++ b/ee/app/assets/javascripts/boards/components/weight_select.vue @@ -1,59 +1,11 @@ <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> diff --git a/ee/app/assets/javascripts/boards/stores/boards_store_ee.js b/ee/app/assets/javascripts/boards/stores/boards_store_ee.js index 4cacdae816320e3da66b4254ce6a4a37d3e20fbb..9964517b8b30d0611d9fbc51fe469a315ff2092f 100644 --- a/ee/app/assets/javascripts/boards/stores/boards_store_ee.js +++ b/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'); diff --git a/ee/app/assets/javascripts/weight_select.js b/ee/app/assets/javascripts/weight_select.js index 639af68aa252f09e1cb74c4b3779a865fcc64dc0..3e669d822f4df30a31627a5ea8b4a2a434e1b90f 100644 --- a/ee/app/assets/javascripts/weight_select.js +++ b/ee/app/assets/javascripts/weight_select.js @@ -1,4 +1,4 @@ -/* 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(); diff --git a/ee/app/finders/ee/issues_finder.rb b/ee/app/finders/ee/issues_finder.rb index c5d090b04088523e1d5fbde90aa5e6395cfae8c0..2cf8636d990d2c10dd1653b17288acbb966e43be 100644 --- a/ee/app/finders/ee/issues_finder.rb +++ b/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 diff --git a/ee/app/models/ee/board.rb b/ee/app/models/ee/board.rb index 4a0f192bd4cb1da2c5925c28f1c3e5fa291a444b..87881e04206f5b78a3f39e9f93d5b891a964e3b8 100644 --- a/ee/app/models/ee/board.rb +++ b/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 diff --git a/ee/spec/features/boards/scoped_issue_board_spec.rb b/ee/spec/features/boards/scoped_issue_board_spec.rb index 263973029c6a3b2d52582bb974e4be07d566b5cc..325aaa0a36e69f2eb27b013db437e3d04fd60bba 100644 --- a/ee/spec/features/boards/scoped_issue_board_spec.rb +++ b/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 diff --git a/ee/spec/finders/issues_finder_spec.rb b/ee/spec/finders/issues_finder_spec.rb index 80c3f7b0f40628c32e5e691bcb41d0a21f646d50..b80797c17237ecb841efde0bd2f3592d7eb4efdd 100644 --- a/ee/spec/finders/issues_finder_spec.rb +++ b/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 diff --git a/ee/spec/javascripts/boards/components/weight_select_spec.js b/ee/spec/javascripts/boards/components/weight_select_spec.js index 9e6cad0e35449658453ee006767032f2570709f5..4270e9a214aff16d784ccfdfa4ba9a3250dd130d 100644 --- a/ee/spec/javascripts/boards/components/weight_select_spec.js +++ b/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(); }); }); diff --git a/ee/spec/models/board_spec.rb b/ee/spec/models/board_spec.rb index 309a1bdbda8217f28ef743b30e1d3c885dd0e8d9..878ea6d3bbe29efb9bb04f80b265983258acdc60 100644 --- a/ee/spec/models/board_spec.rb +++ b/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') diff --git a/ee/spec/requests/api/issues_spec.rb b/ee/spec/requests/api/issues_spec.rb index ba9514e83b3c0fb68168da80fa0944d3552eea12..449a7371b319954e3544bfde7d8330b08be8077c 100644 --- a/ee/spec/requests/api/issues_spec.rb +++ b/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 diff --git a/ee/spec/services/ee/boards/issues/create_service_spec.rb b/ee/spec/services/ee/boards/issues/create_service_spec.rb index 11050fcce709d3fa729213c9ed6739bb1330f8be..de5c70304924a16940b5531eb71e447c30e8d8aa 100644 --- a/ee/spec/services/ee/boards/issues/create_service_spec.rb +++ b/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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0081dbc3dad07c827784dcfd0143fd7b737a9351..21e4876eb07d6361e2234e9269387104f67a1928 100644 --- a/locale/gitlab.pot +++ b/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 ""