From e1b50994b1817b42bf1d2863e853a3560b94871a Mon Sep 17 00:00:00 2001 From: Martin Renvoize 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