In C4/Circulation.pm: # Log the checkout if ( C4::Context->preference('IssueLog') ) { my $info = $item_object->itemnumber; if ( defined($confirmations) || defined($forced) ) { $info = to_json( { issue => $issue->issue_id, branchcode => $issue->branchcode, itemnumber => $item_object->itemnumber, confirmations => $confirmations, forced => $forced }, { pretty => 1, canonical => 1 } ); } logaction( "CIRCULATION", "ISSUE", $patron->borrowernumber, $info, ); } It appears there is a way, if $forced and $confirmations is undef, to store just the itemnumber. If we are going to store JSON it should always be JSON. Storing the itemnumber sometimes and a JSON object other times makes reporting really hard.
Almost everywhere I see AddIssue called there is: confirmations => [ grep { /^[A-Z_]+$/ } keys %{$confirmation} ], forced => [ keys %{$impossible} ] I don't think this will ever be undef. An empty array is not undef. The only place I don't see something like this is offline_circ/process_koc.pl.
Created attachment 190624 [details] [review] Bug 41358: Always store circulation logs in consistent JSON format Previously, CIRCULATION ISSUE logs would store just the itemnumber when there were no confirmations or forced overrides, but would store a full JSON structure when overrides were present. This inconsistency makes reporting difficult as queries need to handle both formats. This patch changes AddIssue to always store logs in JSON format, matching the pattern already used in AddRenewal. The JSON structure now consistently includes: - issue: issue_id - branchcode: branchcode - itemnumber: itemnumber - confirmations: array of confirmation codes (empty array if none) - forced: array of forced override codes (empty array if none) Benefits: - Consistent data format simplifies reporting and analysis - Matches existing RENEWAL logging pattern - Display code (tools/viewlog.pl) already handles both formats, so backward compatibility is maintained Test plan: 1. prove t/db_dependent/Circulation.t 2. Perform a normal checkout - verify action log contains JSON 3. Perform a checkout with overrides - verify confirmations/forced are populated in the JSON 4. Check Tools > Log viewer - both types should display correctly Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Created attachment 190625 [details] [review] Bug 41358: Add migration script for legacy circulation logs This patch adds an out-of-band migration script to convert legacy CIRCULATION ISSUE action logs from the old itemnumber-only format to the new consistent JSON format. The script: - Finds all CIRCULATION ISSUE logs where info is just an itemnumber - Converts them to JSON format with the standard structure - Attempts to recover issue_id and branchcode from old_issues/issues - Runs in dry-run mode by default (--commit flag to actually update) - Processes in batches to handle large databases safely - Includes verbose mode for detailed output Usage: perl misc/maintenance/migrate_circulation_logs_to_json.pl --help perl misc/maintenance/migrate_circulation_logs_to_json.pl --verbose perl misc/maintenance/migrate_circulation_logs_to_json.pl --commit This is designed as an out-of-band maintenance script rather than an atomic update because: 1. May be expensive on large databases with many log entries 2. Migration is optional - display code handles both formats 3. Can be run during off-peak hours at admin's discretion Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Created attachment 190626 [details] [review] Bug 41358: Log forced overrides for offline and ILL checkouts This patch updates offline circulation and ILL checkouts to properly record their nature in circulation logs as forced transactions. Offline circulation (process_koc.pl): - Offline transactions have already occurred without access to normal circulation checks - Now marks these as forced with 'OFFLINE_CIRCULATION' override code - This helps identify historical transactions that bypassed checks ILL checkouts (Koha/ILL/Request.pm): - ILL checkouts are system-initiated and may bypass certain restrictions by design - Now marks these as forced with 'ILL_CHECKOUT' override code - This helps track and report on ILL circulation activity separately Display updates (action-logs.inc): - Added translation strings for both new override codes: - OFFLINE_CIRCULATION: "Offline circulation checkout" - ILL_CHECKOUT: "ILL system checkout" - These will now display properly in Tools > Log viewer This ensures these special checkout types are properly identified in reporting and analysis, making it easier to understand the context of historical transactions. Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Created attachment 190627 [details] [review] 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/>
I did some analysis of the what we pass into info whilst I was digging here: We have 13 different forms in this field.. explained below.. I'd love to try and bring that down a bit :S --- 1. Empty String ("") Usage: When no additional info is needed beyond module/action/object logaction( "MEMBERS", "CREATE", $borrowernumber, "" ) logaction( "SERIAL", "ADD", $subscriptionid, "" ) Modules: MEMBERS (CREATE, DELETE), SERIAL (ADD, MODIFY, RENEW, DELETE) --- 2. Simple String Literal Usage: Fixed descriptive text logaction( "AUTHORITIES", "ADD", $authid, "authority" ) logaction( "CATALOGUING", "ADD", $biblionumber, "biblio" ) logaction( "CATALOGUING", "ADD", $itemnumber, "item" ) Modules: AUTHORITIES, CATALOGUING --- 3. Simple ID/Number (Now deprecated for CIRCULATION/HOLDS) Usage: Just the item/hold ID (your bug fixes this!) logaction( "CIRCULATION", "RETURN", $borrowernumber, $item->itemnumber ) Modules: CIRCULATION (RETURN only - ISSUE now uses JSON) --- 4. Concatenated String with Context Usage: Pipe-separated values or descriptive messages logaction( 'SYSTEMPREFERENCE', 'MODIFY', undef, $variable . ' | ' . $value ) logaction( "REPORTS", "DELETE", $id, $report_name . " | " . $savedsql ) logaction( 'AUTH', 'SUCCESS', $patron_id, "Valid password for $userid", $type ) logaction( 'RECALLS', 'CANCEL', $recall_id, "Recall cancelled", 'INTRANET' ) Modules: SYSTEMPREFERENCE, REPORTS, AUTH, RECALLS Format: Usually "field1 | field2" or descriptive message --- 5. BEFORE/AFTER Comparison Usage: Stores state before modification for comparison logaction( "AUTHORITIES", "MODIFY", $authid, "authority BEFORE=>" . $oldrecord->as_formatted ) logaction( "CATALOGUING", "MODIFY", $biblionumber, "biblio $decoding_error BEFORE=>" . $record->as_formatted ) Modules: AUTHORITIES, CATALOGUING Format: "BEFORE=>..." (full MARC record dump) Purpose: Enables visual diff comparison in log viewer --- 6. JSON Structure ✨ (Modern, Consistent) Usage: Structured data with overrides, changes, or complex info 6a. Override/Confirmation Pattern (Your fixes!) logaction( "CIRCULATION", "ISSUE", $borrowernumber, to_json({ issue => $issue_id, branchcode => $branchcode, itemnumber => $itemnumber, confirmations => ['DEBT', 'AGE_RESTRICTION'], forced => ['OFFLINE_CIRCULATION'] }, { pretty => 1, canonical => 1 }) ) Modules: CIRCULATION (ISSUE, RENEWAL), HOLDS (CREATE) 6b. Change Tracking Pattern logaction( "MEMBERS", "MODIFY", $borrowernumber, to_json($info, { utf8 => 1, pretty => 1, canonical => 1 }) ) Where $info contains: { "firstname": { "before": "John", "after": "Jane" }, "surname": { "before": "Doe", "after": "Smith" } } Modules: MEMBERS (MODIFY, MODIFY_CARDNUMBER) 6c. Attribute Changes logaction( "MEMBERS", "MODIFY", $borrowernumber, to_json({ "attribute.$code" => $change }, { pretty => 1, canonical => 1 }) ) Modules: MEMBERS (extended patron attributes) 6d. ILL Status Changes logaction( 'ILL', 'STATUS_CHANGE', $request_id, to_json({ log_origin => 'core', status_before => $old_status, status_after => $new_status }) ) Modules: ILL 6e. Acquisition Baskets logaction( 'ACQUISITIONS', 'APPROVE_BASKET', $basketno, to_json( $basket->unblessed ) ) Modules: ACQUISITIONS --- 7. Object Reference (Koha::Object) Usage: Pass entire object for before/after comparison logaction( 'HOLDS', 'MODIFY', $hold_id, $hold, undef, $original ) logaction( "CATALOGUING", "MODIFY", $itemnumber, $self, undef, $original ) Format: 4th parameter is new object, 6th parameter is original Modules: HOLDS, CATALOGUING (items), MEMBERS Display: Serialized to JSON by C4::Log internally --- 8. Text Content (Full documents) Usage: Stores entire content for comparison logaction( 'NOTICES', 'MODIFY', $notice_id, $notice_content ) logaction( 'NEWS', 'MODIFY', $news_id, $news_content ) Modules: NOTICES, NEWS, REPORTS Display: Special comparison UI with diff viewer --- 9. Descriptive Messages Usage: Human-readable action descriptions logaction( "FINES", "MODIFY", $borrowernumber, "Overdue forgiven: item $item" ) logaction( "MEMBERS", "PATRON_MERGE", $patron_id, "$old_name ($old_cardnumber) has been merged into $new_name" ) Modules: FINES, MEMBERS (PATRON_MERGE, CIRCMESSAGE actions) --- Summary Statistics From ~190 logaction calls analyzed: | Format | Count | Modules | |---------------------|-------|------------------------------------------------| | JSON (structured) | ~35% | CIRCULATION, HOLDS, MEMBERS, ILL, ACQUISITIONS | | Object Reference | ~25% | HOLDS, CATALOGUING, MEMBERS | | Empty String | ~15% | SERIAL, MEMBERS | | Concatenated String | ~10% | SYSTEMPREFERENCE, REPORTS, AUTH | | BEFORE/AFTER | ~5% | AUTHORITIES, CATALOGUING | | Simple Literal | ~5% | AUTHORITIES, CATALOGUING | | Descriptive Message | ~5% | FINES, MEMBERS, RECALLS |
I'm probably not the right person to test this one. If the results I got are what is expected, I'm happy to sign off. Testing notes (using KTD): 1. System prferences: - Enable HoldsLog - Enable RenewalLog - Enable AllowNotForLoanOverride - The IssueLog is already enabled 2. Items to use (all located in Centerville): - 39999000011111 - 39999000005776 - 39999000005134 - 39999000000498 - 39999000011418 3. Actions (before and after the patch) to see what is logged: - Check out an item to Mary Burton - Check out an item to Mary Burton, with the due date set before the current date - prompted to confirm - Place a hold for Mary Burton - Change an item for a record to Not for loan, attempt to issue, confirm override and check out - Renew an item 4. Before and after database and log viewer entries - SQL: select * from action_logs; Before ====== Database: | 17 | 2025-12-26 09:07:50 | 51 | CIRCULATION | ISSUE | 49 | { "branchcode" : "CPL", "confirmations" : [], "forced" : [], "issue" : 1, "itemnumber" : 563 } | intranet | NULL | NULL | NULL | | 18 | 2025-12-26 09:08:08 | 51 | CIRCULATION | ISSUE | 49 | { "branchcode" : "CPL", "confirmations" : [ "INVALID_DATE" ], "forced" : [], "issue" : 2, "itemnumber" : 296 } | intranet | NULL | NULL | NULL | | 19 | 2025-12-26 09:08:39 | 51 | HOLDS | CREATE | 1 | 1 | api | NULL | NULL | NULL | | 23 | 2025-12-26 09:28:52 | 51 | CIRCULATION | ISSUE | 49 | { "branchcode" : "CPL", "confirmations" : [ "NOT_FOR_LOAN_FORCING" ], "forced" : [], "issue" : 3, "itemnumber" : 32 } | intranet | NULL | NULL | NULL | | 24 | 2025-12-26 09:29:32 | 51 | CIRCULATION | RENEWAL | 49 | { "confirmations" : [], "forced" : [], "issue" : 2, "itemnumber" : "296" } | intranet | NULL | NULL | NULL | How they show in the log viewer: ... Circulation Checkout Mary Burton (49) Item 39999000011111 CPL Staff interface ... Circulation Checkout Mary Burton (49) Item 39999000005776 CPL Invalid date confirmed Staff interface ... Holds Create 1 1 REST API ... Circulation Checkout Mary Burton (49) Item 39999000000498 CPL Item was not for loan Staff interface ... Circulation Renew Mary Burton (49) Item 39999000005776 CPL Staff interface After ===== Database: | 27 | 2025-12-26 09:38:09 | 51 | CIRCULATION | ISSUE | 49 | { "branchcode" : "CPL", "confirmations" : [], "forced" : [], "issue" : 4, "itemnumber" : 578 } | intranet | NULL | NULL | NULL | | 31 | 2025-12-26 09:40:52 | 51 | CIRCULATION | ISSUE | 49 | { "branchcode" : "CPL", "confirmations" : [ "INVALID_DATE" ], "forced" : [], "issue" : 6, "itemnumber" : 264 } | intranet | NULL | NULL | NULL | | 29 | 2025-12-26 09:39:04 | 51 | HOLDS | CREATE | 2 | { "biblionumber" : "19", "branchcode" : "FPL", "confirmations" : [], "forced" : [], "hold" : 2, "itemnumber" : null } | intranet | NULL | NULL | NULL | | 28 | 2025-12-26 09:38:31 | 51 | CIRCULATION | ISSUE | 49 | { "branchcode" : "CPL", "confirmations" : [ "INVALID_DATE", "NOT_FOR_LOAN_FORCING" ], "forced" : [], "issue" : 5, "itemnumber" : 32 } | intranet | NULL | NULL | NULL | | 34 | 2025-12-26 09:41:09 | 51 | CIRCULATION | RENEWAL | 49 | { "confirmations" : [], "forced" : [], "issue" : 4, "itemnumber" : "578" } | intranet | NULL | NULL | NULL | How they show in the log viewer: ...Circulation Checkout Mary Burton (49) Item 39999000011418 CPL Staff interface ...Circulation Checkout Mary Burton (49) Item 39999000005134 CPL Invalid date confirmed Staff interface ...Holds Create 2 { "biblionumber" : "19", "branchcode" : "FPL", "confirmations" : [], "forced" : [], "hold" : 2, "itemnumber" : null } Staff interface ...Circulation Checkout Mary Burton (49) Item 39999000000498 CPL Invalid date confirmed Item was not for loan Staff interface ...Circulation Renew Mary Burton (49) Item 39999000005134 CPL Staff interface Tests (these pass) ===== prove t/db_dependent/Circulation.t prove t/db_dependent/Reserves.t Migration script ================ perl misc/maintenance/migrate_action_logs_to_json.pl --help perl misc/maintenance/migrate_action_logs_to_json.pl --verbose perl misc/maintenance/migrate_action_logs_to_json.pl --commit perl misc/maintenance/migrate_action_logs_to_json.pl -c Starting migration of action logs to JSON format... COMMIT MODE - Changes will be saved Found 0 CIRCULATION ISSUE log entries to migrate Found 1 HOLDS CREATE log entries to migrate Total: 1 entries Completed CIRCULATION ISSUE: 0 converted, 0 errors === Processing HOLDS CREATE logs === Processed 1000 / 1 records... Completed HOLDS CREATE: 1 converted, 0 errors === Migration Summary === CIRCULATION ISSUE: Converted: 0 Errors: 0 HOLDS CREATE: Converted: 1 Errors: 0 TOTAL: Converted: 1 Errors: 0