[Koha-bugs] [Bug 11371] Add a new report : Orders by budget

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

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11371

--- Comment #39 from Katrin Fischer <katrin.fischer at bsz-bw.de> ---
Comment on attachment 45982
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45982
Bug 11371 - Add a new report : Orders by budget

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

I feel we are getting closer with this, even if it might not feel like it. Can
you please take a look at my new comments and the one Jonathan has made?

There is one additional thing not noted in the splitter comments: The new
report should also be added to the reports module navigation (I think it's
reports-menu.inc), that shows up on the left side once you have opened a
reports page.

::: koha-tmpl/intranet-tmpl/prog/en/modules/reports/orders_by_budget.tt
@@ +38,5 @@
> +        <thead>
> +        <tr>
> +        <th>Fund</th>
> +        <th>Basket</th>
> +        <th>Basket Name</th>

Please fix capitalization in the list of column names. We have agreed that only
the first word should be capitalized.

@@ +42,5 @@
> +        <th>Basket Name</th>
> +        <th>Basket By</th>
> +        <th>Title</th>
> +        <th>Currency</th>
> +        <th>Vendor Price</th>

Please use list price to match acquisition terminology.

::: reports/orders_by_fund.pl
@@ +26,5 @@
> +=cut
> +
> +use strict;
> +use warnings;
> +

Please use Modern::Perl.

@@ +111,5 @@
> +
> +        # Format the dates and currencies correctly
> +        $order->{'datereceived'} = Koha::DateUtils::output_pref(Koha::DateUtils::dt_from_string($order->{'datereceived'}));
> +        $order->{'entrydate'} = Koha::DateUtils::output_pref(Koha::DateUtils::dt_from_string($order->{'entrydate'}));
> +        $order->{'listprice'} = sprintf( "%.2f", $order->{'listprice'} );

It would be nicer to handle the display on template level with the new Price TT
plugin.

@@ +221,5 @@
> +    foreach my $thisbudget ( sort {$a->{budget_name} cmp $b->{budget_name}} @non_active_budgets ) {
> +        my $budget_period_desc = C4::Budgets::GetBudgetPeriodDescription($thisbudget->{budget_id});
> +        my %row = (
> +            value       => $thisbudget->{budget_id},
> +            description => "[i] ". $thisbudget->{budget_name},

The [i] is a translation issue. I think as it's in the template now it's not
translatable at all, but I would also prefer another way of displaying it, as
[i] is not very clear and will probably cause confusion for translators.
Looking in other spots of the acquisition module I have some suggestions:
- Don't show the inactive funds to begin with, but only, when a checkbox is
checked (see neworderempty.pl) and then put (inactive) behind the inactiv fund
name.
- Use a hierarchical display for more than one budget and it's funds like on
histsearch.pl

::: t/db_dependent/Budgets.t
@@ +2,2 @@
>  use Modern::Perl;
> +use Test::More ;

Please don't remove the number of tests to run.

-- 
You are receiving this mail because:
You are watching all bug changes.