diff options
author | Jarka Kadlecová <jarka@gitlab.com> | 2018-07-27 11:28:17 +0200 |
---|---|---|
committer | Jarka Kadlecová <jarka@gitlab.com> | 2018-07-31 12:32:08 +0200 |
commit | bdc8396e25e6eba6edcf2896daa49bb49695ef8c (patch) | |
tree | d08f569da8eaf56d192c631fa8dfd3ee791cb66b | |
parent | 7934b91311a70d994c6700201979c6673160fd01 (diff) | |
download | gitlab-ce-bdc8396e25e6eba6edcf2896daa49bb49695ef8c.tar.gz |
Remove todos when project feature visibility changes
-rw-r--r-- | app/services/projects/update_service.rb | 9 | ||||
-rw-r--r-- | app/services/todos/destroy/base_service.rb | 6 | ||||
-rw-r--r-- | app/services/todos/destroy/entity_leave_service.rb | 20 | ||||
-rw-r--r-- | app/services/todos/destroy/private_features_service.rb | 40 | ||||
-rw-r--r-- | app/workers/all_queues.yml | 1 | ||||
-rw-r--r-- | app/workers/todos_destroyer/private_features_worker.rb | 10 | ||||
-rw-r--r-- | spec/services/projects/update_service_spec.rb | 14 | ||||
-rw-r--r-- | spec/services/todos/destroy/entity_leave_service_spec.rb | 71 | ||||
-rw-r--r-- | spec/services/todos/destroy/private_features_service_spec.rb | 143 | ||||
-rw-r--r-- | spec/workers/todos_destroyer/private_features_worker_spec.rb | 12 |
10 files changed, 286 insertions, 40 deletions
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 |