Bug 22005

Summary: System preference DecreaseLoanHighHolds incorrectly counts the number of holds
Product: Koha Reporter: Kelly McElligott <kelly>
Component: Hold requestsAssignee: Magnus Enger <magnus>
Status: Patch doesn't apply --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: ephetteplace, gmcharlt, j.j.brown, magnus, victor
Version: 18.05   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22004
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20567
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 22005 - DecreaseLoanHighHolds counts holds wrong
Bug 22005 - DecreaseLoanHighHolds counts holds wrong

Description Kelly McElligott 2018-12-14 18:57:49 UTC
When using the system preference decreaseLoanHighHolds, if a library has this set to 1 hold.  Then when the 1 person that has a hold on this item attempts to check it out, Koha will invoke the decreaseloanhighholds - although this hold is getting fulfilled.  This system preference should only count holds that are not fulfilled instead of counting all the holds including the one that is getting checked out.  In conclusion, Koha should disregard the first hold as this will get removed when the checkout happens instead of count it towards number of holds on the record.
Comment 1 Jeff Brown 2019-06-04 10:16:34 UTC
At Loughborough University we agree that the decreaseLoanHighHolds should only count holds that are not fulfilled. This is especially important when borrowers collect & issue their own holds.
Comment 2 Magnus Enger 2020-04-03 08:39:58 UTC Comment hidden (obsolete)
Comment 3 Magnus Enger 2020-04-03 08:53:48 UTC Comment hidden (obsolete)
Comment 4 Magnus Enger 2020-04-03 13:37:38 UTC Comment hidden (obsolete)
Comment 5 Magnus Enger 2020-04-20 09:17:19 UTC
(In reply to Kelly McElligott from comment #0)
> When using the system preference decreaseLoanHighHolds, if a library has
> this set to 1 hold.  Then when the 1 person that has a hold on this item
> attempts to check it out, Koha will invoke the decreaseloanhighholds -
> although this hold is getting fulfilled.  This system preference should only
> count holds that are not fulfilled instead of counting all the holds
> including the one that is getting checked out.  In conclusion, Koha should
> disregard the first hold as this will get removed when the checkout happens
> instead of count it towards number of holds on the record.

I think this is the behaviour you get if you set decreaseLoanHighHoldsControl = "on the record". Then it just counts the holds on the record. Have you tried decreaseLoanHighHoldsControl = "over the number og holdable items on the record"?
Comment 6 Magnus Enger 2020-04-20 09:18:18 UTC
I had a long stare at the code around this, and by fleshing out the tests a bit more I think I was able to find and fix a small bug. Will submit a patch for this in a minute.
Comment 7 Magnus Enger 2020-04-20 09:36:54 UTC
Created attachment 103250 [details] [review]
Bug 22005 - DecreaseLoanHighHolds counts holds wrong

This patch adds some more comments and tests to the ones already
in t/db_dependent/DecreaseLoanHighHolds.t. After applying just
this patch on current master, running prove on the tests should.
A followup patch will fix the underlying problem.
Comment 8 Magnus Enger 2020-04-20 09:36:58 UTC
Created attachment 103251 [details] [review]
Bug 22005 - DecreaseLoanHighHolds counts holds wrong

There is a small error in how holds are compared to thresholds in
the code related to DecreaseLoanHighHolds. This should fix the
problem.

To test:
- Apply the first patch from this bug, which fleshes out the tests
  a bit. Run the tests with something like this:
  $ sudo koha-shell -c "prove -v t/db_dependent/DecreaseLoanHighHolds.t" kohadev
  The tests should fail
- Read through the tests and comments and make sure you agree they
  make sense
- Apply this second patch
- Run prove on the tests again
- Check that all tests pass
Comment 9 Magnus Enger 2020-04-23 10:51:11 UTC
(In reply to Magnus Enger from comment #7)
> this patch on current master, running prove on the tests should *FAIL*.
Comment 10 Magnus Enger 2020-04-23 12:21:19 UTC
I think the counts are also influenced by this bug:

Bug 20567 - "Holds per record (count)" limit is not enforced after item is captured for hold
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20567

It should be fixed in versions 20.05.00, 19.11.03, 19.05.08.
Comment 11 Magnus Enger 2020-10-30 13:32:41 UTC
Changing this from enhancement to a normal bug.
Comment 12 Victor Grousset/tuxayo 2020-12-20 17:46:49 UTC
Conflicts in t/db_dependent/DecreaseLoanHighHolds.t

Tried to get my head around this, sorry couldn't review all the test changes. First time learning about decreaseLoanHighHolds.

Maybe the review should be left only for QA? I seems there isn't much people that can review this.