Don't show unpersisted has_one associations

[rwehresmann]

Showing unpersisted has_one associations can be misleading and break the application is some cases.

Let's consider the following example:

class Product < ApplicationRecord
  has_one :product_meta_tag

  def product_meta_tag
    super || build_product_meta_tag
  end
end

That opens for a scenario where a product.product_meta_tag will return a non-persisted instance of a ProductMetaTag. In the _show partial we're just checking if field.data (i.e., ProductMetaTag instance) exists to display it, and a non-persisted instance would be displayed. The link displayed in the partial (link_to( field.display_associated_resource, [namespace, field.data]) would be pointing to the index page of product meta tags, what is not desired. Also, in the absence of an index route, the page would break.

The solution: just check if data is persisted to display it or not.

[pablobm]

Thank you for your PR! On one hand, I agree that we shouldn't be displaying links to unpersisted resources. On the other hand, I wonder if the problem here is not so much with Administrate as with the underlying model, forcing an unpersisted association and confusing the code that enters in contact with it.

I'd like a second opinion on this one. Perhaps @nickcharlton or @c4lliope have thoughts?

[rwehresmann]

@pablobm I understand your point. From what I saw until now in rails projects is quite common to find has_one relationships described in the model like in my example. However, that doesn't mean it's the best approach and I'm also not sure if this is enough reason to merge the PR (I'll keep doing these rewritings in my projects, though). In any case, I updated the commit with the test names you suggested to keep.

[pablobm]

@rwehresmann - Can you also solve Hound's lint warnings, please?

[c4lliope]

Can we add a check like...

<%= @product.product_meta_tag.persisted? ? link_to(@product.product_meta_tag) : @product.product_meta_tag.name %>

So the link is only rendered for persisted models, and otherwise we only display plain text on the page?

Nice catch here, this seems like a use case that I wouldn't want to break peoples' applications over.

[rwehresmann]

@pablobm PR updated with the corrections.

@c4lliope I'm thinking if it's worthy. We actually don't have how to control the name of the model fields (we don't have the warranty that there is a name field, for instance), so the alternative would be to print the model instance as a string, what I think it isn't visually useful.

What we're showing now for unpersisted models:

What we would be showing printing as a string:

[c4lliope]

Could you access product_meta_tag_dashboard.display_resource(...) ?
...[as described here])

administrate/spec/dashboards/customer_dashboard_spec.rb

Lines 70 to 79 in b85abcf

	describe "#display_resource" do
	it "returns the customer's name" do
	customer = double(name: "Example Customer")
	dashboard = CustomerDashboard.new
	rendered_resource = dashboard.display_resource(customer)
	expect(rendered_resource).to eq(customer.name)
	end
	end

).

Or, i believe, there is dashboard.display_associated_resource(), doing similar.

[c4lliope]

Ah, here;
Administrate::Field::HasOne < Administrate::Field::Associative
and ::Associative has...

administrate/lib/administrate/field/associative.rb

Lines 6 to 8 in 844c270

	def display_associated_resource
	associated_dashboard.display_resource(data)
	end

[rwehresmann]

@c4lliope Oh, I see what you mean. I checked and display_resource called inside display_associated_resource is implemented in Administrate::BaseDashboard as

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

The output for the situation we're treating in this PR would be ProductMetaTag #, but if I haven't a product_meta_tag as super || build_product_meta_tag, it would be nil and break when trying to access resource.id.

I could change the display_resource implementation to avoid this, of course. Do you think it's worth it?

[c4lliope]

Ooh, good digging. Hm. Hmmmm. Changing `display_resource` wouldn't be a *breaking* change, but it would be a visible change on peoples' dashboards. I'm curious what our options would be for making that change; I chose `"#{resource.class} ##{resource.id}"` as a sensible default for persisted records; I'm not sure a similar sensible default is possible for unpersisted. Unless, maybe; ```ruby def display_resource(resource) resource.persisted? ? "#{resource.class} ##{resource.id}" : "Unpersisted #{resource.class}" end ```

[c4lliope]

Sorry, I guess email breaks comment formatting.

[rwehresmann]

@c4lliope I still think that from a user perspective display nothing at all is the best option. It's possible that they don't even understand what is "persistence" in the context of the application. We could use "unrecord" for instance, but it seems redundant to me. However, if we should display something, I think your suggestion is the way to go.

I updated the PR with the described solution.

Just to record: I'm calling always try(:persisted?). I never used another ORM then ActiveRecord, and I'm not sure if Administrate works just fine with another. In any case, if the ORM doesn't implement persisted? these alterations will not break the application, we'll just display text instead. If that becomes a problem, it'll be easy to change it to make it work with another ORM.

[c4lliope]

Go ahead and display a blank, I think that is a fine solution also.

…

On Sun, Nov 1, 2020, 9:09 PM Rodrigo Walter Ehresmann < ***@***.***> wrote: @c4lliope <https://github.com/c4lliope> I still think that from a user perspective display nothing at all is the best option. It's possible that they don't even understand what is "persistence" in the context of the application. We could use "unrecord" for instance, but it seems redundant to me. However, if we should display something, I think your suggestion is the way to go. I updated the PR with the described solution. Just to record: I'm calling always try(:persisted?). I never used another ORM then ActiveRecord, and I'm not sure if Administrate works just fine with another. In any case, if the ORM doesn't implement persisted? these alterations will not break the application, we'll just display text instead. If that becomes a problem, it'll be easy to change it to make it work with another ORM. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1794 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHILHKTGWWWCKWFW6EFBASDSNYIEFANCNFSM4SNVJTLQ> .

[nickcharlton]

We don't need to worry about other ORMs, thankfully, so persisted? is fine!

[rwehresmann]

Sorry for the delay on this.

@c4lliope done: display a link if persisted, or blank if not.

@nickcharlton great. Thanks for the tips!