Merge branch 'todos-visibility-change' into 'master'

Delete todos when users loses target read permissions

See merge request gitlab-org/gitlab-ce!20665

spec/services/todos/destroy/entity_leave_service_spec.rb

+require 'spec_helper'
+
+describe Todos::Destroy::EntityLeaveService do
+  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_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
+    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 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])
+        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
+            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)
+          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.update!(confidential: true)
+            issue.assignees << user
+          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
+
+    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 the 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])
+        end
+
+        context 'with nested groups', :nested_groups do
+          let(:subgroup) { create(:group, :private, parent: group) }
+          let(:subproject) { create(:project, group: subgroup) }
+
+          let!(:todo_subproject_user) { create(:todo, user: user, project: subproject) }
+          let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) }
+
+          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(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
+          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

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/services/todos/destroy/project_private_service_spec.rb

+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

spec/workers/todos_destroyer/confidential_issue_worker_spec.rb

+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

spec/workers/todos_destroyer/entity_leave_worker_spec.rb

+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

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

spec/workers/todos_destroyer/project_private_worker_spec.rb

+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