From 068f3bf8706d3375ae0949e542a2ef6b20284aea Mon Sep 17 00:00:00 2001
From: Doug Stull <dstull@gitlab.com>
Date: Wed, 11 Dec 2024 16:03:10 -0500
Subject: [PATCH] Move away from super.tap usage in user assignment creation
- currently we are relying on a cached check if the user
is already assigned to be cached before the work is
done to assign the user. Then after that user is added
we have a check to see that the user isn't already assigned
based on the cache only to retain that knowledge.
It would be better to merely call this on success if
the user is actually assigned during the service execute,
which is what we are trying to measure here.
Changing this could also better insulate us from future
regressions as it is more declaritive about when and
why the work happens.
- see https://gitlab.com/gitlab-org/gitlab/-/issues/508955
---
.../base_create_service.rb | 10 ++++++++--
.../saas/create_service.rb | 13 ++++++------
.../create_without_notification_service.rb | 2 --
.../self_managed/create_service.rb | 20 +++++++------------
4 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/ee/app/services/gitlab_subscriptions/user_add_on_assignments/base_create_service.rb b/ee/app/services/gitlab_subscriptions/user_add_on_assignments/base_create_service.rb
index 6463a7fcae1e..727dd7ac1562 100644
--- a/ee/app/services/gitlab_subscriptions/user_add_on_assignments/base_create_service.rb
+++ b/ee/app/services/gitlab_subscriptions/user_add_on_assignments/base_create_service.rb
@@ -38,9 +38,11 @@ def execute
Rails.cache.delete(user.duo_pro_cache_key_formatted)
log_event('User AddOn assignment created')
-
- ServiceResponse.success
end
+
+ after_success_hook
+
+ ServiceResponse.success
rescue NoSeatsAvailableError => error
Gitlab::ErrorTracking.log_exception(
error, base_log_params.merge({ message: 'User AddOn assignment creation failed' })
@@ -53,6 +55,10 @@ def execute
attr_reader :add_on_purchase, :user
+ def after_success_hook
+ # overridden in inheriting classes.
+ end
+
def validate
return ERROR_NO_SEATS_AVAILABLE unless seats_available?
diff --git a/ee/app/services/gitlab_subscriptions/user_add_on_assignments/saas/create_service.rb b/ee/app/services/gitlab_subscriptions/user_add_on_assignments/saas/create_service.rb
index b86990aae502..8d9d8588720b 100644
--- a/ee/app/services/gitlab_subscriptions/user_add_on_assignments/saas/create_service.rb
+++ b/ee/app/services/gitlab_subscriptions/user_add_on_assignments/saas/create_service.rb
@@ -4,16 +4,15 @@ module GitlabSubscriptions
module UserAddOnAssignments
module Saas
class CreateService < ::GitlabSubscriptions::UserAddOnAssignments::Saas::CreateWithoutNotificationService
- def execute
- super.tap do |response|
- create_iterable_trigger if should_trigger_duo_iterable? response
- end
- end
+ extend ::Gitlab::Utils::Override
private
- def should_trigger_duo_iterable?(response)
- response.success? && duo_pro_or_enterprise? && !user_already_assigned?
+ override :after_success_hook
+ def after_success_hook
+ super
+
+ create_iterable_trigger if duo_pro_or_enterprise?
end
def duo_pro_or_enterprise?
diff --git a/ee/app/services/gitlab_subscriptions/user_add_on_assignments/saas/create_without_notification_service.rb b/ee/app/services/gitlab_subscriptions/user_add_on_assignments/saas/create_without_notification_service.rb
index ab334abf40a1..f79a45c8ab3e 100644
--- a/ee/app/services/gitlab_subscriptions/user_add_on_assignments/saas/create_without_notification_service.rb
+++ b/ee/app/services/gitlab_subscriptions/user_add_on_assignments/saas/create_without_notification_service.rb
@@ -8,8 +8,6 @@ class CreateWithoutNotificationService < ::GitlabSubscriptions::UserAddOnAssignm
private
- attr_reader :add_on_purchase, :user
-
def eligible_for_gitlab_duo_pro_seat?
namespace.eligible_for_gitlab_duo_pro_seat?(user)
end
diff --git a/ee/app/services/gitlab_subscriptions/user_add_on_assignments/self_managed/create_service.rb b/ee/app/services/gitlab_subscriptions/user_add_on_assignments/self_managed/create_service.rb
index 57d91f078bb6..88704aff0aa4 100644
--- a/ee/app/services/gitlab_subscriptions/user_add_on_assignments/self_managed/create_service.rb
+++ b/ee/app/services/gitlab_subscriptions/user_add_on_assignments/self_managed/create_service.rb
@@ -5,16 +5,16 @@ module UserAddOnAssignments
module SelfManaged
class CreateService < ::GitlabSubscriptions::UserAddOnAssignments::BaseCreateService
include Gitlab::Utils::StrongMemoize
-
- def execute
- super.tap do |response|
- send_duo_seat_assignment_email if should_send_duo_seat_assignment_email? response
- end
- end
+ extend ::Gitlab::Utils::Override
private
- attr_reader :add_on_purchase, :user
+ override :after_success_hook
+ def after_success_hook
+ super
+
+ send_duo_seat_assignment_email if Feature.enabled?(:duo_seat_assignment_email_for_sm, :instance)
+ end
def eligible_for_gitlab_duo_pro_seat?
user.eligible_for_self_managed_gitlab_duo_pro?
@@ -25,12 +25,6 @@ def send_duo_seat_assignment_email
DuoSeatAssignmentMailer.duo_pro_email(user).deliver_later if add_on_purchase.add_on.code_suggestions?
DuoSeatAssignmentMailer.duo_enterprise_email(user).deliver_later if add_on_purchase.add_on.duo_enterprise?
end
-
- def should_send_duo_seat_assignment_email?(response)
- Feature.enabled?(:duo_seat_assignment_email_for_sm, :instance) &&
- !user_already_assigned? &&
- response.success?
- end
end
end
end
--
GitLab