Merge branch '330123-change-pipelines-default-per-page-value' into 'master'

Change the number of pipelines per page to 15

See merge request gitlab-org/gitlab!64285

app/controllers/projects/commit_controller.rb

@@ -52,7 +52,8 @@ class Projects::CommitController < Projects::ApplicationController
   # rubocop: disable CodeReuse/ActiveRecord
   def pipelines
     @pipelines = @commit.pipelines.order(id: :desc)
-    @pipelines = @pipelines.where(ref: params[:ref]).page(params[:page]).per(30) if params[:ref]
+    @pipelines = @pipelines.where(ref: params[:ref]) if params[:ref]
+    @pipelines = @pipelines.page(params[:page])
 
     respond_to do |format|
       format.html

app/controllers/projects/merge_requests_controller.rb

@@ -167,7 +167,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
 
   def pipelines
     set_pipeline_variables
-    @pipelines = @pipelines.page(params[:page]).per(30)
+    @pipelines = @pipelines.page(params[:page])
 
     Gitlab::PollingInterval.set_header(response, interval: 10_000)
 

app/controllers/projects/pipelines_controller.rb

@@ -42,7 +42,6 @@ class Projects::PipelinesController < Projects::ApplicationController
       .new(project, current_user, index_params)
       .execute
       .page(params[:page])
-      .per(20)
 
     @pipelines_count = limited_pipelines_count(project)
 

app/models/ci/pipeline.rb

@@ -29,6 +29,8 @@ module Ci
 
     BridgeStatusError = Class.new(StandardError)
 
+    paginates_per 15
+
     sha_attribute :source_sha
     sha_attribute :target_sha
 

lib/gitlab/pagination/offset_pagination.rb

@@ -19,11 +19,10 @@ module Gitlab
       private
 
       def paginate_with_limit_optimization(relation)
-        # do not paginate relation if it is already paginated
-        pagination_data = if relation.respond_to?(:current_page) && relation.current_page == params[:page] && relation.limit_value == params[:per_page]
-                            relation
-                          else
+        pagination_data = if needs_pagination?(relation)
                             relation.page(params[:page]).per(params[:per_page])
+                          else
+                            relation
                           end
 
         return pagination_data unless pagination_data.is_a?(ActiveRecord::Relation)
@@ -39,6 +38,14 @@ module Gitlab
         end
       end
 
+      def needs_pagination?(relation)
+        return true unless relation.respond_to?(:current_page)
+        return true if params[:page].present? && relation.current_page != params[:page].to_i
+        return true if params[:per_page].present? && relation.limit_value != params[:per_page].to_i
+
+        false
+      end
+
       def add_default_order(relation)
         if relation.is_a?(ActiveRecord::Relation) && relation.order_values.empty?
           relation = relation.order(:id) # rubocop: disable CodeReuse/ActiveRecord

spec/controllers/projects/commit_controller_spec.rb

@@ -483,7 +483,7 @@ RSpec.describe Projects::CommitController do
         end
 
         context 'when rendering a JSON format' do
-          it 'responds with serialized pipelines' do
+          it 'responds with serialized pipelines', :aggregate_failures do
             get_pipelines(id: commit.id, format: :json)
 
             expect(response).to be_ok
@@ -491,6 +491,26 @@ RSpec.describe Projects::CommitController do
             expect(json_response['count']['all']).to eq 1
             expect(response).to include_pagination_headers
           end
+
+          context 'with pagination' do
+            let!(:extra_pipeline) { create(:ci_pipeline, project: project, ref: project.default_branch, sha: commit.sha, status: :running) }
+
+            it 'paginates the result when ref is blank' do
+              allow(Ci::Pipeline).to receive(:default_per_page).and_return(1)
+
+              get_pipelines(id: commit.id, format: :json)
+
+              expect(json_response['pipelines'].count).to eq(1)
+            end
+
+            it 'paginates the result when ref is present' do
+              allow(Ci::Pipeline).to receive(:default_per_page).and_return(1)
+
+              get_pipelines(id: commit.id, ref: project.default_branch, format: :json)
+
+              expect(json_response['pipelines'].count).to eq(1)
+            end
+          end
         end
       end
     end

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -860,6 +860,20 @@ RSpec.describe Projects::MergeRequestsController do
         end
       end
     end
