AllowHoldsOnDamagedItems will stop item-specific holds from being placed on damaged items, but does not stop Koha from using damaged items to fill holds. This seems like incorrect behavior. If we don't want holds placed on damaged items, why would we want damaged items to be used to fill a hold?
Created attachment 18899 [details] [review] Bug 10452 - AllowHoldsOnDamagedItems should control using damaged items to fulfill holds AllowHoldsOnDamagedItems will stop item-specific holds from being placed on damaged items, but does not stop Koha from using damaged items to fill holds. This seems like incorrect behavior. Test Plan: 1) Set 'AllowHoldsOnDamagedItems' to "Don't Allow" 2) Pick an item, set it to damaged 3) Place a bib-level hold on this item's record 4) Scan the item though the returns system 5) Koha will ask to use this item to fill the hold, click "ignore" 6) Apply this patch 7) Repeat step 4 8) Koha will not ask to use this item to fill the hold
Created attachment 19784 [details] [review] [SIGNED-OFF] Bug 10452 - AllowHoldsOnDamagedItems should control using damaged items to fulfill holds AllowHoldsOnDamagedItems will stop item-specific holds from being placed on damaged items, but does not stop Koha from using damaged items to fill holds. This seems like incorrect behavior. Test Plan: 1) Set 'AllowHoldsOnDamagedItems' to "Don't Allow" 2) Pick an item, set it to damaged 3) Place a bib-level hold on this item's record 4) Scan the item though the returns system 5) Koha will ask to use this item to fill the hold, click "ignore" 6) Apply this patch 7) Repeat step 4 8) Koha will not ask to use this item to fill the hold Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Kyle, could you please add a regression test for this in a follow-up?
Hm, I see a problem here. As staff member you can overwrite hold limitations and it might make sense to put a hold on a damaged item to make sure it gets set aside when being returned for repair. But with your patch, the item level hold I set on the damaged item is not triggered on return. As there is only one item level hold on this record now, the hold will never trigger.
> As staff member you can overwrite hold limitations and it might make sense > to put a hold on a damaged item to make sure it gets set aside when being > returned for repair. Do you know any libraries that use this workflow? I don't see how that is very useful, as you won't know the item is damaged until it gets returned and the librarian sees the damage. At that point you don't need the hold! > But with your patch, the item level hold I set on the damaged item is not > triggered on return. As there is only one item level hold on this record > now, the hold will never trigger. I'm not quite understanding the problem here. You can also force holds that go against the circulation rules, and those holds will never be filled, so this is no without precedent.
Hi Kyle, about the workflow: yes, I can see that happen. I remember during one of my jobs in a public library we would put in a note if we noticed loose pages on checkout so it could go into repair after return and the patron would not be blamed for the damage. But... as you point out correctly, overwriting holds restrictions from staff is basically not working right. The holds don't get triggered. So I think I agree that we can't block this patch because of it.
I think this change needs unit tests.
I agree, everything holds related can't have enough regression tests.
Created attachment 24498 [details] [review] Bug 10452 [QA Followup] - Unit tests
Created attachment 24499 [details] [review] Bug 10452 [QA Followup] - Unit tests
Created attachment 24544 [details] [review] [PASSED QA] Bug 10452 - AllowHoldsOnDamagedItems should control using damaged items to fulfill holds AllowHoldsOnDamagedItems will stop item-specific holds from being placed on damaged items, but does not stop Koha from using damaged items to fill holds. This seems like incorrect behavior. Test Plan: 1) Set 'AllowHoldsOnDamagedItems' to "Don't Allow" 2) Pick an item, set it to damaged 3) Place a bib-level hold on this item's record 4) Scan the item though the returns system 5) Koha will ask to use this item to fill the hold, click "ignore" 6) Apply this patch 7) Repeat step 4 8) Koha will not ask to use this item to fill the hold Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 24545 [details] [review] [PASSED QA] Bug 10452 [QA Followup] - Unit tests Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script. Also checked t/db_dependent/Holds.t t/db_dependent/HoldsQueue.t Tested holds triggering with different settings of AllowHoldsOnDamagedItems. Works as described.
Pushed to master. Thanks, Kyle! I've opened bug 11919 for the topic of handling staff-generated holds on items that shouldn't fill hold requests for patrons.
Pushed to 3.14.x, will be in 3.14.07