[Koha-bugs] [Bug 7955] Statistics tab in patron module

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955

Jonathan Druart <jonathan.druart at 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 at 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.