diff options
Diffstat (limited to 'spec/services')
62 files changed, 3222 insertions, 631 deletions
diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index b91234ddb1e..e273dfe1552 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -6,14 +6,15 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do let(:current_params) { {} } let(:rsa_key) { OpenSSL::PKey::RSA.generate(512) } let(:payload) { JWT.decode(subject[:token], rsa_key).first } + let(:authentication_abilities) do - [ - :read_container_image, - :create_container_image - ] + [:read_container_image, :create_container_image] end - subject { described_class.new(current_project, current_user, current_params).execute(authentication_abilities: authentication_abilities) } + subject do + described_class.new(current_project, current_user, current_params) + .execute(authentication_abilities: authentication_abilities) + end before do allow(Gitlab.config.registry).to receive_messages(enabled: true, issuer: 'rspec', key: nil) @@ -40,13 +41,11 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end end - shared_examples 'a accessible' do + shared_examples 'an accessible' do let(:access) do - [{ - 'type' => 'repository', + [{ 'type' => 'repository', 'name' => project.path_with_namespace, - 'actions' => actions, - }] + 'actions' => actions }] end it_behaves_like 'a valid token' @@ -59,19 +58,19 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end shared_examples 'a pullable' do - it_behaves_like 'a accessible' do + it_behaves_like 'an accessible' do let(:actions) { ['pull'] } end end shared_examples 'a pushable' do - it_behaves_like 'a accessible' do + it_behaves_like 'an accessible' do let(:actions) { ['push'] } end end shared_examples 'a pullable and pushable' do - it_behaves_like 'a accessible' do + it_behaves_like 'an accessible' do let(:actions) { %w(pull push) } end end @@ -81,15 +80,30 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do it { is_expected.not_to include(:token) } end + shared_examples 'container repository factory' do + it 'creates a new container repository resource' do + expect { subject } + .to change { project.container_repositories.count }.by(1) + end + end + + shared_examples 'not a container repository factory' do + it 'does not create a new container repository resource' do + expect { subject }.not_to change { ContainerRepository.count } + end + end + describe '#full_access_token' do let(:project) { create(:empty_project) } let(:token) { described_class.full_access_token(project.path_with_namespace) } subject { { token: token } } - it_behaves_like 'a accessible' do + it_behaves_like 'an accessible' do let(:actions) { ['*'] } end + + it_behaves_like 'not a container repository factory' end context 'user authorization' do @@ -110,16 +124,20 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'a pushable' + it_behaves_like 'container repository factory' end context 'allow reporter to pull images' do before { project.team << [current_user, :reporter] } - let(:current_params) do - { scope: "repository:#{project.path_with_namespace}:pull" } - end + context 'when pulling from root level repository' do + let(:current_params) do + { scope: "repository:#{project.path_with_namespace}:pull" } + end - it_behaves_like 'a pullable' + it_behaves_like 'a pullable' + it_behaves_like 'not a container repository factory' + end end context 'return a least of privileges' do @@ -130,6 +148,7 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'a pullable' + it_behaves_like 'not a container repository factory' end context 'disallow guest to pull or push images' do @@ -140,6 +159,7 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'an inaccessible' + it_behaves_like 'not a container repository factory' end end @@ -152,6 +172,7 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'a pullable' + it_behaves_like 'not a container repository factory' end context 'disallow anyone to push images' do @@ -160,6 +181,16 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'an inaccessible' + it_behaves_like 'not a container repository factory' + end + + context 'when repository name is invalid' do + let(:current_params) do + { scope: 'repository:invalid:push' } + end + + it_behaves_like 'an inaccessible' + it_behaves_like 'not a container repository factory' end end @@ -173,6 +204,7 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'a pullable' + it_behaves_like 'not a container repository factory' end context 'disallow anyone to push images' do @@ -181,6 +213,7 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'an inaccessible' + it_behaves_like 'not a container repository factory' end end @@ -191,6 +224,7 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'an inaccessible' + it_behaves_like 'not a container repository factory' end end end @@ -198,11 +232,9 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do context 'build authorized as user' do let(:current_project) { create(:empty_project) } let(:current_user) { create(:user) } + let(:authentication_abilities) do - [ - :build_read_container_image, - :build_create_container_image - ] + [:build_read_container_image, :build_create_container_image] end before do @@ -219,6 +251,10 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do it_behaves_like 'a pullable and pushable' do let(:project) { current_project } end + + it_behaves_like 'container repository factory' do + let(:project) { current_project } + end end context 'for other projects' do @@ -231,11 +267,13 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do let(:project) { create(:empty_project, :public) } it_behaves_like 'a pullable' + it_behaves_like 'not a container repository factory' end shared_examples 'pullable for being team member' do context 'when you are not member' do it_behaves_like 'an inaccessible' + it_behaves_like 'not a container repository factory' end context 'when you are member' do @@ -244,12 +282,14 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'a pullable' + it_behaves_like 'not a container repository factory' end context 'when you are owner' do let(:project) { create(:empty_project, namespace: current_user.namespace) } it_behaves_like 'a pullable' + it_behaves_like 'not a container repository factory' end end @@ -263,6 +303,7 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do context 'when you are not member' do it_behaves_like 'an inaccessible' + it_behaves_like 'not a container repository factory' end context 'when you are member' do @@ -271,12 +312,14 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'a pullable' + it_behaves_like 'not a container repository factory' end context 'when you are owner' do let(:project) { create(:empty_project, namespace: current_user.namespace) } it_behaves_like 'a pullable' + it_behaves_like 'not a container repository factory' end end end @@ -296,12 +339,14 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'an inaccessible' + it_behaves_like 'not a container repository factory' end context 'when you are owner' do let(:project) { create(:empty_project, :public, namespace: current_user.namespace) } it_behaves_like 'an inaccessible' + it_behaves_like 'not a container repository factory' end end end @@ -318,6 +363,7 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'an inaccessible' + it_behaves_like 'not a container repository factory' end end end @@ -325,6 +371,7 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do context 'unauthorized' do context 'disallow to use scope-less authentication' do it_behaves_like 'a forbidden' + it_behaves_like 'not a container repository factory' end context 'for invalid scope' do @@ -333,6 +380,7 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'a forbidden' + it_behaves_like 'not a container repository factory' end context 'for private project' do @@ -354,6 +402,7 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'a pullable' + it_behaves_like 'not a container repository factory' end context 'when pushing' do @@ -362,6 +411,7 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do end it_behaves_like 'a forbidden' + it_behaves_like 'not a container repository factory' end end end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index d2f0337c260..b536103ed65 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -9,72 +9,178 @@ describe Ci::CreatePipelineService, services: true do end describe '#execute' do - def execute(params) + def execute_service(after: project.commit.id, message: 'Message', ref: 'refs/heads/master') + params = { ref: ref, + before: '00000000', + after: after, + commits: [{ message: message }] } + described_class.new(project, user, params).execute end context 'valid params' do - let(:pipeline) do - execute(ref: 'refs/heads/master', - before: '00000000', - after: project.commit.id, - commits: [{ message: "Message" }]) - end - - it { expect(pipeline).to be_kind_of(Ci::Pipeline) } - it { expect(pipeline).to be_valid } - it { expect(pipeline).to be_persisted } - it { expect(pipeline).to eq(project.pipelines.last) } - it { expect(pipeline).to have_attributes(user: user) } - it { expect(pipeline.builds.first).to be_kind_of(Ci::Build) } + let(:pipeline) { execute_service } + + let(:pipeline_on_previous_commit) do + execute_service( + after: previous_commit_sha_from_ref('master') + ) + end + + it 'creates a pipeline' do + expect(pipeline).to be_kind_of(Ci::Pipeline) + expect(pipeline).to be_valid + expect(pipeline).to eq(project.pipelines.last) + expect(pipeline).to have_attributes(user: user) + expect(pipeline).to have_attributes(status: 'pending') + expect(pipeline.builds.first).to be_kind_of(Ci::Build) + end + + context '#update_merge_requests_head_pipeline' do + it 'updates head pipeline of each merge request' do + merge_request_1 = create(:merge_request, source_branch: 'master', target_branch: "branch_1", source_project: project) + merge_request_2 = create(:merge_request, source_branch: 'master', target_branch: "branch_2", source_project: project) + + head_pipeline = pipeline + + expect(merge_request_1.reload.head_pipeline).to eq(head_pipeline) + expect(merge_request_2.reload.head_pipeline).to eq(head_pipeline) + end + + context 'when there is no pipeline for source branch' do + it "does not update merge request head pipeline" do + merge_request = create(:merge_request, source_branch: 'other_branch', target_branch: "branch_1", source_project: project) + + head_pipeline = pipeline + + expect(merge_request.reload.head_pipeline).not_to eq(head_pipeline) + end + end + + context 'when merge request target project is different from source project' do + let!(:target_project) { create(:empty_project) } + let!(:forked_project_link) { create(:forked_project_link, forked_to_project: project, forked_from_project: target_project) } + + it 'updates head pipeline for merge request' do + merge_request = + create(:merge_request, source_branch: 'master', target_branch: "branch_1", source_project: project, target_project: target_project) + + head_pipeline = pipeline + + expect(merge_request.reload.head_pipeline).to eq(head_pipeline) + end + end + end + + context 'auto-cancel enabled' do + before do + project.update(auto_cancel_pending_pipelines: 'enabled') + end + + it 'does not cancel HEAD pipeline' do + pipeline + pipeline_on_previous_commit + + expect(pipeline.reload).to have_attributes(status: 'pending', auto_canceled_by_id: nil) + end + + it 'auto cancel pending non-HEAD pipelines' do + pipeline_on_previous_commit + pipeline + + expect(pipeline_on_previous_commit.reload).to have_attributes(status: 'canceled', auto_canceled_by_id: pipeline.id) + end + + it 'does not cancel running outdated pipelines' do + pipeline_on_previous_commit.run + execute_service + + expect(pipeline_on_previous_commit.reload).to have_attributes(status: 'running', auto_canceled_by_id: nil) + end + + it 'cancel created outdated pipelines' do + pipeline_on_previous_commit.update(status: 'created') + pipeline + + expect(pipeline_on_previous_commit.reload).to have_attributes(status: 'canceled', auto_canceled_by_id: pipeline.id) + end + + it 'does not cancel pipelines from the other branches' do + pending_pipeline = execute_service( + ref: 'refs/heads/feature', + after: previous_commit_sha_from_ref('feature') + ) + pipeline + + expect(pending_pipeline.reload).to have_attributes(status: 'pending', auto_canceled_by_id: nil) + end + end + + context 'auto-cancel disabled' do + before do + project.update(auto_cancel_pending_pipelines: 'disabled') + end + + it 'does not auto cancel pending non-HEAD pipelines' do + pipeline_on_previous_commit + pipeline + + expect(pipeline_on_previous_commit.reload) + .to have_attributes(status: 'pending', auto_canceled_by_id: nil) + end + end + + def previous_commit_sha_from_ref(ref) + project.commit(ref).parent.sha + end end context "skip tag if there is no build for it" do it "creates commit if there is appropriate job" do - result = execute(ref: 'refs/heads/master', - before: '00000000', - after: project.commit.id, - commits: [{ message: "Message" }]) - expect(result).to be_persisted + expect(execute_service).to be_persisted end it "creates commit if there is no appropriate job but deploy job has right ref setting" do config = YAML.dump({ deploy: { script: "ls", only: ["master"] } }) stub_ci_pipeline_yaml_file(config) - result = execute(ref: 'refs/heads/master', - before: '00000000', - after: project.commit.id, - commits: [{ message: "Message" }]) - expect(result).to be_persisted + expect(execute_service).to be_persisted end end it 'skips creating pipeline for refs without .gitlab-ci.yml' do stub_ci_pipeline_yaml_file(nil) - result = execute(ref: 'refs/heads/master', - before: '00000000', - after: project.commit.id, - commits: [{ message: 'Message' }]) - expect(result).not_to be_persisted + expect(execute_service).not_to be_persisted expect(Ci::Pipeline.count).to eq(0) end - it 'fails commits if yaml is invalid' do - message = 'message' - allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { message } - stub_ci_pipeline_yaml_file('invalid: file: file') - commits = [{ message: message }] - pipeline = execute(ref: 'refs/heads/master', - before: '00000000', - after: project.commit.id, - commits: commits) - - expect(pipeline).to be_persisted - expect(pipeline.builds.any?).to be false - expect(pipeline.status).to eq('failed') - expect(pipeline.yaml_errors).not_to be_nil + shared_examples 'a failed pipeline' do + it 'creates failed pipeline' do + stub_ci_pipeline_yaml_file(ci_yaml) + + pipeline = execute_service(message: message) + + expect(pipeline).to be_persisted + expect(pipeline.builds.any?).to be false + expect(pipeline.status).to eq('failed') + expect(pipeline.yaml_errors).not_to be_nil + end + end + + context 'when yaml is invalid' do + let(:ci_yaml) { 'invalid: file: fiile' } + let(:message) { 'Message' } + + it_behaves_like 'a failed pipeline' + + context 'when receive git commit' do + before do + allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { message } + end + + it_behaves_like 'a failed pipeline' + end end context 'when commit contains a [ci skip] directive' do @@ -97,11 +203,7 @@ describe Ci::CreatePipelineService, services: true do ci_messages.each do |ci_message| it "skips builds creation if the commit message is #{ci_message}" do - commits = [{ message: ci_message }] - pipeline = execute(ref: 'refs/heads/master', - before: '00000000', - after: project.commit.id, - commits: commits) + pipeline = execute_service(message: ci_message) expect(pipeline).to be_persisted expect(pipeline.builds.any?).to be false @@ -109,58 +211,34 @@ describe Ci::CreatePipelineService, services: true do end end - it "does not skips builds creation if there is no [ci skip] or [skip ci] tag in commit message" do - allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { "some message" } + shared_examples 'creating a pipeline' do + it 'does not skip pipeline creation' do + allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { commit_message } - commits = [{ message: "some message" }] - pipeline = execute(ref: 'refs/heads/master', - before: '00000000', - after: project.commit.id, - commits: commits) + pipeline = execute_service(message: commit_message) - expect(pipeline).to be_persisted - expect(pipeline.builds.first.name).to eq("rspec") + expect(pipeline).to be_persisted + expect(pipeline.builds.first.name).to eq("rspec") + end end - it "does not skip builds creation if the commit message is nil" do - allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { nil } + context 'when commit message does not contain [ci skip] nor [skip ci]' do + let(:commit_message) { 'some message' } - commits = [{ message: nil }] - pipeline = execute(ref: 'refs/heads/master', - before: '00000000', - after: project.commit.id, - commits: commits) - - expect(pipeline).to be_persisted - expect(pipeline.builds.first.name).to eq("rspec") + it_behaves_like 'creating a pipeline' end - it "fails builds creation if there is [ci skip] tag in commit message and yaml is invalid" do - stub_ci_pipeline_yaml_file('invalid: file: fiile') - commits = [{ message: message }] - pipeline = execute(ref: 'refs/heads/master', - before: '00000000', - after: project.commit.id, - commits: commits) + context 'when commit message is nil' do + let(:commit_message) { nil } - expect(pipeline).to be_persisted - expect(pipeline.builds.any?).to be false - expect(pipeline.status).to eq("failed") - expect(pipeline.yaml_errors).not_to be_nil + it_behaves_like 'creating a pipeline' end - end - it "creates commit with failed status if yaml is invalid" do - stub_ci_pipeline_yaml_file('invalid: file') - commits = [{ message: "some message" }] - pipeline = execute(ref: 'refs/heads/master', - before: '00000000', - after: project.commit.id, - commits: commits) - - expect(pipeline).to be_persisted - expect(pipeline.status).to eq("failed") - expect(pipeline.builds.any?).to be false + context 'when there is [ci skip] tag in commit message and yaml is invalid' do + let(:ci_yaml) { 'invalid: file: fiile' } + + it_behaves_like 'a failed pipeline' + end end context 'when there are no jobs for this pipeline' do @@ -170,10 +248,7 @@ describe Ci::CreatePipelineService, services: true do end it 'does not create a new pipeline' do - result = execute(ref: 'refs/heads/master', - before: '00000000', - after: project.commit.id, - commits: [{ message: 'some msg' }]) + result = execute_service expect(result).not_to be_persisted expect(Ci::Build.all).to be_empty @@ -188,10 +263,7 @@ describe Ci::CreatePipelineService, services: true do end it 'does not create a new pipeline' do - result = execute(ref: 'refs/heads/master', - before: '00000000', - after: project.commit.id, - commits: [{ message: 'some msg' }]) + result = execute_service expect(result).to be_persisted expect(result.manual_actions).not_to be_empty @@ -205,10 +277,7 @@ describe Ci::CreatePipelineService, services: true do end it 'creates the environment' do - result = execute(ref: 'refs/heads/master', - before: '00000000', - after: project.commit.id, - commits: [{ message: 'some msg' }]) + result = execute_service expect(result).to be_persisted expect(Environment.find_by(name: "review/master")).not_to be_nil diff --git a/spec/services/ci/play_build_service_spec.rb b/spec/services/ci/play_build_service_spec.rb new file mode 100644 index 00000000000..d6f9fa42045 --- /dev/null +++ b/spec/services/ci/play_build_service_spec.rb @@ -0,0 +1,105 @@ +require 'spec_helper' + +describe Ci::PlayBuildService, '#execute', :services do + let(:user) { create(:user) } + let(:project) { create(:empty_project) } + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:build) { create(:ci_build, :manual, pipeline: pipeline) } + + let(:service) do + described_class.new(project, user) + end + + context 'when project does not have repository yet' do + let(:project) { create(:empty_project) } + + it 'allows user with master role to play build' do + project.add_master(user) + + service.execute(build) + + expect(build.reload).to be_pending + end + + it 'does not allow user with developer role to play build' do + project.add_developer(user) + + expect { service.execute(build) } + .to raise_error Gitlab::Access::AccessDeniedError + end + end + + context 'when project has repository' do + let(:project) { create(:project) } + + it 'allows user with developer role to play a build' do + project.add_developer(user) + + service.execute(build) + + expect(build.reload).to be_pending + end + end + + context 'when build is a playable manual action' do + let(:build) { create(:ci_build, :manual, pipeline: pipeline) } + + before do + project.add_master(user) + end + + it 'enqueues the build' do + expect(service.execute(build)).to eq build + expect(build.reload).to be_pending + end + + it 'reassignes build user correctly' do + service.execute(build) + + expect(build.reload.user).to eq user + end + end + + context 'when build is not a playable manual action' do + let(:build) { create(:ci_build, when: :manual, pipeline: pipeline) } + + before do + project.add_master(user) + end + + it 'duplicates the build' do + duplicate = service.execute(build) + + expect(duplicate).not_to eq build + expect(duplicate).to be_pending + end + + it 'assigns users correctly' do + duplicate = service.execute(build) + + expect(build.user).not_to eq user + expect(duplicate.user).to eq user + end + end + + context 'when build is not action' do + let(:build) { create(:ci_build, :success, pipeline: pipeline) } + + it 'raises an error' do + expect { service.execute(build) } + .to raise_error Gitlab::Access::AccessDeniedError + end + end + + context 'when user does not have ability to trigger action' do + before do + create(:protected_branch, :no_one_can_push, + name: build.ref, project: project) + end + + it 'raises an error' do + expect { service.execute(build) } + .to raise_error Gitlab::Access::AccessDeniedError + end + end +end diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb index d93616c4f50..fc5de5d069a 100644 --- a/spec/services/ci/process_pipeline_service_spec.rb +++ b/spec/services/ci/process_pipeline_service_spec.rb @@ -268,6 +268,24 @@ describe Ci::ProcessPipelineService, '#execute', :services do end end + context 'when there are only manual actions in stages' do + before do + create_build('image', stage_idx: 0, when: 'manual', allow_failure: true) + create_build('build', stage_idx: 1, when: 'manual', allow_failure: true) + create_build('deploy', stage_idx: 2, when: 'manual') + create_build('check', stage_idx: 3) + + process_pipeline + end + + it 'processes all jobs until blocking actions encountered' do + expect(all_builds_statuses).to eq(%w[manual manual manual created]) + expect(all_builds_names).to eq(%w[image build deploy check]) + + expect(pipeline.reload).to be_blocked + end + end + context 'when blocking manual actions are defined' do before do create_build('code:test', stage_idx: 0) @@ -314,6 +332,13 @@ describe Ci::ProcessPipelineService, '#execute', :services do end context 'when pipeline is promoted sequentially up to the end' do + before do + # We are using create(:empty_project), and users has to be master in + # order to execute manual action when repository does not exist. + # + project.add_master(user) + end + it 'properly processes entire pipeline' do process_pipeline @@ -418,62 +443,18 @@ describe Ci::ProcessPipelineService, '#execute', :services do end end - context 'when there are builds that are not created yet' do - let(:pipeline) do - create(:ci_pipeline, config: config) - end - - let(:config) do - { rspec: { stage: 'test', script: 'rspec' }, - deploy: { stage: 'deploy', script: 'rsync' } } - end - - before do - create_build('linux', stage: 'build', stage_idx: 0) - create_build('mac', stage: 'build', stage_idx: 0) - end + context 'updates a list of retried builds' do + subject { described_class.retried.order(:id) } - it 'processes the pipeline' do - # Currently we have five builds with state created - # - expect(builds.count).to eq(0) - expect(all_builds.count).to eq(2) + let!(:build_retried) { create_build('build') } + let!(:build) { create_build('build') } + let!(:test) { create_build('test') } - # Process builds service will enqueue builds from the first stage. - # + it 'returns unique statuses' do process_pipeline - expect(builds.count).to eq(2) - expect(all_builds.count).to eq(2) - - # When builds succeed we will enqueue remaining builds. - # - # We will have 2 succeeded, 1 pending (from stage test), total 4 (two - # additional build from `.gitlab-ci.yml`). - # - succeed_pending - process_pipeline - - expect(builds.success.count).to eq(2) - expect(builds.pending.count).to eq(1) - expect(all_builds.count).to eq(4) - - # When pending merge_when_pipeline_succeeds in stage test, we enqueue deploy stage. - # - succeed_pending - process_pipeline - - expect(builds.pending.count).to eq(1) - expect(builds.success.count).to eq(3) - expect(all_builds.count).to eq(4) - - # When the last one succeeds we have 4 successful builds. - # - succeed_pending - process_pipeline - - expect(builds.success.count).to eq(4) - expect(all_builds.count).to eq(4) + expect(all_builds.latest).to contain_exactly(build, test) + expect(all_builds.retried).to contain_exactly(build_retried) end end @@ -493,6 +474,10 @@ describe Ci::ProcessPipelineService, '#execute', :services do builds.pluck(:name) end + def all_builds_names + all_builds.pluck(:name) + end + def builds_statuses builds.pluck(:status) end @@ -521,7 +506,9 @@ describe Ci::ProcessPipelineService, '#execute', :services do builds.find_by(name: name).play(user) end - delegate :manual_actions, to: :pipeline + def manual_actions + pipeline.manual_actions(true) + end def create_build(name, **opts) create(:ci_build, :created, pipeline: pipeline, name: name, **opts) diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 8567817147b..7254e6b357a 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -16,20 +16,21 @@ describe Ci::RetryBuildService, :services do %i[id status user token coverage trace runner artifacts_expire_at artifacts_file artifacts_metadata artifacts_size created_at updated_at started_at finished_at queued_at erased_by - erased_at].freeze + erased_at auto_canceled_by].freeze IGNORE_ACCESSORS = %i[type lock_version target_url base_tags commit_id deployments erased_by_id last_deployment project_id runner_id tag_taggings taggings tags trigger_request_id - user_id].freeze + user_id auto_canceled_by_id retried].freeze shared_examples 'build duplication' do let(:build) do create(:ci_build, :failed, :artifacts_expired, :erased, :queued, :coverage, :tags, :allowed_to_fail, :on_tag, :teardown_environment, :triggered, :trace, - description: 'some build', pipeline: pipeline) + description: 'some build', pipeline: pipeline, + auto_canceled_by: create(:ci_empty_pipeline)) end describe 'clone accessors' do @@ -114,7 +115,7 @@ describe Ci::RetryBuildService, :services do end describe '#reprocess' do - let(:new_build) { service.reprocess(build) } + let(:new_build) { service.reprocess!(build) } context 'when user has ability to execute build' do before do @@ -130,11 +131,16 @@ describe Ci::RetryBuildService, :services do it 'does not enqueue the new build' do expect(new_build).to be_created end + + it 'does mark old build as retried' do + expect(new_build).to be_latest + expect(build.reload).to be_retried + end end context 'when user does not have ability to execute build' do it 'raises an error' do - expect { service.reprocess(build) } + expect { service.reprocess!(build) } .to raise_error Gitlab::Access::AccessDeniedError end end diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index f1b2d3a4798..d941d56c0d8 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -7,11 +7,13 @@ describe Ci::RetryPipelineService, '#execute', :services do let(:service) { described_class.new(project, user) } context 'when user has ability to modify pipeline' do - let(:user) { create(:admin) } + before do + project.add_master(user) + end context 'when there are already retried jobs present' do before do - create_build('rspec', :canceled, 0) + create_build('rspec', :canceled, 0, retried: true) create_build('rspec', :failed, 0) end @@ -227,6 +229,46 @@ describe Ci::RetryPipelineService, '#execute', :services do end end + context 'when user is not allowed to trigger manual action' do + before do + project.add_developer(user) + end + + context 'when there is a failed manual action present' do + before do + create_build('test', :failed, 0) + create_build('deploy', :failed, 0, when: :manual) + create_build('verify', :canceled, 1) + end + + it 'does not reprocess manual action' do + service.execute(pipeline) + + expect(build('test')).to be_pending + expect(build('deploy')).to be_failed + expect(build('verify')).to be_created + expect(pipeline.reload).to be_running + end + end + + context 'when there is a failed manual action in later stage' do + before do + create_build('test', :failed, 0) + create_build('deploy', :failed, 1, when: :manual) + create_build('verify', :canceled, 2) + end + + it 'does not reprocess manual action' do + service.execute(pipeline) + + expect(build('test')).to be_pending + expect(build('deploy')).to be_failed + expect(build('verify')).to be_created + expect(pipeline.reload).to be_running + end + end + end + def statuses pipeline.reload.statuses end diff --git a/spec/services/ci/stop_environments_service_spec.rb b/spec/services/ci/stop_environments_service_spec.rb index 32c72a9cf5e..98044ad232e 100644 --- a/spec/services/ci/stop_environments_service_spec.rb +++ b/spec/services/ci/stop_environments_service_spec.rb @@ -55,8 +55,22 @@ describe Ci::StopEnvironmentsService, services: true do end context 'when user does not have permission to stop environment' do + context 'when user has no access to manage deployments' do + before do + project.team << [user, :guest] + end + + it 'does not stop environment' do + expect_environment_not_stopped_on('master') + end + end + end + + context 'when branch for stop action is protected' do before do - project.team << [user, :guest] + project.add_developer(user) + create(:protected_branch, :no_one_can_push, + name: 'master', project: project) end it 'does not stop environment' do diff --git a/spec/services/cohorts_service_spec.rb b/spec/services/cohorts_service_spec.rb new file mode 100644 index 00000000000..77595d7ba2d --- /dev/null +++ b/spec/services/cohorts_service_spec.rb @@ -0,0 +1,99 @@ +require 'spec_helper' + +describe CohortsService do + describe '#execute' do + def month_start(months_ago) + months_ago.months.ago.beginning_of_month.to_date + end + + # In the interests of speed and clarity, this example has minimal data. + it 'returns a list of user cohorts' do + 6.times do |months_ago| + months_ago_time = (months_ago * 2).months.ago + + create(:user, created_at: months_ago_time, last_activity_on: Time.now) + create(:user, created_at: months_ago_time, last_activity_on: months_ago_time) + end + + create(:user) # this user is inactive and belongs to the current month + + expected_cohorts = [ + { + registration_month: month_start(11), + activity_months: Array.new(12) { { total: 0, percentage: 0 } }, + total: 0, + inactive: 0 + }, + { + registration_month: month_start(10), + activity_months: [{ total: 2, percentage: 100 }] + Array.new(10) { { total: 1, percentage: 50 } }, + total: 2, + inactive: 0 + }, + { + registration_month: month_start(9), + activity_months: Array.new(10) { { total: 0, percentage: 0 } }, + total: 0, + inactive: 0 + }, + { + registration_month: month_start(8), + activity_months: [{ total: 2, percentage: 100 }] + Array.new(8) { { total: 1, percentage: 50 } }, + total: 2, + inactive: 0 + }, + { + registration_month: month_start(7), + activity_months: Array.new(8) { { total: 0, percentage: 0 } }, + total: 0, + inactive: 0 + }, + { + registration_month: month_start(6), + activity_months: [{ total: 2, percentage: 100 }] + Array.new(6) { { total: 1, percentage: 50 } }, + total: 2, + inactive: 0 + }, + { + registration_month: month_start(5), + activity_months: Array.new(6) { { total: 0, percentage: 0 } }, + total: 0, + inactive: 0 + }, + { + registration_month: month_start(4), + activity_months: [{ total: 2, percentage: 100 }] + Array.new(4) { { total: 1, percentage: 50 } }, + total: 2, + inactive: 0 + }, + { + registration_month: month_start(3), + activity_months: Array.new(4) { { total: 0, percentage: 0 } }, + total: 0, + inactive: 0 + }, + { + registration_month: month_start(2), + activity_months: [{ total: 2, percentage: 100 }] + Array.new(2) { { total: 1, percentage: 50 } }, + total: 2, + inactive: 0 + }, + { + registration_month: month_start(1), + activity_months: Array.new(2) { { total: 0, percentage: 0 } }, + total: 0, + inactive: 0 + }, + { + registration_month: month_start(0), + activity_months: [{ total: 2, percentage: 100 }], + total: 2, + inactive: 1 + } + ] + + expect(described_class.new.execute).to eq(months_included: 12, + cohorts: expected_cohorts) + end + end +end diff --git a/spec/services/create_deployment_service_spec.rb b/spec/services/create_deployment_service_spec.rb index a883705bd45..f35d7a33548 100644 --- a/spec/services/create_deployment_service_spec.rb +++ b/spec/services/create_deployment_service_spec.rb @@ -255,7 +255,7 @@ describe CreateDeploymentService, services: true do environment: 'production', ref: 'master', tag: false, - sha: '97de212e80737a608d939f648d959671fb0a0142b', + sha: '97de212e80737a608d939f648d959671fb0a0142b' } end diff --git a/spec/services/delete_merged_branches_service_spec.rb b/spec/services/delete_merged_branches_service_spec.rb index a41a421fa6e..cae74df9c90 100644 --- a/spec/services/delete_merged_branches_service_spec.rb +++ b/spec/services/delete_merged_branches_service_spec.rb @@ -6,33 +6,22 @@ describe DeleteMergedBranchesService, services: true do let(:project) { create(:project, :repository) } context '#execute' do - context 'unprotected branches' do - before do - service.execute - end + it 'deletes a branch that was merged' do + service.execute - it 'deletes a branch that was merged' do - expect(project.repository.branch_names).not_to include('improve/awesome') - end + expect(project.repository.branch_names).not_to include('improve/awesome') + end - it 'keeps branch that is unmerged' do - expect(project.repository.branch_names).to include('feature') - end + it 'keeps branch that is unmerged' do + service.execute - it 'keeps "master"' do - expect(project.repository.branch_names).to include('master') - end + expect(project.repository.branch_names).to include('feature') end - context 'protected branches' do - before do - create(:protected_branch, name: 'improve/awesome', project: project) - service.execute - end + it 'keeps "master"' do + service.execute - it 'keeps protected branch' do - expect(project.repository.branch_names).to include('improve/awesome') - end + expect(project.repository.branch_names).to include('master') end context 'user without rights' do @@ -42,6 +31,19 @@ describe DeleteMergedBranchesService, services: true do expect { described_class.new(project, user).execute }.to raise_error(Gitlab::Access::AccessDeniedError) end end + + context 'open merge requests' do + it 'does not delete branches from open merge requests' do + fork_link = create(:forked_project_link, forked_from_project: project) + create(:merge_request, :reopened, source_project: project, target_project: project, source_branch: 'branch-merged', target_branch: 'master') + create(:merge_request, :opened, source_project: fork_link.forked_to_project, target_project: project, target_branch: 'improve/awesome', source_branch: 'master') + + service.execute + + expect(project.repository.branch_names).to include('branch-merged') + expect(project.repository.branch_names).to include('improve/awesome') + end + end end context '#async_execute' do diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb index 12c3cdf28c6..ab8df7b74cd 100644 --- a/spec/services/discussions/resolve_service_spec.rb +++ b/spec/services/discussions/resolve_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Discussions::ResolveService do describe '#execute' do - let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:discussion) { create(:diff_note_on_merge_request).to_discussion } let(:project) { merge_request.project } let(:merge_request) { discussion.noteable } let(:user) { create(:user) } @@ -41,7 +41,7 @@ describe Discussions::ResolveService do end it 'can resolve multiple discussions at once' do - other_discussion = Discussion.for_diff_notes([create(:diff_note_on_merge_request, noteable: discussion.noteable, project: discussion.noteable.source_project)]).first + other_discussion = create(:diff_note_on_merge_request, noteable: discussion.noteable, project: discussion.noteable.source_project).to_discussion service.execute([discussion, other_discussion]) diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index f2c2009bcbf..b06cefe071d 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe EventCreateService, services: true do + include UserActivitiesHelpers + let(:service) { EventCreateService.new } describe 'Issues' do @@ -111,6 +113,19 @@ describe EventCreateService, services: true do end end + describe '#push', :redis do + let(:project) { create(:empty_project) } + let(:user) { create(:user) } + + it 'creates a new event' do + expect { service.push(project, user, {}) }.to change { Event.count } + end + + it 'updates user last activity' do + expect { service.push(project, user, {}) }.to change { user_activity(user) } + end + end + describe 'Project' do let(:user) { create :user } let(:project) { create(:empty_project) } diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb index 26aa5b432d4..16bca66766a 100644 --- a/spec/services/files/update_service_spec.rb +++ b/spec/services/files/update_service_spec.rb @@ -7,7 +7,7 @@ describe Files::UpdateService do let(:user) { create(:user) } let(:file_path) { 'files/ruby/popen.rb' } let(:new_contents) { 'New Content' } - let(:target_branch) { project.default_branch } + let(:branch_name) { project.default_branch } let(:last_commit_sha) { nil } let(:commit_params) do @@ -19,7 +19,7 @@ describe Files::UpdateService do last_commit_sha: last_commit_sha, start_project: project, start_branch: project.default_branch, - target_branch: target_branch + branch_name: branch_name } end @@ -73,7 +73,7 @@ describe Files::UpdateService do end context 'when target branch is different than source branch' do - let(:target_branch) { "#{project.default_branch}-new" } + let(:branch_name) { "#{project.default_branch}-new" } it 'fires hooks only once' do expect(GitHooksService).to receive(:new).once.and_call_original diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 0477cac6677..ab06f45dbb9 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -584,7 +584,7 @@ describe GitPushService, services: true do commit = double(:commit) diff = double(:diff, new_path: 'README.md') - expect(commit).to receive(:raw_diffs).with(deltas_only: true). + expect(commit).to receive(:raw_deltas). and_return([diff]) service.push_commits = [commit] @@ -622,12 +622,21 @@ describe GitPushService, services: true do it 'only schedules a limited number of commits' do allow(service).to receive(:push_commits). - and_return(Array.new(1000, double(:commit, to_hash: {}))) + and_return(Array.new(1000, double(:commit, to_hash: {}, matches_cross_reference_regex?: true))) expect(ProcessCommitWorker).to receive(:perform_async).exactly(100).times service.process_commit_messages end + + it "skips commits which don't include cross-references" do + allow(service).to receive(:push_commits). + and_return([double(:commit, to_hash: {}, matches_cross_reference_regex?: false)]) + + expect(ProcessCommitWorker).not_to receive(:perform_async) + + service.process_commit_messages + end end def execute_service(project, user, oldrev, newrev, ref) diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index 2ee11fc8b4c..a37257d1bf4 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -7,6 +7,7 @@ describe Groups::DestroyService, services: true do let!(:group) { create(:group) } let!(:nested_group) { create(:group, parent: group) } let!(:project) { create(:empty_project, namespace: group) } + let!(:notification_setting) { create(:notification_setting, source: group)} let!(:gitlab_shell) { Gitlab::Shell.new } let!(:remove_path) { group.path + "+#{group.id}+deleted" } @@ -23,6 +24,7 @@ describe Groups::DestroyService, services: true do it { expect(Group.unscoped.all).not_to include(group) } it { expect(Group.unscoped.all).not_to include(nested_group) } it { expect(Project.unscoped.all).not_to include(project) } + it { expect(NotificationSetting.unscoped.all).not_to include(notification_setting) } end context 'file system' do diff --git a/spec/services/issuable/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb index 7a1ac027310..6437d00e451 100644 --- a/spec/services/issuable/bulk_update_service_spec.rb +++ b/spec/services/issuable/bulk_update_service_spec.rb @@ -4,11 +4,12 @@ describe Issuable::BulkUpdateService, services: true do let(:user) { create(:user) } let(:project) { create(:empty_project, namespace: user.namespace) } - def bulk_update(issues, extra_params = {}) + def bulk_update(issuables, extra_params = {}) bulk_update_params = extra_params - .reverse_merge(issuable_ids: Array(issues).map(&:id).join(',')) + .reverse_merge(issuable_ids: Array(issuables).map(&:id).join(',')) - Issuable::BulkUpdateService.new(project, user, bulk_update_params).execute('issue') + type = Array(issuables).first.model_name.param_key + Issuable::BulkUpdateService.new(project, user, bulk_update_params).execute(type) end describe 'close issues' do @@ -47,40 +48,77 @@ describe Issuable::BulkUpdateService, services: true do end end - describe 'updating assignee' do - let(:issue) { create(:issue, project: project, assignee: user) } + describe 'updating merge request assignee' do + let(:merge_request) { create(:merge_request, target_project: project, source_project: project, assignee: user) } context 'when the new assignee ID is a valid user' do it 'succeeds' do new_assignee = create(:user) project.team << [new_assignee, :developer] - result = bulk_update(issue, assignee_id: new_assignee.id) + result = bulk_update(merge_request, assignee_id: new_assignee.id) expect(result[:success]).to be_truthy expect(result[:count]).to eq(1) end - it 'updates the assignee to the use ID passed' do + it 'updates the assignee to the user ID passed' do assignee = create(:user) project.team << [assignee, :developer] - expect { bulk_update(issue, assignee_id: assignee.id) } - .to change { issue.reload.assignee }.from(user).to(assignee) + expect { bulk_update(merge_request, assignee_id: assignee.id) } + .to change { merge_request.reload.assignee }.from(user).to(assignee) end end context "when the new assignee ID is #{IssuableFinder::NONE}" do it "unassigns the issues" do - expect { bulk_update(issue, assignee_id: IssuableFinder::NONE) } - .to change { issue.reload.assignee }.to(nil) + expect { bulk_update(merge_request, assignee_id: IssuableFinder::NONE) } + .to change { merge_request.reload.assignee }.to(nil) end end context 'when the new assignee ID is not present' do it 'does not unassign' do - expect { bulk_update(issue, assignee_id: nil) } - .not_to change { issue.reload.assignee } + expect { bulk_update(merge_request, assignee_id: nil) } + .not_to change { merge_request.reload.assignee } + end + end + end + + describe 'updating issue assignee' do + let(:issue) { create(:issue, project: project, assignees: [user]) } + + context 'when the new assignee ID is a valid user' do + it 'succeeds' do + new_assignee = create(:user) + project.team << [new_assignee, :developer] + + result = bulk_update(issue, assignee_ids: [new_assignee.id]) + + expect(result[:success]).to be_truthy + expect(result[:count]).to eq(1) + end + + it 'updates the assignee to the user ID passed' do + assignee = create(:user) + project.team << [assignee, :developer] + expect { bulk_update(issue, assignee_ids: [assignee.id]) } + .to change { issue.reload.assignees.first }.from(user).to(assignee) + end + end + + context "when the new assignee ID is #{IssuableFinder::NONE}" do + it "unassigns the issues" do + expect { bulk_update(issue, assignee_ids: [IssuableFinder::NONE.to_s]) } + .to change { issue.reload.assignees.count }.from(1).to(0) + end + end + + context 'when the new assignee ID is not present' do + it 'does not unassign' do + expect { bulk_update(issue, assignee_ids: []) } + .not_to change{ issue.reload.assignees } end end end @@ -125,7 +163,7 @@ describe Issuable::BulkUpdateService, services: true do { label_ids: labels.map(&:id), add_label_ids: add_labels.map(&:id), - remove_label_ids: remove_labels.map(&:id), + remove_label_ids: remove_labels.map(&:id) } end diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb index 17990f41b3b..bed25fe7ccf 100644 --- a/spec/services/issues/build_service_spec.rb +++ b/spec/services/issues/build_service_spec.rb @@ -11,7 +11,7 @@ describe Issues::BuildService, services: true do context 'for a single discussion' do describe '#execute' do let(:merge_request) { create(:merge_request, title: "Hello world", source_project: project) } - let(:discussion) { Discussion.new([create(:diff_note_on_merge_request, project: project, noteable: merge_request, note: "Almost done")]) } + let(:discussion) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, note: "Almost done").to_discussion } let(:service) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid, discussion_to_resolve: discussion.id) } it 'references the noteable title in the issue title' do @@ -47,7 +47,7 @@ describe Issues::BuildService, services: true do let(:service) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid) } it 'mentions the author of the note' do - discussion = Discussion.new([create(:diff_note_on_merge_request, author: create(:user, username: 'author'))]) + discussion = create(:diff_note_on_merge_request, author: create(:user, username: 'author')).to_discussion expect(service.item_for_discussion(discussion)).to include('@author') end @@ -60,7 +60,7 @@ describe Issues::BuildService, services: true do note_result = " > This is a string\n"\ " > > with a blockquote\n"\ " > > > That has a quote\n" - discussion = Discussion.new([create(:diff_note_on_merge_request, note: note_text)]) + discussion = create(:diff_note_on_merge_request, note: note_text).to_discussion expect(service.item_for_discussion(discussion)).to include(note_result) end end @@ -91,25 +91,23 @@ describe Issues::BuildService, services: true do end describe 'with multiple discussions' do - before do - create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.target_project, line_number: 15) - end + let!(:diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.target_project, line_number: 15) } it 'mentions all the authors in the description' do - authors = merge_request.diff_discussions.map(&:author) + authors = merge_request.resolvable_discussions.map(&:author) expect(issue.description).to include(*authors.map(&:to_reference)) end it 'has a link for each unresolved discussion in the description' do - notes = merge_request.diff_discussions.map(&:first_note) + notes = merge_request.resolvable_discussions.map(&:first_note) links = notes.map { |note| Gitlab::UrlBuilder.build(note) } expect(issue.description).to include(*links) end it 'mentions additional notes' do - create_list(:diff_note_on_merge_request, 2, noteable: merge_request, project: merge_request.target_project, line_number: 15) + create_list(:diff_note_on_merge_request, 2, noteable: merge_request, project: merge_request.target_project, in_reply_to: diff_note) expect(issue.description).to include('(+2 comments)') end @@ -138,7 +136,7 @@ describe Issues::BuildService, services: true do user, title: 'Issue #1', description: 'Issue description', - milestone_id: milestone.id, + milestone_id: milestone.id ).execute expect(issue.title).to eq('Issue #1') diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 7a54373963e..0a1f41719f7 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -4,7 +4,7 @@ describe Issues::CloseService, services: true do let(:user) { create(:user) } let(:user2) { create(:user) } let(:guest) { create(:user) } - let(:issue) { create(:issue, assignee: user2) } + let(:issue) { create(:issue, assignees: [user2]) } let(:project) { issue.project } let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) } @@ -51,8 +51,10 @@ describe Issues::CloseService, services: true do end end - it { expect(issue).to be_valid } - it { expect(issue).to be_closed } + it 'closes the issue' do + expect(issue).to be_valid + expect(issue).to be_closed + end it 'sends email to user2 about assign of new issue' do email = ActionMailer::Base.deliveries.last @@ -96,9 +98,11 @@ describe Issues::CloseService, services: true do described_class.new(project, user).close_issue(issue) end - it { expect(issue).to be_valid } - it { expect(issue).to be_opened } - it { expect(todo.reload).to be_pending } + it 'closes the issue' do + expect(issue).to be_valid + expect(issue).to be_opened + expect(todo.reload).to be_pending + end end end end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 776cbc4296b..dab1a3469f7 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -6,10 +6,10 @@ describe Issues::CreateService, services: true do describe '#execute' do let(:issue) { described_class.new(project, user, opts).execute } + let(:assignee) { create(:user) } + let(:milestone) { create(:milestone, project: project) } context 'when params are valid' do - let(:assignee) { create(:user) } - let(:milestone) { create(:milestone, project: project) } let(:labels) { create_pair(:label, project: project) } before do @@ -20,7 +20,7 @@ describe Issues::CreateService, services: true do let(:opts) do { title: 'Awesome issue', description: 'please fix', - assignee_id: assignee.id, + assignee_ids: [assignee.id], label_ids: labels.map(&:id), milestone_id: milestone.id, due_date: Date.tomorrow } @@ -29,7 +29,7 @@ describe Issues::CreateService, services: true do it 'creates the issue with the given params' do expect(issue).to be_persisted expect(issue.title).to eq('Awesome issue') - expect(issue.assignee).to eq assignee + expect(issue.assignees).to eq [assignee] expect(issue.labels).to match_array labels expect(issue.milestone).to eq milestone expect(issue.due_date).to eq Date.tomorrow @@ -37,6 +37,7 @@ describe Issues::CreateService, services: true do context 'when current user cannot admin issues in the project' do let(:guest) { create(:user) } + before do project.team << [guest, :guest] end @@ -47,7 +48,7 @@ describe Issues::CreateService, services: true do expect(issue).to be_persisted expect(issue.title).to eq('Awesome issue') expect(issue.description).to eq('please fix') - expect(issue.assignee).to be_nil + expect(issue.assignees).to be_empty expect(issue.labels).to be_empty expect(issue.milestone).to be_nil expect(issue.due_date).to be_nil @@ -117,6 +118,22 @@ describe Issues::CreateService, services: true do end end + context 'when assignee is set' do + let(:opts) do + { title: 'Title', + description: 'Description', + assignees: [assignee] } + end + + it 'invalidates open issues counter for assignees when issue is assigned' do + project.team << [assignee, :master] + + described_class.new(project, user, opts).execute + + expect(assignee.assigned_open_issues_count).to eq 1 + end + end + it 'executes issue hooks when issue is not confidential' do opts = { title: 'Title', description: 'Description', confidential: false } @@ -136,12 +153,85 @@ describe Issues::CreateService, services: true do end end - it_behaves_like 'issuable create service' + context 'issue create service' do + context 'assignees' do + before { project.team << [user, :master] } + + it 'removes assignee when user id is invalid' do + opts = { title: 'Title', description: 'Description', assignee_ids: [-1] } + + issue = described_class.new(project, user, opts).execute + + expect(issue.assignees).to be_empty + end + + it 'removes assignee when user id is 0' do + opts = { title: 'Title', description: 'Description', assignee_ids: [0] } + + issue = described_class.new(project, user, opts).execute + + expect(issue.assignees).to be_empty + end + + it 'saves assignee when user id is valid' do + project.team << [assignee, :master] + opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } + + issue = described_class.new(project, user, opts).execute + + expect(issue.assignees).to eq([assignee]) + end + + context "when issuable feature is private" do + before do + project.project_feature.update(issues_access_level: ProjectFeature::PRIVATE, + merge_requests_access_level: ProjectFeature::PRIVATE) + end + + levels = [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC] + + levels.each do |level| + it "removes not authorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do + project.update(visibility_level: level) + opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } + + issue = described_class.new(project, user, opts).execute + + expect(issue.assignees).to be_empty + end + end + end + end + end it_behaves_like 'new issuable record that supports slash commands' + context 'Slash commands' do + context 'with assignee and milestone in params and command' do + let(:opts) do + { + assignee_ids: [create(:user).id], + milestone_id: 1, + title: 'Title', + description: %(/assign @#{assignee.username}\n/milestone %"#{milestone.name}") + } + end + + before do + project.team << [user, :master] + project.team << [assignee, :master] + end + + it 'assigns and sets milestone to issuable from command' do + expect(issue).to be_persisted + expect(issue.assignees).to eq([assignee]) + expect(issue.milestone).to eq(milestone) + end + end + end + context 'resolving discussions' do - let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:discussion) { create(:diff_note_on_merge_request).to_discussion } let(:merge_request) { discussion.noteable } let(:project) { merge_request.source_project } diff --git a/spec/services/issues/resolve_discussions_spec.rb b/spec/services/issues/resolve_discussions_spec.rb index 3a72f92383c..86f218dec12 100644 --- a/spec/services/issues/resolve_discussions_spec.rb +++ b/spec/services/issues/resolve_discussions_spec.rb @@ -1,15 +1,15 @@ require 'spec_helper.rb' -class DummyService < Issues::BaseService - include ::Issues::ResolveDiscussions +describe Issues::ResolveDiscussions, services: true do + class DummyService < Issues::BaseService + include ::Issues::ResolveDiscussions - def initialize(*args) - super - filter_resolve_discussion_params + def initialize(*args) + super + filter_resolve_discussion_params + end end -end -describe DummyService, services: true do let(:project) { create(:project, :repository) } let(:user) { create(:user) } @@ -18,12 +18,12 @@ describe DummyService, services: true do end describe "for resolving discussions" do - let(:discussion) { Discussion.new([create(:diff_note_on_merge_request, project: project, note: "Almost done")]) } + let(:discussion) { create(:diff_note_on_merge_request, project: project, note: "Almost done").to_discussion } let(:merge_request) { discussion.noteable } let(:other_merge_request) { create(:merge_request, source_project: project, source_branch: "other") } describe "#merge_request_for_resolving_discussion" do - let(:service) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid) } + let(:service) { DummyService.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid) } it "finds the merge request" do expect(service.merge_request_to_resolve_discussions_of).to eq(merge_request) @@ -43,7 +43,7 @@ describe DummyService, services: true do describe "#discussions_to_resolve" do it "contains a single discussion when matching merge request and discussion are passed" do - service = described_class.new( + service = DummyService.new( project, user, discussion_to_resolve: discussion.id, @@ -61,7 +61,7 @@ describe DummyService, services: true do noteable: merge_request, project: merge_request.target_project, line_number: 15)]) - service = described_class.new( + service = DummyService.new( project, user, merge_request_to_resolve_discussions_of: merge_request.iid @@ -77,9 +77,9 @@ describe DummyService, services: true do _second_discussion = Discussion.new([create(:diff_note_on_merge_request, :resolved, noteable: merge_request, project: merge_request.target_project, - line_number: 15, + line_number: 15 )]) - service = described_class.new( + service = DummyService.new( project, user, merge_request_to_resolve_discussions_of: merge_request.iid @@ -92,7 +92,7 @@ describe DummyService, services: true do end it "is empty when a discussion and another merge request are passed" do - service = described_class.new( + service = DummyService.new( project, user, discussion_to_resolve: discussion.id, diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 5b324f3c706..5184c1d5f19 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -14,7 +14,7 @@ describe Issues::UpdateService, services: true do let(:issue) do create(:issue, title: 'Old title', description: "for #{user2.to_reference}", - assignee_id: user3.id, + assignee_ids: [user3.id], project: project) end @@ -40,7 +40,7 @@ describe Issues::UpdateService, services: true do { title: 'New title', description: 'Also please fix', - assignee_id: user2.id, + assignee_ids: [user2.id], state_event: 'close', label_ids: [label.id], due_date: Date.tomorrow @@ -53,15 +53,22 @@ describe Issues::UpdateService, services: true do expect(issue).to be_valid expect(issue.title).to eq 'New title' expect(issue.description).to eq 'Also please fix' - expect(issue.assignee).to eq user2 + expect(issue.assignees).to match_array([user2]) expect(issue).to be_closed expect(issue.labels).to match_array [label] expect(issue.due_date).to eq Date.tomorrow end + it 'updates open issue counter for assignees when issue is reassigned' do + update_issue(assignee_ids: [user2.id]) + + expect(user3.assigned_open_issues_count).to eq 0 + expect(user2.assigned_open_issues_count).to eq 1 + end + it 'sorts issues as specified by parameters' do - issue1 = create(:issue, project: project, assignee_id: user3.id) - issue2 = create(:issue, project: project, assignee_id: user3.id) + issue1 = create(:issue, project: project, assignees: [user3]) + issue2 = create(:issue, project: project, assignees: [user3]) [issue, issue1, issue2].each do |issue| issue.move_to_end @@ -87,7 +94,7 @@ describe Issues::UpdateService, services: true do expect(issue).to be_valid expect(issue.title).to eq 'New title' expect(issue.description).to eq 'Also please fix' - expect(issue.assignee).to eq user3 + expect(issue.assignees).to match_array [user3] expect(issue.labels).to be_empty expect(issue.milestone).to be_nil expect(issue.due_date).to be_nil @@ -132,12 +139,23 @@ describe Issues::UpdateService, services: true do end end + context 'when description changed' do + it 'creates system note about description change' do + update_issue(description: 'Changed description') + + note = find_note('changed the description') + + expect(note).not_to be_nil + expect(note.note).to eq('changed the description') + end + end + context 'when issue turns confidential' do let(:opts) do { title: 'New title', description: 'Also please fix', - assignee_id: user2.id, + assignee_ids: [user2], state_event: 'close', label_ids: [label.id], confidential: true @@ -163,12 +181,12 @@ describe Issues::UpdateService, services: true do it 'does not update assignee_id with unauthorized users' do project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC) update_issue(confidential: true) - non_member = create(:user) - original_assignee = issue.assignee + non_member = create(:user) + original_assignees = issue.assignees - update_issue(assignee_id: non_member.id) + update_issue(assignee_ids: [non_member.id]) - expect(issue.reload.assignee_id).to eq(original_assignee.id) + expect(issue.reload.assignees).to eq(original_assignees) end end @@ -205,7 +223,7 @@ describe Issues::UpdateService, services: true do context 'when is reassigned' do before do - update_issue(assignee: user2) + update_issue(assignees: [user2]) end it 'marks previous assignee todos as done' do @@ -408,6 +426,41 @@ describe Issues::UpdateService, services: true do end end + context 'updating asssignee_id' do + it 'does not update assignee when assignee_id is invalid' do + update_issue(assignee_ids: [-1]) + + expect(issue.reload.assignees).to eq([user3]) + end + + it 'unassigns assignee when user id is 0' do + update_issue(assignee_ids: [0]) + + expect(issue.reload.assignees).to be_empty + end + + it 'does not update assignee_id when user cannot read issue' do + update_issue(assignee_ids: [create(:user).id]) + + expect(issue.reload.assignees).to eq([user3]) + end + + context "when issuable feature is private" do + levels = [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC] + + levels.each do |level| + it "does not update with unauthorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do + assignee = create(:user) + project.update(visibility_level: level) + feature_visibility_attr = :"#{issue.model_name.plural}_access_level" + project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE) + + expect{ update_issue(assignee_ids: [assignee.id]) }.not_to change{ issue.assignees } + end + end + end + end + context 'updating mentions' do let(:mentionable) { issue } include_examples 'updating mentions', Issues::UpdateService diff --git a/spec/services/members/authorized_destroy_service_spec.rb b/spec/services/members/authorized_destroy_service_spec.rb new file mode 100644 index 00000000000..8a6732faa19 --- /dev/null +++ b/spec/services/members/authorized_destroy_service_spec.rb @@ -0,0 +1,66 @@ +require 'spec_helper' + +describe Members::AuthorizedDestroyService, services: true do + let(:member_user) { create(:user) } + let(:project) { create(:empty_project, :public) } + let(:group) { create(:group, :public) } + let(:group_project) { create(:empty_project, :public, group: group) } + + def number_of_assigned_issuables(user) + Issue.assigned_to(user).count + MergeRequest.assigned_to(user).count + end + + context 'Invited users' do + # Regression spec for issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/32504 + it 'destroys invited project member' do + project.team << [member_user, :developer] + + member = create :project_member, :invited, project: project + + expect { described_class.new(member, member_user).execute } + .to change { Member.count }.from(2).to(1) + end + + it 'destroys invited group member' do + group.add_developer(member_user) + + member = create :group_member, :invited, group: group + + expect { described_class.new(member, member_user).execute } + .to change { Member.count }.from(2).to(1) + end + end + + context 'Group member' do + it "unassigns issues and merge requests" do + group.add_developer(member_user) + + issue = create :issue, project: group_project, assignees: [member_user] + create :issue, assignees: [member_user] + merge_request = create :merge_request, target_project: group_project, source_project: group_project, assignee: member_user + create :merge_request, target_project: project, source_project: project, assignee: member_user + + member = group.members.find_by(user_id: member_user.id) + + expect { described_class.new(member, member_user).execute } + .to change { number_of_assigned_issuables(member_user) }.from(4).to(2) + + expect(issue.reload.assignee_id).to be_nil + expect(merge_request.reload.assignee_id).to be_nil + end + end + + context 'Project member' do + it "unassigns issues and merge requests" do + project.team << [member_user, :developer] + + create :issue, project: project, assignees: [member_user] + create :merge_request, target_project: project, source_project: project, assignee: member_user + + member = project.members.find_by(user_id: member_user.id) + + expect { described_class.new(member, member_user).execute } + .to change { number_of_assigned_issuables(member_user) }.from(2).to(0) + end + end +end diff --git a/spec/services/merge_requests/assign_issues_service_spec.rb b/spec/services/merge_requests/assign_issues_service_spec.rb index fe75757dd29..d3556020d4d 100644 --- a/spec/services/merge_requests/assign_issues_service_spec.rb +++ b/spec/services/merge_requests/assign_issues_service_spec.rb @@ -15,14 +15,14 @@ describe MergeRequests::AssignIssuesService, services: true do expect(service.assignable_issues.map(&:id)).to include(issue.id) end - it 'ignores issues already assigned to any user' do - issue.update!(assignee: create(:user)) + it 'ignores issues the user cannot update assignee on' do + project.team.truncate expect(service.assignable_issues).to be_empty end - it 'ignores issues the user cannot update assignee on' do - project.team.truncate + it 'ignores issues already assigned to any user' do + issue.assignees = [create(:user)] expect(service.assignable_issues).to be_empty end @@ -44,7 +44,7 @@ describe MergeRequests::AssignIssuesService, services: true do end it 'assigns these to the merge request owner' do - expect { service.execute }.to change { issue.reload.assignee }.to(user) + expect { service.execute }.to change { issue.assignees.first }.to(user) end it 'ignores external issues' do diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index c8bd4d4601a..6f9d1208b1d 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -4,6 +4,8 @@ describe MergeRequests::BuildService, services: true do include RepoHelpers let(:project) { create(:project, :repository) } + let(:source_project) { nil } + let(:target_project) { nil } let(:user) { create(:user) } let(:issue_confidential) { false } let(:issue) { create(:issue, project: project, title: 'A bug', confidential: issue_confidential) } @@ -20,7 +22,9 @@ describe MergeRequests::BuildService, services: true do MergeRequests::BuildService.new(project, user, description: description, source_branch: source_branch, - target_branch: target_branch) + target_branch: target_branch, + source_project: source_project, + target_project: target_project) end before do @@ -256,5 +260,51 @@ describe MergeRequests::BuildService, services: true do ) end end + + context 'upstream project has disabled merge requests' do + let(:upstream_project) { create(:empty_project, :merge_requests_disabled) } + let(:project) { create(:empty_project, forked_from_project: upstream_project) } + let(:commits) { Commit.decorate([commit_1], project) } + + it 'sets target project correctly' do + expect(merge_request.target_project).to eq(project) + end + end + + context 'target_project is set and accessible by current_user' do + let(:target_project) { create(:project, :public, :repository)} + let(:commits) { Commit.decorate([commit_1], project) } + + it 'sets target project correctly' do + expect(merge_request.target_project).to eq(target_project) + end + end + + context 'target_project is set but not accessible by current_user' do + let(:target_project) { create(:project, :private, :repository)} + let(:commits) { Commit.decorate([commit_1], project) } + + it 'sets target project correctly' do + expect(merge_request.target_project).to eq(project) + end + end + + context 'source_project is set and accessible by current_user' do + let(:source_project) { create(:project, :public, :repository)} + let(:commits) { Commit.decorate([commit_1], project) } + + it 'sets target project correctly' do + expect(merge_request.source_project).to eq(source_project) + end + end + + context 'source_project is set but not accessible by current_user' do + let(:source_project) { create(:project, :private, :repository)} + let(:commits) { Commit.decorate([commit_1], project) } + + it 'sets target project correctly' do + expect(merge_request.source_project).to eq(project) + end + end end end diff --git a/spec/services/merge_requests/conflicts/list_service_spec.rb b/spec/services/merge_requests/conflicts/list_service_spec.rb new file mode 100644 index 00000000000..23982b9e6e1 --- /dev/null +++ b/spec/services/merge_requests/conflicts/list_service_spec.rb @@ -0,0 +1,80 @@ +require 'spec_helper' + +describe MergeRequests::Conflicts::ListService do + describe '#can_be_resolved_in_ui?' do + def create_merge_request(source_branch) + create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start') do |mr| + mr.mark_as_unmergeable + end + end + + def conflicts_service(merge_request) + described_class.new(merge_request) + end + + it 'returns a falsey value when the MR can be merged without conflicts' do + merge_request = create_merge_request('master') + merge_request.mark_as_mergeable + + expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey + end + + it 'returns a falsey value when the MR is marked as having conflicts, but has none' do + merge_request = create_merge_request('master') + + expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey + end + + it 'returns a falsey value when one of the MR branches is missing' do + merge_request = create_merge_request('conflict-resolvable') + merge_request.project.repository.rm_branch(merge_request.author, 'conflict-resolvable') + + expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey + end + + it 'returns a falsey value when the MR has a missing ref after a force push' do + merge_request = create_merge_request('conflict-resolvable') + service = conflicts_service(merge_request) + allow(service.conflicts).to receive(:merge_index).and_raise(Rugged::OdbError) + + expect(service.can_be_resolved_in_ui?).to be_falsey + end + + it 'returns a falsey value when the MR does not support new diff notes' do + merge_request = create_merge_request('conflict-resolvable') + merge_request.merge_request_diff.update_attributes(start_commit_sha: nil) + + expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey + end + + it 'returns a falsey value when the conflicts contain a large file' do + merge_request = create_merge_request('conflict-too-large') + + expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey + end + + it 'returns a falsey value when the conflicts contain a binary file' do + merge_request = create_merge_request('conflict-binary-file') + + expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey + end + + it 'returns a falsey value when the conflicts contain a file edited in one branch and deleted in another' do + merge_request = create_merge_request('conflict-missing-side') + + expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey + end + + it 'returns a truthy value when the conflicts are resolvable in the UI' do + merge_request = create_merge_request('conflict-resolvable') + + expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_truthy + end + + it 'returns a truthy value when the conflicts have to be resolved in an editor' do + merge_request = create_merge_request('conflict-contains-conflict-markers') + + expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_truthy + end + end +end diff --git a/spec/services/merge_requests/resolve_service_spec.rb b/spec/services/merge_requests/conflicts/resolve_service_spec.rb index eaf7785e549..19e8d5cc5f1 100644 --- a/spec/services/merge_requests/resolve_service_spec.rb +++ b/spec/services/merge_requests/conflicts/resolve_service_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe MergeRequests::ResolveService do +describe MergeRequests::Conflicts::ResolveService do let(:user) { create(:user) } let(:project) { create(:project, :repository) } @@ -24,6 +24,8 @@ describe MergeRequests::ResolveService do end describe '#execute' do + let(:service) { described_class.new(merge_request) } + context 'with section params' do let(:params) do { @@ -50,7 +52,7 @@ describe MergeRequests::ResolveService do context 'when the source and target project are the same' do before do - MergeRequests::ResolveService.new(project, user, params).execute(merge_request) + service.execute(user, params) end it 'creates a commit with the message' do @@ -74,15 +76,26 @@ describe MergeRequests::ResolveService do branch_name: 'conflict-start') end - before do - MergeRequests::ResolveService.new(fork_project, user, params).execute(merge_request_from_fork) + def resolve_conflicts + described_class.new(merge_request_from_fork).execute(user, params) + end + + it 'gets conflicts from the source project' do + expect(fork_project.repository.rugged).to receive(:merge_commits).and_call_original + expect(project.repository.rugged).not_to receive(:merge_commits) + + resolve_conflicts end it 'creates a commit with the message' do + resolve_conflicts + expect(merge_request_from_fork.source_branch_head.message).to eq(params[:commit_message]) end it 'creates a commit with the correct parents' do + resolve_conflicts + expect(merge_request_from_fork.source_branch_head.parents.map(&:id)). to eq(['404fa3fc7c2c9b5dacff102f353bdf55b1be2813', target_head]) @@ -115,7 +128,7 @@ describe MergeRequests::ResolveService do end before do - MergeRequests::ResolveService.new(project, user, params).execute(merge_request) + service.execute(user, params) end it 'creates a commit with the message' do @@ -154,15 +167,15 @@ describe MergeRequests::ResolveService do } end - let(:service) { MergeRequests::ResolveService.new(project, user, invalid_params) } - it 'raises a MissingResolution error' do - expect { service.execute(merge_request) }. + expect { service.execute(user, invalid_params) }. to raise_error(Gitlab::Conflict::File::MissingResolution) end end context 'when the content of a file is unchanged' do + let(:list_service) { MergeRequests::Conflicts::ListService.new(merge_request) } + let(:invalid_params) do { files: [ @@ -173,17 +186,15 @@ describe MergeRequests::ResolveService do }, { old_path: 'files/ruby/regex.rb', new_path: 'files/ruby/regex.rb', - content: merge_request.conflicts.file_for_path('files/ruby/regex.rb', 'files/ruby/regex.rb').content + content: list_service.conflicts.file_for_path('files/ruby/regex.rb', 'files/ruby/regex.rb').content } ], commit_message: 'This is a commit message!' } end - let(:service) { MergeRequests::ResolveService.new(project, user, invalid_params) } - it 'raises a MissingResolution error' do - expect { service.execute(merge_request) }. + expect { service.execute(user, invalid_params) }. to raise_error(Gitlab::Conflict::File::MissingResolution) end end @@ -202,11 +213,9 @@ describe MergeRequests::ResolveService do } end - let(:service) { MergeRequests::ResolveService.new(project, user, invalid_params) } - it 'raises a MissingFiles error' do - expect { service.execute(merge_request) }. - to raise_error(MergeRequests::ResolveService::MissingFiles) + expect { service.execute(user, invalid_params) }. + to raise_error(described_class::MissingFiles) end end end diff --git a/spec/services/merge_requests/create_from_issue_service_spec.rb b/spec/services/merge_requests/create_from_issue_service_spec.rb new file mode 100644 index 00000000000..1588d30c394 --- /dev/null +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -0,0 +1,74 @@ +require 'spec_helper' + +describe MergeRequests::CreateFromIssueService, services: true do + let(:project) { create(:project, :repository) } + let(:user) { create(:user) } + let(:issue) { create(:issue, project: project) } + + subject(:service) { described_class.new(project, user, issue_iid: issue.iid) } + + before do + project.add_developer(user) + end + + describe '#execute' do + it 'returns an error with invalid issue iid' do + result = described_class.new(project, user, issue_iid: -1).execute + + expect(result[:status]).to eq :error + expect(result[:message]).to eq 'Invalid issue iid' + end + + it 'delegates issue search to IssuesFinder' do + expect_any_instance_of(IssuesFinder).to receive(:execute).once.and_call_original + + described_class.new(project, user, issue_iid: -1).execute + end + + it 'delegates the branch creation to CreateBranchService' do + expect_any_instance_of(CreateBranchService).to receive(:execute).once.and_call_original + + service.execute + end + + it 'creates a branch based on issue title' do + service.execute + + expect(project.repository.branch_exists?(issue.to_branch_name)).to be_truthy + end + + it 'creates a system note' do + expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, issue.to_branch_name) + + service.execute + end + + it 'creates a merge request' do + expect { service.execute }.to change(project.merge_requests, :count).by(1) + end + + it 'sets the merge request title to: "WIP: Resolves "$issue-title"' do + result = service.execute + + expect(result[:merge_request].title).to eq("WIP: Resolve \"#{issue.title}\"") + end + + it 'sets the merge request author to current user' do + result = service.execute + + expect(result[:merge_request].author).to eq user + end + + it 'sets the merge request source branch to the new issue branch' do + result = service.execute + + expect(result[:merge_request].source_branch).to eq issue.to_branch_name + end + + it 'sets the merge request target branch to the project default branch' do + result = service.execute + + expect(result[:merge_request].target_branch).to eq project.default_branch + end + end +end diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 0e16c7cc94b..b70e9d534a4 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -27,10 +27,12 @@ describe MergeRequests::CreateService, services: true do @merge_request = service.execute end - it { expect(@merge_request).to be_valid } - it { expect(@merge_request.title).to eq('Awesome merge_request') } - it { expect(@merge_request.assignee).to be_nil } - it { expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1') } + it 'creates an MR' do + expect(@merge_request).to be_valid + expect(@merge_request.title).to eq('Awesome merge_request') + expect(@merge_request.assignee).to be_nil + expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1') + end it 'executes hooks with default action' do expect(service).to have_received(:execute_hooks).with(@merge_request) @@ -84,7 +86,107 @@ describe MergeRequests::CreateService, services: true do end end - it_behaves_like 'issuable create service' + context 'Slash commands' do + context 'with assignee and milestone in params and command' do + let(:merge_request) { described_class.new(project, user, opts).execute } + let(:milestone) { create(:milestone, project: project) } + + let(:opts) do + { + assignee_id: create(:user).id, + milestone_id: 1, + title: 'Title', + description: %(/assign @#{assignee.username}\n/milestone %"#{milestone.name}"), + source_branch: 'feature', + target_branch: 'master' + } + end + + before do + project.team << [user, :master] + project.team << [assignee, :master] + end + + it 'assigns and sets milestone to issuable from command' do + expect(merge_request).to be_persisted + expect(merge_request.assignee).to eq(assignee) + expect(merge_request.milestone).to eq(milestone) + end + end + end + + context 'merge request create service' do + context 'asssignee_id' do + let(:assignee) { create(:user) } + + before { project.team << [user, :master] } + + it 'removes assignee_id when user id is invalid' do + opts = { title: 'Title', description: 'Description', assignee_id: -1 } + + merge_request = described_class.new(project, user, opts).execute + + expect(merge_request.assignee_id).to be_nil + end + + it 'removes assignee_id when user id is 0' do + opts = { title: 'Title', description: 'Description', assignee_id: 0 } + + merge_request = described_class.new(project, user, opts).execute + + expect(merge_request.assignee_id).to be_nil + end + + it 'saves assignee when user id is valid' do + project.team << [assignee, :master] + opts = { title: 'Title', description: 'Description', assignee_id: assignee.id } + + merge_request = described_class.new(project, user, opts).execute + + expect(merge_request.assignee).to eq(assignee) + end + + context 'when assignee is set' do + let(:opts) do + { + title: 'Title', + description: 'Description', + assignee_id: assignee.id, + source_branch: 'feature', + target_branch: 'master' + } + end + + it 'invalidates open merge request counter for assignees when merge request is assigned' do + project.team << [assignee, :master] + + described_class.new(project, user, opts).execute + + expect(assignee.assigned_open_merge_requests_count).to eq 1 + end + end + + context "when issuable feature is private" do + before do + project.project_feature.update(issues_access_level: ProjectFeature::PRIVATE, + merge_requests_access_level: ProjectFeature::PRIVATE) + end + + levels = [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC] + + levels.each do |level| + it "removes not authorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do + project.update(visibility_level: level) + opts = { title: 'Title', description: 'Description', assignee_id: assignee.id } + + merge_request = described_class.new(project, user, opts).execute + + expect(merge_request.assignee_id).to be_nil + end + end + end + end + end context 'while saving references to issues that the created merge request closes' do let(:first_issue) { create(:issue, project: project) } diff --git a/spec/services/merge_requests/get_urls_service_spec.rb b/spec/services/merge_requests/get_urls_service_spec.rb index 290e00ea1ba..4a7d8ab4c6c 100644 --- a/spec/services/merge_requests/get_urls_service_spec.rb +++ b/spec/services/merge_requests/get_urls_service_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" describe MergeRequests::GetUrlsService do let(:project) { create(:project, :public, :repository) } - let(:service) { MergeRequests::GetUrlsService.new(project) } + let(:service) { described_class.new(project) } let(:source_branch) { "my_branch" } let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{source_branch}" } let(:show_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/#{merge_request.iid}" } @@ -89,7 +89,7 @@ describe MergeRequests::GetUrlsService do let!(:merge_request) { create(:merge_request, source_project: forked_project, target_project: project, source_branch: source_branch) } let(:changes) { existing_branch_changes } # Source project is now the forked one - let(:service) { MergeRequests::GetUrlsService.new(forked_project) } + let(:service) { described_class.new(forked_project) } before do allow(forked_project).to receive(:empty_repo?).and_return(false) diff --git a/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb b/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb index 35804d41b46..935f4710851 100644 --- a/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb +++ b/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe MergeRequests::MergeRequestDiffCacheService do - let(:subject) { MergeRequests::MergeRequestDiffCacheService.new } + let(:subject) { described_class.new } describe '#execute' do it 'retrieves the diff files to cache the highlighted result' do diff --git a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb index 769b3193275..3ef5135e6a3 100644 --- a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb @@ -82,6 +82,10 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do sha: merge_request_head, status: 'success') end + before do + mr_merge_if_green_enabled.update(head_pipeline: triggering_pipeline) + end + it "merges all merge requests with merge when the pipeline succeeds enabled" do expect(MergeWorker).to receive(:perform_async) service.trigger(triggering_pipeline) @@ -124,6 +128,8 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do sha: mr_conflict.diff_head_sha, status: 'success') end + before { mr_conflict.update(head_pipeline: conflict_pipeline) } + it 'does not merge the merge request' do expect(MergeWorker).not_to receive(:perform_async) diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index c22d145ca5d..1f109eab268 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -49,6 +49,7 @@ describe MergeRequests::RefreshService, services: true do context 'push to origin repo source branch' do let(:refresh_service) { service.new(@project, @user) } + before do allow(refresh_service).to receive(:execute_hooks) refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') @@ -70,6 +71,32 @@ describe MergeRequests::RefreshService, services: true do end end + context 'push to origin repo source branch when an MR was reopened' do + let(:refresh_service) { service.new(@project, @user) } + + before do + @merge_request.update(state: :reopened) + + allow(refresh_service).to receive(:execute_hooks) + refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') + reload_mrs + end + + it 'executes hooks with update action' do + expect(refresh_service).to have_received(:execute_hooks). + with(@merge_request, 'update', @oldrev) + + expect(@merge_request.notes).not_to be_empty + expect(@merge_request).to be_open + expect(@merge_request.merge_when_pipeline_succeeds).to be_falsey + expect(@merge_request.diff_head_sha).to eq(@newrev) + expect(@fork_merge_request).to be_open + expect(@fork_merge_request.notes).to be_empty + expect(@build_failed_todo).to be_done + expect(@fork_build_failed_todo).to be_done + end + end + context 'push to origin repo target branch' do before do service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') @@ -321,7 +348,7 @@ describe MergeRequests::RefreshService, services: true do title: 'fixup! Fix issue', work_in_progress?: true, to_reference: 'ccccccc' - ), + ) ]) refresh_service.execute(@oldrev, @newrev, 'refs/heads/wip') reload_mrs diff --git a/spec/services/merge_requests/resolved_discussion_notification_service.rb b/spec/services/merge_requests/resolved_discussion_notification_service_spec.rb index 7ddd812e513..7ddd812e513 100644 --- a/spec/services/merge_requests/resolved_discussion_notification_service.rb +++ b/spec/services/merge_requests/resolved_discussion_notification_service_spec.rb diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index f2ca1e6fcbd..860a7798857 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -59,14 +59,16 @@ describe MergeRequests::UpdateService, services: true do end end - it { expect(@merge_request).to be_valid } - it { expect(@merge_request.title).to eq('New title') } - it { expect(@merge_request.assignee).to eq(user2) } - it { expect(@merge_request).to be_closed } - it { expect(@merge_request.labels.count).to eq(1) } - it { expect(@merge_request.labels.first.title).to eq(label.name) } - it { expect(@merge_request.target_branch).to eq('target') } - it { expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1') } + it 'mathces base expectations' do + expect(@merge_request).to be_valid + expect(@merge_request.title).to eq('New title') + expect(@merge_request.assignee).to eq(user2) + expect(@merge_request).to be_closed + expect(@merge_request.labels.count).to eq(1) + expect(@merge_request.labels.first.title).to eq(label.name) + expect(@merge_request.target_branch).to eq('target') + expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1') + end it 'executes hooks with update action' do expect(service).to have_received(:execute_hooks). @@ -102,6 +104,13 @@ describe MergeRequests::UpdateService, services: true do expect(note.note).to eq 'changed title from **{-Old-} title** to **{+New+} title**' end + it 'creates system note about description change' do + note = find_note('changed the description') + + expect(note).not_to be_nil + expect(note.note).to eq('changed the description') + end + it 'creates system note about branch change' do note = find_note('changed target') @@ -141,9 +150,11 @@ describe MergeRequests::UpdateService, services: true do end end - it { expect(@merge_request).to be_valid } - it { expect(@merge_request.state).to eq('merged') } - it { expect(@merge_request.merge_error).to be_nil } + it 'merges the MR' do + expect(@merge_request).to be_valid + expect(@merge_request.state).to eq('merged') + expect(@merge_request.merge_error).to be_nil + end end context 'with finished pipeline' do @@ -160,18 +171,22 @@ describe MergeRequests::UpdateService, services: true do end end - it { expect(@merge_request).to be_valid } - it { expect(@merge_request.state).to eq('merged') } + it 'merges the MR' do + expect(@merge_request).to be_valid + expect(@merge_request.state).to eq('merged') + end end context 'with active pipeline' do before do service_mock = double - create(:ci_pipeline_with_one_job, + pipeline = create(:ci_pipeline_with_one_job, project: project, ref: merge_request.source_branch, sha: merge_request.diff_head_sha) + merge_request.update(head_pipeline: pipeline) + expect(MergeRequests::MergeWhenPipelineSucceedsService).to receive(:new).with(project, user). and_return(service_mock) expect(service_mock).to receive(:execute).with(merge_request) @@ -193,8 +208,10 @@ describe MergeRequests::UpdateService, services: true do end end - it { expect(@merge_request.state).to eq('opened') } - it { expect(@merge_request.merge_error).not_to be_nil } + it 'does not merge the MR' do + expect(@merge_request.state).to eq('opened') + expect(@merge_request.merge_error).not_to be_nil + end end context 'MR can not be merged when note sha != MR sha' do @@ -290,6 +307,15 @@ describe MergeRequests::UpdateService, services: true do end end + context 'when the assignee changes' do + it 'updates open merge request counter for assignees when merge request is reassigned' do + update_merge_request(assignee_id: user2.id) + + expect(user3.assigned_open_merge_requests_count).to eq 0 + expect(user2.assigned_open_merge_requests_count).to eq 1 + end + end + context 'when the target branch change' do before do update_merge_request({ target_branch: 'target' }) @@ -423,6 +449,54 @@ describe MergeRequests::UpdateService, services: true do end end + context 'updating asssignee_id' do + it 'does not update assignee when assignee_id is invalid' do + merge_request.update(assignee_id: user.id) + + update_merge_request(assignee_id: -1) + + expect(merge_request.reload.assignee).to eq(user) + end + + it 'unassigns assignee when user id is 0' do + merge_request.update(assignee_id: user.id) + + update_merge_request(assignee_id: 0) + + expect(merge_request.assignee_id).to be_nil + end + + it 'saves assignee when user id is valid' do + update_merge_request(assignee_id: user.id) + + expect(merge_request.assignee_id).to eq(user.id) + end + + it 'does not update assignee_id when user cannot read issue' do + non_member = create(:user) + original_assignee = merge_request.assignee + + update_merge_request(assignee_id: non_member.id) + + expect(merge_request.assignee_id).to eq(original_assignee.id) + end + + context "when issuable feature is private" do + levels = [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC] + + levels.each do |level| + it "does not update with unauthorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do + assignee = create(:user) + project.update(visibility_level: level) + feature_visibility_attr = :"#{merge_request.model_name.plural}_access_level" + project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE) + + expect{ update_merge_request(assignee_id: assignee) }.not_to change{ merge_request.assignee } + end + end + end + end + include_examples 'issuable update service' do let(:open_issuable) { merge_request } let(:closed_issuable) { create(:closed_merge_request, source_project: project) } diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb new file mode 100644 index 00000000000..133175769ca --- /dev/null +++ b/spec/services/notes/build_service_spec.rb @@ -0,0 +1,112 @@ +require 'spec_helper' + +describe Notes::BuildService, services: true do + let(:note) { create(:discussion_note_on_issue) } + let(:project) { note.project } + let(:author) { note.author } + + describe '#execute' do + context 'when in_reply_to_discussion_id is specified' do + context 'when a note with that original discussion ID exists' do + it 'sets the note up to be in reply to that note' do + new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute + expect(new_note).to be_valid + expect(new_note.in_reply_to?(note)).to be_truthy + end + end + + context 'when a note with that discussion ID exists' do + it 'sets the note up to be in reply to that note' do + new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute + expect(new_note).to be_valid + expect(new_note.in_reply_to?(note)).to be_truthy + end + end + + context 'when no note with that discussion ID exists' do + it 'sets an error' do + new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: 'foo').execute + expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found') + end + end + + context 'personal snippet note' do + def reply(note, user = nil) + user ||= create(:user) + + described_class.new(nil, + user, + note: 'Test', + in_reply_to_discussion_id: note.discussion_id).execute + end + + let(:snippet_author) { create(:user) } + + context 'when a snippet is public' do + it 'creates a reply note' do + snippet = create(:personal_snippet, :public) + note = create(:discussion_note_on_personal_snippet, noteable: snippet) + + new_note = reply(note) + + expect(new_note).to be_valid + expect(new_note.in_reply_to?(note)).to be_truthy + end + end + + context 'when a snippet is private' do + let(:snippet) { create(:personal_snippet, :private, author: snippet_author) } + let(:note) { create(:discussion_note_on_personal_snippet, noteable: snippet) } + + it 'creates a reply note when the author replies' do + new_note = reply(note, snippet_author) + + expect(new_note).to be_valid + expect(new_note.in_reply_to?(note)).to be_truthy + end + + it 'sets an error when another user replies' do + new_note = reply(note) + + expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found') + end + end + + context 'when a snippet is internal' do + let(:snippet) { create(:personal_snippet, :internal, author: snippet_author) } + let(:note) { create(:discussion_note_on_personal_snippet, noteable: snippet) } + + it 'creates a reply note when the author replies' do + new_note = reply(note, snippet_author) + + expect(new_note).to be_valid + expect(new_note.in_reply_to?(note)).to be_truthy + end + + it 'creates a reply note when a regular user replies' do + new_note = reply(note) + + expect(new_note).to be_valid + expect(new_note.in_reply_to?(note)).to be_truthy + end + + it 'sets an error when an external user replies' do + new_note = reply(note, create(:user, :external)) + + expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found') + end + end + end + end + + it 'builds a note without saving it' do + new_note = described_class.new(project, + author, + noteable_type: note.noteable_type, + noteable_id: note.noteable_id, + note: 'Test').execute + expect(new_note).to be_valid + expect(new_note).not_to be_persisted + end + end +end diff --git a/spec/services/notes/slash_commands_service_spec.rb b/spec/services/notes/slash_commands_service_spec.rb index 1a64c8bbf00..c9954dc3603 100644 --- a/spec/services/notes/slash_commands_service_spec.rb +++ b/spec/services/notes/slash_commands_service_spec.rb @@ -66,7 +66,7 @@ describe Notes::SlashCommandsService, services: true do expect(content).to eq '' expect(note.noteable).to be_closed expect(note.noteable.labels).to match_array(labels) - expect(note.noteable.assignee).to eq(assignee) + expect(note.noteable.assignees).to eq([assignee]) expect(note.noteable.milestone).to eq(milestone) end end @@ -113,7 +113,7 @@ describe Notes::SlashCommandsService, services: true do expect(content).to eq "HELLO\nWORLD" expect(note.noteable).to be_closed expect(note.noteable.labels).to match_array(labels) - expect(note.noteable.assignee).to eq(assignee) + expect(note.noteable.assignees).to eq([assignee]) expect(note.noteable.milestone).to eq(milestone) end end @@ -220,4 +220,31 @@ describe Notes::SlashCommandsService, services: true do let(:note) { build(:note_on_commit, project: project) } end end + + context 'CE restriction for issue assignees' do + describe '/assign' do + let(:project) { create(:empty_project) } + let(:master) { create(:user).tap { |u| project.team << [u, :master] } } + let(:assignee) { create(:user) } + let(:master) { create(:user) } + let(:service) { described_class.new(project, master) } + let(:note) { create(:note_on_issue, note: note_text, project: project) } + + let(:note_text) do + %(/assign @#{assignee.username} @#{master.username}\n") + end + + before do + project.team << [master, :master] + project.team << [assignee, :master] + end + + it 'adds only one assignee from the list' do + _, command_params = service.extract_commands(note) + service.execute(command_params, note) + + expect(note.noteable.assignees.count).to eq(1) + end + end + end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 5c841843b40..de3bbc6b6a1 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -4,6 +4,7 @@ describe NotificationService, services: true do include EmailHelpers let(:notification) { NotificationService.new } + let(:assignee) { create(:user) } around(:each) do |example| perform_enqueued_jobs do @@ -52,7 +53,11 @@ describe NotificationService, services: true do shared_examples 'participating by assignee notification' do it 'emails the participant' do - issuable.update_attribute(:assignee, participant) + if issuable.is_a?(Issue) + issuable.assignees << participant + else + issuable.update_attribute(:assignee, participant) + end notification_trigger @@ -103,17 +108,17 @@ describe NotificationService, services: true do describe 'Notes' do context 'issue note' do let(:project) { create(:empty_project, :private) } - let(:issue) { create(:issue, project: project, assignee: create(:user)) } - let(:mentioned_issue) { create(:issue, assignee: issue.assignee) } + let(:issue) { create(:issue, project: project, assignees: [assignee]) } + let(:mentioned_issue) { create(:issue, assignees: issue.assignees) } let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@mention referenced, @outsider also') } before do build_team(note.project) project.add_master(issue.author) - project.add_master(issue.assignee) + project.add_master(assignee) project.add_master(note.author) create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@subscribed_participant cc this guy') - update_custom_notification(:new_note, @u_guest_custom, project) + update_custom_notification(:new_note, @u_guest_custom, resource: project) update_custom_notification(:new_note, @u_custom_global) end @@ -130,7 +135,7 @@ describe NotificationService, services: true do should_email(@u_watcher) should_email(note.noteable.author) - should_email(note.noteable.assignee) + should_email(note.noteable.assignees.first) should_email(@u_custom_global) should_email(@u_mentioned) should_email(@subscriber) @@ -196,7 +201,7 @@ describe NotificationService, services: true do notification.new_note(note) should_email(note.noteable.author) - should_email(note.noteable.assignee) + should_email(note.noteable.assignees.first) should_email(@u_mentioned) should_email(@u_custom_global) should_not_email(@u_guest_custom) @@ -218,7 +223,7 @@ describe NotificationService, services: true do let(:member) { create(:user) } let(:guest) { create(:user) } let(:admin) { create(:admin) } - let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) } + let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignees: [assignee]) } let(:note) { create(:note_on_issue, noteable: confidential_issue, project: project, note: "#{author.to_reference} #{assignee.to_reference} #{non_member.to_reference} #{member.to_reference} #{admin.to_reference}") } let(:guest_watcher) { create_user_with_notification(:watch, "guest-watcher-confidential") } @@ -244,8 +249,8 @@ describe NotificationService, services: true do context 'issue note mention' do let(:project) { create(:empty_project, :public) } - let(:issue) { create(:issue, project: project, assignee: create(:user)) } - let(:mentioned_issue) { create(:issue, assignee: issue.assignee) } + let(:issue) { create(:issue, project: project, assignees: [assignee]) } + let(:mentioned_issue) { create(:issue, assignees: issue.assignees) } let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@all mentioned') } before do @@ -269,7 +274,7 @@ describe NotificationService, services: true do should_email(@u_guest_watcher) should_email(note.noteable.author) - should_email(note.noteable.assignee) + should_email(note.noteable.assignees.first) should_not_email(note.author) should_email(@u_mentioned) should_not_email(@u_disabled) @@ -345,7 +350,7 @@ describe NotificationService, services: true do create(:note_on_personal_snippet, noteable: snippet, note: 'note', author: @u_participant), create(:note_on_personal_snippet, noteable: snippet, note: 'note', author: @u_mentioned), create(:note_on_personal_snippet, noteable: snippet, note: 'note', author: @u_disabled), - create(:note_on_personal_snippet, noteable: snippet, note: 'note', author: @u_note_author), + create(:note_on_personal_snippet, noteable: snippet, note: 'note', author: @u_note_author) ] end @@ -379,7 +384,7 @@ describe NotificationService, services: true do build_team(note.project) reset_delivered_emails! allow_any_instance_of(Commit).to receive(:author).and_return(@u_committer) - update_custom_notification(:new_note, @u_guest_custom, project) + update_custom_notification(:new_note, @u_guest_custom, resource: project) update_custom_notification(:new_note, @u_custom_global) end @@ -439,7 +444,7 @@ describe NotificationService, services: true do notification.new_note(note) - expect(SentNotification.last.position).to eq(note.position) + expect(SentNotification.last.in_reply_to_discussion_id).to eq(note.discussion_id) end end end @@ -449,7 +454,7 @@ describe NotificationService, services: true do let(:group) { create(:group) } let(:project) { create(:empty_project, :public, namespace: group) } let(:another_project) { create(:empty_project, :public, namespace: group) } - let(:issue) { create :issue, project: project, assignee: create(:user), description: 'cc @participant' } + let(:issue) { create :issue, project: project, assignees: [assignee], description: 'cc @participant' } before do build_team(issue.project) @@ -457,7 +462,7 @@ describe NotificationService, services: true do add_users_with_subscription(issue.project, issue) reset_delivered_emails! - update_custom_notification(:new_issue, @u_guest_custom, project) + update_custom_notification(:new_issue, @u_guest_custom, resource: project) update_custom_notification(:new_issue, @u_custom_global) end @@ -465,7 +470,7 @@ describe NotificationService, services: true do it do notification.new_issue(issue, @u_disabled) - should_email(issue.assignee) + should_email(assignee) should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) @@ -480,10 +485,10 @@ describe NotificationService, services: true do end it do - create_global_setting_for(issue.assignee, :mention) + create_global_setting_for(issue.assignees.first, :mention) notification.new_issue(issue, @u_disabled) - should_not_email(issue.assignee) + should_not_email(issue.assignees.first) end it "emails the author if they've opted into notifications about their activity" do @@ -528,7 +533,7 @@ describe NotificationService, services: true do let(:member) { create(:user) } let(:guest) { create(:user) } let(:admin) { create(:admin) } - let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignee: assignee) } + let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignees: [assignee]) } it "emails subscribers of the issue's labels that can read the issue" do project.add_developer(member) @@ -567,14 +572,14 @@ describe NotificationService, services: true do describe '#reassigned_issue' do before do - update_custom_notification(:reassign_issue, @u_guest_custom, project) + update_custom_notification(:reassign_issue, @u_guest_custom, resource: project) update_custom_notification(:reassign_issue, @u_custom_global) end it 'emails new assignee' do - notification.reassigned_issue(issue, @u_disabled) + notification.reassigned_issue(issue, @u_disabled, [assignee]) - should_email(issue.assignee) + should_email(issue.assignees.first) should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) @@ -588,9 +593,8 @@ describe NotificationService, services: true do end it 'emails previous assignee even if he has the "on mention" notif level' do - issue.update_attribute(:assignee, @u_mentioned) - issue.update_attributes(assignee: @u_watcher) - notification.reassigned_issue(issue, @u_disabled) + issue.assignees = [@u_mentioned] + notification.reassigned_issue(issue, @u_disabled, [@u_watcher]) should_email(@u_mentioned) should_email(@u_watcher) @@ -606,11 +610,11 @@ describe NotificationService, services: true do end it 'emails new assignee even if he has the "on mention" notif level' do - issue.update_attributes(assignee: @u_mentioned) - notification.reassigned_issue(issue, @u_disabled) + issue.assignees = [@u_mentioned] + notification.reassigned_issue(issue, @u_disabled, [@u_mentioned]) - expect(issue.assignee).to be @u_mentioned - should_email(issue.assignee) + expect(issue.assignees.first).to be @u_mentioned + should_email(issue.assignees.first) should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) @@ -624,11 +628,11 @@ describe NotificationService, services: true do end it 'emails new assignee' do - issue.update_attribute(:assignee, @u_mentioned) - notification.reassigned_issue(issue, @u_disabled) + issue.assignees = [@u_mentioned] + notification.reassigned_issue(issue, @u_disabled, [@u_mentioned]) - expect(issue.assignee).to be @u_mentioned - should_email(issue.assignee) + expect(issue.assignees.first).to be @u_mentioned + should_email(issue.assignees.first) should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) @@ -642,17 +646,17 @@ describe NotificationService, services: true do end it 'does not email new assignee if they are the current user' do - issue.update_attribute(:assignee, @u_mentioned) - notification.reassigned_issue(issue, @u_mentioned) + issue.assignees = [@u_mentioned] + notification.reassigned_issue(issue, @u_mentioned, [@u_mentioned]) - expect(issue.assignee).to be @u_mentioned + expect(issue.assignees.first).to be @u_mentioned should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) should_email(@u_custom_global) - should_not_email(issue.assignee) + should_not_email(issue.assignees.first) should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_disabled) @@ -662,7 +666,7 @@ describe NotificationService, services: true do it_behaves_like 'participating notifications' do let(:participant) { create(:user, username: 'user-participant') } let(:issuable) { issue } - let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled) } + let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) } end end @@ -705,7 +709,7 @@ describe NotificationService, services: true do it "doesn't send email to anyone but subscribers of the given labels" do notification.relabeled_issue(issue, [group_label_2, label_2], @u_disabled) - should_not_email(issue.assignee) + should_not_email(issue.assignees.first) should_not_email(issue.author) should_not_email(@u_watcher) should_not_email(@u_guest_watcher) @@ -729,7 +733,7 @@ describe NotificationService, services: true do let(:member) { create(:user) } let(:guest) { create(:user) } let(:admin) { create(:admin) } - let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignee: assignee) } + let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignees: [assignee]) } let!(:label_1) { create(:label, project: project, issues: [confidential_issue]) } let!(:label_2) { create(:label, project: project) } @@ -760,14 +764,14 @@ describe NotificationService, services: true do describe '#close_issue' do before do - update_custom_notification(:close_issue, @u_guest_custom, project) + update_custom_notification(:close_issue, @u_guest_custom, resource: project) update_custom_notification(:close_issue, @u_custom_global) end it 'sends email to issue assignee and issue author' do notification.close_issue(issue, @u_disabled) - should_email(issue.assignee) + should_email(issue.assignees.first) should_email(issue.author) should_email(@u_watcher) should_email(@u_guest_watcher) @@ -791,14 +795,14 @@ describe NotificationService, services: true do describe '#reopen_issue' do before do - update_custom_notification(:reopen_issue, @u_guest_custom, project) + update_custom_notification(:reopen_issue, @u_guest_custom, resource: project) update_custom_notification(:reopen_issue, @u_custom_global) end it 'sends email to issue notification recipients' do notification.reopen_issue(issue, @u_disabled) - should_email(issue.assignee) + should_email(issue.assignees.first) should_email(issue.author) should_email(@u_watcher) should_email(@u_guest_watcher) @@ -826,7 +830,7 @@ describe NotificationService, services: true do it 'sends email to issue notification recipients' do notification.issue_moved(issue, new_issue, @u_disabled) - should_email(issue.assignee) + should_email(issue.assignees.first) should_email(issue.author) should_email(@u_watcher) should_email(@u_guest_watcher) @@ -856,14 +860,14 @@ describe NotificationService, services: true do before do build_team(merge_request.target_project) add_users_with_subscription(merge_request.target_project, merge_request) - update_custom_notification(:new_merge_request, @u_guest_custom, project) + update_custom_notification(:new_merge_request, @u_guest_custom, resource: project) update_custom_notification(:new_merge_request, @u_custom_global) reset_delivered_emails! end describe '#new_merge_request' do before do - update_custom_notification(:new_merge_request, @u_guest_custom, project) + update_custom_notification(:new_merge_request, @u_guest_custom, resource: project) update_custom_notification(:new_merge_request, @u_custom_global) end @@ -952,7 +956,7 @@ describe NotificationService, services: true do describe '#reassigned_merge_request' do before do - update_custom_notification(:reassign_merge_request, @u_guest_custom, project) + update_custom_notification(:reassign_merge_request, @u_guest_custom, resource: project) update_custom_notification(:reassign_merge_request, @u_custom_global) end @@ -1026,7 +1030,7 @@ describe NotificationService, services: true do describe '#closed_merge_request' do before do - update_custom_notification(:close_merge_request, @u_guest_custom, project) + update_custom_notification(:close_merge_request, @u_guest_custom, resource: project) update_custom_notification(:close_merge_request, @u_custom_global) end @@ -1056,7 +1060,7 @@ describe NotificationService, services: true do describe '#merged_merge_request' do before do - update_custom_notification(:merge_merge_request, @u_guest_custom, project) + update_custom_notification(:merge_merge_request, @u_guest_custom, resource: project) update_custom_notification(:merge_merge_request, @u_custom_global) end @@ -1108,7 +1112,7 @@ describe NotificationService, services: true do describe '#reopen_merge_request' do before do - update_custom_notification(:reopen_merge_request, @u_guest_custom, project) + update_custom_notification(:reopen_merge_request, @u_guest_custom, resource: project) update_custom_notification(:reopen_merge_request, @u_custom_global) end @@ -1181,6 +1185,22 @@ describe NotificationService, services: true do should_not_email(@u_disabled) end end + + describe '#project_exported' do + it do + notification.project_exported(project, @u_disabled) + + should_only_email(@u_disabled) + end + end + + describe '#project_not_exported' do + it do + notification.project_not_exported(project, @u_disabled, ['error']) + + should_only_email(@u_disabled) + end + end end describe 'GroupMember' do @@ -1281,40 +1301,172 @@ describe NotificationService, services: true do describe 'Pipelines' do describe '#pipeline_finished' do let(:project) { create(:project, :public, :repository) } - let(:current_user) { create(:user) } let(:u_member) { create(:user) } - let(:u_other) { create(:user) } + let(:u_watcher) { create_user_with_notification(:watch, 'watcher') } + + let(:u_custom_notification_unset) do + create_user_with_notification(:custom, 'custom_unset') + end + + let(:u_custom_notification_enabled) do + user = create_user_with_notification(:custom, 'custom_enabled') + update_custom_notification(:success_pipeline, user, resource: project) + update_custom_notification(:failed_pipeline, user, resource: project) + user + end + + let(:u_custom_notification_disabled) do + user = create_user_with_notification(:custom, 'custom_disabled') + update_custom_notification(:success_pipeline, user, resource: project, value: false) + update_custom_notification(:failed_pipeline, user, resource: project, value: false) + user + end let(:commit) { project.commit } - let(:pipeline) do - create(:ci_pipeline, :success, + + def create_pipeline(user, status) + create(:ci_pipeline, status, project: project, - user: current_user, + user: user, ref: 'refs/heads/master', sha: commit.id, before_sha: '00000000') end before do - project.add_master(current_user) project.add_master(u_member) + project.add_master(u_watcher) + project.add_master(u_custom_notification_unset) + project.add_master(u_custom_notification_enabled) + project.add_master(u_custom_notification_disabled) + reset_delivered_emails! end - context 'without custom recipients' do - it 'notifies the pipeline user' do - notification.pipeline_finished(pipeline) + context 'with a successful pipeline' do + context 'when the creator has default settings' do + before do + pipeline = create_pipeline(u_member, :success) + notification.pipeline_finished(pipeline) + end + + it 'notifies nobody' do + should_not_email_anyone + end + end + + context 'when the creator has watch set' do + before do + pipeline = create_pipeline(u_watcher, :success) + notification.pipeline_finished(pipeline) + end + + it 'notifies nobody' do + should_not_email_anyone + end + end + + context 'when the creator has custom notifications, but without any set' do + before do + pipeline = create_pipeline(u_custom_notification_unset, :success) + notification.pipeline_finished(pipeline) + end + + it 'notifies nobody' do + should_not_email_anyone + end + end + + context 'when the creator has custom notifications disabled' do + before do + pipeline = create_pipeline(u_custom_notification_disabled, :success) + notification.pipeline_finished(pipeline) + end + + it 'notifies nobody' do + should_not_email_anyone + end + end + + context 'when the creator has custom notifications enabled' do + before do + pipeline = create_pipeline(u_custom_notification_enabled, :success) + notification.pipeline_finished(pipeline) + end - should_only_email(current_user, kind: :bcc) + it 'emails only the creator' do + should_only_email(u_custom_notification_enabled, kind: :bcc) + end end end - context 'with custom recipients' do - it 'notifies the custom recipients' do - users = [u_member, u_other] - notification.pipeline_finished(pipeline, users.map(&:notification_email)) + context 'with a failed pipeline' do + context 'when the creator has no custom notification set' do + before do + pipeline = create_pipeline(u_member, :failed) + notification.pipeline_finished(pipeline) + end + + it 'emails only the creator' do + should_only_email(u_member, kind: :bcc) + end + end + + context 'when the creator has watch set' do + before do + pipeline = create_pipeline(u_watcher, :failed) + notification.pipeline_finished(pipeline) + end + + it 'emails only the creator' do + should_only_email(u_watcher, kind: :bcc) + end + end + + context 'when the creator has custom notifications, but without any set' do + before do + pipeline = create_pipeline(u_custom_notification_unset, :failed) + notification.pipeline_finished(pipeline) + end + + it 'emails only the creator' do + should_only_email(u_custom_notification_unset, kind: :bcc) + end + end + + context 'when the creator has custom notifications disabled' do + before do + pipeline = create_pipeline(u_custom_notification_disabled, :failed) + notification.pipeline_finished(pipeline) + end + + it 'notifies nobody' do + should_not_email_anyone + end + end + + context 'when the creator has custom notifications set' do + before do + pipeline = create_pipeline(u_custom_notification_enabled, :failed) + notification.pipeline_finished(pipeline) + end + + it 'emails only the creator' do + should_only_email(u_custom_notification_enabled, kind: :bcc) + end + end + + context 'when the creator has no read_build access' do + before do + pipeline = create_pipeline(u_member, :failed) + project.update(public_builds: false) + project.team.truncate + notification.pipeline_finished(pipeline) + end - should_only_email(*users, kind: :bcc) + it 'does not send emails' do + should_not_email_anyone + end end end end @@ -1385,9 +1537,9 @@ describe NotificationService, services: true do # Create custom notifications # When resource is nil it means global notification - def update_custom_notification(event, user, resource = nil) + def update_custom_notification(event, user, resource: nil, value: true) setting = user.notification_settings_for(resource) - setting.events[event] = true + setting.events[event] = value setting.save end diff --git a/spec/services/preview_markdown_service_spec.rb b/spec/services/preview_markdown_service_spec.rb new file mode 100644 index 00000000000..b2fb5c91313 --- /dev/null +++ b/spec/services/preview_markdown_service_spec.rb @@ -0,0 +1,67 @@ +require 'spec_helper' + +describe PreviewMarkdownService do + let(:user) { create(:user) } + let(:project) { create(:empty_project) } + + before do + project.add_developer(user) + end + + describe 'user references' do + let(:params) { { text: "Take a look #{user.to_reference}" } } + let(:service) { described_class.new(project, user, params) } + + it 'returns users referenced in text' do + result = service.execute + + expect(result[:users]).to eq [user.username] + end + end + + context 'new note with slash commands' do + let(:issue) { create(:issue, project: project) } + let(:params) do + { + text: "Please do it\n/assign #{user.to_reference}", + slash_commands_target_type: 'Issue', + slash_commands_target_id: issue.id + } + end + let(:service) { described_class.new(project, user, params) } + + it 'removes slash commands from text' do + result = service.execute + + expect(result[:text]).to eq 'Please do it' + end + + it 'explains slash commands effect' do + result = service.execute + + expect(result[:commands]).to eq "Assigns #{user.to_reference}." + end + end + + context 'merge request description' do + let(:params) do + { + text: "My work\n/estimate 2y", + slash_commands_target_type: 'MergeRequest' + } + end + let(:service) { described_class.new(project, user, params) } + + it 'removes slash commands from text' do + result = service.execute + + expect(result[:text]).to eq 'My work' + end + + it 'explains slash commands effect' do + result = service.execute + + expect(result[:commands]).to eq 'Sets time estimate to 2y.' + end + end +end diff --git a/spec/services/projects/autocomplete_service_spec.rb b/spec/services/projects/autocomplete_service_spec.rb index 7916c2d957c..c198c3eedfc 100644 --- a/spec/services/projects/autocomplete_service_spec.rb +++ b/spec/services/projects/autocomplete_service_spec.rb @@ -11,7 +11,7 @@ describe Projects::AutocompleteService, services: true do let(:project) { create(:empty_project, :public) } let!(:issue) { create(:issue, project: project, title: 'Issue 1') } let!(:security_issue_1) { create(:issue, :confidential, project: project, title: 'Security issue 1', author: author) } - let!(:security_issue_2) { create(:issue, :confidential, title: 'Security issue 2', project: project, assignee: assignee) } + let!(:security_issue_2) { create(:issue, :confidential, title: 'Security issue 2', project: project, assignees: [assignee]) } it 'does not list project confidential issues for guests' do autocomplete = described_class.new(project, nil) diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 62f21049b0b..033e6ecd18c 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -27,6 +27,22 @@ describe Projects::CreateService, '#execute', services: true do end end + context "admin creates project with other user's namespace_id" do + it 'sets the correct permissions' do + admin = create(:admin) + opts = { + name: 'GitLab', + namespace_id: user.namespace.id + } + project = create_project(admin, opts) + + expect(project).to be_persisted + expect(project.owner).to eq(user) + expect(project.team.masters).to include(user, admin) + expect(project.namespace).to eq(user.namespace) + end + end + context 'group namespace' do let(:group) do create(:group).tap do |group| @@ -144,6 +160,20 @@ describe Projects::CreateService, '#execute', services: true do end end + context 'when a bad service template is created' do + before do + create(:service, type: 'DroneCiService', project: nil, template: true, active: true) + end + + it 'reports an error in the imported project' do + opts[:import_url] = 'http://www.gitlab.com/gitlab-org/gitlab-ce' + project = create_project(user, opts) + + expect(project.errors.full_messages_for(:base).first).to match /Unable to save project. Error: Unable to save DroneCiService/ + expect(project.services.count).to eq 0 + end + end + def create_project(user, opts) Projects::CreateService.new(user, opts).execute end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index b1e10f4562e..4b8589b2736 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -7,6 +7,11 @@ describe Projects::DestroyService, services: true do let!(:remove_path) { path.sub(/\.git\Z/, "+#{project.id}+deleted.git") } let!(:async) { false } # execute or async_execute + before do + stub_container_registry_config(enabled: true) + stub_container_registry_tags(repository: :any, tags: []) + end + shared_examples 'deleting the project' do it 'deletes the project' do expect(Project.unscoped.all).not_to include(project) @@ -89,30 +94,64 @@ describe Projects::DestroyService, services: true do it_behaves_like 'deleting the project with pipeline and build' end - context 'container registry' do - before do - stub_container_registry_config(enabled: true) - stub_container_registry_tags('tag') - end + describe 'container registry' do + context 'when there are regular container repositories' do + let(:container_repository) { create(:container_repository) } + + before do + stub_container_registry_tags(repository: project.full_path + '/image', + tags: ['tag']) + project.container_repositories << container_repository + end + + context 'when image repository deletion succeeds' do + it 'removes tags' do + expect_any_instance_of(ContainerRepository) + .to receive(:delete_tags!).and_return(true) + + destroy_project(project, user) + end + end - context 'tags deletion succeeds' do - it do - expect_any_instance_of(ContainerRegistry::Tag).to receive(:delete).and_return(true) + context 'when image repository deletion fails' do + it 'raises an exception' do + expect_any_instance_of(ContainerRepository) + .to receive(:delete_tags!).and_return(false) - destroy_project(project, user, {}) + expect{ destroy_project(project, user) } + .to raise_error(ActiveRecord::RecordNotDestroyed) + end end end - context 'tags deletion fails' do - before { expect_any_instance_of(ContainerRegistry::Tag).to receive(:delete).and_return(false) } + context 'when there are tags for legacy root repository' do + before do + stub_container_registry_tags(repository: project.full_path, + tags: ['tag']) + end + + context 'when image repository tags deletion succeeds' do + it 'removes tags' do + expect_any_instance_of(ContainerRepository) + .to receive(:delete_tags!).and_return(true) - subject { destroy_project(project, user, {}) } + destroy_project(project, user) + end + end + + context 'when image repository tags deletion fails' do + it 'raises an exception' do + expect_any_instance_of(ContainerRepository) + .to receive(:delete_tags!).and_return(false) - it { expect{subject}.to raise_error(Projects::DestroyService::DestroyError) } + expect { destroy_project(project, user) } + .to raise_error(Projects::DestroyService::DestroyError) + end + end end end - def destroy_project(project, user, params) + def destroy_project(project, user, params = {}) if async Projects::DestroyService.new(project, user, params).async_execute else diff --git a/spec/services/projects/enable_deploy_key_service_spec.rb b/spec/services/projects/enable_deploy_key_service_spec.rb index a37510cf159..78626fbad4b 100644 --- a/spec/services/projects/enable_deploy_key_service_spec.rb +++ b/spec/services/projects/enable_deploy_key_service_spec.rb @@ -21,6 +21,16 @@ describe Projects::EnableDeployKeyService, services: true do end end + context 'add the same key twice' do + before do + project.deploy_keys << deploy_key + end + + it 'returns existing key' do + expect(service.execute).to eq(deploy_key) + end + end + def service Projects::EnableDeployKeyService.new(project, user, params) end diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb index eaf63457b32..fff12beed71 100644 --- a/spec/services/projects/housekeeping_service_spec.rb +++ b/spec/services/projects/housekeeping_service_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Projects::HousekeepingService do - subject { Projects::HousekeepingService.new(project) } + subject { described_class.new(project) } let(:project) { create(:project, :repository) } before do diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index e5917bb0b7a..852a4ac852f 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -26,30 +26,68 @@ describe Projects::ImportService, services: true do result = subject.execute expect(result[:status]).to eq :error - expect(result[:message]).to eq 'The repository could not be created.' + expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - The repository could not be created." end end context 'with known url' do before do project.import_url = 'https://github.com/vim/vim.git' + project.import_type = 'github' end - it 'succeeds if repository import is successfully' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true) + context 'with a Github repository' do + it 'succeeds if repository import is successfully' do + expect_any_instance_of(Repository).to receive(:fetch_remote).and_return(true) + expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(true) - result = subject.execute + result = subject.execute - expect(result[:status]).to eq :success + expect(result[:status]).to eq :success + end + + it 'fails if repository import fails' do + expect_any_instance_of(Repository).to receive(:fetch_remote).and_raise(Gitlab::Shell::Error.new('Failed to import the repository')) + + result = subject.execute + + expect(result[:status]).to eq :error + expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - Failed to import the repository" + end + + it 'does not remove the GitHub remote' do + expect_any_instance_of(Repository).to receive(:fetch_remote).and_return(true) + expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(true) + + subject.execute + + expect(project.repository.raw_repository.remote_names).to include('github') + end end - it 'fails if repository import fails' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_raise(Gitlab::Shell::Error.new('Failed to import the repository')) + context 'with a non Github repository' do + before do + project.import_url = 'https://bitbucket.org/vim/vim.git' + project.import_type = 'bitbucket' + end - result = subject.execute + it 'succeeds if repository import is successfully' do + expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_return(true) + expect_any_instance_of(Gitlab::BitbucketImport::Importer).to receive(:execute).and_return(true) - expect(result[:status]).to eq :error - expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - Failed to import the repository" + result = subject.execute + + expect(result[:status]).to eq :success + end + + it 'fails if repository import fails' do + expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_raise(Gitlab::Shell::Error.new('Failed to import the repository')) + + result = subject.execute + + expect(result[:status]).to eq :error + expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - Failed to import the repository" + end end end @@ -64,8 +102,8 @@ describe Projects::ImportService, services: true do end it 'succeeds if importer succeeds' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true) - expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(true) + allow_any_instance_of(Repository).to receive(:fetch_remote).and_return(true) + allow_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(true) result = subject.execute @@ -73,48 +111,42 @@ describe Projects::ImportService, services: true do end it 'flushes various caches' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository). - with(project.repository_storage_path, project.path_with_namespace, project.import_url). + allow_any_instance_of(Repository).to receive(:fetch_remote). and_return(true) - expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute). + allow_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute). and_return(true) - expect_any_instance_of(Repository).to receive(:expire_emptiness_caches). - and_call_original - - expect_any_instance_of(Repository).to receive(:expire_exists_cache). - and_call_original + expect_any_instance_of(Repository).to receive(:expire_content_cache) subject.execute end it 'fails if importer fails' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true) - expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(false) + allow_any_instance_of(Repository).to receive(:fetch_remote).and_return(true) + allow_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(false) result = subject.execute expect(result[:status]).to eq :error - expect(result[:message]).to eq 'The remote data could not be imported.' + expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - The remote data could not be imported." end it 'fails if importer raise an error' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true) - expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_raise(Projects::ImportService::Error.new('Github: failed to connect API')) + allow_any_instance_of(Gitlab::Shell).to receive(:fetch_remote).and_return(true) + allow_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_raise(Projects::ImportService::Error.new('Github: failed to connect API')) result = subject.execute expect(result[:status]).to eq :error - expect(result[:message]).to eq 'Github: failed to connect API' + expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - Github: failed to connect API" end - it 'expires existence cache after error' do + it 'expires content cache after error' do allow_any_instance_of(Project).to receive(:repository_exists?).and_return(false, true) - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_raise(Gitlab::Shell::Error.new('Failed to import the repository')) - expect_any_instance_of(Repository).to receive(:expire_emptiness_caches).and_call_original - expect_any_instance_of(Repository).to receive(:expire_exists_cache).and_call_original + expect_any_instance_of(Gitlab::Shell).to receive(:fetch_remote).and_raise(Gitlab::Shell::Error.new('Failed to import the repository')) + expect_any_instance_of(Repository).to receive(:expire_content_cache) subject.execute end diff --git a/spec/services/projects/participants_service_spec.rb b/spec/services/projects/participants_service_spec.rb index 063b3bd76eb..0657b7e93fe 100644 --- a/spec/services/projects/participants_service_spec.rb +++ b/spec/services/projects/participants_service_spec.rb @@ -6,7 +6,6 @@ describe Projects::ParticipantsService, services: true do let(:project) { create(:empty_project, :public) } let(:group) { create(:group, avatar: fixture_file_upload(Rails.root + 'spec/fixtures/dk.png')) } let(:user) { create(:user) } - let(:base_url) { Settings.send(:build_base_gitlab_url) } let!(:group_member) { create(:group_member, group: group, user: user) } it 'should return an url for the avatar' do @@ -14,7 +13,7 @@ describe Projects::ParticipantsService, services: true do groups = participants.groups expect(groups.size).to eq 1 - expect(groups.first[:avatar_url]).to eq "#{base_url}/uploads/group/avatar/#{group.id}/dk.png" + expect(groups.first[:avatar_url]).to eq("/uploads/group/avatar/#{group.id}/dk.png") end it 'should return an url for the avatar with relative url' do @@ -25,7 +24,7 @@ describe Projects::ParticipantsService, services: true do groups = participants.groups expect(groups.size).to eq 1 - expect(groups.first[:avatar_url]).to eq "#{base_url}/gitlab/uploads/group/avatar/#{group.id}/dk.png" + expect(groups.first[:avatar_url]).to eq("/gitlab/uploads/group/avatar/#{group.id}/dk.png") end end end diff --git a/spec/services/projects/propagate_service_template_spec.rb b/spec/services/projects/propagate_service_template_spec.rb new file mode 100644 index 00000000000..90eff3bbc1e --- /dev/null +++ b/spec/services/projects/propagate_service_template_spec.rb @@ -0,0 +1,103 @@ +require 'spec_helper' + +describe Projects::PropagateServiceTemplate, services: true do + describe '.propagate' do + let!(:service_template) do + PushoverService.create( + template: true, + active: true, + properties: { + device: 'MyDevice', + sound: 'mic', + priority: 4, + user_key: 'asdf', + api_key: '123456789' + }) + end + + let!(:project) { create(:empty_project) } + + it 'creates services for projects' do + expect(project.pushover_service).to be_nil + + described_class.propagate(service_template) + + expect(project.reload.pushover_service).to be_present + end + + it 'creates services for a project that has another service' do + BambooService.create( + template: true, + active: true, + project: project, + properties: { + bamboo_url: 'http://gitlab.com', + username: 'mic', + password: "password", + build_key: 'build' + } + ) + + expect(project.pushover_service).to be_nil + + described_class.propagate(service_template) + + expect(project.reload.pushover_service).to be_present + end + + it 'does not create the service if it exists already' do + other_service = BambooService.create( + template: true, + active: true, + properties: { + bamboo_url: 'http://gitlab.com', + username: 'mic', + password: "password", + build_key: 'build' + } + ) + + Service.build_from_template(project.id, service_template).save! + Service.build_from_template(project.id, other_service).save! + + expect { described_class.propagate(service_template) }. + not_to change { Service.count } + end + + it 'creates the service containing the template attributes' do + described_class.propagate(service_template) + + expect(project.pushover_service.properties).to eq(service_template.properties) + end + + describe 'bulk update' do + it 'creates services for all projects' do + project_total = 5 + stub_const 'Projects::PropagateServiceTemplate::BATCH_SIZE', 3 + + project_total.times { create(:empty_project) } + + expect { described_class.propagate(service_template) }. + to change { Service.count }.by(project_total + 1) + end + end + + describe 'external tracker' do + it 'updates the project external tracker' do + service_template.update!(category: 'issue_tracker', default: false) + + expect { described_class.propagate(service_template) }. + to change { project.reload.has_external_issue_tracker }.to(true) + end + end + + describe 'external wiki' do + it 'updates the project external tracker' do + service_template.update!(type: 'ExternalWikiService') + + expect { described_class.propagate(service_template) }. + to change { project.reload.has_external_wiki }.to(true) + end + end + end +end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index f8187fefc14..29ccce59c53 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -29,9 +29,12 @@ describe Projects::TransferService, services: true do end context 'disallow transfering of project with tags' do + let(:container_repository) { create(:container_repository) } + before do stub_container_registry_config(enabled: true) - stub_container_registry_tags('tag') + stub_container_registry_tags(repository: :any, tags: ['tag']) + project.container_repositories << container_repository end subject { transfer_project(project, user, group) } diff --git a/spec/services/protected_branches/update_service_spec.rb b/spec/services/protected_branches/update_service_spec.rb new file mode 100644 index 00000000000..62bdd49a4d7 --- /dev/null +++ b/spec/services/protected_branches/update_service_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +describe ProtectedBranches::UpdateService, services: true do + let(:protected_branch) { create(:protected_branch) } + let(:project) { protected_branch.project } + let(:user) { project.owner } + let(:params) { { name: 'new protected branch name' } } + + describe '#execute' do + subject(:service) { described_class.new(project, user, params) } + + it 'updates a protected branch' do + result = service.execute(protected_branch) + + expect(result.reload.name).to eq(params[:name]) + end + + context 'without admin_project permissions' do + let(:user) { create(:user) } + + it "raises error" do + expect{ service.execute(protected_branch) }.to raise_error(Gitlab::Access::AccessDeniedError) + end + end + end +end diff --git a/spec/services/protected_tags/create_service_spec.rb b/spec/services/protected_tags/create_service_spec.rb new file mode 100644 index 00000000000..d91a58e8de5 --- /dev/null +++ b/spec/services/protected_tags/create_service_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +describe ProtectedTags::CreateService, services: true do + let(:project) { create(:empty_project) } + let(:user) { project.owner } + let(:params) do + { + name: 'master', + create_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }] + } + end + + describe '#execute' do + subject(:service) { described_class.new(project, user, params) } + + it 'creates a new protected tag' do + expect { service.execute }.to change(ProtectedTag, :count).by(1) + expect(project.protected_tags.last.create_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER]) + end + end +end diff --git a/spec/services/protected_tags/update_service_spec.rb b/spec/services/protected_tags/update_service_spec.rb new file mode 100644 index 00000000000..e78fde4c48d --- /dev/null +++ b/spec/services/protected_tags/update_service_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +describe ProtectedTags::UpdateService, services: true do + let(:protected_tag) { create(:protected_tag) } + let(:project) { protected_tag.project } + let(:user) { project.owner } + let(:params) { { name: 'new protected tag name' } } + + describe '#execute' do + subject(:service) { described_class.new(project, user, params) } + + it 'updates a protected tag' do + result = service.execute(protected_tag) + + expect(result.reload.name).to eq(params[:name]) + end + + context 'without admin_project permissions' do + let(:user) { create(:user) } + + it "raises error" do + expect{ service.execute(protected_tag) }.to raise_error(Gitlab::Access::AccessDeniedError) + end + end + end +end diff --git a/spec/services/search/global_service_spec.rb b/spec/services/search/global_service_spec.rb new file mode 100644 index 00000000000..cbf4f56213d --- /dev/null +++ b/spec/services/search/global_service_spec.rb @@ -0,0 +1,45 @@ +require 'spec_helper' + +describe Search::GlobalService, services: true do + let(:user) { create(:user) } + let(:internal_user) { create(:user) } + + let!(:found_project) { create(:empty_project, :private, name: 'searchable_project') } + let!(:unfound_project) { create(:empty_project, :private, name: 'unfound_project') } + let!(:internal_project) { create(:empty_project, :internal, name: 'searchable_internal_project') } + let!(:public_project) { create(:empty_project, :public, name: 'searchable_public_project') } + + before do + found_project.add_master(user) + end + + describe '#execute' do + context 'unauthenticated' do + it 'returns public projects only' do + results = Search::GlobalService.new(nil, search: "searchable").execute + + expect(results.objects('projects')).to match_array [public_project] + end + end + + context 'authenticated' do + it 'returns public, internal and private projects' do + results = Search::GlobalService.new(user, search: "searchable").execute + + expect(results.objects('projects')).to match_array [public_project, found_project, internal_project] + end + + it 'returns only public & internal projects' do + results = Search::GlobalService.new(internal_user, search: "searchable").execute + + expect(results.objects('projects')).to match_array [internal_project, public_project] + end + + it 'namespace name is searchable' do + results = Search::GlobalService.new(user, search: found_project.namespace.path).execute + + expect(results.objects('projects')).to match_array [found_project] + end + end + end +end diff --git a/spec/services/search/group_service_spec.rb b/spec/services/search/group_service_spec.rb new file mode 100644 index 00000000000..38f264f6e7b --- /dev/null +++ b/spec/services/search/group_service_spec.rb @@ -0,0 +1,40 @@ +require 'spec_helper' + +describe Search::GroupService, services: true do + shared_examples_for 'group search' do + context 'finding projects by name' do + let(:user) { create(:user) } + let(:term) { "Project Name" } + let(:nested_group) { create(:group, :nested) } + + # These projects shouldn't be found + let!(:outside_project) { create(:empty_project, :public, name: "Outside #{term}") } + let!(:private_project) { create(:empty_project, :private, namespace: nested_group, name: "Private #{term}" )} + let!(:other_project) { create(:empty_project, :public, namespace: nested_group, name: term.reverse) } + + # These projects should be found + let!(:project1) { create(:empty_project, :internal, namespace: nested_group, name: "Inner #{term} 1") } + let!(:project2) { create(:empty_project, :internal, namespace: nested_group, name: "Inner #{term} 2") } + let!(:project3) { create(:empty_project, :internal, namespace: nested_group.parent, name: "Outer #{term}") } + + let(:results) { Search::GroupService.new(user, search_group, search: term).execute } + subject { results.objects('projects') } + + context 'in parent group' do + let(:search_group) { nested_group.parent } + + it { is_expected.to match_array([project1, project2, project3]) } + end + + context 'in subgroup' do + let(:search_group) { nested_group } + + it { is_expected.to match_array([project1, project2]) } + end + end + end + + describe 'basic search' do + include_examples 'group search' + end +end diff --git a/spec/services/search_service_spec.rb b/spec/services/search_service_spec.rb index 6ef5fa008aa..2112f1cf9ea 100644 --- a/spec/services/search_service_spec.rb +++ b/spec/services/search_service_spec.rb @@ -1,65 +1,286 @@ require 'spec_helper' -describe 'Search::GlobalService', services: true do +describe SearchService, services: true do let(:user) { create(:user) } - let(:public_user) { create(:user) } - let(:internal_user) { create(:user) } - let!(:found_project) { create(:empty_project, :private, name: 'searchable_project') } - let!(:unfound_project) { create(:empty_project, :private, name: 'unfound_project') } - let!(:internal_project) { create(:empty_project, :internal, name: 'searchable_internal_project') } - let!(:public_project) { create(:empty_project, :public, name: 'searchable_public_project') } + let(:accessible_group) { create(:group, :private) } + let(:inaccessible_group) { create(:group, :private) } + let!(:group_member) { create(:group_member, group: accessible_group, user: user) } + + let!(:accessible_project) { create(:empty_project, :private, name: 'accessible_project') } + let!(:inaccessible_project) { create(:empty_project, :private, name: 'inaccessible_project') } + let(:note) { create(:note_on_issue, project: accessible_project) } + + let(:snippet) { create(:snippet, author: user) } + let(:group_project) { create(:empty_project, group: accessible_group, name: 'group_project') } + let(:public_project) { create(:empty_project, :public, name: 'public_project') } before do - found_project.team << [user, :master] + accessible_project.add_master(user) + end + + describe '#project' do + context 'when the project is accessible' do + it 'returns the project' do + project = SearchService.new(user, project_id: accessible_project.id).project + + expect(project).to eq accessible_project + end + end + + context 'when the project is not accessible' do + it 'returns nil' do + project = SearchService.new(user, project_id: inaccessible_project.id).project + + expect(project).to be_nil + end + end + + context 'when there is no project_id' do + it 'returns nil' do + project = SearchService.new(user).project + + expect(project).to be_nil + end + end end - describe '#execute' do - context 'unauthenticated' do - it 'returns public projects only' do - context = Search::GlobalService.new(nil, search: "searchable") - results = context.execute - expect(results.objects('projects')).to match_array [public_project] + describe '#group' do + context 'when the group is accessible' do + it 'returns the group' do + group = SearchService.new(user, group_id: accessible_group.id).group + + expect(group).to eq accessible_group end end - context 'authenticated' do - it 'returns public, internal and private projects' do - context = Search::GlobalService.new(user, search: "searchable") - results = context.execute - expect(results.objects('projects')).to match_array [public_project, found_project, internal_project] + context 'when the group is not accessible' do + it 'returns nil' do + group = SearchService.new(user, group_id: inaccessible_group.id).group + + expect(group).to be_nil end + end + + context 'when there is no group_id' do + it 'returns nil' do + group = SearchService.new(user).group - it 'returns only public & internal projects' do - context = Search::GlobalService.new(internal_user, search: "searchable") - results = context.execute - expect(results.objects('projects')).to match_array [internal_project, public_project] + expect(group).to be_nil end + end + end + + describe '#show_snippets?' do + context 'when :snippets is \'true\'' do + it 'returns true' do + show_snippets = SearchService.new(user, snippets: 'true').show_snippets? - it 'namespace name is searchable' do - context = Search::GlobalService.new(user, search: found_project.namespace.path) - results = context.execute - expect(results.objects('projects')).to match_array [found_project] + expect(show_snippets).to be_truthy end + end - context 'nested group' do - let!(:nested_group) { create(:group, :nested) } - let!(:project) { create(:empty_project, namespace: nested_group) } + context 'when :snippets is not \'true\'' do + it 'returns false' do + show_snippets = SearchService.new(user, snippets: 'tru').show_snippets? + + expect(show_snippets).to be_falsey + end + end - before { project.add_master(user) } + context 'when :snippets is missing' do + it 'returns false' do + show_snippets = SearchService.new(user).show_snippets? - it 'returns result from nested group' do - context = Search::GlobalService.new(user, search: project.path) - results = context.execute - expect(results.objects('projects')).to match_array [project] + expect(show_snippets).to be_falsey + end + end + end + + describe '#scope' do + context 'with accessible project_id' do + context 'and allowed scope' do + it 'returns the specified scope' do + scope = SearchService.new(user, project_id: accessible_project.id, scope: 'notes').scope + + expect(scope).to eq 'notes' end + end + + context 'and disallowed scope' do + it 'returns the default scope' do + scope = SearchService.new(user, project_id: accessible_project.id, scope: 'projects').scope - it 'returns result from descendants when search inside group' do - context = Search::GlobalService.new(user, search: project.path, group_id: nested_group.parent) - results = context.execute - expect(results.objects('projects')).to match_array [project] + expect(scope).to eq 'blobs' end end + + context 'and no scope' do + it 'returns the default scope' do + scope = SearchService.new(user, project_id: accessible_project.id).scope + + expect(scope).to eq 'blobs' + end + end + end + + context 'with \'true\' snippets' do + context 'and allowed scope' do + it 'returns the specified scope' do + scope = SearchService.new(user, snippets: 'true', scope: 'snippet_titles').scope + + expect(scope).to eq 'snippet_titles' + end + end + + context 'and disallowed scope' do + it 'returns the default scope' do + scope = SearchService.new(user, snippets: 'true', scope: 'projects').scope + + expect(scope).to eq 'snippet_blobs' + end + end + + context 'and no scope' do + it 'returns the default scope' do + scope = SearchService.new(user, snippets: 'true').scope + + expect(scope).to eq 'snippet_blobs' + end + end + end + + context 'with no project_id, no snippets' do + context 'and allowed scope' do + it 'returns the specified scope' do + scope = SearchService.new(user, scope: 'issues').scope + + expect(scope).to eq 'issues' + end + end + + context 'and disallowed scope' do + it 'returns the default scope' do + scope = SearchService.new(user, scope: 'blobs').scope + + expect(scope).to eq 'projects' + end + end + + context 'and no scope' do + it 'returns the default scope' do + scope = SearchService.new(user).scope + + expect(scope).to eq 'projects' + end + end + end + end + + describe '#search_results' do + context 'with accessible project_id' do + it 'returns an instance of Gitlab::ProjectSearchResults' do + search_results = SearchService.new( + user, + project_id: accessible_project.id, + scope: 'notes', + search: note.note).search_results + + expect(search_results).to be_a Gitlab::ProjectSearchResults + end + end + + context 'with accessible project_id and \'true\' snippets' do + it 'returns an instance of Gitlab::ProjectSearchResults' do + search_results = SearchService.new( + user, + project_id: accessible_project.id, + snippets: 'true', + scope: 'notes', + search: note.note).search_results + + expect(search_results).to be_a Gitlab::ProjectSearchResults + end + end + + context 'with \'true\' snippets' do + it 'returns an instance of Gitlab::SnippetSearchResults' do + search_results = SearchService.new( + user, + snippets: 'true', + search: snippet.content).search_results + + expect(search_results).to be_a Gitlab::SnippetSearchResults + end + end + + context 'with no project_id and no snippets' do + it 'returns an instance of Gitlab::SearchResults' do + search_results = SearchService.new( + user, + search: public_project.name).search_results + + expect(search_results).to be_a Gitlab::SearchResults + end + end + end + + describe '#search_objects' do + context 'with accessible project_id' do + it 'returns objects in the project' do + search_objects = SearchService.new( + user, + project_id: accessible_project.id, + scope: 'notes', + search: note.note).search_objects + + expect(search_objects.first).to eq note + end + end + + context 'with accessible project_id and \'true\' snippets' do + it 'returns objects in the project' do + search_objects = SearchService.new( + user, + project_id: accessible_project.id, + snippets: 'true', + scope: 'notes', + search: note.note).search_objects + + expect(search_objects.first).to eq note + end + end + + context 'with \'true\' snippets' do + it 'returns objects in snippets' do + search_objects = SearchService.new( + user, + snippets: 'true', + search: snippet.content).search_objects + + expect(search_objects.first).to eq snippet + end + end + + context 'with accessible group_id' do + it 'returns objects in the group' do + search_objects = SearchService.new( + user, + group_id: accessible_group.id, + search: group_project.name).search_objects + + expect(search_objects.first).to eq group_project + end + end + + context 'with no project_id, group_id or snippets' do + it 'returns objects in global' do + search_objects = SearchService.new( + user, + search: public_project.name).search_objects + + expect(search_objects.first).to eq public_project + end end end end diff --git a/spec/services/slash_commands/interpret_service_spec.rb b/spec/services/slash_commands/interpret_service_spec.rb index a63281f0eab..e5e400ee281 100644 --- a/spec/services/slash_commands/interpret_service_spec.rb +++ b/spec/services/slash_commands/interpret_service_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe SlashCommands::InterpretService, services: true do let(:project) { create(:empty_project, :public) } let(:developer) { create(:user) } + let(:developer2) { create(:user) } let(:issue) { create(:issue, project: project) } let(:milestone) { create(:milestone, project: project, title: '9.10') } let(:inprogress) { create(:label, project: project, title: 'In Progress') } @@ -42,23 +43,6 @@ describe SlashCommands::InterpretService, services: true do end end - shared_examples 'assign command' do - it 'fetches assignee and populates assignee_id if content contains /assign' do - _, updates = service.execute(content, issuable) - - expect(updates).to eq(assignee_id: developer.id) - end - end - - shared_examples 'unassign command' do - it 'populates assignee_id: nil if content contains /unassign' do - issuable.update(assignee_id: developer.id) - _, updates = service.execute(content, issuable) - - expect(updates).to eq(assignee_id: nil) - end - end - shared_examples 'milestone command' do it 'fetches milestone and populates milestone_id if content contains /milestone' do milestone # populate the milestone @@ -70,7 +54,7 @@ describe SlashCommands::InterpretService, services: true do shared_examples 'remove_milestone command' do it 'populates milestone_id: nil if content contains /remove_milestone' do - issuable.update(milestone_id: milestone.id) + issuable.update!(milestone_id: milestone.id) _, updates = service.execute(content, issuable) expect(updates).to eq(milestone_id: nil) @@ -108,7 +92,7 @@ describe SlashCommands::InterpretService, services: true do shared_examples 'unlabel command' do it 'fetches label ids and populates remove_label_ids if content contains /unlabel' do - issuable.update(label_ids: [inprogress.id]) # populate the label + issuable.update!(label_ids: [inprogress.id]) # populate the label _, updates = service.execute(content, issuable) expect(updates).to eq(remove_label_ids: [inprogress.id]) @@ -117,7 +101,7 @@ describe SlashCommands::InterpretService, services: true do shared_examples 'multiple unlabel command' do it 'fetches label ids and populates remove_label_ids if content contains mutiple /unlabel' do - issuable.update(label_ids: [inprogress.id, bug.id]) # populate the label + issuable.update!(label_ids: [inprogress.id, bug.id]) # populate the label _, updates = service.execute(content, issuable) expect(updates).to eq(remove_label_ids: [inprogress.id, bug.id]) @@ -126,7 +110,7 @@ describe SlashCommands::InterpretService, services: true do shared_examples 'unlabel command with no argument' do it 'populates label_ids: [] if content contains /unlabel with no arguments' do - issuable.update(label_ids: [inprogress.id]) # populate the label + issuable.update!(label_ids: [inprogress.id]) # populate the label _, updates = service.execute(content, issuable) expect(updates).to eq(label_ids: []) @@ -135,7 +119,7 @@ describe SlashCommands::InterpretService, services: true do shared_examples 'relabel command' do it 'populates label_ids: [] if content contains /relabel' do - issuable.update(label_ids: [bug.id]) # populate the label + issuable.update!(label_ids: [bug.id]) # populate the label inprogress # populate the label _, updates = service.execute(content, issuable) @@ -187,7 +171,7 @@ describe SlashCommands::InterpretService, services: true do shared_examples 'remove_due_date command' do it 'populates due_date: nil if content contains /remove_due_date' do - issuable.update(due_date: Date.today) + issuable.update!(due_date: Date.today) _, updates = service.execute(content, issuable) expect(updates).to eq(due_date: nil) @@ -204,7 +188,7 @@ describe SlashCommands::InterpretService, services: true do shared_examples 'unwip command' do it 'returns wip_event: "unwip" if content contains /wip' do - issuable.update(title: issuable.wip_title) + issuable.update!(title: issuable.wip_title) _, updates = service.execute(content, issuable) expect(updates).to eq(wip_event: 'unwip') @@ -371,14 +355,46 @@ describe SlashCommands::InterpretService, services: true do let(:issuable) { issue } end - it_behaves_like 'assign command' do + context 'assign command' do let(:content) { "/assign @#{developer.username}" } - let(:issuable) { issue } + + context 'Issue' do + it 'fetches assignee and populates assignee_id if content contains /assign' do + _, updates = service.execute(content, issue) + + expect(updates).to eq(assignee_ids: [developer.id]) + end + end + + context 'Merge Request' do + it 'fetches assignee and populates assignee_id if content contains /assign' do + _, updates = service.execute(content, merge_request) + + expect(updates).to eq(assignee_id: developer.id) + end + end end - it_behaves_like 'assign command' do - let(:content) { "/assign @#{developer.username}" } - let(:issuable) { merge_request } + context 'assign command with multiple assignees' do + let(:content) { "/assign @#{developer.username} @#{developer2.username}" } + + before{ project.team << [developer2, :developer] } + + context 'Issue' do + it 'fetches assignee and populates assignee_id if content contains /assign' do + _, updates = service.execute(content, issue) + + expect(updates[:assignee_ids]).to match_array([developer.id, developer2.id]) + end + end + + context 'Merge Request' do + it 'fetches assignee and populates assignee_id if content contains /assign' do + _, updates = service.execute(content, merge_request) + + expect(updates).to eq(assignee_id: developer.id) + end + end end it_behaves_like 'empty command' do @@ -391,14 +407,26 @@ describe SlashCommands::InterpretService, services: true do let(:issuable) { issue } end - it_behaves_like 'unassign command' do + context 'unassign command' do let(:content) { '/unassign' } - let(:issuable) { issue } - end - it_behaves_like 'unassign command' do - let(:content) { '/unassign' } - let(:issuable) { merge_request } + context 'Issue' do + it 'populates assignee_ids: [] if content contains /unassign' do + issue.update(assignee_ids: [developer.id]) + _, updates = service.execute(content, issue) + + expect(updates).to eq(assignee_ids: []) + end + end + + context 'Merge Request' do + it 'populates assignee_id: nil if content contains /unassign' do + merge_request.update(assignee_id: developer.id) + _, updates = service.execute(content, merge_request) + + expect(updates).to eq(assignee_id: nil) + end + end end it_behaves_like 'milestone command' do @@ -727,5 +755,282 @@ describe SlashCommands::InterpretService, services: true do end end end + + context '/board_move command' do + let(:todo) { create(:label, project: project, title: 'To Do') } + let(:inreview) { create(:label, project: project, title: 'In Review') } + let(:content) { %{/board_move ~"#{inreview.title}"} } + + let!(:board) { create(:board, project: project) } + let!(:todo_list) { create(:list, board: board, label: todo) } + let!(:inreview_list) { create(:list, board: board, label: inreview) } + let!(:inprogress_list) { create(:list, board: board, label: inprogress) } + + it 'populates remove_label_ids for all current board columns' do + issue.update!(label_ids: [todo.id, inprogress.id]) + + _, updates = service.execute(content, issue) + + expect(updates[:remove_label_ids]).to match_array([todo.id, inprogress.id]) + end + + it 'populates add_label_ids with the id of the given label' do + _, updates = service.execute(content, issue) + + expect(updates[:add_label_ids]).to eq([inreview.id]) + end + + it 'does not include the given label id in remove_label_ids' do + issue.update!(label_ids: [todo.id, inreview.id]) + + _, updates = service.execute(content, issue) + + expect(updates[:remove_label_ids]).to match_array([todo.id]) + end + + it 'does not remove label ids that are not lists on the board' do + issue.update!(label_ids: [todo.id, bug.id]) + + _, updates = service.execute(content, issue) + + expect(updates[:remove_label_ids]).to match_array([todo.id]) + end + + context 'if the project has multiple boards' do + let(:issuable) { issue } + before { create(:board, project: project) } + it_behaves_like 'empty command' + end + + context 'if the given label does not exist' do + let(:issuable) { issue } + let(:content) { '/board_move ~"Fake Label"' } + it_behaves_like 'empty command' + end + + context 'if multiple labels are given' do + let(:issuable) { issue } + let(:content) { %{/board_move ~"#{inreview.title}" ~"#{todo.title}"} } + it_behaves_like 'empty command' + end + + context 'if the given label is not a list on the board' do + let(:issuable) { issue } + let(:content) { %{/board_move ~"#{bug.title}"} } + it_behaves_like 'empty command' + end + + context 'if issuable is not an Issue' do + let(:issuable) { merge_request } + it_behaves_like 'empty command' + end + end + end + + describe '#explain' do + let(:service) { described_class.new(project, developer) } + let(:merge_request) { create(:merge_request, source_project: project) } + + describe 'close command' do + let(:content) { '/close' } + + it 'includes issuable name' do + _, explanations = service.explain(content, issue) + + expect(explanations).to eq(['Closes this issue.']) + end + end + + describe 'reopen command' do + let(:content) { '/reopen' } + let(:merge_request) { create(:merge_request, :closed, source_project: project) } + + it 'includes issuable name' do + _, explanations = service.explain(content, merge_request) + + expect(explanations).to eq(['Reopens this merge request.']) + end + end + + describe 'title command' do + let(:content) { '/title This is new title' } + + it 'includes new title' do + _, explanations = service.explain(content, issue) + + expect(explanations).to eq(['Changes the title to "This is new title".']) + end + end + + describe 'assign command' do + let(:content) { "/assign @#{developer.username} do it!" } + + it 'includes only the user reference' do + _, explanations = service.explain(content, merge_request) + + expect(explanations).to eq(["Assigns @#{developer.username}."]) + end + end + + describe 'unassign command' do + let(:content) { '/unassign' } + let(:issue) { create(:issue, project: project, assignees: [developer]) } + + it 'includes current assignee reference' do + _, explanations = service.explain(content, issue) + + expect(explanations).to eq(["Removes assignee @#{developer.username}."]) + end + end + + describe 'milestone command' do + let(:content) { '/milestone %wrong-milestone' } + let!(:milestone) { create(:milestone, project: project, title: '9.10') } + + it 'is empty when milestone reference is wrong' do + _, explanations = service.explain(content, issue) + + expect(explanations).to eq([]) + end + end + + describe 'remove milestone command' do + let(:content) { '/remove_milestone' } + let(:merge_request) { create(:merge_request, source_project: project, milestone: milestone) } + + it 'includes current milestone name' do + _, explanations = service.explain(content, merge_request) + + expect(explanations).to eq(['Removes %"9.10" milestone.']) + end + end + + describe 'label command' do + let(:content) { '/label ~missing' } + let!(:label) { create(:label, project: project) } + + it 'is empty when there are no correct labels' do + _, explanations = service.explain(content, issue) + + expect(explanations).to eq([]) + end + end + + describe 'unlabel command' do + let(:content) { '/unlabel' } + + it 'says all labels if no parameter provided' do + merge_request.update!(label_ids: [bug.id]) + _, explanations = service.explain(content, merge_request) + + expect(explanations).to eq(['Removes all labels.']) + end + end + + describe 'relabel command' do + let(:content) { '/relabel Bug' } + let!(:bug) { create(:label, project: project, title: 'Bug') } + let(:feature) { create(:label, project: project, title: 'Feature') } + + it 'includes label name' do + issue.update!(label_ids: [feature.id]) + _, explanations = service.explain(content, issue) + + expect(explanations).to eq(["Replaces all labels with ~#{bug.id} label."]) + end + end + + describe 'subscribe command' do + let(:content) { '/subscribe' } + + it 'includes issuable name' do + _, explanations = service.explain(content, issue) + + expect(explanations).to eq(['Subscribes to this issue.']) + end + end + + describe 'unsubscribe command' do + let(:content) { '/unsubscribe' } + + it 'includes issuable name' do + merge_request.subscribe(developer, project) + _, explanations = service.explain(content, merge_request) + + expect(explanations).to eq(['Unsubscribes from this merge request.']) + end + end + + describe 'due command' do + let(:content) { '/due April 1st 2016' } + + it 'includes the date' do + _, explanations = service.explain(content, issue) + + expect(explanations).to eq(['Sets the due date to Apr 1, 2016.']) + end + end + + describe 'wip command' do + let(:content) { '/wip' } + + it 'includes the new status' do + _, explanations = service.explain(content, merge_request) + + expect(explanations).to eq(['Marks this merge request as Work In Progress.']) + end + end + + describe 'award command' do + let(:content) { '/award :confetti_ball: ' } + + it 'includes the emoji' do + _, explanations = service.explain(content, issue) + + expect(explanations).to eq(['Toggles :confetti_ball: emoji award.']) + end + end + + describe 'estimate command' do + let(:content) { '/estimate 79d' } + + it 'includes the formatted duration' do + _, explanations = service.explain(content, merge_request) + + expect(explanations).to eq(['Sets time estimate to 3mo 3w 4d.']) + end + end + + describe 'spend command' do + let(:content) { '/spend -120m' } + + it 'includes the formatted duration and proper verb' do + _, explanations = service.explain(content, issue) + + expect(explanations).to eq(['Substracts 2h spent time.']) + end + end + + describe 'target branch command' do + let(:content) { '/target_branch my-feature ' } + + it 'includes the branch name' do + _, explanations = service.explain(content, merge_request) + + expect(explanations).to eq(['Sets target branch to my-feature.']) + end + end + + describe 'board move command' do + let(:content) { '/board_move ~bug' } + let!(:bug) { create(:label, project: project, title: 'bug') } + let!(:board) { create(:board, project: project) } + + it 'includes the label name' do + _, explanations = service.explain(content, issue) + + expect(explanations).to eq(["Moves issue to ~#{bug.id} column in the board."]) + end + end end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 90cde705b85..7a9cd7553b1 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -6,6 +6,7 @@ describe SystemNoteService, services: true do let(:project) { create(:empty_project) } let(:author) { create(:user) } let(:noteable) { create(:issue, project: project) } + let(:issue) { noteable } shared_examples_for 'a system note' do let(:expected_noteable) { noteable } @@ -155,6 +156,52 @@ describe SystemNoteService, services: true do end end + describe '.change_issue_assignees' do + subject { described_class.change_issue_assignees(noteable, project, author, [assignee]) } + + let(:assignee) { create(:user) } + let(:assignee1) { create(:user) } + let(:assignee2) { create(:user) } + let(:assignee3) { create(:user) } + + it_behaves_like 'a system note' do + let(:action) { 'assignee' } + end + + def build_note(old_assignees, new_assignees) + issue.assignees = new_assignees + described_class.change_issue_assignees(issue, project, author, old_assignees).note + end + + it 'builds a correct phrase when an assignee is added to a non-assigned issue' do + expect(build_note([], [assignee1])).to eq "assigned to @#{assignee1.username}" + end + + it 'builds a correct phrase when assignee removed' do + expect(build_note([assignee1], [])).to eq 'removed assignee' + end + + it 'builds a correct phrase when assignees changed' do + expect(build_note([assignee1], [assignee2])).to eq \ + "assigned to @#{assignee2.username} and unassigned @#{assignee1.username}" + end + + it 'builds a correct phrase when three assignees removed and one added' do + expect(build_note([assignee, assignee1, assignee2], [assignee3])).to eq \ + "assigned to @#{assignee3.username} and unassigned @#{assignee.username}, @#{assignee1.username}, and @#{assignee2.username}" + end + + it 'builds a correct phrase when one assignee changed from a set' do + expect(build_note([assignee, assignee1], [assignee, assignee2])).to eq \ + "assigned to @#{assignee2.username} and unassigned @#{assignee1.username}" + end + + it 'builds a correct phrase when one assignee removed from a set' do + expect(build_note([assignee, assignee1, assignee2], [assignee, assignee1])).to eq \ + "unassigned @#{assignee2.username}" + end + end + describe '.change_label' do subject { described_class.change_label(noteable, project, author, added, removed) } @@ -221,26 +268,23 @@ describe SystemNoteService, services: true do describe '.change_status' do subject { described_class.change_status(noteable, project, author, status, source) } - let(:status) { 'new_status' } - let(:source) { nil } + context 'with status reopened' do + let(:status) { 'reopened' } + let(:source) { nil } - it_behaves_like 'a system note' do - let(:action) { 'status' } + it_behaves_like 'a system note' do + let(:action) { 'opened' } + end end context 'with a source' do + let(:status) { 'opened' } let(:source) { double('commit', gfm_reference: 'commit 123456') } it 'sets the note text' do expect(subject.note).to eq "#{status} via commit 123456" end end - - context 'without a source' do - it 'sets the note text' do - expect(subject.note).to eq status - end - end end describe '.merge_when_pipeline_succeeds' do @@ -295,12 +339,40 @@ describe SystemNoteService, services: true do end end + describe '.change_description' do + subject { described_class.change_description(noteable, project, author) } + + context 'when noteable responds to `description`' do + it_behaves_like 'a system note' do + let(:action) { 'description' } + end + + it 'sets the note text' do + expect(subject.note).to eq('changed the description') + end + end + end + describe '.change_issue_confidentiality' do subject { described_class.change_issue_confidentiality(noteable, project, author) } - context 'when noteable responds to `confidential`' do + context 'issue has been made confidential' do + before do + noteable.update_attribute(:confidential, true) + end + + it_behaves_like 'a system note' do + let(:action) { 'confidential' } + end + + it 'sets the note text' do + expect(subject.note).to eq 'made the issue confidential' + end + end + + context 'issue has been made visible' do it_behaves_like 'a system note' do - let(:action) { 'confidentiality' } + let(:action) { 'visible' } end it 'sets the note text' do @@ -584,7 +656,7 @@ describe SystemNoteService, services: true do end shared_examples 'cross project mentionable' do - include GitlabMarkdownHelper + include MarkupHelper it 'contains cross reference to new noteable' do expect(subject.note).to include cross_project_reference(new_project, new_noteable) @@ -785,7 +857,7 @@ describe SystemNoteService, services: true do end describe '.discussion_continued_in_issue' do - let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:discussion) { create(:diff_note_on_merge_request).to_discussion } let(:merge_request) { discussion.noteable } let(:project) { merge_request.source_project } let(:issue) { create(:issue, project: project) } diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 89b3b6aad10..175a42a32d9 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -25,11 +25,11 @@ describe TodoService, services: true do end describe 'Issues' do - let(:issue) { create(:issue, project: project, assignee: john_doe, author: author, description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } - let(:addressed_issue) { create(:issue, project: project, assignee: john_doe, author: author, description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } - let(:unassigned_issue) { create(:issue, project: project, assignee: nil) } - let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee, description: mentions) } - let(:addressed_confident_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee, description: directly_addressed) } + let(:issue) { create(:issue, project: project, assignees: [john_doe], author: author, description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } + let(:addressed_issue) { create(:issue, project: project, assignees: [john_doe], author: author, description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } + let(:unassigned_issue) { create(:issue, project: project, assignees: []) } + let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignees: [assignee], description: mentions) } + let(:addressed_confident_issue) { create(:issue, :confidential, project: project, author: author, assignees: [assignee], description: directly_addressed) } describe '#new_issue' do it 'creates a todo if assigned' do @@ -43,7 +43,7 @@ describe TodoService, services: true do end it 'creates a todo if assignee is the current user' do - unassigned_issue.update_attribute(:assignee, john_doe) + unassigned_issue.assignees = [john_doe] service.new_issue(unassigned_issue, john_doe) should_create_todo(user: john_doe, target: unassigned_issue, author: john_doe, action: Todo::ASSIGNED) @@ -258,20 +258,20 @@ describe TodoService, services: true do describe '#reassigned_issue' do it 'creates a pending todo for new assignee' do - unassigned_issue.update_attribute(:assignee, john_doe) + unassigned_issue.assignees << john_doe service.reassigned_issue(unassigned_issue, author) should_create_todo(user: john_doe, target: unassigned_issue, action: Todo::ASSIGNED) end it 'does not create a todo if unassigned' do - issue.update_attribute(:assignee, nil) + issue.assignees.destroy_all should_not_create_any_todo { service.reassigned_issue(issue, author) } end it 'creates a todo if new assignee is the current user' do - unassigned_issue.update_attribute(:assignee, john_doe) + unassigned_issue.assignees << john_doe service.reassigned_issue(unassigned_issue, john_doe) should_create_todo(user: john_doe, target: unassigned_issue, author: john_doe, action: Todo::ASSIGNED) @@ -361,7 +361,7 @@ describe TodoService, services: true do describe '#new_note' do let!(:first_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) } let!(:second_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) } - let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) } + let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignees: [assignee]) } let(:note) { create(:note, project: project, noteable: issue, author: john_doe, note: mentions) } let(:addressed_note) { create(:note, project: project, noteable: issue, author: john_doe, note: directly_addressed) } let(:note_on_commit) { create(:note_on_commit, project: project, author: john_doe, note: mentions) } @@ -854,7 +854,7 @@ describe TodoService, services: true do end it 'updates cached counts when a todo is created' do - issue = create(:issue, project: project, assignee: john_doe, author: author, description: mentions) + issue = create(:issue, project: project, assignees: [john_doe], author: author, description: mentions) expect(john_doe.todos_pending_count).to eq(0) expect(john_doe).to receive(:update_todos_count_cache).and_call_original @@ -866,8 +866,8 @@ describe TodoService, services: true do end describe '#mark_todos_as_done' do - let(:issue) { create(:issue, project: project, author: author, assignee: john_doe) } - let(:another_issue) { create(:issue, project: project, author: author, assignee: john_doe) } + let(:issue) { create(:issue, project: project, author: author, assignees: [john_doe]) } + let(:another_issue) { create(:issue, project: project, author: author, assignees: [john_doe]) } it 'marks a relation of todos as done' do create(:todo, :mentioned, user: john_doe, target: issue, project: project) diff --git a/spec/services/projects/upload_service_spec.rb b/spec/services/upload_service_spec.rb index d2cefa46bfa..95ba28dbecd 100644 --- a/spec/services/projects/upload_service_spec.rb +++ b/spec/services/upload_service_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Projects::UploadService, services: true do +describe UploadService, services: true do describe 'File service' do before do @user = create(:user) @@ -68,6 +68,6 @@ describe Projects::UploadService, services: true do end def upload_file(project, file) - Projects::UploadService.new(project, file).execute + described_class.new(project, file, FileUploader).execute end end diff --git a/spec/services/users/activity_service_spec.rb b/spec/services/users/activity_service_spec.rb new file mode 100644 index 00000000000..8d67ebe3231 --- /dev/null +++ b/spec/services/users/activity_service_spec.rb @@ -0,0 +1,48 @@ +require 'spec_helper' + +describe Users::ActivityService, services: true do + include UserActivitiesHelpers + + let(:user) { create(:user) } + + subject(:service) { described_class.new(user, 'type') } + + describe '#execute', :redis do + context 'when last activity is nil' do + before do + service.execute + end + + it 'sets the last activity timestamp for the user' do + expect(last_hour_user_ids).to eq([user.id]) + end + + it 'updates the same user' do + service.execute + + expect(last_hour_user_ids).to eq([user.id]) + end + + it 'updates the timestamp of an existing user' do + Timecop.freeze(Date.tomorrow) do + expect { service.execute }.to change { user_activity(user) }.to(Time.now.to_i.to_s) + end + end + + describe 'other user' do + it 'updates other user' do + other_user = create(:user) + described_class.new(other_user, 'type').execute + + expect(last_hour_user_ids).to match_array([user.id, other_user.id]) + end + end + end + end + + def last_hour_user_ids + Gitlab::UserActivities.new. + select { |k, v| v >= 1.hour.ago.to_i.to_s }. + map { |k, _| k.to_i } + end +end diff --git a/spec/services/users/build_service_spec.rb b/spec/services/users/build_service_spec.rb new file mode 100644 index 00000000000..2a6bfc1b3a0 --- /dev/null +++ b/spec/services/users/build_service_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper' + +describe Users::BuildService, services: true do + describe '#execute' do + let(:params) do + { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass' } + end + + context 'with an admin user' do + let(:admin_user) { create(:admin) } + let(:service) { described_class.new(admin_user, params) } + + it 'returns a valid user' do + expect(service.execute).to be_valid + end + end + + context 'with non admin user' do + let(:user) { create(:user) } + let(:service) { described_class.new(user, params) } + + it 'raises AccessDeniedError exception' do + expect { service.execute }.to raise_error Gitlab::Access::AccessDeniedError + end + end + + context 'with nil user' do + let(:service) { described_class.new(nil, params) } + + it 'returns a valid user' do + expect(service.execute).to be_valid + end + + context 'when "send_user_confirmation_email" application setting is true' do + before do + stub_application_setting(send_user_confirmation_email: true, signup_enabled?: true) + end + + it 'does not confirm the user' do + expect(service.execute).not_to be_confirmed + end + end + + context 'when "send_user_confirmation_email" application setting is false' do + before do + stub_application_setting(send_user_confirmation_email: false, signup_enabled?: true) + end + + it 'confirms the user' do + expect(service.execute).to be_confirmed + end + end + end + end +end diff --git a/spec/services/users/create_service_spec.rb b/spec/services/users/create_service_spec.rb index 66f68650f81..75746278573 100644 --- a/spec/services/users/create_service_spec.rb +++ b/spec/services/users/create_service_spec.rb @@ -1,38 +1,6 @@ require 'spec_helper' describe Users::CreateService, services: true do - describe '#build' do - let(:params) do - { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass' } - end - - context 'with an admin user' do - let(:admin_user) { create(:admin) } - let(:service) { described_class.new(admin_user, params) } - - it 'returns a valid user' do - expect(service.build).to be_valid - end - end - - context 'with non admin user' do - let(:user) { create(:user) } - let(:service) { described_class.new(user, params) } - - it 'raises AccessDeniedError exception' do - expect { service.build }.to raise_error Gitlab::Access::AccessDeniedError - end - end - - context 'with nil user' do - let(:service) { described_class.new(nil, params) } - - it 'returns a valid user' do - expect(service.build).to be_valid - end - end - end - describe '#execute' do let(:admin_user) { create(:admin) } @@ -122,6 +90,32 @@ describe Users::CreateService, services: true do end end + context 'when password_automatically_set parameter is true' do + let(:params) do + { + name: 'John Doe', + username: 'jduser', + email: 'jd@example.com', + password: 'mydummypass', + password_automatically_set: true + } + end + + it 'persists the given attributes' do + user = service.execute + user.reload + + expect(user).to have_attributes( + name: params[:name], + username: params[:username], + email: params[:email], + password: params[:password], + created_by_id: admin_user.id, + password_automatically_set: params[:password_automatically_set] + ) + end + end + context 'when skip_confirmation parameter is true' do let(:params) do { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass', skip_confirmation: true } @@ -159,40 +153,18 @@ describe Users::CreateService, services: true do end let(:service) { described_class.new(nil, params) } - context 'when "send_user_confirmation_email" application setting is true' do - before do - current_application_settings = double(:current_application_settings, send_user_confirmation_email: true, signup_enabled?: true) - allow(service).to receive(:current_application_settings).and_return(current_application_settings) - end - - it 'does not confirm the user' do - expect(service.execute).not_to be_confirmed - end - end - - context 'when "send_user_confirmation_email" application setting is false' do - before do - current_application_settings = double(:current_application_settings, send_user_confirmation_email: false, signup_enabled?: true) - allow(service).to receive(:current_application_settings).and_return(current_application_settings) - end - - it 'confirms the user' do - expect(service.execute).to be_confirmed - end - - it 'persists the given attributes' do - user = service.execute - user.reload - - expect(user).to have_attributes( - name: params[:name], - username: params[:username], - email: params[:email], - password: params[:password], - created_by_id: nil, - admin: false - ) - end + it 'persists the given attributes' do + user = service.execute + user.reload + + expect(user).to have_attributes( + name: params[:name], + username: params[:username], + email: params[:email], + password: params[:password], + created_by_id: nil, + admin: false + ) end end end diff --git a/spec/services/users/destroy_spec.rb b/spec/services/users/destroy_service_spec.rb index 9a28c03d968..de37a61e388 100644 --- a/spec/services/users/destroy_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -17,13 +17,28 @@ describe Users::DestroyService, services: true do expect { Namespace.with_deleted.find(user.namespace.id) }.to raise_error(ActiveRecord::RecordNotFound) end - it 'will delete the project in the near future' do - expect_any_instance_of(Projects::DestroyService).to receive(:async_execute).once + it 'will delete the project' do + expect_any_instance_of(Projects::DestroyService).to receive(:execute).once service.execute(user) end end + context 'projects in pending_delete' do + before do + project.pending_delete = true + project.save + end + + it 'destroys a project in pending_delete' do + expect_any_instance_of(Projects::DestroyService).to receive(:execute).once + + service.execute(user) + + expect { Project.find(project.id) }.to raise_error(ActiveRecord::RecordNotFound) + end + end + context "a deleted user's issues" do let(:project) { create(:project) } @@ -31,43 +46,47 @@ describe Users::DestroyService, services: true do project.add_developer(user) end - context "for an issue the user has created" do - let!(:issue) { create(:issue, project: project, author: user) } + context "for an issue the user was assigned to" do + let!(:issue) { create(:issue, project: project, assignees: [user]) } before do service.execute(user) end - it 'does not delete the issue' do + it 'does not delete issues the user is assigned to' do expect(Issue.find_by_id(issue.id)).to be_present end - it 'migrates the issue so that the "Ghost User" is the issue owner' do + it 'migrates the issue so that it is "Unassigned"' do migrated_issue = Issue.find_by_id(issue.id) - expect(migrated_issue.author).to eq(User.ghost) + expect(migrated_issue.assignees).to be_empty end + end + end - it 'blocks the user before migrating issues to the "Ghost User' do - expect(user).to be_blocked - end + context "a deleted user's merge_requests" do + let(:project) { create(:project) } + + before do + project.add_developer(user) end - context "for an issue the user was assigned to" do - let!(:issue) { create(:issue, project: project, assignee: user) } + context "for an merge request the user was assigned to" do + let!(:merge_request) { create(:merge_request, source_project: project, assignee: user) } before do service.execute(user) end - it 'does not delete issues the user is assigned to' do - expect(Issue.find_by_id(issue.id)).to be_present + it 'does not delete merge requests the user is assigned to' do + expect(MergeRequest.find_by_id(merge_request.id)).to be_present end - it 'migrates the issue so that it is "Unassigned"' do - migrated_issue = Issue.find_by_id(issue.id) + it 'migrates the merge request so that it is "Unassigned"' do + migrated_merge_request = MergeRequest.find_by_id(merge_request.id) - expect(migrated_issue.assignee).to be_nil + expect(migrated_merge_request.assignee).to be_nil end end end @@ -126,5 +145,19 @@ describe Users::DestroyService, services: true do expect(User.exists?(user.id)).to be(false) end end + + context "migrating associated records" do + it 'delegates to the `MigrateToGhostUser` service to move associated records to the ghost user' do + expect_any_instance_of(Users::MigrateToGhostUserService).to receive(:execute).once + + service.execute(user) + end + + it 'does not run `MigrateToGhostUser` if hard_delete option is given' do + expect_any_instance_of(Users::MigrateToGhostUserService).not_to receive(:execute) + + service.execute(user, hard_delete: true) + end + end end end diff --git a/spec/services/users/migrate_to_ghost_user_service_spec.rb b/spec/services/users/migrate_to_ghost_user_service_spec.rb new file mode 100644 index 00000000000..9e1edf1ac30 --- /dev/null +++ b/spec/services/users/migrate_to_ghost_user_service_spec.rb @@ -0,0 +1,82 @@ +require 'spec_helper' + +describe Users::MigrateToGhostUserService, services: true do + let!(:user) { create(:user) } + let!(:project) { create(:project) } + let(:service) { described_class.new(user) } + + context "migrating a user's associated records to the ghost user" do + context 'issues' do + include_examples "migrating a deleted user's associated records to the ghost user", Issue do + let(:created_record) { create(:issue, project: project, author: user) } + let(:assigned_record) { create(:issue, project: project, assignee: user) } + end + end + + context 'merge requests' do + include_examples "migrating a deleted user's associated records to the ghost user", MergeRequest do + let(:created_record) { create(:merge_request, source_project: project, author: user, target_branch: "first") } + let(:assigned_record) { create(:merge_request, source_project: project, assignee: user, target_branch: 'second') } + end + end + + context 'notes' do + include_examples "migrating a deleted user's associated records to the ghost user", Note do + let(:created_record) { create(:note, project: project, author: user) } + end + end + + context 'abuse reports' do + include_examples "migrating a deleted user's associated records to the ghost user", AbuseReport do + let(:created_record) { create(:abuse_report, reporter: user, user: create(:user)) } + end + end + + context 'award emoji' do + include_examples "migrating a deleted user's associated records to the ghost user", AwardEmoji do + let(:created_record) { create(:award_emoji, user: user) } + let(:author_alias) { :user } + + context "when the awardable already has an award emoji of the same name assigned to the ghost user" do + let(:awardable) { create(:issue) } + let!(:existing_award_emoji) { create(:award_emoji, user: User.ghost, name: "thumbsup", awardable: awardable) } + let!(:award_emoji) { create(:award_emoji, user: user, name: "thumbsup", awardable: awardable) } + + it "migrates the award emoji regardless" do + service.execute + + migrated_record = AwardEmoji.find_by_id(award_emoji.id) + + expect(migrated_record.user).to eq(User.ghost) + end + + it "does not leave the migrated award emoji in an invalid state" do + service.execute + + migrated_record = AwardEmoji.find_by_id(award_emoji.id) + + expect(migrated_record).to be_valid + end + end + end + end + + context "when record migration fails with a rollback exception" do + before do + expect_any_instance_of(MergeRequest::ActiveRecord_Associations_CollectionProxy) + .to receive(:update_all).and_raise(ActiveRecord::Rollback) + end + + context "for records that were already migrated" do + let!(:issue) { create(:issue, project: project, author: user) } + let!(:merge_request) { create(:merge_request, source_project: project, author: user, target_branch: "first") } + + it "reverses the migration" do + service.execute + + expect(issue.reload.author).to eq(user) + end + end + end + end +end |