Rails4 fixes #468

merged 26 commits into from Mar 24, 2014

5 participants


Building on work done by @PragTob and @ferrisoxide, here are the changes that I'm using to get Surveyor to work with Rails4. Rspec runs with only one failure on my machine (but that failure is also on NUBIC/rails4), and I've cleaned up all of the deprecation warnings and fixed a bunch of stuff that shows up at run-time.

Several months ago I tried to build the NUBIC rails4 branch but I couldn't get it to run the tests cleanly so I based my work on @ferrisoxide's. I have since (March 2014) merged NUBIC/rails4, although this currently causes one spec to fail.

Thanks for surveyor, it's a useful gem!

ferrisoxide and others added some commits Jul 11, 2013
@ferrisoxide ferrisoxide Merge branch 'feature/rails_4' into develop 82aa04e
@ferrisoxide ferrisoxide merged ce35340
@ferrisoxide ferrisoxide updated README fb511b2
@caboteria caboteria pin database_cleaner to version 1.0.1
The tests were failing with:
~/.rvm/gems/ruby-2.0.0-p247/gems/database_cleaner-1.1.0/lib/database_cleaner/active_record/truncation.rb:123:in `db_version': undefined local variable or method `postgresql_version' for #<ActiveRecord::ConnectionAdapters::SQLite3Adapter:0x007fd28ac70df8> (NameError)

Here's the bug report: DatabaseCleaner/database_cleaner#224

