[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