Currently Koha::Hold::is_at_destination looks to be unnecessary and it could be replaced with Koha::Hold::is_waiting. However, to make sure nobody adds any code paths in the future we should add some checks to Koha::Hold::set_waiting so that it wouldn't allow set Hold waiting if the item's holding branch is not the hold's pickup library. We should also make sure that e.g. a transfer cannot be initiated for a waiting hold without the hold status going to in transit.
Created attachment 117820 [details] [review] Bug 27879: Don't allow set holds to be waiting if not at pickup location If the item is not at pickup location when setting the hold to be waiting it would be clearly error because then the status should be in transit instead.
Created attachment 117821 [details] [review] Bug 27879: Remove unnecessary Koha::Hold::is_at_destination function It does the same thing as is_waiting(). A waiting hold should never be anywhere else than the pickup branch, if it is it is not a waiting hold.
Martin, could you take a look at this transfers and hold refactoring? I think I remember you saying something that we need tighter integration between holds and transfers, so hopefully this would be step to the right direction. Don't you think we need some integration here so that for example Koha::Item->holdingbranch($newbranch) call would give an exception if the we have an waiting hold? Or if someone tries to initiate a new transfer for item it should be blocked because of already waiting hold for the item. Would love to hear your opinion on this.
I completely agree.. took me a little while there to wrap my head around it, but yes, I agree. If a hold is waiting then it needs to stay waiting unless explicitly changed... * It shouldn't become waiting until it's at its pickup location. * It shouldn't be allowed to change location and remain in waiting. So, yes.. I agree an exception being thrown would be great.. probably with the ability to call a different function to reset the waiting status to another explicit state if you really want to change the current location of the item... that may be to trigger a transfer back to the pickup location, or set the hold back into the queue.
Is this ready for testing?
(In reply to Martin Renvoize from comment #5) > Is this ready for testing? Hmm, seems like the patch "Bug 27879: Don't allow set holds to be waiting if not at pickup location" misses unit tests, so those need to be added. I won't be able to spend any time on it now so feel free to add yourself as Co-author in the patches if you will work on this. But besides missing the tests it seems like the patches are OK.
Still valid it seems: is_at_destination still exists on master.
Still valid.