From 54c67a39137114db59d7a0bb014f63b45f07ecc5 Mon Sep 17 00:00:00 2001 From: Matt Blenkinsop Date: Tue, 21 Jul 2026 10:20:34 +0100 Subject: [PATCH] Bug 43119: Restore item and hold links to the log viewer INSERT INTO action_logs (timestamp, user, module, action, object, info, interface) VALUES (NOW(), 5, 'CIRCULATION', 'ISSUE', 5, '{"issue":1,"branchcode":"CPL","itemnumber":1,"confirmations":["TOO_MANY"],"forced":["DEBT"]}', 'intranet'), (NOW(), 5, 'CIRCULATION', 'RETURN', 5, '1', 'intranet'), (NOW(), 5, 'HOLDS', 'CREATE', 7, '{"hold":7,"biblionumber":1,"itemnumber":1,"confirmations":["HIGHHOLDS"],"forced":["HOLD_POLICY_OVERRIDE"]}', 'intranet'); Test plan: 1) Without the patch applied, run the populate SQL above against the kohadev database. 2) Go to Tools > Log viewer 3) Under Modules, select only Circulation - leave the date range empty and click Submit. 4) Observe the ISSUE row's Info column shows the raw JSON payload as plain text, and the RETURN row shows the bare itemnumber "1" as plain text - neither is a clickable link to the item. 5) Go back, select only the Holds module, click Submit, and observe the CREATE row's Info column likewise shows raw JSON with no hold/item links. 6) Apply the patch. 7) Run yarn build && restart_all, then hard refresh the browser. 8) Repeat step 3. 9) Confirm the ISSUE row's Info column now shows a link reading "Item CPL" (e.g. Item 3999900000001 CPL) - the branchcode comes from the JSON payload - and clicking it opens the item page. Override information is also displayed correctly 10) Confirm the RETURN row's Info column now shows a link reading "Item " (no branchcode) opening the same item's detail page - verifying the bare-itemnumber payload path. 11) Repeat step 5. Confirm the CREATE row now shows "Hold 7 on biblio 1" linking to reserve/request.pl?biblionumber=1, followed by " - Item 1" linking to the item detail page. Override information is also displayed correctly Signed-off-by: Lucas Gass Signed-off-by: Lisette Scheer Signed-off-by: Lucas Gass --- Koha/ActionLog.pm | 42 ++++++- api/v1/swagger/definitions/action_log.yaml | 8 ++ api/v1/swagger/paths/action_logs.yaml | 1 + koha-tmpl/intranet-tmpl/prog/js/viewlog.js | 124 ++++++++++++++++++++- t/db_dependent/api/v1/action_logs.t | 59 +++++++++- 5 files changed, 225 insertions(+), 9 deletions(-) diff --git a/Koha/ActionLog.pm b/Koha/ActionLog.pm index 410a46e09e..faf4f9fc95 100644 --- a/Koha/ActionLog.pm +++ b/Koha/ActionLog.pm @@ -19,12 +19,14 @@ package Koha::ActionLog; use Modern::Perl; +use JSON qw( decode_json ); + use C4::Context; # Avoid `use Koha::Patrons` here: Koha::ActionLog is loaded transitively by # C4::Log very early in the dependency chain, before Koha::Patron has been -# fully defined. The accessors below reach for Koha::Patron at call time, -# by which point it is reliably available. +# fully defined. The accessors below reach for Koha::Patron (and Koha::Items) +# at call time, by which point they are reliably available. use base qw(Koha::Object); @@ -70,6 +72,42 @@ sub patron { return Koha::Patron->_new_from_dbic($rs); } +=head3 item + + my $item = $log->item; + +Returns the related I for a CIRCULATION log entry, or I. + +The itemnumber is not stored in a dedicated column; it lives inside the +C field, which for circulation is either a JSON payload carrying an +C key (ISSUE, RENEWAL) or a bare itemnumber string (RETURN). +Returns I for non-circulation rows, when no itemnumber can be +derived, or when the item has since been deleted. + +=cut + +sub item { + my ($self) = @_; + + return unless ( $self->module // q{} ) eq 'CIRCULATION'; + + my $info = $self->info; + return unless defined $info; + + my $itemnumber; + my $decoded = eval { decode_json($info) }; + if ( ref($decoded) eq 'HASH' && $decoded->{itemnumber} ) { + $itemnumber = $decoded->{itemnumber}; + } elsif ( $info =~ /^\s*(\d+)\s*$/ ) { + $itemnumber = $1; + } + + return unless $itemnumber; + + require Koha::Items; + return Koha::Items->find($itemnumber); +} + =head2 Internal methods =head3 _type diff --git a/api/v1/swagger/definitions/action_log.yaml b/api/v1/swagger/definitions/action_log.yaml index 719a2622d1..ac0c5eb8fc 100644 --- a/api/v1/swagger/definitions/action_log.yaml +++ b/api/v1/swagger/definitions/action_log.yaml @@ -67,4 +67,12 @@ properties: Patron object joined on `object` (only meaningful for MEMBERS, CIRCULATION, FINES and APIKEYS rows; only present if requested via x-koha-embed) + item: + type: + - object + - "null" + description: | + Item object derived from the itemnumber carried in `info` (only + meaningful for CIRCULATION rows; only present if requested via + x-koha-embed) additionalProperties: false diff --git a/api/v1/swagger/paths/action_logs.yaml b/api/v1/swagger/paths/action_logs.yaml index abbe580916..15606a8308 100644 --- a/api/v1/swagger/paths/action_logs.yaml +++ b/api/v1/swagger/paths/action_logs.yaml @@ -67,6 +67,7 @@ enum: - librarian - patron + - item collectionFormat: csv responses: "200": diff --git a/koha-tmpl/intranet-tmpl/prog/js/viewlog.js b/koha-tmpl/intranet-tmpl/prog/js/viewlog.js index 4fb0c964e6..522a9e34aa 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/viewlog.js +++ b/koha-tmpl/intranet-tmpl/prog/js/viewlog.js @@ -124,6 +124,109 @@ function renderCircInfo(raw) { return html; } +/** + * Parse a log `info` payload as a JSON object, returning the parsed object + * or null when it isn't a JSON object. + */ +function parseInfoObject(raw) { + if (!raw) return null; + const trimmed = raw.replace(/^\s+|\s+$/g, ""); + if (trimmed.charAt(0) !== "{") return null; + try { + const parsed = JSON.parse(trimmed); + return parsed && typeof parsed === "object" ? parsed : null; + } catch (e) { + return null; + } +} + +/** + * Build the "Item " link for a CIRCULATION log entry. The item is + * embedded on the row based on the itemnumber carried in `info`. For JSON + * payloads (ISSUE/RENEWAL) the item's branchcode is appended after the barcode, + * Returns the anchor markup wrapped in a
, or null when no item is + * embedded (e.g. the item has since been deleted). + */ +function renderCircItemLink(row) { + const item = row.item; + if (!item) return null; + + const itemnumber = item.item_id; + const barcode = item.external_id != null ? item.external_id : itemnumber; + const biblionumber = item.biblio_id; + + const parsed = parseInfoObject(row.info); + const branchcode = parsed && parsed.branchcode ? parsed.branchcode : null; + + const encodedItem = encodeURIComponent(itemnumber); + const encodedBiblio = encodeURIComponent(biblionumber); + const label = branchcode + ? __("Item %s %s").format(escapeHtml(barcode), escapeHtml(branchcode)) + : __("Item %s").format(escapeHtml(barcode)); + + return ( + "
" + + '' + + label + + "" + + "
" + ); +} + +/** + * Build the hold link (and optional item link) for a HOLDS log entry whose + * `info` payload is a JSON object carrying `hold` and `biblionumber`. + * Returns the markup wrapped in a
, or null. + */ +function renderHoldsInfoLink(row) { + const parsed = parseInfoObject(row.info); + if (!parsed || parsed.hold == null || parsed.biblionumber == null) { + return null; + } + + const encodedBiblio = encodeURIComponent(parsed.biblionumber); + let html = + '' + + __("Hold %s on biblio %s").format( + escapeHtml(parsed.hold), + escapeHtml(parsed.biblionumber) + ) + + ""; + + if (parsed.itemnumber != null) { + const encodedItem = encodeURIComponent(parsed.itemnumber); + html += + " - " + + '' + + __("Item %s").format(escapeHtml(parsed.itemnumber)) + + ""; + } + + return "
" + html + "
"; +} + /** * Render Struct::Diff JSON as a human-readable before/after table. * Falls back to a 
 block for non-Struct::Diff JSON or plain text.
@@ -393,21 +496,30 @@ function renderInfo(data, type, row) {
         );
     }
 
-    var body = null;
-    if (mod == "CIRCULATION" || mod == "HOLDS") {
+    let prefix = "";
+    let body = null;
+    if (mod == "CIRCULATION") {
+        prefix = renderCircItemLink(row) || "";
+        body = renderCircInfo(info);
+    } else if (mod == "HOLDS") {
+        prefix = renderHoldsInfoLink(row) || "";
         body = renderCircInfo(info);
     }
-    if (body === null) {
+    // Only fall back to the record/plain-text renderers when we produced
+    // neither a link nor any override text, so we never dump the raw JSON
+    // payload alongside a rendered item/hold link.
+    if (body === null && prefix === "") {
         body = renderRecordInfo(info);
     }
-    if (body === null) {
+    if (body === null && prefix === "") {
         body = escapeHtml(info);
     }
     return (
         '
' +
-        body +
+        prefix +
+        (body || "") +
         "
"
     );
 }
@@ -567,7 +679,7 @@ $(document).ready(function () {
                 ajax: {
                     url: "/api/v1/action_logs",
                 },
-                embed: ["librarian", "patron"],
+                embed: ["librarian", "patron", "item"],
                 order: [[0, "desc"]],
                 pagingType: "full",
                 autoWidth: false,
diff --git a/t/db_dependent/api/v1/action_logs.t b/t/db_dependent/api/v1/action_logs.t
index 7bb73ba4fe..f4a1316638 100755
--- a/t/db_dependent/api/v1/action_logs.t
+++ b/t/db_dependent/api/v1/action_logs.t
@@ -18,7 +18,7 @@
 use Modern::Perl;
 
 use Test::NoWarnings;
-use Test::More tests => 6;
+use Test::More tests => 7;
 use Test::Mojo;
 
 use Mojo::JSON qw( encode_json );
@@ -193,6 +193,63 @@ subtest 'embeds' => sub {
     $schema->storage->txn_rollback;
 };
 
+subtest 'item embed (CIRCULATION)' => sub {
+    plan tests => 14;
+    $schema->storage->txn_begin;
+
+    my ( $librarian, $patron, $password ) = build_fixtures();
+    my $userid = $librarian->userid;
+    my $auth   = { Authorization => _basic( $userid, $password ) };
+
+    my $item = $builder->build_sample_item;
+
+    # ISSUE/RENEWAL log a JSON payload carrying the itemnumber; RETURN logs a
+    # bare itemnumber string. Both must resolve to the same embedded item.
+    my $json_info = encode_json(
+        {
+            issue         => 1,
+            branchcode    => $item->homebranch,
+            itemnumber    => $item->itemnumber,
+            confirmations => [],
+            forced        => [],
+        }
+    );
+    logaction( 'CIRCULATION', 'ISSUE',  $patron->borrowernumber, $json_info,        'INTRANET' );
+    logaction( 'CIRCULATION', 'RETURN', $patron->borrowernumber, $item->itemnumber, 'INTRANET' );
+    logaction( 'MEMBERS',     'MODIFY', $patron->borrowernumber, 'no item here',    'INTRANET' );
+
+    # JSON payload (ISSUE): item resolved from info.itemnumber
+    $t->get_ok( "/api/v1/action_logs?module=CIRCULATION&action=ISSUE" => { 'x-koha-embed' => 'item', %$auth } )
+        ->status_is(200);
+    my $issue = $t->tx->res->json->[0];
+    is( $issue->{item}->{item_id},     $item->itemnumber,   'ISSUE: embedded item_id from JSON info' );
+    is( $issue->{item}->{external_id}, $item->barcode,      'ISSUE: embedded barcode (external_id)' );
+    is( $issue->{item}->{biblio_id},   $item->biblionumber, 'ISSUE: embedded biblio_id' );
+
+    # Bare itemnumber payload (RETURN)
+    $t->get_ok( "/api/v1/action_logs?module=CIRCULATION&action=RETURN" => { 'x-koha-embed' => 'item', %$auth } )
+        ->status_is(200);
+    is( $t->tx->res->json->[0]->{item}->{external_id}, $item->barcode, 'RETURN: item resolved from bare itemnumber' );
+
+    # Non-CIRCULATION row: item is null even when the embed is requested
+    $t->get_ok( "/api/v1/action_logs?module=MEMBERS" => { 'x-koha-embed' => 'item', %$auth } )->status_is(200);
+    is( $t->tx->res->json->[0]->{item}, undef, 'non-CIRCULATION row has null item' );
+
+    # Deleted item: CIRCULATION row whose itemnumber no longer exists -> null
+    my $gone    = $builder->build_sample_item;
+    my $gone_id = $gone->itemnumber;
+    $gone->delete;
+    logaction(
+        'CIRCULATION', 'RENEWAL', $patron->borrowernumber, encode_json( { itemnumber => $gone_id } ),
+        'INTRANET'
+    );
+    $t->get_ok( "/api/v1/action_logs?module=CIRCULATION&action=RENEWAL" => { 'x-koha-embed' => 'item', %$auth } )
+        ->status_is(200);
+    is( $t->tx->res->json->[0]->{item}, undef, 'deleted item resolves to null' );
+
+    $schema->storage->txn_rollback;
+};
+
 subtest 'pagination and sorting' => sub {
     plan tests => 6;
     $schema->storage->txn_begin;