+
+    context 'with pagination' do
+      before do
+        create(:ci_pipeline, project: merge_request.source_project, ref: merge_request.source_branch, sha: merge_request.diff_head_sha)
+      end
+
+      it 'paginates the result' do
+        allow(Ci::Pipeline).to receive(:default_per_page).and_return(1)
+
+        get :pipelines, params: { namespace_id: project.namespace.to_param, project_id: project, id: merge_request.iid }, format: :json
+
+        expect(json_response['pipelines'].count).to eq(1)
+      end
+    end
   end
 
   describe 'GET context commits' do

spec/controllers/projects/pipelines_controller_spec.rb

@@ -66,6 +66,14 @@ RSpec.describe Projects::PipelinesController do
       expect(json_response['pipelines'][0]).not_to include('coverage')
     end
 
+    it 'paginates the result' do
+      allow(Ci::Pipeline).to receive(:default_per_page).and_return(2)
+
+      get_pipelines_index_json
+
+      check_pipeline_response(returned: 2, all: 6)
+    end
+
     context 'when performing gitaly calls', :request_store do
       it 'limits the Gitaly requests' do
         # Isolate from test preparation (Repository#exists? is also cached in RequestStore)

spec/lib/gitlab/pagination/offset_pagination_spec.rb

@@ -130,6 +130,80 @@ RSpec.describe Gitlab::Pagination::OffsetPagination do
           end
         end
 
+        context 'when resource already paginated' do
+          let(:resource) { Project.all.page(1).per(1) }
+
+          context 'when per_page param is specified' do
+            let(:query) { base_query.merge(page: 1, per_page: 2) }
+
+            it 'returns appropriate amount of resources based on per_page param' do
+              expect(subject.paginate(resource).count).to eq 2
+            end
+          end
+
+          context 'when page and per page params are strings' do
+            let(:query) { base_query.merge(page: '1', per_page: '1') }
+
+            it 'returns appropriate amount of resources' do
+              expect(subject.paginate(resource).count).to eq 1
+            end
+          end
+
+          context 'when per_page param is blank' do
+            let(:query) { base_query.merge(page: 1) }
+
+            it 'returns appropriate amount of resources' do
+              expect(subject.paginate(resource).count).to eq 1
+            end
+          end
+
+          context 'when page param is blank' do
+            let(:query) { base_query }
+
+            it 'returns appropriate amount of resources based on resource per(N)' do
+              expect(subject.paginate(resource).count).to eq 1
+            end
+          end
+        end
+
+        context 'when resource does not respond to limit_value' do
+          let(:custom_collection) do
+            Class.new do
+              include Enumerable
+
+              def initialize(items)
+                @collection = items
+              end
+
+              def each
+                @collection.each { |item| yield item }
+              end
+
+              def page(number)
+                Kaminari.paginate_array(@collection).page(number)
+              end
+            end
+          end
+
+          let(:resource) { custom_collection.new(Project.all).page(query[:page]) }
+
+          context 'when page param is blank' do
+            let(:query) { base_query }
+
+            it 'returns appropriate amount of resources' do
+              expect(subject.paginate(resource).count).to eq 3
+            end
+          end
+
+          context 'when per_page param is blank' do
+            let(:query) { base_query.merge(page: 1) }
+
+            it 'returns appropriate amount of resources with default per page value' do
+              expect(subject.paginate(resource).count).to eq 3
+            end
+          end
+        end
+
         context 'when resource is a paginatable array' do
           let(:resource) { Kaminari.paginate_array(Project.all.to_a) }
 

spec/models/ci/pipeline_spec.rb

@@ -11,6 +11,10 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
   let_it_be(:namespace) { create_default(:namespace).freeze }
   let_it_be(:project) { create_default(:project, :repository).freeze }
 
+  it 'paginates 15 pipeleines per page' do
+    expect(described_class.default_per_page).to eq(15)
+  end
+
   it_behaves_like 'having unique enum values'
 
   it { is_expected.to belong_to(:project) }