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.

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

Decorating AA resources with POROs (Plain Old Ruby Objects) #3600

Closed
kiote opened this Issue Nov 7, 2014 · 14 comments

Comments

Projects
None yet
4 participants
@kiote

kiote commented Nov 7, 2014

We have bit complex logic in the admin part of our app, so they say the best way to fight the complexity is to use decorators.
I know Active Admin supports using of Draper gem for that, but I'm not sure I need it (for reasons you could read this article: http://thepugautomatic.com/2014/03/draper).

Thus, we decided to use POROs here. But I'm sticked with it, and can't make AA to use new "decorated" methods, they are just ignored.
Maybe I use wrong idea how the decorator should look to be used by Active Admin..

Here is the sample app to demonstrate: https://github.com/kiote/aa_plus_poro

Could someone please help me to understand, what I'm doing wrong with this kind of decorators?
Thanks!

P.S. it's a copy of message from mailing list, moved here with @timoschilling advice.

@timoschilling timoschilling added the bug? label Nov 8, 2014

@timoschilling

This comment has been minimized.

Show comment
Hide comment
@timoschilling

timoschilling Nov 8, 2014

Member

ActiveAdmin`s decorator integration has hard dependencies to Draper and supports collection wrapping only for draper at the moment.

Here is a quick fix for your problem:

module ActiveAdmin
  class ResourceController < BaseController
    module Decorators
      def apply_collection_decorator(collection)
        if decorate?
          decorator_class.new(collection).decorated_collection
        else
          collection
        end
      end
  end
end
Member

timoschilling commented Nov 8, 2014

ActiveAdmin`s decorator integration has hard dependencies to Draper and supports collection wrapping only for draper at the moment.

Here is a quick fix for your problem:

module ActiveAdmin
  class ResourceController < BaseController
    module Decorators
      def apply_collection_decorator(collection)
        if decorate?
          decorator_class.new(collection).decorated_collection
        else
          collection
        end
      end
  end
end
@rogerkk

This comment has been minimized.

Show comment
Hide comment
@rogerkk

rogerkk Nov 10, 2014

Contributor

Would be great being able to use POROs for this.

For your fix to work, @timoschilling, it seems we not only need our PORO decorator for the actual items themselves, but that the decorator_class.new(collection).decorated_collection call returns a collection decorator similar to draper's Draper::CollectionDecorator?

Contributor

rogerkk commented Nov 10, 2014

Would be great being able to use POROs for this.

For your fix to work, @timoschilling, it seems we not only need our PORO decorator for the actual items themselves, but that the decorator_class.new(collection).decorated_collection call returns a collection decorator similar to draper's Draper::CollectionDecorator?

@timoschilling

This comment has been minimized.

Show comment
Hide comment
@timoschilling

timoschilling Nov 10, 2014

Member

Disclaimer: I'm not very familiar with Draper or Decorators in general.

@rogerkk became you some errors if you tried my workaround?

It could be possible that you need to delegate some methods to the ActiveRecord::Relational object.

My will try to dive deeper into that problem and will improve ActiveAdmin for working with other decorators.

Member

timoschilling commented Nov 10, 2014

Disclaimer: I'm not very familiar with Draper or Decorators in general.

@rogerkk became you some errors if you tried my workaround?

It could be possible that you need to delegate some methods to the ActiveRecord::Relational object.

My will try to dive deeper into that problem and will improve ActiveAdmin for working with other decorators.

@rogerkk

This comment has been minimized.

Show comment
Hide comment
@rogerkk

rogerkk Nov 10, 2014

Contributor

@timoschilling Yes, I do get an error. I haven't dug very much into this, but at least I can tell you what I've done so far:

I've added a decorator which implements something like this:

class MyAwesomeDecorator < MyCustomApplicationDecorator
  def self.decorated_collection(collection)
    collection.map { |item| MyAwesomeDecorator.new(item) }
  end
end

Loading of the index page then yields the following error:

ActionView::Template::Error (undefined method `except' for #<Array:0xe219c0c>):
    1: insert_tag renderer_for(:index)
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/helpers/collection.rb:7:in `collection_size'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/helpers/collection.rb:13:in `collection_is_empty?'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/views/pages/index.rb:45:in `items_in_collection?'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/views/pages/index.rb:49:in `build_collection'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/views/pages/index.rb:28:in `block in main_content'
  arbre (1.0.1) lib/arbre/element/builder_methods.rb:31:in `block in build_tag'
  arbre (1.0.1) lib/arbre/context.rb:92:in `with_current_arbre_element'
  arbre (1.0.1) lib/arbre/element/builder_methods.rb:49:in `with_current_arbre_element'
  arbre (1.0.1) lib/arbre/element/builder_methods.rb:26:in `build_tag'
  arbre (1.0.1) lib/arbre/element/builder_methods.rb:39:in `insert_tag'
  arbre (1.0.1) lib/arbre/element/builder_methods.rb:14:in `batch_action_form'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/views/pages/index.rb:36:in `wrap_with_batch_action_form'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/views/pages/index.rb:26:in `main_content'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/views/pages/base.rb:83:in `block (2 levels) in build_main_content_wrapper'

[...]

I guess draper handles it's delegating of methods to ActiveRecord::Relation by including Draper::Delegation into Draper::CollectionDecorator and that somehow mimicking what this CollectionDecorator does is a key point in getting PORO decorators working?

Contributor

rogerkk commented Nov 10, 2014

@timoschilling Yes, I do get an error. I haven't dug very much into this, but at least I can tell you what I've done so far:

I've added a decorator which implements something like this:

class MyAwesomeDecorator < MyCustomApplicationDecorator
  def self.decorated_collection(collection)
    collection.map { |item| MyAwesomeDecorator.new(item) }
  end
end

Loading of the index page then yields the following error:

ActionView::Template::Error (undefined method `except' for #<Array:0xe219c0c>):
    1: insert_tag renderer_for(:index)
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/helpers/collection.rb:7:in `collection_size'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/helpers/collection.rb:13:in `collection_is_empty?'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/views/pages/index.rb:45:in `items_in_collection?'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/views/pages/index.rb:49:in `build_collection'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/views/pages/index.rb:28:in `block in main_content'
  arbre (1.0.1) lib/arbre/element/builder_methods.rb:31:in `block in build_tag'
  arbre (1.0.1) lib/arbre/context.rb:92:in `with_current_arbre_element'
  arbre (1.0.1) lib/arbre/element/builder_methods.rb:49:in `with_current_arbre_element'
  arbre (1.0.1) lib/arbre/element/builder_methods.rb:26:in `build_tag'
  arbre (1.0.1) lib/arbre/element/builder_methods.rb:39:in `insert_tag'
  arbre (1.0.1) lib/arbre/element/builder_methods.rb:14:in `batch_action_form'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/views/pages/index.rb:36:in `wrap_with_batch_action_form'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/views/pages/index.rb:26:in `main_content'
  /home/vagrant/.rvm/gems/ruby-2.0.0-p481/bundler/gems/active_admin-0e0b3694bace/lib/active_admin/views/pages/base.rb:83:in `block (2 levels) in build_main_content_wrapper'

[...]

I guess draper handles it's delegating of methods to ActiveRecord::Relation by including Draper::Delegation into Draper::CollectionDecorator and that somehow mimicking what this CollectionDecorator does is a key point in getting PORO decorators working?

@amiel

This comment has been minimized.

Show comment
Hide comment
@amiel

amiel Nov 13, 2014

Contributor

Hi, I wrote the original decorator support and have helped maintain it a little, so I thought I'd pipe in to explain the current state and give a few pointers.

History

The original decorator support explicitly did not require draper. The tests showed an example of required methods to implement without draper and it used to be documented in the wiki (theoretically it should be possible to find the old documentation, but I don't see any history).

However, as Draper's collection handling became more complex and it became trickier to deal with all of the collection-level methods that ActiveAdmin uses for pagination, scoping, and filters, we decided to work directly with Draper's collection handling. While I believe that Draper is still not required, it is more integrated now and not tested anymore. This changed in #2662.

Using decorate_with without Draper

@rogerkk We've actually discussed this before, but, as usual, things have changed since then.

As mentioned before, ActiveAdmin uses a large set of the ActiveRecord API for automatic handling of pagination, scoping, and filters. If your PORO decorator delegates by default to it's model, then you should have no issues there, but ActiveAdmin does require a bunch of methods on the collection. For example reorder (from ActiveRecord) and total_count (added to ActiveRecord::Relation by Kaminari).

Draper handles this sort of thing with the collection decorator concept, and ActiveAdmin teaches Draper to delegate the required methods.

I think you should be able to use ActiveAdmin's decorator support as is with POROs instead of Draper, but you will need to explicitly accommodate decorating collections. In particular, your collection decorator needs to respond to each of these methods and return a new decorated collection (as opposed to just delegating through).

I can see two ways to get ActiveAdmin to use your collection decorator.

  1. Make your decorator PORO automatically use the collection decorator when passed a collection.

    class MyDecorator
      def self.decorate(object)
        if is_collection?(object)
          MyCollectionDecorator.decorate object
        else
          new object
        end
      end
    end
  2. Or, override ActiveAdmin::ResourceController::Decorators#collection_decorator.

I'd love to hear about it when you get it working. And it'd be great to have the specifics documented again, maybe at https://github.com/gregbell/active_admin/wiki/Implement-a-custom-decorator. Basically what really needs to be documented is the API that ActiveAdmin requires for decorators.

Good luck, and let me know if you have any other questions :)

Contributor

amiel commented Nov 13, 2014

Hi, I wrote the original decorator support and have helped maintain it a little, so I thought I'd pipe in to explain the current state and give a few pointers.

History

The original decorator support explicitly did not require draper. The tests showed an example of required methods to implement without draper and it used to be documented in the wiki (theoretically it should be possible to find the old documentation, but I don't see any history).

However, as Draper's collection handling became more complex and it became trickier to deal with all of the collection-level methods that ActiveAdmin uses for pagination, scoping, and filters, we decided to work directly with Draper's collection handling. While I believe that Draper is still not required, it is more integrated now and not tested anymore. This changed in #2662.

Using decorate_with without Draper

@rogerkk We've actually discussed this before, but, as usual, things have changed since then.

As mentioned before, ActiveAdmin uses a large set of the ActiveRecord API for automatic handling of pagination, scoping, and filters. If your PORO decorator delegates by default to it's model, then you should have no issues there, but ActiveAdmin does require a bunch of methods on the collection. For example reorder (from ActiveRecord) and total_count (added to ActiveRecord::Relation by Kaminari).

Draper handles this sort of thing with the collection decorator concept, and ActiveAdmin teaches Draper to delegate the required methods.

I think you should be able to use ActiveAdmin's decorator support as is with POROs instead of Draper, but you will need to explicitly accommodate decorating collections. In particular, your collection decorator needs to respond to each of these methods and return a new decorated collection (as opposed to just delegating through).

I can see two ways to get ActiveAdmin to use your collection decorator.

  1. Make your decorator PORO automatically use the collection decorator when passed a collection.

    class MyDecorator
      def self.decorate(object)
        if is_collection?(object)
          MyCollectionDecorator.decorate object
        else
          new object
        end
      end
    end
  2. Or, override ActiveAdmin::ResourceController::Decorators#collection_decorator.

I'd love to hear about it when you get it working. And it'd be great to have the specifics documented again, maybe at https://github.com/gregbell/active_admin/wiki/Implement-a-custom-decorator. Basically what really needs to be documented is the API that ActiveAdmin requires for decorators.

Good luck, and let me know if you have any other questions :)

@timoschilling

This comment has been minimized.

Show comment
Hide comment
@timoschilling

timoschilling Nov 13, 2014

Member

@amiel thanks for all the informations. I don't want to see that in the wiki, we should add that to 11-decorators.md

Member

timoschilling commented Nov 13, 2014

@amiel thanks for all the informations. I don't want to see that in the wiki, we should add that to 11-decorators.md

@amiel

This comment has been minimized.

Show comment
Hide comment
@amiel

amiel Nov 13, 2014

Contributor

@timoschilling sounds good.

@kiote, @rogerkk: let me know what you come up with :)

Contributor

amiel commented Nov 13, 2014

@timoschilling sounds good.

@kiote, @rogerkk: let me know what you come up with :)

