286452 - Time tracking summary reports missing some detailed information

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Shane H. W. Travis]

When getting the updated patch from Kiko, I got only the patches for 
summarize_time.cgi, and not all the mods for summarize-time.html.tmpl. This 
means that there's a line of information missing from the detailed reports.

Patch to follow.

Comment 1

[Shane H. W. Travis]

Attachment: Code patch for tip

Adds missing information to the Time Tracking Summary Reports that should have
been there in the first place.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

"If it's not a regression from 2.18 and it's not a critical problem with
something that's already landed, let's push it off." - Dave

Comment 3

[Vlad Dascalu]

Comment on attachment 177637 [details] [diff] [review]
Code patch for tip

+    <tr class="[% tr_class %]">
+      <td>&nbsp;</td>
+      <td colspan="3">
+	 <table width="100%" cellpadding="0" cellspacing="0">
+	 <tr><td width="33%">
+	     Estimated: [% detail_data.$id.estimated_time FILTER html %]h
+	 </td><td width="33%">
+	     Remaining: [% detail_data.$id.remaining_time FILTER html %]h
+	 </td><td width="33%">
+	   Deadline: [% detail_data.$id.deadline || "<b>Not set</b>"  %]
+	 </td></tr></table>
+    </tr></td>

The last line should probably be "</td></tr>". Due to it, the current patch
would invalidate the HTML.

Also the whole identation style of this block could be improved, in the lines
of:

<tr>
  <td width="33%">
    ....
  </td>
  <td width="33%">
    ....
  </td>

and so on.

Comment 4

[Vlad Dascalu]

Updating status whiteboard according to blocking2.20 flag.

Comment 5

[Marc Schumann [:Wurblzap]]

Attachment: Shane v1.1

Didn't do any real work. Fixed comments and made sure it passes runtests.pl.

Comment 6

[Frédéric Buclin]

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

Comment 7

[Marc Schumann [:Wurblzap]]

Comment on attachment 191576 [details] [diff] [review]
Shane v1.1

Vlad, if you're reviewing this patch at the moment, please leave a comment --
you seem to be busy, so I'm opening the r? to the wind for now.

Comment 8

[Teemu Mannermaa (:wicked)]

Comment on attachment 191576 [details] [diff] [review]
Shane v1.1

>Index: template/en/default/bug/summarize-time.html.tmpl
>===================================================================
>@@ -110,7 +110,7 @@
>-    <tr><td colspan="5" class="owner_header">
>+    <tr><td colspan="4" class="owner_header">

This change looks incorrect. This makes the last column empty and thus it's background changes color.

>+[% BLOCK do_tt_header %]
>+  <tr class="[% tr_class FILTER html %]">
>+    <td>&nbsp;</td>
>+    <td colspan="3">
>+      <table width="100%" cellpadding="0" cellspacing="0">
>+        <tr>
>+          <td width="33%">
>+            Estimated: [% detail_data.$id.estimated_time FILTER html %]h
>+          </td>
>+          <td width="33%">
>+            Remaining: [% detail_data.$id.remaining_time FILTER html %]h
>+          </td>
>+          <td width="33%">
>+            Deadline: [% detail_data.$id.deadline || "<b>Not set</b>"  %]
>+          </td>
>+        </tr>
>+      </table>
>+    </td>
>+  </tr>
>+[% END %]

Looks like this only covers 4 columns when there's sometimes 5 columns. This also makes even rows to have incorrect background color in last column.

In addition Deadline field shows unnecessary time portion.

Also, please move this BLOCK after the two subsequent BLOCKs so that the comment just before this BLOCK still makes sense.

>@@ -167,7 +190,12 @@
>+    [% INCLUDE bug_header id=id bug_status=status colspan=2
>+                          short_desc=items.join(";") %]

I can't see that this change does anything. Param colspan isn't used in bug_header BLOCK.

Comment 9

[Frédéric Buclin]

Only security and dataloss fixes will be accepted on the 2.20 branch.

Comment 10

[Frédéric Buclin]

Maybe is that a better component?

Comment 11

[Frédéric Buclin]

We are freezing the code for 3.0 in two weeks and we don't expect this bug to be fixed on time.

Comment 13

[Dennis Lambe]

Attachment: Add totals for Original, Current, and Hours Left estimates to the Time Summary

This patch puts sums of the "Orig. Est.", "Current Est.", and "Hours Left" fields to the top of the summarize_time report.  These totals are not related to the time period specified, only to the list of bug ids selected.  I believe it does what Bug #286452 asks for.

The template part of this patch is just about the bare minimum it took to support the feature.  I wouldn't mind if it got some TLC.

Comment 14

