Prefer find_by() instead of where().first

These are very similar, differing only if we would expect multiple
results and the sorting is important. However, in all our cases
we're only expecting one result to be returned, and so find_by is
easier to read.

.rubocop.yml

@@ -48,6 +48,9 @@ Naming/MethodParameterName:
 Rails/CreateTableWithTimestamps:
   Enabled: false
 
+Rails/FindBy:
+  IgnoreWhereFirst: false
+
 Rails/FindEach:
   Enabled: false
 

app/controllers/api/traces_controller.rb

@@ -115,7 +115,7 @@ def do_create(file, tags, description, visibility)
       trace.save!
 
       # Finally save the user's preferred privacy level
-      if pref = current_user.preferences.where(:k => "gps.trace.visibility").first
+      if pref = current_user.preferences.find_by(:k => "gps.trace.visibility")
         pref.v = visibility
         pref.save
       else

app/controllers/application_controller.rb

@@ -22,7 +22,7 @@ class ApplicationController < ActionController::Base
 
   def authorize_web
     if session[:user]
-      self.current_user = User.where(:id => session[:user], :status => %w[active confirmed suspended]).first
+      self.current_user = User.find_by(:id => session[:user], :status => %w[active confirmed suspended])
 
       if session[:fingerprint] &&
          session[:fingerprint] != current_user.fingerprint

app/controllers/diary_entries_controller.rb

@@ -61,7 +61,7 @@ def index
   def show
     entries = @user.diary_entries
     entries = entries.visible unless can? :unhide, DiaryEntry
-    @entry = entries.where(:id => params[:id]).first
+    @entry = entries.find_by(:id => params[:id])
     if @entry
       @title = t ".title", :user => params[:display_name], :title => @entry.title
       @comments = can?(:unhidecomment, DiaryEntry) ? @entry.comments : @entry.visible_comments
@@ -74,7 +74,7 @@ def show
   def new
     @title = t ".title"
 
-    default_lang = current_user.preferences.where(:k => "diary.default_language").first
+    default_lang = current_user.preferences.find_by(:k => "diary.default_language")
     lang_code = default_lang ? default_lang.v : current_user.preferred_language
     @diary_entry = DiaryEntry.new(entry_params.merge(:language_code => lang_code))
     set_map_location
@@ -99,7 +99,7 @@ def create
     @diary_entry.user = current_user
 
     if @diary_entry.save
-      default_lang = current_user.preferences.where(:k => "diary.default_language").first
+      default_lang = current_user.preferences.find_by(:k => "diary.default_language")
       if default_lang
         default_lang.v = @diary_entry.language_code
         default_lang.save!

app/controllers/traces_controller.rb

@@ -19,7 +19,7 @@ def index
     # from display name, pick up user id if one user's traces only
     display_name = params[:display_name]
     if display_name.present?
-      target_user = User.active.where(:display_name => display_name).first
+      target_user = User.active.find_by(:display_name => display_name)
       if target_user.nil?
         render_unknown_user display_name
         return
@@ -283,7 +283,7 @@ def do_create(file, tags, description, visibility)
     # Save the trace object
     if trace.save
       # Finally save the user's preferred privacy level
-      if pref = current_user.preferences.where(:k => "gps.trace.visibility").first
+      if pref = current_user.preferences.find_by(:k => "gps.trace.visibility")
         pref.v = visibility
         pref.save
       else
@@ -303,11 +303,11 @@ def offline_redirect
   end
 
   def default_visibility
-    visibility = current_user.preferences.where(:k => "gps.trace.visibility").first
+    visibility = current_user.preferences.find_by(:k => "gps.trace.visibility")
 
     if visibility
       visibility.v
-    elsif current_user.preferences.where(:k => "gps.trace.public", :v => "default").first.nil?
+    elsif current_user.preferences.find_by(:k => "gps.trace.public", :v => "default").nil?
       "private"
     else
       "public"

test/controllers/api/traces_controller_test.rb

@@ -194,7 +194,7 @@ def test_create
 
       # Now authenticated
       create(:user_preference, :user => user, :k => "gps.trace.visibility", :v => "identifiable")
-      assert_not_equal "trackable", user.preferences.where(:k => "gps.trace.visibility").first.v
+      assert_not_equal "trackable", user.preferences.find_by(:k => "gps.trace.visibility").v
       auth_header = basic_authorization_header user.display_name, "test"
       post gpx_create_path, :params => { :file => file, :description => "New Trace", :tags => "new,trace", :visibility => "trackable" }, :headers => auth_header
       assert_response :success
