From ecefe090460687a078e3d1aacf621fd5bff07fb5 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 31 Aug 2018 16:58:13 +0100 Subject: [PATCH 1/5] Render link to branch only when branch still exists --- app/views/projects/pipelines/_info.html.haml | 6 +++++- .../unreleased/42611-removed-branch-link.yml | 5 +++++ .../projects/pipelines/pipeline_spec.rb | 17 +++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/42611-removed-branch-link.yml diff --git a/app/views/projects/pipelines/_info.html.haml b/app/views/projects/pipelines/_info.html.haml index ccb83148ded..57c5f64ee8d 100644 --- a/app/views/projects/pipelines/_info.html.haml +++ b/app/views/projects/pipelines/_info.html.haml @@ -13,7 +13,11 @@ = pluralize @pipeline.total_size, "job" - if @pipeline.ref from - = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name" + - if @project.repository.branch_exists?(@pipeline.ref) + = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name" + - else + %span.ref-name + = @pipeline.ref - if @pipeline.duration in = time_interval_in_words(@pipeline.duration) diff --git a/changelogs/unreleased/42611-removed-branch-link.yml b/changelogs/unreleased/42611-removed-branch-link.yml new file mode 100644 index 00000000000..03a206871b4 --- /dev/null +++ b/changelogs/unreleased/42611-removed-branch-link.yml @@ -0,0 +1,5 @@ +--- +title: Only render link to branch when branch still exists in pipeline page +merge_request: +author: +type: fixed diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb index 603503a531c..4d659cb988e 100644 --- a/spec/features/projects/pipelines/pipeline_spec.rb +++ b/spec/features/projects/pipelines/pipeline_spec.rb @@ -63,6 +63,11 @@ describe 'Pipeline', :js do expect(page).to have_css('#js-tab-pipeline.active') end + it 'shows link to the pipeline ref' do + expect(page).to have_link(pipeline.ref) + end + + it_behaves_like 'showing user status' do let(:user_with_status) { pipeline.user } @@ -208,6 +213,18 @@ describe 'Pipeline', :js do it { expect(page).not_to have_content('Cancel running') } end end + + context 'with deleted branch' do + before do + DeleteBranchService.new(@project, @user).execute(pipeline.ref) + end + + it 'does not render link to the pipeline ref' do + expect(page).not_to have_link(pipeline.ref) + expect(page).to have_content(pipeline.ref) + end + end + end context 'when user does not have access to read jobs' do From 0d583e5e8a36231eef614305208ea67ab91a5b62 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Mon, 8 Oct 2018 16:55:13 +0100 Subject: [PATCH 2/5] Creates ref_exists? method for Pipeline class --- app/models/ci/pipeline.rb | 4 ++++ app/views/projects/pipelines/_info.html.haml | 2 +- spec/features/projects/pipelines/pipeline_spec.rb | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 17024e8a0af..c53b14bd406 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -268,6 +268,10 @@ module Ci stage unless stage.statuses_count.zero? end + def ref_exists? + project.repository.ref_exists?(self.ref) + end + ## # TODO We do not completely switch to persisted stages because of # race conditions with setting statuses gitlab-ce#23257. diff --git a/app/views/projects/pipelines/_info.html.haml b/app/views/projects/pipelines/_info.html.haml index 57c5f64ee8d..ba7f542f68e 100644 --- a/app/views/projects/pipelines/_info.html.haml +++ b/app/views/projects/pipelines/_info.html.haml @@ -13,7 +13,7 @@ = pluralize @pipeline.total_size, "job" - if @pipeline.ref from - - if @project.repository.branch_exists?(@pipeline.ref) + - if @pipeline.ref_exists? = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name" - else %span.ref-name diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb index 8cdefebd4ce..a79dbe7f877 100644 --- a/spec/features/projects/pipelines/pipeline_spec.rb +++ b/spec/features/projects/pipelines/pipeline_spec.rb @@ -244,7 +244,7 @@ describe 'Pipeline', :js do context 'with deleted branch' do before do - DeleteBranchService.new(@project, @user).execute(pipeline.ref) + allow(pipeline).to receive(:ref_exists?).and_return(false) end it 'does not render link to the pipeline ref' do From 49d7c3b3461442a60d06a26c8cc971205afa3642 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Wed, 17 Oct 2018 15:16:28 +0100 Subject: [PATCH 3/5] Removes extra empty lines --- spec/features/projects/pipelines/pipeline_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb index a79dbe7f877..5734c7e355e 100644 --- a/spec/features/projects/pipelines/pipeline_spec.rb +++ b/spec/features/projects/pipelines/pipeline_spec.rb @@ -72,7 +72,6 @@ describe 'Pipeline', :js do expect(page).to have_link(pipeline.ref) end - it_behaves_like 'showing user status' do let(:user_with_status) { pipeline.user } @@ -252,7 +251,6 @@ describe 'Pipeline', :js do expect(page).to have_content(pipeline.ref) end end - end context 'when user does not have access to read jobs' do From 5f412e3a87d1e9444bbef6475a2cd3304f541f7d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 22 Oct 2018 11:37:52 +0200 Subject: [PATCH 4/5] Fix pipeline reference existence check and add specs --- app/models/ci/pipeline.rb | 6 +++--- spec/models/ci/pipeline_spec.rb | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index c53b14bd406..c93f0e0cd55 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -269,7 +269,7 @@ module Ci end def ref_exists? - project.repository.ref_exists?(self.ref) + project.repository.ref_exists?(git_ref) end ## @@ -678,11 +678,11 @@ module Ci def push_details strong_memoize(:push_details) do - Gitlab::Git::Push.new(project, before_sha, sha, push_ref) + Gitlab::Git::Push.new(project, before_sha, sha, git_ref) end end - def push_ref + def git_ref if branch? Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s elsif tag? diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 3b01b39ecab..06f000a7118 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -779,6 +779,29 @@ describe Ci::Pipeline, :mailer do end end + describe 'ref_exists?' do + using RSpec::Parameterized::TableSyntax + + let(:project) { create(:project, :repository) } + + where(:tag, :ref, :result) do + false | 'master' | true + false | 'non-existent-branch' | false + true | 'v1.1.0' | true + true | 'non-existent-tag' | false + end + + with_them do + let(:pipeline) do + create(:ci_empty_pipeline, project: project, tag: tag, ref: ref) + end + + it "correctly detects ref" do + expect(pipeline.ref_exists?).to be result + end + end + end + context 'with non-empty project' do let(:project) { create(:project, :repository) } From 680afb3d77db2f90b1c79d3917ce5d2df187c68b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 22 Oct 2018 13:33:28 +0200 Subject: [PATCH 5/5] Do not raise error when checking pipeline reference Return from the `Ci::Pipeline#ref_exists?` in case when there is no repository present. This also fixes pipeline page feature specs by changing pipeline reference instead of stubbing `ref_exist?` method. --- app/models/ci/pipeline.rb | 2 + .../projects/pipelines/pipeline_spec.rb | 9 +++-- spec/models/ci/pipeline_spec.rb | 40 ++++++++++++------- 3 files changed, 34 insertions(+), 17 deletions(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index c93f0e0cd55..aeee7f0a5d2 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -270,6 +270,8 @@ module Ci def ref_exists? project.repository.ref_exists?(git_ref) + rescue Gitlab::Git::Repository::NoRepository + false end ## diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb index 5734c7e355e..cd6c37bf54d 100644 --- a/spec/features/projects/pipelines/pipeline_spec.rb +++ b/spec/features/projects/pipelines/pipeline_spec.rb @@ -241,9 +241,12 @@ describe 'Pipeline', :js do end end - context 'with deleted branch' do - before do - allow(pipeline).to receive(:ref_exists?).and_return(false) + context 'when pipeline ref does not exist in repository anymore' do + let(:pipeline) do + create(:ci_empty_pipeline, project: project, + ref: 'non-existent', + sha: project.commit.id, + user: user) end it 'does not render link to the pipeline ref' do diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 06f000a7118..153244b2159 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -780,24 +780,36 @@ describe Ci::Pipeline, :mailer do end describe 'ref_exists?' do - using RSpec::Parameterized::TableSyntax + context 'when repository exists' do + using RSpec::Parameterized::TableSyntax - let(:project) { create(:project, :repository) } + let(:project) { create(:project, :repository) } - where(:tag, :ref, :result) do - false | 'master' | true - false | 'non-existent-branch' | false - true | 'v1.1.0' | true - true | 'non-existent-tag' | false - end - - with_them do - let(:pipeline) do - create(:ci_empty_pipeline, project: project, tag: tag, ref: ref) + where(:tag, :ref, :result) do + false | 'master' | true + false | 'non-existent-branch' | false + true | 'v1.1.0' | true + true | 'non-existent-tag' | false end - it "correctly detects ref" do - expect(pipeline.ref_exists?).to be result + with_them do + let(:pipeline) do + create(:ci_empty_pipeline, project: project, tag: tag, ref: ref) + end + + it "correctly detects ref" do + expect(pipeline.ref_exists?).to be result + end + end + end + + context 'when repository does not exist' do + let(:pipeline) do + create(:ci_empty_pipeline, project: project, ref: 'master') + end + + it 'always returns false' do + expect(pipeline.ref_exists?).to eq false end end end