From 12e01daec9f24fda742103f9e6c795c358693de5 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Thu, 15 Nov 2018 14:56:51 +0800 Subject: [PATCH 1/3] Add group milestones in upcoming filter --- app/finders/issuable_finder.rb | 20 ++++++++++++++++--- app/models/milestone.rb | 28 +++++++++++++++++++-------- spec/finders/issues_finder_spec.rb | 19 +++++++++++------- spec/models/milestone_spec.rb | 31 +++++++++++++++++++++++++----- 4 files changed, 75 insertions(+), 23 deletions(-) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index b73a3fa6e01..a144db9fcd1 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -149,6 +149,18 @@ class IssuableFinder end end + def related_groups + if project? && project && project.group && Ability.allowed?(current_user, :read_group, project.group) + project.group.self_and_ancestors + elsif group + [group] + elsif current_user + Gitlab::GroupHierarchy.new(current_user.authorized_groups, current_user.groups).all_groups + else + [] + end + end + def project? params[:project_id].present? end @@ -163,8 +175,10 @@ class IssuableFinder end # rubocop: disable CodeReuse/ActiveRecord - def projects(items = nil) - return @projects = project if project? + def projects + return @projects if defined?(@projects) + + return @projects = [project] if project? projects = if current_user && params[:authorized_only].presence && !current_user_related? @@ -459,7 +473,7 @@ class IssuableFinder elsif filter_by_any_milestone? items = items.any_milestone elsif filter_by_upcoming_milestone? - upcoming_ids = Milestone.upcoming_ids_by_projects(projects(items)) + 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.where('milestones.start_date <= NOW()') diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 6dc0fca68e6..30b5884d405 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -40,6 +40,7 @@ class Milestone < ActiveRecord::Base 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? @@ -129,18 +130,29 @@ class Milestone < ActiveRecord::Base @link_reference_pattern ||= super("milestones", /(?\d+)/) end - def self.upcoming_ids_by_projects(projects) - rel = unscoped.of_projects(projects).active.where('due_date > ?', Time.now) + def self.upcoming_ids(projects, groups) + rel = unscoped + .for_projects_and_groups(projects&.map(&:id), groups&.map(&:id)) + .active.where('milestones.due_date > NOW()') if Gitlab::Database.postgresql? - rel.order(:project_id, :due_date).select('DISTINCT ON (project_id) id') + rel.order(:project_id, :group_id, :due_date).select('DISTINCT ON (project_id, group_id) id') else + # We need to use MySQL's NULL-safe comparison operator `<=>` here + # because one of `project_id` or `group_id` is always NULL + join_clause = <<~HEREDOC + LEFT OUTER JOIN milestones earlier_milestones + ON milestones.project_id <=> earlier_milestones.project_id + AND milestones.group_id <=> earlier_milestones.group_id + AND milestones.due_date > earlier_milestones.due_date + AND earlier_milestones.due_date > NOW() + AND earlier_milestones.state = 'active' + HEREDOC + rel - .group(:project_id, :due_date, :id) - .having('due_date = MIN(due_date)') - .pluck(:id, :project_id, :due_date) - .uniq(&:second) - .map(&:first) + .joins(join_clause) + .where('earlier_milestones.id IS NULL') + .select(:id) end end diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index 80f7232f282..682fae06434 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -174,9 +174,13 @@ describe IssuesFinder do context 'filtering by upcoming milestone' do let(:params) { { milestone_title: Milestone::Upcoming.name } } + let!(:group) { create(:group, :public) } + let!(:group_member) { create(:group_member, group: group, user: user) } + let(:project_no_upcoming_milestones) { create(:project, :public) } let(:project_next_1_1) { create(:project, :public) } let(:project_next_8_8) { create(:project, :public) } + let(:project_in_group) { create(:project, :public, namespace: group) } let(:yesterday) { Date.today - 1.day } let(:tomorrow) { Date.today + 1.day } @@ -187,21 +191,22 @@ describe IssuesFinder do [ create(:milestone, :closed, project: project_no_upcoming_milestones), create(:milestone, project: project_next_1_1, title: '1.1', due_date: two_days_from_now), - create(:milestone, project: project_next_1_1, title: '8.8', due_date: ten_days_from_now), - create(:milestone, project: project_next_8_8, title: '1.1', due_date: yesterday), - create(:milestone, project: project_next_8_8, title: '8.8', due_date: tomorrow) + create(:milestone, project: project_next_1_1, title: '8.9', due_date: ten_days_from_now), + create(:milestone, project: project_next_8_8, title: '1.2', due_date: yesterday), + create(:milestone, project: project_next_8_8, title: '8.8', due_date: tomorrow), + create(:milestone, group: group, title: '9.9', due_date: tomorrow) ] end before do milestones.each do |milestone| - create(:issue, project: milestone.project, milestone: milestone, author: user, assignees: [user]) + create(:issue, project: milestone.project || project_in_group, milestone: milestone, author: user, assignees: [user]) end end - it 'returns issues in the upcoming milestone for each project' do - expect(issues.map { |issue| issue.milestone.title }).to contain_exactly('1.1', '8.8') - expect(issues.map { |issue| issue.milestone.due_date }).to contain_exactly(tomorrow, two_days_from_now) + it 'returns issues in the upcoming milestone for each project or group' do + expect(issues.map { |issue| issue.milestone.title }).to contain_exactly('1.1', '8.8', '9.9') + expect(issues.map { |issue| issue.milestone.due_date }).to contain_exactly(tomorrow, two_days_from_now, tomorrow) end end diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index b3d31e65c85..23f9f054673 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -240,7 +240,22 @@ describe Milestone do end end - describe '.upcoming_ids_by_projects' do + describe '.upcoming_ids' do + let(:group_1) { create(:group) } + let(:group_2) { create(:group) } + let(:group_3) { create(:group) } + let(:groups) { [group_1, group_2, group_3] } + + let!(:past_milestone_group_1) { create(:milestone, group: group_1, due_date: Time.now - 1.day) } + let!(:current_milestone_group_1) { create(:milestone, group: group_1, due_date: Time.now + 1.day) } + let!(:future_milestone_group_1) { create(:milestone, group: group_1, due_date: Time.now + 2.days) } + + let!(:past_milestone_group_2) { create(:milestone, group: group_2, due_date: Time.now - 1.day) } + let!(:closed_milestone_group_2) { create(:milestone, :closed, group: group_2, due_date: Time.now + 1.day) } + let!(:current_milestone_group_2) { create(:milestone, group: group_2, due_date: Time.now + 2.days) } + + let!(:past_milestone_group_3) { create(:milestone, group: group_3, due_date: Time.now - 1.day) } + let(:project_1) { create(:project) } let(:project_2) { create(:project) } let(:project_3) { create(:project) } @@ -258,14 +273,20 @@ describe Milestone do # 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_by_projects(projects).map { |id| id.try(:id) || id } } + let(:milestone_ids) { described_class.upcoming_ids(projects, groups).map { |id| id.try(:id) || id } } - it 'returns the next upcoming open milestone ID for each project' do - expect(milestone_ids).to contain_exactly(current_milestone_project_1.id, current_milestone_project_2.id) + it 'returns the next upcoming open milestone ID for each project and group' do + expect(milestone_ids).to contain_exactly( + current_milestone_project_1.id, + current_milestone_project_2.id, + current_milestone_group_1.id, + current_milestone_group_2.id + ) end - context 'when the projects have no open upcoming milestones' do + context 'when the projects and groups have no open upcoming milestones' do let(:projects) { [project_3] } + let(:groups) { [group_3] } it 'returns no results' do expect(milestone_ids).to be_empty From 030c7068fc8127e1ff8afa7c8daffee034c7b929 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Thu, 15 Nov 2018 15:03:01 +0800 Subject: [PATCH 2/3] Add changelog entry --- .../53431-fix-upcoming-milestone-filter-for-groups.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/53431-fix-upcoming-milestone-filter-for-groups.yml diff --git a/changelogs/unreleased/53431-fix-upcoming-milestone-filter-for-groups.yml b/changelogs/unreleased/53431-fix-upcoming-milestone-filter-for-groups.yml new file mode 100644 index 00000000000..1e9c7f3913c --- /dev/null +++ b/changelogs/unreleased/53431-fix-upcoming-milestone-filter-for-groups.yml @@ -0,0 +1,5 @@ +--- +title: Fix upcoming milestones filter not including group milestones +merge_request: 23098 +author: Heinrich Lee Yu +type: fixed From 2490cfeeb2f8426b1a8f4e24bd0297e41a870ca2 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Mon, 17 Dec 2018 20:59:23 +0800 Subject: [PATCH 3/3] 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 | 2 +- app/models/milestone.rb | 13 ++++--- spec/models/milestone_spec.rb | 70 ++++++++++++++++++++++++++++++++-- 3 files changed, 75 insertions(+), 10 deletions(-) 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(