@@ -206,13 +206,13 @@ def test_create
       assert_not trace.inserted
       assert_equal File.new(fixture).read, trace.file.blob.download
       trace.destroy
-      assert_equal "trackable", user.preferences.where(:k => "gps.trace.visibility").first.v
+      assert_equal "trackable", user.preferences.find_by(:k => "gps.trace.visibility").v
 
       # Rewind the file
       file.rewind
 
       # Now authenticated, with the legacy public flag
-      assert_not_equal "public", user.preferences.where(:k => "gps.trace.visibility").first.v
+      assert_not_equal "public", user.preferences.find_by(:k => "gps.trace.visibility").v
       auth_header = basic_authorization_header user.display_name, "test"
       post gpx_create_path, :params => { :file => file, :description => "New Trace", :tags => "new,trace", :public => 1 }, :headers => auth_header
       assert_response :success
@@ -224,14 +224,14 @@ def test_create
       assert_not trace.inserted
       assert_equal File.new(fixture).read, trace.file.blob.download
       trace.destroy
-      assert_equal "public", user.preferences.where(:k => "gps.trace.visibility").first.v
+      assert_equal "public", user.preferences.find_by(:k => "gps.trace.visibility").v
 
       # Rewind the file
       file.rewind
 
       # Now authenticated, with the legacy private flag
       second_user = create(:user)
-      assert_nil second_user.preferences.where(:k => "gps.trace.visibility").first
+      assert_nil second_user.preferences.find_by(:k => "gps.trace.visibility")
       auth_header = basic_authorization_header second_user.display_name, "test"
       post gpx_create_path, :params => { :file => file, :description => "New Trace", :tags => "new,trace", :public => 0 }, :headers => auth_header
       assert_response :success
@@ -243,7 +243,7 @@ def test_create
       assert_not trace.inserted
       assert_equal File.new(fixture).read, trace.file.blob.download
       trace.destroy
-      assert_equal "private", second_user.preferences.where(:k => "gps.trace.visibility").first.v
+      assert_equal "private", second_user.preferences.find_by(:k => "gps.trace.visibility").v
     end
 
     # Check updating a trace through the api

test/controllers/diary_entries_controller_test.rb

@@ -164,7 +164,7 @@ def test_create_no_body
     assert_response :success
     assert_template :new
 
-    assert_nil UserPreference.where(:user => user, :k => "diary.default_language").first
+    assert_nil UserPreference.find_by(:user => user, :k => "diary.default_language")
   end
 
   def test_create
@@ -189,7 +189,7 @@ def test_create
     # checks if user was subscribed
     assert_equal 1, entry.subscribers.length
 
-    assert_equal "en", UserPreference.where(:user => user, :k => "diary.default_language").first.v
+    assert_equal "en", UserPreference.find_by(:user => user, :k => "diary.default_language").v
   end
 
   def test_create_german
@@ -216,7 +216,7 @@ def test_create_german
     # checks if user was subscribed
     assert_equal 1, entry.subscribers.length
 
-    assert_equal "de", UserPreference.where(:user => user, :k => "diary.default_language").first.v
+    assert_equal "de", UserPreference.find_by(:user => user, :k => "diary.default_language").v
   end
 
   def test_new_spammy

test/controllers/friendships_controller_test.rb

@@ -28,7 +28,7 @@ def test_make_friend
     friend = create(:user)
 
     # Check that the users aren't already friends
-    assert_nil Friendship.where(:befriender => user, :befriendee => friend).first
+    assert_nil Friendship.find_by(:befriender => user, :befriendee => friend)
 
     # When not logged in a GET should ask us to login
     get make_friend_path(friend)
@@ -37,7 +37,7 @@ def test_make_friend
     # When not logged in a POST should error
     post make_friend_path(friend)
     assert_response :forbidden
-    assert_nil Friendship.where(:befriender => user, :befriendee => friend).first
+    assert_nil Friendship.find_by(:befriender => user, :befriendee => friend)
 
     session_for(user)
 
@@ -49,7 +49,7 @@ def test_make_friend
       assert_select "input[type='hidden'][name='referer']", 0
       assert_select "input[type='submit']", 1
     end
