Closed Bug 24789 Opened 25 years ago Closed 22 years ago

[E|A|R] Add Estimated, Actual, Remaining Time Fields

Categories

(Bugzilla :: Bugzilla-General, enhancement, P1)

Other
Other
enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: tomdryan, Assigned: jeff.hedlund)

References

Details

Attachments

(6 files, 24 obsolete files)

20.39 KB, image/gif
Details
1.25 KB, text/html
Details
1.55 KB, text/plain
Details
47.46 KB, patch
bugreport
: review+
justdave
: review+
Details | Diff | Splinter Review
1.94 KB, patch
Details | Diff | Splinter Review
2.06 KB, patch
Details | Diff | Splinter Review
This is request to add a few new fields to Buzillla: 

"Estimated Time Required to Fix"  
"Percent Complete"  
"Actual Time Requried to Fix" 

These fields are critical to anyone using Buzilla to manage the development 
process (I think that includes everyone using Bugzilla). It's one thing to know 
that there are "X" bugs to fix, but it's much more useful to know that it will 
take "Y" man days to get to a milestone.

This can also help when balancing out workloads. Programmer A may have only 5 
bugs, but they will take 100 man days to complete, while Programmer B may have 
25 bugs, but they are relatively simple and can be completed in a week, so he 
can be assigned some of Programmer A's bugs and the job will be completed 
sooner than the 100 days. I need to know that kind of information in order to 
accurately estimate the date a project will be completed.

These fields will also allow developers and managers to become better over time 
about estimating target dates by looking at historical data. "Joe always 
underestimates the time by 20%, so let's factor that in to our estimates". 

Along with these new fields, the summary reports and queries would need to be 
modified to give man day totals.
tara@tequilarista.org is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)
Assignee: terry → tara
It would be nice to be able to add a simple, 1 line comment on what one worked
on to fix the bug, so that a list of time spent and how it was spent could be
examined.  For example:
"3 hours"		"Tried to see if the froboz was causing this bug."
"user"	"today"
The sum of all these times would be the actual time required to fix.  These
times would NOT be emailed to everyone on the list, and would be displayable
via a link, rather than being displayed every time the bug was displayed.
Then, it would be nice to be able to search for time spent by "user" during the
period "x" to "y".
Given a regular bug-query, it would also be nice to be able to get information
on the estimated time, and the completed time, broken down by user.
I like what billw@rdmcorp.com said about the ability to enter comments 
about what stuff was worked on for a feature or bug, and how much time 
was spent on that particular part. This will help a lot when reviewing 
bugs and for project management to determine what time was spent fixing 
bugs or implementing features, and how the time was spread out.

I also would like to refine Tom's original idea to include the 
following fields (all in hours):

 'Original Estimated Time To Fix'
 'Current Estimated Time To Fix'
 'Current Elapsed Time'

The 'Current Elapsed Time' field could be automatically computed as the 
sum of the 'time spent' comments mentioned above. The idea is that the 
programmer responsible for fixing the bug or implementing the feature 
will then extend the 'Current Estimated Time To Fix' field as necessary 
after working on the bug, if it is still not completed. Ie: estimating 
how much time it would take to complete the bug fix tomorrow.

From the above you can then automatically compute the following fields:

 'Time Remaining'
 'Percentage Complete'

Once all this is in place, new reporting sections could be added so 
that programmers can get reports on how accurate their estimates have 
been in the past, which will help programmers to better estimate how 
long bug fixes and feature requests take to get completed.

Finally for many feature requests, it will be useful to be able to 
break down a feature request that is 'broad' in scope by making it 
dependant on a number of sub-feature requests. If a feature request is 
dependant on another request, then the time fields would all be 
calculated automatically as the sum total of all the sub-feature 
requests that it depends on. Ie: 'Add Support for BeOS' could be a 
feature request, that is then broken down into all the sub feature 
requests (which may also be parents to other sub-features) until the 
original feature request is completely defined.

More importantly if people want to know how far along the 'BeOS Port' 
is, you can look at the 'Percentage Complete' field for the feature 
which will be compute automatically from all the sub-features that this 
depends on!

I don't know if the above is very difficult to implement or not (I am 
not a perl programmer unfortunately ;-( ), but it seems to me that once 
the new fields are added to track the above information, the rest is 
just code to logically link it all together.
bugzilla has always been conflicted in that it's part bug tracking system and 
part project management system. adding these fields will be possible once we 
re-design the schema. assigning to me so that i can think about this.
Assignee: tara → cyeh
QA Contact: matty
Whiteboard: 3.4
moving to real milestones...
Target Milestone: --- → Future
Taking all of cyeh's Bugzilla bugs.
Assignee: Chris.Yeh → justdave
Moving to new Bugzilla product ...
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: other → unspecified
If we dont add these fields soon, people will start adding these fields
as custom fields.
Once these fields are supported, people will have trouble migrating their
data.

What are your thoughts on this?
Okay, here is an update as to how SciTech temporarily solved this problem. We 
are VERY happy with how well it is working, even with this workaround solution. 
Finally, we can estimate how long it will take to get to various milestones (of 
course, it would be better if these fields were in Bugzilla, so it could 
automatically do the calculations on each query). 

Here is what we do:

At the beginning of each bug summary we put three numbers in the following 
format: [original estimated hours to fix|actual hours worked thus far|hours 
remaining]. 

For example, the following bug: 

[10|12|3] Fix system crash

... would mean that the owner originally estimated 10 hours to fix the problem, 
they have worked on it for 12 hours and they have 3 hours remaing to complete 
the bug.

Hopefully others will find this system useful too and eventually we can get 
these fields added to Bugzilla proper. Enjoy!
Summary: Estimated/Actual Time to Fix, Percent Complete Fields → [E|A|R] Add Estimated, Actual, Remaining Time Fields
I would like to say that I think an effort should be made to add these fields 
into a version of Bugzilla as soon as possible. Even if there is no reporting 
facilities just having a place to store and mine this data from with external 
SQL tools will be way better than the manual solution that Tom outlined above.

I see this bug if scheduled for 'future' Milestone. If I was a perl/SQL 
programmer I would have tried to do this myself already, but I don't want to 
break anything as I wouldn't know where to start. Surely someone with 
experience with Bugzilla could add this in a few hours, or perhaps less than a 
day??
I have a better suggestion. Since SciTech really wants this feature, perhaps 
there is a Bugzilla programmer out there would be willing to do this under 
contract perhaps??
I suggest you would have better luck finding someone with a wider audience on
the webtools newsgroup.
Time remaining does not directly map to percentage completed, because you may
spend 100h on a problem and realize your initial approach does not work and you
have to start over from 0%, although your time estimate might say 10h remaining.
However, percentage completed is the one I have seen mostly used in reporting.
So, I think it would be best to have all: estimated, actual, remaining hours and
percent completed.

Also it should be obvious that you don't need to use the fields you don't want
to, but to be useful to the maximum number of people all of these fields should
be present.

Adding some people to Cc: that I think might find this interesting.
Summary: [E|A|R] Add Estimated, Actual, Remaining Time Fields → [E|A|R|%] Add Estimated, Actual, Remaining Time Fields, Percent Completed Field
This type of tool is only as good as the data. If you make it too hard to keep 
up to date, it is not going to get used by the people that actually have to 
give you the data. It is easier for them to figure out how much longer it will 
take than to try and calculate some guesstimate of the percent complete.

The "percent remaining" can easily be calculated as follows: 

remaining/(estimated + remaining)

In your example, that would be 10/(100+10). In reality, the entire project will 
take 110 hours, because it took 100 hours to realize that you have 10 more 
hours of work. In practice "percent remaing" is not a very useful number any 
way, since I would rather have 90% remaining on a one hour project than 10% 
remaining on a 100 hour project.
As I said, you can use the fields you like.

I also disagree with your percentage calculation. It can be calculated like that
for the whole project, but it does not tell you how much of the bug/feature set
etc. has been done. There is a fine distiction. Why not let users decide what
fields they want to use? We could have a user option to use the raw percentage
field or calculate it using the formula, or use both.
An estimated landing date would be even more useful, both for tracking/managing,
and for anyone who must plan dependant work.
Right, the estimated landing date/actual landing date should be one more field.
Any more fields that should be fixed by this bug that play with this same theme?
Strictly speaking you could use the dependencies etc. to get a dynamic landing
dates, but that would pretty much require that you set the time estimes and
arbitrary dependencies on all bugs. The date(s) give more flexibility. And
again, you don't need to use them if you don't want to.

Should this bug get fixed a bit sooner than "Future"?
The percentage completed should always be a calculated value IMHO. In the 
example given if you realise at the completion of a particular feature that you 
did it all wrong and need to start over, you have two options:

 1. You re-adjust the hours remaining for the new estimate but keep the 
original estimate intact (ie: so when you do finish it you can determine how 
much over the original estimate you were!). So in this case the hours remaining 
gets bumped up to the new estimate and the percentage complete would be:

             actual hours
   ________________________________
   (actual hours + hours remaining)

