Merge pull request #2679 from HashNotAdam/hotfix/code_review

Code review

.rubocop.yml

@@ -41,6 +41,9 @@ Metrics/PerceivedComplexity:
 Style/AccessModifierIndentation:
   EnforcedStyle: outdent
 
+Style/Alias:
+  Enabled: false
+
 Style/CollectionMethods:
   PreferredMethods:
     map:      'collect'
@@ -76,7 +79,8 @@ Style/RaiseArgs:
 Style/SpaceInsideHashLiteralBraces:
   EnforcedStyle: no_space
 
-Style/TrailingComma:
+Style/TrailingCommaInArguments:
   EnforcedStyleForMultiline: 'comma'
 
-
+Style/TrailingCommaInLiteral:
+  EnforcedStyleForMultiline: 'comma'

Gemfile

@@ -43,7 +43,7 @@ group :test do
   gem 'pundit'
   gem 'rack-cache', require: 'rack/cache'
   gem 'rspec-rails', '>= 2.14'
-  gem 'rubocop', '~> 0.31.0'
+  gem 'rubocop', '~> 0.41.2'
   gem 'simplecov', '>= 0.9', require: false
   gem 'timecop', '>= 0.5'
 

app/controllers/rails_admin/application_controller.rb

@@ -21,13 +21,13 @@ class ApplicationController < Config.parent_controller.constantize
 
     def get_model
       @model_name = to_model_name(params[:model_name])
-      fail(RailsAdmin::ModelNotFound) unless (@abstract_model = RailsAdmin::AbstractModel.new(@model_name))
-      fail(RailsAdmin::ModelNotFound) if (@model_config = @abstract_model.config).excluded?
+      raise(RailsAdmin::ModelNotFound) unless (@abstract_model = RailsAdmin::AbstractModel.new(@model_name))
+      raise(RailsAdmin::ModelNotFound) if (@model_config = @abstract_model.config).excluded?
       @properties = @abstract_model.properties
     end
 
     def get_object
-      fail(RailsAdmin::ObjectNotFound) unless (@object = @abstract_model.get(params[:id]))
+      raise(RailsAdmin::ObjectNotFound) unless (@object = @abstract_model.get(params[:id]))
     end
 
     def to_model_name(param)

app/controllers/rails_admin/main_controller.rb

