From 52b5a22edfbab0e211f5c5a3a8aa51b6fbd25202 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)

Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
---
 .../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    |  9 ++--
 reserve/modrequest.pl                         |  5 +-
 reserve/placerequest.pl                       | 18 +++----
 reserve/request.pl                            | 24 ++-------
 9 files changed, 59 insertions(+), 87 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 99d1d861e8..171656144f 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/clubs-table.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/clubs-table.inc
@@ -24,10 +24,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 %]">
                     <td><a href="[% data_url | url %]">[% c.name | html %]</a></td>
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 59def93d62..e1c0da18c5 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
@@ -671,14 +671,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 ad847d0c10..eaa700a9d3 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
@@ -163,14 +163,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 -->
@@ -180,14 +178,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 -->
@@ -206,7 +202,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 %]
@@ -214,7 +209,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 %]
@@ -449,8 +443,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"/>
@@ -459,7 +455,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 %]
@@ -923,7 +918,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 %]
 
                         [% IF enqueued %]
@@ -1154,7 +1151,7 @@
     [% Asset.js("js/holds.js") | $raw%]
     <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 biblio IN biblioloop %][% FOREACH itemloo IN biblio.itemloop %][% IF ( itemloo.override ) %]
@@ -1220,7 +1217,7 @@
                     var pickup = $("#pickup").val();
                     var url = "?pickup=" + pickup;
                     url += "&borrowernumber=" + borrowernumber;
-                    url += "&biblionumber=" + biblionumber;
+                    url += "&biblionumber=" + biblionumbers[0];
                     window.location.replace(url);
                 });
             [% END %]
@@ -1240,8 +1237,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()
                 };
@@ -1257,12 +1252,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 = {
@@ -1274,11 +1270,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;
@@ -1373,7 +1365,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!
@@ -1386,7 +1377,6 @@
                     }
                     else {
                         var bibnum = $(this).attr("title");
-                        biblionumbers += bibnum + "/";
                     }
                 });
                 if ( pickup_not_set > 0 ) {
@@ -1406,7 +1396,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 09550fe2b4..b5462a8ac3 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
@@ -435,8 +435,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>
@@ -762,11 +761,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;
+    var bib_params = [];
+    $(checkedItems).each(function() {
+        var bib = $(this).val();
+        bib_params.push("biblionumber=" + bib);
+    });
 
-    if ($(checkedItems).size() > 1) {
-        var bibs = "";
-        $(checkedItems).each(function() {
-            var bib = $(this).val();
-            bibs += 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 32194536e0..c7b47bdcb3 100644
--- a/koha-tmpl/intranet-tmpl/prog/js/pages/results.js
+++ b/koha-tmpl/intranet-tmpl/prog/js/pages/results.js
@@ -337,12 +337,15 @@ function placeHold () {
         alert( __("Nothing is selected") );
         return false;
     }
-    var bibs = "";
+    var bibs = [];
     $(checkedItems).each(function() {
         var bib = $(this).val();
-        bibs += bib + "/";
+        bibs.push(bib);
+    });
+    bibs.forEach(function (bib) {
+        var bib_param = $("<input>").attr("type", "hidden").attr("name", "biblionumber").val(bib);
+	$('#hold_form').append(bib_param);
     });
-    $("#hold_form_biblios").val(bibs);
     $("#hold_form").submit();
     return false;
 }
diff --git a/reserve/modrequest.pl b/reserve/modrequest.pl
index d95e82e815..aa8b879656 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 Try::Tiny;
 
@@ -104,7 +105,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 77d1c122f6..c23313d36c 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 );
 
@@ -35,7 +36,7 @@ my $input = CGI->new();
 checkauth($input, 0, { reserveforothers => 'place_holds' }, 'intranet');
 
 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');
@@ -50,14 +51,11 @@ my $non_priority   = $input->param('non_priority');
 
 my $patron = Koha::Patrons->find( $borrowernumber );
 
-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");
@@ -150,10 +148,12 @@ if ( $type eq 'str8' && $patron ) {
         }
     }
 
-    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 9e917a0887..d86481e3da 100755
--- a/reserve/request.pl
+++ b/reserve/request.pl
@@ -184,23 +184,13 @@ 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(
     multi_hold => $multi_hold,
 );
 
-# If we are coming from the search result and only 1 is selected
-$biblionumber ||= $biblionumbers[0] unless $multi_hold;
-
 # If we have the borrowernumber because we've performed an action, then we
 # don't want to try to place another reserve.
 if ($borrowernumber_hold && !$action) {
@@ -706,17 +696,13 @@ if (   ( $findborrower && $borrowernumber_hold || $findclub && $club_hold )
     $template->param( no_reserves_allowed => $no_reserves_allowed );
     $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]));
 } elsif ( ! $multi_hold ) {
-    my $biblio = Koha::Biblios->find( $biblionumber );
+    my $biblio = Koha::Biblios->find( $biblionumbers[0] );
     $template->param( biblio => $biblio );
 }
-
-if ( $multi_hold ) {
-    $template->param( biblionumbers => join('/', @biblionumbers) );
-} else {
-    $template->param( biblionumber => $biblionumber || $biblionumbers[0] );
-}
+$template->param( biblionumbers => \@biblionumbers );
 
 # pass the userenv branch if no pickup location selected
 $template->param( pickup => $pickup || C4::Context->userenv->{branch} );