Lint with up-to-date rubocop

Matching version bump of rubocop

.rubocop.yml

@@ -1,20 +1,35 @@
-Rails: 
+Rails:
   Enabled: true
 
 AllCops:
+  TargetRailsVersion: 4.0
   Exclude:
     - 'bin/*'
     - 'db/schema.rb'
     - 'db/seeds.rb'
     - 'vendor/bundle/**/*'
 
+Layout/DotPosition:
+  EnforcedStyle: trailing
+
+Layout/SpaceInsideHashLiteralBraces:
+  EnforcedStyle: no_space
+
+Layout/AccessModifierIndentation:
+  EnforcedStyle: outdent
+
 Metrics/AbcSize:
   Exclude:
     - 'app/controllers/passwords_controller.rb'
     - 'app/controllers/sessions_controller.rb'
     - 'app/controllers/users_controller.rb'
     - 'lib/thing_importer.rb'
 
+Metrics/BlockLength:
+  Exclude:
+    - 'config/initializers/*'
+  ExcludedMethods: ['test']
+
 Metrics/BlockNesting:
   Max: 2
 
@@ -33,9 +48,6 @@ Metrics/ParameterLists:
   Max: 4
   CountKeywordArgs: true
 
-Style/AccessModifierIndentation:
-  EnforcedStyle: outdent
-
 Style/CollectionMethods:
   PreferredMethods:
     map:      'collect'
@@ -46,15 +58,9 @@ Style/CollectionMethods:
 Style/Documentation:
   Enabled: false
 
-Style/DotPosition:
-  EnforcedStyle: trailing
-
 Style/DoubleNegation:
   Enabled: false
 
-Style/SpaceInsideHashLiteralBraces:
-  EnforcedStyle: no_space
-
 Style/TrailingCommaInArguments:
   EnforcedStyleForMultiline: 'comma'
 

Gemfile

@@ -1,8 +1,6 @@
 source 'https://rubygems.org'
 ruby '2.2.3'
 
-gem 'dotenv-rails', groups: [:development, :test]
-gem 'rails', '~> 4.2.4'
 gem 'airbrake', '~> 5.2'
 gem 'devise', '~> 3.0'
 gem 'geokit', '~> 1.0'
@@ -11,31 +9,34 @@ gem 'http_accept_language', '~> 2.0'
 gem 'local_time', '~> 1.0'
 gem 'obscenity', '~> 1.0', '>= 1.0.2'
 gem 'pg'
+gem 'rails', '~> 4.2.4'
 gem 'rails_admin', '~> 1.0'
 gem 'validates_formatting_of', '~> 0.9.0'
 
-gem 'tzinfo-data', platforms: [:mingw, :mswin, :x64_mingw]
 gem 'paranoia', '~> 2.2'
+gem 'tzinfo-data', platforms: %i[mingw mswin x64_mingw]
+
+gem 'byebug', groups: %i[development test]
+gem 'dotenv-rails', groups: %i[development test]
 
 group :assets do
   gem 'sass-rails', '>= 4.0.3'
   gem 'uglifier'
 end
 
 group :development do
-  gem 'byebug'
   gem 'spring'
 end
 
 group :production do
-  gem 'rails_12factor'
   gem 'puma'
+  gem 'rails_12factor'
   gem 'skylight'
 end
 
 group :test do
   gem 'coveralls', require: false
-  gem 'simplecov', require: false
   gem 'rubocop'
+  gem 'simplecov', require: false
   gem 'webmock'
 end

Rakefile

@@ -12,4 +12,4 @@ end
 
 Rails.application.load_tasks
 
-task default: [:rubocop, :test]
+task default: %i[rubocop test]

app/controllers/info_window_controller.rb

@@ -1,6 +1,6 @@
 class InfoWindowController < ApplicationController
   def index
-    @thing = Thing.find_by_id(params[:thing_id])
+    @thing = Thing.find_by(id: params[:thing_id])
     view = begin
       if @thing.adopted?
         user_signed_in? && current_user == @thing.user ? 'users/thank_you' : 'users/profile'

app/controllers/reminders_controller.rb

@@ -8,7 +8,7 @@ def create
     @reminder.from_user = current_user
     if @reminder.save
       ThingMailer.reminder(@reminder.thing).deliver_later
-      @reminder.update_attribute(:sent, true)
+      @reminder.update_attributes(sent: true)
       render(json: @reminder)
     else
       render(json: {errors: @reminder.errors}, status: 500)

config/initializers/airbrake.rb

@@ -41,7 +41,7 @@
   # environments.
   # NOTE: This option *does not* work if you don't set the 'environment' option.
   # https://github.com/airbrake/airbrake-ruby#ignore_environments
