Bugzilla – Attachment 188749 Details for
Bug 23415
Notify patron fines when renewing
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 23415: Fix FineNoRenewals to use non_issues_charges and improve template
Bug-23415-Fix-FineNoRenewals-to-use-nonissuescharg.patch (text/plain), 16.58 KB, created by
Martin Renvoize (ashimema)
on 2025-10-31 06:58:39 UTC
(
hide
)
Description:
Bug 23415: Fix FineNoRenewals to use non_issues_charges and improve template
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-10-31 06:58:39 UTC
Size:
16.58 KB
patch
obsolete
>From 7412495582606139991263d6b446bcaf123c22a9 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Thu, 30 Oct 2025 13:34:08 +0000 >Subject: [PATCH] Bug 23415: Fix FineNoRenewals to use non_issues_charges and > improve template > >This commit addresses two issues: > >1. Fixes FineNoRenewals logic to properly use non_issues_charges > Both CanBookBeRenewed() and _CanBookBeAutoRenewed() were incorrectly > using the patron's total balance when checking against FineNoRenewals. > They now use non_issues_charges, which returns only charges that > should restrict checkouts (e.g., overdues, lost items) as defined by > debit types with restricts_checkouts=1. This ensures charges like > rental fees don't inappropriately block renewals. >2. Improves renewal template for AllowFineOverrideRenewing > - Adds special handling for auto_too_much_owing error to check > AllowFineOverrideRenewing preference instead of > AllowRenewalLimitOverride > - Adds missing id="barcode" attribute to input field for Cypress tests > - Adds .dialog CSS class to error/success divs for consistent styling > - Fixes HTML structure with proper <ul><li> wrapping > - Adds btn btn-default classes to override button >3. Fixes Cypress test issues > - Corrects data structure access: items[0] instead of item > - Uses external_id instead of barcode (API field name) > - Improves form submission selector specificity > - Uses REPLACE INTO for system preference setup > >Test plan: >1. Run t/cypress/integration/Circulation/FineNoRenewals_spec.ts >2. Verify 4 out of 5 tests pass (one remaining template comparison issue) >3. Manually test that patrons with non-issue charges (rental fees) can > renew when those charges alone don't exceed FineNoRenewals >4. Verify that patrons with issue-blocking charges (overdues) are > properly blocked from renewal when exceeding FineNoRenewals > >Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> >--- > C4/Circulation.pm | 16 +++--- > .../prog/en/modules/circ/renew.tt | 48 +++++++++++----- > .../Circulation/FineNoRenewals_spec.ts | 56 +++++++++---------- > 3 files changed, 66 insertions(+), 54 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index 2865b54fc3c..dc04d65f17f 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -3243,7 +3243,9 @@ sub CanBookBeRenewed { > my $restrictionblockrenewing = C4::Context->preference('RestrictionBlockRenewing'); > my $restricted = $patron->is_debarred; > my $hasoverdues = $patron->has_overdues; >- my $balance = $patron->account->balance; >+ my $account = $patron->account; >+ my $balance = $account->balance; >+ my $non_issues_charges = $account->non_issues_charges; > > if ( $restricted and $restrictionblockrenewing ) { > return ( 0, 'restriction' ); >@@ -3251,7 +3253,7 @@ sub CanBookBeRenewed { > || ( $issue->is_overdue and $overduesblockrenewing eq 'blockitem' ) ) > { > return ( 0, 'overdue' ); >- } elsif ( $finesblockrenewing && $balance > $finesblockrenewing ) { >+ } elsif ( $finesblockrenewing && $non_issues_charges > $finesblockrenewing ) { > return ( 0, 'too_much_owing' ); > } > >@@ -4988,12 +4990,10 @@ sub _CanBookBeAutoRenewed { > } > > if ( C4::Context->preference('OPACFineNoRenewalsBlockAutoRenew') ) { >- my $fine_no_renewals = C4::Context->preference("FineNoRenewals"); >- my $amountoutstanding = >- C4::Context->preference("OPACFineNoRenewalsIncludeCredits") >- ? $patron->account->balance >- : $patron->account->outstanding_debits->total_outstanding; >- if ( $amountoutstanding and $amountoutstanding > $fine_no_renewals ) { >+ my $fine_no_renewals = C4::Context->preference("FineNoRenewals"); >+ my $account = $patron->account; >+ my $non_issues_charges = $account->non_issues_charges; >+ if ( $non_issues_charges and $non_issues_charges > $fine_no_renewals ) { > return "auto_too_much_owing"; > } > } >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/renew.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/renew.tt >index 5da4ffd761e..53b2df6a5ca 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/renew.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/renew.tt >@@ -36,7 +36,7 @@ > [% SET aside = Koha.Preference('CircSidebar') ? 'circ-nav' : '' %] > [% WRAPPER 'main-container.inc' aside=aside %] > [% IF error %] >- <div class="alert alert-warning"> >+ <div class="dialog alert alert-warning"> > <h1>Cannot renew:</h1> > > [% IF error == "no_item" %] >@@ -128,26 +128,44 @@ > [% END %] > [% ELSIF error == "auto_renew" or error == "auto_too_much_owing" %] > <p>[% INCLUDE 'biblio-title.inc' biblio=item.biblio link = 1 %] ( [% item.barcode | html %] ) has been scheduled for automatic renewal. </p> >- [% SET override_limit = error == "auto_renew" ? "AUTO_RENEW" : "TOO_MUCH" %] >- >- [% IF Koha.Preference('AllowRenewalLimitOverride') %] >- <form method="post" action="/cgi-bin/koha/circ/renew.pl"> >- [% INCLUDE 'csrf-token.inc' %] >- <input type="hidden" name="barcode" value="[% item.barcode | html %]" /> >- <input type="hidden" name="override_limit" value="[% override_limit | html %]" /> >- <input type="hidden" name="op" value="cud-renew" /> >- <button type="submit" class="btn btn-default approve"><i class="fa fa-check"></i> Override limit and renew</button> >- </form> >+ [% IF error == "auto_too_much_owing" %] >+ [% IF balance %] >+ <ul> >+ <li>The patron has a debt of [% balance | $Price %] and renewals are restricted at [% Koha.Preference('FineNoRenewals') | $Price %].</li> >+ </ul> >+ [% END %] >+ [% IF Koha.Preference('AllowFineOverrideRenewing') %] >+ <form method="post" action="/cgi-bin/koha/circ/renew.pl"> >+ [% INCLUDE 'csrf-token.inc' %] >+ <input type="hidden" name="barcode" value="[% item.barcode | html %]" /> >+ <input type="hidden" name="override_debt" value="1" /> >+ <input type="hidden" name="op" value="cud-renew" /> >+ <button type="submit" class="btn btn-default approve"><i class="fa fa-check"></i> Override and renew</button> >+ </form> >+ [% END %] >+ [% ELSE %] >+ [% SET override_limit = "AUTO_RENEW" %] >+ [% IF Koha.Preference('AllowRenewalLimitOverride') %] >+ <form method="post" action="/cgi-bin/koha/circ/renew.pl"> >+ [% INCLUDE 'csrf-token.inc' %] >+ <input type="hidden" name="barcode" value="[% item.barcode | html %]" /> >+ <input type="hidden" name="override_limit" value="[% override_limit | html %]" /> >+ <input type="hidden" name="op" value="cud-renew" /> >+ <button type="submit" class="btn btn-default approve"><i class="fa fa-check"></i> Override limit and renew</button> >+ </form> >+ [% END %] > [% END %] > [% ELSIF error == "too_much_owing" %] >- <li>The patron has a debt of [% balance | $Price %].</li> >+ <ul> >+ <li>The patron has a debt of [% balance | $Price %] and renewals are restricted at [% Koha.Preference('FineNoRenewals') | $Price %].</li> >+ </ul> > [% IF Koha.Preference('AllowFineOverrideRenewing') %] > <form method="post" action="/cgi-bin/koha/circ/renew.pl"> > [% INCLUDE 'csrf-token.inc' %] > <input type="hidden" name="barcode" value="[% item.barcode | html %]" /> > <input type="hidden" name="override_debt" value="1" /> > <input type="hidden" name="op" value="cud-renew" /> >- <button type="submit" class="approve"><i class="fa fa-check"></i>Renew checkout(s)</button> >+ <button type="submit" class="btn btn-default approve"><i class="fa fa-check"></i> Override and renew</button> > </form> > [% END %] > [% ELSIF error == "on_reserve" %] >@@ -189,7 +207,7 @@ > [% END %] > > [% IF date_due %] >- <div class="alert alert-info"> >+ <div class="dialog message alert alert-info"> > <h1>Item renewed:</h1> > <p> > [% INCLUDE 'biblio-title.inc' biblio=item.biblio link = 1 %] ( >@@ -219,7 +237,7 @@ > <input value="1" name="unseen" id="unseen" type="checkbox" /> > </div> > [% ELSE %] >- <input name="barcode" size="14" class="barcode focus" type="text" placeholder="Enter item barcode" /> >+ <input name="barcode" id="barcode" size="14" class="barcode focus" type="text" placeholder="Enter item barcode" /> > [% END %] > <input type="hidden" name="op" value="cud-renew" /> > <div id="show-circ-settings"> >diff --git a/t/cypress/integration/Circulation/FineNoRenewals_spec.ts b/t/cypress/integration/Circulation/FineNoRenewals_spec.ts >index 8f49d3979ab..7d28d2b7c15 100644 >--- a/t/cypress/integration/Circulation/FineNoRenewals_spec.ts >+++ b/t/cypress/integration/Circulation/FineNoRenewals_spec.ts >@@ -13,12 +13,15 @@ describe("Circulation - FineNoRenewals and AllowFineOverrideRenewing", () => { > patron = objects.patron; > checkout = objects.checkout; > >- // Set FineNoRenewals to 5 >+ // Disable auto_renew on the checkout to get too_much_owing error instead of auto_too_much_owing > cy.task("query", { >- sql: "UPDATE systempreferences SET value = ? WHERE variable = ?", >- values: ["5", "FineNoRenewals"], >+ sql: "UPDATE issues SET auto_renew = 0 WHERE issue_id = ?", >+ values: [checkout.checkout_id], > }); > >+ // Set FineNoRenewals to 5 >+ cy.set_syspref("FineNoRenewals", "5"); >+ > // Add a fine of 10 to the patron (exceeds limit) > fine_amount = 10; > cy.task("query", { >@@ -27,7 +30,7 @@ describe("Circulation - FineNoRenewals and AllowFineOverrideRenewing", () => { > values: [ > patron.patron_id, > fine_amount, >- objects.item.item_id, >+ objects.items[0].item_id, > ], > }); > >@@ -40,23 +43,19 @@ describe("Circulation - FineNoRenewals and AllowFineOverrideRenewing", () => { > cy.task("deleteSampleObjects", [this.testObjects]); > > // Reset system preferences >- cy.task("query", { >- sql: "UPDATE systempreferences SET value = '100' WHERE variable = 'FineNoRenewals'", >- }); >- cy.task("query", { >- sql: "UPDATE systempreferences SET value = '0' WHERE variable = 'AllowFineOverrideRenewing'", >- }); >+ cy.set_syspref("FineNoRenewals", "100"); >+ cy.set_syspref("AllowFineOverrideRenewing", "0"); > }); > > it("should block renewal when patron has fines over FineNoRenewals limit", function () { >- const barcode = this.testObjects.item.barcode; >+ const barcode = this.testObjects.items[0].external_id; > > // Visit renewal page > cy.visit("/cgi-bin/koha/circ/renew.pl"); > > // Enter barcode > cy.get("#barcode").type(barcode); >- cy.get("form").submit(); >+ cy.get("#barcode").closest("form").submit(); > > // Should see error message about patron debt > cy.get(".dialog.alert").should("be.visible"); >@@ -68,21 +67,20 @@ describe("Circulation - FineNoRenewals and AllowFineOverrideRenewing", () => { > > it("should show override button when AllowFineOverrideRenewing is enabled", function () { > // Enable AllowFineOverrideRenewing >- cy.task("query", { >- sql: "UPDATE systempreferences SET value = '1' WHERE variable = 'AllowFineOverrideRenewing'", >- }); >+ cy.set_syspref("AllowFineOverrideRenewing", "1"); > >- const barcode = this.testObjects.item.barcode; >+ const barcode = this.testObjects.items[0].external_id; > > // Visit renewal page > cy.visit("/cgi-bin/koha/circ/renew.pl"); > > // Enter barcode > cy.get("#barcode").type(barcode); >- cy.get("form").submit(); >+ cy.get("#barcode").closest("form").submit(); > > // Should see error message about patron debt > cy.get(".dialog.alert").should("be.visible"); >+ > cy.get(".dialog.alert li").should( > "contain", > `The patron has a debt of` >@@ -94,24 +92,22 @@ describe("Circulation - FineNoRenewals and AllowFineOverrideRenewing", () => { > ); > cy.get('.dialog.alert form button[type="submit"].approve').should( > "contain", >- "Renew checkout(s)" >+ "Override and renew" > ); > }); > > it("should NOT show override button when AllowFineOverrideRenewing is disabled", function () { > // Ensure AllowFineOverrideRenewing is disabled >- cy.task("query", { >- sql: "UPDATE systempreferences SET value = '0' WHERE variable = 'AllowFineOverrideRenewing'", >- }); >+ cy.set_syspref("AllowFineOverrideRenewing", "0"); > >- const barcode = this.testObjects.item.barcode; >+ const barcode = this.testObjects.items[0].external_id; > > // Visit renewal page > cy.visit("/cgi-bin/koha/circ/renew.pl"); > > // Enter barcode > cy.get("#barcode").type(barcode); >- cy.get("form").submit(); >+ cy.get("#barcode").closest("form").submit(); > > // Should see error message about patron debt > cy.get(".dialog.alert").should("be.visible"); >@@ -128,18 +124,16 @@ describe("Circulation - FineNoRenewals and AllowFineOverrideRenewing", () => { > > it("should allow renewal after override when AllowFineOverrideRenewing is enabled", function () { > // Enable AllowFineOverrideRenewing >- cy.task("query", { >- sql: "UPDATE systempreferences SET value = '1' WHERE variable = 'AllowFineOverrideRenewing'", >- }); >+ cy.set_syspref("AllowFineOverrideRenewing", "1"); > >- const barcode = this.testObjects.item.barcode; >+ const barcode = this.testObjects.items[0].external_id; > > // Visit renewal page > cy.visit("/cgi-bin/koha/circ/renew.pl"); > > // Enter barcode > cy.get("#barcode").type(barcode); >- cy.get("form").submit(); >+ cy.get("#barcode").closest("form").submit(); > > // Click override button > cy.get('.dialog.alert form button[type="submit"].approve').click(); >@@ -160,17 +154,17 @@ describe("Circulation - FineNoRenewals and AllowFineOverrideRenewing", () => { > cy.task("query", { > sql: `INSERT INTO accountlines (borrowernumber, amountoutstanding, debit_type_code, status, interface, itemnumber) > VALUES (?, 3.00, 'OVERDUE', 'UNRETURNED', 'test', ?)`, >- values: [patron.patron_id, this.testObjects.item.item_id], >+ values: [patron.patron_id, this.testObjects.items[0].item_id], > }); > >- const barcode = this.testObjects.item.barcode; >+ const barcode = this.testObjects.items[0].external_id; > > // Visit renewal page > cy.visit("/cgi-bin/koha/circ/renew.pl"); > > // Enter barcode > cy.get("#barcode").type(barcode); >- cy.get("form").submit(); >+ cy.get("#barcode").closest("form").submit(); > > // Should see success message (no error about fines) > cy.get(".dialog.message").should("be.visible"); >-- >2.51.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 23415
:
92039
|
92541
|
98442
|
98634
|
98996
|
98997
|
105893
|
105894
|
108392
|
109871
|
109872
|
109876
|
109880
|
116919
|
116920
|
116921
|
116922
|
118175
|
118176
|
118177
|
118178
|
118183
|
119300
|
119301
|
119302
|
119303
|
121472
|
139670
|
139671
|
139672
|
139673
|
139674
|
146415
|
148198
|
148199
|
148200
|
148201
|
148202
|
152776
|
152777
|
152778
|
152779
|
152780
|
152781
|
154595
|
154596
|
154597
|
154598
|
154599
|
154600
|
158302
|
158303
|
158304
|
158305
|
158306
|
158307
|
161964
|
161965
|
161966
|
161967
|
161968
|
161969
|
162171
|
162172
|
162173
|
162174
|
162175
|
162176
|
162177
|
162178
|
162179
|
162180
|
171620
|
171621
|
171622
|
171623
|
171983
|
171984
|
171985
|
171986
|
172906
|
172907
|
172908
|
172909
|
172910
|
185270
|
185271
|
185272
|
185273
|
185274
|
185275
|
186014
|
188331
|
188332
|
188333
|
188334
|
188335
|
188336
|
188337
|
188338
|
188339
|
188340
|
188341
|
188342
|
188708
|
188709
|
188710
|
188711
|
188712
|
188713
|
188714
|
188715
|
188716
|
188717
|
188718
|
188719
|
188720
|
188738
|
188739
|
188740
|
188741
|
188742
|
188743
|
188744
|
188745
|
188746
|
188747
|
188748
| 188749 |
188750
|
188751