https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 65104 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65104&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |ASSIGNED -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 65150 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65150&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65150|0 |1 is obsolete| | --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 65184 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65184&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com QA Contact|testopia@bugs.koha-communit |josef.moravec@gmail.com |y.org | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 George Williams (NEKLS) <george@nekls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |george@nekls.org -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Sally Healey <sally.healey@cheshiresharedservices.gov.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sally.healey@cheshireshared | |services.gov.uk -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #71232|0 |1 is obsolete| | --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 72210 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=72210&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA: Looking here -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #72210|0 |1 is obsolete| | Attachment #72211|0 |1 is obsolete| | --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 72348 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=72348&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- [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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Andrew <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com --- Comment #19 from Andrew <andrew@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #20 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 92159 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=92159&action=edit Bug 18958: Unit tests -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |major CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #72348|0 |1 is obsolete| | Attachment #72349|0 |1 is obsolete| | Attachment #92159|0 |1 is obsolete| | Attachment #92160|0 |1 is obsolete| | --- Comment #23 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 93106 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93106&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #25 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 93108 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93108&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 ByWater Sandboxes <bws.sandboxes@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #93106|0 |1 is obsolete| | --- Comment #27 from ByWater Sandboxes <bws.sandboxes@gmail.com> --- Created attachment 93111 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93111&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 ByWater Sandboxes <bws.sandboxes@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #93108|0 |1 is obsolete| | --- Comment #29 from ByWater Sandboxes <bws.sandboxes@gmail.com> --- Created attachment 93113 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93113&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Bouzid Fergani <bouzid.fergani@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #93111|0 |1 is obsolete| | --- Comment #30 from Bouzid Fergani <bouzid.fergani@inlibro.com> --- Created attachment 93216 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93216&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Bouzid Fergani <bouzid.fergani@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #93113|0 |1 is obsolete| | --- Comment #32 from Bouzid Fergani <bouzid.fergani@inlibro.com> --- Created attachment 93218 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93218&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_19_11_candidate CC| |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_19_11_candidate | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #36 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 koha-US bug tracker <bugzilla@koha-us.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bugzilla@koha-us.org -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_20_11_target | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #93216|0 |1 is obsolete| | Attachment #93217|0 |1 is obsolete| | Attachment #93218|0 |1 is obsolete| | --- Comment #37 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 107576 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107576&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #39 from Nick Clemens <nick@bywatersolutions.com> --- (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) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #42 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 107585 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107585&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #44 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Marcel - maybe 3rd time this will pass? :) Do you want to continue here? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #47 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #49 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #50 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 108256 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108256&action=edit 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') -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #107584|0 |1 is obsolete| | Attachment #107585|0 |1 is obsolete| | Attachment #107586|0 |1 is obsolete| | Attachment #108274|0 |1 is obsolete| | --- Comment #52 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 108473 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108473&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #54 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108473|0 |1 is obsolete| | --- Comment #56 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 108499 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108499&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #58 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- The test worked for me, but the reserve_id column remained NULL at all times - can you please explain? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #60 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 108937 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108937&action=edit Bug 18958: (QA follow-up) Add missing comma to kohastructure.sql -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|kyle@bywatersolutions.com |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #62 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 109061 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=109061&action=edit Bug 18958: DO NOT PUSH - Schema updates -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #64 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Coming back here soon ;) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #66 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 109222 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=109222&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED --- Comment #68 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Continuing here -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108499|0 |1 is obsolete| | --- Comment #69 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 109640 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=109640&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108937|0 |1 is obsolete| | --- Comment #71 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 109642 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=109642&action=edit Bug 18958: (QA follow-up) Add missing comma to kohastructure.sql Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #109222|0 |1 is obsolete| | --- Comment #73 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 109644 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=109644&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #75 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 --- Comment #76 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 20.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #77 from Lucas Gass <lucas@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18958 Aleisha Amohia <aleisha@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|20.11.00, 20.05.05 |20.11.00, 20.05.05, released in| |19.11.11 CC| |aleisha@catalyst.net.nz Status|Pushed to stable |Pushed to oldstable --- Comment #79 from Aleisha Amohia <aleisha@catalyst.net.nz> --- backported to 19.11.x for 19.11.11 -- You are receiving this mail because: You are watching all bug changes.