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>
The test was failing with RegisterRequired exception because payin_amount()
requires a cash register when UseCashRegisters preference is enabled.
Fixed by setting UseCashRegisters to 0 in the lost item return policies test
to allow payments without requiring a cash register.
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: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch adds test coverage for the new booking status management
in AddIssue:
Test scenarios covered:
- Patron checking out their own booked item (marks as completed)
- Different patron checking out booked item during actual booking period
(cancels booking)
- Checkout conflicts only during lead period (preserves booking, respects
librarian decision)
- Checkout period encompassing entire booking period (cancels booking)
- No booking present (normal operation)
Tests ensure proper booking lifecycle management and validate that
librarian override scenarios work as intended.
To test:
1. Apply all patches
2. Run: prove t/db_dependent/Circulation.t
3. Verify all booking status handling scenarios pass
The test plans now provide:
- Step-by-step instructions for manual testing
- Multiple scenarios covering different use cases
- Clear expectations for what should happen in each case
- Practical workflow that staff would actually follow
- Edge cases like lead period conflicts and overrides
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
After pushing bug 38356 to main, Circulation.t tests
started to fail. Fail happened randomly and was related
to sub is_serial trying to fetch biblios metadata. This
patch changes the way test item is build in
"CanBookBeIssued + needsconfirmation message" tests. It
seems more reliable to use build_sample_item rather than
building item using biblio and biblioitem data.
To test prove t/db_dependent/Circulation.t.
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Typo in system preference call 'OPACFineNoRenewalsIncludeCredit'
=> 'OPACFineNoRenewalsIncludeCredits'
Test plan :
Run : prove t/db_dependent/Circulation.t
Run test plan of Bug 23293
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
rather than a barcode.
Will be tested within previous test plan.
Sponsored-by: Reserve Bank of New Zealand
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This has been generated.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This commit is generated using:
% perl misc/devel/tidy.pl
*within* ktd, to get the same version of perltidy than what will be used
by our CI (currently v20230309).
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
prove t/db_dependent/Circulation.t
Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Need to search by old_issue_id instead of issue_id when checking the
value of an accountline after calling AddReturn on the linked checkout
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This patch removes the last remaining use of updateWrongTransfer and the
method itself.
We replace it with a call to request_transfer that passes the 'replace'
reason of 'WrongTransfer' through to Koha::Item::Transfer->cancel.
Test plan
1/ git grep updateWrongTransfer to confirm there are no more uses of the
method.
2/ Confirm an incorrect transfer still behaves as expected at
circulation returns.
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This patch adds further unit tests for the messaging in _set_found_trigger
prove t/db_dependent/Koha/Item.t
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
prove t/db_dependent/Circulation.t
Sponsored-by: Rapid City Public Library
Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
1) Don't automagically always set a transfer to in transit on checkin..
wait for the user to actually confirm that's the case
2) New transfers triggered by a hold should take precidence, so hide
transfers for any other reason from display
3) Update get_transfer and get_transfers to ensure ordering isn't lost
when prefetch is used and add tests for this
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Address new new version of system pref
Address new version of categories DB table BlockExpiredPatronOpacActions column
Test plan, k-t-d, BEFORE applying patch:
1) Edit some patron categories, visit:
/cgi-bin/koha/admin/categories.pl
2) Set different values for the "Block expired patrons" input config
Set a category to block
Set a category to don't block
Set a category to follow the syspref
3) Take note of the current value of the BlockExpiredPatronOpacActions sys pref
Apply patch, then run the following commands
- koha-plack --restart kohadev
- yarn css:build
- run updatedatabase
4) Visit the patron categories you edited before, make sure they all have the correct values for the new config type
5) Check the BlockExpiredPatronOpacActions sys pref and make sure the new value is correct according to what was before
6) Add a new patron of one of the above categories, test that:
If 'hold' is a blocked action, patron is unable to place a hold in OPAC
If 'renew' is a blocked action, patron is unable to renew in OPAC
If 'follow sys pref' is the option, check that a patron may place a hold or renew an item according to the BlockExpiredPatronOpacActions sys pref value
prove t/db_dependent/Circulation.t
prove t/db_dependent/ILSDI_Services.t
prove t/db_dependent/Koha/Object.t
prove t/db_dependent/Koha/Patron/Categories.t
Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Removing the manual transfer and rightaway doing the Reserve
transfer. One test description was misleading too.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
GetOtherReserves attempts to set the waiting/transit status for the next
hold on the list when applicable, but in practice it either leaves the
hold state unchanged, or sets the itemnumber without setting the found
status (erroneously converting bib-level holds to item-level holds).
The latter situation only occurs when the user has been prompted to
confirm, cancel, or revert the hold, and is able to ignore the prompt.
In those situations, the hold's state should not change.
GetOtherReserves does not need to change the hold state, and it does not
do so correctly. Besides that, it does not do much other than call
CheckReserves, and is only used in 3 places.
This patch removes GetOtherReserves, and refactors returns.pl and
C4::Reserves::ModReserveCancelAll to call CheckReserves directly instead.
To test:
1. Place 2 bib-level holds for 2 different patrons (Patron A and Patron
B) on the same record, both for pickup at the logged-in library
2. Check in an item from that record to fill Patron A's hold
3. Set the hold's expiration date to yesterday by accessing the database
in the command line:
- In a ktd shell prompt, open the db client with koha-mysql kohadev
- UPDATE reserves
SET expirationdate = DATE_SUB(CURDATE(), INTERVAL 1 DAY)
WHERE borrowernumber = <Patron A's borrowernumber>
4. Go to Circulation > Holds Awaiting Pickup, and find the hold in the
"holds waiting past their expiration date" tab
5. Click the "Cancel hold" button in the Actions column next to the hold
(do not check in the book)
6. Return to the bib record and look at Patron B's hold
--> Note that Patron B's hold is now an item-level hold and does not
have a waiting status
7. Cancel Patron B's hold
8. Place 2 new holds on the record: one for Patron A at the logged-in
library, and one for Patron B at a different library
9. Check in an item to fill Patron A's hold
10. Repeat steps 3-5 to expire and cancel Patron A's hold
11. Return to the Holds tab of the bib record and look at Patron B's hold
--> Note that Patron B's hold is now an item-level hold, and there is no
"Revert transit status" button
12. Place 2 bib-level holds for 2 different patrons (Patron A and Patron
B) on the same record, both for pickup at the logged-in library
13. Check in an item from that record to fill Patron A's hold
14. Check in the same item again. A modal will pop up, saying that the
hold is already waiting
15. In the modal, choose a cancellation reason and click "Cancel hold"
--> A new modal will pop up to fill Patron B's hold
16. Click "Ignore" on the modal for Patron B's hold
17. Return to the bib record and look at Patron B's hold
--> Note that Patron B's hold is now an item-level hold and does not
have a waiting status
18. Apply patch
19. Repeat steps 1-6
--> Note that Patron B's hold is still a bib-level/"next available" hold
20. Repeat steps 7-11
--> Note that Patron B's hold is still a bib-level/"next available" hold
21. Repeat steps 12-17
--> Note that Patron B's hold is still a bib-level/"next available" hold
Make sure correct behavior is unchanged:
22. Cancel Patron B's hold
23. Place 2 new holds on the record: one for Patron A at the logged-in
library, and one for Patron B at a different library
24. Check in an item from that record to fill Patron A's hold
25. Check in the same item again. A modal will pop up, saying that the
hold is already waiting
26. In the modal, choose a cancellation reason and click "Cancel hold"
--> A new modal will pop up to fill Patron B's hold
27. Click "Print slip, transfer, and confirm" on the modal for Patron B's hold
--> Confirm that the information on the slip is correct
--> Confirm that the hold is correctly put in transit
22. Set HoldsAutoFill and HoldsAutoFillPrintSlip to "Do"
23. Place a bib-level hold for the logged-in library
24. Check in an item from that bib
--> Confirm the information on the slip is correct
--> Confirm the hold is correctly assigned and set to waiting
25. Place a bib-level hold for a different library
26. Check in an item from that bib
--> Confirm the information on the slip is correct
--> Confirm the hold is correctly put in transit
27. Change the logged-in branch to match the hold pickup location
28. Check the item in
--> Confirm the information on the slip is correct
--> Confirm the hold is correctly assigned and set to waiting
29. Repeat steps 22-26
--> Confirm a correct hold slip pops up for Patron B's hold
--> Confirm that Patron B's hold is correctly put in transit
30. Cancel Patron B's hold
31. Place 2 bib-level holds for 2 different patrons (Patron A and Patron
B) on the same record, both for pickup at the logged-in library
33. Repeat steps 24-26
--> Confirm a correct hold slip pops up for Patron B's hold
--> Confirm Patron B's hold is correctly set to Waiting
34. Prove t/db_dependent/Circulation.t
35. Prove t/db_dependent/Koha/Holds.t
--> Tests pass
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This patch further unit tests highlighting possible issues
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This patch adds unit tests that test the functionality more specifically
and adds a few notes/fixme for things we need to consider here
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Add unit tests for new logic introduced to C4::Circulation::AddReturn
Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This patch adds the unit tests for the new system preferences that will
automatically change an items lost status to an authorised value defined
in these preferences on payment or write off of outstanding balance.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
We now require pickup_library_id and so need to update the unit tests to
reflect that for our sample bookings
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Running either t/db_dependent/Circulation.t or t/db_dependent/Circulation/dateexpiry.t
cause following error to be dispalyed:
You are creating a DateTime object with a far future year (9999) and a time
zone (Europe/Helsinki). If the time zone you specified has future DST changes
this will be very slow.
Smallest allowed value is 4999, so we need to use that rather than 9999 in tests.
To test prove t/db_dependent/Circulation.t and t/db_dependent/Circulation/dateexpiry.t.
Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
To test:
1. Apply patch, updatedatabase & schema, restart all Services
2. Go to Administration > Table setting. Find the holdings table ( Catalog > holdings_table ) and turn the local use column on.
3. Go to an item record and notice the column 'Local uses'
4. Turn the system preference RecordLocalUseOnReturn Off.
5. Check in an item that is not checkout out. No local use should be recorded for the item.
6. Turn RecordLocalUseOnReturn on and check in an item that is not checked out. Local use on that item should increment by 1.
7. Create a statistical patron and check an item out to them. Local use should increment by 1.
8. Go to /api/v1/items?external_id={barcode} and make sure the numbers for localuse look correct.
To test maintainence script:
1. Without the patch, have RecordLocalUseOnReturn on.
2. Check in some items to record localuse in the stats table. Keep note of those stats.
3. Apply the patches, updatedatabase.
4. Run the maintenance script, perl update_localuse_from_statistics.pl
5. Now check that items.localuse is congruent with what is in the stats table
Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
1) Set TrackLastPatronActivityTriggers to at least "Checking out an item"
2) Run tests in t/db_dependent/Circulation.t and verify that
failes with "Invalid value passed, borrowers.updated_on..."
3) Apply patch
4) Run Circulatoint.t tests again and verify that no longer
produces this error
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Koha/Illrequests.pm -> Koha/ILL/Requests.pm
Merged:
t/db_dependent/Koha/Illrequests.t
t/db_dependent/Illrequests.t
Into:
t/db_dependent/Koha/ILL/Requests.t
ILL classes file structure is, for the most part, around 7 years old and doesn't follow a strict logic. It's so confusing that some test files exist redundantly.
This housekeeping should help future work in regards to ISO18626 to add Koha as a supplying agency instead of just requesting agency, as is now.
It should also help future housekeeping of moving backend related logic out of the Illrequest.pm into Illbackend.pm (now ILL/Request.pm and ILL/Backend.pm as of this patchset).
It should also help in structuring the addition of a master generic form (see bug 35570)
This patchset will require existing backends to be updated to match the new class names and structure, if they invoke them.
Test plan, k-t-d, run tests:
prove t/db_dependent/api/v1/ill_*
prove t/db_dependent/Koha/ILL/*
Test plan, k-t-d, manual:
1) Install FreeForm, enable ILL module, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) You'll have to switch the FreeForm repo to the one compatible with this work, like:
cd /kohadevbox/koha/Koha/Illbackends/FreeForm
git checkout reorganize_ILL
3) Do some generic ILL testing:
3.1) Create a request
3.2) Add a comment to a request
3.3) Edit a request
3.4) Edit a request's item metadata
3.5) Confirm a request
3.6) List requests
3.7) Filter requests list using left side filters
4) Install a metadata enrichment plugin:
https://github.com/PTFS-Europe/koha-plugin-api-pubmed
4.1) Create an ILL batch and insert a pubmedid like 123
4.2) Add the request and finish batch
5) Verify all of the above works as expected
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Before this patch we get all holds on a record and see if we can fill them with available items.
This means we check to fill holds that the item in questoion may not be able to fill, especially
in the case where no holds are allowed on the item type, this is wrong
To test:
1 - Find or create a biblio with two items of different item types
2 - Make sure one item type allows holds, and the other has:
"Default holds policy by item type"
Set to "No holds allowed"
3 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow"
4 - Check out the unholdable item to a patron
5 - Set a hold for a different patron on the next available item
6 - Confirm the checked out item can be renewed (don't renew, just view the checkouts page)
7 - Checkout the other item to a third patron
8 - Confirm the first item can still be renewed
9 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Allow"
10 - Confirm the item cannot be renewed now
11 - Apply patch, restart all
12 - Confirm the item can be renewed
13 - Set the item type to a type that allows holds
14 - Confirm the item can no longer be renewed
15 - Restore the item type
16 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow"
17 - Confirm the item can be renewed
18 - Check in the item from the third patron
19 - Confirm the item can still be renewed
20 - prove -v t/db_dependent/Circulation.t - test still pass
Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Whilst writing the test, I found a minor flaw in the logic and fixed
that in CanBookBeIssued at the same time.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This patch adds a unit test for error precidence where autorenewals is
involved.
It is not comprehensive however, and I'm a little confused by the logic
around cron vs non-cron handling...
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Tomás Cohen Arazi
Lucas Gass
Paul Derscheid
Martin Renvoize
Nick Clemens
Emmi Takkinen
Fridolin Somers
Jonathan Druart
Aleisha Amohia
Koha Development Team
Pedro Amorim
Emily Lamancusa
Martin Renvoize
Matt Blenkinsop
Marcel de Rooy
Laura Escamilla
Jacob O'Mara
David Gustafsson