http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 --- Comment #1 from Jonathan Druart <jonathan.druart@biblibre.com> --- biblibre ref: MT5698 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 --- Comment #2 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 9266 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9266&action=edit Bug 7955: Statistics tab for Patron checkouts This patch adds a new 'statistics' tab in the Patron module. For a borrower, this tab contains a table with: - number of checkout for today - number of checkin for today - "precedent state", the number of checkouts the patron had yesterday on its library card - "actual state", the number of checkouts on the borrower card at the current date A new syspref (StatisticsFields) contains a list of fields (separated by pipe (|) on which the table results is based. The default value is location|itype|ccode -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 --- Comment #3 from MathildeF <mathilde.formery@ville-nimes.fr> --- Created attachment 9763 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9763&action=edit Screenshot -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 --- Comment #7 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 9776 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9776&action=edit Bug 7955: Followup: Changes the syspref description -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 MathildeF <mathilde.formery@ville-nimes.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #9 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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). -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #9776|0 |1 is obsolete| | Attachment #10015|0 |1 is obsolete| | --- Comment #11 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 10223 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10223&action=edit Bug 7955: Statistics tab for Patron checkouts (In reply to comment #9)

QA Comment:

Patch appears to have external signoff. But the commit does not show it. Please correct. Done

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

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 ;) I add a $dbh->quote

GetPrecedentStateByBorrower: Please confirm if the second where clause is correct. You say returndate=now while I was expecting returndate<now. Yes! For the precedent state, we want all the issues with an issuedate < today AND all the old issues returned today (but issued before today).

circ-menu.inc, circ-menu.tt, etc.: You are testing for statisticsview. OK. But it is always 1. No, statisticsview = 1 when we are on the statistics.pl page, else it is false. (circ-menu.* are include files)

patrons.pref: Please explain how to fill in this field (with | char). Perhaps mention again the default values. Done

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. Humm, it is not my opinion, but it is done :)

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. If you want, I can replace tilde operator with the eq operator. In this case, that changes nothing.

build_array function: Might need some more comments too. Makes maintenance easier. (The current oneliner does not really explain it to me..) I add few comments for merge and build_array functions

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. Done

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).

Thanks for your review ! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 --- Comment #12 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |paul.poulain@biblibre.com Version|master |rel_3_10 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 Nicole C. Engard <nengard@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |8318 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 --- Comment #17 from Nicole C. Engard <nengard@gmail.com> --- Created attachment 10518 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10518&action=edit 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 ... -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 Nicole C. Engard <nengard@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10410|0 |1 is obsolete| | --- Comment #19 from Nicole C. Engard <nengard@gmail.com> --- Created attachment 10520 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10520&action=edit [SIGNED-OFF] Bug 7955: Followup: Fix badly use of dbh->quote Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 Nicole C. Engard <nengard@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #21 from Nicole C. Engard <nengard@gmail.com> --- Actually I think this is adding another Notices tab to the patron record. I see Notices, Statistics, Notices on the left hand side. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 --- Comment #23 from Paul Poulain <paul.poulain@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 --- Comment #25 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10746|0 |1 is obsolete| | --- Comment #27 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 10747 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10747&action=edit Bug 7955: Followup : Check the syspref value (avoid sql injection) The syspref value is check. It have to filled with only word (\w), integer (\d), underscore (_), dash (-) or pipe (|) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 kyle.m.hall@gmail.com <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10747|0 |1 is obsolete| | --- Comment #28 from kyle.m.hall@gmail.com <kyle@bywatersolutions.com> --- Created attachment 10826 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10826&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 --- Comment #29 from Owen Leonard <oleonard@myacpl.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #31 from Paul Poulain <paul.poulain@biblibre.com> --- QA comment: small follow-up required to have the feature working. passed QA -- You are receiving this mail because: You are watching all bug changes.