-  c.ignore_environments = %w(development test)
+  c.ignore_environments = %w[development test]
 
   # A list of parameters that should be filtered out of what is sent to
   # Airbrake. By default, all "password" attributes will have their contents

db/migrate/00000000000005_create_rails_admin_histories_table.rb

@@ -10,6 +10,6 @@ def change
       t.timestamps
     end
 
-    add_index(:rails_admin_histories, [:item, :table, :month, :year], name: 'index_rails_admin_histories')
+    add_index(:rails_admin_histories, %i[item table month year], name: 'index_rails_admin_histories')
   end
 end

lib/thing_importer.rb

@@ -48,15 +48,15 @@ def import_temp_things(source_url)
       insert_statement_id = SecureRandom.uuid
 
       conn = ActiveRecord::Base.connection
-      conn.execute(<<-SQL)
-CREATE TEMPORARY TABLE "temp_thing_import" (
-  id serial,
-  name varchar,
-  lat numeric(16,14),
-  lng numeric(17,14),
-  city_id integer,
-  system_use_code varchar
-)
+      conn.execute(<<-SQL.strip_heredoc)
+        CREATE TEMPORARY TABLE "temp_thing_import" (
+          id serial,
+          name varchar,
+          lat numeric(16,14),
+          lng numeric(17,14),
+          city_id integer,
+          system_use_code varchar
+        )
       SQL
       conn.raw_connection.prepare(insert_statement_id, 'INSERT INTO temp_thing_import (name, lat, lng, city_id, system_use_code) VALUES($1, $2, $3, $4, $5)')
 
@@ -67,7 +67,8 @@ def import_temp_things(source_url)
         each do |thing|
         conn.raw_connection.exec_prepared(
           insert_statement_id,
-          [thing[:type], thing[:lat], thing[:lng], thing[:city_id], thing[:system_use_code]])
+          [thing[:type], thing[:lat], thing[:lng], thing[:city_id], thing[:system_use_code]],
+        )
       end
 
       conn.execute('CREATE INDEX "temp_thing_import_city_id" ON temp_thing_import(city_id)')
@@ -77,36 +78,36 @@ def import_temp_things(source_url)
     # return the deleted drains partitioned by whether they were adopted
     def delete_non_existing_things
       # mark deleted_at as this is what the paranoia gem uses to scope
-      deleted_things = ActiveRecord::Base.connection.execute(<<-SQL)
-UPDATE things
-SET deleted_at = NOW()
-WHERE things.city_id NOT IN (SELECT city_id from temp_thing_import) AND deleted_at IS NULL
-RETURNING things.city_id, things.user_id
+      deleted_things = ActiveRecord::Base.connection.execute(<<-SQL.strip_heredoc)
+        UPDATE things
+        SET deleted_at = NOW()
+        WHERE things.city_id NOT IN (SELECT city_id from temp_thing_import) AND deleted_at IS NULL
+        RETURNING things.city_id, things.user_id
       SQL
       deleted_things.partition { |thing| thing['user_id'].present? }
     end
 
     def upsert_things
       # postgresql's RETURNING returns both updated and inserted records so we
       # query for the items to be inserted first
-      created_things = ActiveRecord::Base.connection.execute(<<-SQL)
-SELECT temp_thing_import.city_id
-FROM things
-RIGHT JOIN temp_thing_import ON temp_thing_import.city_id = things.city_id
-WHERE things.id IS NULL
+      created_things = ActiveRecord::Base.connection.execute(<<-SQL.strip_heredoc)
+        SELECT temp_thing_import.city_id
+        FROM things
+        RIGHT JOIN temp_thing_import ON temp_thing_import.city_id = things.city_id
+        WHERE things.id IS NULL
       SQL
 
-      ActiveRecord::Base.connection.execute(<<-SQL)
-INSERT INTO things(name, lat, lng, city_id, system_use_code)
-SELECT name, lat, lng, city_id, system_use_code FROM temp_thing_import
-ON CONFLICT(city_id) DO UPDATE SET
-  lat = EXCLUDED.lat,
-  lng = EXCLUDED.lng,
-  name = CASE
-           WHEN things.user_id IS NOT NULL THEN things.name
-           ELSE EXCLUDED.name
-         END,
-  deleted_at = NULL
+      ActiveRecord::Base.connection.execute(<<-SQL.strip_heredoc)
+        INSERT INTO things(name, lat, lng, city_id, system_use_code)
+        SELECT name, lat, lng, city_id, system_use_code FROM temp_thing_import
+        ON CONFLICT(city_id) DO UPDATE SET
+          lat = EXCLUDED.lat,
+          lng = EXCLUDED.lng,
+          name = CASE
+                   WHEN things.user_id IS NOT NULL THEN things.name
+                   ELSE EXCLUDED.name
+                 END,
+          deleted_at = NULL
       SQL
 
       created_things

