From 8c7f4e9d5f36cff46365a7f8c4b9c21578c1e781 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Thu, 18 Jun 2020 11:18:50 +0000 Subject: Add latest changes from gitlab-org/gitlab@13-1-stable-ee --- .../admin/propagate_integration_service_spec.rb | 149 ++++++ .../alert_management/alerts/update_service_spec.rb | 134 +++++ .../create_alert_issue_service_spec.rb | 17 +- .../process_prometheus_alert_service_spec.rb | 40 +- spec/services/audit_event_service_spec.rb | 6 +- .../project_create_service_spec.rb | 50 +- spec/services/auto_merge/base_service_spec.rb | 99 +++- .../ci/build_report_result_service_spec.rb | 51 ++ .../create_cross_project_pipeline_service_spec.rb | 30 +- spec/services/ci/create_pipeline_service_spec.rb | 12 + .../ci/create_web_ide_terminal_service_spec.rb | 143 ++++++ .../ci/expire_pipeline_cache_service_spec.rb | 4 +- .../ci/generate_terraform_reports_service_spec.rb | 26 +- .../ci/pipeline_bridge_status_service_spec.rb | 2 +- spec/services/ci/retry_build_service_spec.rb | 7 +- .../ci/update_ci_ref_status_service_spec.rb | 169 ------- spec/services/ci/web_ide_config_service_spec.rb | 91 ++++ .../check_uninstall_progress_service_spec.rb | 4 +- .../applications/prometheus_config_service_spec.rb | 16 +- ...e_cluster_applications_artifact_service_spec.rb | 14 +- .../concerns/exclusive_lease_guard_spec.rb | 121 +++++ .../update_service_spec.rb | 101 ++++ .../container_expiration_policy_service_spec.rb | 15 + .../delete_designs_service_spec.rb | 26 +- .../design_management/save_designs_service_spec.rb | 25 +- spec/services/discussions/resolve_service_spec.rb | 95 +++- spec/services/draft_notes/create_service_spec.rb | 94 ++++ spec/services/draft_notes/destroy_service_spec.rb | 52 ++ spec/services/draft_notes/publish_service_spec.rb | 261 ++++++++++ spec/services/event_create_service_spec.rb | 102 +++- spec/services/git/branch_hooks_service_spec.rb | 8 +- spec/services/git/wiki_push_service/change_spec.rb | 6 +- spec/services/git/wiki_push_service_spec.rb | 16 +- .../groups/group_links/destroy_service_spec.rb | 10 +- .../groups/import_export/export_service_spec.rb | 21 +- .../groups/import_export/import_service_spec.rb | 63 +++ spec/services/groups/transfer_service_spec.rb | 9 + spec/services/import/github_service_spec.rb | 55 ++ .../integrations/test/project_service_spec.rb | 195 ++++++++ spec/services/issuable/bulk_update_service_spec.rb | 89 +--- spec/services/issues/close_service_spec.rb | 23 +- spec/services/issues/create_service_spec.rb | 2 +- spec/services/issues/import_csv_service_spec.rb | 20 +- spec/services/issues/update_service_spec.rb | 34 +- spec/services/jira/requests/projects_spec.rb | 95 ++++ .../jira_import/start_import_service_spec.rb | 26 +- spec/services/jira_import/users_importer_spec.rb | 77 +++ spec/services/jira_import/users_mapper_spec.rb | 43 ++ .../labels/available_labels_service_spec.rb | 6 + spec/services/merge_requests/close_service_spec.rb | 65 +-- .../services/merge_requests/create_service_spec.rb | 6 +- .../delete_non_latest_diffs_service_spec.rb | 4 +- .../merge_requests/ff_merge_service_spec.rb | 105 ++-- spec/services/merge_requests/merge_service_spec.rb | 41 +- .../merge_requests/refresh_service_spec.rb | 164 +++--- .../services/merge_requests/reopen_service_spec.rb | 22 +- .../namespaces/check_storage_size_service_spec.rb | 8 +- .../notification_recipients/build_service_spec.rb | 52 ++ spec/services/notification_service_spec.rb | 56 ++- .../projects/alerting/notify_service_spec.rb | 65 ++- .../cleanup_tags_service_spec.rb | 43 +- spec/services/projects/create_service_spec.rb | 162 +++++- .../projects/group_links/create_service_spec.rb | 15 +- .../projects/group_links/destroy_service_spec.rb | 18 +- .../projects/group_links/update_service_spec.rb | 56 +++ .../projects/import_export/export_service_spec.rb | 6 +- spec/services/projects/lsif_data_service_spec.rb | 126 ----- .../projects/operations/update_service_spec.rb | 29 +- .../alerts/create_events_service_spec.rb | 2 +- .../prometheus/alerts/notify_service_spec.rb | 146 +++--- .../projects/propagate_service_template_spec.rb | 4 +- .../services/projects/update_pages_service_spec.rb | 17 + .../projects/update_remote_mirror_service_spec.rb | 17 + .../update_repository_storage_service_spec.rb | 6 +- spec/services/projects/update_service_spec.rb | 78 ++- .../create_default_alerts_service_spec.rb | 19 +- spec/services/prometheus/proxy_service_spec.rb | 39 ++ .../proxy_variable_substitution_service_spec.rb | 14 + .../quick_actions/interpret_service_spec.rb | 23 + .../releases/create_evidence_service_spec.rb | 28 ++ spec/services/releases/create_service_spec.rb | 177 +++++++ .../resource_events/change_state_service_spec.rb | 39 ++ spec/services/service_response_spec.rb | 10 + .../services/snippets/bulk_destroy_service_spec.rb | 12 + spec/services/snippets/create_service_spec.rb | 59 ++- spec/services/snippets/update_service_spec.rb | 78 +++ spec/services/spam/akismet_service_spec.rb | 8 +- spec/services/spam/spam_action_service_spec.rb | 12 +- spec/services/spam/spam_verdict_service_spec.rb | 250 +++++++++- spec/services/submit_usage_ping_service_spec.rb | 3 +- spec/services/suggestions/apply_service_spec.rb | 553 ++++++++++++--------- .../system_notes/issuables_service_spec.rb | 15 + spec/services/test_hooks/project_service_spec.rb | 16 - spec/services/test_hooks/system_service_spec.rb | 2 - spec/services/todo_service_spec.rb | 302 +++++------ .../user_project_access_changed_service_spec.rb | 6 +- spec/services/users/destroy_service_spec.rb | 12 + .../users/migrate_to_ghost_user_service_spec.rb | 9 + .../wiki_pages/event_create_service_spec.rb | 8 +- 99 files changed, 4440 insertions(+), 1321 deletions(-) create mode 100644 spec/services/admin/propagate_integration_service_spec.rb create mode 100644 spec/services/alert_management/alerts/update_service_spec.rb create mode 100644 spec/services/ci/build_report_result_service_spec.rb create mode 100644 spec/services/ci/create_web_ide_terminal_service_spec.rb delete mode 100644 spec/services/ci/update_ci_ref_status_service_spec.rb create mode 100644 spec/services/ci/web_ide_config_service_spec.rb create mode 100644 spec/services/concerns/exclusive_lease_guard_spec.rb create mode 100644 spec/services/container_expiration_policies/update_service_spec.rb create mode 100644 spec/services/draft_notes/create_service_spec.rb create mode 100644 spec/services/draft_notes/destroy_service_spec.rb create mode 100644 spec/services/draft_notes/publish_service_spec.rb create mode 100644 spec/services/import/github_service_spec.rb create mode 100644 spec/services/integrations/test/project_service_spec.rb create mode 100644 spec/services/jira/requests/projects_spec.rb create mode 100644 spec/services/jira_import/users_importer_spec.rb create mode 100644 spec/services/jira_import/users_mapper_spec.rb create mode 100644 spec/services/projects/group_links/update_service_spec.rb delete mode 100644 spec/services/projects/lsif_data_service_spec.rb create mode 100644 spec/services/releases/create_evidence_service_spec.rb create mode 100644 spec/services/resource_events/change_state_service_spec.rb (limited to 'spec/services') diff --git a/spec/services/admin/propagate_integration_service_spec.rb b/spec/services/admin/propagate_integration_service_spec.rb new file mode 100644 index 00000000000..843b78a41e9 --- /dev/null +++ b/spec/services/admin/propagate_integration_service_spec.rb @@ -0,0 +1,149 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Admin::PropagateIntegrationService do + describe '.propagate' do + let(:excluded_attributes) { %w[id project_id inherit_from_id instance created_at updated_at title description] } + let!(:project) { create(:project) } + let!(:instance_integration) do + JiraService.create!( + instance: true, + active: true, + push_events: true, + url: 'http://update-jira.instance.com', + username: 'user', + password: 'secret' + ) + end + + let!(:inherited_integration) do + JiraService.create!( + project: create(:project), + inherit_from_id: instance_integration.id, + instance: false, + active: true, + push_events: false, + url: 'http://jira.instance.com', + username: 'user', + password: 'secret' + ) + end + + let!(:not_inherited_integration) do + JiraService.create!( + project: create(:project), + inherit_from_id: nil, + instance: false, + active: true, + push_events: false, + url: 'http://jira.instance.com', + username: 'user', + password: 'secret' + ) + end + + let!(:another_inherited_integration) do + BambooService.create!( + project: create(:project), + inherit_from_id: instance_integration.id, + instance: false, + active: true, + push_events: false, + bamboo_url: 'http://gitlab.com', + username: 'mic', + password: 'password', + build_key: 'build' + ) + end + + shared_examples 'inherits settings from integration' do + it 'updates the inherited integrations' do + described_class.propagate(integration: instance_integration, overwrite: overwrite) + + expect(integration.reload.inherit_from_id).to eq(instance_integration.id) + expect(integration.attributes.except(*excluded_attributes)) + .to eq(instance_integration.attributes.except(*excluded_attributes)) + end + + context 'integration with data fields' do + let(:excluded_attributes) { %w[id service_id created_at updated_at] } + + it 'updates the data fields from inherited integrations' do + described_class.propagate(integration: instance_integration, overwrite: overwrite) + + expect(integration.reload.data_fields.attributes.except(*excluded_attributes)) + .to eq(instance_integration.data_fields.attributes.except(*excluded_attributes)) + end + end + end + + shared_examples 'does not inherit settings from integration' do + it 'does not update the not inherited integrations' do + described_class.propagate(integration: instance_integration, overwrite: overwrite) + + expect(integration.reload.attributes.except(*excluded_attributes)) + .not_to eq(instance_integration.attributes.except(*excluded_attributes)) + end + end + + context 'update only inherited integrations' do + let(:overwrite) { false } + + it_behaves_like 'inherits settings from integration' do + let(:integration) { inherited_integration } + end + + it_behaves_like 'does not inherit settings from integration' do + let(:integration) { not_inherited_integration } + end + + it_behaves_like 'does not inherit settings from integration' do + let(:integration) { another_inherited_integration } + end + + it_behaves_like 'inherits settings from integration' do + let(:integration) { project.jira_service } + end + end + + context 'update all integrations' do + let(:overwrite) { true } + + it_behaves_like 'inherits settings from integration' do + let(:integration) { inherited_integration } + end + + it_behaves_like 'inherits settings from integration' do + let(:integration) { not_inherited_integration } + end + + it_behaves_like 'does not inherit settings from integration' do + let(:integration) { another_inherited_integration } + end + + it_behaves_like 'inherits settings from integration' do + let(:integration) { project.jira_service } + end + end + + it 'updates project#has_external_issue_tracker for issue tracker services' do + described_class.propagate(integration: instance_integration, overwrite: true) + + expect(project.reload.has_external_issue_tracker).to eq(true) + end + + it 'updates project#has_external_wiki for external wiki services' do + instance_integration = ExternalWikiService.create!( + instance: true, + active: true, + push_events: false, + external_wiki_url: 'http://external-wiki-url.com' + ) + + described_class.propagate(integration: instance_integration, overwrite: true) + + expect(project.reload.has_external_wiki).to eq(true) + end + end +end diff --git a/spec/services/alert_management/alerts/update_service_spec.rb b/spec/services/alert_management/alerts/update_service_spec.rb new file mode 100644 index 00000000000..e185e67c5cf --- /dev/null +++ b/spec/services/alert_management/alerts/update_service_spec.rb @@ -0,0 +1,134 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AlertManagement::Alerts::UpdateService do + let_it_be(:user_with_permissions) { create(:user) } + let_it_be(:user_without_permissions) { create(:user) } + let_it_be(:alert, reload: true) { create(:alert_management_alert) } + let_it_be(:project) { alert.project } + + let(:current_user) { user_with_permissions } + let(:params) { {} } + + let(:service) { described_class.new(alert, current_user, params) } + + before_all do + project.add_developer(user_with_permissions) + end + + describe '#execute' do + subject(:response) { service.execute } + + context 'when the current_user is nil' do + let(:current_user) { nil } + + it 'results in an error' do + expect(response).to be_error + expect(response.message).to eq('You have no permissions') + end + end + + context 'when user does not have permission to update alerts' do + let(:current_user) { user_without_permissions } + + it 'results in an error' do + expect(response).to be_error + expect(response.message).to eq('You have no permissions') + end + end + + context 'when no parameters are included' do + it 'results in an error' do + expect(response).to be_error + expect(response.message).to eq('Please provide attributes to update') + end + end + + context 'when an error occures during update' do + let(:params) { { title: nil } } + + it 'results in an error' do + expect { response }.not_to change { alert.reload.notes.count } + expect(response).to be_error + expect(response.message).to eq("Title can't be blank") + end + end + + context 'when a model attribute is included without assignees' do + let(:params) { { title: 'This is an updated alert.' } } + + it 'updates the attribute' do + original_title = alert.title + + expect { response }.to change { alert.title }.from(original_title).to(params[:title]) + expect(response).to be_success + end + + it 'skips adding a todo' do + expect { response }.not_to change(Todo, :count) + end + end + + context 'when assignees are included' do + let(:params) { { assignees: [user_with_permissions] } } + + after do + alert.assignees = [] + end + + it 'assigns the user' do + expect { response }.to change { alert.reload.assignees }.from([]).to(params[:assignees]) + expect(response).to be_success + end + + it 'creates a system note for the assignment' do + expect { response }.to change { alert.reload.notes.count }.by(1) + end + + it 'adds a todo' do + expect { response }.to change { Todo.where(user: user_with_permissions).count }.by(1) + end + + context 'when current user is not the assignee' do + let(:assignee_user) { create(:user) } + let(:params) { { assignees: [assignee_user] } } + + it 'skips adding todo for assignee without permission to read alert' do + expect { response }.not_to change(Todo, :count) + end + + context 'when assignee has read permission' do + before do + project.add_developer(assignee_user) + end + + it 'adds a todo' do + response + + expect(Todo.first.author).to eq(current_user) + end + end + + context 'when current_user is nil' do + let(:current_user) { nil } + + it 'skips adding todo if current_user is nil' do + project.add_developer(assignee_user) + + expect { response }.not_to change(Todo, :count) + end + end + end + + context 'with multiple users included' do + let(:params) { { assignees: [user_with_permissions, user_without_permissions] } } + + it 'assigns the first permissioned user' do + expect { response }.to change { alert.reload.assignees }.from([]).to([user_with_permissions]) + expect(response).to be_success + end + end + end + end +end diff --git a/spec/services/alert_management/create_alert_issue_service_spec.rb b/spec/services/alert_management/create_alert_issue_service_spec.rb index 62afe777165..9bc8b731dc1 100644 --- a/spec/services/alert_management/create_alert_issue_service_spec.rb +++ b/spec/services/alert_management/create_alert_issue_service_spec.rb @@ -94,11 +94,7 @@ RSpec.describe AlertManagement::CreateAlertIssueService do end context 'when alert cannot be updated' do - before do - # invalidate alert - too_many_hosts = Array.new(AlertManagement::Alert::HOSTS_MAX_LENGTH + 1) { |_| 'host' } - alert.update_columns(hosts: too_many_hosts) - end + let(:alert) { create(:alert_management_alert, :with_validation_errors, :triggered, project: project, payload: payload) } it 'responds with error' do expect(execute).to be_error @@ -122,17 +118,6 @@ RSpec.describe AlertManagement::CreateAlertIssueService do expect(execute.message).to eq(_('An issue already exists')) end end - - context 'when alert_management_create_alert_issue feature flag is disabled' do - before do - stub_feature_flags(alert_management_create_alert_issue: false) - end - - it 'responds with error' do - expect(execute).to be_error - expect(execute.message).to eq(_('You have no permissions')) - end - end end context 'when a user is not allowed to create an issue' do diff --git a/spec/services/alert_management/process_prometheus_alert_service_spec.rb b/spec/services/alert_management/process_prometheus_alert_service_spec.rb index 73f9f103902..5b4da5e9077 100644 --- a/spec/services/alert_management/process_prometheus_alert_service_spec.rb +++ b/spec/services/alert_management/process_prometheus_alert_service_spec.rb @@ -5,8 +5,12 @@ require 'spec_helper' RSpec.describe AlertManagement::ProcessPrometheusAlertService do let_it_be(:project) { create(:project) } + before do + allow(ProjectServiceWorker).to receive(:perform_async) + end + describe '#execute' do - subject { described_class.new(project, nil, payload).execute } + subject(:execute) { described_class.new(project, nil, payload).execute } context 'when alert payload is valid' do let(:parsed_alert) { Gitlab::Alerting::Alert.new(project: project, payload: payload) } @@ -37,12 +41,22 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do context 'when alert with the same fingerprint already exists' do let!(:alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: parsed_alert.gitlab_fingerprint) } + it 'increases alert events count' do + expect { execute }.to change { alert.reload.events }.by(1) + end + context 'when status can be changed' do it 'changes status to triggered' do - expect { subject }.to change { alert.reload.triggered? }.to(true) + expect { execute }.to change { alert.reload.triggered? }.to(true) end end + it 'does not executes the alert service hooks' do + expect(alert).not_to receive(:execute_services) + + subject + end + context 'when status change did not succeed' do before do allow(AlertManagement::Alert).to receive(:for_fingerprint).and_return([alert]) @@ -56,7 +70,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do alert_id: alert.id ) - subject + execute end end @@ -66,7 +80,15 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do context 'when alert does not exist' do context 'when alert can be created' do it 'creates a new alert' do - expect { subject }.to change { AlertManagement::Alert.where(project: project).count }.by(1) + expect { execute }.to change { AlertManagement::Alert.where(project: project).count }.by(1) + end + + it 'executes the alert service hooks' do + slack_service = create(:service, type: 'SlackService', project: project, alert_events: true, active: true) + + subject + + expect(ProjectServiceWorker).to have_received(:perform_async).with(slack_service.id, an_instance_of(Hash)) end end @@ -85,7 +107,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do alert_errors: { hosts: ['hosts array is over 255 chars'] } ) - subject + execute end end @@ -99,7 +121,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do context 'when status can be changed' do it 'resolves an existing alert' do - expect { subject }.to change { alert.reload.resolved? }.to(true) + expect { execute }.to change { alert.reload.resolved? }.to(true) end end @@ -116,7 +138,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do alert_id: alert.id ) - subject + execute end end @@ -128,8 +150,8 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do let(:payload) { {} } it 'responds with bad_request' do - expect(subject).to be_error - expect(subject.http_status).to eq(:bad_request) + expect(execute).to be_error + expect(execute.http_status).to eq(:bad_request) end end end diff --git a/spec/services/audit_event_service_spec.rb b/spec/services/audit_event_service_spec.rb index 96df6689bb0..dc86735805c 100644 --- a/spec/services/audit_event_service_spec.rb +++ b/spec/services/audit_event_service_spec.rb @@ -4,12 +4,16 @@ require 'spec_helper' describe AuditEventService do let(:project) { create(:project) } - let(:user) { create(:user) } + let(:user) { create(:user, :with_sign_ins) } let(:project_member) { create(:project_member, user: user) } let(:service) { described_class.new(user, project, { action: :destroy }) } let(:logger) { instance_double(Gitlab::AuditJsonLogger) } describe '#security_event' do + before do + stub_licensed_features(admin_audit_log: false) + end + it 'creates an event and logs to a file' do expect(service).to receive(:file_logger).and_return(logger) expect(logger).to receive(:info).with(author_id: user.id, diff --git a/spec/services/authorized_project_update/project_create_service_spec.rb b/spec/services/authorized_project_update/project_create_service_spec.rb index 49ea538d909..5b3e36af766 100644 --- a/spec/services/authorized_project_update/project_create_service_spec.rb +++ b/spec/services/authorized_project_update/project_create_service_spec.rb @@ -56,21 +56,47 @@ describe AuthorizedProjectUpdate::ProjectCreateService do end context 'membership overrides' do - before do - create(:group_member, access_level: Gitlab::Access::REPORTER, group: group_parent, user: group_user) - create(:group_member, access_level: Gitlab::Access::DEVELOPER, group: group, user: group_user) - ProjectAuthorization.delete_all + context 'group hierarchy' do + before do + create(:group_member, access_level: Gitlab::Access::REPORTER, group: group_parent, user: group_user) + create(:group_member, access_level: Gitlab::Access::DEVELOPER, group: group, user: group_user) + ProjectAuthorization.delete_all + end + + it 'creates project authorization' do + expect { service.execute }.to( + change { ProjectAuthorization.count }.from(0).to(1)) + + project_authorization = ProjectAuthorization.where( + project_id: group_project.id, + user_id: group_user.id, + access_level: Gitlab::Access::DEVELOPER) + expect(project_authorization).to exist + end end - it 'creates project authorization' do - expect { service.execute }.to( - change { ProjectAuthorization.count }.from(0).to(1)) + context 'group sharing' do + let!(:shared_with_group) { create(:group) } - project_authorization = ProjectAuthorization.where( - project_id: group_project.id, - user_id: group_user.id, - access_level: Gitlab::Access::DEVELOPER) - expect(project_authorization).to exist + before do + create(:group_member, access_level: Gitlab::Access::REPORTER, group: group, user: group_user) + create(:group_member, access_level: Gitlab::Access::MAINTAINER, group: shared_with_group, user: group_user) + + create(:group_group_link, shared_group: group, shared_with_group: shared_with_group, group_access: Gitlab::Access::DEVELOPER) + + ProjectAuthorization.delete_all + end + + it 'creates project authorization' do + expect { service.execute }.to( + change { ProjectAuthorization.count }.from(0).to(1)) + + project_authorization = ProjectAuthorization.where( + project_id: group_project.id, + user_id: group_user.id, + access_level: Gitlab::Access::DEVELOPER) + expect(project_authorization).to exist + end end end diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb index 0a6bcb1badc..e08e1d670bf 100644 --- a/spec/services/auto_merge/base_service_spec.rb +++ b/spec/services/auto_merge/base_service_spec.rb @@ -82,9 +82,9 @@ describe AutoMerge::BaseService do end end - context 'when failed to save' do + context 'when failed to save merge request' do before do - allow(merge_request).to receive(:save) { false } + allow(merge_request).to receive(:save!) { raise ActiveRecord::RecordInvalid.new } end it 'does not yield block' do @@ -94,6 +94,39 @@ describe AutoMerge::BaseService do it 'returns failed' do is_expected.to eq(:failed) end + + it 'tracks the exception' do + expect(Gitlab::ErrorTracking) + .to receive(:track_exception).with(kind_of(ActiveRecord::RecordInvalid), + merge_request_id: merge_request.id) + + subject + end + end + + context 'when exception happens in yield block' do + def execute_with_error_in_yield + service.execute(merge_request) { raise 'Something went wrong' } + end + + it 'returns failed status' do + expect(execute_with_error_in_yield).to eq(:failed) + end + + it 'rollback the transaction' do + execute_with_error_in_yield + + merge_request.reload + expect(merge_request).not_to be_auto_merge_enabled + end + + it 'tracks the exception' do + expect(Gitlab::ErrorTracking) + .to receive(:track_exception).with(kind_of(RuntimeError), + merge_request_id: merge_request.id) + + execute_with_error_in_yield + end end end @@ -162,7 +195,7 @@ describe AutoMerge::BaseService do context 'when failed to save' do before do - allow(merge_request).to receive(:save) { false } + allow(merge_request).to receive(:save!) { raise ActiveRecord::RecordInvalid.new } end it 'does not yield block' do @@ -178,9 +211,9 @@ describe AutoMerge::BaseService do it_behaves_like 'Canceled or Dropped' - context 'when failed to save' do + context 'when failed to save merge request' do before do - allow(merge_request).to receive(:save) { false } + allow(merge_request).to receive(:save!) { raise ActiveRecord::RecordInvalid.new } end it 'returns error status' do @@ -188,6 +221,33 @@ describe AutoMerge::BaseService do expect(subject[:message]).to eq("Can't cancel the automatic merge") end end + + context 'when exception happens in yield block' do + def cancel_with_error_in_yield + service.cancel(merge_request) { raise 'Something went wrong' } + end + + it 'returns error' do + result = cancel_with_error_in_yield + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq("Can't cancel the automatic merge") + end + + it 'rollback the transaction' do + cancel_with_error_in_yield + + merge_request.reload + expect(merge_request).to be_auto_merge_enabled + end + + it 'tracks the exception' do + expect(Gitlab::ErrorTracking) + .to receive(:track_exception).with(kind_of(RuntimeError), + merge_request_id: merge_request.id) + + cancel_with_error_in_yield + end + end end describe '#abort' do @@ -200,7 +260,7 @@ describe AutoMerge::BaseService do context 'when failed to save' do before do - allow(merge_request).to receive(:save) { false } + allow(merge_request).to receive(:save!) { raise ActiveRecord::RecordInvalid.new } end it 'returns error status' do @@ -208,5 +268,32 @@ describe AutoMerge::BaseService do expect(subject[:message]).to eq("Can't abort the automatic merge") end end + + context 'when exception happens in yield block' do + def abort_with_error_in_yield + service.abort(merge_request, reason) { raise 'Something went wrong' } + end + + it 'returns error' do + result = abort_with_error_in_yield + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq("Can't abort the automatic merge") + end + + it 'rollback the transaction' do + abort_with_error_in_yield + + merge_request.reload + expect(merge_request).to be_auto_merge_enabled + end + + it 'tracks the exception' do + expect(Gitlab::ErrorTracking) + .to receive(:track_exception).with(kind_of(RuntimeError), + merge_request_id: merge_request.id) + + abort_with_error_in_yield + end + end end end diff --git a/spec/services/ci/build_report_result_service_spec.rb b/spec/services/ci/build_report_result_service_spec.rb new file mode 100644 index 00000000000..dbdfc774314 --- /dev/null +++ b/spec/services/ci/build_report_result_service_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Ci::BuildReportResultService do + describe "#execute" do + subject(:build_report_result) { described_class.new.execute(build) } + + context 'when build is finished' do + let(:build) { create(:ci_build, :success, :test_reports) } + + it 'creates a build report result entry', :aggregate_failures do + expect(build_report_result.tests_name).to eq("test") + expect(build_report_result.tests_success).to eq(2) + expect(build_report_result.tests_failed).to eq(2) + expect(build_report_result.tests_errored).to eq(0) + expect(build_report_result.tests_skipped).to eq(0) + expect(build_report_result.tests_duration).to eq(0.010284) + expect(Ci::BuildReportResult.count).to eq(1) + end + + context 'when feature is disable' do + it 'does not persist the data' do + stub_feature_flags(build_report_summary: false) + + subject + + expect(Ci::BuildReportResult.count).to eq(0) + end + end + + context 'when data has already been persisted' do + it 'raises an error and do not persist the same data twice' do + expect { 2.times { described_class.new.execute(build) } }.to raise_error(ActiveRecord::RecordNotUnique) + + expect(Ci::BuildReportResult.count).to eq(1) + end + end + end + + context 'when build is running and test report does not exist' do + let(:build) { create(:ci_build, :running) } + + it 'does not persist data' do + subject + + expect(Ci::BuildReportResult.count).to eq(0) + end + end + end +end diff --git a/spec/services/ci/create_cross_project_pipeline_service_spec.rb b/spec/services/ci/create_cross_project_pipeline_service_spec.rb index 5c59aaa4ce9..9e2497854bc 100644 --- a/spec/services/ci/create_cross_project_pipeline_service_spec.rb +++ b/spec/services/ci/create_cross_project_pipeline_service_spec.rb @@ -487,10 +487,11 @@ describe Ci::CreateCrossProjectPipelineService, '#execute' do end it 'does not create a pipeline and drops the bridge' do - service.execute(bridge) + expect { service.execute(bridge) }.not_to change(downstream_project.ci_pipelines, :count) expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq('downstream_pipeline_creation_failed') + expect(bridge.options[:downstream_errors]).to eq(['Reference not found']) end end @@ -509,10 +510,35 @@ describe Ci::CreateCrossProjectPipelineService, '#execute' do end it 'does not create a pipeline and drops the bridge' do - service.execute(bridge) + expect { service.execute(bridge) }.not_to change(downstream_project.ci_pipelines, :count) + + expect(bridge.reload).to be_failed + expect(bridge.failure_reason).to eq('downstream_pipeline_creation_failed') + expect(bridge.options[:downstream_errors]).to eq(['No stages / jobs for this pipeline.']) + end + end + + context 'when downstream pipeline has invalid YAML' do + before do + stub_ci_pipeline_yaml_file(config) + end + + let(:config) do + <<-EOY + test: + stage: testx + script: echo 1 + EOY + end + + it 'creates the pipeline but drops the bridge' do + expect { service.execute(bridge) }.to change(downstream_project.ci_pipelines, :count).by(1) expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq('downstream_pipeline_creation_failed') + expect(bridge.options[:downstream_errors]).to eq( + ['test job: chosen stage does not exist; available stages are .pre, build, test, deploy, .post'] + ) end end end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 681ce9669e2..b9456d5fcd4 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -77,6 +77,18 @@ describe Ci::CreatePipelineService do pipeline end + it 'records pipeline size in a prometheus histogram' do + histogram = spy('pipeline size histogram') + + allow(Gitlab::Ci::Pipeline::Chain::Metrics) + .to receive(:new).and_return(histogram) + + execute_service + + expect(histogram).to have_received(:observe) + .with({ source: 'push' }, 5) + end + context 'when merge requests already exist for this source branch' do let(:merge_request_1) do create(:merge_request, source_branch: 'feature', target_branch: "master", source_project: project) diff --git a/spec/services/ci/create_web_ide_terminal_service_spec.rb b/spec/services/ci/create_web_ide_terminal_service_spec.rb new file mode 100644 index 00000000000..2cc67c7cd1d --- /dev/null +++ b/spec/services/ci/create_web_ide_terminal_service_spec.rb @@ -0,0 +1,143 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Ci::CreateWebIdeTerminalService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let(:ref) { 'master' } + + describe '#execute' do + subject { described_class.new(project, user, ref: ref).execute } + + context 'for maintainer' do + shared_examples 'be successful' do + it 'returns a success with pipeline object' do + is_expected.to include(status: :success) + + expect(subject[:pipeline]).to be_a(Ci::Pipeline) + expect(subject[:pipeline]).to be_persisted + expect(subject[:pipeline].stages.count).to eq(1) + expect(subject[:pipeline].builds.count).to eq(1) + end + end + + before do + project.add_maintainer(user) + end + + context 'when web-ide has valid configuration' do + before do + stub_webide_config_file(config_content) + end + + context 'for empty configuration' do + let(:config_content) do + 'terminal: {}' + end + + it_behaves_like 'be successful' + end + + context 'for configuration with container image' do + let(:config_content) do + 'terminal: { image: ruby }' + end + + it_behaves_like 'be successful' + end + + context 'for configuration with ports' do + let(:config_content) do + <<-EOS + terminal: + image: + name: ruby:2.7 + ports: + - 80 + script: rspec + services: + - name: test + alias: test + ports: + - 8080 + EOS + end + + it_behaves_like 'be successful' + end + end + end + + context 'error handling' do + shared_examples 'having an error' do |message| + it 'returns an error' do + is_expected.to eq( + status: :error, + message: message + ) + end + end + + shared_examples 'having insufficient permissions' do + it_behaves_like 'having an error', 'Insufficient permissions to create a terminal' + end + + context 'when user is developer' do + before do + project.add_developer(user) + end + + it_behaves_like 'having insufficient permissions' + end + + context 'when user is maintainer' do + before do + project.add_maintainer(user) + end + + context 'when terminal is already running' do + let!(:webide_pipeline) { create(:ci_pipeline, :webide, :running, project: project, user: user) } + + it_behaves_like 'having an error', 'There is already a terminal running' + end + + context 'when ref is non-existing' do + let(:ref) { 'non-existing-ref' } + + it_behaves_like 'having an error', 'Ref does not exist' + end + + context 'when ref is a tag' do + let(:ref) { 'v1.0.0' } + + it_behaves_like 'having an error', 'Ref needs to be a branch' + end + + context 'when terminal config is missing' do + let(:ref) { 'v1.0.0' } + + it_behaves_like 'having an error', 'Ref needs to be a branch' + end + + context 'when webide config is present' do + before do + stub_webide_config_file(config_content) + end + + context 'config has invalid content' do + let(:config_content) { 'invalid' } + + it_behaves_like 'having an error', 'Invalid configuration format' + end + + context 'config is valid, but does not have terminal' do + let(:config_content) { '{}' } + + it_behaves_like 'having an error', 'Terminal is not configured' + end + end + end + end + end +end diff --git a/spec/services/ci/expire_pipeline_cache_service_spec.rb b/spec/services/ci/expire_pipeline_cache_service_spec.rb index 78e1ba0109a..2962e9dd31e 100644 --- a/spec/services/ci/expire_pipeline_cache_service_spec.rb +++ b/spec/services/ci/expire_pipeline_cache_service_spec.rb @@ -10,9 +10,9 @@ describe Ci::ExpirePipelineCacheService do describe '#execute' do it 'invalidates Etag caching for project pipelines path' do - pipelines_path = "/#{project.full_path}/pipelines.json" + pipelines_path = "/#{project.full_path}/-/pipelines.json" new_mr_pipelines_path = "/#{project.full_path}/-/merge_requests/new.json" - pipeline_path = "/#{project.full_path}/pipelines/#{pipeline.id}.json" + pipeline_path = "/#{project.full_path}/-/pipelines/#{pipeline.id}.json" expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(pipelines_path) expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(new_mr_pipelines_path) diff --git a/spec/services/ci/generate_terraform_reports_service_spec.rb b/spec/services/ci/generate_terraform_reports_service_spec.rb index 4d2c60bed2c..008ecf17b3e 100644 --- a/spec/services/ci/generate_terraform_reports_service_spec.rb +++ b/spec/services/ci/generate_terraform_reports_service_spec.rb @@ -12,15 +12,23 @@ describe Ci::GenerateTerraformReportsService do context 'when head pipeline has terraform reports' do it 'returns status and data' do - result = subject.execute(nil, merge_request.head_pipeline) - - expect(result).to match( - status: :parsed, - data: match( - a_hash_including('tfplan.json' => a_hash_including('create' => 0, 'update' => 1, 'delete' => 0)) - ), - key: an_instance_of(Array) - ) + pipeline = merge_request.head_pipeline + result = subject.execute(nil, pipeline) + + pipeline.builds.each do |build| + expect(result).to match( + status: :parsed, + data: match( + a_hash_including(build.id.to_s => hash_including( + 'create' => 0, + 'delete' => 0, + 'update' => 1, + 'job_name' => build.options.dig(:artifacts, :name).to_s + )) + ), + key: an_instance_of(Array) + ) + end end end diff --git a/spec/services/ci/pipeline_bridge_status_service_spec.rb b/spec/services/ci/pipeline_bridge_status_service_spec.rb index 0b6ae976d97..7e79d222349 100644 --- a/spec/services/ci/pipeline_bridge_status_service_spec.rb +++ b/spec/services/ci/pipeline_bridge_status_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' describe Ci::PipelineBridgeStatusService do let(:user) { build(:user) } - let(:project) { build(:project) } + let_it_be(:project) { create(:project) } let(:pipeline) { build(:ci_pipeline, project: project) } describe '#execute' do diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 0aa603b24ae..90c53d4a346 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -30,7 +30,7 @@ describe Ci::RetryBuildService do created_at updated_at started_at finished_at queued_at erased_by erased_at auto_canceled_by job_artifacts job_artifacts_archive job_artifacts_metadata job_artifacts_trace job_artifacts_junit - job_artifacts_sast job_artifacts_dependency_scanning + job_artifacts_sast job_artifacts_secret_detection job_artifacts_dependency_scanning job_artifacts_container_scanning job_artifacts_dast job_artifacts_license_management job_artifacts_license_scanning job_artifacts_performance job_artifacts_lsif @@ -38,7 +38,8 @@ describe Ci::RetryBuildService do job_artifacts_codequality job_artifacts_metrics scheduled_at job_variables waiting_for_resource_at job_artifacts_metrics_referee job_artifacts_network_referee job_artifacts_dotenv - job_artifacts_cobertura needs job_artifacts_accessibility].freeze + job_artifacts_cobertura needs job_artifacts_accessibility + job_artifacts_requirements].freeze ignore_accessors = %i[type lock_version target_url base_tags trace_sections @@ -49,7 +50,7 @@ describe Ci::RetryBuildService do metadata runner_session trace_chunks upstream_pipeline_id artifacts_file artifacts_metadata artifacts_size commands resource resource_group_id processed security_scans author - pipeline_id].freeze + pipeline_id report_results].freeze shared_examples 'build duplication' do let(:another_pipeline) { create(:ci_empty_pipeline, project: project) } diff --git a/spec/services/ci/update_ci_ref_status_service_spec.rb b/spec/services/ci/update_ci_ref_status_service_spec.rb deleted file mode 100644 index 8b60586318d..00000000000 --- a/spec/services/ci/update_ci_ref_status_service_spec.rb +++ /dev/null @@ -1,169 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Ci::UpdateCiRefStatusService do - describe '#call' do - subject { described_class.new(pipeline) } - - shared_examples 'creates ci_ref' do - it 'creates a ci_ref with the pipeline attributes' do - expect do - expect(subject.call).to eq(true) - end.to change { Ci::Ref.count }.by(1) - - created_ref = pipeline.reload.ref_status - %w[ref tag project status].each do |attr| - expect(created_ref[attr]).to eq(pipeline[attr]) - end - end - - it 'calls PipelineNotificationWorker pasing the ref_status' do - expect(PipelineNotificationWorker).to receive(:perform_async).with(pipeline.id, ref_status: pipeline.status) - - subject.call - end - end - - shared_examples 'updates ci_ref' do - where(:ref_status, :pipeline_status, :next_status) do - [ - %w[failed success fixed], - %w[failed failed failed], - %w[success success success], - %w[success failed failed] - ] - end - - with_them do - let(:ci_ref) { create(:ci_ref, status: ref_status) } - let(:pipeline) { create(:ci_pipeline, status: pipeline_status, project: ci_ref.project, ref: ci_ref.ref) } - - it 'sets ci_ref.status to next_status' do - expect do - expect(subject.call).to eq(true) - expect(ci_ref.reload.status).to eq(next_status) - end.not_to change { Ci::Ref.count } - end - - it 'calls PipelineNotificationWorker pasing the ref_status' do - expect(PipelineNotificationWorker).to receive(:perform_async).with(pipeline.id, ref_status: next_status) - - subject.call - end - end - end - - shared_examples 'does a noop' do - it "doesn't change ci_ref" do - expect do - expect do - expect(subject.call).to eq(false) - end.not_to change { ci_ref.reload.status } - end.not_to change { Ci::Ref.count } - end - - it "doesn't call PipelineNotificationWorker" do - expect(PipelineNotificationWorker).not_to receive(:perform_async) - - subject.call - end - end - - context "ci_ref doesn't exists" do - let(:pipeline) { create(:ci_pipeline, :success, ref: 'new-ref') } - - it_behaves_like 'creates ci_ref' - - context 'when an ActiveRecord::RecordNotUnique validation is raised' do - let(:ci_ref) { create(:ci_ref, status: 'failed') } - let(:pipeline) { create(:ci_pipeline, status: :success, project: ci_ref.project, ref: ci_ref.ref) } - - it 'reloads the ci_ref and retries once' do - subject.instance_variable_set("@ref", subject.send(:build_ref)) - - expect do - expect(subject.call).to eq(true) - end.not_to change { Ci::Ref.count } - expect(ci_ref.reload.status).to eq('fixed') - end - - it 'raises error on multiple retries' do - allow_any_instance_of(Ci::Ref).to receive(:update) - .and_raise(ActiveRecord::RecordNotUnique) - - expect { subject.call }.to raise_error(ActiveRecord::RecordNotUnique) - end - end - end - - context 'ci_ref exists' do - let!(:ci_ref) { create(:ci_ref, status: 'failed') } - let(:pipeline) { ci_ref.pipelines.first } - - it_behaves_like 'updates ci_ref' - - context 'pipeline status is invalid' do - let!(:pipeline) { create(:ci_pipeline, :running, project: ci_ref.project, ref: ci_ref.ref, tag: ci_ref.tag) } - - it_behaves_like 'does a noop' - end - - context 'newer pipeline finished' do - let(:newer_pipeline) { create(:ci_pipeline, :success, project: ci_ref.project, ref: ci_ref.ref, tag: ci_ref.tag) } - - before do - ci_ref.update!(last_updated_by_pipeline: newer_pipeline) - end - - it_behaves_like 'does a noop' - end - - context 'pipeline is retried' do - before do - ci_ref.update!(last_updated_by_pipeline: pipeline) - end - - it_behaves_like 'updates ci_ref' - end - - context 'ref is stale' do - let(:pipeline1) { create(:ci_pipeline, :success, project: ci_ref.project, ref: ci_ref.ref, tag: ci_ref.tag) } - let(:pipeline2) { create(:ci_pipeline, :success, project: ci_ref.project, ref: ci_ref.ref, tag: ci_ref.tag) } - - it 'reloads the ref and retry' do - service1 = described_class.new(pipeline1) - service2 = described_class.new(pipeline2) - - service2.send(:ref) - service1.call - expect(ci_ref.reload.status).to eq('fixed') - expect do - expect(service2.call).to eq(true) - # We expect 'success' in this case rather than 'fixed' because - # the ref is correctly reloaded on stale error. - expect(ci_ref.reload.status).to eq('success') - end.not_to change { Ci::Ref.count } - end - - it 'aborts when a newer pipeline finished' do - service1 = described_class.new(pipeline1) - service2 = described_class.new(pipeline2) - - service2.call - expect do - expect(service1.call).to eq(false) - expect(ci_ref.reload.status).to eq('fixed') - end.not_to change { Ci::Ref.count } - end - end - - context 'ref exists as both tag/branch and tag' do - let(:pipeline) { create(:ci_pipeline, :failed, project: ci_ref.project, ref: ci_ref.ref, tag: true) } - let!(:branch_pipeline) { create(:ci_pipeline, :success, project: ci_ref.project, ref: ci_ref.ref, tag: false) } - - it_behaves_like 'creates ci_ref' - end - end - end -end diff --git a/spec/services/ci/web_ide_config_service_spec.rb b/spec/services/ci/web_ide_config_service_spec.rb new file mode 100644 index 00000000000..7522103ccb7 --- /dev/null +++ b/spec/services/ci/web_ide_config_service_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Ci::WebIdeConfigService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let(:sha) { 'sha' } + + describe '#execute' do + subject { described_class.new(project, user, sha: sha).execute } + + context 'when insufficient permission' do + it 'returns an error' do + is_expected.to include( + status: :error, + message: 'Insufficient permissions to read configuration') + end + end + + context 'for developer' do + before do + project.add_developer(user) + end + + context 'when file is missing' do + it 'returns an error' do + is_expected.to include( + status: :error, + message: "Failed to load Web IDE config file '.gitlab/.gitlab-webide.yml' for sha") + end + end + + context 'when file is present' do + before do + allow(project.repository).to receive(:blob_data_at).with('sha', anything) do + config_content + end + end + + context 'content is not valid' do + let(:config_content) { 'invalid content' } + + it 'returns an error' do + is_expected.to include( + status: :error, + message: "Invalid configuration format") + end + end + + context 'content is valid, but terminal not defined' do + let(:config_content) { '{}' } + + it 'returns success' do + is_expected.to include( + status: :success, + terminal: nil) + end + end + + context 'content is valid, with enabled terminal' do + let(:config_content) { 'terminal: {}' } + + it 'returns success' do + is_expected.to include( + status: :success, + terminal: { + tag_list: [], + yaml_variables: [], + options: { script: ["sleep 60"] } + }) + end + end + + context 'content is valid, with custom terminal' do + let(:config_content) { 'terminal: { before_script: [ls] }' } + + it 'returns success' do + is_expected.to include( + status: :success, + terminal: { + tag_list: [], + yaml_variables: [], + options: { before_script: ["ls"], script: ["sleep 60"] } + }) + end + end + end + end + end +end diff --git a/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb b/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb index ffb658330d3..9dede1947f8 100644 --- a/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb +++ b/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Clusters::Applications::CheckUninstallProgressService do - RESCHEDULE_PHASES = Gitlab::Kubernetes::Pod::PHASES - [Gitlab::Kubernetes::Pod::SUCCEEDED, Gitlab::Kubernetes::Pod::FAILED].freeze + reschedule_phases = Gitlab::Kubernetes::Pod::PHASES - [Gitlab::Kubernetes::Pod::SUCCEEDED, Gitlab::Kubernetes::Pod::FAILED].freeze let(:application) { create(:clusters_applications_prometheus, :uninstalling) } let(:service) { described_class.new(application) } @@ -42,7 +42,7 @@ describe Clusters::Applications::CheckUninstallProgressService do end context 'when application is uninstalling' do - RESCHEDULE_PHASES.each { |phase| it_behaves_like 'a not yet terminated installation', phase } + reschedule_phases.each { |phase| it_behaves_like 'a not yet terminated installation', phase } context 'when installation POD succeeded' do let(:phase) { Gitlab::Kubernetes::Pod::SUCCEEDED } diff --git a/spec/services/clusters/applications/prometheus_config_service_spec.rb b/spec/services/clusters/applications/prometheus_config_service_spec.rb index 993a697b543..b9032e665ec 100644 --- a/spec/services/clusters/applications/prometheus_config_service_spec.rb +++ b/spec/services/clusters/applications/prometheus_config_service_spec.rb @@ -90,23 +90,25 @@ describe Clusters::Applications::PrometheusConfigService do create(:prometheus_alert, project: project, environment: production, - prometheus_metric: metric) + prometheus_metric: metric, + operator: PrometheusAlert.operators['gt'], + threshold: 0) end let(:metric) do create(:prometheus_metric, query: query, project: project) end - let(:query) { '%{ci_environment_slug}' } + let(:query) { 'up{environment="{{ci_environment_slug}}"}' } it 'substitutes query variables' do expect(Gitlab::Prometheus::QueryVariables) .to receive(:call) - .with(production) + .with(production, start_time: nil, end_time: nil) .and_call_original expr = groups.dig(0, 'rules', 0, 'expr') - expect(expr).to include(production.name) + expect(expr).to eq("up{environment=\"#{production.slug}\"} > 0.0") end end @@ -127,13 +129,15 @@ describe Clusters::Applications::PrometheusConfigService do end it 'substitutes query variables once per environment' do + allow(Gitlab::Prometheus::QueryVariables).to receive(:call).and_call_original + expect(Gitlab::Prometheus::QueryVariables) .to receive(:call) - .with(production) + .with(production, start_time: nil, end_time: nil) expect(Gitlab::Prometheus::QueryVariables) .to receive(:call) - .with(staging) + .with(staging, start_time: nil, end_time: nil) subject end diff --git a/spec/services/clusters/parse_cluster_applications_artifact_service_spec.rb b/spec/services/clusters/parse_cluster_applications_artifact_service_spec.rb index f14c929554a..bb0b107eba6 100644 --- a/spec/services/clusters/parse_cluster_applications_artifact_service_spec.rb +++ b/spec/services/clusters/parse_cluster_applications_artifact_service_spec.rb @@ -85,9 +85,21 @@ describe Clusters::ParseClusterApplicationsArtifactService do end end - context 'job has no deployment cluster' do + context 'job has no deployment' do let(:job) { build(:ci_build) } + it 'returns an error' do + result = described_class.new(job, user).execute(artifact) + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('No deployment found for this job') + end + end + + context 'job has no deployment cluster' do + let(:deployment) { create(:deployment) } + let(:job) { deployment.deployable } + it 'returns an error' do result = described_class.new(job, user).execute(artifact) diff --git a/spec/services/concerns/exclusive_lease_guard_spec.rb b/spec/services/concerns/exclusive_lease_guard_spec.rb new file mode 100644 index 00000000000..a38facc7520 --- /dev/null +++ b/spec/services/concerns/exclusive_lease_guard_spec.rb @@ -0,0 +1,121 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ExclusiveLeaseGuard, :clean_gitlab_redis_shared_state do + subject :subject_class do + Class.new do + include ExclusiveLeaseGuard + + def self.name + 'ExclusiveLeaseGuardTestClass' + end + + def call(&block) + try_obtain_lease do + internal_method(&block) + end + end + + def internal_method + yield + end + + def lease_timeout + 1.second + end + end + end + + describe '#try_obtain_lease' do + let(:subject) { subject_class.new } + + it 'obtains the lease, calls internal_method and releases the lease', :aggregate_failures do + expect(subject).to receive(:internal_method).and_call_original + + subject.call do + expect(subject.exclusive_lease.exists?).to be_truthy + end + + expect(subject.exclusive_lease.exists?).to be_falsey + end + + context 'when the lease is already obtained' do + before do + subject.exclusive_lease.try_obtain + end + + after do + subject.exclusive_lease.cancel + end + + it 'does not call internal_method but logs error', :aggregate_failures do + expect(subject).not_to receive(:internal_method) + expect(Gitlab::AppLogger).to receive(:error).with('Cannot obtain an exclusive lease. There must be another instance already in execution.') + + subject.call + end + end + + context 'with overwritten lease_release?' do + subject :overwritten_subject_class do + Class.new(subject_class) do + def lease_release? + false + end + end + end + + let(:subject) { overwritten_subject_class.new } + + it 'does not release the lease after execution', :aggregate_failures do + subject.call do + expect(subject.exclusive_lease.exists?).to be_truthy + end + + expect(subject.exclusive_lease.exists?).to be_truthy + end + end + end + + describe '#exclusive_lease' do + it 'uses the class name as lease key' do + expect(Gitlab::ExclusiveLease).to receive(:new).with('exclusive_lease_guard_test_class', timeout: 1.second) + + subject_class.new.exclusive_lease + end + + context 'with overwritten lease_key' do + subject :overwritten_class do + Class.new(subject_class) do + def lease_key + 'other_lease_key' + end + end + end + + it 'uses the custom lease key' do + expect(Gitlab::ExclusiveLease).to receive(:new).with('other_lease_key', timeout: 1.second) + + overwritten_class.new.exclusive_lease + end + end + end + + describe '#release_lease' do + it 'sends a cancel message to ExclusiveLease' do + expect(Gitlab::ExclusiveLease).to receive(:cancel).with('exclusive_lease_guard_test_class', 'some_uuid') + + subject_class.new.release_lease('some_uuid') + end + end + + describe '#renew_lease!' do + let(:subject) { subject_class.new } + + it 'sends a renew message to the exclusive_lease instance' do + expect(subject.exclusive_lease).to receive(:renew) + subject.renew_lease! + end + end +end diff --git a/spec/services/container_expiration_policies/update_service_spec.rb b/spec/services/container_expiration_policies/update_service_spec.rb new file mode 100644 index 00000000000..ec178f3830f --- /dev/null +++ b/spec/services/container_expiration_policies/update_service_spec.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ContainerExpirationPolicies::UpdateService do + using RSpec::Parameterized::TableSyntax + + let_it_be(:project, reload: true) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be(:params) { { cadence: '3month', keep_n: 100, older_than: '14d', extra_key: 'will_not_be_processed' } } + + let(:container_expiration_policy) { project.container_expiration_policy } + + describe '#execute' do + subject { described_class.new(container: project, current_user: user, params: params).execute } + + RSpec.shared_examples 'returning a success' do + it 'returns a success' do + result = subject + + expect(result.payload[:container_expiration_policy]).to be_present + expect(result.success?).to be_truthy + end + end + + RSpec.shared_examples 'returning an error' do |message, http_status| + it 'returns an error' do + result = subject + + expect(result.message).to eq(message) + expect(result.status).to eq(:error) + expect(result.http_status).to eq(http_status) + end + end + + RSpec.shared_examples 'updating the container expiration policy' do + it_behaves_like 'updating the container expiration policy attributes', mode: :update, from: { cadence: '1d', keep_n: 10, older_than: '90d' }, to: { cadence: '3month', keep_n: 100, older_than: '14d' } + + it_behaves_like 'returning a success' + + context 'with invalid params' do + let_it_be(:params) { { cadence: '20d' } } + + it_behaves_like 'not creating the container expiration policy' + + it "doesn't update the cadence" do + expect { subject } + .not_to change { container_expiration_policy.reload.cadence } + end + + it_behaves_like 'returning an error', 'Cadence is not included in the list', 400 + end + end + + RSpec.shared_examples 'denying access to container expiration policy' do + context 'with existing container expiration policy' do + it_behaves_like 'not creating the container expiration policy' + + it_behaves_like 'returning an error', 'Access Denied', 403 + end + end + + context 'with existing container expiration policy' do + where(:user_role, :shared_examples_name) do + :maintainer | 'updating the container expiration policy' + :developer | 'updating the container expiration policy' + :reporter | 'denying access to container expiration policy' + :guest | 'denying access to container expiration policy' + :anonymous | 'denying access to container expiration policy' + end + + with_them do + before do + project.send("add_#{user_role}", user) unless user_role == :anonymous + end + + it_behaves_like params[:shared_examples_name] + end + end + + context 'without existing container expiration policy' do + let_it_be(:project, reload: true) { create(:project, :without_container_expiration_policy) } + + where(:user_role, :shared_examples_name) do + :maintainer | 'creating the container expiration policy' + :developer | 'creating the container expiration policy' + :reporter | 'denying access to container expiration policy' + :guest | 'denying access to container expiration policy' + :anonymous | 'denying access to container expiration policy' + end + + with_them do + before do + project.send("add_#{user_role}", user) unless user_role == :anonymous + end + + it_behaves_like params[:shared_examples_name] + end + end + end +end diff --git a/spec/services/container_expiration_policy_service_spec.rb b/spec/services/container_expiration_policy_service_spec.rb index b2f2b2e1236..97715b990ef 100644 --- a/spec/services/container_expiration_policy_service_spec.rb +++ b/spec/services/container_expiration_policy_service_spec.rb @@ -27,5 +27,20 @@ describe ContainerExpirationPolicyService do expect(container_expiration_policy.next_run_at).to be > Time.zone.now end + + context 'with an invalid container expiration policy' do + before do + allow(container_expiration_policy).to receive(:valid?).and_return(false) + end + + it 'disables it' do + expect(container_expiration_policy).not_to receive(:schedule_next_run!) + expect(CleanupContainerRepositoryWorker).not_to receive(:perform_async) + + expect { subject } + .to change { container_expiration_policy.reload.enabled }.from(true).to(false) + .and raise_error(ContainerExpirationPolicyService::InvalidPolicyError) + end + end end end diff --git a/spec/services/design_management/delete_designs_service_spec.rb b/spec/services/design_management/delete_designs_service_spec.rb index 2c0c1570cb4..bf5d6b443e6 100644 --- a/spec/services/design_management/delete_designs_service_spec.rb +++ b/spec/services/design_management/delete_designs_service_spec.rb @@ -56,6 +56,10 @@ describe DesignManagement::DeleteDesignsService do let(:enabled) { false } it_behaves_like "a service error" + + it 'does not create any events in the activity stream' do + expect { run_service rescue nil }.not_to change { Event.count } + end end context "when the feature is available" do @@ -72,7 +76,9 @@ describe DesignManagement::DeleteDesignsService do it 'does not log any events' do counter = ::Gitlab::UsageDataCounters::DesignsCounter - expect { run_service rescue nil }.not_to change { counter.totals } + + expect { run_service rescue nil } + .not_to change { [counter.totals, Event.count] } end end @@ -92,6 +98,12 @@ describe DesignManagement::DeleteDesignsService do expect { run_service }.to change { counter.read(:delete) }.by(1) end + it 'creates an event in the activity stream' do + expect { run_service } + .to change { Event.count }.by(1) + .and change { Event.destroyed_action.for_design.count }.by(1) + end + it 'informs the new-version-worker' do expect(::DesignManagement::NewVersionWorker).to receive(:perform_async).with(Integer) @@ -129,14 +141,14 @@ describe DesignManagement::DeleteDesignsService do let!(:designs) { create_designs(2) } - it 'removes those designs' do + it 'makes the correct changes' do + counter = ::Gitlab::UsageDataCounters::DesignsCounter + expect { run_service } .to change { issue.designs.current.count }.from(3).to(1) - end - - it 'logs the correct number of deletion events' do - counter = ::Gitlab::UsageDataCounters::DesignsCounter - expect { run_service }.to change { counter.read(:delete) }.by(2) + .and change { counter.read(:delete) }.by(2) + .and change { Event.count }.by(2) + .and change { Event.destroyed_action.for_design.count }.by(2) end it_behaves_like "a success" diff --git a/spec/services/design_management/save_designs_service_spec.rb b/spec/services/design_management/save_designs_service_spec.rb index 013d5473860..3be3ac9daca 100644 --- a/spec/services/design_management/save_designs_service_spec.rb +++ b/spec/services/design_management/save_designs_service_spec.rb @@ -65,6 +65,10 @@ describe DesignManagement::SaveDesignsService do end it_behaves_like 'a service error' + + it 'does not create an event in the activity stream' do + expect { run_service }.not_to change { Event.count } + end end context 'when the feature is available' do @@ -89,6 +93,12 @@ describe DesignManagement::SaveDesignsService do expect { run_service }.to change { counter.read(:create) }.by(1) end + it 'creates an event in the activity stream' do + expect { run_service } + .to change { Event.count }.by(1) + .and change { Event.for_design.created_action.count }.by(1) + end + it 'creates a commit in the repository' do run_service @@ -166,9 +176,12 @@ describe DesignManagement::SaveDesignsService do expect(updated_designs.first.versions.size).to eq(2) end - it 'increments the update counter' do + it 'records the correct events' do counter = Gitlab::UsageDataCounters::DesignsCounter - expect { run_service }.to change { counter.read(:update) }.by 1 + expect { run_service } + .to change { counter.read(:update) }.by(1) + .and change { Event.count }.by(1) + .and change { Event.for_design.updated_action.count }.by(1) end context 'when uploading a new design' do @@ -217,6 +230,14 @@ describe DesignManagement::SaveDesignsService do .and change { counter.read(:update) }.by(1) end + it 'creates the correct activity stream events' do + expect { run_service } + .to change { Event.count }.by(2) + .and change { Event.for_design.count }.by(2) + .and change { Event.created_action.count }.by(1) + .and change { Event.updated_action.count }.by(1) + end + it 'creates a single commit' do commit_count = -> do design_repository.expire_all_method_caches diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb index 2e9a7a293d1..7461934b455 100644 --- a/spec/services/discussions/resolve_service_spec.rb +++ b/spec/services/discussions/resolve_service_spec.rb @@ -4,28 +4,24 @@ require 'spec_helper' describe Discussions::ResolveService do describe '#execute' do - let(:discussion) { create(:diff_note_on_merge_request).to_discussion } - let(:project) { merge_request.project } - let(:merge_request) { discussion.noteable } - let(:user) { create(:user) } - let(:service) { described_class.new(discussion.noteable.project, user, merge_request: merge_request) } - - before do - project.add_maintainer(user) - end + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user, developer_projects: [project]) } + let_it_be(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds, source_project: project) } + let(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion } + let(:service) { described_class.new(project, user, one_or_more_discussions: discussion) } it "doesn't resolve discussions the user can't resolve" do expect(discussion).to receive(:can_resolve?).with(user).and_return(false) - service.execute(discussion) + service.execute - expect(discussion.resolved?).to be(false) + expect(discussion).not_to be_resolved end it 'resolves the discussion' do - service.execute(discussion) + service.execute - expect(discussion.resolved?).to be(true) + expect(discussion).to be_resolved end it 'executes the notification service' do @@ -33,24 +29,83 @@ describe Discussions::ResolveService do expect(instance).to receive(:execute).with(discussion.noteable) end - service.execute(discussion) + service.execute + end + + it 'schedules an auto-merge' do + expect(AutoMergeProcessWorker).to receive(:perform_async).with(discussion.noteable.id) + + service.execute + end + + context 'with a project that requires all discussion to be resolved' do + before do + project.update(only_allow_merge_if_all_discussions_are_resolved: true) + end + + after do + project.update(only_allow_merge_if_all_discussions_are_resolved: false) + end + + let_it_be(:other_discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion } + + it 'does not schedule an auto-merge' do + expect(AutoMergeProcessWorker).not_to receive(:perform_async) + + service.execute + end + + it 'schedules an auto-merge' do + expect(AutoMergeProcessWorker).to receive(:perform_async) + + described_class.new(project, user, one_or_more_discussions: [discussion, other_discussion]).execute + end end it 'adds a system note to the discussion' do issue = create(:issue, project: project) expect(SystemNoteService).to receive(:discussion_continued_in_issue).with(discussion, project, user, issue) - service = described_class.new(project, user, merge_request: merge_request, follow_up_issue: issue) - service.execute(discussion) + service = described_class.new(project, user, one_or_more_discussions: discussion, follow_up_issue: issue) + service.execute end it 'can resolve multiple discussions at once' do - other_discussion = create(:diff_note_on_merge_request, noteable: discussion.noteable, project: discussion.noteable.source_project).to_discussion + other_discussion = create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion + service = described_class.new(project, user, one_or_more_discussions: [discussion, other_discussion]) + service.execute - service.execute([discussion, other_discussion]) + expect([discussion, other_discussion]).to all(be_resolved) + end + + it 'raises an argument error if discussions do not belong to the same noteable' do + other_merge_request = create(:merge_request) + other_discussion = create(:diff_note_on_merge_request, + noteable: other_merge_request, + project: other_merge_request.source_project).to_discussion + expect do + described_class.new(project, user, one_or_more_discussions: [discussion, other_discussion]) + end.to raise_error( + ArgumentError, + 'Discussions must be all for the same noteable' + ) + end - expect(discussion.resolved?).to be(true) - expect(other_discussion.resolved?).to be(true) + context 'when discussion is not for a merge request' do + let_it_be(:design) { create(:design, :with_file, issue: create(:issue, project: project)) } + let(:discussion) { create(:diff_note_on_design, noteable: design, project: project).to_discussion } + + it 'does not execute the notification service' do + expect(MergeRequests::ResolvedDiscussionNotificationService).not_to receive(:new) + + service.execute + end + + it 'does not schedule an auto-merge' do + expect(AutoMergeProcessWorker).not_to receive(:perform_async) + + service.execute + end end end end diff --git a/spec/services/draft_notes/create_service_spec.rb b/spec/services/draft_notes/create_service_spec.rb new file mode 100644 index 00000000000..8f244ed386b --- /dev/null +++ b/spec/services/draft_notes/create_service_spec.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DraftNotes::CreateService do + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.target_project } + let(:user) { merge_request.author } + + def create_draft(params) + described_class.new(merge_request, user, params).execute + end + + it 'creates a simple draft note' do + draft = create_draft(note: 'This is a test') + + expect(draft).to be_an_instance_of(DraftNote) + expect(draft.note).to eq('This is a test') + expect(draft.author).to eq(user) + expect(draft.project).to eq(merge_request.target_project) + expect(draft.discussion_id).to be_nil + end + + it 'cannot resolve when there is nothing to resolve' do + draft = create_draft(note: 'Not a reply!', resolve_discussion: true) + + expect(draft.errors[:base]).to include('User is not allowed to resolve thread') + expect(draft).not_to be_persisted + end + + context 'in a thread' do + it 'creates a draft note with discussion_id' do + discussion = create(:discussion_note_on_merge_request, noteable: merge_request, project: project).discussion + + draft = create_draft(note: 'A reply!', in_reply_to_discussion_id: discussion.reply_id) + + expect(draft.note).to eq('A reply!') + expect(draft.discussion_id).to eq(discussion.reply_id) + expect(draft.resolve_discussion).to be_falsey + end + + it 'creates a draft that resolves the thread' do + discussion = create(:discussion_note_on_merge_request, noteable: merge_request, project: project).discussion + + draft = create_draft(note: 'A reply!', in_reply_to_discussion_id: discussion.reply_id, resolve_discussion: true) + + expect(draft.note).to eq('A reply!') + expect(draft.discussion_id).to eq(discussion.reply_id) + expect(draft.resolve_discussion).to be true + end + end + + it 'creates a draft note with a position in a diff' do + diff_refs = project.commit(RepoHelpers.sample_commit.id).try(:diff_refs) + + position = Gitlab::Diff::Position.new( + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + diff_refs: diff_refs + ) + + draft = create_draft(note: 'Comment on diff', position: position.to_json) + + expect(draft.note).to eq('Comment on diff') + expect(draft.original_position.to_json).to eq(position.to_json) + end + + context 'diff highlight cache clearing' do + context 'when diff file is unfolded and it is not a reply' do + it 'clears diff highlighting cache' do + expect_next_instance_of(DraftNote) do |draft| + allow(draft).to receive_message_chain(:diff_file, :unfolded?) { true } + end + + expect(merge_request).to receive_message_chain(:diffs, :clear_cache) + + create_draft(note: 'This is a test') + end + end + + context 'when diff file is not unfolded and it is not a reply' do + it 'clears diff highlighting cache' do + expect_next_instance_of(DraftNote) do |draft| + allow(draft).to receive_message_chain(:diff_file, :unfolded?) { false } + end + + expect(merge_request).not_to receive(:diffs) + + create_draft(note: 'This is a test') + end + end + end +end diff --git a/spec/services/draft_notes/destroy_service_spec.rb b/spec/services/draft_notes/destroy_service_spec.rb new file mode 100644 index 00000000000..d0bf88dcdbe --- /dev/null +++ b/spec/services/draft_notes/destroy_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DraftNotes::DestroyService do + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.target_project } + let(:user) { merge_request.author } + + def destroy(draft_note = nil) + DraftNotes::DestroyService.new(merge_request, user).execute(draft_note) + end + + it 'destroys a single draft note' do + drafts = create_list(:draft_note, 2, merge_request: merge_request, author: user) + + expect { destroy(drafts.first) } + .to change { DraftNote.count }.by(-1) + + expect(DraftNote.count).to eq(1) + end + + it 'destroys all draft notes for a user in a merge request' do + create_list(:draft_note, 2, merge_request: merge_request, author: user) + + expect { destroy }.to change { DraftNote.count }.by(-2) + expect(DraftNote.count).to eq(0) + end + + context 'diff highlight cache clearing' do + context 'when destroying all draft notes of a user' do + it 'clears highlighting cache if unfold required for any' do + drafts = create_list(:draft_note, 2, merge_request: merge_request, author: user) + + allow_any_instance_of(DraftNote).to receive_message_chain(:diff_file, :unfolded?) { true } + expect(merge_request).to receive_message_chain(:diffs, :clear_cache) + + destroy(drafts.first) + end + end + + context 'when destroying one draft note' do + it 'clears highlighting cache if unfold required' do + create_list(:draft_note, 2, merge_request: merge_request, author: user) + + allow_any_instance_of(DraftNote).to receive_message_chain(:diff_file, :unfolded?) { true } + expect(merge_request).to receive_message_chain(:diffs, :clear_cache) + + destroy + end + end + end +end diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb new file mode 100644 index 00000000000..4ebae2f9aa2 --- /dev/null +++ b/spec/services/draft_notes/publish_service_spec.rb @@ -0,0 +1,261 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DraftNotes::PublishService do + include RepoHelpers + + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.target_project } + let(:user) { merge_request.author } + let(:commit) { project.commit(sample_commit.id) } + + let(:position) do + Gitlab::Diff::Position.new( + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + diff_refs: commit.diff_refs + ) + end + + def publish(draft: nil) + DraftNotes::PublishService.new(merge_request, user).execute(draft) + end + + context 'single draft note' do + let(:commit_id) { nil } + let!(:drafts) { create_list(:draft_note, 2, merge_request: merge_request, author: user, commit_id: commit_id, position: position) } + + it 'publishes' do + expect { publish(draft: drafts.first) }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1) + expect(DraftNote.count).to eq(1) + end + + it 'does not skip notification', :sidekiq_might_not_need_inline do + expect(Notes::CreateService).to receive(:new).with(project, user, drafts.first.publish_params).and_call_original + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive(:new_note) + end + + result = publish(draft: drafts.first) + + expect(result[:status]).to eq(:success) + end + + context 'commit_id is set' do + let(:commit_id) { commit.id } + + it 'creates note from draft with commit_id' do + result = publish(draft: drafts.first) + + expect(result[:status]).to eq(:success) + expect(merge_request.notes.first.commit_id).to eq(commit_id) + end + end + end + + context 'multiple draft notes' do + let(:commit_id) { nil } + + before do + create(:draft_note, merge_request: merge_request, author: user, note: 'first note', commit_id: commit_id, position: position) + create(:draft_note, merge_request: merge_request, author: user, note: 'second note', commit_id: commit_id, position: position) + end + + context 'when review fails to create' do + before do + expect_next_instance_of(Review) do |review| + allow(review).to receive(:save!).and_raise(ActiveRecord::RecordInvalid.new(review)) + end + end + + it 'does not publish any draft note' do + expect { publish }.not_to change { DraftNote.count } + end + + it 'returns an error' do + result = publish + + expect(result[:status]).to eq(:error) + expect(result[:message]).to match(/Unable to save Review/) + end + end + + it 'returns success' do + result = publish + + expect(result[:status]).to eq(:success) + end + + it 'publishes all draft notes for a user in a merge request' do + expect { publish }.to change { DraftNote.count }.by(-2).and change { Note.count }.by(2).and change { Review.count }.by(1) + expect(DraftNote.count).to eq(0) + + notes = merge_request.notes.order(id: :asc) + expect(notes.first.note).to eq('first note') + expect(notes.last.note).to eq('second note') + end + + it 'sends batch notification' do + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive_message_chain(:async, :new_review).with(kind_of(Review)) + end + + publish + end + + context 'commit_id is set' do + let(:commit_id) { commit.id } + + it 'creates note from draft with commit_id' do + result = publish + + expect(result[:status]).to eq(:success) + + merge_request.notes.each do |note| + expect(note.commit_id).to eq(commit_id) + end + end + end + end + + context 'draft notes with suggestions' do + let(:project) { create(:project, :repository) } + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + let(:suggestion_note) do + <<-MARKDOWN.strip_heredoc + ```suggestion + foo + ``` + MARKDOWN + end + + let!(:draft) { create(:draft_note_on_text_diff, note: suggestion_note, merge_request: merge_request, author: user) } + + it 'creates a suggestion with correct content' do + expect { publish(draft: draft) }.to change { Suggestion.count }.by(1) + .and change { DiffNote.count }.from(0).to(1) + + suggestion = Suggestion.last + + expect(suggestion.from_line).to eq(14) + expect(suggestion.to_line).to eq(14) + expect(suggestion.from_content).to eq(" vars = {\n") + expect(suggestion.to_content).to eq(" foo\n") + end + + context 'when the diff is changed' do + let(:file_path) { 'files/ruby/popen.rb' } + let(:branch_name) { project.default_branch } + let(:commit) { project.repository.commit } + + def update_file(file_path, new_content) + params = { + file_path: file_path, + commit_message: "Update File", + file_content: new_content, + start_project: project, + start_branch: project.default_branch, + branch_name: branch_name + } + + Files::UpdateService.new(project, user, params).execute + end + + before do + project.add_developer(user) + end + + it 'creates a suggestion based on the latest diff content and positions' do + diff_file = merge_request.diffs(paths: [file_path]).diff_files.first + raw_data = diff_file.new_blob.data + + # Add a line break to the beginning of the file + result = update_file(file_path, raw_data.prepend("\n")) + oldrev = merge_request.diff_head_sha + newrev = result[:result] + + expect(newrev).to be_present + + # Generates new MR revision at DB level + refresh = MergeRequests::RefreshService.new(project, user) + refresh.execute(oldrev, newrev, merge_request.source_branch_ref) + + expect { publish(draft: draft) }.to change { Suggestion.count }.by(1) + .and change { DiffNote.count }.from(0).to(1) + + suggestion = Suggestion.last + + expect(suggestion.from_line).to eq(15) + expect(suggestion.to_line).to eq(15) + expect(suggestion.from_content).to eq(" vars = {\n") + expect(suggestion.to_content).to eq(" foo\n") + end + end + end + + it 'only publishes the draft notes belonging to the current user' do + other_user = create(:user) + project.add_maintainer(other_user) + + create_list(:draft_note, 2, merge_request: merge_request, author: user) + create_list(:draft_note, 2, merge_request: merge_request, author: other_user) + + expect { publish }.to change { DraftNote.count }.by(-2).and change { Note.count }.by(2) + expect(DraftNote.count).to eq(2) + end + + context 'with quick actions' do + it 'performs quick actions' do + other_user = create(:user) + project.add_developer(other_user) + + create(:draft_note, merge_request: merge_request, + author: user, + note: "thanks\n/assign #{other_user.to_reference}") + + expect { publish }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(2) + expect(merge_request.reload.assignees).to match_array([other_user]) + expect(merge_request.notes.last).to be_system + end + + it 'does not create a note if it only contains quick actions' do + create(:draft_note, merge_request: merge_request, author: user, note: "/assign #{user.to_reference}") + + expect { publish }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1) + expect(merge_request.reload.assignees).to eq([user]) + expect(merge_request.notes.last).to be_system + end + end + + context 'with drafts that resolve threads' do + let!(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) } + let!(:draft_note) { create(:draft_note, merge_request: merge_request, author: user, resolve_discussion: true, discussion_id: note.discussion.reply_id) } + + it 'resolves the thread' do + publish(draft: draft_note) + + expect(note.discussion.resolved?).to be true + end + + it 'sends notifications if all threads are resolved' do + expect_next_instance_of(MergeRequests::ResolvedDiscussionNotificationService) do |instance| + expect(instance).to receive(:execute).with(merge_request) + end + + publish + end + end + + context 'user cannot create notes' do + before do + allow(Ability).to receive(:allowed?).with(user, :create_note, merge_request).and_return(false) + end + + it 'returns an error' do + expect(publish[:status]).to eq(:error) + end + end +end diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index 987b4ad68f7..73c089334ed 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -5,6 +5,9 @@ require 'spec_helper' describe EventCreateService do let(:service) { described_class.new } + let_it_be(:user, reload: true) { create :user } + let_it_be(:project) { create(:project) } + describe 'Issues' do describe '#open_issue' do let(:issue) { create(:issue) } @@ -13,6 +16,7 @@ describe EventCreateService do it "creates new event" do expect { service.open_issue(issue, issue.author) }.to change { Event.count } + expect { service.open_issue(issue, issue.author) }.to change { ResourceStateEvent.count } end end @@ -23,6 +27,7 @@ describe EventCreateService do it "creates new event" do expect { service.close_issue(issue, issue.author) }.to change { Event.count } + expect { service.close_issue(issue, issue.author) }.to change { ResourceStateEvent.count } end end @@ -33,6 +38,7 @@ describe EventCreateService do it "creates new event" do expect { service.reopen_issue(issue, issue.author) }.to change { Event.count } + expect { service.reopen_issue(issue, issue.author) }.to change { ResourceStateEvent.count } end end end @@ -45,6 +51,7 @@ describe EventCreateService do it "creates new event" do expect { service.open_mr(merge_request, merge_request.author) }.to change { Event.count } + expect { service.open_mr(merge_request, merge_request.author) }.to change { ResourceStateEvent.count } end end @@ -55,6 +62,7 @@ describe EventCreateService do it "creates new event" do expect { service.close_mr(merge_request, merge_request.author) }.to change { Event.count } + expect { service.close_mr(merge_request, merge_request.author) }.to change { ResourceStateEvent.count } end end @@ -65,6 +73,7 @@ describe EventCreateService do it "creates new event" do expect { service.merge_mr(merge_request, merge_request.author) }.to change { Event.count } + expect { service.merge_mr(merge_request, merge_request.author) }.to change { ResourceStateEvent.count } end end @@ -75,13 +84,12 @@ describe EventCreateService do it "creates new event" do expect { service.reopen_mr(merge_request, merge_request.author) }.to change { Event.count } + expect { service.reopen_mr(merge_request, merge_request.author) }.to change { ResourceStateEvent.count } end end end describe 'Milestone' do - let(:user) { create :user } - describe '#open_milestone' do let(:milestone) { create(:milestone) } @@ -167,7 +175,7 @@ describe EventCreateService do wiki_page?: true, valid?: true, persisted?: true, - action: action, + action: action.to_s, wiki_page: wiki_page, author: user ) @@ -193,7 +201,7 @@ describe EventCreateService do end end - (Event::ACTIONS.values - Event::WIKI_ACTIONS).each do |bad_action| + (Event.actions.keys - Event::WIKI_ACTIONS).each do |bad_action| context "The action is #{bad_action}" do it 'raises an error' do expect { service.wiki_event(meta, user, bad_action) }.to raise_error(described_class::IllegalActionError) @@ -203,9 +211,6 @@ describe EventCreateService do end describe '#push', :clean_gitlab_redis_shared_state do - let(:project) { create(:project) } - let(:user) { create(:user) } - let(:push_data) do { commits: [ @@ -227,9 +232,6 @@ describe EventCreateService do end describe '#bulk_push', :clean_gitlab_redis_shared_state do - let(:project) { create(:project) } - let(:user) { create(:user) } - let(:push_data) do { action: :created, @@ -244,9 +246,6 @@ describe EventCreateService do end describe 'Project' do - let(:user) { create :user } - let(:project) { create(:project) } - describe '#join_project' do subject { service.join_project(project, user) } @@ -261,4 +260,81 @@ describe EventCreateService do it { expect { subject }.to change { Event.count }.from(0).to(1) } end end + + describe 'design events' do + let_it_be(:design) { create(:design, project: project) } + let_it_be(:author) { user } + + shared_examples 'feature flag gated multiple event creation' do + context 'the feature flag is off' do + before do + stub_feature_flags(design_activity_events: false) + end + + specify { expect(result).to be_empty } + specify { expect { result }.not_to change { Event.count } } + specify { expect { result }.not_to exceed_query_limit(0) } + end + + context 'the feature flag is enabled for a single project' do + before do + stub_feature_flags(design_activity_events: project) + end + + specify { expect(result).not_to be_empty } + specify { expect { result }.to change { Event.count }.by(1) } + end + end + + describe '#save_designs' do + let_it_be(:updated) { create_list(:design, 5) } + let_it_be(:created) { create_list(:design, 3) } + + let(:result) { service.save_designs(author, create: created, update: updated) } + + specify { expect { result }.to change { Event.count }.by(8) } + + specify { expect { result }.not_to exceed_query_limit(1) } + + it 'creates 3 created design events' do + ids = result.pluck('id') + events = Event.created_action.where(id: ids) + + expect(events.map(&:design)).to match_array(created) + end + + it 'creates 5 created design events' do + ids = result.pluck('id') + events = Event.updated_action.where(id: ids) + + expect(events.map(&:design)).to match_array(updated) + end + + it_behaves_like 'feature flag gated multiple event creation' do + let(:project) { created.first.project } + end + end + + describe '#destroy_designs' do + let_it_be(:designs) { create_list(:design, 5) } + let_it_be(:author) { create(:user) } + + let(:result) { service.destroy_designs(designs, author) } + + specify { expect { result }.to change { Event.count }.by(5) } + + specify { expect { result }.not_to exceed_query_limit(1) } + + it 'creates 5 destroyed design events' do + ids = result.pluck('id') + events = Event.destroyed_action.where(id: ids) + + expect(events.map(&:design)).to match_array(designs) + end + + it_behaves_like 'feature flag gated multiple event creation' do + let(:project) { designs.first.project } + end + end + end end diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb index ae0506ad442..908b9772c40 100644 --- a/spec/services/git/branch_hooks_service_spec.rb +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -91,7 +91,7 @@ describe Git::BranchHooksService do end describe 'Push Event' do - let(:event) { Event.find_by_action(Event::PUSHED) } + let(:event) { Event.pushed_action.first } before do service.execute @@ -101,7 +101,7 @@ describe Git::BranchHooksService do it 'generates a push event with one commit' do expect(event).to be_an_instance_of(PushEvent) expect(event.project).to eq(project) - expect(event.action).to eq(Event::PUSHED) + expect(event).to be_pushed_action expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) expect(event.push_event_payload.commit_from).to eq(oldrev) expect(event.push_event_payload.commit_to).to eq(newrev) @@ -117,7 +117,7 @@ describe Git::BranchHooksService do it 'generates a push event with more than one commit' do expect(event).to be_an_instance_of(PushEvent) expect(event.project).to eq(project) - expect(event.action).to eq(Event::PUSHED) + expect(event).to be_pushed_action expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) expect(event.push_event_payload.commit_from).to be_nil expect(event.push_event_payload.commit_to).to eq(newrev) @@ -133,7 +133,7 @@ describe Git::BranchHooksService do it 'generates a push event with no commits' do expect(event).to be_an_instance_of(PushEvent) expect(event.project).to eq(project) - expect(event.action).to eq(Event::PUSHED) + expect(event).to be_pushed_action expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) expect(event.push_event_payload.commit_from).to eq(oldrev) expect(event.push_event_payload.commit_to).to be_nil diff --git a/spec/services/git/wiki_push_service/change_spec.rb b/spec/services/git/wiki_push_service/change_spec.rb index 547874270ab..4da3f0fc738 100644 --- a/spec/services/git/wiki_push_service/change_spec.rb +++ b/spec/services/git/wiki_push_service/change_spec.rb @@ -89,20 +89,20 @@ describe Git::WikiPushService::Change do context 'the page is deleted' do let(:operation) { :deleted } - it { is_expected.to have_attributes(event_action: Event::DESTROYED) } + it { is_expected.to have_attributes(event_action: :destroyed) } end context 'the page is added' do let(:operation) { :added } - it { is_expected.to have_attributes(event_action: Event::CREATED) } + it { is_expected.to have_attributes(event_action: :created) } end %i[renamed modified].each do |op| context "the page is #{op}" do let(:operation) { op } - it { is_expected.to have_attributes(event_action: Event::UPDATED) } + it { is_expected.to have_attributes(event_action: :updated) } end end end diff --git a/spec/services/git/wiki_push_service_spec.rb b/spec/services/git/wiki_push_service_spec.rb index cdb1dc5a435..b2234c81c24 100644 --- a/spec/services/git/wiki_push_service_spec.rb +++ b/spec/services/git/wiki_push_service_spec.rb @@ -54,7 +54,7 @@ describe Git::WikiPushService, services: true do it 'handles all known actions' do run_service - expect(Event.last(count).pluck(:action)).to match_array(Event::WIKI_ACTIONS) + expect(Event.last(count).pluck(:action)).to match_array(Event::WIKI_ACTIONS.map(&:to_s)) end end @@ -77,7 +77,7 @@ describe Git::WikiPushService, services: true do it 'creates appropriate events' do run_service - expect(Event.last(2)).to all(have_attributes(wiki_page?: true, action: Event::CREATED)) + expect(Event.last(2)).to all(have_attributes(wiki_page?: true, action: 'created')) end end @@ -100,7 +100,7 @@ describe Git::WikiPushService, services: true do it 'creates a wiki page creation event' do expect { run_service }.to change(Event, :count).by(1) - expect(Event.last).to have_attributes(wiki_page?: true, action: Event::CREATED) + expect(Event.last).to have_attributes(wiki_page?: true, action: 'created') end it 'creates one metadata record' do @@ -129,7 +129,7 @@ describe Git::WikiPushService, services: true do expect(Event.last).to have_attributes( wiki_page?: true, - action: Event::CREATED + action: 'created' ) end end @@ -158,7 +158,7 @@ describe Git::WikiPushService, services: true do expect(Event.last).to have_attributes( wiki_page?: true, - action: Event::UPDATED + action: 'updated' ) end end @@ -182,7 +182,7 @@ describe Git::WikiPushService, services: true do expect(Event.last).to have_attributes( wiki_page?: true, - action: Event::UPDATED + action: 'updated' ) end end @@ -206,7 +206,7 @@ describe Git::WikiPushService, services: true do expect(Event.last).to have_attributes( wiki_page?: true, - action: Event::DESTROYED + action: 'destroyed' ) end end @@ -218,7 +218,7 @@ describe Git::WikiPushService, services: true do message = 'something went very very wrong' allow_next_instance_of(WikiPages::EventCreateService, current_user) do |service| allow(service).to receive(:execute) - .with(String, WikiPage, Integer) + .with(String, WikiPage, Symbol) .and_return(ServiceResponse.error(message: message)) end diff --git a/spec/services/groups/group_links/destroy_service_spec.rb b/spec/services/groups/group_links/destroy_service_spec.rb index 284bcd0df2e..8989f024262 100644 --- a/spec/services/groups/group_links/destroy_service_spec.rb +++ b/spec/services/groups/group_links/destroy_service_spec.rb @@ -8,14 +8,20 @@ describe Groups::GroupLinks::DestroyService, '#execute' do let_it_be(:group) { create(:group, :private) } let_it_be(:shared_group) { create(:group, :private) } let_it_be(:project) { create(:project, group: shared_group) } + let_it_be(:owner) { create(:user) } - subject { described_class.new(nil, nil) } + before do + group.add_developer(owner) + shared_group.add_owner(owner) + end + + subject { described_class.new(shared_group, owner) } context 'single link' do let!(:link) { create(:group_group_link, shared_group: shared_group, shared_with_group: group) } it 'destroys link' do - expect { subject.execute(link) }.to change { GroupGroupLink.count }.from(1).to(0) + expect { subject.execute(link) }.to change { shared_group.shared_with_group_links.count }.from(1).to(0) end it 'revokes project authorization' do diff --git a/spec/services/groups/import_export/export_service_spec.rb b/spec/services/groups/import_export/export_service_spec.rb index 7bad68b4e00..ea49b26cc7c 100644 --- a/spec/services/groups/import_export/export_service_spec.rb +++ b/spec/services/groups/import_export/export_service_spec.rb @@ -103,12 +103,14 @@ describe Groups::ImportExport::ExportService do end it 'logs the error' do - expect(shared.logger).to receive(:error).with( - group_id: group.id, - group_name: group.name, - error: expected_message, - message: 'Group Import/Export: Export failed' - ) + expect_next_instance_of(Gitlab::Export::Logger) do |logger| + expect(logger).to receive(:error).with( + group_id: group.id, + group_name: group.name, + errors: expected_message, + message: 'Group Export failed' + ) + end expect { service.execute }.to raise_error(Gitlab::ImportExport::Error) end @@ -132,7 +134,7 @@ describe Groups::ImportExport::ExportService do expect { service.execute }.to raise_error(Gitlab::ImportExport::Error) expect(group.import_export_upload).to be_nil - expect(File.exist?(shared.archive_path)).to eq(false) + expect(Dir.exist?(shared.base_path)).to eq(false) end it 'notifies the user about failed group export' do @@ -157,12 +159,13 @@ describe Groups::ImportExport::ExportService do expect { service.execute }.to raise_error(Gitlab::ImportExport::Error) expect(group.import_export_upload).to be_nil - expect(File.exist?(shared.archive_path)).to eq(false) + expect(Dir.exist?(shared.base_path)).to eq(false) end it 'notifies logger' do allow(service).to receive_message_chain(:tree_exporter, :save).and_return(false) - expect(shared.logger).to receive(:error) + + expect(service.instance_variable_get(:@logger)).to receive(:error) expect { service.execute }.to raise_error(Gitlab::ImportExport::Error) end diff --git a/spec/services/groups/import_export/import_service_spec.rb b/spec/services/groups/import_export/import_service_spec.rb index 256e0a1b3c5..1f7eaccbdbd 100644 --- a/spec/services/groups/import_export/import_service_spec.rb +++ b/spec/services/groups/import_export/import_service_spec.rb @@ -3,6 +3,47 @@ require 'spec_helper' describe Groups::ImportExport::ImportService do + describe '#async_execute' do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + + context 'when the job can be successfully scheduled' do + subject(:import_service) { described_class.new(group: group, user: user) } + + it 'enqueues an import job' do + expect(GroupImportWorker).to receive(:perform_async).with(user.id, group.id) + + import_service.async_execute + end + + it 'marks the group import as in progress' do + import_service.async_execute + + expect(group.import_state.in_progress?).to eq true + end + + it 'returns truthy' do + expect(import_service.async_execute).to be_truthy + end + end + + context 'when the job cannot be scheduled' do + subject(:import_service) { described_class.new(group: group, user: user) } + + before do + allow(GroupImportWorker).to receive(:perform_async).and_return(nil) + end + + it 'returns falsey' do + expect(import_service.async_execute).to be_falsey + end + + it 'does not mark the group import as created' do + expect { import_service.async_execute }.not_to change { group.import_state } + end + end + end + context 'with group_import_ndjson feature flag disabled' do let(:user) { create(:admin) } let(:group) { create(:group) } @@ -60,6 +101,7 @@ describe Groups::ImportExport::ImportService do allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger) allow(import_logger).to receive(:error) allow(import_logger).to receive(:info) + allow(FileUtils).to receive(:rm_rf).and_call_original end context 'when user has correct permissions' do @@ -73,6 +115,16 @@ describe Groups::ImportExport::ImportService do expect(group.import_export_upload.import_file.file).to be_nil end + it 'removes tmp files' do + shared = Gitlab::ImportExport::Shared.new(group) + allow(Gitlab::ImportExport::Shared).to receive(:new).and_return(shared) + + subject + + expect(FileUtils).to have_received(:rm_rf).with(shared.base_path) + expect(Dir.exist?(shared.base_path)).to eq(false) + end + it 'logs the import success' do expect(import_logger).to receive(:info).with( group_id: group.id, @@ -160,6 +212,7 @@ describe Groups::ImportExport::ImportService do allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger) allow(import_logger).to receive(:error) allow(import_logger).to receive(:info) + allow(FileUtils).to receive(:rm_rf).and_call_original end context 'when user has correct permissions' do @@ -173,6 +226,16 @@ describe Groups::ImportExport::ImportService do expect(group.import_export_upload.import_file.file).to be_nil end + it 'removes tmp files' do + shared = Gitlab::ImportExport::Shared.new(group) + allow(Gitlab::ImportExport::Shared).to receive(:new).and_return(shared) + + subject + + expect(FileUtils).to have_received(:rm_rf).with(shared.base_path) + expect(Dir.exist?(shared.base_path)).to eq(false) + end + it 'logs the import success' do expect(import_logger).to receive(:info).with( group_id: group.id, diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index bbf5bbbf814..d7f6bececfe 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -216,6 +216,15 @@ describe Groups::TransferService do end end + context 'when a group is transferred to its subgroup' do + let(:new_parent_group) { create(:group, parent: group) } + + it 'does not execute the transfer' do + expect(transfer_service.execute(new_parent_group)).to be_falsy + expect(transfer_service.error).to match(/Cannot transfer group to one of its subgroup/) + end + end + context 'when transferring a group with group descendants' do let!(:subgroup1) { create(:group, :private, parent: group) } let!(:subgroup2) { create(:group, :internal, parent: group) } diff --git a/spec/services/import/github_service_spec.rb b/spec/services/import/github_service_spec.rb new file mode 100644 index 00000000000..461b17e0e33 --- /dev/null +++ b/spec/services/import/github_service_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Import::GithubService do + let_it_be(:user) { create(:user) } + let_it_be(:token) { 'complex-token' } + let_it_be(:access_params) { { github_access_token: 'github-complex-token' } } + let_it_be(:client) { Gitlab::LegacyGithubImport::Client.new(token) } + let_it_be(:params) { { repo_id: 123, new_name: 'new_repo', target_namespace: 'root' } } + + let(:subject) { described_class.new(client, user, params) } + + before do + allow(subject).to receive(:authorized?).and_return(true) + end + + context 'do not raise an exception on input error' do + let(:exception) { Octokit::ClientError.new(status: 404, body: 'Not Found') } + + before do + expect(client).to receive(:repo).and_raise(exception) + end + + it 'logs the original error' do + expect(Gitlab::Import::Logger).to receive(:error).with({ + message: 'Import failed due to a GitHub error', + status: 404, + error: 'Not Found' + }).and_call_original + + subject.execute(access_params, :github) + end + + it 'returns an error' do + result = subject.execute(access_params, :github) + + expect(result).to include( + message: 'Import failed due to a GitHub error: Not Found', + status: :error, + http_status: :unprocessable_entity + ) + end + end + + it 'raises an exception for unknown error causes' do + exception = StandardError.new('Not Implemented') + + expect(client).to receive(:repo).and_raise(exception) + + expect(Gitlab::Import::Logger).not_to receive(:error) + + expect { subject.execute(access_params, :github) }.to raise_error(exception) + end +end diff --git a/spec/services/integrations/test/project_service_spec.rb b/spec/services/integrations/test/project_service_spec.rb new file mode 100644 index 00000000000..fdb43ca345a --- /dev/null +++ b/spec/services/integrations/test/project_service_spec.rb @@ -0,0 +1,195 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Integrations::Test::ProjectService do + let(:user) { double('user') } + + describe '#execute' do + let(:project) { create(:project) } + let(:integration) { create(:slack_service, project: project) } + let(:event) { nil } + let(:sample_data) { { data: 'sample' } } + let(:success_result) { { success: true, result: {} } } + + subject { described_class.new(integration, user, event).execute } + + context 'without event specified' do + it 'tests the integration with default data' do + allow(Gitlab::DataBuilder::Push).to receive(:build_sample).and_return(sample_data) + + expect(integration).to receive(:test).with(sample_data).and_return(success_result) + expect(subject).to eq(success_result) + end + + context 'PipelinesEmailService' do + let(:integration) { create(:pipelines_email_service, project: project) } + + it_behaves_like 'tests for integration with pipeline data' + end + end + + context 'with event specified' do + context 'event not supported by integration' do + let(:integration) { create(:jira_service, project: project) } + let(:event) { 'push' } + + it 'returns error message' do + expect(subject).to include({ status: :error, message: 'Testing not available for this event' }) + end + end + + context 'push' do + let(:event) { 'push' } + + it 'executes integration' do + allow(Gitlab::DataBuilder::Push).to receive(:build_sample).and_return(sample_data) + + expect(integration).to receive(:test).with(sample_data).and_return(success_result) + expect(subject).to eq(success_result) + end + end + + context 'tag_push' do + let(:event) { 'tag_push' } + + it 'executes integration' do + allow(Gitlab::DataBuilder::Push).to receive(:build_sample).and_return(sample_data) + + expect(integration).to receive(:test).with(sample_data).and_return(success_result) + expect(subject).to eq(success_result) + end + end + + context 'note' do + let(:event) { 'note' } + + it 'returns error message if not enough data' do + expect(integration).not_to receive(:test) + expect(subject).to include({ status: :error, message: 'Ensure the project has notes.' }) + end + + it 'executes integration' do + allow(project).to receive(:notes).and_return([Note.new]) + allow(Gitlab::DataBuilder::Note).to receive(:build).and_return(sample_data) + + expect(integration).to receive(:test).with(sample_data).and_return(success_result) + expect(subject).to eq(success_result) + end + end + + context 'issue' do + let(:event) { 'issue' } + let(:issue) { build(:issue) } + + it 'returns error message if not enough data' do + expect(integration).not_to receive(:test) + expect(subject).to include({ status: :error, message: 'Ensure the project has issues.' }) + end + + it 'executes integration' do + allow(project).to receive(:issues).and_return([issue]) + allow(issue).to receive(:to_hook_data).and_return(sample_data) + + expect(integration).to receive(:test).with(sample_data).and_return(success_result) + expect(subject).to eq(success_result) + end + end + + context 'confidential_issue' do + let(:event) { 'confidential_issue' } + let(:issue) { build(:issue) } + + it 'returns error message if not enough data' do + expect(integration).not_to receive(:test) + expect(subject).to include({ status: :error, message: 'Ensure the project has issues.' }) + end + + it 'executes integration' do + allow(project).to receive(:issues).and_return([issue]) + allow(issue).to receive(:to_hook_data).and_return(sample_data) + + expect(integration).to receive(:test).with(sample_data).and_return(success_result) + expect(subject).to eq(success_result) + end + end + + context 'merge_request' do + let(:event) { 'merge_request' } + + it 'returns error message if not enough data' do + expect(integration).not_to receive(:test) + expect(subject).to include({ status: :error, message: 'Ensure the project has merge requests.' }) + end + + it 'executes integration' do + create(:merge_request, source_project: project) + allow_any_instance_of(MergeRequest).to receive(:to_hook_data).and_return(sample_data) + + expect(integration).to receive(:test).with(sample_data).and_return(success_result) + expect(subject).to eq(success_result) + end + end + + context 'deployment' do + let(:project) { create(:project, :test_repo) } + let(:event) { 'deployment' } + + it 'returns error message if not enough data' do + expect(integration).not_to receive(:test) + expect(subject).to include({ status: :error, message: 'Ensure the project has deployments.' }) + end + + it 'executes integration' do + create(:deployment, project: project) + allow(Gitlab::DataBuilder::Deployment).to receive(:build).and_return(sample_data) + + expect(integration).to receive(:test).with(sample_data).and_return(success_result) + expect(subject).to eq(success_result) + end + end + + context 'pipeline' do + let(:event) { 'pipeline' } + + it 'returns error message if not enough data' do + expect(integration).not_to receive(:test) + expect(subject).to include({ status: :error, message: 'Ensure the project has CI pipelines.' }) + end + + it 'executes integration' do + create(:ci_empty_pipeline, project: project) + allow(Gitlab::DataBuilder::Pipeline).to receive(:build).and_return(sample_data) + + expect(integration).to receive(:test).with(sample_data).and_return(success_result) + expect(subject).to eq(success_result) + end + end + + context 'wiki_page' do + let(:project) { create(:project, :wiki_repo) } + let(:event) { 'wiki_page' } + + it 'returns error message if wiki disabled' do + allow(project).to receive(:wiki_enabled?).and_return(false) + + expect(integration).not_to receive(:test) + expect(subject).to include({ status: :error, message: 'Ensure the wiki is enabled and has pages.' }) + end + + it 'returns error message if not enough data' do + expect(integration).not_to receive(:test) + expect(subject).to include({ status: :error, message: 'Ensure the wiki is enabled and has pages.' }) + end + + it 'executes integration' do + create(:wiki_page, wiki: project.wiki) + allow(Gitlab::DataBuilder::WikiPage).to receive(:build).and_return(sample_data) + + expect(integration).to receive(:test).with(sample_data).and_return(success_result) + expect(subject).to eq(success_result) + end + end + end + end +end diff --git a/spec/services/issuable/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb index f82a3cee1d9..c791c454d70 100644 --- a/spec/services/issuable/bulk_update_service_spec.rb +++ b/spec/services/issuable/bulk_update_service_spec.rb @@ -42,13 +42,11 @@ describe Issuable::BulkUpdateService do let(:issue_no_labels) { create(:issue, project: project) } let(:issues) { [issue_all_labels, issue_bug_and_regression, issue_bug_and_merge_requests, issue_no_labels] } - let(:labels) { [] } let(:add_labels) { [] } let(:remove_labels) { [] } let(:bulk_update_params) do { - label_ids: labels.map(&:id), add_label_ids: add_labels.map(&:id), remove_label_ids: remove_labels.map(&:id) } @@ -58,27 +56,6 @@ describe Issuable::BulkUpdateService do bulk_update(issues, bulk_update_params) end - context 'when label_ids are passed' do - let(:issues) { [issue_all_labels, issue_no_labels] } - let(:labels) { [bug, regression] } - - it 'updates the labels of all issues passed to the labels passed' do - expect(issues.map(&:reload).map(&:label_ids)).to all(match_array(labels.map(&:id))) - end - - it 'does not update issues not passed in' do - expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id) - end - - context 'when those label IDs are empty' do - let(:labels) { [] } - - it 'updates the issues passed to have no labels' do - expect(issues.map(&:reload).map(&:label_ids)).to all(be_empty) - end - end - end - context 'when add_label_ids are passed' do let(:issues) { [issue_all_labels, issue_bug_and_merge_requests, issue_no_labels] } let(:add_labels) { [bug, regression, merge_requests] } @@ -122,69 +99,21 @@ describe Issuable::BulkUpdateService do expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id) end end + end - context 'when add_label_ids and label_ids are passed' do - let(:issues) { [issue_all_labels, issue_bug_and_regression, issue_bug_and_merge_requests] } - let(:labels) { [merge_requests] } - let(:add_labels) { [regression] } - - it 'adds the label IDs to all issues passed' do - expect(issues.map(&:reload).map(&:label_ids)).to all(include(regression.id)) - end - - it 'ignores the label IDs parameter' do - expect(issues.map(&:reload).map(&:label_ids)).to all(include(bug.id)) - end - - it 'does not update issues not passed in' do - expect(issue_no_labels.label_ids).to be_empty - end - end - - context 'when remove_label_ids and label_ids are passed' do - let(:issues) { [issue_no_labels, issue_bug_and_regression] } - let(:labels) { [merge_requests] } - let(:remove_labels) { [regression] } - - it 'removes the label IDs from all issues passed' do - expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(regression.id) - end - - it 'ignores the label IDs parameter' do - expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(merge_requests.id) - end - - it 'does not update issues not passed in' do - expect(issue_all_labels.label_ids).to contain_exactly(bug.id, regression.id, merge_requests.id) - end - end - - context 'when add_label_ids, remove_label_ids, and label_ids are passed' do - let(:issues) { [issue_bug_and_merge_requests, issue_no_labels] } - let(:labels) { [regression] } - let(:add_labels) { [bug] } - let(:remove_labels) { [merge_requests] } - - it 'adds the label IDs to all issues passed' do - expect(issues.map(&:reload).map(&:label_ids)).to all(include(bug.id)) - end + context 'with issuables at a project level' do + let(:parent) { project } - it 'removes the label IDs from all issues passed' do - expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(merge_requests.id) - end + context 'with unpermitted attributes' do + let(:issues) { create_list(:issue, 2, project: project) } + let(:label) { create(:label, project: project) } - it 'ignores the label IDs parameter' do - expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(regression.id) - end + it 'does not update the issues' do + bulk_update(issues, label_ids: [label.id]) - it 'does not update issues not passed in' do - expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id) + expect(issues.map(&:reload).map(&:label_ids)).not_to include(label.id) end end - end - - context 'with issuables at a project level' do - let(:parent) { project } describe 'close issues' do let(:issues) { create_list(:issue, 2, project: project) } diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 6fc1928d47b..78eba565de4 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -224,11 +224,26 @@ describe Issues::CloseService do expect(email.subject).to include(issue.title) end - it 'creates system note about issue reassign' do - close_issue + context 'when resource state events are disabled' do + before do + stub_feature_flags(track_resource_state_change_events: false) + end + + it 'creates system note about the issue being closed' do + close_issue + + note = issue.notes.last + expect(note.note).to include "closed" + end + end - note = issue.notes.last - expect(note.note).to include "closed" + context 'when resource state events are enabled' do + it 'creates resource state event about the issue being closed' do + close_issue + + event = issue.resource_state_events.last + expect(event.state).to eq('closed') + end end it 'marks todos as done' do diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 7a251e03e51..bb02941576a 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -458,7 +458,7 @@ describe Issues::CreateService do context 'when SpamVerdictService requires reCAPTCHA' do before do expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service| - expect(verdict_service).to receive(:execute).and_return(REQUIRE_RECAPTCHA) + expect(verdict_service).to receive(:execute).and_return(CONDITIONAL_ALLOW) end end diff --git a/spec/services/issues/import_csv_service_spec.rb b/spec/services/issues/import_csv_service_spec.rb index aa43892a36d..92b88489af9 100644 --- a/spec/services/issues/import_csv_service_spec.rb +++ b/spec/services/issues/import_csv_service_spec.rb @@ -18,9 +18,7 @@ describe Issues::ImportCsvService do let(:file) { fixture_file_upload('spec/fixtures/banana_sample.gif') } it 'returns invalid file error' do - expect_next_instance_of(Notify) do |instance| - expect(instance).to receive(:import_issues_csv_email) - end + expect(Notify).to receive_message_chain(:import_issues_csv_email, :deliver_later) expect(subject[:success]).to eq(0) expect(subject[:parse_error]).to eq(true) @@ -31,9 +29,7 @@ describe Issues::ImportCsvService do let(:file) { fixture_file_upload('spec/fixtures/csv_gitlab_export.csv') } it 'imports the CSV without errors' do - expect_next_instance_of(Notify) do |instance| - expect(instance).to receive(:import_issues_csv_email) - end + expect(Notify).to receive_message_chain(:import_issues_csv_email, :deliver_later) expect(subject[:success]).to eq(4) expect(subject[:error_lines]).to eq([]) @@ -54,9 +50,7 @@ describe Issues::ImportCsvService do let(:file) { fixture_file_upload('spec/fixtures/csv_comma.csv') } it 'imports CSV without errors' do - expect_next_instance_of(Notify) do |instance| - expect(instance).to receive(:import_issues_csv_email) - end + expect(Notify).to receive_message_chain(:import_issues_csv_email, :deliver_later) expect(subject[:success]).to eq(3) expect(subject[:error_lines]).to eq([]) @@ -77,9 +71,7 @@ describe Issues::ImportCsvService do let(:file) { fixture_file_upload('spec/fixtures/csv_tab.csv') } it 'imports CSV with some error rows' do - expect_next_instance_of(Notify) do |instance| - expect(instance).to receive(:import_issues_csv_email) - end + expect(Notify).to receive_message_chain(:import_issues_csv_email, :deliver_later) expect(subject[:success]).to eq(2) expect(subject[:error_lines]).to eq([3]) @@ -100,9 +92,7 @@ describe Issues::ImportCsvService do let(:file) { fixture_file_upload('spec/fixtures/csv_semicolon.csv') } it 'imports CSV with a blank row' do - expect_next_instance_of(Notify) do |instance| - expect(instance).to receive(:import_issues_csv_email) - end + expect(Notify).to receive_message_chain(:import_issues_csv_email, :deliver_later) expect(subject[:success]).to eq(3) expect(subject[:error_lines]).to eq([4]) diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 80039049bc3..33ae2682d01 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -3,13 +3,13 @@ require 'spec_helper' describe Issues::UpdateService, :mailer do - let(:user) { create(:user) } - let(:user2) { create(:user) } - let(:user3) { create(:user) } - let(:group) { create(:group, :public) } - let(:project) { create(:project, :repository, group: group) } - let(:label) { create(:label, project: project) } - let(:label2) { create(:label) } + let_it_be(:user) { create(:user) } + let_it_be(:user2) { create(:user) } + let_it_be(:user3) { create(:user) } + let_it_be(:group) { create(:group, :public) } + let_it_be(:project, reload: true) { create(:project, :repository, group: group) } + let_it_be(:label) { create(:label, project: project) } + let_it_be(:label2) { create(:label, project: project) } let(:issue) do create(:issue, title: 'Old title', @@ -19,7 +19,7 @@ describe Issues::UpdateService, :mailer do author: create(:user)) end - before do + before_all do project.add_maintainer(user) project.add_developer(user2) project.add_developer(user3) @@ -669,28 +669,24 @@ describe Issues::UpdateService, :mailer do context 'when add_label_ids and label_ids are passed' do let(:params) { { label_ids: [label.id], add_label_ids: [label3.id] } } - it 'ignores the label_ids parameter' do - expect(result.label_ids).not_to include(label.id) + before do + issue.update(labels: [label2]) end - it 'adds the passed labels' do - expect(result.label_ids).to include(label3.id) + it 'replaces the labels with the ones in label_ids and adds those in add_label_ids' do + expect(result.label_ids).to contain_exactly(label.id, label3.id) end end context 'when remove_label_ids and label_ids are passed' do - let(:params) { { label_ids: [], remove_label_ids: [label.id] } } + let(:params) { { label_ids: [label.id, label2.id, label3.id], remove_label_ids: [label.id] } } before do issue.update(labels: [label, label3]) end - it 'ignores the label_ids parameter' do - expect(result.label_ids).not_to be_empty - end - - it 'removes the passed labels' do - expect(result.label_ids).not_to include(label.id) + it 'replaces the labels with the ones in label_ids and removes those in remove_label_ids' do + expect(result.label_ids).to contain_exactly(label2.id, label3.id) end end diff --git a/spec/services/jira/requests/projects_spec.rb b/spec/services/jira/requests/projects_spec.rb new file mode 100644 index 00000000000..f7b9aa7c00c --- /dev/null +++ b/spec/services/jira/requests/projects_spec.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Jira::Requests::Projects do + let(:jira_service) { create(:jira_service) } + let(:params) { {} } + + describe '#execute' do + let(:service) { described_class.new(jira_service, params) } + + subject { service.execute } + + context 'without jira_service' do + before do + jira_service.update!(active: false) + end + + it 'returns an error response' do + expect(subject.error?).to be_truthy + expect(subject.message).to eq('Jira service not configured.') + end + end + + context 'when jira_service is nil' do + let(:jira_service) { nil } + + it 'returns an error response' do + expect(subject.error?).to be_truthy + expect(subject.message).to eq('Jira service not configured.') + end + end + + context 'with jira_service' do + context 'when limit is invalid' do + let(:params) { { limit: 0 } } + + it 'returns a paylod with no projects returned' do + expect(subject.payload[:projects]).to be_empty + end + end + + context 'when validations and params are ok' do + let(:client) { double(options: { site: 'https://jira.example.com' }) } + + before do + expect(service).to receive(:client).at_least(:once).and_return(client) + end + + context 'when the request to Jira returns an error' do + before do + expect(client).to receive(:get).and_raise(Timeout::Error) + end + + it 'returns an error response' do + expect(subject.error?).to be_truthy + expect(subject.message).to eq('Jira request error: Timeout::Error') + end + end + + context 'when the request does not return any values' do + before do + expect(client).to receive(:get).and_return({ 'someKey' => 'value' }) + end + + it 'returns a paylod with no projects returned' do + payload = subject.payload + + expect(subject.success?).to be_truthy + expect(payload[:projects]).to be_empty + expect(payload[:is_last]).to be_truthy + end + end + + context 'when the request returns values' do + before do + expect(client).to receive(:get).and_return( + { 'values' => %w(project1 project2), 'isLast' => false } + ) + expect(JIRA::Resource::Project).to receive(:build).with(client, 'project1').and_return('jira_project1') + expect(JIRA::Resource::Project).to receive(:build).with(client, 'project2').and_return('jira_project2') + end + + it 'returns a paylod with jira projets' do + payload = subject.payload + + expect(subject.success?).to be_truthy + expect(payload[:projects]).to eq(%w(jira_project1 jira_project2)) + expect(payload[:is_last]).to be_falsey + end + end + end + end + end +end diff --git a/spec/services/jira_import/start_import_service_spec.rb b/spec/services/jira_import/start_import_service_spec.rb index 759e4f3363f..9dc8cdb1475 100644 --- a/spec/services/jira_import/start_import_service_spec.rb +++ b/spec/services/jira_import/start_import_service_spec.rb @@ -13,7 +13,7 @@ describe JiraImport::StartImportService do context 'when an error is returned from the project validation' do before do - allow(project).to receive(:validate_jira_import_settings!) + allow(Gitlab::JiraImport).to receive(:validate_project_settings!) .and_raise(Projects::ImportService::Error, 'Jira import feature is disabled.') end @@ -25,7 +25,7 @@ describe JiraImport::StartImportService do before do stub_jira_service_test - allow(project).to receive(:validate_jira_import_settings!) + allow(Gitlab::JiraImport).to receive(:validate_project_settings!) end context 'when Jira project key is not provided' do @@ -45,6 +45,22 @@ describe JiraImport::StartImportService do it_behaves_like 'responds with error', 'Jira import is already running.' end + context 'when an error is raised while scheduling import' do + before do + expect_next_instance_of(JiraImportState) do |jira_impport| + expect(jira_impport).to receive(:schedule!).and_raise(Projects::ImportService::Error, 'Unexpected failure.') + end + end + + it_behaves_like 'responds with error', 'Unexpected failure.' + + it 'saves the error message' do + subject + + expect(JiraImportState.last.error_message).to eq('Unexpected failure.') + end + end + context 'when everything is ok' do it 'returns success response' do expect(subject).to be_a(ServiceResponse) @@ -57,7 +73,7 @@ describe JiraImport::StartImportService do expect(project.latest_jira_import).to be_scheduled end - it 'creates Jira import data' do + it 'creates Jira import data', :aggregate_failures do jira_import = subject.payload[:import_data] expect(jira_import.jira_project_xid).to eq(0) @@ -72,8 +88,8 @@ describe JiraImport::StartImportService do it 'creates Jira label title with correct number' do jira_import = subject.payload[:import_data] - label_title = "jira-import::#{jira_import.jira_project_key}-1" + expect(jira_import.label.title).to eq(label_title) end end @@ -83,8 +99,8 @@ describe JiraImport::StartImportService do it 'creates Jira label title with correct number' do jira_import = subject.payload[:import_data] - label_title = "jira-import::#{jira_import.jira_project_key}-4" + expect(jira_import.label.title).to eq(label_title) end end diff --git a/spec/services/jira_import/users_importer_spec.rb b/spec/services/jira_import/users_importer_spec.rb new file mode 100644 index 00000000000..28ce5f1b44b --- /dev/null +++ b/spec/services/jira_import/users_importer_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe JiraImport::UsersImporter do + include JiraServiceHelper + + let_it_be(:user) { create(:user) } + let_it_be(:project, reload: true) { create(:project) } + let_it_be(:start_at) { 7 } + + let(:importer) { described_class.new(user, project, start_at) } + + subject { importer.execute } + + describe '#execute' do + before do + stub_jira_service_test + project.add_maintainer(user) + end + + context 'when Jira import is not configured properly' do + it 'returns an error' do + expect(subject.errors).to eq(['Jira integration not configured.']) + end + end + + context 'when Jira import is configured correctly' do + let_it_be(:jira_service) { create(:jira_service, project: project, active: true) } + let(:client) { double } + + before do + expect(importer).to receive(:client).and_return(client) + end + + context 'when jira client raises an error' do + it 'returns an error response' do + expect(client).to receive(:get).and_raise(Timeout::Error) + + expect(subject.error?).to be_truthy + expect(subject.message).to include('There was an error when communicating to Jira') + end + end + + context 'when jira client returns result' do + before do + allow(client).to receive(:get).with('/rest/api/2/users?maxResults=50&startAt=7') + .and_return(jira_users) + end + + context 'when jira client returns an empty array' do + let(:jira_users) { [] } + + it 'retturns nil payload' do + expect(subject.success?).to be_truthy + expect(subject.payload).to be_nil + end + end + + context 'when jira client returns an results' do + let(:jira_users) { [{ 'name' => 'user1' }, { 'name' => 'user2' }] } + let(:mapped_users) { [{ jira_display_name: 'user1', gitlab_id: 5 }] } + + before do + expect(JiraImport::UsersMapper).to receive(:new).with(project, jira_users) + .and_return(double(execute: mapped_users)) + end + + it 'returns the mapped users' do + expect(subject.success?).to be_truthy + expect(subject.payload).to eq(mapped_users) + end + end + end + end + end +end diff --git a/spec/services/jira_import/users_mapper_spec.rb b/spec/services/jira_import/users_mapper_spec.rb new file mode 100644 index 00000000000..75dbc41aa2e --- /dev/null +++ b/spec/services/jira_import/users_mapper_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe JiraImport::UsersMapper do + let_it_be(:project) { create(:project) } + + subject { described_class.new(project, jira_users).execute } + + describe '#execute' do + context 'jira_users is nil' do + let(:jira_users) { nil } + + it 'returns an empty array' do + expect(subject).to be_empty + end + end + + context 'when jira_users is present' do + let(:jira_users) do + [ + { 'accountId' => 'abcd', 'displayName' => 'user1' }, + { 'accountId' => 'efg' }, + { 'accountId' => 'hij', 'displayName' => 'user3', 'emailAddress' => 'user3@example.com' } + ] + end + + # TODO: now we only create an array in a proper format + # mapping is tracked in https://gitlab.com/gitlab-org/gitlab/-/issues/219023 + let(:mapped_users) do + [ + { jira_account_id: 'abcd', jira_display_name: 'user1', jira_email: nil, gitlab_id: nil }, + { jira_account_id: 'efg', jira_display_name: nil, jira_email: nil, gitlab_id: nil }, + { jira_account_id: 'hij', jira_display_name: 'user3', jira_email: 'user3@example.com', gitlab_id: nil } + ] + end + + it 'returns users mapped to Gitlab' do + expect(subject).to eq(mapped_users) + end + end + end +end diff --git a/spec/services/labels/available_labels_service_spec.rb b/spec/services/labels/available_labels_service_spec.rb index 3ba1add121d..56826257d6f 100644 --- a/spec/services/labels/available_labels_service_spec.rb +++ b/spec/services/labels/available_labels_service_spec.rb @@ -73,6 +73,12 @@ describe Labels::AvailableLabelsService do expect(result).to match_array([project_label.id, group_label.id]) end + + it 'returns labels in preserved order' do + result = described_class.new(user, project, ids: label_ids.reverse).filter_labels_ids_in_param(:ids) + + expect(result).to eq([group_label.id, project_label.id]) + end end context 'when parent is a group' do diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index b037b73752e..0e51de48fb1 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -19,45 +19,54 @@ describe MergeRequests::CloseService do describe '#execute' do it_behaves_like 'cache counters invalidator' - context 'valid params' do - let(:service) { described_class.new(project, user, {}) } + [true, false].each do |state_tracking_enabled| + context "valid params with state_tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}" do + let(:service) { described_class.new(project, user, {}) } - before do - allow(service).to receive(:execute_hooks) + before do + stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - perform_enqueued_jobs do - @merge_request = service.execute(merge_request) + allow(service).to receive(:execute_hooks) + + perform_enqueued_jobs do + @merge_request = service.execute(merge_request) + end end - end - it { expect(@merge_request).to be_valid } - it { expect(@merge_request).to be_closed } + it { expect(@merge_request).to be_valid } + it { expect(@merge_request).to be_closed } - it 'executes hooks with close action' do - expect(service).to have_received(:execute_hooks) + it 'executes hooks with close action' do + expect(service).to have_received(:execute_hooks) .with(@merge_request, 'close') - end + end - it 'sends email to user2 about assign of new merge_request', :sidekiq_might_not_need_inline do - email = ActionMailer::Base.deliveries.last - expect(email.to.first).to eq(user2.email) - expect(email.subject).to include(merge_request.title) - end + it 'sends email to user2 about assign of new merge_request', :sidekiq_might_not_need_inline do + email = ActionMailer::Base.deliveries.last + expect(email.to.first).to eq(user2.email) + expect(email.subject).to include(merge_request.title) + end - it 'creates system note about merge_request reassign' do - note = @merge_request.notes.last - expect(note.note).to include 'closed' - end + it 'creates system note about merge_request reassign' do + if state_tracking_enabled + event = @merge_request.resource_state_events.last + expect(event.state).to eq('closed') + else + note = @merge_request.notes.last + expect(note.note).to include 'closed' + end + end - it 'marks todos as done' do - expect(todo.reload).to be_done - end + it 'marks todos as done' do + expect(todo.reload).to be_done + end - context 'when auto merge is enabled' do - let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + context 'when auto merge is enabled' do + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } - it 'cancels the auto merge' do - expect(@merge_request).not_to be_auto_merge_enabled + it 'cancels the auto merge' do + expect(@merge_request).not_to be_auto_merge_enabled + end end end end diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 9155db16d17..bb40c399b6e 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -59,7 +59,7 @@ describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do it 'creates exactly 1 create MR event', :sidekiq_might_not_need_inline do attributes = { - action: Event::CREATED, + action: :created, target_id: merge_request.id, target_type: merge_request.class.name } @@ -136,11 +136,11 @@ describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do let!(:pipeline_3) { create(:ci_pipeline, project: project, ref: "other_branch", project_id: project.id) } before do - # rubocop: disable DestroyAll + # rubocop: disable Cop/DestroyAll project.merge_requests .where(source_branch: opts[:source_branch], target_branch: opts[:target_branch]) .destroy_all - # rubocop: enable DestroyAll + # rubocop: enable Cop/DestroyAll end it 'sets head pipeline' do diff --git a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb index a1c86467f34..2adf808619d 100644 --- a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb +++ b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb @@ -48,12 +48,12 @@ describe MergeRequests::DeleteNonLatestDiffsService, :clean_gitlab_redis_shared_ end it 'schedules no removal if there is no non-latest diffs' do - # rubocop: disable DestroyAll + # rubocop: disable Cop/DestroyAll merge_request .merge_request_diffs .where.not(id: merge_request.latest_merge_request_diff_id) .destroy_all - # rubocop: enable DestroyAll + # rubocop: enable Cop/DestroyAll expect(DeleteDiffFilesWorker).not_to receive(:bulk_perform_in) diff --git a/spec/services/merge_requests/ff_merge_service_spec.rb b/spec/services/merge_requests/ff_merge_service_spec.rb index 87fcd70a298..415b351e13a 100644 --- a/spec/services/merge_requests/ff_merge_service_spec.rb +++ b/spec/services/merge_requests/ff_merge_service_spec.rb @@ -21,65 +21,74 @@ describe MergeRequests::FfMergeService do end describe '#execute' do - context 'valid params' do - let(:service) { described_class.new(project, user, valid_merge_params) } - - def execute_ff_merge - perform_enqueued_jobs do - service.execute(merge_request) + [true, false].each do |state_tracking_enabled| + context "valid params with state_tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}" do + let(:service) { described_class.new(project, user, valid_merge_params) } + + def execute_ff_merge + perform_enqueued_jobs do + service.execute(merge_request) + end end - end - before do - allow(service).to receive(:execute_hooks) - end + before do + stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - it "does not create merge commit" do - execute_ff_merge + allow(service).to receive(:execute_hooks) + end - source_branch_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha - target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha + it "does not create merge commit" do + execute_ff_merge - expect(source_branch_sha).to eq(target_branch_sha) - end + source_branch_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha + target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha - it 'keeps the merge request valid' do - expect { execute_ff_merge } - .not_to change { merge_request.valid? } - end + expect(source_branch_sha).to eq(target_branch_sha) + end - it 'updates the merge request to merged' do - expect { execute_ff_merge } - .to change { merge_request.merged? } - .from(false) - .to(true) - end + it 'keeps the merge request valid' do + expect { execute_ff_merge } + .not_to change { merge_request.valid? } + end - it 'sends email to user2 about merge of new merge_request' do - execute_ff_merge + it 'updates the merge request to merged' do + expect { execute_ff_merge } + .to change { merge_request.merged? } + .from(false) + .to(true) + end - email = ActionMailer::Base.deliveries.last - expect(email.to.first).to eq(user2.email) - expect(email.subject).to include(merge_request.title) - end + it 'sends email to user2 about merge of new merge_request' do + execute_ff_merge - it 'creates system note about merge_request merge' do - execute_ff_merge + email = ActionMailer::Base.deliveries.last + expect(email.to.first).to eq(user2.email) + expect(email.subject).to include(merge_request.title) + end - note = merge_request.notes.last - expect(note.note).to include 'merged' - end + it 'creates system note about merge_request merge' do + execute_ff_merge - it 'does not update squash_commit_sha if it is not a squash' do - expect { execute_ff_merge }.not_to change { merge_request.squash_commit_sha } - end + if state_tracking_enabled + event = merge_request.resource_state_events.last + expect(event.state).to eq('merged') + else + note = merge_request.notes.last + expect(note.note).to include 'merged' + end + end - it 'updates squash_commit_sha if it is a squash' do - merge_request.update!(squash: true) + it 'does not update squash_commit_sha if it is not a squash' do + expect { execute_ff_merge }.not_to change { merge_request.squash_commit_sha } + end - expect { execute_ff_merge } - .to change { merge_request.squash_commit_sha } - .from(nil) + it 'updates squash_commit_sha if it is a squash' do + merge_request.update!(squash: true) + + expect { execute_ff_merge } + .to change { merge_request.squash_commit_sha } + .from(nil) + end end end @@ -87,7 +96,7 @@ describe MergeRequests::FfMergeService do let(:service) { described_class.new(project, user, valid_merge_params.merge(commit_message: 'Awesome message')) } before do - allow(Rails.logger).to receive(:error) + allow(Gitlab::AppLogger).to receive(:error) end it 'logs and saves error if there is an exception' do @@ -99,7 +108,7 @@ describe MergeRequests::FfMergeService do service.execute(merge_request) expect(merge_request.merge_error).to include(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end it 'logs and saves error if there is an PreReceiveError exception' do @@ -111,7 +120,7 @@ describe MergeRequests::FfMergeService do service.execute(merge_request) expect(merge_request.merge_error).to include(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end it 'does not update squash_commit_sha if squash merge is not successful' do diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index bcad822b1dc..2274d917527 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -20,7 +20,11 @@ describe MergeRequests::MergeService do end context 'valid params' do + let(:state_tracking) { true } + before do + stub_feature_flags(track_resource_state_change_events: state_tracking) + allow(service).to receive(:execute_hooks) perform_enqueued_jobs do @@ -42,9 +46,22 @@ describe MergeRequests::MergeService do expect(email.subject).to include(merge_request.title) end - it 'creates system note about merge_request merge' do - note = merge_request.notes.last - expect(note.note).to include 'merged' + context 'note creation' do + context 'when resource state event tracking is disabled' do + let(:state_tracking) { false } + + it 'creates system note about merge_request merge' do + note = merge_request.notes.last + expect(note.note).to include 'merged' + end + end + + context 'when resource state event tracking is enabled' do + it 'creates resource state event about merge_request merge' do + event = merge_request.resource_state_events.last + expect(event.state).to eq('merged') + end + end end context 'when squashing' do @@ -55,7 +72,7 @@ describe MergeRequests::MergeService do end let(:merge_request) do - # A merge reqeust with 5 commits + # A merge request with 5 commits create(:merge_request, :simple, author: user2, assignees: [user2], @@ -277,7 +294,7 @@ describe MergeRequests::MergeService do context 'error handling' do before do - allow(Rails.logger).to receive(:error) + allow(Gitlab::AppLogger).to receive(:error) end context 'when source is missing' do @@ -289,7 +306,7 @@ describe MergeRequests::MergeService do service.execute(merge_request) expect(merge_request.merge_error).to eq(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end end @@ -302,7 +319,7 @@ describe MergeRequests::MergeService do service.execute(merge_request) expect(merge_request.merge_error).to include('Something went wrong during merge') - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end it 'logs and saves error if user is not authorized' do @@ -326,7 +343,7 @@ describe MergeRequests::MergeService do service.execute(merge_request) expect(merge_request.merge_error).to include('Something went wrong during merge pre-receive hook') - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end it 'logs and saves error if there is a merge conflict' do @@ -340,7 +357,7 @@ describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil expect(merge_request.merge_error).to include(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end context 'when squashing' do @@ -359,7 +376,7 @@ describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil expect(merge_request.merge_error).to include(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end it 'logs and saves error if there is a squash in progress' do @@ -373,7 +390,7 @@ describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil expect(merge_request.merge_error).to include(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end context 'when fast-forward merge is not allowed' do @@ -393,7 +410,7 @@ describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil expect(merge_request.merge_error).to include(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end end end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 94e65d895ac..e60ff6eb98a 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -362,76 +362,101 @@ describe MergeRequests::RefreshService do end end - context 'push to origin repo target branch', :sidekiq_might_not_need_inline do - context 'when all MRs to the target branch had diffs' do + [true, false].each do |state_tracking_enabled| + context "push to origin repo target branch with state tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}", :sidekiq_might_not_need_inline do before do - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') - reload_mrs + stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) end - it 'updates the merge state' do - expect(@merge_request.notes.last.note).to include('merged') - expect(@merge_request).to be_merged - expect(@fork_merge_request).to be_merged - expect(@fork_merge_request.notes.last.note).to include('merged') - expect(@build_failed_todo).to be_done - expect(@fork_build_failed_todo).to be_done + context 'when all MRs to the target branch had diffs' do + before do + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs + end + + it 'updates the merge state' do + expect(@merge_request).to be_merged + expect(@fork_merge_request).to be_merged + expect(@build_failed_todo).to be_done + expect(@fork_build_failed_todo).to be_done + + if state_tracking_enabled + expect(@merge_request.resource_state_events.last.state).to eq('merged') + expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') + else + expect(@merge_request.notes.last.note).to include('merged') + expect(@fork_merge_request.notes.last.note).to include('merged') + end + end end - end - context 'when an MR to be closed was empty already' do - let!(:empty_fork_merge_request) do - create(:merge_request, - source_project: @fork_project, - source_branch: 'master', - target_branch: 'master', - target_project: @project) + context 'when an MR to be closed was empty already' do + let!(:empty_fork_merge_request) do + create(:merge_request, + source_project: @fork_project, + source_branch: 'master', + target_branch: 'master', + target_project: @project) + end + + before do + # This spec already has a fake push, so pretend that we were targeting + # feature all along. + empty_fork_merge_request.update_columns(target_branch: 'feature') + + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs + empty_fork_merge_request.reload + end + + it 'only updates the non-empty MRs' do + expect(@merge_request).to be_merged + expect(@fork_merge_request).to be_merged + + expect(empty_fork_merge_request).to be_open + expect(empty_fork_merge_request.merge_request_diff.state).to eq('empty') + expect(empty_fork_merge_request.notes).to be_empty + + if state_tracking_enabled + expect(@merge_request.resource_state_events.last.state).to eq('merged') + expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') + else + expect(@merge_request.notes.last.note).to include('merged') + expect(@fork_merge_request.notes.last.note).to include('merged') + end + end end + end + context "manual merge of source branch #{state_tracking_enabled ? 'enabled' : 'disabled'}", :sidekiq_might_not_need_inline do before do - # This spec already has a fake push, so pretend that we were targeting - # feature all along. - empty_fork_merge_request.update_columns(target_branch: 'feature') + stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + # Merge master -> feature branch + @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, 'Test message') + commit = @project.repository.commit('feature') + service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature') reload_mrs - empty_fork_merge_request.reload end - it 'only updates the non-empty MRs' do - expect(@merge_request).to be_merged - expect(@merge_request.notes.last.note).to include('merged') + it 'updates the merge state' do + if state_tracking_enabled + expect(@merge_request.resource_state_events.last.state).to eq('merged') + expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') + else + expect(@merge_request.notes.last.note).to include('merged') + expect(@fork_merge_request.notes.last.note).to include('merged') + end + expect(@merge_request).to be_merged + expect(@merge_request.diffs.size).to be > 0 expect(@fork_merge_request).to be_merged - expect(@fork_merge_request.notes.last.note).to include('merged') - - expect(empty_fork_merge_request).to be_open - expect(empty_fork_merge_request.merge_request_diff.state).to eq('empty') - expect(empty_fork_merge_request.notes).to be_empty + expect(@build_failed_todo).to be_done + expect(@fork_build_failed_todo).to be_done end end end - context 'manual merge of source branch', :sidekiq_might_not_need_inline do - before do - # Merge master -> feature branch - @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, 'Test message') - commit = @project.repository.commit('feature') - service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature') - reload_mrs - end - - it 'updates the merge state' do - expect(@merge_request.notes.last.note).to include('merged') - expect(@merge_request).to be_merged - expect(@merge_request.diffs.size).to be > 0 - expect(@fork_merge_request).to be_merged - expect(@fork_merge_request.notes.last.note).to include('merged') - expect(@build_failed_todo).to be_done - expect(@fork_build_failed_todo).to be_done - end - end - context 'push to fork repo source branch', :sidekiq_might_not_need_inline do let(:refresh_service) { service.new(@fork_project, @user) } @@ -583,20 +608,29 @@ describe MergeRequests::RefreshService do end end - context 'push to origin repo target branch after fork project was removed' do - before do - @fork_project.destroy - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') - reload_mrs - end + [true, false].each do |state_tracking_enabled| + context "push to origin repo target branch after fork project was removed #{state_tracking_enabled ? 'enabled' : 'disabled'}" do + before do + stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - it 'updates the merge request state' do - expect(@merge_request.notes.last.note).to include('merged') - expect(@merge_request).to be_merged - expect(@fork_merge_request).to be_open - expect(@fork_merge_request.notes).to be_empty - expect(@build_failed_todo).to be_done - expect(@fork_build_failed_todo).to be_done + @fork_project.destroy + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs + end + + it 'updates the merge request state' do + if state_tracking_enabled + expect(@merge_request.resource_state_events.last.state).to eq('merged') + else + expect(@merge_request.notes.last.note).to include('merged') + end + + expect(@merge_request).to be_merged + expect(@fork_merge_request).to be_open + expect(@fork_merge_request.notes).to be_empty + expect(@build_failed_todo).to be_done + expect(@fork_build_failed_todo).to be_done + end end end diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb index 25ab79d70c3..3807c44b01f 100644 --- a/spec/services/merge_requests/reopen_service_spec.rb +++ b/spec/services/merge_requests/reopen_service_spec.rb @@ -20,8 +20,11 @@ describe MergeRequests::ReopenService do context 'valid params' do let(:service) { described_class.new(project, user, {}) } + let(:state_tracking) { true } before do + stub_feature_flags(track_resource_state_change_events: state_tracking) + allow(service).to receive(:execute_hooks) perform_enqueued_jobs do @@ -43,9 +46,22 @@ describe MergeRequests::ReopenService do expect(email.subject).to include(merge_request.title) end - it 'creates system note about merge_request reopen' do - note = merge_request.notes.last - expect(note.note).to include 'reopened' + context 'note creation' do + context 'when state event tracking is disabled' do + let(:state_tracking) { false } + + it 'creates system note about merge_request reopen' do + note = merge_request.notes.last + expect(note.note).to include 'reopened' + end + end + + context 'when state event tracking is enabled' do + it 'creates resource state event about merge_request reopen' do + event = merge_request.resource_state_events.last + expect(event.state).to eq('reopened') + end + end end end diff --git a/spec/services/namespaces/check_storage_size_service_spec.rb b/spec/services/namespaces/check_storage_size_service_spec.rb index 50359ef90ab..e192f897cf9 100644 --- a/spec/services/namespaces/check_storage_size_service_spec.rb +++ b/spec/services/namespaces/check_storage_size_service_spec.rb @@ -44,7 +44,7 @@ describe Namespaces::CheckStorageSizeService, '#execute' do end it 'errors when feature flag is activated for the current namespace' do - stub_feature_flags(namespace_storage_limit: namespace ) + stub_feature_flags(namespace_storage_limit: namespace) expect(response).to be_error expect(response.message).to be_present @@ -156,4 +156,10 @@ describe Namespaces::CheckStorageSizeService, '#execute' do expect(response).to include("60%") end end + + describe 'payload root_namespace' do + subject(:response) { service.execute.payload[:root_namespace] } + + it { is_expected.to eq(namespace) } + end end diff --git a/spec/services/notification_recipients/build_service_spec.rb b/spec/services/notification_recipients/build_service_spec.rb index 2e848c2f04d..e203093623d 100644 --- a/spec/services/notification_recipients/build_service_spec.rb +++ b/spec/services/notification_recipients/build_service_spec.rb @@ -58,4 +58,56 @@ describe NotificationRecipients::BuildService do end end end + + describe '#build_new_review_recipients' do + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:review) { create(:review, merge_request: merge_request, project: project, author: merge_request.author) } + let(:notes) { create_list(:note_on_merge_request, 3, review: review, noteable: review.merge_request, project: review.project) } + + shared_examples 'no N+1 queries' do + it 'avoids N+1 queries', :request_store do + create_user + + service.build_new_review_recipients(review) + + control_count = ActiveRecord::QueryRecorder.new do + service.build_new_review_recipients(review) + end + + create_user + + expect { service.build_new_review_recipients(review) }.not_to exceed_query_limit(control_count) + end + end + + context 'when there are multiple watchers' do + def create_user + watcher = create(:user) + create(:notification_setting, source: project, user: watcher, level: :watch) + + other_projects.each do |other_project| + create(:notification_setting, source: other_project, user: watcher, level: :watch) + end + end + + include_examples 'no N+1 queries' + end + + context 'when there are multiple subscribers' do + def create_user + subscriber = create(:user) + merge_request.subscriptions.create(user: subscriber, project: project, subscribed: true) + end + + include_examples 'no N+1 queries' + + context 'when the project is private' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + include_examples 'no N+1 queries' + end + end + end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 46c80a86639..3c1c3e2dfc3 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -243,11 +243,12 @@ describe NotificationService, :mailer do describe '#unknown_sign_in' do let_it_be(:user) { create(:user) } let_it_be(:ip) { '127.0.0.1' } + let_it_be(:time) { Time.current } - subject { notification.unknown_sign_in(user, ip) } + subject { notification.unknown_sign_in(user, ip, time) } it 'sends email to the user' do - expect { subject }.to have_enqueued_email(user, ip, mail: 'unknown_sign_in_email') + expect { subject }.to have_enqueued_email(user, ip, time, mail: 'unknown_sign_in_email') end end @@ -2867,6 +2868,57 @@ describe NotificationService, :mailer do end end + describe '#new_review' do + let(:project) { create(:project, :repository) } + let(:user) { create(:user) } + let(:user2) { create(:user) } + let(:reviewer) { create(:user) } + let(:merge_request) { create(:merge_request, source_project: project, assignees: [user, user2], author: create(:user)) } + let(:review) { create(:review, merge_request: merge_request, project: project, author: reviewer) } + let(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, author: reviewer, review: review) } + + before do + build_team(review.project) + add_users(review.project) + add_user_subscriptions(merge_request) + project.add_maintainer(merge_request.author) + project.add_maintainer(reviewer) + merge_request.assignees.each { |assignee| project.add_maintainer(assignee) } + + create(:diff_note_on_merge_request, + project: project, + noteable: merge_request, + author: reviewer, + review: review, + note: "cc @mention") + end + + it 'sends emails' do + expect(Notify).not_to receive(:new_review_email).with(review.author.id, review.id) + expect(Notify).not_to receive(:new_review_email).with(@unsubscriber.id, review.id) + merge_request.assignee_ids.each do |assignee_id| + expect(Notify).to receive(:new_review_email).with(assignee_id, review.id).and_call_original + end + expect(Notify).to receive(:new_review_email).with(merge_request.author.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@u_watcher.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@u_mentioned.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@subscriber.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@watcher_and_subscriber.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@subscribed_participant.id, review.id).and_call_original + + subject.new_review(review) + end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { review } + let(:notification_trigger) { subject.new_review(review) } + + around do |example| + perform_enqueued_jobs { example.run } + end + end + end + def build_team(project) @u_watcher = create_global_setting_for(create(:user), :watch) @u_participating = create_global_setting_for(create(:user), :participating) diff --git a/spec/services/projects/alerting/notify_service_spec.rb b/spec/services/projects/alerting/notify_service_spec.rb index b88f0ef5149..2f8c2049f85 100644 --- a/spec/services/projects/alerting/notify_service_spec.rb +++ b/spec/services/projects/alerting/notify_service_spec.rb @@ -8,21 +8,21 @@ describe Projects::Alerting::NotifyService do before do # We use `let_it_be(:project)` so we make sure to clear caches project.clear_memoization(:licensed_feature_available) + allow(ProjectServiceWorker).to receive(:perform_async) end - shared_examples 'processes incident issues' do |amount| + shared_examples 'processes incident issues' do let(:create_incident_service) { spy } - let(:new_alert) { instance_double(AlertManagement::Alert, id: 503, persisted?: true) } - it 'processes issues' do - expect(AlertManagement::Alert) - .to receive(:create) - .and_return(new_alert) + before do + allow_any_instance_of(AlertManagement::Alert).to receive(:execute_services) + end + it 'processes issues' do expect(IncidentManagement::ProcessAlertWorker) .to receive(:perform_async) - .with(project.id, kind_of(Hash), new_alert.id) - .exactly(amount).times + .with(project.id, kind_of(Hash), kind_of(Integer)) + .once Sidekiq::Testing.inline! do expect(subject).to be_success @@ -73,6 +73,7 @@ describe Projects::Alerting::NotifyService do describe '#execute' do let(:token) { 'invalid-token' } let(:starts_at) { Time.current.change(usec: 0) } + let(:fingerprint) { 'testing' } let(:service) { described_class.new(project, nil, payload) } let(:payload_raw) do { @@ -82,7 +83,8 @@ describe Projects::Alerting::NotifyService do monitoring_tool: 'GitLab RSpec', service: 'GitLab Test Suite', description: 'Very detailed description', - hosts: ['1.1.1.1', '2.2.2.2'] + hosts: ['1.1.1.1', '2.2.2.2'], + fingerprint: fingerprint }.with_indifferent_access end let(:payload) { ActionController::Parameters.new(payload_raw).permit! } @@ -131,11 +133,37 @@ describe Projects::Alerting::NotifyService do description: payload_raw.fetch(:description), monitoring_tool: payload_raw.fetch(:monitoring_tool), service: payload_raw.fetch(:service), - fingerprint: nil, + fingerprint: Digest::SHA1.hexdigest(fingerprint), ended_at: nil ) end + it 'executes the alert service hooks' do + slack_service = create(:service, type: 'SlackService', project: project, alert_events: true, active: true) + subject + + expect(ProjectServiceWorker).to have_received(:perform_async).with(slack_service.id, an_instance_of(Hash)) + end + + context 'existing alert with same fingerprint' do + let(:fingerprint_sha) { Digest::SHA1.hexdigest(fingerprint) } + let!(:existing_alert) { create(:alert_management_alert, project: project, fingerprint: fingerprint_sha) } + + it 'does not create AlertManagement::Alert' do + expect { subject }.not_to change(AlertManagement::Alert, :count) + end + + it 'increments the existing alert count' do + expect { subject }.to change { existing_alert.reload.events }.from(1).to(2) + end + + it 'does not executes the alert service hooks' do + subject + + expect(ProjectServiceWorker).not_to have_received(:perform_async) + end + end + context 'with a minimal payload' do let(:payload_raw) do { @@ -176,7 +204,7 @@ describe Projects::Alerting::NotifyService do context 'issue enabled' do let(:issue_enabled) { true } - it_behaves_like 'processes incident issues', 1 + it_behaves_like 'processes incident issues' context 'with an invalid payload' do before do @@ -188,6 +216,21 @@ describe Projects::Alerting::NotifyService do it_behaves_like 'does not process incident issues due to error', http_status: :bad_request it_behaves_like 'NotifyService does not create alert' end + + context 'when alert already exists' do + let(:fingerprint_sha) { Digest::SHA1.hexdigest(fingerprint) } + let!(:existing_alert) { create(:alert_management_alert, project: project, fingerprint: fingerprint_sha) } + + context 'when existing alert does not have an associated issue' do + it_behaves_like 'processes incident issues' + end + + context 'when existing alert has an associated issue' do + let!(:existing_alert) { create(:alert_management_alert, :with_issue, project: project, fingerprint: fingerprint_sha) } + + it_behaves_like 'does not process incident issues' + end + end end context 'with emails turned on' do diff --git a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb index 01f09f208fd..11ea7d51673 100644 --- a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' describe Projects::ContainerRepository::CleanupTagsService do let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project, :private) } + let_it_be(:project, reload: true) { create(:project, :private) } let_it_be(:repository) { create(:container_repository, :root, project: project) } let(:service) { described_class.new(project, user, params) } @@ -72,6 +72,47 @@ describe Projects::ContainerRepository::CleanupTagsService do end end + context 'with invalid regular expressions' do + RSpec.shared_examples 'handling an invalid regex' do + it 'keeps all tags' do + expect(Projects::ContainerRepository::DeleteTagsService) + .not_to receive(:new) + subject + end + + it 'returns an error' do + response = subject + + expect(response[:status]).to eq(:error) + expect(response[:message]).to eq('invalid regex') + end + + it 'calls error tracking service' do + expect(Gitlab::ErrorTracking).to receive(:log_exception).and_call_original + + subject + end + end + + context 'when name_regex_delete is invalid' do + let(:params) { { 'name_regex_delete' => '*test*' } } + + it_behaves_like 'handling an invalid regex' + end + + context 'when name_regex is invalid' do + let(:params) { { 'name_regex' => '*test*' } } + + it_behaves_like 'handling an invalid regex' + end + + context 'when name_regex_keep is invalid' do + let(:params) { { 'name_regex_keep' => '*test*' } } + + it_behaves_like 'handling an invalid regex' + end + end + context 'when delete regex matching specific tags is used' do let(:params) do { 'name_regex_delete' => 'C|D' } diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index e542f1e9108..e70ee05ed31 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -43,10 +43,10 @@ describe Projects::CreateService, '#execute' do create_project(user, opts) end - it 'creates associated project settings' do + it 'builds associated project settings' do project = create_project(user, opts) - expect(project.project_setting).to be_persisted + expect(project.project_setting).to be_new_record end end @@ -88,6 +88,116 @@ describe Projects::CreateService, '#execute' do end end + context 'group sharing', :sidekiq_inline do + let_it_be(:group) { create(:group) } + let_it_be(:shared_group) { create(:group) } + let_it_be(:shared_group_user) { create(:user) } + let(:opts) do + { + name: 'GitLab', + namespace_id: shared_group.id + } + end + + before do + create(:group_group_link, shared_group: shared_group, shared_with_group: group) + + shared_group.add_maintainer(shared_group_user) + group.add_developer(user) + end + + it 'updates authorization' do + shared_group_project = create_project(shared_group_user, opts) + + expect( + Ability.allowed?(shared_group_user, :read_project, shared_group_project) + ).to be_truthy + expect( + Ability.allowed?(user, :read_project, shared_group_project) + ).to be_truthy + end + end + + context 'membership overrides', :sidekiq_inline do + let_it_be(:group) { create(:group, :private) } + let_it_be(:subgroup_for_projects) { create(:group, :private, parent: group) } + let_it_be(:subgroup_for_access) { create(:group, :private, parent: group) } + let_it_be(:group_maintainer) { create(:user) } + let(:group_access_level) { Gitlab::Access::REPORTER } + let(:subgroup_access_level) { Gitlab::Access::DEVELOPER } + let(:share_max_access_level) { Gitlab::Access::MAINTAINER } + let(:opts) do + { + name: 'GitLab', + namespace_id: subgroup_for_projects.id + } + end + + before do + group.add_maintainer(group_maintainer) + + create(:group_group_link, shared_group: subgroup_for_projects, + shared_with_group: subgroup_for_access, + group_access: share_max_access_level) + end + + context 'membership is higher from group hierarchy' do + let(:group_access_level) { Gitlab::Access::MAINTAINER } + + it 'updates authorization' do + create(:group_member, access_level: subgroup_access_level, group: subgroup_for_access, user: user) + create(:group_member, access_level: group_access_level, group: group, user: user) + + subgroup_project = create_project(group_maintainer, opts) + + project_authorization = ProjectAuthorization.where( + project_id: subgroup_project.id, + user_id: user.id, + access_level: group_access_level) + + expect(project_authorization).to exist + end + end + + context 'membership is higher from group share' do + let(:subgroup_access_level) { Gitlab::Access::MAINTAINER } + + context 'share max access level is not limiting' do + it 'updates authorization' do + create(:group_member, access_level: group_access_level, group: group, user: user) + create(:group_member, access_level: subgroup_access_level, group: subgroup_for_access, user: user) + + subgroup_project = create_project(group_maintainer, opts) + + project_authorization = ProjectAuthorization.where( + project_id: subgroup_project.id, + user_id: user.id, + access_level: subgroup_access_level) + + expect(project_authorization).to exist + end + end + + context 'share max access level is limiting' do + let(:share_max_access_level) { Gitlab::Access::DEVELOPER } + + it 'updates authorization' do + create(:group_member, access_level: group_access_level, group: group, user: user) + create(:group_member, access_level: subgroup_access_level, group: subgroup_for_access, user: user) + + subgroup_project = create_project(group_maintainer, opts) + + project_authorization = ProjectAuthorization.where( + project_id: subgroup_project.id, + user_id: user.id, + access_level: share_max_access_level) + + expect(project_authorization).to exist + end + end + end + end + context 'error handling' do it 'handles invalid options' do opts[:default_branch] = 'master' @@ -339,29 +449,39 @@ describe Projects::CreateService, '#execute' do end end - context 'when there is an active service template' do - before do - create(:prometheus_service, project: nil, template: true, active: true) - end + describe 'create service for the project' do + subject(:project) { create_project(user, opts) } - it 'creates a service from this template' do - project = create_project(user, opts) + context 'when there is an active instance-level and an active template integration' do + let!(:template_integration) { create(:prometheus_service, :template, api_url: 'https://prometheus.template.com/') } + let!(:instance_integration) { create(:prometheus_service, :instance, api_url: 'https://prometheus.instance.com/') } - expect(project.services.count).to eq 1 - expect(project.errors).to be_empty + it 'creates a service from the instance-level integration' do + expect(project.services.count).to eq(1) + expect(project.services.first.api_url).to eq(instance_integration.api_url) + expect(project.services.first.inherit_from_id).to eq(instance_integration.id) + end end - end - context 'when a bad service template is created' do - it 'sets service to be inactive' do - opts[:import_url] = 'http://www.gitlab.com/gitlab-org/gitlab-foss' - create(:service, type: 'DroneCiService', project: nil, template: true, active: true) + context 'when there is an active service template' do + let!(:template_integration) { create(:prometheus_service, :template, api_url: 'https://prometheus.template.com/') } - project = create_project(user, opts) - service = project.services.first + it 'creates a service from the template' do + expect(project.services.count).to eq(1) + expect(project.services.first.api_url).to eq(template_integration.api_url) + expect(project.services.first.inherit_from_id).to be_nil + end + end - expect(project).to be_persisted - expect(service.active).to be false + context 'when there is an invalid integration' do + before do + create(:service, :template, type: 'DroneCiService', active: true) + end + + it 'creates an inactive service' do + expect(project).to be_persisted + expect(project.services.first.active).to be false + end end end @@ -547,7 +667,9 @@ describe Projects::CreateService, '#execute' do ) expect(AuthorizedProjectUpdate::UserRefreshWithLowUrgencyWorker).to( receive(:bulk_perform_in) - .with(1.hour, array_including([user.id], [other_user.id])) + .with(1.hour, + array_including([user.id], [other_user.id]), + batch_delay: 30.seconds, batch_size: 100) .and_call_original ) diff --git a/spec/services/projects/group_links/create_service_spec.rb b/spec/services/projects/group_links/create_service_spec.rb index 92667184be8..22f7c8bdcb4 100644 --- a/spec/services/projects/group_links/create_service_spec.rb +++ b/spec/services/projects/group_links/create_service_spec.rb @@ -3,16 +3,17 @@ require 'spec_helper' describe Projects::GroupLinks::CreateService, '#execute' do - let(:user) { create :user } - let(:group) { create :group } - let(:project) { create :project } + let_it_be(:user) { create :user } + let_it_be(:group) { create :group } + let_it_be(:project) { create :project } let(:opts) do { link_group_access: '30', expires_at: nil } end - let(:subject) { described_class.new(project, user, opts) } + + subject { described_class.new(project, user, opts) } before do group.add_developer(user) @@ -22,6 +23,12 @@ describe Projects::GroupLinks::CreateService, '#execute' do expect { subject.execute(group) }.to change { project.project_group_links.count }.from(0).to(1) end + it 'updates authorization' do + expect { subject.execute(group) }.to( + change { Ability.allowed?(user, :read_project, project) } + .from(false).to(true)) + end + it 'returns false if group is blank' do expect { subject.execute(nil) }.not_to change { project.project_group_links.count } end diff --git a/spec/services/projects/group_links/destroy_service_spec.rb b/spec/services/projects/group_links/destroy_service_spec.rb index 0fd1fcfe1a5..0a8c9580e70 100644 --- a/spec/services/projects/group_links/destroy_service_spec.rb +++ b/spec/services/projects/group_links/destroy_service_spec.rb @@ -3,15 +3,25 @@ require 'spec_helper' describe Projects::GroupLinks::DestroyService, '#execute' do - let(:project) { create(:project, :private) } - let!(:group_link) { create(:project_group_link, project: project) } - let(:user) { create :user } - let(:subject) { described_class.new(project, user) } + let_it_be(:user) { create :user } + let_it_be(:project) { create(:project, :private) } + let_it_be(:group) { create(:group) } + let!(:group_link) { create(:project_group_link, project: project, group: group) } + + subject { described_class.new(project, user) } it 'removes group from project' do expect { subject.execute(group_link) }.to change { project.project_group_links.count }.from(1).to(0) end + it 'updates authorization' do + group.add_maintainer(user) + + expect { subject.execute(group_link) }.to( + change { Ability.allowed?(user, :read_project, project) } + .from(true).to(false)) + end + it 'returns false if group_link is blank' do expect { subject.execute(nil) }.not_to change { project.project_group_links.count } end diff --git a/spec/services/projects/group_links/update_service_spec.rb b/spec/services/projects/group_links/update_service_spec.rb new file mode 100644 index 00000000000..5be2ae1e0f7 --- /dev/null +++ b/spec/services/projects/group_links/update_service_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Projects::GroupLinks::UpdateService, '#execute' do + let_it_be(:user) { create :user } + let_it_be(:group) { create :group } + let_it_be(:project) { create :project } + let!(:link) { create(:project_group_link, project: project, group: group) } + + let(:expiry_date) { 1.month.from_now.to_date } + let(:group_link_params) do + { group_access: Gitlab::Access::GUEST, + expires_at: expiry_date } + end + + subject { described_class.new(link).execute(group_link_params) } + + before do + group.add_developer(user) + end + + it 'updates existing link' do + expect(link.group_access).to eq(Gitlab::Access::DEVELOPER) + expect(link.expires_at).to be_nil + + subject + + link.reload + + expect(link.group_access).to eq(Gitlab::Access::GUEST) + expect(link.expires_at).to eq(expiry_date) + end + + it 'updates project permissions' do + expect { subject }.to change { user.can?(:create_release, project) }.from(true).to(false) + end + + it 'executes UserProjectAccessChangedService' do + expect_next_instance_of(UserProjectAccessChangedService) do |service| + expect(service).to receive(:execute) + end + + subject + end + + context 'with only param not requiring authorization refresh' do + let(:group_link_params) { { expires_at: Date.tomorrow } } + + it 'does not execute UserProjectAccessChangedService' do + expect(UserProjectAccessChangedService).not_to receive(:new) + + subject + end + end +end diff --git a/spec/services/projects/import_export/export_service_spec.rb b/spec/services/projects/import_export/export_service_spec.rb index 5f496cb1e56..19891341311 100644 --- a/spec/services/projects/import_export/export_service_spec.rb +++ b/spec/services/projects/import_export/export_service_spec.rb @@ -119,9 +119,7 @@ describe Projects::ImportExport::ExportService do end it 'notifies logger' do - allow(Rails.logger).to receive(:error) - - expect(Rails.logger).to receive(:error) + expect(service.instance_variable_get(:@logger)).to receive(:error) end end end @@ -149,7 +147,7 @@ describe Projects::ImportExport::ExportService do end it 'notifies logger' do - expect(Rails.logger).to receive(:error) + expect(service.instance_variable_get(:@logger)).to receive(:error) end it 'does not call the export strategy' do diff --git a/spec/services/projects/lsif_data_service_spec.rb b/spec/services/projects/lsif_data_service_spec.rb deleted file mode 100644 index 4866f848121..00000000000 --- a/spec/services/projects/lsif_data_service_spec.rb +++ /dev/null @@ -1,126 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Projects::LsifDataService do - let(:artifact) { create(:ci_job_artifact, :lsif) } - let(:project) { build_stubbed(:project) } - let(:path) { 'main.go' } - let(:commit_id) { Digest::SHA1.hexdigest(SecureRandom.hex) } - - let(:service) { described_class.new(artifact.file, project, commit_id) } - - describe '#execute' do - def highlighted_value(value) - [{ language: 'go', value: Gitlab::Highlight.highlight(nil, value, language: 'go') }] - end - - context 'fetched lsif file', :use_clean_rails_memory_store_caching do - it 'is cached' do - service.execute(path) - - cached_data = Rails.cache.fetch("project:#{project.id}:lsif:#{commit_id}") - - expect(cached_data.keys).to eq(%w[def_refs doc_ranges docs hover_refs ranges]) - end - end - - context 'for main.go' do - let(:path_prefix) { "/#{project.full_path}/-/blob/#{commit_id}" } - - it 'returns lsif ranges for the file' do - expect(service.execute(path)).to eq([ - { - end_char: 9, - end_line: 6, - start_char: 5, - start_line: 6, - definition_url: "#{path_prefix}/main.go#L7", - hover: highlighted_value('func main()') - }, - { - end_char: 36, - end_line: 3, - start_char: 1, - start_line: 3, - definition_url: "#{path_prefix}/main.go#L4", - hover: highlighted_value('package "github.com/user/hello/morestrings" ("github.com/user/hello/morestrings")') - }, - { - end_char: 12, - end_line: 7, - start_char: 1, - start_line: 7, - definition_url: "#{path_prefix}/main.go#L4", - hover: highlighted_value('package "github.com/user/hello/morestrings" ("github.com/user/hello/morestrings")') - }, - { - end_char: 20, - end_line: 7, - start_char: 13, - start_line: 7, - definition_url: "#{path_prefix}/morestrings/reverse.go#L11", - hover: highlighted_value('func Reverse(s string) string') + [{ value: "This method reverses a string \n\n" }] - }, - { - end_char: 12, - end_line: 8, - start_char: 1, - start_line: 8, - definition_url: "#{path_prefix}/main.go#L4", - hover: highlighted_value('package "github.com/user/hello/morestrings" ("github.com/user/hello/morestrings")') - }, - { - end_char: 18, - end_line: 8, - start_char: 13, - start_line: 8, - definition_url: "#{path_prefix}/morestrings/reverse.go#L5", - hover: highlighted_value('func Func2(i int) string') - } - ]) - end - end - - context 'for morestring/reverse.go' do - let(:path) { 'morestrings/reverse.go' } - - it 'returns lsif ranges for the file' do - expect(service.execute(path).first).to eq({ - end_char: 2, - end_line: 11, - start_char: 1, - start_line: 11, - definition_url: "/#{project.full_path}/-/blob/#{commit_id}/morestrings/reverse.go#L12", - hover: highlighted_value('var a string') - }) - end - end - - context 'for an unknown file' do - let(:path) { 'unknown.go' } - - it 'returns nil' do - expect(service.execute(path)).to eq(nil) - end - end - end - - describe '#doc_id' do - context 'when the passed path matches multiple files' do - let(:path) { 'check/main.go' } - let(:docs) do - { - 1 => 'cmd/check/main.go', - 2 => 'cmd/command.go', - 3 => 'check/main.go', - 4 => 'cmd/nested/check/main.go' - } - end - - it 'fetches the document with the shortest absolute path' do - expect(service.__send__(:find_doc_id, docs, path)).to eq(3) - end - end - end -end diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb index 99a9fdd4184..f4d62b48fe5 100644 --- a/spec/services/projects/operations/update_service_spec.rb +++ b/spec/services/projects/operations/update_service_spec.rb @@ -96,7 +96,8 @@ describe Projects::Operations::UpdateService do let(:params) do { metrics_setting_attributes: { - external_dashboard_url: 'http://gitlab.com' + external_dashboard_url: 'http://gitlab.com', + dashboard_timezone: 'utc' } } end @@ -108,6 +109,7 @@ describe Projects::Operations::UpdateService do expect(project.reload.metrics_setting.external_dashboard_url).to eq( 'http://gitlab.com' ) + expect(project.metrics_setting.dashboard_timezone).to eq('utc') end end @@ -122,22 +124,25 @@ describe Projects::Operations::UpdateService do expect(project.reload.metrics_setting.external_dashboard_url).to eq( 'http://gitlab.com' ) + expect(project.metrics_setting.dashboard_timezone).to eq('utc') end + end - context 'with blank external_dashboard_url in params' do - let(:params) do - { - metrics_setting_attributes: { - external_dashboard_url: '' - } + context 'with blank external_dashboard_url' do + let(:params) do + { + metrics_setting_attributes: { + external_dashboard_url: '', + dashboard_timezone: 'utc' } - end + } + end - it 'destroys the metrics_setting entry in DB' do - expect(result[:status]).to eq(:success) + it 'updates dashboard_timezone' do + expect(result[:status]).to eq(:success) - expect(project.reload.metrics_setting).to be_nil - end + expect(project.reload.metrics_setting.external_dashboard_url).to be(nil) + expect(project.metrics_setting.dashboard_timezone).to eq('utc') end end end diff --git a/spec/services/projects/prometheus/alerts/create_events_service_spec.rb b/spec/services/projects/prometheus/alerts/create_events_service_spec.rb index 35f23afd7a2..61236b5bbdb 100644 --- a/spec/services/projects/prometheus/alerts/create_events_service_spec.rb +++ b/spec/services/projects/prometheus/alerts/create_events_service_spec.rb @@ -89,9 +89,9 @@ describe Projects::Prometheus::Alerts::CreateEventsService do context 'with a resolved payload' do let(:started_at) { truncate_to_second(Time.current) } let(:ended_at) { started_at + 1 } - let(:payload_key) { PrometheusAlertEvent.payload_key_for(alert.prometheus_metric_id, utc_rfc3339(started_at)) } let(:resolved_event) { alert_payload(status: 'resolved', started_at: started_at, ended_at: ended_at) } let(:alerts_payload) { { 'alerts' => [resolved_event] } } + let(:payload_key) { Gitlab::Alerting::Alert.new(project: project, payload: resolved_event).gitlab_fingerprint } context 'with a matching firing event' do before do diff --git a/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 009543f9016..95acedb1e76 100644 --- a/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe Projects::Prometheus::Alerts::NotifyService do + include PrometheusHelpers + let_it_be(:project, reload: true) { create(:project) } let(:service) { described_class.new(project, nil, payload) } @@ -92,9 +94,10 @@ describe Projects::Prometheus::Alerts::NotifyService do end context 'with valid payload' do - let(:alert_firing) { create(:prometheus_alert, project: project) } - let(:alert_resolved) { create(:prometheus_alert, project: project) } - let(:payload_raw) { payload_for(firing: [alert_firing], resolved: [alert_resolved]) } + let_it_be(:alert_firing) { create(:prometheus_alert, project: project) } + let_it_be(:alert_resolved) { create(:prometheus_alert, project: project) } + let_it_be(:cluster) { create(:cluster, :provided_by_user, projects: [project]) } + let(:payload_raw) { prometheus_alert_payload(firing: [alert_firing], resolved: [alert_resolved]) } let(:payload) { ActionController::Parameters.new(payload_raw).permit! } let(:payload_alert_firing) { payload_raw['alerts'].first } let(:token) { 'token' } @@ -116,9 +119,7 @@ describe Projects::Prometheus::Alerts::NotifyService do with_them do before do - cluster = create(:cluster, :provided_by_user, - projects: [project], - enabled: cluster_enabled) + cluster.update!(enabled: cluster_enabled) if status create(:clusters_applications_prometheus, status, @@ -179,6 +180,39 @@ describe Projects::Prometheus::Alerts::NotifyService do end end + context 'with generic alerts integration' do + using RSpec::Parameterized::TableSyntax + + where(:alerts_service, :token, :result) do + :active | :valid | :success + :active | :invalid | :failure + :active | nil | :failure + :inactive | :valid | :failure + nil | nil | :failure + end + + with_them do + let(:valid) { project.alerts_service.token } + let(:invalid) { 'invalid token' } + let(:token_input) { public_send(token) if token } + + before do + if alerts_service + create(:alerts_service, alerts_service, project: project) + end + end + + case result = params[:result] + when :success + it_behaves_like 'notifies alerts' + when :failure + it_behaves_like 'no notifications', http_status: :unauthorized + else + raise "invalid result: #{result.inspect}" + end + end + end + context 'alert emails' do before do create(:prometheus_service, project: project) @@ -227,7 +261,7 @@ describe Projects::Prometheus::Alerts::NotifyService do context 'with multiple firing alerts and resolving alerts' do let(:payload_raw) do - payload_for(firing: [alert_firing, alert_firing], resolved: [alert_resolved]) + prometheus_alert_payload(firing: [alert_firing, alert_firing], resolved: [alert_resolved]) end it 'processes Prometheus alerts' do @@ -258,7 +292,7 @@ describe Projects::Prometheus::Alerts::NotifyService do context 'multiple firing alerts' do let(:payload_raw) do - payload_for(firing: [alert_firing, alert_firing], resolved: []) + prometheus_alert_payload(firing: [alert_firing, alert_firing], resolved: []) end it_behaves_like 'processes incident issues', 2 @@ -266,7 +300,7 @@ describe Projects::Prometheus::Alerts::NotifyService do context 'without firing alerts' do let(:payload_raw) do - payload_for(firing: [], resolved: [alert_resolved]) + prometheus_alert_payload(firing: [], resolved: [alert_resolved]) end it_behaves_like 'processes incident issues', 1 @@ -284,24 +318,17 @@ describe Projects::Prometheus::Alerts::NotifyService do end context 'with invalid payload' do - context 'without version' do + context 'when payload is not processable' do let(:payload) { {} } - it_behaves_like 'no notifications', http_status: :unprocessable_entity - end - - context 'when version is not "4"' do - let(:payload) { { 'version' => '5' } } + before do + allow(described_class).to receive(:processable?).with(payload) + .and_return(false) + end it_behaves_like 'no notifications', http_status: :unprocessable_entity end - context 'with missing alerts' do - let(:payload) { { 'version' => '4' } } - - it_behaves_like 'no notifications', http_status: :unauthorized - end - context 'when the payload is too big' do let(:payload) { { 'the-payload-is-too-big' => true } } let(:deep_size_object) { instance_double(Gitlab::Utils::DeepSize, valid?: false) } @@ -328,50 +355,39 @@ describe Projects::Prometheus::Alerts::NotifyService do end end - private - - def payload_for(firing: [], resolved: []) - status = firing.any? ? 'firing' : 'resolved' - alerts = firing + resolved - alert_name = alerts.first.title - prometheus_metric_id = alerts.first.prometheus_metric_id.to_s - - alerts_map = \ - firing.map { |alert| map_alert_payload('firing', alert) } + - resolved.map { |alert| map_alert_payload('resolved', alert) } - - # See https://prometheus.io/docs/alerting/configuration/#%3Cwebhook_config%3E - { - 'version' => '4', - 'receiver' => 'gitlab', - 'status' => status, - 'alerts' => alerts_map, - 'groupLabels' => { - 'alertname' => alert_name - }, - 'commonLabels' => { - 'alertname' => alert_name, - 'gitlab' => 'hook', - 'gitlab_alert_id' => prometheus_metric_id - }, - 'commonAnnotations' => {}, - 'externalURL' => '', - 'groupKey' => "{}:{alertname=\'#{alert_name}\'}" - } - end + describe '.processable?' do + let(:valid_payload) { prometheus_alert_payload } + + subject { described_class.processable?(payload) } + + context 'with valid payload' do + let(:payload) { valid_payload } + + it { is_expected.to eq(true) } + + context 'containing unrelated keys' do + let(:payload) { valid_payload.merge('unrelated' => 'key') } - def map_alert_payload(status, alert) - { - 'status' => status, - 'labels' => { - 'alertname' => alert.title, - 'gitlab' => 'hook', - 'gitlab_alert_id' => alert.prometheus_metric_id.to_s - }, - 'annotations' => {}, - 'startsAt' => '2018-09-24T08:57:31.095725221Z', - 'endsAt' => '0001-01-01T00:00:00Z', - 'generatorURL' => 'http://prometheus-prometheus-server-URL' - } + it { is_expected.to eq(true) } + end + end + + context 'with invalid payload' do + where(:missing_key) do + described_class::REQUIRED_PAYLOAD_KEYS.to_a + end + + with_them do + let(:payload) { valid_payload.except(missing_key) } + + it { is_expected.to eq(false) } + end + end + + context 'with unsupported version' do + let(:payload) { valid_payload.merge('version' => '5') } + + it { is_expected.to eq(false) } + end end end diff --git a/spec/services/projects/propagate_service_template_spec.rb b/spec/services/projects/propagate_service_template_spec.rb index 7188ac5f733..ddc27c037f8 100644 --- a/spec/services/projects/propagate_service_template_spec.rb +++ b/spec/services/projects/propagate_service_template_spec.rb @@ -62,8 +62,8 @@ describe Projects::PropagateServiceTemplate do } ) - Service.build_from_template(project.id, service_template).save! - Service.build_from_template(project.id, other_service).save! + Service.build_from_integration(project.id, service_template).save! + Service.build_from_integration(project.id, other_service).save! expect { described_class.propagate(service_template) } .not_to change { Service.count } diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index f561a303be4..29c3c300d1b 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -158,6 +158,23 @@ describe Projects::UpdatePagesService do expect(project.pages_metadatum).not_to be_deployed end end + + context 'with background jobs running', :sidekiq_inline do + where(:ci_atomic_processing) do + [true, false] + end + + with_them do + before do + stub_feature_flags(ci_atomic_processing: ci_atomic_processing) + end + + it 'succeeds' do + expect(project.pages_deployed?).to be_falsey + expect(execute).to eq(:success) + end + end + end end end diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index 38c2dc0780e..418973fb0a6 100644 --- a/spec/services/projects/update_remote_mirror_service_spec.rb +++ b/spec/services/projects/update_remote_mirror_service_spec.rb @@ -10,6 +10,10 @@ describe Projects::UpdateRemoteMirrorService do subject(:service) { described_class.new(project, project.creator) } + before do + stub_feature_flags(gitaly_ruby_remote_branches_ls_remote: false) + end + describe '#execute' do subject(:execute!) { service.execute(remote_mirror, 0) } @@ -102,6 +106,19 @@ describe Projects::UpdateRemoteMirrorService do expect(remote_mirror.last_error).to include("refs/heads/develop") end end + + # https://gitlab.com/gitlab-org/gitaly/-/issues/2670 + context 'when `gitaly_ruby_remote_branches_ls_remote` is enabled' do + before do + stub_feature_flags(gitaly_ruby_remote_branches_ls_remote: true) + end + + it 'does not perform a fetch' do + expect(project.repository).not_to receive(:fetch_remote) + + execute! + end + end end def stub_fetch_remote(project, remote_name:, ssh_auth:) diff --git a/spec/services/projects/update_repository_storage_service_spec.rb b/spec/services/projects/update_repository_storage_service_spec.rb index 28b79bc61d9..e37580e7367 100644 --- a/spec/services/projects/update_repository_storage_service_spec.rb +++ b/spec/services/projects/update_repository_storage_service_spec.rb @@ -16,7 +16,7 @@ describe Projects::UpdateRepositoryStorageService do end context 'without wiki and design repository' do - let(:project) { create(:project, :repository, repository_read_only: true, wiki_enabled: false) } + let(:project) { create(:project, :repository, wiki_enabled: false) } let(:destination) { 'test_second_storage' } let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) } let!(:checksum) { project.repository.checksum } @@ -131,7 +131,7 @@ describe Projects::UpdateRepositoryStorageService do context 'with wiki repository' do include_examples 'moves repository to another storage', 'wiki' do - let(:project) { create(:project, :repository, repository_read_only: true, wiki_enabled: true) } + let(:project) { create(:project, :repository, wiki_enabled: true) } let(:repository) { project.wiki.repository } let(:destination) { 'test_second_storage' } let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) } @@ -144,7 +144,7 @@ describe Projects::UpdateRepositoryStorageService do context 'with design repository' do include_examples 'moves repository to another storage', 'design' do - let(:project) { create(:project, :repository, repository_read_only: true) } + let(:project) { create(:project, :repository) } let(:repository) { project.design_repository } let(:destination) { 'test_second_storage' } let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) } diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index ce9765a36ba..8a17884f641 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -254,7 +254,7 @@ describe Projects::UpdateService do it 'logs an error and creates a metric when wiki can not be created' do project.project_feature.update(wiki_access_level: ProjectFeature::DISABLED) - expect_any_instance_of(ProjectWiki).to receive(:wiki).and_raise(ProjectWiki::CouldNotCreateWikiError) + expect_any_instance_of(ProjectWiki).to receive(:wiki).and_raise(Wiki::CouldNotCreateWikiError) expect_any_instance_of(described_class).to receive(:log_error).with("Could not create wiki for #{project.full_name}") counter = double(:counter) @@ -552,6 +552,63 @@ describe Projects::UpdateService do end end end + + describe 'when changing repository_storage' do + let(:repository_read_only) { false } + let(:project) { create(:project, :repository, repository_read_only: repository_read_only) } + let(:opts) { { repository_storage: 'test_second_storage' } } + + before do + stub_storage_settings('test_second_storage' => { 'path' => 'tmp/tests/extra_storage' }) + end + + shared_examples 'the transfer was not scheduled' do + it 'does not schedule the transfer' do + expect do + update_project(project, user, opts) + end.not_to change(project.repository_storage_moves, :count) + end + end + + context 'authenticated as admin' do + let(:user) { create(:admin) } + + it 'schedules the transfer of the repository to the new storage and locks the project' do + update_project(project, admin, opts) + + expect(project).to be_repository_read_only + expect(project.repository_storage_moves.last).to have_attributes( + state: ::ProjectRepositoryStorageMove.state_machines[:state].states[:scheduled].value, + source_storage_name: 'default', + destination_storage_name: 'test_second_storage' + ) + end + + context 'the repository is read-only' do + let(:repository_read_only) { true } + + it_behaves_like 'the transfer was not scheduled' + end + + context 'the storage has not changed' do + let(:opts) { { repository_storage: 'default' } } + + it_behaves_like 'the transfer was not scheduled' + end + + context 'the storage does not exist' do + let(:opts) { { repository_storage: 'nonexistent' } } + + it_behaves_like 'the transfer was not scheduled' + end + end + + context 'authenticated as user' do + let(:user) { create(:user) } + + it_behaves_like 'the transfer was not scheduled' + end + end end describe '#run_auto_devops_pipeline?' do @@ -611,25 +668,6 @@ describe Projects::UpdateService do end end - describe 'repository_storage' do - let(:admin) { create(:admin) } - let(:user) { create(:user) } - let(:project) { create(:project, :repository) } - let(:opts) { { repository_storage: 'test_second_storage' } } - - it 'calls the change repository storage method if the storage changed' do - expect(project).to receive(:change_repository_storage).with('test_second_storage') - - update_project(project, admin, opts).inspect - end - - it "doesn't call the change repository storage for non-admin users" do - expect(project).not_to receive(:change_repository_storage) - - update_project(project, user, opts).inspect - end - end - def update_project(project, user, opts) described_class.new(project, user, opts).execute end diff --git a/spec/services/prometheus/create_default_alerts_service_spec.rb b/spec/services/prometheus/create_default_alerts_service_spec.rb index 3382844c99a..a28c38491de 100644 --- a/spec/services/prometheus/create_default_alerts_service_spec.rb +++ b/spec/services/prometheus/create_default_alerts_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Prometheus::CreateDefaultAlertsService do - let_it_be(:project) { create(:project) } + let_it_be(:project) { create(:project, :repository) } let(:instance) { described_class.new(project: project) } let(:expected_alerts) { described_class::DEFAULT_ALERTS } @@ -45,6 +45,23 @@ describe Prometheus::CreateDefaultAlertsService do .by(expected_alerts.size) end + it 'does not schedule an update to prometheus' do + expect(::Clusters::Applications::ScheduleUpdateService).not_to receive(:new) + execute + end + + context 'cluster with prometheus exists' do + let!(:cluster) { create(:cluster, :with_installed_prometheus, :provided_by_user, projects: [project]) } + + it 'schedules an update to prometheus' do + expect_next_instance_of(::Clusters::Applications::ScheduleUpdateService) do |instance| + expect(instance).to receive(:execute) + end + + execute + end + end + context 'multiple environments' do let!(:production) { create(:environment, project: project, name: 'production') } diff --git a/spec/services/prometheus/proxy_service_spec.rb b/spec/services/prometheus/proxy_service_spec.rb index 656ccea10de..bd451ff00a1 100644 --- a/spec/services/prometheus/proxy_service_spec.rb +++ b/spec/services/prometheus/proxy_service_spec.rb @@ -41,6 +41,27 @@ describe Prometheus::ProxyService do expect(result.params).to eq('query' => '1') end end + + context 'with series method' do + let(:params) do + ActionController::Parameters.new( + match: ['1'], + start: "2020-06-11T10:15:51Z", + end: "2020-06-11T11:16:06Z", + unknown_param: 'val' + ).permit! + end + + it 'allows match, start and end parameters' do + result = described_class.new(environment, 'GET', 'series', params) + + expect(result.params).to eq( + 'match' => ['1'], + 'start' => "2020-06-11T10:15:51Z", + 'end' => "2020-06-11T11:16:06Z" + ) + end + end end describe '#execute' do @@ -182,6 +203,24 @@ describe Prometheus::ProxyService do end end end + + context 'with series API' do + let(:rest_client_response) { instance_double(RestClient::Response, code: 200, body: '') } + + let(:params) do + ActionController::Parameters.new(match: ['1'], start: 1.hour.ago.rfc3339, end: Time.current.rfc3339).permit! + end + + subject { described_class.new(environment, 'GET', 'series', params) } + + it 'calls PrometheusClient with given parameters' do + expect(prometheus_client).to receive(:proxy) + .with('series', params.to_h) + .and_return(rest_client_response) + + subject.execute + end + end end end diff --git a/spec/services/prometheus/proxy_variable_substitution_service_spec.rb b/spec/services/prometheus/proxy_variable_substitution_service_spec.rb index 5982dcbc404..2435dda07b4 100644 --- a/spec/services/prometheus/proxy_variable_substitution_service_spec.rb +++ b/spec/services/prometheus/proxy_variable_substitution_service_spec.rb @@ -186,5 +186,19 @@ describe Prometheus::ProxyVariableSubstitutionService do end end end + + context '__range' do + let(:params_keys) do + { + query: 'topk(5, sum by (method) (rate(rest_client_requests_total[{{__range}}])))', + start_time: '2020-05-29T08:19:07.142Z', + end_time: '2020-05-29T16:19:07.142Z' + } + end + + it_behaves_like 'success' do + let(:expected_query) { "topk(5, sum by (method) (rate(rest_client_requests_total[#{8.hours.to_i}s])))" } + end + end end end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index a9de0a747f6..1bd402e38be 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -1621,6 +1621,29 @@ describe QuickActions::InterpretService do expect(message).to eq("Created branch '#{branch_name}' and a merge request to resolve this issue.") end end + + context 'submit_review command' do + using RSpec::Parameterized::TableSyntax + + where(:note) do + [ + 'I like it', + '/submit_review' + ] + end + + with_them do + let(:content) { '/submit_review' } + let!(:draft_note) { create(:draft_note, note: note, merge_request: merge_request, author: developer) } + + it 'submits the users current review' do + _, _, message = service.execute(content, merge_request) + + expect { draft_note.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect(message).to eq('Submitted the current review.') + end + end + end end describe '#explain' do diff --git a/spec/services/releases/create_evidence_service_spec.rb b/spec/services/releases/create_evidence_service_spec.rb new file mode 100644 index 00000000000..caa36a6b21d --- /dev/null +++ b/spec/services/releases/create_evidence_service_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Releases::CreateEvidenceService do + let_it_be(:project) { create(:project) } + let(:release) { create(:release, project: project) } + let(:service) { described_class.new(release) } + + it 'creates evidence' do + expect { service.execute }.to change { release.reload.evidences.count }.by(1) + end + + it 'saves evidence summary' do + service.execute + evidence = Releases::Evidence.last + + expect(release.tag).not_to be_nil + expect(evidence.summary["release"]["tag_name"]).to eq(release.tag) + end + + it 'saves sha' do + service.execute + evidence = Releases::Evidence.last + + expect(evidence.summary_sha).not_to be_nil + end +end diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb index d0859500440..4e3d9d5f108 100644 --- a/spec/services/releases/create_service_spec.rb +++ b/spec/services/releases/create_service_spec.rb @@ -186,4 +186,181 @@ describe Releases::CreateService do end end end + + context 'Evidence collection' do + let(:sha) { project.repository.commit('master').sha } + let(:params) do + { + name: 'New release', + ref: 'master', + tag: 'v0.1', + description: 'Super nice release', + released_at: released_at + }.compact + end + let(:last_release) { project.releases.last } + + around do |example| + Timecop.freeze { example.run } + end + + subject { service.execute } + + context 'historical release' do + let(:released_at) { 3.weeks.ago } + + it 'does not execute CreateEvidenceWorker' do + expect { subject }.not_to change(CreateEvidenceWorker.jobs, :size) + end + + it 'does not create an Evidence object', :sidekiq_inline do + expect { subject }.not_to change(Releases::Evidence, :count) + end + + it 'is a historical release' do + subject + + expect(last_release.historical_release?).to be_truthy + end + + it 'is not an upcoming release' do + subject + + expect(last_release.upcoming_release?).to be_falsy + end + end + + shared_examples 'uses the right pipeline for evidence' do + it 'creates evidence without pipeline if it does not exist', :sidekiq_inline do + expect_next_instance_of(Releases::CreateEvidenceService, anything, pipeline: nil) do |service| + expect(service).to receive(:execute).and_call_original + end + + expect { subject }.to change(Releases::Evidence, :count).by(1) + end + + it 'uses the last pipeline for evidence', :sidekiq_inline do + create(:ci_empty_pipeline, sha: sha, project: project) # old pipeline + pipeline = create(:ci_empty_pipeline, sha: sha, project: project) + + expect_next_instance_of(Releases::CreateEvidenceService, anything, pipeline: pipeline) do |service| + expect(service).to receive(:execute).and_call_original + end + + expect { subject }.to change(Releases::Evidence, :count).by(1) + end + + context 'when old evidence_pipeline is passed to service' do + let!(:old_pipeline) { create(:ci_empty_pipeline, sha: sha, project: project) } + let!(:new_pipeline) { create(:ci_empty_pipeline, sha: sha, project: project) } + let(:params) do + super().merge( + evidence_pipeline: old_pipeline + ) + end + + it 'uses the old pipeline for evidence', :sidekiq_inline do + expect_next_instance_of(Releases::CreateEvidenceService, anything, pipeline: old_pipeline) do |service| + expect(service).to receive(:execute).and_call_original + end + + expect { subject }.to change(Releases::Evidence, :count).by(1) + end + end + + it 'pipeline is still being used for evidence if new pipeline is being created for tag', :sidekiq_inline do + pipeline = create(:ci_empty_pipeline, sha: sha, project: project) + + expect(project.repository).to receive(:add_tag).and_wrap_original do |m, *args| + create(:ci_empty_pipeline, sha: sha, project: project) + m.call(*args) + end + + expect_next_instance_of(Releases::CreateEvidenceService, anything, pipeline: pipeline) do |service| + expect(service).to receive(:execute).and_call_original + end + + expect { subject }.to change(Releases::Evidence, :count).by(1) + end + + it 'uses the last pipeline for evidence when tag is already created', :sidekiq_inline do + Tags::CreateService.new(project, user).execute('v0.1', 'master', nil) + + expect(project.repository.find_tag('v0.1')).to be_present + + create(:ci_empty_pipeline, sha: sha, project: project) # old pipeline + pipeline = create(:ci_empty_pipeline, sha: sha, project: project) + + expect_next_instance_of(Releases::CreateEvidenceService, anything, pipeline: pipeline) do |service| + expect(service).to receive(:execute).and_call_original + end + + expect { subject }.to change(Releases::Evidence, :count).by(1) + end + end + + context 'immediate release' do + let(:released_at) { nil } + + it 'sets `released_at` to the current dttm' do + subject + + expect(last_release.updated_at).to be_like_time(Time.current) + end + + it 'queues CreateEvidenceWorker' do + expect { subject }.to change(CreateEvidenceWorker.jobs, :size).by(1) + end + + it 'creates Evidence', :sidekiq_inline do + expect { subject }.to change(Releases::Evidence, :count).by(1) + end + + it 'is not a historical release' do + subject + + expect(last_release.historical_release?).to be_falsy + end + + it 'is not an upcoming release' do + subject + + expect(last_release.upcoming_release?).to be_falsy + end + + include_examples 'uses the right pipeline for evidence' + end + + context 'upcoming release' do + let(:released_at) { 1.day.from_now } + + it 'queues CreateEvidenceWorker' do + expect { subject }.to change(CreateEvidenceWorker.jobs, :size).by(1) + end + + it 'queues CreateEvidenceWorker at the released_at timestamp' do + subject + + expect(CreateEvidenceWorker.jobs.last['at'].to_i).to eq(released_at.to_i) + end + + it 'creates Evidence', :sidekiq_inline do + expect { subject }.to change(Releases::Evidence, :count).by(1) + end + + it 'is not a historical release' do + subject + + expect(last_release.historical_release?).to be_falsy + end + + it 'is an upcoming release' do + subject + + expect(last_release.upcoming_release?).to be_truthy + end + + include_examples 'uses the right pipeline for evidence' + end + end end diff --git a/spec/services/resource_events/change_state_service_spec.rb b/spec/services/resource_events/change_state_service_spec.rb new file mode 100644 index 00000000000..e5d2a4ab11e --- /dev/null +++ b/spec/services/resource_events/change_state_service_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ResourceEvents::ChangeStateService do + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + + let(:issue) { create(:issue, project: project) } + let(:merge_request) { create(:merge_request, source_project: project) } + + describe '#execute' do + context 'when resource is an issue' do + %w[opened reopened closed locked].each do |state| + it "creates the expected event if issue has #{state} state" do + described_class.new(user: user, resource: issue).execute(state) + + event = issue.resource_state_events.last + expect(event.issue).to eq(issue) + expect(event.merge_request).to be_nil + expect(event.state).to eq(state) + end + end + end + + context 'when resource is a merge request' do + %w[opened reopened closed locked merged].each do |state| + it "creates the expected event if merge request has #{state} state" do + described_class.new(user: user, resource: merge_request).execute(state) + + event = merge_request.resource_state_events.last + expect(event.issue).to be_nil + expect(event.merge_request).to eq(merge_request) + expect(event.state).to eq(state) + end + end + end + end +end diff --git a/spec/services/service_response_spec.rb b/spec/services/service_response_spec.rb index a6567f52c6f..2c944a63ebb 100644 --- a/spec/services/service_response_spec.rb +++ b/spec/services/service_response_spec.rb @@ -84,4 +84,14 @@ describe ServiceResponse do expect(described_class.error(message: 'Bad apple').error?).to eq(true) end end + + describe '#errors' do + it 'returns an empty array for a successful response' do + expect(described_class.success.errors).to be_empty + end + + it 'returns an array with a correct message for an error response' do + expect(described_class.error(message: 'error message').errors).to eq(['error message']) + end + end end diff --git a/spec/services/snippets/bulk_destroy_service_spec.rb b/spec/services/snippets/bulk_destroy_service_spec.rb index f03d7496f94..6e5623e575f 100644 --- a/spec/services/snippets/bulk_destroy_service_spec.rb +++ b/spec/services/snippets/bulk_destroy_service_spec.rb @@ -69,6 +69,18 @@ describe Snippets::BulkDestroyService do it_behaves_like 'error is raised' do let(:error_message) { "You don't have access to delete these snippets." } end + + context 'when hard_delete option is passed' do + subject { described_class.new(service_user, snippets).execute(hard_delete: true) } + + it 'returns a ServiceResponse success response' do + expect(subject).to be_success + end + + it 'deletes all the snippets that belong to the user' do + expect { subject }.to change(Snippet, :count).by(-2) + end + end end context 'when an error is raised deleting the repository' do diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index 786fc3ec8dd..fa8cbc87563 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -109,7 +109,7 @@ describe Snippets::CreateService do expect(snippet.repository.exists?).to be_truthy end - it 'commit the files to the repository' do + it 'commits the files to the repository' do subject blob = snippet.repository.blob_at('master', base_opts[:file_name]) @@ -230,6 +230,61 @@ describe Snippets::CreateService do end end + shared_examples 'when snippet_files param is present' do + let(:file_path) { 'snippet_file_path.rb' } + let(:content) { 'snippet_content' } + let(:snippet_files) { [{ action: 'create', file_path: file_path, content: content }] } + let(:base_opts) do + { + title: 'Test snippet', + visibility_level: Gitlab::VisibilityLevel::PRIVATE, + snippet_files: snippet_files + } + end + + it 'creates a snippet with the provided attributes' do + expect(snippet.title).to eq(opts[:title]) + expect(snippet.visibility_level).to eq(opts[:visibility_level]) + expect(snippet.file_name).to eq(file_path) + expect(snippet.content).to eq(content) + end + + it 'commit the files to the repository' do + subject + + blob = snippet.repository.blob_at('master', file_path) + + expect(blob.data).to eq content + end + + context 'when content or file_name params are present' do + let(:extra_opts) { { content: 'foo', file_name: 'path' } } + + it 'a validation error is raised' do + response = subject + snippet = response.payload[:snippet] + + expect(response).to be_error + expect(snippet.errors.full_messages_for(:content)).to eq ['Content and snippet files cannot be used together'] + expect(snippet.errors.full_messages_for(:file_name)).to eq ['File name and snippet files cannot be used together'] + expect(snippet.repository.exists?).to be_falsey + end + end + + context 'when snippet_files param is invalid' do + let(:snippet_files) { [{ action: 'invalid_action', file_path: 'snippet_file_path.rb', content: 'snippet_content' }] } + + it 'a validation error is raised' do + response = subject + snippet = response.payload[:snippet] + + expect(response).to be_error + expect(snippet.errors.full_messages_for(:snippet_files)).to eq ['Snippet files have invalid data'] + expect(snippet.repository.exists?).to be_falsey + end + end + end + context 'when ProjectSnippet' do let_it_be(:project) { create(:project) } @@ -244,6 +299,7 @@ describe Snippets::CreateService do it_behaves_like 'an error service response when save fails' it_behaves_like 'creates repository and files' it_behaves_like 'after_save callback to store_mentions', ProjectSnippet + it_behaves_like 'when snippet_files param is present' context 'when uploaded files are passed to the service' do let(:extra_opts) { { files: ['foo'] } } @@ -270,6 +326,7 @@ describe Snippets::CreateService do it_behaves_like 'an error service response when save fails' it_behaves_like 'creates repository and files' it_behaves_like 'after_save callback to store_mentions', PersonalSnippet + it_behaves_like 'when snippet_files param is present' context 'when the snippet description contains files' do include FileMoverHelpers diff --git a/spec/services/snippets/update_service_spec.rb b/spec/services/snippets/update_service_spec.rb index 6c3ae52befc..7e6441ad2f9 100644 --- a/spec/services/snippets/update_service_spec.rb +++ b/spec/services/snippets/update_service_spec.rb @@ -302,6 +302,82 @@ describe Snippets::UpdateService do end end + shared_examples 'when snippet_files param is present' do + let(:file_path) { 'CHANGELOG' } + let(:content) { 'snippet_content' } + let(:new_title) { 'New title' } + let(:snippet_files) { [{ action: 'update', previous_path: file_path, file_path: file_path, content: content }] } + let(:base_opts) do + { + title: new_title, + snippet_files: snippet_files + } + end + + it 'updates a snippet with the provided attributes' do + file_path = 'foo' + snippet_files[0][:action] = 'move' + snippet_files[0][:file_path] = file_path + + response = subject + snippet = response.payload[:snippet] + + expect(response).to be_success + expect(snippet.title).to eq(new_title) + expect(snippet.file_name).to eq(file_path) + expect(snippet.content).to eq(content) + end + + it 'commit the files to the repository' do + subject + + blob = snippet.repository.blob_at('master', file_path) + + expect(blob.data).to eq content + end + + context 'when content or file_name params are present' do + let(:extra_opts) { { content: 'foo', file_name: 'path' } } + + it 'raises a validation error' do + response = subject + snippet = response.payload[:snippet] + + expect(response).to be_error + expect(snippet.errors.full_messages_for(:content)).to eq ['Content and snippet files cannot be used together'] + expect(snippet.errors.full_messages_for(:file_name)).to eq ['File name and snippet files cannot be used together'] + end + end + + context 'when snippet_files param is invalid' do + let(:snippet_files) { [{ action: 'invalid_action' }] } + + it 'raises a validation error' do + response = subject + snippet = response.payload[:snippet] + + expect(response).to be_error + expect(snippet.errors.full_messages_for(:snippet_files)).to eq ['Snippet files have invalid data'] + end + end + + context 'when an error is raised committing the file' do + it 'keeps any snippet modifications' do + expect_next_instance_of(described_class) do |instance| + expect(instance).to receive(:create_repository_for).and_raise(StandardError) + end + + response = subject + snippet = response.payload[:snippet] + + expect(response).to be_error + expect(snippet.title).to eq(new_title) + expect(snippet.file_name).to eq(file_path) + expect(snippet.content).to eq(content) + end + end + end + shared_examples 'only file_name is present' do let(:base_opts) do { @@ -370,6 +446,7 @@ describe Snippets::UpdateService do it_behaves_like 'updates repository content' it_behaves_like 'commit operation fails' it_behaves_like 'committable attributes' + it_behaves_like 'when snippet_files param is present' it_behaves_like 'only file_name is present' it_behaves_like 'only content is present' it_behaves_like 'snippets spam check is performed' do @@ -396,6 +473,7 @@ describe Snippets::UpdateService do it_behaves_like 'updates repository content' it_behaves_like 'commit operation fails' it_behaves_like 'committable attributes' + it_behaves_like 'when snippet_files param is present' it_behaves_like 'only file_name is present' it_behaves_like 'only content is present' it_behaves_like 'snippets spam check is performed' do diff --git a/spec/services/spam/akismet_service_spec.rb b/spec/services/spam/akismet_service_spec.rb index a496cd1890e..413b43d0156 100644 --- a/spec/services/spam/akismet_service_spec.rb +++ b/spec/services/spam/akismet_service_spec.rb @@ -45,9 +45,7 @@ describe Spam::AkismetService do end it 'logs an error' do - logger_spy = double(:logger) - expect(Rails).to receive(:logger).and_return(logger_spy) - expect(logger_spy).to receive(:error).with(/skipping/) + expect(Gitlab::AppLogger).to receive(:error).with(/skipping/) subject.send(method_call) end @@ -98,9 +96,7 @@ describe Spam::AkismetService do end it 'logs an error' do - logger_spy = double(:logger) - expect(Rails).to receive(:logger).and_return(logger_spy) - expect(logger_spy).to receive(:error).with(/skipping check/) + expect(Gitlab::AppLogger).to receive(:error).with(/skipping check/) subject.spam? end diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb index 560833aba97..7b6b65c82b1 100644 --- a/spec/services/spam/spam_action_service_spec.rb +++ b/spec/services/spam/spam_action_service_spec.rb @@ -24,7 +24,7 @@ describe Spam::SpamActionService do end describe '#initialize' do - subject { described_class.new(spammable: issue, request: request) } + subject { described_class.new(spammable: issue, request: request, user: user) } context 'when the request is nil' do let(:request) { nil } @@ -53,7 +53,7 @@ describe Spam::SpamActionService do shared_examples 'only checks for spam if a request is provided' do context 'when request is missing' do - subject { described_class.new(spammable: issue, request: nil) } + subject { described_class.new(spammable: issue, request: nil, user: user) } it "doesn't check as spam" do subject @@ -78,9 +78,9 @@ describe Spam::SpamActionService do let_it_be(:existing_spam_log) { create(:spam_log, user: user, recaptcha_verified: false) } subject do - described_service = described_class.new(spammable: issue, request: request) + described_service = described_class.new(spammable: issue, request: request, user: user) allow(described_service).to receive(:allowlisted?).and_return(allowlisted) - described_service.execute(user: user, api: nil, recaptcha_verified: recaptcha_verified, spam_log_id: existing_spam_log.id) + described_service.execute(api: nil, recaptcha_verified: recaptcha_verified, spam_log_id: existing_spam_log.id) end before do @@ -163,9 +163,9 @@ describe Spam::SpamActionService do end end - context 'when spam verdict service requires reCAPTCHA' do + context 'when spam verdict service conditionally allows' do before do - allow(fake_verdict_service).to receive(:execute).and_return(REQUIRE_RECAPTCHA) + allow(fake_verdict_service).to receive(:execute).and_return(CONDITIONAL_ALLOW) end context 'when allow_possible_spam feature flag is false' do diff --git a/spec/services/spam/spam_verdict_service_spec.rb b/spec/services/spam/spam_verdict_service_spec.rb index 93460a5e7d7..f6d9cd96da5 100644 --- a/spec/services/spam/spam_verdict_service_spec.rb +++ b/spec/services/spam/spam_verdict_service_spec.rb @@ -16,50 +16,266 @@ describe Spam::SpamVerdictService do let(:request) { double(:request, env: env) } let(:check_for_spam) { true } - let(:issue) { build(:issue) } + let_it_be(:user) { create(:user) } + let(:issue) { build(:issue, author: user) } let(:service) do - described_class.new(target: issue, request: request, options: {}) + described_class.new(user: user, target: issue, request: request, options: {}) end describe '#execute' do subject { service.execute } before do - allow_next_instance_of(Spam::AkismetService) do |service| - allow(service).to receive(:spam?).and_return(spam_verdict) + allow(service).to receive(:akismet_verdict).and_return(nil) + allow(service).to receive(:spam_verdict_verdict).and_return(nil) + end + + context 'if all services return nil' do + it 'renders ALLOW verdict' do + expect(subject).to eq ALLOW end end - context 'if Akismet considers it spam' do - let(:spam_verdict) { true } + context 'if only one service returns a verdict' do + context 'and it is supported' do + before do + allow(service).to receive(:akismet_verdict).and_return(DISALLOW) + end - context 'if reCAPTCHA is enabled' do + it 'renders that verdict' do + expect(subject).to eq DISALLOW + end + end + + context 'and it is unexpected' do before do - stub_application_setting(recaptcha_enabled: true) + allow(service).to receive(:akismet_verdict).and_return("unexpected") end - it 'requires reCAPTCHA' do - expect(subject).to eq REQUIRE_RECAPTCHA + it 'allows' do + expect(subject).to eq ALLOW end end + end - context 'if reCAPTCHA is not enabled' do + context 'if more than one service returns a verdict' do + context 'and they are supported' do before do - stub_application_setting(recaptcha_enabled: false) + allow(service).to receive(:akismet_verdict).and_return(DISALLOW) + allow(service).to receive(:spam_verdict).and_return(BLOCK_USER) end - it 'disallows the change' do - expect(subject).to eq DISALLOW + it 'renders the more restrictive verdict' do + expect(subject).to eq BLOCK_USER + end + end + + context 'and one is supported' do + before do + allow(service).to receive(:akismet_verdict).and_return('nonsense') + allow(service).to receive(:spam_verdict).and_return(BLOCK_USER) + end + + it 'renders the more restrictive verdict' do + expect(subject).to eq BLOCK_USER + end + end + + context 'and one is supported' do + before do + allow(service).to receive(:akismet_verdict).and_return('nonsense') + allow(service).to receive(:spam_verdict).and_return(BLOCK_USER) + end + + it 'renders the more restrictive verdict' do + expect(subject).to eq BLOCK_USER + end + end + + context 'and none are supported' do + before do + allow(service).to receive(:akismet_verdict).and_return('nonsense') + allow(service).to receive(:spam_verdict).and_return('rubbish') + end + + it 'renders the more restrictive verdict' do + expect(subject).to eq ALLOW + end + end + end + end + + describe '#akismet_verdict' do + subject { service.send(:akismet_verdict) } + + context 'if Akismet is enabled' do + before do + stub_application_setting(akismet_enabled: true) + allow_next_instance_of(Spam::AkismetService) do |service| + allow(service).to receive(:spam?).and_return(akismet_result) + end + end + + context 'if Akismet considers it spam' do + let(:akismet_result) { true } + + context 'if reCAPTCHA is enabled' do + before do + stub_application_setting(recaptcha_enabled: true) + end + + it 'returns conditionally allow verdict' do + expect(subject).to eq CONDITIONAL_ALLOW + end + end + + context 'if reCAPTCHA is not enabled' do + before do + stub_application_setting(recaptcha_enabled: false) + end + + it 'renders disallow verdict' do + expect(subject).to eq DISALLOW + end + end + end + + context 'if Akismet does not consider it spam' do + let(:akismet_result) { false } + + it 'renders allow verdict' do + expect(subject).to eq ALLOW end end end - context 'if Akismet does not consider it spam' do - let(:spam_verdict) { false } + context 'if Akismet is not enabled' do + before do + stub_application_setting(akismet_enabled: false) + end - it 'allows the change' do + it 'renders allow verdict' do expect(subject).to eq ALLOW end end end + + describe '#spam_verdict' do + subject { service.send(:spam_verdict) } + + context 'if a Spam Check endpoint enabled and set to a URL' do + let(:spam_check_body) { {} } + let(:spam_check_http_status) { nil } + + before do + stub_application_setting(spam_check_endpoint_enabled: true) + stub_application_setting(spam_check_endpoint_url: "http://www.spamcheckurl.com/spam_check") + stub_request(:post, /.*spamcheckurl.com.*/).to_return( body: spam_check_body.to_json, status: spam_check_http_status ) + end + + context 'if the endpoint is accessible' do + let(:spam_check_http_status) { 200 } + let(:error) { nil } + let(:verdict) { nil } + let(:spam_check_body) do + { verdict: verdict, error: error } + end + + context 'the result is a valid verdict' do + let(:verdict) { 'allow' } + + it 'returns the verdict' do + expect(subject).to eq ALLOW + end + end + + context 'the verdict is an unexpected string' do + let(:verdict) { 'this is fine' } + + it 'returns nil' do + expect(subject).to be_nil + end + end + + context 'the JSON is malformed' do + let(:spam_check_body) { 'this is fine' } + + it 'returns allow' do + expect(subject).to eq ALLOW + end + end + + context 'the verdict is an empty string' do + let(:verdict) { '' } + + it 'returns nil' do + expect(subject).to be_nil + end + end + + context 'the verdict is nil' do + let(:verdict) { nil } + + it 'returns nil' do + expect(subject).to be_nil + end + end + + context 'there is an error' do + let(:error) { "Sorry Dave, I can't do that" } + + it 'returns nil' do + expect(subject).to be_nil + end + end + + context 'the HTTP status is not 200' do + let(:spam_check_http_status) { 500 } + + it 'returns nil' do + expect(subject).to be_nil + end + end + + context 'the confused API endpoint returns both an error and a verdict' do + let(:verdict) { 'disallow' } + let(:error) { 'oh noes!' } + + it 'renders the verdict' do + expect(subject).to eq DISALLOW + end + end + end + + context 'if the endpoint times out' do + before do + stub_request(:post, /.*spamcheckurl.com.*/).to_timeout + end + + it 'returns nil' do + expect(subject).to be_nil + end + end + end + + context 'if a Spam Check endpoint is not set' do + before do + stub_application_setting(spam_check_endpoint_url: nil) + end + + it 'returns nil' do + expect(subject).to be_nil + end + end + + context 'if Spam Check endpoint is not enabled' do + before do + stub_application_setting(spam_check_endpoint_enabled: false) + end + + it 'returns nil' do + expect(subject).to be_nil + end + end + end end diff --git a/spec/services/submit_usage_ping_service_spec.rb b/spec/services/submit_usage_ping_service_spec.rb index 26ce5968ad6..981ea0dbec1 100644 --- a/spec/services/submit_usage_ping_service_spec.rb +++ b/spec/services/submit_usage_ping_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe SubmitUsagePingService do include StubRequests + include UsageDataHelpers let(:score_params) do { @@ -76,7 +77,7 @@ describe SubmitUsagePingService do context 'when usage ping is enabled' do before do - allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(false) + stub_usage_data_connections stub_application_setting(usage_ping_enabled: true) end diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb index b04c3278eaa..678e2129181 100644 --- a/spec/services/suggestions/apply_service_spec.rb +++ b/spec/services/suggestions/apply_service_spec.rb @@ -5,41 +5,66 @@ require 'spec_helper' describe Suggestions::ApplyService do include ProjectForksHelper - def build_position(args = {}) - default_args = { old_path: "files/ruby/popen.rb", - new_path: "files/ruby/popen.rb", - old_line: nil, - new_line: 9, - diff_refs: merge_request.diff_refs } - - Gitlab::Diff::Position.new(default_args.merge(args)) + def build_position(**optional_args) + args = { old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 9, + diff_refs: merge_request.diff_refs, + **optional_args } + + Gitlab::Diff::Position.new(args) end - shared_examples 'successfully creates commit and updates suggestion' do - def apply(suggestion) - result = subject.execute(suggestion) - expect(result[:status]).to eq(:success) - end + def create_suggestion(args) + position_args = args.slice(:old_path, :new_path, :old_line, :new_line) + content_args = args.slice(:from_content, :to_content) + + position = build_position(position_args) + + diff_note = create(:diff_note_on_merge_request, + noteable: merge_request, + position: position, + project: project) + + suggestion_args = { note: diff_note }.merge(content_args) + + create(:suggestion, :content_from_repo, suggestion_args) + end + + def apply(suggestions) + result = apply_service.new(user, *suggestions).execute - it 'updates the file with the new contents' do - apply(suggestion) + suggestions.map { |suggestion| suggestion.reload } - blob = project.repository.blob_at_branch(merge_request.source_branch, - position.new_path) + expect(result[:status]).to eq(:success) + end + + shared_examples 'successfully creates commit and updates suggestions' do + it 'updates the files with the new content' do + apply(suggestions) - expect(blob.data).to eq(expected_content) + suggestions.each do |suggestion| + path = suggestion.diff_file.file_path + blob = project.repository.blob_at_branch(merge_request.source_branch, + path) + + expect(blob.data).to eq(expected_content_by_path[path.to_sym]) + end end it 'updates suggestion applied and commit_id columns' do - expect { apply(suggestion) } - .to change(suggestion, :applied) - .from(false).to(true) - .and change(suggestion, :commit_id) - .from(nil) + expect(suggestions.map(&:applied)).to all(be false) + expect(suggestions.map(&:commit_id)).to all(be nil) + + apply(suggestions) + + expect(suggestions.map(&:applied)).to all(be true) + expect(suggestions.map(&:commit_id)).to all(be_present) end it 'created commit has users email and name' do - apply(suggestion) + apply(suggestions) commit = project.repository.commit @@ -53,125 +78,214 @@ describe Suggestions::ApplyService do before do project.update!(suggestion_commit_message: message) - apply(suggestion) + apply(suggestions) end context 'is not specified' do - let(:expected_value) { "Apply suggestion to files/ruby/popen.rb" } - - context 'is nil' do - let(:message) { nil } + let(:message) { '' } - it 'sets default commit message' do - expect(project.repository.commit.message).to eq(expected_value) - end - end - - context 'is an empty string' do - let(:message) { '' } - - it 'sets default commit message' do - expect(project.repository.commit.message).to eq(expected_value) - end + it 'uses the default commit message' do + expect(project.repository.commit.message).to( + match(/\AApply #{suggestions.size} suggestion\(s\) to \d+ file\(s\)\z/) + ) end end context 'is specified' do - let(:message) { 'refactor: %{project_path} %{project_name} %{file_path} %{branch_name} %{username} %{user_full_name}' } + let(:message) do + 'refactor: %{project_name} %{branch_name} %{username}' + end - it 'sets custom commit message' do - expect(project.repository.commit.message).to eq("refactor: project-1 Project_1 files/ruby/popen.rb master test.user Test User") + it 'generates a custom commit message' do + expect(project.repository.commit.message).to( + eq("refactor: Project_1 master test.user") + ) end end end end - let(:project) { create(:project, :repository, path: 'project-1', name: 'Project_1') } - let(:user) { create(:user, :commit_email, name: 'Test User', username: 'test.user') } + subject(:apply_service) { described_class } + + let_it_be(:user) do + create(:user, :commit_email, name: 'Test User', username: 'test.user') + end + + let(:project) do + create(:project, :repository, path: 'project-1', name: 'Project_1') + end + + let(:merge_request) do + create(:merge_request, source_project: project, + target_project: project, + source_branch: 'master') + end let(:position) { build_position } let(:diff_note) do - create(:diff_note_on_merge_request, noteable: merge_request, position: position, project: project) + create(:diff_note_on_merge_request, noteable: merge_request, + position: position, project: project) end let(:suggestion) do create(:suggestion, :content_from_repo, note: diff_note, - to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n") + to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n") end - subject { described_class.new(user) } + let(:suggestion2) do + create_suggestion( + to_content: " *** SUGGESTION CHANGE ***\n", + new_line: 15) + end + + let(:suggestion3) do + create_suggestion( + to_content: " *** ANOTHER SUGGESTION CHANGE ***\n", + old_path: "files/ruby/regex.rb", + new_path: "files/ruby/regex.rb", + new_line: 22) + end + + let(:suggestions) { [suggestion, suggestion2, suggestion3] } context 'patch is appliable' do - let(:expected_content) do + let(:popen_content) do <<-CONTENT.strip_heredoc - require 'fileutils' - require 'open3' + require 'fileutils' + require 'open3' + + module Popen + extend self + + def popen(cmd, path=nil) + unless cmd.is_a?(Array) + raise RuntimeError, 'Explosion' + # explosion? + end + + path ||= Dir.pwd + + vars = { + *** SUGGESTION CHANGE *** + } + + options = { + chdir: path + } + + unless File.directory?(path) + FileUtils.mkdir_p(path) + end + + @cmd_output = "" + @cmd_status = 0 + + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| + @cmd_output << stdout.read + @cmd_output << stderr.read + @cmd_status = wait_thr.value.exitstatus + end - module Popen + return @cmd_output, @cmd_status + end + end + CONTENT + end + + let(:regex_content) do + <<-CONTENT.strip_heredoc + module Gitlab + module Regex extend self - def popen(cmd, path=nil) - unless cmd.is_a?(Array) - raise RuntimeError, 'Explosion' - # explosion? - end + def username_regex + default_regex + end - path ||= Dir.pwd + def project_name_regex + /\\A[a-zA-Z0-9][a-zA-Z0-9_\\-\\. ]*\\z/ + end - vars = { - "PWD" => path - } + def name_regex + /\\A[a-zA-Z0-9_\\-\\. ]*\\z/ + end - options = { - chdir: path - } + def path_regex + default_regex + end - unless File.directory?(path) - FileUtils.mkdir_p(path) - end + def archive_formats_regex + *** ANOTHER SUGGESTION CHANGE *** + end - @cmd_output = "" - @cmd_status = 0 + def git_reference_regex + # Valid git ref regex, see: + # https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html + %r{ + (?! + (?# doesn't begins with) + \\/| (?# rule #6) + (?# doesn't contain) + .*(?: + [\\\/.]\\\.| (?# rule #1,3) + \\/\\/| (?# rule #6) + @\\{| (?# rule #8) + \\\\ (?# rule #9) + ) + ) + [^\\000-\\040\\177~^:?*\\[]+ (?# rule #4-5) + (?# doesn't end with) + (? path - } - - options = { - chdir: path - } - # v2 change - unless File.directory?(path) - FileUtils.mkdir_p(path) - end - - @cmd_output = "" - # v2 change - - Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| - @cmd_output << stdout.read - @cmd_output << stderr.read - @cmd_status = wait_thr.value.exitstatus - end - - return @cmd_output, @cmd_status - end - end - CONTENT - end - - def create_suggestion(diff, old_line: nil, new_line: nil, from_content:, to_content:, path:) - position = Gitlab::Diff::Position.new(old_path: path, - new_path: path, - old_line: old_line, - new_line: new_line, - diff_refs: diff.diff_refs) - - suggestion_note = create(:diff_note_on_merge_request, noteable: merge_request, - original_position: position, - position: position, - project: project) - create(:suggestion, note: suggestion_note, - from_content: from_content, - to_content: to_content) - end - + context 'multiple suggestions applied sequentially' do def apply_suggestion(suggestion) suggestion.reload merge_request.reload merge_request.clear_memoized_shas - result = subject.execute(suggestion) + result = apply_service.new(user, suggestion).execute + suggestion.reload expect(result[:status]).to eq(:success) refresh = MergeRequests::RefreshService.new(project, user) @@ -264,34 +320,31 @@ describe Suggestions::ApplyService do end def fetch_raw_diff(suggestion) - project.reload.commit(suggestion.commit_id).diffs.diff_files.first.diff.diff + project.reload.commit(suggestion.commit_id) + .diffs.diff_files.first.diff.diff end it 'applies multiple suggestions in subsequent versions correctly' do - diff = merge_request.merge_request_diff - path = 'files/ruby/popen.rb' + suggestion1 = create_suggestion( + from_content: "\n", + to_content: "# v1 change\n", + old_line: nil, + new_line: 13) - suggestion_1_changes = { old_line: nil, - new_line: 13, - from_content: "\n", - to_content: "# v1 change\n", - path: path } + suggestion2 = create_suggestion( + from_content: " @cmd_output << stderr.read\n", + to_content: "# v2 change\n", + old_line: 24, + new_line: 31) - suggestion_2_changes = { old_line: 24, - new_line: 31, - from_content: " @cmd_output << stderr.read\n", - to_content: "# v2 change\n", - path: path } + apply_suggestion(suggestion1) + apply_suggestion(suggestion2) - suggestion_1 = create_suggestion(diff, suggestion_1_changes) - suggestion_2 = create_suggestion(diff, suggestion_2_changes) - - apply_suggestion(suggestion_1) - - suggestion_1_diff = fetch_raw_diff(suggestion_1) + suggestion1_diff = fetch_raw_diff(suggestion1) + suggestion2_diff = fetch_raw_diff(suggestion2) # rubocop: disable Layout/TrailingWhitespace - expected_suggestion_1_diff = <<-CONTENT.strip_heredoc + expected_suggestion1_diff = <<-CONTENT.strip_heredoc @@ -10,7 +10,7 @@ module Popen end @@ -304,12 +357,8 @@ describe Suggestions::ApplyService do CONTENT # rubocop: enable Layout/TrailingWhitespace - apply_suggestion(suggestion_2) - - suggestion_2_diff = fetch_raw_diff(suggestion_2) - # rubocop: disable Layout/TrailingWhitespace - expected_suggestion_2_diff = <<-CONTENT.strip_heredoc + expected_suggestion2_diff = <<-CONTENT.strip_heredoc @@ -28,7 +28,7 @@ module Popen Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| @@ -321,14 +370,14 @@ describe Suggestions::ApplyService do CONTENT # rubocop: enable Layout/TrailingWhitespace - expect(suggestion_1_diff.strip).to eq(expected_suggestion_1_diff.strip) - expect(suggestion_2_diff.strip).to eq(expected_suggestion_2_diff.strip) + expect(suggestion1_diff.strip).to eq(expected_suggestion1_diff.strip) + expect(suggestion2_diff.strip).to eq(expected_suggestion2_diff.strip) end end context 'multi-line suggestion' do - let(:expected_content) do - <<~CONTENT + let(:popen_content) do + <<~CONTENT.strip_heredoc require 'fileutils' require 'open3' @@ -365,19 +414,27 @@ describe Suggestions::ApplyService do CONTENT end + let(:expected_content_by_path) do + { + "files/ruby/popen.rb": popen_content + } + end + let(:suggestion) do create(:suggestion, :content_from_repo, note: diff_note, - lines_above: 2, - lines_below: 3, - to_content: "# multi\n# line\n") + lines_above: 2, + lines_below: 3, + to_content: "# multi\n# line\n") end - it_behaves_like 'successfully creates commit and updates suggestion' + let(:suggestions) { [suggestion] } + + it_behaves_like 'successfully creates commit and updates suggestions' end context 'remove an empty line suggestion' do - let(:expected_content) do - <<~CONTENT + let(:popen_content) do + <<~CONTENT.strip_heredoc require 'fileutils' require 'open3' @@ -417,12 +474,19 @@ describe Suggestions::ApplyService do CONTENT end - let(:position) { build_position(new_line: 13) } + let(:expected_content_by_path) do + { + "files/ruby/popen.rb": popen_content + } + end + let(:suggestion) do - create(:suggestion, :content_from_repo, note: diff_note, to_content: "") + create_suggestion( to_content: "", new_line: 13) end - it_behaves_like 'successfully creates commit and updates suggestion' + let(:suggestions) { [suggestion] } + + it_behaves_like 'successfully creates commit and updates suggestions' end end @@ -430,17 +494,23 @@ describe Suggestions::ApplyService do let(:project) { create(:project, :public, :repository) } let(:forked_project) do - fork_project_with_submodules(project, user, repository: project.repository) + fork_project_with_submodules(project, + user, repository: project.repository) end let(:merge_request) do create(:merge_request, - source_branch: 'conflict-resolvable-fork', source_project: forked_project, - target_branch: 'conflict-start', target_project: project) + source_branch: 'conflict-resolvable-fork', + source_project: forked_project, + target_branch: 'conflict-start', + target_project: project) end let!(:diff_note) do - create(:diff_note_on_merge_request, noteable: merge_request, position: position, project: project) + create(:diff_note_on_merge_request, + noteable: merge_request, + position: position, + project: project) end before do @@ -448,11 +518,12 @@ describe Suggestions::ApplyService do end it 'updates file in the source project' do - expect(Files::UpdateService).to receive(:new) - .with(merge_request.source_project, user, anything) - .and_call_original + expect(Files::MultiService).to receive(:new) + .with(merge_request.source_project, + user, + anything).and_call_original - subject.execute(suggestion) + apply_service.new(user, suggestion).execute end end end @@ -460,13 +531,13 @@ describe Suggestions::ApplyService do context 'no permission' do let(:merge_request) do create(:merge_request, source_project: project, - target_project: project) + target_project: project) end let(:diff_note) do create(:diff_note_on_merge_request, noteable: merge_request, - position: position, - project: project) + position: position, + project: project) end context 'user cannot write in project repo' do @@ -475,7 +546,7 @@ describe Suggestions::ApplyService do end it 'returns error' do - result = subject.execute(suggestion) + result = apply_service.new(user, suggestion).execute expect(result).to eq(message: "You are not allowed to push into this branch", status: :error) @@ -486,13 +557,13 @@ describe Suggestions::ApplyService do context 'patch is not appliable' do let(:merge_request) do create(:merge_request, source_project: project, - target_project: project) + target_project: project) end let(:diff_note) do create(:diff_note_on_merge_request, noteable: merge_request, - position: position, - project: project) + position: position, + project: project) end before do @@ -503,29 +574,49 @@ describe Suggestions::ApplyService do it 'returns error message' do expect(suggestion.note).to receive(:latest_diff_file) { nil } - result = subject.execute(suggestion) + result = apply_service.new(user, suggestion).execute - expect(result).to eq(message: 'Suggestion is not appliable', + expect(result).to eq(message: 'A file was not found.', status: :error) end end - context 'suggestion is eligible to be outdated' do - it 'returns error message' do - expect(suggestion).to receive(:outdated?) { true } + context 'when not all suggestions belong to the same branch' do + it 'renders error message' do + merge_request2 = create(:merge_request, + :conflict, + source_project: project, + target_project: project) - result = subject.execute(suggestion) + position2 = Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 15, + diff_refs: merge_request2 + .diff_refs) - expect(result).to eq(message: 'Suggestion is not appliable', + diff_note2 = create(:diff_note_on_merge_request, + noteable: merge_request2, + position: position2, + project: project) + + other_branch_suggestion = create(:suggestion, note: diff_note2) + + result = apply_service.new(user, suggestion, other_branch_suggestion).execute + + expect(result).to eq(message: 'Suggestions must all be on the same branch.', status: :error) end end - context 'suggestion was already applied' do - it 'returns success status' do - result = subject.execute(suggestion) + context 'suggestion is eligible to be outdated' do + it 'returns error message' do + expect(suggestion).to receive(:outdated?) { true } + + result = apply_service.new(user, suggestion).execute - expect(result[:status]).to eq(:success) + expect(result).to eq(message: 'A suggestion is not applicable.', + status: :error) end end @@ -535,9 +626,9 @@ describe Suggestions::ApplyService do end it 'returns error message' do - result = subject.execute(suggestion) + result = apply_service.new(user, suggestion).execute - expect(result).to eq(message: 'Suggestion is not appliable', + expect(result).to eq(message: 'A suggestion is not applicable.', status: :error) end end @@ -548,9 +639,27 @@ describe Suggestions::ApplyService do end it 'returns error message' do - result = subject.execute(suggestion) + result = apply_service.new(user, suggestion).execute + + expect(result).to eq(message: 'A suggestion is not applicable.', + status: :error) + end + end + + context 'lines of suggestions overlap' do + let(:suggestion) do + create_suggestion( + to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n") + end + + let(:overlapping_suggestion) do + create_suggestion(to_content: "I Overlap!") + end + + it 'returns error message' do + result = apply_service.new(user, suggestion, overlapping_suggestion).execute - expect(result).to eq(message: 'Suggestion is not appliable', + expect(result).to eq(message: 'Suggestions are not applicable as their lines cannot overlap.', status: :error) end end diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index 477f9eae39e..c3b3c877583 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -157,7 +157,18 @@ describe ::SystemNotes::IssuablesService do describe '#change_status' do subject { service.change_status(status, source) } + context 'when resource state event tracking is enabled' do + let(:status) { 'reopened' } + let(:source) { nil } + + it { is_expected.to be_nil } + end + context 'with status reopened' do + before do + stub_feature_flags(track_resource_state_change_events: false) + end + let(:status) { 'reopened' } let(:source) { nil } @@ -169,6 +180,10 @@ describe ::SystemNotes::IssuablesService do end context 'with a source' do + before do + stub_feature_flags(track_resource_state_change_events: false) + end + let(:status) { 'opened' } let(:source) { double('commit', gfm_reference: 'commit 123456') } diff --git a/spec/services/test_hooks/project_service_spec.rb b/spec/services/test_hooks/project_service_spec.rb index 8d30f5018dd..3c5bc0d85f2 100644 --- a/spec/services/test_hooks/project_service_spec.rb +++ b/spec/services/test_hooks/project_service_spec.rb @@ -31,15 +31,7 @@ describe TestHooks::ProjectService do let(:trigger) { 'push_events' } let(:trigger_key) { :push_hooks } - it 'returns error message if not enough data' do - allow(project).to receive(:empty_repo?).and_return(true) - - expect(hook).not_to receive(:execute) - expect(service.execute).to include({ status: :error, message: 'Ensure the project has at least one commit.' }) - end - it 'executes hook' do - allow(project).to receive(:empty_repo?).and_return(false) allow(Gitlab::DataBuilder::Push).to receive(:build_sample).and_return(sample_data) expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) @@ -51,15 +43,7 @@ describe TestHooks::ProjectService do let(:trigger) { 'tag_push_events' } let(:trigger_key) { :tag_push_hooks } - it 'returns error message if not enough data' do - allow(project).to receive(:empty_repo?).and_return(true) - - expect(hook).not_to receive(:execute) - expect(service.execute).to include({ status: :error, message: 'Ensure the project has at least one commit.' }) - end - it 'executes hook' do - allow(project).to receive(:empty_repo?).and_return(false) allow(Gitlab::DataBuilder::Push).to receive(:build_sample).and_return(sample_data) expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) diff --git a/spec/services/test_hooks/system_service_spec.rb b/spec/services/test_hooks/system_service_spec.rb index 799b57eb04e..8a86b14a2a1 100644 --- a/spec/services/test_hooks/system_service_spec.rb +++ b/spec/services/test_hooks/system_service_spec.rb @@ -29,7 +29,6 @@ describe TestHooks::SystemService do let(:trigger_key) { :push_hooks } it 'executes hook' do - allow(project).to receive(:empty_repo?).and_return(false) expect(Gitlab::DataBuilder::Push).to receive(:sample_data).and_call_original expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Push::SAMPLE_DATA, trigger_key).and_return(success_result) @@ -55,7 +54,6 @@ describe TestHooks::SystemService do let(:trigger_key) { :repository_update_hooks } it 'executes hook' do - allow(project).to receive(:empty_repo?).and_return(false) expect(Gitlab::DataBuilder::Repository).to receive(:sample_data).and_call_original expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Repository::SAMPLE_DATA, trigger_key).and_return(success_result) diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 4894cf12372..f6e1608acbe 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -27,6 +27,39 @@ describe TodoService do project.add_developer(skipped) end + shared_examples 'reassigned target' do + it 'creates a pending todo for new assignee' do + target_unassigned.assignees = [john_doe] + service.send(described_method, target_unassigned, author) + + should_create_todo(user: john_doe, target: target_unassigned, action: Todo::ASSIGNED) + end + + it 'does not create a todo if unassigned' do + target_assigned.assignees = [] + + should_not_create_any_todo { service.send(described_method, target_assigned, author) } + end + + it 'creates a todo if new assignee is the current user' do + target_assigned.assignees = [john_doe] + service.send(described_method, target_assigned, john_doe) + + should_create_todo(user: john_doe, target: target_assigned, author: john_doe, action: Todo::ASSIGNED) + end + + it 'does not create a todo for guests' do + service.send(described_method, target_assigned, author) + should_not_create_todo(user: guest, target: target_assigned, action: Todo::MENTIONED) + end + + it 'does not create a directly addressed todo for guests' do + service.send(described_method, addressed_target_assigned, author) + + should_not_create_todo(user: guest, target: addressed_target_assigned, action: Todo::DIRECTLY_ADDRESSED) + end + end + describe 'Issues' do let(:issue) { create(:issue, project: project, assignees: [john_doe], author: author, description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } let(:addressed_issue) { create(:issue, project: project, assignees: [john_doe], author: author, description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } @@ -274,12 +307,12 @@ describe TodoService do end end - describe '#mark_pending_todos_as_done' do + describe '#resolve_todos_for_target' do it 'marks related pending todos to the target for the user as done' do first_todo = create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) second_todo = create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) - service.mark_pending_todos_as_done(issue, john_doe) + service.resolve_todos_for_target(issue, john_doe) expect(first_todo.reload).to be_done expect(second_todo.reload).to be_done @@ -293,7 +326,7 @@ describe TodoService do expect(john_doe.todos_pending_count).to eq(1) expect(john_doe).to receive(:update_todos_count_cache).and_call_original - service.mark_pending_todos_as_done(issue, john_doe) + service.resolve_todos_for_target(issue, john_doe) expect(john_doe.todos_done_count).to eq(1) expect(john_doe.todos_pending_count).to eq(0) @@ -301,59 +334,6 @@ describe TodoService do end end - shared_examples 'updating todos state' do |meth, state, new_state| - let!(:first_todo) { create(:todo, state, user: john_doe, project: project, target: issue, author: author) } - let!(:second_todo) { create(:todo, state, user: john_doe, project: project, target: issue, author: author) } - - it 'updates related todos for the user with the new_state' do - service.send(meth, collection, john_doe) - - expect(first_todo.reload.state?(new_state)).to be true - expect(second_todo.reload.state?(new_state)).to be true - end - - it 'returns the updated ids' do - expect(service.send(meth, collection, john_doe)).to match_array([first_todo.id, second_todo.id]) - end - - describe 'cached counts' do - it 'updates when todos change' do - expect(john_doe.todos.where(state: new_state).count).to eq(0) - expect(john_doe.todos.where(state: state).count).to eq(2) - expect(john_doe).to receive(:update_todos_count_cache).and_call_original - - service.send(meth, collection, john_doe) - - expect(john_doe.todos.where(state: new_state).count).to eq(2) - expect(john_doe.todos.where(state: state).count).to eq(0) - end - end - end - - describe '#mark_todos_as_done' do - it_behaves_like 'updating todos state', :mark_todos_as_done, :pending, :done do - let(:collection) { Todo.all } - end - end - - describe '#mark_todos_as_done_by_ids' do - it_behaves_like 'updating todos state', :mark_todos_as_done_by_ids, :pending, :done do - let(:collection) { [first_todo, second_todo].map(&:id) } - end - end - - describe '#mark_todos_as_pending' do - it_behaves_like 'updating todos state', :mark_todos_as_pending, :done, :pending do - let(:collection) { Todo.all } - end - end - - describe '#mark_todos_as_pending_by_ids' do - it_behaves_like 'updating todos state', :mark_todos_as_pending_by_ids, :done, :pending do - let(:collection) { [first_todo, second_todo].map(&:id) } - end - end - describe '#new_note' do let!(:first_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) } let!(:second_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) } @@ -575,51 +555,21 @@ describe TodoService do end describe '#reassigned_issuable' do - shared_examples 'reassigned issuable' do - it 'creates a pending todo for new assignee' do - issuable_unassigned.assignees = [john_doe] - service.reassigned_issuable(issuable_unassigned, author) - - should_create_todo(user: john_doe, target: issuable_unassigned, action: Todo::ASSIGNED) - end - - it 'does not create a todo if unassigned' do - issuable_assigned.assignees = [] - - should_not_create_any_todo { service.reassigned_issuable(issuable_assigned, author) } - end - - it 'creates a todo if new assignee is the current user' do - issuable_assigned.assignees = [john_doe] - service.reassigned_issuable(issuable_assigned, john_doe) - - should_create_todo(user: john_doe, target: issuable_assigned, author: john_doe, action: Todo::ASSIGNED) - end - - it 'does not create a todo for guests' do - service.reassigned_issuable(issuable_assigned, author) - should_not_create_todo(user: guest, target: issuable_assigned, action: Todo::MENTIONED) - end - - it 'does not create a directly addressed todo for guests' do - service.reassigned_issuable(addressed_issuable_assigned, author) - should_not_create_todo(user: guest, target: addressed_issuable_assigned, action: Todo::DIRECTLY_ADDRESSED) - end - end + let(:described_method) { :reassigned_issuable } context 'issuable is a merge request' do - it_behaves_like 'reassigned issuable' do - let(:issuable_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } - let(:addressed_issuable_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } - let(:issuable_unassigned) { create(:merge_request, source_project: project, author: author, assignees: []) } + it_behaves_like 'reassigned target' do + let(:target_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } + let(:addressed_target_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } + let(:target_unassigned) { create(:merge_request, source_project: project, author: author, assignees: []) } end end context 'issuable is an issue' do - it_behaves_like 'reassigned issuable' do - let(:issuable_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } - let(:addressed_issuable_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } - let(:issuable_unassigned) { create(:issue, project: project, author: author, assignees: []) } + it_behaves_like 'reassigned target' do + let(:target_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } + let(:addressed_target_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } + let(:target_unassigned) { create(:issue, project: project, author: author, assignees: []) } end end end @@ -809,6 +759,16 @@ describe TodoService do end end + describe '#assign_alert' do + let(:described_method) { :assign_alert } + + it_behaves_like 'reassigned target' do + let(:target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) } + let(:addressed_target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) } + let(:target_unassigned) { create(:alert_management_alert, project: project, assignees: []) } + end + end + describe '#merge_request_build_failed' do let(:merge_participants) { [mr_unassigned.author, admin] } @@ -1000,121 +960,111 @@ describe TodoService do expect(john_doe.todos_pending_count).to eq(1) end - describe '#mark_todos_as_done' do - let(:issue) { create(:issue, project: project, author: author, assignees: [john_doe]) } - let(:another_issue) { create(:issue, project: project, author: author, assignees: [john_doe]) } + shared_examples 'updating todos state' do |state, new_state, new_resolved_by = nil| + let!(:first_todo) { create(:todo, state, user: john_doe) } + let!(:second_todo) { create(:todo, state, user: john_doe) } + let(:collection) { Todo.all } - it 'marks a relation of todos as done' do - create(:todo, :mentioned, user: john_doe, target: issue, project: project) + it 'updates related todos for the user with the new_state' do + method_call - todos = TodosFinder.new(john_doe, {}).execute - expect { described_class.new.mark_todos_as_done(todos, john_doe) } - .to change { john_doe.todos.done.count }.from(0).to(1) + expect(collection.all? { |todo| todo.reload.state?(new_state)}).to be_truthy end - it 'marks an array of todos as done' do - todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project) + if new_resolved_by + it 'updates resolution mechanism' do + method_call - todos = TodosFinder.new(john_doe, {}).execute - expect { described_class.new.mark_todos_as_done(todos, john_doe) } - .to change { todo.reload.state }.from('pending').to('done') + expect(collection.all? { |todo| todo.reload.resolved_by_action == new_resolved_by }).to be_truthy + end end - it 'returns the ids of updated todos' do # Needed on API - todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project) - - todos = TodosFinder.new(john_doe, {}).execute - expect(described_class.new.mark_todos_as_done(todos, john_doe)).to eq([todo.id]) + it 'returns the updated ids' do + expect(method_call).to match_array([first_todo.id, second_todo.id]) end - context 'when some of the todos are done already' do - let!(:first_todo) { create(:todo, :mentioned, user: john_doe, target: issue, project: project) } - let!(:second_todo) { create(:todo, :mentioned, user: john_doe, target: another_issue, project: project) } - - it 'returns the ids of those still pending' do - described_class.new.mark_pending_todos_as_done(issue, john_doe) - - expect(described_class.new.mark_todos_as_done(Todo.all, john_doe)).to eq([second_todo.id]) - end + describe 'cached counts' do + it 'updates when todos change' do + expect(john_doe.todos.where(state: new_state).count).to eq(0) + expect(john_doe.todos.where(state: state).count).to eq(2) + expect(john_doe).to receive(:update_todos_count_cache).and_call_original - it 'returns an empty array if all are done' do - described_class.new.mark_pending_todos_as_done(issue, john_doe) - described_class.new.mark_pending_todos_as_done(another_issue, john_doe) + method_call - expect(described_class.new.mark_todos_as_done(Todo.all, john_doe)).to eq([]) + expect(john_doe.todos.where(state: new_state).count).to eq(2) + expect(john_doe.todos.where(state: state).count).to eq(0) end end end - describe '#mark_todo_as_done' do - it 'marks a todo done' do - todo1 = create(:todo, :pending, user: john_doe) - - described_class.new.mark_todo_as_done(todo1, john_doe) - - expect(todo1.reload.state).to eq('done') - end - - context 'when todo is already in state done' do - let(:todo1) { create(:todo, :done, user: john_doe) } - - it 'does not update the todo' do - expect { described_class.new.mark_todo_as_done(todo1, john_doe) }.not_to change(todo1.reload, :state) + describe '#resolve_todos' do + it_behaves_like 'updating todos state', :pending, :done, 'mark_done' do + subject(:method_call) do + service.resolve_todos(collection, john_doe, resolution: :done, resolved_by_action: :mark_done) end + end + end - it 'does not update cache count' do - expect(john_doe).not_to receive(:update_todos_count_cache) - - described_class.new.mark_todo_as_done(todo1, john_doe) + describe '#restore_todos' do + it_behaves_like 'updating todos state', :done, :pending do + subject(:method_call) do + service.restore_todos(collection, john_doe) end end end - describe '#mark_all_todos_as_done_by_user' do - it 'marks all todos done' do - todo1 = create(:todo, user: john_doe, state: :pending) - todo2 = create(:todo, user: john_doe, state: :done) - todo3 = create(:todo, user: john_doe, state: :pending) + describe '#resolve_todo' do + let!(:todo) { create(:todo, :assigned, user: john_doe) } - ids = described_class.new.mark_all_todos_as_done_by_user(john_doe) + it 'marks pending todo as done' do + expect do + service.resolve_todo(todo, john_doe) + todo.reload + end.to change { todo.done? }.to(true) + end - expect(ids).to contain_exactly(todo1.id, todo3.id) - expect(todo1.reload.state).to eq('done') - expect(todo2.reload.state).to eq('done') - expect(todo3.reload.state).to eq('done') + it 'saves resolution mechanism' do + expect do + service.resolve_todo(todo, john_doe, resolved_by_action: :mark_done) + todo.reload + end.to change { todo.resolved_by_mark_done? }.to(true) end - end - describe '#mark_todos_as_done_by_ids' do - let(:issue) { create(:issue, project: project, author: author, assignees: [john_doe]) } - let(:another_issue) { create(:issue, project: project, author: author, assignees: [john_doe]) } + context 'cached counts' do + it 'updates when todos change' do + expect(john_doe.todos_done_count).to eq(0) + expect(john_doe.todos_pending_count).to eq(1) + expect(john_doe).to receive(:update_todos_count_cache).and_call_original - it 'marks an array of todo ids as done' do - todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project) - another_todo = create(:todo, :mentioned, user: john_doe, target: another_issue, project: project) + service.resolve_todo(todo, john_doe) - expect { described_class.new.mark_todos_as_done_by_ids([todo.id, another_todo.id], john_doe) } - .to change { john_doe.todos.done.count }.from(0).to(2) + expect(john_doe.todos_done_count).to eq(1) + expect(john_doe.todos_pending_count).to eq(0) + end end + end - it 'marks a single todo id as done' do - todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project) + describe '#restore_todo' do + let!(:todo) { create(:todo, :done, user: john_doe) } - expect { described_class.new.mark_todos_as_done_by_ids(todo.id, john_doe) } - .to change { todo.reload.state }.from('pending').to('done') + it 'marks resolved todo as pending' do + expect do + service.restore_todo(todo, john_doe) + todo.reload + end.to change { todo.pending? }.to(true) end - it 'caches the number of todos of a user', :use_clean_rails_memory_store_caching do - create(:todo, :mentioned, user: john_doe, target: issue, project: project) - todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project) + context 'cached counts' do + it 'updates when todos change' do + expect(john_doe.todos_done_count).to eq(1) + expect(john_doe.todos_pending_count).to eq(0) + expect(john_doe).to receive(:update_todos_count_cache).and_call_original - described_class.new.mark_todos_as_done_by_ids(todo, john_doe) + service.restore_todo(todo, john_doe) - # Make sure no TodosFinder is inialized to perform counting - expect(TodosFinder).not_to receive(:new) - - expect(john_doe.todos_done_count).to eq(1) - expect(john_doe.todos_pending_count).to eq(1) + expect(john_doe.todos_done_count).to eq(0) + expect(john_doe.todos_pending_count).to eq(1) + end end end diff --git a/spec/services/user_project_access_changed_service_spec.rb b/spec/services/user_project_access_changed_service_spec.rb index f27eeb74265..e5ecdd123f7 100644 --- a/spec/services/user_project_access_changed_service_spec.rb +++ b/spec/services/user_project_access_changed_service_spec.rb @@ -20,7 +20,11 @@ describe UserProjectAccessChangedService do it 'permits low-priority operation' do expect(AuthorizedProjectUpdate::UserRefreshWithLowUrgencyWorker).to( - receive(:bulk_perform_in).with(described_class::DELAY, [[1], [2]]) + receive(:bulk_perform_in).with( + described_class::DELAY, + [[1], [2]], + { batch_delay: 30.seconds, batch_size: 100 } + ) ) described_class.new([1, 2]).execute(blocking: false, diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index 6e4b293286b..3db5e66fe05 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -67,6 +67,18 @@ describe Users::DestroyService do end end + it 'calls the bulk snippet destroy service with hard delete option if it is present' do + # this avoids getting into Projects::DestroyService as it would + # call Snippets::BulkDestroyService first! + allow(user).to receive(:personal_projects).and_return([]) + + expect_next_instance_of(Snippets::BulkDestroyService) do |bulk_destroy_service| + expect(bulk_destroy_service).to receive(:execute).with(hard_delete: true).and_call_original + end + + service.execute(user, hard_delete: true) + end + it 'does not delete project snippets that the user is the author of' do repo = create(:project_snippet, :repository, author: user).snippet_repository service.execute(user) diff --git a/spec/services/users/migrate_to_ghost_user_service_spec.rb b/spec/services/users/migrate_to_ghost_user_service_spec.rb index a7d7c16a66f..c2a793b2368 100644 --- a/spec/services/users/migrate_to_ghost_user_service_spec.rb +++ b/spec/services/users/migrate_to_ghost_user_service_spec.rb @@ -84,6 +84,15 @@ describe Users::MigrateToGhostUserService do end end + context 'reviews' do + let!(:user) { create(:user) } + let(:service) { described_class.new(user) } + + include_examples "migrating a deleted user's associated records to the ghost user", Review, [:author] do + let(:created_record) { create(:review, author: user) } + end + end + context "when record migration fails with a rollback exception" do before do expect_any_instance_of(ActiveRecord::Associations::CollectionProxy) diff --git a/spec/services/wiki_pages/event_create_service_spec.rb b/spec/services/wiki_pages/event_create_service_spec.rb index cf971b0a02c..c725c67d7a7 100644 --- a/spec/services/wiki_pages/event_create_service_spec.rb +++ b/spec/services/wiki_pages/event_create_service_spec.rb @@ -11,7 +11,7 @@ describe WikiPages::EventCreateService do describe '#execute' do let_it_be(:page) { create(:wiki_page, project: project) } let(:slug) { generate(:sluggified_title) } - let(:action) { Event::CREATED } + let(:action) { :created } let(:response) { subject.execute(slug, page, action) } context 'feature flag is not enabled' do @@ -38,7 +38,7 @@ describe WikiPages::EventCreateService do end context 'the action is illegal' do - let(:action) { Event::WIKI_ACTIONS.max + 1 } + let(:action) { :illegal_action } it 'returns an error' do expect(response).to be_error @@ -58,7 +58,7 @@ describe WikiPages::EventCreateService do end context 'the action is a deletion' do - let(:action) { Event::DESTROYED } + let(:action) { :destroyed } it 'does not synchronize the wiki metadata timestamps with the git commit' do expect_next_instance_of(WikiPage::Meta) do |instance| @@ -74,7 +74,7 @@ describe WikiPages::EventCreateService do end it 'returns an event in the payload' do - expect(response.payload).to include(event: have_attributes(author: user, wiki_page?: true, action: action)) + expect(response.payload).to include(event: have_attributes(author: user, wiki_page?: true, action: 'created')) end it 'records the slug for the page' do -- cgit v1.2.3