310940 - Add percentages next to tabular reports sub-totals

Status: ASSIGNED

(Bugzilla :: Reporting/Charting, enhancement)

Description

[kino]

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

Comment 1

[kino]

Attachment: Simple mockup of the feature request

Comment 2

[Gervase Markham [:gerv]]

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

Comment 3

[kino]

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.

Comment 4

[Olav Vitters]

kino: Could you post a diff between the old file and the newer one with the
modifications?

Comment 5

[victory <[e-mail]>]

Attachment: patch for tip

i think adding a css rule for ttotal class makes better look though :-p

Comment 6

[Gervase Markham [:gerv]]

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

Comment 7

[victory <[e-mail]>]

(In reply to comment #6)
> >+                    $percent =  sprintf("%04d", $percent * 1000);
> 1000? Are you sure?

see s/(\d)$/.$1/
then it always returns 3 (digit| ) + '.' + 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.

Comment 8

[Gervase Markham [:gerv]]

(In reply to comment #7)
> see s/(\d)$/.$1/
> then it always returns 3 (digit| ) + '.' + 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

Comment 9

[victory <[e-mail]>]

Attachment: screenshot

i'm not sure he or they want(s) this :-(

Comment 10

[victory <[e-mail]>]

Attachment: patch for tip 2

added other row/col so special hack used in previous isnt needed

Comment 11

[Gervase Markham [:gerv]]

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  'ed leading spaces

I don't see any code doing " '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

Comment 12

[victory <[e-mail]>]

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

Comment 13

[Gervase Markham [:gerv]]

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

[victory <[e-mail]>]

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

per previous comment

Comment 15

[GavinS]

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

Comment 16

[GavinS]

Attachment: screenshot of v4 patch

Comment 17

[Frédéric Buclin]

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

This looks cool. I will give it a look.

Comment 18

[Frédéric Buclin]

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

Comment 19

[Frédéric Buclin]

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.