diff options
Diffstat (limited to 'spec/services')
100 files changed, 3372 insertions, 2038 deletions
diff --git a/spec/services/alert_management/alerts/update_service_spec.rb b/spec/services/alert_management/alerts/update_service_spec.rb index 882543fd701..f02607b8174 100644 --- a/spec/services/alert_management/alerts/update_service_spec.rb +++ b/spec/services/alert_management/alerts/update_service_spec.rb @@ -88,7 +88,7 @@ RSpec.describe AlertManagement::Alerts::UpdateService do it_behaves_like 'title update' end - context 'when alert is resolved and another existing open alert' do + context 'when alert is resolved and another existing unresolved alert' do let!(:alert) { create(:alert_management_alert, :resolved, project: project) } let!(:existing_alert) { create(:alert_management_alert, :triggered, project: project) } @@ -193,27 +193,38 @@ RSpec.describe AlertManagement::Alerts::UpdateService do end end - context 'with an opening status and existing open alert' do - let_it_be(:alert) { create(:alert_management_alert, :resolved, :with_fingerprint, project: project) } - let_it_be(:existing_alert) { create(:alert_management_alert, :triggered, fingerprint: alert.fingerprint, project: project) } - let_it_be(:url) { Gitlab::Routing.url_helpers.details_project_alert_management_path(project, existing_alert) } - let_it_be(:link) { ActionController::Base.helpers.link_to(_('alert'), url) } + context 'with existing unresolved alert' do + context 'with fingerprints' do + let_it_be(:existing_alert) { create(:alert_management_alert, :triggered, fingerprint: alert.fingerprint, project: project) } - let(:message) do - "An #{link} with the same fingerprint is already open. " \ - 'To change the status of this alert, resolve the linked alert.' - end + it 'does not query for existing alerts' do + expect(::AlertManagement::Alert).not_to receive(:find_unresolved_alert) - it_behaves_like 'does not add a todo' - it_behaves_like 'does not add a system note' + response + end - it 'has an informative message' do - expect(response).to be_error - expect(response.message).to eq(message) + context 'when status was resolved' do + let_it_be(:alert) { create(:alert_management_alert, :resolved, :with_fingerprint, project: project) } + let_it_be(:existing_alert) { create(:alert_management_alert, :triggered, fingerprint: alert.fingerprint, project: project) } + + let(:url) { Gitlab::Routing.url_helpers.details_project_alert_management_path(project, existing_alert) } + let(:link) { ActionController::Base.helpers.link_to(_('alert'), url) } + let(:message) do + "An #{link} with the same fingerprint is already open. " \ + 'To change the status of this alert, resolve the linked alert.' + end + + it_behaves_like 'does not add a todo' + it_behaves_like 'does not add a system note' + + it 'has an informative message' do + expect(response).to be_error + expect(response.message).to eq(message) + end + end end - context 'fingerprints are blank' do - let_it_be(:alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: nil) } + context 'without fingerprints' do let_it_be(:existing_alert) { create(:alert_management_alert, :triggered, fingerprint: alert.fingerprint, project: project) } it 'successfully changes the status' do diff --git a/spec/services/audit_event_service_spec.rb b/spec/services/audit_event_service_spec.rb index 6963515ba5c..063d250f22b 100644 --- a/spec/services/audit_event_service_spec.rb +++ b/spec/services/audit_event_service_spec.rb @@ -76,11 +76,13 @@ RSpec.describe AuditEventService do it 'creates an authentication event' do expect(AuthenticationEvent).to receive(:new).with( - user: user, - user_name: user.name, - ip_address: user.current_sign_in_ip, - result: AuthenticationEvent.results[:success], - provider: 'standard' + { + user: user, + user_name: user.name, + ip_address: user.current_sign_in_ip, + result: AuthenticationEvent.results[:success], + provider: 'standard' + } ).and_call_original audit_service.for_authentication.security_event diff --git a/spec/services/authorized_project_update/project_create_service_spec.rb b/spec/services/authorized_project_update/project_create_service_spec.rb deleted file mode 100644 index a9d0b82acfb..00000000000 --- a/spec/services/authorized_project_update/project_create_service_spec.rb +++ /dev/null @@ -1,185 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe AuthorizedProjectUpdate::ProjectCreateService do - let_it_be(:group_parent) { create(:group, :private) } - let_it_be(:group) { create(:group, :private, parent: group_parent) } - let_it_be(:group_child) { create(:group, :private, parent: group) } - - let_it_be(:group_project) { create(:project, group: group) } - - let_it_be(:parent_group_user) { create(:user) } - let_it_be(:group_user) { create(:user) } - let_it_be(:child_group_user) { create(:user) } - - let(:access_level) { Gitlab::Access::MAINTAINER } - - subject(:service) { described_class.new(group_project) } - - describe '#perform' do - context 'direct group members' do - before do - create(:group_member, access_level: access_level, 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: access_level) - - expect(project_authorization).to exist - end - end - - context 'inherited group members' do - before do - create(:group_member, access_level: access_level, group: group_parent, user: parent_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: parent_group_user.id, - access_level: access_level) - expect(project_authorization).to exist - end - end - - context 'membership overrides' do - 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 - - context 'group sharing' do - let!(:shared_with_group) { create(:group) } - - 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_member, :minimal_access, source: shared_with_group, user: create(: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 - - it 'does not create project authorization for user with minimal access' do - expect { service.execute }.to( - change { ProjectAuthorization.count }.from(0).to(1)) - end - end - end - - context 'no group member' do - it 'does not create project authorization' do - expect { service.execute }.not_to( - change { ProjectAuthorization.count }.from(0)) - end - end - - context 'unapproved access requests' do - before do - create(:group_member, :guest, :access_request, user: group_user, group: group) - end - - it 'does not create project authorization' do - expect { service.execute }.not_to( - change { ProjectAuthorization.count }.from(0)) - end - end - - context 'member with minimal access' do - before do - create(:group_member, :minimal_access, user: group_user, source: group) - end - - it 'does not create project authorization' do - expect { service.execute }.not_to( - change { ProjectAuthorization.count }.from(0)) - end - end - - context 'project has more user than BATCH_SIZE' do - let(:batch_size) { 2 } - let(:users) { create_list(:user, batch_size + 1 ) } - - before do - stub_const("#{described_class.name}::BATCH_SIZE", batch_size) - - users.each do |user| - create(:group_member, access_level: access_level, group: group_parent, user: user) - end - - ProjectAuthorization.delete_all - end - - it 'bulk creates project authorizations in batches' do - users.each_slice(batch_size) do |batch| - attributes = batch.map do |user| - { user_id: user.id, project_id: group_project.id, access_level: access_level } - end - - expect(ProjectAuthorization).to( - receive(:insert_all).with(array_including(attributes)).and_call_original) - end - - expect { service.execute }.to( - change { ProjectAuthorization.count }.from(0).to(batch_size + 1)) - end - end - - context 'ignores existing project authorizations' do - before do - # ProjectAuthorizations is also created because of an after_commit - # callback on Member model - create(:group_member, access_level: access_level, group: group, user: group_user) - end - - it 'does not create project authorization' do - project_authorization = ProjectAuthorization.where( - project_id: group_project.id, - user_id: group_user.id, - access_level: access_level) - - expect { service.execute }.not_to( - change { project_authorization.reload.exists? }.from(true)) - end - end - end -end diff --git a/spec/services/authorized_project_update/project_group_link_create_service_spec.rb b/spec/services/authorized_project_update/project_group_link_create_service_spec.rb deleted file mode 100644 index 1fd47f78c24..00000000000 --- a/spec/services/authorized_project_update/project_group_link_create_service_spec.rb +++ /dev/null @@ -1,222 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe AuthorizedProjectUpdate::ProjectGroupLinkCreateService do - let_it_be(:group_parent) { create(:group, :private) } - let_it_be(:group) { create(:group, :private, parent: group_parent) } - let_it_be(:group_child) { create(:group, :private, parent: group) } - - let_it_be(:parent_group_user) { create(:user) } - let_it_be(:group_user) { create(:user) } - - let_it_be(:project) { create(:project, :private, group: create(:group, :private)) } - - let(:access_level) { Gitlab::Access::MAINTAINER } - let(:group_access) { nil } - - subject(:service) { described_class.new(project, group, group_access) } - - describe '#perform' do - context 'direct group members' do - before do - create(:group_member, access_level: access_level, 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: project.id, - user_id: group_user.id, - access_level: access_level) - - expect(project_authorization).to exist - end - end - - context 'inherited group members' do - before do - create(:group_member, access_level: access_level, group: group_parent, user: parent_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: project.id, - user_id: parent_group_user.id, - access_level: access_level) - expect(project_authorization).to exist - end - end - - context 'with group_access' do - let(:group_access) { Gitlab::Access::REPORTER } - - before do - create(:group_member, access_level: access_level, group: group_parent, user: parent_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: project.id, - user_id: parent_group_user.id, - access_level: group_access) - expect(project_authorization).to exist - end - 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 - end - - it 'creates project authorization' do - expect { service.execute }.to( - change { ProjectAuthorization.count }.from(0).to(1)) - - project_authorization = ProjectAuthorization.where( - project_id: project.id, - user_id: group_user.id, - access_level: Gitlab::Access::DEVELOPER) - expect(project_authorization).to exist - end - end - - context 'no group member' do - it 'does not create project authorization' do - expect { service.execute }.not_to( - change { ProjectAuthorization.count }.from(0)) - end - end - - context 'unapproved access requests' do - before do - create(:group_member, :guest, :access_request, user: group_user, group: group) - end - - it 'does not create project authorization' do - expect { service.execute }.not_to( - change { ProjectAuthorization.count }.from(0)) - end - end - - context 'minimal access member' do - before do - create(:group_member, :minimal_access, user: group_user, source: group) - end - - it 'does not create project authorization' do - expect { service.execute }.not_to( - change { ProjectAuthorization.count }.from(0)) - end - end - - context 'project has more users than BATCH_SIZE' do - let(:batch_size) { 2 } - let(:users) { create_list(:user, batch_size + 1 ) } - - before do - stub_const("#{described_class.name}::BATCH_SIZE", batch_size) - - users.each do |user| - create(:group_member, access_level: access_level, group: group_parent, user: user) - end - - ProjectAuthorization.delete_all - end - - it 'bulk creates project authorizations in batches' do - users.each_slice(batch_size) do |batch| - attributes = batch.map do |user| - { user_id: user.id, project_id: project.id, access_level: access_level } - end - - expect(ProjectAuthorization).to( - receive(:insert_all).with(array_including(attributes)).and_call_original) - end - - expect { service.execute }.to( - change { ProjectAuthorization.count }.from(0).to(batch_size + 1)) - end - end - - context 'users have existing project authorizations' do - before do - create(:group_member, access_level: access_level, group: group, user: group_user) - ProjectAuthorization.delete_all - - create(:project_authorization, user_id: group_user.id, - project_id: project.id, - access_level: existing_access_level) - end - - context 'when access level is the same' do - let(:existing_access_level) { access_level } - - it 'does not create project authorization' do - project_authorization = ProjectAuthorization.where( - project_id: project.id, - user_id: group_user.id, - access_level: existing_access_level) - - expect(ProjectAuthorization).not_to receive(:insert_all) - - expect { service.execute }.not_to( - change { project_authorization.reload.exists? }.from(true)) - end - end - - context 'when existing access level is lower' do - let(:existing_access_level) { Gitlab::Access::DEVELOPER } - - it 'creates new project authorization' do - project_authorization = ProjectAuthorization.where( - project_id: project.id, - user_id: group_user.id, - access_level: access_level) - - expect { service.execute }.to( - change { project_authorization.reload.exists? }.from(false).to(true)) - end - - it 'deletes previous project authorization' do - project_authorization = ProjectAuthorization.where( - project_id: project.id, - user_id: group_user.id, - access_level: existing_access_level) - - expect { service.execute }.to( - change { project_authorization.reload.exists? }.from(true).to(false)) - end - end - - context 'when existing access level is higher' do - let(:existing_access_level) { Gitlab::Access::OWNER } - - it 'does not create project authorization' do - project_authorization = ProjectAuthorization.where( - project_id: project.id, - user_id: group_user.id, - access_level: existing_access_level) - - expect(ProjectAuthorization).not_to receive(:insert_all) - - expect { service.execute }.not_to( - change { project_authorization.reload.exists? }.from(true)) - end - end - end - end -end diff --git a/spec/services/ci/create_downstream_pipeline_service_spec.rb b/spec/services/ci/create_downstream_pipeline_service_spec.rb index 6142704b00e..11fb564b843 100644 --- a/spec/services/ci/create_downstream_pipeline_service_spec.rb +++ b/spec/services/ci/create_downstream_pipeline_service_spec.rb @@ -35,18 +35,20 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do upstream_project.add_developer(user) end + subject { service.execute(bridge) } + context 'when downstream project has not been found' do let(:trigger) do { trigger: { project: 'unknown/project' } } end it 'does not create a pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } end it 'changes pipeline bridge job status to failed' do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason) @@ -56,12 +58,12 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do context 'when user can not access downstream project' do it 'does not create a new pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } end it 'changes status of the bridge build' do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason) @@ -75,12 +77,12 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'does not create a new pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } end it 'changes status of the bridge build' do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq 'insufficient_bridge_permissions' @@ -96,12 +98,12 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'creates only one new pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change { Ci::Pipeline.count }.by(1) end it 'creates a new pipeline in a downstream project' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.user).to eq bridge.user expect(pipeline.project).to eq downstream_project @@ -111,8 +113,14 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do expect(pipeline.source_bridge).to be_a ::Ci::Bridge end + it_behaves_like 'logs downstream pipeline creation' do + let(:expected_root_pipeline) { upstream_pipeline } + let(:expected_hierarchy_size) { 2 } + let(:expected_downstream_relationship) { :multi_project } + end + it 'updates bridge status when downstream pipeline gets processed' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.reload).to be_created expect(bridge.reload).to be_success @@ -136,7 +144,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do bridge_id: bridge.id, project_id: bridge.project.id) .and_call_original expect(Ci::CreatePipelineService).not_to receive(:new) - expect(service.execute(bridge)).to eq({ message: "Already has a downstream pipeline", status: :error }) + expect(subject).to eq({ message: "Already has a downstream pipeline", status: :error }) end end @@ -146,7 +154,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'is using default branch name' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.ref).to eq 'master' end @@ -158,12 +166,12 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'creates only one new pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change { Ci::Pipeline.count }.by(1) end it 'creates a new pipeline in a downstream project' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.user).to eq bridge.user expect(pipeline.project).to eq downstream_project @@ -174,7 +182,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'updates the bridge status when downstream pipeline gets processed' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.reload).to be_failed expect(bridge.reload).to be_failed @@ -188,12 +196,12 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do context 'detects a circular dependency' do it 'does not create a new pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } end it 'changes status of the bridge build' do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq 'invalid_bridge_trigger' @@ -209,12 +217,12 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do shared_examples 'creates a child pipeline' do it 'creates only one new pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change { Ci::Pipeline.count }.by(1) end it 'creates a child pipeline in the same project' do - pipeline = service.execute(bridge) + pipeline = subject pipeline.reload expect(pipeline.builds.map(&:name)).to match_array(%w[rspec echo]) @@ -227,14 +235,14 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'updates bridge status when downstream pipeline gets processed' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.reload).to be_created expect(bridge.reload).to be_success end it 'propagates parent pipeline settings to the child pipeline' do - pipeline = service.execute(bridge) + pipeline = subject pipeline.reload expect(pipeline.ref).to eq(upstream_pipeline.ref) @@ -264,8 +272,14 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do it_behaves_like 'creates a child pipeline' + it_behaves_like 'logs downstream pipeline creation' do + let(:expected_root_pipeline) { upstream_pipeline } + let(:expected_hierarchy_size) { 2 } + let(:expected_downstream_relationship) { :parent_child } + end + it 'updates the bridge job to success' do - expect { service.execute(bridge) }.to change { bridge.status }.to 'success' + expect { subject }.to change { bridge.status }.to 'success' end context 'when bridge uses "depend" strategy' do @@ -276,7 +290,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'does not update the bridge job status' do - expect { service.execute(bridge) }.not_to change { bridge.status } + expect { subject }.not_to change { bridge.status } end end @@ -306,7 +320,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do it_behaves_like 'creates a child pipeline' it 'propagates the merge request to the child pipeline' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.merge_request).to eq(merge_request) expect(pipeline).to be_merge_request @@ -322,11 +336,17 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'creates the pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change { Ci::Pipeline.count }.by(1) expect(bridge.reload).to be_success end + + it_behaves_like 'logs downstream pipeline creation' do + let(:expected_root_pipeline) { upstream_pipeline.parent_pipeline } + let(:expected_hierarchy_size) { 3 } + let(:expected_downstream_relationship) { :parent_child } + end end context 'when upstream pipeline has a parent pipeline, which has a parent pipeline' do @@ -345,7 +365,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'does not create a second descendant pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } expect(bridge.reload).to be_failed @@ -370,7 +390,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'create the pipeline' do - expect { service.execute(bridge) }.to change { Ci::Pipeline.count }.by(1) + expect { subject }.to change { Ci::Pipeline.count }.by(1) end end @@ -382,11 +402,11 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'creates a new pipeline allowing variables to be passed downstream' do - expect { service.execute(bridge) }.to change { Ci::Pipeline.count }.by(1) + expect { subject }.to change { Ci::Pipeline.count }.by(1) end it 'passes variables downstream from the bridge' do - pipeline = service.execute(bridge) + pipeline = subject pipeline.variables.map(&:key).tap do |variables| expect(variables).to include 'BRIDGE' @@ -444,12 +464,12 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do describe 'cyclical dependency detection' do shared_examples 'detects cyclical pipelines' do it 'does not create a new pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } end it 'changes status of the bridge build' do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq 'pipeline_loop_detected' @@ -458,12 +478,12 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do shared_examples 'passes cyclical pipeline precondition' do it 'creates a new pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change { Ci::Pipeline.count } end it 'expect bridge build not to be failed' do - service.execute(bridge) + subject expect(bridge.reload).not_to be_failed end @@ -537,19 +557,19 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'creates only one new pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change { Ci::Pipeline.count }.by(1) end it 'creates a new pipeline in the downstream project' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.user).to eq bridge.user expect(pipeline.project).to eq downstream_project end it 'drops the bridge' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.reload).to be_failed expect(bridge.reload).to be_failed @@ -573,7 +593,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do bridge_id: bridge.id, downstream_pipeline_id: kind_of(Numeric)) - service.execute(bridge) + subject end end @@ -583,7 +603,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'passes bridge variables to downstream pipeline' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.variables.first) .to have_attributes(key: 'BRIDGE', value: 'var') @@ -596,7 +616,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'does not pass pipeline variables directly downstream' do - pipeline = service.execute(bridge) + pipeline = subject pipeline.variables.map(&:key).tap do |variables| expect(variables).not_to include 'PIPELINE_VARIABLE' @@ -609,7 +629,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'makes it possible to pass pipeline variable downstream' do - pipeline = service.execute(bridge) + pipeline = subject pipeline.variables.find_by(key: 'BRIDGE').tap do |variable| expect(variable.value).to eq 'my-value-var' @@ -622,12 +642,12 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'does not create a new pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } end it 'ignores variables passed downstream from the bridge' do - pipeline = service.execute(bridge) + pipeline = subject pipeline.variables.map(&:key).tap do |variables| expect(variables).not_to include 'BRIDGE' @@ -635,7 +655,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'sets errors', :aggregate_failures do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq('downstream_pipeline_creation_failed') @@ -679,7 +699,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do context 'that include the bridge job' do it 'creates the downstream pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change(downstream_project.ci_pipelines, :count).by(1) end end @@ -692,7 +712,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'changes status of the bridge build' do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq 'insufficient_bridge_permissions' @@ -710,7 +730,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'does not create a pipeline and drops the bridge' do - expect { service.execute(bridge) }.not_to change(downstream_project.ci_pipelines, :count) + expect { subject }.not_to change(downstream_project.ci_pipelines, :count) expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq('downstream_pipeline_creation_failed') @@ -733,7 +753,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'does not create a pipeline and drops the bridge' do - expect { service.execute(bridge) }.not_to change(downstream_project.ci_pipelines, :count) + expect { subject }.not_to change(downstream_project.ci_pipelines, :count) expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq('downstream_pipeline_creation_failed') @@ -755,7 +775,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'creates the pipeline but drops the bridge' do - expect { service.execute(bridge) }.to change(downstream_project.ci_pipelines, :count).by(1) + expect { subject }.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') @@ -787,7 +807,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it 'creates the pipeline' do - expect { service.execute(bridge) }.to change(downstream_project.ci_pipelines, :count).by(1) + expect { subject }.to change(downstream_project.ci_pipelines, :count).by(1) expect(bridge.reload).to be_success end @@ -795,7 +815,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do context 'when not passing the required variable' do it 'does not create the pipeline' do - expect { service.execute(bridge) }.not_to change(downstream_project.ci_pipelines, :count) + expect { subject }.not_to change(downstream_project.ci_pipelines, :count) end end end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 943d70ba142..c39a76ad2fc 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -466,7 +466,7 @@ RSpec.describe Ci::CreatePipelineService do it 'pull it from Auto-DevOps' do pipeline = execute_service.payload expect(pipeline).to be_auto_devops_source - expect(pipeline.builds.map(&:name)).to match_array(%w[brakeman-sast build code_quality eslint-sast secret_detection semgrep-sast test]) + expect(pipeline.builds.map(&:name)).to match_array(%w[brakeman-sast build code_quality container_scanning eslint-sast secret_detection semgrep-sast test]) end end diff --git a/spec/services/ci/generate_kubeconfig_service_spec.rb b/spec/services/ci/generate_kubeconfig_service_spec.rb index b0673d16158..e3088ca6ea7 100644 --- a/spec/services/ci/generate_kubeconfig_service_spec.rb +++ b/spec/services/ci/generate_kubeconfig_service_spec.rb @@ -6,16 +6,17 @@ RSpec.describe Ci::GenerateKubeconfigService do describe '#execute' do let(:project) { create(:project) } let(:build) { create(:ci_build, project: project) } + let(:pipeline) { build.pipeline } let(:agent1) { create(:cluster_agent, project: project) } let(:agent2) { create(:cluster_agent) } let(:template) { instance_double(Gitlab::Kubernetes::Kubeconfig::Template) } - subject { described_class.new(build).execute } + subject { described_class.new(pipeline, token: build.token).execute } before do expect(Gitlab::Kubernetes::Kubeconfig::Template).to receive(:new).and_return(template) - expect(build.pipeline).to receive(:authorized_cluster_agents).and_return([agent1, agent2]) + expect(pipeline).to receive(:authorized_cluster_agents).and_return([agent1, agent2]) end it 'adds a cluster, and a user and context for each available agent' do diff --git a/spec/services/ci/job_artifacts/create_service_spec.rb b/spec/services/ci/job_artifacts/create_service_spec.rb index b8487e438a9..01f240805f5 100644 --- a/spec/services/ci/job_artifacts/create_service_spec.rb +++ b/spec/services/ci/job_artifacts/create_service_spec.rb @@ -42,6 +42,13 @@ RSpec.describe Ci::JobArtifacts::CreateService do subject { service.execute(artifacts_file, params, metadata_file: metadata_file) } context 'when artifacts file is uploaded' do + it 'returns artifact in the response' do + response = subject + new_artifact = job.job_artifacts.last + + expect(response[:artifact]).to eq(new_artifact) + end + it 'saves artifact for the given type' do expect { subject }.to change { Ci::JobArtifact.count }.by(1) @@ -84,7 +91,7 @@ RSpec.describe Ci::JobArtifacts::CreateService do it 'sets expiration date according to application settings' do expected_expire_at = 1.day.from_now - expect(subject).to match(a_hash_including(status: :success)) + expect(subject).to match(a_hash_including(status: :success, artifact: anything)) archive_artifact, metadata_artifact = job.job_artifacts.last(2) expect(job.artifacts_expire_at).to be_within(1.minute).of(expected_expire_at) @@ -100,7 +107,7 @@ RSpec.describe Ci::JobArtifacts::CreateService do it 'sets expiration date according to the parameter' do expected_expire_at = 2.hours.from_now - expect(subject).to match(a_hash_including(status: :success)) + expect(subject).to match(a_hash_including(status: :success, artifact: anything)) archive_artifact, metadata_artifact = job.job_artifacts.last(2) expect(job.artifacts_expire_at).to be_within(1.minute).of(expected_expire_at) diff --git a/spec/services/ci/pipeline_artifacts/coverage_report_service_spec.rb b/spec/services/ci/pipeline_artifacts/coverage_report_service_spec.rb index b48ea70aa4c..98b01e2b303 100644 --- a/spec/services/ci/pipeline_artifacts/coverage_report_service_spec.rb +++ b/spec/services/ci/pipeline_artifacts/coverage_report_service_spec.rb @@ -38,7 +38,7 @@ RSpec.describe ::Ci::PipelineArtifacts::CoverageReportService do context 'when pipeline artifact has already been created' do it 'do not raise an error and do not persist the same artifact twice' do - expect { 2.times { described_class.new.execute(pipeline) } }.not_to raise_error(ActiveRecord::RecordNotUnique) + expect { 2.times { described_class.new.execute(pipeline) } }.not_to raise_error expect(Ci::PipelineArtifact.count).to eq(1) end diff --git a/spec/services/ci/pipeline_creation/start_pipeline_service_spec.rb b/spec/services/ci/pipeline_creation/start_pipeline_service_spec.rb index 2aa810e8ea1..ab4ba20e716 100644 --- a/spec/services/ci/pipeline_creation/start_pipeline_service_spec.rb +++ b/spec/services/ci/pipeline_creation/start_pipeline_service_spec.rb @@ -16,5 +16,11 @@ RSpec.describe Ci::PipelineCreation::StartPipelineService do service.execute end + + it 'creates pipeline ref' do + expect(pipeline.persistent_ref).to receive(:create).once + + service.execute + end end end diff --git a/spec/services/ci/pipeline_trigger_service_spec.rb b/spec/services/ci/pipeline_trigger_service_spec.rb index 29d12b0dd0e..a794dedc658 100644 --- a/spec/services/ci/pipeline_trigger_service_spec.rb +++ b/spec/services/ci/pipeline_trigger_service_spec.rb @@ -187,6 +187,14 @@ RSpec.describe Ci::PipelineTriggerService do expect(result[:status]).to eq(:success) end + it_behaves_like 'logs downstream pipeline creation' do + subject { result[:pipeline] } + + let(:expected_root_pipeline) { pipeline } + let(:expected_hierarchy_size) { 2 } + let(:expected_downstream_relationship) { :multi_project } + end + context 'when commit message has [ci skip]' do before do allow_next_instance_of(Ci::Pipeline) do |instance| diff --git a/spec/services/ci/retry_job_service_spec.rb b/spec/services/ci/retry_job_service_spec.rb index 25aab73ab01..acc7a99637b 100644 --- a/spec/services/ci/retry_job_service_spec.rb +++ b/spec/services/ci/retry_job_service_spec.rb @@ -17,396 +17,276 @@ RSpec.describe Ci::RetryJobService do name: 'test') end - let_it_be_with_refind(:build) { create(:ci_build, :success, pipeline: pipeline, stage_id: stage.id) } - let(:user) { developer } - let(:service) do - described_class.new(project, user) - end + let(:service) { described_class.new(project, user) } before_all do project.add_developer(developer) project.add_reporter(reporter) end - clone_accessors = ::Ci::Build.clone_accessors.without(::Ci::Build.extra_accessors) - - reject_accessors = - %i[id status user token token_encrypted coverage trace runner - artifacts_expire_at - 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_secret_detection job_artifacts_dependency_scanning - job_artifacts_container_scanning job_artifacts_cluster_image_scanning job_artifacts_dast - job_artifacts_license_scanning - job_artifacts_performance job_artifacts_browser_performance job_artifacts_load_performance - job_artifacts_lsif job_artifacts_terraform job_artifacts_cluster_applications - 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 - job_artifacts_requirements job_artifacts_coverage_fuzzing - job_artifacts_api_fuzzing terraform_state_versions].freeze - - ignore_accessors = - %i[type lock_version target_url base_tags trace_sections - commit_id deployment erased_by_id project_id - runner_id tag_taggings taggings tags trigger_request_id - user_id auto_canceled_by_id retried failure_reason - sourced_pipelines artifacts_file_store artifacts_metadata_store - 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 report_results pending_state pages_deployments - queuing_entry runtime_metadata trace_metadata - dast_site_profile dast_scanner_profile].freeze - - shared_examples 'build duplication' do - let_it_be(:another_pipeline) { create(:ci_empty_pipeline, project: project) } + shared_context 'retryable bridge' do + let_it_be(:downstream_project) { create(:project, :repository) } - let_it_be(:build) do - create(:ci_build, :failed, :picked, :expired, :erased, :queued, :coverage, :tags, - :allowed_to_fail, :on_tag, :triggered, :teardown_environment, :resource_group, - description: 'my-job', stage: 'test', stage_id: stage.id, - pipeline: pipeline, auto_canceled_by: another_pipeline, - scheduled_at: 10.seconds.since) + let_it_be_with_refind(:job) do + create( + :ci_bridge, :success, pipeline: pipeline, downstream: downstream_project, + description: 'a trigger job', stage_id: stage.id + ) end - let_it_be(:internal_job_variable) { create(:ci_job_variable, job: build) } + let_it_be(:job_to_clone) { job } - before_all do - # Make sure that build has both `stage_id` and `stage` because FactoryBot - # can reset one of the fields when assigning another. We plan to deprecate - # and remove legacy `stage` column in the future. - build.update!(stage: 'test', stage_id: stage.id) - - # Make sure we have one instance for every possible job_artifact_X - # associations to check they are correctly rejected on build duplication. - Ci::JobArtifact::TYPE_AND_FORMAT_PAIRS.each do |file_type, file_format| - create(:ci_job_artifact, file_format, - file_type: file_type, job: build, expire_at: build.artifacts_expire_at) - end + before do + job.update!(retried: false) + end + end + + shared_context 'retryable build' do + let_it_be_with_refind(:job) { create(:ci_build, :success, pipeline: pipeline, stage_id: stage.id) } + let_it_be(:another_pipeline) { create(:ci_empty_pipeline, project: project) } - create(:ci_job_variable, :dotenv_source, job: build) - create(:ci_build_need, build: build) - create(:terraform_state_version, build: build) + let_it_be(:job_to_clone) do + create(:ci_build, :failed, :picked, :expired, :erased, :queued, :coverage, :tags, + :allowed_to_fail, :on_tag, :triggered, :teardown_environment, :resource_group, + description: 'my-job', stage: 'test', stage_id: stage.id, + pipeline: pipeline, auto_canceled_by: another_pipeline, + scheduled_at: 10.seconds.since) end before do - build.update!(retried: false, status: :success) + job.update!(retried: false, status: :success) + job_to_clone.update!(retried: false, status: :success) end + end - describe 'clone accessors' do - let(:forbidden_associations) do - Ci::Build.reflect_on_all_associations.each_with_object(Set.new) do |assoc, memo| - memo << assoc.name unless assoc.macro == :belongs_to - end - end + shared_examples_for 'clones the job' do + let(:job) { job_to_clone } - clone_accessors.each do |attribute| - it "clones #{attribute} build attribute", :aggregate_failures do - expect(attribute).not_to be_in(forbidden_associations), "association #{attribute} must be `belongs_to`" - expect(build.send(attribute)).not_to be_nil - expect(new_build.send(attribute)).not_to be_nil - expect(new_build.send(attribute)).to eq build.send(attribute) - end + before_all do + # Make sure that job has both `stage_id` and `stage` + job_to_clone.update!(stage: 'test', stage_id: stage.id) + + create(:ci_build_need, build: job_to_clone) + end + + context 'when the user has ability to execute job' do + before do + stub_not_protect_default_branch end - context 'when job has nullified protected' do - before do - build.update_attribute(:protected, nil) - end + context 'when there is a failed job ToDo for the MR' do + let!(:merge_request) { create(:merge_request, source_project: project, author: user, head_pipeline: pipeline) } + let!(:todo) { create(:todo, :build_failed, user: user, project: project, author: user, target: merge_request) } - it "clones protected build attribute" do - expect(new_build.protected).to be_nil - expect(new_build.protected).to eq build.protected + it 'resolves the ToDo for the failed job' do + expect do + service.execute(job) + end.to change { todo.reload.state }.from('pending').to('done') end end - it 'clones only the needs attributes' do - expect(new_build.needs.exists?).to be_truthy - expect(build.needs.exists?).to be_truthy + context 'when the job has needs' do + before do + create(:ci_build_need, build: job, name: 'build1') + create(:ci_build_need, build: job, name: 'build2') + end - expect(new_build.needs_attributes).to match(build.needs_attributes) - expect(new_build.needs).not_to match(build.needs) - end + it 'bulk inserts all the needs' do + expect(Ci::BuildNeed).to receive(:bulk_insert!).and_call_original - it 'clones only internal job variables' do - expect(new_build.job_variables.count).to eq(1) - expect(new_build.job_variables).to contain_exactly(having_attributes(key: internal_job_variable.key, value: internal_job_variable.value)) + new_job + end end - end - describe 'reject accessors' do - reject_accessors.each do |attribute| - it "does not clone #{attribute} build attribute" do - expect(new_build.send(attribute)).not_to eq build.send(attribute) - end + it 'marks the old job as retried' do + expect(new_job).to be_latest + expect(job).to be_retried + expect(job).to be_processed end end - it 'has correct number of known attributes', :aggregate_failures do - processed_accessors = clone_accessors + reject_accessors - known_accessors = processed_accessors + ignore_accessors - - # :tag_list is a special case, this accessor does not exist - # in reflected associations, comes from `act_as_taggable` and - # we use it to copy tags, instead of reusing tags. - # - current_accessors = - Ci::Build.attribute_names.map(&:to_sym) + - Ci::Build.attribute_aliases.keys.map(&:to_sym) + - Ci::Build.reflect_on_all_associations.map(&:name) + - [:tag_list, :needs_attributes, :job_variables_attributes] - - # ee-specific accessors should be tested in ee/spec/services/ci/retry_job_service_spec.rb instead - Ci::Build.extra_accessors - - [:dast_site_profiles_build, :dast_scanner_profiles_build] # join tables - - current_accessors.uniq! - - expect(current_accessors).to include(*processed_accessors) - expect(known_accessors).to include(*current_accessors) - end - end + context 'when the user does not have permission to execute the job' do + let(:user) { reporter } - describe '#execute' do - let(:new_build) do - travel_to(1.second.from_now) do - service.execute(build)[:job] + it 'raises an error' do + expect { service.execute(job) } + .to raise_error Gitlab::Access::AccessDeniedError end end + end - context 'when user has ability to execute build' do - before do - stub_not_protect_default_branch - end - - it_behaves_like 'build duplication' + shared_examples_for 'retries the job' do + it_behaves_like 'clones the job' - it 'creates a new build that represents the old one' do - expect(new_build.name).to eq build.name - end + it 'enqueues the new job' do + expect(new_job).to be_pending + end - it 'enqueues the new build' do - expect(new_build).to be_pending + context 'when there are subsequent processables that are skipped' do + let!(:subsequent_build) do + create(:ci_build, :skipped, stage_idx: 2, + pipeline: pipeline, + stage: 'deploy') end - context 'when there are subsequent processables that are skipped' do - let!(:subsequent_build) do - create(:ci_build, :skipped, stage_idx: 2, + let!(:subsequent_bridge) do + create(:ci_bridge, :skipped, stage_idx: 2, pipeline: pipeline, stage: 'deploy') - end - - let!(:subsequent_bridge) do - create(:ci_bridge, :skipped, stage_idx: 2, - pipeline: pipeline, - stage: 'deploy') - end - - it 'resumes pipeline processing in the subsequent stage' do - service.execute(build) - - expect(subsequent_build.reload).to be_created - expect(subsequent_bridge.reload).to be_created - end - - it 'updates ownership for subsequent builds' do - expect { service.execute(build) }.to change { subsequent_build.reload.user }.to(user) - end - - it 'updates ownership for subsequent bridges' do - expect { service.execute(build) }.to change { subsequent_bridge.reload.user }.to(user) - end - - it 'does not cause n+1 when updaing build ownership' do - control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { service.execute(build) }.count + end - create_list(:ci_build, 2, :skipped, stage_idx: build.stage_idx + 1, pipeline: pipeline, stage: 'deploy') + it 'resumes pipeline processing in the subsequent stage' do + service.execute(job) - expect { service.execute(build) }.not_to exceed_all_query_limit(control_count) - end + expect(subsequent_build.reload).to be_created + expect(subsequent_bridge.reload).to be_created end - context 'when pipeline has other builds' do - let!(:stage2) { create(:ci_stage_entity, project: project, pipeline: pipeline, name: 'deploy') } - let!(:build2) { create(:ci_build, pipeline: pipeline, stage_id: stage.id ) } - let!(:deploy) { create(:ci_build, pipeline: pipeline, stage_id: stage2.id) } - let!(:deploy_needs_build2) { create(:ci_build_need, build: deploy, name: build2.name) } - - context 'when build has nil scheduling_type' do - before do - build.pipeline.processables.update_all(scheduling_type: nil) - build.reload - end - - it 'populates scheduling_type of processables' do - expect(new_build.scheduling_type).to eq('stage') - expect(build.reload.scheduling_type).to eq('stage') - expect(build2.reload.scheduling_type).to eq('stage') - expect(deploy.reload.scheduling_type).to eq('dag') - end - end - - context 'when build has scheduling_type' do - it 'does not call populate_scheduling_type!' do - expect_any_instance_of(Ci::Pipeline).not_to receive(:ensure_scheduling_type!) # rubocop: disable RSpec/AnyInstanceOf + it 'updates ownership for subsequent builds' do + expect { service.execute(job) }.to change { subsequent_build.reload.user }.to(user) + end - expect(new_build.scheduling_type).to eq('stage') - end - end + it 'updates ownership for subsequent bridges' do + expect { service.execute(job) }.to change { subsequent_bridge.reload.user }.to(user) end + end - context 'when the pipeline is a child pipeline and the bridge is depended' do - let!(:parent_pipeline) { create(:ci_pipeline, project: project) } - let!(:bridge) { create(:ci_bridge, :strategy_depend, pipeline: parent_pipeline, status: 'success') } - let!(:source_pipeline) { create(:ci_sources_pipeline, pipeline: pipeline, source_job: bridge) } + context 'when the pipeline has other jobs' do + let!(:stage2) { create(:ci_stage_entity, project: project, pipeline: pipeline, name: 'deploy') } + let!(:build2) { create(:ci_build, pipeline: pipeline, stage_id: stage.id ) } + let!(:deploy) { create(:ci_build, pipeline: pipeline, stage_id: stage2.id) } + let!(:deploy_needs_build2) { create(:ci_build_need, build: deploy, name: build2.name) } - it 'marks source bridge as pending' do - service.execute(build) + context 'when job has a nil scheduling_type' do + before do + job.pipeline.processables.update_all(scheduling_type: nil) + job.reload + end - expect(bridge.reload).to be_pending + it 'populates scheduling_type of processables' do + expect(new_job.scheduling_type).to eq('stage') + expect(job.reload.scheduling_type).to eq('stage') + expect(build2.reload.scheduling_type).to eq('stage') + expect(deploy.reload.scheduling_type).to eq('dag') end end - context 'when there is a failed job todo for the MR' do - let!(:merge_request) { create(:merge_request, source_project: project, author: user, head_pipeline: pipeline) } - let!(:todo) { create(:todo, :build_failed, user: user, project: project, author: user, target: merge_request) } + context 'when job has scheduling_type' do + it 'does not call populate_scheduling_type!' do + expect(job.pipeline).not_to receive(:ensure_scheduling_type!) - it 'resolves the todo for the old failed build' do - expect do - service.execute(build) - end.to change { todo.reload.state }.from('pending').to('done') + expect(new_job.scheduling_type).to eq('stage') end end end - context 'when user does not have ability to execute build' do - let(:user) { reporter } + context 'when the pipeline is a child pipeline and the bridge uses strategy:depend' do + let!(:parent_pipeline) { create(:ci_pipeline, project: project) } + let!(:bridge) { create(:ci_bridge, :strategy_depend, pipeline: parent_pipeline, status: 'success') } + let!(:source_pipeline) { create(:ci_sources_pipeline, pipeline: pipeline, source_job: bridge) } - it 'raises an error' do - expect { service.execute(build) } - .to raise_error Gitlab::Access::AccessDeniedError - end - - context 'when the job is not retryable' do - let(:build) { create(:ci_build, :created, pipeline: pipeline) } + it 'marks the source bridge as pending' do + service.execute(job) - it 'returns a ServiceResponse error' do - response = service.execute(build) - - expect(response).to be_a(ServiceResponse) - expect(response).to be_error - expect(response.message).to eq("Job cannot be retried") - end + expect(bridge.reload).to be_pending end end end describe '#clone!' do - let(:new_build) do - travel_to(1.second.from_now) do - service.clone!(build) - end - end + let(:new_job) { service.clone!(job) } it 'raises an error when an unexpected class is passed' do expect { service.clone!(create(:ci_build).present) }.to raise_error(TypeError) end - context 'when user has ability to execute build' do - before do - stub_not_protect_default_branch - end + context 'when the job to be cloned is a bridge' do + include_context 'retryable bridge' - it_behaves_like 'build duplication' + it_behaves_like 'clones the job' + end - it 'creates a new build that represents the old one' do - expect(new_build.name).to eq build.name - end + context 'when the job to be cloned is a build' do + include_context 'retryable build' - it 'does not enqueue the new build' do - expect(new_build).to be_created - expect(new_build).not_to be_processed - end + let(:job) { job_to_clone } - it 'does mark old build as retried' do - expect(new_build).to be_latest - expect(build).to be_retried - expect(build).to be_processed - end + it_behaves_like 'clones the job' - shared_examples_for 'when build with deployment is retried' do - let!(:build) do + context 'when a build with a deployment is retried' do + let!(:job) do create(:ci_build, :with_deployment, :deploy_to_production, - pipeline: pipeline, stage_id: stage.id, project: project) + pipeline: pipeline, stage_id: stage.id, project: project) end it 'creates a new deployment' do - expect { new_build }.to change { Deployment.count }.by(1) - end - - it 'persists expanded environment name' do - expect(new_build.metadata.expanded_environment_name).to eq('production') + expect { new_job }.to change { Deployment.count }.by(1) end it 'does not create a new environment' do - expect { new_build }.not_to change { Environment.count } + expect { new_job }.not_to change { Environment.count } end end - shared_examples_for 'when build with dynamic environment is retried' do + context 'when a build with a dynamic environment is retried' do let_it_be(:other_developer) { create(:user).tap { |u| project.add_developer(u) } } let(:environment_name) { 'review/$CI_COMMIT_REF_SLUG-$GITLAB_USER_ID' } - let!(:build) do + let!(:job) do create(:ci_build, :with_deployment, environment: environment_name, options: { environment: { name: environment_name } }, pipeline: pipeline, stage_id: stage.id, project: project, user: other_developer) end - it 're-uses the previous persisted environment' do - expect(build.persisted_environment.name).to eq("review/#{build.ref}-#{other_developer.id}") - - expect(new_build.persisted_environment.name).to eq("review/#{build.ref}-#{other_developer.id}") - end - it 'creates a new deployment' do - expect { new_build }.to change { Deployment.count }.by(1) + expect { new_job }.to change { Deployment.count }.by(1) end it 'does not create a new environment' do - expect { new_build }.not_to change { Environment.count } + expect { new_job }.not_to change { Environment.count } end end + end + end + + describe '#execute' do + let(:new_job) { service.execute(job)[:job] } - it_behaves_like 'when build with deployment is retried' - it_behaves_like 'when build with dynamic environment is retried' + context 'when the job to be retried is a bridge' do + include_context 'retryable bridge' - context 'when build has needs' do - before do - create(:ci_build_need, build: build, name: 'build1') - create(:ci_build_need, build: build, name: 'build2') - end + it_behaves_like 'retries the job' + end - it 'bulk inserts all needs' do - expect(Ci::BuildNeed).to receive(:bulk_insert!).and_call_original + context 'when the job to be retried is a build' do + include_context 'retryable build' + + it_behaves_like 'retries the job' - new_build + context 'when there are subsequent jobs that are skipped' do + let!(:subsequent_build) do + create(:ci_build, :skipped, stage_idx: 2, + pipeline: pipeline, + stage: 'deploy') end - end - end - context 'when user does not have ability to execute build' do - let(:user) { reporter } + let!(:subsequent_bridge) do + create(:ci_bridge, :skipped, stage_idx: 2, + pipeline: pipeline, + stage: 'deploy') + end - it 'raises an error' do - expect { service.clone!(build) } - .to raise_error Gitlab::Access::AccessDeniedError + it 'does not cause an N+1 when updating the job ownership' do + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { service.execute(job) }.count + + create_list(:ci_build, 2, :skipped, stage_idx: job.stage_idx + 1, pipeline: pipeline, stage: 'deploy') + + expect { service.execute(job) }.not_to exceed_all_query_limit(control_count) + end end end end diff --git a/spec/services/clusters/agents/delete_service_spec.rb b/spec/services/clusters/agents/delete_service_spec.rb index 1d6bc9618dd..abe1bdaab27 100644 --- a/spec/services/clusters/agents/delete_service_spec.rb +++ b/spec/services/clusters/agents/delete_service_spec.rb @@ -17,7 +17,7 @@ RSpec.describe Clusters::Agents::DeleteService do expect(response.status).to eq(:error) expect(response.message).to eq('You have insufficient permissions to delete this cluster agent') - expect { cluster_agent.reload }.not_to raise_error(ActiveRecord::RecordNotFound) + expect { cluster_agent.reload }.not_to raise_error end end diff --git a/spec/services/clusters/kubernetes/create_or_update_namespace_service_spec.rb b/spec/services/clusters/kubernetes/create_or_update_namespace_service_spec.rb index 98963f57341..90956e7b4ea 100644 --- a/spec/services/clusters/kubernetes/create_or_update_namespace_service_spec.rb +++ b/spec/services/clusters/kubernetes/create_or_update_namespace_service_spec.rb @@ -39,8 +39,6 @@ RSpec.describe Clusters::Kubernetes::CreateOrUpdateNamespaceService, '#execute' stub_kubeclient_put_role_binding(api_url, Clusters::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_BINDING_NAME, namespace: namespace) stub_kubeclient_put_role(api_url, Clusters::Kubernetes::GITLAB_CROSSPLANE_DATABASE_ROLE_NAME, namespace: namespace) stub_kubeclient_put_role_binding(api_url, Clusters::Kubernetes::GITLAB_CROSSPLANE_DATABASE_ROLE_BINDING_NAME, namespace: namespace) - stub_kubeclient_put_role(api_url, Clusters::Kubernetes::GITLAB_CILIUM_ROLE_NAME, namespace: namespace) - stub_kubeclient_put_role_binding(api_url, Clusters::Kubernetes::GITLAB_CILIUM_ROLE_BINDING_NAME, namespace: namespace) stub_kubeclient_get_secret( api_url, diff --git a/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb b/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb index 11045dfe950..a4f018aec0c 100644 --- a/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb +++ b/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb @@ -147,8 +147,6 @@ RSpec.describe Clusters::Kubernetes::CreateOrUpdateServiceAccountService do stub_kubeclient_put_role_binding(api_url, Clusters::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_BINDING_NAME, namespace: namespace) stub_kubeclient_put_role(api_url, Clusters::Kubernetes::GITLAB_CROSSPLANE_DATABASE_ROLE_NAME, namespace: namespace) stub_kubeclient_put_role_binding(api_url, Clusters::Kubernetes::GITLAB_CROSSPLANE_DATABASE_ROLE_BINDING_NAME, namespace: namespace) - stub_kubeclient_put_role(api_url, Clusters::Kubernetes::GITLAB_CILIUM_ROLE_NAME, namespace: namespace) - stub_kubeclient_put_role_binding(api_url, Clusters::Kubernetes::GITLAB_CILIUM_ROLE_BINDING_NAME, namespace: namespace) end it 'creates a namespace object' do @@ -245,47 +243,6 @@ RSpec.describe Clusters::Kubernetes::CreateOrUpdateServiceAccountService do ) ) end - - it 'creates a role granting cilium permissions to the service account' do - subject - - expect(WebMock).to have_requested(:put, api_url + "/apis/rbac.authorization.k8s.io/v1/namespaces/#{namespace}/roles/#{Clusters::Kubernetes::GITLAB_CILIUM_ROLE_NAME}").with( - body: hash_including( - metadata: { - name: Clusters::Kubernetes::GITLAB_CILIUM_ROLE_NAME, - namespace: namespace - }, - rules: [{ - apiGroups: %w(cilium.io), - resources: %w(ciliumnetworkpolicies), - verbs: %w(get list create update patch) - }] - ) - ) - end - - it 'creates a role binding granting cilium permissions to the service account' do - subject - - expect(WebMock).to have_requested(:put, api_url + "/apis/rbac.authorization.k8s.io/v1/namespaces/#{namespace}/rolebindings/#{Clusters::Kubernetes::GITLAB_CILIUM_ROLE_BINDING_NAME}").with( - body: hash_including( - metadata: { - name: Clusters::Kubernetes::GITLAB_CILIUM_ROLE_BINDING_NAME, - namespace: namespace - }, - roleRef: { - apiGroup: 'rbac.authorization.k8s.io', - kind: 'Role', - name: Clusters::Kubernetes::GITLAB_CILIUM_ROLE_NAME - }, - subjects: [{ - kind: 'ServiceAccount', - name: service_account_name, - namespace: namespace - }] - ) - ) - end end end end diff --git a/spec/services/container_expiration_policies/cleanup_service_spec.rb b/spec/services/container_expiration_policies/cleanup_service_spec.rb index a1f76e5e5dd..c265ce74d14 100644 --- a/spec/services/container_expiration_policies/cleanup_service_spec.rb +++ b/spec/services/container_expiration_policies/cleanup_service_spec.rb @@ -25,7 +25,7 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do it 'completely clean up the repository' do expect(Projects::ContainerRepository::CleanupTagsService) .to receive(:new).with(repository, nil, cleanup_tags_service_params).and_return(cleanup_tags_service) - expect(cleanup_tags_service).to receive(:execute).and_return(status: :success) + expect(cleanup_tags_service).to receive(:execute).and_return(status: :success, deleted_size: 1) response = subject @@ -36,6 +36,7 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do expect(repository.reload.cleanup_unscheduled?).to be_truthy expect(repository.expiration_policy_completed_at).not_to eq(nil) expect(repository.expiration_policy_started_at).not_to eq(nil) + expect(repository.last_cleanup_deleted_tags_count).to eq(1) end end end @@ -58,6 +59,7 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do expect(repository.reload.cleanup_unfinished?).to be_truthy expect(repository.expiration_policy_started_at).not_to eq(nil) expect(repository.expiration_policy_completed_at).to eq(nil) + expect(repository.last_cleanup_deleted_tags_count).to eq(nil) end end @@ -94,6 +96,7 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do expect(repository.reload.cleanup_unfinished?).to be_truthy expect(repository.expiration_policy_started_at).not_to eq(nil) expect(repository.expiration_policy_completed_at).to eq(nil) + expect(repository.last_cleanup_deleted_tags_count).to eq(nil) end end end @@ -138,6 +141,7 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do expect(repository.reload.cleanup_unfinished?).to be_truthy expect(repository.expiration_policy_started_at).not_to eq(nil) expect(repository.expiration_policy_completed_at).to eq(nil) + expect(repository.last_cleanup_deleted_tags_count).to eq(nil) end end diff --git a/spec/services/container_expiration_policies/update_service_spec.rb b/spec/services/container_expiration_policies/update_service_spec.rb index d4b6715ae86..7d949b77de7 100644 --- a/spec/services/container_expiration_policies/update_service_spec.rb +++ b/spec/services/container_expiration_policies/update_service_spec.rb @@ -63,7 +63,7 @@ RSpec.describe ContainerExpirationPolicies::UpdateService do 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' + :developer | 'denying access to container expiration policy' :reporter | 'denying access to container expiration policy' :guest | 'denying access to container expiration policy' :anonymous | 'denying access to container expiration policy' @@ -83,7 +83,7 @@ RSpec.describe ContainerExpirationPolicies::UpdateService do where(:user_role, :shared_examples_name) do :maintainer | 'creating the container expiration policy' - :developer | 'creating the container expiration policy' + :developer | 'denying access to container expiration policy' :reporter | 'denying access to container expiration policy' :guest | 'denying access to container expiration policy' :anonymous | 'denying access to container expiration policy' diff --git a/spec/services/container_expiration_policy_service_spec.rb b/spec/services/container_expiration_policy_service_spec.rb deleted file mode 100644 index 41dd890dd35..00000000000 --- a/spec/services/container_expiration_policy_service_spec.rb +++ /dev/null @@ -1,32 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ContainerExpirationPolicyService do - let_it_be(:user) { create(:user) } - let_it_be(:container_expiration_policy) { create(:container_expiration_policy, :runnable) } - - let(:project) { container_expiration_policy.project } - let(:container_repository) { create(:container_repository, project: project) } - - before do - project.add_maintainer(user) - end - - describe '#execute' do - subject { described_class.new(project, user).execute(container_expiration_policy) } - - it 'kicks off a cleanup worker for the container repository' do - expect(CleanupContainerRepositoryWorker).to receive(:perform_async) - .with(nil, container_repository.id, hash_including(container_expiration_policy: true)) - - subject - end - - it 'sets next_run_at on the container_expiration_policy' do - subject - - expect(container_expiration_policy.next_run_at).to be > Time.zone.now - end - end -end diff --git a/spec/services/customer_relations/contacts/create_service_spec.rb b/spec/services/customer_relations/contacts/create_service_spec.rb index 567e1c91e78..db6cce799fe 100644 --- a/spec/services/customer_relations/contacts/create_service_spec.rb +++ b/spec/services/customer_relations/contacts/create_service_spec.rb @@ -20,7 +20,7 @@ RSpec.describe CustomerRelations::Contacts::CreateService do it 'returns an error' do expect(response).to be_error - expect(response.message).to match_array(['You have insufficient permissions to create a contact for this group']) + expect(response.message).to match_array(['You have insufficient permissions to manage contacts for this group']) end end diff --git a/spec/services/customer_relations/contacts/update_service_spec.rb b/spec/services/customer_relations/contacts/update_service_spec.rb index 253bbc23226..729fdc2058b 100644 --- a/spec/services/customer_relations/contacts/update_service_spec.rb +++ b/spec/services/customer_relations/contacts/update_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe CustomerRelations::Contacts::UpdateService do let_it_be(:user) { create(:user) } - let(:contact) { create(:contact, first_name: 'Mark', group: group) } + let(:contact) { create(:contact, first_name: 'Mark', group: group, state: 'active') } subject(:update) { described_class.new(group: group, current_user: user, params: params).execute(contact) } @@ -19,7 +19,7 @@ RSpec.describe CustomerRelations::Contacts::UpdateService do response = update expect(response).to be_error - expect(response.message).to match_array(['You have insufficient permissions to update a contact for this group']) + expect(response.message).to match_array(['You have insufficient permissions to manage contacts for this group']) end end @@ -41,6 +41,29 @@ RSpec.describe CustomerRelations::Contacts::UpdateService do end end + context 'when activating' do + let(:contact) { create(:contact, state: 'inactive') } + let(:params) { { active: true } } + + it 'updates the contact' do + response = update + + expect(response).to be_success + expect(response.payload.active?).to be_truthy + end + end + + context 'when deactivating' do + let(:params) { { active: false } } + + it 'updates the contact' do + response = update + + expect(response).to be_success + expect(response.payload.active?).to be_falsy + end + end + context 'when the contact is invalid' do let(:params) { { first_name: nil } } diff --git a/spec/services/customer_relations/organizations/update_service_spec.rb b/spec/services/customer_relations/organizations/update_service_spec.rb index 8461c98ef0e..4764ba85551 100644 --- a/spec/services/customer_relations/organizations/update_service_spec.rb +++ b/spec/services/customer_relations/organizations/update_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe CustomerRelations::Organizations::UpdateService do let_it_be(:user) { create(:user) } - let(:organization) { create(:organization, name: 'Test', group: group) } + let(:organization) { create(:organization, name: 'Test', group: group, state: 'active') } subject(:update) { described_class.new(group: group, current_user: user, params: params).execute(organization) } @@ -41,6 +41,29 @@ RSpec.describe CustomerRelations::Organizations::UpdateService do end end + context 'when activating' do + let(:organization) { create(:organization, state: 'inactive') } + let(:params) { { active: true } } + + it 'updates the contact' do + response = update + + expect(response).to be_success + expect(response.payload.active?).to be_truthy + end + end + + context 'when deactivating' do + let(:params) { { active: false } } + + it 'updates the organization' do + response = update + + expect(response).to be_success + expect(response.payload.active?).to be_falsy + end + end + context 'when the organization is invalid' do let(:params) { { name: nil } } diff --git a/spec/services/database/consistency_fix_service_spec.rb b/spec/services/database/consistency_fix_service_spec.rb new file mode 100644 index 00000000000..9a0fac2191c --- /dev/null +++ b/spec/services/database/consistency_fix_service_spec.rb @@ -0,0 +1,153 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Database::ConsistencyFixService do + describe '#execute' do + context 'fixing namespaces inconsistencies' do + subject(:consistency_fix_service) do + described_class.new( + source_model: Namespace, + target_model: Ci::NamespaceMirror, + sync_event_class: Namespaces::SyncEvent, + source_sort_key: :id, + target_sort_key: :namespace_id + ) + end + + let(:table) { 'public.namespaces' } + let!(:namespace) { create(:namespace) } + let!(:namespace_mirror) { Ci::NamespaceMirror.find_by(namespace_id: namespace.id) } + + context 'when both objects exist' do + it 'creates a Namespaces::SyncEvent to modify the target object' do + expect do + consistency_fix_service.execute(ids: [namespace.id]) + end.to change { + Namespaces::SyncEvent.where(namespace_id: namespace.id).count + }.by(1) + end + + it 'enqueues the worker to process the Namespaces::SyncEvents' do + expect(::Namespaces::ProcessSyncEventsWorker).to receive(:perform_async) + consistency_fix_service.execute(ids: [namespace.id]) + end + end + + context 'when the source object has been deleted, but not the target' do + before do + namespace.delete + end + + it 'deletes the target object' do + expect do + consistency_fix_service.execute(ids: [namespace.id]) + end.to change { Ci::NamespaceMirror.where(namespace_id: namespace.id).count }.by(-1) + end + end + end + + context 'fixing projects inconsistencies' do + subject(:consistency_fix_service) do + described_class.new( + source_model: Project, + target_model: Ci::ProjectMirror, + sync_event_class: Projects::SyncEvent, + source_sort_key: :id, + target_sort_key: :project_id + ) + end + + let(:table) { 'public.projects' } + let!(:project) { create(:project) } + let!(:project_mirror) { Ci::ProjectMirror.find_by(project_id: project.id) } + + context 'when both objects exist' do + it 'creates a Projects::SyncEvent to modify the target object' do + expect do + consistency_fix_service.execute(ids: [project.id]) + end.to change { + Projects::SyncEvent.where(project_id: project.id).count + }.by(1) + end + + it 'enqueues the worker to process the Projects::SyncEvents' do + expect(::Projects::ProcessSyncEventsWorker).to receive(:perform_async) + consistency_fix_service.execute(ids: [project.id]) + end + end + + context 'when the source object has been deleted, but not the target' do + before do + project.delete + end + + it 'deletes the target object' do + expect do + consistency_fix_service.execute(ids: [project.id]) + end.to change { Ci::ProjectMirror.where(project_id: project.id).count }.by(-1) + end + end + end + end + + describe '#create_sync_event_for' do + context 'when the source model is Namespace' do + let(:namespace) { create(:namespace) } + + let(:service) do + described_class.new( + source_model: Namespace, + target_model: Ci::NamespaceMirror, + sync_event_class: Namespaces::SyncEvent, + source_sort_key: :id, + target_sort_key: :namespace_id + ) + end + + it 'creates a Namespaces::SyncEvent object' do + expect do + service.send(:create_sync_event_for, namespace.id) + end.to change { Namespaces::SyncEvent.where(namespace_id: namespace.id).count }.by(1) + end + end + + context 'when the source model is Project' do + let(:project) { create(:project) } + + let(:service) do + described_class.new( + source_model: Project, + target_model: Ci::ProjectMirror, + sync_event_class: Projects::SyncEvent, + source_sort_key: :id, + target_sort_key: :project_id + ) + end + + it 'creates a Projects::SyncEvent object' do + expect do + service.send(:create_sync_event_for, project.id) + end.to change { Projects::SyncEvent.where(project_id: project.id).count }.by(1) + end + end + end + + context 'when the source model is User' do + let(:service) do + described_class.new( + source_model: User, + target_model: Ci::ProjectMirror, + sync_event_class: Projects::SyncEvent, + source_sort_key: :id, + target_sort_key: :project_id + ) + end + + it 'raises an error' do + expect do + service.send(:create_sync_event_for, 1) + end.to raise_error("Unknown Source Model User") + end + end +end diff --git a/spec/services/dependency_proxy/group_settings/update_service_spec.rb b/spec/services/dependency_proxy/group_settings/update_service_spec.rb index 6f8c55daa8d..4954d9ec267 100644 --- a/spec/services/dependency_proxy/group_settings/update_service_spec.rb +++ b/spec/services/dependency_proxy/group_settings/update_service_spec.rb @@ -42,7 +42,7 @@ RSpec.describe ::DependencyProxy::GroupSettings::UpdateService do where(:user_role, :shared_examples_name) do :maintainer | 'updating the dependency proxy group settings' - :developer | 'updating the dependency proxy group settings' + :developer | 'denying access to dependency proxy group settings' :reporter | 'denying access to dependency proxy group settings' :guest | 'denying access to dependency proxy group settings' :anonymous | 'denying access to dependency proxy group settings' diff --git a/spec/services/dependency_proxy/image_ttl_group_policies/update_service_spec.rb b/spec/services/dependency_proxy/image_ttl_group_policies/update_service_spec.rb index ceac8985c8e..3a6ba2cca71 100644 --- a/spec/services/dependency_proxy/image_ttl_group_policies/update_service_spec.rb +++ b/spec/services/dependency_proxy/image_ttl_group_policies/update_service_spec.rb @@ -72,7 +72,7 @@ RSpec.describe ::DependencyProxy::ImageTtlGroupPolicies::UpdateService do where(:user_role, :shared_examples_name) do :maintainer | 'updating the dependency proxy image ttl policy' - :developer | 'updating the dependency proxy image ttl policy' + :developer | 'denying access to dependency proxy image ttl policy' :reporter | 'denying access to dependency proxy image ttl policy' :guest | 'denying access to dependency proxy image ttl policy' :anonymous | 'denying access to dependency proxy image ttl policy' @@ -92,7 +92,7 @@ RSpec.describe ::DependencyProxy::ImageTtlGroupPolicies::UpdateService do where(:user_role, :shared_examples_name) do :maintainer | 'creating the dependency proxy image ttl policy' - :developer | 'creating the dependency proxy image ttl policy' + :developer | 'denying access to dependency proxy image ttl policy' :reporter | 'denying access to dependency proxy image ttl policy' :guest | 'denying access to dependency proxy image ttl policy' :anonymous | 'denying access to dependency proxy image ttl policy' @@ -108,7 +108,7 @@ RSpec.describe ::DependencyProxy::ImageTtlGroupPolicies::UpdateService do context 'when the policy is not found' do before do - group.add_developer(user) + group.add_maintainer(user) expect(group).to receive(:dependency_proxy_image_ttl_policy).and_return nil end diff --git a/spec/services/environments/stop_service_spec.rb b/spec/services/environments/stop_service_spec.rb index 9e9ef127c67..afbc0ba70f9 100644 --- a/spec/services/environments/stop_service_spec.rb +++ b/spec/services/environments/stop_service_spec.rb @@ -161,8 +161,8 @@ RSpec.describe Environments::StopService do end end - describe '#execute_for_merge_request' do - subject { service.execute_for_merge_request(merge_request) } + describe '#execute_for_merge_request_pipeline' do + subject { service.execute_for_merge_request_pipeline(merge_request) } let(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'master') } let(:project) { merge_request.project } @@ -199,6 +199,19 @@ RSpec.describe Environments::StopService do expect(pipeline.environments_in_self_and_descendants.first).to be_stopped end + context 'when pipeline is a branch pipeline for merge request' do + let(:pipeline) do + create(:ci_pipeline, source: :push, project: project, sha: merge_request.diff_head_sha, + merge_requests_as_head_pipeline: [merge_request]) + end + + it 'does not stop the active environment' do + subject + + expect(pipeline.environments_in_self_and_descendants.first).to be_available + end + end + context 'with environment related jobs ' do let!(:environment) { create(:environment, :available, name: 'staging', project: project) } let!(:prepare_staging_job) { create(:ci_build, :prepare_staging, pipeline: pipeline, project: project) } @@ -210,18 +223,6 @@ RSpec.describe Environments::StopService do expect(prepare_staging_job.persisted_environment.state).to eq('available') end - - context 'when fix_related_environments_for_merge_requests feature flag is disabled' do - before do - stub_feature_flags(fix_related_environments_for_merge_requests: false) - end - - it 'stops unrelated environments too' do - subject - - expect(prepare_staging_job.persisted_environment.state).to eq('stopped') - end - end end end diff --git a/spec/services/error_tracking/base_service_spec.rb b/spec/services/error_tracking/base_service_spec.rb index 2f2052f0189..de3523cb847 100644 --- a/spec/services/error_tracking/base_service_spec.rb +++ b/spec/services/error_tracking/base_service_spec.rb @@ -4,8 +4,8 @@ require 'spec_helper' RSpec.describe ErrorTracking::BaseService do describe '#compose_response' do - let(:project) { double('project') } - let(:user) { double('user', id: non_existing_record_id) } + let(:project) { build_stubbed(:project) } + let(:user) { build_stubbed(:user, id: non_existing_record_id) } let(:service) { described_class.new(project, user) } it 'returns bad_request error when response has an error key' do @@ -19,7 +19,10 @@ RSpec.describe ErrorTracking::BaseService do end it 'returns server error when response has missing key error_type' do - data = { error: 'Unexpected Error', error_type: ErrorTracking::ProjectErrorTrackingSetting::SENTRY_API_ERROR_TYPE_MISSING_KEYS } + data = { + error: 'Unexpected Error', + error_type: ErrorTracking::ProjectErrorTrackingSetting::SENTRY_API_ERROR_TYPE_MISSING_KEYS + } result = service.send(:compose_response, data) @@ -48,7 +51,7 @@ RSpec.describe ErrorTracking::BaseService do context 'when parse_response is implemented' do before do - expect(service).to receive(:parse_response) do |response| + allow(service).to receive(:parse_response) do |response| { animal: response[:thing] } end end diff --git a/spec/services/error_tracking/collect_error_service_spec.rb b/spec/services/error_tracking/collect_error_service_spec.rb index faca3c12a48..159c070c683 100644 --- a/spec/services/error_tracking/collect_error_service_spec.rb +++ b/spec/services/error_tracking/collect_error_service_spec.rb @@ -52,12 +52,13 @@ RSpec.describe ErrorTracking::CollectErrorService do end context 'with unusual payload' do - let(:modified_event) { parsed_event } - let(:event) { described_class.new(project, nil, event: modified_event).execute } + let(:event) { ErrorTracking::ErrorEvent.last! } context 'when transaction is missing' do it 'builds actor from stacktrace' do - modified_event.delete('transaction') + parsed_event.delete('transaction') + + subject.execute expect(event.error.actor).to eq 'find()' end @@ -65,7 +66,9 @@ RSpec.describe ErrorTracking::CollectErrorService do context 'when transaction is an empty string' do \ it 'builds actor from stacktrace' do - modified_event['transaction'] = '' + parsed_event['transaction'] = '' + + subject.execute expect(event.error.actor).to eq 'find()' end @@ -73,7 +76,9 @@ RSpec.describe ErrorTracking::CollectErrorService do context 'when timestamp is numeric' do it 'parses timestamp' do - modified_event['timestamp'] = '1631015580.50' + parsed_event['timestamp'] = '1631015580.50' + + subject.execute expect(event.occurred_at).to eq '2021-09-07T11:53:00.5' end diff --git a/spec/services/error_tracking/issue_details_service_spec.rb b/spec/services/error_tracking/issue_details_service_spec.rb index 8cc2688d198..29f8154a27c 100644 --- a/spec/services/error_tracking/issue_details_service_spec.rb +++ b/spec/services/error_tracking/issue_details_service_spec.rb @@ -14,7 +14,7 @@ RSpec.describe ErrorTracking::IssueDetailsService do let(:params) { { issue_id: detailed_error.id } } before do - expect(error_tracking_setting) + allow(error_tracking_setting) .to receive(:issue_details).and_return(issue: detailed_error) end @@ -40,7 +40,7 @@ RSpec.describe ErrorTracking::IssueDetailsService do include_examples 'error tracking service sentry error handling', :issue_details include_examples 'error tracking service http status handling', :issue_details - context 'integrated error tracking' do + context 'with integrated error tracking' do let_it_be(:error) { create(:error_tracking_error, project: project) } let(:params) { { issue_id: error.id } } @@ -53,6 +53,18 @@ RSpec.describe ErrorTracking::IssueDetailsService do expect(result[:status]).to eq(:success) expect(result[:issue].to_json).to eq(error.to_sentry_detailed_error.to_json) end + + context 'when error does not exist' do + let(:params) { { issue_id: non_existing_record_id } } + + it 'returns the error in detailed format' do + expect(result).to match( + status: :error, + message: /Couldn't find ErrorTracking::Error/, + http_status: :bad_request + ) + end + end end end diff --git a/spec/services/error_tracking/issue_latest_event_service_spec.rb b/spec/services/error_tracking/issue_latest_event_service_spec.rb index e914cb1241e..aa2430ddffb 100644 --- a/spec/services/error_tracking/issue_latest_event_service_spec.rb +++ b/spec/services/error_tracking/issue_latest_event_service_spec.rb @@ -15,7 +15,7 @@ RSpec.describe ErrorTracking::IssueLatestEventService do let(:error_event) { build(:error_tracking_sentry_error_event) } before do - expect(error_tracking_setting) + allow(error_tracking_setting) .to receive(:issue_latest_event).and_return(latest_event: error_event) end @@ -28,7 +28,7 @@ RSpec.describe ErrorTracking::IssueLatestEventService do include_examples 'error tracking service sentry error handling', :issue_latest_event include_examples 'error tracking service http status handling', :issue_latest_event - context 'integrated error tracking' do + context 'with integrated error tracking' do let_it_be(:error) { create(:error_tracking_error, project: project) } let_it_be(:event) { create(:error_tracking_error_event, error: error) } @@ -42,6 +42,18 @@ RSpec.describe ErrorTracking::IssueLatestEventService do expect(result[:status]).to eq(:success) expect(result[:latest_event].to_json).to eq(event.to_sentry_error_event.to_json) end + + context 'when error does not exist' do + let(:params) { { issue_id: non_existing_record_id } } + + it 'returns the error in detailed format' do + expect(result).to match( + status: :error, + message: /Couldn't find ErrorTracking::Error/, + http_status: :bad_request + ) + end + end end end diff --git a/spec/services/error_tracking/issue_update_service_spec.rb b/spec/services/error_tracking/issue_update_service_spec.rb index 31a66654100..a06c3588264 100644 --- a/spec/services/error_tracking/issue_update_service_spec.rb +++ b/spec/services/error_tracking/issue_update_service_spec.rb @@ -13,8 +13,7 @@ RSpec.describe ErrorTracking::IssueUpdateService do it 'does not call the close issue service' do update_service.execute - expect(issue_close_service) - .not_to have_received(:execute) + expect(issue_close_service).not_to have_received(:execute) end it 'does not create system note' do @@ -29,8 +28,7 @@ RSpec.describe ErrorTracking::IssueUpdateService do let(:update_issue_response) { { updated: true } } before do - expect(error_tracking_setting) - .to receive(:update_issue).and_return(update_issue_response) + allow(error_tracking_setting).to receive(:update_issue).and_return(update_issue_response) end it 'returns the response' do @@ -49,12 +47,11 @@ RSpec.describe ErrorTracking::IssueUpdateService do result end - context 'related issue and resolving' do + context 'with related issue and resolving' do let(:issue) { create(:issue, project: project) } let(:sentry_issue) { create(:sentry_issue, issue: issue) } let(:arguments) { { issue_id: sentry_issue.sentry_issue_identifier, status: 'resolved' } } - - let(:issue_close_service) { spy(:issue_close_service) } + let(:issue_close_service) { instance_double('Issues::CloseService') } before do allow_next_instance_of(SentryIssueFinder) do |finder| @@ -78,11 +75,11 @@ RSpec.describe ErrorTracking::IssueUpdateService do .with(issue, system_note: false) end - context 'issues gets closed' do + context 'when issue gets closed' do let(:closed_issue) { create(:issue, :closed, project: project) } before do - expect(issue_close_service) + allow(issue_close_service) .to receive(:execute) .with(issue, system_note: false) .and_return(closed_issue) @@ -99,13 +96,13 @@ RSpec.describe ErrorTracking::IssueUpdateService do end end - context 'issue is already closed' do + context 'when issue is already closed' do let(:issue) { create(:issue, :closed, project: project) } include_examples 'does not perform close issue flow' end - context 'status is not resolving' do + context 'when status is not resolving' do let(:arguments) { { issue_id: sentry_issue.sentry_issue_identifier, status: 'ignored' } } include_examples 'does not perform close issue flow' @@ -115,7 +112,7 @@ RSpec.describe ErrorTracking::IssueUpdateService do include_examples 'error tracking service sentry error handling', :update_issue - context 'integrated error tracking' do + context 'with integrated error tracking' do let(:error) { create(:error_tracking_error, project: project) } let(:arguments) { { issue_id: error.id, status: 'resolved' } } let(:update_issue_response) { { updated: true, status: :success, closed_issue_iid: nil } } diff --git a/spec/services/groups/group_links/create_service_spec.rb b/spec/services/groups/group_links/create_service_spec.rb index 03dac14be54..bfbaedbd06f 100644 --- a/spec/services/groups/group_links/create_service_spec.rb +++ b/spec/services/groups/group_links/create_service_spec.rb @@ -3,23 +3,13 @@ require 'spec_helper' RSpec.describe Groups::GroupLinks::CreateService, '#execute' do - let(:parent_group_user) { create(:user) } - let(:group_user) { create(:user) } - let(:child_group_user) { create(:user) } - let(:prevent_sharing) { false } + let_it_be(:shared_with_group_parent) { create(:group, :private) } + let_it_be(:shared_with_group) { create(:group, :private, parent: shared_with_group_parent) } + let_it_be(:shared_with_group_child) { create(:group, :private, parent: shared_with_group) } let_it_be(:group_parent) { create(:group, :private) } - let_it_be(:group) { create(:group, :private, parent: group_parent) } - let_it_be(:group_child) { create(:group, :private, parent: group) } - let(:ns_for_parent) { create(:namespace_settings, prevent_sharing_groups_outside_hierarchy: prevent_sharing) } - let(:shared_group_parent) { create(:group, :private, namespace_settings: ns_for_parent) } - let(:shared_group) { create(:group, :private, parent: shared_group_parent) } - let(:shared_group_child) { create(:group, :private, parent: shared_group) } - - let(:project_parent) { create(:project, group: shared_group_parent) } - let(:project) { create(:project, group: shared_group) } - let(:project_child) { create(:project, group: shared_group_child) } + let(:group) { create(:group, :private, parent: group_parent) } let(:opts) do { @@ -28,127 +18,161 @@ RSpec.describe Groups::GroupLinks::CreateService, '#execute' do } end - let(:user) { group_user } + subject { described_class.new(group, shared_with_group, user, opts) } - subject { described_class.new(shared_group, group, user, opts) } + shared_examples_for 'not shareable' do + it 'does not share and returns an error' do + expect do + result = subject.execute - before do - group.add_guest(group_user) - shared_group.add_owner(group_user) + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(404) + end.not_to change { group.shared_with_group_links.count } + end end - it 'adds group to another group' do - expect { subject.execute }.to change { group.shared_group_links.count }.from(0).to(1) - end + shared_examples_for 'shareable' do + it 'adds group to another group' do + expect do + result = subject.execute - it 'returns false if shared group is blank' do - expect { described_class.new(nil, group, user, opts) }.not_to change { group.shared_group_links.count } + expect(result[:status]).to eq(:success) + end.to change { group.shared_with_group_links.count }.from(0).to(1) + end end - context 'user does not have access to group' do - let(:user) { create(:user) } - - before do - shared_group.add_owner(user) - end + context 'when user has proper membership to share a group' do + let_it_be(:group_user) { create(:user) } - it 'returns error' do - result = subject.execute + let(:user) { group_user } - expect(result[:status]).to eq(:error) - expect(result[:http_status]).to eq(404) + before do + shared_with_group.add_guest(group_user) + group.add_owner(group_user) end - end - context 'user does not have admin access to shared group' do - let(:user) { create(:user) } + it_behaves_like 'shareable' - before do - group.add_guest(user) - shared_group.add_developer(user) - end + context 'when sharing outside the hierarchy is disabled' do + let_it_be(:group_parent) do + create(:group, + namespace_settings: create(:namespace_settings, prevent_sharing_groups_outside_hierarchy: true)) + end - it 'returns error' do - result = subject.execute + it_behaves_like 'not shareable' - expect(result[:status]).to eq(:error) - expect(result[:http_status]).to eq(404) - end - end + context 'when group is inside hierarchy' do + let(:shared_with_group) { create(:group, :private, parent: group_parent) } - context 'project authorizations based on group hierarchies' do - before do - group_parent.add_owner(parent_group_user) - group.add_owner(group_user) - group_child.add_owner(child_group_user) + it_behaves_like 'shareable' + end end - context 'project authorizations refresh' do - it 'is executed only for the direct members of the group' do - expect(UserProjectAccessChangedService).to receive(:new).with(contain_exactly(group_user.id)).and_call_original + context 'project authorizations based on group hierarchies' do + let_it_be(:child_group_user) { create(:user) } + let_it_be(:parent_group_user) { create(:user) } - subject.execute + before do + shared_with_group_parent.add_owner(parent_group_user) + shared_with_group.add_owner(group_user) + shared_with_group_child.add_owner(child_group_user) end - end - context 'project authorizations' do - context 'group user' do - let(:user) { group_user } + context 'project authorizations refresh' do + it 'is executed only for the direct members of the group' do + expect(UserProjectAccessChangedService).to receive(:new).with(contain_exactly(group_user.id)) + .and_call_original - it 'create proper authorizations' do subject.execute - - expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey - expect(Ability.allowed?(user, :read_project, project)).to be_truthy - expect(Ability.allowed?(user, :read_project, project_child)).to be_truthy end end - context 'parent group user' do - let(:user) { parent_group_user } + context 'project authorizations' do + let(:group_child) { create(:group, :private, parent: group) } + let(:project_parent) { create(:project, group: group_parent) } + let(:project) { create(:project, group: group) } + let(:project_child) { create(:project, group: group_child) } - it 'create proper authorizations' do - subject.execute + context 'group user' do + let(:user) { group_user } + + it 'create proper authorizations' do + subject.execute - expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey - expect(Ability.allowed?(user, :read_project, project)).to be_falsey - expect(Ability.allowed?(user, :read_project, project_child)).to be_falsey + expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey + expect(Ability.allowed?(user, :read_project, project)).to be_truthy + expect(Ability.allowed?(user, :read_project, project_child)).to be_truthy + end end - end - context 'child group user' do - let(:user) { child_group_user } + context 'parent group user' do + let(:user) { parent_group_user } - it 'create proper authorizations' do - subject.execute + it 'create proper authorizations' do + subject.execute + + expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey + expect(Ability.allowed?(user, :read_project, project)).to be_falsey + expect(Ability.allowed?(user, :read_project, project_child)).to be_falsey + end + end - expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey - expect(Ability.allowed?(user, :read_project, project)).to be_falsey - expect(Ability.allowed?(user, :read_project, project_child)).to be_falsey + context 'child group user' do + let(:user) { child_group_user } + + it 'create proper authorizations' do + subject.execute + + expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey + expect(Ability.allowed?(user, :read_project, project)).to be_falsey + expect(Ability.allowed?(user, :read_project, project_child)).to be_falsey + end end end end end - context 'sharing outside the hierarchy is disabled' do - let(:prevent_sharing) { true } + context 'user does not have access to group' do + let(:user) { create(:user) } - it 'prevents sharing with a group outside the hierarchy' do - result = subject.execute + before do + group.add_owner(user) + end - expect(group.reload.shared_group_links.count).to eq(0) - expect(result[:status]).to eq(:error) - expect(result[:http_status]).to eq(404) + it_behaves_like 'not shareable' + end + + context 'user does not have admin access to shared group' do + let(:user) { create(:user) } + + before do + shared_with_group.add_guest(user) + group.add_developer(user) end - it 'allows sharing with a group within the hierarchy' do - sibling_group = create(:group, :private, parent: shared_group_parent) - sibling_group.add_guest(group_user) + it_behaves_like 'not shareable' + end + + context 'when group is blank' do + let(:group_user) { create(:user) } + let(:user) { group_user } + let(:group) { nil } - result = described_class.new(shared_group, sibling_group, user, opts).execute + it 'does not share and returns an error' do + expect do + result = subject.execute - expect(sibling_group.reload.shared_group_links.count).to eq(1) - expect(result[:status]).to eq(:success) + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(404) + end.not_to change { shared_with_group.shared_group_links.count } end end + + context 'when shared_with_group is blank' do + let(:group_user) { create(:user) } + let(:user) { group_user } + let(:shared_with_group) { nil } + + it_behaves_like 'not shareable' + end end diff --git a/spec/services/groups/group_links/destroy_service_spec.rb b/spec/services/groups/group_links/destroy_service_spec.rb index e63adc07313..6aaf5f45069 100644 --- a/spec/services/groups/group_links/destroy_service_spec.rb +++ b/spec/services/groups/group_links/destroy_service_spec.rb @@ -3,54 +3,77 @@ require 'spec_helper' RSpec.describe Groups::GroupLinks::DestroyService, '#execute' do - let(:user) { create(:user) } - + let_it_be(:user) { create(:user) } 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) } - 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) } + context 'when authorizing by user' do + before do + group.add_developer(owner) + shared_group.add_owner(owner) + end + + 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 { shared_group.shared_with_group_links.count }.from(1).to(0) + it 'destroys the link' do + expect { subject.execute(link) }.to change { shared_group.shared_with_group_links.count }.from(1).to(0) + end + + it 'revokes project authorization', :sidekiq_inline do + group.add_developer(user) + + expect { subject.execute(link) }.to( + change { Ability.allowed?(user, :read_project, project) }.from(true).to(false)) + end end - it 'revokes project authorization', :sidekiq_inline do - group.add_developer(user) + context 'multiple links' do + let_it_be(:another_group) { create(:group, :private) } + let_it_be(:another_shared_group) { create(:group, :private) } + + let!(:links) do + [ + create(:group_group_link, shared_group: shared_group, shared_with_group: group), + create(:group_group_link, shared_group: shared_group, shared_with_group: another_group), + create(:group_group_link, shared_group: another_shared_group, shared_with_group: group), + create(:group_group_link, shared_group: another_shared_group, shared_with_group: another_group) + ] + end - expect { subject.execute(link) }.to( - change { Ability.allowed?(user, :read_project, project) }.from(true).to(false)) + it 'updates project authorization once per group' do + expect(GroupGroupLink).to receive(:delete).and_call_original + expect(group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true, blocking: false).once + expect(another_group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true, blocking: false).once + + subject.execute(links) + end end end - context 'multiple links' do - let_it_be(:another_group) { create(:group, :private) } - let_it_be(:another_shared_group) { create(:group, :private) } - - let!(:links) do - [ - create(:group_group_link, shared_group: shared_group, shared_with_group: group), - create(:group_group_link, shared_group: shared_group, shared_with_group: another_group), - create(:group_group_link, shared_group: another_shared_group, shared_with_group: group), - create(:group_group_link, shared_group: another_shared_group, shared_with_group: another_group) - ] + context 'when skipping authorization' do + let!(:link) { create(:group_group_link, shared_group: shared_group, shared_with_group: group) } + + context 'with provided group and owner' do + it 'destroys the link' do + expect do + subject.execute(link, skip_authorization: true) + end.to change { shared_group.shared_with_group_links.count }.from(1).to(0) + end end - it 'updates project authorization once per group' do - expect(GroupGroupLink).to receive(:delete).and_call_original - expect(group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true, blocking: false).once - expect(another_group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true, blocking: false).once + context 'without providing group or owner' do + subject { described_class.new(nil, nil) } - subject.execute(links) + it 'destroys the link' do + expect do + subject.execute(link, skip_authorization: true) + end.to change { shared_group.shared_with_group_links.count }.from(1).to(0) + end end end end diff --git a/spec/services/groups/open_issues_count_service_spec.rb b/spec/services/groups/open_issues_count_service_spec.rb index 7dd8c2a59a0..fca09bfdebe 100644 --- a/spec/services/groups/open_issues_count_service_spec.rb +++ b/spec/services/groups/open_issues_count_service_spec.rb @@ -3,18 +3,12 @@ require 'spec_helper' RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_caching do - let_it_be(:group) { create(:group, :public) } + let_it_be(:group) { create(:group, :public)} let_it_be(:project) { create(:project, :public, namespace: group) } - let_it_be(:admin) { create(:user, :admin) } let_it_be(:user) { create(:user) } - let_it_be(:banned_user) { create(:user, :banned) } - - before do - create(:issue, :opened, project: project) - create(:issue, :opened, confidential: true, project: project) - create(:issue, :opened, author: banned_user, project: project) - create(:issue, :closed, project: project) - end + let_it_be(:issue) { create(:issue, :opened, project: project) } + let_it_be(:confidential) { create(:issue, :opened, confidential: true, project: project) } + let_it_be(:closed) { create(:issue, :closed, project: project) } subject { described_class.new(group, user) } @@ -26,27 +20,17 @@ RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_cac it 'uses the IssuesFinder to scope issues' do expect(IssuesFinder) .to receive(:new) - .with(user, group_id: group.id, state: 'opened', non_archived: true, include_subgroups: true, public_only: true, include_hidden: false) + .with(user, group_id: group.id, state: 'opened', non_archived: true, include_subgroups: true, public_only: true) subject.count end end describe '#count' do - shared_examples 'counts public issues, does not count hidden or confidential' do - it 'counts only public issues' do - expect(subject.count).to eq(1) - end - - it 'uses PUBLIC_COUNT_WITHOUT_HIDDEN_KEY cache key' do - expect(subject.cache_key).to include('group_open_public_issues_without_hidden_count') - end - end - context 'when user is nil' do - let(:user) { nil } - - it_behaves_like 'counts public issues, does not count hidden or confidential' + it 'does not include confidential issues in the issue count' do + expect(described_class.new(group).count).to eq(1) + end end context 'when user is provided' do @@ -55,13 +39,9 @@ RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_cac group.add_reporter(user) end - it 'includes confidential issues and does not include hidden issues in count' do + it 'returns the right count with confidential issues' do expect(subject.count).to eq(2) end - - it 'uses TOTAL_COUNT_WITHOUT_HIDDEN_KEY cache key' do - expect(subject.cache_key).to include('group_open_issues_without_hidden_count') - end end context 'when user cannot read confidential issues' do @@ -69,24 +49,8 @@ RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_cac group.add_guest(user) end - it_behaves_like 'counts public issues, does not count hidden or confidential' - end - - context 'when user is an admin' do - let(:user) { admin } - - context 'when admin mode is enabled', :enable_admin_mode do - it 'includes confidential and hidden issues in count' do - expect(subject.count).to eq(3) - end - - it 'uses TOTAL_COUNT_KEY cache key' do - expect(subject.cache_key).to include('group_open_issues_including_hidden_count') - end - end - - context 'when admin mode is disabled' do - it_behaves_like 'counts public issues, does not count hidden or confidential' + it 'does not include confidential issues' do + expect(subject.count).to eq(1) end end @@ -97,13 +61,11 @@ RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_cac describe '#clear_all_cache_keys' do it 'calls `Rails.cache.delete` with the correct keys' do expect(Rails.cache).to receive(:delete) - .with(['groups', 'open_issues_count_service', 1, group.id, described_class::PUBLIC_COUNT_WITHOUT_HIDDEN_KEY]) + .with(['groups', 'open_issues_count_service', 1, group.id, described_class::PUBLIC_COUNT_KEY]) expect(Rails.cache).to receive(:delete) .with(['groups', 'open_issues_count_service', 1, group.id, described_class::TOTAL_COUNT_KEY]) - expect(Rails.cache).to receive(:delete) - .with(['groups', 'open_issues_count_service', 1, group.id, described_class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY]) - described_class.new(group).clear_all_cache_keys + subject.clear_all_cache_keys end end end diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index 1c4b7aac87e..20ea8b2bf1b 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -574,7 +574,7 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do context 'resets project authorizations' do let_it_be(:old_parent_group) { create(:group) } - let_it_be_with_reload(:group) { create(:group, :private, parent: old_parent_group) } + let_it_be_with_refind(:group) { create(:group, :private, parent: old_parent_group) } let_it_be(:new_group_member) { create(:user) } let_it_be(:old_group_member) { create(:user) } let_it_be(:unique_subgroup_member) { create(:user) } diff --git a/spec/services/import/bitbucket_server_service_spec.rb b/spec/services/import/bitbucket_server_service_spec.rb index 56d93625b91..0b9fe10e95a 100644 --- a/spec/services/import/bitbucket_server_service_spec.rb +++ b/spec/services/import/bitbucket_server_service_spec.rb @@ -48,6 +48,23 @@ RSpec.describe Import::BitbucketServerService do end end + context 'when import source is disabled' do + before do + stub_application_setting(import_sources: nil) + allow(subject).to receive(:authorized?).and_return(true) + allow(client).to receive(:repo).with(project_key, repo_slug).and_return(double(repo)) + end + + it 'returns forbidden' do + result = subject.execute(credentials) + + expect(result).to include( + status: :error, + http_status: :forbidden + ) + end + end + context 'when user is unauthorized' do before do allow(subject).to receive(:authorized?).and_return(false) diff --git a/spec/services/import/github_service_spec.rb b/spec/services/import/github_service_spec.rb index 58afae1e647..1c26677cfa5 100644 --- a/spec/services/import/github_service_spec.rb +++ b/spec/services/import/github_service_spec.rb @@ -111,6 +111,33 @@ RSpec.describe Import::GithubService do end end + context 'when import source is disabled' do + let(:repository_double) do + double({ + name: 'vim', + description: 'test', + full_name: 'test/vim', + clone_url: 'http://repo.com/repo/repo.git', + private: false, + has_wiki?: false + }) + end + + before do + stub_application_setting(import_sources: nil) + allow(client).to receive(:repository).and_return(repository_double) + end + + it 'returns forbidden' do + result = subject.execute(access_params, :github) + + expect(result).to include( + status: :error, + http_status: :forbidden + ) + end + end + context 'when a blocked/local URL is used as github_hostname' do let(:message) { 'Error while attempting to import from GitHub' } let(:error) { "Invalid URL: #{url}" } diff --git a/spec/services/incident_management/timeline_events/create_service_spec.rb b/spec/services/incident_management/timeline_events/create_service_spec.rb new file mode 100644 index 00000000000..38ce15e74f1 --- /dev/null +++ b/spec/services/incident_management/timeline_events/create_service_spec.rb @@ -0,0 +1,117 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IncidentManagement::TimelineEvents::CreateService do + let_it_be(:user_with_permissions) { create(:user) } + let_it_be(:user_without_permissions) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be_with_refind(:incident) { create(:incident, project: project) } + let_it_be(:comment) { create(:note, project: project, noteable: incident) } + + let(:args) do + { + note: 'note', + occurred_at: Time.current, + action: 'new comment', + promoted_from_note: comment + } + end + + let(:current_user) { user_with_permissions } + let(:service) { described_class.new(incident, current_user, args) } + + before_all do + project.add_developer(user_with_permissions) + project.add_reporter(user_without_permissions) + end + + describe '#execute' do + shared_examples 'error response' do |message| + it 'has an informative message' do + expect(execute).to be_error + expect(execute.message).to eq(message) + end + end + + shared_examples 'success response' do + it 'has timeline event', :aggregate_failures do + expect(execute).to be_success + + result = execute.payload[:timeline_event] + expect(result).to be_a(::IncidentManagement::TimelineEvent) + expect(result.author).to eq(current_user) + expect(result.incident).to eq(incident) + expect(result.project).to eq(project) + expect(result.note).to eq(args[:note]) + expect(result.promoted_from_note).to eq(comment) + end + end + + subject(:execute) { service.execute } + + context 'when current user is blank' do + let(:current_user) { nil } + + it_behaves_like 'error response', 'You have insufficient permissions to manage timeline events for this incident' + end + + context 'when user does not have permissions to create timeline events' do + let(:current_user) { user_without_permissions } + + it_behaves_like 'error response', 'You have insufficient permissions to manage timeline events for this incident' + end + + context 'when error occurs during creation' do + let(:args) { {} } + + it_behaves_like 'error response', "Occurred at can't be blank, Note can't be blank, and Note html can't be blank" + end + + context 'with default action' do + let(:args) { { note: 'note', occurred_at: Time.current, promoted_from_note: comment } } + + it_behaves_like 'success response' + + it 'matches the default action', :aggregate_failures do + result = execute.payload[:timeline_event] + + expect(result.action).to eq(IncidentManagement::TimelineEvents::DEFAULT_ACTION) + end + end + + context 'with non_default action' do + it_behaves_like 'success response' + + it 'matches the action from arguments', :aggregate_failures do + result = execute.payload[:timeline_event] + + expect(result.action).to eq(args[:action]) + end + end + + it 'successfully creates a database record', :aggregate_failures do + expect { execute }.to change { ::IncidentManagement::TimelineEvent.count }.by(1) + end + + context 'when incident_timeline feature flag is enabled' do + before do + stub_feature_flags(incident_timeline: project) + end + + it 'creates a system note' do + expect { execute }.to change { incident.notes.reload.count }.by(1) + end + end + + context 'when incident_timeline feature flag is disabled' do + before do + stub_feature_flags(incident_timeline: false) + end + + it 'does not create a system note' do + expect { execute }.not_to change { incident.notes.reload.count } + end + end + end +end diff --git a/spec/services/incident_management/timeline_events/destroy_service_spec.rb b/spec/services/incident_management/timeline_events/destroy_service_spec.rb new file mode 100644 index 00000000000..01daee2b749 --- /dev/null +++ b/spec/services/incident_management/timeline_events/destroy_service_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IncidentManagement::TimelineEvents::DestroyService do + let_it_be(:user_with_permissions) { create(:user) } + let_it_be(:user_without_permissions) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be_with_refind(:incident) { create(:incident, project: project) } + + let!(:timeline_event) { create(:incident_management_timeline_event, incident: incident, project: project) } + let(:current_user) { user_with_permissions } + let(:params) { {} } + let(:service) { described_class.new(timeline_event, current_user) } + + before_all do + project.add_developer(user_with_permissions) + project.add_reporter(user_without_permissions) + end + + describe '#execute' do + shared_examples 'error response' do |message| + it 'has an informative message' do + expect(execute).to be_error + expect(execute.message).to eq(message) + end + end + + subject(:execute) { service.execute } + + context 'when current user is anonymous' do + let(:current_user) { nil } + + it_behaves_like 'error response', 'You have insufficient permissions to manage timeline events for this incident' + end + + context 'when user does not have permissions to remove timeline events' do + let(:current_user) { user_without_permissions } + + it_behaves_like 'error response', 'You have insufficient permissions to manage timeline events for this incident' + end + + context 'when an error occurs during removal' do + before do + allow(timeline_event).to receive(:destroy).and_return(false) + timeline_event.errors.add(:note, 'cannot be removed') + end + + it_behaves_like 'error response', 'Note cannot be removed' + end + + it 'successfully returns the timeline event', :aggregate_failures do + expect(execute).to be_success + + result = execute.payload[:timeline_event] + expect(result).to be_a(::IncidentManagement::TimelineEvent) + expect(result.id).to eq(timeline_event.id) + end + + context 'when incident_timeline feature flag is enabled' do + before do + stub_feature_flags(incident_timeline: project) + end + + it 'creates a system note' do + expect { execute }.to change { incident.notes.reload.count }.by(1) + end + end + + context 'when incident_timeline feature flag is disabled' do + before do + stub_feature_flags(incident_timeline: false) + end + + it 'does not create a system note' do + expect { execute }.not_to change { incident.notes.reload.count } + end + end + end +end diff --git a/spec/services/incident_management/timeline_events/update_service_spec.rb b/spec/services/incident_management/timeline_events/update_service_spec.rb new file mode 100644 index 00000000000..8bc0e5ce0ed --- /dev/null +++ b/spec/services/incident_management/timeline_events/update_service_spec.rb @@ -0,0 +1,148 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IncidentManagement::TimelineEvents::UpdateService do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:incident) { create(:incident, project: project) } + + let!(:timeline_event) { create(:incident_management_timeline_event, project: project, incident: incident) } + let(:occurred_at) { 1.minute.ago } + let(:params) { { note: 'Updated note', occurred_at: occurred_at } } + + before do + stub_feature_flags(incident_timeline: project) + end + + describe '#execute' do + shared_examples 'successful response' do + it 'responds with success', :aggregate_failures do + expect(execute).to be_success + expect(execute.payload).to eq(timeline_event: timeline_event.reload) + end + end + + shared_examples 'error response' do |message| + it 'has an informative message' do + expect(execute).to be_error + expect(execute.message).to eq(message) + end + end + + shared_examples 'passing the correct was_changed value' do |was_changed| + it 'passes the correct was_changed value into SysteNoteService.edit_timeline_event' do + expect(SystemNoteService) + .to receive(:edit_timeline_event) + .with(timeline_event, user, was_changed: was_changed) + .and_call_original + + execute + end + end + + subject(:execute) { described_class.new(timeline_event, user, params).execute } + + context 'when user has permissions' do + before do + project.add_developer(user) + end + + it_behaves_like 'successful response' + + it 'updates attributes' do + expect { execute }.to change { timeline_event.note }.to(params[:note]) + .and change { timeline_event.occurred_at }.to(params[:occurred_at]) + end + + it 'creates a system note' do + expect { execute }.to change { incident.notes.reload.count }.by(1) + end + + it_behaves_like 'passing the correct was_changed value', :occurred_at_and_note + + context 'when incident_timeline feature flag is disabled' do + before do + stub_feature_flags(incident_timeline: false) + end + + it 'does not add a system note' do + expect { execute }.not_to change { incident.notes } + end + end + + context 'when note is nil' do + let(:params) { { occurred_at: occurred_at } } + + it_behaves_like 'successful response' + it_behaves_like 'passing the correct was_changed value', :occurred_at + + it 'does not update the note' do + expect { execute }.not_to change { timeline_event.reload.note } + end + + it 'updates occurred_at' do + expect { execute }.to change { timeline_event.occurred_at }.to(params[:occurred_at]) + end + end + + context 'when note is blank' do + let(:params) { { note: '', occurred_at: occurred_at } } + + it_behaves_like 'successful response' + it_behaves_like 'passing the correct was_changed value', :occurred_at + + it 'does not update the note' do + expect { execute }.not_to change { timeline_event.reload.note } + end + + it 'updates occurred_at' do + expect { execute }.to change { timeline_event.occurred_at }.to(params[:occurred_at]) + end + end + + context 'when occurred_at is nil' do + let(:params) { { note: 'Updated note' } } + + it_behaves_like 'successful response' + it_behaves_like 'passing the correct was_changed value', :note + + it 'updates the note' do + expect { execute }.to change { timeline_event.note }.to(params[:note]) + end + + it 'does not update occurred_at' do + expect { execute }.not_to change { timeline_event.reload.occurred_at } + end + end + + context 'when both occurred_at and note is nil' do + let(:params) { {} } + + it_behaves_like 'successful response' + + it 'does not update the note' do + expect { execute }.not_to change { timeline_event.note } + end + + it 'does not update occurred_at' do + expect { execute }.not_to change { timeline_event.reload.occurred_at } + end + + it 'does not call SysteNoteService.edit_timeline_event' do + expect(SystemNoteService).not_to receive(:edit_timeline_event) + + execute + end + end + end + + context 'when user does not have permissions' do + before do + project.add_reporter(user) + end + + it_behaves_like 'error response', 'You have insufficient permissions to manage timeline events for this incident' + end + end +end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 1f6118e9fcc..344da5a6582 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -279,7 +279,7 @@ RSpec.describe Issues::CloseService do it 'verifies the number of queries' do recorded = ActiveRecord::QueryRecorder.new { close_issue } - expected_queries = 32 + expected_queries = 30 expect(recorded.count).to be <= expected_queries expect(recorded.cached_count).to eq(0) diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 6b7b72d83fc..3934ca04a00 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -47,6 +47,14 @@ RSpec.describe Issues::CreateService do due_date: Date.tomorrow } end + it 'works if base work item types were not created yet' do + WorkItems::Type.delete_all + + expect do + issue + end.to change(Issue, :count).by(1) + end + it 'creates the issue with the given params' do expect(Issuable::CommonSystemNotesService).to receive_message_chain(:new, :execute) diff --git a/spec/services/issues/set_crm_contacts_service_spec.rb b/spec/services/issues/set_crm_contacts_service_spec.rb index b0befb9f77c..5613cc49cc5 100644 --- a/spec/services/issues/set_crm_contacts_service_spec.rb +++ b/spec/services/issues/set_crm_contacts_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Issues::SetCrmContactsService do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group, :crm_enabled) } - let_it_be(:project) { create(:project, group: create(:group, parent: group)) } + let_it_be(:project) { create(:project, group: create(:group, :crm_enabled, parent: group)) } let_it_be(:contacts) { create_list(:contact, 4, group: group) } let_it_be(:issue, reload: true) { create(:issue, project: project) } let_it_be(:issue_contact_1) do @@ -58,6 +58,20 @@ RSpec.describe Issues::SetCrmContactsService do group.add_reporter(user) end + context 'but the crm setting is disabled' do + let(:params) { { replace_ids: [contacts[1].id, contacts[2].id] } } + let(:subgroup_with_crm_disabled) { create(:group, parent: group) } + let(:project_with_crm_disabled) { create(:project, group: subgroup_with_crm_disabled) } + let(:issue_with_crm_disabled) { create(:issue, project: project_with_crm_disabled) } + + it 'returns expected error response' do + response = described_class.new(project: project_with_crm_disabled, current_user: user, params: params).execute(issue_with_crm_disabled) + + expect(response).to be_error + expect(response.message).to eq('You have insufficient permissions to set customer relations contacts for this issue') + end + end + context 'when the contact does not exist' do let(:params) { { replace_ids: [non_existing_record_id] } } diff --git a/spec/services/jira_connect/sync_service_spec.rb b/spec/services/jira_connect/sync_service_spec.rb index c20aecaaef0..7242b1f41f9 100644 --- a/spec/services/jira_connect/sync_service_spec.rb +++ b/spec/services/jira_connect/sync_service_spec.rb @@ -24,13 +24,15 @@ RSpec.describe JiraConnect::SyncService do end def expect_log(type, message) - expect(Gitlab::ProjectServiceLogger) + expect(Gitlab::IntegrationsLogger) .to receive(type).with( - message: 'response from jira dev_info api', - integration: 'JiraConnect', - project_id: project.id, - project_path: project.full_path, - jira_response: message&.to_json + { + message: 'response from jira dev_info api', + integration: 'JiraConnect', + project_id: project.id, + project_path: project.full_path, + jira_response: message&.to_json + } ) end diff --git a/spec/services/keys/expiry_notification_service_spec.rb b/spec/services/keys/expiry_notification_service_spec.rb index 1d1da179cf7..7cb6cbce311 100644 --- a/spec/services/keys/expiry_notification_service_spec.rb +++ b/spec/services/keys/expiry_notification_service_spec.rb @@ -44,7 +44,7 @@ RSpec.describe Keys::ExpiryNotificationService do end context 'with key expiring today', :mailer do - let_it_be_with_reload(:key) { create(:key, expires_at: Time.current, user: user) } + let_it_be_with_reload(:key) { create(:key, expires_at: 10.minutes.from_now, user: user) } let(:expiring_soon) { false } diff --git a/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb index 538d9638879..735f090d926 100644 --- a/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb +++ b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb @@ -181,16 +181,6 @@ RSpec.describe LooseForeignKeys::BatchCleanerService do end end end - - context 'when the lfk_fair_queueing FF is off' do - before do - stub_feature_flags(lfk_fair_queueing: false) - end - - it 'does nothing' do - expect { cleaner.execute }.not_to change { deleted_record.reload.cleanup_attempts } - end - end end end end diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb index 25437be1e78..730175af0bb 100644 --- a/spec/services/members/create_service_spec.rb +++ b/spec/services/members/create_service_spec.rb @@ -7,11 +7,11 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ let_it_be(:user) { create(:user) } let_it_be(:member) { create(:user) } let_it_be(:user_invited_by_id) { create(:user) } - let_it_be(:user_ids) { member.id.to_s } + let_it_be(:user_id) { member.id.to_s } let_it_be(:access_level) { Gitlab::Access::GUEST } let(:additional_params) { { invite_source: '_invite_source_' } } - let(:params) { { user_ids: user_ids, access_level: access_level }.merge(additional_params) } + let(:params) { { user_id: user_id, access_level: access_level }.merge(additional_params) } let(:current_user) { user } subject(:execute_service) { described_class.new(current_user, params.merge({ source: source })).execute } @@ -51,7 +51,7 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ end context 'when user_id is passed as an integer' do - let(:user_ids) { member.id } + let(:user_id) { member.id } it 'successfully creates member' do expect(execute_service[:status]).to eq(:success) @@ -60,8 +60,8 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ end end - context 'with user_ids as an array of integers' do - let(:user_ids) { [member.id, user_invited_by_id.id] } + context 'with user_id as an array of integers' do + let(:user_id) { [member.id, user_invited_by_id.id] } it 'successfully creates members' do expect(execute_service[:status]).to eq(:success) @@ -70,8 +70,8 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ end end - context 'with user_ids as an array of strings' do - let(:user_ids) { [member.id.to_s, user_invited_by_id.id.to_s] } + context 'with user_id as an array of strings' do + let(:user_id) { [member.id.to_s, user_invited_by_id.id.to_s] } it 'successfully creates members' do expect(execute_service[:status]).to eq(:success) @@ -101,7 +101,7 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ end context 'when passing no user ids' do - let(:user_ids) { '' } + let(:user_id) { '' } it 'does not add a member' do expect(execute_service[:status]).to eq(:error) @@ -112,7 +112,7 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ end context 'when passing many user ids' do - let(:user_ids) { 1.upto(101).to_a.join(',') } + let(:user_id) { 1.upto(101).to_a.join(',') } it 'limits the number of users to 100' do expect(execute_service[:status]).to eq(:error) @@ -134,7 +134,7 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ end context 'when passing an existing invite user id' do - let(:user_ids) { create(:project_member, :invited, project: source).invite_email } + let(:user_id) { create(:project_member, :invited, project: source).invite_email } it 'does not add a member' do expect(execute_service[:status]).to eq(:error) @@ -146,7 +146,7 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ context 'when adding a project_bot' do let_it_be(:project_bot) { create(:user, :project_bot) } - let(:user_ids) { project_bot.id } + let(:user_id) { project_bot.id } context 'when project_bot is already a member' do before do @@ -213,7 +213,7 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ end context 'when it is a net_new_user' do - let(:additional_params) { { invite_source: '_invite_source_', user_ids: 'email@example.org' } } + let(:additional_params) { { invite_source: '_invite_source_', user_id: 'email@example.org' } } it 'tracks the invite source from params' do execute_service @@ -248,8 +248,8 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ ) end - context 'when it is an invite by email passed to user_ids' do - let(:user_ids) { 'email@example.org' } + context 'when it is an invite by email passed to user_id' do + let(:user_id) { 'email@example.org' } it 'does not create task issues' do expect(TasksToBeDone::CreateWorker).not_to receive(:perform_async) @@ -263,7 +263,7 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ end let(:another_user) { create(:user) } - let(:user_ids) { [member.id, another_user.id].join(',') } + let(:user_id) { [member.id, another_user.id].join(',') } it 'still creates 2 task issues', :aggregate_failures do expect(TasksToBeDone::CreateWorker) @@ -326,7 +326,7 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ end context 'when a member was already invited' do - let(:user_ids) { create(:project_member, :invited, project: source).invite_email } + let(:user_id) { create(:project_member, :invited, project: source).invite_email } let(:additional_params) do { invite_source: '_invite_source_', tasks_project_id: source.id, tasks_to_be_done: %w(ci code) } end diff --git a/spec/services/members/groups/creator_service_spec.rb b/spec/services/members/groups/creator_service_spec.rb index 4427c4e7d9f..c3ba7c0374d 100644 --- a/spec/services/members/groups/creator_service_spec.rb +++ b/spec/services/members/groups/creator_service_spec.rb @@ -3,14 +3,28 @@ require 'spec_helper' RSpec.describe Members::Groups::CreatorService do - it_behaves_like 'member creation' do - let_it_be(:source, reload: true) { create(:group, :public) } - let_it_be(:member_type) { GroupMember } - end - describe '.access_levels' do it 'returns Gitlab::Access.options_with_owner' do expect(described_class.access_levels).to eq(Gitlab::Access.sym_options_with_owner) end end + + describe '#execute' do + let_it_be(:source, reload: true) { create(:group, :public) } + let_it_be(:user) { create(:user) } + + it_behaves_like 'member creation' do + let_it_be(:member_type) { GroupMember } + end + + context 'authorized projects update' do + it 'schedules a single project authorization update job when called multiple times' do + expect(AuthorizedProjectsWorker).to receive(:bulk_perform_and_wait).once + + 1.upto(3) do + described_class.new(source, user, :maintainer).execute + end + end + end + end end diff --git a/spec/services/members/invite_service_spec.rb b/spec/services/members/invite_service_spec.rb index ab740138a8b..8213e8baae0 100644 --- a/spec/services/members/invite_service_spec.rb +++ b/spec/services/members/invite_service_spec.rb @@ -52,8 +52,8 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end end - context 'with user_ids as integers' do - let(:params) { { user_ids: [project_user.id, user_invited_by_id.id] } } + context 'with user_id as integers' do + let(:params) { { user_id: [project_user.id, user_invited_by_id.id] } } it 'successfully creates members' do expect_to_create_members(count: 2) @@ -61,8 +61,8 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end end - context 'with user_ids as strings' do - let(:params) { { user_ids: [project_user.id.to_s, user_invited_by_id.id.to_s] } } + context 'with user_id as strings' do + let(:params) { { user_id: [project_user.id.to_s, user_invited_by_id.id.to_s] } } it 'successfully creates members' do expect_to_create_members(count: 2) @@ -70,9 +70,9 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end end - context 'with a mixture of emails and user_ids' do + context 'with a mixture of emails and user_id' do let(:params) do - { user_ids: [project_user.id, user_invited_by_id.id], email: %w[email@example.org email2@example.org] } + { user_id: [project_user.id, user_invited_by_id.id], email: %w[email@example.org email2@example.org] } end it 'successfully creates members' do @@ -92,8 +92,8 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end end - context 'with user_ids' do - let(:params) { { user_ids: "#{project_user.id},#{user_invited_by_id.id}" } } + context 'with user_id' do + let(:params) { { user_id: "#{project_user.id},#{user_invited_by_id.id}" } } it 'successfully creates members' do expect_to_create_members(count: 2) @@ -101,9 +101,9 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end end - context 'with a mixture of emails and user_ids' do + context 'with a mixture of emails and user_id' do let(:params) do - { user_ids: "#{project_user.id},#{user_invited_by_id.id}", email: 'email@example.org,email2@example.org' } + { user_id: "#{project_user.id},#{user_invited_by_id.id}", email: 'email@example.org,email2@example.org' } end it 'successfully creates members' do @@ -114,9 +114,9 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end context 'when invites formats are mixed' do - context 'when user_ids is an array and emails is a string' do + context 'when user_id is an array and emails is a string' do let(:params) do - { user_ids: [project_user.id, user_invited_by_id.id], email: 'email@example.org,email2@example.org' } + { user_id: [project_user.id, user_invited_by_id.id], email: 'email@example.org,email2@example.org' } end it 'successfully creates members' do @@ -125,9 +125,9 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end end - context 'when user_ids is a string and emails is an array' do + context 'when user_id is a string and emails is an array' do let(:params) do - { user_ids: "#{project_user.id},#{user_invited_by_id.id}", email: %w[email@example.org email2@example.org] } + { user_id: "#{project_user.id},#{user_invited_by_id.id}", email: %w[email@example.org email2@example.org] } end it 'successfully creates members' do @@ -147,8 +147,8 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end end - context 'when user_ids are passed as an empty string' do - let(:params) { { user_ids: '' } } + context 'when user_id are passed as an empty string' do + let(:params) { { user_id: '' } } it 'returns an error' do expect_not_to_create_members @@ -156,8 +156,8 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end end - context 'when user_ids and emails are both passed as empty strings' do - let(:params) { { user_ids: '', email: '' } } + context 'when user_id and emails are both passed as empty strings' do + let(:params) { { user_id: '', email: '' } } it 'returns an error' do expect_not_to_create_members @@ -166,7 +166,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end context 'when user_id is passed as an integer' do - let(:params) { { user_ids: project_user.id } } + let(:params) { { user_id: project_user.id } } it 'successfully creates member' do expect_to_create_members(count: 1) @@ -196,7 +196,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end context 'with user_id and singular invalid email' do - let(:params) { { user_ids: project_user.id, email: '_bogus_' } } + let(:params) { { user_id: project_user.id, email: '_bogus_' } } it 'has partial success' do expect_to_create_members(count: 1) @@ -219,7 +219,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end context 'with duplicate user ids' do - let(:params) { { user_ids: "#{project_user.id},#{project_user.id}" } } + let(:params) { { user_id: "#{project_user.id},#{project_user.id}" } } it 'only creates one member per unique invite' do expect_to_create_members(count: 1) @@ -228,7 +228,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end context 'with duplicate member by adding as user id and email' do - let(:params) { { user_ids: project_user.id, email: project_user.email } } + let(:params) { { user_id: project_user.id, email: project_user.email } } it 'only creates one member per unique invite' do expect_to_create_members(count: 1) @@ -269,9 +269,9 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end end - context 'with user_ids' do - let(:user_ids) { 1.upto(101).to_a.join(',') } - let(:params) { { user_ids: user_ids } } + context 'with user_id' do + let(:user_id) { 1.upto(101).to_a.join(',') } + let(:params) { { user_id: user_id } } it 'limits the number of users to 100' do expect_not_to_create_members @@ -292,7 +292,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end context 'with user_id' do - let(:params) { { user_ids: project_user.id } } + let(:params) { { user_id: project_user.id } } it_behaves_like 'records an onboarding progress action', :user_added @@ -304,7 +304,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ context 'when assigning tasks to be done' do let(:params) do - { user_ids: project_user.id, tasks_to_be_done: %w(ci code), tasks_project_id: project.id } + { user_id: project_user.id, tasks_to_be_done: %w(ci code), tasks_project_id: project.id } end it 'creates 2 task issues', :aggregate_failures do @@ -332,7 +332,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end context 'with user_id' do - let(:params) { { user_ids: user_invited_by_id.id, access_level: -1 } } + let(:params) { { user_id: user_invited_by_id.id, access_level: -1 } } it 'returns an error' do expect_not_to_create_members @@ -341,7 +341,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end context 'with a mix of user_id and email' do - let(:params) { { user_ids: user_invited_by_id.id, email: project_user.email, access_level: -1 } } + let(:params) { { user_id: user_invited_by_id.id, email: project_user.email, access_level: -1 } } it 'returns errors' do expect_not_to_create_members @@ -387,7 +387,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ context 'with user_id that already exists' do let!(:existing_member) { create(:project_member, project: project, user: project_user) } - let(:params) { { user_ids: existing_member.user_id } } + let(:params) { { user_id: existing_member.user_id } } it 'does not add the member again and is successful' do expect_to_create_members(count: 0) @@ -397,7 +397,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ context 'with user_id that already exists with a lower access_level' do let!(:existing_member) { create(:project_member, :developer, project: project, user: project_user) } - let(:params) { { user_ids: existing_member.user_id, access_level: ProjectMember::MAINTAINER } } + let(:params) { { user_id: existing_member.user_id, access_level: ProjectMember::MAINTAINER } } it 'does not add the member again and updates the access_level' do expect_to_create_members(count: 0) @@ -408,7 +408,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ context 'with user_id that already exists with a higher access_level' do let!(:existing_member) { create(:project_member, :developer, project: project, user: project_user) } - let(:params) { { user_ids: existing_member.user_id, access_level: ProjectMember::GUEST } } + let(:params) { { user_id: existing_member.user_id, access_level: ProjectMember::GUEST } } it 'does not add the member again and updates the access_level' do expect_to_create_members(count: 0) @@ -428,7 +428,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end context 'when access_level is lower than inheriting member' do - let(:params) { { user_ids: group_member.user_id, access_level: ProjectMember::GUEST }} + let(:params) { { user_id: group_member.user_id, access_level: ProjectMember::GUEST }} it 'does not add the member and returns an error' do msg = "Access level should be greater than or equal " \ @@ -440,7 +440,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end context 'when access_level is the same as the inheriting member' do - let(:params) { { user_ids: group_member.user_id, access_level: ProjectMember::DEVELOPER }} + let(:params) { { user_id: group_member.user_id, access_level: ProjectMember::DEVELOPER }} it 'adds the member with correct access_level' do expect_to_create_members(count: 1) @@ -450,7 +450,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end context 'when access_level is greater than the inheriting member' do - let(:params) { { user_ids: group_member.user_id, access_level: ProjectMember::MAINTAINER }} + let(:params) { { user_id: group_member.user_id, access_level: ProjectMember::MAINTAINER }} it 'adds the member with correct access_level' do expect_to_create_members(count: 1) diff --git a/spec/services/members/projects/creator_service_spec.rb b/spec/services/members/projects/creator_service_spec.rb index 7ba183759bc..7605238c3c5 100644 --- a/spec/services/members/projects/creator_service_spec.rb +++ b/spec/services/members/projects/creator_service_spec.rb @@ -3,14 +3,28 @@ require 'spec_helper' RSpec.describe Members::Projects::CreatorService do - it_behaves_like 'member creation' do - let_it_be(:source, reload: true) { create(:project, :public) } - let_it_be(:member_type) { ProjectMember } - end - describe '.access_levels' do it 'returns Gitlab::Access.sym_options_with_owner' do expect(described_class.access_levels).to eq(Gitlab::Access.sym_options_with_owner) end end + + describe '#execute' do + let_it_be(:source, reload: true) { create(:project, :public) } + let_it_be(:user) { create(:user) } + + it_behaves_like 'member creation' do + let_it_be(:member_type) { ProjectMember } + end + + context 'authorized projects update' do + it 'schedules a single project authorization update job when called multiple times' do + expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to receive(:bulk_perform_in).once + + 1.upto(3) do + described_class.new(source, user, :maintainer).execute + end + end + end + end end diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index 9b064da44b8..e500102a00c 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -41,6 +41,12 @@ RSpec.describe MergeRequests::ApprovalService do end context 'with valid approval' do + let(:notification_service) { NotificationService.new } + + before do + allow(service).to receive(:notification_service).and_return(notification_service) + end + it 'creates an approval note and marks pending todos as done' do expect(SystemNoteService).to receive(:approve_mr).with(merge_request, user) expect(merge_request.approvals).to receive(:reset) @@ -59,9 +65,16 @@ RSpec.describe MergeRequests::ApprovalService do service.execute(merge_request) end + it 'sends a notification when approving' do + expect(notification_service).to receive_message_chain(:async, :approve_mr) + .with(merge_request, user) + + service.execute(merge_request) + end + it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: user, merge_request: merge_request) + .with(project: project, current_user: user, merge_request: merge_request, user: user) .and_call_original service.execute(merge_request) diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index d36a2f75cfe..cd1c362a19f 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -97,7 +97,7 @@ RSpec.describe MergeRequests::CloseService do it 'clean up environments for the merge request' do expect_next_instance_of(::Environments::StopService) do |service| - expect(service).to receive(:execute_for_merge_request).with(merge_request) + expect(service).to receive(:execute_for_merge_request_pipeline).with(merge_request) end described_class.new(project: project, current_user: user).execute(merge_request) diff --git a/spec/services/merge_requests/handle_assignees_change_service_spec.rb b/spec/services/merge_requests/handle_assignees_change_service_spec.rb index 26f53f55b0f..fa3b1614e21 100644 --- a/spec/services/merge_requests/handle_assignees_change_service_spec.rb +++ b/spec/services/merge_requests/handle_assignees_change_service_spec.rb @@ -89,18 +89,12 @@ RSpec.describe MergeRequests::HandleAssigneesChangeService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: user, merge_request: merge_request) + .with(project: project, current_user: user, merge_request: merge_request, user: user) .and_call_original execute end - it 'updates attention requested by of assignee' do - execute - - expect(merge_request.find_assignee(assignee).updated_state_by).to eq(user) - end - it 'tracks users assigned event' do expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) .to receive(:track_users_assigned_to_mr).once.with(users: [assignee]) diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index ecb856bd1a4..78deab64b1c 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -149,7 +149,7 @@ RSpec.describe MergeRequests::MergeService do allow(project).to receive(:default_branch).and_return(merge_request.target_branch) end - it 'closes GitLab issue tracker issues' do + it 'closes GitLab issue tracker issues', :sidekiq_inline do issue = create :issue, project: project commit = double('commit', safe_message: "Fixes #{issue.to_reference}", date: Time.current, authored_date: Time.current) allow(merge_request).to receive(:commits).and_return([commit]) diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index 8d9a32c3e9e..f0885365f96 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -59,24 +59,9 @@ RSpec.describe MergeRequests::PostMergeService do expect(diff_removal_service).to have_received(:execute) end - it 'marks MR as merged regardless of errors when closing issues' do - merge_request.update!(target_branch: 'foo') - allow(project).to receive(:default_branch).and_return('foo') - - issue = create(:issue, project: project) - allow(merge_request).to receive(:visible_closing_issues_for).and_return([issue]) - expect_next_instance_of(Issues::CloseService) do |close_service| - allow(close_service).to receive(:execute).with(issue, commit: merge_request).and_raise(RuntimeError) - end - - expect { subject }.to raise_error(RuntimeError) - - expect(merge_request.reload).to be_merged - end - it 'clean up environments for the merge request' do expect_next_instance_of(::Environments::StopService) do |stop_environment_service| - expect(stop_environment_service).to receive(:execute_for_merge_request).with(merge_request) + expect(stop_environment_service).to receive(:execute_for_merge_request_pipeline).with(merge_request) end subject @@ -88,6 +73,67 @@ RSpec.describe MergeRequests::PostMergeService do subject end + context 'when there are issues to be closed' do + let_it_be(:issue) { create(:issue, project: project) } + + before do + merge_request.update!(target_branch: 'foo') + + allow(project).to receive(:default_branch).and_return('foo') + allow(merge_request).to receive(:visible_closing_issues_for).and_return([issue]) + end + + it 'performs MergeRequests::CloseIssueWorker asynchronously' do + expect(MergeRequests::CloseIssueWorker) + .to receive(:perform_async) + .with(project.id, user.id, issue.id, merge_request.id) + + subject + + expect(merge_request.reload).to be_merged + end + + context 'when issue is an external issue' do + let_it_be(:issue) { ExternalIssue.new('JIRA-123', project) } + + it 'executes Issues::CloseService' do + expect_next_instance_of(Issues::CloseService) do |close_service| + expect(close_service).to receive(:execute).with(issue, commit: merge_request) + end + + subject + + expect(merge_request.reload).to be_merged + end + end + + context 'when async_mr_close_issue feature flag is disabled' do + before do + stub_feature_flags(async_mr_close_issue: false) + end + + it 'executes Issues::CloseService' do + expect_next_instance_of(Issues::CloseService) do |close_service| + expect(close_service).to receive(:execute).with(issue, commit: merge_request) + end + + subject + + expect(merge_request.reload).to be_merged + end + + it 'marks MR as merged regardless of errors when closing issues' do + expect_next_instance_of(Issues::CloseService) do |close_service| + allow(close_service).to receive(:execute).with(issue, commit: merge_request).and_raise(RuntimeError) + end + + expect { subject }.to raise_error(RuntimeError) + + expect(merge_request.reload).to be_merged + end + end + end + context 'when the merge request has review apps' do it 'cancels all review app deployments' do pipeline = create(:ci_pipeline, diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index 348ea9ad7d4..338057f23d5 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -20,7 +20,17 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do let(:source_branch) { 'fix' } let(:target_branch) { 'feature' } let(:title) { 'my title' } + let(:draft_title) { 'Draft: my title' } + let(:draft) { true } let(:description) { 'my description' } + let(:multiline_description) do + <<~MD.chomp + Line 1 + Line 2 + Line 3 + MD + end + let(:label1) { 'mylabel1' } let(:label2) { 'mylabel2' } let(:label3) { 'mylabel3' } @@ -64,6 +74,26 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do end end + shared_examples_for 'a service that can set the multiline description of a merge request' do + subject(:last_mr) { MergeRequest.last } + + it 'sets the multiline description' do + service.execute + + expect(last_mr.description).to eq(multiline_description) + end + end + + shared_examples_for 'a service that can set the draft of a merge request' do + subject(:last_mr) { MergeRequest.last } + + it 'sets the draft' do + service.execute + + expect(last_mr.draft).to eq(draft) + end + end + shared_examples_for 'a service that can set the milestone of a merge request' do subject(:last_mr) { MergeRequest.last } @@ -417,6 +447,74 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it_behaves_like 'a service that can set the description of a merge request' + + context 'with a multiline description' do + let(:push_options) { { description: "Line 1\\nLine 2\\nLine 3" } } + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can set the multiline description of a merge request' + end + end + + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' + end + + describe '`draft` push option' do + let(:push_options) { { draft: draft } } + + context 'with a new branch' do + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + service.execute + + expect(service.errors).to include(error_mr_required) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, draft: draft } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the draft of a merge request' + end + end + + context 'with an existing branch but no open MR' do + let(:changes) { existing_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + service.execute + + expect(service.errors).to include(error_mr_required) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, draft: draft } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the draft of a merge request' + end + end + + context 'with an existing branch that has a merge request open' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can set the draft of a merge request' + end + + context 'draft title provided while `draft` push option is set to false' do + let(:push_options) { { create: true, draft: false, title: draft_title } } + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the draft of a merge request' end it_behaves_like 'with a deleted branch' diff --git a/spec/services/merge_requests/rebase_service_spec.rb b/spec/services/merge_requests/rebase_service_spec.rb index a47e626666b..e7aa6e74246 100644 --- a/spec/services/merge_requests/rebase_service_spec.rb +++ b/spec/services/merge_requests/rebase_service_spec.rb @@ -70,11 +70,13 @@ RSpec.describe MergeRequests::RebaseService do it 'logs the error' do expect(service).to receive(:log_error).with(exception: exception, message: described_class::REBASE_ERROR, save_message_on_model: true).and_call_original expect(Gitlab::ErrorTracking).to receive(:track_exception).with(exception, - class: described_class.to_s, - merge_request: merge_request_ref, - merge_request_id: merge_request.id, - message: described_class::REBASE_ERROR, - save_message_on_model: true).and_call_original + { + class: described_class.to_s, + merge_request: merge_request_ref, + merge_request_id: merge_request.id, + message: described_class::REBASE_ERROR, + save_message_on_model: true + }).and_call_original service.execute(merge_request) end diff --git a/spec/services/merge_requests/remove_approval_service_spec.rb b/spec/services/merge_requests/remove_approval_service_spec.rb index ef6a0ec69bd..5a319e90a68 100644 --- a/spec/services/merge_requests/remove_approval_service_spec.rb +++ b/spec/services/merge_requests/remove_approval_service_spec.rb @@ -21,14 +21,20 @@ RSpec.describe MergeRequests::RemoveApprovalService do context 'with a user who has approved' do let!(:approval) { create(:approval, user: user, merge_request: merge_request) } + let(:notification_service) { NotificationService.new } + + before do + allow(service).to receive(:notification_service).and_return(notification_service) + end it 'removes the approval' do expect { execute! }.to change { merge_request.approvals.size }.from(2).to(1) end - it 'creates an unapproval note and triggers web hook' do + it 'creates an unapproval note, triggers a web hook, and sends a notification' do expect(service).to receive(:execute_hooks).with(merge_request, 'unapproved') expect(SystemNoteService).to receive(:unapprove_mr) + expect(notification_service).to receive_message_chain(:async, :unapprove_mr).with(merge_request, user) execute! end diff --git a/spec/services/merge_requests/remove_attention_requested_service_spec.rb b/spec/services/merge_requests/remove_attention_requested_service_spec.rb index 450204ebfdd..576049b9f1b 100644 --- a/spec/services/merge_requests/remove_attention_requested_service_spec.rb +++ b/spec/services/merge_requests/remove_attention_requested_service_spec.rb @@ -3,64 +3,112 @@ require 'spec_helper' RSpec.describe MergeRequests::RemoveAttentionRequestedService do - let(:current_user) { create(:user) } - let(:merge_request) { create(:merge_request, reviewers: [current_user], assignees: [current_user]) } - let(:reviewer) { merge_request.find_reviewer(current_user) } - let(:assignee) { merge_request.find_assignee(current_user) } + let_it_be(:current_user) { create(:user) } + let_it_be(:user) { create(:user) } + let_it_be(:assignee_user) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, reviewers: [user], assignees: [assignee_user]) } + + let(:reviewer) { merge_request.find_reviewer(user) } + let(:assignee) { merge_request.find_assignee(assignee_user) } let(:project) { merge_request.project } - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) } + + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: user + ) + end + let(:result) { service.execute } before do + allow(SystemNoteService).to receive(:remove_attention_request) + project.add_developer(current_user) + project.add_developer(user) end describe '#execute' do - context 'invalid permissions' do - let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request) } + context 'when current user cannot update merge request' do + let(:service) do + described_class.new( + project: project, + current_user: create(:user), + merge_request: merge_request, + user: user + ) + end it 'returns an error' do expect(result[:status]).to eq :error end end - context 'reviewer does not exist' do - let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request) } + context 'when user is not a reviewer nor assignee' do + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: create(:user) + ) + end it 'returns an error' do expect(result[:status]).to eq :error end end - context 'reviewer exists' do + context 'when user is a reviewer' do + before do + reviewer.update!(state: :attention_requested) + end + it 'returns success' do expect(result[:status]).to eq :success end - it 'updates reviewers state' do + it 'updates reviewer state' do service.execute reviewer.reload expect(reviewer.state).to eq 'reviewed' end + it 'creates a remove attention request system note' do + expect(SystemNoteService) + .to receive(:remove_attention_request) + .with(merge_request, merge_request.project, current_user, user) + + service.execute + end + it_behaves_like 'invalidates attention request cache' do - let(:users) { [current_user] } + let(:users) { [user] } end end - context 'assignee exists' do - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) } + context 'when user is an assignee' do + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: assignee_user + ) + end before do - assignee.update!(state: :reviewed) + assignee.update!(state: :attention_requested) end it 'returns success' do expect(result[:status]).to eq :success end - it 'updates assignees state' do + it 'updates assignee state' do service.execute assignee.reload @@ -68,14 +116,40 @@ RSpec.describe MergeRequests::RemoveAttentionRequestedService do end it_behaves_like 'invalidates attention request cache' do - let(:users) { [current_user] } + let(:users) { [assignee_user] } + end + + it 'creates a remove attention request system note' do + expect(SystemNoteService) + .to receive(:remove_attention_request) + .with(merge_request, merge_request.project, current_user, assignee_user) + + service.execute end end - context 'assignee is the same as reviewer' do - let(:merge_request) { create(:merge_request, reviewers: [current_user], assignees: [current_user]) } - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) } - let(:assignee) { merge_request.find_assignee(current_user) } + context 'when user is an assignee and reviewer at the same time' do + let_it_be(:merge_request) { create(:merge_request, reviewers: [user], assignees: [user]) } + + let(:assignee) { merge_request.find_assignee(user) } + + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: user + ) + end + + before do + reviewer.update!(state: :attention_requested) + assignee.update!(state: :attention_requested) + end + + it 'returns success' do + expect(result[:status]).to eq :success + end it 'updates reviewers and assignees state' do service.execute @@ -86,5 +160,24 @@ RSpec.describe MergeRequests::RemoveAttentionRequestedService do expect(assignee.state).to eq 'reviewed' end end + + context 'when state is already not attention_requested' do + before do + reviewer.update!(state: :reviewed) + end + + it 'does not change state' do + service.execute + reviewer.reload + + expect(reviewer.state).to eq 'reviewed' + end + + it 'does not create a remove attention request system note' do + expect(SystemNoteService).not_to receive(:remove_attention_request) + + service.execute + end + end end end diff --git a/spec/services/merge_requests/request_attention_service_spec.rb b/spec/services/merge_requests/request_attention_service_spec.rb new file mode 100644 index 00000000000..813a8150625 --- /dev/null +++ b/spec/services/merge_requests/request_attention_service_spec.rb @@ -0,0 +1,220 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::RequestAttentionService do + let_it_be(:current_user) { create(:user) } + let_it_be(:user) { create(:user) } + let_it_be(:assignee_user) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, reviewers: [user], assignees: [assignee_user]) } + + let(:reviewer) { merge_request.find_reviewer(user) } + let(:assignee) { merge_request.find_assignee(assignee_user) } + let(:project) { merge_request.project } + + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: user + ) + end + + let(:result) { service.execute } + let(:todo_svc) { instance_double('TodoService') } + let(:notification_svc) { instance_double('NotificationService') } + + before do + allow(service).to receive(:todo_service).and_return(todo_svc) + allow(service).to receive(:notification_service).and_return(notification_svc) + allow(todo_svc).to receive(:create_attention_requested_todo) + allow(notification_svc).to receive_message_chain(:async, :attention_requested_of_merge_request) + allow(SystemNoteService).to receive(:request_attention) + + project.add_developer(current_user) + project.add_developer(user) + end + + describe '#execute' do + context 'when current user cannot update merge request' do + let(:service) do + described_class.new( + project: project, + current_user: create(:user), + merge_request: merge_request, + user: user + ) + end + + it 'returns an error' do + expect(result[:status]).to eq :error + end + end + + context 'when user is not a reviewer nor assignee' do + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: create(:user) + ) + end + + it 'returns an error' do + expect(result[:status]).to eq :error + end + end + + context 'when user is a reviewer' do + before do + reviewer.update!(state: :reviewed) + end + + it 'returns success' do + expect(result[:status]).to eq :success + end + + it 'updates reviewers state' do + service.execute + reviewer.reload + + expect(reviewer.state).to eq 'attention_requested' + end + + it 'adds who toggled attention' do + service.execute + reviewer.reload + + expect(reviewer.updated_state_by).to eq current_user + end + + it 'creates a new todo for the reviewer' do + expect(todo_svc).to receive(:create_attention_requested_todo).with(merge_request, current_user, user) + + service.execute + end + + it 'sends email to reviewer' do + expect(notification_svc) + .to receive_message_chain(:async, :attention_requested_of_merge_request) + .with(merge_request, current_user, user) + + service.execute + end + + it 'removes attention requested state' do + expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) + .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user) + .and_call_original + + service.execute + end + + it_behaves_like 'invalidates attention request cache' do + let(:users) { [user] } + end + end + + context 'when user is an assignee' do + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: assignee_user + ) + end + + before do + assignee.update!(state: :reviewed) + end + + it 'returns success' do + expect(result[:status]).to eq :success + end + + it 'updates assignees state' do + service.execute + assignee.reload + + expect(assignee.state).to eq 'attention_requested' + end + + it 'creates a new todo for the reviewer' do + expect(todo_svc).to receive(:create_attention_requested_todo).with(merge_request, current_user, assignee_user) + + service.execute + end + + it 'creates a request attention system note' do + expect(SystemNoteService) + .to receive(:request_attention) + .with(merge_request, merge_request.project, current_user, assignee_user) + + service.execute + end + + it 'removes attention requested state' do + expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) + .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user) + .and_call_original + + service.execute + end + + it_behaves_like 'invalidates attention request cache' do + let(:users) { [assignee_user] } + end + end + + context 'when user is an assignee and reviewer at the same time' do + let_it_be(:merge_request) { create(:merge_request, reviewers: [user], assignees: [user]) } + + let(:assignee) { merge_request.find_assignee(user) } + + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: user + ) + end + + before do + reviewer.update!(state: :reviewed) + assignee.update!(state: :reviewed) + end + + it 'updates reviewers and assignees state' do + service.execute + reviewer.reload + assignee.reload + + expect(reviewer.state).to eq 'attention_requested' + expect(assignee.state).to eq 'attention_requested' + end + end + + context 'when state is attention_requested' do + before do + reviewer.update!(state: :attention_requested) + end + + it 'does not change state' do + service.execute + reviewer.reload + + expect(reviewer.state).to eq 'attention_requested' + end + + it 'does not create a new todo for the reviewer' do + expect(todo_svc).not_to receive(:create_attention_requested_todo).with(merge_request, current_user, user) + + service.execute + end + end + end +end diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb index 387be8471b5..9210242a11e 100644 --- a/spec/services/merge_requests/squash_service_spec.rb +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -222,11 +222,13 @@ RSpec.describe MergeRequests::SquashService do it 'logs the error' do expect(service).to receive(:log_error).with(exception: exception, message: 'Failed to squash merge request').and_call_original expect(Gitlab::ErrorTracking).to receive(:track_exception).with(exception, - class: described_class.to_s, - merge_request: merge_request_ref, - merge_request_id: merge_request.id, - message: 'Failed to squash merge request', - save_message_on_model: false).and_call_original + { + class: described_class.to_s, + merge_request: merge_request_ref, + merge_request_id: merge_request.id, + message: 'Failed to squash merge request', + save_message_on_model: false + }).and_call_original service.execute end diff --git a/spec/services/merge_requests/toggle_attention_requested_service_spec.rb b/spec/services/merge_requests/toggle_attention_requested_service_spec.rb index dcaac5d2699..20bc536b21e 100644 --- a/spec/services/merge_requests/toggle_attention_requested_service_spec.rb +++ b/spec/services/merge_requests/toggle_attention_requested_service_spec.rb @@ -80,7 +80,7 @@ RSpec.describe MergeRequests::ToggleAttentionRequestedService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: current_user, merge_request: merge_request) + .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user) .and_call_original service.execute @@ -129,7 +129,7 @@ RSpec.describe MergeRequests::ToggleAttentionRequestedService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: current_user, merge_request: merge_request) + .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user) .and_call_original service.execute diff --git a/spec/services/merge_requests/update_assignees_service_spec.rb b/spec/services/merge_requests/update_assignees_service_spec.rb index 3a0b17c2768..f5f6f0ca301 100644 --- a/spec/services/merge_requests/update_assignees_service_spec.rb +++ b/spec/services/merge_requests/update_assignees_service_spec.rb @@ -113,6 +113,49 @@ RSpec.describe MergeRequests::UpdateAssigneesService do expect { service.execute(merge_request) } .to issue_fewer_queries_than { update_service.execute(other_mr) } end + + context 'setting state of assignees' do + before do + stub_feature_flags(mr_attention_requests: false) + end + + it 'does not set state as attention_requested if feature flag is disabled' do + update_merge_request + + expect(merge_request.merge_request_assignees[0].state).not_to eq('attention_requested') + end + + context 'feature flag is enabled for current_user' do + before do + stub_feature_flags(mr_attention_requests: user) + end + + it 'sets state as attention_requested' do + update_merge_request + + expect(merge_request.merge_request_assignees[0].state).to eq('attention_requested') + expect(merge_request.merge_request_assignees[0].updated_state_by).to eq(user) + end + + it 'uses reviewers state if it is same user as new assignee' do + merge_request.reviewers << user2 + + update_merge_request + + expect(merge_request.merge_request_assignees[0].state).to eq('unreviewed') + end + + context 'when assignee_ids matches existing assignee' do + let(:opts) { { assignee_ids: [user3.id] } } + + it 'keeps original assignees state' do + update_merge_request + + expect(merge_request.find_assignee(user3).state).to eq('unreviewed') + end + end + end + end end end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 30095ebeb50..7164ba8fac0 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -328,6 +328,49 @@ RSpec.describe MergeRequests::UpdateService, :mailer do update_merge_request(reviewer_ids: [user.id]) end + + context 'setting state of reviewers' do + before do + stub_feature_flags(mr_attention_requests: false) + end + + it 'does not set state as attention_requested if feature flag is disabled' do + update_merge_request(reviewer_ids: [user.id]) + + expect(merge_request.merge_request_reviewers[0].state).not_to eq('attention_requested') + end + + context 'feature flag is enabled for current_user' do + before do + stub_feature_flags(mr_attention_requests: user) + end + + it 'sets state as attention_requested' do + update_merge_request(reviewer_ids: [user2.id]) + + expect(merge_request.merge_request_reviewers[0].state).to eq('attention_requested') + expect(merge_request.merge_request_reviewers[0].updated_state_by).to eq(user) + end + + it 'keeps original reviewers state' do + merge_request.find_reviewer(user2).update!(state: :unreviewed) + + update_merge_request({ + reviewer_ids: [user2.id] + }) + + expect(merge_request.find_reviewer(user2).state).to eq('unreviewed') + end + + it 'uses reviewers state if it is same user as new assignee' do + merge_request.assignees << user + + update_merge_request(reviewer_ids: [user.id]) + + expect(merge_request.merge_request_reviewers[0].state).to eq('unreviewed') + end + end + end end it 'creates a resource label event' do @@ -1066,6 +1109,53 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end end end + + context 'setting state of assignees' do + before do + stub_feature_flags(mr_attention_requests: false) + end + + it 'does not set state as attention_requested if feature flag is disabled' do + update_merge_request({ + assignee_ids: [user2.id] + }) + + expect(merge_request.merge_request_assignees[0].state).not_to eq('attention_requested') + end + + context 'feature flag is enabled for current_user' do + before do + stub_feature_flags(mr_attention_requests: user) + end + + it 'sets state as attention_requested' do + update_merge_request({ + assignee_ids: [user2.id] + }) + + expect(merge_request.merge_request_assignees[0].state).to eq('attention_requested') + expect(merge_request.merge_request_assignees[0].updated_state_by).to eq(user) + end + + it 'keeps original assignees state' do + update_merge_request({ + assignee_ids: [user3.id] + }) + + expect(merge_request.find_assignee(user3).state).to eq('unreviewed') + end + + it 'uses reviewers state if it is same user as new assignee' do + merge_request.reviewers << user2 + + update_merge_request({ + assignee_ids: [user2.id] + }) + + expect(merge_request.merge_request_assignees[0].state).to eq('unreviewed') + end + end + end end context 'when adding time spent' do diff --git a/spec/services/namespaces/package_settings/update_service_spec.rb b/spec/services/namespaces/package_settings/update_service_spec.rb index 030bc03038e..ed385f1cd7f 100644 --- a/spec/services/namespaces/package_settings/update_service_spec.rb +++ b/spec/services/namespaces/package_settings/update_service_spec.rb @@ -71,7 +71,7 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService do where(:user_role, :shared_examples_name) do :maintainer | 'updating the namespace package setting' - :developer | 'updating the namespace package setting' + :developer | 'denying access to namespace package setting' :reporter | 'denying access to namespace package setting' :guest | 'denying access to namespace package setting' :anonymous | 'denying access to namespace package setting' @@ -91,7 +91,7 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService do where(:user_role, :shared_examples_name) do :maintainer | 'creating the namespace package setting' - :developer | 'creating the namespace package setting' + :developer | 'denying access to namespace package setting' :reporter | 'denying access to namespace package setting' :guest | 'denying access to namespace package setting' :anonymous | 'denying access to namespace package setting' diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index b0410123630..c72a9465f20 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -168,7 +168,6 @@ RSpec.describe Notes::CreateService do before do project_with_repo.add_maintainer(user) end - context 'when eligible to have a note diff file' do let(:new_opts) do opts.merge(in_reply_to_discussion_id: nil, @@ -196,6 +195,39 @@ RSpec.describe Notes::CreateService do described_class.new(project_with_repo, user, new_opts).execute(skip_capture_diff_note_position: true) end end + + it 'does not track ipynb note usage data' do + expect(::Gitlab::UsageDataCounters::IpynbDiffActivityCounter).not_to receive(:note_created) + + described_class.new(project_with_repo, user, new_opts).execute + end + + context 'is ipynb file' do + before do + allow_any_instance_of(::Gitlab::Diff::File).to receive(:ipynb?).and_return(true) + stub_feature_flags(ipynbdiff_notes_tracker: false) + end + + context ':ipynbdiff_notes_tracker is off' do + it 'does not track ipynb note usage data' do + expect(::Gitlab::UsageDataCounters::IpynbDiffActivityCounter).not_to receive(:note_created) + + described_class.new(project_with_repo, user, new_opts).execute + end + end + + context ':ipynbdiff_notes_tracker is on' do + before do + stub_feature_flags(ipynbdiff_notes_tracker: true) + end + + it 'tracks ipynb diff note creation' do + expect(::Gitlab::UsageDataCounters::IpynbDiffActivityCounter).to receive(:note_created) + + described_class.new(project_with_repo, user, new_opts).execute + end + end + end end context 'when DiffNote is a reply' do diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index d2d55c5ab79..743a04eabe6 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -1869,6 +1869,61 @@ RSpec.describe NotificationService, :mailer do let(:notification_trigger) { notification.new_merge_request(merge_request, @u_disabled) } end + describe 'Approvals' do + let(:notification_target) { merge_request } + let(:maintainer) { create(:user) } + + describe '#approve_mr' do + it 'will notify the author, subscribers, and assigned users' do + notification.approve_mr(merge_request, maintainer) + + merge_request.assignees.each { |assignee| should_email(assignee) } + should_email(merge_request.author) + should_email(@u_watcher) + should_email(@u_participant_mentioned) + should_email(@subscribed_participant) + should_email(@subscriber) + should_email(@watcher_and_subscriber) + should_email(@u_guest_watcher) + + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + should_not_email(@u_lazy_participant) + + expect(email_recipients.size).to eq(8) + # assignee, author, @u_watcher, + # @u_participant_mentioned, @subscribed_participant, + # @subscriber, @watcher_and_subscriber, @u_guest_watcher + end + end + + describe '#unapprove_mr' do + it 'will notify the author, subscribers, and assigned users' do + notification.unapprove_mr(merge_request, maintainer) + + merge_request.assignees.each { |assignee| should_email(assignee) } + should_email(merge_request.author) + should_email(@u_watcher) + should_email(@u_participant_mentioned) + should_email(@subscribed_participant) + should_email(@subscriber) + should_email(@watcher_and_subscriber) + should_email(@u_guest_watcher) + + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + should_not_email(@u_lazy_participant) + + expect(email_recipients.size).to eq(8) + # assignee, author, @u_watcher, + # @u_participant_mentioned, @subscribed_participant, + # @subscriber, @watcher_and_subscriber, @u_guest_watcher + end + end + end + context 'participating' do it_behaves_like 'participating by assignee notification' do let(:participant) { create(:user, username: 'user-participant')} @@ -3653,6 +3708,26 @@ RSpec.describe NotificationService, :mailer do end end + describe '#inactive_project_deletion_warning' do + let_it_be(:deletion_date) { Date.current } + let_it_be(:project) { create(:project) } + let_it_be(:maintainer) { create(:user) } + let_it_be(:developer) { create(:user) } + + before do + project.add_maintainer(maintainer) + end + + subject { notification.inactive_project_deletion_warning(project, deletion_date) } + + it "sends email to project owners and maintainers" do + expect { subject }.to have_enqueued_email(project, maintainer, deletion_date, + mail: "inactive_project_deletion_warning_email") + expect { subject }.not_to have_enqueued_email(project, developer, deletion_date, + mail: "inactive_project_deletion_warning_email") + 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/after_import_service_spec.rb b/spec/services/projects/after_import_service_spec.rb deleted file mode 100644 index a16aec891a9..00000000000 --- a/spec/services/projects/after_import_service_spec.rb +++ /dev/null @@ -1,131 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Projects::AfterImportService do - include GitHelpers - - subject { described_class.new(project) } - - let(:project) { create(:project, :repository) } - let(:repository) { project.repository } - let(:sha) { project.commit.sha } - let(:housekeeping_service) { double(:housekeeping_service) } - - describe '#execute' do - before do - allow(Repositories::HousekeepingService) - .to receive(:new).with(project).and_return(housekeeping_service) - - allow(housekeeping_service) - .to receive(:execute).and_yield - - allow(housekeeping_service).to receive(:increment!) - end - - it 'performs housekeeping' do - subject.execute - - expect(housekeeping_service).to have_received(:execute) - end - - context 'with some refs in refs/pull/**/*' do - before do - repository.write_ref('refs/pull/1/head', sha) - repository.write_ref('refs/pull/1/merge', sha) - - subject.execute - end - - it 'removes refs/pull/**/*' do - expect(rugged.references.map(&:name)) - .not_to include(%r{\Arefs/pull/}) - end - end - - Repository::RESERVED_REFS_NAMES.each do |name| - context "with a ref in refs/#{name}/tmp" do - before do - repository.write_ref("refs/#{name}/tmp", sha) - - subject.execute - end - - it "does not remove refs/#{name}/tmp" do - expect(rugged.references.map(&:name)) - .to include("refs/#{name}/tmp") - end - end - end - - context 'when after import action throw non-retriable exception' do - let(:exception) { StandardError.new('after import error') } - - before do - allow(repository) - .to receive(:delete_all_refs_except) - .and_raise(exception) - end - - it 'throws after import error' do - expect { subject.execute }.to raise_exception('after import error') - end - end - - context 'when housekeeping service lease is taken' do - let(:exception) { Repositories::HousekeepingService::LeaseTaken.new } - - it 'logs the error message' do - allow_next_instance_of(Repositories::HousekeepingService) do |instance| - expect(instance).to receive(:execute).and_raise(exception) - end - - expect(Gitlab::Import::Logger).to receive(:info).with( - { - message: 'Project housekeeping failed', - project_full_path: project.full_path, - project_id: project.id, - 'error.message' => exception.to_s - }).and_call_original - - subject.execute - end - end - - context 'when after import action throw retriable exception one time' do - let(:exception) { GRPC::DeadlineExceeded.new } - - before do - expect(repository) - .to receive(:delete_all_refs_except) - .and_raise(exception) - expect(repository) - .to receive(:delete_all_refs_except) - .and_call_original - - subject.execute - end - - it 'removes refs/pull/**/*' do - expect(rugged.references.map(&:name)) - .not_to include(%r{\Arefs/pull/}) - end - - it 'records the failures in the database', :aggregate_failures do - import_failure = ImportFailure.last - - expect(import_failure.source).to eq('delete_all_refs') - expect(import_failure.project_id).to eq(project.id) - expect(import_failure.relation_key).to be_nil - expect(import_failure.relation_index).to be_nil - expect(import_failure.exception_class).to eq('GRPC::DeadlineExceeded') - expect(import_failure.exception_message).to be_present - expect(import_failure.correlation_id_value).not_to be_empty - end - end - - def rugged - rugged_repo(repository) - end - end -end diff --git a/spec/services/projects/android_target_platform_detector_service_spec.rb b/spec/services/projects/android_target_platform_detector_service_spec.rb new file mode 100644 index 00000000000..74fd320bb48 --- /dev/null +++ b/spec/services/projects/android_target_platform_detector_service_spec.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::AndroidTargetPlatformDetectorService do + let_it_be(:project) { build(:project) } + + subject { described_class.new(project).execute } + + before do + allow(Gitlab::FileFinder).to receive(:new) { finder } + end + + context 'when project is not an Android project' do + let(:finder) { instance_double(Gitlab::FileFinder, find: []) } + + it { is_expected.to be_nil } + end + + context 'when project is an Android project' do + let(:finder) { instance_double(Gitlab::FileFinder) } + + before do + query = described_class::MANIFEST_FILE_SEARCH_QUERY + allow(finder).to receive(:find).with(query) { [instance_double(Gitlab::Search::FoundBlob)] } + end + + it { is_expected.to eq :android } + end +end diff --git a/spec/services/projects/batch_open_issues_count_service_spec.rb b/spec/services/projects/batch_open_issues_count_service_spec.rb index 17bd5f7a37b..89a4abbf9c9 100644 --- a/spec/services/projects/batch_open_issues_count_service_spec.rb +++ b/spec/services/projects/batch_open_issues_count_service_spec.rb @@ -5,7 +5,6 @@ require 'spec_helper' RSpec.describe Projects::BatchOpenIssuesCountService do let!(:project_1) { create(:project) } let!(:project_2) { create(:project) } - let!(:banned_user) { create(:user, :banned) } let(:subject) { described_class.new([project_1, project_2]) } @@ -13,41 +12,32 @@ RSpec.describe Projects::BatchOpenIssuesCountService do before do create(:issue, project: project_1) create(:issue, project: project_1, confidential: true) - create(:issue, project: project_1, author: banned_user) + create(:issue, project: project_2) create(:issue, project: project_2, confidential: true) - create(:issue, project: project_2, author: banned_user) end - context 'when cache is clean', :aggregate_failures do + context 'when cache is clean' do it 'refreshes cache keys correctly' do - expect(get_cache_key(project_1)).to eq(nil) - expect(get_cache_key(project_2)).to eq(nil) - - subject.count_service.new(project_1).refresh_cache - subject.count_service.new(project_2).refresh_cache - - expect(get_cache_key(project_1)).to eq(1) - expect(get_cache_key(project_2)).to eq(1) + subject.refresh_cache_and_retrieve_data - expect(get_cache_key(project_1, true)).to eq(2) - expect(get_cache_key(project_2, true)).to eq(2) + # It does not update total issues cache + expect(Rails.cache.read(get_cache_key(subject, project_1))).to eq(nil) + expect(Rails.cache.read(get_cache_key(subject, project_2))).to eq(nil) - expect(get_cache_key(project_1, true, true)).to eq(3) - expect(get_cache_key(project_2, true, true)).to eq(3) + expect(Rails.cache.read(get_cache_key(subject, project_1, true))).to eq(1) + expect(Rails.cache.read(get_cache_key(subject, project_1, true))).to eq(1) end end end - def get_cache_key(project, with_confidential = false, with_hidden = false) + def get_cache_key(subject, project, public_key = false) service = subject.count_service.new(project) - if with_confidential && with_hidden - Rails.cache.read(service.cache_key(service.class::TOTAL_COUNT_KEY)) - elsif with_confidential - Rails.cache.read(service.cache_key(service.class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY)) + if public_key + service.cache_key(service.class::PUBLIC_COUNT_KEY) else - Rails.cache.read(service.cache_key(service.class::PUBLIC_COUNT_WITHOUT_HIDDEN_KEY)) + service.cache_key(service.class::TOTAL_COUNT_KEY) end end end diff --git a/spec/services/projects/blame_service_spec.rb b/spec/services/projects/blame_service_spec.rb new file mode 100644 index 00000000000..40b2bc869dc --- /dev/null +++ b/spec/services/projects/blame_service_spec.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::BlameService, :aggregate_failures do + subject(:service) { described_class.new(blob, commit, params) } + + let_it_be(:project) { create(:project, :repository) } + let_it_be(:commit) { project.repository.commit } + let_it_be(:blob) { project.repository.blob_at('HEAD', 'README.md') } + + let(:params) { { page: page } } + let(:page) { nil } + + before do + stub_const("#{described_class.name}::PER_PAGE", 2) + end + + describe '#blame' do + subject { service.blame } + + it 'returns a correct Gitlab::Blame object' do + is_expected.to be_kind_of(Gitlab::Blame) + + expect(subject.blob).to eq(blob) + expect(subject.commit).to eq(commit) + expect(subject.range).to eq(1..2) + end + + describe 'Pagination range calculation' do + subject { service.blame.range } + + context 'with page = 1' do + let(:page) { 1 } + + it { is_expected.to eq(1..2) } + end + + context 'with page = 2' do + let(:page) { 2 } + + it { is_expected.to eq(3..4) } + end + + context 'with page = 3 (overlimit)' do + let(:page) { 3 } + + it { is_expected.to eq(1..2) } + end + + context 'with page = 0 (incorrect)' do + let(:page) { 0 } + + it { is_expected.to eq(1..2) } + end + + context 'when feature flag disabled' do + before do + stub_feature_flags(blame_page_pagination: false) + end + + it { is_expected.to be_nil } + end + end + end + + describe '#pagination' do + subject { service.pagination } + + it 'returns a pagination object' do + is_expected.to be_kind_of(Kaminari::PaginatableArray) + + expect(subject.current_page).to eq(1) + expect(subject.total_pages).to eq(2) + expect(subject.total_count).to eq(4) + end + + context 'when feature flag disabled' do + before do + stub_feature_flags(blame_page_pagination: false) + end + + it { is_expected.to be_nil } + end + + context 'when per_page is above the global max per page limit' do + before do + stub_const("#{described_class.name}::PER_PAGE", 1000) + allow(blob).to receive_message_chain(:data, :lines, :count) { 500 } + end + + it 'returns a correct pagination object' do + is_expected.to be_kind_of(Kaminari::PaginatableArray) + + expect(subject.current_page).to eq(1) + expect(subject.total_pages).to eq(1) + expect(subject.total_count).to eq(500) + end + end + + describe 'Current page' do + subject { service.pagination.current_page } + + context 'with page = 1' do + let(:page) { 1 } + + it { is_expected.to eq(1) } + end + + context 'with page = 2' do + let(:page) { 2 } + + it { is_expected.to eq(2) } + end + + context 'with page = 3 (overlimit)' do + let(:page) { 3 } + + it { is_expected.to eq(1) } + end + + context 'with page = 0 (incorrect)' do + let(:page) { 0 } + + it { is_expected.to eq(1) } + end + end + end +end 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 86c0ba4222c..79904e2bf72 100644 --- a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb @@ -34,8 +34,6 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_ stub_digest_config('sha256:configB', 5.days.ago) stub_digest_config('sha256:configC', 1.month.ago) stub_digest_config('sha256:configD', nil) - - stub_feature_flags(container_registry_expiration_policies_throttling: false) end describe '#execute' do @@ -334,24 +332,17 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_ end end - where(:feature_flag_enabled, :max_list_size, :delete_tags_service_status, :expected_status, :expected_truncated) do - false | 10 | :success | :success | false - false | 10 | :error | :error | false - false | 3 | :success | :success | false - false | 3 | :error | :error | false - false | 0 | :success | :success | false - false | 0 | :error | :error | false - true | 10 | :success | :success | false - true | 10 | :error | :error | false - true | 3 | :success | :error | true - true | 3 | :error | :error | true - true | 0 | :success | :success | false - true | 0 | :error | :error | false + where(:max_list_size, :delete_tags_service_status, :expected_status, :expected_truncated) do + 10 | :success | :success | false + 10 | :error | :error | false + 3 | :success | :error | true + 3 | :error | :error | true + 0 | :success | :success | false + 0 | :error | :error | false end with_them do before do - stub_feature_flags(container_registry_expiration_policies_throttling: feature_flag_enabled) stub_application_setting(container_registry_cleanup_tags_service_max_list_size: max_list_size) allow_next_instance_of(Projects::ContainerRepository::DeleteTagsService) do |service| expect(service).to receive(:execute).and_return(status: delete_tags_service_status) @@ -429,10 +420,10 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_ end # We will ping the container registry for all tags *except* for C because it's cached - expect(ContainerRegistry::Blob).to receive(:new).with(repository, "digest" => "sha256:configA").and_call_original - expect(ContainerRegistry::Blob).to receive(:new).with(repository, "digest" => "sha256:configB").twice.and_call_original - expect(ContainerRegistry::Blob).not_to receive(:new).with(repository, "digest" => "sha256:configC") - expect(ContainerRegistry::Blob).to receive(:new).with(repository, "digest" => "sha256:configD").and_call_original + expect(ContainerRegistry::Blob).to receive(:new).with(repository, { "digest" => "sha256:configA" }).and_call_original + expect(ContainerRegistry::Blob).to receive(:new).with(repository, { "digest" => "sha256:configB" }).twice.and_call_original + expect(ContainerRegistry::Blob).not_to receive(:new).with(repository, { "digest" => "sha256:configC" }) + expect(ContainerRegistry::Blob).to receive(:new).with(repository, { "digest" => "sha256:configD" }).and_call_original expect(subject).to include(cached_tags_count: 1) end diff --git a/spec/services/projects/container_repository/delete_tags_service_spec.rb b/spec/services/projects/container_repository/delete_tags_service_spec.rb index 246ca301cfa..9e6849aa514 100644 --- a/spec/services/projects/container_repository/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/delete_tags_service_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' RSpec.describe Projects::ContainerRepository::DeleteTagsService do + using RSpec::Parameterized::TableSyntax include_context 'container repository delete tags service shared context' let(:service) { described_class.new(project, user, params) } @@ -17,11 +18,13 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do shared_examples 'logging a success response' do it 'logs an info message' do expect(service).to receive(:log_info).with( - service_class: 'Projects::ContainerRepository::DeleteTagsService', - message: 'deleted tags', - container_repository_id: repository.id, - project_id: repository.project_id, - deleted_tags_count: tags.size + { + service_class: 'Projects::ContainerRepository::DeleteTagsService', + message: 'deleted tags', + container_repository_id: repository.id, + project_id: repository.project_id, + deleted_tags_count: tags.size + } ) subject @@ -131,10 +134,6 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do subject { service.execute(repository) } - before do - stub_feature_flags(container_registry_expiration_policies_throttling: false) - end - context 'without permissions' do it { is_expected.to include(status: :error) } end @@ -157,11 +156,39 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do end context 'when the repository is importing' do - before do - repository.update_columns(migration_state: 'importing', migration_import_started_at: Time.zone.now) + where(:migration_state, :called_by_policy, :error_expected) do + 'default' | false | false + 'default' | true | false + 'pre_importing' | false | false + 'pre_importing' | true | true + 'pre_import_done' | false | false + 'pre_import_done' | true | true + 'importing' | false | true + 'importing' | true | true + 'import_done' | false | false + 'import_done' | true | false + 'import_aborted' | false | false + 'import_aborted' | true | false + 'import_skipped' | false | false + 'import_skipped' | true | false end - it { is_expected.to include(status: :error, message: 'repository importing') } + with_them do + let(:params) { { tags: tags, container_expiration_policy: called_by_policy ? true : nil } } + + before do + repository.update_columns(migration_state: migration_state, migration_import_started_at: Time.zone.now, migration_pre_import_started_at: Time.zone.now, migration_pre_import_done_at: Time.zone.now) + end + + it 'returns an error response if expected' do + if error_expected + expect(subject).to include(status: :error, message: 'repository importing') + else + expect(service).to receive(:delete_tags).and_return(status: :success) + expect(subject).not_to include(status: :error) + end + end + end end end diff --git a/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb b/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb index 74f782538c5..8d8907119f0 100644 --- a/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb @@ -12,10 +12,6 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do subject { service.execute } - before do - stub_feature_flags(container_registry_expiration_policies_throttling: false) - end - RSpec.shared_examples 'deleting tags' do it 'deletes the tags by name' do stub_delete_reference_requests(tags) @@ -26,6 +22,8 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do end context 'with tags to delete' do + let(:timeout) { 10 } + it_behaves_like 'deleting tags' it 'succeeds when tag delete returns 404' do @@ -50,59 +48,52 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do end end - context 'with throttling enabled' do - let(:timeout) { 10 } - - before do - stub_feature_flags(container_registry_expiration_policies_throttling: true) - stub_application_setting(container_registry_delete_tags_service_timeout: timeout) - end - - it_behaves_like 'deleting tags' + before do + stub_application_setting(container_registry_delete_tags_service_timeout: timeout) + end - context 'with timeout' do - context 'set to a valid value' do - before do - allow(Time.zone).to receive(:now).and_return(10, 15, 25) # third call to Time.zone.now will be triggering the timeout - stub_delete_reference_requests('A' => 200) - end + context 'with timeout' do + context 'set to a valid value' do + before do + allow(Time.zone).to receive(:now).and_return(10, 15, 25) # third call to Time.zone.now will be triggering the timeout + stub_delete_reference_requests('A' => 200) + end - it { is_expected.to eq(status: :error, message: 'error while deleting tags', deleted: ['A'], exception_class_name: Projects::ContainerRepository::Gitlab::DeleteTagsService::TimeoutError.name) } + it { is_expected.to eq(status: :error, message: 'error while deleting tags', deleted: ['A'], exception_class_name: Projects::ContainerRepository::Gitlab::DeleteTagsService::TimeoutError.name) } - it 'tracks the exception' do - expect(::Gitlab::ErrorTracking) - .to receive(:track_exception).with(::Projects::ContainerRepository::Gitlab::DeleteTagsService::TimeoutError, tags_count: tags.size, container_repository_id: repository.id) + it 'tracks the exception' do + expect(::Gitlab::ErrorTracking) + .to receive(:track_exception).with(::Projects::ContainerRepository::Gitlab::DeleteTagsService::TimeoutError, tags_count: tags.size, container_repository_id: repository.id) - subject - end + subject end + end - context 'set to 0' do - let(:timeout) { 0 } + context 'set to 0' do + let(:timeout) { 0 } - it_behaves_like 'deleting tags' - end + it_behaves_like 'deleting tags' + end - context 'set to nil' do - let(:timeout) { nil } + context 'set to nil' do + let(:timeout) { nil } - it_behaves_like 'deleting tags' - end + it_behaves_like 'deleting tags' end + end - context 'with a network error' do - before do - expect(service).to receive(:delete_tags).and_raise(::Faraday::TimeoutError) - end + context 'with a network error' do + before do + expect(service).to receive(:delete_tags).and_raise(::Faraday::TimeoutError) + end - it { is_expected.to eq(status: :error, message: 'error while deleting tags', deleted: [], exception_class_name: ::Faraday::TimeoutError.name) } + it { is_expected.to eq(status: :error, message: 'error while deleting tags', deleted: [], exception_class_name: ::Faraday::TimeoutError.name) } - it 'tracks the exception' do - expect(::Gitlab::ErrorTracking) - .to receive(:track_exception).with(::Faraday::TimeoutError, tags_count: tags.size, container_repository_id: repository.id) + it 'tracks the exception' do + expect(::Gitlab::ErrorTracking) + .to receive(:track_exception).with(::Faraday::TimeoutError, tags_count: tags.size, container_repository_id: repository.id) - subject - end + subject end end end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index c5c5af3cb01..cd1e629e1d2 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -141,18 +141,6 @@ RSpec.describe Projects::CreateService, '#execute' do expect(project.project_setting).to be_persisted end - context 'create_project_settings feature flag is disabled' do - before do - stub_feature_flags(create_project_settings: false) - end - - it 'builds associated project settings' do - project = create_project(user, opts) - - expect(project.project_setting).to be_new_record - end - end - it_behaves_like 'storing arguments in the application context' do let(:expected_params) { { project: subject.full_path } } @@ -780,6 +768,45 @@ RSpec.describe Projects::CreateService, '#execute' do create_project(user, opts) end + context 'when import source is enabled' do + before do + stub_application_setting(import_sources: ['github']) + end + + it 'does not raise an error when import_source is string' do + opts[:import_type] = 'github' + + project = create_project(user, opts) + + expect(project).to be_persisted + expect(project.errors).to be_blank + end + + it 'does not raise an error when import_source is symbol' do + opts[:import_type] = :github + + project = create_project(user, opts) + + expect(project).to be_persisted + expect(project.errors).to be_blank + end + end + + context 'when import source is disabled' do + before do + stub_application_setting(import_sources: []) + opts[:import_type] = 'git' + end + + it 'raises an error' do + project = create_project(user, opts) + + expect(project).to respond_to(:errors) + expect(project.errors).to have_key(:import_source_disabled) + expect(project.saved?).to be_falsey + end + end + context 'with external authorization enabled' do before do enable_external_authorization_service_check @@ -797,7 +824,7 @@ RSpec.describe Projects::CreateService, '#execute' do it 'saves the project when the user has access to the label' do expect(::Gitlab::ExternalAuthorization) - .to receive(:access_allowed?).with(user, 'new-label', any_args) { true } + .to receive(:access_allowed?).with(user, 'new-label', any_args) { true }.at_least(1).time project = create_project(user, opts.merge({ external_authorization_classification_label: 'new-label' })) diff --git a/spec/services/projects/group_links/create_service_spec.rb b/spec/services/projects/group_links/create_service_spec.rb index 4ea5f2b3a53..65d3085a850 100644 --- a/spec/services/projects/group_links/create_service_spec.rb +++ b/spec/services/projects/group_links/create_service_spec.rb @@ -5,65 +5,104 @@ require 'spec_helper' RSpec.describe Projects::GroupLinks::CreateService, '#execute' do let_it_be(:user) { create :user } let_it_be(:group) { create :group } - let_it_be(:project) { create :project } + let_it_be(:project) { create(:project, namespace: create(:namespace, :with_namespace_settings)) } - let(:group_access) { Gitlab::Access::DEVELOPER } let(:opts) do { - link_group_access: group_access, + link_group_access: Gitlab::Access::DEVELOPER, expires_at: nil } end - subject { described_class.new(project, user, opts) } + subject { described_class.new(project, group, user, opts) } - before do - group.add_developer(user) - end + shared_examples_for 'not shareable' do + it 'does not share and returns an error' do + expect do + result = subject.execute - it 'adds group to project' do - expect { subject.execute(group) }.to change { project.project_group_links.count }.from(0).to(1) + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(404) + end.not_to change { project.project_group_links.count } + end end - it 'updates authorization', :sidekiq_inline do - expect { subject.execute(group) }.to( - change { Ability.allowed?(user, :read_project, project) } - .from(false).to(true)) - end + shared_examples_for 'shareable' do + it 'adds group to project' do + expect do + result = subject.execute - it 'returns false if group is blank' do - expect { subject.execute(nil) }.not_to change { project.project_group_links.count } + expect(result[:status]).to eq(:success) + end.to change { project.project_group_links.count }.from(0).to(1) + end end - it 'returns error if user is not allowed to share with a group' do - expect { subject.execute(create(:group)) }.not_to change { project.project_group_links.count } - end + context 'when user has proper membership to share a group' do + before do + group.add_guest(user) + end - context 'with specialized project_authorization workers' do - let_it_be(:other_user) { create(:user) } + it_behaves_like 'shareable' - before do - group.add_developer(other_user) + it 'updates authorization', :sidekiq_inline do + expect { subject.execute }.to( + change { Ability.allowed?(user, :read_project, project) } + .from(false).to(true)) + end + + context 'with specialized project_authorization workers' do + let_it_be(:other_user) { create(:user) } + + before do + group.add_developer(other_user) + end + + it 'schedules authorization update for users with access to group' do + expect(AuthorizedProjectsWorker).not_to( + receive(:bulk_perform_async) + ) + expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).to( + receive(:perform_async) + .with(project.id) + .and_call_original + ) + expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( + receive(:bulk_perform_in) + .with(1.hour, + array_including([user.id], [other_user.id]), + batch_delay: 30.seconds, batch_size: 100) + .and_call_original + ) + + subject.execute + end end - it 'schedules authorization update for users with access to group' do - expect(AuthorizedProjectsWorker).not_to( - receive(:bulk_perform_async) - ) - expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).to( - receive(:perform_async) - .with(project.id) - .and_call_original - ) - expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( - receive(:bulk_perform_in) - .with(1.hour, - array_including([user.id], [other_user.id]), - batch_delay: 30.seconds, batch_size: 100) - .and_call_original - ) - - subject.execute(group) + context 'when sharing outside the hierarchy is disabled' do + let_it_be(:shared_group_parent) do + create(:group, + namespace_settings: create(:namespace_settings, prevent_sharing_groups_outside_hierarchy: true)) + end + + let_it_be(:project, reload: true) { create(:project, group: shared_group_parent) } + + it_behaves_like 'not shareable' + + context 'when group is inside hierarchy' do + let(:group) { create(:group, :private, parent: shared_group_parent) } + + it_behaves_like 'shareable' + end end end + + context 'when user does not have permissions for the group' do + it_behaves_like 'not shareable' + end + + context 'when group is blank' do + let(:group) { nil } + + it_behaves_like 'not shareable' + end end diff --git a/spec/services/projects/in_product_marketing_campaign_emails_service_spec.rb b/spec/services/projects/in_product_marketing_campaign_emails_service_spec.rb new file mode 100644 index 00000000000..4c51c8a4ac8 --- /dev/null +++ b/spec/services/projects/in_product_marketing_campaign_emails_service_spec.rb @@ -0,0 +1,130 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::InProductMarketingCampaignEmailsService do + describe '#execute' do + let(:user) { create(:user, email_opted_in: true) } + let(:project) { create(:project) } + let(:campaign) { Users::InProductMarketingEmail::BUILD_IOS_APP_GUIDE } + + subject(:execute) do + described_class.new(project, campaign).execute + end + + context 'users can receive marketing emails' do + let(:owner) { create(:user, email_opted_in: true) } + let(:maintainer) { create(:user, email_opted_in: true) } + let(:developer) { create(:user, email_opted_in: true) } + + before do + project.add_owner(owner) + project.add_developer(developer) + project.add_maintainer(maintainer) + end + + it 'sends the email to all project members with access_level >= Developer', :aggregate_failures do + double = instance_double(ActionMailer::MessageDelivery, deliver_later: true) + + [owner, maintainer, developer].each do |user| + email = user.notification_email_or_default + + expect(Notify).to receive(:build_ios_app_guide_email).with(email) { double } + expect(double).to receive(:deliver_later) + end + + execute + end + + it 'records sent emails', :aggregate_failures do + expect { execute }.to change { Users::InProductMarketingEmail.count }.from(0).to(3) + + [owner, maintainer, developer].each do |user| + expect( + Users::InProductMarketingEmail.where( + user: user, + campaign: campaign + ) + ).to exist + end + end + + it 'tracks experiment :email_sent event', :experiment do + expect(experiment(:build_ios_app_guide_email)).to track(:email_sent) + .on_next_instance + .with_context(project: project) + + execute + end + end + + shared_examples 'does nothing' do + it 'does not send the email' do + email = user.notification_email_or_default + expect(Notify).not_to receive(:build_ios_app_guide_email).with(email) + execute + end + + it 'does not create a record of the sent email' do + expect { execute }.not_to change { Users::InProductMarketingEmail.count } + end + end + + context "when user can't receive marketing emails" do + before do + project.add_developer(user) + end + + context 'when user.can?(:receive_notifications) is false' do + it 'does not send the email' do + allow_next_found_instance_of(User) do |user| + allow(user).to receive(:can?).with(:receive_notifications) { false } + + email = user.notification_email_or_default + expect(Notify).not_to receive(:build_ios_app_guide_email).with(email) + + expect( + Users::InProductMarketingEmail.where( + user: user, + campaign: campaign + ) + ).not_to exist + end + + execute + end + end + + context 'when user is not opted in to receive marketing emails' do + let(:user) { create(:user, email_opted_in: false) } + + it_behaves_like 'does nothing' + end + end + + context 'when campaign email has already been sent to the user' do + before do + project.add_developer(user) + create(:in_product_marketing_email, :campaign, user: user, campaign: campaign) + end + + it_behaves_like 'does nothing' + end + + context "when user is a reporter" do + before do + project.add_reporter(user) + end + + it_behaves_like 'does nothing' + end + + context "when user is a guest" do + before do + project.add_guest(user) + end + + it_behaves_like 'does nothing' + end + end +end diff --git a/spec/services/projects/open_issues_count_service_spec.rb b/spec/services/projects/open_issues_count_service_spec.rb index 8710d0c0267..c739fea5ecf 100644 --- a/spec/services/projects/open_issues_count_service_spec.rb +++ b/spec/services/projects/open_issues_count_service_spec.rb @@ -4,102 +4,89 @@ require 'spec_helper' RSpec.describe Projects::OpenIssuesCountService, :use_clean_rails_memory_store_caching do let(:project) { create(:project) } - let(:user) { create(:user) } - let(:banned_user) { create(:user, :banned) } - subject { described_class.new(project, user) } + subject { described_class.new(project) } it_behaves_like 'a counter caching service' - before do - create(:issue, :opened, project: project) - create(:issue, :opened, confidential: true, project: project) - create(:issue, :opened, author: banned_user, project: project) - create(:issue, :closed, project: project) - - described_class.new(project).refresh_cache - end - describe '#count' do - shared_examples 'counts public issues, does not count hidden or confidential' do - it 'counts only public issues' do - expect(subject.count).to eq(1) - end - - it 'uses PUBLIC_COUNT_WITHOUT_HIDDEN_KEY cache key' do - expect(subject.cache_key).to include('project_open_public_issues_without_hidden_count') - end - end - context 'when user is nil' do - let(:user) { nil } + it 'does not include confidential issues in the issue count' do + create(:issue, :opened, project: project) + create(:issue, :opened, confidential: true, project: project) - it_behaves_like 'counts public issues, does not count hidden or confidential' + expect(described_class.new(project).count).to eq(1) + end end context 'when user is provided' do + let(:user) { create(:user) } + context 'when user can read confidential issues' do before do project.add_reporter(user) end - it 'includes confidential issues and does not include hidden issues in count' do - expect(subject.count).to eq(2) + it 'returns the right count with confidential issues' do + create(:issue, :opened, project: project) + create(:issue, :opened, confidential: true, project: project) + + expect(described_class.new(project, user).count).to eq(2) end - it 'uses TOTAL_COUNT_WITHOUT_HIDDEN_KEY cache key' do - expect(subject.cache_key).to include('project_open_issues_without_hidden_count') + it 'uses total_open_issues_count cache key' do + expect(described_class.new(project, user).cache_key_name).to eq('total_open_issues_count') end end - context 'when user cannot read confidential or hidden issues' do + context 'when user cannot read confidential issues' do before do project.add_guest(user) end - it_behaves_like 'counts public issues, does not count hidden or confidential' - end - - context 'when user is an admin' do - let_it_be(:user) { create(:user, :admin) } - - context 'when admin mode is enabled', :enable_admin_mode do - it 'includes confidential and hidden issues in count' do - expect(subject.count).to eq(3) - end + it 'does not include confidential issues' do + create(:issue, :opened, project: project) + create(:issue, :opened, confidential: true, project: project) - it 'uses TOTAL_COUNT_KEY cache key' do - expect(subject.cache_key).to include('project_open_issues_including_hidden_count') - end + expect(described_class.new(project, user).count).to eq(1) end - context 'when admin mode is disabled' do - it_behaves_like 'counts public issues, does not count hidden or confidential' + it 'uses public_open_issues_count cache key' do + expect(described_class.new(project, user).cache_key_name).to eq('public_open_issues_count') end end end - end - - describe '#refresh_cache', :aggregate_failures do - context 'when cache is empty' do - it 'refreshes cache keys correctly' do - expect(Rails.cache.read(described_class.new(project).cache_key(described_class::PUBLIC_COUNT_WITHOUT_HIDDEN_KEY))).to eq(1) - expect(Rails.cache.read(described_class.new(project).cache_key(described_class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY))).to eq(2) - expect(Rails.cache.read(described_class.new(project).cache_key(described_class::TOTAL_COUNT_KEY))).to eq(3) - end - end - context 'when cache is outdated' do - it 'refreshes cache keys correctly' do + describe '#refresh_cache' do + before do + create(:issue, :opened, project: project) create(:issue, :opened, project: project) create(:issue, :opened, confidential: true, project: project) - create(:issue, :opened, author: banned_user, project: project) + end - described_class.new(project).refresh_cache + context 'when cache is empty' do + it 'refreshes cache keys correctly' do + subject.refresh_cache - expect(Rails.cache.read(described_class.new(project).cache_key(described_class::PUBLIC_COUNT_WITHOUT_HIDDEN_KEY))).to eq(2) - expect(Rails.cache.read(described_class.new(project).cache_key(described_class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY))).to eq(4) - expect(Rails.cache.read(described_class.new(project).cache_key(described_class::TOTAL_COUNT_KEY))).to eq(6) + expect(Rails.cache.read(subject.cache_key(described_class::PUBLIC_COUNT_KEY))).to eq(2) + expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_KEY))).to eq(3) + end + end + + context 'when cache is outdated' do + before do + subject.refresh_cache + end + + it 'refreshes cache keys correctly' do + create(:issue, :opened, project: project) + create(:issue, :opened, confidential: true, project: project) + + subject.refresh_cache + + expect(Rails.cache.read(subject.cache_key(described_class::PUBLIC_COUNT_KEY))).to eq(3) + expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_KEY))).to eq(5) + end end end end diff --git a/spec/services/projects/prometheus/alerts/create_service_spec.rb b/spec/services/projects/prometheus/alerts/create_service_spec.rb deleted file mode 100644 index 6b9d43e4e81..00000000000 --- a/spec/services/projects/prometheus/alerts/create_service_spec.rb +++ /dev/null @@ -1,52 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Projects::Prometheus::Alerts::CreateService do - let_it_be(:project) { create(:project) } - let_it_be(:user) { create(:user) } - - let(:service) { described_class.new(project: project, current_user: user, params: params) } - - subject { service.execute } - - describe '#execute' do - context 'with params' do - let_it_be(:environment) { create(:environment, project: project) } - - let_it_be(:metric) do - create(:prometheus_metric, project: project) - end - - let(:params) do - { - environment_id: environment.id, - prometheus_metric_id: metric.id, - operator: '<', - threshold: 1.0 - } - end - - it 'creates an alert' do - expect(subject).to be_persisted - - expect(subject).to have_attributes( - project: project, - environment: environment, - prometheus_metric: metric, - operator: 'lt', - threshold: 1.0 - ) - end - end - - context 'without params' do - let(:params) { {} } - - it 'fails to create' do - expect(subject).to be_new_record - expect(subject).to be_invalid - end - end - end -end diff --git a/spec/services/projects/prometheus/alerts/destroy_service_spec.rb b/spec/services/projects/prometheus/alerts/destroy_service_spec.rb deleted file mode 100644 index a3e9c3516c2..00000000000 --- a/spec/services/projects/prometheus/alerts/destroy_service_spec.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Projects::Prometheus::Alerts::DestroyService do - let_it_be(:project) { create(:project) } - let_it_be(:user) { create(:user) } - let_it_be(:alert) { create(:prometheus_alert, project: project) } - - let(:service) { described_class.new(project: project, current_user: user, params: nil) } - - describe '#execute' do - subject { service.execute(alert) } - - it 'deletes the alert' do - expect(subject).to be_truthy - - expect(alert).to be_destroyed - end - end -end diff --git a/spec/services/projects/prometheus/alerts/update_service_spec.rb b/spec/services/projects/prometheus/alerts/update_service_spec.rb deleted file mode 100644 index ec6766221f6..00000000000 --- a/spec/services/projects/prometheus/alerts/update_service_spec.rb +++ /dev/null @@ -1,53 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Projects::Prometheus::Alerts::UpdateService do - let_it_be(:project) { create(:project) } - let_it_be(:user) { create(:user) } - let_it_be(:environment) { create(:environment, project: project) } - - let_it_be(:alert) do - create(:prometheus_alert, project: project, environment: environment) - end - - let(:service) { described_class.new(project: project, current_user: user, params: params) } - - let(:params) do - { - environment_id: alert.environment_id, - prometheus_metric_id: alert.prometheus_metric_id, - operator: '==', - threshold: 2.0 - } - end - - describe '#execute' do - subject { service.execute(alert) } - - context 'with valid params' do - it 'updates the alert' do - expect(subject).to be_truthy - - expect(alert.reload).to have_attributes( - operator: 'eq', - threshold: 2.0 - ) - end - end - - context 'with invalid params' do - let(:other_environment) { create(:environment) } - - before do - params[:environment_id] = other_environment.id - end - - it 'fails to update' do - expect(subject).to be_falsey - - expect(alert).to be_invalid - end - end - end -end diff --git a/spec/services/projects/prometheus/metrics/destroy_service_spec.rb b/spec/services/projects/prometheus/metrics/destroy_service_spec.rb index 17cc88b27b6..b4af81f2c87 100644 --- a/spec/services/projects/prometheus/metrics/destroy_service_spec.rb +++ b/spec/services/projects/prometheus/metrics/destroy_service_spec.rb @@ -12,17 +12,4 @@ RSpec.describe Projects::Prometheus::Metrics::DestroyService do expect(PrometheusMetric.find_by(id: metric.id)).to be_nil end - - context 'when metric has a prometheus alert associated' do - it 'schedules a prometheus alert update' do - create(:prometheus_alert, project: metric.project, prometheus_metric: metric) - - schedule_update_service = spy - allow(::Clusters::Applications::ScheduleUpdateService).to receive(:new).and_return(schedule_update_service) - - subject.execute - - expect(schedule_update_service).to have_received(:execute) - end - end end diff --git a/spec/services/projects/prometheus/metrics/update_service_spec.rb b/spec/services/projects/prometheus/metrics/update_service_spec.rb deleted file mode 100644 index bf87093150c..00000000000 --- a/spec/services/projects/prometheus/metrics/update_service_spec.rb +++ /dev/null @@ -1,44 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Projects::Prometheus::Metrics::UpdateService do - let(:metric) { create(:prometheus_metric) } - - it 'updates the prometheus metric' do - expect do - described_class.new(metric, { title: "bar" }).execute - end.to change { metric.reload.title }.to("bar") - end - - context 'when metric has a prometheus alert associated' do - let(:schedule_update_service) { spy } - - before do - create(:prometheus_alert, project: metric.project, prometheus_metric: metric) - allow(::Clusters::Applications::ScheduleUpdateService).to receive(:new).and_return(schedule_update_service) - end - - context 'when updating title' do - it 'schedules a prometheus alert update' do - described_class.new(metric, { title: "bar" }).execute - - expect(schedule_update_service).to have_received(:execute) - end - end - - context 'when updating query' do - it 'schedules a prometheus alert update' do - described_class.new(metric, { query: "sum(bar)" }).execute - - expect(schedule_update_service).to have_received(:execute) - end - end - - it 'does not schedule a prometheus alert update without title nor query being changed' do - described_class.new(metric, { y_label: "bar" }).execute - - expect(schedule_update_service).not_to have_received(:execute) - end - end -end diff --git a/spec/services/projects/record_target_platforms_service_spec.rb b/spec/services/projects/record_target_platforms_service_spec.rb index 85311f36428..22ff325a62e 100644 --- a/spec/services/projects/record_target_platforms_service_spec.rb +++ b/spec/services/projects/record_target_platforms_service_spec.rb @@ -5,21 +5,38 @@ require 'spec_helper' RSpec.describe Projects::RecordTargetPlatformsService, '#execute' do let_it_be(:project) { create(:project) } - subject(:execute) { described_class.new(project).execute } + let(:detector_service) { Projects::AppleTargetPlatformDetectorService } + + subject(:execute) { described_class.new(project, detector_service).execute } + + context 'when detector returns target platform values' do + let(:detector_result) { [:ios, :osx] } + let(:service_result) { detector_result.map(&:to_s) } - context 'when project is an XCode project' do before do - double = instance_double(Projects::AppleTargetPlatformDetectorService, execute: [:ios, :osx]) - allow(Projects::AppleTargetPlatformDetectorService).to receive(:new) { double } + double = instance_double(detector_service, execute: detector_result) + allow(detector_service).to receive(:new) { double } end - it 'creates a new setting record for the project', :aggregate_failures do - expect { execute }.to change { ProjectSetting.count }.from(0).to(1) - expect(ProjectSetting.last.target_platforms).to match_array(%w(ios osx)) + shared_examples 'saves and returns detected target platforms' do + it 'creates a new setting record for the project', :aggregate_failures do + expect { execute }.to change { ProjectSetting.count }.from(0).to(1) + expect(ProjectSetting.last.target_platforms).to match_array(service_result) + end + + it 'returns the array of stored target platforms' do + expect(execute).to match_array service_result + end end - it 'returns array of detected target platforms' do - expect(execute).to match_array %w(ios osx) + it_behaves_like 'saves and returns detected target platforms' + + context 'when detector returns a non-array value' do + let(:detector_service) { Projects::AndroidTargetPlatformDetectorService } + let(:detector_result) { :android } + let(:service_result) { [detector_result.to_s] } + + it_behaves_like 'saves and returns detected target platforms' end context 'when a project has an existing setting record' do @@ -49,9 +66,76 @@ RSpec.describe Projects::RecordTargetPlatformsService, '#execute' do end end end + + describe 'Build iOS guide email experiment' do + shared_examples 'tracks experiment assignment event' do + it 'tracks the assignment event', :experiment do + expect(experiment(:build_ios_app_guide_email)) + .to track(:assignment) + .with_context(project: project) + .on_next_instance + + execute + end + end + + context 'experiment candidate' do + before do + stub_experiments(build_ios_app_guide_email: :candidate) + end + + it 'executes a Projects::InProductMarketingCampaignEmailsService' do + service_double = instance_double(Projects::InProductMarketingCampaignEmailsService, execute: true) + + expect(Projects::InProductMarketingCampaignEmailsService) + .to receive(:new).with(project, Users::InProductMarketingEmail::BUILD_IOS_APP_GUIDE) + .and_return service_double + expect(service_double).to receive(:execute) + + execute + end + + it_behaves_like 'tracks experiment assignment event' + end + + shared_examples 'does not send email' do + it 'does not execute a Projects::InProductMarketingCampaignEmailsService' do + expect(Projects::InProductMarketingCampaignEmailsService).not_to receive(:new) + + execute + end + end + + context 'experiment control' do + before do + stub_experiments(build_ios_app_guide_email: :control) + end + + it_behaves_like 'does not send email' + it_behaves_like 'tracks experiment assignment event' + end + + context 'when project is not an iOS project' do + let(:detector_service) { Projects::AppleTargetPlatformDetectorService } + let(:detector_result) { :android } + + before do + stub_experiments(build_ios_app_guide_email: :candidate) + end + + it_behaves_like 'does not send email' + + it 'does not track experiment assignment event', :experiment do + expect(experiment(:build_ios_app_guide_email)) + .not_to track(:assignment) + + execute + end + end + end end - context 'when project is not an XCode project' do + context 'when detector does not return any target platform values' do before do double = instance_double(Projects::AppleTargetPlatformDetectorService, execute: []) allow(Projects::AppleTargetPlatformDetectorService).to receive(:new).with(project) { double } diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 6407b8d3940..777162b6196 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -13,6 +13,7 @@ RSpec.describe Projects::UpdatePagesService do let(:file) { fixture_file_upload("spec/fixtures/pages.zip") } let(:empty_file) { fixture_file_upload("spec/fixtures/pages_empty.zip") } + let(:empty_metadata_filename) { "spec/fixtures/pages_empty.zip.meta" } let(:metadata_filename) { "spec/fixtures/pages.zip.meta" } let(:metadata) { fixture_file_upload(metadata_filename) if File.exist?(metadata_filename) } @@ -91,6 +92,17 @@ RSpec.describe Projects::UpdatePagesService do end end + context 'when archive does not have pages directory' do + let(:file) { empty_file } + let(:metadata_filename) { empty_metadata_filename } + + it 'returns an error' do + expect(execute).not_to eq(:success) + + expect(GenericCommitStatus.last.description).to eq("Error: The `public/` folder is missing, or not declared in `.gitlab-ci.yml`.") + end + end + it 'limits pages size' do stub_application_setting(max_pages_size: 1) expect(execute).not_to eq(:success) diff --git a/spec/services/prometheus/create_default_alerts_service_spec.rb b/spec/services/prometheus/create_default_alerts_service_spec.rb deleted file mode 100644 index 0880799b589..00000000000 --- a/spec/services/prometheus/create_default_alerts_service_spec.rb +++ /dev/null @@ -1,92 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Prometheus::CreateDefaultAlertsService do - let_it_be(:project) { create(:project, :repository) } - - let(:instance) { described_class.new(project: project) } - let(:expected_alerts) { described_class::DEFAULT_ALERTS } - - describe '#execute' do - subject(:execute) { instance.execute } - - shared_examples 'no alerts created' do - it 'does not create alerts' do - expect { execute }.not_to change { project.reload.prometheus_alerts.count } - end - end - - context 'no environment' do - it_behaves_like 'no alerts created' - end - - context 'environment exists' do - let_it_be(:environment) { create(:environment, project: project) } - - context 'no found metric' do - it_behaves_like 'no alerts created' - end - - context 'metric exists' do - before do - create_expected_metrics! - end - - context 'alert exists already' do - before do - create_pre_existing_alerts!(environment) - end - - it_behaves_like 'no alerts created' - end - - it 'creates alerts' do - expect { execute }.to change { project.reload.prometheus_alerts.count } - .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') } - - it 'uses the production environment' do - expect { execute }.to change { production.reload.prometheus_alerts.count } - .by(expected_alerts.size) - end - end - end - end - end - - private - - def create_expected_metrics! - expected_alerts.each do |alert_hash| - create(:prometheus_metric, :common, identifier: alert_hash.fetch(:identifier)) - end - end - - def create_pre_existing_alerts!(environment) - expected_alerts.each do |alert_hash| - metric = PrometheusMetric.for_identifier(alert_hash[:identifier]).first! - create(:prometheus_alert, prometheus_metric: metric, project: project, environment: environment) - end - end -end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 85dbc39edcf..f7a22b1b92f 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -696,6 +696,21 @@ RSpec.describe QuickActions::InterpretService do expect(message).to eq("Assigned #{developer.to_reference}.") end + context 'when the reference does not match the exact case' do + let(:user) { create(:user) } + let(:content) { "/assign #{user.to_reference.upcase}" } + + it 'assigns to the user' do + issuable.project.add_developer(user) + + _, updates, message = service.execute(content, issuable) + + expect(content).not_to include(user.to_reference) + expect(updates).to eq(assignee_ids: [user.id]) + expect(message).to eq("Assigned #{user.to_reference}.") + end + end + context 'when the user has a private profile' do let(:user) { create(:user, :private_profile) } let(:content) { "/assign #{user.to_reference}" } diff --git a/spec/services/service_ping/build_payload_service_spec.rb b/spec/services/service_ping/build_payload_service_spec.rb deleted file mode 100644 index cd2685069c9..00000000000 --- a/spec/services/service_ping/build_payload_service_spec.rb +++ /dev/null @@ -1,47 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ServicePing::BuildPayloadService do - describe '#execute', :without_license do - subject(:service_ping_payload) { described_class.new.execute } - - include_context 'stubbed service ping metrics definitions' do - let(:subscription_metrics) do - [ - metric_attributes('active_user_count', "Subscription") - ] - end - end - - context 'when usage_ping_enabled setting is false' do - before do - # Gitlab::CurrentSettings.usage_ping_enabled? == false - stub_config_setting(usage_ping_enabled: false) - end - - it 'returns empty service ping payload' do - expect(service_ping_payload).to eq({}) - end - end - - context 'when usage_ping_enabled setting is true' do - before do - # Gitlab::CurrentSettings.usage_ping_enabled? == true - stub_config_setting(usage_ping_enabled: true) - end - - it_behaves_like 'complete service ping payload' - - context 'with require stats consent enabled' do - before do - allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: true)) - end - - it 'returns empty service ping payload' do - expect(service_ping_payload).to eq({}) - end - end - end - end -end diff --git a/spec/services/service_ping/permit_data_categories_service_spec.rb b/spec/services/service_ping/permit_data_categories_service_spec.rb deleted file mode 100644 index 550c0ea5e13..00000000000 --- a/spec/services/service_ping/permit_data_categories_service_spec.rb +++ /dev/null @@ -1,42 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ServicePing::PermitDataCategoriesService do - describe '#execute', :without_license do - subject(:permitted_categories) { described_class.new.execute } - - context 'when usage ping setting is set to true' do - before do - allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: false)) - stub_config_setting(usage_ping_enabled: true) - end - - it 'returns all categories' do - expect(permitted_categories).to match_array(%w[standard subscription operational optional]) - end - end - - context 'when usage ping setting is set to false' do - before do - allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: false)) - stub_config_setting(usage_ping_enabled: false) - end - - it 'returns no categories' do - expect(permitted_categories).to match_array([]) - end - end - - context 'when User.single_user&.requires_usage_stats_consent? is required' do - before do - allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: true)) - stub_config_setting(usage_ping_enabled: true) - end - - it 'returns no categories' do - expect(permitted_categories).to match_array([]) - end - end - end -end diff --git a/spec/services/service_ping/service_ping_settings_spec.rb b/spec/services/service_ping/service_ping_settings_spec.rb deleted file mode 100644 index 90a5c6b30eb..00000000000 --- a/spec/services/service_ping/service_ping_settings_spec.rb +++ /dev/null @@ -1,46 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ServicePing::ServicePingSettings do - using RSpec::Parameterized::TableSyntax - - describe '#product_intelligence_enabled?' do - where(:usage_ping_enabled, :requires_usage_stats_consent, :expected_product_intelligence_enabled) do - # Usage ping enabled - true | false | true - true | true | false - - # Usage ping disabled - false | false | false - false | true | false - end - - with_them do - before do - allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: requires_usage_stats_consent)) - stub_config_setting(usage_ping_enabled: usage_ping_enabled) - end - - it 'has the correct product_intelligence_enabled?' do - expect(described_class.product_intelligence_enabled?).to eq(expected_product_intelligence_enabled) - end - end - end - - describe '#enabled?' do - describe 'has the correct enabled' do - it 'when false' do - stub_config_setting(usage_ping_enabled: false) - - expect(described_class.enabled?).to eq(false) - end - - it 'when true' do - stub_config_setting(usage_ping_enabled: true) - - expect(described_class.enabled?).to eq(true) - end - end - end -end diff --git a/spec/services/service_ping/submit_service_ping_service_spec.rb b/spec/services/service_ping/submit_service_ping_service_spec.rb index 73be8f000a9..7a8bd1910fe 100644 --- a/spec/services/service_ping/submit_service_ping_service_spec.rb +++ b/spec/services/service_ping/submit_service_ping_service_spec.rb @@ -51,6 +51,9 @@ RSpec.describe ServicePing::SubmitService do let(:with_dev_ops_score_params) { { dev_ops_score: score_params[:score] } } let(:with_conv_index_params) { { conv_index: score_params[:score] } } let(:with_usage_data_id_params) { { conv_index: { usage_data_id: usage_data_id } } } + let(:service_ping_payload_url) { File.join(described_class::STAGING_BASE_URL, described_class::USAGE_DATA_PATH) } + let(:service_ping_errors_url) { File.join(described_class::STAGING_BASE_URL, described_class::ERROR_PATH) } + let(:service_ping_metadata_url) { File.join(described_class::STAGING_BASE_URL, described_class::METADATA_PATH) } shared_examples 'does not run' do it do @@ -63,7 +66,7 @@ RSpec.describe ServicePing::SubmitService do shared_examples 'does not send a blank usage ping payload' do it do - expect(Gitlab::HTTP).not_to receive(:post).with(subject.url, any_args) + expect(Gitlab::HTTP).not_to receive(:post).with(service_ping_payload_url, any_args) expect { subject.execute }.to raise_error(described_class::SubmissionError) do |error| expect(error.message).to include('Usage data is blank') @@ -117,6 +120,7 @@ RSpec.describe ServicePing::SubmitService do it 'generates service ping' do stub_response(body: with_dev_ops_score_params) + stub_response(body: nil, url: service_ping_metadata_url, status: 201) expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_call_original @@ -129,18 +133,21 @@ RSpec.describe ServicePing::SubmitService do stub_usage_data_connections stub_database_flavor_check stub_application_setting(usage_ping_enabled: true) - stub_response(body: nil, url: subject.error_url, status: 201) + stub_response(body: nil, url: service_ping_errors_url, status: 201) + stub_response(body: nil, url: service_ping_metadata_url, status: 201) end context 'and user requires usage stats consent' do before do - allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: true)) + allow(User).to receive(:single_user) + .and_return(instance_double(User, :user, requires_usage_stats_consent?: true)) end it_behaves_like 'does not run' end it 'sends a POST request' do + stub_response(body: nil, url: service_ping_metadata_url, status: 201) response = stub_response(body: with_dev_ops_score_params) subject.execute @@ -167,7 +174,8 @@ RSpec.describe ServicePing::SubmitService do recorded_at = Time.current usage_data = { uuid: 'uuid', recorded_at: recorded_at } - expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_return(usage_data) + expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values) + .and_return(usage_data) subject.execute @@ -190,7 +198,8 @@ RSpec.describe ServicePing::SubmitService do recorded_at = Time.current usage_data = { uuid: 'uuid', recorded_at: recorded_at } - expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_return(usage_data) + expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values) + .and_return(usage_data) subject.execute @@ -235,7 +244,8 @@ RSpec.describe ServicePing::SubmitService do recorded_at = Time.current usage_data = { uuid: 'uuid', recorded_at: recorded_at } - expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_return(usage_data) + expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values) + .and_return(usage_data) subject.execute @@ -268,7 +278,7 @@ RSpec.describe ServicePing::SubmitService do context 'and usage data is nil' do before do - allow(ServicePing::BuildPayloadService).to receive(:execute).and_return(nil) + allow(ServicePing::BuildPayload).to receive(:execute).and_return(nil) allow(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_return(nil) end @@ -278,29 +288,33 @@ RSpec.describe ServicePing::SubmitService do context 'if payload service fails' do before do stub_response(body: with_dev_ops_score_params) - allow(ServicePing::BuildPayloadService).to receive_message_chain(:new, :execute) + + allow(ServicePing::BuildPayload).to receive_message_chain(:new, :execute) .and_raise(described_class::SubmissionError, 'SubmissionError') end it 'calls Gitlab::Usage::ServicePingReport .for method' do usage_data = build_usage_data - expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_return(usage_data) + expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values) + .and_return(usage_data) subject.execute end it 'submits error' do - expect(Gitlab::HTTP).to receive(:post).with(subject.url, any_args) + expect(Gitlab::HTTP).to receive(:post).with(URI.join(service_ping_payload_url), any_args) + .and_call_original + expect(Gitlab::HTTP).to receive(:post).with(URI.join(service_ping_errors_url), any_args) .and_call_original - expect(Gitlab::HTTP).to receive(:post).with(subject.error_url, any_args) + expect(Gitlab::HTTP).to receive(:post).with(URI.join(service_ping_metadata_url), any_args) .and_call_original subject.execute end end - context 'calls BuildPayloadService first' do + context 'calls BuildPayload first' do before do stub_response(body: with_dev_ops_score_params) end @@ -308,7 +322,7 @@ RSpec.describe ServicePing::SubmitService do it 'returns usage data' do usage_data = build_usage_data - expect_next_instance_of(ServicePing::BuildPayloadService) do |service| + expect_next_instance_of(ServicePing::BuildPayload) do |service| expect(service).to receive(:execute).and_return(usage_data) end @@ -321,7 +335,7 @@ RSpec.describe ServicePing::SubmitService do stub_response(body: with_dev_ops_score_params, status: 404) usage_data = build_usage_data - allow_next_instance_of(ServicePing::BuildPayloadService) do |service| + allow_next_instance_of(ServicePing::BuildPayload) do |service| allow(service).to receive(:execute).and_return(usage_data) end end @@ -329,7 +343,8 @@ RSpec.describe ServicePing::SubmitService do it 'calls Gitlab::Usage::ServicePingReport .for method' do usage_data = build_usage_data - expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_return(usage_data) + expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values) + .and_return(usage_data) # SubmissionError is raised as a result of 404 in response from HTTP Request expect { subject.execute }.to raise_error(described_class::SubmissionError) @@ -349,38 +364,79 @@ RSpec.describe ServicePing::SubmitService do end it 'does not call DevOpsReport service' do - expect(ServicePing::DevopsReportService).not_to receive(:new) + expect(ServicePing::DevopsReport).not_to receive(:new) subject.execute end end end - describe '#url' do - let(:url) { subject.url.to_s } + context 'metadata reporting' do + before do + stub_usage_data_connections + stub_database_flavor_check + stub_application_setting(usage_ping_enabled: true) + stub_response(body: with_conv_index_params) + end + + context 'with feature flag measure_service_ping_metric_collection turned on' do + let(:metric_double) { instance_double(Gitlab::Usage::ServicePing::LegacyMetricTimingDecorator, duration: 123) } + let(:payload) do + { + metric_a: metric_double, + metric_group: { + metric_b: metric_double + }, + metric_without_timing: "value", + recorded_at: Time.current + } + end + + let(:metadata_payload) do + { + metadata: { + metrics: [ + { name: 'metric_a', time_elapsed: 123 }, + { name: 'metric_group.metric_b', time_elapsed: 123 } + ] + } + } + end - context 'when Rails.env is production' do before do - stub_rails_env('production') + stub_feature_flags(measure_service_ping_metric_collection: true) + + allow_next_instance_of(ServicePing::BuildPayload) do |service| + allow(service).to receive(:execute).and_return(payload) + end end - it 'points to the production Version app' do - expect(url).to eq("#{described_class::PRODUCTION_BASE_URL}/#{described_class::USAGE_DATA_PATH}") + it 'submits metadata' do + response = stub_full_request(service_ping_metadata_url, method: :post) + .with(body: metadata_payload) + + subject.execute + + expect(response).to have_been_requested end end - context 'when Rails.env is not production' do + context 'with feature flag measure_service_ping_metric_collection turned off' do before do - stub_rails_env('development') + stub_feature_flags(measure_service_ping_metric_collection: false) end - it 'points to the staging Version app' do - expect(url).to eq("#{described_class::STAGING_BASE_URL}/#{described_class::USAGE_DATA_PATH}") + it 'does NOT submit metadata' do + response = stub_full_request(service_ping_metadata_url, method: :post) + + subject.execute + + expect(response).not_to have_been_requested end end end - def stub_response(url: subject.url, body:, status: 201) + def stub_response(url: service_ping_payload_url, body:, status: 201) stub_full_request(url, method: :post) .to_return( headers: { 'Content-Type' => 'application/json' }, diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index c322ec35e86..741d136b9a0 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -431,6 +431,19 @@ RSpec.describe SystemNoteService do end end + describe '.remove_timelog' do + let(:issue) { create(:issue, project: project) } + let(:timelog) { create(:timelog, user: author, issue: issue, time_spent: 1800)} + + it 'calls TimeTrackingService' do + expect_next_instance_of(::SystemNotes::TimeTrackingService) do |service| + expect(service).to receive(:remove_timelog) + end + + described_class.remove_timelog(noteable, project, author, timelog) + end + end + describe '.handle_merge_request_draft' do it 'calls MergeRequestsService' do expect_next_instance_of(::SystemNotes::MergeRequestsService) do |service| @@ -695,4 +708,38 @@ RSpec.describe SystemNoteService do described_class.change_issue_type(incident, author) end end + + describe '.add_timeline_event' do + let(:timeline_event) { instance_double('IncidentManagement::TimelineEvent', incident: noteable, project: project) } + + it 'calls IncidentsService' do + expect_next_instance_of(::SystemNotes::IncidentsService) do |service| + expect(service).to receive(:add_timeline_event).with(timeline_event) + end + + described_class.add_timeline_event(timeline_event) + end + end + + describe '.edit_timeline_event' do + let(:timeline_event) { instance_double('IncidentManagement::TimelineEvent', incident: noteable, project: project) } + + it 'calls IncidentsService' do + expect_next_instance_of(::SystemNotes::IncidentsService) do |service| + expect(service).to receive(:edit_timeline_event).with(timeline_event, author, was_changed: :occurred_at) + end + + described_class.edit_timeline_event(timeline_event, author, was_changed: :occurred_at) + end + end + + describe '.delete_timeline_event' do + it 'calls IncidentsService' do + expect_next_instance_of(::SystemNotes::IncidentsService) do |service| + expect(service).to receive(:delete_timeline_event).with(author) + end + + described_class.delete_timeline_event(noteable, author) + end + end end diff --git a/spec/services/system_notes/incidents_service_spec.rb b/spec/services/system_notes/incidents_service_spec.rb new file mode 100644 index 00000000000..d1b831e9c4c --- /dev/null +++ b/spec/services/system_notes/incidents_service_spec.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe SystemNotes::IncidentsService do + include Gitlab::Routing + + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be(:author) { create(:user) } + let_it_be(:incident) { create(:incident, project: project, author: user) } + let_it_be(:timeline_event) do + create(:incident_management_timeline_event, project: project, incident: incident, author: author) + end + + describe '#add_timeline_event' do + subject { described_class.new(noteable: incident).add_timeline_event(timeline_event) } + + it_behaves_like 'a system note' do + let(:noteable) { incident } + let(:action) { 'timeline_event' } + end + + it 'posts the correct text to the system note' do + path = project_issues_incident_path(project, incident, anchor: "timeline_event_#{timeline_event.id}") + expect(subject.note).to match("added an [incident timeline event](#{path})") + end + end + + describe '#edit_timeline_event' do + let(:was_changed) { :unknown } + let(:path) { project_issues_incident_path(project, incident, anchor: "timeline_event_#{timeline_event.id}") } + + subject do + described_class.new(noteable: incident).edit_timeline_event(timeline_event, author, was_changed: was_changed) + end + + it_behaves_like 'a system note' do + let(:noteable) { incident } + let(:action) { 'timeline_event' } + end + + context "when only timeline event's occurred_at was changed" do + let(:was_changed) { :occurred_at } + + it 'posts the correct text to the system note' do + expect(subject.note).to match("edited the event time/date on [incident timeline event](#{path})") + end + end + + context "when only timeline event's note was changed" do + let(:was_changed) { :note } + + it 'posts the correct text to the system note' do + expect(subject.note).to match("edited the text on [incident timeline event](#{path})") + end + end + + context "when both timeline events occurred_at and note was changed" do + let(:was_changed) { :occurred_at_and_note } + + it 'posts the correct text to the system note' do + expect(subject.note).to match("edited the event time/date and text on [incident timeline event](#{path})") + end + end + + context "when was changed reason is unknown" do + let(:was_changed) { :unknown } + + it 'posts the correct text to the system note' do + expect(subject.note).to match("edited [incident timeline event](#{path})") + end + end + end + + describe '#delete_timeline_event' do + subject { described_class.new(noteable: incident).delete_timeline_event(author) } + + it_behaves_like 'a system note' do + let(:noteable) { incident } + let(:action) { 'timeline_event' } + end + + it 'posts the correct text to the system note' do + expect(subject.note).to match('deleted an incident timeline event') + end + end +end diff --git a/spec/services/system_notes/time_tracking_service_spec.rb b/spec/services/system_notes/time_tracking_service_spec.rb index ec126cb5447..fdf18f4f29a 100644 --- a/spec/services/system_notes/time_tracking_service_spec.rb +++ b/spec/services/system_notes/time_tracking_service_spec.rb @@ -106,6 +106,30 @@ RSpec.describe ::SystemNotes::TimeTrackingService do end end + describe '#remove_timelog' do + subject { described_class.new(noteable: noteable, project: project, author: author).remove_timelog(timelog) } + + context 'when the timelog has a positive time spent value' do + let_it_be(:noteable, reload: true) { create(:issue, project: project) } + + let(:timelog) { create(:timelog, user: author, issue: noteable, time_spent: 1800, spent_at: '2022-03-30T00:00:00.000Z')} + + it 'sets the note text' do + expect(subject.note).to eq "deleted 30m of spent time from 2022-03-30" + end + end + + context 'when the timelog has a negative time spent value' do + let_it_be(:noteable, reload: true) { create(:issue, project: project) } + + let(:timelog) { create(:timelog, user: author, issue: noteable, time_spent: -1800, spent_at: '2022-03-30T00:00:00.000Z')} + + it 'sets the note text' do + expect(subject.note).to eq "deleted -30m of spent time from 2022-03-30" + end + end + end + describe '#change_time_spent' do subject { described_class.new(noteable: noteable, project: project, author: author).change_time_spent } diff --git a/spec/services/timelogs/delete_service_spec.rb b/spec/services/timelogs/delete_service_spec.rb new file mode 100644 index 00000000000..c52cebdc5bf --- /dev/null +++ b/spec/services/timelogs/delete_service_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Timelogs::DeleteService do + let_it_be(:author) { create(:user) } + let_it_be(:project) { create(:project, :public) } + let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:timelog) { create(:timelog, user: author, issue: issue, time_spent: 1800)} + + let(:service) { described_class.new(timelog, user) } + + describe '#execute' do + subject { service.execute } + + context 'when the timelog exists' do + let(:user) { author } + + it 'removes the timelog' do + expect { subject }.to change { Timelog.count }.by(-1) + end + + it 'returns the removed timelog' do + expect(subject).to be_success + expect(subject.payload).to eq(timelog) + end + end + + context 'when the timelog does not exist' do + let(:user) { create(:user) } + let!(:timelog) { nil } + + it 'returns an error' do + expect(subject).to be_error + expect(subject.message).to eq('Timelog doesn\'t exist or you don\'t have permission to delete it') + expect(subject.http_status).to eq(404) + end + end + + context 'when the user does not have permission' do + let(:user) { create(:user) } + + it 'returns an error' do + expect(subject).to be_error + expect(subject.message).to eq('Timelog doesn\'t exist or you don\'t have permission to delete it') + expect(subject.http_status).to eq(404) + end + end + + context 'when the timelog deletion fails' do + let(:user) { author } + let!(:timelog) { create(:timelog, user: author, issue: issue, time_spent: 1800)} + + before do + allow(timelog).to receive(:destroy).and_return(false) + end + + it 'returns an error' do + expect(subject).to be_error + expect(subject.message).to eq('Failed to remove timelog') + expect(subject.http_status).to eq(400) + end + end + end +end diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index 80a506bb1d6..45dbe83b496 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -73,10 +73,10 @@ RSpec.describe Users::DestroyService do 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 + expect(bulk_destroy_service).to receive(:execute).with({ hard_delete: true }).and_call_original end - service.execute(user, hard_delete: true) + service.execute(user, { hard_delete: true }) end it 'does not delete project snippets that the user is the author of' do @@ -336,35 +336,24 @@ RSpec.describe Users::DestroyService do context 'batched nullify' do let(:other_user) { create(:user) } - context 'when :nullify_in_batches_on_user_deletion feature flag is enabled' do - it 'nullifies related associations in batches' do - expect(other_user).to receive(:nullify_dependent_associations_in_batches).and_call_original + it 'nullifies related associations in batches' do + expect(other_user).to receive(:nullify_dependent_associations_in_batches).and_call_original - described_class.new(user).execute(other_user, skip_authorization: true) - end - - it 'nullifies last_updated_issues and closed_issues' do - issue = create(:issue, closed_by: other_user, updated_by: other_user) - - described_class.new(user).execute(other_user, skip_authorization: true) - - issue.reload - - expect(issue.closed_by).to be_nil - expect(issue.updated_by).to be_nil - end + described_class.new(user).execute(other_user, skip_authorization: true) end - context 'when :nullify_in_batches_on_user_deletion feature flag is disabled' do - before do - stub_feature_flags(nullify_in_batches_on_user_deletion: false) - end + it 'nullifies last_updated_issues, closed_issues, resource_label_events' do + issue = create(:issue, closed_by: other_user, updated_by: other_user) + resource_label_event = create(:resource_label_event, user: other_user) - it 'does not use batching' do - expect(other_user).not_to receive(:nullify_dependent_associations_in_batches) + described_class.new(user).execute(other_user, skip_authorization: true) - described_class.new(user).execute(other_user, skip_authorization: true) - end + issue.reload + resource_label_event.reload + + expect(issue.closed_by).to be_nil + expect(issue.updated_by).to be_nil + expect(resource_label_event.user).to be_nil end end end diff --git a/spec/services/namespaces/in_product_marketing_email_records_spec.rb b/spec/services/users/in_product_marketing_email_records_spec.rb index d80e20135d5..0b9400dcd12 100644 --- a/spec/services/namespaces/in_product_marketing_email_records_spec.rb +++ b/spec/services/users/in_product_marketing_email_records_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Namespaces::InProductMarketingEmailRecords do +RSpec.describe Users::InProductMarketingEmailRecords do let_it_be(:user) { create :user } subject(:records) { described_class.new } @@ -15,8 +15,9 @@ RSpec.describe Namespaces::InProductMarketingEmailRecords do before do allow(Users::InProductMarketingEmail).to receive(:bulk_insert!) - records.add(user, :team_short, 0) - records.add(user, :create, 1) + records.add(user, track: :team_short, series: 0) + records.add(user, track: :create, series: 1) + records.add(user, campaign: Users::InProductMarketingEmail::BUILD_IOS_APP_GUIDE) end it 'bulk inserts added records' do @@ -31,24 +32,34 @@ RSpec.describe Namespaces::InProductMarketingEmailRecords do end describe '#add' do - it 'adds a Users::InProductMarketingEmail record to its records' do + it 'adds a Users::InProductMarketingEmail record to its records', :aggregate_failures do freeze_time do - records.add(user, :team_short, 0) - records.add(user, :create, 1) + records.add(user, track: :team_short, series: 0) + records.add(user, track: :create, series: 1) + records.add(user, campaign: Users::InProductMarketingEmail::BUILD_IOS_APP_GUIDE) - first, second = records.records + first, second, third = records.records expect(first).to be_a Users::InProductMarketingEmail + expect(first.campaign).to be_nil expect(first.track.to_sym).to eq :team_short expect(first.series).to eq 0 expect(first.created_at).to eq Time.zone.now expect(first.updated_at).to eq Time.zone.now expect(second).to be_a Users::InProductMarketingEmail + expect(second.campaign).to be_nil expect(second.track.to_sym).to eq :create expect(second.series).to eq 1 expect(second.created_at).to eq Time.zone.now expect(second.updated_at).to eq Time.zone.now + + expect(third).to be_a Users::InProductMarketingEmail + expect(third.campaign).to eq Users::InProductMarketingEmail::BUILD_IOS_APP_GUIDE + expect(third.track).to be_nil + expect(third.series).to be_nil + expect(third.created_at).to eq Time.zone.now + expect(third.updated_at).to eq Time.zone.now end end end diff --git a/spec/services/users/validate_otp_service_spec.rb b/spec/services/users/validate_manual_otp_service_spec.rb index 46b80b2149f..d71735814f2 100644 --- a/spec/services/users/validate_otp_service_spec.rb +++ b/spec/services/users/validate_manual_otp_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Users::ValidateOtpService do +RSpec.describe Users::ValidateManualOtpService do let_it_be(:user) { create(:user) } let(:otp_code) { 42 } @@ -25,8 +25,8 @@ RSpec.describe Users::ValidateOtpService do allow(::Gitlab.config.forti_authenticator).to receive(:enabled).and_return(true) end - it 'calls FortiAuthenticator strategy' do - expect_next_instance_of(::Gitlab::Auth::Otp::Strategies::FortiAuthenticator) do |strategy| + it 'calls ManualOtp strategy' do + expect_next_instance_of(::Gitlab::Auth::Otp::Strategies::FortiAuthenticator::ManualOtp) do |strategy| expect(strategy).to receive(:validate).with(otp_code).once end @@ -48,4 +48,25 @@ RSpec.describe Users::ValidateOtpService do validate end end + + context 'unexpected error' do + before do + stub_feature_flags(forti_authenticator: user) + allow(::Gitlab.config.forti_authenticator).to receive(:enabled).and_return(true) + end + + it 'returns error' do + error_message = "boom!" + + expect_next_instance_of(::Gitlab::Auth::Otp::Strategies::FortiAuthenticator::ManualOtp) do |strategy| + expect(strategy).to receive(:validate).with(otp_code).once.and_raise(StandardError, error_message) + end + expect(Gitlab::ErrorTracking).to receive(:log_exception) + + result = validate + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq(error_message) + end + end end diff --git a/spec/services/users/validate_push_otp_service_spec.rb b/spec/services/users/validate_push_otp_service_spec.rb new file mode 100644 index 00000000000..960b6bcd3bb --- /dev/null +++ b/spec/services/users/validate_push_otp_service_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::ValidatePushOtpService do + let_it_be(:user) { create(:user) } + + subject(:validate) { described_class.new(user).execute } + + context 'FortiAuthenticator' do + before do + stub_feature_flags(forti_authenticator: user) + allow(::Gitlab.config.forti_authenticator).to receive(:enabled).and_return(true) + end + + it 'calls PushOtp strategy' do + expect_next_instance_of(::Gitlab::Auth::Otp::Strategies::FortiAuthenticator::PushOtp) do |strategy| + expect(strategy).to receive(:validate).once + end + + validate + end + end + + context 'unexpected error' do + before do + stub_feature_flags(forti_authenticator: user) + allow(::Gitlab.config.forti_authenticator).to receive(:enabled).and_return(true) + end + + it 'returns error' do + error_message = "boom!" + + expect_next_instance_of(::Gitlab::Auth::Otp::Strategies::FortiAuthenticator::PushOtp) do |strategy| + expect(strategy).to receive(:validate).once.and_raise(StandardError, error_message) + end + expect(Gitlab::ErrorTracking).to receive(:log_exception) + + result = validate + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq(error_message) + end + end +end diff --git a/spec/services/work_items/delete_task_service_spec.rb b/spec/services/work_items/delete_task_service_spec.rb new file mode 100644 index 00000000000..04944645c9b --- /dev/null +++ b/spec/services/work_items/delete_task_service_spec.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::DeleteTaskService do + let_it_be(:project) { create(:project) } + let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } + let_it_be_with_refind(:task) { create(:work_item, project: project, author: developer) } + let_it_be_with_refind(:list_work_item) do + create(:work_item, project: project, description: "- [ ] #{task.to_reference}+") + end + + let(:current_user) { developer } + let(:line_number_start) { 1 } + let(:params) do + { + line_number_start: line_number_start, + line_number_end: 1, + task: task + } + end + + before_all do + create(:issue_link, source_id: list_work_item.id, target_id: task.id) + end + + shared_examples 'failing WorkItems::DeleteTaskService' do |error_message| + it { is_expected.to be_error } + + it 'does not remove work item or issue links' do + expect do + service_result + list_work_item.reload + end.to not_change(WorkItem, :count).and( + not_change(IssueLink, :count) + ).and( + not_change(list_work_item, :description) + ) + end + + it 'returns an error message' do + expect(service_result.errors).to contain_exactly(error_message) + end + end + + describe '#execute' do + subject(:service_result) do + described_class.new( + work_item: list_work_item, + current_user: current_user, + lock_version: list_work_item.lock_version, + task_params: params + ).execute + end + + context 'when work item params are valid' do + it { is_expected.to be_success } + + it 'deletes the work item and the related issue link' do + expect do + service_result + end.to change(WorkItem, :count).by(-1).and( + change(IssueLink, :count).by(-1) + ) + end + + it 'removes the task list item with the work item reference' do + expect do + service_result + end.to change(list_work_item, :description).from(list_work_item.description).to('') + end + end + + context 'when first operation fails' do + let(:line_number_start) { -1 } + + it_behaves_like 'failing WorkItems::DeleteTaskService', 'line_number_start must be greater than 0' + end + + context 'when last operation fails' do + let_it_be(:non_member_user) { create(:user) } + + let(:current_user) { non_member_user } + + it_behaves_like 'failing WorkItems::DeleteTaskService', 'User not authorized to delete work item' + end + end +end diff --git a/spec/services/work_items/task_list_reference_removal_service_spec.rb b/spec/services/work_items/task_list_reference_removal_service_spec.rb new file mode 100644 index 00000000000..bca72da0efa --- /dev/null +++ b/spec/services/work_items/task_list_reference_removal_service_spec.rb @@ -0,0 +1,151 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::TaskListReferenceRemovalService do + let_it_be(:developer) { create(:user) } + let_it_be(:project) { create(:project, :repository).tap { |project| project.add_developer(developer) } } + let_it_be(:task) { create(:work_item, project: project) } + let_it_be(:single_line_work_item, refind: true) do + create(:work_item, project: project, description: "- [ ] #{task.to_reference}+ single line") + end + + let_it_be(:multiple_line_work_item, refind: true) do + create( + :work_item, + project: project, + description: <<~MARKDOWN + Any text + + * [ ] Item to be converted + #{task.to_reference}+ second line + third line + * [x] task + + More text + MARKDOWN + ) + end + + let(:line_number_start) { 3 } + let(:line_number_end) { 5 } + let(:work_item) { multiple_line_work_item } + let(:lock_version) { work_item.lock_version } + + shared_examples 'successful work item task reference removal service' do |expected_description| + it { is_expected.to be_success } + + it 'removes the task list item containing the task reference' do + expect do + result + end.to change(work_item, :description).from(work_item.description).to(expected_description) + end + + it 'creates system notes' do + expect do + result + end.to change(Note, :count).by(1) + + expect(Note.last.note).to include('changed the description') + end + end + + shared_examples 'failing work item task reference removal service' do |error_message| + it { is_expected.to be_error } + + it 'does not change the work item description' do + expect do + result + work_item.reload + end.to not_change(work_item, :description) + end + + it 'returns an error message' do + expect(result.errors).to contain_exactly(error_message) + end + end + + describe '#execute' do + subject(:result) do + described_class.new( + work_item: work_item, + task: task, + line_number_start: line_number_start, + line_number_end: line_number_end, + lock_version: lock_version, + current_user: developer + ).execute + end + + context 'when task mardown spans a single line' do + let(:line_number_start) { 1 } + let(:line_number_end) { 1 } + let(:work_item) { single_line_work_item } + + it_behaves_like 'successful work item task reference removal service', '' + + context 'when description does not contain a task' do + let_it_be(:no_matching_work_item) { create(:work_item, project: project, description: 'no matching task') } + + let(:work_item) { no_matching_work_item } + + it_behaves_like 'failing work item task reference removal service', 'Unable to detect a task on lines 1-1' + end + + context 'when description reference does not exactly match the task reference' do + before do + work_item.update!(description: work_item.description.gsub(task.to_reference, "#{task.to_reference}200")) + end + + it_behaves_like 'failing work item task reference removal service', 'Unable to detect a task on lines 1-1' + end + end + + context 'when task mardown spans multiple lines' do + it_behaves_like 'successful work item task reference removal service', "Any text\n\n* [x] task\n\nMore text" + end + + context 'when updating the work item fails' do + before do + work_item.title = nil + end + + it_behaves_like 'failing work item task reference removal service', "Title can't be blank" + end + + context 'when description is empty' do + let_it_be(:empty_work_item) { create(:work_item, project: project, description: '') } + + let(:work_item) { empty_work_item } + + it_behaves_like 'failing work item task reference removal service', "Work item description can't be blank" + end + + context 'when line_number_start is lower than 1' do + let(:line_number_start) { 0 } + + it_behaves_like 'failing work item task reference removal service', 'line_number_start must be greater than 0' + end + + context 'when line_number_end is lower than line_number_start' do + let(:line_number_end) { line_number_start - 1 } + + it_behaves_like 'failing work item task reference removal service', + 'line_number_end must be greater or equal to line_number_start' + end + + context 'when lock_version is older than current' do + let(:lock_version) { work_item.lock_version - 1 } + + it_behaves_like 'failing work item task reference removal service', 'Stale work item. Check lock version' + end + + context 'when work item is stale before updating' do + it_behaves_like 'failing work item task reference removal service', 'Stale work item. Check lock version' do + before do + ::WorkItem.where(id: work_item.id).update_all(lock_version: lock_version + 1) + end + end + end + end +end |