Commit afcfaa8c authored by Vasilii Iakliushin's avatar Vasilii Iakliushin

Remove "show_relevant_approval_rule_approvers" feature flag

Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/329153

* Remove if condition for feature flag
* Update tests to verify new behavior

Changelog: other
parent 55d31b13
......@@ -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]
......
---
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
......@@ -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) {
......
......@@ -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) }
......
......@@ -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)
......
......@@ -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' }
......
......@@ -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,
});
});
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment