When checking an item which has multiple holds, clicking 'confirm hold' does notcause the priorities of the remaining reserves to be re-calculated. I started testing with the bib record for the book 'After Henry'. There were 3 items under this bib record. I created six test borrowers: +-----------+---------+------------+ | firstname | surname | cardnumber | +-----------+---------+------------+ | bws a | tester | bws000 | | bws b | tester | bws001 | | bws c | tester | bws003 | | bws d | tester | bws004 | | bws e | tester | bws005 | | bws f | tester | bws006 | +-----------+---------+------------+ I checked items out to the first three borrowers: +---------------+----------------+-----------+---------+------------+ | title | barcode | firstname | surname | cardnumber | +---------------+----------------+-----------+---------+------------+ | After Henry / | 00101000066353 | bws a | tester | bws000 | | After Henry / | ILEA000137401- | bws b | tester | bws001 | | After Henry / | 33080000109101 | bws c | tester | bws003 | +---------------+----------------+-----------+---------+------------+ I checked in the item with barcode '00101000066353' and clicked the 'Confirm hold' button. Checking reserves at this point, I saw the following: +-----------+---------+------------+----------+---------------+ | firstname | surname | cardnumber | priority | title | +-----------+---------+------------+----------+---------------+ | bws d | tester | bws004 | 0 | After Henry / | | bws e | tester | bws005 | 2 | After Henry / | | bws f | tester | bws006 | 3 | After Henry / | +-----------+---------+------------+----------+---------------+ Note that the priorities *should* be 0, 1, 2 at this point. when I check out to 'bws d tester', the priority 0 hold is removed, but the other two holds are not re-prioritized: +-----------+---------+------------+----------+---------------+ | firstname | surname | cardnumber | priority | title | +-----------+---------+------------+----------+---------------+ | bws e | tester | bws005 | 2 | After Henry / | | bws f | tester | bws006 | 3 | After Henry / | +-----------+---------+------------+----------+---------------+ When holds are displayed and the list does not start with priority 1, the holds display out of order, with holds showing at the bottom of the list with priority'1', even though there are no items with priority 1. See the attached screenshot'Screenshot from 2014-03-14 11-37-13.png', which was taken immediately after 'bws d tester' was checked in.
Note that the holds were *Not* item level holds.
(In reply to Barton Chittenden from comment #0) > with priority'1', even though there are no items with priority 1. See the > attached screenshot'Screenshot from 2014-03-14 11-37-13.png', which was > taken immediately after 'bws d tester' was checked in. The screenshot is missing, but I can probably just make my own.
Adding some testing notes (I think there are some missing steps in your description). I issued the three items, and placed three non-item holds. The priorities were 1,2,3. I returned one of the items. I attempted to checkout that item to tester e (who did not have the first priority), but cancelled it (as it was waiting for tester d.) This caused the priorities to become 0,2,3 and the first reserve entry to change to having found=W and an itemnumber defined. This is probably because now there's a copy available. This seems correct overall, however the priorities should have been renumbered. I issued the item put aside for tester d to them. The reserve is removed, however the priorities are now 2,3 This causes the UI to show tester e at priority 2 (correct) and tester f at priority 1 (incorrect) which will cause them to change if the user hits "update holds". So essentially, bug is reproducible.
The priorities showing as '1' at the bottom is because of the UI not allowing for a priority greater than the number of holds, so it's forced to put it as the first. I have a patch that seems to work now. Just need to write up a test plan and attach it.
Created attachment 26395 [details] [review] Bug 11947 - renumber reserves when hold is confirmed Currently when a reserve is moved to "waiting" status because it's acknowledged on checkin, the reserve priorities aren't renumbered. This causes things to go a bit haywire in the UI, in particular, some reserves can unjustly end up with priority 1 when they shouldn't. It also seemed to mess with the logic of who should get it next, but I didn't look too closely at that. This patch forces a renumbering so that all the priorities remain copacetic. Test plan: * have a few borrowers, say 4. * have a biblio with a single item (you can scale this up, it should work just the same.) * issue the item to borrower A * have borrowers B, C, and D place a hold on the item * return the item, acknowledge that it'll be put aside for B. * view the holds on the item. Without the patch: * the hold priorities in the UI end up being "waiting, 2, 1" when they should be "waiting, 1, 2". * in the database "reserves" table, they're really "0, 2, 3" when they should be "0, 1, 2". With the patch: * the hold priorities in the UI end up being "waiting, 1, 2" * in the database, they're "0, 1, 2"
Created attachment 26396 [details] [review] Bug 11947 - renumber reserves when hold is confirmed Currently when a reserve is moved to "waiting" status because it's acknowledged on checkin, the reserve priorities aren't renumbered. This causes things to go a bit haywire in the UI, in particular, some reserves can unjustly end up with priority 1 when they shouldn't. It also seemed to mess with the logic of who should get it next, but I didn't look too closely at that. This patch forces a renumbering so that all the priorities remain copacetic. Test plan: * have a few borrowers, say 4. * have a biblio with a single item (you can scale this up, it should work just the same.) * issue the item to borrower A * have borrowers B, C, and D place a hold on the item * return the item, acknowledge that it'll be put aside for B. * view the holds on the item. Without the patch: * the hold priorities in the UI end up being "waiting, 2, 1" when they should be "waiting, 1, 2". * in the database "reserves" table, they're really "0, 2, 3" when they should be "0, 1, 2". With the patch: * the hold priorities in the UI end up being "waiting, 1, 2" * in the database, they're "0, 1, 2" [New version, including test case.]
Created attachment 26416 [details] Heather's Patch Testing for 11947
After a patch was written for the community bug, it was applied to our test server this morning. I have tested it, and it does not resolve the issues. I'm attaching a PDF as it has screenshots, but here's the three tests, step-by-step that I did. TEST #1: 1. Created new record with 20 items. 2. Placed 7 holds on the record. 3. Checked one item in. 4. Triggered a hold for patron with priority 1. 5. Clicked Confirm hold. 6. Checked reserves.pl page and database priority entries. 7. Priorities are still 2-7 after Waiting status. 8. Clicked Update hold(s) button on reserves.pl and priorities are now 1-6. 9. Now the priorities are in order. TEST #2: 1. Put more holds on the record created above. 2. Checked 3 more items in and confirmed the holds. 3. The holds priorities are now 4-9. 4. Click update hold(s) button, and the holds are again 1-6 priority. Test #3 1. I checked two more items in and confirmed the holds. 2. I checked out three of the waiting items. 3. The reserves.pl and database output showed 3-6 priority for holds. 4. I added an additional hold to the record. 5. It was given priority 7. 7. I clicked Update Holds, and the latest hold was put as #3 priority!! Not priority 5, as expected.
The holds suddenly going out of order on an update without explanation is the most disconcerting part about this bug. (Especially when you have over 100 holds pending on a record across several months, and people at the bottom get moved to the top!).
Created attachment 26427 [details] [review] Bug 11947 - renumber reserves when hold is confirmed Currently when a reserve is moved to "waiting" status because it's acknowledged on checkin, the reserve priorities aren't renumbered. This causes things to go a bit haywire in the UI, in particular, some reserves can unjustly end up with priority 1 when they shouldn't. It also seemed to mess with the logic of who should get it next, but I didn't look too closely at that. This patch forces a renumbering so that all the priorities remain copacetic. Test plan: * have a few borrowers, say 4. * have a biblio with a single item (you can scale this up, it should work just the same.) * issue the item to borrower A * have borrowers B, C, and D place a hold on the item * return the item, acknowledge that it'll be put aside for B. * view the holds on the item. Without the patch: * the hold priorities in the UI end up being "waiting, 2, 1" when they should be "waiting, 1, 2". * in the database "reserves" table, they're really "0, 2, 3" when they should be "0, 1, 2". With the patch: * the hold priorities in the UI end up being "waiting, 1, 2" * in the database, they're "0, 1, 2" [New version, including test case.]
Created attachment 26428 [details] [review] Bug 11947 - renumber reserves when hold is confirmed Currently when a reserve is moved to "waiting" status because it's acknowledged on checkin, the reserve priorities aren't renumbered. This causes things to go a bit haywire in the UI, in particular, some reserves can unjustly end up with priority 1 when they shouldn't. It also seemed to mess with the logic of who should get it next, but I didn't look too closely at that. This patch forces a renumbering so that all the priorities remain copacetic. Test plan: * have a few borrowers, say 4. * have a biblio with a single item (you can scale this up, it should work just the same.) * issue the item to borrower A * have borrowers B, C, and D place a hold on the item * return the item, acknowledge that it'll be put aside for B. * view the holds on the item. Without the patch: * the hold priorities in the UI end up being "waiting, 2, 1" when they should be "waiting, 1, 2". * in the database "reserves" table, they're really "0, 2, 3" when they should be "0, 1, 2". With the patch: * the hold priorities in the UI end up being "waiting, 1, 2" * in the database, they're "0, 1, 2"
Created attachment 26429 [details] Testing procedure illustrating the effects of the patch. I've attached a document with screenshots following the other attachment, however mine shows everything working as it should. I suspect that the patch wasn't applied correctly for testing. I've also rebased the patch against current master and attached that, there's no functional change (but API changes needed a tweak to the test case.) Note that this version is for master, I think I wrote the older one against 3.14 by accident. If applying to 3.14 there will probably be conflicts in the test case, but the actual fix is the same.
Just a guess, but I see that NEKLS is on 3.14.3 and there were a lot of patches for some priority issues on deleting/cancelling holds in 3.14.4 (bug 11336) - could this make a difference?
(In reply to Katrin Fischer from comment #13) > Just a guess, but I see that NEKLS is on 3.14.3 and there were a lot of > patches for some priority issues on deleting/cancelling holds in 3.14.4 (bug > 11336) - could this make a difference? It shouldn't, the first time I did it, I did it on 3.14.something by accident, rather than master. All it's doing is catching when a hold is marked as waiting, and recomputing the priorities for the rest of them.
Robin, I'm in the process of testing again on our test system and will do the real test now on our production server shortly after the patch is applied there. Did you write the patch for only when a hold is marked as waiting? What about when the hold gets marked as in transit? Did you cover that status too in your patch? Because I'm still seeing some priority status numbering going off on new holds if an existing hold is already marked as in transit and a new hold is added to the record.
(In reply to Heather Braum from comment #15) > Did you write the patch for only when a hold is marked as waiting? What > about when the hold gets marked as in transit? Did you cover that status too > in your patch? > > Because I'm still seeing some priority status numbering going off on new > holds if an existing hold is already marked as in transit and a new hold is > added to the record. These are different cases than the one addressed here. It's possible that some of them are also fixed if the code happens to take the same path, but I wouldn't rely on it.
Test plan: * have a few borrowers, say 4. * have a biblio with a single item (you can scale this up, it should work just the same.) * issue the item to borrower A * have borrowers B, C, and D place a hold on the item * return the item, acknowledge that it'll be put aside for B. * view the holds on the item. Without the patch: * the hold priorities in the UI end up being "waiting, 2, 1" when they should be "waiting, 1, 2". * in the database "reserves" table, they're really "0, 2, 3" when they should be "0, 1, 2". With the patch: * the hold priorities in the UI end up being "waiting, 1, 2" * in the database, they're "0, 1, 2" This patch works as expected.
Robin, I filed a separate bug report, #11956, for the in transit issue I'm still seeing with holds priorities. Thanks for your help on starting to unravel this bug.
(In reply to Robin Sheat from comment #14) > (In reply to Katrin Fischer from comment #13) > > Just a guess, but I see that NEKLS is on 3.14.3 and there were a lot of > > patches for some priority issues on deleting/cancelling holds in 3.14.4 (bug > > 11336) - could this make a difference? > > It shouldn't, the first time I did it, I did it on 3.14.something by > accident, rather than master. All it's doing is catching when a hold is > marked as waiting, and recomputing the priorities for the rest of them. Turned out you were totally right there Katrin, btw.
Created attachment 26498 [details] [review] Bug 11947 - [3.14.x] renumber reserves when hold is confirmed Currently when a reserve is moved to "waiting" status because it's acknowledged on checkin, the reserve priorities aren't renumbered. This causes things to go a bit haywire in the UI, in particular, some reserves can unjustly end up with priority 1 when they shouldn't. It also seemed to mess with the logic of who should get it next, but I didn't look too closely at that. This patch forces a renumbering so that all the priorities remain copacetic. Test plan: * have a few borrowers, say 4. * have a biblio with a single item (you can scale this up, it should work just the same.) * issue the item to borrower A * have borrowers B, C, and D place a hold on the item * return the item, acknowledge that it'll be put aside for B. * view the holds on the item. Without the patch: * the hold priorities in the UI end up being "waiting, 2, 1" when they should be "waiting, 1, 2". * in the database "reserves" table, they're really "0, 2, 3" when they should be "0, 1, 2". With the patch: * the hold priorities in the UI end up being "waiting, 1, 2" * in the database, they're "0, 1, 2"
I added a version specifically for 3.14.4+ too, the only difference is in the test case.
These patches do not appear to be signed off. Changed status to "Needs Signoff".
Created attachment 27035 [details] [review] [SIGNED-OFF] Bug 11947 - renumber reserves when hold is confirmed Currently when a reserve is moved to "waiting" status because it's acknowledged on checkin, the reserve priorities aren't renumbered. This causes things to go a bit haywire in the UI, in particular, some reserves can unjustly end up with priority 1 when they shouldn't. It also seemed to mess with the logic of who should get it next, but I didn't look too closely at that. This patch forces a renumbering so that all the priorities remain copacetic. Test plan: * have a few borrowers, say 4. * have a biblio with a single item (you can scale this up, it should work just the same.) * issue the item to borrower A * have borrowers B, C, and D place a hold on the item * return the item, acknowledge that it'll be put aside for B. * view the holds on the item. Without the patch: * the hold priorities in the UI end up being "waiting, 2, 1" when they should be "waiting, 1, 2". * in the database "reserves" table, they're really "0, 2, 3" when they should be "0, 1, 2". With the patch: * the hold priorities in the UI end up being "waiting, 1, 2" * in the database, they're "0, 1, 2" Signed-off-by: Owen Leonard <oleonard@myacpl.org> Test plan confirms that the problem exists and that the patch corrects it.
The bug itself appears to be a regression caused by the patch for bug 9394. Specifically, the checkin sequence for circ/returns.pl does this: - ModReserveAffect(), which prior to Robin's proposed patch did not alter the priorities of other holds, just the one whose item is being set to waiting followed by - GetOtherReserves(), which among its many side-effects did fix up the rest of the priorities, via a call to ModReserveMinusPriority, which in turn called _FixPriority. However, this got broken because ModReserveMinusPriority now wants a reserve_id to identify the request being changed, but CheckReserves doesn't provide one, because _Findgroupreserve was never updated to provide reserve_id in every situation. *whew* CheckReserve and _Findgroupreserve failing to provide a reserve_id is a bug, but I think a separate one. Upshot: not to preempt QA, but I think Robin's patch is on the right track. However, it's time to take a hard look at GetOtherReserves, which is used only by circ/returns.pl, and see if it is time for that routine to go. At the very, very least, it need a name other than "GetOtherReserves".
Bumping up priority -- issues that mangle the priority of hold requests can make life difficult at the circ desk.
See also bug 12079.
See also bug 12085
Created attachment 27122 [details] [review] [PASSED QA] Bug 11947 - renumber reserves when hold is confirmed Currently when a reserve is moved to "waiting" status because it's acknowledged on checkin, the reserve priorities aren't renumbered. This causes things to go a bit haywire in the UI, in particular, some reserves can unjustly end up with priority 1 when they shouldn't. It also seemed to mess with the logic of who should get it next, but I didn't look too closely at that. This patch forces a renumbering so that all the priorities remain copacetic. Test plan: * have a few borrowers, say 4. * have a biblio with a single item (you can scale this up, it should work just the same.) * issue the item to borrower A * have borrowers B, C, and D place a hold on the item * return the item, acknowledge that it'll be put aside for B. * view the holds on the item. Without the patch: * the hold priorities in the UI end up being "waiting, 2, 1" when they should be "waiting, 1, 2". * in the database "reserves" table, they're really "0, 2, 3" when they should be "0, 1, 2". With the patch: * the hold priorities in the UI end up being "waiting, 1, 2" * in the database, they're "0, 1, 2" Signed-off-by: Owen Leonard <oleonard@myacpl.org> Test plan confirms that the problem exists and that the patch corrects it. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script, especially t/db_dependent/Reserves.t. Improves priority calculation.
I think I found another bug here: 1) Appply patch 2) Check out item 3) Place 3 holds, for patron a, b, c 4) Return the item - item is now waiting for a, b = 1, c = 2 5) Check out the book to b(!), revert waiting status as offered You will end up with: a = 1, c = 1, but in the database it's kind of more correct: a = 1, c = 3 I have put this on another bug - bug 12086. Still passing this as this patch just used an existing routine and I believe it's an improvement.
Pushed to master. Thanks, Robin!
Pushed to 3.14.x, will be in 3.14.10