Merge branch 'revert-c29e7010' into 'master'

Revert milestone filter behavior for "any"

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

app/assets/javascripts/milestone_select.js

@@ -55,7 +55,7 @@ export default class MilestoneSelect {
       const $sidebarCollapsedValue = $block.find('.sidebar-collapsed-icon');
       const $value = $block.find('.value');
       const $loading = $block.find('.block-loading').fadeOut();
-      selectedMilestoneDefault = showAny ? __('Any Milestone') : null;
+      selectedMilestoneDefault = showAny ? '' : null;
       selectedMilestoneDefault =
         showNo && defaultNo ? __('No Milestone') : selectedMilestoneDefault;
       selectedMilestone = $dropdown.data('selected') || selectedMilestoneDefault;
@@ -74,16 +74,14 @@ export default class MilestoneSelect {
             if (showAny) {
               extraOptions.push({
                 id: null,
-                // eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings
-                name: 'Any',
+                name: null,
                 title: __('Any Milestone'),
               });
             }
             if (showNo) {
               extraOptions.push({
                 id: -1,
-                // eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings
-                name: 'None',
+                name: __('No Milestone'),
                 title: __('No Milestone'),
               });
             }

app/finders/issuable_finder.rb

@@ -484,19 +484,22 @@ class IssuableFinder
 
   # rubocop: disable CodeReuse/ActiveRecord
   def by_milestone(items)
-    return items unless milestones?
-    return items if filter_by_any_milestone?
-
-    if filter_by_no_milestone?
-      items.left_joins_milestones.where(milestone_id: nil)
-    elsif filter_by_upcoming_milestone?
-      upcoming_ids = Milestone.upcoming_ids(projects, related_groups)
-      items.left_joins_milestones.where(milestone_id: upcoming_ids)
-    elsif filter_by_started_milestone?
-      items.left_joins_milestones.merge(Milestone.started)
-    else
-      items.with_milestone(params[:milestone_title])
+    if milestones?
+      if filter_by_no_milestone?
+        items = items.left_joins_milestones.where(milestone_id: [-1, nil])
+      elsif filter_by_any_milestone?
+        items = items.any_milestone
+      elsif filter_by_upcoming_milestone?
+        upcoming_ids = Milestone.upcoming_ids(projects, related_groups)
+        items = items.left_joins_milestones.where(milestone_id: upcoming_ids)
+      elsif filter_by_started_milestone?
+        items = items.left_joins_milestones.merge(Milestone.started)
+      else
+        items = items.with_milestone(params[:milestone_title])
+      end
     end
+
+    items
   end
   # rubocop: enable CodeReuse/ActiveRecord
 

app/models/milestone.rb

@@ -4,8 +4,8 @@ class Milestone < ApplicationRecord
   # Represents a "No Milestone" state used for filtering Issues and Merge
   # Requests that have no milestone assigned.
   MilestoneStruct = Struct.new(:title, :name, :id)
-  None = MilestoneStruct.new('No Milestone', 'No Milestone', -1)
-  Any = MilestoneStruct.new('Any Milestone', '', nil)
+  None = MilestoneStruct.new('No Milestone', 'No Milestone', 0)
+  Any = MilestoneStruct.new('Any Milestone', '', -1)
   Upcoming = MilestoneStruct.new('Upcoming', '#upcoming', -2)
   Started = MilestoneStruct.new('Started', '#started', -3)
 

changelogs/unreleased/4221-board-milestone-should-persist-any-none-properly.yml

----
-title: For milestone filters, treat Any as No Filter (using null). Use -1 for No Milestone
-merge_request:
-author:
-type: changed

db/migrate/20190703001120_default_milestone_to_nil.rb

-# frozen_string_literal: true
-
-class DefaultMilestoneToNil < ActiveRecord::Migration[5.1]
-  DOWNTIME = false
-
-  def up
-    execute(update_board_milestones_query)
-  end
-
-  def down
-    # no-op
-  end
-
-  private
-
-  # Only 105 records to update, as of 2019/07/18
-  def update_board_milestones_query
-    <<~HEREDOC
-   UPDATE boards
-     SET milestone_id = NULL
-   WHERE boards.milestone_id = -1
-    HEREDOC
-  end
-end

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

@@ -2,39 +2,9 @@
 /* eslint-disable @gitlab/vue-i18n/no-bare-strings */
 import MilestoneSelect from '~/milestone_select';
 import { GlLoadingIcon } from '@gitlab/ui';
-import { __ } from '~/locale';
 
-const ANY_MILESTONE = {
-  title: __('Any Milestone'),
-  titleClass: 'text-secondary',
-  // eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings
-  name: 'Any',
-  id: null,
-};
-
-const NO_MILESTONE = {
-  title: __('No Milestone'),
-  // eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings
-  name: 'None',
-  id: -1,
-};
-
-const DEFAULT_MILESTONE = {
-  title: ANY_MILESTONE.title,
-  titleClass: 'bold',
-  name: '',
-};
-
-function getMilestoneIdFromTitle({ title, id }) {
-  switch (title) {
-    case ANY_MILESTONE.title:
-      return ANY_MILESTONE.id;
-    case NO_MILESTONE.title:
-      return NO_MILESTONE.id;
-    default:
-      return id;
-  }
-}
+const ANY_MILESTONE = 'Any Milestone';
+const NO_MILESTONE = 'No Milestone';
 
 export default {
   components: {
@@ -57,27 +27,22 @@ export default {
   },
 
   computed: {
-    milestone() {
-      switch (this.milestoneId) {
-        case NO_MILESTONE.id:
-          return NO_MILESTONE;
-        case ANY_MILESTONE.id:
-          return ANY_MILESTONE;
-        default:
-          return this.board.milestone || DEFAULT_MILESTONE;
-      }
-    },
     milestoneTitle() {
-      return this.milestone.title;
+      if (this.noMilestone) return NO_MILESTONE;
+      return this.board.milestone ? this.board.milestone.title : ANY_MILESTONE;
+    },
+    noMilestone() {
+      return this.milestoneId === 0;
     },
     milestoneId() {
       return this.board.milestone_id;
     },
     milestoneTitleClass() {
-      return this.milestone.titleClass || DEFAULT_MILESTONE.titleClass;
+      return this.milestoneTitle === ANY_MILESTONE ? 'text-secondary' : 'bold';
     },
     selected() {
-      return this.milestone.name;
+      if (this.noMilestone) return NO_MILESTONE;
+      return this.board.milestone ? this.board.milestone.name : '';
     },
   },
   mounted() {
@@ -87,7 +52,13 @@ export default {
   },
   methods: {
     selectMilestone(milestone) {
-      const id = getMilestoneIdFromTitle(milestone);
+      let { id } = milestone;
+      // swap the IDs of 'Any' and 'No' milestone to what backend requires
+      if (milestone.title === ANY_MILESTONE) {
+        id = -1;
+      } else if (milestone.title === NO_MILESTONE) {
+        id = 0;
+      }
       this.board.milestone_id = id;
       this.board.milestone = {
         ...milestone,

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

@@ -95,12 +95,9 @@ class BoardsStoreEE {
     };
 
     let { milestoneTitle } = this.store.boardConfig;
-    if (this.store.boardConfig.milestoneId === -1) {
+    if (this.store.boardConfig.milestoneId === 0) {
       /* eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings */
-      milestoneTitle = 'None';
-    } else if (this.store.boardConfig.milestoneId === null) {
-      /* eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings */
-      milestoneTitle = 'Any';
+      milestoneTitle = 'No+Milestone';
     } else {
       milestoneTitle = encodeURIComponent(milestoneTitle);
     }

ee/spec/javascripts/boards/milestone_select_spec.js

@@ -17,10 +17,6 @@ function activeDropdownItem(index) {
   return items[index].innerText.trim();
 }
 
-function findDropdownItem(text) {
-  return Array.from(vm.$el.querySelectorAll('li a')).find(({ innerText }) => innerText === text);
-}
-
 const milestone = {
   id: 1,
   title: 'first milestone',
@@ -43,7 +39,7 @@ describe('Milestone select component', () => {
     const Component = Vue.extend(MilestoneSelect);
     vm = new Component({
       propsData: {
-        board: { ...boardObj },
+        board: boardObj,
         milestonePath: '/test/issue-boards/milestones.json',
         canEdit: true,
       },
@@ -76,7 +72,7 @@ describe('Milestone select component', () => {
     });
 
     it('shows No Milestone', done => {
-      vm.board.milestone_id = -1;
+      vm.board.milestone_id = 0;
       Vue.nextTick(() => {
         expect(selectedText()).toContain('No Milestone');
         done();
@@ -108,11 +104,11 @@ describe('Milestone select component', () => {
       });
 
       it('sets Any Milestone', done => {
-        vm.board.milestone_id = -1;
+        vm.board.milestone_id = 0;
         vm.$el.querySelector('.edit-link').click();
 
         setTimeout(() => {
-          findDropdownItem('Any Milestone').click();
+          vm.$el.querySelectorAll('li a')[0].click();
         });
 
         setTimeout(() => {
@@ -126,7 +122,7 @@ describe('Milestone select component', () => {
         vm.$el.querySelector('.edit-link').click();
 
         setTimeout(() => {
-          findDropdownItem('No Milestone').click();
+          vm.$el.querySelectorAll('li a')[1].click();
         });
 
         setTimeout(() => {
@@ -140,7 +136,7 @@ describe('Milestone select component', () => {
         vm.$el.querySelector('.edit-link').click();
 
         setTimeout(() => {
-          findDropdownItem('first milestone').click();
+          vm.$el.querySelectorAll('li a')[4].click();
         });
 
         setTimeout(() => {

ee/spec/models/board_spec.rb

@@ -59,23 +59,10 @@ describe Board do
       end
 
       it 'returns nil for invalid milestone id' do
-        nonsense_board_weight = -6
-        board.milestone_id = nonsense_board_weight
-
-        expect(board.milestone).to be_nil
-      end
-
-      it "returns nil for 'No milestone' value" do
         board.milestone_id = -1
 
         expect(board.milestone).to be_nil
       end
-
-      it "returns nil for 'Any milestone' value" do
-        board.milestone_id = nil
-
-        expect(board.milestone).to be_nil
-      end
     end
 
     it 'returns nil when the feature is not available' do
@@ -92,14 +79,14 @@ 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 nil AND is not "Any 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 weight is not nil AND is not "Any weight"' do
       board = create(:board, milestone: nil, weight: 2, labels: [], assignee: nil)
 
       expect(board).to be_scoped
@@ -126,7 +113,7 @@ describe Board do
     end
 
     it 'returns false when board is not scoped' do
-      board = create(:board, milestone_id: nil, weight: nil, labels: [], assignee: nil)
+      board = create(:board, milestone_id: -1, weight: -1, labels: [], assignee: nil)
 
       expect(board).not_to be_scoped
     end

spec/finders/issues_finder_spec.rb

@@ -113,13 +113,13 @@ describe IssuesFinder do
         let(:params) { { milestone_title: 'Any' } }
 
         it 'returns issues with any assigned milestone' do
-          expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
+          expect(issues).to contain_exactly(issue1)
         end
 
         it 'returns issues with any assigned milestone (deprecated)' do
           params[:milestone_title] = Milestone::Any.title
 
-          expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
+          expect(issues).to contain_exactly(issue1)
         end
       end
 

spec/requests/api/issues/get_project_issues_spec.rb

@@ -389,7 +389,7 @@ describe API::Issues do
     it 'returns an array of issues with any milestone' do
       get api("#{base_url}/issues", user), params: { milestone: any_milestone_title }
 
-      expect_paginated_array_response([issue.id, confidential_issue.id, closed_issue.id])
+      expect_paginated_array_response([issue.id, closed_issue.id])
     end
 
     context 'without sort params' do