Merge branch '329153_remove_show_relevant_approval_rule_approvers_ff' into 'master'

Remove "show_relevant_approval_rule_approvers" feature flag

See merge request gitlab-org/gitlab!77227

app/controllers/projects/merge_requests_controller.rb

@@ -49,10 +49,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     push_frontend_feature_flag(:diff_searching_usage_data, @project, default_enabled: :yaml)
   end
 
-  before_action do
-    push_frontend_feature_flag(:show_relevant_approval_rule_approvers, @project, default_enabled: :yaml)
-  end
-
   around_action :allow_gitaly_ref_name_caching, only: [:index, :show, :discussions]
 
   after_action :log_merge_request_show, only: [:show]

config/feature_flags/development/show_relevant_approval_rule_approvers.yml

----
-name: show_relevant_approval_rule_approvers
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60339
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/329153
-milestone: '13.12'
-type: development
-group: group::source code
-default_enabled: true

ee/app/assets/javascripts/approvals/components/approvers_select.vue

@@ -80,11 +80,6 @@ export default {
       default: false,
     },
   },
-  computed: {
-    isFeatureEnabled() {
-      return Boolean(gon.features?.showRelevantApprovalRuleApprovers);
-    },
-  },
   watch: {
     value(val) {
       if (val.length > 0) {
@@ -140,26 +135,15 @@ export default {
         .then((results) => ({ results }));
     },
     fetchGroups(term) {
-      if (this.isFeatureEnabled) {
-        const hasTerm = term.trim().length > 0;
-        const DEVELOPER_ACCESS_LEVEL = 30;
-
-        return Api.projectGroups(this.projectId, {
-          skip_groups: this.skipGroupIds,
-          ...(hasTerm ? { search: term } : {}),
-          with_shared: true,
-          shared_visible_only: true,
-          shared_min_access_level: DEVELOPER_ACCESS_LEVEL,
-        });
-      }
-
-      // Don't includeAll when search is empty. Otherwise, the user could get a lot of garbage choices.
-      // https://gitlab.com/gitlab-org/gitlab/issues/11566
-      const includeAll = term.trim().length > 0;
+      const hasTerm = term.trim().length > 0;
+      const DEVELOPER_ACCESS_LEVEL = 30;
 
-      return Api.groups(term, {
+      return Api.projectGroups(this.projectId, {
         skip_groups: this.skipGroupIds,
-        all_available: includeAll,
+        ...(hasTerm ? { search: term } : {}),
+        with_shared: true,
+        shared_visible_only: true,
+        shared_min_access_level: DEVELOPER_ACCESS_LEVEL,
       });
     },
     fetchUsers(term) {

ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb

@@ -74,30 +74,6 @@ RSpec.describe 'Merge request > User edits MR with approval rules', :js do
     expect(page_rule_names.last).to have_text(rule_name)
   end
 
-  context 'with show_relevant_approval_rule_approvers feature flag disabled' do
-    before do
-      stub_feature_flags(show_relevant_approval_rule_approvers: false)
-    end
-
-    context "with public group" do
-      let(:group) { create(:group, :public) }
-
-      before do
-        group.add_developer create(:user)
-
-        click_button 'Approval rules'
-        click_button "Add approval rule"
-      end
-
-      it "with empty search, does not show public group" do
-        open_select2 members_selector
-        wait_for_requests
-
-        expect(page).not_to have_selector('.select2-result-label .group-result', text: group.name)
-      end
-    end
-  end
-
   context 'with public group' do
     let(:group) { create(:group, :public) }
 

ee/spec/features/merge_request/user_sets_approvers_spec.rb

@@ -55,20 +55,21 @@ RSpec.describe 'Merge request > User sets approvers', :js do
   end
 
   context "Group approvers" do
+    let(:project) { create(:project, :public, :repository, group: group) }
+    let(:group) { create(:group) }
+
     context 'when creating an MR' do
       let(:other_user) { create(:user) }
 
       before do
         project.add_developer(user)
         project.add_developer(other_user)
+        group.add_developer(other_user)
 
         sign_in(user)
       end
 
       it 'allows setting groups as approvers' do
-        group = create :group
-        group.add_developer(other_user)
-
         visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature' })
 
         open_modal(text: 'Add approval rule')
@@ -99,8 +100,6 @@ RSpec.describe 'Merge request > User sets approvers', :js do
       it 'allows delete approvers group when it is set in project' do
         approver = create :user
         project.add_developer(approver)
-        group = create :group
-        group.add_developer(other_user)
         group.add_developer(approver)
         create :approval_project_rule, project: project, users: [approver], groups: [group], approvals_required: 1
 
@@ -134,42 +133,6 @@ RSpec.describe 'Merge request > User sets approvers', :js do
         sign_in(user)
       end
 
-      context 'with show_relevant_approval_rule_approvers feature flag disabled' do
-        before do
-          stub_feature_flags(show_relevant_approval_rule_approvers: false)
-        end
-
-        it 'allows setting groups as approvers' do
-          group = create :group
-          group.add_developer(other_user)
-
-          visit edit_project_merge_request_path(project, merge_request)
-
-          open_modal(text: 'Add approval rule')
-          open_approver_select
-
-          expect(find('.select2-results')).not_to have_content(group.name)
-
-          close_approver_select
-          group.add_developer(user) # only display groups that user has access to
-          open_approver_select
-
-          expect(find('.select2-results')).to have_content(group.name)
-
-          find('.select2-results .user-result', text: group.name).click
-          close_approver_select
-          within('.modal-content') do
-            click_button 'Add approval rule'
-          end
-
-          click_on("Save changes")
-          wait_for_all_requests
-
-          expect(page).to have_content("Requires 1 approval from eligible users.")
-          expect(page).to have_selector("img[alt='#{other_user.name}']")
-        end
-      end
-
       it 'allows setting groups as approvers when there is possible group approvers' do
         group = create :group
         group_project = create(:project, :public, :repository, namespace: group)

ee/spec/features/projects/settings/merge_request_approvals_settings_spec.rb

@@ -6,8 +6,8 @@ RSpec.describe 'Project settings > [EE] Merge Request Approvals', :js do
   include FeatureApprovalHelper
 
   let_it_be(:user) { create(:user) }
-  let_it_be(:project) { create(:project) }
   let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, namespace: group) }
   let_it_be(:group_member) { create(:user) }
   let_it_be(:non_member) { create(:user) }
   let_it_be(:config_selector) { '.js-approval-rules' }

ee/spec/frontend/approvals/components/approvers_select_spec.js

@@ -73,7 +73,7 @@ describe('Approvals ApproversSelect', () => {
   };
 
   beforeEach(() => {
-    jest.spyOn(Api, 'groups').mockResolvedValue(TEST_GROUPS);
+    jest.spyOn(Api, 'projectGroups').mockResolvedValue(TEST_GROUPS);
     jest.spyOn(Api, 'projectUsers').mockReturnValue(Promise.resolve(TEST_USERS));
   });
 
@@ -123,7 +123,13 @@ describe('Approvals ApproversSelect', () => {
     });
 
     it('fetches all available groups', () => {
-      expect(Api.groups).toHaveBeenCalledWith(term, { skip_groups: [], all_available: true });
+      expect(Api.projectGroups).toHaveBeenCalledWith(TEST_PROJECT_ID, {
+        skip_groups: [],
+        search: term,
+        with_shared: true,
+        shared_visible_only: true,
+        shared_min_access_level: 30,
+      });
     });
 
     it('fetches users', () => {
@@ -154,9 +160,11 @@ describe('Approvals ApproversSelect', () => {
     });
 
     it('skips groups and does not fetch all available', () => {
-      expect(Api.groups).toHaveBeenCalledWith('', {
+      expect(Api.projectGroups).toHaveBeenCalledWith(TEST_PROJECT_ID, {
+        shared_min_access_level: 30,
+        shared_visible_only: true,
         skip_groups: skipGroupIds,
-        all_available: false,
+        with_shared: true,
       });
     });