kiote added a commit to kiote/aa_plus_poro that referenced this issue Nov 21, 2014

new poro-decorator implemented
  with help of @amniel
  following this comment activeadmin/activeadmin#3600 (comment)
@kiote

This comment has been minimized.

Show comment
Hide comment
@kiote

kiote Nov 21, 2014

Hello, @amiel!

First of all, thank you for such a detailed explanation 👍
Things turned to be more clear after that)

This is the modified version of sample-application: https://github.com/kiote/aa_plus_poro

I've tried to implement the first solution you suggested and now I can decorate all columns with no effort :)

But after following any concrete object url (for example: /admin/articles/1) I got an error saying undefined method columns' for ArticlePresenter:Class`. I think it's the result of my misunderstanding of the delegation model here..

kiote commented Nov 21, 2014

Hello, @amiel!

First of all, thank you for such a detailed explanation 👍
Things turned to be more clear after that)

This is the modified version of sample-application: https://github.com/kiote/aa_plus_poro

I've tried to implement the first solution you suggested and now I can decorate all columns with no effort :)

But after following any concrete object url (for example: /admin/articles/1) I got an error saying undefined method columns' for ArticlePresenter:Class`. I think it's the result of my misunderstanding of the delegation model here..

@amiel

This comment has been minimized.

