Optimise query hierarchy for filtered add on eligible users results

What does this MR do and why?

Relates to: https://gitlab.com/gitlab-org/gitlab/-/issues/489895 and https://gitlab.com/gitlab-org/gitlab/-/issues/505472

This MR adjusts the query that fetches filtered add on users by returning all the assigned users directly instead of involving project and groups memberships as the query is quite complex. Showing all assigned users, including blocked, is better because it takes few hours to clean-up blocked users seat via cron jobs. With this, admin can immediately release the seat of blocked user, if needed.

References

• Originating from https://gitlab.com/gitlab-org/gitlab/-/issues/489895#note_2205670149

Database

SQL query

SELECT
  "users"."id",
  "users"."email",
  "users"."encrypted_password",
  "users"."reset_password_token",
  "users"."reset_password_sent_at",
  "users"."remember_created_at",
  "users"."sign_in_count",
  "users"."current_sign_in_at",
  "users"."last_sign_in_at",
  "users"."current_sign_in_ip",
  "users"."last_sign_in_ip",
  "users"."created_at",
  "users"."updated_at",
  "users"."name",
  "users"."admin",
  "users"."projects_limit",
  "users"."failed_attempts",
  "users"."locked_at",
  "users"."username",
  "users"."can_create_group",
  "users"."can_create_team",
  "users"."state",
  "users"."color_scheme_id",
  "users"."password_expires_at",
  "users"."created_by_id",
  "users"."last_credential_check_at",
  "users"."avatar",
  "users"."confirmation_token",
  "users"."confirmed_at",
  "users"."confirmation_sent_at",
  "users"."unconfirmed_email",
  "users"."hide_no_ssh_key",
  "users"."admin_email_unsubscribed_at",
  "users"."notification_email",
  "users"."hide_no_password",
  "users"."password_automatically_set",
  "users"."encrypted_otp_secret",
  "users"."encrypted_otp_secret_iv",
  "users"."encrypted_otp_secret_salt",
  "users"."otp_required_for_login",
  "users"."otp_backup_codes",
  "users"."public_email",
  "users"."dashboard",
  "users"."project_view",
  "users"."consumed_timestep",
  "users"."layout",
  "users"."hide_project_limit",
  "users"."note",
  "users"."unlock_token",
  "users"."otp_grace_period_started_at",
  "users"."external",
  "users"."incoming_email_token",
  "users"."auditor",
  "users"."require_two_factor_authentication_from_group",
  "users"."two_factor_grace_period",
  "users"."last_activity_on",
  "users"."notified_of_own_activity",
  "users"."preferred_language",
  "users"."theme_id",
  "users"."accepted_term_id",
  "users"."feed_token",
  "users"."private_profile",
  "users"."roadmap_layout",
  "users"."include_private_contributions",
  "users"."commit_email",
  "users"."group_view",
  "users"."managing_group_id",
  "users"."first_name",
  "users"."last_name",
  "users"."static_object_token",
  "users"."role",
  "users"."user_type",
  "users"."static_object_token_encrypted",
  "users"."otp_secret_expires_at",
  "users"."onboarding_in_progress",
  "users"."color_mode_id"
FROM
  "users"
WHERE
  "users"."id" IN (
    SELECT
      "subscription_user_add_on_assignments"."user_id"
    FROM
      "subscription_user_add_on_assignments"
      INNER JOIN "subscription_add_on_purchases" ON "subscription_add_on_purchases"."id" = "subscription_user_add_on_assignments"."add_on_purchase_id"
    WHERE
      "subscription_add_on_purchases"."id" = 1
      AND (started_at IS NULL
        OR started_at <= '2024-11-25')
      AND ('2024-11-25' < expires_on))
ORDER BY
  "users"."id" DESC

Analysis: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/33895/commands/104167

How to set up and validate locally

Existing test cases should already account for this scenario.

Related to #505472

changed milestone to %17.7

added Category:Add-on Provisioning Non-project work Unplanned backend database devopsfulfillment groupprovision priority3 sectionfulfillment typemaintenance workflowin dev labels

assigned to @div.ya

added pipelinetier-1 label

added databasereview pending label

	1 Warning
	You've made some app changes, but didn't add any tests. That's OK as long as you're refactoring existing code, but please consider adding any of the maintenancepipelines, maintenancerefactor, maintenanceworkflow, maintenanceperformance, documentation, QA labels.

	2 Messages
	CHANGELOG missing: If this merge request needs a changelog entry, add the Changelog trailer to the commit message you want to add to the changelog. If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
	This merge request adds or changes files that require a review from the Database team.

This merge request requires a database review. To make sure these changes are reviewed, take the following steps:

1. Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
2. Prepare your MR for database review according to the docs.
3. Assign and mention the database reviewer suggested by Reviewer Roulette.

The following files require a review from the Database team:

• ee/app/finders/gitlab_subscriptions/self_managed/add_on_eligible_users_finder.rb
• ee/app/finders/gitlab_subscriptions/add_on_eligible_users_finder.rb

Reviewer roulette

Category	Reviewer	Maintainer
backend	@morefice (UTC+1, same timezone as author)	@tkuah (UTC+13, 12 hours ahead of author)
database	@bdenkovych (UTC+2, 1 hour ahead of author)	@Quintasan (UTC+1, same timezone as author)

Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

	1 Message
	CHANGELOG missing: If this merge request needs a changelog entry, add the Changelog trailer to the commit message you want to add to the changelog. If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

Reviewer roulette

Category	Reviewer	Maintainer
backend	@dbiryukov (UTC+1, same timezone as author)	@tkuah (UTC+13, 12 hours ahead of author)
database	@acroitor (UTC+2, 1 hour ahead of author)	@a_akgun (UTC+3, 2 hours ahead of author)

Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

marked this merge request as ready

changed title from Draft: Adjust query hierarchy for filtered add on eligible users to Optimise query hierarchy for filtered add on eligible users results

changed the description

@morefice Do you mind running an initial backend review, and @bdenkovych for database when you have some time ? Thanks in advance

requested review from @morefice and @bdenkovych

approved this merge request

added pipeline:mr-approved label

requested review from @jessieay

added pipelinetier-2 label and removed pipelinetier-1 label

Before you set this MR to auto-merge

This merge request will progress on pipeline tiers until it reaches the last tier: pipelinetier-3. We will trigger a new pipeline for each transition to a higher tier.

Before you set this MR to auto-merge, please check the following:

• You are the last maintainer of this merge request
• The latest pipeline for this merge request is pipelinetier-3 (You can find which tier it is in the pipeline name)
• This pipeline is recent enough (created in the last 8 hours)

If all the criteria above apply, please set auto-merge for this merge request.

See pipeline tiers and merging a merge request for more details.

Generated by gitlab_quality-test_tooling.

---

Slow tests detected in this merge request. These slow tests might be related to this merge request's changes.

Click to expand

Job	File	Name	Duration	Expected duration
#8464161520	spec/features/admin/users/users_spec.rb#L177	Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking	66.12 s	< 50.13 s
#8488082751	spec/features/admin/users/users_spec.rb#L177	Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking	66.92 s	< 50.13 s
#8488123610	spec/features/admin/users/users_spec.rb#L177	Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking	66.74 s	< 50.13 s

added rspec:slow test detected label

E2E Test Result Summary

allure-report-publisher generated test report!

e2e-test-on-gdk: test report for b100e083

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Create      | 129    | 0      | 22      | 0     | 151   | ✅     |
| Govern      | 75     | 0      | 3       | 0     | 78    | ✅     |
| Verify      | 43     | 0      | 2       | 0     | 45    | ✅     |
| Monitor     | 8      | 0      | 0       | 0     | 8     | ✅     |
| Manage      | 1      | 0      | 1       | 0     | 2     | ✅     |
| Plan        | 76     | 0      | 0       | 0     | 76    | ✅     |
| Package     | 25     | 0      | 11      | 0     | 36    | ✅     |
| Release     | 5      | 0      | 0       | 0     | 5     | ✅     |
| Data Stores | 33     | 0      | 1       | 0     | 34    | ✅     |
| Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
| Fulfillment | 2      | 0      | 0       | 0     | 2     | ✅     |
| Secure      | 4      | 0      | 0       | 0     | 4     | ✅     |
| Ai-powered  | 0      | 0      | 1       | 0     | 1     | ➖     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 403    | 0      | 41      | 0     | 444   | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+

e2e-test-on-cng: test report for b100e083

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Data Stores | 33     | 0      | 10      | 0     | 43    | ✅     |
| Package     | 24     | 0      | 14      | 0     | 38    | ✅     |
| Plan        | 86     | 0      | 8       | 0     | 94    | ✅     |
| Create      | 140    | 0      | 20      | 0     | 160   | ✅     |
| Release     | 5      | 0      | 1       | 0     | 6     | ✅     |
| Govern      | 84     | 0      | 9       | 1     | 93    | ✅     |
| Verify      | 49     | 0      | 16      | 0     | 65    | ✅     |
| Ai-powered  | 0      | 0      | 2       | 0     | 2     | ➖     |
| Monitor     | 8      | 0      | 12      | 0     | 20    | ✅     |
| Secure      | 2      | 0      | 5       | 0     | 7     | ✅     |
| Manage      | 1      | 0      | 9       | 0     | 10    | ✅     |
| Configure   | 0      | 0      | 3       | 0     | 3     | ➖     |
| Fulfillment | 2      | 0      | 7       | 1     | 9     | ✅     |
| Analytics   | 2      | 0      | 0       | 1     | 2     | ✅     |
| ModelOps    | 0      | 0      | 1       | 0     | 1     | ➖     |
| Growth      | 0      | 0      | 2       | 0     | 2     | ➖     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 436    | 0      | 119     | 3     | 555   | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+

