From 589e9802a94e6c6bb7b4cf1770eb4a32498e591f Mon Sep 17 00:00:00 2001
From: Andy Allan <git@gravitystorm.co.uk>
Date: Wed, 1 Feb 2017 17:42:05 +0000
Subject: [PATCH 01/11] Create a simple user factory, and convert some basic
 tests.

---
 test/factories/user.rb   |  7 +++++++
 test/models/user_test.rb | 12 ++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)
 create mode 100644 test/factories/user.rb

diff --git a/test/factories/user.rb b/test/factories/user.rb
new file mode 100644
index 000000000..ccbb84f93
--- /dev/null
+++ b/test/factories/user.rb
@@ -0,0 +1,7 @@
+FactoryGirl.define do
+  factory :user do
+    sequence(:email) { |n| "user#{n}@example.com" }
+    sequence(:display_name) { |n| "User #{n}" }
+    pass_crypt Digest::MD5.hexdigest("test")
+  end
+end
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index f8f46cfaf..6ecc8c796 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -52,20 +52,20 @@ class UserTest < ActiveSupport::TestCase
              輕觸搖晃的遊戲@ah.com も対応します@s.name)
 
     ok.each do |name|
-      user = users(:normal_user)
+      user = build(:user)
       user.email = name
       assert user.valid?(:save), user.errors.full_messages.join(",")
     end
 
     bad.each do |name|
-      user = users(:normal_user)
+      user = build(:user)
       user.email = name
       assert user.invalid?(:save), "#{name} is valid when it shouldn't be"
     end
   end
 
   def test_display_name_length
-    user = users(:normal_user)
+    user = build(:user)
     user.display_name = "123"
     assert user.valid?, " should allow nil display name"
     user.display_name = "12"
@@ -93,13 +93,13 @@ class UserTest < ActiveSupport::TestCase
            "new", "terms", "save", "confirm", "confirm-email",
            "go_public", "reset-password", "forgot-password", "suspended"]
     ok.each do |display_name|
-      user = users(:normal_user)
+      user = build(:user)
       user.display_name = display_name
       assert user.valid?, "#{display_name} is invalid, when it should be"
     end
 
     bad.each do |display_name|
-      user = users(:normal_user)
+      user = build(:user)
       user.display_name = display_name
       assert !user.valid?, "#{display_name} is valid when it shouldn't be"
     end
@@ -150,7 +150,7 @@ class UserTest < ActiveSupport::TestCase
   end
 
   def test_user_preferred_editor
-    user = users(:normal_user)
+    user = create(:user)
     assert_nil user.preferred_editor
     user.preferred_editor = "potlatch"
     assert_equal "potlatch", user.preferred_editor

From 9e591d8ccb264aec201020d537dbf2dca6e15bbf Mon Sep 17 00:00:00 2001
From: Andy Allan <git@gravitystorm.co.uk>
Date: Thu, 2 Feb 2017 12:08:36 +0000
Subject: [PATCH 02/11] Add with_home_location trait for user factories.

---
 test/factories/user.rb   | 5 +++++
 test/models/user_test.rb | 6 +++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/test/factories/user.rb b/test/factories/user.rb
index ccbb84f93..79b9a2a16 100644
--- a/test/factories/user.rb
+++ b/test/factories/user.rb
@@ -3,5 +3,10 @@ FactoryGirl.define do
     sequence(:email) { |n| "user#{n}@example.com" }
     sequence(:display_name) { |n| "User #{n}" }
     pass_crypt Digest::MD5.hexdigest("test")
+
+    trait :with_home_location do
+      home_lat { rand(-90.0...90.0) }
+      home_lon { rand(-180.0...180.0) }
+    end
   end
 end
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 6ecc8c796..5eacd98b9 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -240,7 +240,7 @@ class UserTest < ActiveSupport::TestCase
   end
 
   def test_delete
-    user = users(:normal_user)
+    user = create(:user, :with_home_location, :description => "foo")
     user.delete
     assert_equal "user_#{user.id}", user.display_name
     assert user.description.blank?
@@ -253,7 +253,7 @@ class UserTest < ActiveSupport::TestCase
   end
 
   def test_to_xml
-    user = users(:normal_user)
+    user = build(:user, :with_home_location)
     xml = user.to_xml
     assert_select Nokogiri::XML::Document.parse(xml.to_s), "user" do
       assert_select "[display_name=?]", user.display_name