Show comment
Hide comment
@amiel

amiel Nov 21, 2014

Contributor

@kiote I think my approach would be to look at the backtrace and try to figure out how ActiveAdmin is using columns. I assume this is how ActiveAdmin lists all of the attributes of an instance if you are not specifying the show page.

You may want to write something more generic eventually, but for now, try something like this:

class ArticlePresenter < DelegateClass(Article)
  def self.columns
    Article.columns
  end
end

I'd also be curious, without the columns method above, does it work if you specify the show section in app/admin/article.rb?

ActiveAdmin.register Article do
  decorate_with ArticlePresenter

  permit_params :title

  index do
    column :id
    column :title
    column :hello
    column :link_title
  end

  show do
    attributes_table do
      row :title
    end
  end
end

I think that every NoMethodError you will see is just a part of the ActiveRecord API that ActiveAdmin depends on, and your job is to figure out how to delegate it on appropriately. The work you are doing now will become excellent documentation on the ActiveRecord API surface that ActiveAdmin uses :)

Contributor

amiel commented Nov 21, 2014

@kiote I think my approach would be to look at the backtrace and try to figure out how ActiveAdmin is using columns. I assume this is how ActiveAdmin lists all of the attributes of an instance if you are not specifying the show page.

You may want to write something more generic eventually, but for now, try something like this:

class ArticlePresenter < DelegateClass(Article)
  def self.columns
    Article.columns
  end
end

I'd also be curious, without the columns method above, does it work if you specify the show section in app/admin/article.rb?

ActiveAdmin.register Article do
  decorate_with ArticlePresenter

  permit_params :title

  index do
    column :id
    column :title
    column :hello
    column :link_title
  end

  show do
    attributes_table do
      row :title
    end
  end
end

I think that every NoMethodError you will see is just a part of the ActiveRecord API that ActiveAdmin depends on, and your job is to figure out how to delegate it on appropriately. The work you are doing now will become excellent documentation on the ActiveRecord API surface that ActiveAdmin uses :)

@kiote

This comment has been minimized.

Show comment
Hide comment
@kiote

kiote Nov 24, 2014

Hello, @amiel!

