Bug 27160 - Fatal error when ReservesMaxPickUpDelay is empty and item with hold is checked in
Summary: Fatal error when ReservesMaxPickUpDelay is empty and item with hold is checke...
Status: CLOSED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-07 02:46 UTC by David Cook
Modified: 2021-12-13 21:10 UTC (History)
2 users (show)

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


Attachments
Bug 27160: Prevent 500 when checking an item in if ReservesMaxPickUpDelay has been deleted (3.22 KB, patch)
2020-12-10 08:52 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 David Cook 2020-12-07 02:46:10 UTC
If you don't have any value in ReservesMaxPickUpDelay and you try to check in an item that has a hold, you will get an Internal Server Error.
Comment 1 Jonathan Druart 2020-12-09 12:58:56 UTC
I don't recreate on master. Please provide more detail if you do. What is the exact error?
Comment 2 David Cook 2020-12-09 23:13:23 UTC
(In reply to Jonathan Druart from comment #1)
> I don't recreate on master. Please provide more detail if you do. What is
> the exact error?

Oh, I may have not described it correctly. Try deleting the ReservesMaxPickUpDelay system preference completely. That was the actual situation I was facing...

As for the exact error, it's one of those Plack errors where you just see "Internal Server Error". 

It happens when $max_pickup_delay is undef in "$expirationdate->add(days => $max_pickup_delay);" in Koha::Hold I believe.
Comment 3 Jonathan Druart 2020-12-10 08:44:50 UTC
The error is:

Validation failed for type named Defined declared in package Specio::Library::Builtins (/usr/share/perl5/Specio/Library/Builtins.pm) at line 44 in sub named (eval) with value undef                               
                                                                                                                                                                                                                   
Trace begun at Specio::Exception->new line 57                                                                                                                                                                      
Specio::Exception::throw('Specio::Exception', 'message', 'Validation failed for type named Defined declared in package Specio::Library::Builtins (/usr/share/perl5/Specio/Library/Builtins.pm) at line 44 in sub na
med (eval) with value undef', 'type', 'Specio::Constraint::Simple=HASH(0x560960117338)', 'value', undef) called at (eval 235) line 75                                                                              
DateTime::Duration::_check_new_params('days', undef) called at /usr/lib/x86_64-linux-gnu/perl5/5.32/DateTime/Duration.pm line 64                                                                                   
DateTime::Duration::new('DateTime::Duration', 'days', undef) called at /usr/lib/x86_64-linux-gnu/perl5/5.32/DateTime.pm line 1792                                                                                  
DateTime::_duration_object_from_args('DateTime=HASH(0x56096b1fe938)', 'days', undef) called at /usr/lib/x86_64-linux-gnu/perl5/5.32/DateTime.pm line 1764
DateTime::add('DateTime=HASH(0x56096b1fe938)', 'days', undef) called at /kohadevbox/koha/Koha/Hold.pm line 198                                                                                                     
Koha::Hold::set_waiting('Koha::Hold=HASH(0x56096b2c4b70)', '') called at /kohadevbox/koha/C4/Reserves.pm line 1189                       
C4::Reserves::ModReserveAffect(36, 5, undef, 1, '') called at /kohadevbox/koha/circ/returns.pl line 159                                                                                                            
eval {...} at /kohadevbox/koha/circ/returns.pl line 2                                                                                                                                                              
CGI::Compile::ROOT::kohadevbox_koha_circ_returns_2epl::__ANON__('CGI::Compile=HASH(0x56096894c380)', '', '/kohadevbox/koha/circ/returns.pl', '/kohadevbox/koha/circ', 'ARRAY(0x56096aaa47f8)') called at /usr/share
/perl5/CGI/Compile.pm line 151                                                                                                                                                                                     
CGI::Compile::ROOT::kohadevbox_koha_circ_returns_2epl::returns_2epl at /usr/share/perl5/CGI/Emulate/PSGI.pm line 30
Comment 4 Jonathan Druart 2020-12-10 08:45:37 UTC
I don't think we should handle non-existing syspref. In which situation ReservesMaxPickUpDelay got removed entirely from the DB?
Comment 5 Jonathan Druart 2020-12-10 08:52:42 UTC
Created attachment 114296 [details] [review]
Bug 27160: Prevent 500 when checking an item in if ReservesMaxPickUpDelay has been deleted

We could default to 0 if the syspref does not exist.
However I don't think we should deal with that kind of issue, sysprefs
must be in DB.

Test plan:
Delete ReservesMaxPickUpDelay from the DB
Place an item on hold
Check it in
Confirm the hold
Without this patch you get:
"Validation failed for type named Defined declared in package Specio::Library::Builtins" (see the relevant part of the trace at comment 3)
Comment 6 David Cook 2020-12-11 00:27:53 UTC
(In reply to Jonathan Druart from comment #4)
> In which situation ReservesMaxPickUpDelay got removed entirely from the DB?

A library had applied Bug 8367 to a customized production instance. The patches for that bug removed ReservesMaxPickUpDelay. They then exported their database and tried to load it into a new non-customized Koha 19.11.x instance, and that's where they encountered the issue. 

I will admit that it's an extreme edge case.
Comment 7 David Cook 2020-12-11 00:29:41 UTC
(In reply to Jonathan Druart from comment #5)
> Created attachment 114296 [details] [review] [review]
> Bug 27160: Prevent 500 when checking an item in if ReservesMaxPickUpDelay
> has been deleted
> 
> We could default to 0 if the syspref does not exist.
> However I don't think we should deal with that kind of issue, sysprefs
> must be in DB.
> 

The default in sysprefs.sql is 7, so we might prefer that over 0.

I'm not sure that we should deal with this kind of issue either. As you say, sysprefs must be in the database. 

I'd probably be fine marking this as invalid.
Comment 8 Owen Leonard 2021-04-15 15:05:47 UTC
(In reply to David Cook from comment #7)

> I'd probably be fine marking this as invalid.

I am doing so.