From 501fb04ec65cadcd7dddc6376546db8d8f7f123c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jarka=20Kadlecov=C3=A1?= <jarka@gitlab.com>
Date: Mon, 16 Jul 2018 20:30:17 +0200
Subject: [PATCH 1/3] Delete todos when users loses target read permissions

---
 app/services/issues/update_service.rb         |  2 +
 app/services/members/destroy_service.rb       |  8 ++
 app/services/projects/update_service.rb       | 23 +++--
 app/services/todos/destroy/base_service.rb    | 33 +++++++
 .../destroy/confidential_issue_service.rb     | 30 ++++++
 .../todos/destroy/entity_leave_service.rb     | 49 ++++++++++
 .../todos/destroy/project_private_service.rb  | 30 ++++++
 app/workers/all_queues.yml                    |  4 +
 app/workers/concerns/todos_destroyer_queue.rb | 12 +++
 .../confidential_issue_worker.rb              | 10 ++
 .../todos_destroyer/entity_leave_worker.rb    | 10 ++
 .../todos_destroyer/project_private_worker.rb | 10 ++
 .../unreleased/todos-visibility-change.yml    |  5 +
 config/sidekiq_queues.yml                     |  1 +
 spec/services/issues/update_service_spec.rb   | 17 ++++
 spec/services/members/destroy_service_spec.rb |  5 +
 spec/services/projects/update_service_spec.rb | 22 +++++
 .../confidential_issue_service_spec.rb        | 39 ++++++++
 .../destroy/entity_leave_service_spec.rb      | 96 +++++++++++++++++++
 .../destroy/project_private_service_spec.rb   | 38 ++++++++
 .../confidential_issue_worker_spec.rb         | 12 +++
 .../entity_leave_worker_spec.rb               | 12 +++
 .../project_private_worker_spec.rb            | 12 +++
 23 files changed, 473 insertions(+), 7 deletions(-)
 create mode 100644 app/services/todos/destroy/base_service.rb
 create mode 100644 app/services/todos/destroy/confidential_issue_service.rb
 create mode 100644 app/services/todos/destroy/entity_leave_service.rb
 create mode 100644 app/services/todos/destroy/project_private_service.rb
 create mode 100644 app/workers/concerns/todos_destroyer_queue.rb
 create mode 100644 app/workers/todos_destroyer/confidential_issue_worker.rb
 create mode 100644 app/workers/todos_destroyer/entity_leave_worker.rb
 create mode 100644 app/workers/todos_destroyer/project_private_worker.rb
 create mode 100644 changelogs/unreleased/todos-visibility-change.yml
 create mode 100644 spec/services/todos/destroy/confidential_issue_service_spec.rb
 create mode 100644 spec/services/todos/destroy/entity_leave_service_spec.rb
 create mode 100644 spec/services/todos/destroy/project_private_service_spec.rb
 create mode 100644 spec/workers/todos_destroyer/confidential_issue_worker_spec.rb
 create mode 100644 spec/workers/todos_destroyer/entity_leave_worker_spec.rb
 create mode 100644 spec/workers/todos_destroyer/project_private_worker_spec.rb

diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index c02dddf67b2..faa4c8a5a4f 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -37,6 +37,8 @@ module Issues
       end
 
       if issue.previous_changes.include?('confidential')
+        # don't enqueue immediately to prevent todos removal in case of a mistake
+        TodosDestroyer::ConfidentialIssueWorker.perform_in(1.hour, issue.id) if issue.confidential?
         create_confidentiality_note(issue)
       end
 
diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb
index aca0ba66646..c186a5971dc 100644
--- a/app/services/members/destroy_service.rb
+++ b/app/services/members/destroy_service.rb
@@ -15,6 +15,8 @@ module Members
         notification_service.decline_access_request(member)
       end
 
+      enqeue_delete_todos(member)
+
       after_execute(member: member)
 
       member
@@ -22,6 +24,12 @@ module Members
 
     private
 
+    def enqeue_delete_todos(member)
+      type = member.is_a?(GroupMember) ? 'Group' : 'Project'
+      # don't enqueue immediately to prevent todos removal in case of a mistake
+      TodosDestroyer::EntityLeaveWorker.perform_in(1.hour, member.user_id, member.source_id, type)
+    end
+
     def can_destroy_member?(member)
       can?(current_user, destroy_member_permission(member), member)
     end
diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index d3dc11435fe..a3c688f8a14 100644
--- a/app/services/projects/update_service.rb
+++ b/app/services/projects/update_service.rb
@@ -25,13 +25,7 @@ module Projects
       return validation_failed! if project.errors.any?
 
       if project.update(params.except(:default_branch))
-        if project.previous_changes.include?('path')
-          project.rename_repo
-        else
-          system_hook_service.execute_hooks_for(project, :update)
-        end
-
-        update_pages_config if changing_pages_https_only?
+        after_update
 
         success
       else
@@ -47,6 +41,21 @@ module Projects
 
     private
 
+    def after_update
+      if project.previous_changes.include?(:visibility_level) && project.private?
+        # don't enqueue immediately to prevent todos removal in case of a mistake
+        TodosDestroyer::ProjectPrivateWorker.perform_in(1.hour, project.id)
+      end
+
+      if project.previous_changes.include?('path')
+        project.rename_repo
+      else
+        system_hook_service.execute_hooks_for(project, :update)
+      end
+
+      update_pages_config if changing_pages_https_only?
+    end
+
     def validation_failed!
       model_errors = project.errors.full_messages.to_sentence
       error_message = model_errors.presence || 'Project could not be updated!'
diff --git a/app/services/todos/destroy/base_service.rb b/app/services/todos/destroy/base_service.rb
new file mode 100644
index 00000000000..71cd7921f32
--- /dev/null
+++ b/app/services/todos/destroy/base_service.rb
@@ -0,0 +1,33 @@
+module Todos
+  module Destroy
+    class BaseService
+      def execute
+        return unless todos_to_remove?
+
+        without_authorized(todos).delete_all
+      end
+
+      private
+
+      def without_authorized(items)
+        items.where('user_id NOT IN (?)', authorized_users)
+      end
+
+      def authorized_users
+        ProjectAuthorization.select(:user_id).where(project_id: project_ids)
+      end
+
+      def todos
+        # overridden in subclasses
+      end
+
+      def project_ids
+        # overridden in subclasses
+      end
+
+      def todos_to_remove?
+        # overridden in subclasses
+      end
+    end
+  end
+end
diff --git a/app/services/todos/destroy/confidential_issue_service.rb b/app/services/todos/destroy/confidential_issue_service.rb
new file mode 100644
index 00000000000..06cf308a3cd
--- /dev/null
+++ b/app/services/todos/destroy/confidential_issue_service.rb
@@ -0,0 +1,30 @@
+module Todos
+  module Destroy
+    class ConfidentialIssueService < ::Todos::Destroy::BaseService
+      extend ::Gitlab::Utils::Override
+
+      attr_reader :issue
+
+      def initialize(issue_id)
+        @issue = Issue.find_by(id: issue_id)
+      end
+
+      private
+
+      override :todos
+      def todos
+        Todo.where(target: issue)
+      end
+
+      override :todos_to_remove?
+      def todos_to_remove?
+        issue&.confidential?
+      end
+
+      override :project_ids
+      def project_ids
+        issue.project_id
+      end
+    end
+  end
+end
diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb
new file mode 100644
index 00000000000..328a8b39e7b
--- /dev/null
+++ b/app/services/todos/destroy/entity_leave_service.rb
@@ -0,0 +1,49 @@
+module Todos
+  module Destroy
+    class EntityLeaveService < ::Todos::Destroy::BaseService
+      extend ::Gitlab::Utils::Override
+
+      attr_reader :user_id, :entity
+
+      def initialize(user_id, entity_id, entity_type)
+        unless %w(Group Project).include?(entity_type)
+          raise ArgumentError.new("#{entity_type} is not an entity user can leave")
+        end
+
+        @user_id = user_id
+        @entity = entity_type.constantize.find_by(id: entity_id)
+      end
+
+      private
+
+      override :todos
+      def todos
+        if entity.private?
+          Todo.where(project_id: project_ids, user_id: user_id)
+        else
+          Todo.where(target_id: confidential_issues.select(:id), target_type: Issue)
+        end
+      end
+
+      override :project_ids
+      def project_ids
+        if entity.is_a?(Project)
+          entity.id
+        else
+          Project.select(:id).where(namespace_id: entity.self_and_descendants.select(:id))
+        end
+      end
+
+      override :todos_to_remove?
+      def todos_to_remove?
+        return unless entity
+
+        entity.private? || confidential_issues.count > 0
+      end
+
+      def confidential_issues
+        Issue.where(project_id: project_ids, confidential: true)
+      end
+    end
+  end
+end
diff --git a/app/services/todos/destroy/project_private_service.rb b/app/services/todos/destroy/project_private_service.rb
new file mode 100644
index 00000000000..171933e7cbc
--- /dev/null
+++ b/app/services/todos/destroy/project_private_service.rb
@@ -0,0 +1,30 @@
+module Todos
+  module Destroy
+    class ProjectPrivateService < ::Todos::Destroy::BaseService
+      extend ::Gitlab::Utils::Override
+
+      attr_reader :project
+
+      def initialize(project_id)
+        @project = Project.find_by(id: project_id)
+      end
+
+      private
+
+      override :todos
+      def todos
+        Todo.where(project_id: project_ids)
+      end
+
+      override :project_ids
+      def project_ids
+        project.id
+      end
+
+      override :todos_to_remove?
+      def todos_to_remove?
+        project&.private?
+      end
+    end
+  end
+end
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index 4de35b9bd06..4a6bee8af83 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -73,6 +73,10 @@
 - repository_check:repository_check_batch
 - repository_check:repository_check_single_repository
 
+- todos_destroyer:todos_destroyer_confidential_issue
+- todos_destroyer:todos_destroyer_entity_leave
+- todos_destroyer:todos_destroyer_project_private
+
 - default
 - mailers # ActionMailer::DeliveryJob.queue_name
 
diff --git a/app/workers/concerns/todos_destroyer_queue.rb b/app/workers/concerns/todos_destroyer_queue.rb
new file mode 100644
index 00000000000..8e2b1d30579
--- /dev/null
+++ b/app/workers/concerns/todos_destroyer_queue.rb
@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+##
+# Concern for setting Sidekiq settings for the various Todos Destroyers.
+#
+module TodosDestroyerQueue
+  extend ActiveSupport::Concern
+
+  included do
+    queue_namespace :todos_destroyer
+  end
+end
diff --git a/app/workers/todos_destroyer/confidential_issue_worker.rb b/app/workers/todos_destroyer/confidential_issue_worker.rb
new file mode 100644
index 00000000000..9d640c14963
--- /dev/null
+++ b/app/workers/todos_destroyer/confidential_issue_worker.rb
@@ -0,0 +1,10 @@
+module TodosDestroyer
+  class ConfidentialIssueWorker
+    include ApplicationWorker
+    include TodosDestroyerQueue
+
+    def perform(issue_id)
+      ::Todos::Destroy::ConfidentialIssueService.new(issue_id).execute
+    end
+  end
+end
diff --git a/app/workers/todos_destroyer/entity_leave_worker.rb b/app/workers/todos_destroyer/entity_leave_worker.rb
new file mode 100644
index 00000000000..e62d9876f4a
--- /dev/null
+++ b/app/workers/todos_destroyer/entity_leave_worker.rb
@@ -0,0 +1,10 @@
+module TodosDestroyer
+  class EntityLeaveWorker
+    include ApplicationWorker
+    include TodosDestroyerQueue
+
+    def perform(user_id, entity_id, entity_type)
+      ::Todos::Destroy::EntityLeaveService.new(user_id, entity_id, entity_type).execute
+    end
+  end
+end
diff --git a/app/workers/todos_destroyer/project_private_worker.rb b/app/workers/todos_destroyer/project_private_worker.rb
new file mode 100644
index 00000000000..7a853c36370
--- /dev/null
+++ b/app/workers/todos_destroyer/project_private_worker.rb
@@ -0,0 +1,10 @@
+module TodosDestroyer
+  class ProjectPrivateWorker
+    include ApplicationWorker
+    include TodosDestroyerQueue
+
+    def perform(project_id)
+      ::Todos::Destroy::ProjectPrivateService.new(project_id).execute
+    end
+  end
+end
diff --git a/changelogs/unreleased/todos-visibility-change.yml b/changelogs/unreleased/todos-visibility-change.yml
new file mode 100644
index 00000000000..b7632b94771
--- /dev/null
+++ b/changelogs/unreleased/todos-visibility-change.yml
@@ -0,0 +1,5 @@
+---
+title: Delete todos when user loses access to read the target
+merge_request: 20665
+author:
+type: other
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index 70b584ff9e9..3c85cd07d46 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -45,6 +45,7 @@
   - [github_import_advance_stage, 1]
   - [project_service, 1]
   - [delete_user, 1]
