Add User callback to update highest role

Create, update or destroy the highest role for a User when they were
created, updated or destroyed. If it changed then schedule a job with an
exclusive lease to update it.

app/models/user.rb

@@ -237,6 +237,7 @@ class User < ApplicationRecord
       end
     end
   end
+  after_commit :update_highest_role, on: [:create, :update]
 
   after_initialize :set_projects_limit
 
@@ -1844,6 +1845,21 @@ class User < ApplicationRecord
   def no_recent_activity?
     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?
+
+    run_after_commit_or_now do
+      Members::UpdateHighestRoleService.new(id).execute
+    end
+  end
+  # rubocop: enable CodeReuse/ServiceClass
 end
 
 User.prepend_if_ee('EE::User')

app/services/members/update_highest_role_service.rb

@@ -4,7 +4,8 @@ module Members
   class UpdateHighestRoleService < ::BaseService
     include ExclusiveLeaseGuard
 
-    LEASE_TIMEOUT = 30.minutes.to_i
+    LEASE_TIMEOUT = 10.minutes.to_i
+    DELAY = 10.minutes
 
     attr_reader :user_id
 
@@ -14,7 +15,7 @@ module Members
 
     def execute
       try_obtain_lease do
-        UpdateHighestRoleWorker.perform_async(user_id)
+        UpdateHighestRoleWorker.perform_in(DELAY, user_id)
       end
     end
 

app/services/users/update_highest_member_role_service.rb

@@ -2,7 +2,7 @@
 
 module Users
   class UpdateHighestMemberRoleService < BaseService
-    attr_reader :user, :identity_params
+    attr_reader :user
 
     def initialize(user)
       @user = user

app/workers/update_highest_role_worker.rb

@@ -11,9 +11,15 @@ class UpdateHighestRoleWorker
 
   # rubocop: disable CodeReuse/ActiveRecord
   def perform(user_id)
-    user = User.active.find_by(id: user_id)
+    user = User.find_by(id: user_id)
 
-    Users::UpdateHighestMemberRoleService.new(user).execute if user.present?
+    return unless user.present?
+
+    if user.active? && user.user_type.nil? && !user.internal?
+      Users::UpdateHighestMemberRoleService.new(user).execute
+    else
+      UserHighestRole.where(user_id: user_id).delete_all
+    end
   end
   # rubocop: enable CodeReuse/ActiveRecord
 end

changelogs/unreleased/update_highest_role_with_user_callback.yml

+---
+title: Update user's highest role to keep the users statistics up to date
+merge_request: 28087
+author:
+type: added

spec/models/user_spec.rb

@@ -4441,4 +4441,52 @@ describe User, :do_not_mock_admin_mode do
       end
     end
   end
+
+  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)
+        end
+
+        create(:user)
+      end
+    end
+
+    context 'when user already exists' do
+      let!(:user) { create(:user) }
+
+      describe 'update user' do
+        using RSpec::Parameterized::TableSyntax
+
+        where(:attributes) do
+          [
+            { state: 'blocked' },
+            { ghost: true },
+            { user_type: :alert_bot }
+          ]
+        end
+
+        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
+
+              user.update(attributes)
+            end
+          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)
+
+            user.update(email: 'newmail@example.com')
+          end
+        end
+      end
+    end
+  end
 end

spec/services/members/update_highest_role_service_spec.rb

@@ -22,7 +22,9 @@ describe Members::UpdateHighestRoleService, :clean_gitlab_redis_shared_state do
         expect(service.exclusive_lease.exists?).to be_truthy
       end
 
-      it 'schedules a job' do
+      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

spec/workers/update_highest_role_worker_spec.rb

@@ -8,56 +8,73 @@ describe UpdateHighestRoleWorker, :clean_gitlab_redis_shared_state do
   let(:worker) { described_class.new }
 
   describe '#perform' do
-    let(:active_scope_attributes) do
-      {
-        state: 'active',
-        ghost: false,
-        user_type: nil
-      }
+    context 'when user is not found' do
+      it 'does not update or deletes any highest role', :aggregate_failures do
+        expect { worker.perform(-1) }.not_to change(UserHighestRole, :count)
+      end
     end
-    let(:user) { create(:user, attributes) }
-
-    subject { worker.perform(user.id) }
 
     context 'when user is found' do
-      let(:attributes) { active_scope_attributes }
+      let(:active_attributes) do
+        {
+          state: 'active',
+          ghost: false,
+          user_type: nil
+        }
+      end
+      let(:user) { create(:user, active_attributes) }
 
-      it 'updates the highest role for the user' do
-        user_highest_role = create(:user_highest_role, user: user)
-        create(:group_member, :developer, user: user)
+      subject { worker.perform(user.id) }
 
-        expect { subject }
-          .to change { user_highest_role.reload.highest_access_level }
-          .from(nil)
-          .to(Gitlab::Access::DEVELOPER)
-      end
-    end
+      context 'when user is active and not internal' do
+        context 'when user highest role exists' do
+          it 'updates the highest role for the user' do
+            user_highest_role = create(:user_highest_role, user: user)
+            create(:group_member, :developer, user: user)
 
-    context 'when user is not found' do
-      shared_examples 'no update' do
-        it 'does not update any highest role' do
-          expect(Users::UpdateHighestMemberRoleService).not_to receive(:new)
+            expect { subject }
+              .to change { user_highest_role.reload.highest_access_level }
+              .from(nil)
+              .to(Gitlab::Access::DEVELOPER)
+          end
+        end
+
+        context 'when user highest role does not exist' do
+          it 'creates the highest role for the user' do
+            create(:group_member, :developer, user: user)
 
-          worker.perform(user.id)
+            expect { subject }.to change { UserHighestRole.count }.by(1)
+          end
         end
       end
 
-      context 'when user is blocked' do
-        let(:attributes) { active_scope_attributes.merge(state: 'blocked') }
+      context 'when user is either inactive or internal' do
+        using RSpec::Parameterized::TableSyntax
 
-        it_behaves_like 'no update'
-      end
+        where(:additional_attributes) do
+          [
+            { state: 'blocked' },
+            { ghost: true },
+            { user_type: :alert_bot }
+          ]
+        end
 
-      context 'when user is a ghost' do
-        let(:attributes) { active_scope_attributes.merge(ghost: true) }
+        with_them do
+          it 'deletes highest role' do
+            user = create(:user, active_attributes.merge(additional_attributes))
+            create(:user_highest_role, user: user)
 
-        it_behaves_like 'no update'
-      end
+            expect { worker.perform(user.id) }.to change { UserHighestRole.count }.from(1).to(0)
+          end
+        end
 
-      context 'when user has a user type' do
-        let(:attributes) { active_scope_attributes.merge(user_type: :alert_bot) }
+        context 'when user highest role does not exist' do
+          it 'does not delete a highest role' do
+            user = create(:user, state: 'blocked')
 
-        it_behaves_like 'no update'
+            expect { worker.perform(user.id) }.not_to change(UserHighestRole, :count)
+          end
+        end
       end
     end
   end