Merge branch 'issue-325421_delete_private_subgroup_todos' into 'master'

Delete private subgroups todos when removing member

See merge request gitlab-org/gitlab!68094

app/services/todos/destroy/entity_leave_service.rb

@@ -23,10 +23,10 @@ module Todos
         return if user_has_reporter_access?
 
         remove_confidential_resource_todos
+        remove_group_todos
 
         if entity.private?
           remove_project_todos
-          remove_group_todos
         else
           enqueue_private_features_worker
         end
@@ -68,7 +68,7 @@ module Todos
         return unless entity.is_a?(Namespace)
 
         Todo
-          .for_group(non_authorized_non_public_groups)
+          .for_group(unauthorized_private_groups)
           .for_user(user)
           .delete_all
       end
@@ -104,16 +104,13 @@ module Todos
         GroupsFinder.new(user, min_access_level: Gitlab::Access::REPORTER).execute.select(:id)
       end
 
-      # since the entity is a private group, we can assume all subgroups are also
-      # private.  We can therefore limit GroupsFinder with `all_available: false`.
-      # Otherwise it tries to include all public groups. This generates an expensive
-      # SQL queries: https://gitlab.com/gitlab-org/gitlab/-/issues/325133
       # rubocop: disable CodeReuse/ActiveRecord
-      def non_authorized_non_public_groups
+      def unauthorized_private_groups
         return [] unless entity.is_a?(Namespace)
-        return [] unless entity.private?
 
-        entity.self_and_descendants.select(:id)
+        groups = entity.self_and_descendants.private_only
+
+        groups.select(:id)
           .id_not_in(GroupsFinder.new(user, all_available: false).execute.select(:id).reorder(nil))
       end
       # rubocop: enable CodeReuse/ActiveRecord

ee/spec/services/ee/todos/destroy/entity_leave_service_spec.rb

@@ -35,6 +35,23 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
       end
     end
 
+    context 'when user was a member of public group with private subgroup' do
+      let_it_be(:epic3) { create(:epic, group: group) }
+
+      let!(:todo1) { create(:todo, target: epic3, user: user, group: group) }
+
+      before do
+        group.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+        subgroup.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+      end
+
+      it 'removes epic todos from private subgroup' do
+        described_class.new(user.id, group.id, 'Group').execute
+
+        expect(user.reload.todos.ids).to match_array(todo1.id)
+      end
+    end
+
     context 'when user role is downgraded to guest' do
       before do
         subgroup.add_guest(user)

lib/gitlab/visibility_level.rb

@@ -12,6 +12,7 @@ module Gitlab
     included do
       scope :public_only,               -> { where(visibility_level: PUBLIC) }
       scope :public_and_internal_only,  -> { where(visibility_level: [PUBLIC, INTERNAL] ) }
+      scope :private_only,              -> { where(visibility_level: PRIVATE) }
       scope :non_public_only,           -> { where.not(visibility_level: PUBLIC) }
 
       scope :public_to_user, -> (user = nil) do

spec/models/group_spec.rb

@@ -640,6 +640,12 @@ RSpec.describe Group do
       it { is_expected.to match_array([private_group, internal_group]) }
     end
 
+    describe 'private_only' do
+      subject { described_class.private_only.to_a }
+
+      it { is_expected.to match_array([private_group]) }
+    end
+
     describe 'with_onboarding_progress' do
       subject { described_class.with_onboarding_progress }