@@ -263,7 +263,7 @@ class UserTest < ActiveSupport::TestCase
   end
 
   def test_to_xml_node
-    user = users(:normal_user)
+    user = build(:user, :with_home_location)
     xml = user.to_xml_node
     assert_select Nokogiri::XML::DocumentFragment.parse(xml.to_s), "user" do
       assert_select "[display_name=?]", user.display_name

From c55169659ccab3fdb4aea11b5bea6ad62b6d297d Mon Sep 17 00:00:00 2001
From: Andy Allan <git@gravitystorm.co.uk>
Date: Thu, 2 Feb 2017 12:35:29 +0000
Subject: [PATCH 03/11] Add a user_role factory

Naming the association in the model makes the relationship easier
to express in the factory.
---
 app/models/user_role.rb     | 1 +
 test/factories/user_role.rb | 6 ++++++
 2 files changed, 7 insertions(+)
 create mode 100644 test/factories/user_role.rb

diff --git a/app/models/user_role.rb b/app/models/user_role.rb
index 6bc7435ae..4f80f1952 100644
--- a/app/models/user_role.rb
+++ b/app/models/user_role.rb
@@ -1,5 +1,6 @@
 class UserRole < ActiveRecord::Base
   belongs_to :user
+  belongs_to :granter, :class_name => "User"
 
   ALL_ROLES = %w(administrator moderator).freeze
 
diff --git a/test/factories/user_role.rb b/test/factories/user_role.rb
new file mode 100644
index 000000000..0eea25cee
--- /dev/null
+++ b/test/factories/user_role.rb
@@ -0,0 +1,6 @@
+FactoryGirl.define do
+  factory :user_role do
+    user
+    association :granter, :factory => :user
+  end
+end

From 38fc6331af1e45e077db040a317845607050d2a3 Mon Sep 17 00:00:00 2001
From: Andy Allan <git@gravitystorm.co.uk>
Date: Thu, 2 Feb 2017 12:38:19 +0000
Subject: [PATCH 04/11] Add moderator_user and administrator_user factories.

---
 test/factories/user.rb   | 12 ++++++++++++
 test/models/user_test.rb | 16 ++++++++--------
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/test/factories/user.rb b/test/factories/user.rb
index 79b9a2a16..6376659ab 100644
--- a/test/factories/user.rb
+++ b/test/factories/user.rb
@@ -8,5 +8,17 @@ FactoryGirl.define do
       home_lat { rand(-90.0...90.0) }
       home_lon { rand(-180.0...180.0) }
     end
+
+    factory :moderator_user do
+      after(:create) do |user, _evaluator|
+        create(:user_role, :role => "moderator", :user => user)
+      end
+    end
+
+    factory :administrator_user do
+      after(:create) do |user, _evaluator|
+        create(:user_role, :role => "administrator", :user => user)
+      end
+    end
   end
 end
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 5eacd98b9..6632702e5 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -223,20 +223,20 @@ class UserTest < ActiveSupport::TestCase
   end
 
   def test_moderator?
-    assert_equal false, users(:normal_user).moderator?
-    assert_equal true, users(:moderator_user).moderator?
+    assert_equal false, create(:user).moderator?
+    assert_equal true, create(:moderator_user).moderator?
   end
 
   def test_administrator?
-    assert_equal false, users(:normal_user).administrator?
-    assert_equal true, users(:administrator_user).administrator?
+    assert_equal false, create(:user).administrator?
+    assert_equal true, create(:administrator_user).administrator?
   end
 
   def test_has_role?
-    assert_equal false, users(:normal_user).has_role?("administrator")
-    assert_equal false, users(:normal_user).has_role?("moderator")
-    assert_equal true, users(:administrator_user).has_role?("administrator")
-    assert_equal true, users(:moderator_user).has_role?("moderator")
+    assert_equal false, create(:user).has_role?("administrator")
+    assert_equal false, create(:user).has_role?("moderator")
+    assert_equal true, create(:administrator_user).has_role?("administrator")
+    assert_equal true, create(:moderator_user).has_role?("moderator")
   end
 
   def test_delete

From 0bd2e9ea8c801e5b6cd570837fcd112f6f78b180 Mon Sep 17 00:00:00 2001
From: Andy Allan <git@gravitystorm.co.uk>
Date: Thu, 2 Feb 2017 12:55:32 +0000
Subject: [PATCH 05/11] Add status traits to user factory.

