Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

f.check_box do not work inverting the checked and unchecked value #3995

Closed
sobrinho opened this Issue Dec 15, 2011 · 18 comments

Comments

Projects
None yet
3 participants
Contributor

sobrinho commented Dec 15, 2011

Hello,

I want to invert the logic of _destroy attribute on a specific nested form and I found a bug.

See this input:

f.check_box :_destroy, {}, false, true

I need this:

  • If check box is checked, keep the record on database
  • If check box is unchecked, destroy the record

The issue happens on this piece:

def check_box_checked?(value, checked_value)
  case value
  when TrueClass, FalseClass
    value
  when NilClass
    false
  when Integer
    value != 0
  when String
    value == checked_value
  when Array
    value.include?(checked_value)
  else
    value.to_i != 0
  end
end

Note if the value is a boolean value, this code do not compare with checked value, just returns the value.

The same will happens if nil means checked, or 0 means checked.

Also, the else seems strange for me.

Why not compare with checked_value on all cases?

I didn't tested but this seems more obvious:

def check_box_checked?(value, checked_value)
  if value.is_a?(Array)
    value.include?(checked_value)
  else
    value == checked_value
  end
end

What you think?

Member

sikachu commented Dec 15, 2011

Did you look at the test case why it was implemented that way?

Contributor

sobrinho commented Dec 15, 2011

No, I haven't.

Contributor

sobrinho commented Feb 20, 2012

any news about that?

Contributor

sobrinho commented Mar 4, 2012

I don't know who is responsible by action view :(

@tenderlove are you?

I would to know from someone if I can submit a pull request fixing this issue like I said on first message.

If someone from core team agree, I will submit the pull request :)

@ghost ghost assigned rafaelfranca Apr 28, 2012

Owner

rafaelfranca commented Apr 28, 2012

I'll check this later. If you want to know something about Action View you can ping me or @spastorino.

Contributor

sobrinho commented Apr 29, 2012

@rafaelfranca what you think about that?

def check_box_checked?(value, checked_value)
  if value.is_a?(Array)
    value.include?(checked_value)
  else
    value == checked_value
  end
end

This seems extremely obvious to this method but something strange is happened here:

def check_box_checked?(value, checked_value)
  case value
  # what if I want to invert the logic?
  when TrueClass, FalseClass
    value
  # what this means? nil value can't be a checked value?
  # this could be used to invert the logic too.
  when NilClass
    false
  # what about if checked_value is 0?
  # this could be used to invert the logic too.
  when Integer
    value != 0
  # what about value is not a string?
  # maybe BigDecimal for example, or maybe a hash.
  when String
    value == checked_value
  # **this** is the only case that makes sense
  when Array
    value.include?(checked_value)
  # what about checked_value?
  # what about values that is not boolean, nil, integer, string or array?
  # if i'm using some hash (serialized attributes for example)
  else
    value.to_i != 0
  end
end

Maybe I'm missing something yet :/

Owner

rafaelfranca commented Apr 29, 2012

@sobrinho I need to investigate it better. I'll do today. But your version seems reasonable.

Owner

rafaelfranca commented Apr 29, 2012

@sobrinho checked_value by default is "1". This is the way your method doesn't work. I'm trying to fix your issue but without success til now.

Owner

rafaelfranca commented Apr 29, 2012

About your question:

  1. Boolean with inverted logic [fixed]
  2. nil can't be a valid checked value, since it was not submitted.
  3. Integer with inverted logic [fixed]
  4. BigDecimal [fixed]
  5. Other that these types I think that are not valid.
Contributor

sobrinho commented Apr 30, 2012

@rafaelfranca that doesn't affect me yet but someone may want to use nil value as checked, why not?

Owner

rafaelfranca commented Apr 30, 2012

Why nil is not a valid checked value. If you pass nil as value the Rails will not generate the value attribute

Contributor

sobrinho commented Apr 30, 2012

@rafaelfranca my first proposal handles all cases, reduces the complexity and still handles nil values :)

Owner

rafaelfranca commented Apr 30, 2012

But it doesn't work.

Contributor

sobrinho commented Apr 30, 2012

@rafaelfranca sorry for spam :P

But why re-convert boolean values using !! and why use to_i for unknown values?

Again, that doesn't affect me, but float values aren't handled too :P

Owner

rafaelfranca commented Apr 30, 2012

But. If you want, please try to use it without breaking the current behavior, and open a pull request.

Owner

rafaelfranca commented Apr 30, 2012

why @checked_value can be a string, or number, or something else, either if the value is a boolean. We can remove the to_i in the unknown values and raise an exception, but I prefer try to convert it to integer.

Owner

rafaelfranca commented Apr 30, 2012

Good, point about the float values. I think that we can change it to be handled too.

Thanks

Contributor

sobrinho commented Apr 30, 2012

@rafaelfranca seems like the problem starts on lib/action_view/helpers/form_helper.rb:917 where checked_value and unchecked_value defaults to "1" and "0".

My proposal do not works because we need to handle "1" to true and "0" to false.

If we change the default values to true and false, that implementation will works:

def checked?(value)                                                                                                                         
  if value.is_a?(Array)
    value.include?(@checked_value)
  else
    value == @checked_value
  end
end

This simplifies the code but changes the public api and maybe reflect on active record (I'm not sure if active record handles "true" as true like "1" as true).

I'm not feeling good about this implementation in rails but I can't do anything right now (time and knowledge about action pack and active record).

This requires a lot of effort but I'm not sure this will help on framework.

Thanks @rafaelfranca :)

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