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 _ "&biblionumber=" _ biblionumber %] + [% END %] [% IF multi_hold %] - [% SET data_url = "/cgi-bin/koha/reserve/request.pl?club=" _ c.id _ "&multi_hold=1&biblionumbers=" _ biblionumbers %] - [% ELSE %] - [% SET data_url = "/cgi-bin/koha/reserve/request.pl?club=" _ c.id _ "&biblionumber=" _ biblionumber %] + [% SET data_url = data_url _ "&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