Merge branch 'remove-duplicate-project-pipelines-methods' into 'master'

Remove duplicate project pipelines methods

See merge request gitlab-org/gitlab!41101

app/controllers/projects/pipelines_controller.rb

@@ -261,7 +261,7 @@ class Projects::PipelinesController < Projects::ApplicationController
   end
 
   def latest_pipeline
-    @project.latest_pipeline_for_ref(params['ref'])
+    @project.latest_pipeline(params['ref'])
             &.present(current_user: current_user)
   end
 

app/models/merge_request.rb

@@ -364,7 +364,7 @@ class MergeRequest < ApplicationRecord
     # when it is fast-forward there is no merge commit, so we must fall back to
     # either the squash commit (if the MR was squashed) or the diff head commit.
     sha = merge_commit_sha || squash_commit_sha || diff_head_sha
-    target_project.pipeline_for(target_branch, sha)
+    target_project.latest_pipeline(target_branch, sha)
   end
 
   # Pattern used to extract `!123` merge request references from text

app/models/project.rb

@@ -956,13 +956,12 @@ class Project < ApplicationRecord
     latest_successful_build_for_ref(job_name, ref) || raise(ActiveRecord::RecordNotFound.new("Couldn't find job #{job_name}"))
   end
 
-  def latest_pipeline_for_ref(ref = default_branch)
+  def latest_pipeline(ref = default_branch, sha = nil)
     ref = ref.presence || default_branch
-    sha = commit(ref)&.sha
-
+    sha ||= commit(ref)&.sha
     return unless sha
 
-    ci_pipelines.newest_first(ref: ref, sha: sha).first
+    ci_pipelines.newest_first(ref: ref, sha: sha).take
   end
 
   def merge_base_commit(first_commit_id, second_commit_id)
@@ -1671,21 +1670,6 @@ class Project < ApplicationRecord
     !namespace.share_with_group_lock
   end
 
-  def pipeline_for(ref, sha = nil, id = nil)
-    sha ||= commit(ref).try(:sha)
-    return unless sha
-
-    if id.present?
-      pipelines_for(ref, sha).find_by(id: id)
-    else
-      pipelines_for(ref, sha).take
-    end
-  end
-
-  def pipelines_for(ref, sha)
-    ci_pipelines.order(id: :desc).where(sha: sha, ref: ref)
-  end
-
   def latest_successful_pipeline_for_default_branch
     if defined?(@latest_successful_pipeline_for_default_branch)
       return @latest_successful_pipeline_for_default_branch

app/services/issues/related_branches_service.rb

@@ -24,7 +24,7 @@ module Issues
 
       return unless target
 
-      pipeline = project.pipeline_for(branch_name, target.sha)
+      pipeline = project.latest_pipeline(branch_name, target.sha)
       pipeline.detailed_status(current_user) if can?(current_user, :read_pipeline, pipeline)
     end
 

app/views/projects/buttons/_download.html.haml

@@ -17,7 +17,7 @@
         %section.border-top.pt-1.mt-1
           %h5.m-0.dropdown-bold-header= _('Download artifacts')
           - unless pipeline.latest?
-            %span.unclickable= ci_status_for_statuseable(project.pipeline_for(ref))
+            %span.unclickable= ci_status_for_statuseable(project.latest_pipeline(ref))
           %h6.m-0.dropdown-header= _('Previous Artifacts')
           %ul
             - pipeline.latest_builds_with_artifacts.each do |job|

ee/app/models/concerns/latest_pipeline_information.rb

@@ -29,7 +29,7 @@ module LatestPipelineInformation
   end
 
   def latest_default_branch_pipeline
-    strong_memoize(:pipeline) { latest_pipeline_for_ref }
+    strong_memoize(:pipeline) { latest_pipeline }
   end
 
   def auto_devops_source?

ee/app/presenters/projects/security/configuration_presenter.rb

@@ -90,7 +90,7 @@ module Projects
       end
 
       def gitlab_ci_present?
-        latest_pipeline_for_ref.try(:config_path) == Gitlab::FileDetector::PATTERNS[:gitlab_ci]
+        latest_pipeline.try(:config_path) == Gitlab::FileDetector::PATTERNS[:gitlab_ci]
       end
 
       def features

ee/app/services/ci/subscribe_bridge_service.rb

@@ -35,7 +35,7 @@ module Ci
 
     def upstream_pipeline
       strong_memoize(:upstream_pipeline) do
-        upstream_project.pipeline_for(upstream_project.default_branch)
+        upstream_project.latest_pipeline(upstream_project.default_branch)
       end
     end
   end

ee/spec/presenters/projects/security/configuration_presenter_spec.rb

@@ -190,7 +190,7 @@ RSpec.describe Projects::Security::ConfigurationPresenter do
         end
 
         it 'expects the gitlab_ci_presence to be false if the file is absent' do
-          allow_any_instance_of(described_class).to receive(:latest_pipeline_for_ref).and_return(nil)
+          allow_any_instance_of(described_class).to receive(:latest_pipeline).and_return(nil)
           expect(subject[:gitlab_ci_present]).to eq(false)
         end
       end

lib/api/ci/pipelines.rb

@@ -178,7 +178,7 @@ module API
 
         def latest_pipeline
           strong_memoize(:latest_pipeline) do
-            user_project.latest_pipeline_for_ref(params[:ref])
+            user_project.latest_pipeline(params[:ref])
           end
         end
       end

spec/models/project_spec.rb

@@ -1269,60 +1269,6 @@ RSpec.describe Project do
     end
   end
 
