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: Lucas Gass <lucas@bywatersolutions.com>
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: Lucas Gass <lucas@bywatersolutions.com>
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>
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>
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>
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>
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.
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.
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.
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.
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>
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
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
To test:
1 - place, capture, and revert a hold
2 - view action logs for your hold via SQL so you can see the diff column
3 - confirm reversion of waiting hold generates a MODIFY line in logs with no diff
4 - apply patch, restart_all
5 - capture and revert your hold again
6 - confirm logs include diff showing change to found and priority
Signed-off-by: Sam Sowanick <sam.sowanick@corvallisoregon.gov>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
We can easily replace the calls by ->suspend_hold and ->resume.
Test plan:
No behaviour changes expected.
Confirm that you can still suspend and resume holds (OPAC+staff)
Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This subroutine can easily be replaced and is not really needed.
Test plan:
No changes expected, try to suspend/resume holds from the OPAC
Note that you cannot affect somebody's else holds.
Note for QA: The extra fetch of Koha::Hold will be removed on bug 37868.
Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This subroutine can easily be replaced with
$item->holds->filter_by_found->count \o/
Test plan:
Confirm that the old sub and $item->holds->filter_by_found->count
produce the same query
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Test plan:
Run `perl -c Koha/Old/Hold.pm`
It should print 'syntax OK'
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This removes the unblessing of the patron object and uses fields form the patron
To test:
Confirm tests still pass:
prove -v t/db_dependent/Holds.t t/db_dependent/Circulation.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/Reserves.t t/db_dependent/api/v1/holds.t
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This routine currently takes the agerestriction value from biblioitems and an unblessed borrower object
and uses the date of birth to calculate whether the ptrons DOB is before or after the minimum value required
against the age restriction
We have a routine in the patron object to get the patron's age - we cna use this against the parsed agerestriction
value in a simple comparison and remove the need to unbless and pass the patron.
FIXME: We should move this to a biblioitems or biblio object method
To test:
0 - In Admin -> Koha to MARC mapping, set biblioitems.agerestriction to 521,a
1 - Set syspref AgeRestrictionMarker to 'Age'
2 - Edit a record and set 521$a to 'Age 14'
3 - Add an item or copy the barcode of the item on that record
4 - Attempt to checkout item to Lisa Charles in sample data, or a 15 year old patron
5 - It should checkout fine
6 - Check in item
7 - Edit patron Joyce Gaines to set age to 13 DOB:06/20/2011, or create a 13 year old patron
8 - Attempt to checkout item
9 - Item is blocked
10 - Apply patch
11 - Repeat tests, confirm no change
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Martin Renvoize
Lucas Gass
Pedro Amorim
Nick Clemens
Emily Lamancusa
Johanna Räisä
Baptiste
Marcel de Rooy
Jonathan Druart
Tomas Cohen Arazi
David Cook
Aleisha Amohia
Lisette Scheer
Koha Development Team
Andrew Fuerste Henry
Julian Maurice