time_select not interpreting the multi-parameter correctly

At first I "blamed" an I8n compatiblity problem (wrong format interpeted) and was deeply looking into it, but khagimoto seems to be right. I have been using prompt for "please select" text for hours/minutes with the select_time and it kept giving me problems when selecting values above 12 minutes (also rather at random)... any value on :prompt different then true seems to break the code.

I confirm this is a bug. I'm working on a failing test and a deeper description of the problem(s). Just a couple of hours ...

If you use time_select with the :ignore_date => true option, you will get this error. The patch i've added adds the failing test. The error received is "1 error(s) on assignment of multiparameter attributes". You will NOT get an error if the chosen minute is between 0-12. time select needs a default date, without the date, the Hour, Minute, Second become Year, Month, Day. So the Hour shifts to year, minute shifts to month (so if you have it bigger than 12 you get an error and if its less than 12 you don't get an error), seconds shifts to day (if you seconds greater than 30/31 you will also see an error).

Simply put, i think the documentation and the time_select function needs to be modified to not accept :ignore_date => true as an option.

Please see test "test_time_select_without_date_hidden_fields" in actionpack/test/template/date_helper_test.rb which tests for creation of fields without the date bits.

Once we are convinced about this. I'll submit another patch which will really fix the documentation, code, test etc.

Aditya can you provide a patch please?.
And this is for 3.0 I guess right?

I think this is a problem in 2.3.X also and yes I will provide a patch for that too.

Can someone highlight the use case for ":ignore_date => true" when using time_select by the way?

No point removing the option if there really is a valid use case for it.

This method will also generate 3 input hidden tags, for the actual year, month and day unless the option :ignore_date is set to true.

http://api.rubyonrails.org/classes/ActionView/Helpers/DateHelper.ht...

Thanks Rohit, I read the documentation but it doesn't say WHY we would need the :ignore_date field.

But the gumshoe that i am, the use-case for :ignore_date was found in this ticket more than 2 years ago.

https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets...

The requirement stated in this ticket is dubious to say the least but some how got approved. Emilio wants to use the time_select and date_select on the same field for design reasons. Why dont they just use datetime_select, i dont know?

I would love to hear from the core committers on their viewpoint before i proceed.

If nothing else gets fixed, at the very least the documentation should be update to ensure that if :ignore_date is used on time_select, a date_select on the same field is mandatory. Sounds silly to me anyway.

datetime_select will return a set of selects. What Emilio had asked for is the ability to have the same select tags in different locations. At least that's how I see it. I guess it's best if you wait for some direction on how to proceed here.

Anyways keep up the good work! :)

Aditya I think more coverage would be better for the user: a use case for :ignore_date, why is another helper needed at all, and why one wouldn't use a different helper in the first place instead of that pair.

Xavier, I'm actually at a total loss about the use case for this. Apparently it was introduced 2 years ago for design reasons in ticket #503 Add ignore_date option to date helpers so Rails doesn't hidden field tags.

Did you mean more coverage in tests or documentation?

Frankly, i believe the use case is invalid and ideally we should remove this functionality of :ignore_date => true.

Documentation coverage. This is an unclear option, in such cases it is good to explain why it is useful to the end-user. The motivation for #503 Add ignore_date option to date helpers so Rails doesn't hidden field tags seems a good use-case. The explanation could briefly cover why the helper is generating hidden fields at all, which is the role of the date part in all this.

Xavier, why would one not use datetime_select instead for this? I've updated the documentation with more conviction this time :)

Awesome Andrea,

Actually i'd been working on the exactly the same thing as you but abandoned that line as i thought it might not be way to go.

Anyway, i'm attaching my patch (with tests) for review as well. This patch applies and runs on master branch though.

Aditya: here is my current patch (not in git format-patch yet), as you can see it's pretty similar in principle to yours :)

I want to go over it once more, because I realized there still is a failure case:

params = {"time_test"=>{"sometime(4i)"=>"2009", "sometime(6i)"=>"12"}}

I'm not sure this can happen using our helpers, but I'd rather be extra careful. My current thinking is to simply disallow missing pieces except in the last positions (of course empty ones would still be allowed).

