Fix the column_for_attribute deprecation warning for the last time #3930

seanlinsley · 2015-05-04T01:31:03Z

Turns out we couldn’t use type_for_column since:

it’s not intended as a public method
it would return a null object if the column had the default value (nil)

for example:

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/index_as_table.feature:244 # Scenario: Sorting

houndci-bot · 2015-05-04T01:31:06Z

lib/active_admin/views/components/table_for.rb

-            item.column_for_attribute(data) &&
-            item.column_for_attribute(data).type == :boolean
+        if item.class.respond_to? :columns_hash
+          column = item.class.columns_hash[data.to_s] and column.type == :boolean


Line is too long. [81/80]
Use && instead of and.

Line is too long. [81/80]

@timoschilling I'd prefer the maximum to be 90 or 100 characters. Do you have a strong opinion either way?

Use && instead of and.

Heh, static code analysis can only understand so much.

@seanlinsley There was already a discussion about that in #3841. I think the 120 chars from #3841 are to much, but 90-100 could be ok. In the past I have seen 80 chars as a "soft limit", in cases where a code rewrite is ugly then a long line, I have preferred the long line.

@seanlinsley does the test covers the difference between && and and? Not that some will change it and the test don't show it.

seanlinsley · 2015-05-04T01:36:03Z

It's been a while since I've contributed. Did Travis always run the test suite twice?

Fivell · 2015-05-04T07:28:00Z

@seanlinsley , I think in case when item is decorated item.class.respond_to? :columns_hash will always give false

timoschilling · 2015-05-04T07:38:12Z

@seanlinsley your commit run's as a ActiveAdmin repo commit (continuous-integration/travis-ci/push) and as a PR commit (continuous-integration/travis-ci/pr). That's happens only if a ActiveAdmin contributor creates a PR from a ActiveAdmin branch.

seanlinsley · 2015-05-04T20:58:41Z

@Fivell that might be true, in which case we need more tests!

Fivell · 2015-05-05T08:23:40Z

