According to bug 24789, Patch v.3 has the following notation: "allow negative numbers for "hours worked" to remove time if entered incorrectly." For me, this does not work; entering a negative number brings up the "need_positive_number" error. Because of this, there is no way to back out of a data entry error in Hours Worked. Comments in globals::AppendComment seem to indicate that this is supposed to be allowed, but the problem is these lines in process_bug.cgi =============================== # Validate all timetracking fields foreach my $field ("estimated_time", "work_time", "remaining_time") { if (defined $::FORM{$field}) { my $er_time = trim($::FORM{$field}); if ($er_time ne $::FORM{'dontchange'}) { Bugzilla::Bug::ValidateTime($er_time, $field); } } } =============================== The ValidateTime routine is the one throwing the error. AppendComment has its own check to ensure that work_time contains a number, although it doesn't limit it to 99999.99 like ValidateTime does, so that's bad. Also bad (IMHO) is doing range/value checking in the middle of a subroutine where nobody would look for it. Assuming that this is indeed a bug and not 'working as intended', proper solution seems to be to create Bug::ValidateWorkTime which is the same as ValidateTime save that it does not require numbers to be positive. Call that before calling AppendComment, and take the range/value checking out of the latter subroutine. I have no idea if Jeff Hedlund is still active or not; if he is not, and if this is confirmed, please assign it to me and I'll fix it as outlined above.

Sounds logical to me. I seem to recall some argument about this at some point, but I don't know where, and unless Jeff says otherwise, the comments do seem to indicate that it was the original intent to allow that.

Cleans up some sloppy code that wasn't doing what it thought it was doing. (What it *was* doing, it was doing in the wrong place.) Also makes the errors more generic, in the hopes that someone else may reuse them some day.

Helps if you do a *unified* diff... d'oh!

Comment on attachment 173212 [details] [diff] [review] Code patch for tip, take 2 >+ # Only the "work_time" field is allowed to contain a negative value. >+ if ( ($time < 0) && ($field ne "work_time") ) { >+ ThrowUserError("number_too_small", >+ {field => "$field", num => "$time", min_num => "0"}, >+ "abort"); >+ } Nit: I would prefer min_num to be a variable: my $min_num = 0; if ( ($time < $min_num) && ($field ne "work_time") ) { ThrowUserError("number_too_small", {field => "$field", num => "$time", min_num => $min_num}, "abort"); } >+ if ($time > 99999.99) { >+ ThrowUserError("number_too_large", >+ {field => "$field", num => "$time", max_num => "99999.99"}, >+ "abort"); >+ } Nit: Same thing here, using $max_num = 99999.99; These are minor points, so r=LpSolit

In my opinion, this is a bug in the original implementation of E|A|R time, so I'm requesting 2.18 approval as well. If we're not doing bugfixes on anything other than the tip, then feel free to deny it and it won't break my heart.

Comment on attachment 173212 [details] [diff] [review] Code patch for tip, take 2 oops... Removing my r+ ! It seems that "use Bugzilla::Bug;" is required in globals.cgi in order to use ValidateTime() correctly. I got a software error when adding a new attachment without that line added.

Comment on attachment 173212 [details] [diff] [review] Code patch for tip, take 2 > sub AppendComment { > my ($bugid, $who, $comment, $isprivate, $timestamp, $work_time) = @_; > $work_time ||= 0; >- >+ Bugzilla::Bug::ValidateTime($work_time, "work_time"); OK, I have it: you need to declare "use Bugzilla::Bug;" before using subroutines from Bug.pm. process_bug.cgi (when editing the timetracking table) does not complain because it already calls Bug.pm via "use Bugzilla::Bug;", but attachment.cgi does not call that file. As AppendComment() is the single subroutine to use a function from Bug.pm, the best thing to do is to add "use Bugzilla::Bug;" in AppendComment() so that this .pm file is not loaded everytime globals.cgi is called.

(In reply to comment #7) travis, does it make sense to call ValidateTime() only if $work_time != 0 ? if ($work_time) { use Bugzilla::Bug; Bugzilla::Bug::ValidateTime($work_time, "work_time"); } This way, we don't need to call it everytime AppendComment() is called.

Fixed as per comments above, and IRC discussion.

Comment on attachment 173743 [details] [diff] [review] Code patch for tip, take 3 r=LpSolit

Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.302; previous revision: 1.301 done Checking in Bugzilla/Bug.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v <-- Bug.pm new revision: 1.54; previous revision: 1.53 done Checking in template/en/default/global/user-error.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user- error.html.tmpl,v <-- user-error.html.tmpl new revision: 1.86; previous revision: 1.85 done

I would still like to have approval for 2.18 on this... or at least have it explicitly denied. I forgot that I had requested it when I checked in the tip patch.

Checked in on 2.18. Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.270.2.6; previous revision: 1.270.2.5 done Checking in Bugzilla/Bug.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v <-- Bug.pm new revision: 1.37.2.3; previous revision: 1.37.2.2 done Checking in template/en/default/global/user-error.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user- error.html.tmpl,v <-- user-error.html.tmpl new revision: 1.61.2.10; previous revision: 1.61.2.9 done