Merge branch '210025-migrate-ghost-user-to-user-type-part-2' into 'master'

Migrate ghost user logic to user_type column

See merge request gitlab-org/gitlab!29012

app/models/concerns/has_user_type.rb

+# frozen_string_literal: true
+
+module HasUserType
+  extend ActiveSupport::Concern
+
+  USER_TYPES = {
+    human: nil,
+    support_bot: 1,
+    alert_bot: 2,
+    visual_review_bot: 3,
+    service_user: 4,
+    ghost: 5,
+    project_bot: 6
+  }.with_indifferent_access.freeze
+
+  BOT_USER_TYPES = %w[alert_bot project_bot support_bot visual_review_bot].freeze
+  NON_INTERNAL_USER_TYPES = %w[human project_bot service_user].freeze
+  INTERNAL_USER_TYPES = (USER_TYPES.keys - NON_INTERNAL_USER_TYPES).freeze
+
+  included do
+    scope :humans, -> { where(user_type: :human) }
+    scope :bots, -> { where(user_type: BOT_USER_TYPES) }
+    scope :bots_without_project_bot, -> { where(user_type: BOT_USER_TYPES - ['project_bot']) }
+    scope :non_internal, -> { humans.or(where(user_type: NON_INTERNAL_USER_TYPES)) }
+    scope :without_ghosts, -> { humans.or(where.not(user_type: :ghost)) }
+
+    enum user_type: USER_TYPES
+
+    def human?
+      super || user_type.nil?
+    end
+  end
+
+  def bot?
+    BOT_USER_TYPES.include?(user_type)
+  end
+
+  # The explicit check for project_bot will be removed with Bot Categorization
+  # Ref: https://gitlab.com/gitlab-org/gitlab/-/issues/213945
+  def internal?
+    ghost? || (bot? && !project_bot?)
+  end
+end

app/models/user.rb

@@ -24,6 +24,7 @@ class User < ApplicationRecord
   include HasUniqueInternalUsers
   include IgnorableColumns
   include UpdateHighestRole
+  include HasUserType
 
   DEFAULT_NOTIFICATION_LEVEL = :participating
 
@@ -64,10 +65,10 @@ class User < ApplicationRecord
 
   MINIMUM_INACTIVE_DAYS = 180
 
-  enum user_type: ::UserTypeEnums.types
-
   ignore_column :bot_type, remove_with: '12.11', remove_after: '2020-04-22'
 
+  ignore_column :ghost, remove_with: '13.2', remove_after: '2020-06-22'
+
   # Override Devise::Models::Trackable#update_tracked_fields!
   # to limit database writes to at most once every hour
   # rubocop: disable CodeReuse/ServiceClass
@@ -321,32 +322,26 @@ class User < ApplicationRecord
   scope :admins, -> { where(admin: true) }
   scope :blocked, -> { with_states(:blocked, :ldap_blocked) }
   scope :external, -> { where(external: true) }
+  scope :confirmed, -> { where.not(confirmed_at: nil) }
   scope :active, -> { with_state(:active).non_internal }
   scope :active_without_ghosts, -> { with_state(:active).without_ghosts }
-  scope :without_ghosts, -> { where('ghost IS NOT TRUE') }
   scope :deactivated, -> { with_state(:deactivated).non_internal }
   scope :without_projects, -> { joins('LEFT JOIN project_authorizations ON users.id = project_authorizations.user_id').where(project_authorizations: { user_id: nil }) }
-  scope :order_recent_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'DESC')) }
-  scope :order_oldest_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'ASC')) }
-  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 :confirmed, -> { where.not(confirmed_at: nil) }
   scope :by_username, -> (usernames) { iwhere(username: Array(usernames).map(&:to_s)) }
   scope :for_todos, -> (todos) { where(id: todos.select(:user_id)) }
   scope :with_emails, -> { preload(:emails) }
   scope :with_dashboard, -> (dashboard) { where(dashboard: dashboard) }
   scope :with_public_profile, -> { where(private_profile: false) }
-  scope :bots, -> { where(user_type: UserTypeEnums.bots.values) }
-  scope :bots_without_project_bot, -> { bots.where.not(user_type: UserTypeEnums.bots[:project_bot]) }
-  scope :with_project_bots, -> { humans.or(where.not(user_type: UserTypeEnums.bots.except(:project_bot).values)) }
-  scope :humans, -> { where(user_type: nil) }
-
   scope :with_expiring_and_not_notified_personal_access_tokens, ->(at) do
     where('EXISTS (?)',
           ::PersonalAccessToken
             .where('personal_access_tokens.user_id = users.id')
             .expiring_and_not_notified(at).select(1))
   end
