Merge branch 'ph/323053/removeDiffsGradualLoadFeatureFlag' into 'master'

Remove diffs gradual load feature flag [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!55478

app/assets/javascripts/diffs/constants.js

@@ -59,7 +59,6 @@ export const MIN_RENDERING_MS = 2;
 export const START_RENDERING_INDEX = 200;
 export const INLINE_DIFF_LINES_KEY = 'highlighted_diff_lines';
 export const PARALLEL_DIFF_LINES_KEY = 'parallel_diff_lines';
-export const DIFFS_PER_PAGE = 20;
 
 export const DIFF_COMPARE_BASE_VERSION_INDEX = -1;
 export const DIFF_COMPARE_HEAD_VERSION_INDEX = -2;

app/assets/javascripts/diffs/store/actions.js

@@ -25,7 +25,6 @@ import {
   MIN_RENDERING_MS,
   START_RENDERING_INDEX,
   INLINE_DIFF_LINES_KEY,
-  DIFFS_PER_PAGE,
   DIFF_FILE_MANUAL_COLLAPSE,
   DIFF_FILE_AUTOMATIC_COLLAPSE,
   EVT_PERF_MARK_FILE_TREE_START,
@@ -92,15 +91,9 @@ export const setBaseConfig = ({ commit }, options) => {
 };
 
 export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
-  const diffsGradualLoad = window.gon?.features?.diffsGradualLoad;
-  let perPage = DIFFS_PER_PAGE;
+  let perPage = state.viewDiffsFileByFile ? 1 : 5;
   let increaseAmount = 1.4;
-
-  if (diffsGradualLoad) {
-    perPage = state.viewDiffsFileByFile ? 1 : 5;
-  }
-
-  const startPage = diffsGradualLoad ? 0 : 1;
+  const startPage = 0;
   const id = window?.location?.hash;
   const isNoteLink = id.indexOf('#note') === 0;
   const urlParams = {
@@ -130,11 +123,7 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
           dispatch('setCurrentDiffFileIdFromNote', id.split('_').pop());
         }
 
-        if (
-          (diffsGradualLoad &&
-            (totalLoaded === pagination.total_pages || pagination.total_pages === null)) ||
-          (!diffsGradualLoad && !pagination.next_page)
-        ) {
+        if (totalLoaded === pagination.total_pages || pagination.total_pages === null) {
           commit(types.SET_RETRIEVING_BATCHES, false);
 
           // We need to check that the currentDiffFileId points to a file that exists
@@ -164,15 +153,11 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
           return null;
         }
 
-        if (diffsGradualLoad) {
-          const nextPage = page + perPage;
-          perPage = Math.min(Math.ceil(perPage * increaseAmount), 30);
-          increaseAmount = Math.min(increaseAmount + 0.2, 2);
-
-          return nextPage;
-        }
+        const nextPage = page + perPage;
+        perPage = Math.min(Math.ceil(perPage * increaseAmount), 30);
+        increaseAmount = Math.min(increaseAmount + 0.2, 2);
 
-        return pagination.next_page;
+        return nextPage;
       })
       .then((nextPage) => {
         dispatch('startRenderDiffsQueue');

app/controllers/projects/merge_requests_controller.rb

@@ -35,7 +35,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     push_frontend_feature_flag(:drag_comment_selection, @project, default_enabled: true)
     push_frontend_feature_flag(:default_merge_ref_for_diffs, @project, default_enabled: :yaml)
     push_frontend_feature_flag(:core_security_mr_widget_counts, @project)
-    push_frontend_feature_flag(:diffs_gradual_load, @project, default_enabled: true)
     push_frontend_feature_flag(:local_file_reviews, default_enabled: :yaml)
     push_frontend_feature_flag(:paginated_notes, @project, default_enabled: :yaml)
     push_frontend_feature_flag(:confidential_notes, @project, default_enabled: :yaml)

config/feature_flags/development/diffs_gradual_load.yml

----
-name: diffs_gradual_load
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48253/
-rollout_issue_url:
-milestone: '13.7'
-type: development
-group: group::code review
-default_enabled: true

lib/gitlab/diff/file_collection/merge_request_diff_batch.rb

@@ -21,9 +21,9 @@ module Gitlab
           @paginated_collection = load_paginated_collection(batch_page, batch_size, diff_options)
 
           @pagination_data = {
-            current_page: current_page,
-            next_page: next_page,
-            total_pages: total_pages
+            current_page: nil,
+            next_page: nil,
+            total_pages: @paginated_collection.blank? ? nil : relation.size
           }
         end
 
@@ -62,24 +62,6 @@ module Gitlab
           @merge_request_diff.merge_request_diff_files
         end
 
-        def current_page
-          return if @paginated_collection.blank?
-
-          batch_gradual_load? ? nil : @paginated_collection.current_page
-        end
-
-        def next_page
-          return if @paginated_collection.blank?
-
-          batch_gradual_load? ? nil : @paginated_collection.next_page
-        end
-
-        def total_pages
-          return if @paginated_collection.blank?
-
-          batch_gradual_load? ? relation.size : @paginated_collection.total_pages
-        end
-
         # rubocop: disable CodeReuse/ActiveRecord
         def load_paginated_collection(batch_page, batch_size, diff_options)
           batch_page ||= DEFAULT_BATCH_PAGE
@@ -87,21 +69,12 @@ module Gitlab
 
           paths = diff_options&.fetch(:paths, nil)
 
-          paginated_collection = if batch_gradual_load?
-                                   relation.offset(batch_page).limit([batch_size.to_i, DEFAULT_BATCH_SIZE].min)
-                                 else
-                                   relation.page(batch_page).per(batch_size)
-                                 end
-
+          paginated_collection = relation.offset(batch_page).limit([batch_size.to_i, DEFAULT_BATCH_SIZE].min)
           paginated_collection = paginated_collection.by_paths(paths) if paths
 
           paginated_collection
         end
         # rubocop: enable CodeReuse/ActiveRecord
-
-        def batch_gradual_load?
-          Feature.enabled?(:diffs_gradual_load, @merge_request_diff.project, default_enabled: true)
-        end
       end
     end
   end

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

@@ -88,8 +88,6 @@ RSpec.describe Projects::MergeRequests::DiffsController do
   let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
 
   before do
-    stub_feature_flags(diffs_gradual_load: false)
-
     project.add_maintainer(user)
     sign_in(user)
   end
@@ -486,7 +484,7 @@ RSpec.describe Projects::MergeRequests::DiffsController do
         namespace_id: project.namespace.to_param,
         project_id: project,
         id: merge_request.iid,
-        page: 1,
+        page: 0,
         per_page: 20,
         format: 'json'
       }
@@ -517,7 +515,7 @@ RSpec.describe Projects::MergeRequests::DiffsController do
 
       it_behaves_like 'serializes diffs with expected arguments' do
         let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
-        let(:expected_options) { collection_arguments(current_page: 1, total_pages: 1).merge(merge_ref_head_diff: false) }
+        let(:expected_options) { collection_arguments(current_page: nil, total_pages: 20).merge(merge_ref_head_diff: false) }
       end
 
       it_behaves_like 'successful request'
@@ -557,7 +555,7 @@ RSpec.describe Projects::MergeRequests::DiffsController do
       it_behaves_like 'serializes diffs with expected arguments' do
         let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
         let(:expected_options) do
-          collection_arguments(current_page: 1, total_pages: 1)
+          collection_arguments(current_page: nil, total_pages: 20)
         end
       end
 
@@ -576,18 +574,18 @@ RSpec.describe Projects::MergeRequests::DiffsController do
 
       it_behaves_like 'serializes diffs with expected arguments' do
         let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
-        let(:expected_options) { collection_arguments(current_page: 1, total_pages: 1) }
+        let(:expected_options) { collection_arguments(current_page: nil, total_pages: 20) }
       end
 
       it_behaves_like 'successful request'
     end
 
     context 'with smaller diff batch params' do
-      subject { go(page: 2, per_page: 5) }
+      subject { go(page: 5, per_page: 5) }
 
       it_behaves_like 'serializes diffs with expected arguments' do
         let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
-        let(:expected_options) { collection_arguments(current_page: 2, next_page: 3, total_pages: 4) }
+        let(:expected_options) { collection_arguments(current_page: nil, next_page: nil, total_pages: 20) }
       end
 
       it_behaves_like 'successful request'

spec/features/merge_request/user_sees_versions_spec.rb

@@ -17,8 +17,6 @@ RSpec.describe 'Merge request > User sees versions', :js do
   let!(:params) { {} }
 
   before do
-    stub_feature_flags(diffs_gradual_load: false)
-
     project.add_maintainer(user)
     sign_in(user)
     visit diffs_project_merge_request_path(project, merge_request, params)

spec/frontend/diffs/store/actions_spec.js

@@ -8,7 +8,6 @@ import {
   DIFF_VIEW_COOKIE_NAME,
   INLINE_DIFF_VIEW_TYPE,
   PARALLEL_DIFF_VIEW_TYPE,
-  DIFFS_PER_PAGE,
 } from '~/diffs/constants';
 import {
   setBaseConfig,
@@ -154,16 +153,16 @@ describe('DiffsStoreActions', () => {
 
     it('should fetch batch diff files', (done) => {
       const endpointBatch = '/fetch/diffs_batch';
-      const res1 = { diff_files: [{ file_hash: 'test' }], pagination: { next_page: 2 } };
-      const res2 = { diff_files: [{ file_hash: 'test2' }], pagination: {} };
+      const res1 = { diff_files: [{ file_hash: 'test' }], pagination: { total_pages: 7 } };
+      const res2 = { diff_files: [{ file_hash: 'test2' }], pagination: { total_pages: 7 } };
       mock
         .onGet(
           mergeUrlParams(
             {
               w: '1',
               view: 'inline',
-              page: 1,
-              per_page: DIFFS_PER_PAGE,
+              page: 0,
+              per_page: 5,
             },
             endpointBatch,
           ),
@@ -174,8 +173,8 @@ describe('DiffsStoreActions', () => {
             {
               w: '1',
               view: 'inline',
-              page: 2,
-              per_page: DIFFS_PER_PAGE,
+              page: 5,
+              per_page: 7,
             },
             endpointBatch,
           ),

spec/lib/gitlab/diff/file_collection/merge_request_diff_batch_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do
   let(:merge_request) { create(:merge_request) }
-  let(:batch_page) { 1 }
+  let(:batch_page) { 0 }
   let(:batch_size) { 10 }
   let(:diffable) { merge_request.merge_request_diff }
   let(:diff_files_relation) { diffable.merge_request_diff_files }
@@ -18,19 +18,15 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do
 
   let(:diff_files) { subject.diff_files }
 
-  before do
-    stub_feature_flags(diffs_gradual_load: false)
-  end
-
   describe 'initialize' do
     it 'memoizes pagination_data' do
-      expect(subject.pagination_data).to eq(current_page: 1, next_page: 2, total_pages: 2)
+      expect(subject.pagination_data).to eq(current_page: nil, next_page: nil, total_pages: 20)
     end
   end
 
   describe '#diff_files' do
     let(:batch_size) { 3 }
-    let(:paginated_rel) { diff_files_relation.page(batch_page).per(batch_size) }
+    let(:paginated_rel) { diff_files_relation.offset(batch_page).limit(batch_size) }
 
     let(:expected_batch_files) do
       paginated_rel.map(&:new_path)
@@ -51,7 +47,7 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do
     end
 
     context 'another page' do
-      let(:batch_page) { 2 }
+      let(:batch_page) { 1 }
 
       it 'returns correct diff files' do
         expect(diff_files.map(&:new_path)).to eq(expected_batch_files)
@@ -63,7 +59,7 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do
 
       it 'returns correct diff files' do
         expected_batch_files =
-          diff_files_relation.page(described_class::DEFAULT_BATCH_PAGE).per(batch_size).map(&:new_path)
+          diff_files_relation.offset(described_class::DEFAULT_BATCH_PAGE).limit(batch_size).map(&:new_path)
 
         expect(diff_files.map(&:new_path)).to eq(expected_batch_files)
       end
@@ -74,7 +70,7 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do
 
       it 'returns correct diff files' do
         expected_batch_files =
-          diff_files_relation.page(batch_page).per(described_class::DEFAULT_BATCH_SIZE).map(&:new_path)
+          diff_files_relation.offset(batch_page).limit(described_class::DEFAULT_BATCH_SIZE).map(&:new_path)
 
         expect(diff_files.map(&:new_path)).to eq(expected_batch_files)
       end
@@ -90,29 +86,17 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do
 
     context 'last page' do
       it 'returns correct diff files' do
-        last_page = paginated_rel.total_pages
+        last_page = diff_files_relation.count - batch_size
         collection = described_class.new(diffable,
                                          last_page,
                                          batch_size,
                                          diff_options: nil)
 
-        expected_batch_files = diff_files_relation.page(last_page).per(batch_size).map(&:new_path)
+        expected_batch_files = diff_files_relation.offset(last_page).limit(batch_size).map(&:new_path)
 
         expect(collection.diff_files.map(&:new_path)).to eq(expected_batch_files)
       end
     end
-
-    context 'with diffs gradual load feature flag enabled' do
-      let(:batch_page) { 0 }
-
-      before do
-        stub_feature_flags(diffs_gradual_load: true)
-      end
-
-      it 'returns correct diff files' do
-        expect(subject.diffs.map(&:new_path)).to eq(diff_files_relation.page(1).per(batch_size).map(&:new_path))
-      end
-    end
   end
 
   it_behaves_like 'unfoldable diff' do
@@ -130,7 +114,7 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do
     end
 
     let(:diffable) { merge_request.merge_request_diff }
-    let(:batch_page) { 2 }
+    let(:batch_page) { 10 }
     let(:stub_path) { '.gitignore' }
 
     subject do

spec/models/merge_request_diff_spec.rb

@@ -9,10 +9,6 @@ RSpec.describe MergeRequestDiff do
 
   let(:diff_with_commits) { create(:merge_request).merge_request_diff }
 
-  before do
-    stub_feature_flags(diffs_gradual_load: false)
-  end
-
   describe 'validations' do
     subject { diff_with_commits }
 
@@ -460,19 +456,19 @@ RSpec.describe MergeRequestDiff do
 
       context 'when persisted files available' do
         it 'returns paginated diffs' do
-          diffs = diff_with_commits.diffs_in_batch(1, 10, diff_options: diff_options)
+          diffs = diff_with_commits.diffs_in_batch(0, 10, diff_options: diff_options)
 
           expect(diffs).to be_a(Gitlab::Diff::FileCollection::MergeRequestDiffBatch)
           expect(diffs.diff_files.size).to eq(10)
-          expect(diffs.pagination_data).to eq(current_page: 1,
-                                              next_page: 2,
-                                              total_pages: 2)
+          expect(diffs.pagination_data).to eq(current_page: nil,
+                                              next_page: nil,
+                                              total_pages: 20)
         end
 
         it 'sorts diff files directory first' do
           diff_with_commits.update!(sorted: false) # Mark as unsorted so it'll re-order
 
-          expect(diff_with_commits.diffs_in_batch(1, 10, diff_options: diff_options).diff_file_paths).to eq([
+          expect(diff_with_commits.diffs_in_batch(0, 10, diff_options: diff_options).diff_file_paths).to eq([
             'bar/branch-test.txt',
             'custom-highlighting/test.gitlab-custom',
             'encoding/iso8859.txt',
@@ -491,44 +487,22 @@ RSpec.describe MergeRequestDiff do
             { ignore_whitespace_change: true }
           end
 
-          it 'returns a Gitlab::Diff::FileCollection::Compare with paginated diffs' do
+          it 'returns pagination data from MergeRequestDiffBatch' do
             diffs = diff_with_commits.diffs_in_batch(1, 10, diff_options: diff_options)
+            file_count = diff_with_commits.merge_request_diff_files.count
 
             expect(diffs).to be_a(Gitlab::Diff::FileCollection::Compare)
             expect(diffs.diff_files.size).to eq 10
-            expect(diffs.pagination_data).to eq(current_page: 1, next_page: 2, total_pages: 2)
+            expect(diffs.pagination_data).to eq(current_page: nil, next_page: nil, total_pages: file_count)
           end
 
           it 'returns an empty MergeRequestBatch with empty pagination data when the batch is empty' do
-            diffs = diff_with_commits.diffs_in_batch(3, 10, diff_options: diff_options)
+            diffs = diff_with_commits.diffs_in_batch(30, 10, diff_options: diff_options)
 
             expect(diffs).to be_a(Gitlab::Diff::FileCollection::MergeRequestDiffBatch)
             expect(diffs.diff_files.size).to eq 0
             expect(diffs.pagination_data).to eq(current_page: nil, next_page: nil, total_pages: nil)
           end
-
-          context 'with gradual load enabled' do
-            before do
-              stub_feature_flags(diffs_gradual_load: true)
-            end
-
-            it 'returns pagination data from MergeRequestDiffBatch' do
-              diffs = diff_with_commits.diffs_in_batch(1, 10, diff_options: diff_options)
-              file_count = diff_with_commits.merge_request_diff_files.count
-
-              expect(diffs).to be_a(Gitlab::Diff::FileCollection::Compare)
-              expect(diffs.diff_files.size).to eq 10
-              expect(diffs.pagination_data).to eq(current_page: nil, next_page: nil, total_pages: file_count)
-            end
-
-            it 'returns an empty MergeRequestBatch with empty pagination data when the batch is empty' do
-              diffs = diff_with_commits.diffs_in_batch(30, 10, diff_options: diff_options)
-
-              expect(diffs).to be_a(Gitlab::Diff::FileCollection::MergeRequestDiffBatch)
-              expect(diffs.diff_files.size).to eq 0
-              expect(diffs.pagination_data).to eq(current_page: nil, next_page: nil, total_pages: nil)
-            end
-          end
         end
       end
     end

spec/serializers/paginated_diff_entity_spec.rb

@@ -19,20 +19,16 @@ RSpec.describe PaginatedDiffEntity do
 
   subject { entity.as_json }
 
-  before do
-    stub_feature_flags(diffs_gradual_load: false)
-  end
-
   it 'exposes diff_files' do
     expect(subject[:diff_files]).to be_present
   end
 
   it 'exposes pagination data' do
     expect(subject[:pagination]).to eq(
-      current_page: 2,
-      next_page: 3,
-      next_page_href: "/#{merge_request.project.full_path}/-/merge_requests/#{merge_request.iid}/diffs_batch.json?page=3",
-      total_pages: 7
+      current_page: nil,
+      next_page: nil,
+      next_page_href: nil,
+      total_pages: 20
     )
   end