Commit 70499125 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'todos-filter-project-delete' into 'master'

Ensure we don't show TODOS for projects pending delete

## What does this MR do?

Joins the todos on the projects table in order to run the default scope. Also includes a where clause because the default scope is being removed soon.

## Are there points in the code the reviewer needs to double check?

An alternative approach, more like the Issues page, would be to filter down the list by passing user.authorized_projects into the where clause.

Or we could just be more defensive in the view when iterating.

## Why was this MR needed?

Todos page throws 500 error for users with todos in a project pending deletion.

## What are the relevant issue numbers?

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/17813

cc\ @stanhu 

See merge request !4300
parents 1637e178 86675194
...@@ -34,6 +34,9 @@ v 8.9.0 (unreleased) ...@@ -34,6 +34,9 @@ v 8.9.0 (unreleased)
- Improve error handling importing projects - Improve error handling importing projects
- Put project Files and Commits tabs under Code tab - Put project Files and Commits tabs under Code tab
v 8.8.4
- Fix todos page throwing errors when you have a project pending deletion
v 8.8.3 v 8.8.3
- Fix 404 page when viewing TODOs that contain milestones or labels in different projects. !4312 - Fix 404 page when viewing TODOs that contain milestones or labels in different projects. !4312
- Fixed JS error when trying to remove discussion form. !4303 - Fixed JS error when trying to remove discussion form. !4303
......
...@@ -30,7 +30,7 @@ class TodosFinder ...@@ -30,7 +30,7 @@ class TodosFinder
items = by_state(items) items = by_state(items)
items = by_type(items) items = by_type(items)
items items.reorder(id: :desc)
end end
private private
...@@ -78,6 +78,16 @@ class TodosFinder ...@@ -78,6 +78,16 @@ class TodosFinder
@project @project
end end
def projects
return @projects if defined?(@projects)
if project?
@projects = project
else
@projects = ProjectsFinder.new.execute(current_user)
end
end
def type? def type?
type.present? && ['Issue', 'MergeRequest'].include?(type) type.present? && ['Issue', 'MergeRequest'].include?(type)
end end
...@@ -105,6 +115,8 @@ class TodosFinder ...@@ -105,6 +115,8 @@ class TodosFinder
def by_project(items) def by_project(items)
if project? if project?
items = items.where(project: project) items = items.where(project: project)
elsif projects
items = items.merge(projects).joins(:project)
end end
items items
......
...@@ -3,7 +3,7 @@ require 'rails_helper' ...@@ -3,7 +3,7 @@ require 'rails_helper'
feature 'Todo target states', feature: true do feature 'Todo target states', feature: true do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:author) { create(:user) } let(:author) { create(:user) }
let(:project) { create(:project) } let(:project) { create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC) }
before do before do
login_as user login_as user
......
...@@ -3,7 +3,7 @@ require 'spec_helper' ...@@ -3,7 +3,7 @@ require 'spec_helper'
describe 'Dashboard Todos', feature: true do describe 'Dashboard Todos', feature: true do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:author) { create(:user) } let(:author) { create(:user) }
let(:project) { create(:project) } let(:project) { create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC) }
let(:issue) { create(:issue) } let(:issue) { create(:issue) }
describe 'GET /dashboard/todos' do describe 'GET /dashboard/todos' do
...@@ -49,7 +49,7 @@ describe 'Dashboard Todos', feature: true do ...@@ -49,7 +49,7 @@ describe 'Dashboard Todos', feature: true do
note1 = create(:note_on_issue, note: "Hello #{label1.to_reference(format: :name)}", noteable_id: issue.id, noteable_type: 'Issue', project: issue.project) note1 = create(:note_on_issue, note: "Hello #{label1.to_reference(format: :name)}", noteable_id: issue.id, noteable_type: 'Issue', project: issue.project)
create(:todo, :mentioned, project: project, target: issue, user: user, note_id: note1.id) create(:todo, :mentioned, project: project, target: issue, user: user, note_id: note1.id)
project2 = create(:project) project2 = create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC)
label2 = create(:label, project: project2) label2 = create(:label, project: project2)
issue2 = create(:issue, project: project2) issue2 = create(:issue, project: project2)
note2 = create(:note_on_issue, note: "Test #{label2.to_reference(format: :name)}", noteable_id: issue2.id, noteable_type: 'Issue', project: project2) note2 = create(:note_on_issue, note: "Test #{label2.to_reference(format: :name)}", noteable_id: issue2.id, noteable_type: 'Issue', project: project2)
...@@ -98,5 +98,18 @@ describe 'Dashboard Todos', feature: true do ...@@ -98,5 +98,18 @@ describe 'Dashboard Todos', feature: true do
end end
end end
end end
context 'User has a Todo in a project pending deletion' do
before do
deleted_project = create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC, pending_delete: true)
create(:todo, :mentioned, user: user, project: deleted_project, target: issue, author: author)
login_as(user)
visit dashboard_todos_path
end
it 'shows "All done" message' do
expect(page).to have_content "You're all done!"
end
end
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