Merge branch 'pl-user-bot_type' into 'master'

Refactor Service Desk's bot user

See merge request gitlab-org/gitlab-ee!10353

app/models/user.rb

@@ -537,20 +537,16 @@ class User < ApplicationRecord
     username
   end
 
-  def self.internal_attributes
-    [:ghost]
-  end
-
   def internal?
-    self.class.internal_attributes.any? { |a| self[a] }
+    ghost?
   end
 
   def self.internal
-    where(Hash[internal_attributes.zip([true] * internal_attributes.size)])
+    where(ghost: true)
   end
 
   def self.non_internal
-    where(internal_attributes.map { |attr| "#{attr} IS NOT TRUE" }.join(" AND "))
+    where('ghost IS NOT TRUE')
   end
 
   #

db/schema.rb

@@ -3243,8 +3243,10 @@ ActiveRecord::Schema.define(version: 20190404231137) do
     t.string "commit_email"
     t.integer "group_view"
     t.integer "managing_group_id"
+    t.integer "bot_type", limit: 2
     t.index ["accepted_term_id"], name: "index_users_on_accepted_term_id", using: :btree
     t.index ["admin"], name: "index_users_on_admin", using: :btree
+    t.index ["bot_type"], name: "index_users_on_bot_type", using: :btree
     t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true, using: :btree
     t.index ["created_at"], name: "index_users_on_created_at", using: :btree
     t.index ["email"], name: "index_users_on_email", unique: true, using: :btree
@@ -3258,6 +3260,7 @@ ActiveRecord::Schema.define(version: 20190404231137) do
     t.index ["name"], name: "index_users_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"}
     t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true, using: :btree
     t.index ["state"], name: "index_users_on_state", using: :btree
+    t.index ["state"], name: "index_users_on_state_and_internal", where: "((ghost <> true) AND (bot_type IS NULL))", using: :btree
     t.index ["state"], name: "index_users_on_state_and_internal_attrs", where: "((ghost <> true) AND (support_bot <> true))", using: :btree
     t.index ["support_bot"], name: "index_users_on_support_bot", using: :btree
     t.index ["username"], name: "index_users_on_username", using: :btree

ee/app/models/ee/issue.rb

@@ -24,7 +24,7 @@ module EE
 
     # override
     def check_for_spam?
-      author.support_bot? || super
+      author.bot? || super
     end
 
     # override
@@ -48,7 +48,7 @@ module EE
       # Making the support bot subscribed to every issue is not as bad as it
       # seems, though, since it isn't permitted to :receive_notifications,
       # and doesn't actually show up in the participants list.
-      user.support_bot? || super
+      user.bot? || super
     end
 
     # override

ee/app/models/ee/user.rb

@@ -17,6 +17,8 @@ module EE
     prepended do
       EMAIL_OPT_IN_SOURCE_ID_GITLAB_COM = 1
 
+      ignore_column :support_bot
+
       # We aren't using the `auditor?` method for the `if` condition here
       # because `auditor?` returns `false` when the `auditor` column is `true`
       # and the auditor add-on absent. We want to run this validation
@@ -70,21 +72,32 @@ module EE
       # Note: When adding an option, it's value MUST equal to the last value + 1.
       enum group_view: { details: 1, security_dashboard: 2 }, _prefix: true
       scope :group_view_details, -> { where('group_view = ? OR group_view IS NULL', group_view[:details]) }
+
+      enum bot_type: {
+        support_bot: 1
+      }
     end
 
     class_methods do
+      extend ::Gitlab::Utils::Override
+
       def support_bot
         email_pattern = "support%s@#{Settings.gitlab.host}"
 
