Remove todos when project feature visibility changes

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')

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

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

app/services/todos/destroy/private_features_service.rb

+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

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

app/workers/todos_destroyer/private_features_worker.rb

+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

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)

spec/services/todos/destroy/entity_leave_service_spec.rb

@@ -4,37 +4,38 @@ 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(: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
-        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
 
+        context 'when a user is not an author of confidential issue' do
+          before do
+            issue.update!(confidential: true)
+          end
+
           it 'removes only confidential issues todos' do
             expect { subject }.to change { Todo.count }.from(3).to(2)
           end
@@ -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

spec/services/todos/destroy/private_features_service_spec.rb

+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

spec/workers/todos_destroyer/private_features_worker_spec.rb

+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