Merge branch '208897-migrate-bot-type-to-user-type' into 'master'

Migrate bot_type data to user_type column

Closes #208897

See merge request gitlab-org/gitlab!26981

app/models/user.rb

@@ -21,6 +21,7 @@ class User < ApplicationRecord
   include OptionallySearch
   include FromUnion
   include BatchDestroyDependentAssociations
+  include IgnorableColumns
 
   DEFAULT_NOTIFICATION_LEVEL = :participating
 
@@ -59,9 +60,10 @@ class User < ApplicationRecord
 
   MINIMUM_INACTIVE_DAYS = 180
 
-  enum bot_type: ::UserBotTypeEnums.bots
   enum user_type: ::UserTypeEnums.types
 
+  ignore_column :bot_type, remove_with: '12.11', remove_after: '2020-04-22'
+
   # Override Devise::Models::Trackable#update_tracked_fields!
   # to limit database writes to at most once every hour
   # rubocop: disable CodeReuse/ServiceClass
@@ -337,8 +339,9 @@ class User < ApplicationRecord
   scope :with_emails, -> { preload(:emails) }
   scope :with_dashboard, -> (dashboard) { where(dashboard: dashboard) }
   scope :with_public_profile, -> { where(private_profile: false) }
-  scope :bots, -> { where.not(bot_type: nil) }
-  scope :humans, -> { where(user_type: nil, bot_type: nil) }
+  scope :bots, -> { where(user_type: UserTypeEnums.bots.values) }
+  scope :not_bots, -> { humans.or(where.not(user_type: UserTypeEnums.bots.values)) }
+  scope :humans, -> { where(user_type: nil) }
 
   scope :with_expiring_and_not_notified_personal_access_tokens, ->(at) do
     where('EXISTS (?)',
@@ -618,7 +621,7 @@ class User < ApplicationRecord
     def alert_bot
       email_pattern = "alert%s@#{Settings.gitlab.host}"
 
-      unique_internal(where(bot_type: :alert_bot), 'alert-bot', email_pattern) do |u|
+      unique_internal(where(user_type: :alert_bot), 'alert-bot', email_pattern) do |u|
         u.bio = 'The GitLab alert bot'
         u.name = 'GitLab Alert Bot'
       end
@@ -640,7 +643,7 @@ class User < ApplicationRecord
   end
 
   def bot?
-    bot_type.present?
+    UserTypeEnums.bots.has_key?(user_type)
   end
 
   def internal?
@@ -652,7 +655,7 @@ class User < ApplicationRecord
   end
 
   def self.non_internal
-    without_ghosts.humans
+    without_ghosts.not_bots
   end
 
   #

app/models/user_bot_type_enums.rb

-# frozen_string_literal: true
-
-module UserBotTypeEnums
-  def self.bots
-    {
-      alert_bot: 2
-    }
-  end
-end
-
-UserBotTypeEnums.prepend_if_ee('EE::UserBotTypeEnums')

app/models/user_type_enums.rb

@@ -2,13 +2,13 @@
 
 module UserTypeEnums
   def self.types
-    bots
+    bots.merge(human: nil)
   end
 
   def self.bots
     {
-      AlertBot: 2
-    }
+      alert_bot: 2
+    }.with_indifferent_access
   end
 end
 

changelogs/unreleased/208897-migrate-bot-type-to-user-type.yml

+---
+title: Move bots functionality to user_type column
+merge_request: 26981
+author:
+type: performance

config/pseudonymizer.yml

@@ -469,6 +469,7 @@ tables:
     - ghost
     - last_activity_on
     - notified_of_own_activity
+    - user_type
     - bot_type
     - preferred_language
     - theme_id

db/migrate/20200311074438_migrate_bot_type_to_user_type.rb

+# frozen_string_literal: true
+
+class MigrateBotTypeToUserType < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    execute('UPDATE users SET user_type = bot_type WHERE bot_type IS NOT NULL AND user_type IS NULL')
+  end
+
+  def down
+    execute('UPDATE users SET user_type = NULL WHERE bot_type IS NOT NULL AND bot_type = user_type')
+  end
+end

db/migrate/20200311082301_add_user_state_index.rb