+  scope :order_recent_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'DESC')) }
+  scope :order_oldest_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'ASC')) }
+  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')) }
 
   def active_for_authentication?
     super && can?(:log_in)
@@ -624,7 +619,7 @@ class User < ApplicationRecord
     # owns records previously belonging to deleted users.
     def ghost
       email = 'ghost%s@example.com'
-      unique_internal(where(ghost: true, user_type: :ghost), 'ghost', email) do |u|
+      unique_internal(where(user_type: :ghost), 'ghost', email) do |u|
         u.bio = _('This is a "Ghost User", created to hold all issues authored by users that have since been deleted. This user cannot be removed.')
         u.name = 'Ghost User'
       end
@@ -650,47 +645,14 @@ class User < ApplicationRecord
     end
   end
 
-  def full_path
-    username
-  end
-
-  def bot?
-    UserTypeEnums.bots.has_key?(user_type)
-  end
-
-  # The explicit check for project_bot will be removed with Bot Categorization
-  # Ref: https://gitlab.com/gitlab-org/gitlab/-/issues/213945
-  def internal?
-    ghost? || (bot? && !project_bot?)
-  end
-
-  # We are transitioning from ghost boolean column to user_type
-  # so we need to read from old column for now
-  # @see https://gitlab.com/gitlab-org/gitlab/-/issues/210025
-  def ghost?
-    ghost
-  end
-
-  # The explicit check for project_bot will be removed with Bot Categorization
-  # Ref: https://gitlab.com/gitlab-org/gitlab/-/issues/213945
-  def self.internal
-    where(ghost: true).or(bots_without_project_bot)
-  end
-
-  # The explicit check for project_bot will be removed with Bot Categorization
-  # Ref: https://gitlab.com/gitlab-org/gitlab/-/issues/213945
-  def self.non_internal
-    without_ghosts.with_project_bots
-  end
-
-  def human?
-    user_type.nil?
-  end
-
   #
   # Instance methods
   #
 
+  def full_path
+    username
+  end
+
   def to_param
     username
   end

app/models/user_type_enums.rb

-# frozen_string_literal: true
-
-module UserTypeEnums
-  def self.types
-    @types ||= bots.merge(human: nil, ghost: 5)
-  end
-
-  def self.bots
-    @bots ||= { alert_bot: 2, project_bot: 6 }.with_indifferent_access
-  end
-end
-
-UserTypeEnums.prepend_if_ee('EE::UserTypeEnums')

ee/app/models/ee/user_type_enums.rb

-# frozen_string_literal: true
-
-module EE
-  module UserTypeEnums
-    extend ActiveSupport::Concern
-
-    class_methods do
-      extend ::Gitlab::Utils::Override
-
-      override :types
-      def types
-        @types ||= super.merge(service_user: 4)
-      end
-
-      override :bots
-      def bots
-        @bots ||= super.merge(support_bot: 1, visual_review_bot: 3)
-      end
-    end
-  end
-end

ee/spec/models/ee/user_type_enums_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe UserTypeEnums 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

@@ -87,31 +87,6 @@ describe User do
       end
     end
 
-    describe '.active_without_ghosts' 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, 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
-
     describe '.managed_by' do
       let!(:group) { create(:group_with_managed_accounts) }
       let!(:managed_users) { create_list(:user, 2, managing_group: group) }
@@ -646,27 +621,19 @@ describe User do
       context 'when user is internal' do
         using RSpec::Parameterized::TableSyntax
 
-        where(:bot_user_type) do
-          UserTypeEnums.bots.keys
+        where(:internal_user_type) do
+          described_class::INTERNAL_USER_TYPES
         end
 
         with_them do
-          context 'when user is a bot' do
-            let(:user) { create(:user, user_type: bot_user_type) }
+          context 'when user has internal user type' do
+            let(:user) { create(:user, user_type: internal_user_type) }
 
             it 'returns false' do
               expect(user.using_license_seat?).to eq false
             end
           end
         end
-
-        context 'when user is a ghost' do
-          let(:user) { create(:user, ghost: true) }
-
-          it 'returns false' do
-            expect(user.using_license_seat?).to eq false
-          end
-        end
       end
 
       context 'when user is not internal' do
@@ -1164,7 +1131,7 @@ describe User do
     end
 
     context 'when user is ghost' do
