Skip to content
Snippets Groups Projects

Show members of private group as approvers

All threads resolved!

What does this MR do and why?

Show members of private group as approvers

When an approval rule has a group that is considered hidden to the user then approvers are not shown. This is a problem when a private group has been added to an approval rule.

This change is to consider private groups that are invited to the project associated with the approval rule to be considered non-hidden

For more context: #281029 (comment 1807240087)

Query Plans

This change adds an additional query during project_groups calling project.invited_groups that results in a query like:

SELECT *
FROM "namespaces"
INNER JOIN "project_group_links" ON "namespaces"."id" = "project_group_links"."group_id"
WHERE "namespaces"."type" = 'Group'
  AND "project_group_links"."project_id" = 278964

query plan here: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/27397/commands/85304

The result of that is used in hidden_groups which performs a query like: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/27497/commands/85645

SELECT *
FROM "namespaces"
INNER JOIN "approval_project_rules_groups" ON "namespaces"."id" = "approval_project_rules_groups"."group_id"
WHERE "namespaces"."type" = 'Group'
  AND "approval_project_rules_groups"."approval_project_rule_id" = 14315753
  AND "namespaces"."id" NOT IN (2750817,
                                3887968,
                                5924764,
                                6150316);

Before this change hidden_groups would perform a similar query but without the results from project_groups: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/27497/commands/85648

SELECT *
FROM "namespaces"
INNER JOIN "approval_project_rules_groups" ON "namespaces"."id" = "approval_project_rules_groups"."group_id"
WHERE "namespaces"."type" = 'Group'
  AND "approval_project_rules_groups"."approval_project_rule_id" = 14315753
  AND "namespaces"."id" != 3887968;

Notably sometimes rule is an instance of ApprovalMergeRequestRule, but the resulting queries are similar

For example

SELECT *
FROM "namespaces"
INNER JOIN "approval_merge_request_rules_groups" ON "namespaces"."id" = "approval_merge_request_rules_groups"."group_id"
WHERE "namespaces"."type" = 'Group'
  AND "approval_merge_request_rules_groups"."approval_merge_request_rule_id" = 269
  AND "namespaces"."id" != 2962
 Hash Join  (cost=7.54..14.83 rows=1 width=434) (actual time=0.278..0.338 rows=1 loops=1)
   Hash Cond: (namespaces.id = approval_merge_request_rules_groups.group_id)
   ->  Index Scan using index_namespaces_on_organization_id_for_groups on namespaces  (cost=0.14..7.14 rows=114 width=414) (actual time=0.091..0.271 rows=132 loops=1)
         Filter: (id <> 2962)
         Rows Removed by Filter: 1
   ->  Hash  (cost=7.29..7.29 rows=8 width=20) (actual time=0.039..0.039 rows=2 loops=1)
         Buckets: 1024  Batches: 1  Memory Usage: 9kB
         ->  Index Scan using index_approval_merge_request_rules_groups_1 on approval_merge_request_rules_groups  (cost=0.15..7.29 rows=8 width=20) (actual time=0.029..0.031 rows=2 loops=1)
               Index Cond: (approval_merge_request_rule_id = 269)
 Planning Time: 0.955 ms
 Execution Time: 0.413 ms

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

How to set up and validate locally

  1. Create a private group (private-group-b)
  2. Create a project
  3. Invite the private subgroup to the project
  4. Add a CODEOWNERS file to the project. For example
*.txt @private-group-b
  1. Create a protected branch and require code owner approval
  2. Create merge request that changes something requiring code owner approval
  3. Invite another user to the project
  4. As the other user, see that the approvers for the code owner rule are missing from the approval rules widget in the merge request
  5. Enable the show_private_groups_as_approvers feature flag
  6. Check the widget again and see that the approvers are present. Screenshot_2024-04-04_at_3.40.15_PM

Related to #281029 (closed)

