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

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.

moving to real milestones...

Taking all of cyeh's Bugzilla bugs.

Moving to new Bugzilla product ...

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!

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.

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

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.

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

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

Moves "Remaining Hours" entry field next to "Hours Worked" Allows "estimated_hours" and "remaining_hours" as columns in query result. (should obsolete 'Proposed Patch')

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?

-> Patch Author

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

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

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.

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

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?

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.

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?

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.

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

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! :)

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.

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

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.

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.

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.

See http://landfill.bugzilla.org/bz24789/show_bug.cgi?id=847 specifically, that's where we're playing around.

User now gets an error if hours > 99999.99

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.

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.

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

myk: CCing you because of Zippy relevance. One of us should probably review this at some point :-) Gerv

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

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.

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!

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.

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.

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.

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

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.

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.

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.

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.

That UI gets the Gerv thumbs-up :-) Gerv

what if my locale says that ',' is my fraction sep and '.' is my thousands sep?

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!

This is a new template for calculating the format of the time (using 1 or 2 decimal places)

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!

* Moved to bug/ * Lower case BLOCK names * Added calculatepercentage function

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

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

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

Comment on attachment 102598 [details] [diff] [review] Patch v.14 In combination with attachment 102597 [details], r=joel Jeff: Do you have checkin privs?

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

Checked in for Jeff with change indicated in comment 118

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

*** Bug 268914 has been marked as a duplicate of this bug. ***

these line should not exist on 2.18 though ;-) + <member> + <emphasis>Deadline:</emphasis> + This field shows the deadline of this bug.</member> +

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 ?

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>

sorry for r? spam....

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

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