Refactor Milestone.for_projects_and_groups

Refactored to use Rails 5 ActiveRecord `or` so that it can handle ids,
objects, array of objects, or relations properly.

app/finders/issuable_finder.rb

@@ -155,7 +155,7 @@ class IssuableFinder
     elsif group
       [group]
     elsif current_user
-      Gitlab::GroupHierarchy.new(current_user.authorized_groups, current_user.groups).all_groups
+      Gitlab::ObjectHierarchy.new(current_user.authorized_groups, current_user.groups).all_objects
     else
       []
     end

app/models/milestone.rb

@@ -38,13 +38,14 @@ class Milestone < ActiveRecord::Base
   scope :closed, -> { with_state(:closed) }
   scope :for_projects, -> { where(group: nil).includes(:project) }
-  scope :for_projects_and_groups, -> (project_ids, group_ids) do
-    conditions = []
-    conditions << arel_table[:project_id].in(project_ids) if project_ids&.compact&.any?
-    conditions << arel_table[:group_id].in(group_ids) if group_ids&.compact&.any?
-    where(conditions.reduce(:or))
+  scope :for_projects_and_groups, -> (projects, groups) do
+    projects = projects.compact if projects.is_a? Array
+    projects = [] if projects.nil?
+    groups = groups.compact if groups.is_a? Array
+    groups = [] if groups.nil?
+    where(project: projects).or(where(group: groups))
   end
   scope :order_by_name_asc, -> { order(Arel::Nodes::Ascending.new(arel_table[:title].lower)) }
@@ -132,7 +133,7 @@ class Milestone < ActiveRecord::Base
   def self.upcoming_ids(projects, groups)
     rel = unscoped
-            .for_projects_and_groups(projects&.map(&:id), groups&.map(&:id))
+            .for_projects_and_groups(projects, groups)
             .active.where('milestones.due_date > NOW()')
     if Gitlab::Database.postgresql?

spec/models/milestone_spec.rb

@@ -240,6 +240,72 @@ describe Milestone do
     end
   end
+  describe '#for_projects_and_groups' do
+    let(:project) { create(:project) }
+    let(:project_other) { create(:project) }
+    let(:group) { create(:group) }
+    let(:group_other) { create(:group) }
+
+    before do
+      create(:milestone, project: project)
+      create(:milestone, project: project_other)
+      create(:milestone, group: group)
+      create(:milestone, group: group_other)
+    end
+
+    subject { described_class.for_projects_and_groups(projects, groups) }
+
+    shared_examples 'filters by projects and groups' do
+      it 'returns milestones filtered by project' do
+        milestones = described_class.for_projects_and_groups(projects, [])
+
+        expect(milestones.count).to eq(1)
+        expect(milestones.first.project_id).to eq(project.id)
+      end
+
+      it 'returns milestones filtered by group' do
+        milestones = described_class.for_projects_and_groups([], groups)
+
+        expect(milestones.count).to eq(1)
+        expect(milestones.first.group_id).to eq(group.id)
+      end
+
+      it 'returns milestones filtered by both project and group' do
+        milestones = described_class.for_projects_and_groups(projects, groups)
+
+        expect(milestones.count).to eq(2)
+        expect(milestones).to contain_exactly(project.milestones.first, group.milestones.first)
+      end
+    end
+
+    context 'ids as params' do
+      let(:projects) { [project.id] }
+      let(:groups) { [group.id] }
+
+      it_behaves_like 'filters by projects and groups'
+    end
+
+    context 'relations as params' do
+      let(:projects) { Project.where(id: project.id) }
+      let(:groups) { Group.where(id: group.id) }
+
+      it_behaves_like 'filters by projects and groups'
+    end
+
+    context 'objects as params' do
+      let(:projects) { [project] }
+      let(:groups) { [group] }
+
+      it_behaves_like 'filters by projects and groups'
+    end
+
+    it 'returns no records if projects and groups are nil' do
+      milestones = described_class.for_projects_and_groups(nil, nil)
+
+      expect(milestones).to be_empty
+    end
+  end
+
   describe '.upcoming_ids' do
     let(:group_1) { create(:group) }
     let(:group_2) { create(:group) }
@@ -271,9 +337,7 @@ describe Milestone do
     let!(:past_milestone_project_3) { create(:milestone, project: project_3, due_date: Time.now - 1.day) }
-    # The call to `#try` is because this returns a relation with a Postgres DB,
-    # and an array of IDs with a MySQL DB.
-    let(:milestone_ids) { described_class.upcoming_ids(projects, groups).map { |id| id.try(:id) || id } }
+    let(:milestone_ids) { described_class.upcoming_ids(projects, groups).map(&:id) }
     it 'returns the next upcoming open milestone ID for each project and group' do
       expect(milestone_ids).to contain_exactly(