diff --git a/app/models/group.rb b/app/models/group.rb index a699a997c09d6b8e81995148c82110f20ea4e6a3..8a57c0f4177c0145bc65f02cb67bf037949b47f7 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -1293,6 +1293,26 @@ def pending_delete? deletion_schedule.marked_for_deletion_on.future? end + def unarchive_self_and_descendants! + NamespaceSetting + .where(namespace_id: self_and_descendant_ids, archived: true) + .update_all(archived: false) + end + + def unarchive_descendants! + NamespaceSetting + .where(namespace_id: descendant_ids, archived: true) + .update_all(archived: false) + end + + def unarchive_all_projects! + Project + .joins(:namespace) + .where("namespaces.traversal_ids @> '{?}'", id) + .where(archived: true) + .update_all(archived: false) + end + private def feature_flag_enabled_for_self_or_ancestor?(feature_flag, type: :development) diff --git a/app/models/namespaces/traversal/cached.rb b/app/models/namespaces/traversal/cached.rb index 57bacd483067a02b57cc5531c9e04e7b724941f3..67ca51af1843d77b693bb19d51c9a6ef63d1f971 100644 --- a/app/models/namespaces/traversal/cached.rb +++ b/app/models/namespaces/traversal/cached.rb @@ -28,6 +28,10 @@ def self_and_descendant_ids(skope: self.class) ) end + def descendant_ids(skope: self.class) + self_and_descendant_ids(skope:).id_not_in(id) + end + override :all_project_ids def all_project_ids scope_with_cached_ids( diff --git a/app/services/namespaces/groups/archive_service.rb b/app/services/namespaces/groups/archive_service.rb index 3f051069d2bdd3f6acf9cba64b437f271f0636d6..c6512b30e66b5946a5138117002906b6c195ba2b 100644 --- a/app/services/namespaces/groups/archive_service.rb +++ b/app/services/namespaces/groups/archive_service.rb @@ -28,10 +28,21 @@ def execute return AlreadyArchivedError if group.archived return AncestorAlreadyArchivedError if group.ancestors_archived? - return ArchivingFailedError unless group.namespace_settings.update(archived: true) + + if unarchive_descendants? + group.transaction do + group.namespace_settings.update!(archived: true) + group.unarchive_descendants! + group.unarchive_all_projects! + end + else + group.namespace_settings.update!(archived: true) + end after_archive ServiceResponse.success + rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved + ArchivingFailedError end private @@ -49,6 +60,10 @@ def unlink_project_forks def error_response(message) ServiceResponse.error(message: message) end + + def unarchive_descendants? + Feature.enabled?(:cascade_unarchive_group, group, type: :gitlab_com_derisk) + end end end end diff --git a/app/services/namespaces/groups/unarchive_service.rb b/app/services/namespaces/groups/unarchive_service.rb index e5fe74c1d97c0635e54015280629d5370d275c99..ebea17f229a2ef6aa41e5c6097db00c6310e88b5 100644 --- a/app/services/namespaces/groups/unarchive_service.rb +++ b/app/services/namespaces/groups/unarchive_service.rb @@ -25,10 +25,20 @@ def execute return NotAuthorizedError unless can?(current_user, :archive_group, group) return AncestorArchivedError if group.ancestors_archived? return AlreadyUnarchivedError unless group.archived - return UnarchivingFailedError unless group.namespace_settings.update(archived: false) + + if unarchive_descendants? + group.transaction do + group.unarchive_self_and_descendants! + group.unarchive_all_projects! + end + else + group.namespace_settings.update!(archived: false) + end after_unarchive ServiceResponse.success + rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved + UnarchivingFailedError end private @@ -41,6 +51,10 @@ def after_unarchive def error_response(message) ServiceResponse.error(message: message) end + + def unarchive_descendants? + Feature.enabled?(:cascade_unarchive_group, group, type: :gitlab_com_derisk) + end end end end diff --git a/config/feature_flags/gitlab_com_derisk/cascade_unarchive_group.yml b/config/feature_flags/gitlab_com_derisk/cascade_unarchive_group.yml new file mode 100644 index 0000000000000000000000000000000000000000..43ccf2925d6dc11cc2dab287b1ab1986b8327bc1 --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/cascade_unarchive_group.yml @@ -0,0 +1,11 @@ +--- +name: cascade_unarchive_group +description: Unarchive descendant groups and projects of a group when toggling its archive state +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/578499 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/215688 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/583415 +milestone: '18.7' +observed: true +group: group::organizations +type: gitlab_com_derisk +default_enabled: false diff --git a/lib/api/groups.rb b/lib/api/groups.rb index e8b0c0bdaebcb34c61e37b78f858b22d8e042580..3e650860eba4f8ec52570bcc354b4365b00a9066 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -392,7 +392,7 @@ def check_query_limit; end if response.success? status 200 - present_group_details(params, group, with_projects: params[:with_projects]) + present_group_details(params, group.reset, with_projects: params[:with_projects]) else render_api_error!(response.message, 422) end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index ef354fcccddc821583bf6a20eb3d9603025a2ca6..d0396310e2d2c112be0d56540cff6dc1a2ee8d15 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -865,6 +865,10 @@ it { expect(group.descendants.to_sql).to include 'traversal_ids @>' } end + describe '#descendant_ids' do + it { expect(group.descendant_ids.to_sql).to include 'traversal_ids @>' } + end + describe '#self_and_hierarchy' do it { expect(group.self_and_hierarchy.to_sql).to include 'traversal_ids @>' } end @@ -4673,4 +4677,101 @@ def define_cache_expectations(cache_key) end end end + + describe '#unarchive_self_and_descendants!' do + let_it_be_with_reload(:parent_group) { create(:group) } + let_it_be_with_reload(:group) { create(:group, :archived, parent: parent_group) } + let_it_be_with_reload(:subgroup) { create(:group, :archived, parent: group) } + let_it_be_with_reload(:sub_subgroup) { create(:group, :archived, parent: subgroup) } + + it 'unarchives the group itself' do + group.unarchive_self_and_descendants! + + expect(group.namespace_settings.reload.archived).to be(false) + end + + it 'unarchives all descendant groups', :aggregate_failures do + group.unarchive_self_and_descendants! + + expect(subgroup.namespace_settings.reload.archived).to be(false) + expect(sub_subgroup.namespace_settings.reload.archived).to be(false) + end + + it 'does not unarchive parent groups' do + parent_group.namespace_settings.update!(archived: true) + + group.unarchive_self_and_descendants! + + expect(parent_group.namespace_settings.reload.archived).to be(true) + end + + it 'does not affect groups that are not archived' do + unarchived_group = create(:group, parent: group) + + expect { group.unarchive_self_and_descendants! }.not_to change { + unarchived_group.namespace_settings.reload.archived + } + end + end + + describe '#unarchive_descendants!' do + let_it_be_with_reload(:parent_group) { create(:group) } + let_it_be_with_reload(:group) { create(:group, :archived, parent: parent_group) } + let_it_be_with_reload(:subgroup) { create(:group, :archived, parent: group) } + let_it_be_with_reload(:sub_subgroup) { create(:group, :archived, parent: subgroup) } + + it 'does not unarchive the group itself' do + group.unarchive_descendants! + + expect(group.namespace_settings.reload.archived).to be(true) + end + + it 'unarchives all descendant groups', :aggregate_failures do + group.unarchive_descendants! + + expect(subgroup.namespace_settings.reload.archived).to be(false) + expect(sub_subgroup.namespace_settings.reload.archived).to be(false) + end + + it 'does not unarchive parent groups' do + parent_group.namespace_settings.update!(archived: true) + + group.unarchive_descendants! + + expect(parent_group.namespace_settings.reload.archived).to be(true) + end + + it 'does not affect groups that are not archived' do + unarchived_group = create(:group, parent: group) + + expect { group.unarchive_descendants! }.not_to change { + unarchived_group.namespace_settings.reload.archived + } + end + end + + describe '#unarchive_all_projects!' do + let_it_be_with_reload(:group) { create(:group) } + let_it_be_with_reload(:archived_project_1) { create(:project, :archived, group: group) } + let_it_be_with_reload(:archived_project_2) { create(:project, :archived, group: group) } + let_it_be_with_reload(:non_archived_project) { create(:project, group: group) } + + let_it_be_with_reload(:subgroup) { create(:group, parent: group) } + let_it_be_with_reload(:subgroup_archived_project) { create(:project, :archived, group: subgroup) } + let_it_be_with_reload(:subgroup_non_archived_project) { create(:project, group: subgroup) } + + it 'unarchives all archived projects in the group', :aggregate_failures do + group.unarchive_all_projects! + + expect(archived_project_1.reload.archived).to be(false) + expect(archived_project_2.reload.archived).to be(false) + expect(subgroup_archived_project.reload.archived).to be(false) + end + + it 'does not affect projects that are not archived' do + expect { group.unarchive_all_projects! } + .to not_change { non_archived_project.reload.archived } + .and not_change { subgroup_non_archived_project.reload.archived } + end + end end diff --git a/spec/models/namespaces/traversal/cached_spec.rb b/spec/models/namespaces/traversal/cached_spec.rb index c166e9b8fe3ad9b775fd12cacf0650e5379df113..09158bf950bc993166ab6b6c064c0e9f99a99f64 100644 --- a/spec/models/namespaces/traversal/cached_spec.rb +++ b/spec/models/namespaces/traversal/cached_spec.rb @@ -150,6 +150,43 @@ end end + describe '#descendant_ids' do + subject(:ids) { group.descendant_ids.pluck(:id) } + + it 'returns the cached values excluding self' do + expect(ids).to eq(namespace_descendants.self_and_descendant_group_ids - [group.id]) + end + + context 'when the cache is outdated' do + it 'returns the values from the uncached descendant_ids query' do + namespace_descendants.update!(outdated_at: Time.current) + + expect(ids.sort).to match_array([subgroup.id, subsubgroup.id]) + end + end + + context 'when the scope is specified' do + subject(:ids) { group.descendant_ids(skope: Namespace).pluck(:id) } + + it 'returns the cached values excluding self' do + expect(ids).to eq(namespace_descendants.self_and_descendant_ids - [group.id]) + end + + context 'when the cache is outdated' do + it 'returns the values from the uncached descendant_ids query' do + namespace_descendants.update!(outdated_at: Time.current) + + expect(ids.sort).to eq([ + subgroup.id, subsubgroup.id, + project1.project_namespace_id, + project2.project_namespace_id, + project3.project_namespace_id + ]) + end + end + end + end + describe '#all_project_ids' do subject(:ids) { group.all_project_ids.pluck(:id) } diff --git a/spec/services/namespaces/groups/archive_service_spec.rb b/spec/services/namespaces/groups/archive_service_spec.rb index 0c895a7f635fe87a732ed5813a0c90b60a036e33..122d227c619ab0f2c64d2ed73e7061476891abf9 100644 --- a/spec/services/namespaces/groups/archive_service_spec.rb +++ b/spec/services/namespaces/groups/archive_service_spec.rb @@ -61,13 +61,50 @@ end context 'when the group is not archived' do + let_it_be_with_reload(:subgroup) { create(:group, :archived, parent: group) } + let_it_be_with_reload(:sub_subgroup) { create(:group, :archived, parent: subgroup) } + + let_it_be_with_reload(:archived_project) { create(:project, :archived, group: group) } + let_it_be_with_reload(:archived_subgroup_project) { create(:project, :archived, group: subgroup) } + before do group.namespace_settings.update!(archived: false) end + shared_examples 'rolls back all changes on failure' do + it 'returns an error response' do + response = service_response + expect(response).to be_error + expect(response.message).to eq("Failed to archive group!") + end + + it 'does not persist any changes' do + expect { service_response } + .to not_change { group.namespace_settings.reload.archived } + .and not_change { subgroup.namespace_settings.reload.archived } + .and not_change { archived_project.reload.archived } + end + end + context 'when archiving succeeds' do - it 'updates the namespace_settings archived attribute' do - expect { service_response }.to change { group.namespace_settings.reload.archived }.from(false).to(true) + it 'archives the group' do + service_response + + expect(group.namespace_settings.reload.archived).to be(true) + end + + it 'unarchives all descendant groups', :aggregate_failures do + service_response + + expect(subgroup.namespace_settings.reload.archived).to be(false) + expect(sub_subgroup.namespace_settings.reload.archived).to be(false) + end + + it 'unarchives all projects', :aggregate_failures do + service_response + + expect(archived_project.reload.archived).to be(false) + expect(archived_subgroup_project.reload.archived).to be(false) end it 'returns a success response with the group' do @@ -76,10 +113,10 @@ it 'publishes a GroupArchivedEvent' do expect { service_response }.to publish_event(Namespaces::Groups::GroupArchivedEvent) - .with( - group_id: group.id, - root_namespace_id: group.root_ancestor.id - ) + .with( + group_id: group.id, + root_namespace_id: group.root_ancestor.id + ) end it 'enqueues UnlinkProjectForksWorker' do @@ -88,18 +125,50 @@ service_response end + + context 'when `cascade_unarchive_group` flag is disabled' do + before do + stub_feature_flags(cascade_unarchive_group: false) + end + + it 'archives the group' do + service_response + + expect(group.namespace_settings.reload.archived).to be(true) + end + + it 'does not modify descendant and projects archived state' do + expect { service_response } + .to not_change { subgroup.namespace_settings.reload.archived } + .and not_change { sub_subgroup.namespace_settings.reload.archived } + .and not_change { archived_project.reload.archived } + .and not_change { archived_subgroup_project.reload.archived } + end + end end context 'when archiving fails' do before do - allow(group.namespace_settings).to receive(:update).and_return(false) + allow(group.namespace_settings).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) end - it 'returns an error response with the appropriate message' do - response = service_response - expect(response).to be_error - expect(response.message).to eq("Failed to archive group!") + it_behaves_like 'rolls back all changes on failure' + end + + context 'when unarchiving descendants fails' do + before do + allow(group).to receive(:unarchive_descendants!).and_raise(ActiveRecord::RecordNotSaved) end + + it_behaves_like 'rolls back all changes on failure' + end + + context 'when unarchiving projects fails' do + before do + allow(group).to receive(:unarchive_all_projects!).and_raise(ActiveRecord::RecordNotSaved) + end + + it_behaves_like 'rolls back all changes on failure' end end diff --git a/spec/services/namespaces/groups/unarchive_service_spec.rb b/spec/services/namespaces/groups/unarchive_service_spec.rb index a1fa8b800f63b86c986e361e5d9da8a9b51eaafb..30f130110930947f59a77a68e3eaca3757cb64d5 100644 --- a/spec/services/namespaces/groups/unarchive_service_spec.rb +++ b/spec/services/namespaces/groups/unarchive_service_spec.rb @@ -40,14 +40,45 @@ end context 'when the group is archived' do + let_it_be_with_reload(:subgroup) { create(:group, :archived, parent: group) } + let_it_be_with_reload(:sub_subgroup) { create(:group, :archived, parent: subgroup) } + + let_it_be_with_reload(:archived_project) { create(:project, :archived, group: group) } + let_it_be_with_reload(:archived_subgroup_project) { create(:project, :archived, group: subgroup) } + before do group.namespace_settings.update!(archived: true) end + shared_examples 'rolls back all changes on failure' do + it 'returns an error response' do + response = service_response + expect(response).to be_error + expect(response.message).to eq("Failed to unarchive group!") + end + + it 'does not persist any changes' do + expect { service_response } + .to not_change { group.namespace_settings.reload.archived } + .and not_change { subgroup.namespace_settings.reload.archived } + .and not_change { archived_project.reload.archived } + end + end + context 'when unarchiving succeeds' do - it 'updates namespace_settings archived to false' do - expect(group.namespace_settings).to receive(:update).with(archived: false).and_return(true) + it 'unarchives all descendant groups', :aggregate_failures do service_response + + expect(group.namespace_settings.reload.archived).to be(false) + expect(subgroup.namespace_settings.reload.archived).to be(false) + expect(sub_subgroup.namespace_settings.reload.archived).to be(false) + end + + it 'unarchives all projects', :aggregate_failures do + service_response + + expect(archived_project.reload.archived).to be(false) + expect(archived_subgroup_project.reload.archived).to be(false) end it 'returns a success response with the group' do @@ -56,23 +87,47 @@ it 'publishes a GroupArchivedEvent' do expect { service_response }.to publish_event(Namespaces::Groups::GroupArchivedEvent) - .with( - group_id: group.id, - root_namespace_id: group.root_ancestor.id - ) + .with( + group_id: group.id, + root_namespace_id: group.root_ancestor.id + ) + end + + context 'when `cascade_unarchive_group` flag is disabled' do + before do + stub_feature_flags(cascade_unarchive_group: false) + end + + it 'unarchives the group' do + service_response + + expect(group.namespace_settings.reload.archived).to be(false) + end + + it 'does not modify descendant and projects archived state' do + expect { service_response } + .to not_change { subgroup.namespace_settings.reload.archived } + .and not_change { sub_subgroup.namespace_settings.reload.archived } + .and not_change { archived_project.reload.archived } + .and not_change { archived_subgroup_project.reload.archived } + end end end context 'when unarchiving fails' do before do - allow(group.namespace_settings).to receive(:update).with(archived: false).and_return(false) + allow(group).to receive(:unarchive_self_and_descendants!).and_raise(ActiveRecord::RecordNotSaved) end - it 'returns an error response with the appropriate message' do - response = service_response - expect(response).to be_error - expect(response.message).to eq("Failed to unarchive group!") + it_behaves_like 'rolls back all changes on failure' + end + + context 'when unarchiving projects fails' do + before do + allow(group).to receive(:unarchive_all_projects!).and_raise(ActiveRecord::RecordNotSaved) end + + it_behaves_like 'rolls back all changes on failure' end end diff --git a/spec/support/shared_examples/namespaces/traversal_examples.rb b/spec/support/shared_examples/namespaces/traversal_examples.rb index 2099e9f88e5b75116db05e33f0e7fce60557863b..f3a262f2f0045962813295794f4c70c52f130090 100644 --- a/spec/support/shared_examples/namespaces/traversal_examples.rb +++ b/spec/support/shared_examples/namespaces/traversal_examples.rb @@ -384,4 +384,17 @@ end end end + + describe '#descendant_ids' do + subject { group.descendant_ids.pluck(:id) } + + it { is_expected.to contain_exactly(nested_group.id, deep_nested_group.id, very_deep_nested_group.id) } + + it 'includes project namespaces when when scope is Namespace' do + expect(group.descendant_ids(skope: Namespace).pluck(:id)) + .to contain_exactly( + nested_group.id, deep_nested_group.id, very_deep_nested_group.id, project_namespace.id + ) + end + end end