---
 test/factories/user.rb   | 20 ++++++++++++++++++++
 test/models/user_test.rb | 20 ++++++++++----------
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/test/factories/user.rb b/test/factories/user.rb
index 6376659ab..9ec4e2030 100644
--- a/test/factories/user.rb
+++ b/test/factories/user.rb
@@ -9,6 +9,26 @@ FactoryGirl.define do
       home_lon { rand(-180.0...180.0) }
     end
 
+    trait :pending do
+      status "pending"
+    end
+
+    trait :active do
+      status "active"
+    end
+
+    trait :confirmed do
+      status "confirmed"
+    end
+
+    trait :suspended do
+      status "suspended"
+    end
+
+    trait :deleted do
+      status "deleted"
+    end
+
     factory :moderator_user do
       after(:create) do |user, _evaluator|
         create(:user_role, :role => "moderator", :user => user)
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 6632702e5..356f469c0 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -207,19 +207,19 @@ class UserTest < ActiveSupport::TestCase
   end
 
   def test_visible?
-    assert_equal true, users(:inactive_user).visible?
-    assert_equal true, users(:normal_user).visible?
-    assert_equal true, users(:confirmed_user).visible?
-    assert_equal false, users(:suspended_user).visible?
-    assert_equal false, users(:deleted_user).visible?
+    assert_equal true, build(:user, :pending).visible?
+    assert_equal true, build(:user, :active).visible?
+    assert_equal true, build(:user, :confirmed).visible?
+    assert_equal false, build(:user, :suspended).visible?
+    assert_equal false, build(:user, :deleted).visible?
   end
 
   def test_active?
-    assert_equal false, users(:inactive_user).active?
-    assert_equal true, users(:normal_user).active?
-    assert_equal true, users(:confirmed_user).active?
-    assert_equal false, users(:suspended_user).active?
-    assert_equal false, users(:deleted_user).active?
+    assert_equal false, build(:user, :pending).active?
+    assert_equal true, build(:user, :active).active?
+    assert_equal true, build(:user, :confirmed).active?
+    assert_equal false, build(:user, :suspended).active?
+    assert_equal false, build(:user, :deleted).active?
   end
 
   def test_moderator?

From df6469c76b4d1f069a0831335a68e23ea1ebe0b1 Mon Sep 17 00:00:00 2001
From: Andy Allan <git@gravitystorm.co.uk>
Date: Thu, 2 Feb 2017 13:05:37 +0000
Subject: [PATCH 06/11] Convert class method tests to use User factory.

Also improve the tests by being explicit about which results should
be returned, rather than just hoping that the fixtures cover all
possibilities.
---
 test/models/user_test.rb | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 356f469c0..90499e1b1 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -161,32 +161,50 @@ class UserTest < ActiveSupport::TestCase
   end
 
   def test_visible
-    assert_equal 23, User.visible.count
+    pending = create(:user, :pending)
+    active = create(:user, :active)
+    confirmed = create(:user, :confirmed)
+    suspended = create(:user, :suspended)
+    deleted = create(:user, :deleted)
+
+    assert User.visible.find(pending.id)
+    assert User.visible.find(active.id)
+    assert User.visible.find(confirmed.id)
     assert_raise ActiveRecord::RecordNotFound do
-      User.visible.find(users(:suspended_user).id)
+      User.visible.find(suspended.id)
     end
     assert_raise ActiveRecord::RecordNotFound do
-      User.visible.find(users(:deleted_user).id)
+      User.visible.find(deleted.id)
     end
   end
 
   def test_active
-    assert_equal 22, User.active.count
+    pending = create(:user, :pending)
+    active = create(:user, :active)
+    confirmed = create(:user, :confirmed)
+    suspended = create(:user, :suspended)
+    deleted = create(:user, :deleted)
+
+    assert User.active.find(active.id)
+    assert User.active.find(confirmed.id)
     assert_raise ActiveRecord::RecordNotFound do
-      User.active.find(users(:inactive_user).id)
+      User.active.find(pending.id)
     end
     assert_raise ActiveRecord::RecordNotFound do
-      User.active.find(users(:suspended_user).id)
+      User.active.find(suspended.id)
     end
     assert_raise ActiveRecord::RecordNotFound do
-      User.active.find(users(:deleted_user).id)
+      User.active.find(deleted.id)
     end
   end
 
   def test_identifiable
