Bugzilla – Attachment 186481 Details for
Bug 40817
Holds charges should be accessible from Holds
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 40817: Add comprehensive unit tests for hold-account line linking
Bug-40817-Add-comprehensive-unit-tests-for-hold-ac.patch (text/plain), 19.28 KB, created by
Martin Renvoize (ashimema)
on 2025-09-16 12:57:46 UTC
(
hide
)
Description:
Bug 40817: Add comprehensive unit tests for hold-account line linking
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-09-16 12:57:46 UTC
Size:
19.28 KB
patch
obsolete
>From e1b50994b1817b42bf1d2863e853a3560b94871a Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Tue, 16 Sep 2025 13:49:51 +0100 >Subject: [PATCH] Bug 40817: Add comprehensive unit tests for hold-account line > linking > >This patch adds comprehensive unit test coverage for the hold-account >line linking functionality: > >t/db_dependent/Koha/Account/Line.t: >- Test hold() method returns correct Koha::Hold/Koha::Old::Hold objects >- Test handling of reserve_id and old_reserve_id fields >- Test graceful handling of NULL and invalid references > >t/db_dependent/Koha/Hold.t: >- Test debits() method returns correct Koha::Account::Debits collection >- Test _move_to_old() account line migration functionality >- Test proper linking of fees to holds during creation and lifecycle > >t/db_dependent/Koha/Old/Hold.t: >- Test debits() method for historical holds >- Test account line relationships for completed/cancelled holds > >t/db_dependent/Koha/Account.t: >- Test add_debit() with hold_id parameter for current and old holds >- Test automatic hold detection and field assignment >- Test error handling for non-existent holds >--- > t/db_dependent/Koha/Account.t | 108 +++++++++++++++++++- > t/db_dependent/Koha/Account/Line.t | 99 +++++++++++++++++- > t/db_dependent/Koha/Hold.t | 157 ++++++++++++++++++++++++++++- > t/db_dependent/Koha/Old/Hold.t | 89 +++++++++++++++- > 4 files changed, 449 insertions(+), 4 deletions(-) > >diff --git a/t/db_dependent/Koha/Account.t b/t/db_dependent/Koha/Account.t >index 057c3e9eda8..b27189d51c2 100755 >--- a/t/db_dependent/Koha/Account.t >+++ b/t/db_dependent/Koha/Account.t >@@ -20,7 +20,7 @@ > use Modern::Perl; > > use Test::NoWarnings; >-use Test::More tests => 16; >+use Test::More tests => 17; > use Test::MockModule; > use Test::Exception; > use Test::Warn; >@@ -32,6 +32,8 @@ use Koha::Account::CreditTypes; > use Koha::Account::Lines; > use Koha::Account::Offsets; > use Koha::DateUtils qw( dt_from_string ); >+use Koha::Holds; >+use Koha::Old::Holds; > > use t::lib::Mocks; > use t::lib::TestBuilder; >@@ -1549,3 +1551,107 @@ subtest 'Koha::Account::payin_amount() tests' => sub { > > $schema->storage->txn_rollback; > }; >+ >+subtest 'add_debit() with hold_id tests' => sub { >+ >+ plan tests => 8; >+ >+ $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 } ); >+ >+ # Create a hold >+ my $hold = $builder->build_object( >+ { >+ class => 'Koha::Holds', >+ value => { >+ borrowernumber => $patron->borrowernumber, >+ biblionumber => $biblio->biblionumber, >+ itemnumber => $item->itemnumber, >+ branchcode => $patron->branchcode, >+ } >+ } >+ ); >+ >+ # Create an old hold >+ my $old_hold = $builder->build_object( >+ { >+ class => 'Koha::Old::Holds', >+ value => { >+ borrowernumber => $patron->borrowernumber, >+ biblionumber => $biblio->biblionumber, >+ itemnumber => $item->itemnumber, >+ branchcode => $patron->branchcode, >+ } >+ } >+ ); >+ >+ my $account = $patron->account; >+ >+ # Test 1: add_debit with current hold_id sets reserve_id >+ my $debit1 = $account->add_debit( >+ { >+ amount => 5.00, >+ description => 'Hold fee', >+ type => 'RESERVE', >+ user_id => 1, >+ library_id => $patron->branchcode, >+ interface => 'intranet', >+ hold_id => $hold->id, >+ } >+ ); >+ >+ is( $debit1->reserve_id, $hold->id, 'add_debit() sets reserve_id correctly for current hold' ); >+ is( $debit1->old_reserve_id, undef, 'add_debit() leaves old_reserve_id undef for current hold' ); >+ >+ # Test 2: add_debit with old hold_id sets old_reserve_id >+ my $debit2 = $account->add_debit( >+ { >+ amount => 3.00, >+ description => 'Old hold fee', >+ type => 'RESERVE', >+ user_id => 1, >+ library_id => $patron->branchcode, >+ interface => 'intranet', >+ hold_id => $old_hold->id, >+ } >+ ); >+ >+ is( $debit2->reserve_id, undef, 'add_debit() leaves reserve_id undef for old hold' ); >+ is( $debit2->old_reserve_id, $old_hold->id, 'add_debit() sets old_reserve_id correctly for old hold' ); >+ >+ # Test 3: add_debit without hold_id leaves both fields undef >+ my $debit3 = $account->add_debit( >+ { >+ amount => 2.00, >+ description => 'Regular fee', >+ type => 'OVERDUE', >+ user_id => 1, >+ library_id => $patron->branchcode, >+ interface => 'intranet', >+ } >+ ); >+ >+ is( $debit3->reserve_id, undef, 'add_debit() without hold_id leaves reserve_id undef' ); >+ is( $debit3->old_reserve_id, undef, 'add_debit() without hold_id leaves old_reserve_id undef' ); >+ >+ # Test 4: add_debit with non-existent hold_id handles gracefully >+ my $debit4 = $account->add_debit( >+ { >+ amount => 1.50, >+ description => 'Fee for missing hold', >+ type => 'RESERVE', >+ user_id => 1, >+ library_id => $patron->branchcode, >+ interface => 'intranet', >+ hold_id => 99999, # Non-existent hold ID >+ } >+ ); >+ >+ is( $debit4->reserve_id, undef, 'add_debit() handles non-existent hold_id gracefully (reserve_id)' ); >+ is( $debit4->old_reserve_id, undef, 'add_debit() handles non-existent hold_id gracefully (old_reserve_id)' ); >+ >+ $schema->storage->txn_rollback; >+}; >diff --git a/t/db_dependent/Koha/Account/Line.t b/t/db_dependent/Koha/Account/Line.t >index 07e0f1cfa66..6e8db89e6e9 100755 >--- a/t/db_dependent/Koha/Account/Line.t >+++ b/t/db_dependent/Koha/Account/Line.t >@@ -20,7 +20,7 @@ > use Modern::Perl; > > use Test::NoWarnings; >-use Test::More tests => 16; >+use Test::More tests => 17; > use Test::Exception; > use Test::MockModule; > >@@ -30,7 +30,9 @@ use C4::Circulation qw( AddRenewal CanBookBeRenewed LostItem AddIssue AddReturn > use Koha::Account; > use Koha::Account::Lines; > use Koha::Account::Offsets; >+use Koha::Holds; > use Koha::Items; >+use Koha::Old::Holds; > use Koha::DateUtils qw( dt_from_string ); > > use t::lib::Mocks; >@@ -1470,4 +1472,99 @@ subtest "cancel() tests" => sub { > $schema->storage->txn_rollback; > }; > >+subtest 'hold() tests' => sub { >+ >+ plan tests => 7; >+ >+ $schema->storage->txn_begin; >+ >+ my $library = $builder->build_object( { class => 'Koha::Libraries' } ); >+ my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); >+ my $biblio = $builder->build_sample_biblio; >+ >+ # Create a hold >+ my $hold = $builder->build_object( >+ { >+ class => 'Koha::Holds', >+ value => { >+ borrowernumber => $patron->borrowernumber, >+ biblionumber => $biblio->biblionumber, >+ branchcode => $library->branchcode, >+ } >+ } >+ ); >+ >+ # Create an old hold >+ my $old_hold = $builder->build_object( >+ { >+ class => 'Koha::Old::Holds', >+ value => { >+ borrowernumber => $patron->borrowernumber, >+ biblionumber => $biblio->biblionumber, >+ branchcode => $library->branchcode, >+ } >+ } >+ ); >+ >+ # Test 1: Account line with reserve_id returns current hold >+ my $line_with_hold = Koha::Account::Line->new( >+ { >+ borrowernumber => $patron->borrowernumber, >+ debit_type_code => "RESERVE", >+ status => "UNRETURNED", >+ amount => 5.00, >+ reserve_id => $hold->reserve_id, >+ interface => 'commandline', >+ } >+ )->store; >+ >+ my $returned_hold = $line_with_hold->hold; >+ is( ref($returned_hold), 'Koha::Hold', 'hold() returns a Koha::Hold object when reserve_id is set' ); >+ is( $returned_hold->reserve_id, $hold->reserve_id, 'hold() returns the correct hold' ); >+ >+ # Test 2: Account line with old_reserve_id returns old hold >+ my $line_with_old_hold = Koha::Account::Line->new( >+ { >+ borrowernumber => $patron->borrowernumber, >+ debit_type_code => "RESERVE", >+ status => "UNRETURNED", >+ amount => 5.00, >+ old_reserve_id => $old_hold->reserve_id, >+ interface => 'commandline', >+ } >+ )->store; >+ >+ my $returned_old_hold = $line_with_old_hold->hold; >+ is( >+ ref($returned_old_hold), 'Koha::Old::Hold', >+ 'hold() returns a Koha::Old::Hold object when old_reserve_id is set' >+ ); >+ is( $returned_old_hold->reserve_id, $old_hold->reserve_id, 'hold() returns the correct old hold' ); >+ >+ # Test 3: Account line with neither reserve_id nor old_reserve_id returns undef >+ my $line_without_hold = Koha::Account::Line->new( >+ { >+ borrowernumber => $patron->borrowernumber, >+ debit_type_code => "OVERDUE", >+ status => "RETURNED", >+ amount => 3.00, >+ interface => 'commandline', >+ } >+ )->store; >+ >+ is( $line_without_hold->hold, undef, 'hold() returns undef when neither reserve_id nor old_reserve_id is set' ); >+ >+ # Test 4: Set reserve_id to NULL and verify it returns undef >+ $line_with_hold->reserve_id(undef)->store; >+ >+ is( $line_with_hold->hold, undef, 'hold() returns undef when reserve_id is set to NULL' ); >+ >+ # Test 5: Set old_reserve_id to NULL and verify it returns undef >+ $line_with_old_hold->old_reserve_id(undef)->store; >+ >+ is( $line_with_old_hold->hold, undef, 'hold() returns undef when old_reserve_id is set to NULL' ); >+ >+ $schema->storage->txn_rollback; >+}; >+ > 1; >diff --git a/t/db_dependent/Koha/Hold.t b/t/db_dependent/Koha/Hold.t >index 6fe4e08f8af..da4623024db 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 => 21; >+use Test::More tests => 23; > > use Test::Exception; > use Test::MockModule; >@@ -2202,3 +2202,158 @@ subtest 'charge_hold_fee() tests' => sub { > > $schema->storage->txn_rollback; > }; >+ >+subtest 'debits() tests' => sub { >+ >+ plan tests => 5; >+ >+ $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 } ); >+ >+ # Create a hold >+ my $hold = $builder->build_object( >+ { >+ class => 'Koha::Holds', >+ value => { >+ borrowernumber => $patron->borrowernumber, >+ biblionumber => $biblio->biblionumber, >+ itemnumber => $item->itemnumber, >+ branchcode => $patron->branchcode, >+ } >+ } >+ ); >+ >+ # Test 1: Hold with no debits returns empty collection >+ my $debits = $hold->debits; >+ isa_ok( $debits, 'Koha::Account::Debits', 'debits() returns a Koha::Account::Debits object' ); >+ is( $debits->count, 0, 'debits() returns empty collection when no debits exist' ); >+ >+ # Test 2: Create some account lines (debits and credits) linked to the hold >+ my $account = $patron->account; >+ >+ # Add a debit (hold fee) >+ my $debit1 = $account->add_debit( >+ { >+ amount => 5.00, >+ description => 'Hold fee', >+ type => 'RESERVE', >+ user_id => 1, >+ library_id => $patron->branchcode, >+ interface => 'intranet', >+ hold_id => $hold->id, >+ } >+ ); >+ >+ # Add another debit (expiration fee) >+ my $debit2 = $account->add_debit( >+ { >+ amount => 2.00, >+ description => 'Hold expiration fee', >+ type => 'RESERVE_EXPIRED', >+ user_id => 1, >+ library_id => $patron->branchcode, >+ interface => 'intranet', >+ hold_id => $hold->id, >+ } >+ ); >+ >+ # Add a credit (should not appear in debits) >+ my $credit = $account->add_credit( >+ { >+ amount => 3.00, >+ description => 'Credit for hold', >+ type => 'CREDIT', >+ user_id => 1, >+ library_id => $patron->branchcode, >+ interface => 'intranet', >+ } >+ ); >+ >+ # Link credit to hold manually since add_credit doesn't have hold_id parameter >+ $credit->reserve_id( $hold->id )->store; >+ >+ # Test 3: Hold debits returns only debit lines, ordered by timestamp descending >+ $debits = $hold->debits; >+ is( $debits->count, 2, 'debits() returns correct count of debit lines only' ); >+ >+ my @debit_amounts = sort { $b <=> $a } map { $_->amount + 0 } $debits->as_list; >+ is_deeply( \@debit_amounts, [ 5.0, 2.0 ], 'debits() returns correct amounts in descending order' ); >+ >+ # Test 4: Verify both debit types are present >+ my @debit_types = sort map { $_->debit_type_code } $debits->as_list; >+ is_deeply( \@debit_types, [ 'RESERVE', 'RESERVE_EXPIRED' ], 'debits() returns both expected debit types' ); >+ >+ $schema->storage->txn_rollback; >+}; >+ >+subtest '_move_to_old() account migration 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 } ); >+ >+ # Create a hold >+ my $hold = $builder->build_object( >+ { >+ class => 'Koha::Holds', >+ value => { >+ borrowernumber => $patron->borrowernumber, >+ biblionumber => $biblio->biblionumber, >+ itemnumber => $item->itemnumber, >+ branchcode => $patron->branchcode, >+ } >+ } >+ ); >+ >+ my $account = $patron->account; >+ >+ # Create account lines linked to the hold >+ my $debit1 = $account->add_debit( >+ { >+ amount => 5.00, >+ description => 'Hold fee', >+ type => 'RESERVE', >+ user_id => 1, >+ library_id => $patron->branchcode, >+ interface => 'intranet', >+ hold_id => $hold->id, >+ } >+ ); >+ >+ my $debit2 = $account->add_debit( >+ { >+ amount => 2.00, >+ description => 'Hold expiration fee', >+ type => 'RESERVE_EXPIRED', >+ user_id => 1, >+ library_id => $patron->branchcode, >+ interface => 'intranet', >+ hold_id => $hold->id, >+ } >+ ); >+ >+ # Test initial state >+ is( $debit1->reserve_id, $hold->id, 'Account line initially has reserve_id set' ); >+ is( $debit1->old_reserve_id, undef, 'Account line initially has old_reserve_id unset' ); >+ is( $debit2->reserve_id, $hold->id, 'Second account line initially has reserve_id set' ); >+ >+ # Call _move_to_old >+ my $old_hold = $hold->_move_to_old; >+ >+ # Verify account lines were updated >+ $debit1->discard_changes; >+ $debit2->discard_changes; >+ >+ is( $debit1->reserve_id, undef, 'Account line reserve_id cleared after _move_to_old' ); >+ is( $debit1->old_reserve_id, $hold->id, 'Account line old_reserve_id set after _move_to_old' ); >+ is( $debit2->old_reserve_id, $hold->id, 'Second account line old_reserve_id set after _move_to_old' ); >+ >+ $schema->storage->txn_rollback; >+}; >diff --git a/t/db_dependent/Koha/Old/Hold.t b/t/db_dependent/Koha/Old/Hold.t >index 70b33688406..ac3c5d14fe4 100755 >--- a/t/db_dependent/Koha/Old/Hold.t >+++ b/t/db_dependent/Koha/Old/Hold.t >@@ -17,11 +17,12 @@ > > use Modern::Perl; > >-use Test::More tests => 4; >+use Test::More tests => 5; > use Test::NoWarnings; > use Test::Exception; > > use Koha::Database; >+use Koha::Account; > > use t::lib::Mocks; > use t::lib::TestBuilder; >@@ -148,3 +149,89 @@ subtest 'item/pickup_library() tests' => sub { > > $schema->storage->txn_rollback; > }; >+ >+subtest 'debits() tests' => sub { >+ >+ plan tests => 5; >+ >+ $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 } ); >+ >+ # Create an old hold >+ my $old_hold = $builder->build_object( >+ { >+ class => 'Koha::Old::Holds', >+ value => { >+ borrowernumber => $patron->borrowernumber, >+ biblionumber => $biblio->biblionumber, >+ itemnumber => $item->itemnumber, >+ branchcode => $patron->branchcode, >+ } >+ } >+ ); >+ >+ # Test 1: Old hold with no debits returns empty collection >+ my $debits = $old_hold->debits; >+ isa_ok( $debits, 'Koha::Account::Debits', 'debits() returns a Koha::Account::Debits object' ); >+ is( $debits->count, 0, 'debits() returns empty collection when no debits exist' ); >+ >+ # Test 2: Create account lines (debits and credits) linked to the old hold >+ my $account = $patron->account; >+ >+ # Add a debit linked to old hold >+ my $debit1 = $account->add_debit( >+ { >+ amount => 3.50, >+ description => 'Old hold fee', >+ type => 'RESERVE', >+ user_id => 1, >+ library_id => $patron->branchcode, >+ interface => 'intranet', >+ } >+ ); >+ >+ # Manually set old_reserve_id since add_debit doesn't handle old holds directly >+ $debit1->old_reserve_id( $old_hold->reserve_id )->store; >+ >+ # Add another debit >+ my $debit2 = $account->add_debit( >+ { >+ amount => 1.50, >+ description => 'Old hold expiration fee', >+ type => 'RESERVE_EXPIRED', >+ user_id => 1, >+ library_id => $patron->branchcode, >+ interface => 'intranet', >+ } >+ ); >+ $debit2->old_reserve_id( $old_hold->reserve_id )->store; >+ >+ # Add a credit (should not appear in debits) >+ my $credit = $account->add_credit( >+ { >+ amount => 2.00, >+ description => 'Credit for old hold', >+ type => 'CREDIT', >+ user_id => 1, >+ library_id => $patron->branchcode, >+ interface => 'intranet', >+ } >+ ); >+ $credit->old_reserve_id( $old_hold->reserve_id )->store; >+ >+ # Test 3: Old hold debits returns only debit lines, ordered by timestamp descending >+ $debits = $old_hold->debits; >+ is( $debits->count, 2, 'debits() returns correct count of debit lines only' ); >+ >+ my @debit_amounts = sort { $b <=> $a } map { $_->amount + 0 } $debits->as_list; >+ is_deeply( \@debit_amounts, [ 3.5, 1.5 ], 'debits() returns correct amounts in descending order' ); >+ >+ # Test 4: Verify both debit types are present >+ my @debit_types = sort map { $_->debit_type_code } $debits->as_list; >+ is_deeply( \@debit_types, [ 'RESERVE', 'RESERVE_EXPIRED' ], 'debits() returns both expected debit types' ); >+ >+ $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 40817
:
186476
|
186477
|
186478
|
186479
|
186480
| 186481