From 0e243fc55e4a6e5859d3d226d2980976e90aaf3f Mon Sep 17 00:00:00 2001 From: Manoj M J Date: Fri, 3 Mar 2023 12:36:18 +0530 Subject: [PATCH] BE changed to always perform delayed deletion BE changed to always perform delayed deletion --- .../always_perform_delayed_deletion.yml | 8 +++ .../admin/application_settings_controller.rb | 8 ++- .../helpers/ee/application_settings_helper.rb | 13 ++++- ee/app/models/ee/group.rb | 8 ++- ee/app/models/ee/project.rb | 1 + ee/lib/ee/api/entities/application_setting.rb | 4 +- .../application_settings_controller_spec.rb | 26 ++++++---- .../controllers/projects_controller_spec.rb | 12 ++++- .../features/groups/group_settings_spec.rb | 1 + ee/spec/features/projects_spec.rb | 15 ++++-- ee/spec/helpers/ee/groups_helper_spec.rb | 12 ++++- ee/spec/models/ee/group_spec.rb | 15 ++++-- ee/spec/models/ee/project_spec.rb | 44 ++++++++++------ ee/spec/requests/api/groups_spec.rb | 12 ++++- ee/spec/requests/api/projects_spec.rb | 24 ++++++++- ee/spec/requests/api/settings_spec.rb | 16 +++++- ...tive_projects_deletion_cron_worker_spec.rb | 52 ++++++++++++++----- 17 files changed, 210 insertions(+), 61 deletions(-) create mode 100644 config/feature_flags/development/always_perform_delayed_deletion.yml diff --git a/config/feature_flags/development/always_perform_delayed_deletion.yml b/config/feature_flags/development/always_perform_delayed_deletion.yml new file mode 100644 index 00000000000000..6708b5b9f90483 --- /dev/null +++ b/config/feature_flags/development/always_perform_delayed_deletion.yml @@ -0,0 +1,8 @@ +--- +name: always_perform_delayed_deletion +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113332 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/393622 +milestone: '15.10' +type: development +group: group::tenant scale +default_enabled: false diff --git a/ee/app/controllers/ee/admin/application_settings_controller.rb b/ee/app/controllers/ee/admin/application_settings_controller.rb index da5e5d26532f87..7231ca2f80c640 100644 --- a/ee/app/controllers/ee/admin/application_settings_controller.rb +++ b/ee/app/controllers/ee/admin/application_settings_controller.rb @@ -75,14 +75,18 @@ def visible_application_setting_attributes attrs += EE::ApplicationSettingsHelper.repository_mirror_attributes end + if License.feature_available?(:adjourned_deletion_for_projects_and_groups) && + ::Feature.disabled?(:always_perform_delayed_deletion) + attrs += EE::ApplicationSettingsHelper.delayed_deletion_attributes + end + # License feature => attribute name { custom_project_templates: :custom_project_templates_group_id, email_additional_text: :email_additional_text, custom_file_templates: :file_template_project_id, default_project_deletion_protection: :default_project_deletion_protection, - adjourned_deletion_for_projects_and_groups: [:delayed_project_deletion, :delayed_group_deletion, - :deletion_adjourned_period], + adjourned_deletion_for_projects_and_groups: :deletion_adjourned_period, required_ci_templates: :required_instance_ci_template, disable_name_update_for_users: :updating_name_disabled_for_users, package_forwarding: [:npm_package_requests_forwarding, diff --git a/ee/app/helpers/ee/application_settings_helper.rb b/ee/app/helpers/ee/application_settings_helper.rb index 7efa688271cddb..3d5e066b28c3a0 100644 --- a/ee/app/helpers/ee/application_settings_helper.rb +++ b/ee/app/helpers/ee/application_settings_helper.rb @@ -121,14 +121,13 @@ def self.possible_licensed_attributes merge_request_appovers_rules_attributes + password_complexity_attributes + git_abuse_rate_limit_attributes + + delayed_deletion_attributes + %i[ email_additional_text file_template_project_id git_two_factor_session_expiry group_owners_can_manage_default_branch_protection default_project_deletion_protection - delayed_project_deletion - delayed_group_deletion disable_personal_access_tokens deletion_adjourned_period updating_name_disabled_for_users @@ -168,6 +167,16 @@ def self.git_abuse_rate_limit_attributes ] end + def self.delayed_deletion_attributes + # TODO: Remove in 16.0, after https://gitlab.com/gitlab-org/gitlab/-/issues/393622 is turned ON + # We cannot add a feature flag check in this file, due to the reason mentioned in + # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92218#note_1026250151 + %i[ + delayed_project_deletion + delayed_group_deletion + ] + end + override :registration_features_can_be_prompted? def registration_features_can_be_prompted? !::Gitlab::CurrentSettings.usage_ping_enabled? && !License.current.present? diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 12d37ad852fb05..d330e6dace7c24 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -525,7 +525,13 @@ def self_or_ancestor_marked_for_deletion def adjourned_deletion? feature_available?(:adjourned_deletion_for_projects_and_groups) && ::Gitlab::CurrentSettings.deletion_adjourned_period > 0 && - ::Gitlab::CurrentSettings.delayed_group_deletion + adjourned_deletion_configured? + end + + def adjourned_deletion_configured? + return true if ::Feature.enabled?(:always_perform_delayed_deletion) + + ::Gitlab::CurrentSettings.delayed_group_deletion end def vulnerabilities diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index 14da3fb3414c87..b4b8fa7c0e2ca4 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -1163,6 +1163,7 @@ def requirements_ci_variables # Return the group's setting for delayed deletion, false for user namespace projects def group_deletion_mode_configured? + return true if ::Feature.enabled?(:always_perform_delayed_deletion) && !personal? return false unless group&.namespace_settings group.namespace_settings.delayed_project_removal? diff --git a/ee/lib/ee/api/entities/application_setting.rb b/ee/lib/ee/api/entities/application_setting.rb index 3f6b73fb367abc..ea172819be0d10 100644 --- a/ee/lib/ee/api/entities/application_setting.rb +++ b/ee/lib/ee/api/entities/application_setting.rb @@ -19,8 +19,8 @@ module ApplicationSetting expose :email_additional_text, if: ->(_instance, _opts) { ::License.feature_available?(:email_additional_text) } expose :file_template_project_id, if: ->(_instance, _opts) { ::License.feature_available?(:custom_file_templates) } expose :default_project_deletion_protection, if: ->(_instance, _opts) { ::License.feature_available?(:default_project_deletion_protection) } - expose :delayed_project_deletion, if: ->(_instance, _opts) { ::License.feature_available?(:adjourned_deletion_for_projects_and_groups) } - expose :delayed_group_deletion, if: ->(_instance, _opts) { ::License.feature_available?(:adjourned_deletion_for_projects_and_groups) } + expose :delayed_project_deletion, if: ->(_instance, _opts) { ::License.feature_available?(:adjourned_deletion_for_projects_and_groups) && ::Feature.disabled?(:always_perform_delayed_deletion) } + expose :delayed_group_deletion, if: ->(_instance, _opts) { ::License.feature_available?(:adjourned_deletion_for_projects_and_groups) && ::Feature.disabled?(:always_perform_delayed_deletion) } expose :deletion_adjourned_period, if: ->(_instance, _opts) { ::License.feature_available?(:adjourned_deletion_for_projects_and_groups) } expose :disable_personal_access_tokens, if: ->(_instance, _opts) { ::License.feature_available?(:disable_personal_access_tokens) } expose :updating_name_disabled_for_users, if: ->(_instance, _opts) { ::License.feature_available?(:disable_name_update_for_users) } diff --git a/ee/spec/controllers/admin/application_settings_controller_spec.rb b/ee/spec/controllers/admin/application_settings_controller_spec.rb index 195c2fc1860947..6bb5013157f5c3 100644 --- a/ee/spec/controllers/admin/application_settings_controller_spec.rb +++ b/ee/spec/controllers/admin/application_settings_controller_spec.rb @@ -86,18 +86,24 @@ it_behaves_like 'settings for licensed features' end - context 'default delayed group deletion' do - let(:settings) { { delayed_group_deletion: true } } - let(:feature) { :adjourned_deletion_for_projects_and_groups } + context 'when `always_perform_delayed_deletion` is disabled' do + before do + stub_feature_flags(always_perform_delayed_deletion: false) + end - it_behaves_like 'settings for licensed features' - end + context 'default delayed group deletion' do + let(:settings) { { delayed_group_deletion: true } } + let(:feature) { :adjourned_deletion_for_projects_and_groups } - context 'default delayed project deletion' do - let(:settings) { { delayed_project_deletion: true } } - let(:feature) { :adjourned_deletion_for_projects_and_groups } + it_behaves_like 'settings for licensed features' + end - it_behaves_like 'settings for licensed features' + context 'default delayed project deletion' do + let(:settings) { { delayed_project_deletion: true } } + let(:feature) { :adjourned_deletion_for_projects_and_groups } + + it_behaves_like 'settings for licensed features' + end end context 'updating name disabled for users setting' do @@ -172,7 +178,7 @@ it_behaves_like 'settings for licensed features' end - context 'project deletion delay' do + context 'deletion adjourned period' do let(:settings) { { deletion_adjourned_period: 6 } } let(:feature) { :adjourned_deletion_for_projects_and_groups } diff --git a/ee/spec/controllers/projects_controller_spec.rb b/ee/spec/controllers/projects_controller_spec.rb index 4807ea4ad6f71e..25a41b8c854f2c 100644 --- a/ee/spec/controllers/projects_controller_spec.rb +++ b/ee/spec/controllers/projects_controller_spec.rb @@ -841,7 +841,17 @@ allow(group.namespace_settings).to receive(:delayed_project_removal?).and_return(false) end - it_behaves_like 'deletes project right away' + context 'when `always_perform_delayed_deletion` is disabled' do + before do + stub_feature_flags(always_perform_delayed_deletion: false) + end + + it_behaves_like 'deletes project right away' + end + + context 'when `always_perform_delayed_deletion` is enabled' do + it_behaves_like 'marks project for deletion' + end end context 'when feature is not available for the project' do diff --git a/ee/spec/features/groups/group_settings_spec.rb b/ee/spec/features/groups/group_settings_spec.rb index 1547f360fe287c..ff05725fd2f5d1 100644 --- a/ee/spec/features/groups/group_settings_spec.rb +++ b/ee/spec/features/groups/group_settings_spec.rb @@ -177,6 +177,7 @@ before do stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + stub_feature_flags(always_perform_delayed_deletion: false) gitlab_enable_admin_mode_sign_in(user) visit general_admin_application_settings_path diff --git a/ee/spec/features/projects_spec.rb b/ee/spec/features/projects_spec.rb index 2c0e449b035376..fb248e38e7b334 100644 --- a/ee/spec/features/projects_spec.rb +++ b/ee/spec/features/projects_spec.rb @@ -35,16 +35,21 @@ def remove_with_confirm(button_text, confirm_with, confirm_button_text = 'Confir let(:project) { create(:project, group: group) } let(:user) { create(:user) } - where(:feature_available_on_instance, :delayed_project_removal, :shows_adjourned_delete) do - true | nil | false - true | true | true - false | true | false - false | nil | false + where(:feature_available_on_instance, :delayed_project_removal, :always_perform_delayed_deletion, :shows_adjourned_delete) do + true | nil | true | true + true | true | true | true + false | true | true | false + false | nil | true | false + true | nil | false | false + true | true | false | true + false | true | false | false + false | nil | false | false end before do stub_application_setting(deletion_adjourned_period: 7) stub_licensed_features(adjourned_deletion_for_projects_and_groups: feature_available_on_instance) + stub_feature_flags(always_perform_delayed_deletion: always_perform_delayed_deletion) group.add_member(user, Gitlab::Access::OWNER) sign_in user diff --git a/ee/spec/helpers/ee/groups_helper_spec.rb b/ee/spec/helpers/ee/groups_helper_spec.rb index 4f12678dc111d9..3519a6a5ec7914 100644 --- a/ee/spec/helpers/ee/groups_helper_spec.rb +++ b/ee/spec/helpers/ee/groups_helper_spec.rb @@ -128,7 +128,17 @@ stub_application_setting(delayed_group_deletion: false) end - it_behaves_like 'permanent deletion message' + context 'when `always_perform_delayed_deletion` is disabled' do + before do + stub_feature_flags(always_perform_delayed_deletion: false) + end + + it_behaves_like 'permanent deletion message' + end + + context 'when `always_perform_delayed_deletion` is enabled' do + it_behaves_like 'delayed deletion message' + end end context 'when group delay deletion is enabled and adjourned deletion period is 0' do diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb index 2a4cbfa7434100..4d4a548f222938 100644 --- a/ee/spec/models/ee/group_spec.rb +++ b/ee/spec/models/ee/group_spec.rb @@ -2057,11 +2057,15 @@ def webhook_headers end context 'delayed deletion feature is available' do - where(:adjourned_period, :delayed_group_deletion, :expected) do - 0 | true | false - 0 | false | false - 1 | true | true - 1 | false | false + where(:adjourned_period, :delayed_group_deletion, :always_perform_delayed_deletion, :expected) do + 0 | true | true | false + 0 | false | true | false + 1 | true | true | true + 1 | false | true | true + 0 | true | false | false + 0 | false | false | false + 1 | true | false | true + 1 | false | false | false end with_them do @@ -2069,6 +2073,7 @@ def webhook_headers stub_licensed_features(adjourned_deletion_for_projects_and_groups: true) stub_application_setting(deletion_adjourned_period: adjourned_period) stub_application_setting(delayed_group_deletion: delayed_group_deletion) + stub_feature_flags(always_perform_delayed_deletion: always_perform_delayed_deletion) end it { is_expected.to expected ? be_truthy : be_falsey } diff --git a/ee/spec/models/ee/project_spec.rb b/ee/spec/models/ee/project_spec.rb index d3dfd8103718d3..00304215def7a0 100644 --- a/ee/spec/models/ee/project_spec.rb +++ b/ee/spec/models/ee/project_spec.rb @@ -3114,15 +3114,23 @@ def stub_default_url_options(host) describe '#adjourned_deletion?' do subject { project.adjourned_deletion? } - where(:licensed?, :feature_enabled_on_group?, :adjourned_period, :result) do - true | true | 0 | false - true | true | 1 | true - true | false | 0 | false - true | false | 1 | false - false | true | 0 | false - false | true | 1 | false - false | false | 0 | false - false | false | 1 | false + where(:licensed?, :feature_enabled_on_group?, :adjourned_period, :always_perform_delayed_deletion, :result) do + true | true | 0 | true | false + true | true | 1 | true | true + true | false | 0 | true | false + true | false | 1 | true | true + false | true | 0 | true | false + false | true | 1 | true | false + false | false | 0 | true | false + false | false | 1 | true | false + true | true | 0 | false | false + true | true | 1 | false | true + true | false | 0 | false | false + true | false | 1 | false | false + false | true | 0 | false | false + false | true | 1 | false | false + false | false | 0 | false | false + false | false | 1 | false | false end with_them do @@ -3133,6 +3141,7 @@ def stub_default_url_options(host) stub_licensed_features(adjourned_deletion_for_projects_and_groups: licensed?) stub_application_setting(deletion_adjourned_period: adjourned_period) allow(group.namespace_settings).to receive(:delayed_project_removal?).and_return(feature_enabled_on_group?) + stub_feature_flags(always_perform_delayed_deletion: always_perform_delayed_deletion) end it { is_expected.to be result } @@ -3156,11 +3165,15 @@ def stub_default_url_options(host) describe '#adjourned_deletion_configured?' do subject { project.adjourned_deletion_configured? } - where(:feature_enabled_on_group?, :adjourned_period, :result) do - true | 0 | false - true | 1 | true - false | 0 | false - false | 1 | false + where(:feature_enabled_on_group?, :adjourned_period, :always_perform_delayed_deletion, :result) do + true | 0 | true | false + true | 1 | true | true + false | 0 | true | false + false | 1 | true | true + true | 0 | false | false + true | 1 | false | true + false | 0 | false | false + false | 1 | false | false end with_them do @@ -3170,6 +3183,7 @@ def stub_default_url_options(host) before do stub_application_setting(deletion_adjourned_period: adjourned_period) allow(group.namespace_settings).to receive(:delayed_project_removal?).and_return(feature_enabled_on_group?) + stub_feature_flags(always_perform_delayed_deletion: always_perform_delayed_deletion) end it { is_expected.to be result } @@ -3185,7 +3199,7 @@ def stub_default_url_options(host) end it 'deletes immediately' do - expect(user_project.adjourned_deletion?).to be_falsey + expect(subject).to eq(false) end end end diff --git a/ee/spec/requests/api/groups_spec.rb b/ee/spec/requests/api/groups_spec.rb index 1cd66394980c8f..c838a607e56c1a 100644 --- a/ee/spec/requests/api/groups_spec.rb +++ b/ee/spec/requests/api/groups_spec.rb @@ -1142,7 +1142,17 @@ stub_application_setting(delayed_group_deletion: false) end - it_behaves_like 'immediately enqueues the job to delete the group' + context 'when `always_perform_delayed_deletion` is disabled' do + before do + stub_feature_flags(always_perform_delayed_deletion: false) + end + + it_behaves_like 'immediately enqueues the job to delete the group' + end + + context 'when `always_perform_delayed_deletion` is enabled' do + it_behaves_like 'marks group for delayed deletion' + end end context 'failure' do diff --git a/ee/spec/requests/api/projects_spec.rb b/ee/spec/requests/api/projects_spec.rb index 4a147e9991589f..38df510a17e284 100644 --- a/ee/spec/requests/api/projects_spec.rb +++ b/ee/spec/requests/api/projects_spec.rb @@ -1662,7 +1662,17 @@ stub_application_setting(lock_delayed_project_removal: true, delayed_project_removal: false) end - it_behaves_like 'deletes project immediately' + context 'when `always_perform_delayed_deletion` is disabled' do + before do + stub_feature_flags(always_perform_delayed_deletion: false) + end + + it_behaves_like 'deletes project immediately' + end + + context 'when `always_perform_delayed_deletion` is enabled' do + it_behaves_like 'marks project for deletion' + end end context 'when deletion adjourned period is 0' do @@ -1675,7 +1685,17 @@ end context 'delayed project deletion is disabled for group' do - it_behaves_like 'deletes project immediately' + context 'when `always_perform_delayed_deletion` is disabled' do + before do + stub_feature_flags(always_perform_delayed_deletion: false) + end + + it_behaves_like 'deletes project immediately' + end + + context 'when `always_perform_delayed_deletion` is enabled' do + it_behaves_like 'marks project for deletion' + end end context 'for projects in user namespace' do diff --git a/ee/spec/requests/api/settings_spec.rb b/ee/spec/requests/api/settings_spec.rb index 1709d70d7b3073..f61f51e99f85cd 100644 --- a/ee/spec/requests/api/settings_spec.rb +++ b/ee/spec/requests/api/settings_spec.rb @@ -211,14 +211,26 @@ let(:settings) { { delayed_group_deletion: true } } let(:feature) { :adjourned_deletion_for_projects_and_groups } - it_behaves_like 'settings for licensed features' + context 'when `always_perform_delayed_deletion` is disabled' do + before do + stub_feature_flags(always_perform_delayed_deletion: false) + end + + it_behaves_like 'settings for licensed features' + end end context 'delayed project deletion' do let(:settings) { { delayed_project_deletion: true } } let(:feature) { :adjourned_deletion_for_projects_and_groups } - it_behaves_like 'settings for licensed features' + context 'when `always_perform_delayed_deletion` is disabled' do + before do + stub_feature_flags(always_perform_delayed_deletion: false) + end + + it_behaves_like 'settings for licensed features' + end end context 'group_owners_can_manage_default_branch_protection setting' do diff --git a/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb b/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb index 2042c9fa93ec2e..5c89dcfef74860 100644 --- a/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb +++ b/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb @@ -107,12 +107,8 @@ stub_licensed_features(adjourned_deletion_for_projects_and_groups: true) end - context 'when adjourned_deletion_configured is not configured for the project' do - before do - group.namespace_settings.update!(delayed_project_removal: false) - end - - it 'invokes Projects::DestroyService if adjourned_deletion_configured not configured for the project' do + shared_examples_for 'invokes Projects::DestroyService' do + it 'invokes Projects::DestroyService' do Gitlab::Redis::SharedState.with do |redis| redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", 15.months.ago.to_date.to_s) @@ -135,12 +131,8 @@ end end - context 'when adjourned_deletion_configured is configured for the project' do - before do - group.namespace_settings.update!(delayed_project_removal: true) - end - - it 'invokes Projects::MarkForDeletionService for projects that are inactive even after being notified' do + shared_examples_for 'invokes Projects::MarkForDeletionService' do + it 'invokes Projects::MarkForDeletionService' do Gitlab::Redis::SharedState.with do |redis| redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", 15.months.ago.to_date.to_s) @@ -161,6 +153,42 @@ end end end + + context 'when adjourned_deletion_configured is not configured for the project' do + before do + group.namespace_settings.update!(delayed_project_removal: false) + end + + context 'when `always_perform_delayed_deletion` is disabled' do + before do + stub_feature_flags(always_perform_delayed_deletion: false) + end + + it_behaves_like 'invokes Projects::DestroyService' + end + + context 'when `always_perform_delayed_deletion` is enabled' do + it_behaves_like 'invokes Projects::MarkForDeletionService' + end + end + + context 'when adjourned_deletion_configured is configured for the project' do + before do + group.namespace_settings.update!(delayed_project_removal: true) + end + + context 'when `always_perform_delayed_deletion` is disabled' do + before do + stub_feature_flags(always_perform_delayed_deletion: false) + end + + it_behaves_like 'invokes Projects::MarkForDeletionService' + end + + context 'when `always_perform_delayed_deletion` is enabled' do + it_behaves_like 'invokes Projects::MarkForDeletionService' + end + end end end end -- GitLab