Bug 10452 - AllowHoldsOnDamagedItems should control using damaged items to fulfill holds
Summary: AllowHoldsOnDamagedItems should control using damaged items to fulfill holds
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-12 14:09 UTC by Kyle M Hall
Modified: 2021-04-30 09:30 UTC (History)
5 users (show)

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


Attachments
Bug 10452 - AllowHoldsOnDamagedItems should control using damaged items to fulfill holds (4.75 KB, patch)
2013-06-12 14:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10452 - AllowHoldsOnDamagedItems should control using damaged items to fulfill holds (4.79 KB, patch)
2013-07-19 05:06 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 10452 [QA Followup] - Unit tests (1.62 KB, patch)
2014-01-17 14:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10452 [QA Followup] - Unit tests (1.63 KB, patch)
2014-01-17 14:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 10452 - AllowHoldsOnDamagedItems should control using damaged items to fulfill holds (4.87 KB, patch)
2014-01-18 14:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10452 [QA Followup] - Unit tests (1.89 KB, patch)
2014-01-18 14:38 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-06-12 14:09:18 UTC
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?
Comment 1 Kyle M Hall 2013-06-12 14:17:17 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-07-19 05:06:22 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2013-09-08 20:50:37 UTC
Kyle, could you please add a regression test for this in a follow-up?
Comment 4 Katrin Fischer 2013-09-08 21:16:46 UTC
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.
Comment 5 Kyle M Hall 2014-01-09 17:41:46 UTC
> 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.
Comment 6 Katrin Fischer 2014-01-15 08:22:07 UTC
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.
Comment 7 Jonathan Druart 2014-01-15 14:38:41 UTC
I think this change needs unit tests.
Comment 8 Katrin Fischer 2014-01-16 08:23:25 UTC
I agree, everything holds related can't have enough regression tests.
Comment 9 Kyle M Hall 2014-01-17 14:06:07 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2014-01-17 14:07:35 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2014-01-18 14:38:18 UTC
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>
Comment 12 Katrin Fischer 2014-01-18 14:38:34 UTC
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.
Comment 13 Galen Charlton 2014-03-10 18:21:11 UTC
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.
Comment 14 Fridolin Somers 2014-05-27 14:22:52 UTC
Pushed to 3.14.x, will be in 3.14.07