Bug 7896

Summary: Acquisition statistics wizard: add filters and cell values
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: ReportsAssignee: Julian Maurice <julian.maurice>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: f.demians, katrin.fischer, koha.aixmarseille, kyle, stephane.delaye
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 7175    
Bug Blocks:    
Attachments: Acq statistics wizard: add filters and cell values
Acq statistics wizard: add filters and cell values
Bug 7896: follow-up: Allow translation of strings
[SIGNED-OFF] Bug 7896: Acq statistics wizard: add filters and cell values
[SIGNED-OFF] Bug 7896: follow-up: Allow translation of strings
[SIGNED-OFF] Bug 7896: Acq statistics wizard: add filters and cell values
[SIGNED-OFF] Bug 7896: follow-up: Allow translation of strings

Description Julian Maurice 2012-04-04 10:45:24 UTC
This development, sponsored by Aix-Marseille university, add the following changes:
  - Possibility to select for line and column: items.homebranch and items.ccode
  - Possibility to filter on these fields
  - Possibility to count unique biblios (count(distinct biblionumber)), ordered amount and spent amount (based on aqorders.datereceived)
Comment 1 Julian Maurice 2012-04-04 11:43:44 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2012-04-04 11:45:40 UTC
Note that partially received orders can make the calculation wrong since partially received orders are splitted, but links to created items are not updated. This is fixed by bug 7175, so marking this one as dependent of 7175
Comment 3 delaye 2012-04-04 12:05:06 UTC
i try to test the patch with sandbox 3 but i have this message when i want open the statistic tool


Software error:

Type of arg 1 to keys must be hash (not private variable) at /home/koha/src/reports/acquisitions_stats.pl line 246, near "$branches ) "
Execution of /home/koha/src/reports/acquisitions_stats.pl aborted due to compilation errors.
Comment 4 Julian Maurice 2012-04-04 12:42:54 UTC Comment hidden (obsolete)
Comment 5 delaye 2012-04-04 14:15:11 UTC
i have tested in a sandbox (3) it works

Some details for those who want to test

amount = ammount spended + ammount ordered

collection = items.ccode
Comment 6 Frédéric Demians 2012-07-09 07:41:47 UTC
I can't test completly this patch because I haven't enough acquisition
data. But it seems to work.

Two remarks/demands:

[1] GetMarcSubfieldStructureFromKohaField new routine is querying
marc_subfield_structure table. It should be cached. Or, since it must
have been already retrieved, and cached by GetMarcStructure, the cached
version of frameworks must be used.

[2] Some texts are not translatable. It was already there, but your
patch add text that can't be translated:

In reports/acquisitions_stats.pl, line 126, there is:

 $select{''} = "All Suppliers";

Since it isn't in template, it can't be translated.

The is also:

  given ($i) {
    when (0)  { $cell{crit} = "Placed On From" }
    when (1)  { $cell{crit} = "Placed On To" }
    when (2)  { $cell{crit} = "Received On From" }
    when (3)  { $cell{crit} = "Received On To" }
    when (4)  { $cell{crit} = "Bookseller" }
    when (5)  { $cell{crit} = "Home branch" }
    when (6)  { $cell{crit} = "Collection" }
    when (7)  { $cell{crit} = "Doc Type" }
    when (8)  { $cell{crit} = "Budget" }
    when (9)  { $cell{crit} = "Sort1" }
    when (10) { $cell{crit} = "Sort2" }
    default   { $cell{crit} = "" }

Could you propose a fix to help translate this page?
Comment 7 Julian Maurice 2012-07-24 11:40:29 UTC Comment hidden (obsolete)
Comment 8 Julian Maurice 2012-07-24 11:48:06 UTC
(In reply to comment #6)
> I can't test completly this patch because I haven't enough acquisition
> data. But it seems to work.
> 
> Two remarks/demands:
> 
> [1] GetMarcSubfieldStructureFromKohaField new routine is querying
> marc_subfield_structure table. It should be cached. Or, since it must
> have been already retrieved, and cached by GetMarcStructure, the cached
> version of frameworks must be used.

Why ?

It is used only once, in a report page, outside of any loop.
I don't think we should continue to use the same caching method as the one used
for GetMarcStructure as it is not compatible with Plack and mod_perl (and any
method that provides persistency)

> 
> [2] Some texts are not translatable. It was already there, but your
> patch add text that can't be translated:
> 

Fixed in the new patch.
Comment 9 Kyle M Hall 2012-10-19 13:26:48 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2012-10-19 13:27:06 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2012-12-02 10:40:24 UTC
Hi Julian, 

I looked at the patch and there are lots of smaller things I noted while testing:

1) Terms and capitalization
- Budget/All budgets is labelled wrong, as it really shows the funds
- All Item Types should be 'All item types'
- All Suppliers should be 'All vendors'

2) Untranslatable terms
- $select{''} = "All budgets";
- $select{''} = "All Item Types";

3) POD
- Old copy & paste error: plugin that shows a stats on borrowers

4) Item type - biblio level only?
- Looking at the code it seems like only bibio level itypes are taken into 
  account. I wonder if the stats should use 'item-level itypes'. If that can't
  be done, a little note would be helpful.

5) Error logs

(can be reproduced by sending the form with preselected options)

[Sun Dec 02 10:24:14 2012] [error] [client 127.0.0.1] [Sun Dec  2 10:24:14 2012] acquisitions_stats.pl: Use of uninitialized value in addition (+) at /home/katrin/kohaclone/reports/acquisitions_stats.pl line 582., referer: http://localhost:8080/cgi-bin/koha/reports/acquisitions_stats.pl

That all said, perlcritic passes, all tests pass, and most problems are preexistant to your patches. But I would love to see a follow up addressing some of the other problems.
Comment 12 Katrin Fischer 2012-12-02 10:43:06 UTC
Created attachment 13818 [details] [review]
[SIGNED-OFF] Bug 7896: Acq statistics wizard: add filters and cell values

- Possibility to select for line and column: items.homebranch and
  items.ccode
- Possibility to filter on these fields
- Possibility to count unique biblios (count(distinct biblionumber)),
  ordered amount and spent amount (based on aqorders.datereceived)

Filtering on item homebranch and ccode works only on items that were
created at ordering or receiving (ie items are linked to an order)

Some refactoring is done, mainly replacing switch-like if statements by
given/when

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 13 Katrin Fischer 2012-12-02 10:43:40 UTC
Created attachment 13819 [details] [review]
[SIGNED-OFF] Bug 7896: follow-up: Allow translation of strings

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 14 Jared Camins-Esakov 2012-12-02 14:43:50 UTC
This patch has been pushed to master.

As a note, per this page http://perldoc.perl.org/perlsyn.html#Switch-Statements we try to avoid given statements. As your follow-up removed the given statement, this is a non-issue for this patch, of course.
Comment 15 Julian Maurice 2012-12-10 12:41:46 UTC
Ok in master.