[Koha-bugs] [Bug 11371] Add a new report : Orders by budget
bugzilla-daemon at bugs.koha-community.org
bugzilla-daemon at bugs.koha-community.org
Sat Mar 22 13:52:39 CET 2014
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11371
--- Comment #6 from Katrin Fischer <katrin.fischer at bsz-bw.de> ---
Comment on attachment 25915
--> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25915
[Signed-off]Add the "Orders by budget" report.
Review of attachment 25915:
--> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=11371&attachment=25915)
-----------------------------------------------------------------
Hi Frederick,
I know this has been in the loop for a while already, but I found some things
that I feel should be addressed. I hope my comments are understandable.
I. The QA script points out some things:
QA SCRIPT
testing 1 commit(s) (applied to 7408fa5 'Bug 8168: (follow-up) Use semicolon
a')
OK C4/Budgets.pm
OK pod
OK forbidden patterns
OK valid
OK critic
FAIL reports/orders_by_budget.pl
OK pod
FAIL forbidden patterns
forbidden pattern: Koha is now under the GPLv3 license (line 10)
FAIL valid
Scalar value @filtered_budgets[0] better written as
$filtered_budgets[0]
FAIL critic
I/O layer ":utf8" used at line 154, column 9. Use ":encoding(UTF-8)" to
get strict validation.
FAIL t/db_dependent/Acquisition.t
OK pod
FAIL forbidden patterns
forbidden pattern: tab char (line 135)
forbidden pattern: tab char (line 136)
forbidden pattern: tab char (line 134)
forbidden pattern: tab char (line 137)
OK valid
OK critic
FAIL t/db_dependent/Budgets.t
OK pod
FAIL forbidden patterns
forbidden pattern: tab char (line 68)
forbidden pattern: tab char (line 67)
forbidden pattern: tab char (line 58)
forbidden pattern: tab char (line 49)
forbidden pattern: tab char (line 50)
forbidden pattern: tab char (line 57)
OK valid
OK critic
FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reports/orders_by_budget.tt
FAIL forbidden patterns
forbidden pattern: tab char (line 36)
forbidden pattern: tab char (line 99)
forbidden pattern: tab char (line 88)
forbidden pattern: tab char (line 90)
forbidden pattern: tab char (line 95)
forbidden pattern: tab char (line 98)
forbidden pattern: tab char (line 100)
forbidden pattern: tab char (line 58)
forbidden pattern: tab char (line 101)
forbidden pattern: tab char (line 15)
forbidden pattern: tab char (line 62)
forbidden pattern: tab char (line 61)
forbidden pattern: tab char (line 77)
forbidden pattern: tab char (line 83)
forbidden pattern: tab char (line 78)
forbidden pattern: tab char (line 57)
forbidden pattern: tab char (line 96)
forbidden pattern: tab char (line 14)
forbidden pattern: tab char (line 82)
forbidden pattern: tab char (line 107)
forbidden pattern: tab char (line 80)
forbidden pattern: tab char (line 89)
forbidden pattern: tab char (line 59)
OK tt_valid
OK valid_template
FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt
FAIL forbidden patterns
forbidden pattern: tab char (line 65)
OK tt_valid
OK valid_template
Please fix those.
II. Testing the interface
1) I think the budgets should be funds in the interface, or fund (budget) for
the pull down list.
2) There is a small display issue with the legend 'Filters' on the first
fieldset. The legend shows inside the elemend, instead of above.
3) The list could be improved, showing only funds with orders. But that would
be an enhancement.
4) Lots of the date and currency formatting could be handled in the template.
But it works and dateformat is honored.
III Code review
Generally I think there is a bit of confusion about the terminology, visible in
the interface and in the naming and documentation of the new routines.
The hierarchies in Koha speak are something like:
basket group - basket - order/order line
budget - fund - child funds
I have tried to point those out and also found a few other things:
::: C4/Budgets.pm
@@ +476,4 @@
> }
>
> # -------------------------------------------------------------------
> +sub GetBudgetPeriodDescription {
1) Please add complete documentation to the new routine - I know Budget.pm is a
bit sparse on that, but it's still worth not making it worse :) (minor)
2) Please also document the return values (see also below) (minor)
@@ +725,5 @@
> +=head2 GetBudgetReport
> +
> + &GetBudgetReport();
> +
> +Get one specific budget for reports without cancelled baskets.
3) Documentation could be a bit more clear: Get all orders for a specific fund,
without cancelled orders.
@@ +739,5 @@
> + FROM aqbudgets b
> + INNER JOIN aqorders o
> + ON b.budget_id = o.budget_id
> + WHERE b.budget_id=?
> + AND (o.datecancellationprinted IS NULL OR o.datecancellationprinted='0000-00-00')
4) The SQL could make use of the new orderstatus column in aqorders to filter
out the cancelled orders now. (minor)
@@ +755,5 @@
> +=head2 GetBudgetsReport
> +
> + &GetBudgetsReport();
> +
> +Get all budgets for reports without cancelled baskets.
5) This seems to be mostly a copy of GetActiveBudgetsReport but for the Active
bit. Why not move those into one and use a parameter?
Suggestion for improving the description: Get all but cancelled orders for all
funds.
@@ +779,5 @@
> + }
> + return @results;
> +}
> +
> +=head2 GetActiveBudgets
6) I think the name doesn't fit the use of the routine. GetActiveBudgets will
give you inactive budgets when you call it with parameter 0?
::: koha-tmpl/intranet-tmpl/prog/en/modules/reports/orders_by_budget.tt
@@ +35,5 @@
> + <th>Basket</th>
> + <th>Basket by</th>
> + <th>Title</th>
> + <th>Currency</th>
> + <th>Vendor Price</th>
7) Tiny thing: capitalization (trivial)
::: koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt
@@ +61,4 @@
> <h2>Other</h2>
> <ul>
> <li><a href="/cgi-bin/koha/reports/itemslost.pl">Items lost</a></li>
> + <li><a href="/cgi-bin/koha/reports/orders_by_budget.pl">Orders by budget</a></li>
8) Should be orders by fund.
::: reports/orders_by_budget.pl
@@ +174,5 @@
> + print '"Total RRP"' . $sep;
> + print '"Total cost"' . $sep;
> + print '"Entry date"' . $sep;
> + print '"Date received"' . $sep;
> + print '"Notes"' . "\n";
9) These headers won't be translatable. It's an old problem we have solved
recently using a TT include. Take a look at the other CSV export options in
acquisitions especially. Could be an enhancement.
@@ +204,5 @@
> + }
> +}
> +else {
> + # Set file export choices
> + my $CGIextChoice = CGI::scrolling_list(
10) CGI::scrolling_list shouldn't be used in new scripts, as we are trying to
get rid of it totally - see bug 766.
@@ +212,5 @@
> + -size => 1,
> + -multiple => 0
> + );
> +
> + my $CGIsepChoice = GetDelimiterChoices;
11) I am not sure there, but avoiding CGIsepChoice might also be preferrable.
--
You are receiving this mail because:
You are watching all bug changes.
More information about the Koha-bugs
mailing list