Add percentages next to tabular reports sub-totals

ASSIGNED
Assigned to

Status

()

Bugzilla
Reporting/Charting
--
enhancement
ASSIGNED
12 years ago
4 years ago

People

(Reporter: kino, Assigned: GavinS)

Tracking

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en) AppleWebKit/412.7 (KHTML, like Gecko) Safari/412.5
Build Identifier: 

We use tabular reports to balance workload amongst our engineers and track progress. To this end, it 
would be extremely helpful to have percentages next to the sub-totals. I've attached a screenshot with a 
simple exemple.

Thank you.

Reproducible: Always
(Reporter)

Comment 1

12 years ago
Created attachment 198324 [details]
Simple mockup of the feature request
I'm probably not going to get to this any time soon, but you could probably hack
it in yourself by editing the template. Template Toolkit is probably powerful
enough to add all the figures and turn them into a percentage for you.

Gerv
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 3

12 years ago
That's alright. We already customized the application but wanted to see some changes implemented in the 
main distribution because we currently have to port our modifications each time we upgrade.

Thanks.
Version: unspecified → 2.18

Comment 4

12 years ago
kino: Could you post a diff between the old file and the newer one with the
modifications?
Created attachment 236842 [details] [diff] [review]
patch for tip

i think adding a css rule for ttotal class makes better look though :-p
Attachment #236842 - Flags: review?
Comment on attachment 236842 [details] [diff] [review]
patch for tip

>+            # take a value from 0 to 1 and returns percent like             
>+            # 99.9 or 0.1, with  'ed leading spaces
>+            percent => sub
>+            {
>+                my ($percent) = @_;
>+                    $percent =  sprintf("%04d", $percent * 1000);

1000? Are you sure?

>+[% FOREACH col = col_names %]
>+  [% FOREACH row = row_names %]
>+    [% grand_total = grand_total + data.$tbl.$col.$row %]
>+  [% END %]
>+[% END %]
> <table border="1">

Nit: put a blank line in between the code and the <table>.

>-      [% END %] 
>+      [% END %]
>+      [% row_total_percent = 0 %]
>+      [% row_total_percent = row_total / grand_total IF(row_total != 0) %]

Why do we need the IF part? Same question for the column case. We wouldn't be dividing by zero.

>-        [% row_total %]</a>
>-        [% grand_total = grand_total + row_total %]
>+        [% row_total %]([% row_total_percent FILTER percent FILTER none %]%)</a>

The problem with this is that row_total_percent is not a percent until it's been FILTERED percent. You are doing half the maths (dividing by grand_total) in the template, and half in the filter.

The best thing would probably be to rename row_total_percent as row_total_as_fraction.

You also don't need the FILTER none. You instead need to add "percent" to the list of filters at the bottom of t/008filter.t.

>-        [% col_totals.$col %]</a>
>+        [% col_totals.$col %]([% col_total_percent FILTER percent FILTER none %]%)</a>

What does this do to the formatting? Would it be worth having a separate column for the percentages?

Gerv
Attachment #236842 - Flags: review? → review-
(In reply to comment #6)
> >+                    $percent =  sprintf("%04d", $percent * 1000);
> 1000? Are you sure?

see s/(\d)$/.$1/
then it always returns 3 (digit|&nbsp;) + '.' + 1 digit
this is because: first i tried %3.1f etc but no success

> >+      [% row_total_percent = 0 %]
> >+      [% row_total_percent = row_total / grand_total IF(row_total != 0) %]
> Why do we need the IF part? Same question for the column case. We wouldn't be
> dividing by zero.

grand_total can be 0 if the condition returns no bug,
 and can cause devision by zero in this case.
and row_total(and row_total_percent) is always 0 if grand_total==0
# i don't know whether or not this report does show if there's no bug.
# if don't, that can be removed from the patch.

> >-        [% col_totals.$col %]</a>
> >+        [% col_totals.$col %]([% col_total_percent FILTER percent FILTER none %]%)</a>
> What does this do to the formatting? Would it be worth having a separate column
> for the percentages?

i don't have strong intention about how to show the UI, i don't decide.
no one take this bug and i just dave a suggestion.
(In reply to comment #7)
> see s/(\d)$/.$1/
> then it always returns 3 (digit|&nbsp;) + '.' + 1 digit
> this is because: first i tried %3.1f etc but no success

gerv@otter:bugzilla$ perl -e 'my $foo = sprintf("%3.1f\n", 0.234 * 100); print $foo'
23.4
gerv@otter:bugzilla$

Not sure why it doesn't work for you...

> grand_total can be 0 if the condition returns no bug,
>  and can cause devision by zero in this case.
> and row_total(and row_total_percent) is always 0 if grand_total==0

Oh, I see. It would be easier to understand the code if you check for grand_total != 0 rather than row_total != 0/

> i don't have strong intention about how to show the UI, i don't decide.
> no one take this bug and i just dave a suggestion.

OK, fair enough. Can you make these changes and then attach a new patch and a screenshot? 

Thanks,

Gerv

Created attachment 242279 [details]
screenshot

i'm not sure he or they want(s) this :-(
Created attachment 242280 [details] [diff] [review]
patch for tip 2

added other row/col so special hack used in previous isnt needed
Attachment #236842 - Attachment is obsolete: true
Attachment #242280 - Flags: review?
Comment on attachment 242280 [details] [diff] [review]
patch for tip 2

>+            # take a value from 0 to 1 and returns percent like             
>+            # 99.9 or 0.1, with &nbsp;'ed leading spaces

I don't see any code doing "&nbsp;'ed leading spaces"...

>+      <td class="ttratio">
>+        Ratio
>+      </td>

Please title this column "%", and remove the % sign from the individual figures. The English word "ratio" is used to describe two numbers. E.g. "10:1" is a ratio.

>+      </td>
>+      <td class="ttratio" align="right">
>+        [% row_total_percent FILTER percent FILTER none %]%

Please do not FILTER none here; it's not necessary. FILTER none is used to explicitly mark fields we know we don't need to filter; therefore, it's only ever used on its own.

>+      [% col_total_percent = 0 %]
>+      [% col_total_percent = col_totals.$col / grand_total IF(grand_totals != 0) %]

There is a typo here - you mean "grand_total", not "grand_totals".

Did you test this page with a query which returns a grand_total of 0?

>+    <td class="ttotal align="right">

You are missing a double quote here. Therefore, the alignment won't work. Although this won't be a problem in practice, because this field will always be the widest one.

>+    <td class="ttratio" align="left">
>+      Ratio
>+    </td>

Same as above - please call it "%".

>+      [% col_total_percent = col_totals.$col / grand_total IF(grand_totals != 0) %]

Same typo.

Gerv
Attachment #242280 - Flags: review? → review-
Created attachment 242398 [details] [diff] [review]
patch for tip v3

> Please do not FILTER none here; it's not necessary.

i dont understand what he meant, but FILTER is needed to avoid test 8 fail.
added FILTER html
Attachment #242280 - Attachment is obsolete: true
Attachment #242398 - Flags: review?
> i dont understand what he meant, but FILTER is needed to avoid test 8 fail.
> added FILTER html

You have a strange approach to tests! What do you think this test is testing? Is it right to just do whatever it takes to shut the test up, or is it right to figure out why the test is there, and change your code in the way the test is trying to enforce?

The tests check that every output is filtered in some way. We "FILTER none" to show that we have explicitly marked something as not needing filtering. That is not appropriate in this case, and nor is "FILTER html".

The reason the test is failing is that it does not know about your new "percent" filter. You need to edit the test and add that filter name to the list in the regular expression in the test.

Gerv
Comment on attachment 242398 [details] [diff] [review]
patch for tip v3

per previous comment
Attachment #242398 - Flags: review? → review-
Attachment #242398 - Attachment is obsolete: true

Updated

10 years ago
Assignee: gerv → charting
(Assignee)

Comment 15

8 years ago
Created attachment 404221 [details] [diff] [review]
patch for tip, v4

I've updated this to tip, and hopefully addressing the review comments.

1) Fix grand_totals -> grand_total

2) Remove unused(?) assignment of row_index

3) Changed filtering to remove FILTER HTML from the percentage value,
   leaving just the formatting FILTER, and added it to
   filterexceptions, as it is just a number that we work with in the
   template, so safe. (Though personally I'd prefer to FILTER HTML it
   anyway as a defensive measure, that does not seem to be the norm.)

4) Added some simple css to distinguish the percentages from the totals in the display
Assignee: charting → bugzilla
Status: NEW → ASSIGNED
Attachment #404221 - Flags: review?
(Assignee)

Comment 16

8 years ago
Created attachment 404223 [details]
screenshot of v4 patch

Comment 17

8 years ago
Comment on attachment 404221 [details] [diff] [review]
patch for tip, v4

This looks cool. I will give it a look.
Attachment #404221 - Flags: review? → review?(LpSolit)

Comment 18

8 years ago
(In reply to comment #15)
>    template, so safe. (Though personally I'd prefer to FILTER HTML it
>    anyway as a defensive measure, that does not seem to be the norm.)

I agree that we should use |FILTER html| unless there are very good reasons not to do this. In this specific case, it's fine to add it to filterexceptions.pl as the content is filtered by FILTER format(), but I wouldn't be against using |FILTER html| instead, despite what gerv says in comment 13.
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → Bugzilla 3.8

Updated

8 years ago
Attachment #198324 - Attachment is obsolete: true

Updated

8 years ago
Attachment #242279 - Attachment is obsolete: true

Comment 19

8 years ago
Comment on attachment 404221 [details] [diff] [review]
patch for tip, v4

>Index: template/en/default/reports/report-table.html.tmpl

>+      <td class="tpercent" align="right">
>+        %
>+      </td>

Nit: I think we should display this column only of grand_total > 0.


>+      [% row_total_percent = 0 %]
>+      [% row_total_percent = row_total / grand_total * 100
>+         IF(grand_total != 0) %]

Group these lines with the related cell. The next cell doesn't use row_total_percent, making it hard to understand why you set it here.


>+      <td class="tpercent" align="right">
>+        [% row_total_percent FILTER format('%3.1f') %]
>       </td>

As said above, you should IMO only display the percentage column if grand_total > 0, so that you could simply write:

  [% IF grand_total %]
    <td class="tpercent">
      [% row_total / grand_total * 100 FILTER format('%3.1f') FILTER html %]
    </td>
  [% END %]

Note that I 1) explicitly added FILTER html so that it doesn't need to be in filterexceptions.pl, and 2) removed align="right" which should be set in the tpercent class in the CSS file.


>+      [% col_total_percent = 0 %]
>+      [% col_total_percent = col_totals.$col / grand_total * 100
>+         IF(grand_total != 0) %]

This code is unused, as you leave the loop without using it.


>+    <td class="ttotal tpercent" align="right">
>+      100.0
>+    </td>

Is that true when several tables are displayed?


>+    <td class="tpercent" align="right">

Here too, remove align="right".


>+      [% col_total_percent = 0 %]
>+      [% col_total_percent = col_totals.$col / grand_total * 100
>+         IF(grand_total != 0) %]

Use something similar to what I did above.


>+    <td class="none" align="center">
>+      [%# the intersection of the percentage columns is empty %]
>+    </td>

Add &nbsp; instead.


>Index: skins/standard/global.css

>+.tpercent {

Why is this class added here? I think it should be in skins/standard/reports.css.
Attachment #404221 - Flags: review?(LpSolit) → review-

Updated

7 years ago
Target Milestone: Bugzilla 4.0 → ---
You need to log in before you can comment on or make changes to this bug.