271276 - Cannot enter negative time for 'Hours Worked'

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Shane H. W. Travis]

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

[Dave Miller [:justdave]]

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.

Comment 2

[Shane H. W. Travis]

Attachment: Code patch for tip

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.

Comment 3

[Shane H. W. Travis]

Attachment: Code patch for tip, take 2

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

Comment 4

[Frédéric Buclin]

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

Comment 5

[Shane H. W. Travis]

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 6

[Frédéric Buclin]

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 7

[Frédéric Buclin]

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.

Comment 8

[Frédéric Buclin]

(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.

Comment 9

[Shane H. W. Travis]

Attachment: Code patch for tip, take 3

Fixed as per comments above, and IRC discussion.

Comment 10

[Frédéric Buclin]

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

r=LpSolit

Comment 11

[Shane H. W. Travis]

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

Comment 12

[Shane H. W. Travis]

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.

Comment 13

[Shane H. W. Travis]

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