https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24194 --- Comment #46 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to Aleisha Amohia from comment #39)
(In reply to Joonas Kylmälä from comment #36)
ReserveExpiration should be moved out of set_waiting() method because it makes using set_waiting method really confusing because you are really expecting it to only set the waiting status and not do much more, documentation and testing of the function becomes also more difficult.
I'm not sure I understand what you mean here, expiration date logic is already in the set_waiting method before my patch. I think moving that logic out is beyond the scope of this work.
I mean just moving the ReserveExpiration syspref code you added. The rest of the code is already in OPAC/INTRANET/REST (because that is required due the nature of it) so having this check there wouldn't add any extra code. Also please see below for more explanation. Also note that this DB migration would be needed then:
ReservesMaxPickUpDelay value '' => 0 so then when ReservesMaxPickUpDelay is not an integer value it would simply not add expiration date.
The best way to do this would be to block adding expiration date already when creating a new hold. So please add the necessary code to OPAC, Staff and REST. For OPAC and Staff I see there is already some code, REST is missing completely.
What code is missing for OPAC and staff? I will add it to REST in a follow-up.
You had the code to hide the HTML but if someone submits the request directly it will work but the expiration date is silently dropped. I noticed you handled it now this way as well in the REST API in the follow-up, IMHO there should be error given (e.g. raise the previously mentioned exception in Koha::Hold). Silently discarding data is going to lead to surprises for our API users especially! -- You are receiving this mail because: You are watching all bug changes.