From 02e6c95c0649e7955ff0c7a58d84e159fd7e533b Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Thu, 20 Nov 2025 09:30:15 +0000 Subject: [PATCH 15/16] Bug 23415: (follow-up) Rename sysprefs and add self-checkout control MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This follow-up patch addresses feedback from the QA process: 1. System preference naming consistency Renamed the following preferences to remove "OPAC" prefix and align with the FineNoRenewals naming convention: - OPACFineNoRenewalsIncludeCredits → FineNoRenewalsIncludeCredits - OPACFineNoRenewalsBlockAutoRenew → FineNoRenewalsBlockAutoRenew 2. Self-checkout backward compatibility Added new system preference FineNoRenewalsBlockSelfCheckRenew to control whether fine limits block renewals via web-based self-checkout. Previously, OPACFineNoRenewals did not block self-checkout renewals. With the rename to FineNoRenewals, this behavior changed. The new preference defaults to "0" (don't block) to maintain backward compatibility, allowing sites to opt-in to blocking self-checkout renewals when patrons exceed the fine threshold. Test plan: 1. Run prove t/db_dependent/Circulation.t - all tests should pass 2. Run prove t/db_dependent/api/v1/checkouts.t - all tests should pass 3. Verify system preferences are correctly renamed in the admin interface 4. Test self-checkout renewals with FineNoRenewalsBlockSelfCheckRenew set to 0 (default) - renewals should work even when fines exceed limit 5. Test self-checkout renewals with FineNoRenewalsBlockSelfCheckRenew set to 1 - renewals should be blocked when fines exceed limit Sponsored-by: OpenFifth Signed-off-by: Andrew Fuerste Henry Signed-off-by: Martin Renvoize --- C4/Circulation.pm | 6 ++-- .../data/mysql/atomicupdate/bug_23415.pl | 32 +++++++++++++++++-- installer/data/mysql/mandatory/sysprefs.sql | 5 +-- .../admin/preferences/circulation.pref | 9 +++++- .../en/modules/admin/preferences/opac.pref | 2 +- opac/opac-user.pl | 2 +- opac/sco/sco-main.pl | 20 ++++++++++++ t/db_dependent/Circulation.t | 16 +++++----- 8 files changed, 73 insertions(+), 19 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index ed09449b3a..621297d20f 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -3275,7 +3275,7 @@ sub CanBookBeRenewed { my $balance = $account->balance; my $non_issues_charges = $account->non_issues_charges; my $amountoutstanding_for_renewal = - C4::Context->preference("OPACFineNoRenewalsIncludeCredits") + C4::Context->preference("FineNoRenewalsIncludeCredits") ? $balance : $non_issues_charges; @@ -5052,11 +5052,11 @@ sub _CanBookBeAutoRenewed { } } - if ( C4::Context->preference('OPACFineNoRenewalsBlockAutoRenew') ) { + if ( C4::Context->preference('FineNoRenewalsBlockAutoRenew') ) { my $fine_no_renewals = C4::Context->preference("FineNoRenewals"); my $account = $patron->account; my $amountoutstanding = - C4::Context->preference("OPACFineNoRenewalsIncludeCredits") + C4::Context->preference("FineNoRenewalsIncludeCredits") ? $account->balance : $account->non_issues_charges; if ( $amountoutstanding and $amountoutstanding > $fine_no_renewals ) { diff --git a/installer/data/mysql/atomicupdate/bug_23415.pl b/installer/data/mysql/atomicupdate/bug_23415.pl index 57a3bf9639..f098a7b8da 100755 --- a/installer/data/mysql/atomicupdate/bug_23415.pl +++ b/installer/data/mysql/atomicupdate/bug_23415.pl @@ -2,8 +2,9 @@ use Modern::Perl; return { bug_number => "23415", - description => "Rename OPACFineNoRenewals and add new syspref AllowFineOverrideRenewing", - up => sub { + description => + "Rename OPACFineNoRenewals and related sysprefs, add new sysprefs AllowFineOverrideRenewing and FineNoRenewalsBlockSelfCheckRenew", + up => sub { my ($args) = @_; my ( $dbh, $out ) = @$args{qw(dbh out)}; @@ -13,7 +14,23 @@ return { } ); - say $out "Updated system preference 'OPACFineNoRenewals'"; + say $out "Updated system preference 'OPACFineNoRenewals' to 'FineNoRenewals'"; + + $dbh->do( + q{ + UPDATE IGNORE systempreferences SET variable = "FineNoRenewalsIncludeCredits" WHERE variable = "OPACFineNoRenewalsIncludeCredits"; + } + ); + + say $out "Updated system preference 'OPACFineNoRenewalsIncludeCredits' to 'FineNoRenewalsIncludeCredits'"; + + $dbh->do( + q{ + UPDATE IGNORE systempreferences SET variable = "FineNoRenewalsBlockAutoRenew" WHERE variable = "OPACFineNoRenewalsBlockAutoRenew"; + } + ); + + say $out "Updated system preference 'OPACFineNoRenewalsBlockAutoRenew' to 'FineNoRenewalsBlockAutoRenew'"; $dbh->do( q{ @@ -23,5 +40,14 @@ return { ); say $out "Added new system preference 'AllowFineOverrideRenewing'"; + + $dbh->do( + q{ + INSERT IGNORE INTO systempreferences (variable, value, options, explanation, type) + VALUES ('FineNoRenewalsBlockSelfCheckRenew', '0', '', 'Block self-checkout renewals if the patron owes more than the value of FineNoRenewals.','YesNo') + } + ); + + say $out "Added new system preference 'FineNoRenewalsBlockSelfCheckRenew'"; }, }; diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index bf16754214..eef46aef2c 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -516,8 +516,9 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('OpacExportOptions','bibtex,dc,marcxml,marc8,utf8,marcstd,mods,ris,isbd',NULL,'Define export options available on OPAC detail page.','multiple'), ('OPACFallback', 'prog', 'bootstrap|prog', 'Define the fallback theme for the OPAC interface.', 'Themes'), ('OpacFavicon','',NULL,'Enter a complete URL to an image to replace the default Koha favicon on the OPAC','free'), -('OPACFineNoRenewalsBlockAutoRenew','0',NULL,'Block/Allow auto renewals if the patron owe more than FineNoRenewals','YesNo'), -('OPACFineNoRenewalsIncludeCredits','1',NULL,'If enabled the value specified in FineNoRenewals should include any unapplied account credits in the calculation','YesNo'), +('FineNoRenewalsBlockAutoRenew','0',NULL,'Block/Allow auto renewals if the patron owes more than FineNoRenewals','YesNo'), +('FineNoRenewalsBlockSelfCheckRenew','0',NULL,'Block self-checkout renewals if the patron owes more than the value of FineNoRenewals','YesNo'), +('FineNoRenewalsIncludeCredits','1',NULL,'If enabled the value specified in FineNoRenewals should include any unapplied account credits in the calculation','YesNo'), ('OPACFinesTab','1',NULL,'If OFF the patron fines tab in the OPAC is disabled.','YesNo'), ('OPACFRBRizeEditions','0',NULL,'If ON, the OPAC will query one or more ISBN web services for associated ISBNs and display an Editions tab on the details pages','YesNo'), ('OpacHiddenItems','',NULL,'This syspref allows to define custom rules for hiding specific items at the OPAC. See https://wiki.koha-community.org/wiki/OpacHiddenItems for more information.','Textarea'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref index 0993df9659..7f78af7376 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ -516,11 +516,18 @@ Circulation: block: block renewing for all the patron's items. - - If a patron owes more than the value of FineNoRenewals, - - pref: OPACFineNoRenewalsBlockAutoRenew + - pref: FineNoRenewalsBlockAutoRenew choices: 1: block 0: allow - their auto-renewals. + - + - If a patron owes more than the value of FineNoRenewals, + - pref: FineNoRenewalsBlockSelfCheckRenew + choices: + 1: block + 0: allow + - renewals via the self-checkout system. - - If a patron pays off all fines on an overdue item that is accruing fines, - pref: RenewAccruingItemWhenPaid diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref index 4267d29292..d1957782a3 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref @@ -652,7 +652,7 @@ OPAC: opacrenew: "'OPACRenew'" - as branchcode to store in the statistics table for renewals done in the OPAC. - - - pref: OPACFineNoRenewalsIncludeCredits + - pref: FineNoRenewalsIncludeCredits choices: 1: Include 0: "Don't include" diff --git a/opac/opac-user.pl b/opac/opac-user.pl index fe380f8a75..c9c3011970 100755 --- a/opac/opac-user.pl +++ b/opac/opac-user.pl @@ -128,7 +128,7 @@ my $amountoutstanding = $patron->account->balance; my $no_renewal_amt = C4::Context->preference('FineNoRenewals'); $no_renewal_amt = undef unless looks_like_number($no_renewal_amt); my $amountoutstandingfornewal = - C4::Context->preference("OPACFineNoRenewalsIncludeCredits") + C4::Context->preference("FineNoRenewalsIncludeCredits") ? $amountoutstanding : $patron->account->outstanding_debits->total_outstanding; diff --git a/opac/sco/sco-main.pl b/opac/sco/sco-main.pl index 7741ee8707..a0cca4a383 100755 --- a/opac/sco/sco-main.pl +++ b/opac/sco/sco-main.pl @@ -333,6 +333,16 @@ if ( $patron && ( $op eq 'cud-renew' ) ) { if ( $patron->checkouts->find( { itemnumber => $item->itemnumber } ) ) { my ( $status, $renewerror ) = CanBookBeRenewed( $patron, $item->checkout ); + + # Override FineNoRenewals check if FineNoRenewalsBlockSelfCheckRenew is disabled + if ( !$status + && $renewerror eq 'too_much_owing' + && !C4::Context->preference('FineNoRenewalsBlockSelfCheckRenew') ) + { + $status = 1; + $renewerror = undef; + } + if ($status) { AddRenewal( { @@ -361,6 +371,16 @@ if ($patron) { while ( my $c = $pending_checkouts->next ) { my $checkout = $c->unblessed_all_relateds; my ( $can_be_renewed, $renew_error ) = CanBookBeRenewed( $patron, $c ); + + # Override FineNoRenewals check if FineNoRenewalsBlockSelfCheckRenew is disabled + if ( !$can_be_renewed + && $renew_error eq 'too_much_owing' + && !C4::Context->preference('FineNoRenewalsBlockSelfCheckRenew') ) + { + $can_be_renewed = 1; + $renew_error = undef; + } + $checkout->{can_be_renewed} = $can_be_renewed; # In the future this will be $checkout->can_be_renewed $checkout->{renew_error} = $renew_error; $checkout->{overdue} = $c->is_overdue; diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index fa398c1272..061df922a2 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -1324,7 +1324,7 @@ subtest "CanBookBeRenewed tests" => sub { is( $error, 'auto_renew', 'Cannot renew, renew is automatic' ); }; - subtest "auto_too_much_owing | OPACFineNoRenewalsBlockAutoRenew & OPACFineNoRenewalsIncludeCredits" => sub { + subtest "auto_too_much_owing | FineNoRenewalsBlockAutoRenew & FineNoRenewalsIncludeCredits" => sub { plan tests => 10; my $item_to_auto_renew = $builder->build_sample_item( { @@ -1351,9 +1351,9 @@ subtest "CanBookBeRenewed tests" => sub { } } ); - C4::Context->set_preference( 'OPACFineNoRenewalsBlockAutoRenew', '1' ); - C4::Context->set_preference( 'FineNoRenewals', '10' ); - C4::Context->set_preference( 'OPACFineNoRenewalsIncludeCredits', '1' ); + C4::Context->set_preference( 'FineNoRenewalsBlockAutoRenew', '1' ); + C4::Context->set_preference( 'FineNoRenewals', '10' ); + C4::Context->set_preference( 'FineNoRenewalsIncludeCredits', '1' ); my $fines_amount = 5; my $account = Koha::Account->new( { patron_id => $renewing_borrowernumber } ); $account->add_debit( @@ -1407,19 +1407,19 @@ subtest "CanBookBeRenewed tests" => sub { is( $renewokay, 1, 'Renew, even if renewal is automatic' ); is( $error, 'auto_renew', - 'Can auto renew, FineNoRenewals=10, OPACFineNoRenewalsIncludeCredits=1, patron has 15 debt, 5 credit' + 'Can auto renew, FineNoRenewals=10, FineNoRenewalsIncludeCredits=1, patron has 15 debt, 5 credit' ); - C4::Context->set_preference( 'OPACFineNoRenewalsIncludeCredits', '0' ); + C4::Context->set_preference( 'FineNoRenewalsIncludeCredits', '0' ); ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrower_obj, $issue ); is( $renewokay, 0, 'Do not renew, renewal is automatic' ); is( $error, 'auto_too_much_owing', - 'Cannot auto renew, FineNoRenewals=10, OPACFineNoRenewalsIncludeCredits=1, patron has 15 debt, 5 credit' + 'Cannot auto renew, FineNoRenewals=10, FineNoRenewalsIncludeCredits=0, patron has 15 debt, 5 credit' ); $dbh->do( 'DELETE FROM accountlines WHERE borrowernumber=?', undef, $renewing_borrowernumber ); - C4::Context->set_preference( 'OPACFineNoRenewalsIncludeCredits', '1' ); + C4::Context->set_preference( 'FineNoRenewalsIncludeCredits', '1' ); }; subtest "auto_account_expired | BlockExpiredPatronOpacActions" => sub { -- 2.34.1