Hence when the hours remaining field gets 0 you get to 100% complete. Even 
though it may seem that you now are 0% complete if you have to start over, in 
reality you are perhaps 50% complete since you have already spent 50% of the 
time on a bad solution and that time still has to be accounted for. 

Note also that the original 'estimate' should never be changed and in fact is 
never used in the percent complete calculation. The purpose for the original 
estimate is simply for post-mortem analysis and review to look at the 
performance of your development staff and to get a better handle on project 
deadlines in future projects (ie: if all your estimates are lower than the 
actual times then your staff need to start increasing their estimates in the 
future to make them more accurate).

 2. In the case where the original solution works but perhaps needs to be 
redone for performance reasons or some other reason, you can close out the 
original bugzilla bug and re-open a brand new bug for the re-design project. 
Even in the above case if you want to account better for the fact that the re-
design project is really 0% complete, open a new bug that will indicate it is 
0% complete and close out the old one.

As Tom mentioned including a percent complete field that is manually entered 
will lead to inaccuracies as the fields are intrinsicly tied together.
Oops. I meant: percent complete = remaining/(actual + remaining), thanks for 
catching that Kendall...
Summary: [E|A|R|%] Add Estimated, Actual, Remaining Time Fields, Percent Completed Field → [E|A|R] Add Estimated, Actual, Remaining Time Fields
I agree this should be calculated, and even then I'd only trust a percentage
that represents 100% completion of some subset of dependant tasks.  As tomr
pointed out, these are only as good as the data.  Who takes engineer's estimates
of % complete seriously?  I've long since learned to trust only two such numbers
- 0% and 100%, anything in between just means they've started on it, but haven't
finished.
Attached patch Proposed Patch (obsolete) — Splinter Review
I added a "usetimetracking" param to turn on/off any of the changes I made

I used the formula actual_time/(actual_time+remaining_time) as suggested above.


Also made necessary changes to checksetup.pl to add 3 new fields
Attached image Show Bug View (obsolete) —
This is what show_bug now looks like when "usetimetracking" param is set to on.


This image does not show the comments, the header ("Additional Comments") shows
the hours in parantheses after the time: "(5.2 hours)" or whatever.  If the
hours are 0, then it does not show "(0.0 hours)".
Jeff, this is *********awesome**********!!!!!

Suggestion: Can you change the "hours remaining" at the top to a static field 
and move the "hours remaining" input field down to next to the "hours worked" 
input field under the comments input box? "Hours Remaining" should be updated 
each time "Hours Worked" is updated.

(Wish) Question: Can the new fields be included as columns in queries (with 
totals at the bottom)?

Question: When is this important new functionality going to make its way into 
the main source tree?? 
As far as your suggestion goes, I could do that.  Although I didn't think that
the estimated hours necessarily needed to be updated after each hour update, but
I guess it still makes sense to an "Update Estimated Hours" field next to the
hours worked and make the other one static.

> (Wish) Question: Can the new fields be included as columns in queries (with 
> totals at the bottom)?

Which queries (or reports)?  I was going to add them to forms, but I have not
been a Bugzilla user for a long time, so I was not yet sure which
queries/reports should show these fields (besides the obvious show_bug and
'format for print' pages).

> Question: When is this important new functionality going to make its way into 
> the main source tree?? 

No idea on that one...but probably not for a little while since 2.16 is trying
to meet it's release date.
> As far as your suggestion goes, I could do that.  Although I didn't think that
> the estimated hours necessarily needed to be updated after each hour update, 
> but I guess it still makes sense to an "Update Estimated Hours" field next to 
> the hours worked and make the other one static.

Clarification: The "Estimated Hours" should stay as is (since it should really 
never change). I was talking about moving the "Remiaining Hours" input field, 
which should be updated (or verified) each time the "Hours Worked" field is 
updated.

> Which queries (or reports)?  I was going to add them to forms, but I have not
> been a Bugzilla user for a long time, so I was not yet sure which
> queries/reports should show these fields (besides the obvious show_bug and
> 'format for print' pages).

All queries give you the option to select which columns you want to show (This 
could be an automatic thing as new columns are added, so you might want to try 
it out there). If this works, then EAR and % totals at the bottom of the query 
would be nice.

> > Question: When is this important new functionality going to make its way 
> > into the main source tree?? 

> No idea on that one...but probably not for a little while since 2.16 is trying
> to meet it's release date.

This was more a question to the Bugzilla powers that be. ;)  This is a HUGE new 
increase in Bugzilla functionality. Hopefully, they will get it in there ASAP.
> Clarification: The "Estimated Hours" should stay as is

Agreed, sorry, I meant to say "remaining hours" as well in my last comment. 
Anyway, I will do that.

> All queries give you the option to select which columns you want to show 
<snip> 
> EAR and % totals at the bottom of the query would be nice.

Ah, ok, the bug list.  I will look into that, it did not automatically add them.
I believe that the file you want to look at is: colchange.cgi
Attached patch Patch v.2 (obsolete) — Splinter Review
Moves "Remaining Hours" entry field next to "Hours Worked"

Allows "estimated_hours" and "remaining_hours" as columns in query result.
(should obsolete 'Proposed Patch')
Attached image Show Bug VIew 2
updated view
Currently, adding the sum of the actual hours and calculating the percentage in
the query report are more difficult that I assumed.  I was able to add the
"estimated_hours" and "remaining_hours" as custom fields.

Is there a need to add the other two?   Or would a completely different query be
in order?
Attachment #70764 - Attachment is obsolete: true
Attachment #70765 - Attachment is obsolete: true
-> Patch Author
Assignee: justdave → jeff.hedlund
> Currently, adding the sum of the actual hours and calculating the percentage 
> inthe query report are more difficult that I assumed.  

Darn.

> I was able to add the "estimated_hours" and "remaining_hours" as custom 
> fields.

Great! "Remaining" is by far the most important one to know.

> Is there a need to add the other two?   

The percentage is probably the least important because it is relatively easy to 
calculate manually once you have the other numbers. Total "actual hours" would 
be nice, but what you have right now is a great start. 

The main thing I want to see is that your changes get integrated into the main 
tree ASAP!

BTW, a couple of suggestions (I haven't tried your patch out yet, so I'm not 
sure exacly how it works):

1. when the "estimated" is entered the first time, you can automatically set 
remaining = estimated.

2. If you don't already, you may want to allow negative numbers to be entered 
into the actual hours field, just in case somebody makes a typo. Otherwise 
there appears to be no way to fix the field. ;)
> 1. when the "estimated" is entered the first time, you can automatically set 
> remaining = estimated.

I agree.  The patch does not currently do this. I also don't currently have any
of these fields for the New Bug Entry.  I was thinking of adding just the
"Estimated Hours" field to the new bug entry page and populating both the
estimated_hours and the remaining_hours field with that value. (of course this
field would only be visible if the "usetimetracking" param was set)

> 2. If you don't already, you may want to allow negative numbers to be entered 
> into the actual hours field, just in case somebody makes a typo. Otherwise 
> there appears to be no way to fix the field. ;)

I don't currently allow negative numbers, if it is negative I set it to zero. 
But I see your point.  I originally thought I could just manually change the
value in the DB, but perhaps that's too much work if it happens often.  
Status: NEW → ASSIGNED
>  (of course this field would only be visible if the "usetimetracking" param 
> was set)

If possible, I would add "usetimetracking" AND "can confirm" as a condition for 
display of the field, since you don't necessarily want Joe User telling you 
what the estimated time will be. :) Now that I think about it, that would be 
one way to solve the the milestone entry issue as well. It annoys our users 
here to no end the fact that you have to enter a bug, then edit to add a 
milestone.

> I don't currently allow negative numbers, if it is negative I set it to zero. 
> But I see your point.  

If you do this you may want to do some error checking to ensure that your total 
actual is never less than zero. That could certainly mess up your percentage 
calculations. :)

> I originally thought I could just manually change the
> value in the DB, but perhaps that's too much work if it happens often.  

That would be a little hairy for most of us. :) Besides, the guy making the 
mistake is most likely not going to be the bugzilla admin, so he would have to 
bug the admin to fix the field. I'm sure that the bugzilla admins out there 
would not be too happy about that. :)

Attached patch Patch v.3 (obsolete) — Splinter Review
actual_hours and percentage_complete added to buglist.cgi

allow negative numbers for "hours worked" to remove time if entered
incorrectly.

estimated_hours entry on bugentry (also automatically fills remaining_hours)
I saw elsewhere suggestions that this should use the "customized fields" 
over in bug 91037, however, on giving this a quick glance, I don't think that 
would work for this because there's too much interdependence between 
the fields in this case and calculations going on.  Unless the entire 
concept as a whole from this bug is considered a "field type" I don't see 
how this could fit in the custom fields plan.  Thus I'm targetting this for 
2.18 and we'll try attacking it after we get 2.16 out.
Priority: P3 → P1
Whiteboard: 3.4
Target Milestone: Future → Bugzilla 2.18
Attached patch Patch v.4 (obsolete) — Splinter Review
cleaned up code to pass runtests.sh

added sorting capability to actual_hours