+  - [todos_destroyer, 1]
   - [delete_merged_branches, 1]
   - [authorized_projects, 1]
   - [expire_build_instance_artifacts, 1]
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index fa98d05c61b..5bcfef46b75 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -55,6 +55,8 @@ describe Issues::UpdateService, :mailer do
       end
 
       it 'updates the issue with the given params' do
+        expect(TodosDestroyer::ConfidentialIssueWorker).not_to receive(:perform_in)
+
         update_issue(opts)
 
         expect(issue).to be_valid
@@ -74,6 +76,21 @@ describe Issues::UpdateService, :mailer do
           .to change { project.open_issues_count }.from(1).to(0)
       end
 
+      it 'enqueues ConfidentialIssueWorker when an issue is made confidential' do
+        expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(1.hour, issue.id)
+
+        update_issue(confidential: true)
+      end
+
+      it 'does not enqueue ConfidentialIssueWorker when an issue is made non confidential' do
+        # set confidentiality to true before the actual update
+        issue.update!(confidential: true)
+
+        expect(TodosDestroyer::ConfidentialIssueWorker).not_to receive(:perform_in)
+
+        update_issue(confidential: false)
+      end
+
       it 'updates open issue counter for assignees when issue is reassigned' do
         update_issue(assignee_ids: [user2.id])
 
diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb
index ef47b0a450b..0a5220c7c61 100644
--- a/spec/services/members/destroy_service_spec.rb
+++ b/spec/services/members/destroy_service_spec.rb
@@ -20,6 +20,11 @@ describe Members::DestroyService do
   end
 
   shared_examples 'a service destroying a member' do
+    before do
+      type = member.is_a?(GroupMember) ? 'Group' : 'Project'
+      expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(1.hour, member.user_id, member.source_id, type)
+    end
+
     it 'destroys the member' do
       expect { described_class.new(current_user).execute(member, opts) }.to change { member.source.members_and_requesters.count }.by(-1)
     end
diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb
index ecf1ba05618..d1686e1007c 100644
--- a/spec/services/projects/update_service_spec.rb
+++ b/spec/services/projects/update_service_spec.rb
@@ -15,6 +15,8 @@ describe Projects::UpdateService do
     context 'when changing visibility level' do
       context 'when visibility_level is INTERNAL' do
         it 'updates the project to internal' do
+          expect(TodosDestroyer::ProjectPrivateWorker).not_to receive(:perform_in)
+
           result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL)
 
           expect(result).to eq({ status: :success })
@@ -24,12 +26,30 @@ describe Projects::UpdateService do
 
       context 'when visibility_level is PUBLIC' do
         it 'updates the project to public' do
+          expect(TodosDestroyer::ProjectPrivateWorker).not_to receive(:perform_in)
+
           result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+
           expect(result).to eq({ status: :success })
           expect(project).to be_public
         end
       end
 
+      context 'when visibility_level is PRIVATE' do
+        before do
+          project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+        end
+
+        it 'updates the project to private' do
+          expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(1.hour, project.id)
+
+          result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+
+          expect(result).to eq({ status: :success })
+          expect(project).to be_private
+        end
+      end
+
       context 'when visibility levels are restricted to PUBLIC only' do
         before do
           stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])
@@ -38,6 +58,7 @@ describe Projects::UpdateService do
         context 'when visibility_level is INTERNAL' do
           it 'updates the project to internal' do
             result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+
             expect(result).to eq({ status: :success })
             expect(project).to be_internal
           end
