Merge branch 'exclude-project-bot-from-deactivation' into 'master'

Exclude Project Bots from Deactivation in DeactivateDormantUsersWorker

See merge request gitlab-org/gitlab!73028

app/models/concerns/has_user_type.rb

@@ -28,6 +28,7 @@ module HasUserType
     scope :non_internal, -> { humans.or(where(user_type: NON_INTERNAL_USER_TYPES)) }
     scope :without_ghosts, -> { humans.or(where.not(user_type: :ghost)) }
     scope :without_project_bot, -> { humans.or(where.not(user_type: :project_bot)) }
+    scope :human_or_service_user, -> { humans.or(where(user_type: :service_user)) }
 
     enum user_type: USER_TYPES
 

app/models/user.rb

@@ -454,8 +454,8 @@ class User < ApplicationRecord
   scope :order_recent_last_activity, -> { reorder(Gitlab::Database.nulls_last_order('last_activity_on', 'DESC')) }
   scope :order_oldest_last_activity, -> { reorder(Gitlab::Database.nulls_first_order('last_activity_on', 'ASC')) }
   scope :by_id_and_login, ->(id, login) { where(id: id).where('username = LOWER(:login) OR email = LOWER(:login)', login: login) }
-  scope :dormant, -> { active.where('last_activity_on <= ?', MINIMUM_INACTIVE_DAYS.day.ago.to_date) }
-  scope :with_no_activity, -> { active.where(last_activity_on: nil) }
+  scope :dormant, -> { with_state(:active).human_or_service_user.where('last_activity_on <= ?', MINIMUM_INACTIVE_DAYS.day.ago.to_date) }
+  scope :with_no_activity, -> { with_state(:active).human_or_service_user.where(last_activity_on: nil) }
   scope :by_provider_and_extern_uid, ->(provider, extern_uid) { joins(:identities).merge(Identity.with_extern_uid(provider, extern_uid)) }
   scope :get_ids_by_username, -> (username) { where(username: username).pluck(:id) }
 

db/migrate/20211027203950_add_updated_index_for_dormant_users.rb

+# frozen_string_literal: true
+
+class AddUpdatedIndexForDormantUsers < Gitlab::Database::Migration[1.0]
+  INDEX_NAME = 'index_users_on_id_and_last_activity_on_for_active_human_service'
+
+  disable_ddl_transaction!
+
+  def up
+    index_condition = "state = 'active' AND (users.user_type IS NULL OR users.user_type = 4)"
+
+    add_concurrent_index :users, [:id, :last_activity_on], where: index_condition, name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :users, INDEX_NAME
+  end
+end

db/post_migrate/20211027204011_remove_index_for_dormant_users.rb

+# frozen_string_literal: true
+
+class RemoveIndexForDormantUsers < Gitlab::Database::Migration[1.0]
+  INDEX_NAME = 'index_users_on_id_and_last_activity_on_for_non_internal_active'
+
+  disable_ddl_transaction!
+
+  def up
+    remove_concurrent_index_by_name :users, INDEX_NAME
+  end
+
+  def down
+    index_condition = "state = 'active' AND (users.user_type IS NULL OR users.user_type IN (NULL, 6, 4))"
+
+    add_concurrent_index :users, [:id, :last_activity_on], where: index_condition, name: INDEX_NAME
+  end
+end

db/schema_migrations/20211027203950

+38643fbd719e7d65e5e79eeb279a5732cee5c28774a300859a2bace13d882ee2
\ No newline at end of file

db/schema_migrations/20211027204011

+8542de6f3bf260b4e7596ed497ff8ed4204c81519d8f19e64ac86cd5532e7a61
\ No newline at end of file

db/structure.sql

@@ -26938,7 +26938,7 @@ CREATE INDEX index_users_on_feed_token ON users USING btree (feed_token);
 
 CREATE INDEX index_users_on_group_view ON users USING btree (group_view);
 
-CREATE INDEX index_users_on_id_and_last_activity_on_for_non_internal_active ON users USING btree (id, last_activity_on) WHERE (((state)::text = 'active'::text) AND ((user_type IS NULL) OR (user_type = ANY (ARRAY[NULL::integer, 6, 4]))));
+CREATE INDEX index_users_on_id_and_last_activity_on_for_active_human_service ON users USING btree (id, last_activity_on) WHERE (((state)::text = 'active'::text) AND ((user_type IS NULL) OR (user_type = 4)));
 
 CREATE INDEX index_users_on_incoming_email_token ON users USING btree (incoming_email_token);
 

spec/workers/users/deactivate_dormant_users_worker_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Users::DeactivateDormantUsersWorker do
+  using RSpec::Parameterized::TableSyntax
+
   describe '#perform' do
     let_it_be(:dormant) { create(:user, last_activity_on: User::MINIMUM_INACTIVE_DAYS.days.ago.to_date) }
     let_it_be(:inactive) { create(:user, last_activity_on: nil) }
@@ -22,12 +24,12 @@ RSpec.describe Users::DeactivateDormantUsersWorker do
     context 'when automatic deactivation of dormant users is enabled' do
       before do
         stub_application_setting(deactivate_dormant_users: true)
+        stub_const("#{described_class.name}::PAUSE_SECONDS", 0)
       end
 
       it 'deactivates dormant users' do
         freeze_time do
           stub_const("#{described_class.name}::BATCH_SIZE", 1)
-          stub_const("#{described_class.name}::PAUSE_SECONDS", 0)
 
           expect(worker).to receive(:sleep).twice
 
@@ -37,6 +39,38 @@ RSpec.describe Users::DeactivateDormantUsersWorker do
           expect(User.with_no_activity.count).to eq(0)
         end
       end
+
+      where(:user_type, :expected_state) do
+        :human             | 'deactivated'
+        :support_bot       | 'active'
+        :alert_bot         | 'active'
+        :visual_review_bot | 'active'
+        :service_user      | 'deactivated'
+        :ghost             | 'active'
+        :project_bot       | 'active'
+        :migration_bot     | 'active'
+        :security_bot      | 'active'
+        :automation_bot    | 'active'
+      end
+      with_them do
+        it 'deactivates certain user types' do
+          user = create(:user, user_type: user_type, state: :active, last_activity_on: User::MINIMUM_INACTIVE_DAYS.days.ago.to_date)
+
+          worker.perform
+
+          expect(user.reload.state).to eq(expected_state)
+        end
+      end
+
+      it 'does not deactivate non-active users' do
+        human_user = create(:user, user_type: :human, state: :blocked, last_activity_on: User::MINIMUM_INACTIVE_DAYS.days.ago.to_date)
+        service_user = create(:user, user_type: :service_user, state: :blocked, last_activity_on: User::MINIMUM_INACTIVE_DAYS.days.ago.to_date)
+
+        worker.perform
+
+        expect(human_user.reload.state).to eq('blocked')
+        expect(service_user.reload.state).to eq('blocked')
+      end
     end
 
     context 'when automatic deactivation of dormant users is disabled' do