Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rails 4.2 still complains about deprecated ActiveRecord #column_for_attribute #3838

Closed
rdj opened this issue Mar 6, 2015 · 26 comments
Closed

Rails 4.2 still complains about deprecated ActiveRecord #column_for_attribute #3838

rdj opened this issue Mar 6, 2015 · 26 comments
Assignees
@seanlinsley

Comments

@rdj
Copy link
Contributor

rdj commented Mar 6, 2015

Active Admin produces a deprecation warning with Rails 4.2.1.rc3:

DEPRECATION WARNING: #column_for_attributewill return a null object for non-existent columns in Rails 5. Use#has_attribute? if you need to check for an attribute's existence. (called from is_boolean? at /var/jenkins/.rvm/gems/ruby-2.2.1@project/bundler/gems/active_admin-96c6db140e57/lib/active_admin/views/components/table_for.rb:120)

This was previously reported in #3487 (fixed c8e54d2) and then amended in a way that reintroduced the deprecation warning (48ccc3d).

That last commit provides justification for a why the method should be used. The problem remains, however, that anybody using Active Admin will deal with the noise of the deprecation warning.

Rails provides a way to silence deprecation warnings on a particular section of code when you've decided you know better. Now is the time to use it.
@rdj rdj changed the title Rails 4.2 still complains about deprecated ActiveRecord::Base#column_for_attribute Rails 4.2 still complains about deprecated ActiveRecord #column_for_attribute Mar 6, 2015
@rdj rdj mentioned this issue Mar 6, 2015
Closed
@rdj
Copy link
Contributor Author

rdj commented Mar 7, 2015

Ok, since the PR has been closed, can we review what to actually do here? The commit history I outlined here shows that the aa team seems to have considered and rejected actually taking action to remove the deprecated action. While I agree that silencing deprecation warnings is a bad idea in general, I think that as a library — integrated by many other orgs into their rails apps — ignoring the deprecation warnings is even worse. It will waste tons people's future time since each and every org that uses active admin will have to separately investigate the deprecation warning, deal with their own internal policies about deprecation warnings, etc.

So if we could revisit the previous commits here, can we agree that it would be preferable to have a fix that works correctly and does not emit the deprecation warning?

@gonzedge - you contributed 48ccc3d — I have read the commit message, but I don't completely follow it. Possibly multiple rails versions are involved here, but you didn't specify.

Also, I get tons of failures on my MacBook running 10.10.2 when I try to run the test suite. I see Travis says the build is good? I looked at the contribution guidelines and it doesn't make it sound like running the tests is that hard; should I be able to get them working on my machine or do they only run on Linux or some other caveat I didn't see? I'd love to make a fix that works and doesn't emit the warning, but I wouldn't make substantive changes to a project whose test suite I can't run.

@rdj
Copy link
Contributor Author

rdj commented Mar 7, 2015

Oh, yeah, pull in @timoschilling since he closed the PR. Really would like to figure out something positive to do here rather than just ignoring the problem.

@seanlinsley
Copy link
Contributor

@rdj what sort of test failures are you getting locally? Perhaps we could jump on IRC to try and figure what's happening together.

@timoschilling
Copy link
Member

@rdj I agree with you too. Ignoring is not a solution. Hope you find a better way

@rdj
Copy link
Contributor Author

rdj commented Mar 9, 2015

@seanlinsley Thanks, here's what I'm currently seeing.

$ git ls-remote . refs/heads/master
74769783b78046d4e5cd148beb957317a4f86ca1        refs/heads/master

I start with a completely blank slate in an isolated empty rvm gemset.

I run bundle install, which looks like it installs Rails 3.2.21. Here's the gemfile.lock: https://gist.github.com/rdj/8158b2563eadc5a578ec

I run bundle exec rake test and here's where things fail. Here's the output: https://gist.github.com/rdj/beb89f608c85efd95ee4

Seems like the rake test is trying to run the rails command line to create a rails app and stuff is failing.

Ok, so poking around I see that I can drop a .rails-version file to control what rails version I'm using, plus there's a script/use_rails for hotswitching between. So I decide I'll try rails 4.2.1.rc3 since that's what I'm trying to work fix problems with. I'll clean everything back out first though.

