Skip to content

Commit

Permalink
Don't show unpersisted has_one associations
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
rwehresmann committed Nov 2, 2020
1 parent 4a261c1 commit dab4454
Show file tree
Hide file tree
Showing 7 changed files with 67 additions and 10 deletions.
4 changes: 3 additions & 1 deletion app/views/fields/has_one/_index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@ By default, the relationship is rendered as a link to the associated object.
[1]: http://www.rubydoc.info/gems/administrate/Administrate/Field/HasOne
%>

<% if field.data %>
<% if field.linkable? %>
<%= link_to(
field.display_associated_resource,
[namespace, field.data],
) %>
<% else %>
<%= field.display_associated_resource %>
<% end %>
4 changes: 3 additions & 1 deletion app/views/fields/has_one/_show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ All show page attributes of has_one relationship would be rendered
[1]: http://www.rubydoc.info/gems/administrate/Administrate/Field/HasOne
%>

<% if field.data %>
<% if field.linkable? %>
<fieldset class="attribute--nested">
<legend>
<%= link_to(
Expand All @@ -37,4 +37,6 @@ All show page attributes of has_one relationship would be rendered
</div>
<% end -%>
</fieldset>
<% else %>
<%= field.display_associated_resource %>
<% end %>
4 changes: 3 additions & 1 deletion lib/administrate/base_dashboard.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ def collection_attributes
end

def display_resource(resource)
"#{resource.class} ##{resource.id}"
return "#{resource.class} ##{resource.id}" if resource.try(:persisted?)

"Unpersisted #{resource.class}"
end

def collection_includes
Expand Down
4 changes: 4 additions & 0 deletions lib/administrate/field/has_one.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ def nested_show
)
end

def linkable?
data.try(:persisted?)
end

private

def resolver
Expand Down
9 changes: 7 additions & 2 deletions spec/administrate/views/fields/has_one/_index_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,18 @@
describe "fields/has_one/_index", type: :view do
context "without an associated record" do
it "displays nothing" do
has_one = double(data: nil)
has_one = Administrate::Field::HasOne.new(
:product_meta_tag,
build(:product_meta_tag),
:index,
)

render(
partial: "fields/has_one/index.html.erb",
locals: { field: has_one },
)

expect(rendered.strip).to eq("")
expect(rendered.strip).to eq("Unpersisted ProductMetaTag")
end
end

Expand All @@ -21,6 +25,7 @@
has_one = instance_double(
"Administrate::Field::HasOne",
data: product,
linkable?: true,
display_associated_resource: product.name,
)

Expand Down
11 changes: 6 additions & 5 deletions spec/administrate/views/fields/has_one/_show_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,18 @@
describe "fields/has_one/_show", type: :view do
context "without an associated record" do
it "displays nothing" do
has_one = instance_double(
"Administrate::Field::HasOne",
display_associated_resource: "",
data: nil,
has_one = Administrate::Field::HasOne.new(
:product_meta_tag,
build(:product_meta_tag),
:show,
)

render(
partial: "fields/has_one/show.html.erb",
locals: { field: has_one },
)

expect(rendered.strip).to eq("")
expect(rendered.strip).to eq("Unpersisted ProductMetaTag")
end
end

Expand All @@ -27,6 +27,7 @@
"Administrate::Field::HasOne",
display_associated_resource: product.name,
data: product,
linkable?: true,
nested_show: nested_show,
)

Expand Down
41 changes: 41 additions & 0 deletions spec/lib/fields/has_one_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,45 @@
expect(path).to eq("/fields/has_one/#{page}")
end
end

describe "#linkable?" do
context "when data is persisted" do
it "shows it" do
product_meta_tag = create(:product_meta_tag)
field = described_class.new(
:product_meta_tag,
product_meta_tag,
:show,
)

expect(field.linkable?).to be_truthy
end
end

context "when data isn't persisted" do
it "doesn't shows it" do
product_meta_tag = build(:product_meta_tag)
field = described_class.new(
:product_meta_tag,
product_meta_tag,
:show,
)

expect(field.linkable?).to be_falsey
end
end

context "when data doesn't respond to `persisted?`" do
it "doesn't allows to show" do
product_meta_tag = double(:product_meta_tag)
field = described_class.new(
:product_meta_tag,
product_meta_tag,
:show,
)

expect(field.linkable?).to be_falsey
end
end
end
end

0 comments on commit dab4454

Please sign in to comment.