Bug 28782 - Get rid of custom query param list creation for request.pl
Summary: Get rid of custom query param list creation for request.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Joonas Kylmälä
QA Contact: Testopia
URL:
Keywords:
: 29257 29259 (view as bug list)
Depends on: 29660
Blocks: 29136
  Show dependency treegraph
 
Reported: 2021-07-29 16:42 UTC by Joonas Kylmälä
Modified: 2023-07-01 15:23 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 28782: Use query param list instead of splitting elements using '/' (20.77 KB, patch)
2021-07-29 17:28 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28782: Use query param list instead of splitting elements using '/' (20.77 KB, patch)
2021-07-29 17:33 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28782: Use query param list instead of splitting elements using '/' (20.77 KB, patch)
2021-07-30 12:31 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28782: Use query param list instead of splitting elements using '/' (20.58 KB, patch)
2021-09-04 17:35 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28782: Use query param list instead of splitting elements using '/' (20.70 KB, patch)
2021-09-06 02:25 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 28782: (follow-up) Pass real array to query_form() (1.63 KB, patch)
2021-10-30 22:11 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28782: Use query param list instead of splitting elements using '/' (20.70 KB, patch)
2021-11-02 11:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28782: (follow-up) Pass real array to query_form() (1.70 KB, patch)
2021-11-02 11:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28782: Use query param list instead of splitting elements using '/' (21.33 KB, patch)
2022-01-28 13:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: (follow-up) Pass real array to query_form() (1.70 KB, patch)
2022-01-28 13:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: Fix link when placing a hold for a patron from patron list (2.09 KB, patch)
2022-01-28 13:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: Fix cancel holds in bulk (1.77 KB, patch)
2022-01-28 13:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: Fix link when placing a hold for a patron from patron list (2.13 KB, patch)
2022-02-16 12:38 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28782: Fix cancel holds in bulk (1.82 KB, patch)
2022-02-16 12:38 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28782: Use query param list instead of splitting elements using '/' (21.32 KB, patch)
2022-02-18 16:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28782: (follow-up) Pass real array to query_form() (1.75 KB, patch)
2022-02-18 16:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28782: Fix link when placing a hold for a patron from patron list (2.19 KB, patch)
2022-02-18 16:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28782: Fix cancel holds in bulk (1.88 KB, patch)
2022-02-18 16:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28782: (QA follow-up) Bug 29844 follow-up (1.44 KB, patch)
2022-02-18 16:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28782: (QA follow-up) Fix missing filter (1.40 KB, patch)
2022-02-18 16:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28782: (QA follow-up) Bug 29844 follow-up (846 bytes, patch)
2022-02-18 19:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28782: (QA follow-up) Fix missing filter (1.40 KB, patch)
2022-02-18 19:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28782: Use query param list instead of splitting elements using '/' (21.33 KB, patch)
2022-02-21 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: (follow-up) Pass real array to query_form() (1.70 KB, patch)
2022-02-21 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: Fix link when placing a hold for a patron from patron list (2.09 KB, patch)
2022-02-21 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: Fix cancel holds in bulk (1.77 KB, patch)
2022-02-21 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: (QA follow-up) Bug 29844 follow-up (848 bytes, patch)
2022-02-21 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: (QA follow-up) Fix missing filter (1.40 KB, patch)
2022-02-21 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: Use query param list instead of splitting elements using '/' (21.28 KB, patch)
2022-03-03 19:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: (follow-up) Pass real array to query_form() (1.70 KB, patch)
2022-03-03 19:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: Fix link when placing a hold for a patron from patron list (2.09 KB, patch)
2022-03-03 19:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: Fix cancel holds in bulk (1.77 KB, patch)
2022-03-03 19:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: (QA follow-up) Bug 29844 follow-up (848 bytes, patch)
2022-03-03 19:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28782: (QA follow-up) Fix missing filter (1.40 KB, patch)
2022-03-03 19:31 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2021-07-29 16:42:24 UTC
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.
Comment 1 Joonas Kylmälä 2021-07-29 17:28:14 UTC Comment hidden (obsolete)
Comment 2 Joonas Kylmälä 2021-07-29 17:33:41 UTC
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)
Comment 3 Owen Leonard 2021-07-30 12:21:54 UTC
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
Comment 4 Joonas Kylmälä 2021-07-30 12:31:21 UTC
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)
Comment 5 Joonas Kylmälä 2021-07-30 12:32:16 UTC
(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.
Comment 6 Hayley Pelham 2021-09-03 00:22:43 UTC
Patch doesn't apply
Comment 7 Joonas Kylmälä 2021-09-04 17:35:22 UTC
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)
Comment 8 Joonas Kylmälä 2021-09-04 17:35:45 UTC
(In reply to Hayley Pelham from comment #6)
> Patch doesn't apply

Rebased, ready for testing.
Comment 9 Hayley Pelham 2021-09-06 02:25:41 UTC
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>
Comment 10 Marcel de Rooy 2021-09-06 09:06:02 UTC
Adjusted assignee. Good habit to fill in ;)
Comment 11 Marcel de Rooy 2021-09-06 09:08:37 UTC
Should this be an enhancement?
Comment 12 Katrin Fischer 2021-10-21 19:38:12 UTC
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?
Comment 13 Joonas Kylmälä 2021-10-30 22:11:04 UTC
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>
Comment 14 Joonas Kylmälä 2021-10-30 22:24:03 UTC
(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.
Comment 15 Martin Renvoize 2021-11-02 11:00:09 UTC
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>
Comment 16 Martin Renvoize 2021-11-02 11:00:13 UTC
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>
Comment 17 Martin Renvoize 2021-11-02 11:00:44 UTC
Nice improvement, no regressions found.. QA scripts happy.

Passing QA
Comment 18 Jonathan Druart 2021-11-03 13:11:03 UTC
1. This one seems wrong:
koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt:                        link = `request.pl?biblionumbers=[% biblionumbers | url %]&amp;action=cancelBulk&amp;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 _ "&amp;multi_hold=1&amp;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 (?)
Comment 19 Joonas Kylmälä 2021-11-03 19:26:07 UTC
(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
> %]&amp;action=cancelBulk&amp;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.
Comment 20 Jonathan Druart 2021-11-04 13:31:36 UTC
*** Bug 29259 has been marked as a duplicate of this bug. ***
Comment 21 Jonathan Druart 2021-12-13 11:41:49 UTC
Joonas, do you think you will be back at this one soon?
Comment 22 Jonathan Druart 2022-01-28 13:49:06 UTC
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>
Comment 23 Jonathan Druart 2022-01-28 13:49:12 UTC
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>
Comment 24 Jonathan Druart 2022-01-28 13:49:18 UTC
Created attachment 129941 [details] [review]
Bug 28782: Fix link when placing a hold for a patron from patron list
Comment 25 Jonathan Druart 2022-01-28 13:49:26 UTC
Created attachment 129942 [details] [review]
Bug 28782: Fix cancel holds in bulk

See also bug 29969.
Comment 26 Jonathan Druart 2022-01-28 13:50:06 UTC
Patches rebased on top of bug 29660. And follow-ups submitted.
Comment 27 Jonathan Druart 2022-01-28 13:50:46 UTC
*** Bug 29257 has been marked as a duplicate of this bug. ***
Comment 28 Owen Leonard 2022-02-16 12:38:30 UTC
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>
Comment 29 Owen Leonard 2022-02-16 12:38:35 UTC
Created attachment 130664 [details] [review]
Bug 28782: Fix cancel holds in bulk

See also bug 29969.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 30 Nick Clemens 2022-02-18 16:20:59 UTC
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>
Comment 31 Nick Clemens 2022-02-18 16:21:03 UTC
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>
Comment 32 Nick Clemens 2022-02-18 16:21:07 UTC
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>
Comment 33 Nick Clemens 2022-02-18 16:21:11 UTC
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>
Comment 34 Nick Clemens 2022-02-18 16:21:15 UTC
Created attachment 130872 [details] [review]
Bug 28782: (QA follow-up) Bug 29844 follow-up

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 35 Nick Clemens 2022-02-18 16:21:19 UTC
Created attachment 130873 [details] [review]
Bug 28782: (QA follow-up) Fix missing filter

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 36 Nick Clemens 2022-02-18 19:50:23 UTC
Created attachment 130894 [details] [review]
Bug 28782: (QA follow-up) Bug 29844 follow-up

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 37 Nick Clemens 2022-02-18 19:50:28 UTC
Created attachment 130895 [details] [review]
Bug 28782: (QA follow-up) Fix missing filter

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 38 Jonathan Druart 2022-02-21 10:30:20 UTC
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>
Comment 39 Jonathan Druart 2022-02-21 10:30:25 UTC
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>
Comment 40 Jonathan Druart 2022-02-21 10:30:30 UTC
Created attachment 130940 [details] [review]
Bug 28782: Fix link when placing a hold for a patron from patron list
Comment 41 Jonathan Druart 2022-02-21 10:30:36 UTC
Created attachment 130941 [details] [review]
Bug 28782: Fix cancel holds in bulk

See also bug 29969.
Comment 42 Jonathan Druart 2022-02-21 10:30:41 UTC
Created attachment 130942 [details] [review]
Bug 28782: (QA follow-up) Bug 29844 follow-up

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 43 Jonathan Druart 2022-02-21 10:30:46 UTC
Created attachment 130943 [details] [review]
Bug 28782: (QA follow-up) Fix missing filter

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 44 Fridolin Somers 2022-02-24 23:30:04 UTC
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&amp;action=cancelBulk&amp;ids=2,1
With last patch I see URL :
/cgi-bin/koha/reserve/request.pl?biblionumber%3D359%26biblionumber%3D369&amp;action=cancelBulk&amp;ids=2,1

Without all patches we have URL :
/cgi-bin/koha/reserve/request.pl?biblionumbers=359/369&amp;action=cancelBulk&amp;ids=2,1
Looks it '&amp;' works, but it should be '&'.
Comment 45 Jonathan Druart 2022-03-03 19:31:19 UTC
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>
Comment 46 Jonathan Druart 2022-03-03 19:31:24 UTC
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>
Comment 47 Jonathan Druart 2022-03-03 19:31:29 UTC
Created attachment 131351 [details] [review]
Bug 28782: Fix link when placing a hold for a patron from patron list
Comment 48 Jonathan Druart 2022-03-03 19:31:34 UTC
Created attachment 131352 [details] [review]
Bug 28782: Fix cancel holds in bulk

See also bug 29969.
Comment 49 Jonathan Druart 2022-03-03 19:31:39 UTC
Created attachment 131353 [details] [review]
Bug 28782: (QA follow-up) Bug 29844 follow-up

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 50 Jonathan Druart 2022-03-03 19:31:44 UTC
Created attachment 131354 [details] [review]
Bug 28782: (QA follow-up) Fix missing filter

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 51 Jonathan Druart 2022-03-03 19:31:58 UTC
(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&amp;
> action=cancelBulk&amp;ids=2,1
> With last patch I see URL :
> /cgi-bin/koha/reserve/request.pl?biblionumber%3D359%26biblionumber%3D369&amp;
> action=cancelBulk&amp;ids=2,1
> 
> Without all patches we have URL :
> /cgi-bin/koha/reserve/request.pl?biblionumbers=359/369&amp;
> action=cancelBulk&amp;ids=2,1
> Looks it '&amp;' works, but it should be '&'.

Good catch, fixed! I amended the patch.
Comment 52 Fridolin Somers 2022-03-04 02:11:54 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 53 Jonathan Druart 2022-05-12 09:26:01 UTC
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;
Comment 54 David Cook 2022-05-13 00:04:17 UTC
(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.