added errors for incorrect entry for these new fields (>=0 for
estimated/remaining, and numeric value for hours worked)
Sound great Jeff. Are you ready to mark this one FIXED so it can be added to 
the main source tree?
If I understand the process, then I need to leave it ASSIGNED until it has been
reviewed by 2 reviewers AND until it's been checked-in.  I believe it can get
lost if I mark it FIXED.

Also, I am not quite done;  I've found a few errors and am correcting them for
yet another patch.  And of course, the reviewers may have problems with what
I've done. :)
Attached patch Patch v.5 (obsolete) — Splinter Review
added ability to have percentage_complate (a calculated field from actual_hours
and remaining_hours) to exist in buglist without having to have actual_hours
and/or remaining_hours

withstanding bugs I don't know about, I think this is done -- except for what
I'll have to do when the rest of templatization comes along. (perhaps post
2.16)
Jeff is correct, if it gets marked fixed before it gets checked in, it'll get
lost. :-)   FIXED means it's been checked into cvs.

When you think you have the final version ready to go, add the 'patch' and
'review' keywords to the bug (or if you don't have privs to do that, add a
comment to the bug requesting they be added).  Note that right now this probably
won't get touched until after 2.16 is released (which looks like we blew the
deadline again) however as soon as 2.16 is out, whoever whines the loudest....  ;)
Bug 106529 looks like a dup of this, but both bugs have patches...
*** Bug 106529 has been marked as a duplicate of this bug. ***
*** Bug 103637 has been marked as a duplicate of this bug. ***
Comment on attachment 71324 [details] [diff] [review]
Patch v.5

This needs updating for more templates which were added, but here are some
comments on the untetsted version anyway:

>Index: buglist.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v
>retrieving revision 1.156
>diff -u -r1.156 buglist.cgi
>--- buglist.cgi	2002/01/20 01:44:36	1.156
>+++ buglist.cgi	2002/02/25 18:55:59
>@@ -218,6 +218,16 @@
>         push(@specialchart, ["keywords", $t, $F{'keywords'}]);
>     }
> 
>+    if (Param("usetimetracking")) {
>+        push(@supptables, "longdescs ldhours");
>+        push(@wherepart, "ldhours.bug_id = bugs.bug_id");
>+        if ($::FORM{'order'} =~ /actual_hours/ ) {
>+          if (lsearch(\@fields, "SUM(ldhours.hours) AS actual_hours") == -1) {
>+            push(@fields, "SUM(ldhours.hours) AS actual_hours");
>+          }
>+        }
>+    }

If the param is off, but the order is actual_hours, what happens? Esp in
combination with the order sanity checking we now have.

> sub DefCol {
>-    my ($name, $k, $t, $s, $q) = (@_);
>+    my ($name, $k, $t, $s, $q, $h) = (@_);
> 
>     $::key{$name} = $k;
>     $::title{$name} = $t;
>@@ -1002,6 +1012,11 @@
>         $q = 0;
>     }
>     $::needquote{$name} = $q;
>+
>+    if (!defined $h || $h eq "") {
>+       $h = 0;
>+    }
>+    $::hidecol{$name} = $h;

IF you need to hide it, it should't be there. You never use that part in
def_col, and the later tweaking can happen independantly of this (in the
template, probably)


>+    # not all columns have data in the @row:
>+    if (length($::key{$colname}) > 0) {
>+       $datacolcount++;
>+    }

This is starting to look like a hack...

>Index: checksetup.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v
>retrieving revision 1.125
>diff -u -r1.125 checksetup.pl
>--- checksetup.pl	2002/02/16 03:04:36	1.125
>+++ checksetup.pl	2002/02/25 18:56:03
>@@ -1049,6 +1049,8 @@
>     everconfirmed tinyint not null,
>     reporter_accessible tinyint not null default 1,
>     cclist_accessible tinyint not null default 1,
>+    estimated_hours decimal(5,2) not null default 0,
>+    remaining_hours decimal(5,2) not null default 0,

What about rounding issues with entered decimals?

>     index(bug_id),
>@@ -2020,9 +2023,9 @@
>     if ($buffer eq '') {
>         return;
>     }
>-    $dbh->do("INSERT INTO longdescs (bug_id, who, bug_when, thetext) VALUES " .
>+    $dbh->do("INSERT INTO longdescs (bug_id, who, bug_when, hours, thetext) VALUES " .
>              "($id, $who, " .  time2str("'%Y/%m/%d %H:%M:%S'", $when) .
>-             ", " . $dbh->quote($buffer) . ")");
>+             ", 0, " . $dbh->quote($buffer) . ")");
> }

This is before the updates, so I don't think that this hsould be modified.
> 
> 
>@@ -2688,6 +2691,12 @@
>     DropField("bugs", "qacontact_accessible");
>     DropField("bugs", "assignee_accessible");
> }
>+
>+# 2002-02-20 jeff.hedlund@matrixsi.com
>+# time tracking
>+AddField("longdescs", "hours", "decimal(5,2) not null default 0");

Hmm. I really don't know if this is the best place to put this data.


>Index: globals.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/globals.pl,v
>retrieving revision 1.144
>diff -u -r1.144 globals.pl
>--- globals.pl	2002/02/19 23:22:24	1.144
>+++ globals.pl	2002/02/25 18:56:06
>@@ -300,17 +300,30 @@
>                           "status", "resolution", "summary");
> 
> sub AppendComment {
>-    my ($bugid,$who,$comment) = (@_);
>+    my ($bugid,$who,$comment,$hours) = (@_);

Rather than everyone everywhere else having to specify 0, just do:

$hours ||= 0;

and then don't change the callers
>Index: process_bug.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v
>retrieving revision 1.116
>diff -u -r1.116 process_bug.cgi
>--- process_bug.cgi	2002/02/13 03:05:15	1.116
>+++ process_bug.cgi	2002/02/25 18:56:09
>@@ -593,7 +593,22 @@
>     }
> }
> 
>+# jeff.hedlund@matrixsi.com time tracking data processing:
>+foreach my $field ("estimated_hours", "remaining_hours") {
> 
>+  if (defined $::FORM{$field}) {
>+    my $hours = trim($::FORM{$field});
>+    if ($hours =~ /^(?:\d+(?:\.\d*)?|\.\d+)$/) {
>+      if ($hours ne $::dontchange) {

Aren't these two if's the wrong way arround?
Attachment #71324 - Flags: review-
There is one thing I'm still missing (which #103637 included), access
restrictions. Group bits could be used to restrict access to the effort figures
(no access, read-only access, read/write access) as implemented in #103637.
Restrictions should not only affect the bug form, but also the activities log
and the mail sent out when the trouble ticket changes.

But why all this? We have three different kinds of users. Our customers should
not see any information that concern effort or any measure other of the degree
of completion. Some other users might be able to see effort figures, but should
not be allowed to change them, while our normal development staff must have full
access to these fields.
Attached patch Patch v.7 (obsolete) — Splinter Review
Bringing this patch upto date with the tip.

Applied some changes from bbaetz comments:

- Fixed issue with paramater being off, and actual_hours being in the sort. 
Had to do some minor hacking to get the sorting correct under the new code,
including adding a "GROUP BY" clause to the SELECT statement only when it is
necessary.

- The hiding issue that was mentioned was there for a percentage_complete
column in buglist.  If remaining_hours and actual_hours were not present, they
were necessary to calculate the percentage.  A lot of this code has changed
since v2.15, so I have removed the percentage_complete column for now.

- About the rounding issue with decimal(5,2).  I would be glad to hear other
suggestions, I just knew that 2 decimal places were enough precision for me,
but I can certainly change that...

- As far as the best place to put the "hours" field, I think the 'longdescs'
table is perfect for this field.  The hours can be added as often as comments,
so I figured the comments table was the best choice.  Other options?

- Fixed the ApppendComment as suggested ($hours ||= 0).

Per comment 46, I like the idea of different groups having the ability to
restrict access.  I'm wondering if I should wait for bug 68022 to be fixed to
see what direction the groups take?
Attachment #70874 - Attachment is obsolete: true
Attachment #71053 - Attachment is obsolete: true
Attachment #71282 - Attachment is obsolete: true
Attachment #71324 - Attachment is obsolete: true
Attached patch Patch v.8 (obsolete) — Splinter Review
Thought a little more about the percentage_complete column and have now
integrated it.	There is at least one problem- it should not be sortable yet "%
Complete" is clickable.   I'm still thinking of a way around that.  

It's also a bit of a hack, I put "NULL" in the name field of the DefineColumn
so that it would have a successful SQL SELECT string.  There's probably a
better way of handling this,... like pulling it out of the @selectcolumns list
before it goes to GenerateSQL ?  Not sure the best way to handle that.
Attachment #94499 - Attachment is obsolete: true
Waiting for bug 68022 to be fixed before implementing access restrictions sounds
reasonable to me as I had to modify the code in quite a lot locations as far as
I remember. The problem I see is that its open for more than 1 1/2 years. Its
targeted for BZ2.18, but release cycles are quite long. Maybe we should start
working on access restrictions right now, because the concept for solving bug
68022 seems to be stable. We will of course have to keep in sync with this bug,
but modifications should not be that expensive.
bug 68022 is being worked on by Joel. He has a patch which only has a few issues
left...
fyi - I am watching the group bugs very closely, so as soon as they are ready to
go I'll start working on adding the access restrictions.
I just added bug#162998 which requests a due date field. Perhaps that should be
included as part of this feature? Although I'm more anxious to get the due date
feature before this one.
*** Bug 115025 has been marked as a duplicate of this bug. ***
How's that for Irony...

This bug seems to be waiting for my work on bug 157756 to finish (which will
resolve bug 68022) and I need this this time-tracking feature too.

Even though groups should land very shortly, I think this does not need to be
gated by the groups change.  If you use a named group in the controlling param
i.e.  timetrackgroup instead of usetimetracking, leaving the param blank would
disable the feature and putting a group name in it would enable the feature.  

This is the approach used in bug 143826 which has already been placed in 2.17. 
The changes in bug 143826 should also give you the hooks needed to add some more
privacy to time recording comments.

When the group system from 157756/68022 land, your code will not have to change.
 Instead of burning one of the precious group bits, there will be more to choose
from.  Just make sure you call UserInGroup functions rather than using groupset
math at all.



Jeff - now that the groups system has landed, What's the E/A/R for this feature??
Attached patch Patch v.9 (obsolete) — Splinter Review
Whew.  Sorry for the delay for this patch, and thanks for your comments about
the groups Joel. 

+ Updated to HEAD.

+ This version implements timetrackinggroup instead of a boolean parameter, the
same way insiders/private works (bug 143826).

+ "% Complete" now is not clickable (since it's not sortable).	I did this by
checking for a name in the table.html.tmpl.

+ Using correct ThrowUserError API

+ With the addition of the timetrackinggroup, I had to add a small hack to
buglist.cgi to remove the columns from the display if they were selected.  This
happens (for eg) if the timetrackinggroup is turned on, one or more
timetracking columns selected, and then timetrackinggroup turned off.  Without
the check, the columns continue to appear.

+ Finally, I'm not sure if this is expected behavior, but if a person is not in
timetrackinggroups, should they be able to search using some of the
timetracking fields?  The fields show up in the boolean chart area, but so does
private status stuff, so I was unsure.

On to the nits and bugs! :)
Attachment #94532 - Attachment is obsolete: true
Comment on attachment 100521 [details] [diff] [review]
Patch v.9


The coding style looks good, but there are a few bugs and a nit.

1) In a buglist, if I select the derived fields for display as well as the
status summary, the fields that appear to the right of the derived fields are
messed up.  (Don't fix this by just moving the fields :-)

2) If I have a bug restricted to several groups of which the person querying is
a member, the totals are multiplied by the number of groups.  (probably, you
need some form of distinct here so that you don't sum up the same comment
several times)

3) It is not possible to search for bugs that have had any time recorded on
them or, better, time spent during a particular interval.  This is not a
showstopper, but would be handy.

nit: probably should not display default time spent a 0.00 hours.  It will
encourage people to assume that 1.30 is 1.5 hours because it looks a lot like
an h.mm convention.
Attachment #100521 - Flags: review-
> 1) In a buglist, if I select the derived fields for display as well as the
> status summary, the fields that appear to the right of the derived fields are
> messed up.  (Don't fix this by just moving the fields :-)

I'm having trouble seeing this.  The derived fields are messed up?  Are they
displaying incorrect data, or misaligned?  Mine are showing up fine...though I
am on a small test database, so maybe I'm not seeing it.  

> 2) If I have a bug restricted to several groups of which the person querying is
> a member, the totals are multiplied by the number of groups.  (probably, you
> need some form of distinct here so that you don't sum up the same comment
> several times)

Ah, I'm glad you brought this up.  I noticed something like this in different
queries, but I thought it might have been older code or something... I'll get
this fixed...

> 3) It is not possible to search for bugs that have had any time recorded on
> them or, better, time spent during a particular interval.  This is not a
> showstopper, but would be handy.

