From 4b0d0576c1ebaa89c40e8921c009ded60d8ded27 Mon Sep 17 00:00:00 2001 From: Sam Figueroa Date: Fri, 16 Sep 2022 08:47:38 +0000 Subject: [PATCH] Notify owners of groups that are over free user limit - Add workers to notify over limit owners - There is one cron scheduled worker - The cron worker is responsible for backfilling jobs that process the checks, in case some of the limited capacity workers previously failed: https://docs.gitlab.com/ee/development/sidekiq/limited_capacity_worker.html - The notification worker locks a row from the namespace_details that is scheduled for over limit check and runs the check on that group, if applicable the owners of that group will be notified and the job marked as done by setting next_over_limit_check_at to NULL and marking free_user_cap_over_limit_notified_at with the current timestamp. - Add a finder to query the DB for groups that could potentially be over limit according to the free user criteria. Should make it easy to adapt the parameters if we pivot to another method for determining the cap. - Rename reached_free_user_limit_email to over_free_user_limit_email - Make the data_consistency of the worker :always as stated in the updated guidelines. Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/101579 - Add feature flag: free_user_cap_over_user_limit_mails - Use feature flags to guard over limit workers - Exit worker softly when owner or group doesn't exist - No use making sidekiq retry the job multiple times when it can't succeed due to users or groups having been deleted since it was queued for execution. - Trigger one notification service per group - The notification job uses a service that takes a group and will send multiple mails (one to each owner). This makes it so that at most per group we will only check over_limit? once. - We can not send a single email with all owners in BCC because they might have different locales. - Check over limit once and add date of check to email - In order so save on DB cycles we will only check the over_limit? status of a group once in the notify service. - Refs: https://gitlab.com/gitlab-org/gitlab/-/issues/367549 - Refs: https://gitlab.com/gitlab-org/gitlab/-/issues/367549#note_1158849796 - Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1118970328 - Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1120600468 - Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1118970365 - Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1120564893 - Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1120594491 - Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1120387163 - Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1154445752 - Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1153857483 - Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1156889118 - Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1156889105 - Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1183663760 - Refs: https://gitlab.com/gitlab-org/gitlab/-/jobs/3190747249 - Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1196618950 - Refs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1234377864 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1267173125 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1267173138 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1267173151 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1267173100 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1270251056 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438#note_1270251038 EE:true Changelog: changed Remove Scheduling worker - OverLimitNotificationWorker now has the ability to pull in groups that need checking on it's own we don't need a dedicated worker to set the schedule anymore - Upadate Milestone - Fix ee/ Namespace::Detail spec - Remove superfluous scope from ee/group --- app/models/namespace/detail.rb | 2 + .../free_user_cap_over_user_limit_mails.yml | 8 ++ config/sidekiq_queues.yml | 2 + ...ver_limit_check_at_to_namespace_details.rb | 20 +++++ db/schema_migrations/20221219112632 | 1 + db/structure.sql | 3 +- .../enforceable_groups_finder.rb | 14 +++ ee/app/mailers/ee/preview/notify_preview.rb | 5 +- ee/app/mailers/emails/free_user_cap.rb | 3 +- ee/app/models/ee/namespace/detail.rb | 27 ++++++ ee/app/models/namespaces/free_user_cap.rb | 4 + .../notify_over_limit_groups_service.rb | 57 +++++++++++++ ...l => over_free_user_limit_email.html.haml} | 2 +- .../over_free_user_limit_email.text.erb | 16 ++++ .../reached_free_user_limit_email.text.erb | 16 ---- ee/app/workers/all_queues.yml | 18 ++++ .../backfill_notification_jobs_worker.rb | 23 +++++ .../over_limit_notification_worker.rb | 83 ++++++++++++++++++ .../enforceable_groups_finder_spec.rb | 41 +++++++++ ee/spec/mailers/emails/free_user_cap_spec.rb | 8 +- ee/spec/models/ee/namespace/detail_spec.rb | 69 +++++++++++++++ .../models/namespaces/free_user_cap_spec.rb | 16 ++++ .../notify_over_limit_groups_service_spec.rb | 85 +++++++++++++++++++ .../backfill_notification_jobs_worker_spec.rb | 18 ++++ .../over_limit_notification_worker_spec.rb | 69 +++++++++++++++ locale/gitlab.pot | 2 +- spec/workers/every_sidekiq_worker_spec.rb | 1 + 27 files changed, 588 insertions(+), 25 deletions(-) create mode 100644 config/feature_flags/development/free_user_cap_over_user_limit_mails.yml create mode 100644 db/migrate/20221219112632_add_next_over_limit_check_at_to_namespace_details.rb create mode 100644 db/schema_migrations/20221219112632 create mode 100644 ee/app/finders/namespaces/free_user_cap/enforceable_groups_finder.rb create mode 100644 ee/app/models/ee/namespace/detail.rb create mode 100644 ee/app/services/namespaces/free_user_cap/notify_over_limit_groups_service.rb rename ee/app/views/notify/{reached_free_user_limit_email.html.haml => over_free_user_limit_email.html.haml} (72%) create mode 100644 ee/app/views/notify/over_free_user_limit_email.text.erb delete mode 100644 ee/app/views/notify/reached_free_user_limit_email.text.erb create mode 100644 ee/app/workers/namespaces/free_user_cap/backfill_notification_jobs_worker.rb create mode 100644 ee/app/workers/namespaces/free_user_cap/over_limit_notification_worker.rb create mode 100644 ee/spec/finders/namespaces/free_user_cap/enforceable_groups_finder_spec.rb create mode 100644 ee/spec/models/ee/namespace/detail_spec.rb create mode 100644 ee/spec/services/namespaces/free_user_cap/notify_over_limit_groups_service_spec.rb create mode 100644 ee/spec/workers/namespaces/free_user_cap/backfill_notification_jobs_worker_spec.rb create mode 100644 ee/spec/workers/namespaces/free_user_cap/over_limit_notification_worker_spec.rb diff --git a/app/models/namespace/detail.rb b/app/models/namespace/detail.rb index a5643ab9f790d8..2660d11171e892 100644 --- a/app/models/namespace/detail.rb +++ b/app/models/namespace/detail.rb @@ -11,3 +11,5 @@ class Namespace::Detail < ApplicationRecord self.primary_key = :namespace_id end + +Namespace::Detail.prepend_mod diff --git a/config/feature_flags/development/free_user_cap_over_user_limit_mails.yml b/config/feature_flags/development/free_user_cap_over_user_limit_mails.yml new file mode 100644 index 00000000000000..88b3f59050d1c6 --- /dev/null +++ b/config/feature_flags/development/free_user_cap_over_user_limit_mails.yml @@ -0,0 +1,8 @@ +--- +name: free_user_cap_over_user_limit_mails +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98438 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/378616 +milestone: '15.9' +type: development +group: group::acquisition +default_enabled: false diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 067c4730f0adac..433554b7a5afcf 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -335,6 +335,8 @@ - 1 - - migrate_external_diffs - 1 +- - namespaces_free_user_cap_over_limit_notification + - 1 - - namespaces_process_sync_events - 1 - - namespaces_sync_namespace_name diff --git a/db/migrate/20221219112632_add_next_over_limit_check_at_to_namespace_details.rb b/db/migrate/20221219112632_add_next_over_limit_check_at_to_namespace_details.rb new file mode 100644 index 00000000000000..dd2acbfd0bbf7e --- /dev/null +++ b/db/migrate/20221219112632_add_next_over_limit_check_at_to_namespace_details.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class AddNextOverLimitCheckAtToNamespaceDetails < Gitlab::Database::Migration[2.1] + disable_ddl_transaction! + + TABLE_NAME = :namespace_details + COLUMN = :next_over_limit_check_at + + def up + with_lock_retries do + add_column TABLE_NAME, COLUMN, :datetime_with_timezone, null: true + end + end + + def down + with_lock_retries do + remove_column TABLE_NAME, COLUMN + end + end +end diff --git a/db/schema_migrations/20221219112632 b/db/schema_migrations/20221219112632 new file mode 100644 index 00000000000000..0bba0080af799d --- /dev/null +++ b/db/schema_migrations/20221219112632 @@ -0,0 +1 @@ +400cab0a2d3130dd7406024cf982c7312918019197ae06af06696435f6bb5aaa \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index c1a1ac21349722..bb9ea21db9ba5d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -18279,7 +18279,8 @@ CREATE TABLE namespace_details ( free_user_cap_over_limt_notified_at timestamp with time zone, free_user_cap_over_limit_notified_at timestamp with time zone, dashboard_notification_at timestamp with time zone, - dashboard_enforcement_at timestamp with time zone + dashboard_enforcement_at timestamp with time zone, + next_over_limit_check_at timestamp with time zone ); CREATE TABLE namespace_limits ( diff --git a/ee/app/finders/namespaces/free_user_cap/enforceable_groups_finder.rb b/ee/app/finders/namespaces/free_user_cap/enforceable_groups_finder.rb new file mode 100644 index 00000000000000..895d9e5f6f490a --- /dev/null +++ b/ee/app/finders/namespaces/free_user_cap/enforceable_groups_finder.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module Namespaces + module FreeUserCap + class EnforceableGroupsFinder + def execute + Group + .in_default_plan + .top_most + .non_public_only + end + end + end +end diff --git a/ee/app/mailers/ee/preview/notify_preview.rb b/ee/app/mailers/ee/preview/notify_preview.rb index 590e4ce1c09154..605d39aa461203 100644 --- a/ee/app/mailers/ee/preview/notify_preview.rb +++ b/ee/app/mailers/ee/preview/notify_preview.rb @@ -3,6 +3,7 @@ module EE module Preview module NotifyPreview + FREE_USER_CAP_OVERLIMIT_CHECKED_AT = "2022-11-07 14:33:44 UTC" extend ActiveSupport::Concern # We need to define the methods on the prepender directly because: @@ -80,8 +81,8 @@ def user_cap_reached ::Notify.user_cap_reached(user.id).message end - def reached_free_user_limit_email - ::Notify.reached_free_user_limit_email(user, group).message + def over_free_user_limit_email + ::Notify.over_free_user_limit_email(user, group, FREE_USER_CAP_OVERLIMIT_CHECKED_AT).message end def confirmation_instructions_email diff --git a/ee/app/mailers/emails/free_user_cap.rb b/ee/app/mailers/emails/free_user_cap.rb index 85a20c445c8c7e..0c814694c2f79f 100644 --- a/ee/app/mailers/emails/free_user_cap.rb +++ b/ee/app/mailers/emails/free_user_cap.rb @@ -2,13 +2,14 @@ module Emails module FreeUserCap - def reached_free_user_limit_email(user, namespace) + def over_free_user_limit_email(user, namespace, checked_at) email = user.notification_email_or_default @start_trial_url = new_trial_url @upgrade_url = group_billings_url namespace @manage_users_url = group_usage_quotas_url namespace, anchor: 'seats-quota-tab' @namespace_name = namespace.name + @checked_at = Time.zone.parse checked_at.to_s email_with_layout( to: email, diff --git a/ee/app/models/ee/namespace/detail.rb b/ee/app/models/ee/namespace/detail.rb new file mode 100644 index 00000000000000..2609fdc7bb22f1 --- /dev/null +++ b/ee/app/models/ee/namespace/detail.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module EE + module Namespace + module Detail + extend ActiveSupport::Concern + extend ::Gitlab::Utils::Override + + prepended do + scope :not_over_limit_notified, -> { where free_user_cap_over_limit_notified_at: nil } + + scope :scheduled_for_over_limit_check, -> do + where(next_over_limit_check_at: ..Time.current) + .or(where(next_over_limit_check_at: nil)) + .order(arel_table[:next_over_limit_check_at].asc.nulls_first) + end + + scope :lock_for_over_limit_check, ->(limit, namespace_ids) do + scheduled_for_over_limit_check + .where(namespace_id: namespace_ids) + .limit(limit) + .lock('FOR UPDATE SKIP LOCKED') + end + end + end + end +end diff --git a/ee/app/models/namespaces/free_user_cap.rb b/ee/app/models/namespaces/free_user_cap.rb index c1e183acd79dec..a0e587ac3dcdf7 100644 --- a/ee/app/models/namespaces/free_user_cap.rb +++ b/ee/app/models/namespaces/free_user_cap.rb @@ -14,6 +14,10 @@ def self.notification_or_enforcement_enabled?(namespace) ::Namespaces::FreeUserCap::Enforcement.new(namespace).enforce_cap? end + def self.over_user_limit_mails_enabled? + ::Feature.enabled?(:free_user_cap_over_user_limit_mails) + end + def self.dashboard_limit ::Gitlab::CurrentSettings.dashboard_limit end diff --git a/ee/app/services/namespaces/free_user_cap/notify_over_limit_groups_service.rb b/ee/app/services/namespaces/free_user_cap/notify_over_limit_groups_service.rb new file mode 100644 index 00000000000000..50821e05ce0dd8 --- /dev/null +++ b/ee/app/services/namespaces/free_user_cap/notify_over_limit_groups_service.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +module Namespaces + module FreeUserCap + class NotifyOverLimitGroupsService + ServiceError = Class.new(StandardError) + + attr_accessor :group + + def self.execute(group:) + new(group: group).execute + end + + def initialize(group:) + @group = group.root_ancestor + end + + def execute + return unless enforce_over_limit_mails? + return unless over_limit? + + notify + + ServiceResponse.success + rescue StandardError => e + ServiceResponse.error message: e.message + end + + private + + def over_limit? + Namespaces::FreeUserCap::Enforcement.new(@group).over_limit? + end + + def notify + email_owners + mark_group_as_notified! + end + + def checked_at + @checked_at ||= Time.current + end + + def enforce_over_limit_mails? + ::Namespaces::FreeUserCap.over_user_limit_mails_enabled? && ::Gitlab::CurrentSettings.dashboard_limit_enabled? + end + + def email_owners + @group.owners.each { |owner| ::Notify.over_free_user_limit_email(owner, @group, checked_at).deliver_now } + end + + def mark_group_as_notified! + @group.namespace_details.update free_user_cap_over_limit_notified_at: checked_at + end + end + end +end diff --git a/ee/app/views/notify/reached_free_user_limit_email.html.haml b/ee/app/views/notify/over_free_user_limit_email.html.haml similarity index 72% rename from ee/app/views/notify/reached_free_user_limit_email.html.haml rename to ee/app/views/notify/over_free_user_limit_email.html.haml index fcb7c431f03866..6e4ed090705810 100644 --- a/ee/app/views/notify/reached_free_user_limit_email.html.haml +++ b/ee/app/views/notify/over_free_user_limit_email.html.haml @@ -13,7 +13,7 @@ = s_("FreeUserCap|You've reached your member limit!") %p.spaced-lines - = s_("FreeUserCap|Looks like you've reached your limit of %{free_user_limit} members for \"%{namespace_name}\". You can't add any more, but you can manage your existing members, for example, by removing inactive members and replacing them with new members.") % { namespace_name: @namespace_name, free_user_limit: Namespaces::FreeUserCap.dashboard_limit } + = s_("FreeUserCap|It looks like you've reached your limit of %{free_user_limit} members for \"%{namespace_name}\", according to the check we ran on %{date_time}. You can't add any more, but you can manage your existing members, for example, by removing inactive members and replacing them with new members.") % { namespace_name: @namespace_name, free_user_limit: Namespaces::FreeUserCap.dashboard_limit, date_time: l(@checked_at, format: :long) } %p.spaced-lines{ style: 'margin-bottom: 24px;' } - end_link = "".html_safe diff --git a/ee/app/views/notify/over_free_user_limit_email.text.erb b/ee/app/views/notify/over_free_user_limit_email.text.erb new file mode 100644 index 00000000000000..bb1b8864a44f9e --- /dev/null +++ b/ee/app/views/notify/over_free_user_limit_email.text.erb @@ -0,0 +1,16 @@ +<%= s_("FreeUserCap|You've reached your member limit!") %> + +<%= s_("FreeUserCap|It looks like you've reached your limit of %{free_user_limit} members for \"%{namespace_name}\", according to the check we ran on %{date_time}. You can't add any more, but you can manage your existing members, for example, by removing inactive members and replacing them with new members.") % { namespace_name: @namespace_name, free_user_limit: Namespaces::FreeUserCap.dashboard_limit, date_time: l(@checked_at, format: :long) } %> + +<%= s_("FreeUserCap|To get more members start a trial:") %> +<%= @start_trial_url %> + +<%= s_("FreeUserCap|Alternatively you can upgrade to GitLab Premium or GitLab Ultimate:") %> +<%= @upgrade_url %> + +<%= s_("FreeUserCap|Manage members:") %> +<%= @manage_users_url %> + +<%= s_("FreeUserCap|Explore paid plans:") %> +<%= @upgrade_url %> + diff --git a/ee/app/views/notify/reached_free_user_limit_email.text.erb b/ee/app/views/notify/reached_free_user_limit_email.text.erb deleted file mode 100644 index 515bd65951faeb..00000000000000 --- a/ee/app/views/notify/reached_free_user_limit_email.text.erb +++ /dev/null @@ -1,16 +0,0 @@ -<%= s_("FreeUserCap|You've reached your member limit!") %> - -<%= s_("FreeUserCap|Looks like you've reached your limit of %{free_user_limit} members for \"%{namespace_name}\". You can't add any more, but you can manage your existing members, for example, by removing inactive members and replacing them with new members.") % { namespace_name: @namespace_name, free_user_limit: Namespaces::FreeUserCap.dashboard_limit } %> - -<%= s_("FreeUserCap|To get more members start a trial:") %> -<%= @start_trial_url %> - -<%= s_("FreeUserCap|Alternatively you can upgrade to GitLab Premium or GitLab Ultimate:") %> -<%= @upgrade_url %> - -<%= s_("FreeUserCap|Manage members:") %> -<%= @manage_users_url %> - -<%= s_("FreeUserCap|Explore paid plans:") %> -<%= @upgrade_url %> - diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 70f00ae29d1512..b376c660dbff10 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -426,6 +426,15 @@ :weight: 1 :idempotent: false :tags: [] +- :name: cronjob:namespaces_free_user_cap_backfill_notification_jobs + :worker_name: Namespaces::FreeUserCap::BackfillNotificationJobsWorker + :feature_category: :user_management + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: cronjob:projects_disable_legacy_open_source_license_for_inactive_projects :worker_name: Projects::DisableLegacyOpenSourceLicenseForInactiveProjectsWorker :feature_category: :projects @@ -1380,6 +1389,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: namespaces_free_user_cap_over_limit_notification + :worker_name: Namespaces::FreeUserCap::OverLimitNotificationWorker + :feature_category: :user_management + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: namespaces_sync_namespace_name :worker_name: Namespaces::SyncNamespaceNameWorker :feature_category: :saas_provisioning diff --git a/ee/app/workers/namespaces/free_user_cap/backfill_notification_jobs_worker.rb b/ee/app/workers/namespaces/free_user_cap/backfill_notification_jobs_worker.rb new file mode 100644 index 00000000000000..4d700029964a1c --- /dev/null +++ b/ee/app/workers/namespaces/free_user_cap/backfill_notification_jobs_worker.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Namespaces + module FreeUserCap + class BackfillNotificationJobsWorker + include ApplicationWorker + + # rubocop:disable Scalability/CronWorkerContext + # This worker does not perform work scoped to a context + include CronjobQueue + # rubocop:enable Scalability/CronWorkerContext + + feature_category :user_management + urgency :low + data_consistency :always + idempotent! + + def perform(...) + OverLimitNotificationWorker.perform_with_capacity(...) + end + end + end +end diff --git a/ee/app/workers/namespaces/free_user_cap/over_limit_notification_worker.rb b/ee/app/workers/namespaces/free_user_cap/over_limit_notification_worker.rb new file mode 100644 index 00000000000000..b375596513e63d --- /dev/null +++ b/ee/app/workers/namespaces/free_user_cap/over_limit_notification_worker.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +module Namespaces + module FreeUserCap + class OverLimitNotificationWorker + include ApplicationWorker + include LimitedCapacity::Worker + + feature_category :user_management + data_consistency :always + sidekiq_options retry: false + idempotent! + + MAX_RUNNING_JOBS = 5 + BATCH_SIZE = 1 + SCHEDULE_BUFFER_IN_HOURS = 24 + + def perform_work(*args) + next_batch.map(&:namespace_id).each do |namespace_id| + notify group_id: namespace_id + end + end + + def remaining_work_count(*args) + (Namespace::Detail.scheduled_for_over_limit_check.limit(MAX_RUNNING_JOBS * BATCH_SIZE).count / BATCH_SIZE).ceil + end + + def max_running_jobs + return 0 unless enforce_over_limit_mails? + + MAX_RUNNING_JOBS + end + + private + + def notify(group_id:, checked_at: Time.current) + return unless enforce_over_limit_mails? + + @group = Group.find_by id: group_id # rubocop: disable CodeReuse/ActiveRecord + + NotifyOverLimitGroupsService.execute(group: @group) if @group + end + + def next_batch + Namespace::Detail.transaction do + Namespace::Detail.find_by_sql next_batch_sql # rubocop: disable CodeReuse/ActiveRecord + end + end + + def next_batch_sql + # The alterntive to setting the timestamp would be using something like + # SET "next_over_limit_check_at" = NOW() + INTERVAL '#{SCHEDULE_BUFFER_IN_HOURS} hours' + # that makes it quite hared to spec, since the DB doesn't freeze time along with ruby + <<~SQL.squish + UPDATE "namespace_details" + SET "next_over_limit_check_at" = to_timestamp(#{schedule.to_i}) + WHERE "namespace_details"."namespace_id" IN (#{locked_item_ids_sql}) + RETURNING * + SQL + end + + def locked_item_ids_sql + Namespace::Detail + .not_over_limit_notified + .lock_for_over_limit_check(BATCH_SIZE, namespace_ids) + .select(:namespace_id) + .to_sql + end + + def namespace_ids + ::Namespaces::FreeUserCap::EnforceableGroupsFinder.new.execute.select :id + end + + def schedule + SCHEDULE_BUFFER_IN_HOURS.hours.from_now + end + + def enforce_over_limit_mails? + ::Namespaces::FreeUserCap.over_user_limit_mails_enabled? && ::Gitlab::CurrentSettings.dashboard_limit_enabled? + end + end + end +end diff --git a/ee/spec/finders/namespaces/free_user_cap/enforceable_groups_finder_spec.rb b/ee/spec/finders/namespaces/free_user_cap/enforceable_groups_finder_spec.rb new file mode 100644 index 00000000000000..7bfdcccae493ab --- /dev/null +++ b/ee/spec/finders/namespaces/free_user_cap/enforceable_groups_finder_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +# Interim feature category experimentation_conversion used here while waiting for +# https://gitlab.com/gitlab-com/www-gitlab-com/-/merge_requests/113300 to merge +RSpec.describe Namespaces::FreeUserCap::EnforceableGroupsFinder, feature_category: :experimentation_activation do + subject(:finder) { described_class.new.execute } + + before do + stub_ee_application_setting should_check_namespace_plan: true + end + + describe '#execute', :saas do + let_it_be(:private_free) { create :group_with_plan, :private, plan: :free_plan } + let_it_be(:public_free) { create :group_with_plan, :public, plan: :free_plan } + let_it_be(:private_premium) { create :group_with_plan, :private, plan: :premium_plan } + let_it_be(:private_premium_trial) do + create :group_with_plan, :private, plan: :premium_plan, trial_ends_on: Date.current.advance(months: 1) + end + + context 'with out being previously notified' do + it 'finds private free groups' do + expect(finder.count).to eq(1) + expect(finder.all).to match_array([private_free]) + end + end + + context 'when applicable namespace is not a root namespace' do + let_it_be(:parent) { create :group } + + before do + private_free.update! parent_id: parent.id + end + + it 'finds nothing' do + expect(finder.all).to eq([]) + end + end + end +end diff --git a/ee/spec/mailers/emails/free_user_cap_spec.rb b/ee/spec/mailers/emails/free_user_cap_spec.rb index 854f123d2f0a7e..a6598b5943c4cc 100644 --- a/ee/spec/mailers/emails/free_user_cap_spec.rb +++ b/ee/spec/mailers/emails/free_user_cap_spec.rb @@ -7,16 +7,18 @@ let_it_be(:user) { create :user } let_it_be(:namespace) { create :namespace, name: "foobar" } + let(:checked_at) { "1969-04-20 13:37:42 UTC" } - describe "#reached_free_user_limit_email" do - subject(:email) { Notify.reached_free_user_limit_email user, namespace } + describe "#over_free_user_limit_email" do + subject(:email) { Notify.over_free_user_limit_email user, namespace, checked_at } it "sends mail with expected contents" do allow(Namespaces::FreeUserCap).to receive(:dashboard_limit).and_return(10) expect(email).to have_subject("You've reached your member limit!") - expect(email).to have_body_text("Looks like you've reached your limit") + expect(email).to have_body_text("It looks like you've reached your limit") expect(email).to have_body_text("of 10 members") + expect(email).to have_body_text("according to the check we ran on April 20, 1969 13:37") expect(email).to have_body_text("Manage members") expect(email).to have_body_text("Explore paid plans") expect(email).to have_body_text("foobar/-/billings") diff --git a/ee/spec/models/ee/namespace/detail_spec.rb b/ee/spec/models/ee/namespace/detail_spec.rb new file mode 100644 index 00000000000000..cec9acc5a04884 --- /dev/null +++ b/ee/spec/models/ee/namespace/detail_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Namespace::Detail, type: :model, feature_category: :experimentation_conversion, ee: true do + context 'with scopes' do + def set_schedule(group, time, notified_at = nil) + group.namespace_details.update! next_over_limit_check_at: time, free_user_cap_over_limit_notified_at: notified_at + end + + let_it_be(:group_1) { create :group, :private } + let_it_be(:group_2) { create :group, :private } + let_it_be(:group_3) { create :group, :private } + let_it_be(:group_4) { create :group, :private } + let_it_be(:group_5) { create :group, :private } + + let(:scheduled_items) do + [ + group_1.namespace_details, + group_2.namespace_details, + group_3.namespace_details, + group_5.namespace_details + ] + end + + let(:not_notified_items) do + [ + group_1.namespace_details, + group_2.namespace_details, + group_3.namespace_details, + group_4.namespace_details + ] + end + + let(:namespace_ids) { ::Namespaces::FreeUserCap::EnforceableGroupsFinder.new.execute.select(:id) } + + before do + set_schedule group_1, nil + set_schedule group_2, 2.days.ago + set_schedule group_3, 1.5.days.ago + + set_schedule group_4, 3.days.from_now # not scheduled_item: next_over_limit_check_at too new + set_schedule group_5, 1.day.ago, 2.days.ago # not not_notified_items: has already been notified + end + + context 'for not_over_limit_notified' do + it 'returns only entries that have not been notified of being over limit' do + expect(described_class.not_over_limit_notified).to match_array(not_notified_items) + end + end + + context 'for scheduled_for_over_limit_check' do + it 'returns entries that have been scheduled for over limit checks' do + expect(described_class.scheduled_for_over_limit_check).to eq(scheduled_items) + end + end + + context 'for lock_for_over_limit_check' do + let(:limit) { 2 } + let(:scheduled_items) { [group_1.namespace_details, group_2.namespace_details] } + + subject(:lock_scope_result) { described_class.lock_for_over_limit_check(limit, namespace_ids) } + + it 'only returns scheduled entries up to the limit' do + expect(lock_scope_result).to eq(scheduled_items) + end + end + end +end diff --git a/ee/spec/models/namespaces/free_user_cap_spec.rb b/ee/spec/models/namespaces/free_user_cap_spec.rb index 9cf05bca3e283f..0ac9898ee81100 100644 --- a/ee/spec/models/namespaces/free_user_cap_spec.rb +++ b/ee/spec/models/namespaces/free_user_cap_spec.rb @@ -32,6 +32,22 @@ end end + describe '.over_user_limit_mails_enabled?' do + subject { described_class.over_user_limit_mails_enabled? } + + context 'when feature flag is enabled' do + it { is_expected.to be_truthy } + end + + context 'when feature flag is disabled' do + it 'reflects the state of the feature flag' do + stub_feature_flags free_user_cap_over_user_limit_mails: false + + expect(subject).to be_falsey + end + end + end + describe '.dashboard_limit' do subject { described_class.dashboard_limit } diff --git a/ee/spec/services/namespaces/free_user_cap/notify_over_limit_groups_service_spec.rb b/ee/spec/services/namespaces/free_user_cap/notify_over_limit_groups_service_spec.rb new file mode 100644 index 00000000000000..a817ec4a2cde69 --- /dev/null +++ b/ee/spec/services/namespaces/free_user_cap/notify_over_limit_groups_service_spec.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +require 'spec_helper' + +# Interim feature category experimentation_activation used here while waiting for +# https://gitlab.com/gitlab-com/www-gitlab-com/-/merge_requests/113300 to merge +RSpec.describe Namespaces::FreeUserCap::NotifyOverLimitGroupsService, feature_category: :experimentation_activation, + saas: true do + def create_enforcable_free_group(dashboard_limit: 2) + free_group = create :group_with_plan, :private, plan: :free_plan + create_list :group_member, (dashboard_limit + 1), :active, source: free_group + free_group + end + + let(:frozen_time) { Time.zone.parse "2022-09-22T00:00+0" } + let(:group) { create_enforcable_free_group } + + around do |example| + travel_to(frozen_time) { example.run } + end + + before do + # Change the limit after setup to avoid validation errors during setup + # - We need to create groups that are over the limit which isn't possible + # anymore with net-new namespaces + # @group = create_enforcable_free_group + group.add_owner create(:owner) + + stub_ee_application_setting dashboard_limit: limit + stub_ee_application_setting dashboard_enforcement_limit: limit + stub_ee_application_setting dashboard_limit_enabled: true + stub_ee_application_setting should_check_namespace_plan: true + end + + context "with over limit group" do + let(:limit) { 2 } + + describe '#execute', :saas do + let(:service) { described_class.new group: group } + + it 'records the time of notification in free_user_cap_over_limit_notified_at' do + expect { service.execute } + .to change { group.namespace_details.free_user_cap_over_limit_notified_at } + .from(nil) + .to(Time.zone.parse("2022-09-22T00:00:00.+0")) + + service.execute + end + end + + describe '.execute', :saas do + it 'emails the owner(s) of the group' do + group.owners.each do |owner| + expect(Notify).to receive(:over_free_user_limit_email).with(owner, group, frozen_time).once.and_call_original + end + + described_class.execute group: group + end + end + + describe 'error handling', :saas do + it 'rescues to a ServiceResponse' do + expect(Namespaces::FreeUserCap) + .to receive(:over_user_limit_mails_enabled?).and_raise(StandardError, 'How error doing?') + result = described_class.execute group: group + expect(result).to be_a(ServiceResponse) + expect(result).not_to be_success + end + end + end + + context 'when under the limit' do + let(:limit) { 42 } + + describe '#execute', :saas do + let(:service) { described_class.new group: group } + + it 'exits early' do + expect(Notify).not_to receive(:over_free_user_limit_email) + result = service.execute + expect(result).to be_nil + end + end + end +end diff --git a/ee/spec/workers/namespaces/free_user_cap/backfill_notification_jobs_worker_spec.rb b/ee/spec/workers/namespaces/free_user_cap/backfill_notification_jobs_worker_spec.rb new file mode 100644 index 00000000000000..d6c2eed248fd17 --- /dev/null +++ b/ee/spec/workers/namespaces/free_user_cap/backfill_notification_jobs_worker_spec.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true +require 'spec_helper' + +# Interim feature category experimentation_activation used here while waiting for +# https://gitlab.com/gitlab-com/www-gitlab-com/-/merge_requests/113300 to merge +RSpec.describe Namespaces::FreeUserCap::BackfillNotificationJobsWorker, + type: :worker, + feature_category: :experimentation_activation do + let(:worker) { described_class.new } + + describe '#perform' do + it 'adds a OverLimitNotificationWorker to the limited capacity worker pool' do + expect(Namespaces::FreeUserCap::OverLimitNotificationWorker).to receive(:perform_with_capacity) + + worker.perform + end + end +end diff --git a/ee/spec/workers/namespaces/free_user_cap/over_limit_notification_worker_spec.rb b/ee/spec/workers/namespaces/free_user_cap/over_limit_notification_worker_spec.rb new file mode 100644 index 00000000000000..38a6f75aaf481a --- /dev/null +++ b/ee/spec/workers/namespaces/free_user_cap/over_limit_notification_worker_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +# Interim feature category experimentation_activation used here while waiting for +# https://gitlab.com/gitlab-com/www-gitlab-com/-/merge_requests/113300 to merge +RSpec.describe Namespaces::FreeUserCap::OverLimitNotificationWorker, feature_category: :experimentation_activation, + type: :worker, + ee: true do + let(:frozen_time) { Time.zone.parse "2022-09-22T00:00+0" } + + around do |example| + travel_to(frozen_time) { example.run } + end + + describe '#perform', :saas do + subject(:worker) { described_class.new.perform } + + let(:group) { create :group_with_plan, :private, plan: :free_plan } + let(:owner) { create :owner } + + before do + group.add_owner owner + group.namespace_details.update! next_over_limit_check_at: 2.days.ago + end + + context 'when on gitlab.com', :saas do + before do + stub_ee_application_setting should_check_namespace_plan: true + stub_ee_application_setting dashboard_limit_enabled: true + end + + it 'runs notifiy service and marks next check for group' do + expect(::Namespaces::FreeUserCap::NotifyOverLimitGroupsService).to receive(:execute) + + next_check_time = frozen_time + described_class::SCHEDULE_BUFFER_IN_HOURS.hours + + expect { subject }.to change { group.reload.namespace_details.next_over_limit_check_at }.to(next_check_time) + end + end + + context 'with feature flags enabled/disabled' do + where( + limit_enabled: [true, false, false], + free_user_cap_over_user_limit_mails: [false, true, false] + ) + + before do + stub_ee_application_setting dashboard_limit_enabled: limit_enabled + stub_ee_application_setting should_check_namespace_plan: true + stub_feature_flags free_user_cap_over_user_limit_mails: free_user_cap_over_user_limit_mails + end + + with_them do + it 'triggers mail the namespace owners', :aggregate_failures do + if limit_enabled && free_user_cap_over_user_limit_mails + expect(::Namespaces::FreeUserCap::NotifyOverLimitGroupsService).to receive(:execute) + expect(described_class.new.max_running_jobs).to eq(5) + else + expect(::Namespaces::FreeUserCap::NotifyOverLimitGroupsService).not_to receive(:execute) + expect(described_class.new.max_running_jobs).to eq(0) + end + + subject + end + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index a4f3abb69e87df..642ca5e76d1e07 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -17937,7 +17937,7 @@ msgstr "" msgid "FreeUserCap|Explore paid plans:" msgstr "" -msgid "FreeUserCap|Looks like you've reached your limit of %{free_user_limit} members for \"%{namespace_name}\". You can't add any more, but you can manage your existing members, for example, by removing inactive members and replacing them with new members." +msgid "FreeUserCap|It looks like you've reached your limit of %{free_user_limit} members for \"%{namespace_name}\", according to the check we ran on %{date_time}. You can't add any more, but you can manage your existing members, for example, by removing inactive members and replacing them with new members." msgstr "" msgid "FreeUserCap|Manage members" diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index c444e1f383cdb6..6e306cd18ac3d1 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -363,6 +363,7 @@ 'Onboarding::PipelineCreatedWorker' => 3, 'Onboarding::ProgressWorker' => 3, 'Onboarding::UserAddedWorker' => 3, + 'Namespaces::FreeUserCap::OverLimitNotificationWorker' => false, 'Namespaces::RefreshRootStatisticsWorker' => 3, 'Namespaces::RootStatisticsWorker' => 3, 'Namespaces::ScheduleAggregationWorker' => 3, -- GitLab