use current module's namespace for decorates_association

[ivandenysov]

Description

I have the same issue as author of the issue #809: Our app has two set of decorators: top level decorators and Admin::* decorators. Right now I have to explicitly specify :with option for every decorates_association call inside my admin decorators. This PR helps to simplify the process. All associations will be decorated using decorators from the same namespace as current decorator.

Bonus:

It is now possible to decorate object with namespaced decorator using object.decorate(namespace: "Admin" method instead of using Admin::ObjectDecorator.new(object) approach which is harder to automate.

I don't like my current implementation too much. All that passing of :namespace option from DecoratedAssociation#initialize deep into Decoratable.decorator_classlooks too complicated . It would be easier to build namespaced decorator class name inside DecoratedAssociation#initialize and set it as value of :with option (if not present). This approach would look a lot cleaner but from architectural point of view it seems wrong because it is not DecoratedAssociation's responsibility to build decorator class names.

Testing

You can use any rails project that has has_many->belongs_to relation to test this feature. Let's assume that you have User model that has_many :comments. Sounds a bit too familiar, eh? 😄

# app/decorators/admin/user_decorator.rb
class Admin::UserDecorator < Draper::Decorator
  decorates_association :comments
end

# app/decorators/admin/comment_decorator.rb
class Admin::CommentDecorator < Draper::Decorator
  decorates_association :user
end

comment = Comment.first.decorate(namespace: "Admin")
decorated_comment.class # => Admin::CommentDecorator

decorated_user = decorated_comment.user
decorated_user.class # => Admin::UserDecorator

decorated_comment = decorated_user.comments.first
decorated_comment.class # => Admin::CommentDecorator

To-Dos

• Check if we need to document this feature in README
• I think that there's not enough tests for this feature. Does anyone have any advice on what else to cover with unit tests?

References

Issue #809

[codebycliff]

@john-denisov Thanks for this! Sorry it took me so long to reply. I'll take a look as soon as I get a chance.

[n-rodriguez]

Hi there! Thanks for your PR! This is the missing piece of Draper 👍

But I would see a more general usage than just association or 'Admin' section.

The fact that namespaced decorators are contained within a Ruby module has IMHO a semantic meaning and bring more information to the developer.

I've implemented your fork in my application and the new decorators are a lot cleaner than before :

app/decorators/
├── admin
├── concerns
│   ├── have_attributes
│   └── have_helpers
├── emails
├── french_site
└── royce

(only directories are represented here)

There's a bunch of decorators in admin and french_site and before your PR, they were all located in the root of app/decorators, and all my decorators looked like bags of prefixed methods and blocks of comments to explain that this method is for the french_site, this method is for the main_app, this method is for the Admin section of the main_app, and so on... (this a multi domain Rails app).

This PR brings a good way to respect separation of concerns.

On my side I don't use decorates_association but more decorate on collections like :

# In main app :
Post.all.decorate
# In admin section on main app :
Post.all.decorate(namespace: 'Admin')
# In french site :
Post.all.decorate(namespace: 'FrenchSite')

I've made a patch on your fork to make it works. The patch is not finished yet but it works and my code is a lot cleaner now 👍

[ivandenysov]

@n-rodriguez I'm glad that this feature helped you even before it is merged 👍 😄

[n-rodriguez]

@n-rodriguez I'm glad that this feature helped you even before it is merged +1 smile

yeah... it's kind of risky so I put it in a branch, waiting for this PR to be merged :)

But it's still a great improvement 👍 I can't work on this before 2 weeks, but I will take a look to improve tests and API.

[n-rodriguez]

Hi there! Any news?

[codebycliff]

Sorry I haven't gotten around to reviewing this fully yet. It is on my radar, I have just been really busy lately. I won't have time for a little bit still, but I just wanted to comment to give an update.

[ivandenysov]

@codebycliff Hi. Is there anything I can do to help merge this PR

[codebycliff]

I apologize as I've had no time lately. I did run into a few issues when trying this out on one of our bigger projects, but I never got around to posting the details. I will try to dig them up in the next few weeks.

[n-rodriguez]

Hi there! Any news?

[codebycliff]

I apologize for just now getting a response to you. Things have been pretty hectic. The primary issue I'm seeing currently is that Decorator.object_class is now broken. For example, given a model called Comment and two decorators CommentDecorator and Admin::CommentDecorator:

>> Admin::CommentDecorator.object_class

#=> Draper::UninferrableObjectError: Could not infer an object for Admin::ClientDecorator

This class method should be able to infer the model name correctly. I see this become an issue when combined with other libraries that try to do inference. Here is an example of an issue I see in one of our applications when using pundit and the same model scenario as above:

In the controller, we have:

@comment = Comment.first.decorate(namespace: 'Admin')

and in the view:

<% if policy(@comment).destroy? %>
  <%= link_to ... %>
<% end %>

this blows up with the same Draper::UninferrableObjectError because it can't infer the correct model name, to then in turn infer the correct policy class (e.g. CommentPolicy). We will have to find a solution for this before we can look at merging this feature. I know the original authors wanted to avoid more inference / magic being added to the library, but with that being said, I do see your use case. If you want to take a stab at fixing this issue, I'll give it another look. Let me know if you have questions or anything doesn't make sense.

[n-rodriguez]

this blows up with the same Draper::UninferrableObjectError because it can't infer the correct model name, to then in turn infer the correct policy class (e.g. CommentPolicy)

Hi there! Thanks for your answer ;) I ran into the same issue and I solved it by specifying the model explicitly :

module Admin
  class AlarmDecorator < Admin::ApplicationDecorator
    decorates :alarm
  end
end

It sounds redundant but it works.

[ivandenysov]

@codebycliff Thanks for feedback. Will take a look soon

[codebycliff]

@n-rodriguez While that works, I feel it's not super intuitive. I think it should be possible to get this feature to work without having to do that. If either one of you want to take a stab at that, that would be great.

Either way, I think we should document this feature in the README so people know about it. If we absolutely have to use the workaround above, we can document that as well.

[n-rodriguez]

But it's still a great improvement +1 I can't work on this before 2 weeks, but I will take a look to improve tests and API.

Actually it was 2 years... but yeah I finally solved the Decorator.object_class. Do I open a new PR with all the changeset?

@ivan-denysov @codebycliff

[n-rodriguez]

ping @codebycliff I'd like to finish this work. But #897 needs to be merged before.

[n-rodriguez]

@ivan-denysov can you please rebase your branch on master?

[n-rodriguez]

FYI I've made a fork that implements this feature : https://github.com/jbox-web/draper

[Alexander-Senko]

I think I'll have time to review this soon.

[Alexander-Senko]

I've already reviewed the suggestion in #899 as it looked like that PR obsoletes this one.