Bug 28779

Summary: Calling request.pl with non-existent biblionumber gives internal server error
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: Hold requestsAssignee: Joonas Kylmälä <joonas.kylmala>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: minor    
Priority: P5 - low CC: cbrannon, fridolin.somers, gmcharlt, jonathan.druart, kyle, nick, nugged, victor
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.03,20.11.09
Attachments: Bug 28779: Skip processing of non-existent biblios
Bug 28779: Skip processing of non-existent biblios
Bug 28779: Skip processing of non-existent biblios
Bug 28779: Skip processing of non-existent biblios
Bug 28779: (QA follow-up) More specific message and soem cleanup
Bug 28779: Skip processing of non-existent biblios
Bug 28779: (QA follow-up) More specific message and soem cleanup
Bug 28779: Skip processing of non-existent biblios
Bug 28779: (QA follow-up) More specific message and soem cleanup
Bug 28779: (QA follow-up) Even more specific message

Description Joonas Kylmälä 2021-07-29 09:17:59 UTC
If you try to visit the intranet hold request page and pass a non-existent biblionumber in the biblionumbers paremeter Koha gives internal server error. The URL is (for example):

> /cgi-bin/koha/reserve/request.pl?biblionumbers=488

We should handle even the invalid inputs user might give, i.e. here this happened on a previously existing biblio which was recently deleted and the user tried to access it directly from a bookmark and was confused about why Koha doesn't work!
Comment 1 Joonas Kylmälä 2021-07-29 09:48:28 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2021-07-29 11:21:32 UTC
If there is no record to place a hold on I think we should not allow the user to continue the process. The patron selection form should not appear, and some kind of error message should be displayed.
Comment 3 Joonas Kylmälä 2021-07-29 16:37:48 UTC Comment hidden (obsolete)
Comment 4 Joonas Kylmälä 2021-07-29 16:38:11 UTC
(In reply to Owen Leonard from comment #2)
> If there is no record to place a hold on I think we should not allow the
> user to continue the process. The patron selection form should not appear,
> and some kind of error message should be displayed.

Addressed this in the new patch.
Comment 5 Owen Leonard 2021-07-30 12:02:31 UTC
Created attachment 123304 [details] [review]
Bug 28779: Skip processing of non-existent biblios

Just checking with a regex that whether an input looks like a biblionumber
is not enough, we need to also verify there is a biblio really existing
in the database and skip processing of hold request for non-existent
biblionumbers.

To test:
 1) Go to page /cgi-bin/koha/reserve/request.pl?biblionumbers=XXXXX
    where XXXXX is non-existent biblionumber, notice internal server error
 2) Apply patch
 3) Repeat step 1 and notice we cannot place a hold

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 6 Owen Leonard 2021-07-30 12:02:57 UTC
Thanks for the follow-up!
Comment 7 Nick Clemens 2021-07-30 14:40:38 UTC
Created attachment 123330 [details] [review]
Bug 28779: Skip processing of non-existent biblios

Just checking with a regex that whether an input looks like a biblionumber
is not enough, we need to also verify there is a biblio really existing
in the database and skip processing of hold request for non-existent
biblionumbers.

To test:
 1) Go to page /cgi-bin/koha/reserve/request.pl?biblionumbers=XXXXX
    where XXXXX is non-existent biblionumber, notice internal server error
 2) Apply patch
 3) Repeat step 1 and notice we cannot place a hold

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Nick Clemens 2021-07-30 14:40:41 UTC
Created attachment 123331 [details] [review]
Bug 28779: (QA follow-up) More specific message and soem cleanup

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens 2021-07-30 14:41:26 UTC
Can you check my follow-up Joonas?
Comment 10 Peter Vashchuk 2021-08-02 12:42:28 UTC
Created attachment 123370 [details] [review]
Bug 28779: Skip processing of non-existent biblios

Just checking with a regex that whether an input looks like a biblionumber
is not enough, we need to also verify there is a biblio really existing
in the database and skip processing of hold request for non-existent
biblionumbers.

To test:
 1) Go to page /cgi-bin/koha/reserve/request.pl?biblionumbers=XXXXX
    where XXXXX is non-existent biblionumber, notice internal server error
 2) Apply patch
 3) Repeat step 1 and notice we cannot place a hold

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 11 Peter Vashchuk 2021-08-02 12:42:32 UTC
Created attachment 123371 [details] [review]
Bug 28779: (QA follow-up) More specific message and soem cleanup

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 12 Owen Leonard 2021-08-05 15:40:17 UTC
*** Bug 24500 has been marked as a duplicate of this bug. ***
Comment 13 Katrin Fischer 2021-08-10 13:49:52 UTC
Created attachment 123690 [details] [review]
Bug 28779: Skip processing of non-existent biblios

Just checking with a regex that whether an input looks like a biblionumber
is not enough, we need to also verify there is a biblio really existing
in the database and skip processing of hold request for non-existent
biblionumbers.

To test:
 1) Go to page /cgi-bin/koha/reserve/request.pl?biblionumbers=XXXXX
    where XXXXX is non-existent biblionumber, notice internal server error
 2) Apply patch
 3) Repeat step 1 and notice we cannot place a hold

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2021-08-10 13:49:56 UTC
Created attachment 123691 [details] [review]
Bug 28779: (QA follow-up) More specific message and soem cleanup

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2021-08-10 13:50:01 UTC
Created attachment 123692 [details] [review]
Bug 28779: (QA follow-up) Even more specific message

For the detail page, the MARC view etc. we use the text:

The record you requested does not exist (488).

I think this is better than "not found", because that sounds
like it might be an issue with the search index, which we can
rule out in this case.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Jonathan Druart 2021-08-11 11:10:29 UTC
I think we should add a warning message but let the user continue the process with the available records. No blocking those patches as they are a good enhancement to the current behaviour.
Comment 17 Jonathan Druart 2021-08-11 11:24:10 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 18 Kyle M Hall 2021-08-13 12:01:46 UTC
Pushed to 21.05.x for 21.05.03
Comment 19 Fridolin Somers 2021-08-13 20:25:08 UTC
Pushed to 20.11.x for 20.11.09
Comment 20 Victor Grousset/tuxayo 2021-08-16 21:11:26 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.