Bugzilla – Attachment 129939 Details for
Bug 28782
Get rid of custom query param list creation for request.pl
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 28782: Use query param list instead of splitting elements using '/'
Bug-28782-Use-query-param-list-instead-of-splittin.patch (text/plain), 21.33 KB, created by
Jonathan Druart
on 2022-01-28 13:49:06 UTC
(
hide
)
Description:
Bug 28782: Use query param list instead of splitting elements using '/'
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2022-01-28 13:49:06 UTC
Size:
21.33 KB
patch
obsolete
>From 1621d8a411a8a5ec8ae81bc53ac7d5e383a03df3 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> >--- > .../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 99d1d861e80..171656144f3 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 _ "&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 %]"> > <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 4115ddacf7a..99acc344617 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 cabfc8d2796..22035a70929 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 %] >@@ -448,8 +442,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"/> >@@ -458,7 +454,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 %] >@@ -921,7 +916,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 %] >@@ -1152,7 +1149,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 ) %] >@@ -1218,7 +1215,7 @@ > var pickup = $("#pickup").val(); > var url = "?pickup=" + pickup; > url += "&borrowernumber=" + borrowernumber; >- url += "&biblionumber=" + biblionumber; >+ url += "&biblionumber=" + biblionumbers[0]; > window.location.replace(url); > }); > [% END %] >@@ -1238,8 +1235,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() > }; >@@ -1255,12 +1250,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 = { >@@ -1272,11 +1268,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; >@@ -1371,7 +1363,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! >@@ -1384,7 +1375,6 @@ > } > else { > var bibnum = $(this).attr("title"); >- biblionumbers += bibnum + "/"; > } > }); > if ( pickup_not_set > 0 ) { >@@ -1404,7 +1394,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 09550fe2b4c..b5462a8ac3f 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 b4212c0cdf4..ca81042c295 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 070a8c473de..4255ddf94b6 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 3a7c3deb6b3..6690d0bbaf7 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 77d1c122f65..c23313d36c3 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 d06d5f59875..12a8b17130f 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) { >@@ -705,17 +695,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} ); >-- >2.25.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 28782
:
123286
|
123287
|
123313
|
124510
|
124521
|
127112
|
127208
|
127209
|
129939
|
129940
|
129941
|
129942
|
130663
|
130664
|
130868
|
130869
|
130870
|
130871
|
130872
|
130873
|
130894
|
130895
|
130938
|
130939
|
130940
|
130941
|
130942
|
130943
|
131349
|
131350
|
131351
|
131352
|
131353
|
131354