Introduce concern to update highest role

Avoid calling a service from the model layer by removing the service and
introducing a concern with its logic.

app/models/concerns/update_highest_role.rb

+# frozen_string_literal: true
+
+module UpdateHighestRole
+  extend ActiveSupport::Concern
+
+  HIGHEST_ROLE_LEASE_TIMEOUT = 10.minutes.to_i
+  HIGHEST_ROLE_JOB_DELAY = 10.minutes
+
+  included do
+    after_commit :update_highest_role
+  end
+
+  private
+
+  # Schedule a Sidekiq job to update the highest role for a User
+  #
+  # The job will be called outside of a transaction in order to ensure the changes
+  # to be commited before attempting to update the highest role.
+  # The exlusive lease will not be released after completion to prevent multiple jobs
+  # being executed during the defined timeout.
+  def update_highest_role
+    return unless update_highest_role?
+
+    run_after_commit_or_now do
+      lease_key = "update_highest_role:#{update_highest_role_attribute}"
+      lease = Gitlab::ExclusiveLease.new(lease_key, timeout: HIGHEST_ROLE_LEASE_TIMEOUT)
+
+      if lease.try_obtain
+        UpdateHighestRoleWorker.perform_in(HIGHEST_ROLE_JOB_DELAY, update_highest_role_attribute)
+      else
+        # use same logging as ExclusiveLeaseGuard
+        # rubocop:disable Gitlab/RailsLogger
+        Rails.logger.error('Cannot obtain an exclusive lease. There must be another instance already in execution.')
+        # rubocop:enable Gitlab/RailsLogger
+      end
+    end
+  end
+end

app/models/member.rb

@@ -9,6 +9,7 @@ class Member < ApplicationRecord
   include Presentable
   include Gitlab::Utils::StrongMemoize
   include FromUnion
+  include UpdateHighestRole
 
   attr_accessor :raw_invite_token
 
@@ -100,7 +101,6 @@ class Member < ApplicationRecord
   after_destroy :destroy_notification_setting
   after_destroy :post_destroy_hook, unless: :pending?
   after_commit :refresh_member_authorized_projects
-  after_commit :update_highest_role
 
   default_value_for :notification_level, NotificationSetting.levels[:global]
 
@@ -463,21 +463,15 @@ class Member < ApplicationRecord
     end
   end
 
-  # Triggers the service to schedule a Sidekiq job to update the highest role
-  # for a User
-  #
-  # The job will be called outside of a transaction in order to ensure the changes
-  # for a Member to be commited before attempting to update the highest role.
-  # rubocop: disable CodeReuse/ServiceClass
-  def update_highest_role
+  def update_highest_role?
     return unless user_id.present?
-    return unless previous_changes[:access_level].present?
 
-    run_after_commit_or_now do
-      Members::UpdateHighestRoleService.new(user_id).execute
-    end
+    previous_changes[:access_level].present?
+  end
+
+  def update_highest_role_attribute
+    user_id
   end
-  # rubocop: enable CodeReuse/ServiceClass
 end
 
 Member.prepend_if_ee('EE::Member')

app/models/user.rb

@@ -23,6 +23,7 @@ class User < ApplicationRecord
   include BatchDestroyDependentAssociations
   include HasUniqueInternalUsers
   include IgnorableColumns
+  include UpdateHighestRole
 
   DEFAULT_NOTIFICATION_LEVEL = :participating
 
@@ -238,7 +239,6 @@ class User < ApplicationRecord
       end
     end
   end
-  after_commit :update_highest_role, on: [:create, :update]
 
   after_initialize :set_projects_limit
 
@@ -1854,20 +1854,15 @@ class User < ApplicationRecord
     last_active_at.to_i <= MINIMUM_INACTIVE_DAYS.days.ago.to_i
   end
 
-  # Triggers the service to schedule a Sidekiq job to update the highest role
-  # for a User
-  #
-  # The job will be called outside of a transaction in order to ensure the changes
-  # for a Member to be commited before attempting to update the highest role.
-  # rubocop: disable CodeReuse/ServiceClass
-  def update_highest_role
-    return unless (previous_changes.keys & %w(state user_type ghost)).any?
+  def update_highest_role?
+    return false unless persisted?
 
-    run_after_commit_or_now do
-      Members::UpdateHighestRoleService.new(id).execute
-    end
+    (previous_changes.keys & %w(state user_type ghost)).any?
+  end
+
+  def update_highest_role_attribute
+    id
   end
-  # rubocop: enable CodeReuse/ServiceClass
 end
 
 User.prepend_if_ee('EE::User')

app/services/members/update_highest_role_service.rb

