Refactors the finder

* Adds a unit test for finder
* Fixes some spacing
* Refactors specs
* Makes comment on finder more clear

app/finders/ci/pipeline_jobs_finder.rb

 # frozen_string_literal: true
+
 module Ci
+  # By default this class returns builds for a specific pipeline. If you'd like bridges, use params[:type] bridges. It will never return both.
   class PipelineJobsFinder
     include Gitlab::Allowable
 
@@ -39,15 +41,15 @@ module Ci
       @pipeline ||= project.all_pipelines.find(params[:pipeline_id])
     end
 
-    def filter_by_scope(records, scope)
-      return records unless scope.present?
+    def filter_by_scope(jobs, scope)
+      return jobs unless scope.present?
 
       available_statuses = ::CommitStatus::AVAILABLE_STATUSES
 
       unknown = scope - available_statuses
       raise ArgumentError, 'Scope contains invalid value(s)' unless unknown.empty?
 
-      records.where(status: scope) # rubocop: disable CodeReuse/ActiveRecord
+      jobs.where(status: scope) # rubocop: disable CodeReuse/ActiveRecord
     end
   end
 end

lib/api/ci/pipelines.rb

@@ -24,8 +24,6 @@ module API
                              case scope
                              when String
                                [scope]
-                             when ::Hash
-                               scope.values
                              when ::Array
                                scope
                              else

spec/finders/ci/pipeline_jobs_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::PipelineJobsFinder, '#execute' do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:pipeline) { create(:ci_pipeline, project: project, user: user) }
+  let_it_be(:job) { create(:ci_build, pipeline: pipeline) }
+  let_it_be(:bridge) { create(:ci_bridge, pipeline: pipeline) }
+
+  let(:params) { { pipeline_id: pipeline.id } }
+
+  subject { described_class.new(user, project, params).execute }
+
+  context 'when user is not authorized' do
+    it 'returns an AccessDenied error' do
+      expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
+    end
+  end
+
+  context 'when user is authorized' do
+    before do
+      project.add_maintainer(user)
+    end
+
+    context 'when not given a type parameter' do
+      it 'returns the builds for a pipeline' do
+        expect(subject).to match_array([job])
+      end
+    end
+
+    context 'when given bridges as a type parameter' do
+      let(:params) { { pipeline_id: pipeline.id, type: :bridges } }
+
+      it 'returns bridges' do
+        expect(subject).to match_array([bridge])
+      end
+    end
+
+    context 'when given a scope filter' do
+      let(:params) { { pipeline_id: pipeline.id, scope: ['running'] } }
+      let(:job_2) { create(:ci_build, :running, pipeline: pipeline) }
+
+      it 'filters by the job statuses in the scope' do
+        expect(subject).to match_array([job_2])
+      end
+    end
+  end
+end

spec/requests/api/ci/pipelines_spec.rb

@@ -310,7 +310,7 @@ RSpec.describe API::Ci::Pipelines do
   describe 'GET /projects/:id/pipelines/:pipeline_id/jobs' do
     let(:query) { {} }
     let(:api_user) { user }
-    let!(:job) do
+    let_it_be(:job) do
       create(:ci_build, :success, pipeline: pipeline,
                                   artifacts_expire_at: 1.day.since)
     end
@@ -319,7 +319,6 @@ RSpec.describe API::Ci::Pipelines do
 
     before do |example|
       unless example.metadata[:skip_before_request]
-        job
         project.update!(public_builds: false)
         get api("/projects/#{project.id}/pipelines/#{pipeline.id}/jobs", api_user), params: query
       end
@@ -364,6 +363,12 @@ RSpec.describe API::Ci::Pipelines do
         end
       end
 
+      context 'filter jobs with hash' do
+        let(:query) { { scope: { hello: 'pending', world: 'running' } } }
+
+        it { expect(response).to have_gitlab_http_status(:bad_request) }
+      end
+
       context 'filter jobs with array of scope elements' do
         let(:query) { { scope: %w(pending running) } }
 
@@ -480,7 +485,7 @@ RSpec.describe API::Ci::Pipelines do
       end
 
       context 'filter bridges' do
