From 8e684809765fa866a125c176327825ebc565f5b3 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Mon, 6 Feb 2017 17:37:05 +0530
Subject: [PATCH] Use a `ghost` boolean to track ghost users.

Rather than using a separate `ghost` state. This lets us have the benefits of
both ghost and blocked users (ghost: true, state: blocked) without having to
rewrite a number of queries to include cases for `state: ghost`.
---
 app/models/user.rb                                | 15 ++++++++++-----
 app/services/notification_service.rb              |  1 -
 .../20170206115204_add_column_ghost_to_users.rb   | 15 +++++++++++++++
 db/schema.rb                                      |  1 +
 spec/factories/users.rb                           |  4 ++++
 spec/models/user_spec.rb                          | 14 ++++++++++++++
 6 files changed, 44 insertions(+), 6 deletions(-)
 create mode 100644 db/migrate/20170206115204_add_column_ghost_to_users.rb

diff --git a/app/models/user.rb b/app/models/user.rb
index e4adcd6cde9..f71d7e90d33 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -124,6 +124,7 @@ class User < ActiveRecord::Base
   validate :unique_email, if: ->(user) { user.email_changed? }
   validate :owns_notification_email, if: ->(user) { user.notification_email_changed? }
   validate :owns_public_email, if: ->(user) { user.public_email_changed? }
+  validate :ghost_users_must_be_blocked
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
   before_validation :generate_password, on: :create
@@ -185,8 +186,6 @@ class User < ActiveRecord::Base
           "administrator if you think this is an error."
       end
     end
-
-    state :ghost
   end
 
   mount_uploader :avatar, AvatarUploader
@@ -347,7 +346,7 @@ class User < ActiveRecord::Base
     # Return (create if necessary) the ghost user. The ghost user
     # owns records previously belonging to deleted users.
     def ghost
-      ghost_user = User.with_state(:ghost).first
+      ghost_user = User.find_by_ghost(true)
 
       ghost_user ||
         begin
@@ -359,7 +358,7 @@ class User < ActiveRecord::Base
           # Recheck if a ghost user is already present (one might have been)
           # added between the time we last checked (first line of this method)
           # and the time we acquired the lock.
-          ghost_user = User.with_state(:ghost).first
+          ghost_user = User.find_by_ghost(true)
           return ghost_user if ghost_user.present?
 
           uniquify = Uniquify.new
@@ -373,7 +372,7 @@ class User < ActiveRecord::Base
 
           User.create(
             username: username, password: Devise.friendly_token,
-            email: email, name: "Ghost User", state: :ghost
+            email: email, name: "Ghost User", state: :blocked, ghost: true
           )
         ensure
           advisory_lock.unlock
@@ -477,6 +476,12 @@ class User < ActiveRecord::Base
     errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email)
   end
 
+  def ghost_users_must_be_blocked
+    if ghost? && !blocked?
+      errors.add(:ghost, 'cannot be enabled for a user who is not blocked.')
+    end
+  end
+
   def update_emails_with_primary_email
     primary_email_record = emails.find_by(email: email)
     if primary_email_record
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 97db117aa99..3734e3c4253 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -466,7 +466,6 @@ class NotificationService
 
     users = users.to_a.compact.uniq
     users = users.reject(&:blocked?)
-    users = users.reject(&:ghost?)
 
     users.reject do |user|
       global_notification_setting = user.global_notification_setting
diff --git a/db/migrate/20170206115204_add_column_ghost_to_users.rb b/db/migrate/20170206115204_add_column_ghost_to_users.rb
new file mode 100644
index 00000000000..57b66ca91a8
--- /dev/null
+++ b/db/migrate/20170206115204_add_column_ghost_to_users.rb
@@ -0,0 +1,15 @@
+class AddColumnGhostToUsers < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default :users, :ghost, :boolean, default: false, allow_null: false
+  end
+
+  def down
+    remove_column :users, :ghost
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 82bccda580a..c50d6fc4ad6 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1282,6 +1282,7 @@ ActiveRecord::Schema.define(version: 20170216141440) do
     t.string "incoming_email_token"
     t.boolean "authorized_projects_populated"
     t.boolean "notified_of_own_activity", default: false, null: false
+    t.boolean "ghost", default: false, null: false
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree
diff --git a/spec/factories/users.rb b/spec/factories/users.rb
index 1732b1a0081..58602c54c7a 100644
--- a/spec/factories/users.rb
+++ b/spec/factories/users.rb
@@ -26,6 +26,10 @@ FactoryGirl.define do
       two_factor_via_otp
     end
 
+    trait :ghost do
+      ghost true
+    end
+
     trait :two_factor_via_otp do
       before(:create) do |user|
         user.otp_required_for_login = true
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 861d338ef95..9e2b1c9290f 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -208,6 +208,20 @@ describe User, models: true do
         end
       end
     end
+
+    describe 'ghost users' do
+      it 'does not allow a non-blocked ghost user' do
+        user = build(:user, :ghost, state: :active)
+
+        expect(user).to be_invalid
+      end
+
+      it 'allows a blocked ghost user' do
+        user = build(:user, :ghost, state: :blocked)
+
+        expect(user).to be_valid
+      end
+    end
   end
 
   describe "scopes" do