Merge branch 'update_highest_role_with_user_callback' into 'master'

Add User callback to update highest role

See merge request gitlab-org/gitlab!28087

app/models/member.rb

@@ -470,7 +470,6 @@ class Member < ApplicationRecord
   # for a Member to be commited before attempting to update the highest role.
   # rubocop: disable CodeReuse/ServiceClass
   def update_highest_role
-    return unless Feature.enabled?(:highest_role_callback)
     return unless user_id.present?
     return unless previous_changes[:access_level].present?
 

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

ee/spec/services/geo/design_repository_sync_service_spec.rb

@@ -28,7 +28,8 @@ describe Geo::DesignRepositorySyncService do
     let(:url_to_repo) { "#{primary.url}#{project.full_path}.design.git" }
 
     before do
-      allow_any_instance_of(Member).to receive(:update_highest_role) # avoid stubbing exclusive lease for this method
+      # update_highest_role uses exclusive key too:
+      allow(Gitlab::ExclusiveLease).to receive(:new).and_call_original
 
       stub_exclusive_lease(lease_key, lease_uuid)
       stub_exclusive_lease("geo_project_housekeeping:#{project.id}")

spec/lib/gitlab/application_context_spec.rb

@@ -9,14 +9,16 @@ describe Gitlab::ApplicationContext do
     end
 
     it 'passes the expected context on to labkit' do
+      user = build(:user)
+      project = build(:project)
       fake_proc = duck_type(:call)
       expected_context = hash_including(user: fake_proc, project: fake_proc, root_namespace: fake_proc)
 
       expect(Labkit::Context).to receive(:with_context).with(expected_context)
 
       described_class.with_context(
-        user: build(:user),
-        project: build(:project),
+        user: user,
+        project: project,
         namespace: build(:namespace)) {}
     end
 

spec/models/active_session_spec.rb

@@ -165,10 +165,10 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
       ActiveSession.set(user, request)
 
       Gitlab::Redis::SharedState.with do |redis|
-        expect(redis.scan_each.to_a).to match_array [
+        expect(redis.scan_each.to_a).to include(
           "session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d",
           "session:lookup:user:gitlab:#{user.id}"
-        ]
+        )
       end
     end
 

spec/models/member_spec.rb

@@ -597,7 +597,6 @@ describe Member do
   end
 
   context 'when after_commit :update_highest_role' do
-    context 'with feature flag enabled' do
     where(:member_type, :source_type) do
       :project_member | :project
       :group_member   | :group
@@ -646,51 +645,4 @@ describe Member do
       end
     end
   end
-
-    context 'with feature flag disabled' do
-      before do
-        stub_feature_flags(highest_role_callback: false)
-      end
-
-      where(:member_type, :source_type) do
-        :project_member | :project
-        :group_member   | :group
-      end
-
-      with_them do
-        describe 'create member' do
-          it 'does not initialize a new Members::UpdateHighestRoleService object' do
-            source = create(source_type)
-            user = create(:user)
-
-            expect(Members::UpdateHighestRoleService).not_to receive(:new).with(user.id)
-
-            create(member_type, :guest, user: user, source_type => source)
-          end
-        end
-
-        context 'when member exists' do
-          let!(:member) { create(member_type) }
-
-          describe 'update member' do
-            context 'when access level was changed' do
-              it 'does not initialize a new Members::UpdateHighestRoleService object' do
-                expect(Members::UpdateHighestRoleService).not_to receive(:new).with(member.user_id)
-
-                member.update(access_level: Gitlab::Access::GUEST)
-              end
-            end
-          end
-
-          describe 'destroy member' do
-            it 'does not initialize a new Members::UpdateHighestRoleService object' do
-              expect(Members::UpdateHighestRoleService).not_to receive(:new).with(member.user_id)
-
-              member.destroy
-            end
-          end
-        end
-      end
-    end
-  end
 end

spec/models/user_spec.rb

@@ -1221,6 +1221,10 @@ describe User, :do_not_mock_admin_mode do
     end
 
     it 'uses SecureRandom to generate the incoming email token' do
+      allow_next_instance_of(User) do |user|
+        allow(user).to receive(:update_highest_role)
+      end
+
       expect(SecureRandom).to receive(:hex).and_return('3b8ca303')
 
       user = create(:user)
@@ -4441,4 +4445,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/services/users/activity_service_spec.rb

