From 4fe04456a99c507055fd732a3e40a6197371efc6 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Mon, 18 Sep 2023 16:15:05 +0200 Subject: [PATCH] Add a feature flag around the code with poor performance Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/425166 **Problem** https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130933 has introduced a performance problem. **Solution** Mitigate the problem by adding a feature flag around new code in disabled state. That will allow us to investigate the problem without affecting customers. EE: true --- .../project_templates_without_min_access.yml | 8 +++ ee/app/helpers/ee/projects_helper.rb | 18 +++-- ee/app/models/ee/user.rb | 4 +- .../users/available_group_templates.html.haml | 2 +- ee/spec/helpers/projects_helper_spec.rb | 20 ++++++ ee/spec/models/ee/user_spec.rb | 71 ++++++++++++++++--- 6 files changed, 106 insertions(+), 17 deletions(-) create mode 100644 config/feature_flags/development/project_templates_without_min_access.yml diff --git a/config/feature_flags/development/project_templates_without_min_access.yml b/config/feature_flags/development/project_templates_without_min_access.yml new file mode 100644 index 00000000000000..5de9ba18191dd2 --- /dev/null +++ b/config/feature_flags/development/project_templates_without_min_access.yml @@ -0,0 +1,8 @@ +--- +name: project_templates_without_min_access +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132025 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/425452 +milestone: '16.5' +type: development +group: group::source code +default_enabled: false diff --git a/ee/app/helpers/ee/projects_helper.rb b/ee/app/helpers/ee/projects_helper.rb index 9698c850514883..c909543ca838fa 100644 --- a/ee/app/helpers/ee/projects_helper.rb +++ b/ee/app/helpers/ee/projects_helper.rb @@ -167,14 +167,22 @@ def membership_locked? end def group_project_templates_count(group_id) - projects_not_aimed_for_deletions_for(group_id).count do |project| - can?(current_user, :download_code, project) + if ::Feature.enabled?(:project_templates_without_min_access, current_user) + projects_not_aimed_for_deletions_for(group_id).count do |project| + can?(current_user, :download_code, project) + end + else + projects_not_aimed_for_deletions_for(group_id).count end end - def group_project_templates(group_id) - projects_not_aimed_for_deletions_for(group_id).select do |project| - can?(current_user, :download_code, project) + def group_project_templates(group) + if ::Feature.enabled?(:project_templates_without_min_access, current_user) + projects_not_aimed_for_deletions_for(group.id).select do |project| + can?(current_user, :download_code, project) + end + else + group.projects.not_aimed_for_deletion end end diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index 0032c3761b6b2a..ceee5bda8a0c11 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -296,7 +296,9 @@ def search_membership_ancestry def available_subgroups_with_custom_project_templates(group_id = nil) found_groups = GroupsWithTemplatesFinder.new(self, group_id).execute - ::GroupsFinder.new(self) + params = ::Feature.enabled?(:project_templates_without_min_access, self) ? {} : { min_access_level: ::Gitlab::Access::DEVELOPER } + + ::GroupsFinder.new(self, params) .execute .where(id: found_groups.select(:custom_project_templates_group_id)) .preload(:projects) diff --git a/ee/app/views/users/available_group_templates.html.haml b/ee/app/views/users/available_group_templates.html.haml index 94e818252f5b75..4f08d6e56c5948 100644 --- a/ee/app/views/users/available_group_templates.html.haml +++ b/ee/app/views/users/available_group_templates.html.haml @@ -1,7 +1,7 @@ .custom-project-templates - if @groups_with_project_templates.present? - @groups_with_project_templates.each do |group| - - projects = group_project_templates(group.id) + - projects = group_project_templates(group) .template-group-options.js-template-group-options{ class: ('expanded border-top-0' if @groups_with_project_templates.first == group) } .template-header.d-flex.align-items-center .template-subgroup.d-flex.flex-fill.align-items-center diff --git a/ee/spec/helpers/projects_helper_spec.rb b/ee/spec/helpers/projects_helper_spec.rb index 70dce29dbe35c3..e127ecae248584 100644 --- a/ee/spec/helpers/projects_helper_spec.rb +++ b/ee/spec/helpers/projects_helper_spec.rb @@ -172,6 +172,16 @@ specify do expect(helper.group_project_templates_count(parent_group.id)).to eq 0 end + + context 'when feature flag "project_templates_without_min_access" is disabled' do + before do + stub_feature_flags(project_templates_without_min_access: false) + end + + specify do + expect(helper.group_project_templates_count(parent_group.id)).to eq 1 + end + end end context 'when there are multiple groups' do @@ -234,6 +244,16 @@ specify do expect(helper.group_project_templates(parent_group)).to be_empty end + + context 'when feature flag "project_templates_without_min_access" is disabled' do + before do + stub_feature_flags(project_templates_without_min_access: false) + end + + specify do + expect(helper.group_project_templates(parent_group)).to be_empty + end + end end end diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index 3243dc335cecaa..94bff23b6c5c1f 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -765,25 +765,76 @@ it 'only templates in publicly visible groups with projects are available' do expect(available_subgroups).to match_array([subgroup_1, subsubgroup_1, subsubgroup_4]) end + + context 'when feature flag "project_templates_without_min_access" is disabled' do + before do + stub_feature_flags(project_templates_without_min_access: false) + end + + it 'returns an empty collection' do + expect(available_subgroups).to be_empty + end + end end context 'when a user is a member of the groups' do subject(:available_subgroups) { user.available_subgroups_with_custom_project_templates } - where(:access_level) do - [:guest, :reporter, :developer, :maintainer, :owner] + context 'when the access level is not sufficient' do + where(:access_level) do + [:guest, :reporter] + end + + with_them do + before do + group_1.add_member(user, access_level) + group_2.add_member(user, access_level) + group_3.add_member(user, access_level) + group_4.add_member(user, access_level) + end + + it 'the templates in groups with projects are available' do + expect(available_subgroups).to match_array([subgroup_1, subgroup_2, subsubgroup_1, subsubgroup_4]) + end + + context 'when feature flag "project_templates_without_min_access" is disabled' do + before do + stub_feature_flags(project_templates_without_min_access: false) + end + + it 'returns an empty collection' do + expect(available_subgroups).to be_empty + end + end + end end - with_them do - before do - group_1.add_member(user, access_level) - group_2.add_member(user, access_level) - group_3.add_member(user, access_level) - group_4.add_member(user, access_level) + context 'when the access level is enough' do + where(:access_level) do + [:developer, :maintainer, :owner] end - it 'the templates in groups with projects are available' do - expect(available_subgroups).to match_array([subgroup_1, subgroup_2, subsubgroup_1, subsubgroup_4]) + with_them do + before do + group_1.add_member(user, access_level) + group_2.add_member(user, access_level) + group_3.add_member(user, access_level) + group_4.add_member(user, access_level) + end + + it 'the templates in groups with projects are available' do + expect(available_subgroups).to match_array([subgroup_1, subgroup_2, subsubgroup_1, subsubgroup_4]) + end + + context 'when feature flag "project_templates_without_min_access" is disabled' do + before do + stub_feature_flags(project_templates_without_min_access: false) + end + + it 'the templates in groups with projects are available' do + expect(available_subgroups).to match_array([subgroup_1, subgroup_2, subsubgroup_1, subsubgroup_4]) + end + end end end end -- GitLab