Commit Graph

509 Commits

Author SHA1 Message Date
Martin Renvoize 811d8bfd37
Bug 42310: (follow-up) Move Koha::Calendar tree under Koha::Library::Calendar
Koha::Calendar is, by construction, a per-library object — it croaks
without a branchcode argument and every method is branch-scoped. The
top-level namespace hides that. Move the whole tree under
Koha::Library::Calendar to match the actual semantics, the existing
/libraries/{library_id}/calendar REST routes, and the established
Koha::Library::* convention (Group, Hour, FloatLimit, OverDriveInfo).

This commit:
- renames the Koha::Calendar(::*) tree to Koha::Library::Calendar(::*)
- updates package declarations, POD and internal 'use' statements
  within the moved tree
- updates the four DBIC Schema Result overrides (koha_object_class /
  koha_objects_class) to the new namespace
- updates callers inside C4::* modules (Calendar, Circulation,
  Reserves, Overdues, HoldsQueue), Koha::{Patron,Hold,Checkouts,
  CurbsidePickup}, circ/returns.pl, the installer and cronjobs
- updates the 42310-owned tests (Calendar tests, HoldsQueue.t,
  Holds/WaitingReserves.t, Holidays.t, Koha/CurbsidePickups.t,
  Reserves/CancelExpiredReserves.t and the libraries.t CRUD block)

Koha::Library->calendar, the REST controllers and the 42311 tests
are updated in separate follow-up commits under their respective
bugs.

Test plan:
- prove t/db_dependent/Koha/Library/Calendar.t
- prove t/db_dependent/Calendar.t
- prove t/db_dependent/Holidays.t t/db_dependent/HoldsQueue.t
- prove t/db_dependent/Holds/WaitingReserves.t
- prove t/db_dependent/Reserves/CancelExpiredReserves.t

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
2026-09-15 16:14:02 +00:00
Martin Renvoize 4ff58e553b
Bug 31391: (QA follow-up) Remove unused variables in ModReserve recall conversion
$due_interval and $due_date (and $recall itself) returned by
Koha::Recalls->add_recall were captured but never used in either
branch of the new rank => "recall" handling. Drop the unused
assignment - no behaviour change.

Test plan:
1) prove t/db_dependent/Holds.t t/db_dependent/Reserves.t - all pass

Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
2026-09-15 14:45:16 +00:00
Katrin Fischer 3df7914925
Bug 31391: (QA follow-up) Make sure pickup-library is kept when converting holds to recalls
The code was using the patron's home library instead of passing the pickup-location
of the original hold. This is a simple variable change in 2 places for record level and item
level holds.

A) Set 'UseRecalls' to 'staff' at http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=UseRecalls
B) At http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl set the default rule: 'Recalls allowed' = 5, 'Recalls per record' = 3, 'On shelf recalls allowed' = 'if any unavailable' (not 'all').
C) Use biblionumber '437' (has just one item, barcode '39999000019186'), check that item out to 'Henry Acevedo' (Patron A).
D) Place a record-level hold on it for 'Johnny Bryan' (Patron B), who's from 'Midway' — but set the hold's pickup location to 'Centerville' instead.
E) On http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumber=437, tick the hold, click 'Convert selected to recall', confirm.
F) Check the new recall's pickup location on http://localhost:8081/cgi-bin/koha/recalls/request.pl?biblionumber=437
- Expected: 'Centerville'
- Without the patch applied, it will be the patron's home library: Midway
- With the patch applied, the pickup-location of the hold is kept: Centerville.

G) Cancel the recall.
H) Repeat the test plan, but in step D) place an item level hold instead of a record level hold.

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
2026-09-14 14:16:01 +00:00
Aleisha Amohia 98c8add185
Bug 31391: Request recalls and convert reserves on staff interface
This enhancement adds the ability to place recalls via the staff
interface by changing the UseRecalls system preference to have choices which
describe where recalls can be placed. Recalls can either be placed via
the OPAC, or the staff interface, or both. To be set to OPAC would be
consistent with current 'on' behaviour. UseRecalls is turned off by default.

This enhancement also adds the ability to convert existing holds to
recalls easily via the staff interface. The hold will be cancelled
and a recall placed using the hold's information.

To test:
1) Update database and restart services
2) Set UseRecalls to OPAC and set the relevant recalls circulation rules
OPAC only by default
3) Check out Item A to Patron B.
4) Log into the OPAC as Patron A. Confirm you can place, view and cancel
recalls as normal.
5) In the staff interface, set UseRecalls to Staff interface only.
6) In the OPAC, confirm you can still view and cancel recalls, but can
no longer place recalls.
7) In the staff interface, search for Item A and go to the Holds tab.
Place a hold for Patron C.
8) Check the checkbox for your hold and confirm the counts in the buttons above the Holds table are incremented as expected.
9) Click the button to Convert selected holds to recalls, a modal should pop up. Confirm all of the information is correct.
10) Confirm the conversion and once the page refreshes, your hold should be correctly cancelled using the new RECALLED reason.
11) Go to the Recalls tab. Confirm your hold has been converted to a
recall, and details like the patron, expiration, and pickup location have
carried across to the recall.
12) On this Recalls page, use the patron search to find Patron D. Test
placing a record-level recall by choosing to recall the next available item.
13) When the page refreshes, confirm the recall was successfully placed
with the correct details stored.
14) Repeat steps 12 and 13 with Patron E, this time place an item-level
recall by choosing an item in the 'place a recall on a specific item'
table. When selecting one of these items, the 'recall next available
item' section should become disabled.
15) Set UseRecalls back to OPAC only. Go back to the record and
view the Recalls tab.
16) Confirm you cannot search for a patron to place a recall, but can
still view and cancel recalls. Confirm you also cannot convert holds
to recalls.
17) Set UseRecalls to both staff interface and OPAC. Confirm you
can place recalls on both interfaces.
18) On the staff interface, test placing recalls that your circulation
rules do not allow. For example, if 'recalls per record' is 1, ensure
you're blocked from placing a second recall on a record for a patron. Another example, testing converting a hold to a recall when there are NO checked out items, as recalls can only be placed if something is checked out.
19) Place a hold for one of the patron's that already has a recall.
Once complete, try to convert this hold into a recall. Confirm you
are blocked and shown a message that the hold cannot be converted to a
recall.
20) Confirm patron messaging preferences for recalls notices can still be configured as expected.
21) Ensure tests pass:

* t/db_dependent/Circulation.t
* t/db_dependent/Circulation/transferbook.t
* t/db_dependent/Holds.t
* t/db_dependent/Koha/Biblio.t
* t/db_dependent/Koha/Item.t
* t/db_dependent/SIP/Transaction.t
* t/db_dependent/Search.t
* t/db_dependent/XSLT.t

Sponsored-by: Auckland University of Technology
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
2026-09-14 14:15:54 +00:00
Tomás Cohen Arazi b9868ce2d2
Bug 42529: Use Koha::Policy::Biblio::AgeRestriction for issuing and holds
This patch updates CanBookBeIssued and CanItemBeReserved to use the
new policy class instead of calling GetAgeRestriction directly.

Test plan:
1. Run tests without this patch:
   $ ktd --shell
   k$ prove t/db_dependent/Circulation.t \
            t/db_dependent/Reserves.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat step 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
2026-08-06 16:07:29 +00:00
Martin Renvoize 4b23e5c5d1
Bug 41410: (follow-up) Decouple exclusivity from LocalHoldsPriority
LocalHoldsExclusivity and LocalHoldsPriority are adjacent but distinct
features: priority alters *which patron's hold* gets filled first, while
exclusivity restricts *which items* may fill a hold during a time window.
Each should be usable independently of the other.

Changes:
- Rename LocalHoldsPriorityExclusivityPeriod to LocalHoldsExclusivityPeriod
- Add LocalHoldsExclusivityPatronControl (PickupLibrary/HomeLibrary)
- Add LocalHoldsExclusivityItemControl (homebranch/holdingbranch)
- HoldsQueue: add _flag_local_holdgroup_matches() that flags
  local_holdgroup_match on allocated items using hold-group matching
  and the new exclusivity control prefs, regardless of LocalHoldsPriority
- Reserves: CheckReserves exclusivity check now computes its own
  "is this item local" using the new prefs and hold-group siblings,
  independent of the LocalHoldsPriority matching variables
- Atomicupdate handles both fresh install and rename-from-old-pref paths
- Pref UI note clarifies independence from LocalHoldsPriority

Test plan:
1. Apply patch, run updatedatabase.pl
2. Verify three new sysprefs exist in Administration > System preferences
   > Circulation: LocalHoldsExclusivityPeriod, LocalHoldsExclusivityPatronControl,
   LocalHoldsExclusivityItemControl
3. Set LocalHoldsPriority to "Don't give" (None)
4. Create a library hold group with at least two libraries (e.g. Lib-A, Lib-B)
5. Create a third library (Lib-C) NOT in the hold group
6. Create an item at Lib-B
7. Place a hold for a patron at Lib-A (pickup at Lib-A)
8. Set LocalHoldsExclusivityPeriod to 7, PatronControl to HomeLibrary,
   ItemControl to homebranch
9. Run the holds queue builder (misc/cronjobs/holds/build_holds_queue.pl)
10. Verify hold_fill_targets.local_holdgroup_match = 1 for that hold
    (item at Lib-B is in the same hold group as patron at Lib-A)
11. Check in the item at Lib-C — it should NOT fill the hold (exclusivity
    blocks the non-local item within the 7-day window)
12. Check in the item at Lib-B — it SHOULD fill the hold (local match)
13. Reset: delete the hold, create a new one with a patron at Lib-C
    (pickup at Lib-C). Run the queue again.
14. Verify hold_fill_targets.local_holdgroup_match = 0 for this hold
15. The item at Lib-B should now fill this hold at Lib-C checkin
    (non-local patron, no exclusivity protection)
16. Set LocalHoldsExclusivityPeriod to 0 — verify non-local items can
    fill holds normally regardless of hold_fill_targets flag
17. prove t/db_dependent/Holds/LocalHoldsPriority.t — passes
18. prove t/db_dependent/HoldsQueue.t — passes
19. prove t/db_dependent/Reserves.t — passes

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
2026-07-29 15:50:12 +00:00
Martin Renvoize a2ef73fe95
Bug 41410: Add local holds priority exclusivity period
When LocalHoldsPriority is enabled, the holds queue picks a local-group
item for eligible holds.  Until now there was nothing to reserve that
priority at checkin time: if a non-local item was returned first, it
would happily fill the hold and the local-priority decision was lost.

This patch adds a time-bounded exclusivity window so that, for a
configurable number of days after a hold is placed, a hold whose queue
target is a local-group item will not be filled by a non-local item.

Changes:
 - C4/HoldsQueue.pm: in MapItemsToHoldRequests, flag %item_map entries
   that matched on local library (GiveLibrary/GiveLibraryAndGroup) or
   local hold group (GiveLibraryGroup/GiveLibraryAndGroup) with
   local_holdgroup_match => 1, and propagate that into both the
   tmp_holdsqueue and hold_fill_targets INSERTs.
 - C4/Reserves.pm: in the CheckReserves loop, when
   LocalHoldsPriorityExclusivityPeriod is set, the hold is still within
   that window, and the item being evaluated is not itself a local
   match, skip the reserve if its hold_fill_targets row has
   local_holdgroup_match = 1.  Missing hold_fill_targets row means "no
   information" -- fall through so holds are never trapped between
   queue runs.
 - circulation.pref: expose the new LocalHoldsPriorityExclusivityPeriod
   preference next to the existing LocalHoldsPriority* entries.

The feature is a no-op unless both LocalHoldsPriority is set to
something other than "Don't give" AND
LocalHoldsPriorityExclusivityPeriod is > 0.