-    assert_equal 24, User.identifiable.count
+    public_user = create(:user, :data_public => true)
+    private_user = create(:user, :data_public => false)
+
+    assert User.identifiable.find(public_user.id)
     assert_raise ActiveRecord::RecordNotFound do
-      User.identifiable.find(users(:normal_user).id)
+      User.identifiable.find(private_user.id)
     end
   end
 

From 584748fe0a9b20f7af5f9c6ebf127ec15b1350ad Mon Sep 17 00:00:00 2001
From: Andy Allan <git@gravitystorm.co.uk>
Date: Thu, 2 Feb 2017 13:09:39 +0000
Subject: [PATCH 07/11] User factory for uniqueness tests

---
 test/models/user_test.rb | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 90499e1b1..3f66c5827 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -20,8 +20,9 @@ class UserTest < ActiveSupport::TestCase
   end
 
   def test_unique_email
+    existing_user = create(:user)
     new_user = User.new(
-      :email => users(:normal_user).email,
+      :email => existing_user.email,
       :status => "active",
       :pass_crypt => Digest::MD5.hexdigest("test"),
       :display_name => "new user",
@@ -33,11 +34,12 @@ class UserTest < ActiveSupport::TestCase
   end
 
   def test_unique_display_name
+    existing_user = create(:user)
     new_user = User.new(
       :email => "tester@openstreetmap.org",
       :status => "pending",
       :pass_crypt => Digest::MD5.hexdigest("test"),
-      :display_name => users(:normal_user).display_name,
+      :display_name => existing_user.display_name,
       :data_public => 1,
       :description => "desc"
     )

From f96c563c10e7d2fd3e90e9bcf1c58b0dc804a3d9 Mon Sep 17 00:00:00 2001
From: Andy Allan <git@gravitystorm.co.uk>
Date: Thu, 2 Feb 2017 13:20:51 +0000
Subject: [PATCH 08/11] Convert test_friend_with to use factories, and use
 alice/bob/charlie for easier understanding.

---
 test/models/user_test.rb | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 3f66c5827..dd2f9691d 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -108,13 +108,17 @@ class UserTest < ActiveSupport::TestCase
   end
 
   def test_friend_with
-    create(:friend, :befriender => users(:normal_user), :befriendee => users(:public_user))
-    assert users(:normal_user).is_friends_with?(users(:public_user))
-    assert !users(:normal_user).is_friends_with?(users(:inactive_user))
-    assert !users(:public_user).is_friends_with?(users(:normal_user))
-    assert !users(:public_user).is_friends_with?(users(:inactive_user))
-    assert !users(:inactive_user).is_friends_with?(users(:normal_user))
-    assert !users(:inactive_user).is_friends_with?(users(:public_user))
+    alice = create(:user, :active)
+    bob = create(:user, :active)
+    charlie = create(:user, :active)
+    create(:friend, :befriender => alice, :befriendee => bob)
+
+    assert alice.is_friends_with?(bob)
+    assert !alice.is_friends_with?(charlie)
+    assert !bob.is_friends_with?(alice)
+    assert !bob.is_friends_with?(charlie)
+    assert !charlie.is_friends_with?(bob)
+    assert !charlie.is_friends_with?(alice)
   end
 
   def test_users_nearby

From b83271de92a114a960e3432b8c281109b1eda508 Mon Sep 17 00:00:00 2001
From: Andy Allan <git@gravitystorm.co.uk>
Date: Thu, 2 Feb 2017 13:24:46 +0000
Subject: [PATCH 09/11] Refactor the friend_users test, and remove the tests
 which duplicate the (renamed) test_friends_with above.

---
 test/models/user_test.rb | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index dd2f9691d..34f2a5491 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -107,7 +107,7 @@ class UserTest < ActiveSupport::TestCase
     end
   end
 
-  def test_friend_with
+  def test_friends_with
     alice = create(:user, :active)
     bob = create(:user, :active)
     charlie = create(:user, :active)
@@ -134,25 +134,16 @@ class UserTest < ActiveSupport::TestCase
     assert_equal [], users(:confirmed_user).nearby
   end
 
-  def test_friends_with
-    # normal user is a friend of second user
-    # it should be a one way friend associatation
-    norm = users(:normal_user)
-    sec = users(:public_user)
+  def test_friend_users
+    norm = create(:user, :active)
+    sec = create(:user, :active)
     create(:friend, :befriender => norm, :befriendee => sec)
-    assert_equal 1, Friend.count
+
     assert_equal [sec], norm.friend_users
     assert_equal 1, norm.friend_users.size
-    assert_equal 1, Friend.count
-    assert norm.is_friends_with?(sec)
-    assert !sec.is_friends_with?(norm)
-    assert !users(:normal_user).is_friends_with?(users(:inactive_user))
-    assert !users(:public_user).is_friends_with?(users(:normal_user))
-    assert !users(:public_user).is_friends_with?(users(:inactive_user))
-    assert !users(:inactive_user).is_friends_with?(users(:normal_user))
-    assert !users(:inactive_user).is_friends_with?(users(:public_user))
-    # Friend.delete(friend)
-    # assert_equal 0, Friend.count
+
+    assert_equal [], sec.friend_users
+    assert_equal 0, sec.friend_users.size
   end
 
   def test_user_preferred_editor

From 3b26ff0c40fa127e0d0732bce3d7116e2fa7695d Mon Sep 17 00:00:00 2001
From: Andy Allan <git@gravitystorm.co.uk>
Date: Thu, 2 Feb 2017 13:33:15 +0000
Subject: [PATCH 10/11] Use factory for user language tests

---
 test/models/user_test.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 34f2a5491..43ccb6a45 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -210,14 +210,14 @@ class UserTest < ActiveSupport::TestCase
     create(:language, :code => "de")
     create(:language, :code => "sl")
 
-    user = users(:normal_user)
+    user = create(:user, :languages => ["en"])
     assert_equal ["en"], user.languages
     user.languages = %w(de fr en)
     assert_equal %w(de fr en), user.languages
     user.languages = %w(fr de sl)
     assert_equal "de", user.preferred_language
     assert_equal %w(fr de sl), user.preferred_languages.map(&:to_s)
-    user = users(:public_user)
+    user = create(:user, :languages => %w(en de))
     assert_equal %w(en de), user.languages
   end
 

From 1041ead2538cc6cc69b91df1e650d4f32096ff09 Mon Sep 17 00:00:00 2001
From: Andy Allan <git@gravitystorm.co.uk>
Date: Thu, 2 Feb 2017 13:52:26 +0000
Subject: [PATCH 11/11] Convert the test_users_nearby to user factories.

---
 test/models/user_test.rb | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 43ccb6a45..a2a45e203 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -122,16 +122,24 @@ class UserTest < ActiveSupport::TestCase
   end
 
   def test_users_nearby
-    # second user has their data public and is close by normal user
-    assert_equal [users(:public_user), users(:german_user)], users(:normal_user).nearby
-    # second_user has normal user nearby, but normal user has their data private
-    assert_equal [users(:german_user)], users(:public_user).nearby
-    # inactive_user has no user nearby
-    assert_equal [], users(:inactive_user).nearby
-    # north_pole_user has no user nearby, and doesn't throw exception
-    assert_equal [], users(:north_pole_user).nearby
-    # confirmed_user has no home location
-    assert_equal [], users(:confirmed_user).nearby
+    alice = create(:user, :active, :home_lat => 51.0, :home_lon => 1.0, :data_public => false)
+    bob = create(:user, :active, :home_lat => 51.1, :home_lon => 1.0, :data_public => true)
+    charlie = create(:user, :active, :home_lat => 51.1, :home_lon => 1.1, :data_public => true)
+    david = create(:user, :active, :home_lat => 10.0, :home_lon => -123.0, :data_public => true)
+    _edward = create(:user, :suspended, :home_lat => 10.0, :home_lon => -123.0, :data_public => true)
+    south_pole_user = create(:user, :active, :home_lat => -90.0, :home_lon => 0.0, :data_public => true)
+    vagrant_user = create(:user, :active, :home_lat => nil, :home_lon => nil, :data_public => true)
+
+    # bob and charlie are both near alice
+    assert_equal [bob, charlie], alice.nearby
+    # charlie and alice are both near bob, but alice has their data private
+    assert_equal [charlie], bob.nearby
+    # david has no user nearby, since edward is not active
+    assert_equal [], david.nearby
+    # south_pole_user has no user nearby, and doesn't throw exception
+    assert_equal [], south_pole_user.nearby
+    # vagrant_user has no home location
+    assert_equal [], vagrant_user.nearby
   end
 
   def test_friend_users