From e17b89e89fa932673f7dc2cf5a2fd3437ec0eda6 Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Thu, 20 Oct 2016 22:00:56 +0100 Subject: [PATCH] Fix rubocop warnings --- .rubocop.yml | 3 ++ .rubocop_todo.yml | 47 ++++++++++++++----- app/controllers/application_controller.rb | 14 +++--- app/controllers/changeset_controller.rb | 20 ++++---- app/controllers/diary_entry_controller.rb | 4 +- app/controllers/notes_controller.rb | 6 +-- app/controllers/oauth_controller.rb | 2 +- app/controllers/swf_controller.rb | 4 +- app/controllers/user_controller.rb | 40 ++++++++-------- app/helpers/trace_helper.rb | 4 +- app/models/client_application.rb | 2 +- app/models/user.rb | 4 +- lib/potlatch.rb | 19 ++++---- .../oauth_clients_controller_test.rb | 2 +- .../controllers/redactions_controller_test.rb | 2 +- test/integration/oauth_test.rb | 10 ++-- test/integration/user_login_test.rb | 2 +- 17 files changed, 105 insertions(+), 80 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 7b8ced040..975457d95 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -37,3 +37,6 @@ Style/HashSyntax: Style/StringLiterals: EnforcedStyle: double_quotes + +Rails/HttpPositionalArguments: + Enabled: false diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 984529135..95ddea8f8 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2016-09-07 11:03:27 +0100 using RuboCop version 0.41.2. +# on 2016-10-20 21:45:27 +0100 using RuboCop version 0.44.1. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -48,9 +48,14 @@ Lint/ShadowingOuterLocalVariable: Exclude: - 'app/views/changeset/list.atom.builder' -# Offense count: 626 +# Offense count: 630 Metrics/AbcSize: - Max: 277 + Max: 271 + +# Offense count: 35 +# Configuration parameters: CountComments. +Metrics/BlockLength: + Max: 295 # Offense count: 12 Metrics/BlockNesting: @@ -61,17 +66,17 @@ Metrics/BlockNesting: Metrics/ClassLength: Max: 1652 -# Offense count: 68 +# Offense count: 69 Metrics/CyclomaticComplexity: Max: 20 -# Offense count: 2561 -# Configuration parameters: AllowHeredoc, AllowURI, URISchemes. +# Offense count: 2826 +# Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives. # URISchemes: http, https Metrics/LineLength: Max: 962 -# Offense count: 604 +# Offense count: 612 # Configuration parameters: CountComments. Metrics/MethodLength: Max: 179 @@ -79,14 +84,14 @@ Metrics/MethodLength: # Offense count: 1 # Configuration parameters: CountComments. Metrics/ModuleLength: - Max: 139 + Max: 147 # Offense count: 4 # Configuration parameters: CountKeywordArgs. Metrics/ParameterLists: Max: 9 -# Offense count: 70 +# Offense count: 71 Metrics/PerceivedComplexity: Max: 23 @@ -106,6 +111,17 @@ Rails/HasAndBelongsToMany: - 'app/models/changeset.rb' - 'app/models/user.rb' +# Offense count: 5 +# Configuration parameters: Include. +# Include: db/migrate/*.rb +Rails/NotNullColumn: + Exclude: + - 'db/migrate/002_cleanup_osm_db.rb' + - 'db/migrate/020_populate_node_tags_and_remove.rb' + - 'db/migrate/021_move_to_innodb.rb' + - 'db/migrate/025_add_end_time_to_changesets.rb' + - 'db/migrate/20120404205604_add_user_and_description_to_redaction.rb' + # Offense count: 17 Rails/OutputSafety: Exclude: @@ -120,7 +136,7 @@ Rails/OutputSafety: - 'lib/rich_text.rb' - 'test/helpers/application_helper_test.rb' -# Offense count: 65 +# Offense count: 74 # Configuration parameters: EnforcedStyle, SupportedStyles. # SupportedStyles: strict, flexible Rails/TimeZone: @@ -140,10 +156,17 @@ Style/AsciiComments: Exclude: - 'test/models/message_test.rb' -# Offense count: 218 +# Offense count: 220 Style/Documentation: Enabled: false +# Offense count: 1 +# Cop supports --auto-correct. +# Configuration parameters: MaxLineLength. +Style/IfUnlessModifier: + Exclude: + - 'app/controllers/way_controller.rb' + # Offense count: 60 # Cop supports --auto-correct. Style/LineEndConcatenation: @@ -159,7 +182,7 @@ Style/LineEndConcatenation: - 'test/controllers/relation_controller_test.rb' - 'test/controllers/way_controller_test.rb' -# Offense count: 69 +# Offense count: 71 # Cop supports --auto-correct. Style/NumericLiterals: MinDigits: 11 diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f3b77f810..0c50276b6 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -62,7 +62,7 @@ class ApplicationController < ActionController::Base unless current_token.nil? unless current_token.read_attribute(cap) report_error "OAuth token doesn't have that capability.", :forbidden - return false + false end end end @@ -74,7 +74,7 @@ class ApplicationController < ActionController::Base if params[:cookie_test].nil? session[:cookie_test] = true redirect_to Hash[params].merge(:cookie_test => "true") - return false + false else flash.now[:warning] = t "application.require_cookies.cookies_needed" end @@ -192,7 +192,7 @@ class ApplicationController < ActionController::Base # check user is a moderator unless @user.moderator? render :text => errormessage, :status => :forbidden - return false + false end end @@ -220,14 +220,14 @@ class ApplicationController < ActionController::Base def check_api_readable if api_status == :offline report_error "Database offline for maintenance", :service_unavailable - return false + false end end def check_api_writable unless api_status == :online report_error "Database offline for maintenance", :service_unavailable - return false + false end end @@ -262,7 +262,7 @@ class ApplicationController < ActionController::Base def require_public_data unless @user.data_public? report_error "You must make your edits public to upload new data", :forbidden - return false + false end end @@ -375,7 +375,7 @@ class ApplicationController < ActionController::Base ## # ensure that there is a "this_user" instance variable def lookup_this_user - unless @this_user = User.active.find_by_display_name(params[:display_name]) + unless @this_user = User.active.find_by(:display_name => params[:display_name]) render_unknown_user params[:display_name] end end diff --git a/app/controllers/changeset_controller.rb b/app/controllers/changeset_controller.rb index 89ba5d131..09bad34bc 100644 --- a/app/controllers/changeset_controller.rb +++ b/app/controllers/changeset_controller.rb @@ -256,7 +256,7 @@ class ChangesetController < ApplicationController end if params[:display_name] - user = User.find_by_display_name(params[:display_name]) + user = User.find_by(:display_name => params[:display_name]) if !user || !user.active? render_unknown_user params[:display_name] return @@ -481,7 +481,7 @@ class ChangesetController < ApplicationController raise OSM::APIBadUserInput.new("invalid user ID") if user.to_i < 1 u = User.find(user.to_i) else - u = User.find_by_display_name(name) + u = User.find_by(:display_name => name) end # make sure we found a user @@ -535,10 +535,10 @@ class ChangesetController < ApplicationController # if parameter 'open' is nill then open and closed changesets are returned def conditions_open(changesets, open) if open.nil? - return changesets + changesets else - return changesets.where("closed_at >= ? and num_changes <= ?", - Time.now.getutc, Changeset::MAX_ELEMENTS) + changesets.where("closed_at >= ? and num_changes <= ?", + Time.now.getutc, Changeset::MAX_ELEMENTS) end end @@ -547,10 +547,10 @@ class ChangesetController < ApplicationController # ('closed at' time has passed or changes limit is hit) def conditions_closed(changesets, closed) if closed.nil? - return changesets + changesets else - return changesets.where("closed_at < ? or num_changes > ?", - Time.now.getutc, Changeset::MAX_ELEMENTS) + changesets.where("closed_at < ? or num_changes > ?", + Time.now.getutc, Changeset::MAX_ELEMENTS) end end @@ -559,12 +559,12 @@ class ChangesetController < ApplicationController # (either specified as array or comma-separated string) def conditions_ids(changesets, ids) if ids.nil? - return changesets + changesets elsif ids.empty? raise OSM::APIBadUserInput.new("No changesets were given to search for") else ids = ids.split(",").collect(&:to_i) - return changesets.where(:id => ids) + changesets.where(:id => ids) end end diff --git a/app/controllers/diary_entry_controller.rb b/app/controllers/diary_entry_controller.rb index bf9f2a9a2..61d95ba11 100644 --- a/app/controllers/diary_entry_controller.rb +++ b/app/controllers/diary_entry_controller.rb @@ -102,7 +102,7 @@ class DiaryEntryController < ApplicationController def list if params[:display_name] - @this_user = User.active.find_by_display_name(params[:display_name]) + @this_user = User.active.find_by(:display_name => params[:display_name]) if @this_user @title = t "diary_entry.list.user_title", :user => @this_user.display_name @@ -150,7 +150,7 @@ class DiaryEntryController < ApplicationController def rss if params[:display_name] - user = User.active.find_by_display_name(params[:display_name]) + user = User.active.find_by(:display_name => params[:display_name]) if user @entries = user.diary_entries diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index 1366038f0..fde27e8b2 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -127,7 +127,7 @@ class NotesController < ApplicationController comment = params[:text] # Find the note and check it is valid - @note = Note.find_by_id(id) + @note = Note.find_by(:id => id) raise OSM::APINotFoundError unless @note raise OSM::APIAlreadyDeletedError.new("note", @note.id) unless @note.visible? raise OSM::APINoteAlreadyClosedError.new(@note) if @note.closed? @@ -157,7 +157,7 @@ class NotesController < ApplicationController comment = params[:text] # Find the note and check it is valid - @note = Note.find_by_id(id) + @note = Note.find_by(:id => id) raise OSM::APINotFoundError unless @note raise OSM::APIAlreadyDeletedError.new("note", @note.id) unless @note.visible? || @user.moderator? raise OSM::APINoteAlreadyOpenError.new(@note) unless @note.closed? || !@note.visible? @@ -277,7 +277,7 @@ class NotesController < ApplicationController # Display a list of notes by a specified user def mine if params[:display_name] - if @this_user = User.active.find_by_display_name(params[:display_name]) + if @this_user = User.active.find_by(:display_name => params[:display_name]) @title = t "note.mine.title", :user => @this_user.display_name @heading = t "note.mine.heading", :user => @this_user.display_name @description = t "note.mine.subheading", :user => render_to_string(:partial => "user", :object => @this_user) diff --git a/app/controllers/oauth_controller.rb b/app/controllers/oauth_controller.rb index 59ebfd631..2e847fcd5 100644 --- a/app/controllers/oauth_controller.rb +++ b/app/controllers/oauth_controller.rb @@ -27,7 +27,7 @@ class OauthController < ApplicationController end def revoke - @token = current_user.oauth_tokens.find_by_token params[:token] + @token = current_user.oauth_tokens.find_by :token => params[:token] if @token @token.invalidate! flash[:notice] = t("oauth.revoke.flash", :application => @token.client_application.name) diff --git a/app/controllers/swf_controller.rb b/app/controllers/swf_controller.rb index 9033a0733..282f61613 100644 --- a/app/controllers/swf_controller.rb +++ b/app/controllers/swf_controller.rb @@ -161,10 +161,10 @@ class SwfController < ApplicationController def swf_record(id, r) if r.length > 62 # Long header: tag id, 0x3F, length - return pack_u16((id << 6) + 0x3F) + pack_u32(r.length) + r + pack_u16((id << 6) + 0x3F) + pack_u32(r.length) + r else # Short header: tag id, length - return pack_u16((id << 6) + r.length) + r + pack_u16((id << 6) + r.length) + r end end diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index e49b01ebe..197b28914 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -149,7 +149,7 @@ class UserController < ApplicationController @title = t "user.lost_password.title" if params[:user] && params[:user][:email] - user = User.visible.find_by_email(params[:user][:email]) + user = User.visible.find_by(:email => params[:user][:email]) if user.nil? users = User.visible.where("LOWER(email) = LOWER(?)", params[:user][:email]) @@ -172,7 +172,7 @@ class UserController < ApplicationController @title = t "user.reset_password.title" if params[:token] - token = UserToken.find_by_token(params[:token]) + token = UserToken.find_by(:token => params[:token]) if token @user = token.user @@ -270,7 +270,7 @@ class UserController < ApplicationController if params[:session] == request.session_options[:id] if session[:token] - token = UserToken.find_by_token(session[:token]) + token = UserToken.find_by(:token => session[:token]) token.destroy if token session.delete(:token) end @@ -286,7 +286,7 @@ class UserController < ApplicationController def confirm if request.post? - token = UserToken.find_by_token(params[:confirm_string]) + token = UserToken.find_by(:token => params[:confirm_string]) if token && token.user.active? flash[:error] = t("user.confirm.already active") redirect_to :action => "login" @@ -303,7 +303,7 @@ class UserController < ApplicationController token.destroy if session[:token] - token = UserToken.find_by_token(session[:token]) + token = UserToken.find_by(:token => session[:token]) session.delete(:token) else token = nil @@ -321,15 +321,15 @@ class UserController < ApplicationController end end else - user = User.find_by_display_name(params[:display_name]) + user = User.find_by(:display_name => params[:display_name]) redirect_to root_path if user.nil? || user.active? end end def confirm_resend - user = User.find_by_display_name(params[:display_name]) - token = UserToken.find_by_token(session[:token]) + user = User.find_by(:display_name => params[:display_name]) + token = UserToken.find_by(:token => session[:token]) if user.nil? || token.nil? || token.user != user flash[:error] = t "user.confirm_resend.failure", :name => params[:display_name] @@ -343,7 +343,7 @@ class UserController < ApplicationController def confirm_email if request.post? - token = UserToken.find_by_token(params[:confirm_string]) + token = UserToken.find_by(:token => params[:confirm_string]) if token && token.user.new_email? @user = token.user @user.email = @user.new_email @@ -393,7 +393,7 @@ class UserController < ApplicationController end def view - @this_user = User.find_by_display_name(params[:display_name]) + @this_user = User.find_by(:display_name => params[:display_name]) if @this_user && (@this_user.visible? || (@user && @user.administrator?)) @@ -404,7 +404,7 @@ class UserController < ApplicationController end def make_friend - @new_friend = User.find_by_display_name(params[:display_name]) + @new_friend = User.find_by(:display_name => params[:display_name]) if @new_friend if request.post? @@ -432,7 +432,7 @@ class UserController < ApplicationController end def remove_friend - @friend = User.find_by_display_name(params[:display_name]) + @friend = User.find_by(:display_name => params[:display_name]) if @friend if request.post? @@ -530,11 +530,11 @@ class UserController < ApplicationController redirect_to :action => "terms" else - user = User.find_by_auth_provider_and_auth_uid(provider, uid) + user = User.find_by(:auth_provider => provider, :auth_uid => uid) if user.nil? && provider == "google" openid_url = auth_info[:extra][:id_info]["openid_id"] - user = User.find_by_auth_provider_and_auth_uid("openid", openid_url) if openid_url + user = User.find_by(:auth_provider => "openid", :auth_uid => openid_url) if openid_url user.update(:auth_provider => provider, :auth_uid => uid) if user end @@ -601,15 +601,15 @@ class UserController < ApplicationController # try and come up with the correct URL based on what the user entered def openid_expand_url(openid_url) if openid_url.nil? - return nil + nil elsif openid_url.match(%r{(.*)gmail.com(/?)$}) || openid_url.match(%r{(.*)googlemail.com(/?)$}) # Special case gmail.com as it is potentially a popular OpenID # provider and, unlike yahoo.com, where it works automatically, Google # have hidden their OpenID endpoint somewhere obscure this making it # somewhat less user friendly. - return "https://www.google.com/accounts/o8/id" + "https://www.google.com/accounts/o8/id" else - return openid_url + openid_url end end @@ -766,7 +766,7 @@ class UserController < ApplicationController ## # ensure that there is a "this_user" instance variable def lookup_user_by_name - @this_user = User.find_by_display_name(params[:display_name]) + @this_user = User.find_by(:display_name => params[:display_name]) rescue ActiveRecord::RecordNotFound redirect_to :action => "view", :display_name => params[:display_name] unless @this_user end @@ -823,9 +823,9 @@ class UserController < ApplicationController # display a message about th current status of the gravatar setting def gravatar_status_message(user) if user.image_use_gravatar - return t "user.account.gravatar.enabled" + t "user.account.gravatar.enabled" else - return t "user.account.gravatar.disabled" + t "user.account.gravatar.disabled" end end end diff --git a/app/helpers/trace_helper.rb b/app/helpers/trace_helper.rb index 3922ce2b3..15bc32313 100644 --- a/app/helpers/trace_helper.rb +++ b/app/helpers/trace_helper.rb @@ -1,9 +1,9 @@ module TraceHelper def link_to_tag(tag) if @action == "mine" - return link_to(tag, :tag => tag, :page => nil) + link_to(tag, :tag => tag, :page => nil) else - return link_to(tag, :tag => tag, :display_name => @display_name, :page => nil) + link_to(tag, :tag => tag, :display_name => @display_name, :page => nil) end end end diff --git a/app/models/client_application.rb b/app/models/client_application.rb index 156eeafc7..152b3912d 100644 --- a/app/models/client_application.rb +++ b/app/models/client_application.rb @@ -18,7 +18,7 @@ class ClientApplication < ActiveRecord::Base attr_accessor :token_callback_url def self.find_token(token_key) - token = OauthToken.find_by_token(token_key, :include => :client_application) + token = OauthToken.includes(:client_application).find_by(:token => token_key) token if token && token.authorized? end diff --git a/app/models/user.rb b/app/models/user.rb index 3ff9277f0..2cdb94046 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -83,7 +83,7 @@ class User < ActiveRecord::Base user = nil end elsif options[:token] - token = UserToken.find_by_token(options[:token]) + token = UserToken.find_by(:token => options[:token]) user = token.user if token end @@ -240,7 +240,7 @@ class User < ActiveRecord::Base ## # return an oauth access token for a specified application def access_token(application_key) - ClientApplication.find_by_key(application_key).access_token_for_user(self) + ClientApplication.find_by(:key => application_key).access_token_for_user(self) end private diff --git a/lib/potlatch.rb b/lib/potlatch.rb index 4b56414a4..165e37b0c 100644 --- a/lib/potlatch.rb +++ b/lib/potlatch.rb @@ -47,16 +47,15 @@ module Potlatch # Parse and get value def self.getvalue(s) case s.getbyte - when 0 then return getdouble(s) # number - when 1 then return s.getbyte # boolean - when 2 then return getstring(s) # string - when 3 then return getobject(s) # object/hash - when 5 then return nil # null - when 6 then return nil # undefined - when 8 then s.read(4) # mixedArray - return getobject(s) # | - when 10 then return getarray(s) # array - else return nil # error + when 0 then getdouble(s) # number + when 1 then s.getbyte # boolean + when 2 then getstring(s) # string + when 3 then getobject(s) # object/hash + when 5 then nil # null + when 6 then nil # undefined + when 8 then s.read(4) # mixedArray + getobject(s) # | + when 10 then getarray(s) # array end end diff --git a/test/controllers/oauth_clients_controller_test.rb b/test/controllers/oauth_clients_controller_test.rb index 6bafa62cc..a1c6fbc21 100644 --- a/test/controllers/oauth_clients_controller_test.rb +++ b/test/controllers/oauth_clients_controller_test.rb @@ -99,7 +99,7 @@ class OauthClientsControllerTest < ActionController::TestCase }, { :user => user } end assert_response :redirect - assert_redirected_to oauth_client_path(:id => ClientApplication.find_by_name("Test Application").id) + assert_redirected_to oauth_client_path(:id => ClientApplication.find_by(:name => "Test Application").id) end def test_show diff --git a/test/controllers/redactions_controller_test.rb b/test/controllers/redactions_controller_test.rb index 1df2a6dff..3d71012b3 100644 --- a/test/controllers/redactions_controller_test.rb +++ b/test/controllers/redactions_controller_test.rb @@ -73,7 +73,7 @@ class RedactionsControllerTest < ActionController::TestCase post :create, :redaction => { :title => "Foo", :description => "Description here." } assert_response :redirect - assert_redirected_to(redaction_path(Redaction.find_by_title("Foo"))) + assert_redirected_to(redaction_path(Redaction.find_by(:title => "Foo"))) end def test_create_moderator_invalid diff --git a/test/integration/oauth_test.rb b/test/integration/oauth_test.rb index 323147030..00a9ae06d 100644 --- a/test/integration/oauth_test.rb +++ b/test/integration/oauth_test.rb @@ -94,7 +94,7 @@ class OAuthTest < ActionDispatch::IntegrationTest post "/oauth/revoke", :token => token.token assert_redirected_to oauth_clients_url(token.user.display_name) - token = OauthToken.find_by_token(token.token) + token = OauthToken.find_by(:token => token.token) assert_not_nil token.invalidated_at signed_get "/api/0.6/user/preferences", :consumer => client, :token => token @@ -172,7 +172,7 @@ class OAuthTest < ActionDispatch::IntegrationTest post "/oauth/revoke", :token => token.token assert_redirected_to oauth_clients_url(token.user.display_name) - token = OauthToken.find_by_token(token.token) + token = OauthToken.find_by(:token => token.token) assert_not_nil token.invalidated_at signed_get "/api/0.6/gpx/2", :consumer => client, :token => token @@ -231,7 +231,7 @@ class OAuthTest < ActionDispatch::IntegrationTest post "/oauth/revoke", :token => token.token assert_redirected_to oauth_clients_url(token.user.display_name) - token = OauthToken.find_by_token(token.token) + token = OauthToken.find_by(:token => token.token) assert_not_nil token.invalidated_at signed_get "/api/0.6/user/preferences", :consumer => client, :token => token @@ -282,7 +282,7 @@ class OAuthTest < ActionDispatch::IntegrationTest post "/oauth/revoke", :token => token.token assert_redirected_to oauth_clients_url(token.user.display_name) - token = OauthToken.find_by_token(token.token) + token = OauthToken.find_by(:token => token.token) assert_not_nil token.invalidated_at signed_get "/api/0.6/gpx/2", :consumer => client, :token => token @@ -356,7 +356,7 @@ class OAuthTest < ActionDispatch::IntegrationTest def parse_token(response) params = CGI.parse(response.body) - token = OauthToken.find_by_token(params["oauth_token"].first) + token = OauthToken.find_by(:token => params["oauth_token"].first) assert_equal token.secret, params["oauth_token_secret"].first token diff --git a/test/integration/user_login_test.rb b/test/integration/user_login_test.rb index 5652955c7..86b9390d8 100644 --- a/test/integration/user_login_test.rb +++ b/test/integration/user_login_test.rb @@ -555,7 +555,7 @@ class UserLoginTest < ActionDispatch::IntegrationTest assert_template "changeset/history" assert_select "span.username", "openIDuser" - user = User.find_by_display_name("openIDuser") + user = User.find_by(:display_name => "openIDuser") assert_equal "google", user.auth_provider assert_equal "987654321", user.auth_uid end -- 2.43.2