Skip to content

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:

  1. 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 the current_user but for the other users.

  2. PendingTodosFinder has been rewritten to allow finding pending todos on some object, without restricting the search to specific users. By making the users parameter optional.

  3. PendingTodosFinder now allows preloading the users on the todos. To avoid N + 1 queries. Once we get the todos, we issue one select query on the users table to get them. This is needed, because after we get the users, we need to call update_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;

Getting the access request pending todos for the group gitlab

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

  1. Login from three different browser sessions using three users. For the this MR, I am using the users ["root", "user1", "user2"]
  2. Create or locate a group or project (feel free to test both).
  3. Make sure the group/project has the users root and user1 as owners. And that user2 is not member of the group/project or any of its ancestors. For simplicity, you can choose a top-level group -> project for testing.
  4. 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.
  5. 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

  1. user2 creates an access request to the group/project.
  2. A new pending access request Todo is created for root and user1.
  3. user2 withdraws their access request.
  4. The newly created Todos from (2) are marked as complete. The todo counter for both root and user1 are updated (cache update).

Scenario 2: Accepting the access request by one of the owners.

  1. user2 creates an access request to the group/project.
  2. A new pending access request Todo is created for root and user1.
  3. root or user1 accepts the access request.
  4. The newly created Todos from (2) are marked as complete. The todo counter for both root and user1 are updated (cache update).

Scenario 3: Accepting the access request by one of the owners.

  1. user2 creates an access request to the group/project.
  2. A new pending access request Todo is created for root and user1.
  3. root or user1 rejects the access request.
  4. The newly created Todos from (2) are marked as complete. The todo counter for both root and user1 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.

Related to #374726 (closed)

Edited by Omar Qunsul

Merge request reports