Bugzilla – Attachment 193818 Details for
Bug 41882
Batch hold modification tool updates pickup locations to disallowed libraries
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 41882: Enfore pickup location rules
Bug-41882-Enfore-pickup-location-rules.patch (text/plain), 17.76 KB, created by
Lucas Gass (lukeg)
on 2026-02-24 22:36:10 UTC
(
hide
)
Description:
Bug 41882: Enfore pickup location rules
Filename:
MIME Type:
Creator:
Lucas Gass (lukeg)
Created:
2026-02-24 22:36:10 UTC
Size:
17.76 KB
patch
obsolete
>From a94fe0a8e51126bb43bfb806e43018608d8cc0da Mon Sep 17 00:00:00 2001 >From: Lucas Gass <lucas@bywatersolutions.com> >Date: Tue, 24 Feb 2026 22:20:20 +0000 >Subject: [PATCH] Bug 41882: Enfore pickup location rules > >To test: >1- Set ReservesControlBranch to 'item's home library' >2- Set Main library's 'Holds and bookings policies by item type' so that PASS items can only be picked up at Main >3- Place a hold on a bib whose only item is a PASS belonging to Main >4- Use the Batch hold modification tool to identify the hold you just placed >5- Update the pickup location to North. Note that the update is successful, and there's no indication that the pickup location is now a problem. >6- Check in Main's PASS, and note that the hold doesn't trigger to fill >7- APPLY PATCH, restart_all >8- Try again, this time the hold should fail to update at all. >9- You should see a new table with the 'Failed hold' in it, it should also include a 'Result' column which explains why the hold failed (New pickup location is invalid). >10- Try modifying a hold to an invalid pickup location, but also attempt to modify something else on the hold at the same time, like expiration date. >11- The hold should not modify anything. If the pickup location is invalid all other modifications should NOT happen on that hold. >12 - Play with the Table settings for the holds_modified tables, ensuring you can hide/show all the columns. >--- > admin/columns_settings.yml | 2 + > .../en/modules/tools/batch_modify_holds.tt | 192 ++++++++++++------ > tools/batch_modify_holds.pl | 71 +++++-- > 3 files changed, 186 insertions(+), 79 deletions(-) > >diff --git a/admin/columns_settings.yml b/admin/columns_settings.yml >index c249e0d0fda..100ec269d25 100644 >--- a/admin/columns_settings.yml >+++ b/admin/columns_settings.yml >@@ -2671,6 +2671,8 @@ modules: > columnname: suspended_until > - > columnname: notes >+ - >+ columnname: result > logviewer: > logst: > default_display_length: 20 >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/batch_modify_holds.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/batch_modify_holds.tt >index de32ade2238..da052274797 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/batch_modify_holds.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/batch_modify_holds.tt >@@ -103,63 +103,133 @@ > </div> > <!-- /#modify_holds_search --> > [% ELSIF view == "report" %] >- <h1>Modified holds</h1> >- <div id="modified_holds_results-wrapper"> >- <div id="modified_holds_results" class="page-section"> >- <table id="modified_holds"> >- <thead> >- <tr> >- <th>Title</th> >- <th>Patron</th> >- <th>Pickup location</th> >- <th>Priority</th> >- <th>Expiration date</th> >- <th>Status</th> >- <th>Suspended</th> >- <th>Suspended until</th> >- <th>Notes</th> >- <!-- Add other columns as needed --> >- </tr> >- </thead> >- <tbody> >- [% FOREACH hold IN updated_holds_obj %] >+ <h1>Modified holds report</h1> >+ [% IF updated_holds_obj %] >+ <div id="modified_holds_results-wrapper"> >+ <div id="modified_holds_results" class="page-section"> >+ <h2>Updated holds ([% total_updated | html %])</h2> >+ <table id="modified_holds"> >+ <thead> > <tr> >- <td>[% INCLUDE 'biblio-title.inc' biblio=hold.biblio link = 1 %]</td> >- <td>[% INCLUDE 'patron-title.inc' invert_name=1 patron=hold.borrower hide_patron_infos_if_needed=1 link_to="circulation_reserves" %]</td> >- <td>[% hold.branch.branchname | html %]</td> >- <td>[% hold.priority | html %]</td> >- <td>[% hold.expirationdate | $KohaDates %]</td> >- <td> >- [% IF hold.found == 'F' %] >- <span>Fulfilled</span> >- [% ELSIF hold.cancellationdate %] >- <span>Cancelled</span> >- [% ELSIF hold.found == 'W' %] >- [% IF hold.cancellation_requests.count == 0 %] >- <span>Waiting</span> >- [% ELSE %] >+ <th>Title</th> >+ <th>Patron</th> >+ <th>Pickup location</th> >+ <th>Priority</th> >+ <th>Expiration date</th> >+ <th>Status</th> >+ <th>Suspended</th> >+ <th>Suspended until</th> >+ <th>Notes</th> >+ <th>Result</th> >+ <!-- Add other columns as needed --> >+ </tr> >+ </thead> >+ <tbody> >+ [% FOREACH hold IN updated_holds_obj %] >+ <tr> >+ <td>[% INCLUDE 'biblio-title.inc' biblio=hold.biblio link = 1 %]</td> >+ <td>[% INCLUDE 'patron-title.inc' invert_name=1 patron=hold.borrower hide_patron_infos_if_needed=1 link_to="circulation_reserves" %]</td> >+ <td>[% hold.branch.branchname | html %]</td> >+ <td>[% hold.priority | html %]</td> >+ <td>[% hold.expirationdate | $KohaDates %]</td> >+ <td> >+ [% IF hold.found == 'F' %] >+ <span>Fulfilled</span> >+ [% ELSIF hold.cancellationdate %] > <span>Cancelled</span> >+ [% ELSIF hold.found == 'W' %] >+ [% IF hold.cancellation_requests.count == 0 %] >+ <span>Waiting</span> >+ [% ELSE %] >+ <span>Cancelled</span> >+ [% END %] >+ [% ELSIF hold.found == 'T' %] >+ <span>In transit</span> >+ [% ELSE %] >+ <span>Pending</span> > [% END %] >- [% ELSIF hold.found == 'T' %] >- <span>In transit</span> >- [% ELSE %] >- <span>Pending</span> >- [% END %] >- </td> >- <td>[% IF hold.suspend %]Yes[% ELSE %]No[% END %]</td> >- <td>[% IF hold.suspend_until %][% hold.suspend_until | $KohaDates %][% ELSE %]-[% END %]</td> >- <td>[% hold.reservenotes | html %]</td> >+ </td> >+ <td>[% IF hold.suspend %]Yes[% ELSE %]No[% END %]</td> >+ <td>[% IF hold.suspend_until %][% hold.suspend_until | $KohaDates %][% ELSE %]-[% END %]</td> >+ <td>[% hold.reservenotes | html %]</td> >+ <td><span class="badge rounded-pill bg-success">Success</span></td> >+ </tr> >+ [% END %] >+ </tbody> >+ </table> >+ </div> >+ <!-- /#modified_holds_results --> >+ </div> >+ <!-- /#modified_holds_results-wrapper --> >+ [% END %] >+ [% IF failed_holds_obj %] >+ <div id="failed_holds_results-wrapper"> >+ <div id="failed_holds_results" class="page-section"> >+ <h2>Failed holds ([% total_failed | html %])</h2> >+ <div class="alert alert-warning">Important: The following holds were not modified.</div> >+ <table id="failed_holds"> >+ <thead> >+ <tr> >+ <th>Title</th> >+ <th>Patron</th> >+ <th>Pickup location</th> >+ <th>Priority</th> >+ <th>Expiration date</th> >+ <th>Status</th> >+ <th>Suspended</th> >+ <th>Suspended until</th> >+ <th>Notes</th> >+ <th>Result</th> >+ <!-- Add other columns as needed --> > </tr> >- [% END %] >- </tbody> >- </table> >+ </thead> >+ <tbody> >+ [% FOREACH failure IN failed_holds_obj %] >+ [% SET hold = failure.hold %] >+ <tr> >+ <td>[% INCLUDE 'biblio-title.inc' biblio=hold.biblio link = 1 %]</td> >+ <td>[% INCLUDE 'patron-title.inc' invert_name=1 patron=hold.borrower hide_patron_infos_if_needed=1 link_to="circulation_reserves" %]</td> >+ <td>[% hold.branch.branchname | html %]</td> >+ <td>[% hold.priority | html %]</td> >+ <td>[% hold.expirationdate | $KohaDates %]</td> >+ <td> >+ [% IF hold.found == 'F' %] >+ <span>Fulfilled</span> >+ [% ELSIF hold.cancellationdate %] >+ <span>Cancelled</span> >+ [% ELSIF hold.found == 'W' %] >+ [% IF hold.cancellation_requests.count == 0 %] >+ <span>Waiting</span> >+ [% ELSE %] >+ <span>Cancelled</span> >+ [% END %] >+ [% ELSIF hold.found == 'T' %] >+ <span>In transit</span> >+ [% ELSE %] >+ <span>Pending</span> >+ [% END %] >+ </td> >+ <td>[% IF hold.suspend %]Yes[% ELSE %]No[% END %]</td> >+ <td>[% IF hold.suspend_until %][% hold.suspend_until | $KohaDates %][% ELSE %]-[% END %]</td> >+ <td>[% hold.reservenotes | html %]</td> >+ <td> >+ <span class="badge rounded-pill bg-warning text-dark">Failed</span> >+ [% IF failure.reason == 'pickupLocationNotAllowed' %] >+ New pickup location is invalid >+ [% END %] >+ </td> >+ </tr> >+ [% END %] >+ </tbody> >+ </table> >+ </div> >+ <!-- /#failed_holds_results --> > </div> >- <!-- /#modified_holds_results --> >- <fieldset class="action"> >- <a href="/cgi-bin/koha/tools/batch_modify_holds.pl">Return to batch hold modification</a> >- </fieldset> >- </div> >- <!-- /#modified_holds_results-wrapper --> >+ <!-- /#failed_holds_results-wrapper --> >+ [% END %] >+ <fieldset class="action"> >+ <a href="/cgi-bin/koha/tools/batch_modify_holds.pl">Return to batch hold modification</a> >+ </fieldset> > [% END %] > <div id="modify_holds_results-wrapper" style="display:none;"> > <div id="toolbar" class="btn-toolbar"> >@@ -642,12 +712,20 @@ > return true; > }); > >- //Modified holds table >+ //Modified holds table and failed holds table share table settings > var table_settings = [% TablesSettings.GetTableSettings( 'circ', 'holds', 'modified_holds', 'json' ) | $raw %]; >- $("#modified_holds").kohaTable({ >- pagingType: "full", >- autoWidth: false >- }, table_settings); >+ [% IF updated_holds_obj %] >+ $("#modified_holds").kohaTable({ >+ pagingType: "full", >+ autoWidth: false >+ }, table_settings); >+ [% END %] >+ [% IF failed_holds_obj %] >+ $("#failed_holds").kohaTable({ >+ pagingType: "full", >+ autoWidth: false >+ }, table_settings); >+ [% END %] > > }); > </script> >diff --git a/tools/batch_modify_holds.pl b/tools/batch_modify_holds.pl >index 4ba25f9cf40..2d7b5b41e3b 100755 >--- a/tools/batch_modify_holds.pl >+++ b/tools/batch_modify_holds.pl >@@ -59,52 +59,79 @@ if ( $op eq 'cud-form' ) { > my $clear_hold_notes = $input->param('clear_hold_notes'); > > @hold_ids = $input->multi_param('hold_id'); >- my @holds_data = (); >+ my @holds_data = (); >+ my @failed_holds = (); > > my $holds_to_update = > Koha::Holds->search( { reserve_id => { -in => \@hold_ids } }, { join => [ "item", "biblio" ] } ); > > while ( my $hold = $holds_to_update->next ) { >+ my $hold_failed = 0; >+ if ( $new_pickup_loc && ( $hold->branchcode ne $new_pickup_loc ) ) { >+ my $patron = $hold->patron; >+ my $item = $hold->item; >+ my $locations; > >- if ($new_expiration_date) { >- $hold->expirationdate($new_expiration_date)->store; >- } >+ if ($item) { >+ >+ #item level pickup location allowed? >+ $locations = $item->pickup_locations( { patron => $patron } ); >+ } else { >+ >+ #record level pickup location allowed? >+ $locations = $hold->biblio->pickup_locations( { patron => $patron } ); >+ } >+ >+ unless ( $locations->search( { branchcode => $new_pickup_loc } )->count ) { >+ push @failed_holds, { hold => $hold, reason => 'pickupLocationNotAllowed' }; >+ $hold_failed = 1; >+ next; >+ } > >- if ( $new_pickup_loc && ( $hold->branchcode ne $new_pickup_loc ) ) { > $hold->branchcode($new_pickup_loc)->store; > } > >- if ( $new_suspend_status ne "" ) { >- if ( $new_suspend_status && !$hold->is_found ) { >- $hold->suspend(1)->store; >- if ($new_suspend_date) { >+ #only continue to update the hold if it hasn't failed >+ unless ($hold_failed) { >+ if ( $new_suspend_status ne "" ) { >+ if ( $new_suspend_status && !$hold->is_found ) { >+ $hold->suspend(1)->store; >+ if ($new_suspend_date) { >+ $hold->suspend_until($new_suspend_date)->store; >+ } else { >+ $hold->suspend_until(undef)->store; >+ } >+ } elsif ( !$new_suspend_status && $new_suspend_date ) { >+ $hold->suspend(1)->store; > $hold->suspend_until($new_suspend_date)->store; > } else { >+ $hold->suspend(0)->store; > $hold->suspend_until(undef)->store; > } >- } elsif ( !$new_suspend_status && $new_suspend_date ) { >- $hold->suspend(1)->store; >- $hold->suspend_until($new_suspend_date)->store; >- } else { >- $hold->suspend(0)->store; >- $hold->suspend_until(undef)->store; > } >- } > >- if ($new_hold_note) { >- $hold->reservenotes($new_hold_note)->store; >- } >+ if ($new_expiration_date) { >+ $hold->expirationdate($new_expiration_date)->store; >+ } >+ >+ if ($new_hold_note) { >+ $hold->reservenotes($new_hold_note)->store; >+ } >+ >+ if ($clear_hold_notes) { >+ $hold->reservenotes(undef)->store; >+ } > >- if ($clear_hold_notes) { >- $hold->reservenotes(undef)->store; >+ push @holds_data, $hold; > } >- push @holds_data, $hold; > } > > $template->param( > updated_holds => to_json( \@hold_ids ), > updated_holds_obj => \@holds_data, >+ failed_holds_obj => \@failed_holds, > total_updated => scalar @holds_data, >+ total_failed => scalar @failed_holds, > view => 'report', > ); > >-- >2.39.5
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 41882
:
193667
|
193818
|
194175