From: Tom Hughes Date: Sat, 22 Sep 2018 16:21:06 +0000 (+0100) Subject: Fix Style/SafeNavigation rubocop warnings X-Git-Tag: live~2821 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/64146b4f3614854e6a0f8430f27261fe0a0ca26c Fix Style/SafeNavigation rubocop warnings --- diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index fbae24baa..c59c4efca 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -206,28 +206,6 @@ Style/NumericPredicate: - 'lib/gpx.rb' - 'test/controllers/amf_controller_test.rb' -# Offense count: 30 -# Cop supports --auto-correct. -# Configuration parameters: ConvertCodeThatCanStartToReturnNil, Whitelist. -# Whitelist: present?, blank?, presence, try, try! -Style/SafeNavigation: - Exclude: - - 'app/controllers/amf_controller.rb' - - 'app/controllers/application_controller.rb' - - 'app/controllers/browse_controller.rb' - - 'app/controllers/issues_controller.rb' - - 'app/controllers/notes_controller.rb' - - 'app/controllers/old_controller.rb' - - 'app/controllers/traces_controller.rb' - - 'app/controllers/user_controller.rb' - - 'app/helpers/user_roles_helper.rb' - - 'app/models/changeset.rb' - - 'app/models/client_application.rb' - - 'app/models/notifier.rb' - - 'app/models/relation.rb' - - 'app/models/way.rb' - - 'script/deliver-message' - # Offense count: 3080 # Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns. # URISchemes: http, https diff --git a/app/controllers/amf_controller.rb b/app/controllers/amf_controller.rb index 8fbd8dea9..b164eddaf 100644 --- a/app/controllers/amf_controller.rb +++ b/app/controllers/amf_controller.rb @@ -510,7 +510,7 @@ class AmfController < ApplicationController rels = [] if searchterm.to_i > 0 rel = Relation.where(:id => searchterm.to_i).first - rels.push([rel.id, rel.tags, rel.members, rel.version]) if rel && rel.visible + rels.push([rel.id, rel.tags, rel.members, rel.version]) if rel&.visible else RelationTag.where("v like ?", "%#{searchterm}%").limit(11).each do |t| rels.push([t.relation.id, t.relation.tags, t.relation.members, t.relation.version]) if t.relation.visible @@ -889,12 +889,10 @@ class AmfController < ApplicationController # in the +tags+ hash. def strip_non_xml_chars(tags) new_tags = {} - unless tags.nil? - tags.each do |k, v| - new_k = k.delete "\000-\037\ufffe\uffff", "^\011\012\015" - new_v = v.delete "\000-\037\ufffe\uffff", "^\011\012\015" - new_tags[new_k] = new_v - end + tags&.each do |k, v| + new_k = k.delete "\000-\037\ufffe\uffff", "^\011\012\015" + new_v = v.delete "\000-\037\ufffe\uffff", "^\011\012\015" + new_tags[new_k] = new_v end new_tags end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index db4ae9ad3..f35493b26 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -283,8 +283,7 @@ class ApplicationController < ActionController::Base # TODO: some sort of escaping of problem characters in the message response.headers["Error"] = message - if request.headers["X-Error-Format"] && - request.headers["X-Error-Format"].casecmp("xml").zero? + if request.headers["X-Error-Format"]&.casecmp("xml")&.zero? result = OSM::API.new.get_xml_doc result.root.name = "osmError" result.root << (XML::Node.new("status") << "#{Rack::Utils.status_code(status)} #{Rack::Utils::HTTP_STATUS_CODES[status]}") @@ -310,7 +309,7 @@ class ApplicationController < ActionController::Base helper_method :preferred_languages def set_locale(reset = false) - if current_user && current_user.languages.empty? && !http_accept_language.user_preferred_languages.empty? + if current_user&.languages&.empty? && !http_accept_language.user_preferred_languages.empty? current_user.languages = http_accept_language.user_preferred_languages current_user.save end @@ -435,7 +434,7 @@ class ApplicationController < ActionController::Base def preferred_editor editor = if params[:editor] params[:editor] - elsif current_user && current_user.preferred_editor + elsif current_user&.preferred_editor current_user.preferred_editor else DEFAULT_EDITOR diff --git a/app/controllers/browse_controller.rb b/app/controllers/browse_controller.rb index b4cb4594f..89b9c6ca3 100644 --- a/app/controllers/browse_controller.rb +++ b/app/controllers/browse_controller.rb @@ -58,7 +58,7 @@ class BrowseController < ApplicationController def changeset @type = "changeset" @changeset = Changeset.find(params[:id]) - @comments = if current_user && current_user.moderator? + @comments = if current_user&.moderator? @changeset.comments.unscope(:where => :visible).includes(:author) else @changeset.comments.includes(:author) @@ -77,7 +77,7 @@ class BrowseController < ApplicationController def note @type = "note" - if current_user && current_user.moderator? + if current_user&.moderator? @note = Note.find(params[:id]) @note_comments = @note.comments.unscope(:where => :visible) else diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb index d4e9a3bdf..ad38454f0 100644 --- a/app/controllers/issues_controller.rb +++ b/app/controllers/issues_controller.rb @@ -18,7 +18,7 @@ class IssuesController < ApplicationController @issues = Issue.visible_to(current_user) # If search - if params[:search_by_user] && params[:search_by_user].present? + if params[:search_by_user]&.present? @find_user = User.find_by(:display_name => params[:search_by_user]) if @find_user @issues = @issues.where(:reported_user_id => @find_user.id) @@ -28,11 +28,11 @@ class IssuesController < ApplicationController end end - @issues = @issues.where(:status => params[:status]) if params[:status] && params[:status].present? + @issues = @issues.where(:status => params[:status]) if params[:status]&.present? - @issues = @issues.where(:reportable_type => params[:issue_type]) if params[:issue_type] && params[:issue_type].present? + @issues = @issues.where(:reportable_type => params[:issue_type]) if params[:issue_type]&.present? - if params[:last_updated_by] && params[:last_updated_by].present? + if params[:last_updated_by]&.present? last_updated_by = params[:last_updated_by].to_s == "nil" ? nil : params[:last_updated_by].to_i @issues = @issues.where(:updated_by => last_updated_by) end diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index 94d0fdb55..0c3392d60 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -211,7 +211,7 @@ class NotesController < ApplicationController # Find the note and check it is valid @note = Note.find(params[:id]) raise OSM::APINotFoundError unless @note - raise OSM::APIAlreadyDeletedError.new("note", @note.id) unless @note.visible? || (current_user && current_user.moderator?) + raise OSM::APIAlreadyDeletedError.new("note", @note.id) unless @note.visible? || current_user&.moderator? # Render the result respond_to do |format| @@ -286,7 +286,7 @@ class NotesController < ApplicationController @page = (params[:page] || 1).to_i @page_size = 10 @notes = @user.notes - @notes = @notes.visible unless current_user && current_user.moderator? + @notes = @notes.visible unless current_user&.moderator? @notes = @notes.order("updated_at DESC, id").distinct.offset((@page - 1) * @page_size).limit(@page_size).preload(:comments => :author).to_a else @title = t "user.no_such_user.title" diff --git a/app/controllers/old_controller.rb b/app/controllers/old_controller.rb index 9adf141d9..4f01b1e2a 100644 --- a/app/controllers/old_controller.rb +++ b/app/controllers/old_controller.rb @@ -70,6 +70,6 @@ class OldController < ApplicationController private def show_redactions? - current_user && current_user.moderator? && params[:show_redactions] == "true" + current_user&.moderator? && params[:show_redactions] == "true" end end diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb index c77d884b3..b78ae2959 100644 --- a/app/controllers/traces_controller.rb +++ b/app/controllers/traces_controller.rb @@ -92,8 +92,8 @@ class TracesController < ApplicationController def show @trace = Trace.find(params[:id]) - if @trace && @trace.visible? && - (@trace.public? || @trace.user == current_user) + if @trace&.visible? && + (@trace&.public? || @trace&.user == current_user) @title = t ".title", :name => @trace.name else flash[:error] = t ".trace_not_found" @@ -318,7 +318,7 @@ class TracesController < ApplicationController visibility = params[:visibility] if visibility.nil? - visibility = if params[:public] && params[:public].to_i.nonzero? + visibility = if params[:public]&.to_i&.nonzero? "public" else "private" diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index be261e8c8..78299dccf 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -29,7 +29,7 @@ class UserController < ApplicationController else @title = t "user.terms.title" - if current_user && current_user.terms_agreed? + if current_user&.terms_agreed? # Already agreed to terms, so just show settings redirect_to :action => :account, :display_name => current_user.display_name elsif current_user.nil? && session[:new_user].nil? @@ -276,7 +276,7 @@ class UserController < ApplicationController if params[:session] == session.id if session[:token] token = UserToken.find_by(:token => session[:token]) - token.destroy if token + token&.destroy session.delete(:token) end session.delete(:user) @@ -292,7 +292,7 @@ class UserController < ApplicationController def confirm if request.post? token = UserToken.find_by(:token => params[:confirm_string]) - if token && token.user.active? + if token&.user&.active? flash[:error] = t("user.confirm.already active") redirect_to :action => "login" elsif !token || token.expired? @@ -349,7 +349,7 @@ class UserController < ApplicationController def confirm_email if request.post? token = UserToken.find_by(:token => params[:confirm_string]) - if token && token.user.new_email? + if token&.user&.new_email? self.current_user = token.user current_user.email = current_user.new_email current_user.new_email = nil @@ -413,7 +413,7 @@ class UserController < ApplicationController @user = User.find_by(:display_name => params[:display_name]) if @user && - (@user.visible? || (current_user && current_user.administrator?)) + (@user.visible? || (current_user&.administrator?)) @title = @user.display_name else render_unknown_user params[:display_name] @@ -552,7 +552,7 @@ class UserController < ApplicationController if user.nil? && provider == "google" openid_url = auth_info[:extra][:id_info]["openid_id"] user = User.find_by(:auth_provider => "openid", :auth_uid => openid_url) if openid_url - user.update(:auth_provider => provider, :auth_uid => uid) if user + user&.update(:auth_provider => provider, :auth_uid => uid) end if user diff --git a/app/helpers/user_roles_helper.rb b/app/helpers/user_roles_helper.rb index 93e090b0f..0b7e9c616 100644 --- a/app/helpers/user_roles_helper.rb +++ b/app/helpers/user_roles_helper.rb @@ -6,7 +6,7 @@ module UserRolesHelper end def role_icon(user, role) - if current_user && current_user.administrator? + if current_user&.administrator? if user.has_role?(role) image = "roles/#{role}" alt = t("user.show.role.revoke.#{role}") diff --git a/app/models/changeset.rb b/app/models/changeset.rb index 4d305ea44..5cdfeb994 100644 --- a/app/models/changeset.rb +++ b/app/models/changeset.rb @@ -208,7 +208,7 @@ class Changeset < ActiveRecord::Base user_display_name_cache = {} if user_display_name_cache.nil? - if user_display_name_cache && user_display_name_cache.key?(user_id) + if user_display_name_cache&.key?(user_id) # use the cache if available elsif user.data_public? user_display_name_cache[user_id] = user.display_name diff --git a/app/models/client_application.rb b/app/models/client_application.rb index 1d5922d8e..d11942beb 100644 --- a/app/models/client_application.rb +++ b/app/models/client_application.rb @@ -51,7 +51,7 @@ class ClientApplication < ActiveRecord::Base def self.find_token(token_key) token = OauthToken.includes(:client_application).find_by(:token => token_key) - token if token && token.authorized? + token if token&.authorized? end def self.verify_request(request, options = {}, &block) diff --git a/app/models/notifier.rb b/app/models/notifier.rb index 554d1e364..3cc1be1e6 100644 --- a/app/models/notifier.rb +++ b/app/models/notifier.rb @@ -181,8 +181,8 @@ class Notifier < ActionMailer::Base end def user_avatar_file_path(user) - image = user && user.image - if image && image.file? + image = user&.image + if image&.file? return image.path(:small) else return Rails.root.join("app", "assets", "images", "users", "images", "small.png") diff --git a/app/models/relation.rb b/app/models/relation.rb index b17a3b604..42ac3d6dc 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -260,7 +260,7 @@ class Relation < ActiveRecord::Base element = model.lock("for share").find_by(:id => m[1]) # and check that it is OK to use. - raise OSM::APIPreconditionFailedError, "Relation with id #{id} cannot be saved due to #{m[0]} with id #{m[1]}" unless element && element.visible? && element.preconditions_ok? + raise OSM::APIPreconditionFailedError, "Relation with id #{id} cannot be saved due to #{m[0]} with id #{m[1]}" unless element&.visible? && element&.preconditions_ok? hash[m[1]] = true end diff --git a/app/models/way.rb b/app/models/way.rb index d6f0011d9..2d149863e 100644 --- a/app/models/way.rb +++ b/app/models/way.rb @@ -127,7 +127,7 @@ class Way < ActiveRecord::Base ordered_nodes[nd.sequence_id] = nd.node_id.to_s if visible_nodes[nd.node_id] else # otherwise, manually go to the db to check things - ordered_nodes[nd.sequence_id] = nd.node_id.to_s if nd.node && nd.node.visible? + ordered_nodes[nd.sequence_id] = nd.node_id.to_s if nd.node&.visible? end end diff --git a/script/deliver-message b/script/deliver-message index 98e00d332..2c440c3ec 100755 --- a/script/deliver-message +++ b/script/deliver-message @@ -23,7 +23,7 @@ end exit 0 unless token == digest[0, 6] exit 0 if date < 1.month.ago -message.update(:message_read => true) if message +message&.update(:message_read => true) mail = Mail.new(STDIN.read .encode(:universal_newline => true)