Address reviewer comments

- Use BackgroundMigration
- use say in migration instead of logger
- make update query more concise
- make `by_milestone` method more concise

3 files changed, 15 insertions, 21 deletions

diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 6c8db32e7c3..c99cf9a4bb7 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -485,22 +485,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: nil)
-      elsif filter_by_any_milestone?
-        items
-      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
 
diff --git a/db/migrate/20190703001120_default_milestone_to_nil.rb b/db/migrate/20190703001120_default_milestone_to_nil.rb
index a4a2f88fd03..f0d2c6b19ee 100644
--- a/db/migrate/20190703001120_default_milestone_to_nil.rb
+++ b/db/migrate/20190703001120_default_milestone_to_nil.rb
@@ -1,5 +1,4 @@
 # frozen_string_literal: true
-# rubocop:disable Style/Documentation
 
 class DefaultMilestoneToNil < ActiveRecord::Migration[5.1]
   include Gitlab::Database::MigrationHelpers
@@ -20,6 +19,8 @@ class DefaultMilestoneToNil < ActiveRecord::Migration[5.1]
     Board.where(milestone_id: -1).each_batch(of: 50) do |batch|
       range = batch.pluck('MIN(id)', 'MAX(id)').first
 
+      say "Setting board weights from None to Any: #{range[0]} - #{range[1]}"
+
       Gitlab::BackgroundMigration::UpdateBoardMilestonesFromNoneToAny.new.perform(*range)
     end
   end
diff --git a/lib/gitlab/background_migration/update_board_milestones_from_none_to_any.rb b/lib/gitlab/background_migration/update_board_milestones_from_none_to_any.rb
index 27da8a5f23b..a9ca776af1e 100644
--- a/lib/gitlab/background_migration/update_board_milestones_from_none_to_any.rb
+++ b/lib/gitlab/background_migration/update_board_milestones_from_none_to_any.rb
@@ -9,11 +9,7 @@ module Gitlab
       end
 
       def perform(start_id, stop_id)
-        Rails.logger.info("Setting board milestones from None to Any: #{start_id} - #{stop_id}") # rubocop:disable Gitlab/RailsLogger
-
-        update = 'UPDATE boards SET milestone_id = null'
-
-        Board.where(id: start_id..stop_id).update_all(update)
+        Board.where(id: start_id..stop_id).update_all(milestone_id: nil)
       end
     end
   end