I totally agree, it would be extremely handy for my installation as well. I'll
add it.

> nit: probably should not display default time spent a 0.00 hours.  It will
> encourage people to assume that 1.30 is 1.5 hours because it looks a lot like
> an h.mm convention.

Ok, so just "0" then I assume.
The trick to see the column issue is probably to select ALL of the columns in
the "Change Columns" option and the last column displayed and look to see if one
of the summary columns turns into a column of integers.

The multiplied totals is happening because the underlying query gets a row for
each bug/group combination and then uses a DISTINCT to prevent repetition.  At
worst, you could use a COUNT() of those repetitions within each bug and divide
the total you get by that number. With luck, there is a better way.  Search.pm
is not my favorite topic.

*** Bug 70706 has been marked as a duplicate of this bug. ***
Attached patch Updates to patch (obsolete) — Splinter Review
Jeff,

    I updated your patch to mesh with HEAD and changed it's computation to
factor in the fact that all of the joins generate duplicates of the same bug
prior to the DISTINCT.	I did NOT address the issue about disrupting some of
the remaining columns.	Hopefully, this will unstick this item.
Attachment #100521 - Attachment is obsolete: true
I now see what is messing up the columns.  percent_complete is not treated in a
manner consistant with all the other columns.

This problem would go away if we let the SQL server compute the percentage and
format it.

Attached patch Better yet, a complete fix (obsolete) — Splinter Review
Attachment #101285 - Attachment is obsolete: true
Attached patch updated to tip again (obsolete) — Splinter Review
Tried to apply the previous patch to test it and got a bunch of conflicts with
the multiple IP login checkin, so I updated the patch.

This is now live at http://landfill.bugzilla.org/bz24789/ if anyone wants to
try it out.
Attachment #101298 - Attachment is obsolete: true
See http://landfill.bugzilla.org/bz24789/show_bug.cgi?id=847 specifically,
that's where we're playing around.
Attached patch Revised - error on out-of-range (obsolete) — Splinter Review
User now gets an error if hours > 99999.99
Attachment #101498 - Attachment is obsolete: true
Comment on attachment 101503 [details] [diff] [review]
Revised - error on out-of-range


>+DefineColumn("estimated_hours"   , "bugs.estimated_hours"       , "Estimated Hours"  );
>+DefineColumn("remaining_hours"   , "bugs.remaining_hours"       , "Remaining Hours"  );
>+DefineColumn("actual_hours"      , "(SUM(ldhours.hours)*COUNT(DISTINCT ldhours.bug_when)/COUNT(bugs.bug_id)) AS actual_hours", "Actual Hours");
>+DefineColumn("percentage_complete","(FORMAT(100*(SUM(ldhours.hours)*COUNT(DISTINCT ldhours.bug_when)/COUNT(bugs.bug_id))/bugs.estimated_hours,0))", "% Complete"); 

Is FORMAT a MySQLism?

>-    if ($comment =~ /^\s*$/) {  # Nothin' but whitespace.
>-        return;
>+
>+    # allowing negatives though so people can back out errors in time reporting
>+    if (defined $hours) {
>+       if ($hours !~ /^-?(?:\d+(?:\.\d*)?|\.\d+)$/) { 
>+          ThrowUserError("need_numeric_value");
>+          return;
>+       }
>+    } else { $hours = 0 };
>+
>+    if ($comment =~ /^\s*$/) {  # Nothin' but whitespace
>+        if( $hours == 0) {
>+            return;
>+        } else {
>+            $comment = "*** Hours Added ***";
>+        }
>     }

As discussed on IRC, either 

a) users should be required to post a comment or 
b) this comment should be more informational (i.e. "X hours added|subtracted to
estimated|remaining times"

Since no one will be able to agree on a or b, it should probably be a param.

>+    if ($hours =~ /^(?:\d+(?:\.\d*)?|\.\d+)$/) {

Where did this regex come from? (did you write it, or did you steal it from
somewhere?)

>+      <input name="estimated_hours" size="6" maxlength="6" value="0.0"/>

You might verify that this is correct for the doctype we're spewing; I'll try
to find the bug myself.

Joel: I skimmed over a lot of things, so find me when you're ready for another
go-around, and I'll take another look.
Attachment #101503 - Flags: review-
Jeff:
    I think with the updates here and fixing the items noted in comment 67, this
should be land-able.   Please advise if you will be doing the next patch
revision or if I should pick it up.
Joel,

I can finish it up, sorry about the lack of comments from me, I was out of town
for last weekend and had a busy work week!

WRT Comment 67:
> As discussed on IRC, either 
> a) users should be required to post a comment or 
> b) this comment should be more informational (i.e. "X hours added|subtracted
to estimated|remaining times" 
> Since no one will be able to agree on a or b, it should probably be a param.

One problem that might need to be discussed:  We are hiding time information
from people who are not in the time tracking group, but people not in that group
would be able to see the comment of "X hours added".  I don't think they should
see any comment if just hours were added, but is that making this patch too complex?

>>+    if ($hours =~ /^(?:\d+(?:\.\d*)?|\.\d+)$/) {
> Where did this regex come from? (did you write it, or did you steal it from
> somewhere?)

I modified it from somewhere, I can't remember where.  Possibly from O'Reilly's
"Programming Perl"... Is there something wrong with it?

JayPee is correct, FORMAT is mysql-specific.  We should replace that with ROUND().

I think the information leakage issue is a good argument for option a - require
users to post a comment.  If you want option b, you could follow the same model
as private comments do, but I don't think it is worth it.

The regex does look like a good contestant for an obscure perl contest....
How about something like....
if ($hours =~ /^-?\d+\.?\d*$/)



that doesn't allow me to say ".1"
jeff's regex does.
Jeff's regexp looks fine to me.  Makes logical sense...

/^(?:\d+(?:\.\d*)?|\.\d+)$/

?: at the beginning of a () group means "this is only parenthesized so I can use
a group conditional or an 'or' construct, don't use it as a $1 match"

so we have one or more digits, optionally followed by a period and zero or more
digits, OR we have a period followed by one or more digits.
From dicsussion on IRC....
When processmail gets a change to a bug that is a mix of items that want to go
to evreryone on the bug (like status changes) and a comment indicating that
hours have been updated, timetrackers and non-timetrackers will need a
differrent email.

Currently, the text of the email is composed and then the list of potentially
interested parties is scanned to see who gets the entire message.

The current plan is that, rather than make a one-size-fits-all message or
building the message from scratch inside the mail-sending loop, we will do a
"final-assembly" of the message insider the mail-sending loop.

The code outside the loop will assemble a list or hash of strings that, if all
concatenated, form the entire message.  The mail-sending loop can concatenate a
subset of those strings as appropriate.

WRT comment 72:
It looks like the regexp is not necessrily so obscure, but probably warrents a
comment in the code.

Attached patch Patch v.11 (obsolete) — Splinter Review
Ok:

* Removed the hack I made in table.html.tmpl for nameless/non-sortable columns
  
* Removed FORMAT() from the SQL query.	Instead of using ROUND(), I figured it
would be even better to use the template to format the output.	So I added
'format_value' attribute to the abbrev list (in table.html.tmpl) that allows
for formatting.  This seems cleaner.
  (NOTE:  The calculation in the SQL statements allows for negative percentages
if there is a negative number of hours.  I suppose this is okay, since most
likely people won't have a negative number of hours.. I just noticed it in
using my test db)

* The percentage complete column is now sortable!

* Cleaned up buglist.cgi somewhat (from what I hacked in)

* Comment is required on submission of hours.  Instead of using "X hours
added".  If we were to use "X hours added", that would destroy the security
context of being able to see the hours if not in the time tracking group (and
in email).

* Changed default hours from "0.0" to "0", just for time spent, not for E or R.


* Email now sends out estimated hours, remaining hours, and hours added to
those in the time tracking group ONLY.	
  (NOTE:  In IRC we talked about needing to hide a comment that was about the
hours added, and we came to the conclusion [for now], that you will need to use
insider group if you want to hide a comment)

Still TODO:

- Protect querying of hours by non-timetracking group members (?)
- Query bugs that have hours added/subtracted (not just E and R)
- Comment the regexp
- Fix bugs
- Fix nits
Attachment #101503 - Attachment is obsolete: true
myk: CCing you because of Zippy relevance. One of us should probably review this
at some point :-)

Gerv
Attached patch Patch v.12 (obsolete) — Splinter Review
Ok, I think this takes care of most of the remaining items:

+ Non-timetrackinggroup members cannot see the estimated or remaining hours in
the bug activity
+ Fixed problem in processmail of not forming the header of the actions
properly in certain conditions
+ Added hours to the activity logging
+ Added hours to the chfields for the "bug changes" query area (which searches
via the activity table)
+ NEW: Cannot resolve bug without setting remaining_hours to 0.  I've been
working with timetracking on my system for a while, and sometimes I forget to
deplete the hours remaining to 0, so I think this is helpful.  I can't think of
a reason to have a remaining_hours on a resolved bug.
+ Hours are now available for querying in the boolean charts (this searches the
longdescs table)
+ Non-timetrackinggroup members cannot query on hours, estimated_hours, or
remaining_hours.  I did this by simply removing them from the available query
options.  Does there need to be more security than that?
+ Added commenting to the regular expression for the decimal.  Used justdave's
comment 72, since that made it pretty clear
+ some small bug fixes
+ general cleanup
+ brought to HEAD

TODO: 
- I think all the features have been added, so just any bug fixes or nits that
reviewers find in this patch.  I think. :)
Attachment #101666 - Attachment is obsolete: true
Comment on attachment 102344 [details] [diff] [review]
Patch v.12

Code structure works for me, but one strange bug...

If you add an estimated time to a bug, then go to that bug again (not back),
add some time and remaining time with no comment.  Get the missing comment
warning, Then, back up and add a comment, submit and (suprisingly), it midairs.
 If you throw away the changes, some are recorded anyway.
Attachment #102344 - Flags: review-
Attached patch Patch v.12.1 (obsolete) — Splinter Review
Not so strange... it was throwing the user error after already making some
inserts.   

I wonder if we should file another bug to put a line in the code to show where
inserts begin to be made, and no more user entry error checking should be made.
 I'm not sure if it would have helped me, since I added the ThrowUserError for
no comments well after I placed that section of code... but might be a good
idea to make it clear to developers where the error checking seperates the
database modification point.  *shrug*

Thanks for the catch!
Attachment #102344 - Attachment is obsolete: true
http://landfill.bugzilla.org/bz24789/ has been updated with patch v.12.1.  I
haven't had a chance to play with it yet, though.  If anyone else wants to, feel
free.

http://landfill.bugzilla.org/bz24789/show_bug.cgi?id=847 is the bug we've been
playing with, or feel free to start a new one.

Attached patch Patch v.12.2 (obsolete) — Splinter Review
Ok, 12.1 broke the activity logging due to the $timestamp variable not getting
set until later.  So I moved that up, and it looks good again.
Attachment #102420 - Attachment is obsolete: true
There are several UI issues here, unless I'm missing something:

- The common usage case will be an engineer saying "I've worked 3 hours on
this". So, it should be possible to say "3 hours worked", and have that added to
hours worked and taken off hours remaining. Currently, you have to edit two
fields to do this. I think this is because:

- Hours remaining should not be an editable field. It should be calculated from
Estimated hours and actual hours. If the hours you think are remaining is not
(estimated - actual), then you change the estimate :-) Having an estimate which
is not "actual hours + hours remaining" makes no sense, because you are
estimating two different things. :-)

- By default, only the assignee, or perhaps also the QA contact should get the
UI to define number of hours worked. If people don't want this, they can hack
templates.

- %complete should have no decimal places. To avoid premature 100%-age, round down.

- The other fields should have only one decimal place when displayed. No-one
tracks their time to any closer than every 6 minutes.

So, a possible new UI might be:

Estimated Hours   Actual Hours   Hours Remaining   %Complete
[ 10.5 ]          [ 6   ]          4.5              63%

When an owner has done some work, they bump up the Actual Hours figure. If
they've mis-estimated, they change Estimated Hours. The other two fields are
always calculated from those two.

Another possibility (more similar to the current UI) would be:

Estimated Hours   Actual Hours   Hours Remaining   %Complete
[ 10.5 ]           6              4.5              63%

...

Hours Worked: [ 0  ]

And you change the 0 to e.g. a 2, and it gets added on to the Actual Hours. But
I think this is more clunky.

Gerv
Further to that, having looked at the emails:

- The emails should also give numbers to one decimal place. 
- The "hours worked" should not be in the comment title. This is easy to miss
and not logical IMO. The hours worked field should be another bug field, like
anything else, and changes to it should be indicated in the normal way.

I note from reading the comments that there's a field on the longdescs table for
this. I don't think this is the right approach - for a start, it bloats the DB,
particularly for people not using time tracking. Logically, the "estimated
hours" and "hours worked so far" are two properties of a bug, not a comment. The
bugs_activity table should record changes in these values, like any other values.

Sorry to weigh in at this late stage :-)

Gerv
Testing on Landfill I saw that when you fill a news bug and specify
  "Hours Estimated"=23
this information is not send in the initial e-mail.

Now entering "Hours Worked"=34, "Remaining Hours"=3, you get an email:
             What    |Removed                     |Added
  ----------------------------------------------------------------------------
      Remaining Hours|23.00                       |3.00
i.e. the Hours worked is not mailed and not in the activity log:
  http://landfill.bugzilla.org/bz24789/show_activity.cgi?id=849
WRT Comment 82:

based on the requirements of several of the constituants for this one....

1) The common situation is actually that someone indicates "8 hours worked and I
am no closer to a solution."  Defaulting to altering the remaining effort to
reach a solution presumes a degree of infallability to the person who did the
original estimate.

2) This feature is intended for support contracts where it is very common for
hours to be accrued by several people during a period of time where one of them
is assigned.  If we want to add a feature that restricts time logging to the
assignee, that should be a seperate bug blocked by this one and should be
controlled by a param.  It does seem like a strange thing, though, to have this
be the very first thing that is restricted to the assignee.

3) The hours worked does need to be a itemized listing, not just a rolled-up
total per bug.  There is room for debate on storing it either in a new table or
in bugs_activity rather than allocating a number in the longdescs table, but it
cannot be just a field in a bug.  Doing it as a bugs_activity that tracks the
total hours and presumes that any change to the total is time logged does
contain the necessary information, but it makes report extraction into something
fairly complex.

4) Estimated is not just a derived field containing the total hours logged plus
remaining.  Tracking estimated versus actual is a mechanism to perfect the art
of estimation.

Comment on attachment 102421 [details] [diff] [review]
Patch v.12.2

r=joel
Requesting either a 2xr  from myk and/or dkl or a confirmation that the way the
feature is defined (comment 72 question) is correct for Zippy and/or RH
requirements.
Attachment #102421 - Flags: review+
> 1) The common situation is actually that someone indicates "8 hours worked and 
> I am no closer to a solution."  

How pessimistic :-)

> Defaulting to altering the remaining effort to
> reach a solution presumes a degree of infallability to the person who did the
> original estimate.

I don't agree. If you worked 8 hours and are no closer to a solution, then how
can you maintain that the estimated time left is still the same as when you
started? You should update this field, which will be registered and logged as a
change in your estimate, to reflect the new status.

Alternatively, if the estimate field is always the _original_ estimate, then it
shouldn't be editable. But that would suck. :-)

> 2) This feature is intended for support contracts where it is very common for
> hours to be accrued by several people during a period of time where one of them
> is assigned.  

OK, scrap that idea. :-)

> 3) The hours worked does need to be a itemized listing, not just a rolled-up
> total per bug. 

OK, I buy your argument. But the current placing of the number in the comment
title next to the timestamp is not the right place IMO.
I suggest:
Additional hours worked: 3.3
immediately under the title. 
And I think you should adopt UI suggestion B) for the UI at the top of the bug,
labelling the single field: "Additional hours worked:" to make it clear it'll be
added on.

I take it the estimate is stored in the bugs table? That's definitely a per-bug
field.

> 4) Estimated is not just a derived field containing the total hours logged plus
> remaining.  Tracking estimated versus actual is a mechanism to perfect the art
> of estimation.

I never claimed that it was. I said that time remaining is a derived field,
derived from the estimate plus the total hours logged. Time remaining is not
independent of estimate; the following makes no sense:
Estimate: 10 hours
Time Worked: 5 hours
Time Remaining: 7 hours

So, is your estimate 10 hours or 12 hours? :-)

Gerv
My view is that the estimate didn't change, it just turn out that it was a bit
optimistic.  At the present time, we expect that the bug will be completed after
a total of 12 hours.  This is usefull for anyone who is trying to refine the
fine art of estimation.  In fact, I don't think that any organization can
acheieve SEI/CMM Level 4 without tracking the estimation versus the final.

My view is that the estimate didn't change, it just turn out that it was a bit
optimistic.  At the present time, we expect that the bug will be completed after
a total of 12 hours.  

Then why does the estimate not say "12 hours"? :-)

> This is usefull for anyone who is trying to refine the
> fine art of estimation.  In fact, I don't think that any organization can
> acheieve SEI/CMM Level 4 without tracking the estimation versus the final.

The original estimate is recorded in the bugs_activity table. If you feel that
the wild-ass guess people always make at the beginning of a task should be
referred back to throughout, then have "Original Estimate" and "Current
Estimate" fields, where Original Estimate is the original estimate, and is not
editable, and Current Estimate, is the current estimate, and it is.

I'm not arguing this point for academic reasons; the way my company works is to
make an estimate, start work, and change the estimate if necessary (while
tracking changes in the estimate as "gain" or "slip".) So an "original estimate"
field would be a good addition.

Original Est.  Current Est.  Actual Hours   Hours Left   %Complete  Gain
  8            [ 10.5 ]        6              4.5          63%      -2.5

...

Hours Worked: [ 0 ]

(The Gain column is for illustration purposes only; that could easily be a
template customisation.)

Gerv

WRT comment 89:
  So if the "estimate" in the original design is refered to as an "advance
estimate", then the only differrent between these is the following...
  In the Gerv approach, there would be an updateable current estimate and the
hours remaining would be calculated.
  In the Jeff approach there would be an updatable hours remaining and the
currently projected total is derived for the percetage calculation only.
 ???


To me it seems like it would be easier to estimate how much time is left than to
try to re-estimate the entire time needed.  I like Gerv's suggested UI, except I
think the remaining time should be editable rather than the "current estimate".
 The "current estimate" would be calculated based on time remaining + actual
time spent.

Now the confusion comes because doing the above makes me have to remember to
subtract my time spent by hand off the remaining time when I update the bug to
show that I did work.  And I can't think of a real good way to make this less
painless, unless we automatically update that field via javascript when the
hours spent is changed, and then the person can of course override it if they
need to add more time back onto it or whatever.  I'm a little nervous about
requiring javascript for something like that.  Then again, it is just a
convenience item, and if they don't have javascript they just have to do the
subtraction by hand.
> To me it seems like it would be easier to estimate how much time is left than to
> try to re-estimate the entire time needed.  I like Gerv's suggested UI, except I
> think the remaining time should be editable rather than the "current estimate".
> The "current estimate" would be calculated based on time remaining + actual
> time spent.

But then is the "current estimate" really necessary to see in the bug?  Can a
seperate query show that and be more useful to the people who need to compare
that sort of thing? I think the idea would be to look at the actual time spent
on a resolved bug compared to it's estimate.  I don't see the advantage in
seeing a moving "current estimate" while the bug is open.  ???

> [remembering to fill out remaining hours]

In comment 23, it was suggested to move the remaining hours field next to the
'hours worked' field in order to help people remember to deduct.  I've been
using the patch for some time, and I forget to reduce the remaining hours all
the time.  So if there is a good suggestion, I'd love to hear it.  

The javascript idea works for me.
Attached patch Patch v.12.3 (obsolete) — Splinter Review
Changes in this patch:
* Updated to HEAD
* Estimated Hours now shown in new bug e-mail for those in the
timetrackinggroup
* 'Additional Hours Worked' for the input label on edit bug
* 'Additional hours worked' immediately below the comment line instead of 
  imbedded in the Additional Comment line.
* Removed 'hours worked' from the comment area of the bug email, since it is in

  the activity section
* Percentage complete is now shown without decimals, and truncated
* I changed all the hours to show 1 decimal place, but changed it back to two 
  after realizing that I use 15 minute intervals (.25, etc) all the time and a
  1 decimal representation breaks this for me.
Attachment #102421 - Attachment is obsolete: true
Dave is right. (Slightly compressed) new UI:

Original Est.  Current Est.  Hours Worked  Hours Left   %Complete  Gain
  8              10.5          6            [ 4.5 ]        63%      -2.5
...
Hours Worked: [ 0 ]

The problem here is that, with this design, the two fields which might need
updating are quite far from each other. JavaScript would certainly help.

The other option is something like:
Orig. Est.  Current Est.  Hours Worked   Hours Left   %Complete  Gain
    8          10.5         6 + [ 0 ]      [ 4.5 ]       63%     -2.5

How about that? It keeps all the UI together, and the relationship between the
two fields is obvious. When you change the 0, the 4.5 decreases. (There would be
a JS variable containing the original value of Hours Left, so we could always 
just subtract the current "Delta Hours Worked" from it.)

One other point: it should be the case that the only thing in the patch which
defines these numbers as "hours" should be the text in the UI. So, if someone
wants to track e.g. days, or even weeks instead (my organisation tracks days),
then they should be able to do so just by changing the template. This means the
names of the internal fields should be suitably unit-free :-)

Significant figures: Can you show a second decimal only when it's a 5, or is
that too hard? I think two decimal places all the time gives false precision.

Gerv
> The other option is something like:
> Orig. Est.  Current Est.  Hours Worked   Hours Left   %Complete  Gain
>     8          10.5         6 + [ 0 ]      [ 4.5 ]       63%     -2.5

> How about that? It keeps all the UI together, and the relationship between the
> two fields is obvious. 

I think this is good...

> One other point: it should be the case that the only thing in the patch which
> defines these numbers as "hours" should be the text in the UI. So, if someone
> wants to track e.g. days, or even weeks instead (my organisation tracks days),
> then they should be able to do so just by changing the template. This means the
> names of the internal fields should be suitably unit-free :-)

You just want me to type more? ;) I can change it from estimated_hours to
estimated_time, if that's acceptable.

> Significant figures: Can you show a second decimal only when it's a 5, or is
> that too hard? I think two decimal places all the time gives false precision.

Actually, I don't think it's too bad.  I created a global function that makes it
2 decimal places, and checks to see if has a trailing 0, and if it does makes it
1 decimal place.

I can do the same functionality in the templates, but is there a good place to
put a global use BLOCK?  I don't want to put a BLOCK that converts the number to
1 or 2 decimal places in mutliple templates.
Attached file Mock-Up Based on Gerv's Suggestion (obsolete) —
Here is my mock-up of Gerv's UI suggestion.
Apologies for the spam, I meant to ask this when I attached the mock up UI:

One thing taken away from your suggestion, Gerv, is the ability to modify the
original estimate.  Do people want to be able to modify the original estimate?
> You just want me to type more? ;) I can change it from estimated_hours to
> estimated_time, if that's acceptable.

estimated_time would be lovely - or just estimate.

> Actually, I don't think it's too bad.  I created a global function that makes 
> it 2 decimal places, and checks to see if has a trailing 0, and if it does 
> makes it 1 decimal place.

Er... I don't think that's the right algorithm. Surely s/if it has a trailing
0/if it doesn't have a 5 as the second decimal place/?

> I can do the same functionality in the templates, but is there a good place to
> put a global use BLOCK?  I don't want to put a BLOCK that converts the number
> to 1 or 2 decimal places in mutliple templates.

Then put it in its own template, and INCLUDE or PROCESS (look up the difference
:-) it if it's needed. That's perfectly reasonable.

> One thing taken away from your suggestion, Gerv, is the ability to modify the
> original estimate.  Do people want to be able to modify the original estimate?

Hmm. It's not really the "original" estimate any more if they do, is it? But
then, if the only way to set this is on the bug filing page, that's not going to
work for bugs which get e.g. filed by QA and then adopted by someone. So yes, it
needs to be editable. :-|

Your mockup looks OK, although it's very wide. Can we abbreviate the column
names somewhat, as I did? People will get the idea :-)

Gerv
So, far above the knobs, there would be an esitmated time box that could be used
to set or reset the original estimate.

Down closer to the knobs, there would be a form resembling the mockup that
whould show derived fields and update the work and remaining fields.

I think that should cover everything I can think of.
Attached file UI v.2
Ok, got an editable estimated hours, and shorter column names.

> Er... I don't think that's the right algorithm. Surely s/if it has a trailing

> 0/if it doesn't have a 5 as the second decimal place/?

No, I wasn't coding for that.  I was coding to remove the trailing 0 with two
decimal places.  If we truncate (or round) anything without a 5 in the last
spot, then we get this problem:  A user enters 2.99 for remaining hours. 
Reloading the bug shows 2.9 (or 3.0 if rounded).  But then they (or another
person) enters a comment.  It then sends another update for remaining hours,
because the UI changed it from what's stored (2.99).  

If we don't want people entering anything like that, then there should be an
error to force them to use a 5 on the end or not to get that precise.  

If we were to do that, my algorithm still works.  If we don't do that, my
algorithm allows for two decimals whenever the last spot is not a 0, and
doesn't keep changing the field values.
Attachment #102499 - Attachment is obsolete: true
That UI gets the Gerv thumbs-up :-)

Gerv
what if my locale says that ',' is my fraction sep and '.' is my thousands sep?
Attached patch Patch v.13 (obsolete) — Splinter Review
whew!  Changes:

* Added functions (1 for cgi, 1 for templates) to use 2 decimal places unless
the most significant digit is a zero, then only use 1 decimal place
* Changed UI as discussed
* Remaining hours now deducted via javascript (can be overridden)
  (note:  If overridden, I set the override value to the new original, so if
hours are added after manually changing the remaining, then it will deduct 
from the new value, not the original)
* ViewActivity and buglist now show correct decimal places (1 or 2 depending)
* Fieldnames have changed:
      old		    new
      bugs.estimated_hours  bugs.estimated_time
      bugs.remaining_hours  bugs.remaining_time
      longdescs.hours	    longdescs.work_time

Bugs? Nits?  Bring 'em on!
Attachment #102474 - Attachment is obsolete: true
Attached file global/time.html.tmpl (obsolete) —
This is a new template for calculating the format of the time (using 1 or 2
decimal places)
Attachment #102548 - Attachment mime type: application/octet-stream → text/plain
A few quick points (this isn't a proper review):

global probably isn't the right place for that new template; what other
templates use it? Are they in a common directory? I suggest the "bug" directory. 

+    if (Param("timetrackinggroup") && UserInGroup(Param("timetrackinggroup"))) {

This is redundant; if the Param is "" then UserInGroup will return false anyway.

+        $bug{'percentage_complete'} = 
+            CalculatePercentage($bug{'actual_time'},
+                                $bug{'actual_time'} + $bug{'remaining_time'});

This calculation should be done in the template, I think. Get raw data from the
CGIs, present it in the template :-) Pass a reference to the function.

+# Remove the timetracking columns if they are not a part of the group
+# (happens if a user had access to time tracking and it was revoked/disabled)
+if( !Param("timetrackinggroup") || !UserInGroup(Param("timetrackinggroup"))) {
+   @displaycolumns = grep($_ ne 'estimated_time', @displaycolumns);
+   @displaycolumns = grep($_ ne 'remaining_time', @displaycolumns);
+   @displaycolumns = grep($_ ne 'actual_time', @displaycolumns);
+   @displaycolumns = grep($_ ne 'percentage_complete', @displaycolumns);
+}

This is getting horribly inefficient. @displaycolumns should be a hash. No, you
don't have to make this change :-)

+AddFDef("estimated_time", "Estimated Hours", 1);

I think 0 is a better default, for consistency with other fields. Some people
won't use the tracker; also, a value of 0 easily means "no-one has estimated
this yet."

+var fRemainingTime = -1;   // holds the original value

Then why not
+var fRemainingTime = [% orig_estimate %];
(or whatever the name is)?

+                 value="[% PROCESS FormatTimeUnit 

Lower-case block names is the convention.

Gerv
> global probably isn't the right place for that new template; what other
> templates use it? Are they in a common directory? I suggest the "bug" directory. 

It is used in:
  bug/activity/table.html.tmpl
  bug/edit.html.tmpl
  bug/show-multiple.html.tmpl
  bug/comments.html.tmpl
  list/table.html.tmpl

I will move it to bug, unless something in the above makes you think otherwise.

> +AddFDef("estimated_time", "Estimated Hours", 1);
> 
> I think 0 is a better default, for consistency with other fields. Some people
> won't use the tracker; also, a value of 0 easily means "no-one has estimated
> this yet."

This "1" is not the default, it's for the mailhead value.  And it will only show
up if the timetracking is on (and the user receiving the mail is in the
timetrackinggroup)

Other points well noted, thanks!
Attached file bug/time.html.tmpl
* Moved to bug/
* Lower case BLOCK names
* Added calculatepercentage function
Attachment #102548 - Attachment is obsolete: true
Attached patch Patch v.14Splinter Review
* Removed redudant Param() && UserInGroup(Param()) checks
* Moved percentage calculations to templates entirely
* Added percentage_complete to the boolean charts, allowable searches:
    equal, not equal, less than, greater than, contains regexp, not contain
regexp
  (note: I added this because I thought it might be useful to search for bugs
that
	 are > 95% done, or < 25% done.  So mostly for the greater/less than
uses)
Attachment #102547 - Attachment is obsolete: true
> +        $bug{'percentage_complete'} = 
> +            CalculatePercentage($bug{'actual_time'},
> +                                $bug{'actual_time'} + $bug{'remaining_time'});
>
> This calculation should be done in the template, I think. Get raw data from the
> CGIs, present it in the template :-) Pass a reference to the function.

No it shouldn't!! Mixing code with layout is the whole reason it was hard for
people to modify bugzilla to make it look how they wanted when everything was in
CGIs. It was hard to move your look & feel changes across versions because code
was in the way. Going to a template system (as in 2.16) was supposed to make
this easier, but it *still* doesn't if you continue to mix code in with the layout.

<rant>This was the reason I suggested we use a less "powerful" template module
like HTML::Template instead of TT because it doesn't allow you to put any code
in the template. It enforces a separation between your code/logic and your
layout/presentation. You have to put your code in the cgi WHERE IT BELONGS! and
not in the template. This keeps you from screwing yourself over by not only
continuing to mix code with layout and presentation but also separating code
into many many different places which only makes things harder and harder to
work on.</rant>
> It was hard to move your look & feel changes across versions because code
> was in the way. Going to a template system (as in 2.16) was supposed to make
> this easier, but it *still* doesn't if you continue to mix code in with the 
> layout.

I think you are confusing layout and presentation. The % done figure is
definitely presentation, because it's not the raw data. Some templates may
choose not to display it. Others may do different calculations.

Another example is the reporting templates. The totals for the rows, columns and
tables are calculated in the template, rather than the CGI - this is because the
e.g. CSV version of the template doesn't need them.

Gerv
We could go back and forth on this forever, but I think you are confusing
presentation with logic. 1 + 2 = 3 is logic. Not a different presentation of 1
and 2. The templates should only be used for changing where things are displayed
on a page if at all. They should not be generating their own things to display.
I think totals should be calculated in code as well. The template should then
decide whether or not to display it. This is more inline with MVC. Bugzilla
doesn't really have very much separation between Model and Controller, and the
templates were supposed to at least separate the View, but since most of them
contains large amounts of calulations and other logic, that wasn't accomplished
either.
> We could go back and forth on this forever, but I think you are confusing
> presentation with logic. 1 + 2 = 3 is logic. Not a different presentation of 1
? and 2.

But some other template may want 1 * 2, and yet another 1 / 2. You can't do
every vaguely sane calculation on your data inside the CGI in case one of them
happens to be useful to your current template.

