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

Already on GitHub? Sign in to your account

Support automatic use of decorators #1117

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
9 participants
Contributor

amiel commented Mar 8, 2012

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.

@pcreux pcreux referenced this pull request Mar 21, 2012

Closed

Decorators #1079

Contributor

nathancarnes commented Apr 9, 2012

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

Contributor

amiel commented Apr 11, 2012

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

Contributor

pcreux commented May 3, 2012

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

Contributor

amiel commented May 3, 2012

:(

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.

Contributor

amiel commented May 3, 2012

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
Contributor

amiel commented May 3, 2012

Also I rebased on master again

pcreux added a commit that referenced this pull request May 14, 2012

Merge pull request #1117 from carnesmedia/active_admin
---

## Background

See discussion at #1079.

Includes brand new shiny specs for `ActiveAdmin::Views::Pages::Index` and `ActiveAdmin::Views::Pages::Show`.

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.

## Current functionality

### index

If `#{active_admin_config.resource_class}Decorator` is defined, the view will get
the collection returned by `#{active_admin_config.resource_class}Decorator.decorate(collection)`

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.

A different decorator class can be passed with the decorator option. Examples

```ruby
index decorator: AwesomeDecorator do; end

index decorator: nil; end # To disable the use of decorators
```

### Show

The show version works a little differently. If the resource object `responds_to?(:decorator)`
then that is what will be used. It would be possible to configure the decorator class the same
way as `Index` for consistency, but for now you can override `#decorator` on the model.
Contributor

gregbell commented May 16, 2012

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?

Contributor

gregbell commented May 16, 2012

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.

@ghost ghost assigned gregbell May 16, 2012

Contributor

amiel commented May 16, 2012

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

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

Contributor

amiel commented May 21, 2012

@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.

Contributor

gregbell commented May 23, 2012

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.

Contributor

amiel commented May 23, 2012

Thanks, that's really helpful.

Contributor

gregbell commented May 23, 2012

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

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

This pull request fails (merged be01eec8 into c7ca563).

Contributor

amiel commented May 25, 2012

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...

Contributor

pcreux commented May 25, 2012

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

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

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

Contributor

amiel commented May 29, 2012

@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: gregbell#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?

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

Contributor

amiel commented May 29, 2012

This last failure is baffling me.

Contributor

gregbell commented May 30, 2012

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 :)

Contributor

amiel commented Jun 1, 2012

@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.
Contributor

amiel commented Jun 5, 2012

@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?

Contributor

pcreux commented Jun 5, 2012

Hello @amiel,

Please add a page to the doc/ directory. Feel free to add more details to a wiki page if you want to. :)

Contributor

amiel commented Jun 5, 2012

I still have some documentation to write, but I believe this is ready for some testing!

This pull request passes (merged be70bb0b into 9cb9405).

This pull request passes (merged 07749636 into 9cb9405).

This pull request passes (merged 1c29be6d into 9cb9405).

This pull request passes (merged 0e27179a into 9cb9405).

This pull request passes (merged 9e95d191 into 9cb9405).

This pull request passes (merged c72c9170 into 9cb9405).

This pull request passes (merged 68a81961 into 9cb9405).

This pull request passes (merged a8c87b66 into 9cb9405).

Contributor

pcreux commented Jun 27, 2012

Hello @amiel, could you please rebase against master so that I can merge this in?

Cheers!

Contributor

amiel commented Jun 27, 2012

Yep, comin' right up

This pull request fails (merged 5fde41bc into 5e67c3a).

This pull request fails (merged a90bc3bb into 5e67c3a).

This pull request fails (merged c3e479ce into 5e67c3a).

Contributor

amiel commented Jun 27, 2012

This latest failure is actually bug in arbre. Pull request on the way, and about to push a work-around here...

Contributor

amiel commented Jun 27, 2012

Here is the related pull-request on arbre: gregbell/arbre#2

This pull request fails (merged 03e374a6 into 5e67c3a).

Contributor

pcreux commented Jun 28, 2012

Thanks @amiel, this is awesome! I'll try to find some time this week to test & merge it.

Contributor

gregbell commented Jun 28, 2012

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

Contributor

amiel commented Jun 28, 2012

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?

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

Contributor

amiel commented Aug 2, 2012

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.

This pull request fails (merged b69e59c2 into 3130933).

Contributor

jpmckinney commented Aug 23, 2012

Indeed, that is an odd test failure...

Contributor

amiel commented Aug 23, 2012

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

Contributor

jpmckinney commented Aug 23, 2012

Worth a try!

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

Contributor

amiel commented Aug 24, 2012

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

Contributor

dapi commented Sep 2, 2012

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

Contributor

amiel commented Sep 4, 2012

@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.

Contributor

dapi commented Sep 5, 2012

ok. I get it

Contributor

amiel commented Sep 5, 2012

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

Contributor

dapi commented Sep 6, 2012

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

Contributor

amiel commented Sep 6, 2012

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

Contributor

dapi commented Sep 6, 2012

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 - gregbell#1647

Contributor

amiel commented Sep 6, 2012

@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?

Contributor

jpmckinney commented Sep 6, 2012

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

Contributor

amiel commented Sep 6, 2012

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

Contributor

jpmckinney commented Sep 6, 2012

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

@jpmckinney jpmckinney closed this Sep 6, 2012

Contributor

rogerkk commented May 19, 2013

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?

Contributor

macfanatic commented May 20, 2013

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.

Contributor

amiel commented May 20, 2013

@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.

Contributor

rogerkk commented May 23, 2013

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!

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