-      let(:user) { build(:user, email: 'test@gitlab.com', ghost: true) }
+      let(:user) { build(:user, :ghost, email: 'test@gitlab.com') }
 
       it { is_expected.to be false }
     end

spec/factories/users.rb

@@ -40,7 +40,7 @@ FactoryBot.define do
     end
 
     trait :ghost do
-      ghost { true }
+      user_type { :ghost }
       after(:build) { |user, _| user.block! }
     end
 

spec/models/concerns/has_user_type_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe User do
+  specify 'types consistency checks', :aggregate_failures do
+    expect(described_class::USER_TYPES)
+      .to include(*%i[human ghost alert_bot project_bot support_bot service_user visual_review_bot])
+    expect(described_class::USER_TYPES).to include(*described_class::BOT_USER_TYPES)
+    expect(described_class::USER_TYPES).to include(*described_class::NON_INTERNAL_USER_TYPES)
+    expect(described_class::USER_TYPES).to include(*described_class::INTERNAL_USER_TYPES)
+  end
+
+  describe 'scopes & predicates' do
+    User::USER_TYPES.keys.each do |type|
+      let_it_be(type) { create(:user, username: type, user_type: type) }
+    end
+    let(:bots) { User::BOT_USER_TYPES.map { |type| public_send(type) } }
+    let(:non_internal) { User::NON_INTERNAL_USER_TYPES.map { |type| public_send(type) } }
+    let(:everyone) { User::USER_TYPES.keys.map { |type| public_send(type) } }
+
+    describe '.humans' do
+      it 'includes humans only' do
+        expect(described_class.humans).to match_array([human])
+      end
+    end
+
+    describe '.bots' do
+      it 'includes all bots' do
+        expect(described_class.bots).to match_array(bots)
+      end
+    end
+
+    describe '.bots_without_project_bot' do
+      it 'includes all bots except project_bot' do
+        expect(described_class.bots_without_project_bot).to match_array(bots - [project_bot])
+      end
+    end
+
+    describe '.non_internal' do
+      it 'includes all non_internal users' do
+        expect(described_class.non_internal).to match_array(non_internal)
+      end
+    end
+
+    describe '.without_ghosts' do
+      it 'includes everyone except ghosts' do
+        expect(described_class.without_ghosts).to match_array(everyone - [ghost])
+      end
+    end
+
+    describe '#bot?' do
+      it 'is true for all bot user types and false for others' do
+        expect(bots).to all(be_bot)
+
+        (everyone - bots).each do |user|
+          expect(user).not_to be_bot
+        end
+      end
+    end
+
+    describe '#human?' do
+      it 'is true for humans only' do
+        expect(human).to be_human
+        expect(alert_bot).not_to be_human
+        expect(User.new).to be_human
+      end
+    end
+
+    describe '#internal?' do
+      it 'is true for all internal user types and false for others' do
+        expect(everyone - non_internal).to all(be_internal)
+
+        non_internal.each do |user|
+          expect(user).not_to be_internal
+        end
+      end
+    end
+  end
+end

spec/models/user_spec.rb

@@ -813,7 +813,7 @@ describe User, :do_not_mock_admin_mode do
     describe '.active_without_ghosts' 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(:user3) { create(:user, :ghost) }
       let_it_be(:user4) { create(:user) }
 
       it 'returns all active users but ghost users' do
@@ -824,7 +824,7 @@ describe User, :do_not_mock_admin_mode do
     describe '.without_ghosts' 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(:user3) { create(:user, :ghost) }
 
       it 'returns users without ghosts users' do
         expect(described_class.without_ghosts).to match_array([user1, user2])
@@ -3275,7 +3275,6 @@ describe User, :do_not_mock_admin_mode do
       expect(ghost.namespace).not_to be_nil
       expect(ghost.namespace).to be_persisted
       expect(ghost.user_type).to eq 'ghost'
-      expect(ghost.ghost).to eq true
     end
 
     it "does not create a second ghost user if one is already present" do
@@ -4077,7 +4076,7 @@ describe User, :do_not_mock_admin_mode do
 
     context 'in single-user environment' do
       it 'requires user consent after one week' do
-        create(:user, ghost: true)
+        create(:user, :ghost)
 
         expect(user.requires_usage_stats_consent?).to be true
       end
@@ -4355,31 +4354,15 @@ describe User, :do_not_mock_admin_mode do
     end
   end
 
