diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index d91dbe4bf4923fab8c75a0c3ad093999ea38ba41..49c56ecafd31dc75f681379cb71bb65f59429a19 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1097,6 +1097,16 @@ class MergeRequest < ApplicationRecord @environments[current_user] end + ## + # This method is for looking for active environments which created via pipelines for merge requests. + # Since deployments run on a merge request ref (e.g. `refs/merge-requests/:iid/head`), + # we cannot look up environments with source branch name. + def environments + return Environment.none unless actual_head_pipeline&.triggered_by_merge_request? + + actual_head_pipeline.environments + end + def state_human_name if merged? "Merged" diff --git a/app/presenters/ci/pipeline_presenter.rb b/app/presenters/ci/pipeline_presenter.rb index 1c1347c5a570b01545b4b069bb26173ab58c5d81..944895904fe4b13767f3674ca2ac34fec3c54277 100644 --- a/app/presenters/ci/pipeline_presenter.rb +++ b/app/presenters/ci/pipeline_presenter.rb @@ -63,19 +63,11 @@ module Ci end def link_to_merge_request_source_branch - return unless merge_request_presenter - - link_to(merge_request_presenter.source_branch, - merge_request_presenter.source_branch_commits_path, - class: 'ref-name') + merge_request_presenter&.source_branch_link end def link_to_merge_request_target_branch - return unless merge_request_presenter - - link_to(merge_request_presenter.target_branch, - merge_request_presenter.target_branch_commits_path, - class: 'ref-name') + merge_request_presenter&.target_branch_link end private diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index 55103c0a95cbdfe117376e3404ba8c8181bd21b6..99ea7849f0bba77744248074b16f7174cc2450b0 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -213,6 +213,22 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated help_page_path('ci/merge_request_pipelines/index.md') end + def source_branch_link + if source_branch_exists? + link_to(source_branch, source_branch_commits_path, class: 'ref-name') + else + content_tag(:span, source_branch, class: 'ref-name') + end + end + + def target_branch_link + if target_branch_exists? + link_to(target_branch, target_branch_commits_path, class: 'ref-name') + else + content_tag(:span, target_branch, class: 'ref-name') + end + end + private def cached_can_be_reverted? diff --git a/app/services/ci/stop_environments_service.rb b/app/services/ci/stop_environments_service.rb index 973ae5ce5aab15c29e5cc5458f6f9abcebd2d7bf..d9a800791f2a305311ec436d2abef9b53cdaebff 100644 --- a/app/services/ci/stop_environments_service.rb +++ b/app/services/ci/stop_environments_service.rb @@ -9,12 +9,11 @@ module Ci return unless @ref.present? - environments.each do |environment| - next unless environment.stop_action_available? - next unless can?(current_user, :stop_environment, environment) + environments.each { |environment| stop(environment) } + end - environment.stop_with_action!(current_user) - end + def execute_for_merge_request(merge_request) + merge_request.environments.each { |environment| stop(environment) } end private @@ -24,5 +23,12 @@ module Ci .new(project, current_user, ref: @ref, recently_updated: true) .execute end + + def stop(environment) + return unless environment.stop_action_available? + return unless can?(current_user, :stop_environment, environment) + + environment.stop_with_action!(current_user) + end end end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 8a9e5ebb014ef166c2ce3ad0d2de9b2395ae82f0..efb08b5ee1d5e4695a0260d219133c929b46b5cd 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -24,6 +24,11 @@ module MergeRequests end end + def cleanup_environments(merge_request) + Ci::StopEnvironmentsService.new(merge_request.source_project, current_user) + .execute_for_merge_request(merge_request) + end + private def handle_wip_event(merge_request) diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index 04527bb9713a4a5879e5fc0097f3c6e97f36be13..e77051bb1c9878aa57be13a45e67f09fc0f4093e 100644 --- a/app/services/merge_requests/close_service.rb +++ b/app/services/merge_requests/close_service.rb @@ -17,6 +17,7 @@ module MergeRequests execute_hooks(merge_request, 'close') invalidate_cache_counts(merge_request, users: merge_request.assignees) merge_request.update_project_counter_caches + cleanup_environments(merge_request) end merge_request diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index f26e3bee06f68cb35875064108c32769dda98715..c13f7dd508821848d521fee0e46c7717316983bd 100644 --- a/app/services/merge_requests/post_merge_service.rb +++ b/app/services/merge_requests/post_merge_service.rb @@ -18,6 +18,7 @@ module MergeRequests invalidate_cache_counts(merge_request, users: merge_request.assignees) merge_request.update_project_counter_caches delete_non_latest_diffs(merge_request) + cleanup_environments(merge_request) end private diff --git a/changelogs/unreleased/fix-environment-on-stop-not-work.yml b/changelogs/unreleased/fix-environment-on-stop-not-work.yml new file mode 100644 index 0000000000000000000000000000000000000000..72e58b26c4dffac404582ad983e546bc52900454 --- /dev/null +++ b/changelogs/unreleased/fix-environment-on-stop-not-work.yml @@ -0,0 +1,5 @@ +--- +title: "`on_stop` is not automatically triggered with pipelines for merge requests" +merge_request: 27618 +author: +type: fixed diff --git a/changelogs/unreleased/fix-ref-text-of-mr-pipelines.yml b/changelogs/unreleased/fix-ref-text-of-mr-pipelines.yml new file mode 100644 index 0000000000000000000000000000000000000000..8803f9b52a46f6b7a43b3a834ce1363e730adc18 --- /dev/null +++ b/changelogs/unreleased/fix-ref-text-of-mr-pipelines.yml @@ -0,0 +1,6 @@ +--- +title: Fix pipelines for merge requests does not show pipeline page when source branch + is removed +merge_request: 27803 +author: +type: fixed diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb index cf334e1e4da9de43b5179310b74c502d87648630..4ec44cb05b3ac546d683541058011991843034d8 100644 --- a/spec/features/projects/pipelines/pipeline_spec.rb +++ b/spec/features/projects/pipelines/pipeline_spec.rb @@ -331,11 +331,9 @@ describe 'Pipeline', :js do merge_request.all_pipelines.last end - before do + it 'shows the pipeline information' do visit_pipeline - end - it 'shows the pipeline information' do within '.pipeline-info' do expect(page).to have_content("#{pipeline.statuses.count} jobs " \ "for !#{merge_request.iid} " \ @@ -347,6 +345,21 @@ describe 'Pipeline', :js do end end + context 'when source branch does not exist' do + before do + project.repository.rm_branch(user, merge_request.source_branch) + end + + it 'does not link to the source branch commit path' do + visit_pipeline + + within '.pipeline-info' do + expect(page).not_to have_link(merge_request.source_branch) + expect(page).to have_content(merge_request.source_branch) + end + end + end + context 'when source project is a forked project' do let(:source_project) { fork_project(project, user, repository: true) } @@ -386,11 +399,11 @@ describe 'Pipeline', :js do before do pipeline.update(user: user) - - visit_pipeline end it 'shows the pipeline information' do + visit_pipeline + within '.pipeline-info' do expect(page).to have_content("#{pipeline.statuses.count} jobs " \ "for !#{merge_request.iid} " \ @@ -405,6 +418,21 @@ describe 'Pipeline', :js do end end + context 'when target branch does not exist' do + before do + project.repository.rm_branch(user, merge_request.target_branch) + end + + it 'does not link to the target branch commit path' do + visit_pipeline + + within '.pipeline-info' do + expect(page).not_to have_link(merge_request.target_branch) + expect(page).to have_content(merge_request.target_branch) + end + end + end + context 'when source project is a forked project' do let(:source_project) { fork_project(project, user, repository: true) } diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 6f34ef9c1bcf89e66fa6e1631a5bd0b34e66d18c..191fa688f28d44d94bdf302cb48624dd195c4ae9 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -2304,6 +2304,50 @@ describe MergeRequest do end end + describe "#environments" do + subject { merge_request.environments } + + let(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'master') } + let(:project) { merge_request.project } + + let(:pipeline) do + create(:ci_pipeline, + source: :merge_request_event, + merge_request: merge_request, project: project, + sha: merge_request.diff_head_sha, + merge_requests_as_head_pipeline: [merge_request]) + end + + let!(:job) { create(:ci_build, :start_review_app, pipeline: pipeline, project: project) } + + it 'returns environments' do + is_expected.to eq(pipeline.environments) + expect(subject.count).to be(1) + end + + context 'when pipeline is not associated with environments' do + let!(:job) { create(:ci_build, pipeline: pipeline, project: project) } + + it 'returns empty array' do + is_expected.to be_empty + end + end + + context 'when pipeline is not a pipeline for merge request' do + let(:pipeline) do + create(:ci_pipeline, + project: project, + ref: 'feature', + sha: merge_request.diff_head_sha, + merge_requests_as_head_pipeline: [merge_request]) + end + + it 'returns empty relation' do + is_expected.to be_empty + end + end + end + describe "#reload_diff" do it 'calls MergeRequests::ReloadDiffsService#execute with correct params' do user = create(:user) diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb index 4a0f91c4c7a8cac0023a57112018717259c00684..2d6b63cff85956ff77d1a54c9e39f1d3bca544b3 100644 --- a/spec/presenters/merge_request_presenter_spec.rb +++ b/spec/presenters/merge_request_presenter_spec.rb @@ -439,6 +439,52 @@ describe MergeRequestPresenter do end end + describe '#source_branch_link' do + subject { presenter.source_branch_link } + + let(:presenter) { described_class.new(resource, current_user: user) } + + context 'when source branch exists' do + it 'returns link' do + allow(resource).to receive(:source_branch_exists?) { true } + + is_expected + .to eq("#{presenter.source_branch}") + end + end + + context 'when source branch does not exist' do + it 'returns text' do + allow(resource).to receive(:source_branch_exists?) { false } + + is_expected.to eq("#{presenter.source_branch}") + end + end + end + + describe '#target_branch_link' do + subject { presenter.target_branch_link } + + let(:presenter) { described_class.new(resource, current_user: user) } + + context 'when target branch exists' do + it 'returns link' do + allow(resource).to receive(:target_branch_exists?) { true } + + is_expected + .to eq("#{presenter.target_branch}") + end + end + + context 'when target branch does not exist' do + it 'returns text' do + allow(resource).to receive(:target_branch_exists?) { false } + + is_expected.to eq("#{presenter.target_branch}") + end + end + end + describe '#source_branch_with_namespace_link' do subject do described_class.new(resource, current_user: user).source_branch_with_namespace_link diff --git a/spec/services/ci/stop_environments_service_spec.rb b/spec/services/ci/stop_environments_service_spec.rb index cdd3d851f61e30dd2f1ed4069e3da5a0f20b37dd..002a8f6da8e44f3c1433d324eb8a501fea8e53f7 100644 --- a/spec/services/ci/stop_environments_service_spec.rb +++ b/spec/services/ci/stop_environments_service_spec.rb @@ -103,6 +103,82 @@ describe Ci::StopEnvironmentsService do end end + describe '#execute_for_merge_request' do + subject { service.execute_for_merge_request(merge_request) } + + let(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'master') } + let(:project) { merge_request.project } + let(:user) { create(:user) } + + let(:pipeline) do + create(:ci_pipeline, + source: :merge_request_event, + merge_request: merge_request, + project: project, + sha: merge_request.diff_head_sha, + merge_requests_as_head_pipeline: [merge_request]) + end + + let!(:review_job) { create(:ci_build, :start_review_app, pipeline: pipeline, project: project) } + let!(:stop_review_job) { create(:ci_build, :stop_review_app, :manual, pipeline: pipeline, project: project) } + + before do + review_job.deployment.success! + end + + it 'has active environment at first' do + expect(pipeline.environments.first).to be_available + end + + context 'when user is a developer' do + before do + project.add_developer(user) + end + + it 'stops the active environment' do + subject + + expect(pipeline.environments.first).to be_stopped + end + end + + context 'when user is a reporter' do + before do + project.add_reporter(user) + end + + it 'does not stop the active environment' do + subject + + expect(pipeline.environments.first).to be_available + end + end + + context 'when pipeline is not associated with environments' do + let!(:job) { create(:ci_build, pipeline: pipeline, project: project) } + + it 'does not raise exception' do + expect { subject }.not_to raise_exception + end + end + + context 'when pipeline is not a pipeline for merge request' do + let(:pipeline) do + create(:ci_pipeline, + project: project, + ref: 'feature', + sha: merge_request.diff_head_sha, + merge_requests_as_head_pipeline: [merge_request]) + end + + it 'does not stop the active environment' do + subject + + expect(pipeline.environments.first).to be_available + end + end + end + def expect_environment_stopped_on(branch) expect_any_instance_of(Environment) .to receive(:stop!) diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 433ffbd97f015845ddb286fac6908f095e27ec56..93f5b092cab5a628769e6bb7c75df364c4077cc7 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -72,6 +72,14 @@ describe MergeRequests::CloseService do .to change { project.open_merge_requests_count }.from(1).to(0) end + it 'clean up environments for the merge request' do + expect_next_instance_of(Ci::StopEnvironmentsService) do |service| + expect(service).to receive(:execute_for_merge_request).with(merge_request) + end + + described_class.new(project, user).execute(merge_request) + end + context 'current user is not authorized to close merge request' do before do perform_enqueued_jobs do diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index 5ad6f5528f97331083a87c63469af40719c6583f..0fbcd0efb86de4c75bc2e9d8bbb04c9a1e64d0c6 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -60,5 +60,13 @@ describe MergeRequests::PostMergeService do expect(merge_request.reload).to be_merged end + + it 'clean up environments for the merge request' do + expect_next_instance_of(Ci::StopEnvironmentsService) do |service| + expect(service).to receive(:execute_for_merge_request).with(merge_request) + end + + described_class.new(project, user).execute(merge_request) + end end end