diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 71cfc79fdc..0bb4e60c56 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -271,7 +271,6 @@ sub AddReserve { # Log the hold creation if ( C4::Context->preference('HoldsLog') ) { - my $info = to_json( { hold => $hold->id, @@ -289,12 +288,11 @@ sub AddReserve { patron_expiration_date => $hold->patron_expiration_date, priority => $hold->priority, reservenotes => $hold->reservenotes, - confirmations => $confirmations, - forced => $forced + confirmations => $confirmations || [], + forced => $forced || [] }, { pretty => 1, canonical => 1 } ); - logaction( 'HOLDS', 'CREATE', $hold->id, $info ); } diff --git a/koha-tmpl/intranet-tmpl/prog/js/viewlog.js b/koha-tmpl/intranet-tmpl/prog/js/viewlog.js index 1b11ceedf7..4fb0c964e6 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/viewlog.js +++ b/koha-tmpl/intranet-tmpl/prog/js/viewlog.js @@ -75,10 +75,10 @@ function renderRecordInfo(raw) { /* global override_labels */ /** - * Render the info column for a CIRCULATION log entry whose payload is a - * JSON object containing `forced` and/or `confirmations` arrays of - * override codes. Returns rendered HTML, or null if the payload doesn't - * look like a circulation JSON object. + * Render the info column for a CIRCULATION or HOLDS log entry whose + * payload is a JSON object containing `forced` and/or `confirmations` + * arrays of override codes. Returns rendered HTML, or null if the + * payload doesn't look like a circulation JSON object. */ function renderCircInfo(raw) { if (!raw) return null; @@ -394,7 +394,7 @@ function renderInfo(data, type, row) { } var body = null; - if (mod == "CIRCULATION") { + if (mod == "CIRCULATION" || mod == "HOLDS") { body = renderCircInfo(info); } if (body === null) { diff --git a/misc/maintenance/migrate_action_logs_to_json.pl b/misc/maintenance/migrate_action_logs_to_json.pl index 4c554ace19..a24df26938 100755 --- a/misc/maintenance/migrate_action_logs_to_json.pl +++ b/misc/maintenance/migrate_action_logs_to_json.pl @@ -27,10 +27,12 @@ use C4::Context; use Koha::ActionLogs; use Koha::Checkouts; use Koha::Old::Checkouts; +use Koha::Holds; +use Koha::Old::Holds; =head1 NAME -migrate_action_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 @@ -44,14 +46,17 @@ migrate_action_logs_to_json.pl [ -c | --commit ] [ -v | --verbose ] [ --help ] =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); -print "Found $total_count log entries to migrate\n"; +# 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); + +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,15 +146,19 @@ if ( $total_count == 0 ) { exit 0; } -# Process in batches using keyset pagination by action_id. -# Using OFFSET/LIMIT here is unsafe in commit mode: each UPDATE removes the -# row from the matching set, shifting subsequent OFFSETs and silently skipping -# records. Paging by "action_id > last_seen" is stable under mutation and works -# identically in dry-run mode. -my $last_action_id = 0; -my $processed = 0; +# Both loops below use keyset pagination by action_id rather than OFFSET/LIMIT. +# Under --commit each UPDATE removes the row from the matching set, so +# OFFSET-based pagination would silently skip records as the set shrinks. +# Paging on "action_id > last_seen" is stable under mutation and works the +# same in dry-run mode. + +# Process CIRCULATION ISSUE logs +print "=== Processing CIRCULATION ISSUE logs ===\n\n" if $circ_count > 0; + my $converted = 0; my $errors = 0; +my $circ_last_id = 0; +my $circ_processed = 0; while (1) { my $select_sql = q( @@ -139,7 +175,7 @@ while (1) { ); my $sth = $dbh->prepare($select_sql); - $sth->execute( $last_action_id, $batch_size ); + $sth->execute( $circ_last_id, $batch_size ); my $batch_count = 0; @@ -149,7 +185,7 @@ while (1) { my $borrowernumber = $row->{object}; my $timestamp = $row->{timestamp}; - $last_action_id = $action_id; + $circ_last_id = $action_id; # The info should be an itemnumber my $itemnumber = $info; @@ -233,20 +269,150 @@ while (1) { last unless $batch_count; - $processed += $batch_count; + $circ_processed += $batch_count; unless ($verbose) { - print "Processed $processed / $total_count records...\r"; + print "Processed $circ_processed / $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; +my $holds_last_id = 0; +my $holds_processed = 0; + +while (1) { + 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]+$' + AND action_id > ? + ORDER BY action_id + LIMIT ? + ); + + my $sth = $dbh->prepare($select_sql); + $sth->execute( $holds_last_id, $batch_size ); + + my $batch_count = 0; + + while ( my $row = $sth->fetchrow_hashref ) { + my $action_id = $row->{action_id}; + my $info = $row->{info}; + + $holds_last_id = $action_id; + + # 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 => [] + }; + + # Match the pretty/canonical encoding used by AddReserve so the + # converted entries are byte-identical to freshly logged ones. + my $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++; + } + + last unless $batch_count; + + $holds_processed += $batch_count; + + unless ($verbose) { + print "Processed $holds_processed / $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 fbd2668347..9e37a6d23d 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -2090,7 +2090,7 @@ subtest 'CheckReserves() item type tests' => sub { }; subtest 'Bug 40866: AddReserve override JSON logging' => sub { - plan tests => 9; + plan tests => 11; $schema->storage->txn_begin; @@ -2115,7 +2115,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, @@ -2135,11 +2135,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; - + my $log = $logs->next; my $log_data = eval { from_json( $log->info ) }; - ok( !$@, 'Log info is valid JSON' ); - ok( !defined( $log_data->{confirmations} ), 'Confirmations array is undefined' ); + 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); diff --git a/tools/viewlog.pl b/tools/viewlog.pl index bb4f8adccf..80e4e9f5c9 100755 --- a/tools/viewlog.pl +++ b/tools/viewlog.pl @@ -205,7 +205,7 @@ if ($do_it) { } } - if ( $log->module eq "CIRCULATION" ) { + if ( $log->module eq "CIRCULATION" || $log->module eq "HOLDS" ) { my $info = $log->info; my $decoded; my $is_json = eval { @@ -214,13 +214,17 @@ if ($do_it) { }; if ( $is_json && ref($decoded) ) { - my $item = Koha::Items->find( $decoded->{itemnumber} ); - if ($item) { - $decoded->{biblionumber} = $item->biblionumber; - $decoded->{biblioitemnumber} = $item->biblionumber; - $decoded->{barcode} = $item->barcode; + if ( $decoded->{itemnumber} ) { + my $item = Koha::Items->find( $decoded->{itemnumber} ); + if ($item) { + $result->{'biblionumber'} = $item->biblionumber; + $result->{'biblioitemnumber'} = $item->biblionumber; + $result->{'barcode'} = $item->barcode; + } } - } else { + } elsif ( $log->module eq "CIRCULATION" ) { + + # Legacy CIRCULATION ISSUE format: info is the bare itemnumber my $item = Koha::Items->find($info); if ($item) { $result->{'biblionumber'} = $item->biblionumber;