From cb4c45e6d043150ff4d01f7c2471290d44d3a71f Mon Sep 17 00:00:00 2001 From: Martin Renvoize 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 --- 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": , "branchcode": , @@ -60,6 +65,16 @@ The script will: "forced": [] } + For HOLDS CREATE: + { + "hold": , + "branchcode": , + "biblionumber": , + "itemnumber": , + "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