diff options
| -rw-r--r-- | app/models/error_tracking/project_error_tracking_setting.rb | 8 | ||||
| -rw-r--r-- | app/models/grafana_integration.rb | 8 | ||||
| -rw-r--r-- | app/models/todo.rb | 1 | ||||
| -rw-r--r-- | app/services/groups/destroy_service.rb | 15 | ||||
| -rw-r--r-- | app/services/todos/destroy/entity_leave_service.rb | 9 | ||||
| -rw-r--r-- | db/post_migrate/20220712175029_add_index_with_target_type_to_todos.rb | 18 | ||||
| -rw-r--r-- | db/post_migrate/20220712181304_remove_deprecated_indexes_from_todos.rb | 23 | ||||
| -rw-r--r-- | db/schema_migrations/20220712175029 | 1 | ||||
| -rw-r--r-- | db/schema_migrations/20220712181304 | 1 | ||||
| -rw-r--r-- | db/structure.sql | 8 | ||||
| -rw-r--r-- | spec/models/error_tracking/project_error_tracking_setting_spec.rb | 32 | ||||
| -rw-r--r-- | spec/models/grafana_integration_spec.rb | 34 | ||||
| -rw-r--r-- | spec/models/todo_spec.rb | 10 | ||||
| -rw-r--r-- | spec/services/groups/destroy_service_spec.rb | 14 | ||||
| -rw-r--r-- | spec/services/projects/operations/update_service_spec.rb | 7 | ||||
| -rw-r--r-- | spec/services/todos/destroy/entity_leave_service_spec.rb | 122 |
16 files changed, 253 insertions, 58 deletions
diff --git a/app/models/error_tracking/project_error_tracking_setting.rb b/app/models/error_tracking/project_error_tracking_setting.rb index 30382a1c205..4953f24755c 100644 --- a/app/models/error_tracking/project_error_tracking_setting.rb +++ b/app/models/error_tracking/project_error_tracking_setting.rb @@ -44,6 +44,8 @@ module ErrorTracking key: Settings.attr_encrypted_db_key_base_32, algorithm: 'aes-256-gcm' + before_validation :reset_token + after_save :clear_reactive_cache! # When a user enables the integrated error tracking @@ -182,6 +184,12 @@ module ErrorTracking private + def reset_token + if api_url_changed? && !encrypted_token_changed? + self.token = nil + end + end + def ensure_issue_belongs_to_project!(project_id_from_api) raise 'The Sentry issue appers to be outside of the configured Sentry project' if Integer(project_id_from_api) != ensure_sentry_project_id! end diff --git a/app/models/grafana_integration.rb b/app/models/grafana_integration.rb index 00213732fee..0358e37c58b 100644 --- a/app/models/grafana_integration.rb +++ b/app/models/grafana_integration.rb @@ -18,6 +18,8 @@ class GrafanaIntegration < ApplicationRecord validates :enabled, inclusion: { in: [true, false] } + before_validation :reset_token + scope :enabled, -> { where(enabled: true) } def client @@ -36,6 +38,12 @@ class GrafanaIntegration < ApplicationRecord private + def reset_token + if grafana_url_changed? && !encrypted_token_changed? + self.token = nil + end + end + def token decrypt(:token, encrypted_token) end diff --git a/app/models/todo.rb b/app/models/todo.rb index cff7a93f72f..c698783d750 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -74,6 +74,7 @@ class Todo < ApplicationRecord scope :for_commit, -> (id) { where(commit_id: id) } scope :with_entity_associations, -> { preload(:target, :author, :note, group: :route, project: [:route, { namespace: [:route, :owner] }]) } scope :joins_issue_and_assignees, -> { left_joins(issue: :assignees) } + scope :for_internal_notes, -> { joins(:note).where(note: { confidential: true }) } enum resolved_by_action: { system_done: 0, api_all_done: 1, api_done: 2, mark_all_done: 3, mark_done: 4 }, _prefix: :resolved_by diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb index c88c139a22e..bcf3110ca21 100644 --- a/app/services/groups/destroy_service.rb +++ b/app/services/groups/destroy_service.rb @@ -35,6 +35,8 @@ module Groups user_ids_for_project_authorizations_refresh = obtain_user_ids_for_project_authorizations_refresh + destroy_group_bots + group.destroy if user_ids_for_project_authorizations_refresh.present? @@ -76,6 +78,19 @@ module Groups group.users_ids_of_direct_members end + + # rubocop:disable CodeReuse/ActiveRecord + def destroy_group_bots + bot_ids = group.members_and_requesters.joins(:user).merge(User.project_bot).pluck(:user_id) + current_user_id = current_user.id + + group.run_after_commit do + bot_ids.each do |user_id| + DeleteUserWorker.perform_async(current_user_id, user_id, skip_authorization: true) + end + end + end + # rubocop:enable CodeReuse/ActiveRecord end end diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb index 1fe397d24e7..5b04d2fd3af 100644 --- a/app/services/todos/destroy/entity_leave_service.rb +++ b/app/services/todos/destroy/entity_leave_service.rb @@ -41,11 +41,20 @@ module Todos end def remove_confidential_resource_todos + # Deletes todos for confidential issues Todo .for_target(confidential_issues.select(:id)) .for_type(Issue.name) .for_user(user) .delete_all + + # Deletes todos for internal notes on unauthorized projects + Todo + .for_type(Issue.name) + .for_internal_notes + .for_project(non_authorized_reporter_projects) # Only Reporter+ can read internal notes + .for_user(user) + .delete_all end def remove_project_todos diff --git a/db/post_migrate/20220712175029_add_index_with_target_type_to_todos.rb b/db/post_migrate/20220712175029_add_index_with_target_type_to_todos.rb new file mode 100644 index 00000000000..077e8ed4bbe --- /dev/null +++ b/db/post_migrate/20220712175029_add_index_with_target_type_to_todos.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class AddIndexWithTargetTypeToTodos < Gitlab::Database::Migration[2.0] + disable_ddl_transaction! + + INDEX_FOR_PROJECTS_NAME = 'index_requirements_project_id_user_id_id_and_target_type' + INDEX_FOR_TARGET_TYPE_NAME = 'index_requirements_user_id_and_target_type' + + def up + add_concurrent_index :todos, [:project_id, :user_id, :id, :target_type], name: INDEX_FOR_PROJECTS_NAME + add_concurrent_index :todos, [:user_id, :target_type], name: INDEX_FOR_TARGET_TYPE_NAME + end + + def down + remove_concurrent_index_by_name :todos, INDEX_FOR_PROJECTS_NAME + remove_concurrent_index_by_name :todos, INDEX_FOR_TARGET_TYPE_NAME + end +end diff --git a/db/post_migrate/20220712181304_remove_deprecated_indexes_from_todos.rb b/db/post_migrate/20220712181304_remove_deprecated_indexes_from_todos.rb new file mode 100644 index 00000000000..4f99caa10a4 --- /dev/null +++ b/db/post_migrate/20220712181304_remove_deprecated_indexes_from_todos.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +class RemoveDeprecatedIndexesFromTodos < Gitlab::Database::Migration[2.0] + disable_ddl_transaction! + + PROJECTS_INDEX = 'index_todos_on_project_id_and_user_id_and_id' + USERS_INDEX = 'index_todos_on_user_id' + + # These indexes are deprecated in favor of two new ones + # added in a previous migration: + # + # * index_requirements_project_id_user_id_target_type_and_id + # * index_requirements_user_id_and_target_type + def up + remove_concurrent_index_by_name :todos, PROJECTS_INDEX + remove_concurrent_index_by_name :todos, USERS_INDEX + end + + def down + add_concurrent_index :todos, [:project_id, :user_id, :id], name: PROJECTS_INDEX + add_concurrent_index :todos, :user_id, name: USERS_INDEX + end +end diff --git a/db/schema_migrations/20220712175029 b/db/schema_migrations/20220712175029 new file mode 100644 index 00000000000..bb7fdca340f --- /dev/null +++ b/db/schema_migrations/20220712175029 @@ -0,0 +1 @@ +f6638435457f57f5c566e107de4f4557a1d87b5dd27acc9e5345999197d18e6e
\ No newline at end of file diff --git a/db/schema_migrations/20220712181304 b/db/schema_migrations/20220712181304 new file mode 100644 index 00000000000..ff111fe7c28 --- /dev/null +++ b/db/schema_migrations/20220712181304 @@ -0,0 +1 @@ +ff9ad44a43be82867da8e0f51e68a2284065cab6b2eb7cf6496108dce1cdd657
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index cb0d4696931..28082885574 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -29549,6 +29549,10 @@ CREATE INDEX index_requirements_on_title_trigram ON requirements USING gin (titl CREATE INDEX index_requirements_on_updated_at ON requirements USING btree (updated_at); +CREATE INDEX index_requirements_project_id_user_id_id_and_target_type ON todos USING btree (project_id, user_id, id, target_type); + +CREATE INDEX index_requirements_user_id_and_target_type ON todos USING btree (user_id, target_type); + CREATE INDEX index_resource_iteration_events_on_issue_id ON resource_iteration_events USING btree (issue_id); CREATE INDEX index_resource_iteration_events_on_iteration_id ON resource_iteration_events USING btree (iteration_id); @@ -29867,12 +29871,8 @@ CREATE INDEX index_todos_on_note_id ON todos USING btree (note_id); CREATE INDEX index_todos_on_project_id_and_id ON todos USING btree (project_id, id); -CREATE INDEX index_todos_on_project_id_and_user_id_and_id ON todos USING btree (project_id, user_id, id); - CREATE INDEX index_todos_on_target_type_and_target_id ON todos USING btree (target_type, target_id); -CREATE INDEX index_todos_on_user_id ON todos USING btree (user_id); - CREATE INDEX index_todos_on_user_id_and_id_done ON todos USING btree (user_id, id) WHERE ((state)::text = 'done'::text); CREATE INDEX index_todos_on_user_id_and_id_pending ON todos USING btree (user_id, id) WHERE ((state)::text = 'pending'::text); diff --git a/spec/models/error_tracking/project_error_tracking_setting_spec.rb b/spec/models/error_tracking/project_error_tracking_setting_spec.rb index 15b6b45eaba..ebfd9f04f6a 100644 --- a/spec/models/error_tracking/project_error_tracking_setting_spec.rb +++ b/spec/models/error_tracking/project_error_tracking_setting_spec.rb @@ -123,6 +123,38 @@ RSpec.describe ErrorTracking::ProjectErrorTrackingSetting do end end + describe 'before_validation :reset_token' do + context 'when a token was previously set' do + subject { create(:project_error_tracking_setting, project: project) } + + it 'resets token if url changed' do + subject.api_url = 'http://sentry.com/api/0/projects/org-slug/proj-slug/' + + expect(subject).not_to be_valid + expect(subject.token).to be_nil + end + + it "does not reset token if new url is set together with the same token" do + subject.api_url = 'http://sentrytest.com/api/0/projects/org-slug/proj-slug/' + current_token = subject.token + subject.token = current_token + + expect(subject).to be_valid + expect(subject.token).to eq(current_token) + expect(subject.api_url).to eq('http://sentrytest.com/api/0/projects/org-slug/proj-slug/') + end + + it 'does not reset token if new url is set together with a new token' do + subject.api_url = 'http://sentrytest.com/api/0/projects/org-slug/proj-slug/' + subject.token = 'token' + + expect(subject).to be_valid + expect(subject.token).to eq('token') + expect(subject.api_url).to eq('http://sentrytest.com/api/0/projects/org-slug/proj-slug/') + end + end + end + describe '.extract_sentry_external_url' do subject { described_class.extract_sentry_external_url(sentry_url) } diff --git a/spec/models/grafana_integration_spec.rb b/spec/models/grafana_integration_spec.rb index bb822187e0c..73ec2856c05 100644 --- a/spec/models/grafana_integration_spec.rb +++ b/spec/models/grafana_integration_spec.rb @@ -86,4 +86,38 @@ RSpec.describe GrafanaIntegration do end end end + + describe 'Callbacks' do + describe 'before_validation :reset_token' do + context 'when a token was previously set' do + subject(:grafana_integration) { create(:grafana_integration) } + + it 'resets token if url changed' do + grafana_integration.grafana_url = 'http://gitlab1.com' + + expect(grafana_integration).not_to be_valid + expect(grafana_integration.send(:token)).to be_nil + end + + it "does not reset token if new url is set together with the same token" do + grafana_integration.grafana_url = 'http://gitlab_edited.com' + current_token = grafana_integration.send(:token) + grafana_integration.token = current_token + + expect(grafana_integration).to be_valid + expect(grafana_integration.send(:token)).to eq(current_token) + expect(grafana_integration.grafana_url).to eq('http://gitlab_edited.com') + end + + it 'does not reset token if new url is set together with a new token' do + grafana_integration.grafana_url = 'http://gitlab_edited.com' + grafana_integration.token = 'token' + + expect(grafana_integration).to be_valid + expect(grafana_integration.send(:token)).to eq('token') + expect(grafana_integration.grafana_url).to eq('http://gitlab_edited.com') + end + end + end + end end diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb index 7df22078c6d..18b0cb36cc6 100644 --- a/spec/models/todo_spec.rb +++ b/spec/models/todo_spec.rb @@ -495,4 +495,14 @@ RSpec.describe Todo do it { is_expected.to contain_exactly(user1.id, user2.id) } end + + describe '.for_internal_notes' do + it 'returns todos created from internal notes' do + internal_note = create(:note, confidential: true ) + todo = create(:todo, note: internal_note) + create(:todo) + + expect(described_class.for_internal_notes).to contain_exactly(todo) + end + end end diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index 57a151efda6..f43f64fdf89 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -35,6 +35,20 @@ RSpec.describe Groups::DestroyService do it { expect(NotificationSetting.unscoped.all).not_to include(notification_setting) } end + context 'bot tokens', :sidekiq_might_not_need_inline do + it 'removes group bot', :aggregate_failures do + bot = create(:user, :project_bot) + group.add_developer(bot) + token = create(:personal_access_token, user: bot) + + destroy_group(group, user, async) + + expect(PersonalAccessToken.find_by(id: token.id)).to be_nil + expect(User.find_by(id: bot.id)).to be_nil + expect(User.find_by(id: user.id)).not_to be_nil + end + end + context 'mattermost team', :sidekiq_might_not_need_inline do let!(:chat_team) { create(:chat_team, namespace: group) } diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb index bee91c358ce..95f2176dbc0 100644 --- a/spec/services/projects/operations/update_service_spec.rb +++ b/spec/services/projects/operations/update_service_spec.rb @@ -306,6 +306,11 @@ RSpec.describe Projects::Operations::UpdateService do let(:params) do { error_tracking_setting_attributes: { + api_host: 'https://sentrytest.gitlab.com/', + project: { + slug: 'sentry-project', + organization_slug: 'sentry-org' + }, enabled: false, token: '*' * 8 } @@ -313,7 +318,7 @@ RSpec.describe Projects::Operations::UpdateService do end before do - create(:project_error_tracking_setting, project: project, token: 'token') + create(:project_error_tracking_setting, project: project, token: 'token', api_url: 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project/') end it 'does not update token' do diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb index 03fa2482bbf..225e7933d79 100644 --- a/spec/services/todos/destroy/entity_leave_service_spec.rb +++ b/spec/services/todos/destroy/entity_leave_service_spec.rb @@ -3,21 +3,24 @@ require 'spec_helper' RSpec.describe Todos::Destroy::EntityLeaveService do - let_it_be(:user, reload: true) { create(:user) } - let_it_be(:user2, reload: true) { create(:user) } - - let(:group) { create(:group, :private) } - let(:project) { create(:project, :private, group: group) } - let(:issue) { create(:issue, project: project) } - let(:issue_c) { create(:issue, project: project, confidential: true) } - let!(:todo_group_user) { create(:todo, user: user, group: group) } - let!(:todo_group_user2) { create(:todo, user: user2, group: group) } - - 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_c_user) { create(:todo, user: user, target: issue_c, project: project) } + let_it_be(:user, reload: true) { create(:user) } + let_it_be(:user2, reload: true) { create(:user) } + let_it_be_with_refind(:group) { create(:group, :private) } + let_it_be(:project) { create(:project, :private, group: group) } + + let(:issue) { create(:issue, project: project) } + let(:issue_c) { create(:issue, project: project, confidential: true) } + let!(:todo_group_user) { create(:todo, user: user, group: group) } + let!(:todo_group_user2) { create(:todo, user: user2, group: group) } + 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_c_user) { create(:todo, user: user, target: issue_c, project: project) } let!(:todo_issue_c_user2) { create(:todo, user: user2, target: issue_c, project: project) } + let(:internal_note) { create(:note, noteable: issue, project: project, confidential: true ) } + let!(:todo_for_internal_note) do + create(:todo, user: user, target: issue, project: project, note: internal_note) + end shared_examples 'using different access permissions' do before do @@ -34,20 +37,28 @@ RSpec.describe Todos::Destroy::EntityLeaveService do it { does_not_remove_any_todos } end - shared_examples 'removes only confidential issues todos' do - it { removes_only_confidential_issues_todos } + shared_examples 'removes confidential issues and internal notes todos' do + it { removes_confidential_issues_and_internal_notes_todos } + end + + shared_examples 'removes only internal notes todos' do + it { removes_only_internal_notes_todos } end def does_not_remove_any_todos expect { subject }.not_to change { Todo.count } end - def removes_only_confidential_issues_todos - expect { subject }.to change { Todo.count }.from(6).to(5) + def removes_only_internal_notes_todos + expect { subject }.to change { Todo.count }.from(7).to(6) + end + + def removes_confidential_issues_and_internal_notes_todos + expect { subject }.to change { Todo.count }.from(7).to(5) end - def removes_confidential_issues_and_merge_request_todos - expect { subject }.to change { Todo.count }.from(6).to(4) + def removes_confidential_issues_and_internal_notes_and_merge_request_todos + expect { subject }.to change { Todo.count }.from(7).to(4) expect(user.todos).to match_array([todo_issue_user, todo_group_user]) end @@ -70,7 +81,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do context 'when project is private' do context 'when user is not a member of the project' do it 'removes project todos for the provided user' do - expect { subject }.to change { Todo.count }.from(6).to(3) + expect { subject }.to change { Todo.count }.from(7).to(3) expect(user.todos).to match_array([todo_group_user]) expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2]) @@ -81,11 +92,11 @@ RSpec.describe Todos::Destroy::EntityLeaveService do where(:group_access, :project_access, :method_name) do [ [nil, :reporter, :does_not_remove_any_todos], - [nil, :guest, :removes_confidential_issues_and_merge_request_todos], + [nil, :guest, :removes_confidential_issues_and_internal_notes_and_merge_request_todos], [:reporter, nil, :does_not_remove_any_todos], - [:guest, nil, :removes_confidential_issues_and_merge_request_todos], + [:guest, nil, :removes_confidential_issues_and_internal_notes_and_merge_request_todos], [:guest, :reporter, :does_not_remove_any_todos], - [:guest, :guest, :removes_confidential_issues_and_merge_request_todos] + [:guest, :guest, :removes_confidential_issues_and_internal_notes_and_merge_request_todos] ] end @@ -97,11 +108,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do # a private project in an internal/public group is valid context 'when project is private in an internal/public group' do - let(:group) { create(:group, :internal) } + let_it_be(:group) { create(:group, :internal) } + let_it_be(:project) { create(:project, :private, group: group) } context 'when user is not a member of the project' do it 'removes project todos for the provided user' do - expect { subject }.to change { Todo.count }.from(6).to(3) + expect { subject }.to change { Todo.count }.from(7).to(3) expect(user.todos).to match_array([todo_group_user]) expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2]) @@ -112,11 +124,11 @@ RSpec.describe Todos::Destroy::EntityLeaveService do where(:group_access, :project_access, :method_name) do [ [nil, :reporter, :does_not_remove_any_todos], - [nil, :guest, :removes_confidential_issues_and_merge_request_todos], + [nil, :guest, :removes_confidential_issues_and_internal_notes_and_merge_request_todos], [:reporter, nil, :does_not_remove_any_todos], - [:guest, nil, :removes_confidential_issues_and_merge_request_todos], + [:guest, nil, :removes_confidential_issues_and_internal_notes_and_merge_request_todos], [:guest, :reporter, :does_not_remove_any_todos], - [:guest, :guest, :removes_confidential_issues_and_merge_request_todos] + [:guest, :guest, :removes_confidential_issues_and_internal_notes_and_merge_request_todos] ] end @@ -142,7 +154,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do context 'confidential issues' do context 'when a user is not an author of confidential issue' do - it_behaves_like 'removes only confidential issues todos' + it_behaves_like 'removes confidential issues and internal notes todos' end context 'when a user is an author of confidential issue' do @@ -150,7 +162,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do issue_c.update!(author: user) end - it_behaves_like 'does not remove any todos' + it_behaves_like 'removes only internal notes todos' end context 'when a user is an assignee of confidential issue' do @@ -158,18 +170,18 @@ RSpec.describe Todos::Destroy::EntityLeaveService do issue_c.assignees << user end - it_behaves_like 'does not remove any todos' + it_behaves_like 'removes only internal notes todos' end context 'access permissions' do where(:group_access, :project_access, :method_name) do [ [nil, :reporter, :does_not_remove_any_todos], - [nil, :guest, :removes_only_confidential_issues_todos], + [nil, :guest, :removes_confidential_issues_and_internal_notes_todos], [:reporter, nil, :does_not_remove_any_todos], - [:guest, nil, :removes_only_confidential_issues_todos], + [:guest, nil, :removes_confidential_issues_and_internal_notes_todos], [:guest, :reporter, :does_not_remove_any_todos], - [:guest, :guest, :removes_only_confidential_issues_todos] + [:guest, :guest, :removes_confidential_issues_and_internal_notes_todos] ] end @@ -186,7 +198,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end it 'removes only users issue todos' do - expect { subject }.to change { Todo.count }.from(6).to(5) + expect { subject }.to change { Todo.count }.from(7).to(5) end end end @@ -199,7 +211,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do context 'when group is private' do context 'when a user leaves a group' do it 'removes group and subproject todos for the user' do - expect { subject }.to change { Todo.count }.from(6).to(2) + expect { subject }.to change { Todo.count }.from(7).to(2) expect(user.todos).to be_empty expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2]) @@ -210,11 +222,11 @@ RSpec.describe Todos::Destroy::EntityLeaveService do where(:group_access, :project_access, :method_name) do [ [nil, :reporter, :does_not_remove_any_todos], - [nil, :guest, :removes_confidential_issues_and_merge_request_todos], + [nil, :guest, :removes_confidential_issues_and_internal_notes_and_merge_request_todos], [:reporter, nil, :does_not_remove_any_todos], - [:guest, nil, :removes_confidential_issues_and_merge_request_todos], + [:guest, nil, :removes_confidential_issues_and_internal_notes_and_merge_request_todos], [:guest, :reporter, :does_not_remove_any_todos], - [:guest, :guest, :removes_confidential_issues_and_merge_request_todos] + [:guest, :guest, :removes_confidential_issues_and_internal_notes_and_merge_request_todos] ] end @@ -224,12 +236,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end context 'with nested groups' do - let(:parent_group) { create(:group, :public) } - let(:parent_subgroup) { create(:group)} - let(:subgroup) { create(:group, :private, parent: group) } - let(:subgroup2) { create(:group, :private, parent: group) } - let(:subproject) { create(:project, group: subgroup) } - let(:subproject2) { create(:project, group: subgroup2) } + let_it_be_with_refind(:parent_group) { create(:group, :public) } + let_it_be_with_refind(:parent_subgroup) { create(:group) } + let_it_be(:subgroup) { create(:group, :private, parent: group) } + let_it_be(:subgroup2) { create(:group, :private, parent: group) } + let_it_be(:subproject) { create(:project, group: subgroup) } + let_it_be(:subproject2) { create(:project, group: subgroup2) } let!(:todo_subproject_user) { create(:todo, user: user, project: subproject) } let!(:todo_subproject2_user) { create(:todo, user: user, project: subproject2) } @@ -238,6 +250,10 @@ RSpec.describe Todos::Destroy::EntityLeaveService do let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) } let!(:todo_subpgroup_user2) { create(:todo, user: user2, group: subgroup) } let!(:todo_parent_group_user) { create(:todo, user: user, group: parent_group) } + let(:subproject_internal_note) { create(:note, noteable: issue, project: project, confidential: true ) } + let!(:todo_for_internal_subproject_note) do + create(:todo, user: user, target: issue, project: project, note: subproject_internal_note) + end before do group.update!(parent: parent_group) @@ -245,7 +261,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do context 'when the user is not a member of any groups/projects' do it 'removes todos for the user including subprojects todos' do - expect { subject }.to change { Todo.count }.from(13).to(5) + expect { subject }.to change { Todo.count }.from(15).to(5) expect(user.todos).to eq([todo_parent_group_user]) expect(user2.todos) @@ -269,7 +285,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end it 'does not remove group and subproject todos' do - expect { subject }.to change { Todo.count }.from(13).to(8) + expect { subject }.to change { Todo.count }.from(15).to(8) expect(user.todos) .to match_array( @@ -288,7 +304,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end it 'does not remove subproject and group todos' do - expect { subject }.to change { Todo.count }.from(13).to(8) + expect { subject }.to change { Todo.count }.from(15).to(8) expect(user.todos) .to match_array( @@ -319,13 +335,13 @@ RSpec.describe Todos::Destroy::EntityLeaveService do context 'access permissions' do where(:group_access, :project_access, :method_name) do [ - [nil, nil, :removes_only_confidential_issues_todos], + [nil, nil, :removes_confidential_issues_and_internal_notes_todos], [nil, :reporter, :does_not_remove_any_todos], - [nil, :guest, :removes_only_confidential_issues_todos], + [nil, :guest, :removes_confidential_issues_and_internal_notes_todos], [:reporter, nil, :does_not_remove_any_todos], - [:guest, nil, :removes_only_confidential_issues_todos], + [:guest, nil, :removes_confidential_issues_and_internal_notes_todos], [:guest, :reporter, :does_not_remove_any_todos], - [:guest, :guest, :removes_only_confidential_issues_todos] + [:guest, :guest, :removes_confidential_issues_and_internal_notes_todos] ] end |