Looks like it might be a while until a fix happens so for the time
being we'll pin database_cleaner to version 1.0.1 which appears to
@caboteria caboteria replace rspec stub!() with stub() to quiet deprecation warnings
DEPRECATION: stub! is deprecated. Use stub instead. Called from ~/surveyor/spec/models/dependency_spec.rb:71:in `block (2 levels) in <top (required)>'.


   The original method was stub!, which I will confess to introducing,
   and for the wrong reasons, due to my misunderstanding of the meaning
   of ! in ruby methods.

   I recently added stub (with no !) with no particular fanfare, as I
   don't intend to remove stub! (at least not any time soon). Right now
   the docs don't make it clear, so I need to fix that - but I'd say that
   stub() is the method to use and stub!() is there for backwards
@caboteria caboteria replace Relation#all with other methods to quiet deprecation warnings
DEPRECATION WARNING: Relation#all is deprecated. If you want to eager-load a relation, you can call #load (e.g. `Post.where(published: true).load`). If you want to get an array of records from a relation, you can call #to_a (e.g. `Post.where(published: true).to_a`). (called from to_hash at /Users/tcabot/Code/surveyor/lib/surveyor/models/dependency_condition_methods.rb:39)
@caboteria caboteria refactor call to find(:first) to quiet deprecation warnings
DEPRECATION WARNING: Calling #find(:first) is deprecated. Please call #first directly instead. (called from edit at ~/surveyor/lib/surveyor/surveyor_controller_methods.rb:63)

Refactored so that we call either first() or find() depending on
whether we've got a primary key or not.
@caboteria caboteria fix deprecation warnings about calling find_by with options
DEPRECATION WARNING: Calling find_by or find_by! methods with options is deprecated. Build a scope instead, e.g. User.where('age > 21').find_by_name('Jon'). (called from block in load_and_update_response_set at ~/surveyor/lib/surveyor/surveyor_controller_methods.rb:112)
@caboteria caboteria fix deprecation warnings about calling scope() with a hash
DEPRECATION WARNING: Calling #scope or #default_scope with a hash is deprecated. Please use a lambda containing a scope. E.g. scope :red, -> { where(color: 'red') }. (called from included at ~/surveyor/lib/surveyor/models/answer_methods.rb:13)

We use instance_eval because we need the scope lambda to run in the
context of the including Class, not the Module in which the code is
@caboteria caboteria fix deprecation warnings relating to has_many declarations
DEPRECATION WARNING: The following options in your ResponseSet.has_many :responses declaration are deprecated: :order. Please use a scope block instead.

We use instance_eval because we need the scope lambda to run in the
context of the including Class, not the Module in which the code is
@caboteria caboteria removed "unloadable" declarations
They were causing circular dependency errors in development when
classes changed and had to be recompiled.  Turns out they're no longer
needed for Rails 4:


In development if I change one of the classes it recompiles, even
without the "unloadable" declaration, and I no longer get circular
dependency errors.
@caboteria caboteria move images to where sprockets expects them to be
Sprockets expects images in an engine to be in an [engine name]
directory, but the jquery ui images weren't.

The jquery ui stylesheet had to change, too, to reference images
starting at "/" rather than relative URL's.
@caboteria caboteria fix code that figures out if the asset pipeline is enabled
The Rails3-compatible algorithm to figure out if the asset pipeline is
doesn't work with Rails4.  Quite frankly, the new one doesn't seem as
clean as the old one, but hopefully this code will work in all cases.


Note that the specs didn't seem to test both cases, or else the image
path is now the same whether the pipeline is enabled or not, but in
any case the specs don't need to determine whether the pipeline is
enabled, they can use the same path in all cases.
@caboteria caboteria added a dependency on rails-observers
This used to be part of core Rails but in 4.0 was moved into its own
gem, so we need to indicate that we depend on it.  Otherwise, in an
eager_load=true environment we'll get:
uninitialized constant ActionController::Caching::Sweeper
@caboteria caboteria fix slow_updates monkey patch
Needed to inherit from ApplicationController.
@caboteria caboteria add dependency on selenium-webdriver
Without the explicit dependency the tests would crash immediately.
@caboteria caboteria downgrade back to older versions of cucumber-rails and capybara
I was hoping to upgrade these to the latest versions but they're
incompatible enough that it will take some effort to upgrade them.
@caboteria caboteria kludge: load formtastic enhancements explicitly
Many cucumber tests were failing because the views couldn't find a
"QuietInput" class.  It has something to do with dynamic reloading of
classes but I couldn't figure it out (sorry) so I fell back to loading
the classes explicitly before each scenario.  it's a kludge but it
@caboteria caboteria fix regexp serialization
Evidently in Rails 3.x active record would implicitly serialize Regexp
fields into YAML so for, for example, the ValidationCondition regexp
field would be implicitly converted from /[0-9a-zA-z\. #]/ to "---
!ruby/regexp '/[0-9a-zA-z\\. #]/'\n" when the model was saved.  In 4.0
this implicit serialization doesn't happen so we need to tell AR to do
it explicitly using AR's serialize method.
@caboteria caboteria update database_cleaner
Database cleaner used to have a bug that caused our tests to fail, but
they've released 1.2.0 which works so we no longer need to pin to the
old 1.0.1 release.
@caboteria caboteria fix deprecation warnings about #find(:first)
DEPRECATION WARNING: Calling #find(:first) is deprecated. Please call #first directly instead. You have also used finder options. These are also deprecated. Please build a scope instead of using finder options. (called from block (2 levels) in <top (required)> at /Users/tcabot/Code/surveyor/features/step_definitions/parser_steps.rb:85)
@caboteria caboteria fix deprecation warnings about the :value option
DEPRECATION WARNING: The :value option is deprecated in favour of `:input_html => { :value => '...'}` and will be removed in the next version. (called from block (2 levels) in ___sers_tcabot__ode_surveyor_app_views_partials__question_html_haml__1185397536205946121_70301830176800 at /Users/tcabot/Code/surveyor/app/views/partials/_question.html.haml:15)
@caboteria caboteria fix deprecation warnings about find_all_by_ dynamic finder
DEPRECATION WARNING: This dynamic method is deprecated. Please use e.g. Post.where(...).all instead. (called from block in <top (required)> at /Users/tcabot/Code/surveyor/features/step_definitions/parser_steps.rb:136)
@caboteria caboteria fix deprecation warnings about Hash#diff
DEPRECATION WARNING: Hash#diff is no longer used inside of Rails, and is being deprecated with no replacement. If you're using it to compare hashes for the purpose of testing, please use MiniTest's assert_equal instead. (called from unparse at /Users/tcabot/Code/surveyor/lib/surveyor/unparser.rb:32)

Since Rails' Hash#diff has no replacement I cribbed the code from Rails
and added it to Surveyor::Unparser, with only the changes needed to make
it take both hashes as parameters.  Now we'll be OK when Rails removes

Awesome. I've been a bit slack with regards to surveyor but I'm getting back into the swing. Keen to start contributing to the Rails 4 branch. Will definitely be helping out here over. Well done caboteria

@caboteria caboteria Merge remote-tracking branch 'refs/remotes/nubic/rails4' into rails4
@caboteria caboteria fix ResponseSet spec
It didn't account for the new access patterns so we needed to add some
additional stubs.
@yoon yoon commented on the diff Mar 24, 2014
@@ -6,6 +6,11 @@ def self.unparse(survey)
survey.unparse(dsl = "")
+ # cribbed from rails source: http://apidock.com/rails/v3.2.13/Hash/diff
+ def self.hash_diff(h1, h2)
yoon Mar 24, 2014 Northwestern University Biomedical Informatics Center member

@caboteria Where is this used?

caboteria Mar 24, 2014

I was using hash_diff in the various unparse() methods in lib/surveyor/unparser.rb (see commit 597f9c7) but it looks as if you fixed the same issue in a different way and I took your changes over mine so that code might not be used anymore.

@yoon yoon commented on the diff Mar 24, 2014
@@ -19,6 +19,7 @@ Gem::Specification.new do |s|
s.add_dependency('rails', '>= 3.2')
s.add_dependency('haml', '~> 4.0')
+ s.add_dependency('rails-observers', '~> 0.1')
yoon Mar 24, 2014 Northwestern University Biomedical Informatics Center member

@caboteria Unless I'm mistaken, we don't need observers any more

@yoon yoon commented on the diff Mar 24, 2014
@@ -2,13 +2,13 @@
class AddDisplayTypeToAnswers < ActiveRecord::Migration
def self.up
add_column :answers, :display_type, :string
- Answer.all.each{|a| a.update_attributes(:display_type => "hidden_label") if a.hide_label == true}
+ Answer.find_each{|a| a.update_attributes(:display_type => "hidden_label") if a.hide_label == true}
yoon Mar 24, 2014 Northwestern University Biomedical Informatics Center member

@caboteria I have a hard time supporting changes in migrations that people might already have in their apps. Is this absolutely necessary?

caboteria Mar 24, 2014

The code is functionally equivalent and doesn't cause deprecation warnings, but if you can live with the warnings then it's not necessary, at least until whatever they're warning about is removed. I don't think it should cause any trouble, though, since it will generate the same results.

rsutphin Mar 24, 2014

doesn't cause deprecation warnings

I don't think the old way would have caused deprecation warnings either. It's a weird quirk of Rails 4 that it wants you to change some_relation.all to some_relation.to_a, but does not warn about model_class.all or support model_class.to_a.

But I agree — find_each is functionally equivalent here.

@yoon yoon merged commit 5281b31 into NUBIC:master Mar 24, 2014

1 check failed

Details default The Travis CI build could not complete due to an error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment