From 93c103a499689f8fe773cf26a5ebcd7355918d4f Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Sun, 1 Mar 2026 16:48:27 +0000 Subject: [PATCH] Bug 17387: (follow-up) address review feedback - Fix permission description to explicitly mention bibliographic records and items (not just 'records') - Fix date filter to use correct DB column per table: deletedbiblio uses 'timestamp', deleteditems uses 'deleted_on' - Remove leftover CAN_user_editcatalogue_edit_catalogue condition from the Batch editing section in cat-menu.inc; a user with only edit_catalogue would see an empty section heading - Wrap biblio and item restore operations in DB transactions so that a failed store cannot leave a record inserted without being removed from the deleted table (or vice-versa) - Catch Koha::Exceptions::Object::DuplicateID in the item restore controller and return a human-readable 409 instead of a generic 500 - When 'Restore bibliographic record' is clicked in the deleted-bib warning modal, fetch the biblio and open the full restore modal with the originally clicked item pre-selected, rather than restoring immediately without item selection Signed-off-by: Michaela Sponsored-by: ByWater Solutions Signed-off-by: Martin Renvoize --- Koha/Old/Biblio.pm | 49 ++++++------ Koha/Old/Item.pm | 14 +++- Koha/REST/V1/DeletedItems.pm | 7 ++ .../data/mysql/mandatory/userpermissions.sql | 2 +- .../prog/en/includes/cat-menu.inc | 2 +- .../prog/en/includes/permissions.inc | 2 +- .../prog/en/modules/tools/restore-records.tt | 75 ++++++++++--------- 7 files changed, 89 insertions(+), 62 deletions(-) diff --git a/Koha/Old/Biblio.pm b/Koha/Old/Biblio.pm index a8fd3be7193..3bff29ff60e 100644 --- a/Koha/Old/Biblio.pm +++ b/Koha/Old/Biblio.pm @@ -181,35 +181,42 @@ sub restore { $params //= {}; - my $biblio_data = $self->unblessed; - my $biblioitem = $self->biblioitem; - my $biblioitem_data = $biblioitem->unblessed; - my $metadata = $self->metadata; - my $metadata_data = $metadata->unblessed; - - my $new_biblio = Koha::Biblio->new($biblio_data)->store; - - $biblioitem_data->{biblionumber} = $new_biblio->biblionumber; - $biblioitem_data->{biblioitemnumber} = $new_biblio->biblionumber; - Koha::Biblioitem->new($biblioitem_data)->store; - - delete $metadata_data->{id}; - $metadata_data->{biblionumber} = $new_biblio->biblionumber; - Koha::Biblio::Metadata->new($metadata_data)->store; - - $metadata->delete; - $biblioitem->delete; - $self->delete; - my $patron = $params->{patron}; my $item_ids = $params->{item_ids}; my $restore_all = $params->{restore_all} // 0; + my $biblionumber = $self->biblionumber; my @restored_items; my @skipped_items; + my $new_biblio = Koha::Database->schema->txn_do( + sub { + my $biblio_data = $self->unblessed; + my $biblioitem = $self->biblioitem; + my $biblioitem_data = $biblioitem->unblessed; + my $metadata = $self->metadata; + my $metadata_data = $metadata->unblessed; + + my $biblio = Koha::Biblio->new($biblio_data)->store; + + $biblioitem_data->{biblionumber} = $biblio->biblionumber; + $biblioitem_data->{biblioitemnumber} = $biblio->biblionumber; + Koha::Biblioitem->new($biblioitem_data)->store; + + delete $metadata_data->{id}; + $metadata_data->{biblionumber} = $biblio->biblionumber; + Koha::Biblio::Metadata->new($metadata_data)->store; + + $metadata->delete; + $biblioitem->delete; + $self->delete; + + return $biblio; + } + ); + if ( $restore_all || $item_ids ) { - my $deleted_items = $self->items; + my $deleted_items = Koha::Old::Items->search( { biblionumber => $biblionumber } ); while ( my $deleted_item = $deleted_items->next ) { my $should_restore = 0; diff --git a/Koha/Old/Item.pm b/Koha/Old/Item.pm index 12b0a8b2ff8..ba675f79939 100644 --- a/Koha/Old/Item.pm +++ b/Koha/Old/Item.pm @@ -23,6 +23,7 @@ use C4::Context; use C4::Log qw( logaction ); use Koha::Biblio; use Koha::Biblios; +use Koha::Database; use Koha::Exceptions; use Koha::Item; use Koha::Old::Biblios; @@ -78,12 +79,17 @@ sub restore { Koha::Exceptions::ObjectNotFound->throw("Bibliographic record not found for item") unless $biblio; - my $item_data = $self->unblessed; - delete $item_data->{deleted_on}; + my $new_item = Koha::Database->schema->txn_do( + sub { + my $item_data = $self->unblessed; + delete $item_data->{deleted_on}; - my $new_item = Koha::Item->new($item_data)->store; + my $item = Koha::Item->new($item_data)->store; + $self->delete; - $self->delete; + return $item; + } + ); my $indexer = Koha::SearchEngine::Indexer->new( { index => $Koha::SearchEngine::BIBLIOS_INDEX } ); $indexer->index_records( $new_item->biblionumber, "specialUpdate", "biblioserver" ); diff --git a/Koha/REST/V1/DeletedItems.pm b/Koha/REST/V1/DeletedItems.pm index a6f297b396a..04eda65d0ce 100644 --- a/Koha/REST/V1/DeletedItems.pm +++ b/Koha/REST/V1/DeletedItems.pm @@ -20,6 +20,7 @@ use Modern::Perl; use Mojo::Base 'Mojolicious::Controller'; use Koha::Exceptions; +use Koha::Exceptions::Object; use Koha::Old::Items; use Scalar::Util qw( blessed ); @@ -110,6 +111,12 @@ sub restore { } ); } + if ( blessed $_ && $_->isa('Koha::Exceptions::Object::DuplicateID') ) { + return $c->render( + status => 409, + openapi => { error => "Cannot restore item: another item with the same barcode already exists." } + ); + } $c->unhandled_exception($_); }; } diff --git a/installer/data/mysql/mandatory/userpermissions.sql b/installer/data/mysql/mandatory/userpermissions.sql index c1d5291cecc..bd9e2b373d8 100644 --- a/installer/data/mysql/mandatory/userpermissions.sql +++ b/installer/data/mysql/mandatory/userpermissions.sql @@ -132,7 +132,7 @@ INSERT INTO permissions (module_bit, code, description) VALUES (13, 'records_batchmod', 'Perform batch modification of records (biblios or authorities)'), (13, 'marc_modification_templates', 'Manage marc modification templates'), (13, 'records_batchdel', 'Perform batch deletion of records (bibliographic or authority)'), - (13, 'records_restore', 'Restore deleted records'), + (13, 'records_restore', 'Restore deleted bibliographic records and items'), (13, 'access_files', 'Access to the files stored on the server'), (13, 'upload_general_files', 'Upload any file'), (13, 'upload_manage', 'Manage uploaded files'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/cat-menu.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/cat-menu.inc index 8839186e6e0..5ebcbdc3297 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/cat-menu.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/cat-menu.inc @@ -43,7 +43,7 @@ [% END %] - [% IF ( CAN_user_tools_items_batchmod || CAN_user_tools_items_batchdel || CAN_user_tools_records_batchmod || CAN_user_tools_records_batchdel || CAN_user_tools_records_restore || CAN_user_tools_marc_modification_templates || CAN_user_editcatalogue_edit_catalogue ) %] + [% IF ( CAN_user_tools_items_batchmod || CAN_user_tools_items_batchdel || CAN_user_tools_records_batchmod || CAN_user_tools_records_batchdel || CAN_user_tools_records_restore || CAN_user_tools_marc_modification_templates ) %]
Batch editing
    [% IF ( CAN_user_tools_items_batchmod ) %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc index abd49440b2d..ab26bb19b94 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc @@ -461,7 +461,7 @@ Perform batch modification of records (biblios or authorities) ([% name | html %]) [%- CASE 'records_restore' -%] - Restore deleted records + Restore deleted bibliographic records and items ([% name | html %]) [%- CASE 'rotating_collections' -%] Manage rotating collections diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/restore-records.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/restore-records.tt index 6bc918b4f41..607d05415f6 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/restore-records.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/restore-records.tt @@ -216,7 +216,7 @@ } }, 100); - function buildApiUrl(baseUrl) { + function buildApiUrl(baseUrl, dateField) { var from = $("#from").val(); var to = $("#to").val(); @@ -226,15 +226,15 @@ var query = {}; if (from && to) { - query["me.timestamp"] = { + query[dateField] = { "-between": [from + "T00:00:00Z", to + "T23:59:59Z"], }; } else if (from) { - query["me.timestamp"] = { + query[dateField] = { ">=": from + "T00:00:00Z", }; } else if (to) { - query["me.timestamp"] = { + query[dateField] = { "<=": to + "T23:59:59Z", }; } @@ -245,7 +245,7 @@ // Deleted biblios DataTable var biblios_table = $("#deleted_biblios_table").kohaTable({ ajax: { - url: buildApiUrl("/api/v1/deleted/biblios"), + url: buildApiUrl("/api/v1/deleted/biblios", "me.timestamp"), }, embed: "items", order: [[3, "desc"]], @@ -313,7 +313,7 @@ // Deleted items DataTable var items_table = $("#deleted_items_table").kohaTable({ ajax: { - url: buildApiUrl("/api/v1/deleted/items"), + url: buildApiUrl("/api/v1/deleted/items", "me.deleted_on"), }, embed: "biblio", order: [[5, "desc"]], @@ -405,30 +405,20 @@ var modal_items_table = null; var modal_items_table_api = null; - // Restore biblio handler - $("#deleted_biblios_table").on("click", ".restore-biblio", function (e) { - e.preventDefault(); - var button = $(this); - var biblio_id = button.data("biblio-id"); - var title = button.data("title"); - var row = biblios_table_api.row(button.closest("tr")).data(); - - // Populate modal with biblio info + // Populate and show the restore biblio modal. + // preselectedItemId: optional item_id to pre-check in the items list. + function showRestoreBiblioModal(biblio_id, title, author, items, preselectedItemId) { $("#restore-modal-biblio-id").text(biblio_id); $("#restore-modal-biblio-title").text(title || _("(No title)")); - $("#restore-modal-biblio-author").text(row.author || _("(No author)")); + $("#restore-modal-biblio-author").text(author || _("(No author)")); - // Check if there are deleted items - var items = row.items || []; if (items.length > 0) { $("#items-section").show(); - // Destroy existing DataTable if it exists if (modal_items_table_api) { modal_items_table_api.destroy(); } - // Initialize kohaTable with the items data modal_items_table = $("#deleted-items-list").kohaTable({ data: items, paging: false, @@ -439,7 +429,8 @@ if (type === "display") { var canEdit = canEditItem(row.home_library_id); var disabled = canEdit ? '' : ' disabled'; - var checkbox = ''; + var preselected = (preselectedItemId && row.item_id == preselectedItemId && canEdit) ? ' checked' : ''; + var checkbox = ''; if (!canEdit) { return '' + checkbox + ''; @@ -501,12 +492,21 @@ $("#items-section").hide(); } - // Store biblio data $("#restore-biblio-with-items-button").data("biblio-id", biblio_id); - $("#restore-biblio-with-items-button").data("restore-button", button); var modal = new bootstrap.Modal(document.getElementById("restoreBiblioModal")); modal.show(); + } + + // Restore biblio handler (from deleted biblios table) + $("#deleted_biblios_table").on("click", ".restore-biblio", function (e) { + e.preventDefault(); + var button = $(this); + var biblio_id = button.data("biblio-id"); + var row = biblios_table_api.row(button.closest("tr")).data(); + + $("#restore-biblio-with-items-button").data("restore-button", button); + showRestoreBiblioModal(biblio_id, row.title, row.author, row.items || []); }); // Restore item handler @@ -561,30 +561,37 @@ }); }); - // Restore biblio from modal + // "Restore bibliographic record" button in the deleted-bib warning modal: + // fetch the biblio (with items) and open the full restore modal with the + // originally clicked item pre-selected. $("#restore-biblio-button").on("click", function () { var button = $(this); var biblio_id = button.data("biblio-id"); - var item_id = button.data("item-id"); + var original_item_id = button.data("item-id"); var item_button = button.data("item-button"); button.prop("disabled", true); $.ajax({ url: "/api/v1/deleted/biblios/" + biblio_id, - type: "PUT", + type: "GET", headers: { - "x-koha-request-id": Math.random(), + "x-koha-embed": "items", }, - success: function (data) { - showMessage(_("Bibliographic record %s restored successfully").format(biblio_id), "success"); + success: function (biblio) { bootstrap.Modal.getInstance(document.getElementById("deletedBiblioModal")).hide(); - biblios_table_api.ajax.reload(); - items_table_api.ajax.reload(); + $("#restore-biblio-with-items-button").data("restore-button", item_button); + showRestoreBiblioModal( + biblio.biblio_id, + biblio.title, + biblio.author, + biblio.items || [], + original_item_id + ); button.prop("disabled", false); }, error: function (xhr) { - var error_msg = _("Error restoring bibliographic record %s").format(biblio_id); + var error_msg = _("Error loading bibliographic record %s").format(biblio_id); if (xhr.responseJSON && xhr.responseJSON.error) { error_msg += ": " + xhr.responseJSON.error; } @@ -678,8 +685,8 @@ }); $("#filter_table").on("click", function () { - biblios_table_api.ajax.url(buildApiUrl("/api/v1/deleted/biblios")).load(); - items_table_api.ajax.url(buildApiUrl("/api/v1/deleted/items")).load(); + biblios_table_api.ajax.url(buildApiUrl("/api/v1/deleted/biblios", "me.timestamp")).load(); + items_table_api.ajax.url(buildApiUrl("/api/v1/deleted/items", "me.deleted_on")).load(); }); $("#clear_filters").on("click", function () { -- 2.53.0