Bug 7955 - Statistics tab in patron module
Summary: Statistics tab in patron module
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 8318 8319
  Show dependency treegraph
 
Reported: 2012-04-13 10:16 UTC by Jonathan Druart
Modified: 2013-05-23 06:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7955: Statistics tab for Patron checkouts (17.19 KB, patch)
2012-04-20 09:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Screenshot (21.33 KB, image/jpeg)
2012-05-24 15:41 UTC, MathildeF
Details
Bug 7955: Statistics tab for Patron checkouts (17.23 KB, patch)
2012-05-25 07:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7955: Followup: Changes the syspref description (2.00 KB, patch)
2012-05-25 12:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Rebased patch (17.25 KB, patch)
2012-06-09 11:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7955: Statistics tab for Patron checkouts (19.00 KB, patch)
2012-06-11 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Statistics table after checkouts and checkins (7.13 KB, image/png)
2012-06-21 12:58 UTC, Owen Leonard
Details
Bug 7955: Followup: Fix badly use of dbh->quote (954 bytes, patch)
2012-06-21 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7955: Followup: Fix badly use of dbh->quote (999 bytes, patch)
2012-06-21 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
item type, collections,locations not showing (17.75 KB, image/png)
2012-06-26 15:00 UTC, Nicole C. Engard
Details
[SIGNED-OFF] Bug 7955: Followup: Fix badly use of dbh->quote (1.04 KB, patch)
2012-06-26 15:29 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 7955: Followup : Check the syspref value (avoid sql injection) (2.71 KB, patch)
2012-07-10 14:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7955: Followup : Check the syspref value (avoid sql injection) (2.71 KB, patch)
2012-07-10 14:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7955: Followup : Check the syspref value (avoid sql injection) (2.88 KB, patch)
2012-07-13 16:39 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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