Commit a511a122 authored by Douwe Maan's avatar Douwe Maan

Merge branch...

Merge branch '17227-upcoming-milestone-is-confusing-when-projects-have-different-milestones' into 'master'

Make upcoming milestone work across projects

Before: we took the next milestone due across all projects in the
search and found issues whose milestone title matched that
one. Problems:

1. The milestone could be closed.
2. Different projects have milestones with different schedules.
3. Different projects have milestones with different titles.
4. Different projects can have milestones with different schedules, but
   the _same_ title. That means we could show issues from a past
   milestone, or one that's far in the future.

After: gather the ID of the next milestone on each project we're looking
at, and find issues with those milestone IDs. Problems:

1. For a lot of projects, this can return a lot of IDs.
2. The SQL query has to be different between Postgres and MySQL, because
   MySQL is much more lenient with HAVING: as well as the columns
   appearing in GROUP BY or in aggregate clauses, MySQL allows them to
   appear in the SELECT list (un-aggregated).

Closes #17227.

See merge request !4125
parents f2343889 e8058bd2
......@@ -15,6 +15,7 @@ v 8.8.0 (unreleased)
- Make build status canceled if any of the jobs was canceled and none failed
- Upgrade Sidekiq to 4.1.2
- Added /health_check endpoint for checking service status
- Make 'upcoming' filter for milestones work better across projects
- Sanitize repo paths in new project error message
- Bump mail_room to 0.7.0 to fix stuck IDLE connections
- Remove future dates from contribution calendar graph.
......
......@@ -252,8 +252,8 @@ class IssuableFinder
if filter_by_no_milestone?
items = items.where(milestone_id: [-1, nil])
elsif filter_by_upcoming_milestone?
upcoming = Milestone.where(project_id: projects).upcoming
items = items.joins(:milestone).where(milestones: { title: upcoming.try(:title) })
upcoming_ids = Milestone.upcoming_ids_by_projects(projects)
items = items.joins(:milestone).where(milestone_id: upcoming_ids)
else
items = items.joins(:milestone).where(milestones: { title: params[:milestone_title] })
......
......@@ -67,8 +67,18 @@ class Milestone < ActiveRecord::Base
@link_reference_pattern ||= super("milestones", /(?<milestone>\d+)/)
end
def self.upcoming
self.where('due_date > ?', Time.now).reorder(due_date: :asc).first
def self.upcoming_ids_by_projects(projects)
rel = unscoped.of_projects(projects).active.where('due_date > ?', Time.now)
if Gitlab::Database.postgresql?
rel.order(:project_id, :due_date).select('DISTINCT ON (project_id) id')
else
rel.
group(:project_id).
having('due_date = MIN(due_date)').
pluck(:id, :project_id, :due_date).
map(&:first)
end
end
def to_reference(from_project = nil)
......
require 'spec_helper'
describe IssuesFinder do
let(:user) { create :user }
let(:user2) { create :user }
let(:project1) { create(:project) }
let(:project2) { create(:project) }
let(:user) { create(:user) }
let(:user2) { create(:user) }
let(:project1) { create(:empty_project) }
let(:project2) { create(:empty_project) }
let(:milestone) { create(:milestone, project: project1) }
let(:label) { create(:label, project: project2) }
let(:issue1) { create(:issue, author: user, assignee: user, project: project1, milestone: milestone) }
......@@ -16,101 +16,147 @@ describe IssuesFinder do
project1.team << [user, :master]
project2.team << [user, :developer]
project2.team << [user2, :developer]
end
describe :execute do
before :each do
issue1
issue2
issue3
end
describe '#execute' do
let(:search_user) { user }
let(:params) { {} }
let(:issues) { IssuesFinder.new(search_user, params.merge(scope: scope, state: 'opened')).execute }
context 'scope: all' do
it 'should filter by all' do
params = { scope: "all", state: 'opened' }
issues = IssuesFinder.new(user, params).execute
expect(issues.size).to eq(3)
let(:scope) { 'all' }
it 'returns all issues' do
expect(issues).to contain_exactly(issue1, issue2, issue3)
end
it 'should filter by assignee id' do
params = { scope: "all", assignee_id: user.id, state: 'opened' }
issues = IssuesFinder.new(user, params).execute
expect(issues.size).to eq(2)
context 'filtering by assignee ID' do
let(:params) { { assignee_id: user.id } }
it 'returns issues assigned to that user' do
expect(issues).to contain_exactly(issue1, issue2)
end
end
it 'should filter by author id' do
params = { scope: "all", author_id: user2.id, state: 'opened' }
issues = IssuesFinder.new(user, params).execute
expect(issues).to eq([issue3])
context 'filtering by author ID' do
let(:params) { { author_id: user2.id } }
it 'returns issues created by that user' do
expect(issues).to contain_exactly(issue3)
end
end
it 'should filter by milestone id' do
params = { scope: "all", milestone_title: milestone.title, state: 'opened' }
issues = IssuesFinder.new(user, params).execute
expect(issues).to eq([issue1])
context 'filtering by milestone' do
let(:params) { { milestone_title: milestone.title } }
it 'returns issues assigned to that milestone' do
expect(issues).to contain_exactly(issue1)
end
end
it 'should filter by no milestone id' do
params = { scope: "all", milestone_title: Milestone::None.title, state: 'opened' }
issues = IssuesFinder.new(user, params).execute
expect(issues).to match_array([issue2, issue3])
context 'filtering by no milestone' do
let(:params) { { milestone_title: Milestone::None.title } }
it 'returns issues with no milestone' do
expect(issues).to contain_exactly(issue2, issue3)
end
end
it 'should filter by label name' do
params = { scope: "all", label_name: label.title, state: 'opened' }
issues = IssuesFinder.new(user, params).execute
expect(issues).to eq([issue2])
context 'filtering by upcoming milestone' do
let(:params) { { milestone_title: Milestone::Upcoming.name } }
let(:project_no_upcoming_milestones) { create(:empty_project, :public) }
let(:project_next_1_1) { create(:empty_project, :public) }
let(:project_next_8_8) { create(:empty_project, :public) }
let(:yesterday) { Date.today - 1.day }
let(:tomorrow) { Date.today + 1.day }
let(:two_days_from_now) { Date.today + 2.days }
let(:ten_days_from_now) { Date.today + 10.days }
let(:milestones) 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)
]
end
it 'returns unique issues when filtering by multiple labels' do
label2 = create(:label, project: project2)
before do
milestones.each do |milestone|
create(:issue, project: milestone.project, milestone: milestone, author: user, assignee: user)
end
end
create(:label_link, label: label2, target: issue2)
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)
end
end
context 'filtering by label' do
let(:params) { { label_name: label.title } }
it 'returns issues with that label' do
expect(issues).to contain_exactly(issue2)
end
end
params = {
scope: 'all',
label_name: [label.title, label2.title].join(','),
state: 'opened'
}
context 'filtering by multiple labels' do
let(:params) { { label_name: [label.title, label2.title].join(',') } }
let(:label2) { create(:label, project: project2) }
issues = IssuesFinder.new(user, params).execute
before { create(:label_link, label: label2, target: issue2) }
expect(issues).to eq([issue2])
it 'returns the unique issues with any of those labels' do
expect(issues).to contain_exactly(issue2)
end
end
context 'filtering by no label' do
let(:params) { { label_name: Label::None.title } }
it 'should filter by no label name' do
params = { scope: "all", label_name: Label::None.title, state: 'opened' }
issues = IssuesFinder.new(user, params).execute
expect(issues).to match_array([issue1, issue3])
it 'returns issues with no labels' do
expect(issues).to contain_exactly(issue1, issue3)
end
end
context 'when the user is unauthorized' do
let(:search_user) { nil }
it 'should be empty for unauthorized user' do
params = { scope: "all", state: 'opened' }
issues = IssuesFinder.new(nil, params).execute
expect(issues.size).to be_zero
it 'returns no results' do
expect(issues).to be_empty
end
end
context 'when the user can see some, but not all, issues' do
let(:search_user) { user2 }
it 'should not include unauthorized issues' do
params = { scope: "all", state: 'opened' }
issues = IssuesFinder.new(user2, params).execute
expect(issues.size).to eq(2)
expect(issues).not_to include(issue1)
expect(issues).to include(issue2)
expect(issues).to include(issue3)
it 'returns only issues they can see' do
expect(issues).to contain_exactly(issue2, issue3)
end
end
end
context 'personal scope' do
it 'should filter by assignee' do
params = { scope: "assigned-to-me", state: 'opened' }
issues = IssuesFinder.new(user, params).execute
expect(issues.size).to eq(2)
let(:scope) { 'assigned-to-me' }
it 'returns issue assigned to the user' do
expect(issues).to contain_exactly(issue1, issue2)
end
it 'should filter by project' do
params = { scope: "assigned-to-me", state: 'opened', project_id: project1.id }
issues = IssuesFinder.new(user, params).execute
expect(issues.size).to eq(1)
context 'filtering by project' do
let(:params) { { project_id: project1.id } }
it 'returns issues assigned to the user in that project' do
expect(issues).to contain_exactly(issue1)
end
end
end
end
......
......@@ -204,4 +204,37 @@ describe Milestone, models: true do
to eq([milestone])
end
end
describe '.upcoming_ids_by_projects' do
let(:project_1) { create(:empty_project) }
let(:project_2) { create(:empty_project) }
let(:project_3) { create(:empty_project) }
let(:projects) { [project_1, project_2, project_3] }
let!(:past_milestone_project_1) { create(:milestone, project: project_1, due_date: Time.now - 1.day) }
let!(:current_milestone_project_1) { create(:milestone, project: project_1, due_date: Time.now + 1.day) }
let!(:future_milestone_project_1) { create(:milestone, project: project_1, due_date: Time.now + 2.days) }
let!(:past_milestone_project_2) { create(:milestone, project: project_2, due_date: Time.now - 1.day) }
let!(:closed_milestone_project_2) { create(:milestone, :closed, project: project_2, due_date: Time.now + 1.day) }
let!(:current_milestone_project_2) { create(:milestone, project: project_2, due_date: Time.now + 2.days) }
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) { Milestone.upcoming_ids_by_projects(projects).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)
end
context 'when the projects have no open upcoming milestones' do
let(:projects) { [project_3] }
it 'returns no results' do
expect(milestone_ids).to be_empty
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment