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>&nbsp;</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