This bug is opened to simplify the query param handling of reserve.pl which currently supports passing the reservable biblionumber using two different query param variables: biblionumber=XXXX biblionumbers=XXXX/YYYY/ZZZZ Almost all our other scripts use the query param duplication and ->multi_param() instead of custom list creation.
Created attachment 123286 [details] [review] 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)
Created attachment 123287 [details] [review] 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)
Everything tests well except for the cart. I found that I can't place holds because of a JavaScript error: Uncaught TypeError: bib_params.size is not a function placeHold http://127.0.0.1:8081/intranet-tmpl/prog/js/cart_21.0600010.js:16
Created attachment 123313 [details] [review] 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)
(In reply to Owen Leonard from comment #3) > Everything tests well except for the cart. I found that I can't place holds > because of a JavaScript error: > > Uncaught TypeError: bib_params.size is not a function > placeHold > http://127.0.0.1:8081/intranet-tmpl/prog/js/cart_21.0600010.js:16 Thanks for spotting, I was using the jQuery size() function instead of native JS length function accidentally. That's now fixed.
Patch doesn't apply
Created attachment 124510 [details] [review] 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)
(In reply to Hayley Pelham from comment #6) > Patch doesn't apply Rebased, ready for testing.
Created attachment 124521 [details] [review] 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>
Adjusted assignee. Good habit to fill in ;)
Should this be an enhancement?
I really like the idea of this, but I wonder if we should use biblionumber= for the URL parameter or something a little shorter? I seem to remember that we had issues with extra long URLs at some point, but couldn't pin it down. I looked it up and apparently the max size of an URL is 2048 characters? So I tried this: 1) On a standard Koha sample database, activate numSearchResultsDropdown 2) Search in the catalog for 'e' 3) Select them all for placing a hold 4) Look up Henry (or another patron) to place the holds for 5) Place the holds 6) You are redirected to: http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumber=14410755=117107233=22210712=2510728=3110733=3610758=120107167=172107175=178107181=182107185=220107221=283107328=334107338=366107367=383107385=386107387=393107394=2010779=86107186=188107195=199107208=271107301=312107340=3541077=1510746=4810799=103107104=135107153=163107169=187107231=235107249=264107265=352107363=36410724=306107434=11073=810710=1610719=2110722=2310726=2710730=3410735=3810741=4210744=5610757=6410766=6710769=7110791=9610798=102107107=14410755=117107233=1210725=2810731=3310736=58107120=167107172=181107182=185107220=221107283=328107334=338107366=367107383=385107386=387107393=39410720=86107186=188107195=199107208=271107301=312107340=3541077=1510746=4810799=103107104=135107153=163107187=231107235=249107264=265107352=363107364=24107434=11078=1010716=1910721=2210723=2610727=3010734=3510738=4110742=5610757=6410766=6710769=7110791=9610798=102 ... and the display will look a bit broken. I wonder how this relates to bug 29257?
Created attachment 127112 [details] [review] Bug 28782: (follow-up) Pass real array to query_form() It excepts to receive a real array and not a list, in order to be able to generate the query parameters. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
(In reply to Katrin Fischer from comment #12) > I really like the idea of this, but I wonder if we should use biblionumber= > for the URL parameter or something a little shorter? It's what we use basically everywhere in koha for biblionumber so I think it would be preferable to keep it that way. > I seem to remember that we had issues with extra long URLs at some point, > but couldn't pin it down. I looked it up and apparently the max size of an > URL is 2048 characters? Yeah, it seems to be a limitation of some browsers. If we wanted to support browsers that have an URL length limitation then we should start an effort to fix all places in Koha where we generate URLs with arbitrary length. Realistically though, here we are not gonna run into that limitation easily, you would have to make a hold for over 70 biblios (even if your bibnums were in the millions) to reach the limit. > ... and the display will look a bit broken. Thanks, fixed in the follow-up! > I wonder how this relates to bug 29257? That would be not needed if this is pushed.
Created attachment 127208 [details] [review] 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>
Created attachment 127209 [details] [review] Bug 28782: (follow-up) Pass real array to query_form() It excepts to receive a real array and not a list, in order to be able to generate the query parameters. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Nice improvement, no regressions found.. QA scripts happy. Passing QA
1. This one seems wrong: koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt: link = `request.pl?biblionumbers=[% biblionumbers | url %]&action=cancelBulk&ids=${$('.holds_table .select_hold:checked').toArray().map(el => $(el).data('id')).join(',')}`; I don't have the time to confirm now but could do later. 2. This one is wrong: koha-tmpl/intranet-tmpl/prog/en/includes/circ-patron-search-results.inc: [% SET data_url = "/cgi-bin/koha/reserve/request.pl?borrowernumber=" _ borrower.borrowernumber _ "&multi_hold=1&biblionumbers=" _ biblionumbers %] to recreate: search, select at least 2 records, place holds, search for patron (which will give you several results), select one => url is /reserve/request.pl?borrowernumber=19&multi_hold=1&biblionumbers=ARRAY(0x55def41723e8) 3. The occurrences of biblionumers in virtualshelves/addbybiblionumber.tt seem wrong but not related (?)
(In reply to Jonathan Druart from comment #18) > 1. This one seems wrong: > koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt: > link = `request.pl?biblionumbers=[% biblionumbers | url > %]&action=cancelBulk&ids=${$('.holds_table > .select_hold:checked').toArray().map(el => $(el).data('id')).join(',')}`; > > I don't have the time to confirm now but could do later. it is wrong, have to re-do the patch, there was some changes pushed to master from Bug 23678 that are not compatible with this.
*** Bug 29259 has been marked as a duplicate of this bug. ***
Joonas, do you think you will be back at this one soon?
Created attachment 129939 [details] [review] 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>
Created attachment 129940 [details] [review] Bug 28782: (follow-up) Pass real array to query_form() It excepts to receive a real array and not a list, in order to be able to generate the query parameters. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 129941 [details] [review] Bug 28782: Fix link when placing a hold for a patron from patron list
Created attachment 129942 [details] [review] Bug 28782: Fix cancel holds in bulk See also bug 29969.
Patches rebased on top of bug 29660. And follow-ups submitted.
*** Bug 29257 has been marked as a duplicate of this bug. ***
Created attachment 130663 [details] [review] Bug 28782: Fix link when placing a hold for a patron from patron list Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 130664 [details] [review] Bug 28782: Fix cancel holds in bulk See also bug 29969. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 130868 [details] [review] 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: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130869 [details] [review] Bug 28782: (follow-up) Pass real array to query_form() It excepts to receive a real array and not a list, in order to be able to generate the query parameters. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130870 [details] [review] Bug 28782: Fix link when placing a hold for a patron from patron list Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130871 [details] [review] Bug 28782: Fix cancel holds in bulk See also bug 29969. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130872 [details] [review] Bug 28782: (QA follow-up) Bug 29844 follow-up Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130873 [details] [review] Bug 28782: (QA follow-up) Fix missing filter Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130894 [details] [review] Bug 28782: (QA follow-up) Bug 29844 follow-up Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130895 [details] [review] Bug 28782: (QA follow-up) Fix missing filter Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130938 [details] [review] 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>
Created attachment 130939 [details] [review] Bug 28782: (follow-up) Pass real array to query_form() It excepts to receive a real array and not a list, in order to be able to generate the query parameters. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 130940 [details] [review] Bug 28782: Fix link when placing a hold for a patron from patron list
Created attachment 130941 [details] [review] Bug 28782: Fix cancel holds in bulk See also bug 29969.
Created attachment 130942 [details] [review] Bug 28782: (QA follow-up) Bug 29844 follow-up Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130943 [details] [review] Bug 28782: (QA follow-up) Fix missing filter Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
There are some problems with filters. I create holds for 2 records and cancel them immediately. Without last patch "Bug 28782: (QA follow-up) Fix missing filter" I see URL : /cgi-bin/koha/reserve/request.pl?biblionumber=359&biblionumber=369&action=cancelBulk&ids=2,1 With last patch I see URL : /cgi-bin/koha/reserve/request.pl?biblionumber%3D359%26biblionumber%3D369&action=cancelBulk&ids=2,1 Without all patches we have URL : /cgi-bin/koha/reserve/request.pl?biblionumbers=359/369&action=cancelBulk&ids=2,1 Looks it '&' works, but it should be '&'.
Created attachment 131349 [details] [review] 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>
Created attachment 131350 [details] [review] Bug 28782: (follow-up) Pass real array to query_form() It excepts to receive a real array and not a list, in order to be able to generate the query parameters. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131351 [details] [review] Bug 28782: Fix link when placing a hold for a patron from patron list
Created attachment 131352 [details] [review] Bug 28782: Fix cancel holds in bulk See also bug 29969.
Created attachment 131353 [details] [review] Bug 28782: (QA follow-up) Bug 29844 follow-up Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 131354 [details] [review] Bug 28782: (QA follow-up) Fix missing filter Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Fridolin Somers from comment #44) > There are some problems with filters. > > I create holds for 2 records and cancel them immediately. > Without last patch "Bug 28782: (QA follow-up) Fix missing filter" I see URL : > /cgi-bin/koha/reserve/request.pl?biblionumber=359&biblionumber=369& > action=cancelBulk&ids=2,1 > With last patch I see URL : > /cgi-bin/koha/reserve/request.pl?biblionumber%3D359%26biblionumber%3D369& > action=cancelBulk&ids=2,1 > > Without all patches we have URL : > /cgi-bin/koha/reserve/request.pl?biblionumbers=359/369& > action=cancelBulk&ids=2,1 > Looks it '&' works, but it should be '&'. Good catch, fixed! I amended the patch.
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
This introduces a warning in the log Use of uninitialized value $bad_bibs_param in split at /kohadevbox/koha/reserve/placerequest.pl line 55. We could fix it with: diff --git a/reserve/placerequest.pl b/reserve/placerequest.pl index 0a8d6dd64e6..e85c6ad292a 100755 --- a/reserve/placerequest.pl +++ b/reserve/placerequest.pl @@ -51,7 +51,7 @@ my $non_priority = $input->param('non_priority'); my $patron = Koha::Patrons->find( $borrowernumber ); -my $bad_bibs_param = $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;
(In reply to Jonathan Druart from comment #53) > This introduces a warning in the log > > Use of uninitialized value $bad_bibs_param in split at > /kohadevbox/koha/reserve/placerequest.pl line 55. > > We could fix it with: > > diff --git a/reserve/placerequest.pl b/reserve/placerequest.pl > index 0a8d6dd64e6..e85c6ad292a 100755 > --- a/reserve/placerequest.pl > +++ b/reserve/placerequest.pl > @@ -51,7 +51,7 @@ my $non_priority = $input->param('non_priority'); > > my $patron = Koha::Patrons->find( $borrowernumber ); > > -my $bad_bibs_param = $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; Alternatively, @bad_bibs could be initialised ahead of time, and the split function call could only be done if 'bad_bibs' is passed to the server.