Test plan:
 1) Apply the patches and run the atomicupdate.
 2) Set up a library group flagged as a local hold group containing
    two libraries (call them A and B).  Add a third library C that is
    NOT in the group.
 3) Add one item to each library (itemA, itemB, itemC) on the same
    biblio.
 4) Administration -> System preferences -> Circulation:
    - Set LocalHoldsPriority to "Give library group".
    - Set LocalHoldsPriorityPatronControl to "home library".
    - Set LocalHoldsPriorityItemControl to "home library".
    - Set LocalHoldsPriorityExclusivityPeriod to 7.
 5) As a patron whose home library is A, place a title-level hold with
    pickup at A.
 6) Run the holds queue (misc/cronjobs/holds/build_holds_queue.pl).
    -> The queue should target itemA (or itemB) for this hold.
    -> Confirm in the database:
         SELECT reserve_id, itemnumber, local_holdgroup_match
           FROM hold_fill_targets;
       The targeted row must have local_holdgroup_match = 1.
    -> Holds to pull / holds queue report should also show the flag
       (tmp_holdsqueue.local_holdgroup_match = 1).
 7) At library C, check in itemC (the non-local item).
    -> The hold must NOT be captured -- itemC simply goes back on the
       shelf.
 8) At library A, check in itemA (the local item).
    -> The hold IS captured for the patron.
 9) Cancel and re-place the hold, run the queue again.  Manually age
    the hold past the window:
         UPDATE reserves
            SET reservedate = DATE_SUB(CURRENT_DATE, INTERVAL 10 DAY)
          WHERE reserve_id = <id>;
    Now check in itemC again.
    -> The hold IS captured (exclusivity window has expired).
10) Re-place the hold but do NOT run the queue.  With no
    hold_fill_targets row for the reserve, check in itemC.
    -> The hold IS captured (we never trap holds between queue runs).
11) Flip LocalHoldsPriorityExclusivityPeriod back to 0 and repeat
    step 7.
    -> The hold IS captured; the feature is fully disabled.
12) Run:
      prove t/db_dependent/Holds/LocalHoldsPriority.t \
            t/db_dependent/HoldsQueue.t \
            t/db_dependent/Reserves.t \
            t/db_dependent/Koha/Hold.t
    -> All tests pass.

Sponsored-by: Main Library Alliance <https://www.mainlib.org/>

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
2026-07-29 15:50:09 +00:00
Martin Renvoize 04381f6aba
Bug 41358: Apply consistent JSON logging to holds (Bug 40866)
Bug 40866 introduced JSON logging for HOLDS CREATE but stored null for
'confirmations' and 'forced' when no overrides were present. This patch
mirrors the earlier change to CIRCULATION ISSUE by defaulting them to
empty arrays, keeping the two log streams structurally identical and
simplifying reporting queries.

Changes:

C4/Reserves.pm:
- Default confirmations/forced to [] in the AddReserve log payload.

t/db_dependent/Reserves.t:
- Update the Bug 40866 subtest to assert the empty-array defaults on
  normal holds, and fix a 'my $log_data' redeclaration.

tools/viewlog.pl:
- Extend the CSV export JSON-decode block to also cover HOLDS logs,
  guarding the item lookup on itemnumber being present (HOLDS payloads
  may lack one) and filling the CSV item columns from the decoded
  payload. The legacy bare-itemnumber fallback is restricted to
  CIRCULATION so a legacy HOLDS hold_id is not mistaken for an
  itemnumber.

koha-tmpl/intranet-tmpl/prog/js/viewlog.js:
- Extend the info column override rendering (renderCircInfo) to HOLDS
  entries so forced/confirmations arrays display translated, exactly as
  they do for CIRCULATION. The log viewer moved to an ajax datatable in
  bug 29016, so this replaces the earlier json_found change to
  viewlog.pl/viewlog.tt.

misc/maintenance/migrate_action_logs_to_json.pl:
- Extended to handle legacy pre-Bug 40866 HOLDS CREATE logs where info
  is a bare hold_id, recovering branchcode / biblionumber / itemnumber
  from old_reserves / reserves.
- Uses the same keyset-pagination pattern as the circulation loop so
  rows are not skipped under --commit.

Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
2026-07-16 10:54:22 +00:00
Martin Renvoize 5b2293c778
Bug 42764: Fix false positives caused by POD letter-code formatting
Two patterns in the codebase produce spurious codespell hits because
codespell tokenises word fragments out of formatted letter codes:

- B<P>rovider in C4/Auth_with_shibboleth.pm produces 'rovider'
- T(ransit), W(aiting), F(inished), P(rocessing) in C4/Reserves.pm
  produce 'ransit', 'aiting', 'inished', 'rocessing'

For the shibboleth POD line the bold-letter markup cannot be simplified
without losing the formatting intent, so a per-line # codespell:ignore
comment is the right suppression.

For the Reserves.pm status table the fix is made at source: the notation
is changed from X(word) to X (Word) so codespell sees the full English
word and no fragment is produced.

Because the per-line comment on the shibboleth line now suppresses
'rovider' directly, the global .codespell-ignore entry for that fragment
is removed as redundant.

Test plan:
1. Apply patch
2. prove xt/author/codespell.t
=> No new codespell failures

Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
2026-06-03 16:09:05 +00:00
Martin Renvoize 04809de849
Bug 42764: Fix spelling errors across the codebase
Correct genuine spelling errors identified by the codespell 2.4.x
full scan after the KTD container OS upgrade. Changes are in
comments, POD, strings, and template text across 55 files.

"SAN Ouest Provence" (the French library co-operative and Koha
sponsor) is preserved as a proper noun. Perl copyright lines
receive an inline # codespell:ignore annotation; about.tt files
(contributor attribution pages) are added to the Koha::Devel::Files
codespell exception list so that future uses of "Provence" as a
typo for "Province" are still caught in all other files.

Notable user-visible fixes:
- "submiting" -> "submitting" in authority and bibliographic merge screens
- "deleteable" -> "deletable" in patron exception messages
- "Acquistions" -> "Acquisitions" in reports dictionary

Test plan:
1. Apply patches
2. In the staff interface, go to Cataloguing > Merge records and
   confirm the submit button reads "Submitting" (not "Submiting")
3. In the staff interface, go to Authorities > Merge and confirm
   the same correction
4. Attempt to delete a patron with outstanding issues and confirm
   the error message reads "deletable" (not "deleteable")
5. prove xt/author/codespell.t
=> All tests pass

Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
2026-06-03 16:09:04 +00:00
Pedro Amorim e56c9cc9bb
Bug 42255: Maximum total holds allowed (count)
Make 'Maximum total holds allowed (count)' (aka max_holds) consider hold groups

Test plan:
Same as the previous test plan, but set 'Maximum total holds allowed (count)' as 2 and instead set 'Holds allowed (total)' as unlimited.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2026-05-18 09:30:15 -06:00
Pedro Amorim d5f130da5a
Bug 42255: Holds allowed (total)
Make 'Holds allowed (total)' (aka reservesallowed) consider each hold group as an individual hold

Test plan:
1) Enable DisplayAddHoldGroups sys pref
2) Set Holds allowed (total) to 2, visit:
http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl
3) Scroll to the right, click the 'Edit' button under 'Actions' column.
4) On the 'Holds allowed (total)' column cell, enter '2' and click 'Save' under the 'Actions' column to the right.
5) Search  for 'test':
http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=test
6) Tick the checkbox next the first and second results, click 'Place hold' button at the top toolbar
7) Search for 'koha' user and pick Koha (42)
8) Set a pickup location for each hold and tick the 'Treat as hold group' checkbox. Click 'Place holds'.
9) Try to place another single hold for the same patron. Should succeed, because the 2 grouped holds count as 1. Patron now should have 1 group + 1 ungrouped hold = effective count of 2.
10) Try to place yet another hold, should be blocked with a tooManyHolds error.
11) Prior to this patch, step 9 would've failed because the 2 grouped holds counted as 2.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2026-05-18 09:30:08 -06:00
Pedro Amorim c32d08e560
Bug 37762: Catalogue: Hook to biblios
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2026-05-08 12:25:44 -06:00
Pedro Amorim 950c9ccb96
Bug 37762: Circulation: Hook to holds
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2026-05-08 12:25:43 -06:00
Lucas Gass 9539b21089
Bug 41267: (follow-up) Check if exists
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2026-04-10 09:47:51 -06:00
Nick Clemens 876869544f
Bug 41267: Add fill_other_biblio_holds_policy circulation rule
This patch adds a new rule for itemtypes to allow/prevent filling other holds on a bibliographic record
The rule will obey ReservesControlBranch for determining the rules

To test:
 1 - Apply patches
 2 - Setup/choose 2 item types in Administration->Item types
 3 - In Administration -> Circulation and fine rules scroll to the bottom
 4 - Set one item type to "Fill other holds on record" = 'Yes' and the other to 'No'
 5 - Place a next available hold on the record
 6 - Checkout the 'Yes' item type to the patron
 7 - Their hold should fill
 8 - Check in the items
 9 - Place a next available hold on the record
10 - Checkout the 'No' itemtype to the patron
11 - Their hold should not be filled
12 - Check in the item and confirm the hold
13 - Checkout the itme to the patron, the hold fills
     NOTE: While seemingly counter intuitive, the rule controls whether a hold NOT tied to this item will fill
     Confirming the hold adds an itemnumber to the hold. In most cases, these items will not be holdable, so won't trigger at checkin
14 - Set a rule for the patrons branch to No and set system preference ReservesControlBranch to 'Patrons library'
15 - repeat 5&6
16 - The hold should not fill
17 - Change reserves control branch to items home library
18 - checkin /checkout the yes item type
19 - The hold should fill

Signed-off-by: Andrew Auld <andrew.auld@openfifth.co.uk>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2026-04-09 08:27:26 -06:00
Emily Lamancusa 83e2c3888d
Bug 42147: Log all relevant hold data on hold creation
When logging a hold create action, include all hold fields that may be
defined at hold creation.

Fields that are not defined until later (such as found status, waiting
date, etc) are not included to avoid clutter.

To test:
1. System preference setup
    i.  Enable HoldsLog system preference
    ii. Set AllowHoldPolicyOverride to Allow
2. Place a hold in the staff interface
3. View the action logs entry for that hold creation
    i. Go to Tools > Log viewer
    ii. Select Modules: Holds and Action: Create
    iii. Click Submit
--> Note that the "info" column only shows the hold id
4. Apply patch and restart_all
5. Repeat steps 2-3
--> The "info" column should now show hold details

Signed-off-by: Emmanuel Bétemps <e.betemps@gmail.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
2026-03-26 14:21:14 +01:00
Martin Renvoize 9ed90d53fb
Bug 41801: (RM follow-up) Add POD for _cache 2026-03-21 20:28:21 +00:00
Nick Clemens 48386d8570
Bug 41801: (QA follow-up) Ensure toggling priority sets correct rank 2026-03-21 20:28:16 +00:00
Martin Renvoize e0bdf3e859
Bug 41801: (QA follow-up) Prevent race condition with txn_do and FOR UPDATE
FixPriority has a read-modify-write structure: it SELECTs all active
holds for a biblionumber, computes new priority order in Perl, then
issues N UPDATEs.  Two concurrent callers (e.g. two patrons adjusting
holds on a popular title at the same moment) could both read the same
snapshot and then overwrite each other's renumbering, leaving duplicate
or incorrect priority values.

Fix by:

1. Wrapping the entire function body in txn_do so it runs atomically.
   txn_do uses MySQL savepoints when called inside an existing
   transaction, so callers that already hold a transaction are unaffected.

2. Adding FOR UPDATE to the initial SELECT so the first caller
   acquires row-level locks on all holds for that bib.  A second
   concurrent caller will block at the SELECT until the first commits,
   then reads the already-corrected priorities and produces a consistent
   result.

Note: FOR UPDATE locks only rows that exist at read time; a concurrent
INSERT of a brand-new hold can still slip through, but priority
assignment for new holds goes via AddReserve → FixPriority, which will
also acquire the lock and renumber correctly.
2026-03-21 20:28:16 +00:00
Martin Renvoize 4d61ac803f
Bug 41801: (QA follow-up) Replace MAX subquery with in-memory array count for rank clamping
The lowestPriority rank-clamping block fired a second SQL query to get
max(priority) WHERE lowestPriority=0 and used that value as both a
threshold and a splice index.  This was fragile: if priorities had
become discontiguous (e.g. due to a race or a bug elsewhere), the DB
priority value would not match the 1-based position in @priority, and
the splice could land the hold in the wrong slot.

The @priority array already contains all the information needed: it is
sorted lowestPriority ASC so the count of rows where lowestPriority=0
is exactly the number of non-lowest holds, which equals the last valid
rank for a normal hold.  Fetch lowestPriority in the existing SELECT and
use scalar grep { !$_->{lowestPriority} } @priority instead.

This eliminates the extra DB round-trip and makes the clamping operate
on the same data snapshot that will be written back.
2026-03-21 20:28:16 +00:00
Martin Renvoize 4f32ba4323
Bug 41801: (QA follow-up) Guard lowestPriority rank clamping against empty rank
When FixPriority is called with a reserve_id but no rank (rank defaults to ''),
the empty string evaluates to 0 numerically, satisfying `$rank <=
$highest_non_lowest_priority`. This caused the clamping block to unexpectedly
set rank to a positive value, which then triggered an unintended splice and
moved the hold.

Add `$rank > 0` to the guard condition so the block only fires when an explicit
positive rank was requested, matching the pre-existing semantics of the splice
block below it.
2026-03-21 20:28:15 +00:00
Martin Renvoize 8e8a5f914e
Bug 41801: (QA follow-up) Remove dead ignoreSetLowestRank param and update POD
The recursive self-call loop that ignoreSetLowestRank guarded against was
removed by the main patch. Remove the now-unused parameter from both the
function body and the POD documentation, and fix a double-word typo in the
POD while here.
2026-03-21 20:28:15 +00:00
Nick Clemens 1f089c41c1
Bug 41801: Remove recursive calls from FixPriority
This patch removes the recursive calls that set each 'lowestPriority' hold to the bottom of the priority list
and replaces it them by simply ordering the list of 'remaining' holds by lowestPriorty, then priority.

A check is also added to ensure that if a lowestPriority hold is being adjusted, it cannot be moved above holds
not marked as lowest priority.

To test:
 1 - Place many holds on a record, 50 - 150
     https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl
     This will randomly make some of the holds lowest priority
 2 - View the record in the staff interface:
     http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=312
 3 - Click on the 'Holds' tab
 4 - Adjust the priority of several holds using the dropdowns
 5 - Note the timing
 6 - Apply the patch
 7 - Adjust the priority of several holds using the dropdowns
 8 - Confirm the timing is improved
 9 - Confirm that holds adjust as expected
10 - Confirm you cannot move a lowest priority hold above a hold not marked lowest priority
11 - Confirm holds move correctly on a record where all holds are lowestPriority
12 - Confirm holds move correctly on a record where no holds are lowestPriority

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
2026-03-21 20:28:15 +00:00
Johanna Räisä ee4f4f3357
Bug 23269: (follow-up) Add tests for item_level and item_id in holds API
This patch adds tests for the item_level and item_id parameters in the holds API.
It also updates the C4/Reserves.pm module to handle these parameters correctly.

Test plan:
1. prove t/db_dependent/api/v1/holds.t

Sponsored-by: Koha-Suomi Oy
2026-03-21 20:28:14 +00:00
Martin Renvoize f1b20b645c
Bug 3492: Modernize hold fee system and migrate to circulation rules
This commit completely modernizes the hold fee system by migrating from
the deprecated categories.reservefee column to the circulation rules framework
and implementing modern Koha::Objects patterns.

WHAT CHANGED:

1. MIGRATION TO CIRCULATION RULES:
   - Moved hold fees from categories.reservefee to circulation_rules table
   - Added database migration script with atomic update
   - Maintains existing fee amounts during migration
   - Provides rule hierarchy: category+itemtype+branch specific rules

2. NEW SYSTEM PREFERENCE: TitleHoldFeeStrategy
   - Options: highest, lowest, most_common
   - Addresses inconsistent title-level hold fees when items have different fees
   - Default: 'highest' (safest approach for library revenue)

3. MODERN Koha::Objects API:
   - Koha::Item->holds_fee($patron): Calculate fee for specific item
   - Koha::Hold->calculate_hold_fee(): Enhanced item/title-level logic
   - Koha::Hold->should_charge($stage): Centralized charging logic
   - Koha::Hold->charge_hold_fee(): Modern fee charging with proper accounting
   - Removed C4::Reserves::GetReserveFee and ChargeReserveFee

4. ENHANCED FUNCTIONALITY:
   - Proper ReservesControlBranch logic (patron home vs item home library)
   - Complete HoldFeeMode implementation (placement/collection/not_always)
   - Clear OPAC messaging with "You will be charged..." commitment
   - Comprehensive test coverage for all fee scenarios

5. TECHNICAL IMPROVEMENTS:
   - Object-oriented design following Koha patterns
   - Proper error handling and edge case management
   - Modern test structure with comprehensive coverage
   - Clean separation between fee calculation and charging

TEST PLAN:

A) MIGRATION AND SETUP:
   1. Apply all patches and run database update
   2. Verify existing hold fees migrated to circulation rules:
      - Administration > Circulation and fine rules
      - Previous categories.reservefee values now in "Hold fee" column
   3. Confirm old categories.reservefee column no longer used

B) BASIC FEE CALCULATION:
   1. Set up patron categories with different hold fees in circulation rules
   2. Create items with different item types, each with different hold fees
   3. Test item-level holds charge correct fees based on specific item
   4. Test title-level holds use TitleHoldFeeStrategy preference

C) TITLE-LEVEL HOLD STRATEGIES:
   1. Set TitleHoldFeeStrategy to "highest":
      - Create title with items having fees $1.00, $2.00, $3.00
      - Verify title-level holds charge $3.00
   2. Set TitleHoldFeeStrategy to "lowest":
      - Verify same title charges $1.00
   3. Set TitleHoldFeeStrategy to "most_common":
      - Add multiple items with $2.00 fee
      - Verify title-level holds charge $2.00 (most common)

D) HOLDFEEMODE PREFERENCES:
   1. Set HoldFeeMode to "any_time_is_placed":
      - Place holds and verify immediate fee charging
      - Check patron account for correct hold fee debits
   2. Set HoldFeeMode to "any_time_is_collected":
      - Place holds, verify no immediate charges
      - Check out items to patron, verify fee charged on collection
   3. Set HoldFeeMode to "not_always":
      - With available items: verify no fee charged
      - With all items checked out but no other holds: verify no fee
      - With all items checked out and other holds exist: verify fee charged

E) RULE HIERARCHY TESTING:
   1. Set different fees for different category/itemtype/branch combinations
   2. Verify most specific rule applies:
      - Branch + Category + ItemType (most specific)
      - Category + ItemType
      - Category only (most general)
   3. Test with different patron categories and item types

F) USER INTERFACE TESTING:
   1. OPAC interface:
      - Verify "You will be charged..." messages display correct amounts
      - Test both item-level and title-level hold fee messages
      - Confirm fees match actual charges in patron account
   2. Staff interface:
      - Place holds for patrons and verify fee calculations
      - Check Administration > Circulation rules interface works correctly

G) EDGE CASES AND ERROR HANDLING:
   1. Test $0.00 fees (no charge scenarios)
   2. Test patrons with no applicable fee rules (should default to $0.00)
   3. Test items with no holdable copies (should return $0.00)
   4. Test invalid/missing itemtypes or categories
   5. Test holds on biblios with no items

H) REGRESSION TESTING:
   1. Verify existing hold placement workflows unchanged
   2. Check patron account charging remains accurate
   3. Confirm OPAC and staff interfaces display correctly
   4. Test that existing holds continue to work properly

I) UNIT TESTS:
   Run the following test suites to verify functionality:
   - prove t/db_dependent/Koha/Hold.t
   - prove t/db_dependent/Koha/Item.t
   - prove t/db_dependent/Reserves.t
   - prove t/db_dependent/Circulation.t

BACKWARDS COMPATIBILITY:
- All existing functionality preserved
- Hold fee amounts maintained during migration
- Existing workflows continue unchanged
- Legacy code updated to use new API seamlessly

Sponsored-by: OpenFifth <https://openfifth.co.uk>
Signed-off-by: Jackie Usher <jackie.usher@westsussex.gov.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2026-02-09 12:57:56 -07:00
Baptiste c800ea71de
Bug 40960: Print notification only if selected type is impossible
Test plan (ktd):
1 - Have henry acevedo have no mail and set messaging preference "Hold filled to sms"
2 - Have edna acosta have not messaging preference
3 - Place an hold on two books for edna acosta and henry acevedo, check in both books and select the button to fill the hold
4 - Check in notices and see that both received a notice
5 - Apply tests and run tests -> they should fail
6 - Apply patch, run rest -> they should pass
7 - Unfill both holds and fill them back. Henry should keep receiving notices while Edna should not anymore.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2026-02-06 15:17:31 -07:00
Lucas Gass d2a3fe93f2
Bug 41475: Make hostitemnumbers an array ref
To test:

1. Set 'EasyAnalyticalRecords' to 'Show'.
2. Find or create a parent record that has at least 2 items on it. Note the itemnumbers and the framework.
3. Edit that framework so you can add a 773$0, 773$9, and 773$t
4. Find or create a child record.
5. Make at least 2 773 entries on the child record. Add the biblionumber of the host record to the 773$0.
6. In each entry for the 773$9 add one of the itemnumbers from the host record.
7. For the 773$t and the biblio title of the host record.
8. Save.
9. Now attempt to put a hold on the child record.
10. 500 error:
11. APPLY PATCH and restart_all
12. Try placing the hold again, this time it succeeds.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2026-02-06 15:17:30 -07:00
Marcel de Rooy 56410f9c90
Bug 19690: Change comments in IsAvailableForItemLevelRequest
The term 'If any unavailable' is not very clear but it is also
very hard to find something short and better.

A next available hold is possible when at least one item is
unavailable (checked out, on order, waiting/transit hold).
A item level hold is possible when the specific item is
unavailable.

Test plan:
Read the patch. The code did not change.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2026-02-06 15:17:29 -07:00
Nick Clemens f21255ca54
Bug 41416: Add skip_fix_priority option to ModReserve
This patch adds a new parameter to ModReserve: skip_fix_priority

The page for altering priority ensures each hold has a unique rank, so doesn't allow for setting two
holds to the same rank, so we don't need to reorder everything until the end (in case of some cancellations etc)

This patch also makes FixPriority exported and updates the name as it is already called in a few places outside of
C4:Reserve

To test:
 1 - Place many holds on a record, 50 - 150
     https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl
 2 - View the record in the staff interface:
     http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=312
 3 - Click on the 'Holds' tab
 4 - Adjust the priority of several holds using the dropdowns
 5 - Open the network tab in the browser console (F12) to view the timing
 6 - Click 'Update hold(s)' and note the time
 7 - Apply patch, restart all
 8 - Repeat
 9 - Confirm timing is better and numbering updates correctly
10 - Sign off!

Note: The script adds holds that may be marked as 'lowestPriority' these will always all group at the bottom of the record
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jan Kissig <bibliothek@th-wildau.de>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2026-01-16 14:07:18 -07:00
Jonathan Druart beac9eae8f
Bug 40680: Move EXPORT_OK right after Modern::Perl
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>
2025-12-01 08:33:56 -07:00
Pedro Amorim 3569da3996
Bug 40529: Behavior change: Main patch
When a hold that belongs to a hold group becomes 'In transit' or 'Waiting',
it now becomes that hold group's target hold.
Subsequent check-ins that would satisfy other holds in the same hold
group are skipped, as they are part of a group that already has a
target.

The behavior change introduced here that differs from the groundwork
from bug 15516 are:
1) Before, only 'Waiting' holds would consider an action based on hold
   groups i.e. cancel the other holds in the group. Now 'In transit'
   also does this.
2) This hold groups related action used to be to cancel other holds in
   the group. This is no longer the case. Now, the respective hold is
   set as the hold group's target. This causes the holds in the same group that
   are not the target to be skipped when a check-in that would otherwise
   satisfy one of those holds happens. This behavior change allows the following
   scenario which wasn't possible before:

   A hold is marked 'Waiting'. It becomes its group target. For any reason,
   that hold is then cancelled. Any subsequent check-in of an item of a
   hold in this group may now becomes that group's new target.
   Similarly, the same use case above is also true for 'In transit' holds.

----

   The 'skip non-target holds' behavior has been added to check-in logic
(Reserves::_Findgroupreserve), 'holds to pull' logic
(pendingreserves.pl) and holds queue (HoldsQueue.pm).

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2025-11-03 14:13:22 -07:00
Pedro Amorim 0a0ba6fea0
Bug 40517: (QA follow-up): Cleanup obsolete code
The patch 'Move creation of hold group into Patron class' is
moving the creation of the hold group logic to the Patron.pm class file
but it didn't remove all the obsolete code.
This is fixed here.

Signed-off-by: Anneli <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2025-10-29 11:50:10 -06:00
Martin Renvoize 1884f68c5b
Bug 40866: Implement Alternative pattern for holds override logging (Part 4)
This patch implements proper override logging for holds following the
Alternative implementation pattern used for checkouts and renewals.

Instead of just removing the orphaned logging code from the original
Bug 9762 implementation, this patch properly implements override logging
by updating AddReserve to accept and log override information, similar
to how AddIssue handles confirmations and forced overrides.

Changes:

C4/Reserves.pm:
- Added JSON qw(to_json) import
- Updated AddReserve to accept confirmations and forced parameters
- Enhanced the existing logaction call to include override information
  in JSON format when overrides are present
- Log format matches the Alternative implementation pattern:
  * hold ID
  * branchcode
  * biblionumber
  * itemnumber
  * confirmations array
  * forced array

Koha/REST/V1/Holds.pm:
- Added confirmations array building in the add() method
- Pass 'HOLD_POLICY_OVERRIDE' confirmation when override_all is true
- Pass confirmations parameter to AddReserve call

reserve/request.pl:
- Removed orphaned original implementation logging code
- Removed unused variables and imports (message, @message, user, etc.)
- Removed C4::Log import

koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc:
- Added 'HOLD_POLICY_OVERRIDE' case to translate_log_override block
- Displays as "Hold policy override" in log viewer

koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt:
- Added JSON display handling for HOLDS module (similar to CIRCULATION)
- Displays hold ID, biblionumber, and optional itemnumber with links
- Shows forced and confirmation overrides using translate_log_override

This ensures hold overrides are logged consistently with the Alternative
implementation pattern, providing proper audit trails for policy overrides
that are properly displayed in the log viewer.

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>
2025-10-09 14:58:34 -06:00
Tomas Cohen Arazi 1873d97c07
Bug 40163: Fix several http addresses in the code docs
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>
2025-09-23 16:21:29 -06:00
Tomas Cohen Arazi 47f0aab19f
Bug 40636: (follow-up) Cache the calendar object
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2025-09-05 08:02:32 -06:00
Tomas Cohen Arazi 31dced9c8b
Bug 40636: Fix holiday logic in CancelExpiredReserves with comprehensive edge case handling
CancelExpiredReserves had incorrect holiday logic that checked if **today** is a holiday instead of checking if the **hold's expiration date** was a holiday.

This caused:
* Holds that expired on non-holidays to remain active if cleanup script ran on holidays
* Holds that expired on holidays to be cancelled if cleanup script ran on non-holidays

The simple fix exposed additional edge cases around server downtime that could leave holds active indefinitely.

Changes:
* Implements comprehensive holiday logic when `ExpireReservesOnHolidays = 0`
* Same-day protection: Don't cancel holds that expire TODAY if today is a holiday
* Historical cleanup: Cancel holds that expired in the past when running on business days
* Retrospective cancellation: Handle server downtime by checking for missed business days
* Comprehensive test suite covering all scenarios including long-term expired holds

To test:
1. Apply the tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Reserves/CancelExpiredReserves.t
=> FAIL: Tests fail! The logic in the method is wrong!
3. Apply this patch
4. Repeat 2
=> SUCCESS: All tests pass, demonstrating correct behavior in all scenarios
5. Sign off :-D

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2025-09-05 08:02:31 -06:00
David Cook c0ea7bd469
Bug 15516: (follow-up) fix count_holds() for CanBookBeReserved()
This patch adds search_attrs to count_holds() since these get
used with CanBookBeReserved.

Test plan:
0. Apply the patch
1. prove t/db_dependent/Koha/Holds.t t/db_dependent/Reserves/HoldGroup.t t/db_dependent/Reserves.t

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2025-09-03 11:42:04 -06:00
David Cook 4864cd79cc
Bug 15516: (follow-up) Fix some merge bugs
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2025-09-03 11:42:02 -06:00
Aleisha Amohia 8bfe38b3ba
Bug 15516: Relevant controller changes and tests
Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2025-09-03 11:42:00 -06:00
Tomas Cohen Arazi 4ad2c94690
Bug 40671: Update method calls from revert_waiting to revert_found
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>
2025-08-29 10:43:26 -06:00
Tomas Cohen Arazi a137c7a183
Bug 40058: Remove RevertWaitingStatus()
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>
2025-08-14 16:05:59 -06:00
Tomas Cohen Arazi 1886165eae
Bug 40055: Make MoveReserve be passed objects instead of ids
The `MoveReserve` method is called once, in `AddIssue`. The latter has the related `Koha::Item` object already fetched from the DB, but it passed the `itemnumber` which is used by `MoveReserve` to fetch the item from the DB. This should not happen.

To test:
1. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Reserves* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Hold* \
           t/db_dependent/Circulation*
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2025-06-13 13:21:43 -06:00
Tomas Cohen Arazi 854783c836
Bug 40034: Only check notforloan on itemtype if it exists
This patch makes the code in `CheckReserves` survive the non-existence
of the referenced item type in the `items` table for the passed item.

To test:
1. Apply the regression tests
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Reserves.t
=> FAIL: Tests explode like this:

```
Can't call method "notforloan" on an undefined value at /kohadevbox/koha/C4/Reserves.pm line 870.
```

3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
2025-06-06 09:41:14 -06:00
Lisette Scheer e061d0a718
Bug 20747: Allow LocalHoldsPriority to fill by hierarchical groups system rather than individual library
This patch adds new options to the LocalHoldsPriority system preference to allow for the following options:
'None'                 the equivalent to 'Don't' pre-patch
'GiveLibrary'          the equivalent to 'Do' pre-patch
'GiveLibraryAndGroup'  Allows for checking the library first, then hold groups
'GiveGroup'            Allows to prioritize within a group without prioritizing any library first.

The included patches will update the preference in existing installations to maintain current behavior.

To Test:
BEFORE APPLYING THE PATCH
/Setup Groups and confirm current behavior/
1. Set the " LocalHoldsPriority " system preference to "Give", "LocalHoldsPriorityPatronControl" to "home library" and "LocalHoldsPriorityItemControl" to "home library"
2. Create 2 Library hold Groups
  - I called mine group 1 & group 2
3. Add 2 libraries each to each group.
  - Group 1
    - Centerville
    - Fairfield
  - Group 2
    - Fairview
    - Franklin
4. Find a patron for each branch in the groups
  - Centerville - 23529000035676 (Henry)
  - Fairfield - Changed 23529000050113
  - Fairview - Changed 23529001000463 (Edna)
  - Franklin - 	23529000121682
5. Find a record with at least 2 items
6. If needed change the home and holding libraries so you have at least one item for each hold group
  - Centerville - 39999000000252
  - Fairview - Changed 39999000000238
7. Place 2 record level holds for the record.
  - One for 23529000035676 for pickup at Centerville (Priority 1)
  - One for 23529001000463 for pickup at fairview (Priority 2)
8. Check in 39999000000238. The hold popup should be for the second hold, not the first.

/apply patch and confirm behavior still works/
9. Apply patch
10. Rebuild the database then restart_all (in sandboxes it's an action, in ktd " dbic && updatedatabase && restart_all ")
11. Confirm that " LocalHoldsPriority " is set to "Give Library"
12. Repeat steps 7-8, behavior should be the same.

/ check library then library group setting/
13. Change " LocalHoldsPriority " to "Give library then library group"
14. Set the following record level holds for the record:
    - Pickup at Fairfield -  23529000050113
    - Pickup at Franklin - 	23529000121682
    - Pickup at Centerville - 23529000035676 (Henry)
    - Pickup at Fairview -  23529001000463 (Edna)
15. Check in 39999000000252 - should try and fill to Centerville - 23529000035676 (Henry)
16. Check in 39999000000238 - should try and fill to Fairview -  23529001000463 (Edna)
17. Cancel the hold for 23529001000463
18. Check in 39999000000238 should try and fill to 23529000121682

/ check library group setting /
19. Reset the holds to match step 14.
20. Check in 39999000000252 - should try and fill to Fairfield
21. Check in 39999000000238 - should try and fill to Franklin

/check no priority /
22. Reset the holds to match step 14.
23. Check in 39999000000238 - Should fill the Fairfield hold
24. Check in 39999000000252 - Should fill the Franklin hold

/Also check/
Item level holds behave as expected
Different combinations of LocalHoldsPriorityItemControl and LocalHoldsPriorityPatronControl

Sponsored by: Cape Libraries Automated Materials Sharing
Sponsored by: Northeast Kansas Library System (NEKLS)

Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
2025-05-15 08:34:18 +02:00
Nick Clemens 1df229719c
Bug 38650: Only fill title level holds, or holds on the exact item
We change the conditions for filling holds to require that either:
- the hold is title level
- the hold is on the exact item

To test:
 1 - Place a hold for patron A on item #1 of a record
 2 - Place a hold for patron B on item #2 of a record
 3 - Issue item #2 to patron A - their hold on item #1 is filled incorrectly, check the item in
 4 - Apply patch, restart all
 5 - Place hold for patron A on item #1
 6 - Issue item #2 to patron A - their hold on item #1 is not filled, check the item in
 7 - Cancel that hold
 8 - Place a title level hold for the record for patron A
 9 - Check item #1 in at a different branch and confirm transfer
 9 - Issue item #2 to patron A
10 - The hold is filled as title level, even though item #1 was selected for the hold, check the item in
11 - Place an item level hold for item #2 for patron A
12 - Issue item #2 to patron A, hold is filled

Signed-off-by: Stephanie Petruso <spetruso@aacpl.net>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
2025-05-08 07:59:07 +02:00
Jonathan Druart 69b8341a8c
Bug 39325: Manual fixes
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>
2025-04-28 18:51:23 +02:00
Jonathan Druart 1d844a7d2f
Bug 39325: Automatic fix
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>
2025-04-28 18:51:22 +02:00
Koha Development Team d659526b5a
Bug 38664: Tidy the whole codebase
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>
2025-02-11 14:58:24 +01:00
Nick Clemens 723ed39c2a
Bug 38777: Use branch reply to for HOLD messages
When sending a notice of a hold ready for pickup (not digest) we use the library where the hold is at.

We should also use this when setting the reply to.

To test:
1 - Set the email/replyto for a patron's library to be:
    patron@example.com
2 - Set the email/replyto for another branch to be:
    holdbranch@example.com
3 - Place a hold for the patron for pickup at the other branch
4 - Check in and set hold waiting
5 - SELECT * FROM message_queue
6 - Note the message is from the hold branch, but has a null replyto
    This means we will use patron branch when sending
7 - Apply patch
8 - Revert hold waiting status
9 - Checkin and confirm hold again
10  -Check message queue
11 - Hold now has reply address of branch

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
2025-01-07 15:47:52 +01:00