Time tracking summary reports missing some detailed information

RESOLVED FIXED in Bugzilla 3.4

Status

()

--
minor
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: shane.h.w.travis, Assigned: wicked)

Tracking

2.19.2
Bugzilla 3.4
Dependency tree / graph
Bug Flags:
approval +
blocking2.20 -

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
Posted patch Code patch for tip (obsolete) — Splinter Review
Adds missing information to the Time Tracking Summary Reports that should have
been there in the first place.
Attachment #177637 - Flags: review?
(Reporter)

Updated

14 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.20
(Reporter)

Updated

14 years ago
Flags: blocking2.20?
Flags: blocking2.20? → blocking2.20+
"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
Flags: blocking2.20+
Whiteboard: [wanted for 2.20]
Flags: blocking2.20-

Comment 3

14 years ago
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.
Attachment #177637 - Flags: review? → review-

Comment 4

14 years ago
Updating status whiteboard according to blocking2.20 flag.
Whiteboard: [wanted for 2.20]
Posted patch Shane v1.1 (obsolete) — Splinter Review
Didn't do any real work. Fixed comments and made sure it passes runtests.pl.
Attachment #177637 - Attachment is obsolete: true
Attachment #191576 - Flags: review?(vladd)

Comment 6

14 years ago
*** Bug 304421 has been marked as a duplicate of this bug. ***
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.
Attachment #191576 - Flags: review?(vladd) → review?
(Assignee)

Comment 8

14 years ago
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.
Attachment #191576 - Flags: review? → review-

Comment 9

13 years ago
Only security and dataloss fixes will be accepted on the 2.20 branch.
Assignee: shane.h.w.travis → gerv
Severity: normal → minor
Status: ASSIGNED → NEW
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22

Comment 10

13 years ago
Maybe is that a better component?
Assignee: gerv → query-and-buglist
Component: Reporting/Charting → Query/Bug List

Comment 11

13 years ago
We are freezing the code for 3.0 in two weeks and we don't expect this bug to be fixed on time.
Target Milestone: Bugzilla 2.22 → ---

Updated

12 years ago
Duplicate of this bug: 383240

Comment 13

12 years ago
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

12 years ago
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.
Attachment #270591 - Flags: review?

Comment 15

12 years ago
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

12 years ago
This patch provides the same functionality as my previous patch, but it uses the more idiomatic style requested by Frédéric Buclin.
Attachment #270591 - Attachment is obsolete: true
Attachment #270591 - Flags: review?
(Assignee)

Comment 17

12 years ago
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..
Attachment #272050 - Flags: review?

Comment 18

12 years ago
(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.
(Assignee)

Comment 19

12 years ago
> 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.

Updated

12 years ago
Attachment #272050 - Flags: review? → review?(kiko)

Updated

12 years ago
Depends on: 389313

Comment 20

12 years ago
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.
Attachment #272050 - Flags: review?(kiko) → review-

Comment 21

12 years ago
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.

Updated

12 years ago
Duplicate of this bug: 232914

Comment 23

11 years ago
What's the link between this bug and bug 316425 ?

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

Updated

11 years ago
Assignee: query-and-buglist → wicked
(Assignee)

Comment 24

10 years ago
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.
Attachment #191576 - Attachment is obsolete: true
Attachment #272050 - Attachment is obsolete: true
Attachment #354674 - Flags: review?(mkanat)
Attachment #354674 - Flags: review?(LpSolit)

Comment 25

10 years ago
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.
Attachment #354674 - Flags: review?(mkanat)
Attachment #354674 - Flags: review?(LpSolit)
Attachment #354674 - Flags: review+

Updated

10 years ago
Status: NEW → ASSIGNED
Flags: approval+
Target Milestone: --- → Bugzilla 3.4
(Assignee)

Comment 26

10 years ago
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!)
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Keywords: relnote

Updated

10 years ago
Blocks: 488027
Added to the release notes for Bugzilla 3.4 in bug 494037.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.