348282 - importxml.pl throws Bugzilla::Bug::ValidateTime warnings if the time value is not defined and param timetrackinggroup defined.

Status: RESOLVED FIXED

(Bugzilla :: Bug Import/Export & Moving, defect)

Description

[Max Kanat-Alexander]

importxml.pl calls Bugzilla::Bug::ValidateTime inside of an eval, to avoid dying. That's fine. However, functions still throw warnings right to STDERR inside of an evail, and this one does, in this case.

importxml.pl calls ValidateTime on the three time-tracking fields, but if the exporter wasn't in the timetrackinggroup, ValidateTime will throw the warrning:

Use of uninitialized value in pattern match (m//) at Bugzilla/Bug.pm line 775

Comment 1

[Guzmán Brasó]

I'll accept this bug.
This apply on 2.22 and the tip.
And it does not affect if user is in timetracking group or not.
It apply if param timetrackinggroup is defined and one or more of the three values are undefined in the xml file.

Patch is checking if ($bug_fields{xxxx} ) for remaining_time, and estimated_time. If not defined eval don't run and value is not pushed into @values neither @query.

Then for actual_time I check if defined on eval. Period. Then I check as usual if eval result in error OR for actual_time be undefined. As with actual_time I need to define a valid value in case of undefined or in case of error from eval.

Comment 2

[Guzmán Brasó]

Attachment: Patch 1.0 for 2.22-BRANCH

Comment 3

[Guzmán Brasó]

Attachment: Patch 1.0 for the tip.

Comment 4

[Frédéric Buclin]

Comment on attachment 254103 [details] [diff] [review]
Patch 1.0 for the tip.

>--- importxml.pl	2007-02-06 02:55:54.000000000 +0100
>+        if ( $bug_fields{'estimated_time'} ) {
>+            eval {
>+                Bugzilla::Bug::ValidateTime($bug_fields{'estimated_time'}, "e");
>+            };

0 is a valid time and should not be excluded. The problem is only when times are undefined. So it should be if (defined $bug_fields{'...'}).


>+        if ( $bug_fields{'remaining_time'} ) {

Same here.


>         eval {
>             Bugzilla::Bug::ValidateTime($bug_fields{'actual_time'}, "a");
>+        } if ( $bug_fields{'actual_time'} );
>+        if ($@ || !$bug_fields{'actual_time'} ) {

I prefer a IF block here, because the error message is inaccurate if actual_time is undefined. In this case, it should fallback to 0 without throwing an error. Either that or the error message should mention that we used 0 because no actual_time value was found.


This bug is a perfect illustration where:
1) Bug::ValidateTime() should throw an error if $time is undefined, internally, to avoid the error reported here.
2) error_mode == ERROR_MODE_WARN would avoid all these eval {}.

Comment 5

[Frédéric Buclin]

Comment on attachment 254102 [details] [diff] [review]
Patch 1.0 for 2.22-BRANCH

r- for the same reason.

Comment 6

[Guzmán Brasó]

I thought the same about Bug::ValidateTime(), didn't offer a patch for it as I've never used time tracking features yet so I really don't know any of the consequences of such change and I'm not willing to spend time finding it out through grep :-).

I'm uploading patch for both with the changes you requested.

Comment 7

[Guzmán Brasó]

Attachment: Patch 1.1 for 2.22-BRANCH

Comment 8

[Guzmán Brasó]

Attachment: Patch 1.1 for the tip

Comment 9

[Frédéric Buclin]

(In reply to comment #6)
> I thought the same about Bug::ValidateTime(), didn't offer a patch for it as
> I've never used time tracking features yet so I really don't know any of the
> consequences of such change and I'm not willing to spend time finding it out
> through grep :-).

I checked, it's safe as callers already do this check themselves.

Comment 10

[Frédéric Buclin]

Comment on attachment 254158 [details] [diff] [review]
Patch 1.1 for the tip

>+        else {
>             $bug_fields{'actual_time'} = 0.0;
>+            $err .= "Actual time not comming. Setting to 0.0\n";

Nit: "Actual time not defined" is better than "not comming". To be fixed on checkin.

r=LpSolit

Comment 11

[Frédéric Buclin]

Comment on attachment 254157 [details] [diff] [review]
Patch 1.1 for 2.22-BRANCH

No need to attach a separate backport if it's exactly the same patch as the one for the trunk.

Comment 12

[Frédéric Buclin]

tip:

Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.74; previous revision: 1.73
done

2.22.2:

Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.47.2.9; previous revision: 1.47.2.8
done