rm -rf Gemfile.lock coverage spec/rails
rvm gemset empty
script/use_rails 4.2.1.rc3

Here's the Gemfile.lock for that: https://gist.github.com/rdj/786a8667a523088c0f2e

Then when I run bundle exec rake test I get 71 failures. Here's the output: https://gist.github.com/rdj/1765492f84b684bccaf2

@rdj
Copy link
Contributor Author

rdj commented Mar 9, 2015

Double checked everything and get the same results. I also tried Rails 4.2 just for completeness and got a different type of failure: https://gist.github.com/rdj/5e7ee9c5b88f3a0197eb

@rdj
Copy link
Contributor Author

rdj commented Mar 10, 2015

Spun up a clean Ubuntu Trusty machine on EC2 and I get the same results, so I guess at least it's not Mac specific. (Though on linux I had to manually add therubyracer to the Gemfile.)

@timoschilling
Copy link
Member

@rdj any updates?

@rdj
Copy link
Contributor Author

rdj commented Apr 9, 2015

@timoschilling I was never able to get tests running, so I'm in no position to prepare a fix.

In my app, these deprecations were coming up where an auto-formatted column (column(:foo)) was used with a highly tweaked scoped_collection with a group by and some aggregate function columns. When I replaced it with a manual column (column(:foo) { |r| r.foo }) I stopped getting the deprecations. This could be a good clue as to how to reproduce the problem.

I see now that the IRC channel is specified in the README.md, which I had missed at the time. I'm working on a different project at the moment, but perhaps I will have some time in the next couple of weeks to pop in. Like I said, I'd be happy to investigate if I can get the tests running.

@Fivell
Copy link
Member

Fivell commented May 3, 2015

@timoschilling , why we need is_boolean? method at all?
https://github.com/activeadmin/activeadmin/blob/master/lib/active_admin/views/components/table_for.rb#L113

Why not to check if value is boolean and not to check column type and so on? something like

[true, false].include? value

What do you think?

@Fivell Fivell mentioned this issue May 3, 2015
Closed
@timoschilling
Copy link
Member

What's about nil?

@Fivell
Copy link
Member

Fivell commented May 3, 2015

@timoschilling, I'm looking at tests and looks like it was decided to render nil as "No" for boolean columns , is that true ? Can we change this or no ?

@seanlinsley
Copy link
Contributor

@Fivell yes, nil should be rendered as "No".

It wasn't well documented on the original Rails pull request, but in our case we should update our code to use type_for_attribute if it's defined (4.2 and above), else use column_for_attribute.

@seanlinsley
Copy link
Contributor

Wait, this was already done in #3487, but then it later got reverted by #3730?

The reasoning for that change was:

The #is_boolean? implementation calls #type_for_attribute or #column_for_attribute depending on the Rails version. However, #type_for_attribute returns an instance of ActiveRecord::Type::Value with a null #type, which makes the #status_tag tests fail.

They didn't go into detail how exactly the status_tag test fails, but that test (or the logic causing the failure) should be changed to properly handle a ActiveRecord::Type::Value that returns a nil type.

@seanlinsley
Copy link
Contributor

CC @gonzedge

@Fivell
Copy link
Member

Fivell commented May 3, 2015

@seanlinsley, let me describe "problematic" example.
#column_for_attribute(data) is raising deprecation warning in case there is no db column for attribute, there were few issues about this.
However in case if virtual columns like

activeadmin/features/index/index_as_table.feature

Line 270 in 6bde1f5

Post.select("id, title, length(title) as title_length")

calling #column_for_attribute will be always nil , which will give deprecation warning for rails 4.2.
However they can be still boolean , thats why I though checking value can help

@Fivell
Copy link
Member

Fivell commented May 3, 2015

which makes the #status_tag tests fail.

@gonzedge, can you tell what exact tests are failing?

@seanlinsley seanlinsley mentioned this issue May 4, 2015
Merged
@gonzedge
Copy link
Contributor

gonzedge commented May 5, 2015

I'm taking a look at the commit right before my pull request. I'll be back soon with the test failures.

@gonzedge
Copy link
Contributor

