Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level
Summary: If patron has multiple record level holds on one record transferring first ho...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-19 15:04 UTC by Kyle M Hall
Modified: 2020-10-26 17:23 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Manual merge request:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.05, 19.11.11


Attachments
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level (2.71 KB, patch)
2017-07-19 15:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level (2.93 KB, patch)
2017-07-20 15:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level (3.03 KB, patch)
2017-07-21 13:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level (3.05 KB, patch)
2018-02-02 19:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level (3.10 KB, patch)
2018-02-05 14:35 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level (2.88 KB, patch)
2018-02-26 14:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18958 [QA Followup] - Bring back data needed for the diffbranch block of the template (1.70 KB, patch)
2018-02-26 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level (3.76 KB, patch)
2018-03-02 11:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18958: (Follow-up) Pass patron to template for confirm hold (1.22 KB, patch)
2018-03-02 11:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18958: Unit tests (3.20 KB, patch)
2019-08-12 19:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958: Make check reserves find in transit holds (2.78 KB, patch)
2019-08-12 19:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level (3.72 KB, patch)
2019-09-21 17:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958: Unit tests (3.20 KB, patch)
2019-09-21 17:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958: Make check reserves find in transit holds (2.78 KB, patch)
2019-09-21 17:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level (3.78 KB, patch)
2019-09-21 20:37 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 18958: Unit tests (3.25 KB, patch)
2019-09-21 20:37 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 18958: Make check reserves find in transit holds (2.84 KB, patch)
2019-09-21 20:37 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level (3.83 KB, patch)
2019-09-27 20:17 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 18958: Unit tests (3.31 KB, patch)
2019-09-27 20:17 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 18958: Make check reserves find in transit holds (2.89 KB, patch)
2019-09-27 20:17 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 18958: Make check reserves find in transit holds (2.90 KB, patch)
2020-07-30 11:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958: Unit tests (3.31 KB, patch)
2020-07-30 11:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958: (follow-up) Also find waiting holds for the current item/patron (2.86 KB, patch)
2020-07-30 12:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958: Make check reserves find in transit holds (2.97 KB, patch)
2020-07-30 12:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18958: Unit tests (3.38 KB, patch)
2020-07-30 12:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18958: (follow-up) Also find waiting holds for the current item/patron (2.93 KB, patch)
2020-07-30 12:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18958: [alternate] Make hold_fill_targets specific to reserves (6.48 KB, patch)
2020-08-14 11:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958: [alternate] Make hold_fill_targets specific to reserves (6.17 KB, patch)
2020-08-14 13:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958: Unit tests (3.37 KB, patch)
2020-08-18 11:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958: Make hold_fill_targets specific to reserves (7.77 KB, patch)
2020-08-18 11:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958: Make hold_fill_targets specific to reserves (7.79 KB, patch)
2020-08-18 12:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958: Unit tests (3.43 KB, patch)
2020-08-18 13:03 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 18958: Make hold_fill_targets specific to reserves (7.85 KB, patch)
2020-08-18 13:03 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 18958: Make hold_fill_targets specific to reserves (7.85 KB, patch)
2020-08-22 16:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18958: (QA follow-up) Add missing comma to kohastructure.sql (1015 bytes, patch)
2020-08-22 16:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18958: DO NOT PUSH - Schema updates (1.46 KB, patch)
2020-08-25 10:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958: (follow-up) Ensure hold fill target reserve_id is set for all hold types (4.50 KB, patch)
2020-08-27 10:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18958: Unit tests (3.53 KB, patch)
2020-09-04 07:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18958: Make hold_fill_targets specific to reserves (7.96 KB, patch)
2020-09-04 07:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18958: (QA follow-up) Add missing comma to kohastructure.sql (1.09 KB, patch)
2020-09-04 07:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18958: DO NOT PUSH - Schema updates (1.55 KB, patch)
2020-09-04 07:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18958: (follow-up) Ensure hold fill target reserve_id is set for all hold types (4.46 KB, patch)
2020-09-04 07:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18958: (QA follow-up) Fix number of tests (744 bytes, patch)
2020-09-04 07:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2017-07-19 15:04:41 UTC
Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively. Now, a patron places to record level holds for pickup at LibraryD. When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

To reproduce:
1. Create and use a patron that can place multiple record level holds per record
2. Create a record with X items, each at a different library
3. Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
4. Check in LibraryA's copy as LibraryA and confirm the hold
5. Revisit request.pl for the record, notice the next hold in line is now item-specific
6. Attempt to place another hold for your patron, notice that it requires an item-specific hold
Comment 1 Kyle M Hall 2017-07-19 15:10:15 UTC
Created attachment 65104 [details] [review]
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level

Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively. Now, a patron places to record level holds for pickup at LibraryD. When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

Test Plan:
1. Create and use a patron that can place multiple record level holds per record
2. Create a record with X items, each at a different library
3. Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
4. Check in LibraryA's copy as LibraryA and confirm the hold
5. Revisit request.pl for the record, notice the next hold in line is now item-specific
6. Attempt to place another hold for your patron, notice that it requires an item-specific hold
7. Apply this patch
8. Repeat steps 1-4
9. Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
Comment 2 Kyle M Hall 2017-07-20 15:08:23 UTC
Created attachment 65150 [details] [review]
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level

Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively. Now, a patron places to record level holds for pickup at LibraryD. When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

Test Plan:
1. Create and use a patron that can place multiple record level holds per record
2. Create a record with X items, each at a different library
3. Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
4. Rebuild the holds queue
5. Check in LibraryA's copy as LibraryA and confirm the hold
6. Revisit request.pl for the record, notice the next hold in line is now item-specific
7. Attempt to place another hold for your patron, notice that it requires an item-specific hold
8. Apply this patch
9. Repeat steps 1-4
10. Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
Comment 3 Kyle M Hall 2017-07-21 13:50:40 UTC
Created attachment 65184 [details] [review]
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level

Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively. Now, a patron places to record level holds for pickup at LibraryD. When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

Test Plan:
1. Create and use a patron that can place multiple record level holds per record
2. Create a record with X items, each at a different library
3. Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
4. Rebuild the holds queue
5. Check in LibraryA's copy as LibraryA and confirm the hold
6. Revisit request.pl for the record, notice the next hold in line is now item-specific
7. Attempt to place another hold for your patron, notice that it requires an item-specific hold
8. Apply this patch
9. Repeat steps 1-4
10. Revisit request.pl for the record, notice the next hold in line has *not* become item-specific

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jason Robb <jrobb@sekls.org>
Comment 4 Kyle M Hall 2018-02-02 19:05:15 UTC
Created attachment 71164 [details] [review]
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level

Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively. Now, a patron places to record level holds for pickup at LibraryD. When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

