Commit f7073755 authored by Pavel Shutsin's avatar Pavel Shutsin

Migrate bot_type data to user_type column

With new generalized `user_type` column
usage of separate bot_type is not justified.
So we make bots functionality dependent on
user_type column
parent dfeb0b6f
......@@ -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
#
......
# frozen_string_literal: true
module UserBotTypeEnums
def self.bots
{
alert_bot: 2
}
end
end
UserBotTypeEnums.prepend_if_ee('EE::UserBotTypeEnums')
......@@ -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
......
---
title: Move bots functionality to user_type column
merge_request: 26981
author:
type: performance
......@@ -469,6 +469,7 @@ tables:
- ghost
- last_activity_on
- notified_of_own_activity
- user_type
- bot_type
- preferred_language
- theme_id
......
# 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
# 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
......@@ -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"
......
......@@ -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
......
# 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
......@@ -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
......
......@@ -18,7 +18,7 @@ FactoryBot.modify do
end
trait :service_user do
user_type { :ServiceUser }
user_type { :service_user }
end
end
......
......@@ -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
......
# 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
......@@ -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
......
......@@ -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
......
# 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
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment