http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8236
David Cook <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |[email protected] --- Comment #30 from David Cook <[email protected]> --- First of all, I want to say that this patch is quite important and I'm keen to see it get through ASAP. However, I'm failing it for a few reasons... 1) The code isn't applied consistently/doesn't take the system preference into account across all the different scripts: The code in ILSDI/Services.pm looks ok: "my $norenewal = 1 if ( $overduesblockrenew eq 'blockall' and $memberblocked == -1 ) or ( $overduesblockrenew eq 'blockitem' and $overdue ) or $memberblocked == 1;# last to check if patron is restricted" - However, circulation.pl, moremember.pl, and opac-user.pl all use: "my $norenewal = 1 if $overduesblockrenew eq 'blockall' and $memberblocked == -1;" - They really should all be using the first code snippet, since that correctly takes the "OverduesBlockRenew" syspref into account. PLUS, it's important to check for $memberblocked == 1 to make sure that the patron/borrower isn't restricted! -- 2) That all said, I agree with JDruart. This code really should be either factorized into CanBookBeRenewed or into its own sub/function. I understand what you're saying Oliver about not wanting to call the code for every item, except...the system preference has the "blockitem" option, which will need to be checked for every item in the loop. -- 3) I haven't tried this patch yet, but I think the templates (at least circulation.tt) might have some issues too...there are separate checks for "previssue.renew_error_too_many" and "previssue.renew_error_overdue" which means that you could in theory have a line in the check out window that says "Not renewable Renewal not allowed (overdue)". These should probably be 1 If/ElseIf statement rather than 2 separate If statements. -- Ultimately, I think this needs an overhaul, but it's certainly a worthwhile patch that I hope gets in soon :) -- 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/