@seanlinsley, I can add cucumber feature failing/or not here (https://github.com/activeadmin/activeadmin/blob/master/features/index/index_as_table.feature) if you need it

deivid-rodriguez · 2015-07-17T16:17:18Z

@Fivell Are you still up for adding that failing test?

timoschilling · 2015-08-18T01:41:53Z

@Fivell maybe you can continue the work on this PR?

we couldn’t use `type_for_column` since: - it’s not intended as a public method and - it would return a null object if the column had the default value (nil) for example: ``` 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/index_as_table.feature:244 # Scenario: Sorting ```

seanlinsley · 2015-10-11T14:55:09Z

I'm working adding that test now.

@seanlinsley your commit run's as a ActiveAdmin repo commit (continuous-integration/travis-ci/push) and as a PR commit (continuous-integration/travis-ci/pr). That's happens only if a ActiveAdmin contributor creates a PR from a ActiveAdmin branch.

It seems like we should make it so continuous-integration/travis-ci/pr only happens on master, and on version branches like 0-6-stable.

forced to fix them. This is one deprecation: >DEPRECATION WARNING: Using ActiveRecord's `find` on a CollectionDecorator is deprecated. Call `find` on a model, and then decorate the result. (called from block in add_default_batch_action at lib/active_admin/batch_actions/resource_extension.rb:73) (StandardError) This issue was introduced by #3775, which started decorating collections for batch actions. The Draper ticket that resulted in the deprecation is: drapergem/draper#509. The simple solution would be to no longer decorate the batch action collections, which is what this commit does.

seanlinsley · 2015-10-11T18:36:26Z

Okay, this is ready for review.

seanlinsley · 2015-10-13T14:14:49Z

@Fivell this PR stops decorating the batch action collection, because I couldn't figure out how to get around the Draper deprecation from calling find on a collection decorator. Unless I'm mis-remembering, before #3775 we didn't decorate batch actions. Why did you add that in the first place?

Fivell · 2015-10-13T14:48:28Z

@seanlinsley , how do you think isn't it confusing when you're declaring decorator, you still have some places where it is decorated and some places where it isn't. However I have no idea too how to fix this stupid deprecation warning as well.

seanlinsley · 2015-10-13T15:19:03Z

It seems like the only way we could continue to decorate the collection is by doing the filtering for the user, which would be a breaking change.

batch_action :archive do |collection|
  collection.each{ |record| record.update archived: true }
end

We might want to do that for AA 2.0, but for now I don't think we can support decoration.

Fivell · 2015-10-13T15:23:50Z

@seanlinsley , maybe we can introduce new specific method filtered_collection on controller level to make it possible to use for AA users who want to have decorated items, other can use batch_collection collection which will be documented about it contains undecorated items ?

seanlinsley · 2015-10-13T15:38:00Z

I don't think it makes sense to provide both filtered_collection and batch_collection. If you're applying decoration to Active Admin, there's no reason to provide an undecorated version.

A nice way to provide the new DSL without needing to deprecate anything would be to still pass the IDs as a block argument, but provide collection as a method that would automatically apply filtering and decoration.

batch_action :archive do |ids|
  ids == [1,2,3]
  collection.is_a? Draper::CollectionDecorator # with filtering applied

Is there a reason we would use batch_collection instead of collection?

Fivell · 2015-10-13T15:56:00Z

@seanlinsley , sorry not clear for me how it helps

fabn · 2015-11-06T16:59:29Z

What's the status of this PR? Does it solve the deprecation warning? Thanks.

fabn · 2015-11-09T15:16:39Z

For other seeking for a solution until this is merged. I imported this in my code using a monkey patch, ugly but effective

module ActiveAdmin
  # Module used to define patches using Module#prepend
  module Patches
    # Original module
    module Views
      # Monkeypatch for https://github.com/activeadmin/activeadmin/pull/3930
      module TableFor
        protected

        def is_boolean?(data, item)
          if item.class.respond_to? :columns_hash
            (column = item.class.columns_hash[data.to_s]) and column.type == :boolean
          end
        end
      end

    end
  end
end

ActiveAdmin::Views::TableFor.prepend ActiveAdmin::Patches::Views::TableFor

seanlinsley · 2015-11-23T03:18:04Z

@fabn this does indeed fix the deprecation. I'm going to go ahead and merge it, moving the decorator discussion to #4211

houndci-bot reviewed May 4, 2015
View reviewed changes

seanlinsley mentioned this pull request May 4, 2015

remove TableFor#is_boolean, detect boolean by attribute type #3929

Closed

Fivell mentioned this pull request May 5, 2015

Rails 4.2 still complains about deprecated ActiveRecord #column_for_attribute #3838

Closed

c-lliope mentioned this pull request May 30, 2015

Create a dashbard generator thoughtbot/administrate#20

Merged

seanlinsley added 2 commits October 11, 2015 09:51

make deprecations cause the test suite to fail

e3908e5

seanlinsley force-pushed the 3838-column_for_attribute-deprecation branch from fedccff to 714b092 Compare October 11, 2015 14:52

seanlinsley added 3 commits October 11, 2015 10:12

ensure that boolean fields are decorated

87e5828

simplify collection_applies

e4aa309

seanlinsley mentioned this pull request Nov 23, 2015

Automatic collection lookup for batch actions #4211

Closed

seanlinsley merged commit e4aa309 into master Nov 23, 2015

seanlinsley deleted the 3838-column_for_attribute-deprecation branch November 23, 2015 03:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the column_for_attribute deprecation warning for the last time #3930

Fix the column_for_attribute deprecation warning for the last time #3930

seanlinsley commented May 4, 2015

houndci-bot May 4, 2015

seanlinsley May 4, 2015

timoschilling May 11, 2015

timoschilling May 11, 2015

seanlinsley commented May 4, 2015

Fivell commented May 4, 2015

timoschilling commented May 4, 2015

seanlinsley commented May 4, 2015

Fivell commented May 5, 2015

deivid-rodriguez commented Jul 17, 2015

timoschilling commented Aug 18, 2015

seanlinsley commented Oct 11, 2015

seanlinsley commented Oct 11, 2015

seanlinsley commented Oct 13, 2015

Fivell commented Oct 13, 2015

seanlinsley commented Oct 13, 2015

Fivell commented Oct 13, 2015

seanlinsley commented Oct 13, 2015

Fivell commented Oct 13, 2015

fabn commented Nov 6, 2015

fabn commented Nov 9, 2015

seanlinsley commented Nov 23, 2015

Fix the column_for_attribute deprecation warning for the last time #3930

Fix the column_for_attribute deprecation warning for the last time #3930

Conversation

seanlinsley commented May 4, 2015

houndci-bot May 4, 2015

Choose a reason for hiding this comment

seanlinsley May 4, 2015

Choose a reason for hiding this comment

timoschilling May 11, 2015

Choose a reason for hiding this comment

timoschilling May 11, 2015

Choose a reason for hiding this comment

seanlinsley commented May 4, 2015

Fivell commented May 4, 2015

timoschilling commented May 4, 2015

seanlinsley commented May 4, 2015

Fivell commented May 5, 2015

deivid-rodriguez commented Jul 17, 2015

timoschilling commented Aug 18, 2015

seanlinsley commented Oct 11, 2015

seanlinsley commented Oct 11, 2015

seanlinsley commented Oct 13, 2015

Fivell commented Oct 13, 2015

seanlinsley commented Oct 13, 2015

Fivell commented Oct 13, 2015

seanlinsley commented Oct 13, 2015

Fivell commented Oct 13, 2015

fabn commented Nov 6, 2015

fabn commented Nov 9, 2015

seanlinsley commented Nov 23, 2015