This will now find the corresponding ISO18626 request and update it accordingly when a check-out happens.
Use-case this is addressing:
1. Place an item-level hold for Henry
2. Check-in that item e.g. 39999000003130 (make it waiting)
3. Create a new ISO18626 request and 'Search to hold', place a bib-level hold on the same bib as above (for patron Walker)
4. Check-out the item from 2. 39999000003130 to Walker
5. Confirm an ISO18626 message input is shown and that the respective ISO18626 request is updated to 'Loaned' once the check-out happens (i.e. override Henry's hold).
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit improves upon the original fix by making the intent more
explicit when handling empty string date parameters in AddRenewal and
AddIssue functions.
WHY THIS IS BETTER:
The original fix (changing `defined $datedue` to just `$datedue`)
works correctly but relies on Perl's implicit truthy/falsy semantics:
if ( $datedue && ref $datedue ne 'DateTime' )
This is subtle because:
- Empty string "" is falsy, so parsing is skipped ✓
- But it's not immediately obvious that empty strings are being
handled specially
- The code doesn't explicitly show its intent
THIS APPROACH:
if ( defined $datedue && $datedue ne '' && ref $datedue ne 'DateTime' )
Benefits:
1. EXPLICIT: Clearly states "only parse if defined AND not empty
AND not already a DateTime"
2. SELF-DOCUMENTING: Intent is obvious to future developers
3. NO RELIANCE on truthy/falsy semantics
4. CONSISTENT: Same pattern applied across all date parameter handling
COMPREHENSIVE CHANGES:
1. AddRenewal (C4/Circulation.pm:3449)
- Made datedue empty string check explicit
2. AddIssue (C4/Circulation.pm:1611)
- Made datedue empty string check explicit
- Also updated issuedate handling (line 1616) to explicitly handle
empty strings
3. Test Coverage (t/db_dependent/Circulation/issue.t)
- Added test for AddIssue with empty string datedue
- Added test for AddIssue with empty string issuedate
- Ensures both functions properly calculate dates from circulation
rules instead of using "now"
The existing test for AddRenewal already covered empty string datedue
(added in the original Bug 37966 commit). These new tests ensure the
same correct behavior for AddIssue.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
When renewing a book on hold with the RenewalOnHoldOverride sys pref set
to "Allow", if the due date is not specified then it becomes "now". By
checking the input is set rather than just defined the due date is set as
expected based on circulation rules.
Also change the description of the RenewalOnHoldOverride sys pref as the
due date would no longer be required to be entered with this change.
Test plan:
1. make sure RenewalOnHoldOverride is set to 'Allow'
2. check out a book, then place a title-level hold on it
3. renew the book, see the hold override dialog
4. leave the due date empty and hit the override button
5. observe the new due date is "now"
6. apply patch and restart plack
7. renew the book again, overriding the hold, and see the expected due date
as per circulation rules.
Signed-off-by: Marion Durand <marion.durand@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Amended-by: Jonathan Druart
Tidy C4/Search.pm
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
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 <https://openfifth.co.uk/>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
This patch fixes a regression introduced in the previous commits where
we changed FineNoRenewals checking to use non_issues_charges instead
of balance. While this change was correct for focusing on blocking
charges, we inadvertently removed the handling of the
OPACFineNoRenewalsIncludeCredits preference.
The OPACFineNoRenewalsIncludeCredits preference controls whether
unapplied account credits should be included when calculating if a
patron exceeds the FineNoRenewals threshold. When this preference is
enabled, the patron's net balance (debits minus credits) should be
used instead of just their outstanding debits.
This patch restores the preference handling for both:
1. Manual renewals in CanBookBeRenewed()
2. Auto renewals in _CanBookBeAutoRenewed()
When OPACFineNoRenewalsIncludeCredits = 1:
Use balance (all charges net of credits)
When OPACFineNoRenewalsIncludeCredits = 0:
Use non_issues_charges (only blocking charges, no credits)
This ensures that patrons with sufficient credits can renew their
items when the preference is enabled, while still maintaining the
focus on non-issuing charges introduced in the previous commits.
Test plan:
1. Run prove t/db_dependent/Circulation.t
2. All tests should pass, particularly the
"auto_too_much_owing | OPACFineNoRenewalsBlockAutoRenew &
OPACFineNoRenewalsIncludeCredits" subtest
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
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
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Standardizes error code spelling across the codebase:
- Changes 'too_much_oweing' to 'too_much_owing'
- Changes 'auto_too_much_oweing' to 'auto_too_much_owing'
The word "owing" is the correct spelling in English.
Files updated:
- C4/Circulation.pm
- C4/SIP/Sip/MsgType.pm
- circ/renew.pl
- koha-tmpl/intranet-tmpl/prog/en/modules/circ/renew.tt
- koha-tmpl/intranet-tmpl/prog/js/checkouts.js
- koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
- koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
- misc/cronjobs/automatic_renewals.pl
- opac/opac-reserve.pl
- opac/opac-user.pl
- t/db_dependent/Circulation.t
All tests should pass successfully.
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
This commit addresses several issues found in QA review:
1. Fixed logic bug in C4::Circulation::CanBookBeRenewed
- Removed incorrect check of AllowFineOverrideRenewing preference
- The function now always returns 'too_much_oweing' error when
patron balance exceeds FineNoRenewals limit
- AllowFineOverrideRenewing should only control UI override
capability, not the core renewal check
2. Added permission check in circ/renew.pl
- Override is now only allowed when both override_debt parameter
is set AND AllowFineOverrideRenewing preference is enabled
- Prevents security issue where staff could bypass the preference
by crafting POST requests
3. Fixed template to conditionally display override button
- Override button in renew.tt now only shows when
AllowFineOverrideRenewing is enabled
- Prevents confusion when override is not permitted
4. Added test coverage
- Tests verify CanBookBeRenewed behavior with AllowFineOverrideRenewing
both enabled and disabled
- Confirms error is always returned regardless of preference setting
5. Fixed minor issues
- Fixed typo: "he patron" -> "the patron" in checkouts.js
- Fixed typo: OPACFineNoRenewalsIncludeCredit ->
OPACFineNoRenewalsIncludeCredits in test
All tests should pass successfully.
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
This patch adds new syspref AllowFineOverrideRenewing. It allows
staff to renew items for patrons whose fines are over limit set
in syspref FineNoRenewals. Otherwise renewing is prevented. If
renewing items is allowed staff is still required to confirm
if they really want to renew items for the patron.
To test:
1. Apply this patch, update database and restart all services.
2. Find patron with fines over limit set in FineNoRenewals and
renewable items.
=> Note that renewing is prevented with message "Cannot renew,
the patron has a debt of..."
3. Set syspref AllowFineOverrideRenewing as "Allow".
=> Note that items can now be renewed and renewing them displays
confirmation pop-up.
Sponsored-by: Koha-Suomi Oy
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-of-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
This moves the basic calculation for too_much_oweing out of the
controller and into the class method.
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
OPACFineNoRenewals no longer applies just to OPAC.
To avoid confusion rename it as FineNoRenewals and
also rename all occurences in code.
To test, confirm everything works as described in
previous test plans.
Also prove t/db_dependent/Circulation.t
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This patch ensures that the accountlines.branchcode field correctly records
which branch's circulation rules were applied when creating OVERDUE, LOST,
and PROCESSING fee debits. Previously, this field was often NULL or set
to the logged-in user's branch, making it difficult to track which branch's
policies governed the fee calculation.
Changes made:
1. New helper method: Koha::Checkout->branch_for_fee_context()
- Centralizes branch resolution logic based on system preferences
- For LOST/PROCESSING: honors LostChargesControl preference
- For OVERDUE: honors CircControl preference
- Respects HomeOrHoldingBranch when choosing item's branch
- Supports both Koha objects and plain hashrefs
- Returns branchcode or undef if context unavailable
2. C4::Accounts::chargelostitem()
- Now accepts optional $opts hashref parameter
- Accepts interface, library_id, and issue_id in options
- Determines appropriate branch using branch_for_fee_context
- Passes resolved branch to add_debit as library_id
- Issue_id lookup handles bundled items correctly
3. C4::Circulation::LostItem()
- Calls branch_for_fee_context before chargelostitem
- Passes interface context ('cron' for cronjob, otherwise current)
- Passes resolved branch and issue_id to chargelostitem
4. C4::Overdues::UpdateFine()
- Calls branch_for_fee_context to determine OVERDUE branch
- Passes result as library_id to add_debit
- Resolves long-standing FIXME about branch selection
5. misc/cronjobs/longoverdue.pl
- Determines branch context before marking items lost
- Passes library_id to LostItem when charging fees
The branch resolution logic respects these system preferences:
- CircControl: PatronLibrary | PickupLibrary | ItemHomeLibrary
- LostChargesControl: PatronLibrary | PickupLibrary | ItemHomeLibrary
- HomeOrHoldingBranch: homebranch | holdingbranch
Test plan:
1. Create three test libraries (e.g., FPL, CPL, PVL)
2. Create a test patron with home library PVL
3. Create an item with:
- homebranch = FPL
- holdingbranch = CPL
4. Set system preferences:
- FinesMode: Calculate and charge
- Create circulation rule that charges overdue fines
5. Test OVERDUE fees with CircControl:
a. Set CircControl = PatronLibrary, HomeOrHoldingBranch = homebranch
b. Check out item to patron at CPL, backdate to be overdue
c. Run: perl /usr/share/koha/bin/cronjobs/fines.pl
d. Check accountlines: SELECT accountlines_id, branchcode, debit_type_code
FROM accountlines ORDER BY accountlines_id DESC LIMIT 5;
e. Verify branchcode = PVL (patron's home library)
f. Delete the fine, repeat with CircControl = PickupLibrary
g. Verify branchcode = CPL (checkout library)
h. Delete the fine, repeat with CircControl = ItemHomeLibrary
i. Verify branchcode = FPL (item's home library)
j. Change HomeOrHoldingBranch = holdingbranch, repeat
k. Verify branchcode = CPL (item's holding library)
6. Test LOST fees with LostChargesControl:
a. Set WhenLostChargeReplacementFee = Charge
b. Set item replacement price > 0
c. Ensure item is checked out to patron
d. Set LostChargesControl = PatronLibrary
e. Mark item as lost in staff interface
f. Check accountlines for LOST debit
g. Verify branchcode = PVL (patron's home library)
h. Return item, re-checkout, repeat with LostChargesControl = PickupLibrary
i. Verify branchcode = CPL (checkout library)
j. Repeat with LostChargesControl = ItemHomeLibrary + homebranch
k. Verify branchcode = FPL (item's home)
l. Repeat with LostChargesControl = ItemHomeLibrary + holdingbranch
m. Verify branchcode = CPL (item's holding)
7. Test longoverdue cronjob:
a. Re-checkout item with due date triggering longoverdue
b. Run: perl /usr/share/koha/bin/cronjobs/longoverdue.pl --lost 30=1 --charge 1 --confirm --verbose
c. Verify LOST debit created with correct branchcode per LostChargesControl
8. Test edge cases:
- WhenLostChargeReplacementFee = Don't charge → no LOST debit
- Replacement price = 0 → no LOST debit
- FinesMode = Don't calculate → no OVERDUE debits
Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch additionally fetches the reply to address for advance notices
To test:
1 - Have some issues that will be due soon for a patron(s)
Checkout with the due date set to tomorrow
2 - Verify the patron(s) have an email defined, verify the issuing branch has a reply to set
3 - Verify the patron(s) have messaging preference set to receive Advance notices by email 1 day in advance
4 - perl misc/cronjobs/advance_notices.pl -v -c
5 - Check the message queue
SELECT * FROM message_queue WHERE borrowernumber={##}
6 - Note no rpely to is set
7 - Apply patch
8 - perl misc/cronjobs/advance_notices.pl -v -c
9 - Check the message queue
10 - Confirm newer message has the reply to populated
11 - Ensure the issued item has a different homebranch with a reply to address se
12 - perl misc/cronjobs/advance_notices.pl -v -c --frombranch item-homebranch
13 - Confirm message has correct reply to
14 - Set patron to want digest in messaging preferences
15 - Generate notices
16 - Confirm digets has the right reply to
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tidied inline
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Note: tests have to be performed on addReturn and have therefore been
added in a separate test
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch adds the optional 'tranferbranch' parameter to the
CanBookBeReturned routine. This allows for passing the correct
'ReturnToX' transfer to be tested at return time for transfer limit
restrictions.
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Mathieu Saby <mathsabypro@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This follow-up patch actually fixes the issue.
This patch forbids a patron to return a book to a library
from where the book may not return to its home or holding library.
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Mathieu Saby <mathsabypro@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Test case:
* UseBranchTransferLimits must be set
* define your branch transfer limit. Refuse transfers from libraryA to libraryB
* checkout a book owned by libraryB, from libraryB, with a librarian located at libraryB
* move the librarian to libraryA ("Set Library" link top/right)
* check-in the book => it's possible whatever your setup
After the patch, the behaviour respect the branch transfer limit parameter:
you can check-in if you accept transfers, you can't if you refuse them.
(Note: IndependantBranches must be OFF, otherwise it's not possible to do the checkin whatever the branch transfer limits)
TEST PLAN:
Note: older signoffs did not validate this test plan
Considering you use a ktd-like database.
Log in at centerville
Allow transfers between centerville, Midway and Franklin, don't allow transfers between Fairfield, Liberty and all other libraries. In addition, allow the transfer between Liberty and Midway
Transfer as much books as you can from Centerville to Midway.
Log in as Midway, check out as many books from Centerville as you can to Henry Acevedo. If you don't have enough books during the test plan, feel free to check out books again. During the whole test, without further precision, items checkout out in such conditions should be checked in.
You will also need 6 checked out floating books at midway
Note:
'-> ok' means the checkin is allowed
'-> not ok' means the checkin is not allowed
Apply patch
I - Test AllowReturnToBranch
During this part of the test plan, you want to have all following configuration :
'CircControlReturnsBranch' -> 'The library the item is checked in at'
In CIRC rules: 'Default holds policy and return policy' -> 'Item returns home' (make sure you have no other holds policy on any library)
'AllowReturnToBranch' -> as follows
1 - Set to "Either at the library the item is from or at the library the item was checked out from"
X a - Check in to Centerville -> ok
X b - Check in to Midway -> ok
X c - Check in to Franklin -> not ok
X d - Check in to Fairfield -> not ok
2 - Set to "Only at the library the item was checked out from"
X a - Check in to Centerville -> not ok
X b - Check in to Midway -> ok
X c - Check in to Franklin -> not ok
X d - Check in to Fairfield -> not ok
3 - Set to "Only at the library the item is from"
X a - Check in to Centerville -> ok
X b - Check in to Midway -> not ok
X c - Check in to Franklin -> not ok
X d - Check in to Fairfield -> not ok
4 - Set to "Any library"
X a - Check in to Centerville -> ok
X b - Check in to Midway -> ok
X c - Check in to Franklin -> ok
X d - Check in to Fairfield -> not ok
II - Transfer and check out 6 floating items at midway.
1 -
X a - Check in to Centerville -> ok
X b - Check in to Midway -> ok
X c - Check in to Franklin -> ok
X d - Check in to Fairfield -> ok
2 - Set 'AllowReturnToBranch' to 'home library'
X a - Check in to Centerville -> ok
X b - Check in to Midway -> not ok
III - Pathological & asymetrical cases.
During this part of the test plan, you want to have all following configuration :
'CircControlReturnsBranch' -> 'The library the item is checked in at'
'Default holds policy and return policy' -> 'Item returns home' (make sure you have no other holds policy on any library)
'AllowReturnToBranch' -> 'Any library'
FOR THIS PART OF THE TESTS ONLY (until IV): Don't allow tranfers from midway to centerville, but allow from centerville to midway
X 1 - Check in a book at midway -> not ok
Set 'Default holds policy and return policy' -> 'Item returns to issuing library'
X 2 - Check in a book at centerville -> ok
FOR THIS PART OF THE TESTS ONLY (until IV): Don't allow tranfers from centerville to midway
X 3 - Check in a book at centerville -> not ok
X 4 - Set 'AllowReturnToBranch' to "Only at the library the item was checked out from"
X a - check in to Centerville -> not ok
X b - check in to Midway -> not ok
IV - Test 'CircControlReturnsBranch'
(remove the precedent transfer restriction)
During this part of the test plan, you want to have all following configuration :
'Default holds policy and return policy' -> 'Item returns home' (unchanged)
'Default checkout, hold and return policy for Midway' -> 'Item returns to issuing library'
'Default checkout, hold and return policy for Franklin' -> 'Item floats'
'Default checkout, hold and return policy for Fairfield' -> 'Item returns to issuing library'
'Default checkout, hold and return policy for Liberty' -> 'Item returns home'
'AllowReturnToBranch' -> "Any library" (unchanged)
'CircControlReturnsBranch' -> as follows
1 - 'The library the item is checked in at'
X a - Check in to Centerville -> ok
X b - Check in to Midway -> ok
X c - Check in to Franklin -> ok
X d - Check in to Fairfield -> not ok
X e - Check in to Liberty -> not ok
2 - 'The library the current item is held by
X a - Check in to Centerville -> ok
X b - Check in to Midway -> ok
X c - Check in to Franklin -> ok
X d - Check in to Fairfield -> not ok
X e - Check in to Liberty -> ok
3 - 'the library the item is owned by'
X a - Check in to Centerville -> ok
X b - Check in to Midway -> ok
X c - Check in to Franklin -> ok
X d - Check in to Fairfield -> not ok
X e - Check in to Liberty -> not ok
Sponsored-by: National Library of Finland
Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Mathieu Saby <mathsabypro@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
To test:
1. APPLY PATCH, updatedatabase, restart_all
2. Go to your circulation rules and do the following:
-Automatic renewal: Yes
-No renewal before: 5
-No automatic renewal before: 5
-Renewal period: 5
3. Set ManualRenewalPeriodBase to "the old due date of the checkout".
4. Set AutomaticRenewalPeriodBase to "current date".
5. Check out an item and set the due date to 2 days from today
6. Attempt to manually renewal the item.
7. Notice the new date is calculated based on the old checkout date.
8. Check out an item another item and set the due date to 2 days from today.
9. Run `perl /kohadevbox/koha/misc/cronjobs/automatic_renewals.pl -c -v`
10. Notice the new date is calculated based on the current date.
11. Set ManualRenewalPeriodBase to "current date".
12. Set AutomaticRenewalPeriodBase to "the old due date of the checkout".
13. Check out an item and set the due date to 2 days from today
14. Attempt to manually renewal the item.
15. Notice the new date is calculated based on the current date.
16. Check out an item another item and set the due date to 2 days from today.
17. Run `perl /kohadevbox/koha/misc/cronjobs/automatic_renewals.pl -c -v`
18. Notice the new date is calculated based on the old checkout date.
Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Test plan:
1. APPLY patch, restart_all, updatedatabase
2. Search for the StoreLastBorrower system preference, it should now allow you to enter a number of borrowers to store
3. Set the pref to 3.
4. Check an item out to any patron. Then immediately check it back in.
5. On catalogue/moredetail.pl?biblionumber=X ( where X is you biblionumber ) you should see a "Last returned by:" entry with the correct cardnumber for the borrower.
7. Confirm this is right in the database with: SELECT * FROM items_last_borrower where itemnumber = X; ( Where X is your itemnumber )
8. Repeat step 4 to a different borrower.
9. Repeat steps 5 and 6. You should see 2 cardnumbers of each of the borrowers on catalogue/moredetail.pl?biblionumber=X
10. Repeat step 4 with another new borrower.
11. Repeat steps 5 and 6. You should see 3 cardnumbers of each of the borrowers on catalogue/moredetail.pl?biblionumber=X
12. Choose a different item and repeat steps 4 - 11 for this item.
13. Now set the StoreLastBorrower preference to 0.
14. Using the original item from step 4, check something out to a patron and immediately check it back in.
15. catalogue/moredetail.pl?biblionumber=X should NOT have an entry for "Last returned by:"
16. Check the database, all entries for the itemnumber from step 14 should be deleted.
Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
CanBookBeRenewed will by default pass the skip_future_holds flag
to item->current_holds so that renewals are not blocked by future
holds.
The new pref FutureHoldsBlockRenewals allows you to change that
historical behavior.
Test plan:
[1] Set pref ConfirmFutureHolds=3, FutureHoldsBlockRenewals=Allow,
OPACAllowHoldDateInFuture=Allow
[2] Checkout some item to patron A.
[3] Place hold on same item for patron B, reserve date tomorrow.
[4] Try to renew item with patron A on account page. Should be
marked as Not renewable.
[5] Set pref FutureHoldsBlockRenewals=Dont
[6] Repeat step 4. Renew should be allowed.
[7] prove t/db_dependent/Holds.t t/db_dependent/Reserves.t
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
So it is done before the other use statements.
Also uniformize the pattern, must now be:
```
use Modern::Perl;
use base 'Exporter';
BEGIN {
our @EXPORT_OK = qw(subroutine list);
}
```
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Bug38936 exported redirect_if_opac_suppressed
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch removes the duplicate logging code from the original
Bug 9762 implementation that was not properly removed when the
Alternative implementation was merged.
The original implementation added separate logaction() calls in
CanBookBeIssued and circulation.pl that created additional log
entries with a different JSON format. This caused double logging
when the Alternative implementation (which logs via AddIssue) was
merged on top.
Changes in this patch:
C4/Circulation.pm:
- Removed @message_log array and all pushes to it
- Removed the separate logaction() call at the end of CanBookBeIssued
- Removed 5th return value (@message_log) from CanBookBeIssued
- Removed all if ($issueconfirmed) blocks that were populating @message_log
circ/circulation.pl:
- Removed the problematic line that added OVERRIDDEN to every confirmed
checkout: $needsconfirmation->{'OVERRIDDEN'} = $issueconfirmed
This addresses the double logging issue where circulation overrides
were being logged twice with different formats.
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Writing these I found that transferbook was generating two queue rebuilds - fixed that
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch does two things:
1:
It prevents rebulding the real time holds queue during a checkin if a reserve was found.
The reserve will either:
a - be set to waiting if at correct location and no confirmation - this is fine because we will rebuild the queue when
filling the hold
b - trigger a popup whihc will rebuild the queue when confirming the transfer, or be ignored
2:
It adds a form to ignoring a hold so that we can rebuild the holds queue if we decided not to use the current item
to fill the found hold
To test:
0 - Enable RealTimeHoldsQueue
1 - Place several holds for different libraries on a title
2 - Checkout a copy to a patron at current library (ignore current holds)
3 - Clean up DB so we can see outcomes from next steps:
delete from branchtransfers; delete from background_jobs; update reserves SET found=NULL where found='T';
4 - Check in item, confirm hold
5 - Check background jobs, note two rebuilds of the holds queue
6 - Check item out to previous patron, repeat 3
7 - Check in item, ignore the hold
8 - Check background jobs, note only one rebuild
9 - Apply patch, restart all
10 - Checkout item, repeat 3
11 - Checkin item, confirm hold
12 - Check background jobs, onlt one rebuild of holds queue
13 - Checkout item, repeat 3
14 - Checkin item, ignore hold
15 - Check background jobs, only one rebuild of holds queue
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
When implemented, the message here was believed to either be about Duplicate IDs, or guarantor issues.
We are now seeing a wider variety of issues possible from SQL errors, lock timeout or missings rows as examples
This patch adds an 'ErrorMessage' message to the return from AddReturn and displays it to the user.
To test:
1 - Issue an item to a patron, note the itemnumber
2 - On the command line, lock the row:
sudo koha-mysql kohadev
START TRANSACTION;
SELECT * FROM issues WHERE itemnumber=975 FOR UPDATE;
leave mysql open at this point
3 - On the interface, check in the item, wait a bit
4 - After a bit you get the error to check the config page, but nothing is there
5 - close mysql, apply patch, restart all
6 - Repeat 2
7 - Check in the item, wait a bit
8 - Note you now get the error from the DB:
Error: (DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Lock wait timeout exceeded; try restarting transaction at /kohadevbox/koha/Koha/Object.pm line 174 )
9 - Sign off!
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch adds a booking_id field to the issues and old_issues tables
to create a linkage between checkouts and the bookings that generated
them. When a patron with a booking checks out the reserved item, the
checkout is linked to their booking and the booking status is updated
to 'completed'.
The booking_id is preserved during renewals and when checkouts are
moved to old_issues upon return.
Database changes:
- Add booking_id column to issues and old_issues tables
- Add foreign key constraints to maintain referential integrity
Test plan:
1. Apply the database update
2. Restart services
3. Create a booking for a patron
4. When the patron checks out the booked item:
a. Verify the checkout links to the booking (booking_id is set)
b. Verify the booking status changes to 'completed'
5. Renew the checkout
a. Verify the booking_id is preserved after renewal
b. Check the database: SELECT booking_id FROM issues WHERE issue_id = X
6. Return the item
a. Verify the checkout moves to old_issues
b. Verify the booking_id is preserved in old_issues
7. Test the new Koha::Booking methods:
a. For a booking with a checkout, verify $booking->checkout returns
the correct Koha::Checkout object
b. For a booking with an old checkout, verify $booking->old_checkout
returns the correct Koha::Old::Checkout object
8. Test the API:
a. Verify checkout endpoints include booking_id in responses
b. Verify the booking relationship can be embedded using
x-koha-embed: booking
9. Run the test suite:
prove t/db_dependent/Circulation.t
prove t/db_dependent/Koha/Booking.t
prove t/db_dependent/Koha/Checkout.t
NOTE: I also thought about the inverse: linking to a checkout from the
booking, but as Koha is a circulation centric system, this approach makes
more sense in my opinion. I also thought about bidirectional linking via
FKs but that is too much maintenance overhead (and potential error source)
that we don't need. The inverse lookups should still be performant through
the FKs.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit adds two key improvements to the Bug 9762 circulation override logging:
1. **Consistent JSON Logging**: Updated AddRenewal to always log in JSON format,
matching AddIssue behavior. Previously AddRenewal only logged JSON when
confirmations/forced were present, otherwise logged just the item number.
2. **Integration Tests**: Added comprehensive tests for both AddIssue and AddRenewal
JSON logging functionality:
- Test normal operations (empty confirmations/forced arrays)
- Test override operations (populated confirmations/forced arrays)
- Verify JSON structure and content accuracy
The AddRenewal function now consistently logs:
```json
{
"issue": 123,
"itemnumber": 456,
"confirmations": [...],
"forced": [...]
}
```
This ensures uniform action log format across all circulation operations and
provides better integration test coverage for the override logging functionality.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
An alternative approach to the original submission for bug 9762. Instead
of recording a distinct action log line for a set of override messages
in addition to the standard issue record, this patch updates the
existing issue log to include any confirmation or impossible responses
from the preceeding CanBookBeIssued call.
We continue to record these as a JSON structure as the prior patch did,
but instead of human messages we use the predefined codes as returned
from the existing CanBookBeIssued response.
To test:
After each step, a single action log message should be displayed under
`Tools > Log viewer`. The message itself will now consist of a JSON
string with an array of 'confirmations' and 'forced' strings
representing the overriden feedback from CanBookBeIssued.
Circulation -- Checking out
1) Check out an item that is age-restricted (AgeRestrictionOverride syspref)
2) Check out an item that has a “not for loan” status (AllowNotForLoanOverride
syspref)
3) Check out an item that has a “lost” status (IssueLostItem syspref)
4) Check out an item to a patron who has reached the checkout limit
(AllowTooManyOverride syspref)
5) Check out an item to a patron who is not allowed to borrow this item type
6) Check out an item to a patron who has unpaid fines (AllFinesNeedOverride
and/or AllowFineOverride + noissuescharge sysprefs)
7) Check out an item on hold for someone else
8) Check out an item on hold ready for pickup by someone else
9) Check out an item already checked out to someone else
(AutoReturnCheckedOutItems syspref)
10) Check out to a patron who has restrictions
Circulation -- Checking in
11) Ignore a hold upon check in
12) Ignore a hold and transfer upon check in
Circulation – Renewing
13) Renew an item on hold for someone else
(AllowRenewalOnHoldOverride syspref)
14) Renew an item that has reached the maximum number of renewals
(AllowRenewalLimitOverride syspref)
Holds
15) Place a hold for a patron who is not allowed to place this item
type on hold (AllowHoldPolicyOverride syspref)
16) Place a hold for a patron who has reached the maximum number of
holds
17) Place an item-level hold when rules dictate that this is not
allowed
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
We never actually pass `$message` back to anywhere so there's no reason
to create it in the first place.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch allows you to save commands that have been overridden by someone.
The patch has been updated but is not yet completed. Tests 10, 13, 14, and 17 are still not functional. Tests 15 and 16 need to be fixed as they do not display the barcode. All other tests should be functional.
To test:
--After each step, a message will be displayed in the action_logs table and in Tools > Log viewer --
Circulation -- Checking out
1) Check out an item that is age-restricted (AgeRestrictionOverride syspref)
2) Check out an item that has a “not for loan” status (AllowNotForLoanOverride syspref)
3) Check out an item that has a “lost” status (IssueLostItem syspref)
4) Check out an item to a patron who has reached the checkout limit (AllowTooManyOverride syspref)
5) Check out an item to a patron who is not allowed to borrow this item type
6) Check out an item to a patron who has unpaid fines (AllFinesNeedOverride and/or AllowFineOverride + noissuescharge sysprefs)
7) Check out an item on hold for someone else
8) Check out an item on hold ready for pickup by someone else
9) Check out an item already checked out to someone else (AutoReturnCheckedOutItems syspref)
10) Check out to a patron who has restrictions
Circulation -- Checking in
11) Ignore a hold upon check in
12) Ignore a hold and transfer upon check in
Circulation – Renewing
13) Renew an item on hold for someone else (AllowRenewalOnHoldOverride syspref)
14) Renew an item that has reached the maximum number of renewals (AllowRenewalLimitOverride syspref)
Holds
15) Place a hold for a patron who is not allowed to place this item type on hold (AllowHoldPolicyOverride syspref)
16) Place a hold for a patron who has reached the maximum number of holds
17) Place an item-level hold when rules dictate that this is not allowed (In circulation rules put "On shelf holds allowed" to "If any unavailable")
Sponsored-by: Solutions inLibro inc. <https://inlibro.com/>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
We need a skip_future_holds flag in item->current_holds to make renewals
act as they did before. There is no need to add it to biblio->current_holds.
Test plan:
Run t/db_dependent/Holds.t
Interface test: Issue a book, place hold on tomorrow, set ConfirmFutureHolds
to 2 days. Verify that renew is not blocked by this hold.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch updates all existing calls to the renamed method throughout
the codebase to maintain compatibility.
Changes:
* Updated C4::Circulation calls to use revert_found()
* Updated C4::Reserves calls to use revert_found()
* Updated test files to use revert_found()
* Maintained all existing functionality and behavior
All existing functionality is preserved - this is purely a method
name change to reflect the expanded capability.
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."
This option takes precedence over patron and patron category preferences.
TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.
Signed-off-by: Laurence <laurence.rault@biblibre.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Pedro Amorim
Martin Renvoize
Michael Hafen
Petro Vashchuk
Emmi Takkinen
Martin Renvoize
Laura_Escamilla
Nick Clemens
Lucas Gass
Baptiste Wojtkowski
Arthur Suzuki
Lari Taskula
Marcel de Rooy
Jonathan Druart
Tomas Cohen Arazi
Paul Derscheid
Hammat Wele
Matthias Le Gac
emilyrose