gonzedge commented May 5, 2015

Below are my results on ruby 2.2.2. I'll try running the same with ruby 2.2.0 later, since that was the version I was using at the time of the pull request. I will let you know what I find. I hope this helps the discussion.

The commit 38bf023 was right before my pull request (#3730).

git checkout 38bf023

Once you checkout that commit, you'll have to change the Gemfile to use the specific commit used at the time for inherited_resources, since the rails-4-2 branch no longer exists. Replace the inherited_resources entry in the Gemfile with:

gem 'inherited_resources', github: 'josevalim/inherited_resources', ref: '9f1c0edd6b4c'

Then, bundle and test (which will fail in the unit tests):

rm Gemfile.lock # it's possible that this part isn't such a great idea
RAILS=4.2.0 bundle
RAILS=4.2.0 bundle exec rake test

These are the unit test failures I'm getting. The important ones here are 1 and 5 on spec/unit/filters/filter_form_builder_spec.rb:297 and spec/unit/views/components/table_for_spec.rb:278, but for some reason I'm getting failures 2, 3 & 4. Interestingly, 2, 3 & 4 are also failing on the pull request commits, so let's not pay attention to them for now:

  1) ActiveAdmin::Filters::ViewHelper belongs_to when using a custom foreign key should should ignore that foreign key and let Ransack handle it
     Failure/Error: expect(Post.reflections[:category].foreign_key).to eq :custom_category_id
     NoMethodError:
       undefined method `foreign_key' for nil:NilClass
     # ./spec/unit/filters/filter_form_builder_spec.rb:297:in `block (4 levels) in <top (required)>'

  2) ActiveAdmin::FormBuilder with has many inputs with allow destroy with an existing post should include a boolean field for _destroy
     Failure/Error: allow(f.object.posts.build).to receive(:new_record?).and_return(false)
     NoMethodError:
       undefined method `allow' for #<ActiveAdmin::Views::ActiveAdminForm:0x007f8ac80d71e0>
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element.rb:182:in `method_missing'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/rails/forms.rb:37:in `method_missing'
     # ./spec/unit/form_builder_spec.rb:493:in `block (6 levels) in <top (required)>'
     # ./lib/active_admin/views/components/active_admin_form.rb:35:in `instance_eval'
     # ./lib/active_admin/views/components/active_admin_form.rb:35:in `build'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element/builder_methods.rb:28:in `block in build_tag'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/context.rb:92:in `with_current_arbre_element'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element/builder_methods.rb:26:in `build_tag'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element/builder_methods.rb:39:in `insert_tag'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element/builder_methods.rb:14:in `active_admin_form_for'
     # ./spec/unit/form_builder_spec.rb:37:in `block in build_form'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/context.rb:45:in `instance_eval'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/context.rb:45:in `initialize'
     # ./spec/rails_helper.rb:53:in `new'
     # ./spec/rails_helper.rb:53:in `arbre'
     # ./spec/rails_helper.rb:57:in `render_arbre_component'
     # ./spec/unit/form_builder_spec.rb:36:in `build_form'
     # ./spec/unit/form_builder_spec.rb:492:in `block (5 levels) in <top (required)>'
     # ./spec/unit/form_builder_spec.rb:501:in `block (5 levels) in <top (required)>'

  3) ActiveAdmin::FormBuilder with has many inputs with allow destroy with an existing post should have a check box with 'Remove' as its label
     Failure/Error: allow(f.object.posts.build).to receive(:new_record?).and_return(false)
     NoMethodError:
       undefined method `allow' for #<ActiveAdmin::Views::ActiveAdminForm:0x007f8ac80ea2e0>
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element.rb:182:in `method_missing'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/rails/forms.rb:37:in `method_missing'
     # ./spec/unit/form_builder_spec.rb:493:in `block (6 levels) in <top (required)>'
     # ./lib/active_admin/views/components/active_admin_form.rb:35:in `instance_eval'
     # ./lib/active_admin/views/components/active_admin_form.rb:35:in `build'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element/builder_methods.rb:28:in `block in build_tag'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/context.rb:92:in `with_current_arbre_element'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element/builder_methods.rb:26:in `build_tag'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element/builder_methods.rb:39:in `insert_tag'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element/builder_methods.rb:14:in `active_admin_form_for'
     # ./spec/unit/form_builder_spec.rb:37:in `block in build_form'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/context.rb:45:in `instance_eval'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/context.rb:45:in `initialize'
     # ./spec/rails_helper.rb:53:in `new'
     # ./spec/rails_helper.rb:53:in `arbre'
     # ./spec/rails_helper.rb:57:in `render_arbre_component'
     # ./spec/unit/form_builder_spec.rb:36:in `build_form'
     # ./spec/unit/form_builder_spec.rb:492:in `block (5 levels) in <top (required)>'
     # ./spec/unit/form_builder_spec.rb:505:in `block (5 levels) in <top (required)>'

  4) ActiveAdmin::FormBuilder with has many inputs with allow destroy with an existing post should wrap the destroy field in an li with class 'has_many_delete'
     Failure/Error: allow(f.object.posts.build).to receive(:new_record?).and_return(false)
     NoMethodError:
       undefined method `allow' for #<ActiveAdmin::Views::ActiveAdminForm:0x007f8ac6c5d3b8>
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element.rb:182:in `method_missing'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/rails/forms.rb:37:in `method_missing'
     # ./spec/unit/form_builder_spec.rb:493:in `block (6 levels) in <top (required)>'
     # ./lib/active_admin/views/components/active_admin_form.rb:35:in `instance_eval'
     # ./lib/active_admin/views/components/active_admin_form.rb:35:in `build'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element/builder_methods.rb:28:in `block in build_tag'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/context.rb:92:in `with_current_arbre_element'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element/builder_methods.rb:26:in `build_tag'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element/builder_methods.rb:39:in `insert_tag'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/element/builder_methods.rb:14:in `active_admin_form_for'
     # ./spec/unit/form_builder_spec.rb:37:in `block in build_form'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/context.rb:45:in `instance_eval'
     # ~/.rvm/gems/ruby-2.2.2@activeadmin/gems/arbre-1.0.3/lib/arbre/context.rb:45:in `initialize'
     # ./spec/rails_helper.rb:53:in `new'
     # ./spec/rails_helper.rb:53:in `arbre'
     # ./spec/rails_helper.rb:57:in `render_arbre_component'
     # ./spec/unit/form_builder_spec.rb:36:in `build_form'
     # ./spec/unit/form_builder_spec.rb:492:in `block (5 levels) in <top (required)>'
     # ./spec/unit/form_builder_spec.rb:509:in `block (5 levels) in <top (required)>'

  5) ActiveAdmin::Views::TableFor creating with the dsl when record attribute is boolean should render boolean attribute within status tag
     Failure/Error: expect(table.find_by_tag("span").first.class_list.to_a.join(' ')).to eq "status_tag yes"
     NoMethodError:
       undefined method `class_list' for nil:NilClass
     # ./spec/unit/views/components/table_for_spec.rb:278:in `block (4 levels) in <top (required)>'

