When decoration fails, show original class

[pascalbetz]

Description

Thanks for the work on Draper to all the maintainers 👍

This provides a fix for #819

When decorating an object without decorator, then the last class which was checked for a decorator is contained in the error message. In case of Rails this is most often ActiveRecord::Base.
This is a bit misleading and it would be better if the class which was actually tried to decorate is shown in the error message.
This is caused by the recursive nature of the decorate method which moves up the hierarchy when no decorator class found. And raises when it can not move further up.

Before:

Something::Bar.decorate
Draper::UninferrableDecoratorError: Could not infer a decorator for ActiveRecord::Base.

After:

Something::Bar.decorate
Draper::UninferrableDecoratorError: Could not infer a decorator for Something::Bar.

Testing

Outline steps to test your changes.

• Run the specs, tests have been added

Alternatively:

• on a subclass of AR, which does not have a decorator, call #decorate
• Check that the name of the subclass is contained in the error message and not ActiveRecord::Base

(The fix is not restricted to AR)

References

• Misleading Error Message: Could not infer a decorator for ActiveRecord::Base. #819

[codebycliff]

@pascalbetz Thanks for the contribution! I am not crazy about a public(ish) method taking a parameter that is only relevant to draper's internals. However, I can't think of a better way to accomplish this and it definitely solves a longstanding (and very annoying) problem.

Can you rebase with master before I merge? There has been some changes to this method in master as well and want to make sure the build passes. After that, I think we are good to go.

[pascalbetz]

@codebycliff I understand.
This was the easiest way to solve the problem... Alternatively we could change from recursion to iteration, this would get rid of the parameter. WDYT?

[codebycliff]

@pascalbetz It looks like however you merged, it brought in master changes to this pull request... Can you try again with rebase?

As far as the solution goes, I'm not sure. I'm not super opposed to this solution, however, I am interested in what the iteration solution looks like. Especially now that decorator_class returns nil if can't be found. It might make the iteration easier.

[pascalbetz]

@codebycliff sorry for messing up the rebase (thought I can easily do this while eating, watching a movie and solve some SQL stuff I'm working on. Looks like I'm not as smart as I thought ;-) ). Will clean this up when you decide to merge the recursive solution instead of the iterative solution at #823

[codebycliff]

@pascalbetz I think we'll go with the recursive solution for now. It's much simpler to read and understand. Plus, decorator_class isn't truly public API as it's not documented. I might want to re-visit the iterative solution at some point, but let's move forward with this for now. If you can clean up the commit history we can get this merged.

[pascalbetz]

@codebycliff PR should be cleaner now.

Here is the iterative solution for documentation purpose:

def decorator_class
        current = self
        decorator_name_constant = nil

        while !decorator_name_constant && current do
          prefix = current.respond_to?(:model_name) ? current.model_name : current.name
          decorator_name = "#{prefix}Decorator"
          decorator_name_constant = decorator_name.safe_constantize
          if !decorator_name_constant && current.superclass.respond_to?(:decorator_class)
            current = current.superclass
          else
            current = nil
          end
        end

        if !decorator_name_constant
          raise Draper::UninferrableDecoratorError.new(self)
        end

        decorator_name_constant
end

[codebycliff]

@pascalbetz Thanks again! Sorry it took me so long to get it merged.

[pascalbetz]

No problem at all. Thanks for your work on Draper. I use it in various projects.