+# frozen_string_literal: true
+
+class AddUserStateIndex < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:users, [:state, :user_type], where: 'ghost IS NOT TRUE', name: 'index_users_on_state_and_user_type_internal')
+    remove_concurrent_index_by_name(:users, 'index_users_on_state_and_internal_ee')
+    remove_concurrent_index_by_name(:users, 'index_users_on_state_and_internal')
+  end
+
+  def down
+    add_concurrent_index(:users, :state, where: 'ghost IS NOT TRUE AND bot_type IS NULL', name: 'index_users_on_state_and_internal_ee')
+    add_concurrent_index(:users, :state, where: 'ghost IS NOT TRUE', name: 'index_users_on_state_and_internal')
+    remove_concurrent_index_by_name(:users, 'index_users_on_state_and_internal_ee')
+  end
+end

db/schema.rb

@@ -4431,9 +4431,8 @@ ActiveRecord::Schema.define(version: 2020_03_13_123934) do
     t.index ["name"], name: "index_users_on_name_trigram", opclass: :gin_trgm_ops, using: :gin
     t.index ["public_email"], name: "index_users_on_public_email", where: "((public_email)::text <> ''::text)"
     t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true
+    t.index ["state", "user_type"], name: "index_users_on_state_and_user_type_internal", where: "(ghost IS NOT TRUE)"
     t.index ["state"], name: "index_users_on_state"
-    t.index ["state"], name: "index_users_on_state_and_internal", where: "(ghost IS NOT TRUE)"
-    t.index ["state"], name: "index_users_on_state_and_internal_ee", where: "((ghost IS NOT TRUE) AND (bot_type IS NULL))"
     t.index ["static_object_token"], name: "index_users_on_static_object_token", unique: true
     t.index ["unconfirmed_email"], name: "index_users_on_unconfirmed_email", where: "(unconfirmed_email IS NOT NULL)"
     t.index ["user_type"], name: "index_users_on_user_type"

ee/app/models/ee/user.rb

@@ -92,7 +92,7 @@ module EE
       def support_bot
         email_pattern = "support%s@#{Settings.gitlab.host}"
 
-        unique_internal(where(bot_type: :support_bot), 'support-bot', email_pattern) do |u|
+        unique_internal(where(user_type: :support_bot), 'support-bot', email_pattern) do |u|
           u.bio = 'The GitLab support bot used for Service Desk'
           u.name = 'GitLab Support Bot'
         end
@@ -101,7 +101,7 @@ module EE
       def visual_review_bot
         email_pattern = "visual_review%s@#{Settings.gitlab.host}"
 
-        unique_internal(where(bot_type: :visual_review_bot), 'visual-review-bot', email_pattern) do |u|
+        unique_internal(where(user_type: :visual_review_bot), 'visual-review-bot', email_pattern) do |u|
           u.bio = 'The Gitlab Visual Review feedback bot'
           u.name = 'Gitlab Visual Review Bot'
         end

ee/app/models/ee/user_bot_type_enums.rb

-# frozen_string_literal: true
-
-module EE
-  module UserBotTypeEnums
-    extend ActiveSupport::Concern
-
-    class_methods do
-      extend ::Gitlab::Utils::Override
-
-      override :bots
-      def bots
-        super.merge(
-          support_bot: 1,
-          visual_review_bot: 3
-        )
-      end
-    end
-  end
-end

ee/app/models/ee/user_type_enums.rb

@@ -9,12 +9,12 @@ module EE
 
       override :types
       def types
-        super.merge(ServiceUser: 4)
+        super.merge(service_user: 4)
       end
 
       override :bots
       def bots
-        super.merge(SupportBot: 1, VisualReviewBot: 3)
+        super.merge(support_bot: 1, visual_review_bot: 3)
       end
     end
   end

ee/spec/factories/users.rb

@@ -18,7 +18,7 @@ FactoryBot.modify do
     end
 
     trait :service_user do
-      user_type { :ServiceUser }
+      user_type { :service_user }
     end
   end
 

ee/spec/features/admin/admin_dashboard_spec.rb

@@ -17,8 +17,8 @@ describe 'Admin Dashboard' do
       project1.add_reporter(user)
       project2.add_developer(user)
 
-      create(:user, bot_type: :support_bot)
-      create(:user, bot_type: :alert_bot)
+      create(:user, user_type: :support_bot)
+      create(:user, user_type: :alert_bot)
 
       sign_in(create(:admin))
     end

