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
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
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
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>
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>
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>
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?
(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.
(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.
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>
Created attachment 72211 [details] [review] Bug 18958 [QA Followup] - Bring back data needed for the diffbranch block of the template
QA: Looking here
Still working on it. Suspect that we are not ready yet.. Moving to BLOCKED temporarily
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
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>
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.
[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.
(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!
(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.
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.
Created attachment 92159 [details] [review] Bug 18958: Unit tests
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
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
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
Created attachment 93107 [details] [review] Bug 18958: Unit tests
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
(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
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>
Created attachment 93112 [details] [review] Bug 18958: Unit tests Signed-off-by: Bonnie Gardner <bgardner@cityoflewiston.org>
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>
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>
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>
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>
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?
(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
QAing
(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!
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>
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>
(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)
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
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>
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>
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>
Marcel - maybe 3rd time this will pass? :) Do you want to continue here?
(In reply to Katrin Fischer from comment #44) > Marcel - maybe 3rd time this will pass? :) Do you want to continue here? Will do
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 );
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
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.
(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
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')
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')
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>
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
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.
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
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>
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>
The test worked for me, but the reserve_id column remained NULL at all times - can you please explain?
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>
Created attachment 108937 [details] [review] Bug 18958: (QA follow-up) Add missing comma to kohastructure.sql
(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.
Created attachment 109061 [details] [review] Bug 18958: DO NOT PUSH - Schema updates
(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? ;)
Coming back here soon ;)
(In reply to Marcel de Rooy from comment #64) > Coming back here soon ;) Happily leaving it for you ;)
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
(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
Continuing here
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>
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>
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>
Created attachment 109643 [details] [review] Bug 18958: DO NOT PUSH - Schema updates Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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>
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
Pushed to master for 20.11, thanks to everybody involved!
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
Thanks Nick, backported to 20.05.x for 20.05.05
backported to 19.11.x for 19.11.11
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.