From 9af1f9abb70fa4654b13d6041971b7c315c9384f Mon Sep 17 00:00:00 2001 From: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Date: Thu, 28 Mar 2024 08:19:20 +0200 Subject: [PATCH] Bug 36135: Fix QA issues and add improvements This patch: - Fixes issues reported by qa-tool - Adds info about how many holds were modified - Add title and barcode columns to table of modified holds - Adds div with class "page-section" to "Modify holds" table Sponsored-by: Koha-Suomi Oy Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> --- .../data/mysql/atomicupdate/bug_36135.pl | 10 +- .../en/modules/tools/batch_modify_holds.tt | 128 +++++++++--------- tools/batch_modify_holds.pl | 85 ++++++------ 3 files changed, 115 insertions(+), 108 deletions(-) diff --git a/installer/data/mysql/atomicupdate/bug_36135.pl b/installer/data/mysql/atomicupdate/bug_36135.pl index 4bd58cbac7..40aa318e1e 100755 --- a/installer/data/mysql/atomicupdate/bug_36135.pl +++ b/installer/data/mysql/atomicupdate/bug_36135.pl @@ -1,13 +1,15 @@ use Modern::Perl; return { - bug_number => "36135", + bug_number => "36135", description => "Add new permission batch_modify_holds", - up => sub { + up => sub { my ($args) = @_; - my ($dbh, $out) = @$args{qw(dbh out)}; + my ( $dbh, $out ) = @$args{qw(dbh out)}; - $dbh->do(q{INSERT IGNORE permissions (module_bit, code, description) VALUES (13, 'batch_modify_holds', 'Perform batch modification of holds')}); + $dbh->do( + q{INSERT IGNORE permissions (module_bit, code, description) VALUES (13, 'batch_modify_holds', 'Perform batch modification of holds')} + ); say $out "Added new permission 'batch_modify_holds'"; }, 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 d9a6ddef49..9ccba1fc48 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 @@ -48,15 +48,12 @@ <span class="hint">NOTE! Column patron_expiration_date is used as a main filter for expiration date field. Otherwise column expirationdate is used.</span> <ol> <li> - <label for="suspend_until">Expiration date:</label> - <li> - <label for="expirationdate_from">From</label> - <input type="text" size="10" id="from" name="expirationdate_from" class="flatpickr" data-date_to="to" /> - </li> - <li> - <label for="expirationdate_to">To</label> - <input type="text" size="10" id="to" name="expirationdate_to" class="flatpickr"/> - </li> + <label for="expirationdate_from">Expiration date from:</label> + <input type="text" size="10" id="expirationdate_from" name="expirationdate_from" class="flatpickr" data-date_to="to" /> + </li> + <li> + <label for="expirationdate_to">Expiration date to:</label> + <input type="text" size="10" id="expirationdate_to" name="expirationdate_to" class="flatpickr"/> </li> <li> <label for="branchcodes">Libraries:</label> @@ -82,15 +79,12 @@ </select> </li> <li> - <label for="suspend_until">Suspended until:</label> - <li> - <label for="suspend_until_from">From</label> - <input type="text" size="10" id="from" name="suspend_until_from" class="flatpickr" data-date_to="to" /> - </li> - <li> - <label for="suspend_until_to">To</label> - <input type="text" size="10" id="to" name="suspend_until_to" class="flatpickr"/> - </li> + <label for="suspend_until_from">Suspended until from:</label> + <input type="text" size="10" id="suspend_until_from" name="suspend_until_from" class="flatpickr" data-date_to="to" /> + </li> + <li> + <label for="suspend_until_to">Suspended until to:</label> + <input type="text" size="10" id="suspend_until_to" name="suspend_until_to" class="flatpickr"/> </li> <li> <label for="reservenotes">Hold note:</label> @@ -108,7 +102,7 @@ <form action="/cgi-bin/koha/tools/batch_modify_holds.pl" method="post" id="process"> [% INCLUDE 'csrf-token.inc' %] [% IF holds.count %] - <div><p>Found [% holds.count | html %][% IF holds.count > 1 %] holds [% ELSE %] hold [% END %]to modify.</p><div> + <div><p>Found [% holds.count | html %][% IF holds.count > 1 %] holds [% ELSE %] hold [% END %]to modify.</p></div> <div class="page-section"> <div class="btn-toolbar selections-toolbar"> <a id="selectall" href="#"><i class="fa fa-check"></i> Select all</a> @@ -120,7 +114,7 @@ <th> </th> <th>Expiration date</th> <th>Patron expiration date</th> - <th>Title</title> + <th>Title</th> <th>Barcode</th> <th>Status</th> <th>Hold pickup library</th> @@ -131,12 +125,16 @@ </thead> <tbody> [% FOREACH hold IN holds %] - <tr [% IF hold.found %]class="highlighted-row"[% END %]> + [% IF hold.found %] + <tr class="highlighted-row"> + [% ELSE %] + <tr> + [% END %] <td><input type="checkbox" name="hold_id" value="[% hold.reserve_id | html %]"/></td> <td>[% hold.expirationdate | $KohaDates %]</td> <td>[% hold.patron_expiration_date | $KohaDates %]</td> - <td><a href="/cgi-bin/koha/catalogue/detail.pl?biblionumber=[% hold.item.biblio.biblionumber | uri %]">[% hold.item.biblio.title | html %]</a></td> - <td><a href="/cgi-bin/koha/catalogue/moredetail.pl?itemnumber=[% hold.itemnumber | uri %]&biblionumber=[% hold.item.biblio.biblionumber | uri %]&bi=[% hold.item.biblio.biblionumber | uri %]#item[% hold.itemnumber | uri %]">[% hold.item.barcode | html %]</a></td> + <td><a href="/cgi-bin/koha/catalogue/detail.pl?biblionumber=[% hold.biblionumber | uri %]">[% hold.biblio.title | html %]</a></td> + <td><a href="/cgi-bin/koha/catalogue/moredetail.pl?itemnumber=[% hold.itemnumber | uri %]&biblionumber=[% hold.biblionumber | uri %]&bi=[% hold.biblionumber | uri %]#item[% hold.itemnumber | uri %]">[% hold.item.barcode | html %]</a></td> <td class="found_status" data-found="[% hold.found | html %]"> [% IF hold.found == "T" %] In transit @@ -158,43 +156,45 @@ </table> <!-- /#holds --> </div> <!-- /.page-section --> <h2>Modify holds</h2> - <table id="hold_modifies"> - <thead> - <tr> - <th>New expiration date</th> - <th>New pickup library</th> - <th>Suspend holds</th> - <th>Suspend until</th> - <th>New reserve note</th> - </tr> - </thead> - <tbody> - <tr> - <td> - <input type="text" id="new_expiration_date" name="new_expiration_date" class="flatpickr" data-flatpickr-futuredate="true"/> - </td> - <td> - <select id="new_pickup_loc" name="new_pickup_loc"> - <option value="" selected="selected"></option> - [% PROCESS options_for_libraries libraries => Branches.pickup_locations(selected = "") %] - </select> - </td> - <td> - <select id="new_suspend_status" name="new_suspend_status"> - <option value="" selected="selected"></option> - <option value="not_suspended">Not suspended</option> - <option value="suspend">Suspend</option> - </select> - </td> - <td> - <input type="text" id="new_suspend_date" name="new_suspend_date" class="flatpickr" data-flatpickr-futuredate="true"/> - </td> - <td> - <input type="text" id="new_reserve_note" name="new_reserve_note"/> - </td> - </tr> - </tbody> - </table> <!-- /#hold_modifies --> + <div class="page-section"> + <table id="hold_modifies"> + <thead> + <tr> + <th>New expiration date</th> + <th>New pickup library</th> + <th>Suspend holds</th> + <th>Suspend until</th> + <th>New reserve note</th> + </tr> + </thead> + <tbody> + <tr> + <td> + <input type="text" id="new_expiration_date" name="new_expiration_date" class="flatpickr" data-flatpickr-futuredate="true"/> + </td> + <td> + <select id="new_pickup_loc" name="new_pickup_loc"> + <option value="" selected="selected"></option> + [% PROCESS options_for_libraries libraries => Branches.pickup_locations(selected = "") %] + </select> + </td> + <td> + <select id="new_suspend_status" name="new_suspend_status"> + <option value="" selected="selected"></option> + <option value="not_suspended">Not suspended</option> + <option value="suspend">Suspend</option> + </select> + </td> + <td> + <input type="text" id="new_suspend_date" name="new_suspend_date" class="flatpickr" data-flatpickr-futuredate="true"/> + </td> + <td> + <input type="text" id="new_reserve_note" name="new_reserve_note"/> + </td> + </tr> + </tbody> + </table> <!-- /#hold_modifies --> + </div> <fieldset class="action"> <input type="hidden" name="op" value="cud-modify"/> <input type="submit" class="btn btn-primary" value="Modify selected holds"/> @@ -209,13 +209,15 @@ [% END %] [% ELSIF view == 'report' %] <div class="dialog message"> - Holds have been modified! + [% updated_holds.count | html %] holds have been modified! </div> <div class="page-section"> <table id="holds"> <thead> <tr> <th>Expiration date</th> + <th>Title</th> + <th>Barcode</th> <th>Hold pickup library</th> <th>Suspended</th> <th>Suspended until</th> @@ -226,6 +228,8 @@ [% FOREACH hold IN updated_holds %] <tr> <td>[% hold.expirationdate | $KohaDates %]</td> + <td><a href="/cgi-bin/koha/catalogue/detail.pl?biblionumber=[% hold.biblionumber | uri %]">[% hold.biblio.title | html %]</a></td> + <td><a href="/cgi-bin/koha/catalogue/moredetail.pl?itemnumber=[% hold.itemnumber | uri %]&biblionumber=[% hold.biblionumber | uri %]&bi=[% hold.biblionumber | uri %]#item[% hold.itemnumber | uri %]">[% hold.item.barcode | html %]</a></td> <td>[% Branches.GetName( hold.branchcode ) | html %]</td> <td>[% IF hold.suspend == 0 %]No[% ELSE %]Yes[% END %]</td> <td>[% hold.suspend_until | $KohaDates %]</td> @@ -309,7 +313,7 @@ } if( ( !new_suspend_status || new_suspend_status == "not_suspended") && new_suspend_date ){ e.preventDefault(); - alert(_('You have to suspend holds if new suspend until date is set.')); + alert(_("You have to suspend holds if new suspend until date is set.")); return false; } diff --git a/tools/batch_modify_holds.pl b/tools/batch_modify_holds.pl index c1438c3c61..8d32b96513 100755 --- a/tools/batch_modify_holds.pl +++ b/tools/batch_modify_holds.pl @@ -29,14 +29,14 @@ use Koha::DateUtils qw( dt_from_string ); use Koha::Holds; my $input = CGI->new; -my $op = $input->param('op') // q|form|; +my $op = $input->param('op') // q|form|; my ( $template, $loggedinuser, $cookie ) = get_template_and_user( { - template_name => 'tools/batch_modify_holds.tt', - query => $input, - type => "intranet", - flagsrequired => { tools => 'batch_modify_holds.tt' }, + template_name => 'tools/batch_modify_holds.tt', + query => $input, + type => "intranet", + flagsrequired => { tools => 'batch_modify_holds.tt' }, } ); @@ -44,8 +44,7 @@ my @hold_ids; if ( $op eq 'form' ) { $template->param( view => 'form', ); -} -elsif ( $op eq 'list' ) { +} elsif ( $op eq 'list' ) { my @reserve_ids = $input->multi_param('reserve_ids'); my $expirationdate_from = $input->param('expirationdate_from'); @@ -59,36 +58,39 @@ elsif ( $op eq 'list' ) { my $search_params; - if( @reserve_ids ){ + if (@reserve_ids) { $search_params->{'me.reserve_id'} = { -in => \@reserve_ids }; } else { - if( $expirationdate_from && $expirationdate_to ){ - my $expirationdate_params = { -or => { - patron_expiration_date => { - -between => [ - $expirationdate_from, - $expirationdate_to - ] - }, - expirationdate => { - -between => [ - $expirationdate_from, - $expirationdate_to - ] + if ( $expirationdate_from && $expirationdate_to ) { + my $expirationdate_params = { + -or => { + patron_expiration_date => { + -between => [ + $expirationdate_from, + $expirationdate_to + ] + }, + expirationdate => { + -between => [ + $expirationdate_from, + $expirationdate_to + ] + } } - }}; + }; $search_params = $expirationdate_params; } - if ( @branchcodes ) { + if (@branchcodes) { $search_params->{'me.branchcode'} = { -in => \@branchcodes }; } - if ( @found_status ) { + if (@found_status) { my $found_params = {}; + # if NULL is used as filter, use it as "is" param - if( grep {$_ eq "NULL"} @found_status ) { + if ( grep { $_ eq "NULL" } @found_status ) { $found_params->{'-or'}{'-is'} = undef; $found_params->{'-or'}{'-in'} = \@found_status; } else { @@ -98,11 +100,11 @@ elsif ( $op eq 'list' ) { $search_params->{'me.found'} = $found_params; } - if( $suspend_status ne "none" ){ + if ( $suspend_status ne "none" ) { $search_params->{'me.suspend'} = $suspend_status; } - if( $suspend_until_from && $suspend_until_to ){ + if ( $suspend_until_from && $suspend_until_to ) { $search_params->{'me.suspend_until'} = { -between => [ $suspend_until_from, @@ -111,16 +113,14 @@ elsif ( $op eq 'list' ) { }; } - if( $reservenotes ){ - $search_params->{'me.reservenotes'} = { -like => "%".$reservenotes."%" } + if ($reservenotes) { + $search_params->{'me.reservenotes'} = { -like => "%" . $reservenotes . "%" }; } } my $holds = Koha::Holds->search( $search_params, - { - join => ["item"] - } + { join => [ "item", "biblio" ] } ); $template->param( @@ -139,33 +139,34 @@ if ( $op eq 'cud-modify' ) { @hold_ids = $input->multi_param('hold_id'); - my $holds_to_update = Koha::Holds->search( { reserve_id => { -in => \@hold_ids } } ); + my $holds_to_update = + Koha::Holds->search( { reserve_id => { -in => \@hold_ids } }, { join => [ "item", "biblio" ] } ); while ( my $hold = $holds_to_update->next ) { - if( $new_expiration_date ){ + if ($new_expiration_date) { $hold->expirationdate($new_expiration_date)->store; } - if($new_pickup_loc && ($hold->branchcode ne $new_pickup_loc)){ + if ( $new_pickup_loc && ( $hold->branchcode ne $new_pickup_loc ) ) { $hold->branchcode($new_pickup_loc)->store; } - if( $new_suspend_status && !$hold->is_found ){ - if( $new_suspend_status eq "suspend" ){ + if ( $new_suspend_status && !$hold->is_found ) { + if ( $new_suspend_status eq "suspend" ) { $hold->suspend(1)->store; - if( $new_suspend_date ){ - $hold->suspend_until( $new_suspend_date )->store; + if ($new_suspend_date) { + $hold->suspend_until($new_suspend_date)->store; } else { - $hold->suspend_until( undef )->store; + $hold->suspend_until(undef)->store; } } else { $hold->suspend(0)->store; - $hold->suspend_until( undef )->store; + $hold->suspend_until(undef)->store; } } - if( $new_reserve_note ){ + if ($new_reserve_note) { $hold->reservenotes($new_reserve_note)->store; } } -- 2.30.2