Merge branch '276917-remove-default-merge-ref-for-diffs-ff' into 'master'

Remove default_merge_ref_for_diffs feature flag

See merge request gitlab-org/gitlab!82069

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

@@ -15,24 +15,19 @@ export const diffCompareDropdownTargetVersions = (state, getters) => {
   // startVersion only exists if the user has selected a version other
   // than "base" so if startVersion is null then base must be selected
 
-  const defaultMergeRefForDiffs = window.gon?.features?.defaultMergeRefForDiffs || false;
   const diffHeadParam = getParameterByName('diff_head');
-  const diffHead = parseBoolean(diffHeadParam) || (!diffHeadParam && defaultMergeRefForDiffs);
-  const isBaseSelected = !state.startVersion && !diffHead;
+  const diffHead = parseBoolean(diffHeadParam) || !diffHeadParam;
+  const isBaseSelected = !state.startVersion;
   const isHeadSelected = !state.startVersion && diffHead;
   let baseVersion = null;
 
-  if (
-    !defaultMergeRefForDiffs ||
-    (defaultMergeRefForDiffs && !state.mergeRequestDiff.head_version_path)
-  ) {
+  if (!state.mergeRequestDiff.head_version_path) {
     baseVersion = {
       versionName: state.targetBranchName,
       version_index: DIFF_COMPARE_BASE_VERSION_INDEX,
       href: state.mergeRequestDiff.base_version_path,
       isBase: true,
-      selected:
-        isBaseSelected || (defaultMergeRefForDiffs && !state.mergeRequestDiff.head_version_path),
+      selected: isBaseSelected,
     };
   }
 

app/controllers/projects/merge_requests_controller.rb

@@ -37,7 +37,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   before_action only: [:show] do
     push_frontend_feature_flag(:file_identifier_hash)
     push_frontend_feature_flag(:merge_request_widget_graphql, project, default_enabled: :yaml)
-    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(:paginated_notes, project, default_enabled: :yaml)
     push_frontend_feature_flag(:confidential_notes, project, default_enabled: :yaml)
@@ -553,12 +552,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   end
 
   def endpoint_metadata_url(project, merge_request)
-    params = request.query_parameters
-    params[:view] = "inline"
-
-    if Feature.enabled?(:default_merge_ref_for_diffs, project, default_enabled: :yaml)
-      params = params.merge(diff_head: true)
-    end
+    params = request.query_parameters.merge(view: 'inline', diff_head: true)
 
     diffs_metadata_project_json_merge_request_path(project, merge_request, 'json', params)
   end

app/services/merge_requests/reload_merge_head_diff_service.rb

@@ -9,7 +9,6 @@ module MergeRequests
     end
 
     def execute
-      return error("default_merge_ref_for_diffs feature flag is disabled") unless enabled?
       return error("Merge request has no merge ref head.") unless merge_request.merge_ref_head.present?
 
       error_msg = recreate_merge_head_diff
@@ -23,10 +22,6 @@ module MergeRequests
 
     attr_reader :merge_request
 
-    def enabled?
-      Feature.enabled?(:default_merge_ref_for_diffs, merge_request.project, default_enabled: :yaml)
-    end
-
     def recreate_merge_head_diff
       merge_request.merge_head_diff&.destroy!
 

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

@@ -76,9 +76,7 @@
       = render "projects/merge_requests/tabs/pane", name: "pipelines", id: "pipelines", class: "pipelines" do
         - if @number_of_pipelines.nonzero?
           = render 'projects/commit/pipelines_list', disable_initialization: true, endpoint: pipelines_project_merge_request_path(@project, @merge_request)
-      - params = request.query_parameters
-      - if Feature.enabled?(:default_merge_ref_for_diffs, @project, default_enabled: :yaml)
-        - params = params.merge(diff_head: true)
+      - params = request.query_parameters.merge(diff_head: true)
       = render "projects/merge_requests/tabs/pane", name: "diffs", id: "js-diffs-app", class: "diffs", data: diffs_tab_pane_data(@project, @merge_request, params)
 
     .mr-loading-status

config/feature_flags/development/default_merge_ref_for_diffs.yml

----
-name: default_merge_ref_for_diffs
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/34472
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/276917
-milestone: '13.4'
-type: development
-group: group::code review
-default_enabled: true

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

@@ -220,29 +220,6 @@ RSpec.describe Projects::MergeRequests::DiffsController do
       end
     end
 
-    context "with the :default_merge_ref_for_diffs flag on" do
-      let(:diffable_merge_ref) { true }
-
-      subject do
-        go(diff_head: true,
-           diff_id: merge_request.merge_request_diff.id,
-           start_sha: merge_request.merge_request_diff.start_commit_sha)
-      end
-
-      it "correctly generates the right diff between versions" do
-        MergeRequests::MergeToRefService.new(project: project, current_user: merge_request.author).execute(merge_request)
-
-        expect_next_instance_of(CompareService) do |service|
-          expect(service).to receive(:execute).with(
-            project,
-            merge_request.merge_request_diff.head_commit_sha,
-            straight: true)
-        end
-
-        subject
-      end
-    end
-
     context 'with diff_head param passed' do
       before do
         allow(merge_request).to receive(:diffable_merge_ref?)
@@ -259,6 +236,23 @@ RSpec.describe Projects::MergeRequests::DiffsController do
 
           expect(response).to have_gitlab_http_status(:ok)
         end
+
+        context 'when diff_id and start_sha are set' do
+          it 'correctly generates the right diff between versions' do
+            MergeRequests::MergeToRefService.new(project: project, current_user: merge_request.author).execute(merge_request)
+
+            expect_next_instance_of(CompareService) do |service|
+              expect(service).to receive(:execute).with(
+                project,
+                merge_request.merge_request_diff.head_commit_sha,
+                straight: true)
+            end
+
+            go(diff_head: true,
+               diff_id: merge_request.merge_request_diff.id,
+               start_sha: merge_request.merge_request_diff.start_commit_sha)
+          end
+        end
       end
 
       context 'the merge request cannot be compared with head' do

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -68,6 +68,18 @@ RSpec.describe Projects::MergeRequestsController do
     end
 
     describe 'as html' do
+      it 'sets the endpoint_metadata_url' do
+        go
+
+        expect(assigns["endpoint_metadata_url"]).to eq(
+          diffs_metadata_project_json_merge_request_path(
+            project,
+            merge_request,
+            'json',
+            diff_head: true,
+            view: 'inline'))
+      end
+
       context 'when diff files were cleaned' do
         render_views
 
@@ -85,23 +97,6 @@ RSpec.describe Projects::MergeRequestsController do
         end
       end
 
-      context 'with `default_merge_ref_for_diffs` feature flag enabled' do
-        before do
-          stub_feature_flags(default_merge_ref_for_diffs: true)
-          go
-        end
-
-        it 'adds the diff_head parameter' do
-          expect(assigns["endpoint_metadata_url"]).to eq(
-            diffs_metadata_project_json_merge_request_path(
-              project,
-              merge_request,
-              'json',
-              diff_head: true,
-              view: 'inline'))
-        end
-      end
-
       context 'when diff is missing' do
         render_views
 

spec/frontend/diffs/store/getters_versions_dropdowns_spec.js

@@ -79,27 +79,21 @@ describe('Compare diff version dropdowns', () => {
       };
     };
 
-    const assertVersions = (targetVersions) => {
-      // base and head should be the last two versions in that order
-      const targetBaseVersion = targetVersions[targetVersions.length - 2];
-      const targetHeadVersion = targetVersions[targetVersions.length - 1];
+    const assertVersions = (targetVersions, checkBaseVersion) => {
+      const targetLatestVersion = targetVersions[targetVersions.length - 1];
       expect(targetVersions[0]).toEqual(expectedFirstVersion);
-      expect(targetBaseVersion).toEqual(expectedBaseVersion);
-      expect(targetHeadVersion).toEqual(expectedHeadVersion);
+
+      if (checkBaseVersion) {
+        expect(targetLatestVersion).toEqual(expectedBaseVersion);
+      } else {
+        expect(targetLatestVersion).toEqual(expectedHeadVersion);
+      }
     };
 
     afterEach(() => {
       setWindowLocation(originalLocation);
     });
 
-    it('base version selected', () => {
-      setupTest();
-      expectedBaseVersion.selected = true;
-
-      const targetVersions = getters.diffCompareDropdownTargetVersions(localState, getters);
-      assertVersions(targetVersions);
-    });
-
     it('head version selected', () => {
       setupTest(true);
 
@@ -126,6 +120,21 @@ describe('Compare diff version dropdowns', () => {
       });
       assertVersions(targetVersions);
     });
+
+    describe('when state.mergeRequestDiff.head_version_path is null', () => {
+      beforeEach(() => {
+        localState.mergeRequestDiff.head_version_path = null;
+      });
+
+      it('base version selected', () => {
+        setupTest(true);
+
+        expectedBaseVersion.selected = true;
+
+        const targetVersions = getters.diffCompareDropdownTargetVersions(localState, getters);
+        assertVersions(targetVersions, true);
+      });
+    });
   });
 
   it('diffCompareDropdownSourceVersions', () => {

spec/services/merge_requests/reload_merge_head_diff_service_spec.rb

@@ -47,15 +47,5 @@ RSpec.describe MergeRequests::ReloadMergeHeadDiffService do
         expect(merge_request.reload.merge_head_diff).not_to eq(existing_merge_head_diff)
       end
     end
-
-    context 'when default_merge_ref_for_diffs feature flag is disabled' do
-      before do
-        stub_feature_flags(default_merge_ref_for_diffs: false)
-      end
-
-      it 'returns error' do
-        expect(subject[:status]).to eq(:error)
-      end
-    end
   end
 end