Bug 20567 - "Holds per record (count)" limit is not enforced after item is captured for hold
Summary: "Holds per record (count)" limit is not enforced after item is captured for hold
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 14695
Blocks:
  Show dependency treegraph
 
Reported: 2018-04-11 20:54 UTC by Margaret
Modified: 2020-01-15 14:26 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Multiple holds on one item record in staff client (242.40 KB, image/png)
2018-04-11 20:54 UTC, Margaret
Details
Bug 20567: Add tests (1.23 KB, patch)
2020-01-14 09:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20567: Count found holds for holds limit (747 bytes, patch)
2020-01-14 09:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20567: Add tests (1.30 KB, patch)
2020-01-15 11:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20567: Count found holds for holds limit (820 bytes, patch)
2020-01-15 11:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20567: Add tests (1.35 KB, patch)
2020-01-15 11:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20567: Count found holds for holds limit (870 bytes, patch)
2020-01-15 11:44 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Margaret 2018-04-11 20:54:23 UTC
Created attachment 74042 [details]
Multiple holds on one item record in staff client

"Holds per record (count)" limit is not enforced after an item on the record is captured for hold and moves from Pending to Waiting Pickup status. At this point it is possible to place another hold on that record in violation of the "Holds per record (count)" setting.

I was able to replicate this behavior with the following steps: 

1. Set "Holds per record (count)" to 1 for your patron category/item type rule 
2. Create a record and add at least two items, making sure the MARC and Item Koha Item Type is filled in
3. Place a hold on the record for a patron 
4. Check in one of the items and confirm/transfer/capture the hold for the patron 
5. Place a second hold on the same record for the same patron  

In either Staff or Opac the second hold on this bib is allowed after the first hold moves to Waiting for Pickup status.

I think that this problem is very closely related to bugzilla 17978 and or 15404, but I did not see a specific mention of the "Holds per record (count)" limit upon perusal of those bugs.
Comment 1 Christopher Brannon 2018-07-13 16:10:29 UTC
I would not classify this as a minor bug.  If limits are not being enforced in all conditions, this is a problem.  At the very least, there needs to be a preference to change this oversight, if for some reason there is justification to keep this unusual behavior.
Comment 2 Jonathan Druart 2019-12-11 11:02:43 UTC
It seems that it is the expected behavior.

C4/Circulation.pm, in CanItemBeReserved:
 394             found          => undef, # Found holds don't count against a patron's holds limit

Should not it be?
Comment 3 Hans Pålsson 2020-01-13 12:46:11 UTC
(In reply to Jonathan Druart from comment #2)
> It seems that it is the expected behavior.
> 
> C4/Circulation.pm, in CanItemBeReserved:
>  394             found          => undef, # Found holds don't count against
> a patron's holds limit
> 
> Should not it be?

No, a patron should only be allowed 1 hold per biblio, also after that hold is filled. It translates to patrons should not be able to place holds on biblios for which they have checked out items or already placed a hold. The current behaviour becomes a loophole.
Comment 4 Hans Pålsson 2020-01-14 08:22:20 UTC
(In reply to Jonathan Druart from comment #2)
> C4/Circulation.pm, in CanItemBeReserved:
>  394             found          => undef, # Found holds don't count against
> a patron's holds limit

I think it is C4/Reserves.pm.
Comment 5 Katrin Fischer 2020-01-14 09:25:57 UTC
I understand why you would NOT want to count the waiting holds, but I wonder if we are missing a use case here if we change that too quickly. 

Maybe we coudl compare to how other hold counts, like the daily limit, work here?
Comment 6 Jonathan Druart 2020-01-14 09:36:13 UTC
Created attachment 97336 [details] [review]
Bug 20567: Add tests
Comment 7 Jonathan Druart 2020-01-14 09:36:17 UTC
Created attachment 97337 [details] [review]
Bug 20567: Count found holds for holds limit
Comment 8 Jonathan Druart 2020-01-14 09:38:02 UTC
Same as Katrin here, I submitted a patch that should modify the behavior as you expect, but I am not sure it's correct (side effects).
That would be good to get Kyle's opinion on this, as he wrote that bit of code (the comment on the line I remove especially).
Comment 9 Hans Pålsson 2020-01-14 10:19:39 UTC
The problem is on item level more than on biblio level. If a library only allows 1 item to be reserved per biblio, then the current code creates the loophole. That is the problem we have and the same as reported in this bug.
If the amount of holds placed should count towards a patrons total amount of holds placed on different biblios, then the current code makes sense. I think...
Comment 10 Kyle M Hall 2020-01-14 11:11:04 UTC
(In reply to Jonathan Druart from comment #8)
> Same as Katrin here, I submitted a patch that should modify the behavior as
> you expect, but I am not sure it's correct (side effects).
> That would be good to get Kyle's opinion on this, as he wrote that bit of
> code (the comment on the line I remove especially).

I'm researching the issue to see if the original development funder(s) had this behavior as a requirement or not. In any case it would be trivial to syspref this to satisfy both those who want the existing behavior and those who do not.
Comment 11 Hans Pålsson 2020-01-14 13:27:35 UTC
Signing of because patch solves initial problem.
Comment 12 Benjamin Daeuber 2020-01-14 19:54:27 UTC
We are the sponsoring library Kyle refers to. While we hadn't noticed this, I guess I would agree with the opinion here, a hold in waiting status is still a hold. 

That said, if someone wants a syspref, that's fine, but I guess everyone here seems to agree waiting hold and pending holds ought to be counted together.
Comment 13 Katrin Fischer 2020-01-14 20:17:29 UTC
(In reply to Benjamin Daeuber from comment #12)
> We are the sponsoring library Kyle refers to. While we hadn't noticed this,
> I guess I would agree with the opinion here, a hold in waiting status is
> still a hold. 
> 
> That said, if someone wants a syspref, that's fine, but I guess everyone
> here seems to agree waiting hold and pending holds ought to be counted
> together.

Thx Benjamin for commenting!
Comment 14 Kyle M Hall 2020-01-15 11:29:52 UTC
Based on Ben's response, my opinion is that this change in behavior does not need to be syspref'ed at this time!

(In reply to Katrin Fischer from comment #13)
> (In reply to Benjamin Daeuber from comment #12)
> > We are the sponsoring library Kyle refers to. While we hadn't noticed this,
> > I guess I would agree with the opinion here, a hold in waiting status is
> > still a hold. 
> > 
> > That said, if someone wants a syspref, that's fine, but I guess everyone
> > here seems to agree waiting hold and pending holds ought to be counted
> > together.
> 
> Thx Benjamin for commenting!
Comment 15 Kyle M Hall 2020-01-15 11:38:44 UTC
Created attachment 97386 [details] [review]
Bug 20567: Add tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2020-01-15 11:39:03 UTC
Created attachment 97387 [details] [review]
Bug 20567: Count found holds for holds limit

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2020-01-15 11:44:30 UTC
Created attachment 97388 [details] [review]
Bug 20567: Add tests

Signed-off-by: Hans Palsson <hans.palsson@hkr.se>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2020-01-15 11:44:38 UTC
Created attachment 97389 [details] [review]
Bug 20567: Count found holds for holds limit

Signed-off-by: Hans Palsson <hans.palsson@hkr.se>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Martin Renvoize 2020-01-15 13:56:52 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 20 Hans Pålsson 2020-01-15 14:26:36 UTC
(In reply to Martin Renvoize from comment #19)
> Nice work everyone!
> 
> Pushed to master for 20.05

:-)