@@ -54,6 +75,7 @@ describe Projects::UpdateService do
           context 'when updated by an admin' do
             it 'updates the project to public' do
               result = update_project(project, admin, visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+
               expect(result).to eq({ status: :success })
               expect(project).to be_public
             end
diff --git a/spec/services/todos/destroy/confidential_issue_service_spec.rb b/spec/services/todos/destroy/confidential_issue_service_spec.rb
new file mode 100644
index 00000000000..5c214df49bc
--- /dev/null
+++ b/spec/services/todos/destroy/confidential_issue_service_spec.rb
@@ -0,0 +1,39 @@
+require 'spec_helper'
+
+describe Todos::Destroy::ConfidentialIssueService do
+  let(:project)        { create(:project, :public) }
+  let(:user)           { create(:user) }
+  let(:project_member) { create(:user) }
+  let(:issue)          { create(:issue, project: project) }
+
+  let!(:todo_issue_non_member)   { create(:todo, user: user, target: issue, project: project) }
+  let!(:todo_issue_member)       { create(:todo, user: project_member, target: issue, project: project) }
+  let!(:todo_another_non_member) { create(:todo, user: user, project: project) }
+
+  describe '#execute' do
+    before do
+      project.add_developer(project_member)
+    end
+
+    subject { described_class.new(issue.id).execute }
+
+    context 'when provided issue is confidential' do
+      before do
+        issue.update!(confidential: true)
+      end
+
+      it 'removes issue todos for a user who is not a project member' do
+        expect { subject }.to change { Todo.count }.from(3).to(2)
+
+        expect(user.todos).to match_array([todo_another_non_member])
+        expect(project_member.todos).to match_array([todo_issue_member])
+      end
+    end
+
+    context 'when provided issue is not confidential' do
+      it 'does not remove any todos' do
+        expect { subject }.not_to change { Todo.count }
+      end
+    end
+  end
+end
diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb
new file mode 100644
index 00000000000..e5673383df8
--- /dev/null
+++ b/spec/services/todos/destroy/entity_leave_service_spec.rb
@@ -0,0 +1,96 @@
+require 'spec_helper'
+
+describe Todos::Destroy::EntityLeaveService do
+  let(:group)          { create(:group, :private) }
+  let(:project)        { create(:project, group: group) }
+  let(:user)           { create(:user) }
+  let(:project_member) { create(:user) }
+  let(:issue)          { create(:issue, :confidential, project: project) }
+
+  let!(:todo_non_member)            { create(:todo, user: user, project: project) }
+  let!(:todo_conf_issue_non_member) { create(:todo, user: user, target: issue, project: project) }
+  let!(:todo_conf_issue_member)     { create(:todo, user: project_member, target: issue, project: project) }
+
+  describe '#execute' do
+    before do
+      project.add_developer(project_member)
+    end
+
+    context 'when a user leaves a project' do
+      subject { described_class.new(user.id, project.id, 'Project').execute }
+
+      context 'when project is private' do
+        it 'removes todos for a user who is not a member' do
+          expect { subject }.to change { Todo.count }.from(3).to(1)
+
+          expect(user.todos).to be_empty
+          expect(project_member.todos).to match_array([todo_conf_issue_member])
+        end
+      end
+
+      context 'when project is not private' do
+        before do
+          group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+          project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+        end
+
+        it 'removes only confidential issues todos' do
+          expect { subject }.to change { Todo.count }.from(3).to(2)
+        end
+      end
+    end
+
+    context 'when a user leaves a group' do
+      subject { described_class.new(user.id, group.id, 'Group').execute }
+
+      context 'when group is private' do
+        it 'removes todos for a user who is not a member' do
+          expect { subject }.to change { Todo.count }.from(3).to(1)
+
+          expect(user.todos).to be_empty
+          expect(project_member.todos).to match_array([todo_conf_issue_member])
+        end
+
+        context 'with nested groups', :nested_groups do
+          let(:subgroup) { create(:group, :private, parent: group) }
+          let(:subproject) { create(:project, group: subgroup) }
+
+          let!(:todo_subproject_non_member) { create(:todo, user: user, project: subproject) }
+          let!(:todo_subproject_member) { create(:todo, user: project_member, project: subproject) }
+
+          it 'removes todos for a user who is not a member' do
+            expect { subject }.to change { Todo.count }.from(5).to(2)
+
+            expect(user.todos).to be_empty
+            expect(project_member.todos)
+              .to match_array([todo_conf_issue_member, todo_subproject_member])
+          end
+        end
+      end
+
+      context 'when group is not private' do
+        before do
+          group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+        end
+
+        it 'removes only confidential issues todos' do
+          expect { subject }.to change { Todo.count }.from(3).to(2)
+        end
+      end
+    end
+
+    context 'when entity type is not valid' do
+      it 'raises an exception' do
+        expect { described_class.new(user.id, group.id, 'GroupWrongly').execute }
+          .to raise_error(ArgumentError)
+      end
+    end
+
+    context 'when entity was not found' do
+      it 'does not remove any todos' do
+        expect { described_class.new(user.id, 999999, 'Group').execute }
+          .not_to change { Todo.count }
+      end
+    end
+  end
+end
diff --git a/spec/services/todos/destroy/project_private_service_spec.rb b/spec/services/todos/destroy/project_private_service_spec.rb
new file mode 100644
index 00000000000..badf3f913a5
--- /dev/null
+++ b/spec/services/todos/destroy/project_private_service_spec.rb
@@ -0,0 +1,38 @@
+require 'spec_helper'
+
+describe Todos::Destroy::ProjectPrivateService do
+  let(:project)        { create(:project, :public) }
+  let(:user)           { create(:user) }
+  let(:project_member) { create(:user) }
+
+  let!(:todo_issue_non_member)   { create(:todo, user: user, project: project) }
+  let!(:todo_issue_member)       { create(:todo, user: project_member, project: project) }
+  let!(:todo_another_non_member) { create(:todo, user: user, project: project) }
+
+  describe '#execute' do
+    before do
+      project.add_developer(project_member)
+    end
+
+    subject { described_class.new(project.id).execute }
+
+    context 'when a project set to private' do
+      before do
+        project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+      end
+
+      it 'removes issue todos for a user who is not a member' do
+        expect { subject }.to change { Todo.count }.from(3).to(1)
+
+        expect(user.todos).to be_empty
+        expect(project_member.todos).to match_array([todo_issue_member])
+      end
+    end
+
+    context 'when project is not private' do
+      it 'does not remove any todos' do
+        expect { subject }.not_to change { Todo.count }
+      end
+    end
+  end
+end
diff --git a/spec/workers/todos_destroyer/confidential_issue_worker_spec.rb b/spec/workers/todos_destroyer/confidential_issue_worker_spec.rb
new file mode 100644
index 00000000000..9d7c0b8f560
--- /dev/null
+++ b/spec/workers/todos_destroyer/confidential_issue_worker_spec.rb
@@ -0,0 +1,12 @@
+require 'spec_helper'
+
+describe TodosDestroyer::ConfidentialIssueWorker do
+  it "calls the Todos::Destroy::ConfidentialIssueService with the params it was given" do
+    service = double
+
+    expect(::Todos::Destroy::ConfidentialIssueService).to receive(:new).with(100).and_return(service)
+    expect(service).to receive(:execute)
+
+    described_class.new.perform(100)
+  end
+end
diff --git a/spec/workers/todos_destroyer/entity_leave_worker_spec.rb b/spec/workers/todos_destroyer/entity_leave_worker_spec.rb
new file mode 100644
index 00000000000..955447906aa
--- /dev/null
+++ b/spec/workers/todos_destroyer/entity_leave_worker_spec.rb
@@ -0,0 +1,12 @@
+require 'spec_helper'
+
+describe TodosDestroyer::EntityLeaveWorker do
+  it "calls the Todos::Destroy::EntityLeaveService with the params it was given" do
+    service = double
+
+    expect(::Todos::Destroy::EntityLeaveService).to receive(:new).with(100, 5, 'Group').and_return(service)
+    expect(service).to receive(:execute)
+
+    described_class.new.perform(100, 5, 'Group')
+  end
+end
diff --git a/spec/workers/todos_destroyer/project_private_worker_spec.rb b/spec/workers/todos_destroyer/project_private_worker_spec.rb
new file mode 100644
index 00000000000..15d926fa9d5
--- /dev/null
+++ b/spec/workers/todos_destroyer/project_private_worker_spec.rb
@@ -0,0 +1,12 @@
+require 'spec_helper'
+
+describe TodosDestroyer::ProjectPrivateWorker do
+  it "calls the Todos::Destroy::ProjectPrivateService with the params it was given" do
+    service = double
+
+    expect(::Todos::Destroy::ProjectPrivateService).to receive(:new).with(100).and_return(service)
+    expect(service).to receive(:execute)
+
+    described_class.new.perform(100)
+  end
+end

From 7934b91311a70d994c6700201979c6673160fd01 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jarka=20Kadlecov=C3=A1?= <jarka@gitlab.com>
Date: Thu, 26 Jul 2018 16:53:50 +0200
Subject: [PATCH 2/3] Fix removing todos for confidential issues

- dont remove todos for authos & assignees
- remove todos for project guests
---
 .../destroy/confidential_issue_service.rb     |  9 +++++
 .../todos/destroy/entity_leave_service.rb     |  4 ++
 .../confidential_issue_service_spec.rb        | 12 +++++-
 .../destroy/entity_leave_service_spec.rb      | 38 ++++++++++++++++---
 4 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/app/services/todos/destroy/confidential_issue_service.rb b/app/services/todos/destroy/confidential_issue_service.rb
index 06cf308a3cd..c5b66df057a 100644
--- a/app/services/todos/destroy/confidential_issue_service.rb
+++ b/app/services/todos/destroy/confidential_issue_service.rb
@@ -14,6 +14,8 @@ module Todos
       override :todos
       def todos
         Todo.where(target: issue)
+          .where('user_id != ?', issue.author_id)
+          .where('user_id NOT IN (?)', issue.assignees.select(:id))
       end
 
       override :todos_to_remove?
@@ -25,6 +27,13 @@ module Todos
       def project_ids
         issue.project_id
       end
+
+      override :authorized_users
+      def authorized_users
+        ProjectAuthorization.select(:user_id)
+          .where(project_id: project_ids)
+          .where('access_level >= ?', Gitlab::Access::REPORTER)
+      end
     end
   end
 end
diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb
index 328a8b39e7b..129e5505a21 100644
--- a/app/services/todos/destroy/entity_leave_service.rb
+++ b/app/services/todos/destroy/entity_leave_service.rb
@@ -42,7 +42,11 @@ module Todos
       end
 
       def confidential_issues
+        assigned_ids = IssueAssignee.select(:issue_id).where(user_id: user_id)
+
         Issue.where(project_id: project_ids, confidential: true)
+          .where('author_id != ?', user_id)
+          .where('id NOT IN (?)', assigned_ids)
       end
     end
   end
diff --git a/spec/services/todos/destroy/confidential_issue_service_spec.rb b/spec/services/todos/destroy/confidential_issue_service_spec.rb
index 5c214df49bc..54d1d7e83f1 100644
--- a/spec/services/todos/destroy/confidential_issue_service_spec.rb
+++ b/spec/services/todos/destroy/confidential_issue_service_spec.rb
@@ -3,16 +3,23 @@ require 'spec_helper'
 describe Todos::Destroy::ConfidentialIssueService do
   let(:project)        { create(:project, :public) }
   let(:user)           { create(:user) }
+  let(:author)         { create(:user) }
+  let(:assignee)       { create(:user) }
+  let(:guest)          { create(:user) }
   let(:project_member) { create(:user) }
-  let(:issue)          { create(:issue, project: project) }
+  let(:issue)          { create(:issue, project: project, author: author, assignees: [assignee]) }
 
   let!(:todo_issue_non_member)   { create(:todo, user: user, target: issue, project: project) }
   let!(:todo_issue_member)       { create(:todo, user: project_member, target: issue, project: project) }
+  let!(:todo_issue_author)       { create(:todo, user: author, target: issue, project: project) }
+  let!(:todo_issue_asignee)      { create(:todo, user: assignee, target: issue, project: project) }
+  let!(:todo_issue_guest)        { create(:todo, user: guest, target: issue, project: project) }
   let!(:todo_another_non_member) { create(:todo, user: user, project: project) }
 
   describe '#execute' do
     before do
       project.add_developer(project_member)
+      project.add_guest(guest)
     end
 
     subject { described_class.new(issue.id).execute }
@@ -23,9 +30,10 @@ describe Todos::Destroy::ConfidentialIssueService do
       end
 
       it 'removes issue todos for a user who is not a project member' do
-        expect { subject }.to change { Todo.count }.from(3).to(2)
+        expect { subject }.to change { Todo.count }.from(6).to(4)
 
         expect(user.todos).to match_array([todo_another_non_member])
+        expect(author.todos).to match_array([todo_issue_author])
         expect(project_member.todos).to match_array([todo_issue_member])
       end
     end
diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb
index e5673383df8..52175ed9032 100644
--- a/spec/services/todos/destroy/entity_leave_service_spec.rb
+++ b/spec/services/todos/destroy/entity_leave_service_spec.rb
@@ -29,13 +29,41 @@ describe Todos::Destroy::EntityLeaveService do
       end
 
       context 'when project is not private' do
-        before do
-          group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
-          project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+        context 'when a user is not an author of confidential issue' do
+          before do
+            group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+            project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+          end
+
+          it 'removes only confidential issues todos' do
+            expect { subject }.to change { Todo.count }.from(3).to(2)
+          end
         end
 
-        it 'removes only confidential issues todos' do
-          expect { subject }.to change { Todo.count }.from(3).to(2)
+        context 'when a user is an author of confidential issue' do
+          before do
+            issue.update!(author: user)
+
+            group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+            project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+          end
+
+          it 'removes only confidential issues todos' do
+            expect { subject }.not_to change { Todo.count }
+          end
+        end
+
+        context 'when a user is an assignee of confidential issue' do
+          before do
+            issue.assignees << user
+
+            group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+            project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+          end
+
+          it 'removes only confidential issues todos' do
+            expect { subject }.not_to change { Todo.count }
+          end
         end
       end
     end

From bdc8396e25e6eba6edcf2896daa49bb49695ef8c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jarka=20Kadlecov=C3=A1?= <jarka@gitlab.com>
Date: Fri, 27 Jul 2018 11:28:17 +0200
Subject: [PATCH 3/3] Remove todos when project feature visibility changes

---
 app/services/projects/update_service.rb       |   9 ++
 app/services/todos/destroy/base_service.rb    |   6 +-
 .../todos/destroy/entity_leave_service.rb     |  20 ++-
 .../todos/destroy/private_features_service.rb |  40 +++++
 app/workers/all_queues.yml                    |   1 +
 .../private_features_worker.rb                |  10 ++
 spec/services/projects/update_service_spec.rb |  14 ++
 .../destroy/entity_leave_service_spec.rb      |  71 +++++----
 .../destroy/private_features_service_spec.rb  | 143 ++++++++++++++++++
 .../private_features_worker_spec.rb           |  12 ++
 10 files changed, 286 insertions(+), 40 deletions(-)
 create mode 100644 app/services/todos/destroy/private_features_service.rb
 create mode 100644 app/workers/todos_destroyer/private_features_worker.rb
 create mode 100644 spec/services/todos/destroy/private_features_service_spec.rb
 create mode 100644 spec/workers/todos_destroyer/private_features_worker_spec.rb

diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index a3c688f8a14..31ab4fbe49e 100644
--- a/app/services/projects/update_service.rb
+++ b/app/services/projects/update_service.rb
@@ -42,9 +42,18 @@ module Projects
     private
 
     def after_update
+      todos_features_changes = %w(
+        issues_access_level
+        merge_requests_access_level
+        repository_access_level
+      )
+      project_changed_feature_keys = project.project_feature.previous_changes.keys
+
       if project.previous_changes.include?(:visibility_level) && project.private?
         # don't enqueue immediately to prevent todos removal in case of a mistake
         TodosDestroyer::ProjectPrivateWorker.perform_in(1.hour, project.id)
+      elsif (project_changed_feature_keys & todos_features_changes).present?
+        TodosDestroyer::PrivateFeaturesWorker.perform_in(1.hour, project.id)
       end
 
       if project.previous_changes.include?('path')
diff --git a/app/services/todos/destroy/base_service.rb b/app/services/todos/destroy/base_service.rb
index 71cd7921f32..dff5e1f30e5 100644
--- a/app/services/todos/destroy/base_service.rb
+++ b/app/services/todos/destroy/base_service.rb
@@ -18,15 +18,15 @@ module Todos
       end
 
       def todos
-        # overridden in subclasses
+        raise NotImplementedError
       end
 
       def project_ids
-        # overridden in subclasses
+        raise NotImplementedError
       end
 
       def todos_to_remove?
-        # overridden in subclasses
+        raise NotImplementedError
       end
     end
   end
diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb
index 129e5505a21..2ff9f94b718 100644
--- a/app/services/todos/destroy/entity_leave_service.rb
+++ b/app/services/todos/destroy/entity_leave_service.rb
@@ -21,24 +21,30 @@ module Todos
         if entity.private?
           Todo.where(project_id: project_ids, user_id: user_id)
         else
-          Todo.where(target_id: confidential_issues.select(:id), target_type: Issue)
+          project_ids.each do |project_id|
+            TodosDestroyer::PrivateFeaturesWorker.perform_async(project_id, user_id)
+          end
+
+          Todo.where(
+            target_id: confidential_issues.select(:id), target_type: Issue, user_id: user_id
+          )
         end
       end
 
       override :project_ids
       def project_ids
-        if entity.is_a?(Project)
-          entity.id
-        else
+        case entity
+        when Project
+          [entity.id]
+        when Namespace
           Project.select(:id).where(namespace_id: entity.self_and_descendants.select(:id))
         end
       end
 
       override :todos_to_remove?
       def todos_to_remove?
-        return unless entity
-
-        entity.private? || confidential_issues.count > 0
+        # if an entity is provided we want to check always at least private features
+        !!entity
       end
 
       def confidential_issues
diff --git a/app/services/todos/destroy/private_features_service.rb b/app/services/todos/destroy/private_features_service.rb
new file mode 100644
index 00000000000..4d8e2877bfb
--- /dev/null
+++ b/app/services/todos/destroy/private_features_service.rb
@@ -0,0 +1,40 @@
+module Todos
+  module Destroy
+    class PrivateFeaturesService < ::Todos::Destroy::BaseService
+      attr_reader :project_ids, :user_id
+
+      def initialize(project_ids, user_id = nil)
+        @project_ids = project_ids
+        @user_id = user_id
+      end
+
+      def execute
+        ProjectFeature.where(project_id: project_ids).each do |project_features|
+          target_types = []
+          target_types << Issue if private?(project_features.issues_access_level)
+          target_types << MergeRequest if private?(project_features.merge_requests_access_level)
+          target_types << Commit if private?(project_features.repository_access_level)
+
+          next if target_types.empty?
+
+          remove_todos(project_features.project_id, target_types)
+        end
+      end
+
+      private
+
+      def private?(feature_level)
+        feature_level == ProjectFeature::PRIVATE
+      end
+
+      def remove_todos(project_id, target_types)
+        items = Todo.where(project_id: project_id)
+        items = items.where(user_id: user_id) if user_id
+
+        items.where('user_id NOT IN (?)', authorized_users)
+          .where(target_type: target_types)
+          .delete_all
+      end
+    end
+  end
+end
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index 4a6bee8af83..f2651cb54ec 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -76,6 +76,7 @@
 - todos_destroyer:todos_destroyer_confidential_issue
 - todos_destroyer:todos_destroyer_entity_leave
 - todos_destroyer:todos_destroyer_project_private
+- todos_destroyer:todos_destroyer_private_features
 
 - default
 - mailers # ActionMailer::DeliveryJob.queue_name
diff --git a/app/workers/todos_destroyer/private_features_worker.rb b/app/workers/todos_destroyer/private_features_worker.rb
new file mode 100644
index 00000000000..f457d5e0471
--- /dev/null
+++ b/app/workers/todos_destroyer/private_features_worker.rb
@@ -0,0 +1,10 @@
+module TodosDestroyer
+  class PrivateFeaturesWorker
+    include ApplicationWorker
+    include TodosDestroyerQueue
+
+    def perform(project_id, user_id = nil)
+      ::Todos::Destroy::PrivateFeaturesService.new(project_id, user_id).execute
+    end
+  end
+end
diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb
index d1686e1007c..e6871545a0b 100644
--- a/spec/services/projects/update_service_spec.rb
+++ b/spec/services/projects/update_service_spec.rb
@@ -188,6 +188,20 @@ describe Projects::UpdateService do
       end
     end
 
+    context 'when changing feature visibility to private' do
+      it 'updates the visibility correctly' do
+        expect(TodosDestroyer::PrivateFeaturesWorker)
+          .to receive(:perform_in).with(1.hour, project.id)
+
+        result = update_project(project, user, project_feature_attributes:
+                                 { issues_access_level: ProjectFeature::PRIVATE }
+                               )
+
+        expect(result).to eq({ status: :success })
+        expect(project.project_feature.issues_access_level).to be(ProjectFeature::PRIVATE)
+      end
+    end
+
     context 'when updating a project that contains container images' do
       before do
         stub_container_registry_config(enabled: true)
diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb
index 52175ed9032..bad408a314e 100644
--- a/spec/services/todos/destroy/entity_leave_service_spec.rb
+++ b/spec/services/todos/destroy/entity_leave_service_spec.rb
@@ -1,38 +1,39 @@
 require 'spec_helper'
 
 describe Todos::Destroy::EntityLeaveService do
-  let(:group)          { create(:group, :private) }
-  let(:project)        { create(:project, group: group) }
-  let(:user)           { create(:user) }
-  let(:project_member) { create(:user) }
-  let(:issue)          { create(:issue, :confidential, project: project) }
+  let(:group)   { create(:group, :private) }
+  let(:project) { create(:project, group: group) }
+  let(:user)    { create(:user) }
+  let(:user2)   { create(:user) }
+  let(:issue)   { create(:issue, project: project) }
+  let(:mr)      { create(:merge_request, source_project: project) }
 
-  let!(:todo_non_member)            { create(:todo, user: user, project: project) }
-  let!(:todo_conf_issue_non_member) { create(:todo, user: user, target: issue, project: project) }
-  let!(:todo_conf_issue_member)     { create(:todo, user: project_member, target: issue, project: project) }
+  let!(:todo_mr_user)     { create(:todo, user: user, target: mr, project: project) }
+  let!(:todo_issue_user)  { create(:todo, user: user, target: issue, project: project) }
+  let!(:todo_issue_user2) { create(:todo, user: user2, target: issue, project: project) }
 
   describe '#execute' do
-    before do
-      project.add_developer(project_member)
-    end
-
     context 'when a user leaves a project' do
       subject { described_class.new(user.id, project.id, 'Project').execute }
 
       context 'when project is private' do
-        it 'removes todos for a user who is not a member' do
+        it 'removes todos for the provided user' do
           expect { subject }.to change { Todo.count }.from(3).to(1)
 
           expect(user.todos).to be_empty
-          expect(project_member.todos).to match_array([todo_conf_issue_member])
+          expect(user2.todos).to match_array([todo_issue_user2])
         end
       end
 
       context 'when project is not private' do
+        before do
+          group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+          project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+        end
+
         context 'when a user is not an author of confidential issue' do
           before do
-            group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
-            project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+            issue.update!(confidential: true)
           end
 
           it 'removes only confidential issues todos' do
@@ -42,10 +43,7 @@ describe Todos::Destroy::EntityLeaveService do
 
         context 'when a user is an author of confidential issue' do
           before do
-            issue.update!(author: user)
-
-            group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
-            project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+            issue.update!(author: user, confidential: true)
           end
 
           it 'removes only confidential issues todos' do
@@ -55,16 +53,26 @@ describe Todos::Destroy::EntityLeaveService do
 
         context 'when a user is an assignee of confidential issue' do
           before do
+            issue.update!(confidential: true)
             issue.assignees << user
-
-            group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
-            project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
           end
 
           it 'removes only confidential issues todos' do
             expect { subject }.not_to change { Todo.count }
           end
         end
+
+        context 'feature visibility check' do
+          context 'when issues are visible only to project members' do
+            before do
+              project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+            end
+
+            it 'removes only users issue todos' do
+              expect { subject }.to change { Todo.count }.from(3).to(2)
+            end
+          end
+        end
       end
     end
 
@@ -72,33 +80,36 @@ describe Todos::Destroy::EntityLeaveService do
       subject { described_class.new(user.id, group.id, 'Group').execute }
 
       context 'when group is private' do
-        it 'removes todos for a user who is not a member' do
+        it 'removes todos for the user' do
           expect { subject }.to change { Todo.count }.from(3).to(1)
 
           expect(user.todos).to be_empty
-          expect(project_member.todos).to match_array([todo_conf_issue_member])
+          expect(user2.todos).to match_array([todo_issue_user2])
         end
 
         context 'with nested groups', :nested_groups do
           let(:subgroup) { create(:group, :private, parent: group) }
           let(:subproject) { create(:project, group: subgroup) }
 
-          let!(:todo_subproject_non_member) { create(:todo, user: user, project: subproject) }
-          let!(:todo_subproject_member) { create(:todo, user: project_member, project: subproject) }
+          let!(:todo_subproject_user) { create(:todo, user: user, project: subproject) }
+          let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) }
 