-# frozen_string_literal: true
-
-module Members
-  class UpdateHighestRoleService < ::BaseService
-    include ExclusiveLeaseGuard
-
-    LEASE_TIMEOUT = 10.minutes.to_i
-    DELAY = 10.minutes
-
-    attr_reader :user_id
-
-    def initialize(user_id)
-      @user_id = user_id
-    end
-
-    def execute
-      try_obtain_lease do
-        UpdateHighestRoleWorker.perform_in(DELAY, user_id)
-      end
-    end
-
-    private
-
-    def lease_key
-      "update_highest_role:#{user_id}"
-    end
-
-    def lease_timeout
-      LEASE_TIMEOUT
-    end
-
-    # Do not release the lease before the timeout to
-    # prevent multiple jobs being executed during the
-    # defined timeout
-    def lease_release?
-      false
-    end
-  end
-end

changelogs/unreleased/introduce_update_highest_role_concern.yml

+---
+title: Use concern instead of service to update highest role
+merge_request: 28791
+author:
+type: other

spec/models/member_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Member do
+  include ExclusiveLeaseHelpers
+
   using RSpec::Parameterized::TableSyntax
 
   describe "Associations" do
@@ -593,6 +595,9 @@ describe Member do
   end
 
   context 'when after_commit :update_highest_role' do
+    let!(:user) { create(:user) }
+    let(:user_id) { user.id }
+
     where(:member_type, :source_type) do
       :project_member | :project
       :group_member   | :group
@@ -600,43 +605,34 @@ describe Member do
 
     with_them do
       describe 'create member' do
-        it 'initializes a new Members::UpdateHighestRoleService object' do
-          source = create(source_type) # source owner initializes a new service object too
-          user = create(:user)
+        let!(:source) { create(source_type) }
 
-          expect(Members::UpdateHighestRoleService).to receive(:new).with(user.id).and_call_original
+        subject { create(member_type, :guest, user: user, source_type => source) }
 
-          create(member_type, :guest, user: user, source_type => source)
-        end
+        include_examples 'update highest role with exclusive lease'
       end
 
       context 'when member exists' do
-        let!(:member) { create(member_type) }
+        let!(:member) { create(member_type, user: user) }
 
         describe 'update member' do
           context 'when access level was changed' do
-            it 'initializes a new Members::UpdateHighestRoleService object' do
-              expect(Members::UpdateHighestRoleService).to receive(:new).with(member.user_id).and_call_original
+            subject { member.update(access_level: Gitlab::Access::GUEST) }
 
-              member.update(access_level: Gitlab::Access::GUEST)
-            end
+            include_examples 'update highest role with exclusive lease'
           end
 
           context 'when access level was not changed' do
-            it 'does not initialize a new Members::UpdateHighestRoleService object' do
-              expect(Members::UpdateHighestRoleService).not_to receive(:new).with(member.user_id)
+            subject { member.update(notification_level: NotificationSetting.levels[:disabled]) }
 
-              member.update(notification_level: NotificationSetting.levels[:disabled])
-            end
+            include_examples 'does not update the highest role'
           end
         end
 
         describe 'destroy member' do
-          it 'initializes a new Members::UpdateHighestRoleService object' do
-            expect(Members::UpdateHighestRoleService).to receive(:new).with(member.user_id).and_call_original
+          subject { member.destroy }
 
-            member.destroy
-          end
+          include_examples 'update highest role with exclusive lease'
         end
       end
     end

spec/models/user_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 describe User, :do_not_mock_admin_mode do
   include ProjectForksHelper
   include TermsHelper
+  include ExclusiveLeaseHelpers
 
   it_behaves_like 'having unique enum values'
 
@@ -4535,17 +4536,22 @@ describe User, :do_not_mock_admin_mode do
 
   context 'when after_commit :update_highest_role' do
     describe 'create user' do
-      it 'initializes a new Members::UpdateHighestRoleService object' do
-        expect_next_instance_of(Members::UpdateHighestRoleService) do |service|
-          expect(service).to receive(:execute)
+      subject { create(:user) }
+
+      it 'schedules a job in the future', :aggregate_failures, :clean_gitlab_redis_shared_state do
+        allow_next_instance_of(Gitlab::ExclusiveLease) do |instance|
+          allow(instance).to receive(:try_obtain).and_return('uuid')
         end
 
-        create(:user)
+        expect(UpdateHighestRoleWorker).to receive(:perform_in).and_call_original
+
+        expect { subject }.to change(UpdateHighestRoleWorker.jobs, :size).by(1)
       end
     end
 
     context 'when user already exists' do
       let!(:user) { create(:user) }
+      let(:user_id) { user.id }
 
       describe 'update user' do
         using RSpec::Parameterized::TableSyntax
@@ -4560,24 +4566,24 @@ describe User, :do_not_mock_admin_mode do
 
         with_them do
           context 'when state was changed' do
