I was working on bug 40058, with the idea of adding some more fine grained tests, when I found I didn't quite understand how priorities work. It smells like a bug, though. I generated: * 1 item * 4 random patrons ($patron_1, ..., $patron_4) Placing 4 item-level holds one for each patron gives this results: ## priority = 1 Using the following snippet on the 4 patron holds (replacing `$patron_1` with the other patrons: ``` C4::Reserves::AddReserve( { branchcode => $library->branchcode, borrowernumber => $patron_1->borrowernumber, biblionumber => $item->biblionumber, priority => 1, itemnumber => $item->itemnumber, } ) ``` Priorities end up being (consistently): * 4 * 1 * 3 * 2 ## priority = undef Using the following snippet on the 4 patron holds: ``` C4::Reserves::AddReserve( { branchcode => $library->branchcode, borrowernumber => $patron_1->borrowernumber, biblionumber => $item->biblionumber, # priority => 1, itemnumber => $item->itemnumber, } ) ``` Priorities end up being (consistently): * 1 * 4 * 3 * 2
Created attachment 182928 [details] Bug 40062: Trying to understand priority calculation
I think the issue is with how we select the holds: # get whats left my $query = " SELECT reserve_id, borrowernumber, reservedate FROM reserves WHERE biblionumber = ? AND ((found <> 'W' AND found <> 'T' AND found <> 'P') OR found IS NULL) ORDER BY priority ASC "; If we change the ORDER BY to "priority ASC, reserve_id ASC" I think it will behave more intuitively!
(In reply to Kyle M Hall (khall) from comment #2) > > If we change the ORDER BY to "priority ASC, reserve_id ASC" I think it will > behave more intuitively! I just tried it with my patch and the results are similar. The big question here is: what is the expected behavior? If I add 4 holds on the same item, with priority=1. For me, if 1 is the highest, then it means the result should be: * p=1 -> hold_4 * p=2 -> hold_3 * p=3 -> hold_2 * p=4 -> hold_1 i.e. the hold is inserted with the requested priority, and the existing ones are shifted if required. And if we had: * p=1 -> hold_1 * p=2 -> hold_2 * p=3 -> hold_3 and we added `hold_4` with priority=2, I would expect the result to be: * p=1 -> hold_1 * p=2 -> hold_4 * p=3 -> hold_2 * p=4 -> hold_3 Does it make sense? I got it wrong?
(In reply to Tomás Cohen Arazi (tcohen) from comment #3) > I just tried it with my patch and the results are similar. The big question > here is: what is the expected behavior? If I add 4 holds on the same item, > with priority=1. For me, if 1 is the highest, then it means the result > should be: > > * p=1 -> hold_4 > * p=2 -> hold_3 > * p=3 -> hold_2 > * p=4 -> hold_1 > > i.e. the hold is inserted with the requested priority, and the existing ones > are shifted if required. This makes sense to me > And if we had: > > * p=1 -> hold_1 > * p=2 -> hold_2 > * p=3 -> hold_3 > > and we added `hold_4` with priority=2, I would expect the result to be: > > * p=1 -> hold_1 > * p=2 -> hold_4 > * p=3 -> hold_2 > * p=4 -> hold_3 > > Does it make sense? I got it wrong? This also makes sense to me. I would also expect holds without a defined priority to go at the end of the list.
Yikes, I see multiple bugs here, in the subs and one in the tests. Thanks for catching all this, Tomas!! 1. AllowHoldDateInFuture was turned on in a previous test and not rolled back, so _ShiftPriority gets called in these tests too. 2. AddReserves calls _ShiftPriority and passes the priority *of* the new hold, but _ShiftPriority shifts holds to open up a spot with priority *1 greater than* the value it was passed, and then returns that value to be the priority of the new hold. So if a hold is added when one or more holds already exists, the new hold ends up with a priority 1 greater than it is supposed to. On the other hand, if an undefined priority is passed to _ShiftPriority, it does nothing. 3. If no priority is specified, the correct priority is never actually calculated anywhere. According to the POD for CalculatePriority, AddReserves used to call CalculatePriority at the time that POD was written, but it doesn't anymore (presumably once it was rebased to call Hold->new instead). Koha::Hold->store doesn't have any checks regarding the priority at all, so the newly created hold is inserted with priority 1 anyway! (But we still end up with different end results because of bugs #1 and #2) 4. If _ShiftPriority didn't shift anything (or wasn't called), AddReserves adds the hold at priority => 1 while another hold already has priority => 1. It then calls _FixPriority passing only the biblionumber as a parameter, but _FixPriority does weird things because it isn't meant to be used that way, according to the POD: > In the second form, where a biblionumber is passed, the holds on that > bib (that are not captured) are sorted in order of increasing priority, > then have reserves.priority set so that the first non-captured hold > has its priority set to 1, the second non-captured hold has its priority > set to 2, and so forth. Simply changing the SQL in _FixPriority to "ORDER BY priority ASC, reserve_id ASC" wouldn't wholly fix it either, because it would produce unexpected behavior if the user had already changed hold priorities, so that the hold priorities no longer reflected the order in which they were placed. 5. _ShiftPriority also has a FIXME right in the middle saying "This whole sub must be rewritten, especially to highlight what is done when reserve_id is not given" The reason we don't see these bugs in production is because placerequest.pl explicitly passes the hold rank, and there's no way to specify a priority other than last when placing a hold in the UI, so none of the other holds need to be adjusted. (I didn't look thoroughly at the other places AddReserve is called, so if it's possible to place a hold with priority other than last through some other route, the bugs may show up there) So...we are probably better off adding new methods to Koha::Holds to cover this logic and removing the old ones, rather than debugging all this.
You’re right Emily. The main motivation of this bug is actually to come up with a good spec for implementing high level methods for replacing this code.
(In reply to Tomás Cohen Arazi (tcohen) from comment #6) > You’re right Emily. The main motivation of this bug is actually to come up > with a good spec for implementing high level methods for replacing this code. Oh, awesome! Sorry, I missed that that was the intention here! In that case, yes, I too agree that hold priority should work the way you think it should!