http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7955
Jonathan Druart <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #9776|0 |1 is obsolete| | Attachment #10015|0 |1 is obsolete| | --- Comment #11 from Jonathan Druart <[email protected]> --- 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. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