Test Plan:
1. Create and use a patron that can place multiple record level holds per record
2. Create a record with X items, each at a different library
3. Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
4. Rebuild the holds queue
5. Check in LibraryA's copy as LibraryA and confirm the hold
6. Revisit request.pl for the record, notice the next hold in line is now item-specific
7. Attempt to place another hold for your patron, notice that it requires an item-specific hold
8. Apply this patch
9. Repeat steps 1-4
10. Revisit request.pl for the record, notice the next hold in line has *not* become item-specific

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jason Robb <jrobb@sekls.org>
Comment 5 Josef Moravec 2018-02-05 14:35:56 UTC
Created attachment 71232 [details] [review]
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level

Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively. Now, a patron places to record level holds for pickup at LibraryD. When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

Test Plan:
1. Create and use a patron that can place multiple record level holds per record
2. Create a record with X items, each at a different library
3. Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
4. Rebuild the holds queue
5. Check in LibraryA's copy as LibraryA and confirm the hold
6. Revisit request.pl for the record, notice the next hold in line is now item-specific
7. Attempt to place another hold for your patron, notice that it requires an item-specific hold
8. Apply this patch
9. Repeat steps 1-4
10. Revisit request.pl for the record, notice the next hold in line has *not* become item-specific

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jason Robb <jrobb@sekls.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 6 Jonathan Druart 2018-02-08 19:44:12 UTC
Hum, I am going to postpone this change for my mental health (holds...)

But at first glance there is something wrong: the diffbranch flag will never be set and we will have a lot of useless code in the template.