My preference is to give nils for missing components between 1 and the maximum provided key param to the class and let the class initializer handle or raise an error, except for date/time for which we default the date bits. For example, Time can handle a nil minute/hour.

Good point. I'll give that I try.

Andrea, My patch takes care of filling in the missing bits with nil.

Your patch makes sense if we want to implicitly accept a missing part of a multiparameter and interpret it just the same as if it where empty.

In other words, should these two cases behave the same?

  def test_multiparameter_attributes_on_time_with_raise_on_small_time_if_missing_date_parts
    attributes = {
      "written_on(4i)" => "16", "written_on(5i)" => "12", "written_on(6i)" => "02"
    }
    topic = Topic.find(1)
    topic.attributes = attributes
    assert_equal Time.local(1, 1, 1, 16, 12, 2), topic.written_on
  end


  def test_multiparameter_attributes_on_time_with_seconds_will_ignore_date_if_empty
    attributes = {
      "written_on(1i)" => "", "written_on(2i)" => "", "written_on(3i)" => "",
      "written_on(4i)" => "16", "written_on(5i)" => "12", "written_on(6i)" => "02"
    }
    topic = Topic.find(1)
    topic.attributes = attributes
    assert_equal Time.local(1, 1, 1, 16, 12, 02), topic.written_on
  end

Note that this is not specific to times and dates, the same can be argued of:

  def test_multiparameter_assignment_of_aggregation_with_missing_values
    customer = Customer.new
    address = Address.new("The Street", "The City", "The Country")
    attributes = { "address(2)" => address.city, "address(3)" => address.country }
    customer.attributes = attributes
    assert_equal Address.new(nil, "The City", "The Country"), customer.address
  end

  def test_multiparameter_assignment_of_aggregation_with_missing_values
    customer = Customer.new
    address = Address.new("The Street", "The City", "The Country")
    attributes = { "address(1)" => "", "address(2)" => address.city, "address(3)" => address.country }
    customer.attributes = attributes
    assert_equal Address.new(nil, "The City", "The Country"), customer.address
  end

I would say no, because in the latter the developer has explicitly told us "I know there is a Date portion of the attribute, and I want to overwrite it" whereas in the former we don't know for sure, it may be an oversight.

That's why my proposal is to instead raise if any part if missing, except at the end.

That said, since you've provided a patch I'm not going to object too strongly; I'll let someone else chime in.

However, I do have a few comments:

• defaulting to 2001 feels totally arbitrary, especially given that the current code for Date detaults to year 1;
• you definitely need more tests; I will attach mine (which your diff passes), and we may still need more;
• this method is already unreadable, it badly needs some refactoring; your code introduces a few new issues (please don't take this badly, nothing personal, I'm just known to be picky):
  • there's no need to call to_a on a range before iterating, this is good enough: set_values = (1..values_with_empty_parameters.keys.max.to_i).collect do |position|
  • you seem to have missed a couple of conversions from @values@ to @set_values@, in the @rescue@ blocks:

  • in fact, @values@ looks unused now except for the initial if, I would rewrite it to be more efficient:

    if values_with_empty_parameters.values.any? { |v| !v.nil? }
      send(name + "=", nil)
    else
    

  • adding a @#to_i@ to @find_parameter_position@ would let you remove the @#to_i@ / @#to_s@ when you fill @set_values@

I left this at the end because I couldn't think of a solution (in fact, it affected the patch I was working on, too), but with your patch it's quite trivial to perform a denial of service attack by making Ruby run out of memory:

attributes = { "address(100000000)" => "" } # add enough 0

That's a non-trivial problem that we would be introducing, and I think it needs to be addressed--if nothing else, by capping the position to 100 or so.

I'm attaching my additional tests (applied on top of your changes), see if they are any use and feel free to change them and integrate them in your final diff.
I'll be glad to review it and upvote it once the last concerns are addressed.

Edited by Rohit Arondekar for formatting.

Always humbled and glad that i'm doing more open source now. Your feedback is constructive, and i appreciate your attention to detail.

 Time.local(1,1,1,1,1,1) => Mon Jan 01 01:01:01 0530 2001

