Bugzilla – Attachment 185827 Details for
Bug 9762
Log circulation overrides
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 9762: (follow-up) Add integration tests and improve AddRenewal logging consistency
Bug-9762-follow-up-Add-integration-tests-and-impro.patch (text/plain), 9.36 KB, created by
Martin Renvoize (ashimema)
on 2025-08-27 14:35:57 UTC
(
hide
)
Description:
Bug 9762: (follow-up) Add integration tests and improve AddRenewal logging consistency
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-08-27 14:35:57 UTC
Size:
9.36 KB
patch
obsolete
>From 08ace1ee8a164da2063b68a73d9f73d0a003d013 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Wed, 27 Aug 2025 15:24:21 +0100 >Subject: [PATCH] Bug 9762: (follow-up) Add integration tests and improve > AddRenewal logging consistency > >This commit adds two key improvements to the Bug 9762 circulation override logging: > >1. **Consistent JSON Logging**: Updated AddRenewal to always log in JSON format, > matching AddIssue behavior. Previously AddRenewal only logged JSON when > confirmations/forced were present, otherwise logged just the item number. > >2. **Integration Tests**: Added comprehensive tests for both AddIssue and AddRenewal > JSON logging functionality: > - Test normal operations (empty confirmations/forced arrays) > - Test override operations (populated confirmations/forced arrays) > - Verify JSON structure and content accuracy > >The AddRenewal function now consistently logs: >```json >{ > "issue": 123, > "itemnumber": 456, > "confirmations": [...], > "forced": [...] >} >``` > >This ensures uniform action log format across all circulation operations and >provides better integration test coverage for the override logging functionality. > >Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> >--- > C4/Circulation.pm | 27 +++---- > t/db_dependent/Circulation.t | 140 ++++++++++++++++++++++++++++++++++- > 2 files changed, 151 insertions(+), 16 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index dc84be20727..7f79c6b95a8 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -3682,21 +3682,18 @@ sub AddRenewal { > > #Log the renewal > if ( C4::Context->preference("RenewalLog") ) { >- my $info = $itemnumber; >- if ( defined($confirmations) || defined($forced) ) { >- $info = to_json( >- { >- issue => $issue->issue_id, >- itemnumber => $itemnumber, >- confirmations => $confirmations, >- forced => $forced >- }, >- { >- pretty => 1, >- canonical => 1 >- } >- ); >- } >+ my $info = to_json( >+ { >+ issue => $issue->issue_id, >+ itemnumber => $itemnumber, >+ confirmations => $confirmations || [], >+ forced => $forced || [] >+ }, >+ { >+ pretty => 1, >+ canonical => 1 >+ } >+ ); > logaction( "CIRCULATION", "RENEWAL", $borrowernumber, $info ); > } > >diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t >index b78dca3eff7..aa5821daf0b 100755 >--- a/t/db_dependent/Circulation.t >+++ b/t/db_dependent/Circulation.t >@@ -19,7 +19,7 @@ use Modern::Perl; > use utf8; > > use Test::NoWarnings; >-use Test::More tests => 78; >+use Test::More tests => 80; > use Test::Exception; > use Test::MockModule; > use Test::Deep qw( cmp_deeply ); >@@ -28,6 +28,7 @@ use Test::Warn; > use Data::Dumper; > use DateTime; > use Time::Fake; >+use JSON qw( from_json ); > use POSIX qw( floor ); > use t::lib::Mocks; > use t::lib::TestBuilder; >@@ -7769,6 +7770,143 @@ subtest 'ChildNeedsGuarantor' => sub { > "AddReturn generates no warning when can_be_guarantee type patron has a guarantor and not ChildNeedsGuarantor"; > }; > >+subtest 'Bug 9762: AddIssue override JSON logging' => sub { >+ plan tests => 8; >+ >+ my $library = $builder->build_object( { class => 'Koha::Libraries' } ); >+ my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); >+ my $item = $builder->build_sample_item( { library => $library->branchcode } ); >+ >+ t::lib::Mocks::mock_userenv( { patron => $patron, branchcode => $library->branchcode } ); >+ t::lib::Mocks::mock_preference( 'IssueLog', 1 ); >+ >+ # Clear any existing logs >+ Koha::ActionLogs->search( { module => 'CIRCULATION', action => 'ISSUE' } )->delete; >+ >+ # Test 1: Normal checkout without overrides - should log item number only >+ my $issue = C4::Circulation::AddIssue( $patron, $item->barcode ); >+ ok( $issue, 'Item checked out successfully' ); >+ >+ my $logs = Koha::ActionLogs->search( >+ { >+ module => 'CIRCULATION', >+ action => 'ISSUE', >+ object => $patron->borrowernumber >+ }, >+ { order_by => { -desc => 'timestamp' } } >+ ); >+ is( $logs->count, 1, 'One log entry created for normal checkout' ); >+ my $log = $logs->next; >+ is( $log->info, $item->itemnumber, 'Normal checkout logs item number only' ); >+ >+ # Return the item for next test >+ C4::Circulation::AddReturn( $item->barcode, $library->branchcode ); >+ >+ # Test 2: Checkout with confirmations - should log JSON with confirmations >+ my $item2 = $builder->build_sample_item( { library => $library->branchcode } ); >+ >+ my $issue2 = C4::Circulation::AddIssue( >+ $patron, >+ $item2->barcode, >+ undef, undef, undef, undef, >+ { >+ confirmations => [ 'DEBT', 'AGE_RESTRICTION' ], >+ forced => [] >+ } >+ ); >+ ok( $issue2, 'Item with confirmations checked out successfully' ); >+ >+ $logs = Koha::ActionLogs->search( >+ { >+ module => 'CIRCULATION', >+ action => 'ISSUE', >+ object => $patron->borrowernumber, >+ info => { '!=', $item->itemnumber } # Exclude the first test log >+ }, >+ { order_by => { -desc => 'timestamp' } } >+ ); >+ is( $logs->count, 1, 'One log entry created for override checkout' ); >+ $log = $logs->next; >+ >+ my $log_data = eval { from_json( $log->info ) }; >+ ok( !$@, 'Log info is valid JSON' ); >+ ok( exists $log_data->{confirmations}, 'JSON contains confirmations array' ); >+ is_deeply( $log_data->{confirmations}, [ 'DEBT', 'AGE_RESTRICTION' ], 'Confirmations logged correctly' ); >+}; >+ >+subtest 'Bug 9762: AddRenewal override JSON logging' => sub { >+ plan tests => 11; >+ >+ my $library = $builder->build_object( { class => 'Koha::Libraries' } ); >+ my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); >+ my $item = $builder->build_sample_item( { library => $library->branchcode } ); >+ >+ t::lib::Mocks::mock_userenv( { patron => $patron, branchcode => $library->branchcode } ); >+ t::lib::Mocks::mock_preference( 'RenewalLog', 1 ); >+ >+ # Issue the item first >+ my $issue = C4::Circulation::AddIssue( $patron, $item->barcode ); >+ >+ # Clear any existing renewal logs >+ Koha::ActionLogs->search( { module => 'CIRCULATION', action => 'RENEWAL' } )->delete; >+ >+ # Test 1: Normal renewal without overrides - should log item number only >+ C4::Circulation::AddRenewal( >+ { >+ borrowernumber => $patron->borrowernumber, >+ itemnumber => $item->itemnumber, >+ branch => $library->branchcode >+ } >+ ); >+ >+ my $logs = Koha::ActionLogs->search( >+ { >+ module => 'CIRCULATION', >+ action => 'RENEWAL', >+ object => $patron->borrowernumber >+ }, >+ { order_by => { -desc => 'timestamp' } } >+ ); >+ is( $logs->count, 1, 'One log entry created for normal renewal' ); >+ my $log = $logs->next; >+ >+ my $log_data = eval { from_json( $log->info ) }; >+ ok( !$@, 'Normal renewal log info is valid JSON' ); >+ is( $log_data->{itemnumber}, $item->itemnumber, 'Normal renewal JSON contains itemnumber' ); >+ is_deeply( $log_data->{confirmations}, [], 'Normal renewal has empty confirmations array' ); >+ is_deeply( $log_data->{forced}, [], 'Normal renewal has empty forced array' ); >+ >+ # Test 2: Renewal with overrides - should log JSON with confirmations >+ C4::Circulation::AddRenewal( >+ { >+ borrowernumber => $patron->borrowernumber, >+ itemnumber => $item->itemnumber, >+ branch => $library->branchcode, >+ confirmations => [ 'ON_RESERVE', 'RENEWAL_LIMIT' ], >+ forced => ['TOO_MUCH'] >+ } >+ ); >+ >+ # Get only the most recent renewal log (the one with overrides) >+ my $override_log = Koha::ActionLogs->search( >+ { >+ module => 'CIRCULATION', >+ action => 'RENEWAL', >+ object => $patron->borrowernumber, >+ info => { 'like' => '%ON_RESERVE%' } # Only get the log with our test overrides >+ }, >+ { order_by => { -desc => 'timestamp' }, rows => 1 } >+ )->next; >+ >+ ok( $override_log, 'Found override renewal log entry' ); >+ $log_data = eval { from_json( $override_log->info ) }; >+ ok( !$@, 'Override renewal log info is valid JSON' ); >+ ok( exists $log_data->{confirmations}, 'JSON contains confirmations array' ); >+ ok( exists $log_data->{forced}, 'JSON contains forced array' ); >+ is_deeply( $log_data->{confirmations}, [ 'ON_RESERVE', 'RENEWAL_LIMIT' ], 'Confirmations logged correctly' ); >+ is_deeply( $log_data->{forced}, ['TOO_MUCH'], 'Forced overrides logged correctly' ); >+}; >+ > $schema->storage->txn_rollback; > C4::Context->clear_syspref_cache(); > $branches = Koha::Libraries->search(); >-- >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 9762
:
149554
|
156222
|
163847
|
164022
|
164067
|
164068
|
164302
|
164387
|
164426
|
164479
|
164508
|
164509
|
164510
|
164519
|
170271
|
170272
|
170273
|
176742
|
177294
|
177295
|
177296
|
178029
|
178030
|
178031
|
178036
|
181303
|
181304
|
181305
|
181306
|
181307
|
181308
|
181340
|
181341
|
181342
|
181343
|
181344
|
181345
|
181346
|
181357
|
181358
|
181359
|
181360
|
181361
|
181362
|
181363
|
181371
|
181381
|
181390
|
181393
|
181545
|
181546
|
185118
|
185314
|
185315
|
185316
|
185317
|
185318
|
185319
|
185320
|
185321
|
185322
|
185323
|
185815
|
185816
|
185817
|
185818
|
185819
|
185820
|
185821
|
185822
|
185823
|
185824
|
185825
|
185826
| 185827