Skip to content
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

Don't use #to_s to display models in Administrate. #129

Closed
rikkipitt opened this issue Nov 2, 2015 · 15 comments
Closed

Don't use #to_s to display models in Administrate. #129

rikkipitt opened this issue Nov 2, 2015 · 15 comments
Milestone

Comments

@rikkipitt
Copy link
Contributor

Just a minor note to request the documentation of the use of the to_s method for better representing objects across the various Administrate dashboards. E.g. has_many select lists and belongs_to on index lists. This might catch out some newer Rails/Ruby users if not explicitly mentioned...

Something along the lines of:

to_s is the standard Ruby method for converting an object to a string. You can define to_s on your model class when you want a custom string representation.

Example:

class Tag < ActiveRecord::Base
  def to_s
    name
  end
end

Thus instead of displaying #<Tag:0x007fad07b7ad98>, we can display the actual name of the tag.

Thoughts?

@c-lliope
Copy link
Contributor

c-lliope commented Nov 2, 2015

@rikkipitt good call. We've gone back and forth on to_s a little bit (see #5 and #70), but you're right that it should absolutely be documented.

Where would you expect to see it? I'm thinking of adding a note to the README and to https://administrate-docs.herokuapp.com/getting_started.

@rikkipitt
Copy link
Contributor Author

@Graysonwright thanks for the quick reply. I missed those pull requests, cheers for referencing them.

Albeit a short page, could it warrant its own section under something like "Customising Fields"? It might get lost in the Getting started page...

I've used RailsAdmin extensively and I believe they try :name or :title before falling back to a default with the object id... Looks like this has been mulled over by you guys too.

@c-lliope
Copy link
Contributor

c-lliope commented Nov 3, 2015

Yeah, it's hard to get a decent solution without getting too "magical". 😄

@jcutrell
Copy link

jcutrell commented Nov 3, 2015

I think I'd like to see something like a method to override in the model - administrate_title, possibly - rather than to_s, since other things (like loggers) may rely on to_s.

@c-lliope
Copy link
Contributor

c-lliope commented Nov 3, 2015

@jcutrell I like that idea - it might make it easier to fallback to a sensible default.

It might work well in a helper method, like:

def display_record(record)
  record.try(:administrate_title) || "#{record.class} ##{record.id}"
end

@marisawallace
Copy link

I agree with @jcutrell 's suggestion. Using to_s for Administrate's representation of the model makes me a bit nervous, because there's no way to isolate admin-specific display logic from how the Model is represented elsewhere in the app (in the console, etc.).

I would consider even going a step further, and have users put their administrate_title methods in Administrate-specific views, rather in the Models themselves. This might make it clearer that the display logic is admin-specific.

@rikkipitt
Copy link
Contributor Author

Agreed. We need to make sure things don't get too DSL'ly as that's the first guideline principle of the whole project...

@jcutrell
Copy link

jcutrell commented Nov 6, 2015

Perhaps it's best placed in the dashboard class somehow? Or maybe some kind of presentational class can be generated to accompany the dashboard?

I agree - having an extra method on the model just for display doesn't seem right to me. I think something like a ViewModel (or whatever term you want to use) should wrap the model instance and decorate it with administrate-specific methods that delegate to underlying fallback methods when not defined by the end user.

On Nov 6, 2015, at 10:33 AM, Max Wallace notifications@github.com wrote:

I agree with @jcutrell 's suggestion. Using to_s for Administrate's representation of the model makes me a bit nervous, because there's no way to isolate admin-specific display logic from how the Model is represented elsewhere in the app (in the console, etc.).

I would consider even going a step further, and have users put their administrate_title methods in Administrate-specific views, rather in the Models themselves. This might make it clearer that the display logic is admin-specific.


Reply to this email directly or view it on GitHub.

@c-lliope c-lliope changed the title Documentation request: mention to_s Don't use #to_s to display models in Administrate. Nov 7, 2015
@c-lliope
Copy link
Contributor

c-lliope commented Nov 7, 2015

@jcutrell I think at the moment the Dashboard class is the best place for this. If we find ourselves adding a lot of view-specific methods in there, we can consider extracting a decorator layer.

I'm working on a PR now.

@c-lliope
Copy link
Contributor

c-lliope commented Nov 8, 2015

#191 introduces a fix for this issue - please give feedback there!

@c-lliope c-lliope modified the milestone: 0.1.1 Nov 8, 2015
@lucianosousa
Copy link
Contributor

Why are you guys not using the to_param rails method?
http://api.rubyonrails.org/classes/ActiveModel/Conversion.html#method-i-to_param

@marisawallace
Copy link

@lucianosousa I think the difference here is that we're looking to customize how the model identifier is displayed for the sake of an admin user (who may be non-technical), rather than display a serialized form of the model's primary key.

Admins (or at least the ones I'm developing for) don't care about IDs-- they want to see something more descriptive, and what that means changes based on the domain.

@lucianosousa
Copy link
Contributor

@maxkwallace I always - or most of the cases - assume the same URL in admin and regular pages.
I - most of the times - override the to_param just using the name/title variable, using ID's just un rescue cases.
eg:

class User < ActiveRecord::Model

  def to_param
    if name
      name.parameterize
    else
      id
    end
  end

end

I mean, there is none complex stuff in this issue. It's better use some default method to to this than generate a lot of complex code for different models/types

@c-lliope
Copy link
Contributor

c-lliope commented Nov 9, 2015

@lucianosousa - I think we're talking about different things. This PR isn't related to URLs at all. We're changing how labels like these get generated in the app:

edit_everardo_hudson___administrate_prototype

I think you might be talking about the URLs, which are being generated by the to_param method (see this code).

monopoly___administrate_prototype

@lucianosousa
Copy link
Contributor

hum, I see.

my fault

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants