Don’t do authorisation checks for todos

app/finders/todos_finder.rb

@@ -39,7 +39,6 @@ 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 = visible_to_user(items)
 
     sort(items)
   end
@@ -160,28 +159,14 @@ class TodosFinder
   def by_group(items)
     if group?
       groups = group.self_and_descendants
-      items = items.where(
-        'project_id IN (?) OR group_id IN (?)',
-        Project.where(group: groups).select(:id),
-        groups.select(:id)
-      )
-    end
+      project_todos = items.where(project_id: Project.where(group: groups).select(:id))
+      group_todos = items.where(group_id: groups.select(:id))
 
-    items
-  end
-
-  def visible_to_user(items)
-    projects = Project.public_or_visible_to_user(current_user)
-    groups = Group.public_or_visible_to_user(current_user)
+      union = Gitlab::SQL::Union.new([project_todos, group_todos])
+      items = Todo.from("(#{union.to_sql}) #{Todo.table_name}")
+    end
 
     items
-      .joins('LEFT JOIN namespaces ON namespaces.id = todos.group_id')
-      .joins('LEFT JOIN projects ON projects.id = todos.project_id')
-      .where(
-        'project_id IN (?) OR group_id IN (?)',
-        projects.select(:id),
-        groups.select(:id)
-      )
   end
 
   def by_state(items)

app/models/group.rb

@@ -88,12 +88,6 @@ class Group < Namespace
       where(id: user.authorized_groups.select(:id).reorder(nil))
     end
 
-    def public_or_visible_to_user(user)
-      where('id IN (?) OR namespaces.visibility_level IN (?)',
-        user.authorized_groups.select(:id),
-        Gitlab::VisibilityLevel.levels_for_user(user))
-    end
-
     def select_for_project_authorization
       if current_scope.joins_values.include?(:shared_projects)
         joins('INNER JOIN namespaces project_namespace ON project_namespace.id = projects.namespace_id')

db/migrate/20180608091413_add_group_to_todos.rb

@@ -5,8 +5,14 @@ class AddGroupToTodos < ActiveRecord::Migration
 
   disable_ddl_transaction!
 
+  class Todo < ActiveRecord::Base
+    self.table_name = 'todos'
+
+    include ::EachBatch
+  end
+
   def up
-    add_column :todos, :group_id, :integer
+    add_column(:todos, :group_id, :integer) unless group_id_exists?
     add_concurrent_foreign_key :todos, :namespaces, column: :group_id, on_delete: :cascade
     add_concurrent_index :todos, :group_id
 
@@ -14,13 +20,11 @@ class AddGroupToTodos < ActiveRecord::Migration
   end
 
   def down
-    return unless group_id_exists?
-
-    remove_foreign_key :todos, column: :group_id
-    remove_index :todos, :group_id if index_exists?(:todos, :group_id)
-    remove_column :todos, :group_id
+    remove_foreign_key_without_error(:todos, column: :group_id)
+    remove_concurrent_index(:todos, :group_id)
+    remove_column(:todos, :group_id) if group_id_exists?
 
-    execute "DELETE FROM todos WHERE project_id IS NULL"
+    Todo.where(project_id: nil).each_batch { |batch| batch.delete_all }
     change_column_null :todos, :project_id, false
   end
 

spec/finders/todos_finder_spec.rb

@@ -14,32 +14,6 @@ describe TodosFinder do
     end
 
     describe '#execute' do
-      context 'visibility' do
-        let(:private_group_access) { create(:group, :private) }
-        let(:private_group_hidden) { create(:group, :private) }
-        let(:public_project) { create(:project, :public) }
-        let(:private_project_hidden) { create(:project) }
-        let(:public_group) { create(:group) }
-
-        let!(:todo1) { create(:todo, user: user, project: project, group: nil) }
-        let!(:todo2) { create(:todo, user: user, project: public_project, group: nil) }
-        let!(:todo3) { create(:todo, user: user, project: private_project_hidden, group: nil) }
-        let!(:todo4) { create(:todo, user: user, project: nil, group: group) }
-        let!(:todo5) { create(:todo, user: user, project: nil, group: private_group_access) }
-        let!(:todo6) { create(:todo, user: user, project: nil, group: private_group_hidden) }
-        let!(:todo7) { create(:todo, user: user, project: nil, group: public_group) }
-
-        before do
-          private_group_access.add_developer(user)
-        end
-
-        it 'returns only todos with a target a user has access to' do
-          todos = finder.new(user).execute
-
-          expect(todos).to match_array([todo1, todo2, todo4, todo5, todo7])
-        end
-      end
-
       context 'filtering' do
         let!(:todo1) { create(:todo, user: user, project: project, target: issue) }
         let!(:todo2) { create(:todo, user: user, group: group, target: merge_request) }

spec/requests/api/todos_spec.rb

 require 'spec_helper'
 
 describe API::Todos do
-  let(:project_1) { create(:project, :repository) }
+  let(:group)     { create(:group) }
+  let(:project_1) { create(:project, :repository, group: group) }
   let(:project_2) { create(:project) }
   let(:author_1) { create(:user) }
   let(:author_2) { create(:user) }
@@ -92,6 +93,17 @@ describe API::Todos do
         end
       end
 
+      context 'and using the group filter' do
+        it 'filters based on project_id param' do
+          get api('/todos', john_doe), { group_id: group.id, sort: :target_id }
+
+          expect(response.status).to eq(200)
+          expect(response).to include_pagination_headers
+          expect(json_response).to be_an Array
+          expect(json_response.length).to eq(2)
+        end
+      end
+
       context 'and using the action filter' do
         it 'filters based on action param' do
           get api('/todos', john_doe), { action: 'mentioned' }

spec/services/members/destroy_service_spec.rb

@@ -43,7 +43,7 @@ describe Members::DestroyService do
   shared_examples 'a service destroying a member with access' do
     it_behaves_like 'a service destroying a member'
 
-    it 'invalidates cached counts for todos and assigned issues and merge requests', :aggregate_failures do
+    it 'invalidates cached counts for assigned issues and merge requests', :aggregate_failures do
       create(:issue, project: group_project, assignees: [member_user])
       create(:merge_request, source_project: group_project, assignee: member_user)
       create(:todo, :pending, project: group_project, user: member_user)
@@ -51,15 +51,11 @@ describe Members::DestroyService do
 
       expect(member_user.assigned_open_merge_requests_count).to be(1)
       expect(member_user.assigned_open_issues_count).to be(1)
-      expect(member_user.todos_pending_count).to be(1)
-      expect(member_user.todos_done_count).to be(1)
 
       described_class.new(current_user).execute(member, opts)
 
       expect(member_user.assigned_open_merge_requests_count).to be(0)
       expect(member_user.assigned_open_issues_count).to be(0)
-      expect(member_user.todos_pending_count).to be(0)
-      expect(member_user.todos_done_count).to be(0)
     end
   end