-  describe '#pipeline_for' do
-    let(:project) { create(:project, :repository) }
-
-    shared_examples 'giving the correct pipeline' do
-      it { is_expected.to eq(pipeline) }
-
-      context 'return latest' do
-        let!(:pipeline2) { create_pipeline(project) }
-
-        it { is_expected.to eq(pipeline2) }
-      end
-    end
-
-    context 'with a matching pipeline' do
-      let!(:pipeline) { create_pipeline(project) }
-
-      context 'with explicit sha' do
-        subject { project.pipeline_for('master', pipeline.sha) }
-
-        it_behaves_like 'giving the correct pipeline'
-
-        context 'with supplied id' do
-          let!(:other_pipeline) { create_pipeline(project) }
-
-          subject { project.pipeline_for('master', pipeline.sha, other_pipeline.id) }
-
-          it { is_expected.to eq(other_pipeline) }
-        end
-      end
-
-      context 'with implicit sha' do
-        subject { project.pipeline_for('master') }
-
-        it_behaves_like 'giving the correct pipeline'
-      end
-    end
-
-    context 'when there is no matching pipeline' do
-      subject { project.pipeline_for('master') }
-
-      it { is_expected.to be_nil }
-    end
-  end
-
-  describe '#pipelines_for' do
-    let(:project) { create(:project, :repository) }
-    let!(:pipeline) { create_pipeline(project) }
-    let!(:other_pipeline) { create_pipeline(project) }
-
-    subject { project.pipelines_for(project.default_branch, project.commit.sha) }
-
-    it { is_expected.to contain_exactly(pipeline, other_pipeline) }
-  end
-
   describe '#builds_enabled' do
     let(:project) { create(:project) }
 
@@ -2369,42 +2315,90 @@ RSpec.describe Project do
     end
   end
 
-  describe '#latest_pipeline_for_ref' do
+  describe '#latest_pipeline' do
     let(:project) { create(:project, :repository) }
     let(:second_branch) { project.repository.branches[2] }
 
     let!(:pipeline_for_default_branch) do
-      create(:ci_empty_pipeline, project: project, sha: project.commit.id,
-                                 ref: project.default_branch)
+      create(:ci_pipeline, project: project, sha: project.commit.id,
+                           ref: project.default_branch)
     end
 
     let!(:pipeline_for_second_branch) do
-      create(:ci_empty_pipeline, project: project, sha: second_branch.target,
-                                 ref: second_branch.name)
+      create(:ci_pipeline, project: project, sha: second_branch.target,
+                           ref: second_branch.name)
     end
 
-    before do
-      create(:ci_empty_pipeline, project: project, sha: project.commit.parent.id,
-                                 ref: project.default_branch)
+    let!(:other_pipeline_for_default_branch) do
+      create(:ci_pipeline, project: project, sha: project.commit.parent.id,
+                           ref: project.default_branch)
     end
 
     context 'default repository branch' do
-      subject { project.latest_pipeline_for_ref(project.default_branch) }
+      context 'when explicitly provided' do
+        subject { project.latest_pipeline(project.default_branch) }
+
+        it { is_expected.to eq(pipeline_for_default_branch) }
+      end
+
+      context 'when not provided' do
+        subject { project.latest_pipeline }
+
+        it { is_expected.to eq(pipeline_for_default_branch) }
+      end
 
-      it { is_expected.to eq(pipeline_for_default_branch) }
+      context 'with provided sha' do
+        subject { project.latest_pipeline(project.default_branch, project.commit.parent.id) }
+
+        it { is_expected.to eq(other_pipeline_for_default_branch) }
+      end
     end
 
     context 'provided ref' do
-      subject { project.latest_pipeline_for_ref(second_branch.name) }
+      subject { project.latest_pipeline(second_branch.name) }
 
       it { is_expected.to eq(pipeline_for_second_branch) }
+
+      context 'with provided sha' do
+        let!(:latest_pipeline_for_ref) do
+          create(:ci_pipeline, project: project, sha: pipeline_for_second_branch.sha,
+                               ref: pipeline_for_second_branch.ref)
+        end
+
+        subject { project.latest_pipeline(second_branch.name, second_branch.target) }
+
+        it { is_expected.to eq(latest_pipeline_for_ref) }
+      end
     end
 
     context 'bad ref' do
-      subject { project.latest_pipeline_for_ref(SecureRandom.uuid) }
+      before do
+        # ensure we don't skip the filter by ref by mistakenly return this pipeline
+        create(:ci_pipeline, project: project)
+      end
+
+      subject { project.latest_pipeline(SecureRandom.uuid) }
 
       it { is_expected.to be_nil }
     end
+
+    context 'on deleted ref' do
+      let(:branch) { project.repository.branches.last }
+
+      let!(:pipeline_on_deleted_ref) do
+        create(:ci_pipeline, project: project, sha: branch.target, ref: branch.name)
+      end
+
+      before do
+        project.repository.rm_branch(project.owner, branch.name)
+      end
+
+      subject { project.latest_pipeline(branch.name) }
+
+      it 'always returns nil despite a pipeline exists' do
+        expect(subject).to be_nil
+      end
+    end
   end
 
   describe '#latest_successful_build_for_sha' do

spec/services/issues/related_branches_service_spec.rb

@@ -57,7 +57,7 @@ RSpec.describe Issues::RelatedBranchesService do
           unreadable_branch_name => unreadable_pipeline
         }.each do |name, pipeline|
           allow(repo).to receive(:find_branch).with(name).and_return(make_branch)
-          allow(project).to receive(:pipeline_for).with(name, sha).and_return(pipeline)
+          allow(project).to receive(:latest_pipeline).with(name, sha).and_return(pipeline)
         end
 
         allow(repo).to receive(:find_branch).with(missing_branch).and_return(nil)