From 1700c23dd1018b66a268b4c76fcf9fb2a09a35ab Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 4 Oct 2023 17:53:58 +0100 Subject: [PATCH 1/1] 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 | 3 ++ app/controllers/api/traces_controller.rb | 2 +- app/controllers/application_controller.rb | 2 +- app/controllers/diary_entries_controller.rb | 6 ++-- app/controllers/traces_controller.rb | 8 ++--- .../controllers/api/traces_controller_test.rb | 12 +++---- .../diary_entries_controller_test.rb | 6 ++-- .../friendships_controller_test.rb | 32 +++++++++---------- test/controllers/traces_controller_test.rb | 6 ++-- test/models/node_test.rb | 6 ++-- test/models/trace_test.rb | 2 +- 11 files changed, 44 insertions(+), 41 deletions(-) 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 -- 2.45.2