Edited by Jerry Seto

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Zamir Martins approved this merge request

    approved this merge request

  • Zamir Martins removed review request for @zmartins

    removed review request for @zmartins

  • E2E Test Result Summary

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :white_check_mark: test report for a63ee20e

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Create      | 83     | 0      | 9       | 0     | 92    | ✅     |
    | Monitor     | 7      | 0      | 0       | 0     | 7     | ✅     |
    | Govern      | 66     | 0      | 0       | 0     | 66    | ✅     |
    | Plan        | 51     | 0      | 2       | 0     | 53    | ✅     |
    | Data Stores | 31     | 0      | 0       | 0     | 31    | ✅     |
    | Package     | 24     | 0      | 6       | 0     | 30    | ✅     |
    | Verify      | 35     | 0      | 1       | 0     | 36    | ✅     |
    | Release     | 5      | 0      | 0       | 0     | 5     | ✅     |
    | Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
    | Manage      | 0      | 0      | 1       | 0     | 1     | ➖     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 304    | 0      | 19      | 0     | 323   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+

    e2e-package-and-test: :white_check_mark: test report for a63ee20e

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Create      | 570    | 0      | 66      | 10    | 636   | ✅     |
    | Plan        | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Govern      | 6      | 0      | 0       | 0     | 6     | ✅     |
    | Monitor     | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Data Stores | 4      | 0      | 0       | 0     | 4     | ✅     |
    | Package     | 0      | 0      | 2       | 0     | 2     | ➖     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 596    | 0      | 68      | 10    | 664   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
  • Jerry Seto added 1 commit

    added 1 commit

    Compare with previous version

  • Jerry Seto requested review from @splattael

    requested review from @splattael

  • Peter Leitzen
  • Peter Leitzen approved this merge request

    approved this merge request

  • Peter Leitzen removed review request for @splattael

    removed review request for @splattael

  • Imre Farkas removed review request for @ifarkas

    removed review request for @ifarkas

  • Jerry Seto added 467 commits

    added 467 commits

    Compare with previous version

  • Jerry Seto reset approvals from @splattael by pushing to the branch

    reset approvals from @splattael by pushing to the branch

  • Jerry Seto changed the description

    changed the description

  • Jerry Seto requested review from @ifarkas

    requested review from @ifarkas

  • Peter Leitzen approved this merge request

    approved this merge request

  • Imre Farkas approved this merge request

    approved this merge request

  • Imre Farkas requested review from @mayra-cabrera and removed review request for @ifarkas

    requested review from @mayra-cabrera and removed review request for @ifarkas

    • Resolved by Jerry Seto

      The result of that is used in hidden_groups which performs a query like:

      Notably sometimes rule is an instance of ApprovalMergeRequestRule, but the resulting queries are similar

      None of these queries return any result. Could we please analyze a query that returns some rows? Just to understand what would be the average query plan. Also, could we use the gitlab-org namespace (9970)?

  • Mayra Cabrera removed review request for @mayra-cabrera

    removed review request for @mayra-cabrera

  • Jerry Seto changed the description

    changed the description

  • Jerry Seto added 1 commit

    added 1 commit

    • a63ee20e - Return an activerecord collection rather than an Array

    Compare with previous version

  • Jerry Seto reset approvals from @splattael and @ifarkas by pushing to the branch

    reset approvals from @splattael and @ifarkas by pushing to the branch

  • Jerry Seto changed the description

    changed the description

  • Jerry Seto requested review from @mayra-cabrera

    requested review from @mayra-cabrera

  • Mayra Cabrera approved this merge request

    approved this merge request

  • added databaseapproved label and removed databasereviewed label

  • Thanks @j.seto. database LGTM. I'll proceed to merge since the MR has been approved by a backend maintainer !148720 (comment 1854507434)

  • Mayra Cabrera resolved all threads

    resolved all threads

  • Mayra Cabrera enabled an automatic merge when the pipeline for c22c8631 succeeds

    enabled an automatic merge when the pipeline for c22c8631 succeeds

  • Hello @j.seto :wave:

    The database team is looking for ways to improve the database review process and we would love your help!

    If you'd be open to someone on the database team reaching out to you for a chat, or if you'd like to leave some feedback asynchronously, just post a reply to this comment mentioning:

    @gitlab-org/database-team

    And someone will be by shortly!

    Thanks for your help! :heart:

    This message was generated automatically. You're welcome to improve it.

  • Mayra Cabrera mentioned in commit 7dc52246

    mentioned in commit 7dc52246

  • added workflowstaging label and removed workflowcanary label

  • mentioned in issue #281029 (closed)

  • mentioned in issue #506056 (closed)

  • Please register or sign in to reply
    Loading