Bug 25220

To Test:
1) Set maxoutstanding to blank
2) give yourself $0.50 in fines
3) go to opac, confirm you're blocked from placing holds
4) set maxoutstanding=1
5) confirm you are no longer blocked from placing holds

Created attachment 103377 [details] [review]
Bug 25220: maxoutstanding ignored if null in opac-reserve.pl

To Test:
1) set maxoutstanding to empty
2) give yourself a fine of $1
3) try to place a hold on the opac
4) confirm you cannot place hold
5) apply patch, restart_all
6) reload opac
7) confirm hold is allowed
8) set maxoutstanding to $2
9) confirm hold is now blocked

Created attachment 103378 [details] [review]
Bug 25220: make maxoutstanding set to null not give warning on opac-user.pl

To test:
1) set maxoutstanding to empty
2) give yourself $1 in fine
3) go to your account on OPAC
4) confirm you have message that holds are blocked
5) apply patch, restart_all
6) reload OPAC
7) confirm message is gone
8) set maxoutstanding to $2
9) reload opac, confirm message is back

Correcting / consolidating test plan:

To test:
1) set maxoutstanding to empty
2) give yourself $1 in fine
3) go to your account on OPAC
4) confirm you have message that holds are blocked
5) try to place a hold on opac
6) confirm hold is blocked
7) apply patch, restart_all
8) reload OPAC
9) confirm you can now place a hold
10) confirm message on your account is gone
11) set maxoutstanding to $.5
12) reload opac
13) confirm you are blocked from hold
14) confirm message on account is back

Created attachment 103388 [details] [review]
Bug 25220: atomicupdate to maintain behavior of existing installs

To test:
1) on an existing install, set maxoutstanding to null
2) apply patch
3) confirm maxoutstanding is now set to zero

Created attachment 104506 [details] [review]
Bug 25220: maxoutstanding ignored if null in opac-reserve.pl

To Test:
1) set maxoutstanding to empty
2) give yourself a fine of $1
3) try to place a hold on the opac
4) confirm you cannot place hold
5) apply patch, restart_all
6) reload opac
7) confirm hold is allowed
8) set maxoutstanding to $2
9) confirm hold is now blocked

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 104507 [details] [review]
Bug 25220: make maxoutstanding set to null not give warning on opac-user.pl

To test:
1) set maxoutstanding to empty
2) give yourself $1 in fine
3) go to your account on OPAC
4) confirm you have message that holds are blocked
5) apply patch, restart_all
6) reload OPAC
7) confirm message is gone
8) set maxoutstanding to $2
9) reload opac, confirm message is back

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 104508 [details] [review]
Bug 25220: atomicupdate to maintain behavior of existing installs

To test:
1) on an existing install, set maxoutstanding to null
2) apply patch
3) confirm maxoutstanding is now set to zero

Amended during signoff: Corrected atomicupdate file extension from .pl
to .perl.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

C4/SIP/ILS/Patron.pm:          && $self->fee_amount > C4::Context->preference("maxoutstanding") );
circ/offline.pl:$template->{'VARS'}->{'maxoutstanding'} =
circ/offline.pl:  C4::Context->preference('maxoutstanding') || 0;
installer/data/mysql/atomicupdate/bug_25220.perl:    $dbh->do( "UPDATE systempreferences set value=0 where variable='maxoutstanding' and (value='' or value is null)" );
installer/data/mysql/atomicupdate/bug_25220.perl:    NewVersion( $DBversion, 25220, "maxoutstanding should be disabled by a null value");
installer/data/mysql/sysprefs.sql:('maxoutstanding','5','','maximum amount withstanding to be able make holds','Integer'),
koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref:            - pref: maxoutstanding
koha-tmpl/intranet-tmpl/prog/en/modules/circ/offline.tt:                if (parseInt(patron.fine) > [% maxoutstanding | html %]) {
koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt:                                    [% IF amount_outstanding && Koha.Preference('maxoutstanding') && amount_outstanding > Koha.Preference('maxoutstanding') %]
koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt:                [% IF ( expiry || diffbranch || patron.is_debarred || ( amount_outstanding && Koha.Preference('maxoutstanding') && amount_outstanding > Koha.Preference('maxoutstanding') ) ) %]
koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt:                            [% IF amount_outstanding && Koha.Preference('maxoutstanding') && amount_outstanding > Koha.Preference('maxoutstanding') %]


opac/opac-reserve.pl:my $maxoutstanding = C4::Context->preference("maxoutstanding");
opac/opac-reserve.pl:$maxoutstanding = undef unless looks_like_number($maxoutstanding);
opac/opac-reserve.pl:$template->param( noreserve => 1 ) unless $maxoutstanding;
opac/opac-reserve.pl:if ( $amountoutstanding && defined($maxoutstanding) && ($amountoutstanding > $maxoutstanding) ) {
opac/opac-user.pl:my $maxoutstanding = C4::Context->preference('maxoutstanding');
opac/opac-user.pl:$maxoutstanding = undef unless looks_like_number ( $maxoutstanding );
opac/opac-user.pl:if ( $amountoutstanding && defined($maxoutstanding) && ( $amountoutstanding > $maxoutstanding ) ){

Are you sure that you only needed to change opac-reserve and opac-user ? If I am looking at this git grep, I wouldnt.
Also note that the test Koha.Pref( ) && a > b, looks a bit more elegant, although it could trigger warnings on non-numerical comparison if you wrote "abc" in maxoutstanding. But who would?

+    $dbh->do( "UPDATE systempreferences set value=0 where variable='maxoutstanding' and (value='' or value is null)" );
This does not look good. Value is a text field. You mean '0'? :)
I think we dont need this change. And also you dont correct 'abc'?