Bugzilla – Attachment 194247 Details for
Bug 17387
Add an undelete feature for items/biblios
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 17387: (follow-up) address review feedback
93c103a.patch (text/plain), 17.38 KB, created by
Martin Renvoize (ashimema)
on 2026-03-01 17:06:30 UTC
(
hide
)
Description:
Bug 17387: (follow-up) address review feedback
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2026-03-01 17:06:30 UTC
Size:
17.38 KB
patch
obsolete
>From 93c103a499689f8fe773cf26a5ebcd7355918d4f Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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 <michaela.sieber@kit.edu> >Sponsored-by: ByWater Solutions >Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> >--- > 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 @@ > </ul> > [% 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 ) %] > <h5>Batch editing</h5> > <ul> > [% 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 @@ > <span class="sub_permission records_batchmod_subpermission"> Perform batch modification of records (biblios or authorities) </span> > <span class="permissioncode">([% name | html %])</span> > [%- CASE 'records_restore' -%] >- <span class="sub_permission records_restore_subpermission"> Restore deleted records </span> >+ <span class="sub_permission records_restore_subpermission"> Restore deleted bibliographic records and items </span> > <span class="permissioncode">([% name | html %])</span> > [%- CASE 'rotating_collections' -%] > <span class="sub_permission rotating_collections_subpermission"> Manage rotating collections </span> >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 = '<input type="checkbox" class="item-checkbox" data-item-id="' + row.item_id + '"' + disabled + '>'; >+ var preselected = (preselectedItemId && row.item_id == preselectedItemId && canEdit) ? ' checked' : ''; >+ var checkbox = '<input type="checkbox" class="item-checkbox" data-item-id="' + row.item_id + '"' + disabled + preselected + '>'; > > if (!canEdit) { > return '<span title="' + _("You do not have permission to restore items from this library") + '">' + checkbox + '</span>'; >@@ -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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 17387
:
188670
|
188671
|
188672
|
188673
|
188674
|
188675
|
188676
|
188677
|
188678
|
188679
|
188680
|
188681
|
188682
|
188683
|
188684
|
188685
|
188686
|
188687
|
188688
|
188689
|
188690
|
188691
|
188692
|
188693
|
188694
|
188695
|
188696
|
188697
|
188698
|
188699
|
188700
|
188701
|
188780
|
188781
|
188782
|
188783
|
188784
|
188785
|
188786
|
188787
|
188788
|
188789
|
188790
|
188791
|
188792
|
188793
|
188794
|
188795
|
189046
|
189047
|
189048
|
189049
|
189050
|
189051
|
189052
|
189053
|
189054
|
189055
|
189056
|
189057
|
189058
|
189059
|
189060
|
189061
|
189062
|
189082
|
189083
|
189084
|
189085
|
189086
|
189087
|
189088
|
189089
|
189090
|
189091
|
189092
|
189093
|
189094
|
189095
|
189096
|
189097
|
189098
|
189099
|
189100
|
189101
|
189102
|
189103
|
189104
|
189105
|
189106
|
189107
|
189108
|
189109
|
189110
|
189111
|
189112
|
189113
|
189114
|
189115
|
189116
|
189117
|
189118
|
189119
|
194228
|
194229
|
194230
|
194231
|
194232
|
194233
|
194234
|
194235
|
194236
|
194237
|
194238
|
194239
|
194240
|
194241
|
194242
|
194243
|
194244
|
194245
|
194246
| 194247 |
194248