Both suggested methods are working, thanks! I just wanted to be sure I'm on the right way here. I'm really eager to get some stable version and add documentation info about the way of AA+PORO :) Thanks for your inspiration.

kiote commented Nov 24, 2014

Hello, @amiel!

Both suggested methods are working, thanks! I just wanted to be sure I'm on the right way here. I'm really eager to get some stable version and add documentation info about the way of AA+PORO :) Thanks for your inspiration.

@rogerkk

This comment has been minimized.

Show comment
Hide comment
@rogerkk

rogerkk Nov 24, 2014

Contributor

Thanks for the valuable information, @amiel! Indeed, we discussed this earlier, but I never got to prioritize this work.

Seems like @kiote is better suited than me to figure this out. :-)

Contributor

rogerkk commented Nov 24, 2014

Thanks for the valuable information, @amiel! Indeed, we discussed this earlier, but I never got to prioritize this work.

Seems like @kiote is better suited than me to figure this out. :-)

@kiote

This comment has been minimized.

Show comment
Hide comment
@kiote

kiote Dec 8, 2014

I've created a small gem meanwhile: https://github.com/kiote/activeadmin-poro-decorator
Maybe it would help somebody before maintainers will include poro-supporting to Active Admin

kiote commented Dec 8, 2014

I've created a small gem meanwhile: https://github.com/kiote/activeadmin-poro-decorator
Maybe it would help somebody before maintainers will include poro-supporting to Active Admin

@amiel

This comment has been minimized.

Show comment
Hide comment
@amiel

amiel Dec 9, 2014

Contributor

Having a gem to help with this seems great. In fact, I don't really see what ActiveAdmin would do differently to support poro decorators.

Here's how I see it: ActiveRecord exposes a very large API surface. ActiveAdmin uses a large set of the ActiveRecord API. You can use ActiveAdmin's decorator support as long as you make sure that it handles the required API. It is not really possible for ActiveAdmin to set that up for POROs. Doing so is, in essence, writing a decorator library and I don't think that belongs in ActiveAdmin core, especially when we can just include Draper.

I agree with the the article you referenced, but I do think that Draper makes plenty of sense in the context of ActiveAdmin. And I think most of the arguments against Draper in the article are moot when using ActiveAdmin.

If you don't want things like the "fairly dark magic" and "jumps through hoops", then I would suggest not using ActiveAdmin. It is full of such things and depends heavily on other gems that are also full of such things.

That being said, I applaud you for getting ActiveAdmin decorator support to work without Draper and for releasing a gem to make it easier.

Contributor

amiel commented Dec 9, 2014

Having a gem to help with this seems great. In fact, I don't really see what ActiveAdmin would do differently to support poro decorators.

Here's how I see it: ActiveRecord exposes a very large API surface. ActiveAdmin uses a large set of the ActiveRecord API. You can use ActiveAdmin's decorator support as long as you make sure that it handles the required API. It is not really possible for ActiveAdmin to set that up for POROs. Doing so is, in essence, writing a decorator library and I don't think that belongs in ActiveAdmin core, especially when we can just include Draper.

I agree with the the article you referenced, but I do think that Draper makes plenty of sense in the context of ActiveAdmin. And I think most of the arguments against Draper in the article are moot when using ActiveAdmin.

If you don't want things like the "fairly dark magic" and "jumps through hoops", then I would suggest not using ActiveAdmin. It is full of such things and depends heavily on other gems that are also full of such things.

That being said, I applaud you for getting ActiveAdmin decorator support to work without Draper and for releasing a gem to make it easier.

@amiel

This comment has been minimized.

Show comment
Hide comment
@amiel

amiel Dec 9, 2014

Contributor

In terms of resolving this bug, I suggest that this be fixed with documentation. I'll work on a PR. @kiote is it ok to link to your gem in the documentation? @timoschilling, would that be acceptable?

Contributor

amiel commented Dec 9, 2014

In terms of resolving this bug, I suggest that this be fixed with documentation. I'll work on a PR. @kiote is it ok to link to your gem in the documentation? @timoschilling, would that be acceptable?

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