From 1700c23dd1018b66a268b4c76fcf9fb2a09a35ab Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 4 Oct 2023 17:53:58 +0100 Subject: [PATCH] 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 dc2a33a35b..1e18afd835 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 629617f0b1..b66aead38e 100644 --- a/app/controllers/api/traces_controller.rb +++ b/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 diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a30816a8ed..c830d4bcd3 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 a1cd6ab0ed..0b2fcb73e1 100644 --- a/app/controllers/diary_entries_controller.rb +++ b/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! diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb index 90ab34a48c..242f8113c2 100644 --- a/app/controllers/traces_controller.rb +++ b/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" diff --git a/test/controllers/api/traces_controller_test.rb b/test/controllers/api/traces_controller_test.rb index 468af852b7..b26782a3f0 100644 --- a/test/controllers/api/traces_controller_test.rb +++ b/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 diff --git a/test/controllers/diary_entries_controller_test.rb b/test/controllers/diary_entries_controller_test.rb index b505d9cdb8..0c1e655512 100644 --- a/test/controllers/diary_entries_controller_test.rb +++ b/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 diff --git a/test/controllers/friendships_controller_test.rb b/test/controllers/friendships_controller_test.rb index 6273caaf29..19de4ae8e6 100644 --- a/test/controllers/friendships_controller_test.rb +++ b/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 diff --git a/test/controllers/traces_controller_test.rb b/test/controllers/traces_controller_test.rb index fe3ecdea67..595284bb6d 100644 --- a/test/controllers/traces_controller_test.rb +++ b/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 diff --git a/test/models/node_test.rb b/test/models/node_test.rb index 2e61082343..ee0a77649e 100644 --- a/test/models/node_test.rb +++ b/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 diff --git a/test/models/trace_test.rb b/test/models/trace_test.rb index 66107771ae..af219db435 100644 --- a/test/models/trace_test.rb +++ b/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