Remove confidential issue todos for project guestsfix-conf-issues-remove

4 files changed, 104 insertions, 31 deletions

diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb
index 2ff9f94b718..13983417e2a 100644
--- a/app/services/todos/destroy/entity_leave_service.rb
+++ b/app/services/todos/destroy/entity_leave_service.rb
@@ -14,23 +14,42 @@ module Todos
         @entity = entity_type.constantize.find_by(id: entity_id)
       end
 
-      private
+      def execute
+        return unless entity
+        # only reporters can see confidential issues
+        return if has_reporter_permissions?
 
-      override :todos
-      def todos
-        if entity.private?
-          Todo.where(project_id: project_ids, user_id: user_id)
+        if user_project_authorization
+          remove_confidential_issue_todos
         else
-          project_ids.each do |project_id|
-            TodosDestroyer::PrivateFeaturesWorker.perform_async(project_id, user_id)
+          if entity.private?
+            remove_project_todos
+          else
+            remove_confidential_issue_todos
+            project_ids.each do |project_id|
+              TodosDestroyer::PrivateFeaturesWorker.perform_async(project_id, user_id)
+            end
           end
-
-          Todo.where(
-            target_id: confidential_issues.select(:id), target_type: Issue, user_id: user_id
-          )
         end
       end
 
+      private
+
+      def user_project_authorization
+        ProjectAuthorization.select(:user_id, :access_level)
+          .where(project_id: project_ids, user_id: user_id).first
+      end
+
+      def has_reporter_permissions?
+        return unless user_project_authorization
+
+        user_project_authorization.access_level >= Gitlab::Access::REPORTER
+      end
+
+      def remove_project_todos
+        Todo.where(project_id: project_ids, user_id: user_id).delete_all
+      end
+
       override :project_ids
       def project_ids
         case entity
@@ -41,10 +60,10 @@ module Todos
         end
       end
 
-      override :todos_to_remove?
-      def todos_to_remove?
-        # if an entity is provided we want to check always at least private features
-        !!entity
+      def remove_confidential_issue_todos
+        Todo.where(
+          target_id: confidential_issues.select(:id), target_type: Issue, user_id: user_id
+        ).delete_all
       end
 
       def confidential_issues
diff --git a/app/services/todos/destroy/project_private_service.rb b/app/services/todos/destroy/project_private_service.rb
index 171933e7cbc..440ddf48738 100644
--- a/app/services/todos/destroy/project_private_service.rb
+++ b/app/services/todos/destroy/project_private_service.rb
@@ -9,6 +9,14 @@ module Todos
         @project = Project.find_by(id: project_id)
       end
 
+      def execute
+        Issue.where(project_id: project_ids, confidential: true).each do |issue|
+          TodosDestroyer::ConfidentialIssueWorker.perform_async(issue.id)
+        end
+
+        super
+      end
+
       private
 
       override :todos
diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb
index bad408a314e..d34f0153410 100644
--- a/spec/services/todos/destroy/entity_leave_service_spec.rb
+++ b/spec/services/todos/destroy/entity_leave_service_spec.rb
@@ -14,14 +14,40 @@ describe Todos::Destroy::EntityLeaveService do
 
   describe '#execute' do
     context 'when a user leaves a project' do
+      before do
+        issue.update!(confidential: true)
+      end
+
       subject { described_class.new(user.id, project.id, 'Project').execute }
 
       context 'when project is private' do
-        it 'removes todos for the provided user' do
-          expect { subject }.to change { Todo.count }.from(3).to(1)
+        context 'when the provided user is not a project 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(user2.todos).to match_array([todo_issue_user2])
+            expect(user.todos).to be_empty
+            expect(user2.todos).to match_array([todo_issue_user2])
+          end
+        end
+
+        context 'when the provided user is guest' do
+          before do
+            project.add_guest(user)
+          end
+
+          it 'removes only confidential issues todos for the provided user' do
+            expect { subject }.to change { Todo.count }.from(3).to(2)
+          end
+        end
+
+        context 'when the provided user is developer' do
+          before do
+            project.add_developer(user)
+          end
+
+          it 'does not remove any todos for the provided user' do
+            expect { subject }.not_to change { Todo.count }
+          end
         end
       end
 
@@ -41,6 +67,17 @@ describe Todos::Destroy::EntityLeaveService do
           end
         end
 
+        context 'when a user is a guest of the project' do
+          before do
+            project.add_guest(user)
+            issue.update!(confidential: true)
+          end
+
+          it 'removes only confidential issues todos' do
+            expect { subject }.to change { Todo.count }.from(3).to(2)
+          end
+        end
+
         context 'when a user is an author of confidential issue' do
           before do
             issue.update!(author: user, confidential: true)
diff --git a/spec/services/todos/destroy/project_private_service_spec.rb b/spec/services/todos/destroy/project_private_service_spec.rb
index badf3f913a5..7a4625c633a 100644
--- a/spec/services/todos/destroy/project_private_service_spec.rb
+++ b/spec/services/todos/destroy/project_private_service_spec.rb
@@ -1,17 +1,25 @@
 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) }
+  let(:project)            { create(:project, :public) }
+  let(:user)               { create(:user) }
+  let(:project_member)     { create(:user) }
+  let(:guest)              { create(:user) }
+  let(:issue)              { create(:issue, project: project) }
+  let(:confidential_issue) { create(:issue, confidential: true, project: project) }
+
+  let!(:todo_issue_non_member)     { create(:todo, user: user, project: project, target: issue) }
+  let!(:todo_issue_guest)          { create(:todo, user: guest, project: project, target: issue) }
+  let!(:todo_issue_member)         { create(:todo, user: project_member, project: project, target: issue) }
+  let!(:todo_conf_issue_non_guest) { create(:todo, user: user, project: project, target: confidential_issue) }
+  let!(:todo_conf_issue_guest)     { create(:todo, user: guest, project: project, target: confidential_issue) }
+  let!(:todo_conf_issue_member)    { create(:todo, user: project_member, project: project, target: confidential_issue) }
+  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(project.id).execute }
@@ -21,17 +29,18 @@ describe Todos::Destroy::ProjectPrivateService 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)
+      it 'removes todos of non members and confidential issue todos of guests' do
+        expect { subject }.to change { Todo.count }.from(7).to(3)
 
         expect(user.todos).to be_empty
-        expect(project_member.todos).to match_array([todo_issue_member])
+        expect(guest.todos).to match_array([todo_issue_guest])
+        expect(project_member.todos).to match_array([todo_issue_member, todo_conf_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 }
+      it 'removes only confidential issue todos for guests and non members' do
+        expect { subject }.to change { Todo.count }.from(7).to(5)
       end
     end
   end