-    assert_nil Friendship.where(:befriender => user, :befriendee => friend).first
+    assert_nil Friendship.find_by(:befriender => user, :befriendee => friend)
 
     # When logged in a POST should add the friendship
     assert_difference "ActionMailer::Base.deliveries.size", 1 do
@@ -59,7 +59,7 @@ def test_make_friend
     end
     assert_redirected_to user_path(friend)
     assert_match(/is now your friend/, flash[:notice])
-    assert Friendship.where(:befriender => user, :befriendee => friend).first
+    assert Friendship.find_by(:befriender => user, :befriendee => friend)
     email = ActionMailer::Base.deliveries.first
     assert_equal 1, email.to.count
     assert_equal friend.email, email.to.first
@@ -73,7 +73,7 @@ def test_make_friend
     end
     assert_redirected_to user_path(friend)
     assert_match(/You are already friends with/, flash[:warning])
-    assert Friendship.where(:befriender => user, :befriendee => friend).first
+    assert Friendship.find_by(:befriender => user, :befriendee => friend)
   end
 
   def test_make_friend_with_referer
@@ -83,7 +83,7 @@ def test_make_friend_with_referer
     session_for(user)
 
     # Check that the users aren't already friends
-    assert_nil Friendship.where(:befriender => user, :befriendee => friend).first
+    assert_nil Friendship.find_by(:befriender => user, :befriendee => friend)
 
     # The GET should preserve any referer
     get make_friend_path(friend), :params => { :referer => "/test" }
@@ -93,7 +93,7 @@ def test_make_friend_with_referer
       assert_select "input[type='hidden'][name='referer'][value='/test']", 1
       assert_select "input[type='submit']", 1
     end
-    assert_nil Friendship.where(:befriender => user, :befriendee => friend).first
+    assert_nil Friendship.find_by(:befriender => user, :befriendee => friend)
 
     # When logged in a POST should add the friendship and refer us
     assert_difference "ActionMailer::Base.deliveries.size", 1 do
@@ -103,7 +103,7 @@ def test_make_friend_with_referer
     end
     assert_redirected_to "/test"
     assert_match(/is now your friend/, flash[:notice])
-    assert Friendship.where(:befriender => user, :befriendee => friend).first
+    assert Friendship.find_by(:befriender => user, :befriendee => friend)
     email = ActionMailer::Base.deliveries.first
     assert_equal 1, email.to.count
     assert_equal friend.email, email.to.first
@@ -125,7 +125,7 @@ def test_remove_friend
     create(:friendship, :befriender => user, :befriendee => friend)
 
     # Check that the users are friends
-    assert Friendship.where(:befriender => user, :befriendee => friend).first
+    assert Friendship.find_by(:befriender => user, :befriendee => friend)
 
     # When not logged in a GET should ask us to login
     get remove_friend_path(friend)
@@ -134,7 +134,7 @@ def test_remove_friend
     # When not logged in a POST should error
     post remove_friend_path, :params => { :display_name => friend.display_name }
     assert_response :forbidden
-    assert Friendship.where(:befriender => user, :befriendee => friend).first
+    assert Friendship.find_by(:befriender => user, :befriendee => friend)
 
     session_for(user)
 
@@ -146,19 +146,19 @@ def test_remove_friend
       assert_select "input[type='hidden'][name='referer']", 0
       assert_select "input[type='submit']", 1
     end
-    assert Friendship.where(:befriender => user, :befriendee => friend).first
+    assert Friendship.find_by(:befriender => user, :befriendee => friend)
 
     # When logged in a POST should remove the friendship
     post remove_friend_path(friend)
     assert_redirected_to user_path(friend)
     assert_match(/was removed from your friends/, flash[:notice])
-    assert_nil Friendship.where(:befriender => user, :befriendee => friend).first
+    assert_nil Friendship.find_by(:befriender => user, :befriendee => friend)
 
     # A second POST should report that the friendship does not exist
     post remove_friend_path(friend)
     assert_redirected_to user_path(friend)
     assert_match(/is not one of your friends/, flash[:error])
-    assert_nil Friendship.where(:befriender => user, :befriendee => friend).first
+    assert_nil Friendship.find_by(:befriender => user, :befriendee => friend)
   end
 
   def test_remove_friend_with_referer
@@ -169,7 +169,7 @@ def test_remove_friend_with_referer
     session_for(user)
 
     # Check that the users are friends
-    assert Friendship.where(:befriender => user, :befriendee => friend).first
+    assert Friendship.find_by(:befriender => user, :befriendee => friend)
 
     # The GET should preserve any referer
     get remove_friend_path(friend), :params => { :referer => "/test" }
