Closed
Bug 271276
Opened 20 years ago
Closed 20 years ago
Cannot enter negative time for 'Hours Worked'
Categories
(Bugzilla :: Bugzilla-General, defect)
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)
4.97 KB,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•20 years ago
|
||
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
Assignee | ||
Comment 2•20 years ago
|
||
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)
Assignee | ||
Comment 3•20 years ago
|
||
Helps if you do a *unified* diff... d'oh!
Assignee | ||
Updated•20 years ago
|
Attachment #173210 -
Attachment is obsolete: true
Attachment #173212 -
Flags: review?(LpSolit)
Assignee | ||
Updated•20 years ago
|
Attachment #173210 -
Flags: review?(LpSolit)
Comment 4•20 years ago
|
||
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+
Updated•20 years ago
|
Status: NEW → ASSIGNED
Flags: approval?
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
Assignee | ||
Comment 5•20 years ago
|
||
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 6•20 years ago
|
||
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-
Updated•20 years ago
|
Flags: approval?
Comment 7•20 years ago
|
||
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.
Updated•20 years ago
|
Flags: approval2.18?
Comment 8•20 years ago
|
||
(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.
Assignee | ||
Comment 9•20 years ago
|
||
Fixed as per comments above, and IRC discussion.
Attachment #173212 -
Attachment is obsolete: true
Attachment #173743 -
Flags: review?(LpSolit)
Comment 10•20 years ago
|
||
Comment on attachment 173743 [details] [diff] [review]
Code patch for tip, take 3
r=LpSolit
Attachment #173743 -
Flags: review?(LpSolit) → review+
Updated•20 years ago
|
Flags: approval?
Assignee | ||
Updated•20 years ago
|
Flags: approval2.18?
Updated•20 years ago
|
Flags: approval? → approval+
Assignee | ||
Comment 11•20 years ago
|
||
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
Assignee | ||
Comment 12•20 years ago
|
||
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 → ---
Assignee | ||
Updated•20 years ago
|
Whiteboard: Checked in on 2.20: awaiting approval for 2.18
Updated•20 years ago
|
Flags: approval2.18? → approval2.18+
Assignee | ||
Comment 13•20 years ago
|
||
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 ago → 20 years ago
Resolution: --- → FIXED
Target Milestone: Bugzilla 2.20 → Bugzilla 2.18
Assignee | ||
Updated•20 years ago
|
Whiteboard: Checked in on 2.20: awaiting approval for 2.18
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•