[Frédéric Buclin]

Comment on attachment 270591 [details] [diff] [review]
Add totals for Original, Current, and Hours Left estimates to the Time Summary

Putting this patch in our radar.

Comment 15

[Frédéric Buclin]

Comment on attachment 270591 [details] [diff] [review]
Add totals for Original, Current, and Hours Left estimates to the Time Summary

>+    my $sth = $dbh->prepare($q);
>+    $sth->execute();
>+    $res = $sth->fetch->[0];

Please write: my $total = $dbh->selectrow_array('SELECT ...'); instead. This is shorter and cleaner.

Comment 16

[Dennis Lambe]

Attachment: Revised patch for overall time summaries

This patch provides the same functionality as my previous patch, but it uses the more idiomatic style requested by Frédéric Buclin.

Comment 17

[Teemu Mannermaa (:wicked)]

Comment on attachment 272050 [details] [diff] [review]
Revised patch for overall time summaries

I'm assuming you want this reviewed to get it checked in someday..

Comment 18

[Dennis Lambe]

(In reply to comment #17)
> (From update of attachment 272050 [details] [diff] [review])
> I'm assuming you want this reviewed to get it checked in someday..

Yes, that would be great.  I get the sense that I'm not the only one interested in this functionality (especially from Bug 304421).  Is there something special I have to do to request review and check-in?

Feel free to email me privately if you don't want to clutter up the bug comments with instructions.

Comment 19

[Teemu Mannermaa (:wicked)]

> in this functionality (especially from Bug 304421).  Is there something special
> I have to do to request review and check-in?

Yes, you need to request a review by setting the review flag for the patch but I took care of that when I made my comment. I did ask it from wind (iow, from no specific reviewer) so it might take a while to get it reviewed. Review process is documented in http://wiki.mozilla.org/Bugzilla:Review, which you can refer for details and also for information of how to select a specific reviewer.

Comment 20

[Frédéric Buclin]

Comment on attachment 272050 [details] [diff] [review]
Revised patch for overall time summaries

This patch doesn't display time fields per bug, which is what travis wanted in his original patch.
Before updating your patch, wait for the one from bug 389313, which will make your life much easier as you won't have to edit the .cgi script anymore.

Comment 21

[Dennis Lambe]

That's true, it doesn't display time fields per bug.  It's actually more appropriately a patch to bug #383240, but since that bug was marked as a duplicate of this one, I was assuming that this bug was meant to serve as an umbrella for a couple of different time-tracking improvements.  If that's not the case, then bug #383240 should probably be re-opened since it's not really a duplicate of this one - it asks for overall summaries of time remaining, not per-bug information.

Comment 23

[bigstijn]

What's the link between this bug and bug 316425 ?

I have the feeling they depend on each other since bug 316425, comment 16 .

Comment 24

[Teemu Mannermaa (:wicked)]

Attachment: Add more detail information, V2

This is a combination of patches made by Shane and Dennis. This makes following changes to summary report when "detailed summaries" is enabled:
 1) Adds estimated, remaining and deadline information to each shown bug.
 2) Calculates and shows total of estimated and remaining hours as well as latest deadline for all summarized bugs.

Patch adds filter exceptions so two reviews will be needed for that part.

Comment 25

[Frédéric Buclin]

Comment on attachment 354674 [details] [diff] [review]
Add more detail information, V2

The patch looks good and works fine, but I have some additional comments, which could lead to new separate bugs:

- When grouped by bug number, it would make sense (IMHO) to not exclude bugs which have some remaining time, but which got no traction yet. This way, the global remaining time would be more accurate (we shouldn't exclude resolved bugs here, because QA may work on RESOLVED bugs, before marking them as VERIFIED).

- Also, the global deadline should not take resolved bugs deadline into account, IMO, because if the bug is already resolved, then that's not something you have to worry about anymore, and so you would get a false "too far in the future" deadline. Or in case QA still needs to verify the resolved bugs before the given deadline, then we should still take them into account assuming there is some remaining time left.

- We should add some text at the top of the "worked time" column to describe what the sub-totals represent. Now that we have many fields displayed, this may look confusing otherwise.


Otherwise, I like the patch. This summarize_time page still has a large place for improvements, and this patch is a start in this direction. So r=LpSolit, and I will file new bugs for each of the comment above.

Comment 26

[Teemu Mannermaa (:wicked)]

Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.124; previous revision: 1.123
done
Checking in template/en/default/bug/summarize-time.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/summarize-time.html.tmpl,v  <--  summarize-time.html.tmpl
new revision: 1.14; previous revision: 1.13
done

(Yay! This was my very first checking!)

Comment 27

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.4 in bug 494037.