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

[faucct]

Yes, and I have stated it at the start of this issue. I am pretty sure that this definition is reasonable and won't be changed inside draper. Are you suggesting that this should be fixed this by putting another guard similar to resource.method(method).source_location? Is not my solution simpler and more reliant though it removes some undocumented and untested behaviour?

[timoschilling]

Just define a display_name method on your model or decorator and every thing is fine

[timoschilling]

It is a bug in draper, he delegates a method which didn't exist on the decorated object

[faucct]

Object#to_s exists in every object thanks to Kernel#to_s.

[timoschilling]

Yes but for that we have the resource.method(method).source_location check

[timoschilling]

Why you didn't define a display_name method?

[faucct]

Well, the check is not perfect and this issue is about it. Do you think we should improve the check, for example if the method is a delegate and the delegated to method does not have a source location?
I did define a display_name method calling DISPLAY_NAME_FALLBACK as soon the issue has popped, but should not this be fixed inside AA?

[timoschilling]

Should we really fix a bug of draper in AA?

But we can improve the method detection in AA of course. But we should do that in a way that don't depend on a decorator implementation.

[faucct]

Is it a bug that draper does not override Object#method delegating to the decorated object, so we could check if it is a kernel method?
I have suggested a way we could do this for this specific case of Draper::Decorator, but I believe that there are people who use different decorator libraries and it won't work for them, so it is not reliable and will have to be improved forever.

[timoschilling]

The problem is that we can check that to_s is overwritten, but we can't check how has overwritten it. That's way the solution is to define your own display_name method. Yes it's not elegant that everyone needs to do that, but AA can't fix 3 party problems, otherwise we will end up with code like this:

          if reource.is?(Draper::Decorator)
            DISPLAY_NAME_FALLBACK
          elsif reource.is?(Foo::Decorator)
            ...
          elsif reource.is?(Bar::Decorator)
            ...
          elsif method != :to_s || resource.method(method).source_location
            method
          else
            DISPLAY_NAME_FALLBACK
          end

[faucct]

We can fix the issue by accepting my PR. The only people affected will be ones relying on to_s methods defined in their ActiveRecord models to be displayed in activeadmin. Instead it will display "Post 13" which is not as scary as "#<Post:0x0123456789>" is. All they will have to do to bring this back is rename the method to display_name or give it the alias if it is used anywhere else.

[timoschilling]

Many people have models like this:

class User
  def name
    "#{firstname} #{lastname}"
  end

  def to_s
    name
  end
end

For that we have the to_s in display_name_methods, not for #<Post:0x0123456789>. I assert that more people overwrite to_s then using a decorator.

[timoschilling]

What you want is solving your problem by moving the same problem to other people.

[faucct]

The behaviour of display_name with the User model does not change as name is still in display_name_methods.
Anyway, I am tired of proving my point. Are you okay with this change?

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
    end || DISPLAY_NAME_FALLBACK
  end
end

Now the display_name_method_for does not expect resource to respond to all methods, so it is possible for me to remove :to_s from display_name_methods. The existing behaviour you are talking about does not change.

[deivid-rodriguez]

@timoschilling Is the last proposal acceptable if it comes with a test ensuring that the existing behavior you talked about is not broken?

[javierjulio]

This is something I think we should revisit. We've run into this issue by just implementing Draper decorators. Everything seems to work fine with that integration across many models but this is the one issue that comes up. The problem becomes we would now have to implement a custom display name across many decorators when all we want is the default behavior. I don't know how easy it would be to do here.

[deivid-rodriguez]

@javierjulio Did you try @faucct patches, specifically the last one?

[javierjulio]

Yes but the result isn't the same. For example, with a multi word named model I noticed the case was different after removing :to_s and with or without the patch (capitalized vs. titlecased).

To be clear, just only removing :to_s from the config.display_name_methods list, without any patch, does resolve the issue.

Regarding the patch though, I'm not familiar with the use of the || after the if statement. I wasn't sure how that would be different result wise from the original implementation.

[deivid-rodriguez]

I see...

Regarding the patch though, I'm not familiar with the use of the || after the if statement. I wasn't sure how that would be different result wise from the original implementation.

I'm not sure either 🤔

I'll review this in more depth.