@@ -93,7 +93,7 @@ def sanitize_params_for!(action, model_config = @model_config, target_params = p
       return unless target_params.present?
       fields = visible_fields(action, model_config)
       allowed_methods = fields.collect(&:allowed_methods).flatten.uniq.collect(&:to_s) << 'id' << '_destroy'
-      fields.each { |field|  field.parse_input(target_params) }
+      fields.each { |field| field.parse_input(target_params) }
       target_params.slice!(*allowed_methods)
       target_params.permit! if target_params.respond_to?(:permit!)
       fields.select(&:nested_form).each do |association|
@@ -110,7 +110,7 @@ def handle_save_error(whereto = :new)
 
       respond_to do |format|
         format.html { render whereto, status: :not_acceptable }
-        format.js   { render whereto, layout: false, status: :not_acceptable  }
+        format.js   { render whereto, layout: false, status: :not_acceptable }
       end
     end
 

app/helpers/rails_admin/application_helper.rb

@@ -47,8 +47,8 @@ def logout_path
       if defined?(Devise)
         scope = Devise::Mapping.find_scope!(_current_user)
         main_app.send("destroy_#{scope}_session_path") rescue false
-      else
-        main_app.logout_path if main_app.respond_to?(:logout_path)
+      elsif main_app.respond_to?(:logout_path)
+        main_app.logout_path
       end
     end
 

app/helpers/rails_admin/form_builder.rb

@@ -20,10 +20,17 @@ def generate(options = {})
     end
 
     def fieldset_for(fieldset, nested_in)
-      return unless (fields = fieldset.with(form: self, object: @object, view: @template, controller: @template.controller).visible_fields).length > 0
+      fields = fieldset.with(
+        form: self,
+        object: @object,
+        view: @template,
+        controller: @template.controller,
+      ).visible_fields
+      return if fields.empty?
+
       @template.content_tag :fieldset do
         contents = []
-        contents << @template.content_tag(:legend, %(<i class="icon-chevron-#{(fieldset.active? ? 'down' : 'right')}"></i> #{fieldset.label}).html_safe, style: "#{fieldset.name == :default ? 'display:none' : ''}")
+        contents << @template.content_tag(:legend, %(<i class="icon-chevron-#{(fieldset.active? ? 'down' : 'right')}"></i> #{fieldset.label}).html_safe, style: fieldset.name == :default ? 'display:none' : '')
         contents << @template.content_tag(:p, fieldset.help) if fieldset.help.present?
         contents << fields.collect { |field| field_wrapper_for(field, nested_in) }.join
         contents.join.html_safe

app/helpers/rails_admin/main_helper.rb

@@ -63,7 +63,7 @@ def ordered_filter_string
         filter_name = filter_for_field.keys.first
         filter_hash = filter_for_field.values.first
         unless (field = filterable_fields.find { |f| f.name == filter_name.to_sym })
-          fail "#{filter_name} is not currently filterable; filterable fields are #{filterable_fields.map(&:name).join(', ')}"
+          raise "#{filter_name} is not currently filterable; filterable fields are #{filterable_fields.map(&:name).join(', ')}"
         end
         case field.type
         when :enum

config/initializers/active_record_extensions.rb

@@ -6,11 +6,11 @@ def self.rails_admin(&block)
       end
 
       def rails_admin_default_object_label_method
-        self.new_record? ? "new #{self.class}" : "#{self.class} ##{id}"
+        new_record? ? "new #{self.class}" : "#{self.class} ##{id}"
       end
 
       def safe_send(value)
-        if self.has_attribute?(value)
+        if has_attribute?(value)
           read_attribute(value)
         else
           send(value)

gemfiles/rails_4.0.gemfile

@@ -49,7 +49,7 @@ group :test do
   gem "pundit"
   gem "rack-cache", :require => "rack/cache"
   gem "rspec-rails", ">= 2.14"
-  gem "rubocop", "~> 0.31.0"
+  gem "rubocop", "~> 0.41.2"
   gem "simplecov", ">= 0.9", :require => false
   gem "timecop", ">= 0.5"
 

gemfiles/rails_4.1.gemfile

@@ -47,7 +47,7 @@ group :test do
   gem "pundit"
   gem "rack-cache", :require => "rack/cache"
   gem "rspec-rails", ">= 2.14"
-  gem "rubocop", "~> 0.31.0"
+  gem "rubocop", "~> 0.41.2"
   gem "simplecov", ">= 0.9", :require => false
   gem "timecop", ">= 0.5"
 

gemfiles/rails_4.2.gemfile

@@ -48,7 +48,7 @@ group :test do
   gem "pundit"
   gem "rack-cache", :require => "rack/cache"
   gem "rspec-rails", ">= 2.14"
-  gem "rubocop", "~> 0.31.0"
+  gem "rubocop", "~> 0.41.2"
   gem "simplecov", ">= 0.9", :require => false
   gem "timecop", ">= 0.5"
 

gemfiles/rails_5.0.gemfile

@@ -47,7 +47,7 @@ group :test do
   gem "pundit"
   gem "rack-cache", :require => "rack/cache"
   gem "rspec-rails", ">= 2.14"
-  gem "rubocop", "~> 0.31.0"
+  gem "rubocop", "~> 0.41.2"
   gem "simplecov", ">= 0.9", :require => false
   gem "timecop", ">= 0.5"
 

lib/rails_admin/abstract_model.rb

@@ -147,7 +147,7 @@ def build_statement_for_type_generic
       end
 
       def build_statement_for_type
-        fail('You must override build_statement_for_type in your StatementBuilder')
+        raise('You must override build_statement_for_type in your StatementBuilder')
       end
 
       def build_statement_for_integer_decimal_or_float
@@ -185,11 +185,11 @@ def build_statement_for_datetime_or_timestamp
       end
 
       def unary_operators
-        fail('You must override unary_operators in your StatementBuilder')
+        raise('You must override unary_operators in your StatementBuilder')
       end
 
       def range_filter(_min, _max)
-        fail('You must override range_filter in your StatementBuilder')
+        raise('You must override range_filter in your StatementBuilder')
       end
 
       class FilteringDuration

lib/rails_admin/adapters/active_record.rb

@@ -6,7 +6,7 @@
 module RailsAdmin
   module Adapters
     module ActiveRecord
-      DISABLED_COLUMN_TYPES = [:tsvector, :blob, :binary, :spatial, :hstore, :geometry]
+      DISABLED_COLUMN_TYPES = [:tsvector, :blob, :binary, :spatial, :hstore, :geometry].freeze
 
       def new(params = {})
         AbstractObject.new(model.new(params))
@@ -56,8 +56,8 @@ def associations
       def properties
         columns = model.columns.reject do |c|
           c.type.blank? ||
-          DISABLED_COLUMN_TYPES.include?(c.type.to_sym) ||
-          c.try(:array)
+            DISABLED_COLUMN_TYPES.include?(c.type.to_sym) ||
+            c.try(:array)
         end
         columns.collect do |property|
           Property.new(property, model)
@@ -99,11 +99,12 @@ def initialize(scope)
 
         def add(field, value, operator)
           field.searchable_columns.flatten.each do |column_infos|
-            if value.is_a?(Array)
-              value = value.map { |v| field.parse_value(v) }
-            else
-              value = field.parse_value(value)
-            end
+            value =
+              if value.is_a?(Array)
+                value.map { |v| field.parse_value(v) }
+              else
+                field.parse_value(value)
+              end
             statement, value1, value2 = StatementBuilder.new(column_infos[:column], column_infos[:type], value, operator).to_statement
             @statements << statement if statement.present?
             @values << value1 unless value1.nil?
@@ -115,7 +116,7 @@ def add(field, value, operator)
 
         def build
           scope = @scope.where(@statements.join(' OR '), *@values)
-          scope = scope.references(*(@tables.uniq)) if @tables.any?
+          scope = scope.references(*@tables.uniq) if @tables.any?
           scope
         end
       end
@@ -211,7 +212,7 @@ def build_statement_for_string_or_text
             when 'ends_with'
               "%#{@value.downcase}"
             when 'is', '='
-              "#{@value.downcase}"
+              @value.downcase
             else
               return
             end

lib/rails_admin/adapters/mongoid.rb

@@ -8,7 +8,7 @@
 module RailsAdmin
   module Adapters
     module Mongoid
-      DISABLED_COLUMN_TYPES = %w(Range Moped::BSON::Binary BSON::Binary Mongoid::Geospatial::Point)
+      DISABLED_COLUMN_TYPES = %w(Range Moped::BSON::Binary BSON::Binary Mongoid::Geospatial::Point).freeze
 
       def parse_object_id(value)
         Bson.parse_object_id(value)
@@ -190,7 +190,7 @@ def sort_by(options, scope)
         when String
           field_name, collection_name = options[:sort].split('.').reverse
           if collection_name && collection_name != table_name
-            fail('sorting by associated model column is not supported in Non-Relational databases')
+            raise('sorting by associated model column is not supported in Non-Relational databases')
           end
         when Symbol
           field_name = options[:sort].to_s

lib/rails_admin/adapters/mongoid/association.rb

@@ -28,7 +28,7 @@ def type
           when :has_and_belongs_to_many, :references_and_referenced_in_many
             :has_and_belongs_to_many
           else
-            fail("Unknown association type: #{macro.inspect}")
+            raise("Unknown association type: #{macro.inspect}")
           end
         end
 
@@ -83,7 +83,7 @@ def read_only?
         def nested_options
           nested = nested_attributes_options.try { |o| o[name] }
           if !nested && [:embeds_one, :embeds_many].include?(macro.to_sym) && !association.cyclic
-            fail <<-MSG.gsub(/^\s+/, '')
+            raise <<-MSG.gsub(/^\s+/, '')
             Embbeded association without accepts_nested_attributes_for can't be handled by RailsAdmin,
             because embedded model doesn't have top-level access.
             Please add `accepts_nested_attributes_for :#{association.name}' line to `#{model}' model.

lib/rails_admin/adapters/mongoid/extension.rb

@@ -16,7 +16,7 @@ def rails_admin(&block)
         end
 
         def rails_admin_default_object_label_method
-          self.new_record? ? "new #{self.class}" : "#{self.class} ##{id}"
+          new_record? ? "new #{self.class}" : "#{self.class} ##{id}"
         end
 
         def safe_send(value)

lib/rails_admin/adapters/mongoid/property.rb

@@ -2,7 +2,7 @@ module RailsAdmin
   module Adapters
     module Mongoid
       class Property
-        STRING_TYPE_COLUMN_NAMES = [:name, :title, :subject]
+        STRING_TYPE_COLUMN_NAMES = [:name, :title, :subject].freeze
         attr_reader :property, :model
 
         def initialize(property, model)

lib/rails_admin/bootstrap-sass.rb

@@ -14,15 +14,13 @@ def self.load!
       require 'sass-rails' if rails?
 
       unless rails? || compass?
-        fail(Bootstrap::FrameworkNotFound.new('bootstrap-sass requires either Rails > 3.1 or Compass, neither of which are loaded'))
+        raise(Bootstrap::FrameworkNotFound.new('bootstrap-sass requires either Rails > 3.1 or Compass, neither of which are loaded'))
       end
 
       stylesheets = File.expand_path(File.join('..', 'vendor', 'assets', 'stylesheets'))
       ::Sass.load_paths << stylesheets
     end
 
-  private
-
     def self.asset_pipeline?
       defined?(::Sprockets)
     end

lib/rails_admin/bootstrap-sass/compass_functions.rb

@@ -8,7 +8,7 @@ def image_path(source, _options = {})
           image_url(source, Sass::Script::Bool.new(true))
         else
           # Revert to the old compass-agnostic path determination
-          asset_sans_quotes = source.value.gsub('"', '')
+          asset_sans_quotes = source.value.delete('"')
           Sass::Script::String.new("/images/#{asset_sans_quotes}", :string)
         end
       end

lib/rails_admin/config.rb

@@ -113,8 +113,8 @@ def audit_with(*args, &block)
           @audit = proc do
             @auditing_adapter = RailsAdmin::AUDITING_ADAPTERS[extension].new(*([self] + args).compact)
           end
-        else
-          @audit = block if block
+        elsif block
+          @audit = block
         end
         @audit || DEFAULT_AUDIT
       end
@@ -148,8 +148,8 @@ def authorize_with(*args, &block)
           @authorize = proc do
             @authorization_adapter = RailsAdmin::AUTHORIZATION_ADAPTERS[extension].new(*([self] + args).compact)
           end
-        else
-          @authorize = block if block
+        elsif block
+          @authorize = block
         end
         @authorize || DEFAULT_AUTHORIZE
       end
@@ -193,7 +193,7 @@ def default_search_operator=(operator)
         if %w(default like starts_with ends_with is =).include? operator
           @default_search_operator = operator
         else
-          fail(ArgumentError.new("Search operator '#{operator}' not supported"))
+          raise(ArgumentError.new("Search operator '#{operator}' not supported"))
         end
       end
 

lib/rails_admin/config/actions.rb

@@ -96,7 +96,7 @@ def add_action_custom_key(action, &block)
           action.instance_eval(&block) if block
           @@actions ||= []
           if action.custom_key.in?(@@actions.collect(&:custom_key))
-            fail "Action #{action.custom_key} already exists. Please change its custom key."
+            raise "Action #{action.custom_key} already exists. Please change its custom key."
           else
             @@actions << action
           end

lib/rails_admin/config/actions/base.rb

@@ -115,12 +115,11 @@ class Base
 
         # Breadcrumb parent
         register_instance_option :breadcrumb_parent do
-          case
-          when root?
+          if root?
             [:dashboard]
-          when collection?
+          elsif collection?
             [:index, bindings[:abstract_model]]
-          when member?
+          elsif member?
             [:show, bindings[:abstract_model], bindings[:object]]
           end
         end

lib/rails_admin/config/actions/bulk_delete.rb

@@ -43,13 +43,11 @@ class BulkDelete < RailsAdmin::Config::Actions::Base
 
               if destroyed.nil?
                 flash[:error] = t('admin.flash.error', name: pluralize(0, @model_config.label), action: t('admin.actions.delete.done'))
-                redirect_to back_or_index
               else
                 flash[:success] = t('admin.flash.successful', name: pluralize(destroyed.count, @model_config.label), action: t('admin.actions.delete.done')) unless destroyed.empty?
                 flash[:error] = t('admin.flash.error', name: pluralize(not_destroyed.count, @model_config.label), action: t('admin.actions.delete.done')) unless not_destroyed.empty?
-                redirect_to back_or_index
               end
-
+              redirect_to back_or_index
             end
           end
         end