-        unique_internal(where(support_bot: true), 'support-bot', email_pattern) do |u|
+        unique_internal(where(bot_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
       end
 
-      # override
-      def internal_attributes
-        super + [:support_bot]
+      override :internal
+      def internal
+        super.or(where.not(bot_type: nil))
+      end
+
+      override :non_internal
+      def non_internal
+        super.where(bot_type: nil)
       end
 
       def non_ldap
@@ -224,6 +237,14 @@ module EE
       super
     end
 
+    def internal?
+      super || bot?
+    end
+
+    def bot?
+      has_bot_type? ? bot_type.present? : old_support_bot
+    end
+
     protected
 
     override :password_required?
@@ -235,6 +256,14 @@ module EE
 
     private
 
+    def has_bot_type?
+      has_attribute?(:bot_type)
+    end
+
+    def old_support_bot
+      read_attribute(:support_bot)
+    end
+
     def namespace_union(select = :id)
       ::Gitlab::SQL::Union.new([
         ::Namespace.select(select).where(type: nil, owner: self),

ee/db/migrate/20190401150745_add_users_bot_type.rb

+# frozen_string_literal: true
+
+class AddUsersBotType < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    change_table :users do |t|
+      t.integer :bot_type, limit: 2
+    end
+  end
+end

ee/db/migrate/20190401150746_index_users_bot_type.rb

+# frozen_string_literal: true
+
+class IndexUsersBotType < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class User < ActiveRecord::Base
+    enum bot_type: { support_bot: 1 }
+  end
+
+  def up
+    remove_concurrent_index :users, :bot_type
+    add_concurrent_index :users, :bot_type
+
+    remove_concurrent_index :users, :state, name: internal_index
+    add_concurrent_index :users, :state,
+      name: internal_index,
+      where: 'ghost <> true AND bot_type IS NULL'
+
+    User
+      .where(support_bot: true)
+      .update_all(bot_type: User.bot_types[:support_bot])
+  end
+
+  def down
+    User
+      .where(bot_type: User.bot_types[:support_bot])
+      .update_all(support_bot: true)
+
+    remove_concurrent_index :users, :state, name: internal_index
+    remove_concurrent_index :users, :bot_type
+  end
+
+  private
+
+  def internal_index
+    'index_users_on_state_and_internal'
+  end
+end

ee/lib/banzai/reference_parser/ee/base_parser.rb

@@ -9,7 +9,7 @@ module Banzai
         # the policies framework, but CE currently
         # manually checks for team membership and the like.
         def nodes_user_can_reference(user, nodes)
-          return [] if user.support_bot?
+          return [] if user.bot?
 
           super
         end

ee/spec/migrations/index_users_bot_type_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('ee', 'db', 'migrate', '20190401150746_index_users_bot_type.rb')
+
+describe IndexUsersBotType, :migration do
+  BOT_TYPE = {
+    support_bot: 1
+  }.freeze
+
+  let(:migration) { described_class.new }
+  let(:users) { table(:users) }
+
+  let!(:user) { create_user(username: 'test') }
+
+  describe '#up' do
+    let!(:support_bot) do
+      create_user(username: 'support_bot', support_bot: true)
+    end
+
+    it 'converts support_bot column to enum and adds indexes' do
+      migration.up
+
+      expect(user.reload.bot_type).to be_nil
+      expect(support_bot.reload.support_bot).to eq(true)
+      expect(support_bot.reload.bot_type).to eq(BOT_TYPE[:support_bot])
+
+      expect(index_exists?(:bot_type)).to eq(true)
+      expect(index_exists?(:state, name: internal_index)).to eq(true)
+    end
+  end
+
+  describe '#down' do
+    let!(:support_bot) do
+      create_user(username: 'support_bot', bot_type: BOT_TYPE[:support_bot])
+    end
+
+    it 'converts support_bot column to enum and removes indexes' do
+      migration.down
+
+      expect(user.reload.support_bot).to be_nil
+      expect(support_bot.reload.support_bot).to eq(true)
+
+      expect(index_exists?(:bot_type)).to eq(false)
+      expect(index_exists?(:state, name: internal_index)).to eq(false)
+    end
+  end
+
+  private
+
+  def create_user(username:, **params)
+    users.create!(
+      email: "#{username}@example.com",
+      projects_limit: 0,
+      username: username,
+      **params
+    )
+  end
+
+  def index_exists?(column, options = {})
+    migration.index_exists?(:users, column, options)
+  end
+
+  def internal_index
+    'index_users_on_state_and_internal'
+  end
+end

ee/spec/models/user_spec.rb

@@ -519,4 +519,31 @@ describe User do
       end
     end
   end
+
+  describe 'internal methods' do
+    let!(:user) { create(:user) }
+    let!(:ghost) { described_class.ghost }
+    let!(:support_bot) { described_class.support_bot }
+    let!(:non_internal) { [user] }
+    let!(:internal) { [ghost, support_bot] }
+
+    it 'returns non internal users' do
+      expect(described_class.internal).to eq(internal)
+      expect(internal.all?(&:internal?)).to eq(true)
+    end
+
+    it 'returns internal users' do
+      expect(described_class.non_internal).to eq(non_internal)
+      expect(non_internal.all?(&:internal?)).to eq(false)
+    end
+
+    describe '#bot?' do
+      it 'marks bot users' do
+        expect(user.bot?).to eq(false)
+        expect(ghost.bot?).to eq(false)
+
+        expect(support_bot.bot?).to eq(true)
+      end
+    end
+  end
 end