Bug 7955

Summary: Statistics tab in patron module
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: PatronsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, kyle, m.de.rooy, mathilde.formery, nengard, paul.poulain
Version: 3.10   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 8318, 8319    
Attachments: Bug 7955: Statistics tab for Patron checkouts
Screenshot
Bug 7955: Statistics tab for Patron checkouts
Bug 7955: Followup: Changes the syspref description
Rebased patch
Bug 7955: Statistics tab for Patron checkouts
Statistics table after checkouts and checkins
Bug 7955: Followup: Fix badly use of dbh->quote
Bug 7955: Followup: Fix badly use of dbh->quote
item type, collections,locations not showing
[SIGNED-OFF] Bug 7955: Followup: Fix badly use of dbh->quote
Bug 7955: Followup : Check the syspref value (avoid sql injection)
Bug 7955: Followup : Check the syspref value (avoid sql injection)
Bug 7955: Followup : Check the syspref value (avoid sql injection)

Description Jonathan Druart 2012-04-13 10:16:11 UTC
It would be great to have a tab (in the patron module) with statistics (number of issues by document type)
Comment 1 Jonathan Druart 2012-04-13 11:11:24 UTC
biblibre ref: MT5698
Comment 2 Jonathan Druart 2012-04-20 09:33:05 UTC Comment hidden (obsolete)
Comment 3 MathildeF 2012-05-24 15:41:40 UTC
Created attachment 9763 [details]
Screenshot
Comment 4 MathildeF 2012-05-24 15:51:09 UTC
I'm not sure to understand, or i'm not sure it works correctly :-)

see the screenshot

A borrower, with 3 docs. column "precedent state" ->OK

I make 3 issues (2 CD, 1 book) and 1 return (1 book)

Column "actuel state" row Total : 5 items -> OK
but only 3 items in the detail (it's the 3 items that I have issued ?)

Same case with the column "Issues" and "issues returned".
Total is OK, but i do not explain the other rows
Comment 5 Jonathan Druart 2012-05-25 07:49:06 UTC Comment hidden (obsolete)
Comment 6 MathildeF 2012-05-25 12:37:15 UTC
Indeed, this is much clearer !

everything works now

Maybe specify in the syspref that these are the fields of the table items ? (isn't it ?)

A tiny comment about the translation : the only term currently translated : "Issues" is "fascicule" in French ...
Comment 7 Jonathan Druart 2012-05-25 12:57:03 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2012-05-25 13:01:04 UTC
Mathilde,
If patches work as expected, could you change the status to signed off, please ?
Comment 9 Marcel de Rooy 2012-06-09 11:16:43 UTC
QA Comment:

Rebased the updatedatabase part. Will attach it.
Patch appears to have external signoff. But the commit does not show it. Please correct. 

Statistics.pm: $VERSION = 3.02; Do we need it? Why 3.02? Seems just to be copied..

construct_query: This routine is not very clear. Note also that you do not check the contents of the pref: column whatever can be put into the pref and will just be copied over. So it is somehwat error sensitive (not even talking about security issues).
This point is not a definitive blocker for me, but if you can improve this code, you are welcome ;)

GetPrecedentStateByBorrower: Please confirm if the second where clause is correct. You say returndate=now while I was expecting returndate<now. 
circ-menu.inc, circ-menu.tt, etc.: You are testing for statisticsview. OK. But it is always 1.
patrons.pref: Please explain how to fill in this field (with | char). Perhaps mention again the default values.

statistics.pl
general remark: In your code I saw some case like this one:
my $precedent_state = GetPrecedentStateByBorrower $borrowernumber;
Since the function is declared before, this is allowed. But calling subroutines without parentheses could cause problems, especially with multiple pars. I would prefer to always add the parentheses. It also makes it more visible as a function call. 

merge function: Not sure what you are doing there exactly and why in that way. Can this be done easier and more transparently? 
This line in particular calls for clarification: 
if ( not $ch->{$cn} ~~ $h->{$cn} ) {
Note that tilde tilde operator is a way to force scalar context. It makes code obscure; better avoid it.
Please clarify. 

build_array function: Might need some more comments too. Makes maintenance easier. (The current oneliner does not really explain it to me..)

Warning in the logfile: [Sat Jun 09 10:34:39 2012] [error] [client 129.215.5.255] [Sat Jun  9 12:34:39 2012] statistics.pl: Use of uninitialized value in addition (+) at /usr/share/koha/testclone/members/statistics.pl line 74. Please check undefined values. 

statistics.tt: variable unknowuser: Is a correct typo ;) Should be corrected in members.pl and some templates. (But outside scope of your report)

In conclusion: There are a few comments and questions needing attention before  passing qa now rightaway. Please clarify or correct to "save this kitten" (hackfest term).
Comment 10 Marcel de Rooy 2012-06-09 11:20:58 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2012-06-11 15:06:19 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2012-06-14 10:26:18 UTC
Hi Jonathan:
Just came across this sentence from Perl Best Practices:

More importantly, leaving off the parentheses on subroutines makes them harder to distinguish from builtins, and therefore increases the mental search space when the reader is confronted with either type of construct. Your code will be easier to read and understand if the subroutines always use parentheses and the built-in functions always don’t.
Comment 13 Marcel de Rooy 2012-06-14 11:41:48 UTC
Checked the revised patch.
All points mentioned before got attention. As well as adding Datatables and a check on existing borrower.
Passed QA
Comment 14 Owen Leonard 2012-06-21 12:58:44 UTC
Created attachment 10407 [details]
Statistics table after checkouts and checkins

After two checkouts and three or four checkins this is what the new statistics page looks like. Are those numbers wrong or am I missing something? Why do the location, itype, and ccode columns say "location," "itype," and "ccode?"
Comment 15 Jonathan Druart 2012-06-21 14:11:45 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2012-06-21 14:18:19 UTC Comment hidden (obsolete)
Comment 17 Nicole C. Engard 2012-06-26 15:00:37 UTC
Created attachment 10518 [details]
item type, collections,locations not showing

I'm on master as of this morning and my stats look like the attached. It doesn't show item types, locations or collection - instead it just repeats the headings again ...
Comment 18 Jonathan Druart 2012-06-26 15:04:57 UTC
Hi Nicole,
I proposed a followup to fix this issue.
See attachment 10410 [details] [review]
Comment 19 Nicole C. Engard 2012-06-26 15:29:11 UTC Comment hidden (obsolete)
Comment 20 Nicole C. Engard 2012-06-26 15:29:41 UTC
Why yes you did! Somehow I missed that.  Signed off!!
Comment 21 Nicole C. Engard 2012-06-29 15:59:10 UTC
Actually I think this is adding another Notices tab to the patron record. I see Notices, Statistics, Notices on the left hand side.
Comment 22 Jonathan Druart 2012-07-02 10:05:46 UTC
Hi Nicole,

It's not this patch. The issue exists in master. I switch back to signed off.

I point out the problem to Paul.
Comment 23 Paul Poulain 2012-07-02 12:54:27 UTC
(In reply to comment #21)
> Actually I think this is adding another Notices tab to the patron record. I
> see Notices, Statistics, Notices on the left hand side.

Was a duplicate introduced by a mistake in rebase of bug 8130. I just pushed a follow-up to fix this problem.
Comment 24 Paul Poulain 2012-07-04 15:09:57 UTC
QA comment (and some discussion with joubu)

(In reply to comment #9)
> construct_query: This routine is not very clear. Note also that you do not
> check the contents of the pref: column whatever can be put into the pref and
> will just be copied over. So it is somehwat error sensitive (not even
> talking about security issues).

Anyone who can set systempreferences can completely break Koha anyway. So I don't think it's a necessity to add some sanity check.
The way the follow-up patch is made look quite ugly to me, it would be better to check if we've only letters, numbers, - and _ as column names.

And, imo, we don't need any check at all in fact. Marcel, your opinion on that ?
Comment 25 Marcel de Rooy 2012-07-05 06:57:22 UTC
(In reply to comment #24)
> QA comment (and some discussion with joubu)
> 
> Anyone who can set systempreferences can completely break Koha anyway. So I
> don't think it's a necessity to add some sanity check.
> The way the follow-up patch is made look quite ugly to me, it would be
> better to check if we've only letters, numbers, - and _ as column names.
> 
> And, imo, we don't need any check at all in fact. Marcel, your opinion on
> that ?

We will not improve quality by skipping checks :-) Let's not make it worse (sounds like a commercial..).
I agree that the followup does look somewhat "ugly". Adding quotes and removing them again.. 
A check on valid column names would be nice, but I understand that it could be a little over the top for this routine.
In conclusion: If you still have a good idea, please submit. Or remove the current quote thing..
Comment 26 Jonathan Druart 2012-07-10 14:05:36 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2012-07-10 14:07:08 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2012-07-13 16:39:32 UTC
Created attachment 10826 [details] [review]
Bug 7955: Followup : Check the syspref value (avoid sql injection)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Works as expected. Fields with disallowed characters do not show up.
Added 'if $debug' to an psuedo-unconditional warn.
Comment 29 Owen Leonard 2012-07-31 18:21:08 UTC
(In reply to comment #14)
> Created attachment 10407 [details]
> Statistics table after checkouts and checkins
> 
> After two checkouts and three or four checkins this is what the new
> statistics page looks like. Are those numbers wrong or am I missing
> something? Why do the location, itype, and ccode columns say "location,"
> "itype," and "ccode?"

My comment still applies to this feature's current state. It doesn't look to me like statistics are being recorded at all.
Comment 30 Jonathan Druart 2012-07-31 19:43:00 UTC
(In reply to comment #29)

> My comment still applies to this feature's current state. It doesn't look to
> me like statistics are being recorded at all.

Hi Owen, 

I proposed a patch (attachment 10826 [details] [review]) the 2nd of July. It has been signed off, but still waits for QA. Could you retry after applying this patch and confirm  it fixes your issue please ?
Comment 31 Paul Poulain 2012-08-01 14:07:22 UTC
QA comment: small follow-up required to have the feature working. passed QA