Are you sure this change is correct?
Comment 7 Kyle M Hall 2018-02-22 18:04:32 UTC
(In reply to Jonathan Druart from comment #6)
> Hum, I am going to postpone this change for my mental health (holds...)
> 
> But at first glance there is something wrong: the diffbranch flag will never
> be set and we will have a lot of useless code in the template.
> 
> Are you sure this change is correct?

I'm not sure what you mean. This doesn't change ModItemTransfer at all. It only replaces a bunch of code with a call to ModItemTransfer *if* the item needs transferred. This reduces code and complexity, and does not affect any other use of ModItemTransfer.
Comment 8 Jonathan Druart 2018-02-26 13:59:25 UTC
(In reply to Kyle M Hall from comment #7)
> (In reply to Jonathan Druart from comment #6)
> > Hum, I am going to postpone this change for my mental health (holds...)
> > 
> > But at first glance there is something wrong: the diffbranch flag will never
> > be set and we will have a lot of useless code in the template.
> > 
> > Are you sure this change is correct?
> 
> I'm not sure what you mean. This doesn't change ModItemTransfer at all. It
> only replaces a bunch of code with a call to ModItemTransfer *if* the item
> needs transferred. This reduces code and complexity, and does not affect any
> other use of ModItemTransfer.

I was not talking about ModItemTransfer.

You removed this line:
-            diffbranch     => 1,

diffbranch is only sent to the template once, after your patch it will never be set.
Comment 9 Kyle M Hall 2018-02-26 14:23:54 UTC
Created attachment 72210 [details] [review]
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level

Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively. Now, a patron places to record level holds for pickup at LibraryD. When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

Test Plan:
1. Create and use a patron that can place multiple record level holds per record
2. Create a record with X items, each at a different library
3. Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
4. Rebuild the holds queue
5. Check in LibraryA's copy as LibraryA and confirm the hold
6. Revisit request.pl for the record, notice the next hold in line is now item-specific
7. Attempt to place another hold for your patron, notice that it requires an item-specific hold
8. Apply this patch
9. Repeat steps 1-4
10. Revisit request.pl for the record, notice the next hold in line has *not* become item-specific

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jason Robb <jrobb@sekls.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Kyle M Hall 2018-02-26 14:24:01 UTC
Created attachment 72211 [details] [review]
Bug 18958 [QA Followup] - Bring back data needed for the diffbranch block of the template
Comment 11 Marcel de Rooy 2018-03-02 08:05:48 UTC
QA: Looking here
Comment 12 Marcel de Rooy 2018-03-02 09:00:05 UTC
Still working on it. Suspect that we are not ready yet.. Moving to BLOCKED temporarily
Comment 13 Marcel de Rooy 2018-03-02 11:13:14 UTC
Created attachment 72348 [details] [review]
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level

Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively. Now, a patron places to record level holds for pickup at LibraryD. When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

Test Plan:
1. Create and use a patron that can place multiple record level holds per record
2. Create a record with X items, each at a different library
3. Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
4. Rebuild the holds queue
5. Check in LibraryA's copy as LibraryA and confirm the hold
6. Revisit request.pl for the record, notice the next hold in line is now item-specific
7. Attempt to place another hold for your patron, notice that it requires an item-specific hold
8. Apply this patch
9. Repeat steps 1-4
10. Revisit request.pl for the record, notice the next hold in line has *not* become item-specific

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jason Robb <jrobb@sekls.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 18958 [QA Followup] - Bring back data needed for the diffbranch block of the template
Comment 14 Marcel de Rooy 2018-03-02 11:13:19 UTC
Created attachment 72349 [details] [review]
Bug 18958: (Follow-up) Pass patron to template for confirm hold

Removing the FIXME. Trivial fix.
Not sure where to put it; can be moved on its own.

Test plan:
Confirm a hold and verify that patron details are shown in popup.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2018-03-02 11:18:59 UTC
Squashed both patches. Follow-up may need to be moved.
Tested this quite a bit. Hard to reproduce at first. (Alternative test plan in next comment.)
Failing QA because this patch has side-effects when you cancel a hold from the confirm hold popup. Before we called GetOtherReserves and now we dont. This changes behavior.
Also I am wondering if the passed patron etc. at lines 160/170 makes any sense. If you confirm the transfer or cancel, you come back to the default returns.pl form.
Comment 16 Marcel de Rooy 2018-03-02 11:21:00 UTC
[Without patch]
Three items (CPL MPL RMA)
2 record level patron A and item level from patron B (CPL)
Checkin CPL item at RMA
Confirm
Waiting OK
Checkin this item again and Cancel
Finally reproduced the error: Second patron A hold became Item level (Bug)
Checkin again, confirm
Checkin and cancel
What happened? Priority last hold = 1. Transfer created. No message to staff. (Bug: transit should be a T hold?)

[With patch]
Three items
2 record level for patron A
1 item level for patron B on CPL item
Goto RMA Checkin CPL item, confirm for patron A Waiting
So far so good. Checkin again and cancel.
Second record level hold at 1. This cancel created no transfer. No message. (Behavior change?)
Checkin again, confirm. Waiting OK
Checkin again, cancel.
Last item level hold at 1. No transfer. No message.
Comment 17 Kyle M Hall 2018-03-23 13:21:13 UTC
(In reply to Marcel de Rooy from comment #16)
> [With patch]
> Three items
> 2 record level for patron A
> 1 item level for patron B on CPL item
> Goto RMA Checkin CPL item, confirm for patron A Waiting
> So far so good. Checkin again and cancel.
> Second record level hold at 1. This cancel created no transfer. No message.
> (Behavior change?)
> Checkin again, confirm. Waiting OK
> Checkin again, cancel.
> Last item level hold at 1. No transfer. No message.

Could you restate what the change in behavior is? I'm not sure I understand. Thanks!
Comment 18 Marcel de Rooy 2018-04-06 06:55:24 UTC
(In reply to Kyle M Hall from comment #17)
> Could you restate what the change in behavior is? I'm not sure I understand.
> Thanks!

Holds are not easy :)
See: Failing QA because this patch has side-effects when you cancel a hold from the confirm hold popup. Before we called GetOtherReserves and now we dont. This changes behavior.
Note that GetOtherReserves does not only 'get' but may call ModItemTransfer.

In my 'test plan':
> So far so good. Checkin again and cancel.
> Second record level hold at 1. This cancel created no transfer. No message.
> (Behavior change?)
This confirms that we no longer call ModItemTransfer.
Comment 19 Andrew Fuerste-Henry 2019-07-11 14:43:27 UTC
Hi all,

I don't know if this will prove useful or not, but I was testing this behavior out before I found that there was already a bug for it and noticed something bad/interesting:

- have 3 bib-level holds for the same patron to be picked up at Branch A, reserve_ids 1,2, and 3
- check item in at Branch B
- reserve 1 goes to transit, reserve 2 becomes item-level, reserve 3 unchange
- check item in at Branch A
- reserve 1 REMAINS in transit, reserve 2 becomes Waiting, reserve 3 becomes item-level

Apologies if this is irrelevant and/or something you've all noticed already.
Comment 20 Nick Clemens 2019-08-12 19:06:47 UTC
Created attachment 92159 [details] [review]
Bug 18958: Unit tests
Comment 21 Nick Clemens 2019-08-12 19:06:50 UTC
Created attachment 92160 [details] [review]
Bug 18958: Make check reserves find in transit holds

To test:
Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively.
Now, a patron places two record level holds for pickup at LibraryD.
When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is
that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

The problem seems to be that we use GetOtherReserves to initiate transfers for holds
This routine calls CheckReserves to see what holds are on the record. We have an implicit assumption here
that anything matching the holds queue is the correct hold. This worked when we couldn't have multiple holds on a record,
but now it can end up with false positive.

The solution seems to be returning the in-transit hold before returning a second hold on the record to make sure we affect
the correct reserve.

Test Plan:
 1 - Create and use a patron that can place multiple record level holds per record
 2 - Create a record with X items, each at a different library
 3 - Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
 4 - perl misc/cronjobs/holds/build_holdsqueue.pl
 5 - Check in LibraryA's copy as LibraryA and confirm the hold
 6 - Revisit request.pl for the record, notice the next hold in line is now item-specific
 7 - Checkout the item to the patron, notice the remaining hold is marked waiting
 8 - Attempt to place another hold for your patron, notice that it requires an item-specific hold
 8 - Apply this patch
 9 - Repeat steps 1-5
10 - Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
11 - Checkout the item to the patron, ensure the first hold is filled and the second remains record level
12 - Repeat whole test plan without building holds queue to confirm holds are still treated correctly
Comment 22 Bonnie Gardner 2019-09-21 17:23:24 UTC
Kyle,

I tested this at koha-US 2019 and it worked properly.
When we tried to sign it off in the BWS sandboxes we got this message:

Rewrite 44049ad57e1928f2870abed15e4659f8357c2f19 (1/4) (0 seconds passed, remaining 0 predicted)
Rewrite 957d583d2efce66e31fe05f229fda91c58324bc2 (2/4) (0 seconds passed, remaining 0 predicted)
Rewrite 36c734e68438b40401d0eee8ae823ae05da7ce79 (3/4) (0 seconds passed, remaining 0 predicted)
Rewrite f245f5ed00da991cd3284ea26e37b3fcebbd90b1 (4/4) (0 seconds passed, remaining 0 predicted)
Ref 'refs/heads/master' was rewritten
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level
4309a6c Bug 21852: Add more columns and column configuration to overdues report
7ce3f2a Bug 21390: Send registration verification emails immediately
838c3ff Bug 21390: Send registration verification emails immediately
e6a76dc Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level
Attach? [yn] Moving to starting point
Adding bug reference   4309a6c Bug 21852: Add more columns and column configuration to overdues report
Adding bug reference   7ce3f2a Bug 21390: Send registration verification emails immediately
Adding bug reference   838c3ff Bug 21390: Send registration verification emails immediately
HEAD detached from e7a84dacfe
You are in the middle of an am session.
nothing to commit, working tree clean
Cleaning up back to original state on error
Comment 23 Nick Clemens 2019-09-21 17:40:27 UTC
Created attachment 93106 [details] [review]
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level

Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively. Now, a patron places to record level holds for pickup at LibraryD. When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

Test Plan:
1. Create and use a patron that can place multiple record level holds per record
2. Create a record with X items, each at a different library
3. Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
4. Rebuild the holds queue
5. Check in LibraryA's copy as LibraryA and confirm the hold
6. Revisit request.pl for the record, notice the next hold in line is now item-specific
7. Attempt to place another hold for your patron, notice that it requires an item-specific hold
8. Apply this patch
9. Repeat steps 1-4
10. Revisit request.pl for the record, notice the next hold in line has *not* become item-specific

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jason Robb <jrobb@sekls.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 18958 [QA Followup] - Bring back data needed for the diffbranch block of the template
Comment 24 Nick Clemens 2019-09-21 17:40:33 UTC
Created attachment 93107 [details] [review]
Bug 18958: Unit tests
Comment 25 Nick Clemens 2019-09-21 17:40:39 UTC
Created attachment 93108 [details] [review]
Bug 18958: Make check reserves find in transit holds

To test:
Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively.
Now, a patron places two record level holds for pickup at LibraryD.
When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is
that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

The problem seems to be that we use GetOtherReserves to initiate transfers for holds
This routine calls CheckReserves to see what holds are on the record. We have an implicit assumption here
that anything matching the holds queue is the correct hold. This worked when we couldn't have multiple holds on a record,
but now it can end up with false positive.

The solution seems to be returning the in-transit hold before returning a second hold on the record to make sure we affect
the correct reserve.

Test Plan:
 1 - Create and use a patron that can place multiple record level holds per record
 2 - Create a record with X items, each at a different library
 3 - Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
 4 - perl misc/cronjobs/holds/build_holdsqueue.pl
 5 - Check in LibraryA's copy as LibraryA and confirm the hold
 6 - Revisit request.pl for the record, notice the next hold in line is now item-specific
 7 - Checkout the item to the patron, notice the remaining hold is marked waiting
 8 - Attempt to place another hold for your patron, notice that it requires an item-specific hold
 8 - Apply this patch
 9 - Repeat steps 1-5
10 - Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
11 - Checkout the item to the patron, ensure the first hold is filled and the second remains record level
12 - Repeat whole test plan without building holds queue to confirm holds are still treated correctly
Comment 26 Nick Clemens 2019-09-21 17:41:52 UTC
(In reply to Bonnie Gardner from comment #22)
> Kyle,
> 
> I tested this at koha-US 2019 and it worked properly.
> When we tried to sign it off in the BWS sandboxes we got this message:

Hi Bonnie,

It looks like there was a conflict. I rebased the patches and they should apply cleanly and allow you to sign off.

-Nick
Comment 27 ByWater Sandboxes 2019-09-21 20:37:29 UTC
Created attachment 93111 [details] [review]
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level

Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively. Now, a patron places to record level holds for pickup at LibraryD. When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

Test Plan:
1. Create and use a patron that can place multiple record level holds per record
2. Create a record with X items, each at a different library
3. Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
4. Rebuild the holds queue
5. Check in LibraryA's copy as LibraryA and confirm the hold
6. Revisit request.pl for the record, notice the next hold in line is now item-specific
7. Attempt to place another hold for your patron, notice that it requires an item-specific hold
8. Apply this patch
9. Repeat steps 1-4
10. Revisit request.pl for the record, notice the next hold in line has *not* become item-specific

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jason Robb <jrobb@sekls.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 18958 [QA Followup] - Bring back data needed for the diffbranch block of the template

Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
Comment 28 ByWater Sandboxes 2019-09-21 20:37:32 UTC
Created attachment 93112 [details] [review]
Bug 18958: Unit tests

Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
Comment 29 ByWater Sandboxes 2019-09-21 20:37:35 UTC
Created attachment 93113 [details] [review]
Bug 18958: Make check reserves find in transit holds

To test:
Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively.
Now, a patron places two record level holds for pickup at LibraryD.
When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is
that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

The problem seems to be that we use GetOtherReserves to initiate transfers for holds
This routine calls CheckReserves to see what holds are on the record. We have an implicit assumption here
that anything matching the holds queue is the correct hold. This worked when we couldn't have multiple holds on a record,
but now it can end up with false positive.

The solution seems to be returning the in-transit hold before returning a second hold on the record to make sure we affect
the correct reserve.

Test Plan:
 1 - Create and use a patron that can place multiple record level holds per record
 2 - Create a record with X items, each at a different library
 3 - Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
 4 - perl misc/cronjobs/holds/build_holdsqueue.pl
 5 - Check in LibraryA's copy as LibraryA and confirm the hold
 6 - Revisit request.pl for the record, notice the next hold in line is now item-specific
 7 - Checkout the item to the patron, notice the remaining hold is marked waiting
 8 - Attempt to place another hold for your patron, notice that it requires an item-specific hold
 8 - Apply this patch
 9 - Repeat steps 1-5
10 - Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
11 - Checkout the item to the patron, ensure the first hold is filled and the second remains record level
12 - Repeat whole test plan without building holds queue to confirm holds are still treated correctly

Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
Comment 30 Bouzid Fergani 2019-09-27 20:17:18 UTC
Created attachment 93216 [details] [review]
Bug 18958 - If patron has multiple record level holds on one record transferring first hold causes next hold to become item level

Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively. Now, a patron places to record level holds for pickup at LibraryD. When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

Test Plan:
1. Create and use a patron that can place multiple record level holds per record
2. Create a record with X items, each at a different library
3. Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
4. Rebuild the holds queue
5. Check in LibraryA's copy as LibraryA and confirm the hold
6. Revisit request.pl for the record, notice the next hold in line is now item-specific
7. Attempt to place another hold for your patron, notice that it requires an item-specific hold
8. Apply this patch
9. Repeat steps 1-4
10. Revisit request.pl for the record, notice the next hold in line has *not* become item-specific

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jason Robb <jrobb@sekls.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 18958 [QA Followup] - Bring back data needed for the diffbranch block of the template

Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 31 Bouzid Fergani 2019-09-27 20:17:25 UTC
Created attachment 93217 [details] [review]
Bug 18958: Unit tests

Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 32 Bouzid Fergani 2019-09-27 20:17:32 UTC
Created attachment 93218 [details] [review]
Bug 18958: Make check reserves find in transit holds

To test:
Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively.
Now, a patron places two record level holds for pickup at LibraryD.
When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is
that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

The problem seems to be that we use GetOtherReserves to initiate transfers for holds
This routine calls CheckReserves to see what holds are on the record. We have an implicit assumption here
that anything matching the holds queue is the correct hold. This worked when we couldn't have multiple holds on a record,
but now it can end up with false positive.

The solution seems to be returning the in-transit hold before returning a second hold on the record to make sure we affect
the correct reserve.

Test Plan:
 1 - Create and use a patron that can place multiple record level holds per record
 2 - Create a record with X items, each at a different library
 3 - Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
 4 - perl misc/cronjobs/holds/build_holdsqueue.pl
 5 - Check in LibraryA's copy as LibraryA and confirm the hold
 6 - Revisit request.pl for the record, notice the next hold in line is now item-specific
 7 - Checkout the item to the patron, notice the remaining hold is marked waiting
 8 - Attempt to place another hold for your patron, notice that it requires an item-specific hold
 8 - Apply this patch
 9 - Repeat steps 1-5
10 - Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
11 - Checkout the item to the patron, ensure the first hold is filled and the second remains record level
12 - Repeat whole test plan without building holds queue to confirm holds are still treated correctly

Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 33 Katrin Fischer 2019-10-17 06:11:30 UTC
Pretty sure I am missing something here, but I can't reproduce the problem on current master. - But I am pretty sure I did see the problem a longer while ago. Is it possible this was solved already?

Test Plan:
1. Create and use a patron that can place multiple record level holds per record
2. Create a record with X items, each at a different library
3. Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
4. Rebuild the holds queue
5. Check in LibraryA's copy as LibraryA and confirm the hold
6. Revisit request.pl for the record, notice the next hold in line is now item-specific

Why is the rebuild of the holds_queue required here?
Comment 34 Nick Clemens 2019-10-17 12:55:37 UTC
(In reply to Katrin Fischer from comment #33)
> Pretty sure I am missing something here, but I can't reproduce the problem
> on current master. - But I am pretty sure I did see the problem a longer
> while ago. Is it possible this was solved already?
> 
> Test Plan:
> 1. Create and use a patron that can place multiple record level holds per
> record
> 2. Create a record with X items, each at a different library
> 3. Place X 'Next available' holds on the record for the patron using the
> 'Holds to place' box
> 4. Rebuild the holds queue
> 5. Check in LibraryA's copy as LibraryA and confirm the hold
> 6. Revisit request.pl for the record, notice the next hold in line is now
> item-specific
> 
> Why is the rebuild of the holds_queue required here?

The holds queue is checked in _Findgroupreserve and Getotherreserves - there is code in there that alters the holds, but it only happens when they are in the holds queue
Comment 35 Marcel de Rooy 2019-11-01 07:07:33 UTC
QAing
Comment 36 Marcel de Rooy 2019-11-01 07:44:14 UTC
(In reply to Marcel de Rooy from comment #15)
> Failing QA because this patch has side-effects when you cancel a hold from
> the confirm hold popup. Before we called GetOtherReserves and now we dont.

Still looking at my older comment, and having the feeling that it was kind of ignored.
Let me rephrase it by showing this diff part:

     } else {
         my $diffBranchSend = ($userenv_branch ne $diffBranchReturned) ? $diffBranchReturned : undef;
+
         # diffBranchSend tells ModReserveAffect whether document is expected in this library or not,
         # i.e., whether to apply waiting status
         ModReserveAffect( $itemnumber, $borrowernumber, $diffBranchSend, $reserve_id );
-    }
-#   check if we have other reserves for this document, if we have a return send the message of transfer
-    my ( $messages, $nextreservinfo ) = GetOtherReserves($itemnumber);

As you can see here, the GetOtherReserves call and code around it was placed AFTER the else, but we remove the closing accolade and put the new code INTO the else branch.
So what happens: if you come from the if branch (you canceled a hold), you wont jump into the GetOtherReserves stuff anymore.
Still does not look good to me. Please convince me with some good arguments!
Comment 37 Nick Clemens 2020-07-30 11:41:35 UTC
Created attachment 107576 [details] [review]
Bug 18958: Make check reserves find in transit holds

To test:
Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively.
Now, a patron places two record level holds for pickup at LibraryD.
When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is
that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

The problem seems to be that we use GetOtherReserves to initiate transfers for holds
This routine calls CheckReserves to see what holds are on the record. We have an implicit assumption here
that anything matching the holds queue is the correct hold. This worked when we couldn't have multiple holds on a record,
but now it can end up with false positive.

The solution seems to be returning the in-transit hold before returning a second hold on the record to make sure we affect
the correct reserve.

Test Plan:
 1 - Create and use a patron that can place multiple record level holds per record
 2 - Create a record with X items, each at a different library
 3 - Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
 4 - perl misc/cronjobs/holds/build_holdsqueue.pl
 5 - Check in LibraryA's copy as LibraryA and confirm the hold
 6 - Revisit request.pl for the record, notice the next hold in line is now item-specific
 7 - Checkout the item to the patron, notice the remaining hold is marked waiting
 8 - Attempt to place another hold for your patron, notice that it requires an item-specific hold
 8 - Apply this patch
 9 - Repeat steps 1-5
10 - Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
11 - Checkout the item to the patron, ensure the first hold is filled and the second remains record level
12 - Repeat whole test plan without building holds queue to confirm holds are still treated correctly

Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 38 Nick Clemens 2020-07-30 11:42:07 UTC
Created attachment 107577 [details] [review]
Bug 18958: Unit tests

Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 39 Nick Clemens 2020-07-30 11:45:48 UTC
(In reply to Marcel de Rooy from comment #36)
> (In reply to Marcel de Rooy from comment #15)
> > Failing QA because this patch has side-effects when you cancel a hold from
> > the confirm hold popup. Before we called GetOtherReserves and now we dont.
> 
I obsoleted Kyle's patch in favor of mine - I think I was confused before and added mine when I meant to replace

Kyle's patch attempted to prevent the call to GetOtherReserves to fix the problem

My patch alters _Findgroupreserves to find the correct hold which should fix the call to GetOtherReserves
(GetOtherReserves calls CheckReserves call _Findgroupreserves)
Comment 40 Nick Clemens 2020-07-30 12:37:53 UTC
Created attachment 107582 [details] [review]
Bug 18958: (follow-up) Also find waiting holds for the current item/patron

In the first patch we deal with the situation where the pickup location doesn't match the checked in location
This patch takes care fo the situation for holds not being transferred

Same test plan, but holds and items can all be for single library
Comment 41 Kyle M Hall 2020-07-30 12:45:39 UTC
Created attachment 107584 [details] [review]
Bug 18958: Make check reserves find in transit holds

To test:
Imagine you have a record with 3 items, each item is held by LibraryA, LibraryB and LibraryC respectively.
Now, a patron places two record level holds for pickup at LibraryD.
When any of the items are checked in, that item is trapped to fill the first hold, but a side affect is
that the next open hold because item level for the item that was just checked in!

This is clearly incorrect and prevents the patron from placing more record level holds on the record.

The problem seems to be that we use GetOtherReserves to initiate transfers for holds
This routine calls CheckReserves to see what holds are on the record. We have an implicit assumption here
that anything matching the holds queue is the correct hold. This worked when we couldn't have multiple holds on a record,
but now it can end up with false positive.

The solution seems to be returning the in-transit hold before returning a second hold on the record to make sure we affect
the correct reserve.

Test Plan:
 1 - Create and use a patron that can place multiple record level holds per record
 2 - Create a record with X items, each at a different library
 3 - Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
 4 - perl misc/cronjobs/holds/build_holdsqueue.pl
 5 - Check in LibraryA's copy as LibraryA and confirm the hold
 6 - Revisit request.pl for the record, notice the next hold in line is now item-specific
 7 - Checkout the item to the patron, notice the remaining hold is marked waiting
 8 - Attempt to place another hold for your patron, notice that it requires an item-specific hold
 8 - Apply this patch
 9 - Repeat steps 1-5
10 - Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
11 - Checkout the item to the patron, ensure the first hold is filled and the second remains record level
12 - Repeat whole test plan without building holds queue to confirm holds are still treated correctly

Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 42 Kyle M Hall 2020-07-30 12:45:51 UTC
Created attachment 107585 [details] [review]
Bug 18958: Unit tests

Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Kyle M Hall 2020-07-30 12:45:55 UTC
Created attachment 107586 [details] [review]
Bug 18958: (follow-up) Also find waiting holds for the current item/patron

In the first patch we deal with the situation where the pickup location doesn't match the checked in location
This patch takes care fo the situation for holds not being transferred

Same test plan, but holds and items can all be for single library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 44 Katrin Fischer 2020-07-30 22:37:11 UTC
Marcel - maybe 3rd time this will pass? :) Do you want to continue here?
Comment 45 Marcel de Rooy 2020-08-03 06:05:42 UTC
(In reply to Katrin Fischer from comment #44)
> Marcel - maybe 3rd time this will pass? :) Do you want to continue here?

Will do
Comment 46 Marcel de Rooy 2020-08-14 08:57:29 UTC
This is so horrible. Just checking occurrences of CheckReserves calls. Here we are in Circulation::CanBookBeRenewed.
First we call CheckReserves, but then note the $itemholds=search call and repeated calls to CheckReserves in the while(1)..

    my ( $resfound, $resrec, undef ) = C4::Reserves::CheckReserves($itemnumber);

    # This item can fill one or more unfilled reserve, can those unfilled reserves
    # all be filled by other available items?
    if ( $resfound
        && C4::Context->preference('AllowRenewalIfOtherItemsAvailable') )
    {
        my $schema = Koha::Database->new()->schema();

        my $item_holds = $schema->resultset('Reserve')->search( { itemnumber => $itemnumber, found => undef } )->count();
        if ($item_holds) {
            # There is an item level hold on this item, no other item can fill the hold
            $resfound = 1;
        }
        else {

            # Get all other items that could possibly fill reserves
            my @itemnumbers = $schema->resultset('Item')->search(
                {
                    biblionumber => $resrec->{biblionumber},
                    onloan       => undef,
                    notforloan   => 0,
                    -not         => { itemnumber => $itemnumber }
                },
                { columns => 'itemnumber' }
            )->get_column('itemnumber')->all();

            # Get all other reserves that could have been filled by this item
            my @borrowernumbers;
            while (1) {
                my ( $reserve_found, $reserve, undef ) =
                  C4::Reserves::CheckReserves( $itemnumber, undef, undef, \@borrowernumbers );
Comment 47 Marcel de Rooy 2020-08-14 09:44:41 UTC
Just for reference:

PASS t/db_dependent/Circulation.t
PASS t/db_dependent/Holds.t
PASS t/db_dependent/Holds/HoldFulfillmentPolicy.t
PASS t/db_dependent/Holds/HoldItemtypeLimit.t
PASS t/db_dependent/Holds/LocalHoldsPriority.t
PASS t/db_dependent/Reserves.t

_Findgroupreserve only called by CheckReserves

CheckReserves is used by:
C4/Circulation.pm
* transferbook, CanBookBeIssued, AddReturn, CanBookBeRenewed
C4/Reserves.pm
* GetOtherReserves, MoveReserve
C4/RotatingCollections.pm
* TransferCollection
Comment 48 Marcel de Rooy 2020-08-14 09:55:25 UTC
Just puzzled by this change. Since CheckReserves is such a vital routine. How is it possible that we just now need to add waiting and transit holds? Related to use of holds queue?

Please note that _Findgroupreserve contains three queries. The first one is item level targeted. Why didnt you add the clause here? Since waiting and transit are item level.

If no results, the second is run. Which should be "title level". But inconsistently it does not contain an itemnumber IS NULL clause. And that fact helps you now to add the W/T clause. Does not look very consistent to me.

The first two runs are based on hold_fill_targets. If there are no results, the third query gets record and item level holds from the reserves table.
My question here is: If we do not change the second query, the third one should find them too, since it does not contain a clause on found?

Changing status for feedback.
Comment 49 Nick Clemens 2020-08-14 11:07:09 UTC
(In reply to Marcel de Rooy from comment #48)
> Just puzzled by this change. Since CheckReserves is such a vital routine.
> How is it possible that we just now need to add waiting and transit holds?
> Related to use of holds queue?

Related to the fact that once upon a time a patron could only have a single hold per biblio. We didn't run the risk of returning the wrong hold for a borrower. The problem we are facing specifically is triggered by the holds queue as well

> 
> Please note that _Findgroupreserve contains three queries. The first one is
> item level targeted. Why didnt you add the clause here? Since waiting and
> transit are item level.
> 

If a patron has multiple item level holds, no problem. Each has a specific itemnumber and holdsqueue can only select one item

If a patron has multiple title level holds but at least one entry in hold_fill_targets is where we hit the problem. The second title level hold joins to the hold_fill_target that was really meant for the first hold. So we end up returning only the second title level hold in the second query - this is where we need to make sure to return the hold that we just marked waiting/transit

Look at the unit test, that helps clarify the problem - run it without patches, always get the wrong reserve_id

> If no results, the second is run. Which should be "title level". But
> inconsistently it does not contain an itemnumber IS NULL clause. And that
> fact helps you now to add the W/T clause. Does not look very consistent to
> me.

It is title level, but title level joins to an item level holds fill target

> 
> The first two runs are based on hold_fill_targets. If there are no results,
> the third query gets record and item level holds from the reserves table.
> My question here is: If we do not change the second query, the third one
> should find them too, since it does not contain a clause on found?

The problem is not the third query not returning the hold - it is the second query returning the wrong hold

Alternate patch coming
Comment 50 Nick Clemens 2020-08-14 11:11:15 UTC
Created attachment 108256 [details] [review]
Bug 18958: [alternate] Make hold_fill_targets specific to reserves

After looking at Marcel's comments, the problem is in our matching
to hold_fill_targets - rather than adjusting to find filled/waiting holds we
could ensure that hold_fill_targets only refers to the specific hold it
is intended to

Use same test plan as other patches.

This patch is clearer, if slightly less performant than last (we now return all
the reserves and have to find the 'highest')
Comment 51 Nick Clemens 2020-08-14 13:01:03 UTC
Created attachment 108274 [details] [review]
Bug 18958: [alternate] Make hold_fill_targets specific to reserves

After looking at Marcel's comments, the problem is in our matching
to hold_fill_targets - rather than adjusting to find filled/waiting holds we
could ensure that hold_fill_targets only refers to the specific hold it
is intended to

Use same test plan as other patches.

This patch is clearer, if slightly less performant than last (we now return all
the reserves and have to find the 'highest')
Comment 52 Nick Clemens 2020-08-18 11:51:17 UTC
Created attachment 108473 [details] [review]
Bug 18958: Unit tests

Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 53 Nick Clemens 2020-08-18 11:51:22 UTC
Created attachment 108474 [details] [review]
Bug 18958: Make hold_fill_targets specific to reserves

After looking at Marcel's comments, the problem is in our matching
to hold_fill_targets - rather than adjusting to find filled/waiting holds we
could ensure that hold_fill_targets only refers to the specific hold it
is intended to

This patch is clearer, if slightly less performant than last (we now return all
the reserves and have to find the 'highest')

Test Plan:
 1 - Create and use a patron that can place multiple record level holds per record
 2 - Create a record with X items, each at a different library
 3 - Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
 4 - perl misc/cronjobs/holds/build_holdsqueue.pl
 5 - Check in LibraryA's copy as LibraryA and confirm the hold
 6 - Revisit request.pl for the record, notice the next hold in line is now item-specific
 7 - Checkout the item to the patron, notice the remaining hold is marked waiting
 8 - Attempt to place another hold for your patron, notice that it requires an item-specific hold
 8 - Apply this patch
 9 - Repeat steps 1-5
10 - Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
11 - Checkout the item to the patron, ensure the first hold is filled and the second remains record level
12 - Repeat whole test plan without building holds queue to confirm holds are still treated correctly
Comment 54 Andrew Fuerste-Henry 2020-08-18 12:40:20 UTC
I was able to follow the whole test plan and it all worked as expected, except I got this error when building the holds queue: 
DBD::mysql::st execute failed: called with 6 bind variables when 5 are needed [for Statement "
        INSERT INTO hold_fill_targets (borrowernumber, biblionumber, itemnumber, source_branchcode, item_level_request, reserve_id)
                               VALUES (?, ?, ?, ?, ?)
    " with ParamValues: 0=Null!, 1=Null!, 2=Null!, 3=Null!, 4=Null!] at /kohadevbox/koha/C4/HoldsQueue.pm line 729.
Comment 55 Nick Clemens 2020-08-18 12:55:37 UTC
Created attachment 108497 [details] [review]
Bug 18958: Make hold_fill_targets specific to reserves

After looking at Marcel's comments, the problem is in our matching
to hold_fill_targets - rather than adjusting to find filled/waiting holds we
could ensure that hold_fill_targets only refers to the specific hold it
is intended to

This patch is clearer, if slightly less performant than last (we now return all
the reserves and have to find the 'highest')

Test Plan:
 1 - Create and use a patron that can place multiple record level holds per record
 2 - Create a record with X items, each at a different library
 3 - Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
 4 - perl misc/cronjobs/holds/build_holdsqueue.pl
 5 - Check in LibraryA's copy as LibraryA and confirm the hold
 6 - Revisit request.pl for the record, notice the next hold in line is now item-specific
 7 - Checkout the item to the patron, notice the remaining hold is marked waiting
 8 - Attempt to place another hold for your patron, notice that it requires an item-specific hold
 8 - Apply this patch
 9 - Repeat steps 1-5
10 - Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
11 - Checkout the item to the patron, ensure the first hold is filled and the second remains record level
12 - Repeat whole test plan without building holds queue to confirm holds are still treated correctly
Comment 56 Andrew Fuerste-Henry 2020-08-18 13:03:24 UTC
Created attachment 108499 [details] [review]
Bug 18958: Unit tests

Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 57 Andrew Fuerste-Henry 2020-08-18 13:03:29 UTC
Created attachment 108500 [details] [review]
Bug 18958: Make hold_fill_targets specific to reserves

After looking at Marcel's comments, the problem is in our matching
to hold_fill_targets - rather than adjusting to find filled/waiting holds we
could ensure that hold_fill_targets only refers to the specific hold it
is intended to

This patch is clearer, if slightly less performant than last (we now return all
the reserves and have to find the 'highest')

Test Plan:
 1 - Create and use a patron that can place multiple record level holds per record
 2 - Create a record with X items, each at a different library
 3 - Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
 4 - perl misc/cronjobs/holds/build_holdsqueue.pl
 5 - Check in LibraryA's copy as LibraryA and confirm the hold
 6 - Revisit request.pl for the record, notice the next hold in line is now item-specific
 7 - Checkout the item to the patron, notice the remaining hold is marked waiting
 8 - Attempt to place another hold for your patron, notice that it requires an item-specific hold
 8 - Apply this patch
 9 - Repeat steps 1-5
10 - Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
11 - Checkout the item to the patron, ensure the first hold is filled and the second remains record level
12 - Repeat whole test plan without building holds queue to confirm holds are still treated correctly

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 58 Katrin Fischer 2020-08-22 16:37:10 UTC
The test worked for me, but the reserve_id column remained NULL at all times - can you please explain?
Comment 59 Katrin Fischer 2020-08-22 16:37:59 UTC
Created attachment 108936 [details] [review]
Bug 18958: Make hold_fill_targets specific to reserves

After looking at Marcel's comments, the problem is in our matching
to hold_fill_targets - rather than adjusting to find filled/waiting holds we
could ensure that hold_fill_targets only refers to the specific hold it
is intended to

This patch is clearer, if slightly less performant than last (we now return all
the reserves and have to find the 'highest')

Test Plan:
 1 - Create and use a patron that can place multiple record level holds per record
 2 - Create a record with X items, each at a different library
 3 - Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
 4 - perl misc/cronjobs/holds/build_holdsqueue.pl
 5 - Check in LibraryA's copy as LibraryA and confirm the hold
 6 - Revisit request.pl for the record, notice the next hold in line is now item-specific
 7 - Checkout the item to the patron, notice the remaining hold is marked waiting
 8 - Attempt to place another hold for your patron, notice that it requires an item-specific hold
 8 - Apply this patch
 9 - Repeat steps 1-5
10 - Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
11 - Checkout the item to the patron, ensure the first hold is filled and the second remains record level
12 - Repeat whole test plan without building holds queue to confirm holds are still treated correctly

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 60 Katrin Fischer 2020-08-22 16:39:26 UTC
Created attachment 108937 [details] [review]
Bug 18958: (QA follow-up) Add missing comma to kohastructure.sql
Comment 61 Katrin Fischer 2020-08-22 16:39:56 UTC
(In reply to Katrin Fischer from comment #58)
> The test worked for me, but the reserve_id column remained NULL at all times
> - can you please explain?

Making sure this is seen after I added a follow-up for a comma.
Comment 62 Nick Clemens 2020-08-25 10:39:32 UTC
Created attachment 109061 [details] [review]
Bug 18958: DO NOT PUSH - Schema updates
Comment 63 Katrin Fischer 2020-08-26 23:00:54 UTC
(In reply to Nick Clemens from comment #62)
> Created attachment 109061 [details] [review] [review]
> Bug 18958: DO NOT PUSH - Schema updates

I guess you are trying to tell me I should have run dbic? ;)
Comment 64 Marcel de Rooy 2020-08-27 05:58:54 UTC
Coming back here soon ;)
Comment 65 Katrin Fischer 2020-08-27 07:45:12 UTC
(In reply to Marcel de Rooy from comment #64)
> Coming back here soon ;)

Happily leaving it for you ;)
Comment 66 Nick Clemens 2020-08-27 10:17:18 UTC
Created attachment 109222 [details] [review]
Bug 18958: (follow-up) Ensure hold fill target reserve_id is set for all hold types

MapItemsToHoldRequests has three sections: Local holds, item level holds, bib level holds

Only one of them was setting the reserve_id. This patch makes al three set it and adds tests

To test:
1 - Repeat test plan on bug
2 - sudo koha-mysql kohadev
    SELECT * FROM hold_fill_targets
3 - Ensure reserve_id is set at appropriate times
4 - prove -v  t/db_dependent/HoldsQueue.t
Comment 67 Nick Clemens 2020-08-27 10:18:00 UTC
(In reply to Katrin Fischer from comment #63)
> (In reply to Nick Clemens from comment #62)
> > Created attachment 109061 [details] [review] [review] [review]
> > Bug 18958: DO NOT PUSH - Schema updates
> 
> I guess you are trying to tell me I should have run dbic? ;)

Yes, but I also forgot to attach last patch
Comment 68 Marcel de Rooy 2020-09-04 06:06:16 UTC
Continuing here
Comment 69 Marcel de Rooy 2020-09-04 07:09:57 UTC
Created attachment 109640 [details] [review]
Bug 18958: Unit tests

Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 70 Marcel de Rooy 2020-09-04 07:10:06 UTC
Created attachment 109641 [details] [review]
Bug 18958: Make hold_fill_targets specific to reserves

After looking at Marcel's comments, the problem is in our matching
to hold_fill_targets - rather than adjusting to find filled/waiting holds we
could ensure that hold_fill_targets only refers to the specific hold it
is intended to

This patch is clearer, if slightly less performant than last (we now return all
the reserves and have to find the 'highest')

Test Plan:
 1 - Create and use a patron that can place multiple record level holds per record
 2 - Create a record with X items, each at a different library
 3 - Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
 4 - perl misc/cronjobs/holds/build_holdsqueue.pl
 5 - Check in LibraryA's copy as LibraryA and confirm the hold
 6 - Revisit request.pl for the record, notice the next hold in line is now item-specific
 7 - Checkout the item to the patron, notice the remaining hold is marked waiting
 8 - Attempt to place another hold for your patron, notice that it requires an item-specific hold
 8 - Apply this patch
 9 - Repeat steps 1-5
10 - Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
11 - Checkout the item to the patron, ensure the first hold is filled and the second remains record level
12 - Repeat whole test plan without building holds queue to confirm holds are still treated correctly

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 71 Marcel de Rooy 2020-09-04 07:10:15 UTC
Created attachment 109642 [details] [review]
Bug 18958: (QA follow-up) Add missing comma to kohastructure.sql

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 72 Marcel de Rooy 2020-09-04 07:10:25 UTC
Created attachment 109643 [details] [review]
Bug 18958: DO NOT PUSH - Schema updates

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 73 Marcel de Rooy 2020-09-04 07:10:38 UTC
Created attachment 109644 [details] [review]
Bug 18958: (follow-up) Ensure hold fill target reserve_id is set for all hold types

MapItemsToHoldRequests has three sections: Local holds, item level holds, bib level holds

Only one of them was setting the reserve_id. This patch makes al three set it and adds tests

To test:
1 - Repeat test plan on bug
2 - sudo koha-mysql kohadev
    SELECT * FROM hold_fill_targets
3 - Ensure reserve_id is set at appropriate times
4 - prove -v  t/db_dependent/HoldsQueue.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 74 Marcel de Rooy 2020-09-04 07:10:48 UTC
Created attachment 109645 [details] [review]
Bug 18958: (QA follow-up) Fix number of tests

In HoldsQueue.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 75 Marcel de Rooy 2020-09-04 07:13:11 UTC
Looks good to me now.
Cant resist saying that the support of holds with and without holds queue in Koha seems kind of a source of bugs on itself to me ;)
The design of the two tables tmpholdsqueue and hold_fill_targets looks quite odd.

[FIXED] # Looks like you planned 54 tests but ran 55.
[OPEN] Should we add a FK for reserve_id in hold_fill_targets ?

Passed QA
Comment 76 Jonathan Druart 2020-09-18 09:53:21 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 77 Lucas Gass 2020-10-16 15:41:43 UTC
In 20.05.x I cannot make the tests for t/db_dependent/HoldsQueue.t pass:


C4::HoldsQueue::AddToHoldTargetMap(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'reserve_id' in 'field list' [for Statement "
        INSERT INTO hold_fill_targets (borrowernumber, biblionumber, itemnumber, source_branchcode, item_level_request, reserve_id)
                               VALUES (?, ?, ?, ?, ?, ?)
    " with ParamValues: 0=55, 1=448, 2='986', 3='toQrEw', 4=0, 5=2] at /kohadevbox/koha/C4/HoldsQueue.pm line 215


No backport
Comment 78 Lucas Gass 2020-10-16 17:06:13 UTC
Thanks Nick,

backported to 20.05.x for 20.05.05
Comment 79 Aleisha Amohia 2020-10-20 02:31:37 UTC
backported to 19.11.x for 19.11.11
Comment 80 Victor Grousset/tuxayo 2020-10-26 17:23:50 UTC
Got a huge conflict while trying to backport the following patch:
Bug 18958: (follow-up) Ensure hold fill target reserve_id is set for all hold types

If there is an interest in having this backported, feel free to submit a patch for 19.05.