252159 - centralize time validation

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Alexandre Michetti Manduca]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a2) Gecko/20040612 Firefox/0.8.0+
Build Identifier: 

maybe is a good idea to centralize the validation of time fields such as worked
time or estimated time in a function

Reproducible: Always
Steps to Reproduce:

Comment 1

[Christian Reis]

Yeah, there's code at least in two places in process_bug.cgi -- it's easy to
find it, just grep for 99999.99 -- agh :-P

Comment 2

[Alexandre Michetti Manduca]

Attachment: implements validateTime to centralize time validation

Comment 3

[Christian Reis]

Comment on attachment 153804 [details] [diff] [review]
implements validateTime to centralize time validation

>Index: post_bug.cgi
Bugzilla::Bug::validateTime($est_time,'estimated_time');

[nit] space after the comma

>Index: process_bug.cgi
Bugzilla::Bug::validateTime($er_time,$field);

[ditto]

>Index: Bugzilla/Bug.pm
> 1;
>+
>+sub validateTime{
>+  my ($time,$field) = @_;
>+  if ($time > 99999.99 || $time < 0 || !($time =~ /^(?:\d+(?:\.\d*)?|\.\d+)$/)){
>+      ThrowUserError("need_positive_number",{field => "$field"});
>+  }
>+}

This function should be named ValidateTime (it's not a	method, so it uses this
weird casing that EmitDependList does, I guess). It should also appear *before*
the 1; up there -- right before AUTOLOAD, to be precise.

Comment 4

[Alexandre Michetti Manduca]

Attachment: patch 1 with modifications

Comment 5

[Alexandre Michetti Manduca]

Attachment: no tabs

Comment 6

[Christian Reis]

Comment on attachment 153812 [details] [diff] [review]
no tabs

>Index: process_bug.cgi
>+    Bugzilla::Bug::validateTime($::FORM{'work_time'},'work_time');

Please test and read short patches before attaching, you forgot to update this
call :-)

Comment 7

[Alexandre Michetti Manduca]

Attachment: again

Comment 8

[Christian Reis]

Comment on attachment 153839 [details] [diff] [review]
again

>Index: process_bug.cgi
>+    Bugzilla::Bug::validateTime($::FORM{'work_time'}, 
'work_time');

No, not the space after the comma (or that too), but the method name:
ValidateTime, not validateTime.

Comment 9

[Alexandre Michetti Manduca]

Attachment: ...

Comment 10

[Christian Reis]

Comment on attachment 153900 [details] [diff] [review]
...

Looks good!

Comment 11

[Christian Reis]

/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.90; previous revision: 1.89
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.208; previous revision: 1.207
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.38; previous revision: 1.37

Thanks!

Comment 12

[Christian Reis]

Note that the patch checked in included an "abort" to the ThrowUserError call,
which was missing in any case from the original code -- when validating work
time the table lock is held.