Merge branch 'ph/removeSingleDiffViewFeatureFlag' into 'master'

Removes the single diff view feature flag

See merge request gitlab-org/gitlab!40406

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

@@ -229,7 +229,6 @@ export default {
       projectPath: this.projectPath,
       dismissEndpoint: this.dismissEndpoint,
       showSuggestPopover: this.showSuggestPopover,
-      useSingleDiffStyle: this.glFeatures.singleMrDiffView,
       viewDiffsFileByFile: this.viewDiffsFileByFile,
     });
 
@@ -306,14 +305,10 @@ export default {
       );
     },
     needsReload() {
-      return (
-        this.glFeatures.singleMrDiffView &&
-        this.diffFiles.length &&
-        isSingleViewStyle(this.diffFiles[0])
-      );
+      return this.diffFiles.length && isSingleViewStyle(this.diffFiles[0]);
     },
     needsFirstLoad() {
-      return this.glFeatures.singleMrDiffView && !this.diffFiles.length;
+      return !this.diffFiles.length;
     },
     fetchData(toggleTree = true) {
       if (this.glFeatures.diffsBatchLoad) {

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

@@ -52,7 +52,6 @@ export const setBaseConfig = ({ commit }, options) => {
     projectPath,
     dismissEndpoint,
     showSuggestPopover,
-    useSingleDiffStyle,
   } = options;
   commit(types.SET_BASE_CONFIG, {
     endpoint,
@@ -62,7 +61,6 @@ export const setBaseConfig = ({ commit }, options) => {
     projectPath,
     dismissEndpoint,
     showSuggestPopover,
-    useSingleDiffStyle,
   });
 };
 
@@ -70,13 +68,10 @@ 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;
 
-  if (state.useSingleDiffStyle) {
-    urlParams.view = window.gon?.features?.unifiedDiffLines ? 'inline' : state.diffViewType;
-  }
-
   commit(types.SET_LOADING, true);
 
   worker.addEventListener('message', ({ data }) => {
@@ -111,12 +106,9 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
   const urlParams = {
     per_page: DIFFS_PER_PAGE,
     w: state.showWhitespace ? '0' : '1',
+    view: window.gon?.features?.unifiedDiffLines ? 'inline' : state.diffViewType,
   };
 
-  if (state.useSingleDiffStyle) {
-    urlParams.view = window.gon?.features?.unifiedDiffLines ? 'inline' : state.diffViewType;
-  }
-
   commit(types.SET_BATCH_LOADING, true);
   commit(types.SET_RETRIEVING_BATCHES, true);
 
@@ -175,11 +167,9 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
 
 export const fetchDiffFilesMeta = ({ commit, state }) => {
   const worker = new TreeWorker();
-  const urlParams = {};
-
-  if (state.useSingleDiffStyle) {
-    urlParams.view = window.gon?.features?.unifiedDiffLines ? 'inline' : state.diffViewType;
-  }
+  const urlParams = {
+    view: window.gon?.features?.unifiedDiffLines ? 'inline' : state.diffViewType,
+  };
 
   commit(types.SET_LOADING, true);
 
@@ -240,10 +230,7 @@ export const assignDiscussionsToDiff = (
 ) => {
   const id = window?.location?.hash;
   const isNoteLink = id.indexOf('#note') === 0;
-  const diffPositionByLineCode = getDiffPositionByLineCode(
-    state.diffFiles,
-    state.useSingleDiffStyle,
-  );
+  const diffPositionByLineCode = getDiffPositionByLineCode(state.diffFiles);
   const hash = getLocationHash();
 
   discussions

app/assets/javascripts/diffs/store/modules/diff_state.js

@@ -41,5 +41,4 @@ export default () => ({
   fileFinderVisible: false,
   dismissEndpoint: '',
   showSuggestPopover: true,
-  useSingleDiffStyle: false,
 });

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

@@ -25,7 +25,6 @@ export default {
       projectPath,
       dismissEndpoint,
       showSuggestPopover,
-      useSingleDiffStyle,
     } = options;
     Object.assign(state, {
       endpoint,
@@ -35,7 +34,6 @@ export default {
       projectPath,
       dismissEndpoint,
       showSuggestPopover,
-      useSingleDiffStyle,
     });
   },
 

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

@@ -495,11 +495,11 @@ export function prepareDiffData(diff, priorFiles = []) {
   return deduplicateFilesList([...priorFiles, ...cleanedFiles]);
 }
 
-export function getDiffPositionByLineCode(diffFiles, useSingleDiffStyle) {
+export function getDiffPositionByLineCode(diffFiles) {
   let lines = [];
   const hasInlineDiffs = diffFiles.some(file => file.highlighted_diff_lines.length > 0);
 
-  if (!useSingleDiffStyle || hasInlineDiffs) {
+  if (hasInlineDiffs) {
     // In either of these cases, we can use `highlighted_diff_lines` because
     // that will include all of the parallel diff lines, too
 

app/controllers/projects/merge_requests_controller.rb

@@ -27,7 +27,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   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(:single_mr_diff_view, @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)
     push_frontend_feature_flag(:widget_visibility_polling, @project, default_enabled: true)

app/serializers/diff_file_entity.rb

@@ -71,15 +71,11 @@ class DiffFileEntity < DiffFileBaseEntity
   private
 
   def parallel_diff_view?(options, diff_file)
-    return true unless Feature.enabled?(:single_mr_diff_view, diff_file.repository.project, default_enabled: true)
-
     # If we're not rendering inline, we must be rendering parallel
     !inline_diff_view?(options, diff_file)
   end
 
   def inline_diff_view?(options, diff_file)
-    return true unless Feature.enabled?(:single_mr_diff_view, diff_file.repository.project, default_enabled: true)
-
     # If nothing is present, inline will be the default.
     options.fetch(:diff_view, :inline).to_sym == :inline
   end

spec/features/merge_request/maintainer_edits_fork_spec.rb

@@ -20,8 +20,6 @@ RSpec.describe 'a maintainer edits files on a source-branch of an MR from a fork
   end
 
   before do
-    stub_feature_flags(single_mr_diff_view: false)
-
     target_project.add_maintainer(user)
     sign_in(user)
 

spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb

@@ -10,7 +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(single_mr_diff_view: project)
     stub_feature_flags(diffs_batch_load: true)
 
     sign_in(project.owner)

spec/frontend/diffs/components/app_spec.js

@@ -160,10 +160,6 @@ describe('diffs/components/app', () => {
         });
       }
 
-      beforeEach(() => {
-        wrapper.vm.glFeatures.singleMrDiffView = true;
-      });
-
       it('fetches diffs if it has none', done => {
         wrapper.vm.isLatestVersion = () => false;
 

spec/frontend/diffs/store/actions_spec.js

@@ -101,7 +101,6 @@ describe('DiffsStoreActions', () => {
       const projectPath = '/root/project';
       const dismissEndpoint = '/-/user_callouts';
       const showSuggestPopover = false;
-      const useSingleDiffStyle = false;
 
       testAction(
         setBaseConfig,
@@ -113,7 +112,6 @@ describe('DiffsStoreActions', () => {
           projectPath,
           dismissEndpoint,
           showSuggestPopover,
-          useSingleDiffStyle,
         },
         {
           endpoint: '',
@@ -123,7 +121,6 @@ describe('DiffsStoreActions', () => {
           projectPath: '',
           dismissEndpoint: '',
           showSuggestPopover: true,
-          useSingleDiffStyle: true,
         },
         [
           {
@@ -136,7 +133,6 @@ describe('DiffsStoreActions', () => {
               projectPath,
               dismissEndpoint,
               showSuggestPopover,
-              useSingleDiffStyle,
             },
           },
         ],
@@ -169,13 +165,6 @@ describe('DiffsStoreActions', () => {
           done();
         },
       );
-
-      fetchDiffFiles({ state: { endpoint }, commit: () => null })
-        .then(data => {
-          expect(data).toEqual(res);
-          done();
-        })
-        .catch(done.fail);
     });
   });
 
@@ -223,7 +212,7 @@ describe('DiffsStoreActions', () => {
       testAction(
         fetchDiffFilesBatch,
         {},
-        { endpointBatch, useSingleDiffStyle: true, diffViewType: 'inline' },
+        { endpointBatch, diffViewType: 'inline' },
         [
           { type: types.SET_BATCH_LOADING, payload: true },
           { type: types.SET_RETRIEVING_BATCHES, payload: true },
@@ -253,7 +242,6 @@ describe('DiffsStoreActions', () => {
           commit: () => {},
           state: {
             endpointBatch: `${endpointBatch}?view=${otherView}`,
-            useSingleDiffStyle: true,
             diffViewType: viewStyle,
           },
         })
@@ -283,146 +271,7 @@ describe('DiffsStoreActions', () => {
       testAction(
         fetchDiffFilesMeta,
         {},
-        { endpointMetadata },
-        [
-          { type: types.SET_LOADING, payload: true },
-          { type: types.SET_LOADING, payload: false },
-          { type: types.SET_MERGE_REQUEST_DIFFS, payload: diffMetadata.merge_request_diffs },
-          { type: types.SET_DIFF_DATA, payload: noFilesData },
-        ],
-        [],
-        () => {
-          mock.restore();
-          done();
-        },
-      );
-    });
-  });
-
-  describe('when the single diff view feature flag is off', () => {
-    describe('fetchDiffFiles', () => {
-      it('should fetch diff files', done => {
-        const endpoint = '/fetch/diff/files?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',
-            useSingleDiffStyle: false,
-            currentDiffFileId: null,
-          },
-          [
-            { 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();
-          },
-        );
-
-        fetchDiffFiles({ state: { endpoint }, commit: () => null })
-          .then(data => {
-            expect(data).toEqual(res);
-            done();
-          })
-          .catch(done.fail);
-      });
-    });
-
-    describe('fetchDiffFilesBatch', () => {
-      let mock;
-
-      beforeEach(() => {
-        mock = new MockAdapter(axios);
-      });
-
-      afterEach(() => {
-        mock.restore();
-      });
-
-      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: {} };
-        mock
-          .onGet(mergeUrlParams({ per_page: DIFFS_PER_PAGE, w: '1', page: 1 }, endpointBatch))
-          .reply(200, res1)
-          .onGet(mergeUrlParams({ per_page: DIFFS_PER_PAGE, w: '1', page: 2 }, endpointBatch))
-          .reply(200, res2);
-
-        testAction(
-          fetchDiffFilesBatch,
-          {},
-          { endpointBatch, useSingleDiffStyle: false, currentDiffFileId: null },
-          [
-            { type: types.SET_BATCH_LOADING, payload: true },
-            { type: types.SET_RETRIEVING_BATCHES, payload: true },
-            { type: types.SET_DIFF_DATA_BATCH, payload: { diff_files: res1.diff_files } },
-            { type: types.SET_BATCH_LOADING, payload: false },
-            { type: types.UPDATE_CURRENT_DIFF_FILE_ID, payload: 'test' },
-            { type: types.SET_DIFF_DATA_BATCH, payload: { diff_files: res2.diff_files } },
-            { type: types.SET_BATCH_LOADING, payload: false },
-            { type: types.UPDATE_CURRENT_DIFF_FILE_ID, payload: 'test2' },
-            { type: types.SET_RETRIEVING_BATCHES, payload: false },
-          ],
-          [],
-          done,
-        );
-      });
-
-      it.each`
-        querystrings        | requestUrl
-        ${'?view=parallel'} | ${'/fetch/diffs_batch?view=parallel'}
-        ${'?view=inline'}   | ${'/fetch/diffs_batch?view=inline'}
-        ${''}               | ${'/fetch/diffs_batch'}
-      `(
-        'should use the endpoint $requestUrl if the endpointBatch in state includes `$querystrings` as a querystring',
-        ({ querystrings, requestUrl }) => {
-          const endpointBatch = '/fetch/diffs_batch';
-
-          fetchDiffFilesBatch({
-            commit: () => {},
-            state: {
-              endpointBatch: `${endpointBatch}${querystrings}`,
-              diffViewType: 'inline',
-            },
-          })
-            .then(() => {
-              expect(mock.history.get[0].url).toEqual(requestUrl);
-            })
-            .catch(() => {});
-        },
-      );
-    });
-
-    describe('fetchDiffFilesMeta', () => {
-      const endpointMetadata = '/fetch/diffs_metadata.json';
-      const noFilesData = { ...diffMetadata };
-      let mock;
-
-      beforeEach(() => {
-        mock = new MockAdapter(axios);
-
-        delete noFilesData.diff_files;
-
-        mock.onGet(endpointMetadata).reply(200, diffMetadata);
-      });
-      it('should fetch diff meta information', done => {
-        testAction(
-          fetchDiffFilesMeta,
-          {},
-          { endpointMetadata, useSingleDiffStyle: false },
+        { endpointMetadata, diffViewType: 'inline' },
         [
           { type: types.SET_LOADING, payload: true },
           { type: types.SET_LOADING, payload: false },
@@ -437,7 +286,6 @@ describe('DiffsStoreActions', () => {
       );
     });
   });
-  });
 
   describe('fetchCoverageFiles', () => {
     let mock;
@@ -589,7 +437,7 @@ describe('DiffsStoreActions', () => {
       testAction(
         assignDiscussionsToDiff,
         [],
-        { diffFiles: [], useSingleDiffStyle: true },
+        { diffFiles: [] },
         [],
         [{ type: 'setCurrentDiffFileIdFromNote', payload: '123' }],
         done,

spec/frontend/diffs/store/mutations_spec.js

@@ -11,13 +11,11 @@ describe('DiffsStoreMutations', () => {
       const state = {};
       const endpoint = '/diffs/endpoint';
       const projectPath = '/root/project';
-      const useSingleDiffStyle = false;
 
-      mutations[types.SET_BASE_CONFIG](state, { endpoint, projectPath, useSingleDiffStyle });
+      mutations[types.SET_BASE_CONFIG](state, { endpoint, projectPath });
 
       expect(state.endpoint).toEqual(endpoint);
       expect(state.projectPath).toEqual(projectPath);
-      expect(state.useSingleDiffStyle).toEqual(useSingleDiffStyle);
     });
   });
 

spec/support/shared_examples/serializers/diff_file_entity_shared_examples.rb

@@ -57,16 +57,6 @@ RSpec.shared_examples 'diff file entity' do
         expect(subject).to include(:highlighted_diff_lines)
       end
     end
-
-    context 'when the `single_mr_diff_view` feature is disabled' do
-      before do
-        stub_feature_flags(single_mr_diff_view: false)
-      end
-
-      it 'contains both kinds of diffs' do
-        expect(subject).to include(:highlighted_diff_lines, :parallel_diff_lines)
-      end
-    end
   end
 end