Refactor project.latest_successful_builds_for def

`project.latest_successful_builds_for(ref)` is being used to find a
single job all the time. This results into us having to call `find_by`
inside of the controller which violates our CodeReuse/ActiveRecord
rubocop rule.

Refactor `project.latest_successful_builds_for(ref)` to
`project.latest_successful_build_for(job_name, ref)` which will execute
the `find_by` inside of the model.

Also create `project.latest_successful_build_for!(job_name, ref)` which
raises an exception instead of returning nil.

app/controllers/projects/artifacts_controller.rb

@@ -91,14 +91,11 @@ class Projects::ArtifactsController < Projects::ApplicationController
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def build_from_ref
     return unless @ref_name
 
-    builds = project.latest_successful_builds_for(@ref_name)
-    builds.find_by(name: params[:job])
+    project.latest_successful_build_for(params[:job], @ref_name)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def artifacts_file
     @artifacts_file ||= build&.artifacts_file_for_type(params[:file_type] || :archive)

app/controllers/projects/build_artifacts_controller.rb

@@ -50,12 +50,9 @@ class Projects::BuildArtifactsController < Projects::ApplicationController
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def job_from_ref
     return unless @ref_name
 
-    jobs = project.latest_successful_builds_for(@ref_name)
-    jobs.find_by(name: params[:job])
+    project.latest_successful_build_for(params[:job], @ref_name)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 end

app/models/project.rb

@@ -644,19 +644,15 @@ class Project < ActiveRecord::Base
   end
 
   # ref can't be HEAD, can only be branch/tag name or SHA
-  def latest_successful_builds_for(ref = default_branch)
+  def latest_successful_build_for(job_name, ref = default_branch)
     latest_pipeline = ci_pipelines.latest_successful_for(ref)
+    return unless latest_pipeline
 
-    if latest_pipeline
-      latest_pipeline.builds.latest.with_artifacts_archive
-    else
-      builds.none
-    end
+    latest_pipeline.builds.latest.with_artifacts_archive.find_by(name: job_name)
   end
 
-  def latest_successful_build_for(job_name, ref = default_branch)
-    builds = latest_successful_builds_for(ref)
-    builds.find_by!(name: job_name)
+  def latest_successful_build_for!(job_name, ref = default_branch)
+    latest_successful_build_for(job_name, ref) or raise ActiveRecord::RecordNotFound.new("Couldn't find job #{job_name}", job_name)
   end
 
   def merge_base_commit(first_commit_id, second_commit_id)

lib/api/job_artifacts.rb

@@ -23,17 +23,14 @@ module API
         requires :job,      type: String, desc: 'The name for the job'
       end
       route_setting :authentication, job_token_allowed: true
-      # rubocop: disable CodeReuse/ActiveRecord
       get ':id/jobs/artifacts/:ref_name/download',
         requirements: { ref_name: /.+/ } do
         authorize_download_artifacts!
 
-        builds = user_project.latest_successful_builds_for(params[:ref_name])
-        latest_build = builds.find_by!(name: params[:job])
+        latest_build = user_project.latest_successful_build_for!(params[:job], params[:ref_name])
 
         present_carrierwave_file!(latest_build.artifacts_file)
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
       desc 'Download a specific file from artifacts archive from a ref' do
         detail 'This feature was introduced in GitLab 11.5'
@@ -48,7 +45,7 @@ module API
           requirements: { ref_name: /.+/ } do
         authorize_download_artifacts!
 
-        build = user_project.latest_successful_build_for(params[:job], params[:ref_name])
+        build = user_project.latest_successful_build_for!(params[:job], params[:ref_name])
 
         path = Gitlab::Ci::Build::Artifacts::Path
                  .new(params[:artifact_path])

spec/models/project_spec.rb

@@ -1921,7 +1921,7 @@ describe Project do
     end
   end
 
-  describe '#latest_successful_builds_for and #latest_successful_build_for' do
+  describe '#latest_successful_build_for' do
     def create_pipeline(status = 'success')
       create(:ci_pipeline, project: project,
                            sha: project.commit.sha,
@@ -1946,12 +1946,10 @@ describe Project do
         create_build(pipeline1, 'test')
         create_build(pipeline1, 'test2')
         build1_p2 = create_build(pipeline2, 'test')
-        build2_p2 = create_build(pipeline2, 'test2')
+        create_build(pipeline2, 'test2')
 
-        latest_builds = project.latest_successful_builds_for
         single_build = project.latest_successful_build_for(build1_p2.name)
 
-        expect(latest_builds).to contain_exactly(build2_p2, build1_p2)
         expect(single_build).to eq(build1_p2)
       end
     end
@@ -1961,22 +1959,19 @@ describe Project do
 
       context 'standalone pipeline' do
         it 'returns builds for ref for default_branch' do
-          builds = project.latest_successful_builds_for
           single_build = project.latest_successful_build_for(build.name)
 
-          expect(builds).to contain_exactly(build)
           expect(single_build).to eq(build)
         end
 
-        it 'returns empty relation if the build cannot be found for #latest_successful_builds_for' do
-          builds = project.latest_successful_builds_for('TAIL')
+        it 'returns empty relation if the build cannot be found for #latest_successful_build_for' do
+          builds = project.latest_successful_build_for('TAIL')
 
-          expect(builds).to be_kind_of(ActiveRecord::Relation)
-          expect(builds).to be_empty
+          expect(builds).to be_nil
         end
 
-        it 'returns exception if the build cannot be found for #latest_successful_build_for' do
-          expect { project.latest_successful_build_for(build.name, 'TAIL') }.to raise_error(ActiveRecord::RecordNotFound)
+        it 'returns exception if the build cannot be found for #latest_successful_build_for!' do
+          expect { project.latest_successful_build_for!(build.name, 'TAIL') }.to raise_error(ActiveRecord::RecordNotFound)
         end
       end
 
@@ -1986,26 +1981,21 @@ describe Project do
         end
 
         it 'gives the latest build from latest pipeline' do
-          latest_builds = project.latest_successful_builds_for
           last_single_build = project.latest_successful_build_for(build.name)
 
-          expect(latest_builds).to contain_exactly(build)
           expect(last_single_build).to eq(build)
         end
       end
     end
 
     context 'with pending pipeline' do
-      before do
+      it 'returns empty relation' do
         pipeline.update(status: 'pending')
-        create_build(pipeline)
-      end
+        pending_build = create_build(pipeline)
 
-      it 'returns empty relation' do
-        builds = project.latest_successful_builds_for
+        expect(project.latest_successful_build_for(pending_build.name)).to be_nil
 
-        expect(builds).to be_kind_of(ActiveRecord::Relation)
-        expect(builds).to be_empty
+        expect { project.latest_successful_build_for!(pending_build.name) }.to raise_error(ActiveRecord::RecordNotFound)
       end
     end
   end