From: Andy Allan <git@gravitystorm.co.uk>
Date: Wed, 4 Oct 2023 16:53:58 +0000 (+0100)
Subject: Prefer find_by() instead of where().first
X-Git-Tag: live~1034^2
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/1700c23dd1018b66a268b4c76fcf9fb2a09a35ab

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.
---

diff --git a/.rubocop.yml b/.rubocop.yml
index dc2a33a35..1e18afd83 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -48,6 +48,9 @@ Naming/MethodParameterName:
 Rails/CreateTableWithTimestamps:
   Enabled: false
 
+Rails/FindBy:
+  IgnoreWhereFirst: false
+
 Rails/FindEach:
   Enabled: false
 
diff --git a/app/controllers/api/traces_controller.rb b/app/controllers/api/traces_controller.rb
index 629617f0b..b66aead38 100644
--- a/app/controllers/api/traces_controller.rb
+++ b/app/controllers/api/traces_controller.rb
@@ -115,7 +115,7 @@ module Api
       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
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index a30816a8e..c830d4bcd 100644
--- a/app/controllers/application_controller.rb
+++ b/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
diff --git a/app/controllers/diary_entries_controller.rb b/app/controllers/diary_entries_controller.rb
index a1cd6ab0e..0b2fcb73e 100644
--- a/app/controllers/diary_entries_controller.rb
+++ b/app/controllers/diary_entries_controller.rb
@@ -61,7 +61,7 @@ class DiaryEntriesController < ApplicationController
   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 @@ class DiaryEntriesController < ApplicationController
   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 @@ class DiaryEntriesController < ApplicationController
     @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!
diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb
index 90ab34a48..242f8113c 100644
--- a/app/controllers/traces_controller.rb
+++ b/app/controllers/traces_controller.rb
@@ -19,7 +19,7 @@ class TracesController < ApplicationController
     # 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 @@ class TracesController < ApplicationController
     # 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 @@ class TracesController < ApplicationController
   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"
diff --git a/test/controllers/api/traces_controller_test.rb b/test/controllers/api/traces_controller_test.rb
index 468af852b..b26782a3f 100644
--- a/test/controllers/api/traces_controller_test.rb
+++ b/test/controllers/api/traces_controller_test.rb
@@ -194,7 +194,7 @@ module Api
 
       # 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 @@ module Api
       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 @@ module Api
       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 @@ module Api
       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
diff --git a/test/controllers/diary_entries_controller_test.rb b/test/controllers/diary_entries_controller_test.rb
index b505d9cdb..0c1e65551 100644
--- a/test/controllers/diary_entries_controller_test.rb
+++ b/test/controllers/diary_entries_controller_test.rb
@@ -164,7 +164,7 @@ class DiaryEntriesControllerTest < ActionDispatch::IntegrationTest
     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 @@ class DiaryEntriesControllerTest < ActionDispatch::IntegrationTest
     # 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 @@ class DiaryEntriesControllerTest < ActionDispatch::IntegrationTest
     # 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
diff --git a/test/controllers/friendships_controller_test.rb b/test/controllers/friendships_controller_test.rb
index 6273caaf2..19de4ae8e 100644
--- a/test/controllers/friendships_controller_test.rb
+++ b/test/controllers/friendships_controller_test.rb
@@ -28,7 +28,7 @@ class FriendshipsControllerTest < ActionDispatch::IntegrationTest
     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 @@ class FriendshipsControllerTest < ActionDispatch::IntegrationTest
     # 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 @@ class FriendshipsControllerTest < ActionDispatch::IntegrationTest
       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 @@ class FriendshipsControllerTest < ActionDispatch::IntegrationTest
     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 @@ class FriendshipsControllerTest < ActionDispatch::IntegrationTest
     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 @@ class FriendshipsControllerTest < ActionDispatch::IntegrationTest
     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 @@ class FriendshipsControllerTest < ActionDispatch::IntegrationTest
       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 @@ class FriendshipsControllerTest < ActionDispatch::IntegrationTest
     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 @@ class FriendshipsControllerTest < ActionDispatch::IntegrationTest
     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 @@ class FriendshipsControllerTest < ActionDispatch::IntegrationTest
     # 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 @@ class FriendshipsControllerTest < ActionDispatch::IntegrationTest
       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 @@ class FriendshipsControllerTest < ActionDispatch::IntegrationTest
     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 @@ class FriendshipsControllerTest < ActionDispatch::IntegrationTest
       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
diff --git a/test/controllers/traces_controller_test.rb b/test/controllers/traces_controller_test.rb
index fe3ecdea6..595284bb6 100644
--- a/test/controllers/traces_controller_test.rb
+++ b/test/controllers/traces_controller_test.rb
@@ -658,7 +658,7 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
 
     # 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 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
     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 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
 
     # 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
diff --git a/test/models/node_test.rb b/test/models/node_test.rb
index 2e6108234..ee0a77649 100644
--- a/test/models/node_test.rb
+++ b/test/models/node_test.rb
@@ -87,7 +87,7 @@ class NodeTest < ActiveSupport::TestCase
     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 @@ class NodeTest < ActiveSupport::TestCase
     # 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 @@ class NodeTest < ActiveSupport::TestCase
     # 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
diff --git a/test/models/trace_test.rb b/test/models/trace_test.rb
index 66107771a..af219db43 100644
--- a/test/models/trace_test.rb
+++ b/test/models/trace_test.rb
@@ -202,7 +202,7 @@ class TraceTest < ActiveSupport::TestCase
 
     # 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