ee/spec/models/ee/user_bot_type_enums_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe UserBotTypeEnums do
-  it 'has no type conflicts between CE and EE', :aggregate_failures do
-    described_class.public_methods(false).each do |method_name|
-      method = described_class.method(method_name)
-
-      ee_result = method.call
-      ce_result = method.super_method.call
-
-      failure_message = "expected #{method} to have no value conflicts, but it has.\n
-      Please make sure you are not overriding values.\n
-      Actual values: EE #{ee_result}, CE #{ce_result}"
-
-      expect(ee_result).to include(ce_result), failure_message
-      expect(ee_result.values).to match_array(ee_result.values.uniq)
-    end
-  end
-end

ee/spec/models/user_spec.rb

@@ -91,13 +91,26 @@ describe User do
       let_it_be(:user1) { create(:user, :external) }
       let_it_be(:user2) { create(:user, state: 'blocked') }
       let_it_be(:user3) { create(:user, ghost: true) }
-      let_it_be(:user4) { create(:user, bot_type: 'support_bot') }
-      let_it_be(:user5) { create(:user, state: 'blocked', bot_type: 'support_bot') }
+      let_it_be(:user4) { create(:user, user_type: :support_bot) }
+      let_it_be(:user5) { create(:user, state: 'blocked', user_type: :support_bot) }
 
       it 'returns all active users including active bots but ghost users' do
         expect(described_class.active_without_ghosts).to match_array([user1, user4])
       end
     end
+
+    describe '.non_internal' do
+      let!(:user) { create(:user) }
+      let!(:service_user) { create(:user, user_type: :service_user) }
+      let!(:ghost) { described_class.ghost }
+      let!(:alert_bot) { described_class.alert_bot }
+      let!(:non_internal) { [user, service_user] }
+
+      it 'returns users without ghosts and bots' do
+        expect(described_class.non_internal).to match_array(non_internal)
+        expect(non_internal.all?(&:internal?)).to eq(false)
+      end
+    end
   end
 
   describe '.find_by_smartcard_identity' do
@@ -600,13 +613,13 @@ describe User do
       context 'when user is internal' do
         using RSpec::Parameterized::TableSyntax
 
-        where(:bot_type) do
-          User.bot_types.keys
+        where(:bot_user_type) do
+          UserTypeEnums.bots.keys
         end
 
         with_them do
           context 'when user is a bot' do
-            let(:user) { create(:user, bot_type: bot_type) }
+            let(:user) { create(:user, user_type: bot_user_type) }
 
             it 'returns false' do
               expect(user.using_license_seat?).to eq false

spec/factories/users.rb

@@ -24,7 +24,7 @@ FactoryBot.define do
     end
 
     trait :bot do
-      bot_type { User.bot_types[:alert_bot] }
+      user_type { :alert_bot }
     end
 
     trait :external do

spec/migrations/migrate_bot_type_to_user_type_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require Rails.root.join('db', 'migrate', '20200311074438_migrate_bot_type_to_user_type.rb')
+
+describe MigrateBotTypeToUserType, :migration do
+  let(:users) { table(:users) }
+
+  it 'updates bots & ignores humans' do
+    users.create!(email: 'human', bot_type: nil, projects_limit: 0)
+    users.create!(email: 'support_bot', bot_type: 1, projects_limit: 0)
+    users.create!(email: 'alert_bot', bot_type: 2, projects_limit: 0)
+    users.create!(email: 'visual_review_bot', bot_type: 3, projects_limit: 0)
+
+    migrate!
+
+    expect(users.where('user_type IS NOT NULL').map(&:user_type)).to match_array([1, 2, 3])
+  end
+end

spec/models/user_spec.rb

@@ -4244,12 +4244,12 @@ describe User, :do_not_mock_admin_mode do
     let!(:non_internal) { [user] }
     let!(:internal) { [ghost, alert_bot] }
 
-    it 'returns non internal users' do
+    it 'returns internal users' do
       expect(described_class.internal).to eq(internal)
       expect(internal.all?(&:internal?)).to eq(true)
     end
 
-    it 'returns internal users' do
+    it 'returns non internal users' do
       expect(described_class.non_internal).to eq(non_internal)
       expect(non_internal.all?(&:internal?)).to eq(false)
     end