diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index a144db9fcd1..1a69ec85d18 100644 --- a/app/finders/issuable_finder.rb +++ b/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 diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 30b5884d405..7c397f3499f 100644 --- a/app/models/milestone.rb +++ b/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 = [] + scope :for_projects_and_groups, -> (projects, groups) do + projects = projects.compact if projects.is_a? Array + projects = [] if projects.nil? - 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? + groups = groups.compact if groups.is_a? Array + groups = [] if groups.nil? - where(conditions.reduce(:or)) + 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? diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 23f9f054673..015db4d4e96 100644 --- a/spec/models/milestone_spec.rb +++ b/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(