Merge branch 'ph/removeBatchDiffsFeatureFlag' into 'master'

Removes batch diffs feature flag

See merge request gitlab-org/gitlab!40493

app/assets/javascripts/diffs/components/app.vue

@@ -278,7 +278,6 @@ export default {
     ...mapActions('diffs', [
       'moveToNeighboringCommit',
       'setBaseConfig',
-      'fetchDiffFiles',
       'fetchDiffFilesMeta',
       'fetchDiffFilesBatch',
       'fetchCoverageFiles',
@@ -311,50 +310,29 @@ export default {
       return !this.diffFiles.length;
     },
     fetchData(toggleTree = true) {
-      if (this.glFeatures.diffsBatchLoad) {
-        this.fetchDiffFilesMeta()
-          .then(({ real_size }) => {
-            this.diffFilesLength = parseInt(real_size, 10);
-            if (toggleTree) this.hideTreeListIfJustOneFile();
+      this.fetchDiffFilesMeta()
+        .then(({ real_size }) => {
+          this.diffFilesLength = parseInt(real_size, 10);
+          if (toggleTree) this.hideTreeListIfJustOneFile();
 
-            this.startDiffRendering();
-          })
-          .catch(() => {
-            createFlash(__('Something went wrong on our end. Please try again!'));
-          });
-
-        this.fetchDiffFilesBatch()
-          .then(() => {
-            // Guarantee the discussions are assigned after the batch finishes.
-            // Just watching the length of the discussions or the diff files
-            // isn't enough, because with split diff loading, neither will
-            // change when loading the other half of the diff files.
-            this.setDiscussions();
-          })
-          .then(() => this.startDiffRendering())
-          .catch(() => {
-            createFlash(__('Something went wrong on our end. Please try again!'));
-          });
-      } else {
-        this.fetchDiffFiles()
-          .then(({ real_size }) => {
-            this.diffFilesLength = parseInt(real_size, 10);
-            if (toggleTree) {
-              this.hideTreeListIfJustOneFile();
-            }
+          this.startDiffRendering();
+        })
+        .catch(() => {
+          createFlash(__('Something went wrong on our end. Please try again!'));
+        });
 
-            requestIdleCallback(
-              () => {
-                this.setDiscussions();
-                this.startRenderDiffsQueue();
-              },
-              { timeout: 1000 },
-            );
-          })
-          .catch(() => {
-            createFlash(__('Something went wrong on our end. Please try again!'));
-          });
-      }
+      this.fetchDiffFilesBatch()
+        .then(() => {
+          // Guarantee the discussions are assigned after the batch finishes.
+          // Just watching the length of the discussions or the diff files
+          // isn't enough, because with split diff loading, neither will
+          // change when loading the other half of the diff files.
+          this.setDiscussions();
+        })
+        .then(() => this.startDiffRendering())
+        .catch(() => {
+          createFlash(__('Something went wrong on our end. Please try again!'));
+        });
 
       if (this.endpointCoverage) {
         this.fetchCoverageFiles();

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

@@ -64,42 +64,6 @@ export const setBaseConfig = ({ commit }, options) => {
   });
 };
 
-export const fetchDiffFiles = ({ state, commit }) => {
-  const worker = new TreeWorker();
-  const urlParams = {
-    w: state.showWhitespace ? '0' : '1',
-    view: window.gon?.features?.unifiedDiffLines ? 'inline' : state.diffViewType,
-  };
-  let returnData;
-
-  commit(types.SET_LOADING, true);
-
-  worker.addEventListener('message', ({ data }) => {
-    commit(types.SET_TREE_DATA, data);
-
-    worker.terminate();
-  });
-
-  return axios
-    .get(mergeUrlParams(urlParams, state.endpoint))
-    .then(res => {
-      commit(types.SET_LOADING, false);
-
-      commit(types.SET_MERGE_REQUEST_DIFFS, res.data.merge_request_diffs || []);
-      commit(types.SET_DIFF_DATA, res.data);
-
-      worker.postMessage(state.diffFiles);
-
-      returnData = res.data;
-      return Vue.nextTick();
-    })
-    .then(() => {
-      handleLocationHash();
-      return returnData;
-    })
-    .catch(() => worker.terminate());
-};
-
 export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
   const id = window?.location?.hash;
   const isNoteLink = id.indexOf('#note') === 0;

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

@@ -56,10 +56,7 @@ export default {
   [types.SET_DIFF_DATA](state, data) {
     let files = state.diffFiles;
 
-    if (
-      !(gon?.features?.diffsBatchLoad && window.location.search.indexOf('diff_id') === -1) &&
-      data.diff_files
-    ) {
+    if (window.location.search.indexOf('diff_id') !== -1 && data.diff_files) {
       files = prepareDiffData(data, files);
     }
 

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -21,15 +21,15 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   end
 
   def diffs_batch
-    return render_404 unless Feature.enabled?(:diffs_batch_load, @merge_request.project, default_enabled: true)
-
     diffs = @compare.diffs_in_batch(params[:page], params[:per_page], diff_options: diff_options)
     positions = @merge_request.note_positions_for_paths(diffs.diff_file_paths, current_user)
+    environment = @merge_request.environments_for(current_user, latest: true).last
 
     diffs.unfold_diff_files(positions.unfoldable)
     diffs.write_cache
 
     options = {
+      environment: environment,
       merge_request: @merge_request,
       diff_view: diff_view,
       pagination_data: diffs.pagination_data

app/controllers/projects/merge_requests_controller.rb

@@ -25,7 +25,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   before_action :authenticate_user!, only: [:assign_related_issues]
   before_action :check_user_can_push_to_source_branch!, only: [:rebase]
   before_action only: [:show] do
-    push_frontend_feature_flag(:diffs_batch_load, @project, default_enabled: true)
     push_frontend_feature_flag(:deploy_from_footer, @project, default_enabled: true)
     push_frontend_feature_flag(:suggest_pipeline) if experiment_enabled?(:suggest_pipeline)
     push_frontend_feature_flag(:code_navigation, @project, default_enabled: true)

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

@@ -414,6 +414,7 @@ RSpec.describe Projects::MergeRequests::DiffsController do
 
     def collection_arguments(pagination_data = {})
       {
+        environment: nil,
         merge_request: merge_request,
         diff_view: :inline,
         pagination_data: {
@@ -439,18 +440,6 @@ RSpec.describe Projects::MergeRequests::DiffsController do
 
     it_behaves_like '404 for unexistent diffable'
 
-    context 'when feature is disabled' do
-      before do
-        stub_feature_flags(diffs_batch_load: false)
-      end
-
-      it 'returns 404' do
-        go
-
-        expect(response).to have_gitlab_http_status(:not_found)
-      end
-    end
-
     context 'when not authorized' do
       let(:other_user) { create(:user) }
 

spec/features/merge_request/batch_comments_spec.rb

@@ -20,8 +20,6 @@ RSpec.describe 'Merge request > Batch comments', :js do
 
   context 'Feature is enabled' do
     before do
-      stub_feature_flags(diffs_batch_load: false)
-
       visit_diffs
     end
 

spec/features/merge_request/user_expands_diff_spec.rb

@@ -7,8 +7,6 @@ RSpec.describe 'User expands diff', :js do
   let(:merge_request) { create(:merge_request, source_branch: 'expand-collapse-files', source_project: project, target_project: project) }
 
   before do
-    stub_feature_flags(diffs_batch_load: false)
-
     allow(Gitlab::Git::Diff).to receive(:size_limit).and_return(100.kilobytes)
     allow(Gitlab::Git::Diff).to receive(:collapse_limit).and_return(10.kilobytes)
 
@@ -18,7 +16,7 @@ RSpec.describe 'User expands diff', :js do
   end
 
   it 'allows user to expand diff' do
-    page.within find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9"]') do
+    page.within find('[id="19763941ab80e8c09871c0a425f0560d9053bcb3"]') do
       click_link 'Click to expand it.'
 
       wait_for_requests

spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb

@@ -10,8 +10,6 @@ RSpec.describe 'Batch diffs', :js do
   let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'master', target_branch: 'empty-branch') }
 
   before do
-    stub_feature_flags(diffs_batch_load: true)
-
     sign_in(project.owner)
 
     visit diffs_project_merge_request_path(merge_request.project, merge_request)

spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb

@@ -15,10 +15,6 @@ RSpec.describe 'Merge request > User resolves diff notes and threads', :js do
           diff_refs: merge_request.diff_refs)
   end
 
-  before do
-    stub_feature_flags(diffs_batch_load: false)
-  end
-
   context 'no threads' do
     before do
       project.add_maintainer(user)

spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb

@@ -20,7 +20,6 @@ RSpec.describe 'Merge request > User sees avatars on diff notes', :js do
   let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) }
 
   before do
-    stub_feature_flags(diffs_batch_load: false)
     project.add_maintainer(user)
     sign_in user
 

spec/features/merge_request/user_sees_diff_spec.rb

@@ -9,10 +9,6 @@ RSpec.describe 'Merge request > User sees diff', :js do
   let(:project) { create(:project, :public, :repository) }
   let(:merge_request) { create(:merge_request, source_project: project) }
 
-  before do
-    stub_feature_flags(diffs_batch_load: false)
-  end
-
   context 'when linking to note' do
     describe 'with unresolved note' do
       let(:note) { create :diff_note_on_merge_request, project: project, noteable: merge_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_batch_load: false)
-
     project.add_maintainer(user)
     sign_in(user)
     visit diffs_project_merge_request_path(project, merge_request, params)

spec/features/merge_request/user_views_diffs_spec.rb

@@ -11,7 +11,6 @@ RSpec.describe 'User views diffs', :js do
   let(:view) { 'inline' }
 
   before do
-    stub_feature_flags(diffs_batch_load: false)
     visit(diffs_project_merge_request_path(project, merge_request, view: view))
 
     wait_for_requests
@@ -62,7 +61,7 @@ RSpec.describe 'User views diffs', :js do
   end
 
   it 'expands all diffs' do
-    first('#a5cc2925ca8258af241be7e5b0381edf30266302 .js-file-title').click
+    first('.js-file-title').click
 
     expect(page).to have_button('Expand all')
 

spec/features/merge_requests/user_views_diffs_commit_spec.rb

@@ -10,7 +10,6 @@ RSpec.describe 'User views diff by commit', :js do
   let(:project) { create(:project, :public, :repository) }
 
   before do
-    stub_feature_flags(diffs_batch_load: false)
     visit(diffs_project_merge_request_path(project, merge_request, commit_id: merge_request.diff_head_sha))
   end
 

spec/features/projects/view_on_env_spec.rb

@@ -9,8 +9,6 @@ RSpec.describe 'View on environment', :js do
   let(:user) { project.creator }
 
   before do
-    stub_feature_flags(diffs_batch_load: false)
-
     project.add_maintainer(user)
   end
 

spec/frontend/diffs/components/app_spec.js

@@ -108,7 +108,6 @@ describe('diffs/components/app', () => {
       };
       jest.spyOn(window, 'requestIdleCallback').mockImplementation(fn => fn());
       createComponent();
-      jest.spyOn(wrapper.vm, 'fetchDiffFiles').mockImplementation(fetchResolver);
       jest.spyOn(wrapper.vm, 'fetchDiffFilesMeta').mockImplementation(fetchResolver);
       jest.spyOn(wrapper.vm, 'fetchDiffFilesBatch').mockImplementation(fetchResolver);
       jest.spyOn(wrapper.vm, 'fetchCoverageFiles').mockImplementation(fetchResolver);
@@ -139,22 +138,10 @@ describe('diffs/components/app', () => {
         parallel_diff_lines: ['line'],
       };
 
-      function expectFetchToOccur({
-        vueInstance,
-        done = () => {},
-        batch = false,
-        existingFiles = 1,
-      } = {}) {
+      function expectFetchToOccur({ vueInstance, done = () => {}, existingFiles = 1 } = {}) {
         vueInstance.$nextTick(() => {
           expect(vueInstance.diffFiles.length).toEqual(existingFiles);
-
-          if (!batch) {
-            expect(vueInstance.fetchDiffFiles).toHaveBeenCalled();
-            expect(vueInstance.fetchDiffFilesBatch).not.toHaveBeenCalled();
-          } else {
-            expect(vueInstance.fetchDiffFiles).not.toHaveBeenCalled();
-            expect(vueInstance.fetchDiffFilesBatch).toHaveBeenCalled();
-          }
+          expect(vueInstance.fetchDiffFilesBatch).toHaveBeenCalled();
 
           done();
         });
@@ -165,7 +152,7 @@ describe('diffs/components/app', () => {
 
         store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
 
-        expectFetchToOccur({ vueInstance: wrapper.vm, batch: false, existingFiles: 0, done });
+        expectFetchToOccur({ vueInstance: wrapper.vm, existingFiles: 0, done });
       });
 
       it('fetches diffs if it has both view styles, but no lines in either', done => {
@@ -196,89 +183,46 @@ describe('diffs/components/app', () => {
       });
 
       it('fetches batch diffs if it has none', done => {
-        wrapper.vm.glFeatures.diffsBatchLoad = true;
-
         store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
 
-        expectFetchToOccur({ vueInstance: wrapper.vm, batch: true, existingFiles: 0, done });
+        expectFetchToOccur({ vueInstance: wrapper.vm, existingFiles: 0, done });
       });
 
       it('fetches batch diffs if it has both view styles, but no lines in either', done => {
-        wrapper.vm.glFeatures.diffsBatchLoad = true;
-
         store.state.diffs.diffFiles.push(noLinesDiff);
         store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
 
-        expectFetchToOccur({ vueInstance: wrapper.vm, batch: true, done });
+        expectFetchToOccur({ vueInstance: wrapper.vm, done });
       });
 
       it('fetches batch diffs if it only has inline view style', done => {
-        wrapper.vm.glFeatures.diffsBatchLoad = true;
-
         store.state.diffs.diffFiles.push(inlineLinesDiff);
         store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
 
-        expectFetchToOccur({ vueInstance: wrapper.vm, batch: true, done });
+        expectFetchToOccur({ vueInstance: wrapper.vm, done });
       });
 
       it('fetches batch diffs if it only has parallel view style', done => {
-        wrapper.vm.glFeatures.diffsBatchLoad = true;
-
         store.state.diffs.diffFiles.push(parallelLinesDiff);
         store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
 
-        expectFetchToOccur({ vueInstance: wrapper.vm, batch: true, done });
-      });
-
-      it('does not fetch diffs if it has already fetched both styles of diff', () => {
-        wrapper.vm.glFeatures.diffsBatchLoad = false;
-
-        store.state.diffs.diffFiles.push(fullDiff);
-        store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
-
-        expect(wrapper.vm.diffFiles.length).toEqual(1);
-        expect(wrapper.vm.fetchDiffFiles).not.toHaveBeenCalled();
-        expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled();
+        expectFetchToOccur({ vueInstance: wrapper.vm, done });
       });
 
       it('does not fetch batch diffs if it has already fetched both styles of diff', () => {
-        wrapper.vm.glFeatures.diffsBatchLoad = true;
-
         store.state.diffs.diffFiles.push(fullDiff);
         store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
 
         expect(wrapper.vm.diffFiles.length).toEqual(1);
-        expect(wrapper.vm.fetchDiffFiles).not.toHaveBeenCalled();
-        expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled();
-      });
-    });
-
-    it('calls fetchDiffFiles if diffsBatchLoad is not enabled', done => {
-      expect(wrapper.vm.diffFilesLength).toEqual(0);
-      wrapper.vm.glFeatures.diffsBatchLoad = false;
-      wrapper.vm.fetchData(false);
-
-      expect(wrapper.vm.fetchDiffFiles).toHaveBeenCalled();
-      setImmediate(() => {
-        expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
-        expect(wrapper.vm.fetchDiffFilesMeta).not.toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled();
-        expect(wrapper.vm.fetchCoverageFiles).toHaveBeenCalled();
-        expect(wrapper.vm.unwatchDiscussions).toHaveBeenCalled();
-        expect(wrapper.vm.diffFilesLength).toEqual(100);
-        expect(wrapper.vm.unwatchRetrievingBatches).toHaveBeenCalled();
-
-        done();
       });
     });
 
     it('calls batch methods if diffsBatchLoad is enabled, and not latest version', done => {
       expect(wrapper.vm.diffFilesLength).toEqual(0);
-      wrapper.vm.glFeatures.diffsBatchLoad = true;
       wrapper.vm.isLatestVersion = () => false;
       wrapper.vm.fetchData(false);
 
-      expect(wrapper.vm.fetchDiffFiles).not.toHaveBeenCalled();
       setImmediate(() => {
         expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesMeta).toHaveBeenCalled();
@@ -293,10 +237,8 @@ describe('diffs/components/app', () => {
 
     it('calls batch methods if diffsBatchLoad is enabled, and latest version', done => {
       expect(wrapper.vm.diffFilesLength).toEqual(0);
-      wrapper.vm.glFeatures.diffsBatchLoad = true;
       wrapper.vm.fetchData(false);
 
-      expect(wrapper.vm.fetchDiffFiles).not.toHaveBeenCalled();
       setImmediate(() => {
         expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesMeta).toHaveBeenCalled();

spec/frontend/diffs/store/actions_spec.js

@@ -13,7 +13,6 @@ import {
 } from '~/diffs/constants';
 import {
   setBaseConfig,
-  fetchDiffFiles,
   fetchDiffFilesBatch,
   fetchDiffFilesMeta,
   fetchCoverageFiles,
@@ -142,32 +141,6 @@ describe('DiffsStoreActions', () => {
     });
   });
 
-  describe('fetchDiffFiles', () => {
-    it('should fetch diff files', done => {
-      const endpoint = '/fetch/diff/files?view=inline&w=1';
-      const mock = new MockAdapter(axios);
-      const res = { diff_files: 1, merge_request_diffs: [] };
-      mock.onGet(endpoint).reply(200, res);
-
-      testAction(
-        fetchDiffFiles,
-        {},
-        { endpoint, diffFiles: [], showWhitespace: false, diffViewType: 'inline' },
-        [
-          { type: types.SET_LOADING, payload: true },
-          { type: types.SET_LOADING, payload: false },
-          { type: types.SET_MERGE_REQUEST_DIFFS, payload: res.merge_request_diffs },
-          { type: types.SET_DIFF_DATA, payload: res },
-        ],
-        [],
-        () => {
-          mock.restore();
-          done();
-        },
-      );
-    });
-  });
-
   describe('fetchDiffFilesBatch', () => {
     let mock;
 

spec/frontend/diffs/store/mutations_spec.js

@@ -68,12 +68,13 @@ describe('DiffsStoreMutations', () => {
   });
 
   describe('SET_DIFF_DATA', () => {
-    it('should set diff data type properly', () => {
+    it('should not modify the existing state', () => {
       const state = {
         diffFiles: [
           {
-            ...diffFileMockData,
-            parallel_diff_lines: [],
+            content_sha: diffFileMockData.content_sha,
+            file_hash: diffFileMockData.file_hash,
+            highlighted_diff_lines: [],
           },
         ],
       };
@@ -83,43 +84,7 @@ describe('DiffsStoreMutations', () => {
 
       mutations[types.SET_DIFF_DATA](state, diffMock);
 
-      const firstLine = state.diffFiles[0].parallel_diff_lines[0];
-
-      expect(firstLine.right.text).toBeUndefined();
-      expect(state.diffFiles.length).toEqual(1);
-      expect(state.diffFiles[0].renderIt).toEqual(true);
-      expect(state.diffFiles[0].collapsed).toEqual(false);
-    });
-
-    describe('given diffsBatchLoad feature flag is enabled', () => {
-      beforeEach(() => {
-        gon.features = { diffsBatchLoad: true };
-      });
-
-      afterEach(() => {
-        delete gon.features;
-      });
-
-      it('should not modify the existing state', () => {
-        const state = {
-          diffFiles: [
-            {
-              content_sha: diffFileMockData.content_sha,
-              file_hash: diffFileMockData.file_hash,
-              highlighted_diff_lines: [],
-            },
-          ],
-        };
-        const diffMock = {
-          diff_files: [diffFileMockData],
-        };
-
-        mutations[types.SET_DIFF_DATA](state, diffMock);
-
-        // If the batch load is enabled, there shouldn't be any processing
-        // done on the existing state object, so we shouldn't have this.
-        expect(state.diffFiles[0].parallel_diff_lines).toBeUndefined();
-      });
+      expect(state.diffFiles[0].parallel_diff_lines).toBeUndefined();
     });
   });