[Koha-bugs] [Bug 8236] Prevent renewing if overdue or restriction

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

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

David Cook <dcook at prosentient.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Signed Off                  |Failed QA
                 CC|                            |dcook at prosentient.com.au

--- Comment #30 from David Cook <dcook at prosentient.com.au> ---
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.