From 7b10aa4c8d7f5be459dcf38b302d5cffcbe7d4e1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Joonas=20Kylm=C3=A4l=C3=A4?= <joonas.kylmala@helsinki.fi>
Date: Thu, 29 Jul 2021 17:23:01 +0300
Subject: [PATCH] Bug 28782: Use query param list instead of splitting elements
 using '/'

This removes the need to handle single and multiple cases separately,
thus removing bunch if-else cases and simplifying our code. This
coding style is also in line with our other .pl scripts.

To test:
 1) Make sure placing a hold still works from the following pages:

   /cgi-bin/koha/catalogue/detail.pl?biblionumber=XXX

   /cgi-bin/koha/catalogue/search.pl?q=a

   /cgi-bin/koha/virtualshelves/shelves.pl?op=view&shelfnumber=XXXX

   /cgi-bin/koha/clubs/clubs.pl (create a new club and add a patron
   there and through the clubs.pl create a hold to a bib)
---
 .../prog/en/includes/clubs-table.inc          |  8 +--
 .../prog/en/modules/catalogue/results.tt      |  2 -
 .../prog/en/modules/reserve/request.tt        | 51 ++++++++-----------
 .../prog/en/modules/virtualshelves/shelves.tt |  8 ++-
 koha-tmpl/intranet-tmpl/prog/js/cart.js       | 21 +++-----
 .../intranet-tmpl/prog/js/pages/results.js    | 10 ++--
 reserve/modrequest.pl                         |  5 +-
 reserve/placerequest.pl                       | 18 +++----
 reserve/request.pl                            | 14 ++---
 9 files changed, 59 insertions(+), 78 deletions(-)

diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/clubs-table.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/clubs-table.inc
index 233ccf3d04..ea90d84864 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/clubs-table.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/clubs-table.inc
@@ -20,10 +20,12 @@
     <tbody>
         [% FOREACH c IN clubs %]
             [% IF destination == 'holds' %]
+                [% SET data_url = "/cgi-bin/koha/reserve/request.pl?club=" _ c.id %]
+                [% FOREACH biblionumber IN biblionumbers %]
+                    [% SET data_url = data_url _ "&amp;biblionumber=" _ biblionumber %]
+                [% END %]
                 [% IF multi_hold %]
-                    [% SET data_url = "/cgi-bin/koha/reserve/request.pl?club=" _ c.id _ "&amp;multi_hold=1&amp;biblionumbers=" _ biblionumbers %]
-                [% ELSE %]
-                    [% SET data_url = "/cgi-bin/koha/reserve/request.pl?club=" _ c.id _ "&amp;biblionumber=" _ biblionumber %]
+                    [% SET data_url = data_url _ "&amp;multi_hold=1" %]
                 [% END %]
                 <tr class="clickable" data-url="[% data_url | html %]">
             [% ELSE %]
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
index cbaec622ab..6dd5d42e4e 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
@@ -714,14 +714,12 @@
 
                 <form id="hold_form" method="get" action="/cgi-bin/koha/reserve/request.pl">
                     <!-- Value will be set here by placeHold() -->
-                    <input id="hold_form_biblios" type="hidden" name="biblionumbers" value="" />
                     <input type="hidden" name="findborrower" id="holdFor" value="" />
                     <input type="hidden" name="club" id="holdForClub" value="" />
                 </form>
 
                 <form id="list_form" method="get" action="/cgi-bin/koha/reserve/request.pl">
                     <!-- Value will be set here by addToList() -->
-                    <input id="list_form_biblios" type="hidden" name="biblionumbers" value="" />
                     <input type="hidden" name="multi_listadd" value="1"/>
                 </form>
 
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
index 5a8545c612..edb89d2d8a 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
@@ -127,14 +127,12 @@
                             [% END %]
                         </ul>
                         <div id="holds_patronsearch_pane">
-                            <form id="holds_patronsearch" action="request.pl?biblionumbers=[% biblionumbers | html %]" method="post">
+                            <form id="holds_patronsearch" action="request.pl" method="post">
                                 <div class="hint">Enter patron card number or partial name:</div>
                                 <input type="text" size="40" id="patron" class="focus" name="findborrower" autocomplete="off" />
                                 <input type="submit" value="Search" />
