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..31ab4fbe49e 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,30 @@ 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') + 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..dff5e1f30e5 --- /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 + raise NotImplementedError + end + + def project_ids + raise NotImplementedError + end + + def todos_to_remove? + raise NotImplementedError + 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..c5b66df057a --- /dev/null +++ b/app/services/todos/destroy/confidential_issue_service.rb @@ -0,0 +1,39 @@ +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) + .where('user_id != ?', issue.author_id) + .where('user_id NOT IN (?)', issue.assignees.select(:id)) + end + + override :todos_to_remove? + def todos_to_remove? + issue&.confidential? + end + + override :project_ids + 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 new file mode 100644 index 00000000000..2ff9f94b718 --- /dev/null +++ b/app/services/todos/destroy/entity_leave_service.rb @@ -0,0 +1,59 @@ +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 + 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 + 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? + # if an entity is provided we want to check always at least private features + !!entity + 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 +end 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/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..f2651cb54ec 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -73,6 +73,11 @@ - 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 +- todos_destroyer:todos_destroyer_private_features + - 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/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/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..e6871545a0b 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 @@ -166,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/confidential_issue_service_spec.rb b/spec/services/todos/destroy/confidential_issue_service_spec.rb new file mode 100644 index 00000000000..54d1d7e83f1 --- /dev/null +++ b/spec/services/todos/destroy/confidential_issue_service_spec.rb @@ -0,0 +1,47 @@ +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, 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 } + + 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(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 + + 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..bad408a314e --- /dev/null +++ b/spec/services/todos/destroy/entity_leave_service_spec.rb @@ -0,0 +1,135 @@ +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 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/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/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 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