on my system. I think this depends on system architecture. I didn't look deep enough earlier and incorrectly defaulted to 2001.

I think the tests too have to be written so we're correctly checking for raised asserts, otherwise we'll get false positives.

Now this is how i've refactored it

• If its a Date parameter, missing values are defaulted to 1

• If its a Time parameter

  • Error raised if NO date parameters were provided.
  • If date parmeters were provided but were blank, they're defaulted to 1.
  • If time parameters were not provided, they are defaulted to 0. (This is done because hour does not default to 0 if the year is 1, something quirky in how Time handles it and how DateTime.civil handles it).

• For other klasses

  • If there are any missing parameters, an error is raised
  • If the parameter is provided but is blank, it is passed to the initializer of that klass.
  • A maximum of 100 parameters can be passed to the initializer.

Have another review of it Andrew. Thanks heaps.

+1 to this version, thanks for cleaning it up :)

You should probably rebase the two commit together, just for clarity, but that's just me. Nice work regardless.

Re: git, you can easily do that with (assuming these two are the only commits on this branch):

git rebase -i HEAD^^

and then changing the second commit from keep to @f@.

Re: milestone change, how did you do that? I thought adding a tag of milestone:3.x would do the trick (there's always something to learn :D ).

Adding rebased squashed patch.

(PS. I can change the milestone because i recently became a member of the Rails LH team. As pseudo-admins, there are extra input elements and edit links).

+1 from me :)

@fxn, what do you think? would appreciate your views when you get a chance.

Automatic cleanup of spam.

Automatic cleanup of spam.

Rebased against master (but secretly and gently bumping).

Forewarning: will bump again in 2 weeks if nothing happens in the meantime. :)

Im using this time_select in my rails 3 project and I am facing the same problem.

Till the patch is committed to mainline, How do i fix it ?

Here is how I fixed it in my form for now. Hope this helps.

https://gist.github.com/715819

@lakshmanan your usage of :ignore_date => false seems just fine.

Just wondering about the rationale for saying that I have to provide the date bits for time? Why do I have to send the Year, Month and Date if all I care about is the Hour and Minute?

It just seems harsh to complain if date bits aren't given when I can use helpers to generate a form that won't have any date bits, (e.g. time_select(...., :include_date => false)). Especially if I'm coupling this to a time field where I do not care about date portions, I only have to because there's no Duration class in the stdlib to let me represent time periods without artificially saddling them to a date.

Obviously, I don't think it's a huge problem, I'd rather this patch went in than didn't. Just curious.

@fxn, this time i'm bumping this after 1 month. Kindly have a look!

Hi all! I'm kind of new but found out that using a prompt or :include_blank => true to the time_select triggers the same multi-parameter error. I find this buggy cause if the time_select is optional in my form, it shouldn't default to 00:00 (which is a valid time). I have an events form that may or maynot have a start time, and I always get a valid start time of 00:00. Is this the correct way to use the time_select?

Is this patch going to address this too? I would gladly help to test it.

I just wanted to clearify that sometimes it triggers the multipart error and sometimes it just saves 00:00 to the db. Can't find out the logic of it yet

@fcarrettoni, I encourage you to take this patch, apply it in your rails app and give it a go. I think we've covered almost all the cases this time. You acceptance and testing will help. If there are still issues left, we'll be keen to know and fix them. If you think the problem is solved, your upvoting will be welcome. From your last message it's not clear to me in what repeatable use case do you see this problem.

I stumbled on this bug today and it was a real pain to figure out. As I see it you can't use the :prompt option because it'll cause the 1i, 2i, and 3i (year, month, day) hidden field values to go blank, thus resulting in an invalid date error from Date.civil.

Seriously: This should be fixed in the next release. Someone even took the time to make a patch for it!

Aditya, maybe you should try submitting a pull request to the repo on GitHub. Perhaps your patch will get due attention there.

@fxn i know it's been a while. I'll rebase and reapply the patch, but can i please kindly urge you to look at this ticket again?

@Aditya, yes please rebase. I have a few open tickets that I plan to work on when I finish the extraction of Prototype and RJS.

@Aditya did you finally rebase? The diff does not apply clean right now.