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!
Created attachment 123280 [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 are greeted with a reserve page for no particular biblio (same behavior if you inputted a non- integer value)
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.
Created attachment 123285 [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
(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.
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>
Thanks for the follow-up!
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>
Created attachment 123331 [details] [review] Bug 28779: (QA follow-up) More specific message and soem cleanup Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Can you check my follow-up Joonas?
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>
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>
*** Bug 24500 has been marked as a duplicate of this bug. ***
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>
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>
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>
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.
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.03
Pushed to 20.11.x for 20.11.09
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.