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.
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.
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?
(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.
(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.
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?
Created attachment 97336 [details] [review] Bug 20567: Add tests
Created attachment 97337 [details] [review] Bug 20567: Count found holds for holds limit
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).
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...
(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.
Signing of because patch solves initial problem.
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.
(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!
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!
Created attachment 97386 [details] [review] Bug 20567: Add tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 97387 [details] [review] Bug 20567: Count found holds for holds limit Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
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>
Nice work everyone! Pushed to master for 20.05
(In reply to Martin Renvoize from comment #19) > Nice work everyone! > > Pushed to master for 20.05 :-)
Pushed to 19.11.x branch for 19.11.03
backported to 19.05.x for 19.05.08
Enhancement/patch will not be backported to 18.11.x series.