`display_name_method_for` works not as intended with Draper

[faucct]

This is the breadcrumb I get while using Draper decorator with delegate_all option:

ActiveAdmin can not look up the display method name correctly here:

      def display_name_method_for(resource)
        @@display_name_methods_cache ||= {}
        @@display_name_methods_cache[resource.class] ||= begin
          methods = active_admin_application.display_name_methods - association_methods_for(resource)
          method  = methods.detect{ |method| resource.respond_to? method }

          if method != :to_s || resource.method(method).source_location
            method
          else
            DISPLAY_NAME_FALLBACK
          end
        end
      end

My decorator instance does not respond to any other than to_s methods, as the model does. In this case it is supposed to use the fallback, but the problem is that decorator to_s method got a source location: ["~/.rvm/gems/ruby-2.1.5/gems/draper-2.1.0/lib/draper/delegation.rb", 10].

[Fivell]

@faucct , what is your suggestion ? ActiveAdmin provides display_name_methods option for you and you are able configure it in your app and you're able to define any method from this list in your decorator.

[faucct]

I would suggest removing :to_s from display_name_methods as it is already present in DISPLAY_NAME_FALLBACK. Do you mind me PR this change?

[Fivell]

@faucct , looks like yes, :to_s can be removed from display_name_methods , we have specs that it uses to_s after all 40e42b3

[timoschilling]

Why didn't you simply solve your problem by overriding to_s on your decorator or using display_name. You can use display_name as alias to to_s on your ApplicationRecord.

[faucct]

Are you okay with everyone using decorators solving the problem on their own by copying the code from DISPLAY_NAME_FALLBACK?

[timoschilling]

Are you ok with breaking existing behavior?

There are many simple ways to solve your problem without copying code or breaking AA.

[faucct]

Are you ok with breaking existing behavior?

Any change is about breaking existing behaviour. Right now I am breaking existing behaviour of DisplayHelper rendering Object#to_s, e.g. "#<Post:0x0123456789>", for decorators. The "existing behaviour" you are talking about is not covered with tests, so yes, I am ok with breaking this. It is partially covered by these tests, but they would still work for m == :to_s.
https://github.com/faucct/activeadmin/blob/b1eee7ce1f17ebc6ac51fdbf7e89cb59599c2bff/spec/unit/view_helpers/display_helper_spec.rb#L26

There are many simple ways to solve your problem without copying code or breaking AA.

Are you talking about using the DISPLAY_NAME_FALLBACK in my code? This is the way I have dealt with that issue once it popped but I still don't think this is okay to let the AA user deal with this. If this is okay, then you should probably add this solution to the Using Decorators docs.

[timoschilling]

I'm talking about define a method on your model or decorator. Your decorator breaks the from AR expected behavior by overwriting AR#to_s

[faucct]

I am not sure if I have understood you here. Are you suggesting me defining a method on a model/decorator to fix the problem or are you explaining to me why the problem is arising? Probably the first one since the problem arises even with no methods defined in the decorator, but I have already answered that it seems weird for an activeadmin user to deal with this.

[timoschilling]

Both

[faucct]

But my decorator does not override anything: it is just an empty decorator with delegates_all as you can see in the added tests, these would fail with a decorator you have in Decorators docs.

[timoschilling]

Draper defines a to_s method in each decorator. https://github.com/drapergem/draper/blob/master/lib/draper/decorator.rb#L206