@@ -30,7 +30,7 @@ describe Users::ActivityService do
       end
 
       it 'tries to obtain ExclusiveLease' do
-        expect(Gitlab::ExclusiveLease).to receive(:new).and_call_original
+        expect(Gitlab::ExclusiveLease).to receive(:new).with("activity_service:#{user.id}", anything).and_call_original
 
         subject.execute
       end
@@ -56,7 +56,7 @@ describe Users::ActivityService do
       end
 
       it 'does not try to obtain ExclusiveLease' do
-        expect(Gitlab::ExclusiveLease).not_to receive(:new)
+        expect(Gitlab::ExclusiveLease).not_to receive(:new).with("activity_service:#{user.id}", anything)
 
         subject.execute
       end

spec/workers/cluster_update_app_worker_spec.rb

@@ -52,6 +52,8 @@ describe ClusterUpdateAppWorker do
       let(:lease_key) { "#{described_class.name.underscore}-#{application.id}" }
 
       before do
+        # update_highest_role uses exclusive key too:
+        allow(Gitlab::ExclusiveLease).to receive(:new).and_call_original
         stub_exclusive_lease_taken(lease_key)
       end
 
@@ -62,8 +64,6 @@ describe ClusterUpdateAppWorker do
       end
 
       it 'does not allow same app to be updated concurrently by different project', :aggregate_failures do
-        stub_exclusive_lease("refresh_authorized_projects:#{user.id}")
-        stub_exclusive_lease("update_highest_role:#{user.id}")
         project1 = create(:project, namespace: create(:namespace, owner: user))
 
         expect(Clusters::Applications::PrometheusUpdateService).not_to receive(:new)
@@ -87,8 +87,6 @@ describe ClusterUpdateAppWorker do
         application2 = create(:clusters_applications_prometheus, :installed)
         lease_key2 = "#{described_class.name.underscore}-#{application2.id}"
 
-        stub_exclusive_lease("refresh_authorized_projects:#{user.id}")
-        stub_exclusive_lease("update_highest_role:#{user.id}")
         project2 = create(:project, namespace: create(:namespace, owner: user))
 
         stub_exclusive_lease(lease_key2)

spec/workers/new_issue_worker_spec.rb

@@ -30,9 +30,11 @@ describe NewIssueWorker do
       end
 
       it 'logs an error' do
+        issue = create(:issue)
+
         expect(Rails.logger).to receive(:error).with('NewIssueWorker: couldn\'t find User with ID=99, skipping job')
 
-        worker.perform(create(:issue).id, 99)
+        worker.perform(issue.id, 99)
       end
     end
 

spec/workers/new_merge_request_worker_spec.rb

@@ -15,9 +15,11 @@ describe NewMergeRequestWorker do
       end
 
       it 'logs an error' do
+        user = create(:user)
+
         expect(Rails.logger).to receive(:error).with('NewMergeRequestWorker: couldn\'t find MergeRequest with ID=99, skipping job')
 
-        worker.perform(99, create(:user).id)
+        worker.perform(99, user.id)
       end
     end
 
@@ -30,9 +32,11 @@ describe NewMergeRequestWorker do
       end
 
       it 'logs an error' do
+        merge_request = create(:merge_request)
+
         expect(Rails.logger).to receive(:error).with('NewMergeRequestWorker: couldn\'t find User with ID=99, skipping job')
 
-        worker.perform(create(:merge_request).id, 99)
+        worker.perform(merge_request.id, 99)
       end
     end
 

spec/workers/update_highest_role_worker_spec.rb

@@ -8,20 +8,26 @@ describe UpdateHighestRoleWorker, :clean_gitlab_redis_shared_state do
   let(:worker) { described_class.new }
 
   describe '#perform' do
-    let(:active_scope_attributes) do
+    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
+
+    context 'when user is found' do
+      let(:active_attributes) do
         {
           state: 'active',
           ghost: false,
           user_type: nil
         }
       end
-    let(:user) { create(:user, attributes) }
+      let(:user) { create(:user, active_attributes) }
 
       subject { worker.perform(user.id) }
 
-    context 'when user is found' do
-      let(:attributes) { active_scope_attributes }
-
+      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)
@@ -33,31 +39,42 @@ describe UpdateHighestRoleWorker, :clean_gitlab_redis_shared_state do
           end
         end
 
-    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)
+        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'
+        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'
+            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