Support automatic use of decorators

[amiel]

Background

See discussion at #1079.

All of this is consistant with the jcasimir/draper api, but does not specifically require draper.
It is also fully backwards compatible for folks that do not use decorators.

Notes

Because of the pagination and scoping in ActiveAdmin, this requires that #decorate return a
collection proxy and not just an array of decorators. This is the case in jcasimir/draper starting at v0.9.3.

[nathancarnes]

Any word on getting this merged in? Would love to see this as part of AA.

[amiel]

I've rebased this back to master to ensure a clean merge.

[pcreux]

@amiel Thanks for rebasing it. It does not pass on Travis-CI. http://travis-ci.org/#!/gregbell/active_admin/builds/1234486

[amiel]

:(

It looks like I'm using some ruby 1.9 syntax, which is why it broken in ree
However, the failures on 1.9.2 are related to ActiveAdmin::Comment. I'm not sure what happened there, but I'll look into it.

[amiel]

Ok, it looks like the ActiveAdmin::Comment failures are unrelated (see #1273).

I've removed the ruby 1.9 specific syntax and test pass for me in ruby-1.9.3, and mostly in ree-1.8.7-2012.02. However, I'm getting the following error running the cucumber tests only in ree-1.8.7-2012.02.

(::) failed steps (::)

uninitialized constant Store (NameError)
./features/step_definitions/factory_steps.rb:48:in `/^a store named "([^"]*)" exists$/'
features/i18n.feature:7:in `And a store named "Hello words" exists'

(eval):1:in `load_active_admin_configuration': uninitialized constant ActiveAdminReloading::Store (NameError)
./features/step_definitions/configuration_steps.rb:5:in `load_active_admin_configuration'
./features/step_definitions/configuration_steps.rb:49:in `eval'
./features/step_definitions/configuration_steps.rb:5:in `load_active_admin_configuration'
./features/step_definitions/configuration_steps.rb:49:in `/^a configuration of:$/'
features/i18n.feature:23:in `And a configuration of:'

Failing Scenarios:
cucumber features/i18n.feature:5 # Scenario: Store's model name was translated to "Bookstore"
cucumber features/i18n.feature:21 # Scenario: Switching language at runtime

[amiel]

Also I rebased on master again

[gregbell]

Hi @amiel,

Thanks for the pull request, this is an awesome feature. Definitely something that I want to see merged in.

The issue with the current implementation (as I see it) is that the view components are building the decorator objects. It should not be the responsibility of the view to know about decorators, rather it should just receive the decorated resource or collection.

To do this, we should override the #collection and #resource methods that are provided by the Inherited Resources. There is reference code in a ticket on the Draper project: https://github.com/jcasimir/draper/issues/99

Then, we could add a configuration to resource like so:

ActiveAdmin.register Post do
  decorate_with PostDecorator
end

Is this a refactoring you have some time to take on?

[gregbell]

Also, the above example, for the collection it should use:

PostDecorator.decorate(collection)

And for a single instance it should use:

PostDecorator.new(single_instance)

This will keep the API simple and will allow users to create decorators that don't depend on Draper.

[amiel]

@gregbell thanks for the feedback. I'll try to make the changes you've requested during my next available open-source-time.

[travisbot]

This pull request fails (merged 92e1b8f4 into c7ca563).

[amiel]

@gregbell What would be the preferred place to implement decorate_with?

It's fine if you don't have a preference, I'll find somewhere's to put it.

[gregbell]

To add a method to the DSL, simple add an instance method to ActiveAdmin::ResourceDSL. The instance of ResourceDSL has access to an instance of ActiveAdmin::Resource at ResourceDSL#config.

Basically ResourceDSL#decorate_with should be a thin method that sets the decorator on the Resource instance. The Resource is the object that holds on to all the configuration for a given resource, so it should hold an instance var with the decorator. Then the view has access to the resource instance through the active_admin_config helper method.

[amiel]

Thanks, that's really helpful.

[gregbell]

Not a problem. Let me know if you have any other questions.

[travisbot]

This pull request fails (merged 222b22e0 into c7ca563).

[travisbot]

This pull request fails (merged be01eec8 into c7ca563).

[amiel]

Hmm, I'm a little confused because the travis-ci failures have nothing to do with what I'm changing, and all the tests pass locally. Travisbot tells me it's merged, but when I do the same merge, and run the tests again, they all still pass.

I'm gonna keep on truckin' and hope this issue might be solved in a future rebase...

[pcreux]

@amiel Tests are failing on Travis-CI for the master branch as well. See #1359.

[travisbot]

This pull request fails (merged 6a1bc58e into 73b051f).

[travisbot]

This pull request fails (merged a984605f into 73b051f).

[amiel]

@gregbell -- I think I'm getting pretty close here.

Just a couple of notes while I'm thinking about them:

1. I've been sticking with the interface that you outlined here: Support automatic use of decorators #1117 (comment), (new and decorate). I like the concept that we stick to a simple interface so that users are not tied to using draper, but I'd like to point out that implementing the collection method Decorator.decorate is not as simple as returning an array of decorated objects. See Draper's DecoratedEnumerableProxy for an example.
2. I'm out of open source time for this week, so the rest will have to wait until at least next week. However, I plan to try using this branch in another project to test it out a bit, if I get a chance.
3. I still plan to document this. Specifically, how to enable the use of decorators, and the required decorator interface.

Anyway, that's my status.

BTW, as I start to wrap this up, what's your opinion on git history with the change of direction. Do you want the original history of this feature? or would you like me to rebase to remove the process of writing the original implementation?

[travisbot]

This pull request fails (merged 8eca77dd into 73b051f).

[amiel]

This last failure is baffling me.

[gregbell]

Thanks @amiel! This is really coming along.

1. Yes, you are correct with the Decorator.decorate method. I wonder if we should only require the Decorator#new and implement our own collection decorator wrapping? I don't want to add too much scope to getting this released, so maybe we can use the Decorator.decorate to get this into the wild, then implement our own collection wrapping which would allow users to create very simple class decorators. Thoughts?
2. I don't think that the git repo needs to have all these design changes. I know that we have in the past, however I think that slimming down the commits is a good thing. So, I would prefer to see this compacted into one commit when it's being merged in.

Thanks for your work on this! This is going to be a very useful feature. In another couple of apps, I'm already wishing this was in place to use :)

[amiel]

@gregbell Thanks.

As I said before, I'll try to get working on this again next week. For now, a couple of thoughts.

1. I seems out of scope to me for ActiveAdmin to provide a decorated collection implementation, especially since it's present in draper. My thought would be to suggest the use of draper, but clearly document the required api (probably in a wiki page). Once this is released, if using ActiveAdmin with decorators but not Draper becomes a known common use-case, then I'd reconsider including a collection wrapper...
   Another thought, however, is that an internal collection wrapper might help ActiveAdmin support collections of plain ruby objects instead of ActiveRecord backed objects, but I'm not sure.
2. I'll rebase -i my commits to clean up the pull-request then.

[amiel]

@gregbell Ok, I've added a cucumber test, cleaned up a few files, and squashed all this stuff into one commit.

I've tested this in a couple of apps I've written for work that use ActiveAdmin and Draper.

My next step is to write documentation. My first thought was to write in the README so that my changes would be part of the pull-request, but after reading through what's there, it doesn't really make sense to write there. Where should I write documentation? Should I start writing in the wiki? Should I add a page to the docs directory?

[gregbell]

I merged in the pull request to Arbre (activeadmin/arbre#2). I'll release a new version and bump the version in AA shortly.

[amiel]

Sweet. Thanks guys, I'm looking forward to this being a part of Active Admin.

Travis is still failing only for RAILS=3.2.3 on ree.

The failure is:

undefined method `title' for #<Post:0xc385110>

which I completely don't understand. All of the other scenarios pass so I'm not sure how to deal with that.

Is it possible to run the tests again? Maybe it was a fluke in the universe?

Thoughts?

[travisbot]

This pull request fails (merged 9411f95d into 3130933).

[amiel]

ok guys, I've rebased again to try to keep up.

What's weird is that I'm getting failing tests locally that pass on travis-ci.
The one test that fails on travis-ci is

undefined method `title' for #<Post:0xb848310>

which is strange since the Post model has a title and it works in every other environment.

[travisbot]

This pull request fails (merged b69e59c2 into 3130933).

[jpmckinney]

Indeed, that is an odd test failure...

[amiel]

@jpmckinney Any ideas? do you want me to rebase again?

[jpmckinney]

Worth a try!

[travisbot]

This pull request fails (merged a180c76 into 1a20a16).

[amiel]

Hmmm, it looks like something changed in the rebase. I'm going to have to look at this tomorrow.

[dapi]

I'm very interested in this request. Can I help?

[amiel]

@dapi This feature is pretty much finished. There are still two things left to do:

1. Get travis to pass.
   When I wrote this, everything was passing on travisci, except for one weird failure (discussed above) only when RAILS=3.2.3 on ree.
   Now there is another failure, I haven't had time to look into it, but it is certainly from a rebase.
2. Testing.
   If you have a chance, try this branch out in your project. I've been using it on some of my projects, but it would be helpful to have other people try it out and verify that it is stable.

Thanks for your interest, I'm hopeful that we can get this merged in to master soon.

[dapi]

ok. I get it

[amiel]

@dapi Please let me know if you find any issues with your testing.

[dapi]

It look's like a bug in rails_apps_composer with rvm usages. Fixing..

[amiel]

@dapi Wow, thanks for looking into this. I look forward to seeing your changes.

[dapi]

rails_apps_composer is not guilty.

The problem was in DelegateClass. It does not response to attributes of model.

I replace it with SimpleDelegator a96d652

Here is a request - #1647

[amiel]

@dapi Wow, thanks so much for figuring this out, I was baffled.

@jpmckinney @gregbell @pcreux Would you like me to rebase this on master again, or can you take it from here?

[jpmckinney]

@amiel can you review #1647 and let us know if it does what you were trying to accomplish in this pull request?

[amiel]

Yep, the only difference between #1647 and this pull-request is that #1647 passes travis-ci thanks to @dapi.

In other words, yes, #1647 contains exactly what I'm hoping to accomplish with this pull-request.

Thanks

[jpmckinney]

Cool, I'll close this issue then, and review #1647.

[rogerkk]

I'm trying to find out how to make use of this functionality without using draper, as I'm already using non-draper decorators. I found this page on the wiki, which still says this functionality is a work in progress: https://github.com/gregbell/active_admin/wiki/Implement-a-custom-decorator

Anyone know how to do this?

[macfanatic]

I believe all the code is implemented in this file: https://github.com/gregbell/active_admin/blob/master/lib/active_admin/resource_controller/decorators.rb

The decorator_class setting is what you pass in when you decorate your resource, such as:

ActiveAdmin.register Post do
  decorate_with PostDecorator
end

As long as your decorator responds to #decorate_collection to decorate the collection, and will take a resource in the constructor, you should be all set.

[amiel]

@macfanatic The only tricky part is making sure collections are handled correctly. For example, it must handle being filtered with scopes, pagination, filters, etc., and still return the decorated collection.

The integration tests use this: spec/support/templates/post_decorator.rb, it may prove to be useful.

[rogerkk]

Thanks guys. I got as far as trying to implement #decorate_collection before I posed my question, but I could not figure out how to do it, as a I kept bumping into new errors. Thanks for the hint about the code in the integration tests, @amiel, I'll see if I can make that work!