-                                [% IF multi_hold %]
-                                    <input type="hidden" name="biblionumbers" value="[% biblionumbers | html %]"/>
-                                [% ELSE %]
-                                    <input type="hidden" name="biblionumber" value="[% biblionumber | html %]" />
+                                [% FOREACH biblionumber IN biblionumbers %]
+                                <input type="hidden" name="biblionumber" value="[% biblionumber | html %]"/>
                                 [% END %]
 
                             </form> <!-- /#holds_patronsearch -->
@@ -144,14 +142,12 @@
                         </div>
                         [% IF clubcount %]
                             <div id="holds_clubsearch_pane">
-                                <form id="holds_clubsearch" action="request.pl?biblionumbers=[% biblionumbers | html %]" method="post">
+                                <form id="holds_clubsearch" action="request.pl" method="post">
                                     <div class="hint">Enter club ID or partial name:</div>
                                     <input type="text" size="40" id="club" class="focus" name="findclub" autocomplete="off" />
                                     <input type="submit" value="Search" />
-                                    [% IF multi_hold %]
-                                        <input type="hidden" name="biblionumbers" value="[% biblionumbers | html %]"/>
-                                    [% ELSE %]
-                                        <input type="hidden" name="biblionumber" value="[% biblionumber | html %]" />
+                                    [% FOREACH biblionumber IN biblionumbers %]
+                                    <input type="hidden" name="biblionumber" value="[% biblionumber | html %]"/>
                                     [% END %]
 
                                 </form> <!-- /#holds_patronsearch -->
@@ -170,7 +166,6 @@
                     <form action="/api/v1/clubs/[% club.id | html %]/holds" method="post" name="form" id="club-request-form">
 
                         [% IF ( multi_hold ) %]
-                            <input type="hidden" name="biblionumbers" id="multi_hold_bibs" value="[% biblionumbers | html %]"/>
                             <input type="hidden" name="bad_bibs" id="bad_bibs" value=""/>
                             <input type="hidden" name="request" value="any"/>
                             [% FOREACH biblioloo IN biblioloop %]
@@ -178,7 +173,6 @@
                                 <input type="hidden" name="rank_[% biblioloo.biblionumber | html %]" value="[% biblioloo.rank | html %]"/>
                             [% END %]
                         [% ELSE %]