-        before do
+        before_all do
           create_bridge(pipeline, :pending)
           create_bridge(pipeline, :running)
         end
@@ -513,9 +518,23 @@ RSpec.describe API::Ci::Pipelines do
       end
 
       context 'respond 400 when scope contains invalid state' do
-        let(:query) { { scope: %w(unknown running) } }
+        context 'in an array' do
+          let(:query) { { scope: %w(unknown running) } }
 
-        it { expect(response).to have_gitlab_http_status(:bad_request) }
+          it { expect(response).to have_gitlab_http_status(:bad_request) }
+        end
+
+        context 'in a hash' do
+          let(:query) { { scope: { unknown: true } } }
+
+          it { expect(response).to have_gitlab_http_status(:bad_request) }
+        end
+
+        context 'in a string' do
+          let(:query) { { scope: "unknown" } }
+
+          it { expect(response).to have_gitlab_http_status(:bad_request) }
+        end
       end
 
       context 'bridges in different pipelines' do

spec/support/shared_examples/ci/jobs_shared_examples.rb

 # frozen_string_literal: true
 
 RSpec.shared_examples 'a job with artifacts and trace' do |result_is_array: true|
-    context 'with artifacts and trace' do
-      let!(:second_job) { create(:ci_build, :trace_artifact, :artifacts, :test_reports, pipeline: pipeline) }
+  context 'with artifacts and trace' do
+    let!(:second_job) { create(:ci_build, :trace_artifact, :artifacts, :test_reports, pipeline: pipeline) }
 
-      it 'returns artifacts and trace data', :skip_before_request do
-        get api(api_endpoint, api_user)
-        json_job = json_response.is_a?(Array) ? json_response.find { |job| job['id'] == second_job.id } : json_response
+    it 'returns artifacts and trace data', :skip_before_request do
+      get api(api_endpoint, api_user)
+      json_job = json_response.is_a?(Array) ? json_response.find { |job| job['id'] == second_job.id } : json_response
 
-        expect(json_job['artifacts_file']).not_to be_nil
-        expect(json_job['artifacts_file']).not_to be_empty
-        expect(json_job['artifacts_file']['filename']).to eq(second_job.artifacts_file.filename)
-        expect(json_job['artifacts_file']['size']).to eq(second_job.artifacts_file.size)
-        expect(json_job['artifacts']).not_to be_nil
-        expect(json_job['artifacts']).to be_an Array
-        expect(json_job['artifacts'].size).to eq(second_job.job_artifacts.length)
-        json_job['artifacts'].each do |artifact|
-          expect(artifact).not_to be_nil
-          file_type = Ci::JobArtifact.file_types[artifact['file_type']]
-          expect(artifact['size']).to eq(second_job.job_artifacts.find_by(file_type: file_type).size)
-          expect(artifact['filename']).to eq(second_job.job_artifacts.find_by(file_type: file_type).filename)
-          expect(artifact['file_format']).to eq(second_job.job_artifacts.find_by(file_type: file_type).file_format)
-        end
+      expect(json_job['artifacts_file']).not_to be_nil
+      expect(json_job['artifacts_file']).not_to be_empty
+      expect(json_job['artifacts_file']['filename']).to eq(second_job.artifacts_file.filename)
+      expect(json_job['artifacts_file']['size']).to eq(second_job.artifacts_file.size)
+      expect(json_job['artifacts']).not_to be_nil
+      expect(json_job['artifacts']).to be_an Array
+      expect(json_job['artifacts'].size).to eq(second_job.job_artifacts.length)
+      json_job['artifacts'].each do |artifact|
+        expect(artifact).not_to be_nil
+        file_type = Ci::JobArtifact.file_types[artifact['file_type']]
+        expect(artifact['size']).to eq(second_job.job_artifacts.find_by(file_type: file_type).size)
+        expect(artifact['filename']).to eq(second_job.job_artifacts.find_by(file_type: file_type).filename)
+        expect(artifact['file_format']).to eq(second_job.job_artifacts.find_by(file_type: file_type).file_format)
       end
     end
   end
+end