approved this merge request

@div.ya Thanks for providing the query plan and for the link to existing test cases. The MR LGTM from database perspective.

However, from backend perspective I would suggest to update specs in this MR or by a follow-up-MR to describe that both classes return blocked assigned users too when filter_by_assigned_seat is used since it is desired behavior and to prevent regression in the future.

patch

diff --git ee/spec/finders/gitlab_subscriptions/add_on_eligible_users_finder_spec.rb ee/spec/finders/gitlab_subscriptions/add_on_eligible_users_finder_spec.rb
index 566b3af4e16d..0a6201ae5083 100644
--- ee/spec/finders/gitlab_subscriptions/add_on_eligible_users_finder_spec.rb
+++ ee/spec/finders/gitlab_subscriptions/add_on_eligible_users_finder_spec.rb
@@ -195,6 +195,7 @@
       let_it_be(:add_on_purchase) { create(:gitlab_subscription_add_on_purchase, :gitlab_duo_pro) }
       let_it_be(:owner) { create(:user, name: 'Owner User') }
       let_it_be(:assigned_user) { create(:user, name: 'Assigned User') }
+      let_it_be(:blocked_assigned_user) { create(:user, :blocked, name: 'Blocked Assigned User') }
       let_it_be(:non_assigned_user) { create(:user, name: 'Non Assigned User') }
 
       before_all do
@@ -203,6 +204,7 @@
         subgroup.add_developer(non_assigned_user)
         add_on_purchase.assigned_users.create!(user: owner)
         add_on_purchase.assigned_users.create!(user: assigned_user)
+        add_on_purchase.assigned_users.create!(user: blocked_assigned_user)
       end
 
       context 'when filter_by_assigned_seat is true' do
@@ -216,7 +218,7 @@
             filter_options: filter_options
           )
 
-          expect(finder.execute).to match_array([owner, assigned_user])
+          expect(finder.execute).to match_array([owner, assigned_user, blocked_assigned_user])
         end
       end
 
diff --git ee/spec/finders/gitlab_subscriptions/self_managed/add_on_eligible_users_finder_spec.rb ee/spec/finders/gitlab_subscriptions/self_managed/add_on_eligible_users_finder_spec.rb
index 8b10db44bf2d..2b8127c81c7c 100644
--- ee/spec/finders/gitlab_subscriptions/self_managed/add_on_eligible_users_finder_spec.rb
+++ ee/spec/finders/gitlab_subscriptions/self_managed/add_on_eligible_users_finder_spec.rb
@@ -101,10 +101,12 @@
     context 'when supplied a filter option' do
       let(:add_on_purchase) { create(:gitlab_subscription_add_on_purchase, :self_managed, :gitlab_duo_pro) }
       let(:assigned_user) { create(:user, name: 'Assigned User') }
+      let(:blocked_assigned_user) { create(:user, :blocked, name: 'Blocked Assigned User') }
       let(:non_assigned_user) { create(:user, name: 'Non Assigned User') }
 
       before do
         add_on_purchase.assigned_users.create!(user: assigned_user)
+        add_on_purchase.assigned_users.create!(user: blocked_assigned_user)
       end
 
       context 'when filter_by_assigned_seat is true' do
@@ -118,6 +120,7 @@
           )
 
           expect(finder.execute).to include(assigned_user)
+          expect(finder.execute).to include(blocked_assigned_user)
           expect(finder.execute).not_to include(non_assigned_user)
         end
       end

added databasereviewed label and removed databasereview pending label

approved this merge request

requested review from @Quintasan

mentioned in issue bdenkovych/notes#6

added 1 commit

• b100e083 - Adjust query hierarchy for filtered add on eligible users

Compare with previous version

reset approvals from @bdenkovych and @jessieay by pushing to the branch

approved this merge request

added databaseapproved label and removed databasereviewed label

added pipelinetier-3 pipeline:run-e2e-omnibus-once labels and removed pipelinetier-2 label

enabled automatic add to merge train when checks pass

resolved all threads

started a merge train

merged

mentioned in commit 84224727

Hello @div.ya

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!

This message was generated automatically. Improve it or delete it.

added workflowstaging-canary label and removed workflowin dev label

added workflowcanary label and removed workflowstaging-canary label

added workflowstaging label and removed workflowcanary label

added workflowproduction label and removed workflowstaging label

added workflowpost-deploy-db-production label and removed workflowproduction label

added releasedcandidate label

added releasedpublished label and removed releasedcandidate label