From ca16c3734b7b89f71bdc9e1c18152aa1599c4f89 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 3 Feb 2017 18:25:50 +0530
Subject: [PATCH] Extract code from `Namespace#clean_path` for ghost user
 generation.

1. Create a `Uniquify` class, which generalizes the process of generating unique
   strings, by accepting a function that defines what "uniqueness" means in a
   given context.

2. WIP: Make sure tests for `Namespace` pass, add more if necessary.

3. WIP: Add tests for `Uniquify`
---
 app/models/concerns/uniquify.rb | 25 +++++++++++++++++++++++++
 app/models/namespace.rb         | 10 ++--------
 app/models/user.rb              | 23 +++++++++++------------
 spec/models/user_spec.rb        |  4 ++--
 4 files changed, 40 insertions(+), 22 deletions(-)
 create mode 100644 app/models/concerns/uniquify.rb

diff --git a/app/models/concerns/uniquify.rb b/app/models/concerns/uniquify.rb
new file mode 100644
index 00000000000..1485ab6ae99
--- /dev/null
+++ b/app/models/concerns/uniquify.rb
@@ -0,0 +1,25 @@
+class Uniquify
+  # Return a version of the given 'base' string that is unique
+  # by appending a counter to it. Uniqueness is determined by
+  # repeated calls to `exists_fn`.
+  #
+  # If `base` is a function/proc, we expect that calling it with a
+  # candidate counter returns a string to test/return.
+  def string(base, exists_fn)
+    @counter = nil
+
+    if base.respond_to?(:call)
+      increment_counter! while exists_fn[base.call(@counter)]
+      base.call(@counter)
+    else
+      increment_counter! while exists_fn["#{base}#{@counter}"]
+      "#{base}#{@counter}"
+    end
+  end
+
+  private
+
+  def increment_counter!
+    @counter = @counter ? @counter.next : 1
+  end
+end
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index bd0336c984a..8cc3c1473e2 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -98,14 +98,8 @@ class Namespace < ActiveRecord::Base
       # Work around that by setting their username to "blank", followed by a counter.
       path = "blank" if path.blank?
 
-      counter = 0
-      base = path
-      while Namespace.find_by_path_or_name(path)
-        counter += 1
-        path = "#{base}#{counter}"
-      end
-
-      path
+      uniquify = Uniquify.new
+      uniquify.string(path, -> (s) { Namespace.find_by_path_or_name(s) })
     end
   end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 23beaa6c7c8..13529c9997a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -351,20 +351,19 @@ class User < ActiveRecord::Base
 
       ghost_user ||
         begin
-          users = Enumerator.new do |y|
-            n = nil
-            loop do
-              user = User.new(
-                username: "ghost#{n}", password: Devise.friendly_token,
-                email: "ghost#{n}@example.com", name: "Ghost User", state: :ghost
-              )
+          uniquify = Uniquify.new
 
-              y.yield(user)
-              n = n ? n.next : 0
-            end
-          end
+          username = uniquify.string("ghost", -> (s) { User.find_by_username(s) })
 
-          users.lazy.select { |user| user.valid? }.first.tap(&:save!)
+          email = uniquify.string(
+            -> (n) { "ghost#{n}@example.com" },
+            -> (s) { User.find_by_email(s) }
+          )
+
+          User.create(
+            username: username, password: Devise.friendly_token,
+            email: email, name: "Ghost User", state: :ghost
+          )
         end
     end
   end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 00e7927bf90..861d338ef95 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1513,7 +1513,7 @@ describe User, models: true do
         ghost = User.ghost
 
         expect(ghost).to be_persisted
-        expect(ghost.username).to eq('ghost0')
+        expect(ghost.username).to eq('ghost1')
       end
     end
 
@@ -1523,7 +1523,7 @@ describe User, models: true do
         ghost = User.ghost
 
         expect(ghost).to be_persisted
-        expect(ghost.email).to eq('ghost0@example.com')
+        expect(ghost.email).to eq('ghost1@example.com')
       end
     end
   end