Fix todos page when commit todos are presentfix-commit-todos

We need to preload both:
- The target's project, route, and namespace
- The todo's project, route, and namespace

That's because the view helpers will sometimes need the target's
project, and sometimes need the project - even though they will always
be the same.

Also, don't preload the target for commit todos, as commits are a
special type of target that doesn't exist in the database.

3 files changed, 56 insertions, 7 deletions

diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb
index 13d33a1c31b..1d7f33d057f 100644
--- a/app/finders/todos_finder.rb
+++ b/app/finders/todos_finder.rb
@@ -24,7 +24,6 @@ class TodosFinder
 
   def execute
     items = current_user.todos
-    items = include_associations(items)
     items = by_action_id(items)
     items = by_action(items)
     items = by_author(items)
@@ -33,8 +32,9 @@ class TodosFinder
     # Filtering by project HAS TO be the last because we use
     # the project IDs yielded by the todos query thus far
     items = by_project(items)
+    items = sort(items)
 
-    sort(items)
+    include_associations(items)
   end
 
   private
@@ -43,11 +43,19 @@ class TodosFinder
     return items unless params[:include_associations]
 
     items.includes(
-      [
-        target: { project: [:route, namespace: :route] },
-        author: { namespace: :route },
-      ]
+      project: [:route, namespace: :route],
+      author: { namespace: :route },
     )
+
+    # We can't preload target nicely, because Commit is not an AR model. So we
+    # have to preload the targets manually.
+    #
+    ActiveRecord::Associations::Preloader.new.preload(
+      items.reject(&:for_commit?),
+      target: { project: [:route, namespace: :route] }
+    )
+
+    items
   end
 
   def action_id?
diff --git a/spec/features/todos/todos_spec.rb b/spec/features/todos/todos_spec.rb
index 850020109d4..c054f381fb4 100644
--- a/spec/features/todos/todos_spec.rb
+++ b/spec/features/todos/todos_spec.rb
@@ -299,5 +299,39 @@ describe 'Dashboard Todos', feature: true do
         expect(page).to have_link "merge request #{todo.target.to_reference(full: true)}", href
       end
     end
+
+    context 'user has todos of every type on multiple projects' do
+      def create_todo(params)
+        create(:todo, params.merge(user: user))
+      end
+
+      before do
+        project2 = create(:project, :public)
+
+        merge_request_build_failed = create(:merge_request, source_project: project)
+        merge_request_unmergeable = create(:merge_request, source_project: project2)
+        issue_added = create(:issue, project: project2)
+
+        create_todo(author: author, target: issue, project: project, action: Todo::ASSIGNED)
+        create(:on_commit_todo, author: author, commit_id: project.repository.commit.id, project: project, action: Todo::MENTIONED, user: user)
+        create_todo(author: author, target: merge_request_build_failed, project: project, action: Todo::BUILD_FAILED)
+        create_todo(author: author, target: merge_request_unmergeable, project: project2, action: Todo::UNMERGEABLE)
+        create_todo(author: author, target: issue_added, project: project2, action: Todo::MARKED)
+        create(:on_commit_todo, author: author, commit_id: project2.repository.commit.id, project: project2, action: Todo::DIRECTLY_ADDRESSED, user: user)
+
+        login_as user
+      end
+
+      it 'avoids N+1 database queries' do
+        control_count = ActiveRecord::QueryRecorder.new { visit dashboard_todos_path }.count
+
+        new_project = create(:empty_project, :public)
+        new_author = create(:user)
+        new_issue = create(:issue, project: new_project)
+        create_todo(author: new_author, target: new_issue, project: new_project, action: Todo::ASSIGNED)
+
+        expect { visit dashboard_todos_path }.not_to exceed_query_limit(control_count)
+      end
+    end
   end
 end
diff --git a/spec/helpers/todos_helper_spec.rb b/spec/helpers/todos_helper_spec.rb
index 21e0e74e008..0a91bba01ce 100644
--- a/spec/helpers/todos_helper_spec.rb
+++ b/spec/helpers/todos_helper_spec.rb
@@ -5,10 +5,12 @@ describe TodosHelper do
 
   describe '#todo_target_path' do
     let(:project)       { create(:project) }
+    let(:commit)        { project.repository.commit }
     let(:merge_request) { create(:merge_request, target_project: project, source_project: project) }
     let(:issue)         { create(:issue, project: project) }
     let(:note)          { create(:note_on_issue, noteable: issue, project: project) }
 
+    let(:commit_todo)       { build(:todo, project: project, target_type: 'Commit', commit_id: commit.id) }
     let(:mr_todo)           { build(:todo, project: project, target: merge_request) }
     let(:issue_todo)        { build(:todo, project: project, target: issue) }
     let(:note_todo)         { build(:todo, project: project, target: issue, note: note) }
@@ -19,6 +21,11 @@ describe TodosHelper do
         to eq("/#{project.full_path}/merge_requests/#{merge_request.iid}")
     end
 
+    it 'returns correct path to the todo MR' do
+      expect(todo_target_path(commit_todo)).
+        to eq("/#{project.full_path}/commit/#{commit.id}")
+    end
+
     it 'returns correct path to the todo issue' do
       expect(todo_target_path(issue_todo)).
         to eq("/#{project.full_path}/issues/#{issue.iid}")
@@ -29,7 +36,7 @@ describe TodosHelper do
         to eq("/#{project.full_path}/issues/#{issue.iid}#note_#{note.id}")
     end
 
-    it 'returns correct path to build_todo MR when pipeline failed' do
+    it 'returns correct path to build_failed MR when pipeline failed' do
       expect(todo_target_path(build_failed_todo)).
         to eq("/#{project.full_path}/merge_requests/#{merge_request.iid}/pipelines")
     end