Bugzilla – Attachment 190627 Details for
Bug 41358
action logs info column should always store JSON
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 41358: Apply consistent JSON logging to holds (Bug 40866)
cb4c45e.patch (text/plain), 14.25 KB, created by
Martin Renvoize (ashimema)
on 2025-12-18 15:58:28 UTC
(
hide
)
Description:
Bug 41358: Apply consistent JSON logging to holds (Bug 40866)
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-12-18 15:58:28 UTC
Size:
14.25 KB
patch
obsolete
>From cb4c45e6d043150ff4d01f7c2471290d44d3a71f Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Thu, 18 Dec 2025 15:00:41 +0000 >Subject: [PATCH] Bug 41358: Apply consistent JSON logging to holds (Bug 40866) > >This patch extends the bug fix to also cover HOLDS CREATE logs, which >had the same inconsistent logging pattern introduced in Bug 40866. > >Similar to CIRCULATION ISSUE logs, HOLDS CREATE logs would store just >the hold ID when there were no confirmations or forced overrides, but >would store a full JSON structure when overrides were present. This >makes reporting difficult as queries need to handle both formats. > >Changes: > >C4/Reserves.pm: >- Updated AddReserve to always store logs in JSON format >- Removed conditional logic - now always creates consistent JSON with > confirmations and forced defaulting to empty arrays > >t/db_dependent/Reserves.t: >- Updated "Bug 40866: AddReserve override JSON logging" test >- Changed test plan from 8 to 11 tests >- Now expects JSON format even for normal holds without overrides >- Fixed variable redeclaration warning > >misc/maintenance/migrate_action_logs_to_json.pl: >- Renamed from migrate_circulation_logs_to_json.pl >- Extended to handle both CIRCULATION ISSUE and HOLDS CREATE logs >- Processes both log types in batches >- Shows separate and combined statistics >- Added Koha::Holds and Koha::Old::Holds imports >- For HOLDS logs, attempts to recover branchcode, biblionumber, > and itemnumber from old_reserves/reserves tables > >This ensures both circulation and holds logs use consistent JSON >format for easier reporting and analysis. > >Sponsored-by: OpenFifth <https://openfifth.co.uk/> >--- > C4/Reserves.pm | 25 +-- > ...json.pl => migrate_action_logs_to_json.pl} | 199 ++++++++++++++++-- > t/db_dependent/Reserves.t | 14 +- > 3 files changed, 199 insertions(+), 39 deletions(-) > rename misc/maintenance/{migrate_circulation_logs_to_json.pl => migrate_action_logs_to_json.pl} (50%) > >diff --git a/C4/Reserves.pm b/C4/Reserves.pm >index 624cf08444d..ebb6d1a98ba 100644 >--- a/C4/Reserves.pm >+++ b/C4/Reserves.pm >@@ -265,20 +265,17 @@ sub AddReserve { > > # Log the hold creation > if ( C4::Context->preference('HoldsLog') ) { >- my $info = $hold->id; >- if ( defined($confirmations) || defined($forced) ) { >- $info = to_json( >- { >- hold => $hold->id, >- branchcode => $hold->branchcode, >- biblionumber => $hold->biblionumber, >- itemnumber => $hold->itemnumber, >- confirmations => $confirmations, >- forced => $forced >- }, >- { pretty => 1, canonical => 1 } >- ); >- } >+ my $info = to_json( >+ { >+ hold => $hold->id, >+ branchcode => $hold->branchcode, >+ biblionumber => $hold->biblionumber, >+ itemnumber => $hold->itemnumber, >+ confirmations => $confirmations || [], >+ forced => $forced || [] >+ }, >+ { pretty => 1, canonical => 1 } >+ ); > logaction( 'HOLDS', 'CREATE', $hold->id, $info ); > } > >diff --git a/misc/maintenance/migrate_circulation_logs_to_json.pl b/misc/maintenance/migrate_action_logs_to_json.pl >similarity index 50% >rename from misc/maintenance/migrate_circulation_logs_to_json.pl >rename to misc/maintenance/migrate_action_logs_to_json.pl >index 933f7ce15a9..7cf6fc9e465 100755 >--- a/misc/maintenance/migrate_circulation_logs_to_json.pl >+++ b/misc/maintenance/migrate_action_logs_to_json.pl >@@ -27,14 +27,16 @@ use C4::Context; > use Koha::ActionLogs; > use Koha::Checkouts; > use Koha::Old::Checkouts; >+use Koha::Holds; >+use Koha::Old::Holds; > > =head1 NAME > >-migrate_circulation_logs_to_json.pl - Migrate old circulation log entries to JSON format >+migrate_action_logs_to_json.pl - Migrate old circulation and holds log entries to JSON format > > =head1 SYNOPSIS > >-migrate_circulation_logs_to_json.pl [ -c | --commit ] [ -v | --verbose ] [ --help ] >+migrate_action_logs_to_json.pl [ -c | --commit ] [ -v | --verbose ] [ --help ] > > Options: > --help or -h Brief usage message >@@ -44,14 +46,17 @@ migrate_circulation_logs_to_json.pl [ -c | --commit ] [ -v | --verbose ] [ --hel > > =head1 DESCRIPTION > >-This script migrates old CIRCULATION ISSUE action log entries from the legacy >-itemnumber-only format to the new consistent JSON format. This is necessary >-after the fix for Bug 41358 which ensures all circulation logs are stored in >-JSON format for consistent reporting. >+This script migrates old action log entries from the legacy ID-only format to >+the new consistent JSON format. This is necessary after the fix for Bug 41358 >+which ensures all circulation and holds logs are stored in JSON format for >+consistent reporting. > > The script will: > 1. Find all CIRCULATION ISSUE log entries where info is just an itemnumber (not JSON) >-2. Convert them to JSON format with the structure: >+2. Find all HOLDS CREATE log entries where info is just a hold_id (not JSON) >+3. Convert them to JSON format with the appropriate structure: >+ >+ For CIRCULATION ISSUE: > { > "issue": <issue_id if available>, > "branchcode": <branchcode if available>, >@@ -60,6 +65,16 @@ The script will: > "forced": [] > } > >+ For HOLDS CREATE: >+ { >+ "hold": <hold_id>, >+ "branchcode": <branchcode if available>, >+ "biblionumber": <biblionumber if available>, >+ "itemnumber": <itemnumber if available>, >+ "confirmations": [], >+ "forced": [] >+ } >+ > By default, the script runs in dry-run mode. Use --commit to actually update the database. > > =head1 WARNING >@@ -87,14 +102,14 @@ if ($help) { > > my $dbh = C4::Context->dbh; > >-print "Starting migration of CIRCULATION ISSUE logs to JSON format...\n"; >+print "Starting migration of action logs to JSON format...\n"; > print $commit > ? "COMMIT MODE - Changes will be saved\n" > : "DRY RUN MODE - No changes will be saved (use --commit to save)\n"; > print "\n"; > >-# First, count how many entries need conversion >-my $count_sql = q( >+# Count CIRCULATION ISSUE entries >+my $circ_count_sql = q( > SELECT COUNT(*) > FROM action_logs > WHERE module = 'CIRCULATION' >@@ -104,9 +119,26 @@ my $count_sql = q( > AND info REGEXP '^[0-9]+$' > ); > >-my ($total_count) = $dbh->selectrow_array($count_sql); >+my ($circ_count) = $dbh->selectrow_array($circ_count_sql); >+ >+# Count HOLDS CREATE entries >+my $holds_count_sql = q( >+ SELECT COUNT(*) >+ FROM action_logs >+ WHERE module = 'HOLDS' >+ AND action = 'CREATE' >+ AND info IS NOT NULL >+ AND info NOT LIKE '{%' >+ AND info REGEXP '^[0-9]+$' >+); >+ >+my ($holds_count) = $dbh->selectrow_array($holds_count_sql); > >-print "Found $total_count log entries to migrate\n"; >+my $total_count = $circ_count + $holds_count; >+ >+print "Found $circ_count CIRCULATION ISSUE log entries to migrate\n"; >+print "Found $holds_count HOLDS CREATE log entries to migrate\n"; >+print "Total: $total_count entries\n"; > print "\n"; > > if ( $total_count == 0 ) { >@@ -114,13 +146,14 @@ if ( $total_count == 0 ) { > exit 0; > } > >-# Process in batches to avoid memory issues >-my $offset = 0; >+# Process CIRCULATION ISSUE logs >+print "=== Processing CIRCULATION ISSUE logs ===\n\n" if $circ_count > 0; >+ > my $converted = 0; >-my $skipped = 0; > my $errors = 0; > >-while ( $offset < $total_count ) { >+my $offset = 0; >+while ( $offset < $circ_count ) { > my $select_sql = q( > SELECT action_id, info, object, timestamp > FROM action_logs >@@ -229,17 +262,143 @@ while ( $offset < $total_count ) { > $offset += $batch_size; > > unless ($verbose) { >- print "Processed $offset / $total_count records...\r"; >+ print "Processed $offset / $circ_count records...\r"; > } > } > >-print "\n"; >-print "Migration complete!\n"; >+print "\n" unless $verbose; >+print "Completed CIRCULATION ISSUE: $converted converted, $errors errors\n\n"; >+ >+# Process HOLDS CREATE logs >+print "=== Processing HOLDS CREATE logs ===\n\n" if $holds_count > 0; >+ >+my $holds_converted = 0; >+my $holds_errors = 0; >+ >+$offset = 0; >+while ( $offset < $holds_count ) { >+ my $select_sql = q( >+ SELECT action_id, info, object, timestamp >+ FROM action_logs >+ WHERE module = 'HOLDS' >+ AND action = 'CREATE' >+ AND info IS NOT NULL >+ AND info NOT LIKE '{%' >+ AND info REGEXP '^[0-9]+$' >+ ORDER BY action_id >+ LIMIT ? OFFSET ? >+ ); >+ >+ my $sth = $dbh->prepare($select_sql); >+ $sth->execute( $batch_size, $offset ); >+ >+ my $batch_count = 0; >+ >+ while ( my $row = $sth->fetchrow_hashref ) { >+ my $action_id = $row->{action_id}; >+ my $info = $row->{info}; >+ >+ # The info should be a hold_id >+ my $hold_id = $info; >+ >+ # Try to find the corresponding hold to get full details >+ my $branchcode; >+ my $biblionumber; >+ my $itemnumber; >+ >+ # First check old_reserves (most likely location for old logs) >+ my $old_hold = Koha::Old::Holds->search( >+ { reserve_id => $hold_id }, >+ { rows => 1 } >+ )->next; >+ >+ if ($old_hold) { >+ $branchcode = $old_hold->branchcode; >+ $biblionumber = $old_hold->biblionumber; >+ $itemnumber = $old_hold->itemnumber; >+ } else { >+ >+ # Try current reserves table (unlikely but possible) >+ my $current_hold = Koha::Holds->search( { reserve_id => $hold_id } )->next; >+ >+ if ($current_hold) { >+ $branchcode = $current_hold->branchcode; >+ $biblionumber = $current_hold->biblionumber; >+ $itemnumber = $current_hold->itemnumber; >+ } >+ } >+ >+ # Build the JSON structure >+ my $json_data = { >+ hold => $hold_id, >+ branchcode => $branchcode, >+ biblionumber => $biblionumber, >+ itemnumber => $itemnumber, >+ confirmations => [], >+ forced => [] >+ }; >+ >+ my $json_info = encode_json($json_data); >+ >+ # Make it pretty like the code does >+ $json_data = decode_json($json_info); >+ $json_info = JSON->new->pretty(1)->canonical(1)->encode($json_data); >+ >+ if ($verbose) { >+ print "Converting action_id $action_id:\n"; >+ print " Old: $info\n"; >+ print " New: " . ( $json_info =~ s/\n/ /gr ) . "\n"; >+ print " hold_id: $hold_id\n"; >+ print " branchcode: " . ( $branchcode // 'NULL' ) . "\n"; >+ print " biblionumber: " . ( $biblionumber // 'NULL' ) . "\n"; >+ print " itemnumber: " . ( $itemnumber // 'NULL' ) . "\n"; >+ print "\n"; >+ } >+ >+ if ($commit) { >+ my $update_sql = q( >+ UPDATE action_logs >+ SET info = ? >+ WHERE action_id = ? >+ ); >+ my $update_sth = $dbh->prepare($update_sql); >+ if ( $update_sth->execute( $json_info, $action_id ) ) { >+ $holds_converted++; >+ } else { >+ print STDERR "ERROR: Failed to update action_id $action_id: " . $dbh->errstr . "\n"; >+ $holds_errors++; >+ } >+ } else { >+ $holds_converted++; >+ } >+ >+ $batch_count++; >+ } >+ >+ $offset += $batch_size; >+ >+ unless ($verbose) { >+ print "Processed $offset / $holds_count records...\r"; >+ } >+} >+ >+print "\n" unless $verbose; >+print "Completed HOLDS CREATE: $holds_converted converted, $holds_errors errors\n\n"; >+ >+# Final summary >+print "=== Migration Summary ===\n"; >+print "CIRCULATION ISSUE:\n"; > print " Converted: $converted\n"; > print " Errors: $errors\n"; >+print "HOLDS CREATE:\n"; >+print " Converted: $holds_converted\n"; >+print " Errors: $holds_errors\n"; >+print "TOTAL:\n"; >+print " Converted: " . ( $converted + $holds_converted ) . "\n"; >+print " Errors: " . ( $errors + $holds_errors ) . "\n"; > print "\n"; > >-if ( !$commit && $converted > 0 ) { >+if ( !$commit && ( $converted > 0 || $holds_converted > 0 ) ) { > print "This was a DRY RUN. Use --commit to actually update the database.\n"; > } > >diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t >index 33a99d714e5..c533166dec1 100755 >--- a/t/db_dependent/Reserves.t >+++ b/t/db_dependent/Reserves.t >@@ -2007,7 +2007,7 @@ subtest 'CheckReserves() item type tests' => sub { > }; > > subtest 'Bug 40866: AddReserve override JSON logging' => sub { >- plan tests => 8; >+ plan tests => 11; > > $schema->storage->txn_begin; > >@@ -2032,7 +2032,7 @@ subtest 'Bug 40866: AddReserve override JSON logging' => sub { > # Clear any existing logs > Koha::ActionLogs->search( { module => 'HOLDS', action => 'CREATE' } )->delete; > >- # Test 1: Normal hold without overrides - should log hold ID only >+ # Test 1: Normal hold without overrides - should log JSON with empty arrays > my $hold_id = C4::Reserves::AddReserve( > { > branchcode => $library->branchcode, >@@ -2052,8 +2052,12 @@ subtest 'Bug 40866: AddReserve override JSON logging' => sub { > { order_by => { -desc => 'timestamp' } } > ); > is( $logs->count, 1, 'One log entry created for normal hold' ); >- my $log = $logs->next; >- is( $log->info, $hold_id, 'Normal hold logs hold ID only' ); >+ my $log = $logs->next; >+ my $log_data = eval { from_json( $log->info ) }; >+ ok( !$@, 'Normal hold log info is valid JSON' ); >+ is( $log_data->{hold}, $hold_id, 'JSON contains correct hold ID' ); >+ is_deeply( $log_data->{confirmations}, [], 'Confirmations is empty array for normal hold' ); >+ is_deeply( $log_data->{forced}, [], 'Forced is empty array for normal hold' ); > > # Cancel the hold for next test > my $hold = Koha::Holds->find($hold_id); >@@ -2083,7 +2087,7 @@ subtest 'Bug 40866: AddReserve override JSON logging' => sub { > is( $logs->count, 1, 'One log entry created for override hold' ); > $log = $logs->next; > >- my $log_data = eval { from_json( $log->info ) }; >+ $log_data = eval { from_json( $log->info ) }; > ok( !$@, 'Log info is valid JSON' ); > ok( exists $log_data->{confirmations}, 'JSON contains confirmations array' ); > is_deeply( >-- >2.52.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 41358
:
190624
|
190625
|
190626
| 190627