Commit f4fefc28 authored by Brett Walker's avatar Brett Walker Committed by Adam Hegyi

Get non-public groups when removing group todos

Since the group is private, and we're only looking at it's
descendants, we don't need to consider public/internal groups
parent de555479
...@@ -65,8 +65,10 @@ module Todos ...@@ -65,8 +65,10 @@ module Todos
end end
def remove_group_todos def remove_group_todos
return unless entity.is_a?(Namespace)
Todo Todo
.for_group(non_authorized_groups) .for_group(non_authorized_non_public_groups)
.for_user(user) .for_user(user)
.delete_all .delete_all
end end
...@@ -102,12 +104,19 @@ module Todos ...@@ -102,12 +104,19 @@ module Todos
GroupsFinder.new(user, min_access_level: Gitlab::Access::REPORTER).execute.select(:id) GroupsFinder.new(user, min_access_level: Gitlab::Access::REPORTER).execute.select(:id)
end end
def non_authorized_groups # 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
return [] unless entity.is_a?(Namespace) return [] unless entity.is_a?(Namespace)
return [] unless entity.private?
entity.self_and_descendants.select(:id) entity.self_and_descendants.select(:id)
.id_not_in(GroupsFinder.new(user).execute.select(:id)) .id_not_in(GroupsFinder.new(user, all_available: false).execute.select(:id).reorder(nil))
end end
# rubocop: enable CodeReuse/ActiveRecord
def non_authorized_reporter_groups def non_authorized_reporter_groups
entity.self_and_descendants.select(:id) entity.self_and_descendants.select(:id)
......
---
title: Speed up destroying of group Todos when user leaves group
merge_request: 56995
author:
type: performance
...@@ -224,6 +224,8 @@ RSpec.describe Todos::Destroy::EntityLeaveService do ...@@ -224,6 +224,8 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
end end
context 'with nested groups' do context 'with nested groups' do
let(:parent_group) { create(:group, :public) }
let(:parent_subgroup) { create(:group)}
let(:subgroup) { create(:group, :private, parent: group) } let(:subgroup) { create(:group, :private, parent: group) }
let(:subgroup2) { create(:group, :private, parent: group) } let(:subgroup2) { create(:group, :private, parent: group) }
let(:subproject) { create(:project, group: subgroup) } let(:subproject) { create(:project, group: subgroup) }
...@@ -235,12 +237,17 @@ RSpec.describe Todos::Destroy::EntityLeaveService do ...@@ -235,12 +237,17 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
let!(:todo_subgroup2_user) { create(:todo, user: user, group: subgroup2) } let!(:todo_subgroup2_user) { create(:todo, user: user, group: subgroup2) }
let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) } let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) }
let!(:todo_subpgroup_user2) { create(:todo, user: user2, group: subgroup) } let!(:todo_subpgroup_user2) { create(:todo, user: user2, group: subgroup) }
let!(:todo_parent_group_user) { create(:todo, user: user, group: parent_group) }
before do
group.update!(parent: parent_group)
end
context 'when the user is not a member of any groups/projects' do context 'when the user is not a member of any groups/projects' do
it 'removes todos for the user including subprojects todos' do it 'removes todos for the user including subprojects todos' do
expect { subject }.to change { Todo.count }.from(12).to(4) expect { subject }.to change { Todo.count }.from(13).to(5)
expect(user.todos).to be_empty expect(user.todos).to eq([todo_parent_group_user])
expect(user2.todos) expect(user2.todos)
.to match_array( .to match_array(
[todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]
...@@ -250,8 +257,6 @@ RSpec.describe Todos::Destroy::EntityLeaveService do ...@@ -250,8 +257,6 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
context 'when the user is member of a parent group' do context 'when the user is member of a parent group' do
before do before do
parent_group = create(:group)
group.update!(parent: parent_group)
parent_group.add_developer(user) parent_group.add_developer(user)
end end
...@@ -264,9 +269,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do ...@@ -264,9 +269,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
end end
it 'does not remove group and subproject todos' do it 'does not remove group and subproject todos' do
expect { subject }.to change { Todo.count }.from(12).to(7) expect { subject }.to change { Todo.count }.from(13).to(8)
expect(user.todos).to match_array([todo_group_user, todo_subgroup_user, todo_subproject_user]) expect(user.todos)
.to match_array(
[todo_group_user, todo_subgroup_user, todo_subproject_user, todo_parent_group_user]
)
expect(user2.todos) expect(user2.todos)
.to match_array( .to match_array(
[todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]
...@@ -280,9 +288,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do ...@@ -280,9 +288,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
end end
it 'does not remove subproject and group todos' do it 'does not remove subproject and group todos' do
expect { subject }.to change { Todo.count }.from(12).to(7) expect { subject }.to change { Todo.count }.from(13).to(8)
expect(user.todos).to match_array([todo_subgroup_user, todo_group_user, todo_subproject_user]) expect(user.todos)
.to match_array(
[todo_subgroup_user, todo_group_user, todo_subproject_user, todo_parent_group_user]
)
expect(user2.todos) expect(user2.todos)
.to match_array( .to match_array(
[todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]
......
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