test/controllers/info_window_controller_test.rb

@@ -64,7 +64,7 @@ class InfoWindowControllerTest < ActionController::TestCase
 
   test 'should show special link on adoption form if it has one' do
     sign_in @user
-    Thing.stub :find_by_id, @thing do
+    Thing.stub :find_by, @thing do
       @thing.stub :detail_link, 'http://example.com' do
         get :index, thing_id: @thing.id
       end

test/lib/thing_importer_test.rb

@@ -4,21 +4,21 @@
 
 class ThingImporterTest < ActiveSupport::TestCase
   test 'import does not modify data if endpoint fails' do
-    thing_1 = things(:thing_1)
+    thing1 = things(:thing_1)
 
     fake_url = 'http://sf-drain-data.org'
     stub_request(:get, fake_url).to_return(status: [500, 'Internal Server Error'], body: nil)
     assert_raises OpenURI::HTTPError do
       ThingImporter.load(fake_url)
     end
-    assert_not_nil Thing.find(thing_1.id)
+    assert_not_nil Thing.find(thing1.id)
   end
 
   test 'loading things, deletes existing things not in data set, updates properties on rest' do
     admin = users(:admin)
-    thing_1 = things(:thing_1)
-    thing_11 = things(:thing_11)
-    thing_10 = things(:thing_10).tap do |thing|
+    thing1 = things(:thing_1)
+    thing11 = things(:thing_11)
+    thing10 = things(:thing_10).tap do |thing|
       thing.update!(name: 'Erik drain', user_id: users(:erik).id)
     end
     things(:thing_9).tap do |thing|
@@ -43,11 +43,11 @@ class ThingImporterTest < ActiveSupport::TestCase
     email = ActionMailer::Base.deliveries.last
     assert_equal email.to, [admin.email]
     assert_equal email.subject, 'Adopt-a-Drain San Francisco import (1 adopted drains removed, 1 drains added, 7 unadopted drains removed)'
-    thing_11.reload
-    thing_10.reload
+    thing11.reload
+    thing10.reload
 
     # Asserts thing_1 is deleted
-    assert_nil Thing.find_by(id: thing_1.id)
+    assert_nil Thing.find_by(id: thing1.id)
 
     # Asserts thing_3 is reified
     assert_equal Thing.find_by(city_id: 3).id, deleted_thing.id
@@ -59,12 +59,12 @@ class ThingImporterTest < ActiveSupport::TestCase
     assert_equal new_thing.lng, BigDecimal.new(-121.40, 16)
 
     # Asserts properties on thing_11 have been updated
-    assert_equal thing_11.lat, BigDecimal.new(37.75, 16)
-    assert_equal thing_11.lng, BigDecimal.new(-122.40, 16)
+    assert_equal thing11.lat, BigDecimal.new(37.75, 16)
+    assert_equal thing11.lng, BigDecimal.new(-122.40, 16)
 
     # Asserts properties on thing_10 have been updated, but not the name
-    assert_equal 'Erik drain', thing_10.name
-    assert_equal BigDecimal.new(36.75, 16), thing_10.lat
-    assert_equal BigDecimal.new(-121.40, 16), thing_10.lng
+    assert_equal 'Erik drain', thing10.name
+    assert_equal BigDecimal.new(36.75, 16), thing10.lat
+    assert_equal BigDecimal.new(-121.40, 16), thing10.lng
   end
 end

test/mailers/thing_mailer_test.rb

@@ -41,18 +41,18 @@ class ThingMailerTest < ActionMailer::TestCase
   end
 
   test 'thing_update_report' do
-    admin_1 = users(:admin)
-    admin_2 = users(:admin)
-    admin_2.update(email: '[email protected]')
+    admin1 = users(:admin)
+    admin2 = users(:admin)
+    admin2.update(email: '[email protected]')
     email = nil
     deleted_thing = things(:thing_1)
 
     assert_emails(1) do
       email = ThingMailer.thing_update_report([deleted_thing], [], []).deliver_now
     end
 
-    assert_includes email.to, admin_1.email
-    assert_includes email.to, admin_2.email
+    assert_includes email.to, admin1.email
+    assert_includes email.to, admin2.email
 
     assert_equal email.subject, 'Adopt-a-Drain San Francisco import (1 adopted drains removed, 0 drains added, 0 unadopted drains removed)'
   end