Bug 11947 - Hold priorities not re-calculated when hold is confirmed on checkin.
Summary: Hold priorities not re-calculated when hold is confirmed on checkin.
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Robin Sheat
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 12086
  Show dependency treegraph
 
Reported: 2014-03-14 20:24 UTC by Barton Chittenden
Modified: 2015-06-04 23:30 UTC (History)
9 users (show)

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


Attachments
Bug 11947 - renumber reserves when hold is confirmed (1.90 KB, patch)
2014-03-17 05:45 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11947 - renumber reserves when hold is confirmed (4.01 KB, patch)
2014-03-17 06:39 UTC, Robin Sheat
Details | Diff | Splinter Review
Heather's Patch Testing for 11947 (1.12 MB, application/pdf)
2014-03-17 16:42 UTC, HB-NEKLS
Details
Bug 11947 - renumber reserves when hold is confirmed (3.80 KB, patch)
2014-03-17 22:57 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11947 - renumber reserves when hold is confirmed (3.89 KB, patch)
2014-03-17 23:07 UTC, Robin Sheat
Details | Diff | Splinter Review
Testing procedure illustrating the effects of the patch. (204.18 KB, application/vnd.oasis.opendocument.text)
2014-03-17 23:09 UTC, Robin Sheat
Details
Bug 11947 - [3.14.x] renumber reserves when hold is confirmed (3.99 KB, patch)
2014-03-21 01:42 UTC, Robin Sheat
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11947 - renumber reserves when hold is confirmed (4.07 KB, patch)
2014-04-11 17:05 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 11947 - renumber reserves when hold is confirmed (4.18 KB, patch)
2014-04-15 09:41 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2014-03-14 20:24:06 UTC
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.
Comment 1 Barton Chittenden 2014-03-14 20:25:01 UTC
Note that the holds were *Not* item level holds.
Comment 2 Robin Sheat 2014-03-17 02:41:52 UTC
(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.
Comment 3 Robin Sheat 2014-03-17 05:14:02 UTC
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.
Comment 4 Robin Sheat 2014-03-17 05:32:20 UTC
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.
Comment 5 Robin Sheat 2014-03-17 05:45:06 UTC Comment hidden (obsolete)
Comment 6 Robin Sheat 2014-03-17 06:39:51 UTC Comment hidden (obsolete)
Comment 7 HB-NEKLS 2014-03-17 16:42:46 UTC
Created attachment 26416 [details]
Heather's Patch Testing for 11947
Comment 8 HB-NEKLS 2014-03-17 16:43:05 UTC
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.
Comment 9 HB-NEKLS 2014-03-17 16:45:43 UTC
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!).
Comment 10 Robin Sheat 2014-03-17 22:57:31 UTC Comment hidden (obsolete)
Comment 11 Robin Sheat 2014-03-17 23:07:07 UTC Comment hidden (obsolete)
Comment 12 Robin Sheat 2014-03-17 23:09:08 UTC
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.
Comment 13 Katrin Fischer 2014-03-18 21:16:10 UTC
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?
Comment 14 Robin Sheat 2014-03-18 21:30:21 UTC
(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.
Comment 15 HB-NEKLS 2014-03-18 21:44:37 UTC
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.
Comment 16 Robin Sheat 2014-03-18 22:37:54 UTC
(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.
Comment 17 HB-NEKLS 2014-03-19 00:39:31 UTC
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.
Comment 18 HB-NEKLS 2014-03-19 00:40:45 UTC
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.
Comment 19 Robin Sheat 2014-03-21 01:34:04 UTC
(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.
Comment 20 Robin Sheat 2014-03-21 01:42:55 UTC
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"
Comment 21 Robin Sheat 2014-03-21 01:43:36 UTC
I added a version specifically for 3.14.4+ too, the only difference is in the test case.
Comment 22 Kyle M Hall 2014-04-11 11:44:47 UTC
These patches do not appear to be signed off. Changed status to "Needs Signoff".
Comment 23 Owen Leonard 2014-04-11 17:05:09 UTC Comment hidden (obsolete)
Comment 24 Galen Charlton 2014-04-14 18:25:14 UTC
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".
Comment 25 Galen Charlton 2014-04-14 18:32:54 UTC
Bumping up priority -- issues that mangle the priority of hold requests can make life difficult at the circ desk.
Comment 26 Galen Charlton 2014-04-14 18:49:42 UTC
See also bug 12079.
Comment 27 paxed 2014-04-15 09:32:13 UTC
See also bug 12085
Comment 28 Katrin Fischer 2014-04-15 09:41:09 UTC
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.
Comment 29 Katrin Fischer 2014-04-15 09:43:30 UTC
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.
Comment 30 Galen Charlton 2014-04-15 16:26:26 UTC
Pushed to master.  Thanks, Robin!
Comment 31 Fridolin Somers 2014-08-01 13:26:08 UTC
Pushed to 3.14.x, will be in 3.14.10