Bugzilla – Attachment 186715 Details for
Bug 3492
Move hold fee setting into circulation rules
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 3492: Modernize hold fee system and migrate to circulation rules
Bug-3492-Modernize-hold-fee-system-and-migrate-to-.patch (text/plain), 63.96 KB, created by
Tomás Cohen Arazi (tcohen)
on 2025-09-22 14:12:44 UTC
(
hide
)
Description:
Bug 3492: Modernize hold fee system and migrate to circulation rules
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2025-09-22 14:12:44 UTC
Size:
63.96 KB
patch
obsolete
>From a0b9541902e4529253d598cd9d56bcc0c588b5ec Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Mon, 8 Sep 2025 18:48:31 +0100 >Subject: [PATCH] Bug 3492: Modernize hold fee system and migrate to > circulation rules >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >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> >--- > C4/Reserves.pm | 86 +--- > Koha/Hold.pm | 225 ++++++++++- > Koha/Item.pm | 33 ++ > installer/data/mysql/atomicupdate/bug_3492.pl | 23 +- > .../admin/preferences/circulation.pref | 7 + > .../bootstrap/en/modules/opac-reserve.tt | 68 +++- > opac/opac-reserve.pl | 43 +- > t/db_dependent/Koha/Hold.t | 379 +++++++++++++++++- > t/db_dependent/Koha/Item.t | 62 ++- > t/db_dependent/Reserves.t | 35 +- > t/db_dependent/Reserves/GetReserveFee.t | 281 ------------- > 11 files changed, 816 insertions(+), 426 deletions(-) > mode change 100644 => 100755 installer/data/mysql/atomicupdate/bug_3492.pl > delete mode 100755 t/db_dependent/Reserves/GetReserveFee.t > >diff --git a/C4/Reserves.pm b/C4/Reserves.pm >index 84957f1179..6d56367c02 100644 >--- a/C4/Reserves.pm >+++ b/C4/Reserves.pm >@@ -106,9 +106,6 @@ BEGIN { > > GetReserveStatus > >- ChargeReserveFee >- GetReserveFee >- > ModReserveAffect > ModReserve > ModReserveStatus >@@ -269,9 +266,8 @@ sub AddReserve { > my $reserve_id = $hold->id(); > > # add a reserve fee if needed >- if ( C4::Context->preference('HoldFeeMode') ne 'any_time_is_collected' ) { >- my $reserve_fee = GetReserveFee( $borrowernumber, $biblionumber ); >- ChargeReserveFee( $borrowernumber, $reserve_fee, $title ); >+ if ( $hold->should_charge('placement') ) { >+ $hold->charge_hold_fee(); > } > > _FixPriority( { biblionumber => $biblionumber } ); >@@ -706,84 +702,6 @@ sub CanItemBeReserved { > return _cache { status => 'OK' }; > } > >-=head2 ChargeReserveFee >- >- $fee = ChargeReserveFee( $borrowernumber, $fee, $title ); >- >- Charge the fee for a reserve (if $fee > 0) >- >-=cut >- >-sub ChargeReserveFee { >- my ( $borrowernumber, $fee, $title ) = @_; >- return if !$fee || $fee == 0; # the last test is needed to include 0.00 >- Koha::Account->new( { patron_id => $borrowernumber } )->add_debit( >- { >- amount => $fee, >- description => $title, >- note => undef, >- user_id => C4::Context->userenv ? C4::Context->userenv->{'number'} : undef, >- library_id => C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef, >- interface => C4::Context->interface, >- invoice_type => undef, >- type => 'RESERVE', >- item_id => undef >- } >- ); >-} >- >-=head2 GetReserveFee >- >- $fee = GetReserveFee( $borrowernumber, $biblionumber ); >- >- Calculate the fee for a reserve (if applicable). >- >-=cut >- >-sub GetReserveFee { >- my ( $borrowernumber, $biblionumber ) = @_; >- my $borquery = qq{ >-SELECT reservefee FROM borrowers LEFT JOIN categories ON borrowers.categorycode = categories.categorycode WHERE borrowernumber = ? >- }; >- my $issue_qry = qq{ >-SELECT COUNT(*) FROM items >-LEFT JOIN issues USING (itemnumber) >-WHERE items.biblionumber=? AND issues.issue_id IS NULL >- }; >- my $holds_qry = qq{ >-SELECT COUNT(*) FROM reserves WHERE biblionumber=? AND borrowernumber<>? >- }; >- >- my $dbh = C4::Context->dbh; >- my ($fee) = $dbh->selectrow_array( $borquery, undef, ($borrowernumber) ); >- $fee += 0; >- my $hold_fee_mode = C4::Context->preference('HoldFeeMode') || 'not_always'; >- if ( $fee and $fee > 0 and $hold_fee_mode eq 'not_always' ) { >- >- # This is a reconstruction of the old code: >- # Compare number of items with items issued, and optionally check holds >- # If not all items are issued and there are no holds: charge no fee >- # NOTE: Lost, damaged, not-for-loan, etc. are just ignored here >- my ( $notissued, $reserved ); >- ($notissued) = $dbh->selectrow_array( >- $issue_qry, undef, >- ($biblionumber) >- ); >- if ( $notissued == 0 ) { >- >- # all items are issued >- ($reserved) = $dbh->selectrow_array( >- $holds_qry, undef, >- ( $biblionumber, $borrowernumber ) >- ); >- $fee = 0 if $reserved == 0; >- } else { >- $fee = 0; >- } >- } >- return $fee; >-} >- > =head2 GetReserveStatus > > $reservestatus = GetReserveStatus($itemnumber); >diff --git a/Koha/Hold.pm b/Koha/Hold.pm >index 9dd9e6602e..801d052fe8 100644 >--- a/Koha/Hold.pm >+++ b/Koha/Hold.pm >@@ -933,21 +933,8 @@ sub fill { > # now fix the priority on the others.... > C4::Reserves::_FixPriority( { biblionumber => $self->biblionumber } ); > >- if ( C4::Context->preference('HoldFeeMode') eq 'any_time_is_collected' ) { >- my $fee = $patron->category->reservefee // 0; >- if ( $fee > 0 ) { >- $patron->account->add_debit( >- { >- amount => $fee, >- description => $self->biblio->title, >- user_id => C4::Context->userenv ? C4::Context->userenv->{'number'} : undef, >- library_id => C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef, >- interface => C4::Context->interface, >- type => 'RESERVE', >- item_id => $self->itemnumber >- } >- ); >- } >+ if ( $self->should_charge('collection') ) { >+ $self->charge_hold_fee(); > } > > C4::Log::logaction( 'HOLDS', 'FILL', $self->id, $self, undef, $original ) >@@ -1151,6 +1138,214 @@ sub hold_group { > return; > } > >+=head3 calculate_hold_fee >+ >+ my $fee = $hold->calculate_hold_fee(); >+ >+Calculate the hold fee for this hold using circulation rules. >+Returns the fee amount as a decimal. >+ >+=cut >+ >+sub calculate_hold_fee { >+ my ($self) = @_; >+ >+ my $item = $self->item; >+ >+ if ($item) { >+ >+ # Item-level hold - straightforward fee calculation >+ return $item->holds_fee( $self->patron ); >+ } else { >+ >+ # Title-level hold - use strategy to determine fee >+ return $self->_calculate_title_hold_fee(); >+ } >+} >+ >+=head3 should_charge >+ >+ my $should_charge = $hold->should_charge($stage); >+ >+Returns true if the hold fee should be charged at the given stage >+based on HoldFeeMode preference and current hold state. >+ >+Stage can be: >+- 'placement': When the hold is first placed >+- 'collection': When the hold is filled/collected >+ >+=cut >+ >+sub should_charge { >+ my ( $self, $stage ) = @_; >+ >+ return 0 unless $stage; >+ return 0 unless $stage =~ /^(placement|collection)$/; >+ >+ my $mode = C4::Context->preference('HoldFeeMode') || 'not_always'; >+ >+ if ( $stage eq 'placement' ) { >+ return 0 if $mode eq 'any_time_is_collected'; # Don't charge at placement >+ return 1 if $mode eq 'any_time_is_placed'; # Always charge at placement >+ >+ # 'not_always' mode - check conditions at placement time >+ return $self->_should_charge_not_always_mode(); >+ } elsif ( $stage eq 'collection' ) { >+ return 1 if $mode eq 'any_time_is_collected'; # Charge at collection >+ return 0 if $mode eq 'any_time_is_placed'; # Already charged at placement >+ >+ # 'not_always' mode - no additional fee at collection >+ return 0; >+ } >+ >+ return 0; >+} >+ >+=head3 charge_hold_fee >+ >+ $hold->charge_hold_fee({ amount => $fee }); >+ >+Charge the patron for the hold fee. >+ >+=cut >+ >+sub charge_hold_fee { >+ my ( $self, $params ) = @_; >+ >+ my $amount = $params->{amount} // $self->calculate_hold_fee(); >+ return unless $amount && $amount > 0; >+ >+ my $line = $self->patron->account->add_debit( >+ { >+ amount => $amount, >+ description => $self->biblio->title, >+ type => 'RESERVE', >+ item_id => $self->itemnumber, >+ user_id => C4::Context->userenv ? C4::Context->userenv->{number} : undef, >+ library_id => C4::Context->userenv ? C4::Context->userenv->{branch} : undef, >+ interface => C4::Context->interface, >+ } >+ ); >+ >+ return $line; >+} >+ >+=head3 _calculate_title_hold_fee >+ >+ my $fee = $hold->_calculate_title_hold_fee(); >+ >+Calculate the hold fee for a title-level hold using the TitleHoldFeeStrategy >+system preference to determine which fee to charge when items have different fees. >+ >+=cut >+ >+sub _calculate_title_hold_fee { >+ my ($self) = @_; >+ >+ # Get all holdable items for this biblio and calculate their fees >+ my $biblio = $self->biblio; >+ my @holdable_items = $biblio->items->search( >+ { >+ -or => [ >+ { 'me.notforloan' => 0 }, >+ { 'me.notforloan' => undef } >+ ] >+ } >+ )->as_list; >+ >+ my @fees; >+ foreach my $item (@holdable_items) { >+ >+ # Check if item is holdable for this patron >+ next unless C4::Reserves::CanItemBeReserved( $self->patron, $item )->{status} eq 'OK'; >+ >+ my $fee = $item->holds_fee( $self->patron ); >+ push @fees, $fee; >+ } >+ >+ return 0 unless @fees; >+ >+ # Apply the strategy from system preference >+ my $strategy = C4::Context->preference('TitleHoldFeeStrategy') || 'highest'; >+ >+ if ( $strategy eq 'highest' ) { >+ return ( sort { $b <=> $a } @fees )[0]; # Maximum fee >+ } elsif ( $strategy eq 'lowest' ) { >+ return ( sort { $a <=> $b } @fees )[0]; # Minimum fee >+ } elsif ( $strategy eq 'most_common' ) { >+ return $self->_get_most_common_fee(@fees); >+ } else { >+ >+ # Default to highest if unknown strategy >+ return ( sort { $b <=> $a } @fees )[0]; >+ } >+} >+ >+=head3 _get_most_common_fee >+ >+Helper method to find the most frequently occurring fee in a list. >+ >+=cut >+ >+sub _get_most_common_fee { >+ my ( $self, @fees ) = @_; >+ >+ return 0 unless @fees; >+ >+ # Count frequency of each fee >+ my %fee_count; >+ for my $fee (@fees) { >+ $fee_count{$fee}++; >+ } >+ >+ # Sort by frequency (desc), then by value (desc) as tie breaker >+ my $most_common_fee = >+ ( sort { $fee_count{$b} <=> $fee_count{$a} || $b <=> $a } keys %fee_count )[0]; >+ >+ return $most_common_fee; >+} >+ >+=head3 _should_charge_not_always_mode >+ >+Helper method to implement the 'not_always' HoldFeeMode logic. >+Returns true if a fee should be charged in not_always mode. >+ >+=cut >+ >+sub _should_charge_not_always_mode { >+ my ($self) = @_; >+ >+ # First check if we have a calculated fee > 0 >+ my $potential_fee = $self->calculate_hold_fee(); >+ return 0 unless $potential_fee > 0; >+ >+ # Apply not_always logic: >+ # - If items are available (not issued) â No fee >+ # - If all items issued AND no other holds â No fee >+ # - If all items issued AND other holds exist â Charge fee >+ >+ # Count items that are not on loan (available) >+ my $biblio = $self->biblio; >+ my $available_items = $biblio->items->search( { onloan => undef } )->count; >+ >+ if ( $available_items > 0 ) { >+ >+ # Items are available, no fee needed >+ return 0; >+ } >+ >+ # All items are issued, check for other holds >+ my $other_holds = Koha::Holds->search( >+ { >+ biblionumber => $self->biblionumber, >+ borrowernumber => { '!=' => $self->borrowernumber } >+ } >+ )->count; >+ >+ # Charge fee only if there are other holds (patron joins a queue) >+ return $other_holds > 0 ? 1 : 0; >+} >+ > =head3 _move_to_old > > my $is_moved = $hold->_move_to_old; >diff --git a/Koha/Item.pm b/Koha/Item.pm >index c675d0638e..3ef39de008 100644 >--- a/Koha/Item.pm >+++ b/Koha/Item.pm >@@ -1261,6 +1261,39 @@ sub has_pending_hold { > return $self->_result->tmp_holdsqueue ? 1 : 0; > } > >+=head3 holds_fee >+ >+ my $fee = $item->holds_fee($patron); >+ >+Calculate the hold fee for placing a hold on this specific item >+for the given patron. Uses ReservesControlBranch preference to determine >+which library's rules apply. Returns the fee amount as a decimal. >+ >+=cut >+ >+sub holds_fee { >+ my ( $self, $patron, $params ) = @_; >+ >+ return 0 unless $patron; >+ >+ # Use ReservesControlBranch policy to determine fee calculation branch >+ my $control_branch = $patron->branchcode; # Default to patron's home library >+ if ( C4::Context->preference('ReservesControlBranch') eq 'ItemHomeLibrary' ) { >+ $control_branch = $self->homebranch; >+ } >+ >+ my $rule = Koha::CirculationRules->get_effective_rule( >+ { >+ branchcode => $control_branch, >+ categorycode => $patron->categorycode, >+ itemtype => $self->effective_itemtype, >+ rule_name => 'hold_fee', >+ } >+ ); >+ >+ return $rule ? $rule->rule_value : 0; >+} >+ > =head3 has_pending_recall { > > my $has_pending_recall >diff --git a/installer/data/mysql/atomicupdate/bug_3492.pl b/installer/data/mysql/atomicupdate/bug_3492.pl >old mode 100644 >new mode 100755 >index 57f3ee9a02..b732244d5c >--- a/installer/data/mysql/atomicupdate/bug_3492.pl >+++ b/installer/data/mysql/atomicupdate/bug_3492.pl >@@ -3,23 +3,12 @@ use Koha::Installer::Output qw(say_warning say_failure say_success say_info); > > return { > bug_number => "3492", >- description => "Migrate reservefee from categories to circulation rules and remove deprecated column", >+ description => "Migrate reservefee from categories to circulation rules", > up => sub { > my ($args) = @_; > my ( $dbh, $out ) = @$args{qw(dbh out)}; > >- # Check if the reservefee column still exists >- my $column_exists = $dbh->selectrow_array( >- q{ >- SELECT COUNT(*) >- FROM INFORMATION_SCHEMA.COLUMNS >- WHERE TABLE_SCHEMA = DATABASE() >- AND TABLE_NAME = 'categories' >- AND COLUMN_NAME = 'reservefee' >- } >- ); >- >- if ($column_exists) { >+ if ( column_exists( 'categories', 'reservefee' ) ) { > > # Check if we have any existing reservefees to migrate > my $existing_fees = $dbh->selectall_arrayref( >@@ -73,6 +62,14 @@ return { > say_info( $out, "The reservefee column has already been removed from the categories table." ); > } > >+ # Add the new system preference >+ $dbh->do( >+ q{ >+ INSERT IGNORE INTO systempreferences (variable, value, options, explanation, type) VALUES >+ ('TitleHoldFeeStrategy', 'highest', 'highest|lowest|most_common', 'Strategy for calculating fees on title-level holds when items have different fees: highest = charge maximum fee, lowest = charge minimum fee, most_common = charge most frequently occurring fee', 'Choice') >+ } >+ ); >+ say_success( $out, "Added TitleHoldFeeStrategy system preference" ); > say_info( $out, "Hold fees can now be configured in Administration > Circulation and fine rules." ); > say_success( $out, "Migration complete: Hold fees are now fully managed through circulation rules." ); > }, >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref >index 82070552d6..1cc80c052b 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref >@@ -1223,6 +1223,13 @@ Circulation: > any_time_is_placed: "any time a hold is placed." > not_always: "only if all items are checked out and the record has at least one hold already." > any_time_is_collected: "any time a hold is collected." >+ - >+ - "When a title-level hold is placed and different items could result in different hold fees, choose the fee level to apply as" >+ - pref: TitleHoldFeeStrategy >+ choices: >+ highest: "the highest applicable fee among the items." >+ lowest: "the lowest applicable fee among the items." >+ most_common: "the most common fee among the items." > - > - pref: useDefaultReplacementCost > choices: >diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt >index 363702f926..08630ccd46 100644 >--- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt >+++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt >@@ -145,12 +145,51 @@ > </div> > [% END %] > >- [% IF ( bibitemloo.reserve_charge ) %] >+ [% IF ( bibitemloo.reserve_charge || bibitemloo.item_hold_fees.size ) %] > <div class="alert" id="reserve_fee"> >- [% IF Koha.Preference('HoldFeeMode') == 'any_time_is_collected' %] >- <span>You will be charged a hold fee of [% bibitemloo.reserve_charge | $Price %] when you collect this item</span> >- [% ELSE %] >- <span>You will be charged a hold fee of [% bibitemloo.reserve_charge | $Price %] for placing this hold</span> >+ [% IF bibitemloo.item_hold_fees.size %] >+ [% SET fee_message_time = Koha.Preference('HoldFeeMode') == 'any_time_is_collected' ? 'when you collect' : 'for placing this hold' %] >+ [% SET unique_fees = {} %] >+ [% SET max_fee = 0 %] >+ [% FOREACH fee IN bibitemloo.item_hold_fees %] >+ [% SET unique_fees.${fee.fee} = fee.fee %] >+ [% IF fee.fee > max_fee %][% SET max_fee = fee.fee %][% END %] >+ [% END %] >+ [% SET fee_list = unique_fees.keys.sort('cmp_numeric') %] >+ >+ [% IF fee_list.size == 1 && fee_list.0 > 0 %] >+ <span>You will be charged a hold fee of [% fee_list.0 | $Price %] [% fee_message_time | html %]</span> >+ [% ELSIF fee_list.size > 1 %] >+ [% SET strategy = Koha.Preference('TitleHoldFeeStrategy') || 'highest' %] >+ [% IF strategy == 'highest' %] >+ <span>You will be charged a hold fee of [% max_fee | $Price %] [% fee_message_time | html %].</span> >+ [% ELSIF strategy == 'lowest' %] >+ [% SET min_fee = fee_list.sort('cmp_numeric').0 %] >+ <span>You will be charged a hold fee of [% min_fee | $Price %] [% fee_message_time | html %].</span> >+ [% ELSIF strategy == 'most_common' %] >+ [% SET fee_counts = {} %] >+ [% FOREACH fee IN fee_list %] >+ [% SET fee_counts.$fee = fee_counts.$fee + 1 || 1 %] >+ [% END %] >+ [% SET most_common_fee = fee_list.0 %] >+ [% SET max_count = 0 %] >+ [% FOREACH fee IN fee_counts.keys %] >+ [% IF fee_counts.$fee > max_count %] >+ [% SET max_count = fee_counts.$fee %] >+ [% SET most_common_fee = fee %] >+ [% END %] >+ [% END %] >+ <span>You will be charged a hold fee of [% most_common_fee | $Price %] [% fee_message_time | html %].</span> >+ [% ELSE %] >+ <span>You will be charged a hold fee of [% max_fee | $Price %] [% fee_message_time | html %].</span> >+ [% END %] >+ [% END %] >+ [% ELSIF bibitemloo.reserve_charge %] >+ [% IF Koha.Preference('HoldFeeMode') == 'any_time_is_collected' %] >+ <span>You will be charged a hold fee of [% bibitemloo.reserve_charge | $Price %] when you collect this item</span> >+ [% ELSE %] >+ <span>You will be charged a hold fee of [% bibitemloo.reserve_charge | $Price %] for placing this hold</span> >+ [% END %] > [% END %] > </div> > [% END %] >@@ -378,6 +417,9 @@ > [% END %] > <th>Notes</th> > <th>Information</th> >+ [% IF bibitemloo.item_hold_fees.size > 1 %] >+ <th>Hold fee</th> >+ [% END %] > </tr> > </thead> > <tbody> >@@ -494,6 +536,22 @@ > <span class="notonhold">Not on hold</span> > [% END # / IF ( itemLoo.first_hold ) %] > </td> >+ [% IF bibitemloo.item_hold_fees.size > 1 %] >+ <td class="hold_fee"> >+ [% SET item_fee = 0 %] >+ [% FOREACH fee IN bibitemloo.item_hold_fees %] >+ [% IF fee.itemnumber == itemLoo.itemnumber %] >+ [% SET item_fee = fee.fee %] >+ [% LAST %] >+ [% END %] >+ [% END %] >+ [% IF item_fee > 0 %] >+ [% item_fee | $Price %] >+ [% ELSE %] >+ <span>No fee</span> >+ [% END %] >+ </td> >+ [% END %] > </tr> > [% END # / FOREACH itemLoo IN bibitemloo.itemLoop %] > </tbody> >diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl >index 0ba22c25f3..a8fb645e29 100755 >--- a/opac/opac-reserve.pl >+++ b/opac/opac-reserve.pl >@@ -24,7 +24,7 @@ use CGI qw ( -utf8 ); > use C4::Auth qw( get_template_and_user ); > use C4::Koha qw( getitemtypeimagelocation getitemtypeimagesrc ); > use C4::Circulation qw( GetBranchItemRule ); >-use C4::Reserves qw( CanItemBeReserved CanBookBeReserved AddReserve IsAvailableForItemLevelRequest GetReserveFee ); >+use C4::Reserves qw( CanItemBeReserved CanBookBeReserved AddReserve IsAvailableForItemLevelRequest ); > use C4::Biblio qw( GetBiblioData GetFrameworkCode ); > use C4::Output qw( output_html_with_http_headers ); > use C4::Context; >@@ -584,8 +584,45 @@ foreach my $biblioNum (@biblionumbers) { > $biblioLoopIter{forced_hold_level} = $forced_hold_level; > } > >- # Pass through any reserve charge >- $biblioLoopIter{reserve_charge} = GetReserveFee( $patron->id, $biblioNum ); >+ # Pass through any reserve charge - get fees per item for better display >+ my $biblio_obj = Koha::Biblios->find($biblioNum); >+ my @item_fees; >+ >+ if ($biblio_obj) { >+ >+ # Get holdable items and calculate fees using object methods >+ my @holdable_items = $biblio_obj->items->search( >+ { >+ -or => [ >+ { 'me.notforloan' => 0 }, >+ { 'me.notforloan' => undef } >+ ] >+ } >+ )->as_list; >+ >+ foreach my $item (@holdable_items) { >+ >+ # Check if item is holdable for this patron >+ next unless C4::Reserves::CanItemBeReserved( $patron, $item )->{status} eq 'OK'; >+ >+ my $fee = $item->holds_fee($patron); >+ >+ push @item_fees, { >+ itemnumber => $item->id, >+ fee => $fee, >+ barcode => $item->barcode, >+ callnumber => $item->itemcallnumber, >+ location => $item->location, >+ }; >+ } >+ } >+ >+ if (@item_fees) { >+ $biblioLoopIter{reserve_charge} = $item_fees[0]->{fee}; # Legacy compatibility >+ $biblioLoopIter{item_hold_fees} = \@item_fees; # New array for templates >+ } else { >+ $biblioLoopIter{reserve_charge} = 0; >+ } > > push @$biblioLoop, \%biblioLoopIter; > >diff --git a/t/db_dependent/Koha/Hold.t b/t/db_dependent/Koha/Hold.t >index 060b592970..6fe4e08f8a 100755 >--- a/t/db_dependent/Koha/Hold.t >+++ b/t/db_dependent/Koha/Hold.t >@@ -20,7 +20,7 @@ > use Modern::Perl; > > use Test::NoWarnings; >-use Test::More tests => 18; >+use Test::More tests => 21; > > use Test::Exception; > use Test::MockModule; >@@ -32,6 +32,7 @@ use t::lib::Mocks; > use C4::Reserves qw(AddReserve); > > use Koha::ActionLogs; >+use Koha::CirculationRules; > use Koha::DateUtils qw(dt_from_string); > use Koha::Holds; > use Koha::Libraries; >@@ -115,19 +116,27 @@ subtest 'fill() tests' => sub { > > my $fee = 15; > >- my $category = $builder->build_object( >- { >- class => 'Koha::Patron::Categories', >- value => { reservefee => $fee } >- } >- ); >- my $patron = $builder->build_object( >+ my $category = $builder->build_object( { class => 'Koha::Patron::Categories' } ); >+ my $patron = $builder->build_object( > { > class => 'Koha::Patrons', > value => { categorycode => $category->id } > } > ); > my $manager = $builder->build_object( { class => 'Koha::Patrons' } ); >+ my $library = $builder->build_object( { class => 'Koha::Libraries' } ); >+ >+ # Set up circulation rules for hold fees >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => $category->id, >+ itemtype => undef, >+ rules => { >+ hold_fee => $fee, >+ } >+ } >+ ); > > my $title = 'Do what you want'; > my $biblio = $builder->build_sample_biblio( { title => $title } ); >@@ -139,6 +148,7 @@ subtest 'fill() tests' => sub { > biblionumber => $biblio->id, > borrowernumber => $patron->id, > itemnumber => $item->id, >+ branchcode => $library->branchcode, > timestamp => dt_from_string('2021-06-25 14:05:35'), > priority => 10, > } >@@ -167,12 +177,25 @@ subtest 'fill() tests' => sub { > > subtest 'item_id parameter' => sub { > plan tests => 1; >- $category->reservefee(0)->store; # do not disturb later accounts >+ >+ # Clear hold fee rule to not disturb later accounts >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => $category->id, >+ itemtype => undef, >+ rules => { >+ hold_fee => 0, >+ } >+ } >+ ); > $hold = $builder->build_object( > { > class => 'Koha::Holds', >- value => >- { biblionumber => $biblio->id, borrowernumber => $patron->id, itemnumber => undef, priority => 1 } >+ value => { >+ biblionumber => $biblio->id, borrowernumber => $patron->id, itemnumber => undef, priority => 1, >+ branchcode => $library->branchcode >+ } > } > ); > >@@ -181,7 +204,18 @@ subtest 'fill() tests' => sub { > $old_hold = Koha::Old::Holds->find( $hold->id ); > is( $old_hold->itemnumber, $item->itemnumber, 'The itemnumber has been saved in old_reserves by fill' ); > $old_hold->delete; >- $category->reservefee($fee)->store; # restore >+ >+ # Restore hold fee rule >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => $category->id, >+ itemtype => undef, >+ rules => { >+ hold_fee => $fee, >+ } >+ } >+ ); > }; > > subtest 'fee applied tests' => sub { >@@ -1847,3 +1881,324 @@ subtest 'move_hold() tests' => sub { > > $schema->storage->txn_rollback; > }; >+ >+subtest 'calculate_hold_fee() tests' => sub { >+ plan tests => 12; >+ >+ $schema->storage->txn_begin; >+ >+ # Create patron category and patron >+ my $cat = $builder->build( { source => 'Category', value => { categorycode => 'XYZ1' } } ); >+ my $patron1 = $builder->build_object( >+ { >+ class => 'Koha::Patrons', >+ value => { categorycode => 'XYZ1' } >+ } >+ ); >+ my $patron2 = $builder->build_object( >+ { >+ class => 'Koha::Patrons', >+ value => { categorycode => 'XYZ1' } >+ } >+ ); >+ >+ # Create itemtypes with different fees >+ my $itemtype1 = $builder->build( { source => 'Itemtype' } ); >+ my $itemtype2 = $builder->build( { source => 'Itemtype' } ); >+ my $itemtype3 = $builder->build( { source => 'Itemtype' } ); >+ >+ # Set up circulation rules with different hold fees for different itemtypes >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => 'XYZ1', >+ itemtype => $itemtype1->{itemtype}, >+ rules => { hold_fee => 1.00, reservesallowed => 10 } >+ } >+ ); >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => 'XYZ1', >+ itemtype => $itemtype2->{itemtype}, >+ rules => { hold_fee => 3.00, reservesallowed => 10 } >+ } >+ ); >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => 'XYZ1', >+ itemtype => $itemtype3->{itemtype}, >+ rules => { hold_fee => 2.00, reservesallowed => 10 } >+ } >+ ); >+ >+ # Set generic rules for permission checking >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => undef, >+ itemtype => undef, >+ rules => { reservesallowed => 10, holds_per_record => 10, holds_per_day => 10 } >+ } >+ ); >+ >+ my $biblio = $builder->build_sample_biblio(); >+ >+ # Create multiple items with different itemtypes and fees >+ my $item1 = $builder->build_sample_item( >+ { >+ biblionumber => $biblio->biblionumber, >+ itype => $itemtype1->{itemtype}, >+ notforloan => 0, >+ } >+ ); >+ my $item2 = $builder->build_sample_item( >+ { >+ biblionumber => $biblio->biblionumber, >+ itype => $itemtype2->{itemtype}, >+ notforloan => 0, >+ } >+ ); >+ my $item3 = $builder->build_sample_item( >+ { >+ biblionumber => $biblio->biblionumber, >+ itype => $itemtype3->{itemtype}, >+ notforloan => 0, >+ } >+ ); >+ >+ # Test 1: Item-level hold calculates fee correctly >+ my $item_hold = $builder->build_object( >+ { >+ class => 'Koha::Holds', >+ value => { >+ borrowernumber => $patron1->borrowernumber, >+ biblionumber => $biblio->biblionumber, >+ itemnumber => $item2->itemnumber, # Use item2 with 3.00 fee >+ } >+ } >+ ); >+ >+ my $fee = $item_hold->calculate_hold_fee(); >+ is( $fee, 3.00, 'Item-level hold calculates fee correctly' ); >+ >+ # Create title-level hold for comprehensive testing >+ my $title_hold = $builder->build_object( >+ { >+ class => 'Koha::Holds', >+ value => { >+ borrowernumber => $patron2->borrowernumber, >+ biblionumber => $biblio->biblionumber, >+ itemnumber => undef, >+ } >+ } >+ ); >+ >+ # Test 2: Default 'highest' strategy should return 3.00 (highest fee) >+ t::lib::Mocks::mock_preference( 'TitleHoldFeeStrategy', 'highest' ); >+ $fee = $title_hold->calculate_hold_fee(); >+ is( $fee, 3.00, 'Title-level hold: highest strategy returns highest fee (3.00)' ); >+ >+ # Test 3: 'lowest' strategy should return 1.00 (lowest fee) >+ t::lib::Mocks::mock_preference( 'TitleHoldFeeStrategy', 'lowest' ); >+ $fee = $title_hold->calculate_hold_fee(); >+ is( $fee, 1.00, 'Title-level hold: lowest strategy returns lowest fee (1.00)' ); >+ >+ # Test 4: 'most_common' strategy with unique fees should return highest >+ t::lib::Mocks::mock_preference( 'TitleHoldFeeStrategy', 'most_common' ); >+ $fee = $title_hold->calculate_hold_fee(); >+ is( $fee, 3.00, 'Title-level hold: most_common strategy with unique fees returns highest fee (3.00)' ); >+ >+ # Test 5: Add another item with same fee to test most_common properly >+ my $item4 = $builder->build_sample_item( >+ { >+ biblionumber => $biblio->biblionumber, >+ itype => $itemtype1->{itemtype}, >+ notforloan => 0, >+ } >+ ); >+ $fee = $title_hold->calculate_hold_fee(); >+ is( $fee, 1.00, 'Title-level hold: most_common strategy returns most common fee (1.00 - 2 items)' ); >+ >+ # Test 6: Unknown/invalid strategy defaults to 'highest' >+ t::lib::Mocks::mock_preference( 'TitleHoldFeeStrategy', 'invalid_strategy' ); >+ $fee = $title_hold->calculate_hold_fee(); >+ is( $fee, 3.00, 'Title-level hold: invalid strategy defaults to highest fee (3.00)' ); >+ >+ # Test 7: Empty preference defaults to 'highest' >+ t::lib::Mocks::mock_preference( 'TitleHoldFeeStrategy', '' ); >+ $fee = $title_hold->calculate_hold_fee(); >+ is( $fee, 3.00, 'Title-level hold: empty strategy preference defaults to highest fee (3.00)' ); >+ >+ # Test 8: No holdable items returns 0 >+ # Make all items not holdable >+ $item1->notforloan(1)->store; >+ $item2->notforloan(1)->store; >+ $item3->notforloan(1)->store; >+ $item4->notforloan(1)->store; >+ >+ $fee = $title_hold->calculate_hold_fee(); >+ is( $fee, 0, 'Title-level hold: no holdable items returns 0 fee' ); >+ >+ # Test 9: Mix of holdable and non-holdable items (highest) >+ # Make some items holdable again >+ $item2->notforloan(0)->store; # Fee: 3.00 >+ $item3->notforloan(0)->store; # Fee: 2.00 >+ >+ t::lib::Mocks::mock_preference( 'TitleHoldFeeStrategy', 'highest' ); >+ $fee = $title_hold->calculate_hold_fee(); >+ is( $fee, 3.00, 'Title-level hold: highest strategy with mixed holdable items returns 3.00' ); >+ >+ # Test 10: Mix of holdable and non-holdable items (lowest) >+ t::lib::Mocks::mock_preference( 'TitleHoldFeeStrategy', 'lowest' ); >+ $fee = $title_hold->calculate_hold_fee(); >+ is( $fee, 2.00, 'Title-level hold: lowest strategy with mixed holdable items returns 2.00' ); >+ >+ # Test 11: Items with 0 fee >+ my $itemtype_free = $builder->build( { source => 'Itemtype' } ); >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => 'XYZ1', >+ itemtype => $itemtype_free->{itemtype}, >+ rules => { hold_fee => 0, reservesallowed => 10 } >+ } >+ ); >+ >+ my $item_free = $builder->build_sample_item( >+ { >+ biblionumber => $biblio->biblionumber, >+ itype => $itemtype_free->{itemtype}, >+ notforloan => 0, >+ } >+ ); >+ >+ t::lib::Mocks::mock_preference( 'TitleHoldFeeStrategy', 'lowest' ); >+ $fee = $title_hold->calculate_hold_fee(); >+ is( $fee, 0, 'Title-level hold: lowest strategy with free item returns 0' ); >+ >+ # Test 12: All items have same fee >+ # Set all holdable items to same fee >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => 'XYZ1', >+ itemtype => $itemtype2->{itemtype}, >+ rules => { hold_fee => 2.50, reservesallowed => 10 } >+ } >+ ); >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => 'XYZ1', >+ itemtype => $itemtype3->{itemtype}, >+ rules => { hold_fee => 2.50, reservesallowed => 10 } >+ } >+ ); >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => 'XYZ1', >+ itemtype => $itemtype_free->{itemtype}, >+ rules => { hold_fee => 2.50, reservesallowed => 10 } >+ } >+ ); >+ >+ $fee = $title_hold->calculate_hold_fee(); >+ is( $fee, 2.50, 'Title-level hold: all items with same fee returns that fee regardless of strategy' ); >+ >+ $schema->storage->txn_rollback; >+}; >+ >+subtest 'should_charge() tests' => sub { >+ plan tests => 6; >+ >+ $schema->storage->txn_begin; >+ >+ my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); >+ my $biblio = $builder->build_sample_biblio(); >+ my $item = $builder->build_sample_item( { biblionumber => $biblio->biblionumber } ); >+ >+ my $hold = $builder->build_object( >+ { >+ class => 'Koha::Holds', >+ value => { >+ borrowernumber => $patron->borrowernumber, >+ biblionumber => $biblio->biblionumber, >+ itemnumber => $item->itemnumber, >+ } >+ } >+ ); >+ >+ # Test any_time_is_placed mode >+ t::lib::Mocks::mock_preference( 'HoldFeeMode', 'any_time_is_placed' ); >+ ok( $hold->should_charge('placement'), 'any_time_is_placed charges at placement' ); >+ ok( !$hold->should_charge('collection'), 'any_time_is_placed does not charge at collection' ); >+ >+ # Test any_time_is_collected mode >+ t::lib::Mocks::mock_preference( 'HoldFeeMode', 'any_time_is_collected' ); >+ ok( !$hold->should_charge('placement'), 'any_time_is_collected does not charge at placement' ); >+ ok( $hold->should_charge('collection'), 'any_time_is_collected charges at collection' ); >+ >+ # Test not_always mode (basic case - items available) >+ t::lib::Mocks::mock_preference( 'HoldFeeMode', 'not_always' ); >+ ok( !$hold->should_charge('placement'), 'not_always does not charge at placement when items available' ); >+ ok( !$hold->should_charge('collection'), 'not_always does not charge at collection' ); >+ >+ $schema->storage->txn_rollback; >+}; >+ >+subtest 'charge_hold_fee() tests' => sub { >+ plan tests => 2; >+ >+ $schema->storage->txn_begin; >+ >+ my $cat = $builder->build( { source => 'Category', value => { categorycode => 'XYZ1' } } ); >+ >+ # Set up circulation rules for hold fees >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => 'XYZ1', >+ itemtype => undef, >+ rules => { >+ hold_fee => 2.00, >+ } >+ } >+ ); >+ >+ my $patron = $builder->build_object( >+ { >+ class => 'Koha::Patrons', >+ value => { categorycode => 'XYZ1' } >+ } >+ ); >+ t::lib::Mocks::mock_userenv( { patron => $patron, branchcode => $patron->branchcode } ); >+ >+ my $biblio = $builder->build_sample_biblio(); >+ my $item = $builder->build_sample_item( { biblionumber => $biblio->biblionumber } ); >+ >+ my $hold = $builder->build_object( >+ { >+ class => 'Koha::Holds', >+ value => { >+ borrowernumber => $patron->borrowernumber, >+ biblionumber => $biblio->biblionumber, >+ itemnumber => $item->itemnumber, >+ } >+ } >+ ); >+ >+ my $account = $patron->account; >+ my $balance_before = $account->balance; >+ >+ # Charge fee >+ my $account_line = $hold->charge_hold_fee(); >+ is( $account_line->amount, 2.00, 'charge_hold_fee returns account line with correct amount' ); >+ >+ my $balance_after = $account->balance; >+ is( $balance_after, $balance_before + 2.00, 'Patron account charged correctly' ); >+ >+ $schema->storage->txn_rollback; >+}; >diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t >index d0b0a16dfb..b86f9609a0 100755 >--- a/t/db_dependent/Koha/Item.t >+++ b/t/db_dependent/Koha/Item.t >@@ -21,7 +21,7 @@ use Modern::Perl; > use utf8; > > use Test::NoWarnings; >-use Test::More tests => 41; >+use Test::More tests => 42; > use Test::Exception; > use Test::MockModule; > use Test::Warn; >@@ -3875,3 +3875,63 @@ subtest 'effective_bookable() tests' => sub { > > $schema->storage->txn_rollback; > }; >+ >+subtest 'holds_fee() tests' => sub { >+ plan tests => 3; >+ >+ $schema->storage->txn_begin; >+ >+ my $cat1 = $builder->build( { source => 'Category', value => { categorycode => 'XYZ1' } } ); >+ my $cat2 = $builder->build( { source => 'Category', value => { categorycode => 'XYZ2' } } ); >+ >+ # Set up circulation rules for hold fees >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => 'XYZ1', >+ itemtype => undef, >+ rules => { >+ hold_fee => 2.00, >+ } >+ } >+ ); >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => 'XYZ2', >+ itemtype => undef, >+ rules => { >+ hold_fee => 0, >+ } >+ } >+ ); >+ >+ my $patron1 = $builder->build_object( >+ { >+ class => 'Koha::Patrons', >+ value => { categorycode => 'XYZ1' } >+ } >+ ); >+ my $patron2 = $builder->build_object( >+ { >+ class => 'Koha::Patrons', >+ value => { categorycode => 'XYZ2' } >+ } >+ ); >+ >+ my $item = $builder->build_sample_item(); >+ >+ # Test with fee rule >+ my $fee = $item->holds_fee($patron1); >+ is( $fee, 2.00, 'Item holds_fee returns correct fee from circulation rule' ); >+ >+ # Test with no fee rule >+ $fee = $item->holds_fee($patron2); >+ is( $fee, 0, 'Item holds_fee returns 0 when no fee configured' ); >+ >+ # Test without patron >+ $fee = $item->holds_fee(undef); >+ is( $fee, 0, 'Item holds_fee returns 0 when no patron provided' ); >+ >+ $schema->storage->txn_rollback; >+}; >diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t >index 79d6796f82..ab3f461c84 100755 >--- a/t/db_dependent/Reserves.t >+++ b/t/db_dependent/Reserves.t >@@ -34,7 +34,7 @@ use C4::Biblio qw( GetMarcFromKohaField ModBiblio ); > use C4::HoldsQueue; > use C4::Members; > use C4::Reserves >- qw( AddReserve AlterPriority CheckReserves ModReserve ModReserveAffect ReserveSlip CalculatePriority CanBookBeReserved IsAvailableForItemLevelRequest MoveReserve ChargeReserveFee CanItemBeReserved MergeHolds ); >+ qw( AddReserve AlterPriority CheckReserves ModReserve ModReserveAffect ReserveSlip CalculatePriority CanBookBeReserved IsAvailableForItemLevelRequest MoveReserve CanItemBeReserved MergeHolds ); > use Koha::ActionLogs; > use Koha::Biblios; > use Koha::Caches; >@@ -999,29 +999,40 @@ subtest 'ReservesNeedReturns' => sub { > t::lib::Mocks::mock_preference( 'ReservesNeedReturns', 1 ); # Don't affect other tests > }; > >-subtest 'ChargeReserveFee tests' => sub { >+subtest 'Hold fee charging tests' => sub { > > plan tests => 8; > > my $library = $builder->build_object( { class => 'Koha::Libraries' } ); > my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); >- >- my $fee = 20; >- my $title = 'A title'; >+ my $biblio = $builder->build_sample_biblio(); >+ my $item = $builder->build_sample_item( { biblionumber => $biblio->biblionumber } ); > > my $context = Test::MockModule->new('C4::Context'); > $context->mock( userenv => { branch => $library->id } ); > >- my $line = C4::Reserves::ChargeReserveFee( $patron->id, $fee, $title ); >+ my $hold = $builder->build_object( >+ { >+ class => 'Koha::Holds', >+ value => { >+ borrowernumber => $patron->id, >+ biblionumber => $biblio->biblionumber, >+ itemnumber => $item->itemnumber, >+ } >+ } >+ ); >+ >+ my $fee = 20; >+ my $line = $hold->charge_hold_fee( { amount => $fee } ); > > is( ref($line), 'Koha::Account::Line', 'Returns a Koha::Account::Line object' ); > ok( $line->is_debit, 'Generates a debit line' ); >- is( $line->debit_type_code, 'RESERVE', 'generates RESERVE debit_type' ); >- is( $line->borrowernumber, $patron->id, 'generated line belongs to the passed patron' ); >- is( $line->amount, $fee, 'amount set correctly' ); >- is( $line->amountoutstanding, $fee, 'amountoutstanding set correctly' ); >- is( $line->description, "$title", 'description is title of reserved item' ); >- is( $line->branchcode, $library->id, "Library id is picked from userenv and stored correctly" ); >+ is( $line->debit_type_code, 'RESERVE', 'generates RESERVE debit_type' ); >+ is( $line->borrowernumber, $patron->id, 'generated line belongs to the passed patron' ); >+ is( $line->amount, $fee, 'amount set correctly' ); >+ is( $line->amountoutstanding, $fee, 'amountoutstanding set correctly' ); >+ is( $line->description, $biblio->title, 'description is title of reserved item' ); >+ is( $line->branchcode, $library->id, "Library id is picked from userenv and stored correctly" ); > }; > > subtest 'MoveReserve additional test' => sub { >diff --git a/t/db_dependent/Reserves/GetReserveFee.t b/t/db_dependent/Reserves/GetReserveFee.t >deleted file mode 100755 >index 5bd241d900..0000000000 >--- a/t/db_dependent/Reserves/GetReserveFee.t >+++ /dev/null >@@ -1,281 +0,0 @@ >-#!/usr/bin/perl >- >-# This script includes tests for GetReserveFee and ChargeReserveFee >- >-# Copyright 2015 Rijksmuseum >-# >-# This file is part of Koha. >-# >-# Koha is free software; you can redistribute it and/or modify it >-# under the terms of the GNU General Public License as published by >-# the Free Software Foundation; either version 3 of the License, or >-# (at your option) any later version. >-# >-# Koha is distributed in the hope that it will be useful, but >-# WITHOUT ANY WARRANTY; without even the implied warranty of >-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >-# GNU General Public License for more details. >-# >-# You should have received a copy of the GNU General Public License >-# along with Koha; if not, see <http://www.gnu.org/licenses>. >- >-use Modern::Perl; >- >-use Test::NoWarnings; >-use Test::More tests => 4; >-use Test::MockModule; >-use t::lib::TestBuilder; >-use t::lib::Mocks; >- >-use C4::Circulation qw( AddIssue ); >-use C4::Reserves qw( GetReserveFee ChargeReserveFee AddReserve ); >-use Koha::Database; >- >-my $schema = Koha::Database->new->schema; >-$schema->storage->txn_begin; >- >-my $builder = t::lib::TestBuilder->new(); >-my $library = $builder->build( >- { >- source => 'Branch', >- } >-); >-my $mContext = Test::MockModule->new('C4::Context'); >-$mContext->mock( >- 'userenv', >- sub { >- return { branch => $library->{branchcode} }; >- } >-); >- >-my $dbh = C4::Context->dbh; # after start transaction of testbuilder >- >-# Category with hold fee, two patrons >-$builder->build( >- { >- source => 'Category', >- value => { >- categorycode => 'XYZ1', >- reservefee => 2, >- }, >- } >-); >-$builder->build( >- { >- source => 'Category', >- value => { >- categorycode => 'XYZ2', >- reservefee => 0, >- }, >- } >-); >-my $patron1 = $builder->build_object( >- { >- class => 'Koha::Patrons', >- value => { >- categorycode => 'XYZ1', >- }, >- } >-); >-my $patron2 = $builder->build_object( >- { >- class => 'Koha::Patrons', >- value => { >- categorycode => 'XYZ1', >- }, >- } >-); >-my $patron3 = $builder->build_object( >- { >- class => 'Koha::Patrons', >- } >-); >-my $patron4 = $builder->build_object( >- { >- class => 'Koha::Patrons', >- value => { >- categorycode => 'XYZ2', >- }, >- } >-); >- >-# One biblio and two items >-my $biblio = $builder->build_sample_biblio; >-my $item1 = $builder->build_sample_item( >- { >- biblionumber => $biblio->biblionumber, >- notforloan => 0, >- } >-); >-my $item2 = $builder->build_sample_item( >- { >- biblionumber => $biblio->biblionumber, >- notforloan => 0, >- } >-); >- >-subtest 'GetReserveFee' => sub { >- plan tests => 6; >- >- C4::Circulation::AddIssue( $patron1, $item1->barcode, '2015-12-31', 0, undef, 0, {} ) >- ; # the date does not really matter >- C4::Circulation::AddIssue( $patron3, $item2->barcode, '2015-12-31', 0, undef, 0, {} ) >- ; # the date does not really matter >- my $acc2 = acctlines( $patron2->borrowernumber ); >- my $res1 = addreserve( $patron1->borrowernumber ); >- >- t::lib::Mocks::mock_preference( 'HoldFeeMode', 'not_always' ); >- my $fee = C4::Reserves::GetReserveFee( $patron2->borrowernumber, $biblio->biblionumber ); >- is( $fee > 0, 1, 'Patron 2 should be charged cf GetReserveFee' ); >- C4::Reserves::ChargeReserveFee( $patron2->borrowernumber, $fee, $biblio->title ); >- is( acctlines( $patron2->borrowernumber ), $acc2 + 1, 'Patron 2 has been charged by ChargeReserveFee' ); >- >- # If we delete the reserve, there should be no charge >- $dbh->do( "DELETE FROM reserves WHERE borrowernumber = ?", undef, ( $patron1->borrowernumber ) ); >- $fee = C4::Reserves::GetReserveFee( $patron2->borrowernumber, $biblio->biblionumber ); >- is( $fee, 0, 'HoldFeeMode=not_always, Patron 2 should not be charged' ); >- >- t::lib::Mocks::mock_preference( 'HoldFeeMode', 'any_time_is_placed' ); >- $fee = C4::Reserves::GetReserveFee( $patron2->borrowernumber, $biblio->biblionumber ); >- is( int($fee), 2, 'HoldFeeMode=any_time_is_placed, Patron 2 should be charged' ); >- >- t::lib::Mocks::mock_preference( 'HoldFeeMode', 'any_time_is_collected' ); >- $fee = C4::Reserves::GetReserveFee( $patron2->borrowernumber, $biblio->biblionumber ); >- is( int($fee), 2, 'HoldFeeMode=any_time_is_collected, Patron 2 should be charged' ); >- >- t::lib::Mocks::mock_preference( 'HoldFeeMode', 'any_time_is_placed' ); >- $fee = C4::Reserves::GetReserveFee( $patron4->borrowernumber, $biblio->biblionumber ); >- is( $fee, 0, 'HoldFeeMode=any_time_is_placed ; fee == 0, Patron 4 should not be charged' ); >-}; >- >-subtest 'Integration with AddReserve' => sub { >- plan tests => 2; >- >- my $dbh = C4::Context->dbh; >- >- subtest 'Items are not issued' => sub { >- plan tests => 3; >- >- t::lib::Mocks::mock_preference( 'HoldFeeMode', 'not_always' ); >- $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->biblionumber ); >- $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->borrowernumber ); >- addreserve( $patron1->borrowernumber ); >- is( acctlines( $patron1->borrowernumber ), 0, 'not_always - No fee charged for patron 1 if not issued' ); >- >- t::lib::Mocks::mock_preference( 'HoldFeeMode', 'any_time_is_placed' ); >- $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->biblionumber ); >- $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->borrowernumber ); >- addreserve( $patron1->borrowernumber ); >- is( acctlines( $patron1->borrowernumber ), 1, 'any_time_is_placed - Patron should be always charged' ); >- >- t::lib::Mocks::mock_preference( 'HoldFeeMode', 'any_time_is_collected' ); >- $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->biblionumber ); >- $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->borrowernumber ); >- addreserve( $patron1->borrowernumber ); >- is( >- acctlines( $patron1->borrowernumber ), 0, >- 'any_time_is_collected - Patron should not be charged when placing a hold' >- ); >- }; >- >- subtest 'Items are issued' => sub { >- plan tests => 4; >- >- $dbh->do( "DELETE FROM issues WHERE itemnumber=?", undef, $item1->itemnumber ); >- $dbh->do( "DELETE FROM issues WHERE itemnumber=?", undef, $item2->itemnumber ); >- C4::Circulation::AddIssue( $patron2, $item1->barcode, '2015-12-31', 0, undef, 0, {} ); >- >- t::lib::Mocks::mock_preference( 'HoldFeeMode', 'not_always' ); >- $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->biblionumber ); >- $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->borrowernumber ); >- addreserve( $patron1->borrowernumber ); >- is( >- acctlines( $patron1->borrowernumber ), 0, >- 'not_always - Patron should not be charged if items are not all checked out' >- ); >- >- $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->biblionumber ); >- $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->borrowernumber ); >- addreserve( $patron3->borrowernumber ); >- addreserve( $patron1->borrowernumber ); >- is( >- acctlines( $patron1->borrowernumber ), 0, >- 'not_always - Patron should not be charged if all the items are not checked out, even if 1 hold is already placed' >- ); >- >- C4::Circulation::AddIssue( $patron3, $item2->barcode, '2015-12-31', 0, undef, 0, {} ); >- $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->biblionumber ); >- $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->borrowernumber ); >- addreserve( $patron1->borrowernumber ); >- is( >- acctlines( $patron1->borrowernumber ), 0, >- 'not_always - Patron should not be charged if all items are checked out but no holds are placed' >- ); >- >- $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->biblionumber ); >- $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->borrowernumber ); >- addreserve( $patron3->borrowernumber ); >- addreserve( $patron1->borrowernumber ); >- is( >- acctlines( $patron1->borrowernumber ), 1, >- 'not_always - Patron should only be charged if all items are checked out and at least 1 hold is already placed' >- ); >- }; >-}; >- >-subtest 'Integration with AddIssue' => sub { >- plan tests => 5; >- >- $dbh->do( "DELETE FROM issues WHERE borrowernumber = ?", undef, $patron1->borrowernumber ); >- $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->biblionumber ); >- $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->borrowernumber ); >- >- t::lib::Mocks::mock_preference( 'HoldFeeMode', 'not_always' ); >- C4::Circulation::AddIssue( $patron1, $item1->barcode, '2015-12-31', 0, undef, 0, {} ); >- is( acctlines( $patron1->borrowernumber ), 0, 'not_always - Patron should not be charged' ); >- >- t::lib::Mocks::mock_preference( 'HoldFeeMode', 'any_time_is_placed' ); >- $dbh->do( "DELETE FROM issues WHERE borrowernumber = ?", undef, $patron1->borrowernumber ); >- C4::Circulation::AddIssue( $patron1, $item1->barcode, '2015-12-31', 0, undef, 0, {} ); >- is( acctlines( $patron1->borrowernumber ), 0, 'not_always - Patron should not be charged' ); >- >- t::lib::Mocks::mock_preference( 'HoldFeeMode', 'any_time_is_collected' ); >- $dbh->do( "DELETE FROM issues WHERE borrowernumber = ?", undef, $patron1->borrowernumber ); >- C4::Circulation::AddIssue( $patron1, $item1->barcode, '2015-12-31', 0, undef, 0, {} ); >- is( >- acctlines( $patron1->borrowernumber ), 0, >- 'any_time_is_collected - Patron should not be charged when checking out an item which was not placed hold for him' >- ); >- >- $dbh->do( "DELETE FROM issues WHERE borrowernumber = ?", undef, $patron1->borrowernumber ); >- my $id = addreserve( $patron1->borrowernumber ); >- is( >- acctlines( $patron1->borrowernumber ), 0, >- 'any_time_is_collected - Patron should not be charged yet (just checking to make sure)' >- ); >- C4::Circulation::AddIssue( $patron1, $item1->barcode, '2015-12-31', 0, undef, 0, {} ); >- is( >- acctlines( $patron1->borrowernumber ), 1, >- 'any_time_is_collected - Patron should not be charged when checking out an item which was not placed hold for him' >- ); >-}; >- >-sub acctlines { #calculate number of accountlines for a patron >- my @temp = $dbh->selectrow_array( "SELECT COUNT(*) FROM accountlines WHERE borrowernumber=?", undef, ( $_[0] ) ); >- return $temp[0]; >-} >- >-sub addreserve { >- return AddReserve( >- { >- branchcode => $library->{branchcode}, >- borrowernumber => $_[0], >- biblionumber => $biblio->biblionumber, >- priority => '1', >- title => $biblio->title, >- } >- ); >-} >- >-$schema->storage->txn_rollback; >- >-- >2.51.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 3492
:
1356
|
1357
|
119909
|
186336
|
186337
|
186338
|
186339
|
186340
|
186361
|
186691
|
186692
|
186693
|
186694
|
186695
|
186696
|
186711
|
186712
|
186713
|
186714
| 186715 |
186716