biblibre ref: MT5698

Created attachment 9763 [details]
Screenshot

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

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

Mathilde,
If patches work as expected, could you change the status to signed off, please ?

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

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.

Checked the revised patch.
All points mentioned before got attention. As well as adding Datatables and a check on existing borrower.
Passed QA

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?"

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

Hi Nicole,
I proposed a followup to fix this issue.
See attachment 10410 [details] [review]

Why yes you did! Somehow I missed that.  Signed off!!

Actually I think this is adding another Notices tab to the patron record. I see Notices, Statistics, Notices on the left hand side.

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.

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

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 ?

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

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.

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

(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 ?

QA comment: small follow-up required to have the feature working. passed QA