Merge branch '326312-project-compare-controller-use-pagination' into 'master'

Enable pagination in the CompareController

See merge request gitlab-org/gitlab!59162

app/controllers/projects/commit_controller.rb

@@ -24,7 +24,7 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   BRANCH_SEARCH_LIMIT = 1000
-  COMMIT_DIFFS_PER_PAGE = 75
+  COMMIT_DIFFS_PER_PAGE = 20
 
   feature_category :source_code_management
 

app/controllers/projects/compare_controller.rb

@@ -26,6 +26,10 @@ class Projects::CompareController < Projects::ApplicationController
 
   feature_category :source_code_management
 
+  # Diffs may be pretty chunky, the less is better in this endpoint.
+  # Pagination design guides: https://design.gitlab.com/components/pagination/#behavior
+  COMMIT_DIFFS_PER_PAGE = 20
+
   def index
   end
 

app/helpers/commits_helper.rb

@@ -128,7 +128,7 @@ module CommitsHelper
     %w(btn gpg-status-box) + Array(additional_classes)
   end
 
-  def conditionally_paginate_diff_files(diffs, paginate:, per: Projects::CommitController::COMMIT_DIFFS_PER_PAGE)
+  def conditionally_paginate_diff_files(diffs, paginate:, per:)
     if paginate
       Kaminari.paginate_array(diffs.diff_files.to_a).page(params[:page]).per(per)
     else

app/views/projects/commit/show.html.haml

@@ -12,7 +12,12 @@
 .container-fluid{ class: [limited_container_width, container_class] }
   = render "commit_box"
   = render "ci_menu"
-  = render "projects/diffs/diffs", diffs: @diffs, environment: @environment, diff_page_context: "is-commit", paginate_diffs: true
+  = render "projects/diffs/diffs",
+    diffs: @diffs,
+    environment: @environment,
+    diff_page_context: "is-commit",
+    paginate_diffs: true,
+    paginate_diffs_per_page: Projects::CommitController::COMMIT_DIFFS_PER_PAGE
 
   .limited-width-notes
     = render "shared/notes/notes_with_form", :autocomplete => true

app/views/projects/compare/show.html.haml

@@ -6,8 +6,15 @@
     #js-compare-selector{ data: project_compare_selector_data(@project, @merge_request, params) }
 
 - if @commits.present?
-  = render "projects/commits/commit_list"
-  = render "projects/diffs/diffs", diffs: @diffs, environment: @environment, diff_page_context: "is-compare"
+  -# Only show commit list in the first page
+  - hide_commit_list = params[:page].present? && params[:page] != '1'
+  = render "projects/commits/commit_list" unless hide_commit_list
+  = render "projects/diffs/diffs",
+    diffs: @diffs,
+    environment: @environment,
+    diff_page_context: "is-compare",
+    paginate_diffs: true,
+    paginate_diffs_per_page: Projects::CompareController::COMMIT_DIFFS_PER_PAGE
 - else
   .card.bg-light
     .center

app/views/projects/diffs/_diffs.html.haml

@@ -4,7 +4,8 @@
 - diff_page_context = local_assigns.fetch(:diff_page_context, nil)
 - load_diff_files_async = Feature.enabled?(:async_commit_diff_files, @project) && diff_page_context == "is-commit"
 - paginate_diffs = local_assigns.fetch(:paginate_diffs, false) && !load_diff_files_async
-- diff_files = conditionally_paginate_diff_files(diffs, paginate: paginate_diffs)
+- paginate_diffs_per_page = local_assigns.fetch(:paginate_diffs_per_page, nil)
+- diff_files = conditionally_paginate_diff_files(diffs, paginate: paginate_diffs, per: paginate_diffs_per_page)
 
 .content-block.oneline-block.files-changed.diff-files-changed.js-diff-files-changed
   .files-changed-inner

changelogs/unreleased/326312-project-compare-controller-use-pagination.yml

+---
+title: Enable pagination in the CompareController
+merge_request: 59162
+author:
+type: performance

spec/features/projects/compare_spec.rb

@@ -118,6 +118,34 @@ RSpec.describe "Compare", :js do
         end
       end
     end
+
+    context "pagination" do
+      before do
+        stub_const("Projects::CompareController::COMMIT_DIFFS_PER_PAGE", 1)
+      end
+
+      it "shows an adjusted count for changed files on this page" do
+        visit project_compare_index_path(project, from: "feature", to: "master")
+
+        click_button('Compare')
+
+        expect(page).to have_content("Showing 1 changed file")
+      end
+
+      it "shows commits list only on the first page" do
+        visit project_compare_index_path(project, from: "feature", to: "master")
+        click_button('Compare')
+
+        expect(page).to have_content 'Commits (29)'
+
+        # go to the second page
+        within(".files .gl-pagination") do
+          click_on("2")
+        end
+
+        expect(page).not_to have_content 'Commits (29)'
+      end
+    end
   end
 
   describe "tags" do

spec/helpers/commits_helper_spec.rb

@@ -144,7 +144,7 @@ RSpec.describe CommitsHelper do
       }
     end
 
-    subject { helper.conditionally_paginate_diff_files(diffs_collection, paginate: paginate) }
+    subject { helper.conditionally_paginate_diff_files(diffs_collection, paginate: paginate, per: Projects::CommitController::COMMIT_DIFFS_PER_PAGE) }
 
     before do
       allow(helper).to receive(:params).and_return(params)
@@ -168,15 +168,15 @@ RSpec.describe CommitsHelper do
         let(:page) { 1 }
 
         it "has 20 diffs" do
-          expect(subject.size).to eq(75)
+          expect(subject.size).to eq(20)
         end
       end
 
-      context "page 2" do
-        let(:page) { 2 }
+      context "page 5" do
+        let(:page) { 5 }
 
-        it "has the remaining 10 diffs" do
-          expect(subject.size).to eq(10)
+        it "has the remaining 5 out of 85 diffs" do
+          expect(subject.size).to eq(5)
         end
       end
     end