@@ -179,13 +179,13 @@ def test_remove_friend_with_referer
       assert_select "input[type='hidden'][name='referer'][value='/test']", 1
       assert_select "input[type='submit']", 1
     end
-    assert Friendship.where(:befriender => user, :befriendee => friend).first
+    assert Friendship.find_by(:befriender => user, :befriendee => friend)
 
     # When logged in a POST should remove the friendship and refer
     post remove_friend_path(friend), :params => { :referer => "/test" }
     assert_redirected_to "/test"
     assert_match(/was removed from your friends/, flash[:notice])
-    assert_nil Friendship.where(:befriender => user, :befriendee => friend).first
+    assert_nil Friendship.find_by(:befriender => user, :befriendee => friend)
   end
 
   def test_remove_friend_unknown_user

test/controllers/traces_controller_test.rb

@@ -658,7 +658,7 @@ def test_create_post
 
     # Now authenticated
     create(:user_preference, :user => user, :k => "gps.trace.visibility", :v => "identifiable")
-    assert_not_equal "trackable", user.preferences.where(:k => "gps.trace.visibility").first.v
+    assert_not_equal "trackable", user.preferences.find_by(:k => "gps.trace.visibility").v
     session_for(user)
     post traces_path, :params => { :trace => { :gpx_file => file, :description => "New Trace", :tagstring => "new,trace", :visibility => "trackable" } }
     assert_response :redirect
@@ -672,7 +672,7 @@ def test_create_post
     assert_not trace.inserted
     assert_equal File.new(fixture).read, trace.file.blob.download
     trace.destroy
-    assert_equal "trackable", user.preferences.where(:k => "gps.trace.visibility").first.v
+    assert_equal "trackable", user.preferences.find_by(:k => "gps.trace.visibility").v
   end
 
   # Test creating a trace with validation errors
@@ -684,7 +684,7 @@ def test_create_post_with_validation_errors
 
     # Now authenticated
     create(:user_preference, :user => user, :k => "gps.trace.visibility", :v => "identifiable")
-    assert_not_equal "trackable", user.preferences.where(:k => "gps.trace.visibility").first.v
+    assert_not_equal "trackable", user.preferences.find_by(:k => "gps.trace.visibility").v
     session_for(user)
     post traces_path, :params => { :trace => { :gpx_file => file, :description => "", :tagstring => "new,trace", :visibility => "trackable" } }
     assert_template :new

test/models/node_test.rb

@@ -87,7 +87,7 @@ def test_create
     assert_equal node_template.timestamp.to_i, node.timestamp.to_i
 
     assert_equal(1, OldNode.where(:node_id => node_template.id).count)
-    old_node = OldNode.where(:node_id => node_template.id).first
+    old_node = OldNode.find_by(:node_id => node_template.id, :version => 1)
     assert_not_nil old_node
     assert_equal node_template.latitude, old_node.latitude
     assert_equal node_template.longitude, old_node.longitude
@@ -120,7 +120,7 @@ def test_update
     # assert_equal node_template.tags, node.tags
 
     assert_equal(2, OldNode.where(:node_id => node_template.id).count)
-    old_node = OldNode.where(:node_id => node_template.id, :version => 2).first
+    old_node = OldNode.find_by(:node_id => node_template.id, :version => 2)
     assert_not_nil old_node
     assert_equal node_template.latitude, old_node.latitude
     assert_equal node_template.longitude, old_node.longitude
@@ -149,7 +149,7 @@ def test_delete
     # assert_equal node_template.tags, node.tags
 
     assert_equal(2, OldNode.where(:node_id => node_template.id).count)
-    old_node = OldNode.where(:node_id => node_template.id, :version => 2).first
+    old_node = OldNode.find_by(:node_id => node_template.id, :version => 2)
     assert_not_nil old_node
     assert_equal node_template.latitude, old_node.latitude
     assert_equal node_template.longitude, old_node.longitude

test/models/trace_test.rb

@@ -202,7 +202,7 @@ def test_import_creates_tracepoints
 
     # Check that the tile has been set prior to the bulk import
     # i.e. that the callbacks have been run correctly
-    assert_equal 3221331576, Tracepoint.where(:trace => trace).first.tile
+    assert_equal 3221331576, Tracepoint.find_by(:trace => trace).tile
   end
 
   def test_import_creates_icon