-  describe 'internal methods' do
-    let_it_be(:user) { create(:user) }
-    let_it_be(:ghost) { described_class.ghost }
-    let_it_be(:alert_bot) { described_class.alert_bot }
-    let_it_be(:project_bot) { create(:user, :project_bot) }
-    let_it_be(:non_internal) { [user, project_bot] }
-    let_it_be(:internal) { [ghost, alert_bot] }
-
-    it 'returns internal users' do
-      expect(described_class.internal).to match_array(internal)
-      expect(internal.all?(&:internal?)).to eq(true)
-    end
+  describe '.active_without_ghosts' do
+    let_it_be(:user1) { create(:user, :external) }
+    let_it_be(:user2) { create(:user, state: 'blocked') }
+    let_it_be(:user3) { create(:user, :ghost) }
+    let_it_be(:user4) { create(:user, user_type: :support_bot) }
+    let_it_be(:user5) { create(:user, state: 'blocked', user_type: :support_bot) }
 
-    it 'returns non internal users' do
-      expect(described_class.non_internal).to match_array(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(alert_bot.bot?).to eq(true)
-      end
+    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
 
@@ -4417,19 +4400,6 @@ describe User, :do_not_mock_admin_mode do
     end
   end
 
-  describe 'bots & humans' do
-    it 'returns corresponding users' do
-      human = create(:user)
-      bot = create(:user, :bot)
-      project_bot = create(:user, :project_bot)
-
-      expect(described_class.humans).to match_array([human])
-      expect(described_class.bots).to match_array([bot, project_bot])
-      expect(described_class.bots_without_project_bot).to match_array([bot])
-      expect(described_class.with_project_bots).to match_array([human, project_bot])
-    end
-  end
-
   describe '#hook_attrs' do
     it 'includes name, username, avatar_url, and email' do
       user = create(:user)
@@ -4503,7 +4473,7 @@ describe User, :do_not_mock_admin_mode do
         where(:attributes) do
           [
             { state: 'blocked' },
-            { ghost: true },
+            { user_type: :ghost },
             { user_type: :alert_bot }
           ]
         end
@@ -4546,7 +4516,7 @@ describe User, :do_not_mock_admin_mode do
 
     context 'when user is a ghost user' do
       before do
-        user.update(ghost: true)
+        user.update(user_type: :ghost)
       end
 
       it { is_expected.to be false }
@@ -4585,7 +4555,7 @@ describe User, :do_not_mock_admin_mode do
 
     context 'when user is an internal user' do
       before do
-        user.update(ghost: true)
+        user.update(user_type: :ghost)
       end
 
       it { is_expected.to be User::LOGIN_FORBIDDEN }
@@ -4625,26 +4595,4 @@ describe User, :do_not_mock_admin_mode do
       it_behaves_like 'does not require password to be present'
     end
   end
-
-  describe '#human?' do
-    subject { user.human? }
-
-    let_it_be(:user) { create(:user) }
-
-    context 'when user is a human' do
-      before do
-        user.update(user_type: nil)
-      end
-
-      it { is_expected.to be true }
-    end
-
-    context 'when user is not a human' do
-      before do
-        user.update(user_type: 'alert_bot')
-      end
-
-      it { is_expected.to be false }
-    end
-  end
 end

spec/models/user_type_enums_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe UserTypeEnums do
-  it '.types' do
-    expect(described_class.types.keys).to include('alert_bot', 'project_bot', 'human', 'ghost')
-  end
-
-  it '.bots' do
-    expect(described_class.bots.keys).to include('alert_bot', 'project_bot')
-  end
-end

spec/views/admin/users/_user.html.haml_spec.rb

@@ -9,7 +9,7 @@ describe 'admin/users/_user.html.haml' do
 
   context 'internal users' do
     context 'when showing a `Ghost User`' do
-      let(:user) { create(:user, ghost: true) }
+      let(:user) { create(:user, :ghost) }
 
       it 'does not render action buttons' do
         render

spec/workers/update_highest_role_worker_spec.rb

@@ -18,7 +18,6 @@ describe UpdateHighestRoleWorker, :clean_gitlab_redis_shared_state do
       let(:active_attributes) do
         {
           state: 'active',
-          ghost: false,
           user_type: nil
         }
       end
@@ -54,7 +53,6 @@ describe UpdateHighestRoleWorker, :clean_gitlab_redis_shared_state do
         where(:additional_attributes) do
           [
             { state: 'blocked' },
-            { ghost: true },
             { user_type: :alert_bot }
           ]
         end