[Koha-bugs] [Bug 6934] New report Cash Register Statistics

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6934

--- Comment #23 from Katrin Fischer <katrin.fischer at bsz-bw.de> ---
Comment on attachment 36476
  --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36476
[SIGNED-OFF] Bug 6934 New report Cash Register Statistics

Review of attachment 36476:
 --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=6934&attachment=36476)
-----------------------------------------------------------------

Hi Simith,

I have added a few comments from code review - please fix in a follow-up. I
have only marked the first occurrence of each, please also look below and above
to make sure to get them all. 

Thx!

::: koha-tmpl/intranet-tmpl/prog/en/includes/reports-menu.inc
@@ +10,4 @@
>  		<li><a href="/cgi-bin/koha/reports/catalogue_stats.pl">Catalog</a></li>
>  		<li><a href="/cgi-bin/koha/reports/issues_stats.pl">Circulation</a></li>
>          <li><a href="/cgi-bin/koha/reports/serials_stats.pl">Serials</a></li>
> +        <li><a href="/cgi-bin/koha/reports/cash_register_stats.pl">Cash Register</a></li>

Please fix capitalization here and in the other template files to obey our
rules. Thx!

::: koha-tmpl/intranet-tmpl/prog/en/modules/reports/cash_register_stats.tt
@@ +7,5 @@
> +<script type="text/javascript" src="[% themelang %]/js/datatables.js"></script>
> +<script type="text/javascript" id="js">$(document).ready(function() {
> + $(document).ready(function() {
> +    $("#tbl_cash_register_stats").dataTable($.extend(true, {}, dataTablesDefaults, {
> +        "iDisplayLength": 50

Be careful, without adding a paging option here, the paging on the table is
broken (see recent 'paging' bugs for examples on how to fix)

@@ +143,5 @@
> +                        <option value="F">Fine</option>
> +                        [% END %]
> +
> +                        [% IF transaction_type == "FU" %]
> +                        <option value="FU" selected="selected">Fine - long period</option>

Please check fine descriptions with those in the other templates - I think this
one is not quite right.

@@ +233,5 @@
> +        <thead>
> +        <tr>
> +            <th>Manager name</th>
> +            <th>Borrower cardnumber</th>
> +            <th>Borrower name</th>

Please use patron instead of borrower.

@@ +234,5 @@
> +        <tr>
> +            <th>Manager name</th>
> +            <th>Borrower cardnumber</th>
> +            <th>Borrower name</th>
> +            <th>Branch</th>

Please use library instead of branch.

@@ +240,5 @@
> +            <th>Transaction type</th>
> +            <th>Amount</th>
> +            <th>Biblio title</th>
> +            <th>Barcode</th>
> +            <th>Document type</th>

Please use item type.

::: reports/cash_register_stats.pl
@@ +14,5 @@
> +# with Koha; if not, write to the Free Software Foundation, Inc.,
> +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +
> +use strict;
> +use warnings;

Please use Modern::Perl; instead.

@@ +22,5 @@
> +use C4::Reports;
> +use C4::Output;
> +use C4::Koha;
> +use C4::Circulation;
> +use C4::Dates qw/format_date format_date_in_iso/;

Please avoid use of C4::Dates as it's deprecated.

@@ +45,5 @@
> +my $output           = $input->param("output");
> +my $basename         = $input->param("basename");
> +my $transaction_type = $input->param("transaction_type") || 'ACT';
> +my $branchcode       = $input->param("branch") || C4::Context->userenv->{'branch'};
> +our $sep = ",";

I think the separator should not be hardcoded, it would be better to use a pull
down like on the other reports or using the delimiter system preference.

@@ +49,5 @@
> +our $sep = ",";
> +
> +$template->param(
> +    do_it => $do_it,
> +    DHTMLcalendar_dateformat => C4::Dates->DHTMLcalendar(),

We are no longer using DHTML calendar, please use the TT Dates plugin instead.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are watching all bug changes.