252151 - Implement timetracking bar in attachment screens (new and edit)

Status: NEW

(Bugzilla :: Attachments & Requests, enhancement)

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: 

I add the worked time field in new bug, new attachment and edit attachment.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Alexandre Michetti Manduca]

Attachment: Implements the worked time field in other places

Comment 2

[Christian Reis]

Comment on attachment 153689 [details] [diff] [review]
Implements the worked time field in other places


Thanks for the patch! Some comments follow:

>Index: attachment.cgi
>+  if ( UserInGroup(Param('timetrackinggroup')) ) {
>+      AppendComment($::FORM{'bugid'}, 
>+                    $::COOKIE{"Bugzilla_login"},
>+                    $comment,
>+                    $isprivate,
>+                    ,"",
>+                    $::FORM{'work_time'});
>+     SendSQL("UPDATE bugs SET remaining_time = (remaining_time - " . SqlQuote($::FORM{'work_time'}) . ") WHERE bug_id = " . $::FORM{'bugid'} . ";"); 

It would be nice if this could be set in an UpdateRemainingTime function (maybe
in Bug.pm as a function right now)


>@@ -1151,9 +1163,16 @@ sub update
>     my $neverused = $::userid;
> 
>     # Append the comment to the list of comments in the database.
>-    AppendComment($bugid, $who, $wrappedcomment, $::FORM{'isprivate'}, $timestamp);
>-
>+    if ( UserInGroup(Param('timetrackinggroup')) ) {
>+        AppendComment($bugid, $who, $wrappedcomment, $::FORM{'isprivate'}, $timestamp, $::FORM{'work_time'});
>+        SendSQL("UPDATE bugs SET remaining_time = (remaining_time - " . SqlQuote($::FORM{'work_time'}) . ") WHERE bug_id = " . $bugid . ";");

That would avoid this duplication.

>Index: globals.pl
> 
>+sub validateWorkTime{
>+  if (UserInGroup(Param('timetrackinggroup'))){
>+    if(!defined $::FORM{'work_time'}){
>+      ThrowUserError("need_positive_number",{field => 'work_time'});
>+    } else {
>+      my $wk_time = trim($::FORM{'work_time'});
>+      if ($wk_time > 99999.99 || $wk_time < 0 || !($wk_time =~ /^(?:\d+(?:\.\d*)?|\.\d+)$/)){
>+        ThrowUserError("invalid_work_time",{field => 'work_time'});
>+      }
>+    }
>+  }
>+}

This function probably lives best in Bug.pm as well. Maybe it would make sense
to separate it into another bug -- to clean up the callsites we have that
duplicate this code.

If so, you'd supply work_time as a parameter and the field name as a second
parameter. You'd also do the timetrackinggroup check in the callsite, not here.

>Index: post_bug.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v
>retrieving revision 1.88
>diff -u -p -r1.88 post_bug.cgi
>--- post_bug.cgi	27 Mar 2004 03:51:44 -0000	1.88
>+++ post_bug.cgi	19 Jul 2004 18:41:55 -0000
>@@ -343,8 +343,10 @@ if (UserInGroup(Param("timetrackinggroup
>     defined $::FORM{'estimated_time'}) {
> 
>     my $est_time = $::FORM{'estimated_time'};
>+    my $wk_time = $::FORM{'work_time'};
>+    validateWorkTime();
>     if ($est_time =~ /^(?:\d+(?:\.\d*)?|\.\d+)$/) {
>-        $sql .= SqlQuote($est_time) . "," . SqlQuote($est_time);
>+        $sql .= SqlQuote($est_time) . "," . SqlQuote($est_time-$wk_time);
>     } else {

Hmmm. This doesn't look quite right -- shouldn't you be setting estimated time,
remaining time and work time here together? You should be supplying three
variables, no?

> # Add the comment
>-SendSQL("INSERT INTO longdescs (bug_id, who, bug_when, thetext) 
>-         VALUES ($id, $::userid, now(), " . 
SqlQuote($comment) . ")");

For the record, why can't this function use AppendComment?

>Index: template/en/default/attachment/create.html.tmpl
>+    [% IF (Param("timetrackinggroup")) %]

Hmmm. Shouldn't you be checking 

  IF (UserInGroup(Param("timetrackinggroup"))

-- instead of just this? Like you do in edit.html.tmpl.

>Index: template/en/default/bug/create/create.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tmpl,v
>retrieving revision 1.30
>diff -u -p -r1.30 create.html.tmpl
>--- template/en/default/bug/create/create.html.tmpl	7 Jul 2004 23:44:01 -0000	1.30
>+++ template/en/default/bug/create/create.html.tmpl	19 Jul 2004 18:41:56 -0000
>@@ -209,6 +209,12 @@ function set_assign_to() {
>       <input name="estimated_time" size="6" maxlength="6" value="0.0">
>     </td>
>   </tr>
>+  <tr>
>+    <td align="right"><strong>Worked Hours:</strong></td>
>+    <td colspan="3">
>+      <input name="work_time" size="6" maxlength="6" value="0.0">
>+    </td>
>+  </tr>

This needs a UserInGroup check too..

>Index: template/en/default/global/user-error.html.tmpl

I believe the extra errors in this message can be made unnecessary if you use
errors that have been used elsewhere.

Comment 3

[Christian Reis]

Confirming because we miss this too.

Comment 4

[Alexandre Michetti Manduca]

Attachment: work time in more places, using ValidateTime Bug: 252159

Comment 5

[Christian Reis]

Comment on attachment 154023 [details] [diff] [review]
work time in more places, using ValidateTime Bug: 252159

>Index: attachment.cgi
>+  if (UserInGroup(Param('timetrackinggroup'))) {
>+    Bugzilla::Bug::ValidateTime($::FORM{'work_time'}, 'work_time');
>+  }

Hmmm. Shouldn't we only be testing validation if a work_time was actually set,
or does a work time of zero validate correctly?

Or is entering TT information mandatory?

>@@ -869,10 +876,20 @@ sub insert
>   $Text::Wrap::huge = 'overflow';
>   $comment = Text::Wrap::wrap('', '', $comment);
> 
>-  AppendComment($::FORM{'bugid'}, 
>-                Bugzilla->user->login,
>-                $comment,
>-                $isprivate);
>+  if (UserInGroup(Param('timetrackinggroup')) ) {
>+    AppendComment($::FORM{'bugid'},
>+                  $::COOKIE{"Bugzilla_login"},
>+                  $comment,
>+                  $isprivate,
>+                  ,"",
>+                  $::FORM{'work_time'}); 
>+    Bugzilla::Bug::UpdateRemainingTime($::FORM{'work_time'}, $::FORM{'bugid'});
>+  } else {
>+      AppendComment($::FORM{'bugid'},
>+                    $::COOKIE{"Bugzilla_login"},
>+                    $comment,
>+                    $isprivate);
>+  }                      

Instead of duplicating this code, would it be possible to do:

  my $work_time;
  if (UserInGroup(Param('timetrackinggroup')) ) {
     $work_time = $::FORM{'work_time'};
     Bugzilla::Bug::UpdateRemainingTime($work_time, 
				   $::FORM{'bugid'});
  }
  AppendComment($::COOKIE{"Bugzilla_login"},
		$comment,
		$isprivate,
		,"",
		$work_time); 

and a single call to AppendComment? 

>     # Append the comment to the list of comments in the database.
>-    AppendComment($bugid, $who, $wrappedcomment, $::FORM{'isprivate'}, $timestamp);
>-
>+    if (UserInGroup(Param('timetrackinggroup'))) {
>+      AppendComment($bugid, $who, $wrappedcomment,
>+                    $::FORM{'isprivate'}, $timestamp, $::FORM{'work_time'});
>+      Bugzilla::Bug::UpdateRemainingTime($::FORM{'work_time'}, $bugid);
>+    } else {
>+      AppendComment($bugid, $who, $wrappedcomment, $::FORM{'isprivate'},  $timestamp);
>+    } 

Same here.

More coming up.

Comment 6

[Christian Reis]

Comment on attachment 154023 [details] [diff] [review]
work time in more places, using ValidateTime Bug: 252159

>Index: post_bug.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v
>retrieving revision 1.90
>diff -u -p -r1.90 post_bug.cgi
>--- post_bug.cgi	22 Jul 2004 17:48:37 -0000	1.90
>+++ post_bug.cgi	22 Jul 2004 19:35:36 -0000
>@@ -343,7 +343,12 @@ if (UserInGroup(Param("timetrackinggroup
> 
>     my $est_time = $::FORM{'estimated_time'};
>     Bugzilla::Bug::ValidateTime($est_time, 'estimated_time');
>-    $sql .= SqlQuote($est_time) . "," . SqlQuote($est_time);
>+
>+    my $wk_time = $::FORM{'work_time'};
>+    Bugzilla::Bug::ValidateTime($wk_time, 'work_time');

What happens if the person entered work time but zero estimated time? Should we
accept or raise an error?

>+if (UserInGroup(Param('timetrackinggroup'))) {
>+  Bugzilla::Bug::ValidateTime($::FORM{'work_time'}, 'work_time');

I think you don't need to validate this time once again, do you? It's already
been done above.

>+  SendSQL("INSERT INTO longdescs (bug_id, who, bug_when, thetext, work_time)
>+           VALUES ($id, $::userid, now(), " . SqlQuote($comment) .
>+           ", " . SqlQuote($::FORM{'work_time'}) . ")");
>+} else {
>+  SendSQL("INSERT INTO longdescs (bug_id, who, bug_when, thetext)
>+           VALUES ($id, $::userid, now(), " . SqlQuote($comment) . ")");

You can probably use the same technique I suggested above and supply a
work_time of zero. If you can, also mark an XXX around here that says that this
should use AppendComent (instead of duplicating).

>Index: Bugzilla/Bug.pm
>+sub UpdateRemainingTime{
>+  my ($wk_time, $bug_id) = @_;
>+
>+  &::SendSQL("UPDATE bugs SET remaining_time = (remaining_time - " .
>+    &::SqlQuote($wk_time) . ") WHERE bug_id = ".
>+    &::SqlQuote($bug_id) .";");
>+}

Hmmm. Use dbh instead of SendSQL. 

I guess this should really become a method of Bug, but that would require
serious changes to post_bug.cgi.

>Index: template/en/default/attachment/edit.html.tmpl
>+          <b>Worked Hours:</b><br>
>+            Please specify the time spent on editing this attachment<br>

No "on" there:

  .. time spend editing this attachment ..

>Index: template/en/default/bug/create/create.html.tmpl
>+    <td align="right"><strong>Worked Hours:</strong></td>

Maybe

  Hours worked (so far):

-- or is that too confusing?

Comment 7

[Alexandre Michetti Manduca]

Attachment: check if work time is defined

Comment 8

[Christian Reis]

Comment on attachment 154143 [details] [diff] [review]
check if work time is defined

This patch doesn't address the second set of issues I raised..

Comment 9

[Alexandre Michetti Manduca]

Attachment: remaining time test

This patch check if the worked time is greater then estimated time, but only
when adding new bugs. When remaining time is negative, I call
need_positive_number error, but the message is "The Hours Left field requires a
positive number.", but there isn' any hours left field. There is other error I
can use???
This patch also include changes in UpdateRemainingTime.

Comment 10

[Alexandre Michetti Manduca]

Attachment: tab correction

Comment 11

[Christian Reis]

Spoke with Joel, and we both agree that the best idea here is to use the *same*
form as we use to control timetracking in show_bug.cgi. This involves moving the
template out into a separate file/block and moving the handling code into
Bugzilla/Bug.pm.

Comment 12

[Alexandre Michetti Manduca]

Attachment: timetracking bar in edit attachment, create attachment and showbug

I moved the timetracking bar code to a separated file in
/bug/timeform.html.tmpl and the javascript that updates the time left to
/js/timeform.js.
I created a function called UpdateTime in Bug.pm that update the remaining time
and estimated time in the table bugs. The work_time is still updated by
AppendComment or SendSQL.
Some code has to be included in /bug/show.html.tmpl,
/attachement/create.html.tmpl and /attachement/edit.html.tmpl because I need to
declare 2 variables (fRemainingTime and form) as global for the javascript.
I did some modifications in /default/filterexceptions.pl because remaining_time
was moved to diferent templates.

Comment 13

[Christian Reis]

Comment on attachment 154775 [details] [diff] [review]
timetracking bar in edit attachment, create attachment and showbug

>Index: attachment.cgi
>+  if (UserInGroup(Param('timetrackinggroup'))) {

You can also Bugzilla->user->in_group() instead of UserInGroup here..

>+    Bugzilla::Bug::ValidateTime($::FORM{'estimated_time'}, 'estimated_time') if
>+    $::FORM{'estimated_time'};
>+    Bugzilla::Bug::ValidateTime($::FORM{'work_time'}, 'work_time') if
>+    $::FORM{'work_time'};
>+    Bugzilla::Bug::ValidateTime($::FORM{'remaining_time'}, 'remaining_time') if
>+    $::FORM{'remaining_time'};
>+  }

Can we use a local function here to check these values (validateTimeTracking?)
This will avoid the duplication there. Or maybe the right thing to do is
something like the loop Tiago added for validation in bug 253604?

>+  my $work_time;
>+  if (UserInGroup(Param('timetrackinggroup')) ) {
>+    $work_time = $::FORM{'work_time'};
>+    Bugzilla::Bug::UpdateTime($::FORM{'estimated_time'},
>+                              $::FORM{'remaining_time'},
>+                              $::FORM{'bugid'});

Hmmm. Okay, so you call UpdateTime *before* AppendComment here. Why do you call
it after in the next chunk?

>Index: post_bug.cgi
>+    if ($::FORM{'estimated_time'}){
>+      Bugzilla::Bug::ValidateTime($est_time, 'estimated_time');
>+    }  
>+    
>+    my $wk_time = $::FORM{'work_time'};
>+    if ($::FORM{'work_time'}){
>+      Bugzilla::Bug::ValidateTime($wk_time, 'work_time');
>+    }  
>+            
>+    my $rm_time = $est_time - $wk_time;
>+    if ($rm_time){
>+      Bugzilla::Bug::ValidateTime($rm_time, 'remaining_time');
>+    }  

Hmmm. What if $rm_time is zero here? No need to validate it? Or is the problem
that ValidateTime doesn't like zero time?

>Index: Bugzilla/Bug.pm
>+sub UpdateTime{
>+  my ($est_time, $rm_time, $bugid) = (@_);
>+
>+  $est_time = &::SqlQuote($est_time);
>+  $rm_time = &::SqlQuote($rm_time);
>+  $bugid = &::SqlQuote($bugid);

I think the bugid should be the first attribute here. 

(You could also argue that you should *really* be implementing a bug method
here ($bug->update_time(...)) but the CGIs you're changing haven't been updated
to use Bug objects right now so you don't need to do that here)

>+  my $dbh = Bugzilla->dbh;
>+
>+  $dbh->do("UPDATE bugs SET estimated_time = $est_time,
>+                            remaining_time = $rm_time
>+            WHERE           bug_id = $bugid
>+          ;");

I'm not sure SqlQuote is the right solution here, or if you should use ?
markers and then call execute on your saved statement. Joel?

>Index: js/timeform.js
>+function updateRemainingTime(form) {
>+  // if the remaining time is changed manually, update fRemainingTime
>+   fRemainingTime = form.remaining_time.value;
>+}


ISTM that we should be defining fRemainingTime in this JS file and not in the
main template. Is it possible?

template/en/default/attachment/create.html.tmpl
>+[% js_data = BLOCK %]
>+  [% IF UserInGroup(Param('timetrackinggroup')) %]
>+    var fRemainingTime = [% bug.remaining_time %];
>+    var form = this;

Do you actually need this var form = this; here? It seems.. wrong. Can't you
supply the form name into the timeform template so you can supply it to the
javascript function calls instead of supplying form directly? 

If so, you could avoid js_data entirely. 

This comment applies to most of the templates in this patch.

>+        <th>Time Tracking:</th>
>+        <td>
>+          <em>(optional) Please specify the time you spent on this
>+          attachment.</em> <br>

Is this text necessary, or is it easy to understand what the user is supposed
to do once he sees the timeform?

>Index: template/en/default/attachment/edit.html.tmpl
>+  <br> <br>
>+  <small>
>+  <b>Time Tracking:</b>
>+  </small>
>+  <br>

Hmmmm. Are you really looking for an <h4> or <h3> here instead of all these
br/small/b elements? :)

>Index: template/en/default/bug/edit.html.tmpl
>-[% PROCESS bug/time.html.tmpl %]
>-
[snip]
>Index: template/en/default/bug/timeform.html.tmpl
>+  [% PROCESS bug/time.html.tmpl %]

Maybe you want to eliminate bug/time.html.tmpl altogether and include the
functions in your timeform template file?

This looks good, the comments above are just really optimization and style
issues.

Comment 14

[Joel Peshkin]

Comment on attachment 154775 [details] [diff] [review]
timetracking bar in edit attachment, create attachment and showbug

I believe that bad things will happen if you add time without commenting. 
(work_time != 0 and comment = '')

Make sure you test that.

Comment 15

[Alexandre Michetti Manduca]

Kiko,
>>Index: attachment.cgi
>>+  my $work_time;
>>+  if (UserInGroup(Param('timetrackinggroup')) ) {
>>+    $work_time = $::FORM{'work_time'};
>>+    Bugzilla::Bug::UpdateTime($::FORM{'estimated_time'},
>>+                              $::FORM{'remaining_time'},
>>+                              $::FORM{'bugid'});
>
>Hmmm. Okay, so you call UpdateTime *before* AppendComment here. Why do you call
>it after in the next chunk?

The first time I call UpdateTime is in "insert" function and here I will always
have a default comment like "Created an attachment ID DESCRIPTION", but the
second time, in "update" function, if the user didn't make any comment but
changes timetracking bar values like remaining_time or estimated_time I have to
save them, so the UpdateTime call is after "if ($::FORM{'comment'} ||
($::FORM{'work_time'} != 0))" and AppendComment.

>>Index: post_bug.cgi
>>+    if ($::FORM{'estimated_time'}){
>>+      Bugzilla::Bug::ValidateTime($est_time, 'estimated_time');
>>+    }  
>>+    
>>+    my $wk_time = $::FORM{'work_time'};
>>+    if ($::FORM{'work_time'}){
>>+      Bugzilla::Bug::ValidateTime($wk_time, 'work_time');
>>+    }  
>>+            
>>+    my $rm_time = $est_time - $wk_time;
>>+    if ($rm_time){
>>+      Bugzilla::Bug::ValidateTime($rm_time, 'remaining_time');
>>+    }  
>
>Hmmm. What if $rm_time is zero here? No need to validate it? Or is the problem
>that ValidateTime doesn't like zero time?

I believe that if the $rm_time is 0 or '' we just need to save it, no validation
is needed, am I wrong? 

Joel,
>I believe that bad things will happen if you add time without commenting. 
>(work_time != 0 and comment = '')
>
>Make sure you test that.

if work_time != 0 OR comment not '' a default comment "Aditional Hours Worked: "
and "From Update of attachment x" will be posted. There isn't any comment only
when you change estimated_time and remaining_time without commenting.

Comment 16

[Alexandre Michetti Manduca]

Attachment: timetracking bar with the asked changes

have to include a detaint_float in Bugzilla/Util.pm to use dbh->prepare.

Comment 17

[Christian Reis]

Comment on attachment 155102 [details] [diff] [review]
timetracking bar with the asked changes

>Index: attachment.cgi
>+  if (Bugzilla->user->in_group(Param("timetrackinggroup"))) {
>+    foreach my $field ("estimated_time", "work_time", "remaining_time"){
>+      Bugzilla::Bug::ValidateTime($::FORM{$field}, $field) if
>+      $::FORM{$field};
>+    }
>+  }

Can we have a validateTimeTracking() function that does this here to avoid the
duplication?

>+# XXX
>+# Maybe whe can use AppendComment here like in attachment.cgi

s/whe/we and put this on the same line as the XXX

>+sub detaint_float {
>+    $_[0] =~ /^(\d+(?:\.\d+){0,1})$/;

Can you check if you can swap the {0,1} for a "?"?

>+<form name="attachmentform" method="post" action="attachment.cgi"

maybe

  name="edit_attachment"

or something like that?

>Index: template/en/default/bug/timeform.html.tmpl
>+  [% PROCESS bug/time.html.tmpl %]

Did you consider placing the contents of time.html.tmpl in this file as well to
avoid the extra PROCESS and file?

>+          <input name="work_time" value="0" size="3" maxlength="6"
>+                 onchange="adjustRemainingTime(form);">

Hmmm. How does "form" actually work? I thought you would need to at least
supply the name of the form to timeform.html.tmpl, and use
document.forms['formname'] no? 

Look up how to supply parameters to a BLOCK, and ensure you have [% formname %]
available in timeform.html.tmpl.

The rest looks good.

Comment 18

[Alexandre Michetti Manduca]

(In reply to comment #17)
> (From update of attachment 155102 [details] [diff] [review])
> >Index: attachment.cgi
> >+  if (Bugzilla->user->in_group(Param("timetrackinggroup"))) {
> >+    foreach my $field ("estimated_time", "work_time", "remaining_time"){
> >+      Bugzilla::Bug::ValidateTime($::FORM{$field}, $field) if
> >+      $::FORM{$field};
> >+    }
> >+  }
> 
> Can we have a validateTimeTracking() function that does this here to avoid the
> duplication?

  I will create this function in Bug.pm, ok?
 
> >+# XXX
> >+# Maybe whe can use AppendComment here like in attachment.cgi
> 
> s/whe/we and put this on the same line as the XXX

  Oops
 
> >+sub detaint_float {
> >+    $_[0] =~ /^(\d+(?:\.\d+){0,1})$/;
> 
> Can you check if you can swap the {0,1} for a "?"?

  I swap it to "?", but I don't know if we really need this function because the
validateTime will not permite any value different from numbers with a dot
between them. There isn't any different way to detaint those fields?
 
> >Index: template/en/default/bug/timeform.html.tmpl
> >+  [% PROCESS bug/time.html.tmpl %]
> 
> Did you consider placing the contents of time.html.tmpl in this file as well to
> avoid the extra PROCESS and file?

  I think I can't do that because bug/time.html.tmpl is used by other forms like 
 /bug/comments.html.tmpl or /bug/show-multiple.html.tmpl.

> >+          <input name="work_time" value="0" size="3" maxlength="6"
> >+                 onchange="adjustRemainingTime(form);">
> 
> Hmmm. How does "form" actually work? I thought you would need to at least
> supply the name of the form to timeform.html.tmpl, and use
> document.forms['formname'] no? 

  I think form do exactly this...


  I will also put Estimated Time and Hours Worked fields in the same line when
creating a new bug.

Comment 19

[Alexandre Michetti Manduca]

Attachment: timetracking bar

I put ValidateTimeTracking in Bug.pm.
I left "form" in calls to adjustRemaingTime script.

Comment 20

[Christian Reis]

Comment on attachment 155231 [details] [diff] [review]
timetracking bar

>Index: attachment.cgi
>+  if (Bugzilla->user->in_group(Param("timetrackinggroup"))) {
>+    Bugzilla::Bug::ValidateTimeTracking();
>+  }

You need to supply the three parameters to it explicitly -- we don't want to
rely on global variables. Specially not the evil and deprecated $::FORM! 

Ugh, but this has a problem. You need to know the names of the fields to be
able to report the error properly. That sooo sucks. That's why I didn't want to
use a function in the first place. I don't really see a way out of this.. 

>Index: Bugzilla/Bug.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v
>retrieving revision 1.40
>diff -u -p -u -r1.40 Bug.pm
>--- Bugzilla/Bug.pm	29 Jul 2004 02:45:37 -0000	1.40
>+++ Bugzilla/Bug.pm	5 Aug 2004 01:10:44 -0000
>@@ -490,6 +490,28 @@ sub EmitDependList {
>   return @list;
> }
> 
>+sub UpdateTime{
>+  my ($bugid, $est_time, $rm_time) = (@_);
>+
>+  # time values should come in pre-validated
>+  trick_taint($est_time);
>+  trick_taint($rm_time); 

Did you try trick_tainting inside ValidateTime?

>Index: js/timeform.js
>+function adjustRemainingTime(form) {
>+  // subtracts time spent from remaining time
>+  var new_time;
>+
>+  new_time = form.original_remaining_time.value - form.work_time.value;

You can merge these lines into:

  var new_time = form.original_remaining_time.value - form.work_time.value;

>Index: template/en/default/bug/timeform.html.tmpl
>+          <input name="work_time" value="0" size="3" maxlength="6"
>+                 onchange="adjustRemainingTime(form);">

Weren't you going to fix this to use a BLOCK parameter?

Comment 21

[Alexandre Michetti Manduca]

Attachment: timetracking

> Did you try trick_tainting inside ValidateTime?
  I try but it didn't work...

>>Index: template/en/default/bug/timeform.html.tmpl
>>+	     <input name="work_time" value="0" size="3" maxlength="6"
>>+		    onchange="adjustRemainingTime(form);">>
>
>Weren't you going to fix this to use a BLOCK parameter?
  Fixed

Now validateTimeTracking() is on attachment.cgi

Comment 22

[Christian Reis]

Comment on attachment 155306 [details] [diff] [review]
timetracking

>+if (UserInGroup(Param("timetrackinggroup"))) {
> 
>     my $est_time = $::FORM{'estimated_time'};
>-    Bugzilla::Bug::ValidateTime($est_time, 'estimated_time');
>-    $sql .= SqlQuote($est_time) . "," . SqlQuote($est_time);
>+    if ($::FORM{'estimated_time'}){

You can use $est_time here..

>+      Bugzilla::Bug::ValidateTime($est_time, 'estimated_time');
>+    }  
>+    
>+    my $wk_time = $::FORM{'work_time'};
>+    if ($::FORM{'work_time'}){

and $wk_time here..

>+      Bugzilla::Bug::ValidateTime($wk_time, 'work_time');
>+    }  
>+            
>+    my $rm_time = $est_time - $wk_time;
>+    if ($rm_time){
>+      Bugzilla::Bug::ValidateTime($rm_time, 'remaining_time');
>+    }  

You can perhaps rewrite this using statements like:

	Bugzilla::Bug::ValidateTime($rm_time, 'remaining_time') if $rm_time

but I'm not sure this is the best idea, so only do it if it makes sense.

>Index: Bugzilla/Bug.pm
>+sub UpdateTime{
>+  my ($bugid, $est_time, $rm_time) = (@_);
>+
>+  # time values should come in pre-validated
>+  trick_taint($est_time);
>+  trick_taint($rm_time); 

You know, this is a potential security problem. We should play it safe and
revive detaint_float() :-/

This was really good work. I'll be happy to r+ a patch that includes those.

Comment 23

[Alexandre Michetti Manduca]

Attachment: timetracking

Comment 24

[Alexandre Michetti Manduca]

Attachment: some indentation corrections

Comment 25

[Christian Reis]

Comment on attachment 155359 [details] [diff] [review]
some indentation corrections

Joel, can you get a quick look at this? I've already grilled the patch a few
times and I think it looks good.

Comment 26

[Joel Peshkin]

Comment on attachment 155359 [details] [diff] [review]
some indentation corrections


Code looks good, but....

Added time while adding a new attachment....

Software error:

invalid bug attribute UpdateTime at Bugzilla/Bug.pm line 507
	Bugzilla::Bug::AUTOLOAD(1,0.0,0) called at
/home/bugzilla/head/attachment.cgi line 910
	main::insert('Index:
attachment.cgi\x{a}=======================================...') called at
/home/bugzilla/head/attachment.cgi line 127

Comment 27

[Joel Peshkin]

OK, I missed a conflict in the patch.... it's bitrotten, so UpdateTime did not
get applied.

Comment 28

[Joel Peshkin]

If I manually merge Bug.pm, the error goes away.  But, if I comment on a bug and
log time, bugmail includes the hours worked. If I comment from the attachment
page and log time, bugmail does not include the hours worked.

Comment 29

[Alexandre Michetti Manduca]

Attachment: email corrections

I'd like to put Hours Worked on the email send when a new bug is created. Can
anybody help?

Comment 30

[Alexandre Michetti Manduca]

Comment on attachment 155592 [details] [diff] [review]
email corrections

Can you take a look?

Comment 31

[Joel Peshkin]

You might have a tough time adding the hours worked.  Normally, what goes out in
email on a new bug is the same thing to everyone.  I'm not aware of any hooks to
customize it based on recipient.  You;d have to dig through the code for it.

Comment 32

[Joel Peshkin]

Comment on attachment 155592 [details] [diff] [review]
email corrections

This is still bitrotten. Bug.pm patch does not apply and results in errors.

Comment 33

[Alexandre Michetti Manduca]

Attachment: this one applies correctly

Comment 34

[Joel Peshkin]

Comment on attachment 156043 [details] [diff] [review]
this one applies correctly

Almost there, but tinme worked is ommitted from email on new bugs and new time
entered against attachments shows a change in remaining time even though the
same operation on a bug does not.

Comment 35

[Alexandre Michetti Manduca]

  OK, I will include work_time in emails send when a new bug is created.

> new time entered against attachments shows a change in remaining time even
> though the same operation on a bug does not.
>   
  This is when creating new bugs or editing some bug? Because when editing some
existing bug the email include the change in remaining time field and when
creating a new bug the email send the initial estimative.

Comment 36

[Joel Peshkin]

If I have 9 hours left in estimated time and I comment on the bug and mark one
hour worked, the bugmail shows 1 hour worked.

If I have 9 hours left in estimated time and I do something to an attachment and
mark one hour worked, the bugmail shows both one hour worked and the estimated
time revised to 8 hours.

Comment 37

[Alexandre Michetti Manduca]

(In reply to comment #36)
> If I have 9 hours left in estimated time and I comment on the bug and mark one
> hour worked, the bugmail shows 1 hour worked.
> 
> If I have 9 hours left in estimated time and I do something to an attachment and
> mark one hour worked, the bugmail shows both one hour worked and the estimated
> time revised to 8 hours.
> 

humm, are you sure? this don´t happens here... in both cases the mail comes with
hours worked and remaining time updated (not estimated time)...

Comment 38

[Alexandre Michetti Manduca]

Attachment: some corrections

Comment 39

[Christian Reis]

(In reply to comment #36)
> If I have 9 hours left in estimated time and I do something to an attachment and
> mark one hour worked, the bugmail shows both one hour worked and the estimated
> time revised to 8 hours.

The estimated time should never be changed automatically; just the remaining time.

Comment 40

[Alexandre Michetti Manduca]

(In reply to comment #39)
> (In reply to comment #36)
> > If I have 9 hours left in estimated time and I do something to an attachment and
> > mark one hour worked, the bugmail shows both one hour worked and the estimated
> > time revised to 8 hours.
> 
> The estimated time should never be changed automatically; just the remaining time.
> 
  I did the same thing Joel did, but the email only shows Hours Worked and
Remaining Hours(remaining time) not estimated time.

Comment 41

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 42

[Christian Reis]

This is pretty much ready to go, we were really just waiting for Joel's review.

Comment 43

[Joel Peshkin]

Kiko....   yours also.

Comment 44

[Christian Reis]

Yeah, yeah. It's just that because I've looked over and helped write part of
this code, I feel someone else will be more qualified to do review.

Comment 45

[Joel Peshkin]

Comment on attachment 156607 [details] [diff] [review]
some corrections

Could be bitrot, but even after resolving merge conflicts, attachment.cgi wont
compile

Comment 46

[Alexandre Michetti Manduca]

Attachment: "new" patch

Comment 47

[Marc Schumann [:Wurblzap]]

Comment on attachment 155357 [details] [diff] [review]
timetracking

Removing r? from obsolete attachment.

Comment 48

[Marc Schumann [:Wurblzap]]

Comment on attachment 156607 [details] [diff] [review]
some corrections

Removing r? from obsolete attachment.

Comment 49

[Shane H. W. Travis]

Alexandre: I just came across this today, and it looks like you've done an 
great job of pushing this as far as you did. It's unfortunate that it sort of 
died on the vine a few months ago, as I think that this would be something 
worth having.

I tried applying your patch to the tip today, and (not surprisingly) there's a 
fair amount of bitrot. There have also been some changes in how timetracking 
works since then, which may change the design and/or implementation of parts of 
this feature.

If you're still interested in getting this done, please spin a new patch that 
applies to the tip and put me as the reviewer; I've been doing much of the work 
on timetracking lately. If you no longer have the time or inclination to 
continue working on it, please let me know and I'll try and pick things up from 
where you left off.

Comment 50

[Alexandre Michetti Manduca]

(In reply to comment #49)
> Alexandre: I just came across this today, and it looks like you've done an 
> great job of pushing this as far as you did. It's unfortunate that it sort of 
> died on the vine a few months ago, as I think that this would be something 
> worth having.

  Thanks :-)

> I tried applying your patch to the tip today, and (not surprisingly) there's a 
> fair amount of bitrot. There have also been some changes in how timetracking 
> works since then, which may change the design and/or implementation of parts of 
> this feature.
> 
> If you're still interested in getting this done, please spin a new patch that 
> applies to the tip and put me as the reviewer; I've been doing much of the work 
> on timetracking lately. If you no longer have the time or inclination to 
> continue working on it, please let me know and I'll try and pick things up from 
> where you left off.

  Sure. I will make the new patch as soon as possible.

Comment 51

[Alexandre Michetti Manduca]

> I tried applying your patch to the tip today, and (not surprisingly) there's a 
> fair amount of bitrot. There have also been some changes in how timetracking 
> works since then, which may change the design and/or implementation of parts of 
> this feature.

  Shane, can you explain what changes in how timetracking works?

Comment 52

[Shane H. W. Travis]

(In reply to comment #51)
>   Shane, can you explain what changes in how timetracking works?

Come to think of it, I guess most of what has happened has been discussion on 
how to change things: bug 271913 and bug 281354 have seen most of that action.

The only things that have actually changed are relatively minor: see bug 
271276, bug 283139. The one thing you WILL want to accommodate is 
the 'deadline' field: see bug 103636 for that discussion and implementation, if 
you're not already aware of it.

Hope this helps.

Comment 53

[Frédéric Buclin]

Comment on attachment 159477 [details] [diff] [review]
"new" patch

what a surprise: bitrotten! %::FORM no longer exists.

Comment 54

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Only bug fixes are accepted, no enhancement bugs. As this bug has a pretty low activity (especially from the assignee), it's retargetted to ---. If you want to work on it and you think you can have it fixed for 2.24, please retarget it accordingly (to 2.24).

Comment 55

[Max Kanat-Alexander]

We have this well-enough on new bugs now--we should still have it on the attachment screens, though. That would help a lot, at Everything Solved.

Comment 56

[Max Kanat-Alexander]

FWIW, I think the real solution here is to move the attachment-adding/editing screen on to show_bug.cgi so that *everything* can be set when adding or editing an attachment. (Or to put the bug editing form on attachment.cgi as well.)