-                            <input type="hidden" name="biblionumber" value="[% biblionumber | html %]" />
                             <input type="hidden" name="title" value="[% biblio.title | html %]" />
                             <input type="hidden" name="rank-request" value="[% fixedRank | html %]" />
                         [% END # /IF multi_hold %]
@@ -413,8 +407,10 @@
                         <input type="hidden" name="borrowernumber" value="[% patron.borrowernumber | html %]" />
                         <input type="hidden" name="type" value="str8" />
 
+                        [% FOREACH biblionumber IN biblionumbers %]
+                        <input type="hidden" name="biblionumber" value="[% biblionumber | html %]"/>
+                        [% END %]
                         [% IF ( multi_hold ) %]
-                            <input type="hidden" name="biblionumbers" id="multi_hold_bibs" value="[% biblionumbers | html %]"/>
                             <input type="hidden" name="multi_holds" id="multi_holds" value="1" />
                             <input type="hidden" name="bad_bibs" id="bad_bibs" value=""/>
                             <input type="hidden" name="request" value="any"/>
@@ -423,7 +419,6 @@
                                 <input type="hidden" name="rank_[% biblioloo.biblionumber | html %]" value="[% biblioloo.rank | html %]"/>
                             [% END %]
                         [% ELSE %]
-                            <input type="hidden" name="biblionumber" value="[% biblionumber | html %]" />
                             <input type="hidden" name="title" value="[% biblio.title | html %]" />
                             <input type="hidden" name="rank-request" value="[% fixedRank | html %]" />
                         [% END # /IF multi_hold %]
@@ -871,7 +866,9 @@
                 [% IF ( reserveloop ) %]
                     <form id="existing_holds" name="T[% time | html %]" action="modrequest.pl" method="post" style="display:block">
                         [% IF ( multi_hold ) %]
-                            <input type = "hidden" name="biblionumbers" value="[% biblionumbers | html %]"/>
+                            [% FOREACH biblionumber IN biblionumbers %]
+                            <input type="hidden" name="biblionumber" value="[% biblionumber | html %]"/>
+                            [% END %]
                         [% END %]
 
                         <h2>Existing holds</h2>
@@ -1058,7 +1055,7 @@
     [% INCLUDE 'select2.inc' %]
     <script>
         var Sticky;
-        var biblionumber = "[% biblionumber | $raw %]";
+        var biblionumbers = [[% biblionumbers.join(', ') | $raw %]];
         var borrowernumber = "[% patron.borrowernumber | $raw %]";
         var patron_homebranch = "[% To.json( Branches.GetName( patron.branchcode ) ) | $raw %]";
         var override_items = {[% FOREACH bibitemloo IN bibitemloop %][% FOREACH itemloo IN bibitemloo.itemloop %][% IF ( itemloo.override ) %]
@@ -1122,7 +1119,7 @@
                     var pickup = $("#pickup").val();
                     var url = "?pickup=" + pickup;
                     url += "&borrowernumber=" + borrowernumber;
-                    url += "&biblionumber=" + biblionumber;
+                    url += "&biblionumber=" + biblionumbers[0];
                     window.location.replace(url);
                 });
             [% END %]
@@ -1152,8 +1149,6 @@
             $("#club-request-form").on("submit", function() {
                 let $t = $(this);
                 $('.clubalert, .holdalert').addClass('hide');
-                let biblionumbers = [biblionumber];
-                let biblionumbers_text;
                 const data = {
                     pickup_library_id: $('select[name="pickup"]').val()
                 };
@@ -1173,12 +1168,13 @@
                 if($('input[name="default_patron_home"]:checked').length) {
                     data.default_patron_home = 1;
                 }
-                if($('input[name="biblionumbers"]').length) {
-                    biblionumbers_text = $('input[name="biblionumbers"]').val();
-                    biblionumbers = biblionumbers_text.replace(/\/$/, '').split('/')
-                }
 
                 const count = $('input[name="holds_to_place_count"]').length?$('input[name="holds_to_place_count"]').val():1;
+                var newloc = 'request.pl?';
+                biblionumbers.forEach(function (biblionumber) {
+                    newloc += '&biblionumber=' + biblionumber;
+                });
+
                 biblionumbers.forEach(function(biblionumber) {
                     data.biblio_id = biblionumber;
                     let options = {
@@ -1190,11 +1186,7 @@
                     for(let i = 0; i < count; i++) {
                         $.ajax(options)
                         .then(function(result) {
-                            let url = 'request.pl?biblionumber='+biblionumber;
-                            if(biblionumbers_text) {
-                                url = 'request.pl?biblionumbers='+biblionumbers_text;
-                            }
-                            document.location = url;
+                            document.location = newloc;
                         })
                         .fail(function(err) {
                             var message = err.responseJSON.error;
@@ -1405,7 +1397,6 @@
 
         function checkMultiHold() {
 
-            var biblionumbers = "";
             var selected_bibs = $(".multi_hold_item_checkbox:checked");
             if ( selected_bibs.length > 0 ) {
                 // there are biblios selected in the form!
@@ -1418,7 +1409,6 @@
                     }
                     else {
                         var bibnum = $(this).attr("title");
-                        biblionumbers += bibnum + "/";
                     }
                 });
                 if ( pickup_not_set > 0 ) {
@@ -1438,7 +1428,6 @@
                 badBibs += bibnum + "/";
             });
 
-            $("#multi_hold_bibs").val(biblionumbers);
             $("#bad_bibs").val(badBibs);
 
             $('#hold-request-form').preventDoubleFormSubmit();
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
index b2f12a4b4e..013425a61b 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
@@ -437,8 +437,7 @@
 [% END %]
 
 <form id="hold_form" method="get" action="/cgi-bin/koha/reserve/request.pl">
-    <!-- Value will be set here by placeHold() -->
-    <input id="hold_form_biblios" type="hidden" name="biblionumbers" value="" />
+    <!-- Values will be set here by placeHold() -->
 </form>
 
             </main>
@@ -768,11 +767,10 @@
                 alert(MSG_NO_ITEM_SELECTED);
                 return false;
             }
-            var bibs = "";
             $(checkedItems).each(function() {
-                bibs += $(this).val() + "/";
+                var bib_param = $("<input>").attr("type", "hidden").attr("name", "biblionumber").val($(this).val());
+                $('#hold_form').append(bib_param);
             });
-            $("#hold_form_biblios").val(bibs);
             $("#hold_form").submit();
             return false;
         }
diff --git a/koha-tmpl/intranet-tmpl/prog/js/cart.js b/koha-tmpl/intranet-tmpl/prog/js/cart.js
index b4212c0cdf..ca81042c29 100644
--- a/koha-tmpl/intranet-tmpl/prog/js/cart.js
+++ b/koha-tmpl/intranet-tmpl/prog/js/cart.js
@@ -7,22 +7,17 @@ function placeHold () {
         return false;
     }
 
-    var newloc;
-
-    if ($(checkedItems).size() > 1) {
-        var bibs = "";
-        $(checkedItems).each(function() {
-            var bib = $(this).val();
-            bibs += bib + "/";
-        });
+    var bib_params = [];
+    $(checkedItems).each(function() {
+        var bib = $(this).val();
+        bib_params.push("biblionumber=" + bib);
+    });
 
-        newloc = "/cgi-bin/koha/reserve/request.pl?biblionumbers=" + bibs + "&multi_hold=1";
-    } else {
-        var bib = checkedItems[0].value;
-        newloc = "/cgi-bin/koha/reserve/request.pl?biblionumber=" + bib;
+    if (bib_params.length > 1) {
+        bib_params.push('multi_hold=1');
     }
 
-    window.opener.location = newloc;
+    window.opener.location = "/cgi-bin/koha/reserve/request.pl?" + bib_params.join('&');
     window.close();
 }
 
diff --git a/koha-tmpl/intranet-tmpl/prog/js/pages/results.js b/koha-tmpl/intranet-tmpl/prog/js/pages/results.js
index 785817c097..1a88a88cef 100644
--- a/koha-tmpl/intranet-tmpl/prog/js/pages/results.js
+++ b/koha-tmpl/intranet-tmpl/prog/js/pages/results.js
@@ -337,7 +337,7 @@ function placeHold () {
         alert( __("Nothing is selected") );
         return false;
     }
-    var bibs = "";
+    var bibs = [];
     var badBibs = false;
     $(checkedItems).each(function() {
         var bib = $(this).val();
@@ -346,12 +346,16 @@ function placeHold () {
             badBibs = true;
             return false;
         }
-        bibs += bib + "/";
+        bibs.push(bib);
     });
     if (badBibs) {
         return false;
     }
-    $("#hold_form_biblios").val(bibs);
+
+    bibs.forEach(function (bib) {
+        var bib_param = $("<input>").attr("type", "hidden").attr("name", "biblionumber").val(bib);
+	$('#hold_form').append(bib_param);
+    });
     $("#hold_form").submit();
     return false;
 }
diff --git a/reserve/modrequest.pl b/reserve/modrequest.pl
index 3a7c3deb6b..6690d0bbaf 100755
--- a/reserve/modrequest.pl
+++ b/reserve/modrequest.pl
@@ -24,6 +24,7 @@
 
 use Modern::Perl;
 use CGI qw ( -utf8 );
+use URI;
 use List::MoreUtils qw( uniq );
 use C4::Output;
 use C4::Reserves qw( ModReserve ModReserveCancelAll );
@@ -94,7 +95,7 @@ if ( $from eq 'borrower'){
 } elsif ( $from eq 'circ'){
     print $query->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrower[0]");
 } else {
-     my $url = "/cgi-bin/koha/reserve/request.pl?";
-     $url .= "biblionumbers=" . join('/', @biblionumber);
+     my $url = URI->new("/cgi-bin/koha/reserve/request.pl");
+     $url->query_form( biblionumber => @biblionumber);
      print $query->redirect($url);
 }
diff --git a/reserve/placerequest.pl b/reserve/placerequest.pl
index 5ee8dce2f6..b5fdf4c4d7 100755
--- a/reserve/placerequest.pl
+++ b/reserve/placerequest.pl
@@ -24,6 +24,7 @@
 use Modern::Perl;
 
 use CGI qw ( -utf8 );
+use URI;
 use C4::Reserves qw( CanItemBeReserved AddReserve CanBookBeReserved );
 use C4::Auth qw( checkauth );
 
@@ -36,7 +37,7 @@ checkauth($input, 0, { reserveforothers => 'place_holds' }, 'intranet');
 
 my @bibitems       = $input->multi_param('biblioitem');
 my @reqbib         = $input->multi_param('reqbib');
-my $biblionumber   = $input->param('biblionumber');
+my @biblionumbers   = $input->multi_param('biblionumber');
 my $borrowernumber = $input->param('borrowernumber');
 my $notes          = $input->param('notes');
 my $branch         = $input->param('pickup');
@@ -52,14 +53,11 @@ my $non_priority   = $input->param('non_priority');
 my $borrower = Koha::Patrons->find( $borrowernumber );
 $borrower = $borrower->unblessed if $borrower;
 
-my $biblionumbers = $input->param('biblionumbers');
-$biblionumbers ||= $biblionumber . '/';
-
-my $bad_bibs = $input->param('bad_bibs');
+my $bad_bibs_param = $input->param('bad_bibs');
+my @bad_bibs = split '/', $bad_bibs_param;
 my $holds_to_place_count = $input->param('holds_to_place_count') || 1;
 
 my %bibinfos = ();
-my @biblionumbers = split '/', $biblionumbers;
 foreach my $bibnum (@biblionumbers) {
     my %bibinfo = ();
     $bibinfo{title}  = $input->param("title_$bibnum");
@@ -164,10 +162,12 @@ if ( $type eq 'str8' && $borrower ) {
         }
     }
 
-    if ($bad_bibs) {
-        $biblionumbers .= $bad_bibs;
+    if (@bad_bibs) {
+        push @biblionumbers, @bad_bibs;
     }
-    print $input->redirect("request.pl?biblionumbers=$biblionumbers");
+    my $redirect_url = URI->new("request.pl");
+    $redirect_url->query_form( biblionumber => @biblionumbers);
+    print $input->redirect($redirect_url);
 }
 elsif ( $borrowernumber eq '' ) {
     print $input->header();
diff --git a/reserve/request.pl b/reserve/request.pl
index a6fbe528f4..400537b0aa 100755
--- a/reserve/request.pl
+++ b/reserve/request.pl
@@ -157,14 +157,7 @@ if($findclub) {
     }
 }
 
-my @biblionumbers = ();
-my $biblionumber = $input->param('biblionumber');
-my $biblionumbers = $input->param('biblionumbers');
-if ( $biblionumbers ) {
-    @biblionumbers = split '/', $biblionumbers;
-} else {
-    push @biblionumbers, $input->multi_param('biblionumber');
-}
+my @biblionumbers = $input->multi_param('biblionumber');
 
 my $multi_hold = @biblionumbers > 1;
 $template->param(
@@ -754,10 +747,11 @@ foreach my $biblionumber (@biblionumbers) {
 
 $template->param( biblioloop => \@biblioloop );
 $template->param( no_reserves_allowed => $no_reserves_allowed );
-$template->param( biblionumbers => join('/', @biblionumbers) );
+$template->param( biblionumbers => \@biblionumbers );
 $template->param( exceeded_maxreserves => $exceeded_maxreserves );
 $template->param( exceeded_holds_per_record => $exceeded_holds_per_record );
-$template->param( subscriptionsnumber => CountSubscriptionFromBiblionumber($biblionumber));
+# FIXME: getting just the first bib's result doesn't seem right
+$template->param( subscriptionsnumber => CountSubscriptionFromBiblionumber($biblionumbers[0]));
 
 # pass the userenv branch if no pickup location selected
 $template->param( pickup => $pickup || C4::Context->userenv->{branch} );
-- 
2.25.1