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

Display model's class and id when to_s not defined #70

Closed
wants to merge 5 commits into from

Conversation

c-lliope
Copy link
Contributor

@c-lliope c-lliope commented Sep 1, 2015

https://trello.com/c/9OfUvC3x

Why?

Administrate relied on #to_s to display objects throughout the system,
but in order for it to work correctly developers needed to define #to_s
on each model that they wanted to use with Administrate.

No more.

Now, if a developer doesn't define a custom #to_s on a model,
Administrate will detect that and default to showing the object's class
and database id.

@@ -52,7 +52,7 @@

def find_option(associated_model, field_id)
field = find("#order_" + field_id)
field.find("option", text: associated_model.to_s)
field.find("option", text: "#{associated_model.class.to_s.titleize} ##{associated_model.id}")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [99/80]

https://trello.com/c/9OfUvC3x

Why?

Administrate relied on `#to_s` to display objects throughout the system,
but in order for it to work correctly developers needed to define #to_s
on each model that they wanted to use with Administrate.

No more.

Now, if a developer doesn't define a custom #to_s on a model,
Administrate will detect that and default to showing the object's class
and database id.

TODO:

refactor... especially HasMany and BelongsTo relationships.
@c-lliope
Copy link
Contributor Author

c-lliope commented Sep 2, 2015

This could also be resolved by monkey patching ActiveRecord::Base:

module ActiveRecord
  class Base
    def to_s
      "#{self.class.to_s.titleize} ##{id}"
    end
  end
end

Thoughts on which method is better?

@mike-burns
Copy link
Contributor

If Kernel#to_s isn't what the user wants to see, then maybe we should define a different method. #display_name or #administrate_name or the like?

@c-lliope
Copy link
Contributor Author

c-lliope commented Sep 2, 2015

We might be able to add a method to ...Dashboard classes that presents the resource. Something like:

require "administrate/base_dashboard"

class CustomerDashboard < Administrate::BaseDashboard
  ATTRIBUTE_TYPES = {
    created_at: Field::String,
    # ...
    updated_at: Field::String,
  }

  TABLE_ATTRIBUTES = ATTRIBUTE_TYPES.keys
  SHOW_PAGE_ATTRIBUTES = ATTRIBUTE_TYPES.keys - [:name]
  FORM_ATTRIBUTES = [:name, :email]

  def display_resource(resource)
    "#{resouce.class.to_s.titleize} ##{resource.id}"
  end
end

Thoughts?

@mike-burns
Copy link
Contributor

That method is asking, not telling. The main issue with that is that it requires the objects to expose their instance variables.

I'd be very comfortable seeing this in various bits of code:

def display_resource
  to_s
end

@c-lliope
Copy link
Contributor Author

c-lliope commented Sep 2, 2015

If we used a method like display_resource on the model, developers would have to define the method on objects throughout their system. At that point, I don't see much benefit to using it over using to_s directly.

I think the solution I'm most comfortable with at this point is to tell developers something like:

You will want to define #to_s on any model you want to use with Administrate, because Ruby's default #to_s does not give a consistent string between different page requests.

If you're too lazy to implement #to_s on all of your models yourself, add gem "activerecord_consistent_strings" to your Gemfile. It monkey-patches Activerecord to save you a grand total of three lines of code per class.

...then we'd need to release the activerecord_consistent_strings gem as well.

@mike-burns
Copy link
Contributor

Oh I didn't understand: you want to use AR's #to_s method, and allow users to override it?

if __getobj__.method(:to_s).owner == Kernel
"#{__getobj__.class.to_s.titleize} ##{__getobj__.id}"
else
__getobj__.to_s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on extracting __getobj__ to a private method? Maybe decorated_resource or similar?

@c-lliope
Copy link
Contributor Author

c-lliope commented Sep 2, 2015

@mike-burns if I understand your question, then yep!

When we need to display foo in the UI (where class Foo < ActiveRecord::Base), we're using foo.to_s.

Here's a screenshot that shows how we're using #to_s in the app, and why users need to override it:

image

The decision to use #to_s was originally made back in #5 - it was better than the solution at the time, of specifying a single attribute to use as the title.

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

Successfully merging this pull request may close these issues.

4 participants