[Koha-bugs] [Bug 7955] Statistics tab in patron module
bugzilla-daemon at bugs.koha-community.org
bugzilla-daemon at bugs.koha-community.org
Sat Jun 9 13:16:43 CEST 2012
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955
M. de Rooy <m.de.rooy at rijksmuseum.nl> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|Signed Off |Failed QA
--- Comment #9 from M. de Rooy <m.de.rooy at 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.
More information about the Koha-bugs
mailing list