Marking target owner todos as finished upon handling access request
What does this MR do and why?
In this MR, we expand marking the TODOs of the corresponding access request, not only to the owner who takes action, but for all the other owners as well.
Changelog: added
Addressing issue: #374726 (closed)
Context (what has been done before this MR)
Users can request access to groups/projects that are visible to them (public or internal). Only if the settings of the group/project allows this setting.
When a user creates an access request, an object of type Member
that means an access request. This is meant by setting the requested_at
attribute, see here.
In the MR and MR we started creating TODOs for the most active owners of the group/project regarding those access requests. And in the MR we mark the TODO,of the owner who takes action on the access request, as done.
Refactoring that has been done in this MR
In the current codebase of GitLab, we usualy take action on the TODOs of the current user only. We don't have code path that allows the current_user
to mark the todos of other users as done. And this what brought some challenge to this MR.
In this MR:
-
The
TodoService#resolve_access_request_todos
has been rewritten to mark all the access request pending TODOs of all the pending todos of some target object. Not only for thecurrent_user
but for the other users. -
PendingTodosFinder
has been rewritten to allow finding pending todos on some object, without restricting the search to specific users. By making theusers
parameter optional. -
PendingTodosFinder
now allows preloading the users on thetodos
. To avoid N + 1 queries. Once we get the todos, we issue one select query on theusers
table to get them. This is needed, because after we get theusers
, we need to callupdate_todos_count_cache
on each user, after marking their todo as done. See the (newly introduced database queries) section of this MR's description.
Newly introduced database queries
Preparing test data on postgres.ai
Generating test data: Adding 10 access request todos for group 9970 to 10 users (group membership is not needed for this test)
exec
WITH selected_users AS (select id FROM users limit 10)
INSERT INTO todos(author_id, user_id, target_id, target_type, created_at, updated_at, state, action)
SELECT 1614863, id, 9970, 'Namespace', NOW(), NOW(), 'pending', 10 FROM selected_users;
gitlab
Getting the access request pending todos for the group explain SELECT "todos"."id", "todos"."user_id", "todos"."project_id", "todos"."target_id", "todos"."target_type", "todos"."author_id", "todos"."action", "todos"."state", "todos"."created_at", "todos"."updated_at", "todos"."commit_id", "todos"."group_id", "todos"."resolved_by_action", "todos"."note_id" FROM "todos" WHERE ("todos"."state" IN ('pending')) AND "todos"."target_id" = 9970 AND "todos"."target_type" = 'Namespace' AND "todos"."author_id" = 1614863 AND "todos"."action" = 10;
https://postgres.ai/console/gitlab/gitlab-production-tunnel-pg12/sessions/18118/commands/60201
Getting the users that own these todos (10 random user_ids)
explain SELECT "users".* FROM "users" WHERE "users"."id" IN (1614863, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
https://postgres.ai/console/gitlab/gitlab-production-tunnel-pg12/sessions/18118/commands/60184
Updating the todos to done
explain UPDATE "todos" SET "state" = 'done', "resolved_by_action" = 0, "updated_at" = '2023-05-03 14:43:43.284105' WHERE ("todos"."state" IN ('pending')) AND "todos"."target_id" = 9970 AND "todos"."target_type" = 'Namespace' AND "todos"."author_id" = 1614863 AND "todos"."action" = 10 AND "todos"."state" != 'done'
https://postgres.ai/console/gitlab/gitlab-production-tunnel-pg12/sessions/18118/commands/60209
Screenshots or screen recordings
Scenario 1:
Requester deleting/canceling their access request. Requester is using Mozilla Firefox in the video
requester_deleting_their_access_request
Scenarios 2 / 3:
- One owner accepting the access request. The owner is on the left, using Google Chrome
- One owner rejecting the access request. The owner is on the, left using Google Chrome
requester_deleting_their_access_request
How to set up and validate locally
Prerequisites
- Login from three different browser sessions using three users. For the this MR, I am using the users
["root", "user1", "user2"]
- Create or locate a group or project (feel free to test both).
- Make sure the group/project has the users
root
anduser1
as owners. And thatuser2
is not member of the group/project or any of its ancestors. For simplicity, you can choose a top-level group -> project for testing. - Important: Make sure that the group/project are public or internal. To allow other users to see them, and request accessing to them. This can be changed from the group/project page -> Settings -> General.
- Important step: Make sure that the group/project allows other users to request access to them. See this documentation page for hints.
Scenario 1: Withdrawing Access Request by the Requester
-
user2
creates an access request to the group/project. - A new pending access request Todo is created for
root
anduser1
. -
user2
withdraws their access request. - The newly created Todos from (2) are marked as complete. The todo counter for both
root
anduser1
are updated (cache update).
Scenario 2: Accepting the access request by one of the owners.
-
user2
creates an access request to the group/project. - A new pending access request Todo is created for
root
anduser1
. -
root
oruser1
accepts the access request. - The newly created Todos from (2) are marked as complete. The todo counter for both
root
anduser1
are updated (cache update).
Scenario 3: Accepting the access request by one of the owners.
-
user2
creates an access request to the group/project. - A new pending access request Todo is created for
root
anduser1
. -
root
oruser1
rejects the access request. - The newly created Todos from (2) are marked as complete. The todo counter for both
root
anduser1
are updated (cache update).
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #374726 (closed)