I agree that this is a continuum; but doing
Percentage: [% CalculatePercentage(done, total) %]
instead of
Percentage: [% percentage %]

doesn't seem to me like a heinous example of mixing data with presentation.

Gerv
If you decide that you want another logical representation of a piece of data
why is it so much harder to add that to the code instead of to the template?
It's even better to do it in code, because now this new logical representation
is available to be used in even more templates. Instead of having to copy &
paste some calculation that is done in some funky template language, you just
have to use the new variable name. I have been doing MVC web systems for 6 years
and trust me it is better to keep all logic in the actual code base and not mix
it up with your layout. Otherwise you might as well be using php or coldfusion.
Comment on attachment 102598 [details] [diff] [review]
Patch v.14

Missing bug/time.html.tmpl from patch.
Attachment #102598 - Flags: review-
Comment on attachment 102598 [details] [diff] [review]
Patch v.14

I stand corrected... the missing file is in a seperate attachment.

In response to the query, the trick I use to keep it all together is 
diff -u /dev/null template/en/default..../time.html.tmpl >> patchfile
Attachment #102598 - Flags: review-
Comment on attachment 102598 [details] [diff] [review]
Patch v.14

In combination with attachment 102597 [details], r=joel

Jeff: Do you have checkin privs?
Attachment #102598 - Flags: review+
> In response to the query, the trick I use to keep it all together is 
> diff -u /dev/null template/en/default..../time.html.tmpl >> patchfile

Good tip, thanks!

> Jeff: Do you have checkin privs?

No...
Comment on attachment 102598 [details] [diff] [review]
Patch v.14

r= justdave on the condition that you change the fielddef for work_time from
"Hours" to "Hours Worked" before you check it in.
Attachment #102598 - Flags: review+
Checked in for Jeff with change indicated in comment 118

Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
> r= justdave on the condition that you change the fielddef for work_time from
> "Hours" to "Hours Worked" before you check it in.

Er... I asked Jeff to change all references to "Hours" to be unit-neutral,
because many organisations may like to track days worked rather than hours (e.g.
mine.) What happened to this one? :-)

Gerv

> Er... I asked Jeff to change all references to "Hours" to be unit-neutral,
> because many organisations may like to track days worked rather than hours (e.g.
> mine.) What happened to this one? :-)

I was under the impression you just wanted fieldnames to be unit-neutral, and
they are.

The templates and field description have "Hour" in them as the default.
The database columns all got made unit-neutral.  Everything that's left is the
user-readable description (only used in queries and email) within the fielddefs
and the stuff that's in templates, which are the things you would need to
customize if you wanted a different unit.
Do you mean fielddef (as in, database fielddefs table) or field-desc, as in
field-descs.html.tmpl? I can understand the latter having Hours, but I didn't
think the former contained any presented-to-the-user text.

Gerv
They are in the table fielddefs.  fielddefs descriptions do get presented to
users, that is where the description for processmail is grabbed from.  It is
also where the description for show_activity.cgi gets pulled from.


> They are in the table fielddefs.  fielddefs descriptions do get presented to
> users, that is where the description for processmail is grabbed from.  It is
> also where the description for show_activity.cgi gets pulled from.

This is a localisation problem, then. But it's OK; this column of the fielddefs
table will go away when mail gets templatised.

OK, fair enough. Query withdrawn :-)

Gerv
coming in very late to this discussion, so please dismiss if you feel I'm not
making sense.

Wouldn't it make more sense to have "current estimate" as a changeable field,
and "Original Estimate" and "Hours Left" as non-changing fields (with the
remaining_time field in the DB being changed to a current_estimate field)?  All
other timing data can be worked out from the "current est" and "original est"
fields.

My reasoning is this:

If the original estimate was 10 hours, but I realised that this will take
longer, what I really want to do is change the estimate from 10 hours to 20
hours.  I currently do this by the "hours left" field, or, if I am in the time
tracking group, I can also change the "est time" field.

There are problems with this:
1) Changing the "Est Time" field stuffs up all time calculation.  IE the 
"current estimate" field is time worked + time_remaining, the time_remaining
field is the same, though I have added 10 hours to the estimate.  The gain/loss
is totally mucked up.  Secondly, if I am attempting to track how good my
original estimates are, changing the original estimate is a bad idea.

2) It doesn't feel right changing the "hours left" field when I am adding to the
estimate.  Feels counter-intuative that whenver I want to change how long it
will take overall, I change the "hours left" field instead of the "estimate" field.

My thoughts are that a better implementation would be to change the Database
field from "hours left" to "current estimate".  Hours Left is then the "current
estimate - hours worked", and all the other data falls in from there.

Anyway...what are your thoughts?  Have I gotton myself hopelessly confused WRT
how we are meant to use timing?  Or do I have some merit to what I say?

I'm happy to do any changes myself and submit them.  Or to put it another
way...I'll be changing our version of bugzilla, and I'm happy to submit changes
to the main tree if you guys are keen.

Appreciate any thoughts.

<background>
we were using a heavily customised 2.14.1 release of bugzilla, and I'm updating
it to 2.17.3 as well as implementing various changes asked for by our people. 
We have 200 odd accounts, and heavily use bugzilla for problem tracking etc. 
i'm finding a lot of the requests are already in 2.17, while I'm implementing
some of our own...like basing templates on user prefs or groups.

All our users have timing access (or will, as it was not implemented in our old
roll out), as we are happy for people to set their own estimates.  In terms of
testing how good we are at estimates, we're only letting people set the estimate
once (from ZERO to whatever).  After that people can change current estimate.
</background>
*** Bug 236992 has been marked as a duplicate of this bug. ***
Flags: documentation?
Flags: documentation2.18?
*** Bug 268914 has been marked as a duplicate of this bug. ***
Blocks: 271276
Attached patch docs patch for tip v1 (obsolete) — Splinter Review
these line should not exist on 2.18 though ;-)
+        <member>
+        <emphasis>Deadline:</emphasis>
+        This field shows the deadline of this bug.</member>
+
Attachment #213423 - Flags: review?(documentation)
Flags: documentation2.22?
Flags: documentation2.20?
Comment on attachment 213423 [details] [diff] [review]
docs patch for tip v1

Hours Worked
+        This field shows how much hours worked.
This field shows the number of hours worked.


Hours Left
...
+        This value + <quote>Hours Worked</quote> will become new
+        Current Est.
... will become _the_ new ...

%Complete
+        This field shows percentage of completion.</member>
Awkward.
This field shows what percent of the task is complete.
?

Gain
+        This field shows how much hours ahead of the Orig. Est..
1. I don't know what this means.
2. you want "how many", not "how much".

Deadline
+        This field shows the deadline of this bug
deadline _for_ this bug ?
Attachment #213423 - Flags: review?(documentation) → review-
Attached patch docs patch for tip v2 (obsolete) — Splinter Review
Attachment #213423 - Attachment is obsolete: true
Attachment #213425 - Flags: review?(documentation)
Comment on attachment 213425 [details] [diff] [review]
docs patch for tip v2

Sorry. it's late, and i'm only slowing recognizing what i don't like in this text.

+        <emphasis>*Time Tracking:</emphasis>
That quote isn't closed!

+        specified by <quote>timetrackinggroup</quote> parameter.
by _the_ ...

Orig. Est.
+        This field shows original estimated time.</member>
shows _the_

Current Est
+        This field shows current estimated time.
shows _the_

what kind of time is his stuff? for some reason i sort of want to write "time estimate", but i don't understand this stuff enough to know.

Hours Worked
+        This field shows how much hours worked.</member>

I know i suggested it, but i've changed my mind, how about:
... shows _the number of hours worked. :)

Hours Left
+        This field shows <quote>Current Est.</quote> -
shows _the_

%Complete
+        This field shows what percent of the task is complete.</member>
Ask someone else if it should be percent or percentage.

Gain
+        This field shows how many hours ahead of the Orig. Est..</member>
Attached patch docs patch for tip v3 (obsolete) — Splinter Review
sorry for r? spam....
Attachment #213425 - Attachment is obsolete: true
Attachment #213742 - Flags: review?(documentation)
Attachment #213425 - Flags: review?(documentation)
Comment on attachment 213742 [details] [diff] [review]
docs patch for tip v3

>+        <emphasis>Gain:</emphasis>
>+        This field shows how many hours ahead of the Orig. Est..</member>

This field shows the number of hours that the [%terms.bug%] is ahead of the origest.
(sentence for general English, not copy+pasting into the patch :)
Attachment #213742 - Flags: review?(documentation) → review+
Deadline doesn't exist on 2.18 branch
tip:

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.48; previous revision: 1.47
done

2.22rc1:

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.37.2.9; previous revision: 1.37.2.8
done

2.20.1:

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.33.2.12; previous revision: 1.33.2.11
done

2.18.5:

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.21.2.10; previous revision: 1.21.2.9
done
Flags: documentation?
Flags: documentation2.22?
Flags: documentation2.22+
Flags: documentation2.20?
Flags: documentation2.20+
Flags: documentation2.18?
Flags: documentation2.18+
Flags: documentation+
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.