Closed Bug 271276 Opened 20 years ago Closed 20 years ago

Cannot enter negative time for 'Hours Worked'

Categories

(Bugzilla :: Bugzilla-General, defect)

2.18
defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: shane.h.w.travis, Assigned: shane.h.w.travis)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: justdave → travis
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Bugzilla 2.22
Attached patch Code patch for tip (obsolete) — Splinter Review
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.
Attachment #173210 - Flags: review?(LpSolit)
Attached patch Code patch for tip, take 2 (obsolete) — Splinter Review
Helps if you do a *unified* diff... d'oh!
Attachment #173210 - Attachment is obsolete: true
Attachment #173212 - Flags: review?(LpSolit)
Attachment #173210 - Flags: review?(LpSolit)
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
Attachment #173212 - Flags: review?(LpSolit) → review+
Status: NEW → ASSIGNED
Flags: approval?
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
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.
Flags: approval2.18?
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.
Attachment #173212 - Flags: review+ → review-
Flags: approval?
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.
Flags: approval2.18?
(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.
Attachment #173212 - Attachment is obsolete: true
Attachment #173743 - Flags: review?(LpSolit)
Comment on attachment 173743 [details] [diff] [review] Code patch for tip, take 3 r=LpSolit
Attachment #173743 - Flags: review?(LpSolit) → review+
Flags: approval?
Flags: approval2.18?
Flags: approval? → approval+
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
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Severity: normal → major
Keywords: relnote
Whiteboard: Checked in on 2.20: awaiting approval for 2.18
Flags: approval2.18? → approval2.18+
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
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
Target Milestone: Bugzilla 2.20 → Bugzilla 2.18
Whiteboard: Checked in on 2.20: awaiting approval for 2.18
Keywords: relnote
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: