Bug 27879 - Koha::Hold::is_at_destination function should be removed
Summary: Koha::Hold::is_at_destination function should be removed
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-03-05 10:54 UTC by Joonas Kylmälä
Modified: 2023-07-01 18:01 UTC (History)
1 user (show)

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


Attachments
Bug 27879: Don't allow set holds to be waiting if not at pickup location (1.47 KB, patch)
2021-03-05 11:37 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27879: Remove unnecessary Koha::Hold::is_at_destination function (5.03 KB, patch)
2021-03-05 11:37 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2021-03-05 10:54:17 UTC
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.
Comment 1 Joonas Kylmälä 2021-03-05 11:37:26 UTC
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.
Comment 2 Joonas Kylmälä 2021-03-05 11:37:30 UTC
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.
Comment 3 Joonas Kylmälä 2021-03-05 11:40:57 UTC
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.
Comment 4 Martin Renvoize 2021-03-05 14:44:52 UTC
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.
Comment 5 Martin Renvoize 2021-06-15 13:28:29 UTC
Is this ready for testing?
Comment 6 Joonas Kylmälä 2021-06-16 15:53:27 UTC
(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.
Comment 7 Katrin Fischer 2022-02-19 22:42:32 UTC
Still valid it seems: is_at_destination still exists on master.
Comment 8 Katrin Fischer 2023-07-01 18:01:51 UTC
Still valid.