-          it 'removes todos for a user who is not a member' do
+          it 'removes todos for the user including subprojects todos' do
             expect { subject }.to change { Todo.count }.from(5).to(2)
 
             expect(user.todos).to be_empty
-            expect(project_member.todos)
-              .to match_array([todo_conf_issue_member, todo_subproject_member])
+            expect(user2.todos)
+              .to match_array([todo_issue_user2, todo_subproject_user2])
           end
         end
       end
 
       context 'when group is not private' do
         before do
+          issue.update!(confidential: true)
+
           group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+          project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
         end
 
         it 'removes only confidential issues todos' do
diff --git a/spec/services/todos/destroy/private_features_service_spec.rb b/spec/services/todos/destroy/private_features_service_spec.rb
new file mode 100644
index 00000000000..be8b5bb3979
--- /dev/null
+++ b/spec/services/todos/destroy/private_features_service_spec.rb
@@ -0,0 +1,143 @@
+require 'spec_helper'
+
+describe Todos::Destroy::PrivateFeaturesService do
+  let(:project)        { create(:project, :public) }
+  let(:user)           { create(:user) }
+  let(:another_user)   { create(:user) }
+  let(:project_member) { create(:user) }
+  let(:issue)          { create(:issue, project: project) }
+  let(:mr)             { create(:merge_request, source_project: project) }
+
+  let!(:todo_mr_non_member)      { create(:todo, user: user, target: mr, project: project) }
+  let!(:todo_mr_non_member2)     { create(:todo, user: another_user, target: mr, project: project) }
+  let!(:todo_mr_member)          { create(:todo, user: project_member, target: mr, project: project) }
+  let!(:todo_issue_non_member)   { create(:todo, user: user, target: issue, project: project) }
+  let!(:todo_issue_non_member2)  { create(:todo, user: another_user, target: issue, project: project) }
+  let!(:todo_issue_member)       { create(:todo, user: project_member, target: issue, project: project) }
+  let!(:commit_todo_non_member)  { create(:on_commit_todo, user: user, project: project) }
+  let!(:commit_todo_non_member2) { create(:on_commit_todo, user: another_user, project: project) }
+  let!(:commit_todo_member)      { create(:on_commit_todo, user: project_member, project: project) }
+
+  before do
+    project.add_developer(project_member)
+  end
+
+  context 'when user_id is provided' do
+    subject { described_class.new(project.id, user.id).execute }
+
+    context 'when all feaures have same visibility as the project' do
+      it 'removes only user issue todos' do
+        expect { subject }.not_to change { Todo.count }
+      end
+    end
+
+    context 'when issues are visible only to project members but the user is a member' do
+      before do
+        project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+        project.add_developer(user)
+      end
+
+      it 'does not remove any todos' do
+        expect { subject }.not_to change { Todo.count }
+      end
+    end
+
+    context 'when issues are visible only to project members' do
+      before do
+        project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+      end
+
+      it 'removes only user issue todos' do
+        expect { subject }.to change { Todo.count }.from(9).to(8)
+      end
+    end
+
+    context 'when mrs, builds and repository are visible only to project members' do
+      before do
+        # builds and merge requests cannot have higher visibility than repository
+        project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+        project.project_feature.update!(builds_access_level: ProjectFeature::PRIVATE)
+        project.project_feature.update!(repository_access_level: ProjectFeature::PRIVATE)
+      end
+
+      it 'removes only user mr and commit todos' do
+        expect { subject }.to change { Todo.count }.from(9).to(7)
+      end
+    end
+
+    context 'when mrs are visible only to project members' do
+      before do
+        project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+      end
+
+      it 'removes only user merge request todo' do
+        expect { subject }.to change { Todo.count }.from(9).to(8)
+      end
+    end
+
+    context 'when mrs and issues are visible only to project members' do
+      before do
+        project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+        project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+      end
+
+      it 'removes only user merge request and issue todos' do
+        expect { subject }.to change { Todo.count }.from(9).to(7)
+      end
+    end
+  end
+
+  context 'when user_id is not provided' do
+    subject { described_class.new(project.id).execute }
+
+    context 'when all feaures have same visibility as the project' do
+      it 'does not remove any todos' do
+        expect { subject }.not_to change { Todo.count }
+      end
+    end
+
+    context 'when issues are visible only to project members' do
+      before do
+        project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+      end
+
+      it 'removes only non members issue todos' do
+        expect { subject }.to change { Todo.count }.from(9).to(7)
+      end
+    end
+
+    context 'when mrs, builds and repository are visible only to project members' do
+      before do
+        # builds and merge requests cannot have higher visibility than repository
+        project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+        project.project_feature.update!(builds_access_level: ProjectFeature::PRIVATE)
+        project.project_feature.update!(repository_access_level: ProjectFeature::PRIVATE)
+      end
+
+      it 'removes only non members mr and commit todos' do
+        expect { subject }.to change { Todo.count }.from(9).to(5)
+      end
+    end
+
+    context 'when mrs are visible only to project members' do
+      before do
+        project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+      end
+
+      it 'removes only non members merge request todos' do
+        expect { subject }.to change { Todo.count }.from(9).to(7)
+      end
+    end
+
+    context 'when mrs and issues are visible only to project members' do
+      before do
+        project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+        project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+      end
+
+      it 'removes only non members merge request and issue todos' do
+        expect { subject }.to change { Todo.count }.from(9).to(5)
+      end
+    end
+  end
+end
diff --git a/spec/workers/todos_destroyer/private_features_worker_spec.rb b/spec/workers/todos_destroyer/private_features_worker_spec.rb
new file mode 100644
index 00000000000..9599f5ee071
--- /dev/null
+++ b/spec/workers/todos_destroyer/private_features_worker_spec.rb
@@ -0,0 +1,12 @@
+require 'spec_helper'
+
+describe TodosDestroyer::PrivateFeaturesWorker do
+  it "calls the Todos::Destroy::PrivateFeaturesService with the params it was given" do
+    service = double
+
+    expect(::Todos::Destroy::PrivateFeaturesService).to receive(:new).with(100, nil).and_return(service)
+    expect(service).to receive(:execute)
+
+    described_class.new.perform(100)
+  end
+end