-            it 'initializes a new Members::UpdateHighestRoleService object' do
-              expect_next_instance_of(Members::UpdateHighestRoleService) do |service|
-                expect(service).to receive(:execute)
-              end
+            subject { user.update(attributes) }
 
-              user.update(attributes)
-            end
+            include_examples 'update highest role with exclusive lease'
           end
         end
 
         context 'when state was not changed' do
-          it 'does not initialize a new Members::UpdateHighestRoleService object' do
-            expect(Members::UpdateHighestRoleService).not_to receive(:new)
+          subject { user.update(email: 'newmail@example.com') }
 
-            user.update(email: 'newmail@example.com')
-          end
+          include_examples 'does not update the highest role'
         end
       end
+
+      describe 'destroy user' do
+        subject { user.destroy }
+
+        include_examples 'does not update the highest role'
+      end
     end
   end
 

spec/services/members/update_highest_role_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-require 'sidekiq/testing'
-
-describe Members::UpdateHighestRoleService, :clean_gitlab_redis_shared_state do
-  include ExclusiveLeaseHelpers
-
-  let_it_be(:user) { create(:user) }
-  let_it_be(:lease_key) { "update_highest_role:#{user.id}" }
-  let(:service) { described_class.new(user.id) }
-
-  describe '#perform' do
-    subject { service.execute }
-
-    context 'when lease is obtained' do
-      it 'takes the lease but does not release it', :aggregate_failures do
-        expect_to_obtain_exclusive_lease(lease_key, 'uuid', timeout: described_class::LEASE_TIMEOUT)
-
-        subject
-
-        expect(service.exclusive_lease.exists?).to be_truthy
-      end
-
-      it 'schedules a job in the future', :aggregate_failures do
-        expect(UpdateHighestRoleWorker).to receive(:perform_in).with(described_class::DELAY, user.id).and_call_original
-
-        Sidekiq::Testing.fake! do
-          expect { subject }.to change(UpdateHighestRoleWorker.jobs, :size).by(1)
-        end
-      end
-    end
-
-    context 'when lease cannot be obtained' do
-      it 'only schedules one job' do
-        Sidekiq::Testing.fake! do
-          stub_exclusive_lease_taken(lease_key, timeout: described_class::LEASE_TIMEOUT)
-
-          expect { subject }.not_to change(UpdateHighestRoleWorker.jobs, :size)
-        end
-      end
-    end
-  end
-end

spec/support/shared_examples/models/update_highest_role_shared_examples.rb

+# frozen_string_literal: true
+
+# requires a subject and a user_id
+RSpec.shared_examples 'update highest role with exclusive lease' do
+  include ExclusiveLeaseHelpers
+
+  let(:lease_key) { "update_highest_role:#{user_id}" }
+
+  before do
+    allow(Gitlab::ExclusiveLease).to receive(:new).and_call_original
+  end
+
+  context 'when lease is obtained', :clean_gitlab_redis_shared_state do
+    it 'takes the lease but does not release it', :aggregate_failures do
+      expect_to_obtain_exclusive_lease(lease_key, 'uuid', timeout: described_class::HIGHEST_ROLE_LEASE_TIMEOUT)
+      expect(Gitlab::ExclusiveLease).not_to receive(:cancel).with(lease_key, 'uuid')
+
+      subject
+    end
+
+    it 'schedules a job in the future', :aggregate_failures do
+      allow_next_instance_of(Gitlab::ExclusiveLease) do |instance|
+        allow(instance).to receive(:try_obtain).and_return('uuid')
+      end
+
+      expect(UpdateHighestRoleWorker).to receive(:perform_in).with(described_class::HIGHEST_ROLE_JOB_DELAY, user_id).and_call_original
+
+      expect { subject }.to change(UpdateHighestRoleWorker.jobs, :size).by(1)
+    end
+  end
+
+  context 'when lease cannot be obtained', :clean_gitlab_redis_shared_state do
+    it 'only schedules one job' do
+      stub_exclusive_lease_taken(lease_key, timeout: described_class::HIGHEST_ROLE_LEASE_TIMEOUT)
+
+      expect { subject }.not_to change(UpdateHighestRoleWorker.jobs, :size)
+    end
+  end
+end
+
+# requires a subject and a user_id
+RSpec.shared_examples 'does not update the highest role' do
+  it 'does not obtain an exclusive lease' do
+    allow(Gitlab::ExclusiveLease).to receive(:new).and_call_original
+
+    lease = stub_exclusive_lease("update_highest_role:#{user_id}", 'uuid', timeout: described_class::HIGHEST_ROLE_LEASE_TIMEOUT)
+
+    expect(lease).not_to receive(:try_obtain)
+
+    subject
+  end
+end