Merge branch 'id-remove-merge-ref-head-comments-ff' into 'master'

Remove merge_ref_head_comments FF

See merge request gitlab-org/gitlab!44433

app/assets/javascripts/diffs/components/diff_row_utils.js

-import { getParameterByName, parseBoolean } from '~/lib/utils/common_utils';
 import { __ } from '~/locale';
 import {
   MATCH_LINE_TYPE,
@@ -23,21 +22,8 @@ export const isMatchLine = type => type === MATCH_LINE_TYPE;
 export const isMetaLine = type =>
   [OLD_NO_NEW_LINE_TYPE, NEW_NO_NEW_LINE_TYPE, EMPTY_CELL_TYPE].includes(type);
 
-export const shouldRenderCommentButton = (
-  isLoggedIn,
-  isCommentButtonRendered,
-  featureMergeRefHeadComments = false,
-) => {
-  if (!isCommentButtonRendered) {
-    return false;
-  }
-
-  if (isLoggedIn) {
-    const isDiffHead = parseBoolean(getParameterByName('diff_head'));
-    return !isDiffHead || featureMergeRefHeadComments;
-  }
-
-  return false;
+export const shouldRenderCommentButton = (isLoggedIn, isCommentButtonRendered) => {
+  return isCommentButtonRendered && isLoggedIn;
 };
 
 export const hasDiscussions = line => line?.discussions?.length > 0;

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

@@ -81,11 +81,7 @@ export default {
       return utils.addCommentTooltip(this.line);
     },
     shouldRenderCommentButton() {
-      return utils.shouldRenderCommentButton(
-        this.isLoggedIn,
-        true,
-        gon.features?.mergeRefHeadComments,
-      );
+      return utils.shouldRenderCommentButton(this.isLoggedIn, true);
     },
     shouldShowCommentButton() {
       return utils.shouldShowCommentButton(

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

@@ -102,11 +102,7 @@ export default {
       return utils.addCommentTooltip(this.line.right);
     },
     shouldRenderCommentButton() {
-      return utils.shouldRenderCommentButton(
-        this.isLoggedIn,
-        this.isCommentButtonRendered,
-        gon.features?.mergeRefHeadComments,
-      );
+      return utils.shouldRenderCommentButton(this.isLoggedIn, this.isCommentButtonRendered);
     },
     shouldShowCommentButtonLeft() {
       return utils.shouldShowCommentButton(

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -173,7 +173,6 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   end
 
   def update_diff_discussion_positions!
-    return unless Feature.enabled?(:merge_ref_head_comments, @merge_request.target_project, default_enabled: true)
     return unless Feature.enabled?(:merge_red_head_comments_position_on_demand, @merge_request.target_project, default_enabled: true)
     return if @merge_request.has_any_diff_note_positions?
 

app/controllers/projects/merge_requests_controller.rb

@@ -29,7 +29,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   before_action only: [:show] do
     push_frontend_experiment(:suggest_pipeline)
     push_frontend_feature_flag(:widget_visibility_polling, @project, default_enabled: true)
-    push_frontend_feature_flag(:merge_ref_head_comments, @project, default_enabled: true)
     push_frontend_feature_flag(:mr_commit_neighbor_nav, @project, default_enabled: true)
     push_frontend_feature_flag(:multiline_comments, @project, default_enabled: true)
     push_frontend_feature_flag(:file_identifier_hash)

app/serializers/discussion_entity.rb

@@ -69,9 +69,6 @@ class DiscussionEntity < Grape::Entity
   end
 
   def display_merge_ref_discussions?(discussion)
-    return unless discussion.diff_discussion?
-    return if discussion.legacy_diff_discussion?
-
-    Feature.enabled?(:merge_ref_head_comments, discussion.project, default_enabled: true)
+    discussion.diff_discussion? && !discussion.legacy_diff_discussion?
   end
 end

app/services/merge_requests/mergeability_check_service.rb

@@ -125,8 +125,6 @@ module MergeRequests
     end
 
     def update_diff_discussion_positions!
-      return if Feature.disabled?(:merge_ref_head_comments, merge_request.target_project, default_enabled: true)
-
       Discussions::CaptureDiffNotePositionsService.new(merge_request).execute
     end
 

app/services/notes/create_service.rb

@@ -70,7 +70,7 @@ module Notes
         Gitlab::Tracking.event('Notes::CreateService', 'execute', tracking_data_for(note))
       end
 
-      if Feature.enabled?(:merge_ref_head_comments, project, default_enabled: true) && note.for_merge_request? && note.diff_note? && note.start_of_discussion?
+      if note.for_merge_request? && note.diff_note? && note.start_of_discussion?
         Discussions::CaptureDiffNotePositionService.new(note.noteable, note.diff_file&.paths).execute(note.discussion)
       end
     end

config/feature_flags/development/merge_ref_head_comments.yml

----
-name: merge_ref_head_comments
-introduced_by_url: 
-rollout_issue_url: 
-group: 
-type: development
-default_enabled: true

spec/frontend/diffs/components/inline_diff_table_row_spec.js

 import { shallowMount } from '@vue/test-utils';
-import { TEST_HOST } from 'helpers/test_constants';
 import { createStore } from '~/mr_notes/stores';
 import InlineDiffTableRow from '~/diffs/components/inline_diff_table_row.vue';
 import DiffGutterAvatars from '~/diffs/components/diff_gutter_avatars.vue';
@@ -28,13 +27,6 @@ describe('InlineDiffTableRow', () => {
     });
   };
 
-  const setWindowLocation = value => {
-    Object.defineProperty(window, 'location', {
-      writable: true,
-      value,
-    });
-  };
-
   beforeEach(() => {
     store = createStore();
     store.state.notes.userData = TEST_USER;
@@ -122,22 +114,15 @@ describe('InlineDiffTableRow', () => {
       const findNoteButton = () => wrapper.find({ ref: 'addDiffNoteButton' });
 
       it.each`
-        userData     | query                | mergeRefHeadComments | expectation
-        ${TEST_USER} | ${'diff_head=false'} | ${false}             | ${true}
-        ${TEST_USER} | ${'diff_head=true'}  | ${true}              | ${true}
-        ${TEST_USER} | ${'diff_head=true'}  | ${false}             | ${false}
-        ${null}      | ${''}                | ${true}              | ${false}
-      `(
-        'exists is $expectation - with userData ($userData) query ($query)',
-        ({ userData, query, mergeRefHeadComments, expectation }) => {
-          store.state.notes.userData = userData;
-          gon.features = { mergeRefHeadComments };
-          setWindowLocation({ href: `${TEST_HOST}?${query}` });
-          createComponent({}, store);
-
-          expect(findNoteButton().exists()).toBe(expectation);
-        },
-      );
+        userData     | expectation
+        ${TEST_USER} | ${true}
+        ${null}      | ${false}
+      `('exists is $expectation - with userData ($userData)', ({ userData, expectation }) => {
+        store.state.notes.userData = userData;
+        createComponent({}, store);
+
+        expect(findNoteButton().exists()).toBe(expectation);
+      });
 
       it.each`
         isHover  | line                                                       | expectation

spec/frontend/diffs/components/parallel_diff_table_row_spec.js

 import Vue from 'vue';
 import { shallowMount } from '@vue/test-utils';
 import { createComponentWithStore } from 'helpers/vue_mount_component_helper';
-import { TEST_HOST } from 'helpers/test_constants';
 import { createStore } from '~/mr_notes/stores';
 import ParallelDiffTableRow from '~/diffs/components/parallel_diff_table_row.vue';
 import diffFileMockData from '../mock_data/diff_file';
@@ -186,13 +185,6 @@ describe('ParallelDiffTableRow', () => {
       });
     };
 
-    const setWindowLocation = value => {
-      Object.defineProperty(window, 'location', {
-        writable: true,
-        value,
-      });
-    };
-
     beforeEach(() => {
       // eslint-disable-next-line prefer-destructuring
       thisLine = diffFileMockData.parallel_diff_lines[2];
@@ -228,19 +220,15 @@ describe('ParallelDiffTableRow', () => {
       const findNoteButton = () => wrapper.find({ ref: 'addDiffNoteButtonLeft' });
 
       it.each`
-        hover    | line                        | userData     | query                | mergeRefHeadComments | expectation
-        ${true}  | ${{}}                       | ${TEST_USER} | ${'diff_head=false'} | ${false}             | ${true}
-        ${true}  | ${{ line: { left: null } }} | ${TEST_USER} | ${'diff_head=false'} | ${false}             | ${false}
-        ${true}  | ${{}}                       | ${TEST_USER} | ${'diff_head=true'}  | ${true}              | ${true}
-        ${true}  | ${{}}                       | ${TEST_USER} | ${'diff_head=true'}  | ${false}             | ${false}
-        ${true}  | ${{}}                       | ${null}      | ${''}                | ${true}              | ${false}
-        ${false} | ${{}}                       | ${TEST_USER} | ${'diff_head=false'} | ${false}             | ${false}
+        hover    | line                        | userData     | expectation
+        ${true}  | ${{}}                       | ${TEST_USER} | ${true}
+        ${true}  | ${{ line: { left: null } }} | ${TEST_USER} | ${false}
+        ${true}  | ${{}}                       | ${null}      | ${false}
+        ${false} | ${{}}                       | ${TEST_USER} | ${false}
       `(
-        'exists is $expectation - with userData ($userData) query ($query)',
-        async ({ hover, line, userData, query, mergeRefHeadComments, expectation }) => {
+        'exists is $expectation - with userData ($userData)',
+        async ({ hover, line, userData, expectation }) => {
           store.state.notes.userData = userData;
-          gon.features = { mergeRefHeadComments };
-          setWindowLocation({ href: `${TEST_HOST}?${query}` });
           createComponent(line, store);
           if (hover) await wrapper.find('.line_holder').trigger('mouseover');
 

spec/serializers/discussion_entity_spec.rb

@@ -79,13 +79,5 @@ RSpec.describe DiscussionEntity do
         :active
       )
     end
-
-    context 'diff_head_compare feature is disabled' do
-      it 'does not expose positions and line_codes attributes' do
-        stub_feature_flags(merge_ref_head_comments: false)
-
-        expect(subject.keys).not_to include(:positions, :line_codes)
-      end
-    end
   end
 end

spec/services/merge_requests/mergeability_check_service_spec.rb

@@ -41,16 +41,6 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar
       subject
     end
 
-    context 'when merge_ref_head_comments is disabled' do
-      it 'does not update diff discussion positions' do
-        stub_feature_flags(merge_ref_head_comments: false)
-
-        expect(Discussions::CaptureDiffNotePositionsService).not_to receive(:new)
-
-        subject
-      end
-    end
-
     it 'updates the merge ref' do
       expect { subject }.to change(merge_request, :merge_ref_head).from(nil)
     end

spec/services/notes/create_service_spec.rb

@@ -163,14 +163,6 @@ RSpec.describe Notes::CreateService do
           expect(note.note_diff_file).to be_present
           expect(note.diff_note_positions).to be_present
         end
-
-        it 'does not create diff positions merge_ref_head_comments is disabled' do
-          stub_feature_flags(merge_ref_head_comments: false)
-
-          expect(Discussions::CaptureDiffNotePositionService).not_to receive(:new)
-
-          described_class.new(project_with_repo, user, new_opts).execute
-        end
       end
 
       context 'when DiffNote is a reply' do