Merge branch '4221-board-milestone-should-persist-any-none-properly' into 'master'

For milestone filters, treat `Any` as `No filter`

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

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 ? '' : null;
+      selectedMilestoneDefault = showAny ? __('Any Milestone') : null;
       selectedMilestoneDefault =
         showNo && defaultNo ? __('No Milestone') : selectedMilestoneDefault;
       selectedMilestone = $dropdown.data('selected') || selectedMilestoneDefault;
@@ -74,14 +74,16 @@ export default class MilestoneSelect {
             if (showAny) {
               extraOptions.push({
                 id: null,
-                name: null,
+                // eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings
+                name: 'Any',
                 title: __('Any Milestone'),
               });
             }
             if (showNo) {
               extraOptions.push({
                 id: -1,
-                name: __('No Milestone'),
+                // eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings
+                name: 'None',
                 title: __('No Milestone'),
               });
             }

app/finders/issuable_finder.rb

@@ -484,22 +484,19 @@ class IssuableFinder
 
   # rubocop: disable CodeReuse/ActiveRecord
   def by_milestone(items)
-    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
+    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])
     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', 0)
-  Any = MilestoneStruct.new('Any Milestone', '', -1)
+  None = MilestoneStruct.new('No Milestone', 'No Milestone', -1)
+  Any = MilestoneStruct.new('Any Milestone', '', nil)
   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

 <script>
 import MilestoneSelect from '~/milestone_select';
 import { GlLoadingIcon } from '@gitlab/ui';
+import { __ } from '~/locale';
 
-const ANY_MILESTONE = 'Any Milestone';
-const NO_MILESTONE = 'No Milestone';
+const ANY_MILESTONE = {
+  title: __('Any Milestone'),
+  titleClass: 'text-secondary',
+  name: 'Any',
+  id: null,
+};
+
+const NO_MILESTONE = {
+  title: __('No Milestone'),
+  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;
+  }
+}
 
 export default {
   components: {
@@ -26,22 +54,27 @@ export default {
   },
 
   computed: {
-    milestoneTitle() {
-      if (this.noMilestone) return NO_MILESTONE;
-      return this.board.milestone ? this.board.milestone.title : ANY_MILESTONE;
+    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;
+      }
     },
-    noMilestone() {
-      return this.milestoneId === 0;
+    milestoneTitle() {
+      return this.milestone.title;
     },
     milestoneId() {
       return this.board.milestone_id;
     },
     milestoneTitleClass() {
-      return this.milestoneTitle === ANY_MILESTONE ? 'text-secondary' : 'bold';
+      return this.milestone.titleClass || DEFAULT_MILESTONE.titleClass;
     },
     selected() {
-      if (this.noMilestone) return NO_MILESTONE;
-      return this.board.milestone ? this.board.milestone.name : '';
+      return this.milestone.name;
     },
   },
   mounted() {
@@ -51,13 +84,7 @@ export default {
   },
   methods: {
     selectMilestone(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;
-      }
+      const id = getMilestoneIdFromTitle(milestone);
       this.board.milestone_id = id;
       this.board.milestone = {
         ...milestone,

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

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

ee/spec/javascripts/boards/milestone_select_spec.js

@@ -17,6 +17,10 @@ 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',
@@ -39,7 +43,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,
       },
@@ -72,7 +76,7 @@ describe('Milestone select component', () => {
     });
 
     it('shows No Milestone', done => {
-      vm.board.milestone_id = 0;
+      vm.board.milestone_id = -1;
       Vue.nextTick(() => {
         expect(selectedText()).toContain('No Milestone');
         done();
@@ -104,11 +108,11 @@ describe('Milestone select component', () => {
       });
 
       it('sets Any Milestone', done => {
-        vm.board.milestone_id = 0;
+        vm.board.milestone_id = -1;
         vm.$el.querySelector('.edit-link').click();
 
         setTimeout(() => {
-          vm.$el.querySelectorAll('li a')[0].click();
+          findDropdownItem('Any Milestone').click();
         });
 
         setTimeout(() => {
@@ -122,7 +126,7 @@ describe('Milestone select component', () => {
         vm.$el.querySelector('.edit-link').click();
 
         setTimeout(() => {
-          vm.$el.querySelectorAll('li a')[1].click();
+          findDropdownItem('No Milestone').click();
         });
 
         setTimeout(() => {
@@ -136,7 +140,7 @@ describe('Milestone select component', () => {
         vm.$el.querySelector('.edit-link').click();
 
         setTimeout(() => {
-          vm.$el.querySelectorAll('li a')[4].click();
+          findDropdownItem('first milestone').click();
         });
 
         setTimeout(() => {

ee/spec/models/board_spec.rb

@@ -59,10 +59,23 @@ 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
@@ -79,14 +92,14 @@ describe Board do
       stub_licensed_features(scoped_issue_board: true)
     end
 
-    it 'returns true when milestone is not nil AND is not "Any 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 weight is not nil AND is not "Any 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
@@ -113,7 +126,7 @@ describe Board do
     end
 
     it 'returns false when board is not scoped' do
-      board = create(:board, milestone_id: -1, weight: -1, labels: [], assignee: nil)
+      board = create(:board, milestone_id: nil, weight: nil, 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)
+          expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
         end
 
         it 'returns issues with any assigned milestone (deprecated)' do
           params[:milestone_title] = Milestone::Any.title
 
-          expect(issues).to contain_exactly(issue1)
+          expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
         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, closed_issue.id])
+      expect_paginated_array_response([issue.id, confidential_issue.id, closed_issue.id])
     end
 
     context 'without sort params' do