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

:if_blank on association does not allow for false values #72

Closed
stellard opened this issue Sep 26, 2014 · 7 comments
Closed

:if_blank on association does not allow for false values #72

stellard opened this issue Sep 26, 2014 · 7 comments

Comments

@stellard
Copy link

Given the following example:

        <%= s.association :price_updates, :if_blank => nil do |update| %>
          <% if update %>
            <li><%= update.new_price %></li>
          <% else %>
            No Updates
          <% end %>
        <% end %>

blank_value will never return the false value

    def blank_value(options)
      options.delete(:if_blank) || translate_blank_html
    end

this forces you to check in the collection block what kind of object is being passed through.

  <% if update.respond_to? :new_price %>

should blank value be something more like?

    def blank_value(options)
      options.has_key?(:if_blank) ? options.delete(:if_blank) : translate_blank_html
    end
@carlosantoniodasilva
Copy link
Member

Do you want to show a different value for each association if blank? I am not sure I followed what you are trying to do.

If you just want to show a message in case no price updates exist, you should be able to use if_blank or even a conditional wrapping the show for call.

If not, please explain it further. I'm giving it a close for now, please feel free to comment back and we will gladly reopen if necessary. Thanks!

@stellard
Copy link
Author

stellard commented Jan 3, 2015

What I am trying to accomplish is to set the if_blank value to be nil. I am trying to use the behaviour you are speaking about but it does not work when the if_blank value is set to nil (or anything false)

This is not possible due to the logic in the blank_value function.

    #current implementation
    def blank_value(options)
      #options = { if_blank: nil }
      options.delete(:if_blank) || translate_blank_html
    end

So even though I have supply an if_blank value it acts as though I have not and uses the default translate_blank_html instead. I would not expect the explicitly assigned value to be ignored.

The blank value method should check if if_blank was passed through and use that regardless of whether that value is false

    #suggested implementation
    def blank_value(options)
      options.has_key?(:if_blank) ? options.delete(:if_blank) : translate_blank_html
    end

@carlosantoniodasilva
Copy link
Member

I understand the case of setting it to nil, I do not understand exactly why, or what you're trying to accomplish with that. What's the use case? What is the expected result? What do you actually have to do (think of it as without Show For for example).

My understanding so far is that it could be solved either this way:

<%= s.association :price_updates, :if_blank => 'No Updates' do |update| %>
  <li><%= update.new_price %></li>
<% end %>

or this way (which should be pretty much the same output):

<%# @object is the object you pass to show for %>
<% if @object.price_updates.present? %>
  <%= s.association :price_updates do |update| %>
    <li><%= update.new_price %></li>
  <% end %>
<% else %>
  No Updates
<% end %>

Can you please clarify how your intent is different from what I understood?

@stellard
Copy link
Author

stellard commented Jan 3, 2015

I know this is not the only way to accomplish this however I thought that the behaviour of ignoring the explicitly set if_blank value was a bug.

On your first example:

<%= s.association :price_updates, :if_blank => 'No Updates' do |update| %>
  <li><%= update.new_price %></li>
<% end %>

When price_updates is blank, update inside the block is equal to 'No Updates' and results in the error:

`undefined method `new_price' for "No Updates":String`

Its confusing because inside the block you either get an instance of a price update or a string. Forcing a type check on the object. I would prefer to check that an object is there or not rather than its type.

<%= s.association :price_updates, :if_blank => 'No Updates' do |update| %>
  <li><%= update.new_price if update.respond_to? :new_price %></li>
<% end %>

This is fine in some cases but it gets even more confusing when your if_blank value is ignored as well.

On your second example. This would work for the scenario I described, but again, the reason I raised the issue wasn't really because it was impossible to implement this. It was just that I thought the behaviour was not expected.

I tried to simplify the example because I didn't think the use case of nil for blank was relevant but as an example. Something like this.

  <%= s.association :price_updates, :if_blank => nil do |update| %>
    <% if update %>
      <li><%= link_to "Prices on #{update.valid_from}", prices_on_date_path(update.valid_from) %></li>
    <% else %>
      <li>No price updates <%= link_to "Create one", new_price_updates_path %></li>
    <% end %>
  <% end %>

Cheers.

@carlosantoniodasilva
Copy link
Member

Hm I see.. The point is that it seems we should not be yielding to the block in case the association is blank, we should just return the if_blank content, so the first scenario would not fail, and you would not need to write it as the last one I believe.

I'll reopen and try to see what's going on now. Thanks.

carlosantoniodasilva added a commit that referenced this issue Jan 3, 2015
Tries to simulate what's being described in #72, but the test passes.
@carlosantoniodasilva
Copy link
Member

@stellard I've tried to simulate your use case with the example I've showed before on these tests: 00f9981, but they pass, nothing is yielded to the block.

If you could provide a failing case on our test suite, or an application showing the issue, I'd be happy to try to dig deeper, otherwise I'm considering it as closed. Thanks!

@stellard
Copy link
Author

stellard commented Jan 3, 2015

Ok no problem. Ill have a look.
-- 
Scott Ellard

On 3 January 2015 at 13:24:35, Carlos Antonio da Silva (notifications@github.com) wrote:

@stellard I've tried to simulate your use case with the example I've showed before on these tests: 00f9981, but they pass, nothing is yielded to the block.

If you could provide a failing case on our test suite, or an application showing the issue, I'd be happy to try to dig deeper, otherwise I'm considering it as closed. Thanks!


Reply to this email directly or view it on GitHub.

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

No branches or pull requests

2 participants