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

Confirming because we miss this too.

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 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 on attachment 154143 [details] [diff] [review] check if work time is defined This patch doesn't address the second set of issues I raised..

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.

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.

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

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.

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

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.

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

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

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?

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

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

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.

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

Comment on attachment 155592 [details] [diff] [review] email corrections Can you take a look?

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 on attachment 155592 [details] [diff] [review] email corrections This is still bitrotten. Bug.pm patch does not apply and results in errors.

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.

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.

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.

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

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

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

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.

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

Kiko.... yours also.

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 on attachment 156607 [details] [diff] [review] some corrections Could be bitrot, but even after resolving merge conflicts, attachment.cgi wont compile

Comment on attachment 155357 [details] [diff] [review] timetracking Removing r? from obsolete attachment.

Comment on attachment 156607 [details] [diff] [review] some corrections Removing r? from obsolete attachment.

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.

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

> 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?

(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 on attachment 159477 [details] [diff] [review] "new" patch what a surprise: bitrotten! %::FORM no longer exists.

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

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.

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