607909 - Hours worked / work_time is marked as changing when commenting even when you don't enter a value

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[LegNeato]

Attachment: Check for the string '0.00', do nothing if that's what's set

If you are a timetracker and you comment in a bug, an entry in the history db gets created saying you worked 0.00 hours. Also, the value is included in bugmail.

Attached is a patch that fixes this.

Comment 1

[Frédéric Buclin]

Ah, I planned to file this bug some days ago, but I forgot. Fortunately you are here. :)

This bug only affects Bugzilla 4.1, right? 4.0 is not affected?

Comment 2

[LegNeato]

I haven't tried 4.0. I'm pretty sure it is fallout from bug 590334, which I believe is 4.1 only.

Comment 3

[Frédéric Buclin]

Yeah, it's 4.2 only, so that's probably the bug you are pointing to. Now, I don't think this is the right fix. You should IMO rather fix _check_time() to treat the passed time as an number and not as a string. Perl sees 0.0, 0.00 etc... as strings. To do an implicit conversion into a number, you just have to add this line in the validator:

$time += 0;

Comment 4

[LegNeato]

That won't work I think.

"The DECIMAL type is an unpacked floating-point number type. NUMERIC is an alias for DECIMAL. It behaves like a CHAR column;"

So every time we pull it from the database it'll be a string (I think). Fun.

Comment 5

[Max Kanat-Alexander]

Yes, and you must treat it as a string, because otherwise you will get an IEEE  double, which may not have the same value as the decimal.

However, LpSolit is right that for the "there is no value here" check, adding 0 would be a good way to go. So you'd just do a "return undef if $value + 0 == 0" or something like that.

Comment 6

[LegNeato]

Attachment: Check if work_time is zero before adding to the activity table

Ok, this should do it I think.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 486773 [details] [diff] [review]
Check if work_time is zero before adding to the activity table

Hmm, how about doing it in the validator instead? That is, if the value is 0, return a literal integer 0 instead of 0.00? That could simplify logic elsewhere, too.

Comment 8

[LegNeato]

Because I stuck it everywhere I could and none of it made any difference except for the fix in the attached patch. I put the code in every getter/setter/validator function in Bug.pm, Comment.pm, and Object.pm I could see and it didn't fix it. Maybe I missed the magic one that would do it :-)

Comment 9

[Frédéric Buclin]

(In reply to comment #3)
> $time += 0;

write this in Object::check_time(). This should do it.

Comment 10

[LegNeato]

It doesn't from my testing.

Comment 11

[Frédéric Buclin]

Comment on attachment 486574 [details] [diff] [review]
Check for the string '0.00', do nothing if that's what's set

>     foreach my $comment (@{$self->{added_comments} || []}) {
>         $comment = Bugzilla::Comment->insert_create_data($comment);
>-        if ($comment->{work_time}) {
>+        if ($comment->{work_time} && $comment->{work_time} ne '0.00') {

OK, this was actually the right fix (modulo a minor change):

 foreach my $comment (@{$self->{added_comments} || []}) {

At this point, $comment is a hash, where $comment->{work_time} contains 0.

 $comment = Bugzilla::Comment->insert_create_data($comment);

Here, insert_create_data() returns an object, meaning that $comment is no longer a hash but an object.

 if ($comment->{work_time}) {

$comment->{work_time} now contains a number in the d.dd format, where d is a digit. But 0.00 is seen as true by Perl (it sees a string here, not a number), which explains why 0.00 is constantly added to the bug history.

So the problem has nothing to do with validators, but with $comment->{work_time} being always of the form d.dd.

To avoid unwanted side-effects, we shoul limit our changes to this if () check. As $comment is an object here, you should call $comment->work_time instead of its internal value, and because work_time() can never be undefined (the DB column has NOTNULL), you can simply write:

if ($comment->work_time ne '0.00') {

or

if ($comment->work_time + 0 != 0) {

I think the 2nd form is safer in case we one day change the format of longdescs.work_time to another number of digits.

Comment 12

[Frédéric Buclin]

Comment on attachment 486773 [details] [diff] [review]
Check if work_time is zero before adding to the activity table

This is actually the right fix, modulo a minor change, see my previous comment. r=LpSolit

Comment 13

[Max Kanat-Alexander]

Comment on attachment 486773 [details] [diff] [review]
Check if work_time is zero before adding to the activity table

Okay, I have a better idea.

First, you should be using the accessor here, not accessing the hash.

Now, the accessor should return a literal "0" int if and only if the value is equivalent to 0.

Comment 14

[Frédéric Buclin]

(In reply to comment #13)
> Now, the accessor should return a literal "0" int if and only if the value is
> equivalent to 0.

Christian, do you want to do it or should I?

Comment 15

[LegNeato]

Attachment: Check in the accessor for 0, use the accessor in Bug.pm

Comment 16

[Frédéric Buclin]

Comment on attachment 487691 [details] [diff] [review]
Check in the accessor for 0, use the accessor in Bug.pm

Works fine. Thanks! r=LpSolit

Now that $comment->work_time correctly returns 0 when not set, it would be cool to also remove hacks introduced in bug/show.xml.tmpl line 77 and bug/comments.html.tmpl lines 106 and 168. I will file a separate bug so that we can fix this regression now.

Comment 17

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Bug.pm
modified Bugzilla/Comment.pm
Committed revision 7594.