From rake cucumber, the important failure happens in features/index/index_as_table.feature:244. For some reason, I'm also getting a new failure I wasn't getting before (features/index/filters.feature:109), but it's also failing in the pull request commits, so let's ignore it for now as well:

(::) failed steps (::)

One or more errors were raised in the Javascript code on the page. If you don't care about these errors, you can ignore them by setting js_errors: false in your Poltergeist configuration (see documentation for details).

ReferenceError: Can't find variable: options
ReferenceError: Can't find variable: options
    at http://127.0.0.1:53633/assets/active_admin.js:20350 in CheckboxToggler
    at http://127.0.0.1:53633/assets/active_admin.js:20794 in TableCheckboxToggler
    at http://127.0.0.1:53633/assets/active_admin.js:12966
    at http://127.0.0.1:53633/assets/active_admin.js:385
    at http://127.0.0.1:53633/assets/active_admin.js:137
    at http://127.0.0.1:53633/assets/active_admin.js:12968
    at http://127.0.0.1:53633/assets/active_admin.js:20324
    at http://127.0.0.1:53633/assets/active_admin.js:3144
    at http://127.0.0.1:53633/assets/active_admin.js:3256
    at http://127.0.0.1:53633/assets/active_admin.js:3468
    at http://127.0.0.1:53633/assets/active_admin.js:3499 in completed (Capybara::Poltergeist::JavascriptError)
./features/step_definitions/web_steps.rb:23:in `/^(?:I )am on (.+)$/'
./features/step_definitions/configuration_steps.rb:65:in `/^a(?:n? (index|show))? configuration of:$/'
features/index/filters.feature:113:in `And an index configuration of:'


expected: "No"
     got: ""

(compared using ==)
 (RSpec::Expectations::ExpectationNotMetError)
./features/step_definitions/table_steps.rb:96:in `assert_cells_match'
./features/step_definitions/table_steps.rb:79:in `block (2 levels) in assert_tables_match'
./features/step_definitions/table_steps.rb:75:in `each_index'
./features/step_definitions/table_steps.rb:75:in `block in assert_tables_match'
./features/step_definitions/table_steps.rb:74:in `each_index'
./features/step_definitions/table_steps.rb:74:in `assert_tables_match'
./features/step_definitions/table_steps.rb:115:in `/^I should see the "([^"]*)" table:$/'
features/index/index_as_table.feature:252:in `Then I should see the "index_table_posts" table:'

Failing Scenarios:
cucumber features/index/filters.feature:109 # Scenario: Clearing filter preserves custom parameters
cucumber features/index/index_as_table.feature:244 # Scenario: Sorting

Notice the expected: "No" got: "" failure above.

For rake spec:integration and rake cucumber:class_reloading, I get no failures.

After updating to commit e7dafc5, I don't get the highlighted failures anymore (
spec/unit/filters/filter_form_builder_spec.rb:297 and spec/unit/views/components/table_for_spec.rb:278 for rake spec:unit and features/index/index_as_table.feature:244 for rake cucumber).

@gonzedge
Copy link
Contributor

gonzedge commented May 5, 2015

Running the tests with ruby 2.2.0 yielded the same results.
Let me know what else I can help with @timoschilling @seanlinsley @Fivell @rdj

@Fivell
Copy link
Member

Fivell commented May 5, 2015

@seanlinsley , I'm still voting for detecting is_boolean by value not by db columns, we can change rendering nil values as grey status tag with "NULL"/"NONE"/"EMPTY".

This will give us next

  • boolean methods like verified?, checked?, etc will render status_tag by default
  • dynamic boolean columns (from joined tables or sql functions results) will work too

none of this will work if you will rely on active-record column , also if we are talking about #3930 , there can be problems with decorated collection, As I know class methods are not available for such records

@seanlinsley
Copy link
Contributor

I don't agree, @Fivell. We can detect whether the column is boolean, and we have done so historically, therefore we should continue to.

That's not to say that we can't also automatically use status_tag for dynamic values.

@Fivell
Copy link
Member

Fivell commented May 5, 2015

@seanlinsley, maybe at least we can use or logic ?
I mean try to check if db column is boolean and if we can't check (decorated, dynamic column etc) check by value? This is really good feature to wrap boolean attributes with status_tag, but current implementation doesn't cover all cases , as result- sometimes I should manually render status_tag sometimes not.

@seanlinsley
Copy link
Contributor

Yes, that's what I'm suggesting we do.

@timoschilling timoschilling mentioned this issue Sep 11, 2015
Closed
@dkniffin
Copy link
Contributor

dkniffin commented Apr 5, 2016

Forgive me for digging up an old issue, but was this ever actually resolved? I'm trying to clean up deprecation warnings in my app, and I'm using the latest version of AA (1.0.0.pre2), and I still see this warning. If it was resolved, can someone point me to the commit that resolved it?

Edit: found it: 3b1737b So, how close are we to another version bump?

@seanlinsley
Copy link
Contributor

@dkniffin the problem is that there are a lot of breaking changes in master that aren't document in the changelog. Once I find the time to update the changelog, I'll release 1.0.0 proper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@seanlinsley seanlinsley

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

remove TableFor#is_boolean, detect boolean by attribute type
6 participants
@rdj @Fivell @timoschilling @gonzedge @seanlinsley @dkniffin