Fix rubocop warnings

.rubocop.yml

@@ -37,3 +37,6 @@ Style/HashSyntax:
 
 Style/StringLiterals:
   EnforcedStyle: double_quotes
+
+Rails/HttpPositionalArguments:
+  Enabled: false

.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
+# Offense count: 2826
-# Configuration parameters: AllowHeredoc, AllowURI, URISchemes.
+# 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

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

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 <= ?",
+      changesets.where("closed_at >= ? and num_changes <= ?",
-                              Time.now.getutc, Changeset::MAX_ELEMENTS)
+                       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 > ?",
+      changesets.where("closed_at < ? or num_changes > ?",
-                              Time.now.getutc, Changeset::MAX_ELEMENTS)
+                       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
 

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

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)

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)

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
 

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])
+    user = User.find_by(:display_name => params[:display_name])
-    token = UserToken.find_by_token(session[:token])
+    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

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

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
 

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

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 0 then getdouble(s)                  # number
-      when 1 then return s.getbyte                     # boolean
+      when 1 then s.getbyte                     # boolean
-      when 2 then return getstring(s)                  # string
+      when 2 then getstring(s)                  # string
-      when 3 then return getobject(s)                  # object/hash
+      when 3 then getobject(s)                  # object/hash
-      when 5 then return nil                           # null
+      when 5 then nil                           # null
-      when 6 then return nil                           # undefined
+      when 6 then nil                           # undefined
-      when 8 then s.read(4)                            # mixedArray
+      when 8 then s.read(4) # mixedArray
-                  return getobject(s)                  #  |
+                  getobject(s)                  #  |
-      when 10 then return getarray(s)                  # array
+      when 10 then getarray(s)                  # array
-      else         return nil                          # error
       end
     end
 

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

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

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

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