[Bug 22005] New: System preference DecreaseLoanHighHolds incorrectly counts the number of holds
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 Bug ID: 22005 Summary: System preference DecreaseLoanHighHolds incorrectly counts the number of holds Change sponsored?: --- Product: Koha Version: 18.05 Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: kelly@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 Jeff Brown <j.j.brown@lboro.ac.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |j.j.brown@lboro.ac.uk --- Comment #1 from Jeff Brown <j.j.brown@lboro.ac.uk> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=22004 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |magnus@libriotech.no --- Comment #2 from Magnus Enger <magnus@libriotech.no> --- I wonder if this code is introducing a bit of a off-by-one problem: C4::Circulation::checkHighHolds(), line 1230-1234 # If the number of holds is less than the count of items we have # plus the number of holds allowed above that count, we can stop here if ( $holds->count() <= $threshold ) { return $return_data; } The comment says "less than" but the code says "less than or equal". -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 --- Comment #3 from Magnus Enger <magnus@libriotech.no> --- A couple of tests seem to be failing for me: $ sudo koha-shell -c "prove -v t/db_dependent/DecreaseLoanHighHolds.t" kohadev t/db_dependent/DecreaseLoanHighHolds.t .. 1..17 ok 1 - userenv set ok 2 - Static mode should exceed threshold ok 3 - Should have 5 outstanding holds ok 4 - Should have duration of 1 ok 5 - due_date should be a DateTime object ok 6 - New due hour is equal to original due hour. ok 7 - New due minute is equal to original due minute. ok 8 - New due date second is zero. not ok 9 - Should not exceed threshold # Failed test 'Should not exceed threshold' # at t/db_dependent/DecreaseLoanHighHolds.t line 142. # got: '1' # expected: '0' ok 10 - Should exceed threshold of 1 not ok 11 - Should not exceed threshold of 2 # Failed test 'Should not exceed threshold of 2' # at t/db_dependent/DecreaseLoanHighHolds.t line 160. # got: '1' # expected: '0' ok 12 - Should exceed threshold with one damaged item ok 13 - Should exceed threshold with one lost item ok 14 - Should exceed threshold with one notforloan item ok 15 - Should exceed threshold with one withdrawn item ok 16 - High holds checkout needs confirmation ok 17 - High holds checkout does not need confirmation # Looks like you failed 2 tests of 17. Dubious, test returned 2 (wstat 512, 0x200) Failed 2/17 subtests Test Summary Report ------------------- t/db_dependent/DecreaseLoanHighHolds.t (Wstat: 512 Tests: 17 Failed: 2) Failed tests: 9, 11 Non-zero exit status: 2 Files=1, Tests=17, 12 wallclock secs ( 0.02 usr 0.01 sys + 9.69 cusr 0.70 csys = 10.42 CPU) Result: FAIL -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 --- Comment #4 from Magnus Enger <magnus@libriotech.no> --- I really don't understand the logic of the CheckHighHolds sub: sub checkHighHolds { my ( $item, $borrower ) = @_; ... if ( $holds->count() ) { ... if ( $decreaseLoanHighHoldsControl eq 'static' ) { ... # If the number of holds is less than the threshold, we can stop here if ( $holds->count() < $decreaseLoanHighHoldsValue ) { return $return_data; } } elsif ( $decreaseLoanHighHoldsControl eq 'dynamic' ) { ... # If the number of holds is less than the count of items we have # plus the number of holds allowed above that count, we can stop here if ( $holds->count() <= $threshold ) { return $return_data; } } ... if ( DateTime->compare( $reduced_datedue, $orig_due ) == -1 ) { $return_data->{exceeded} = 1; $return_data->{duration} = $decreaseLoanHighHoldsDuration; $return_data->{due_date} = $reduced_datedue; } } return $return_data; } As far as I can tell, it does the right thing for decreaseLoanHighHoldsControl = static/dynamic, but then it goes and calculates the regular/original due date and the decreased due date, and if the decreased date is less than the original date, then the decreased date is returned? Why is this done? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 --- Comment #5 from Magnus Enger <magnus@libriotech.no> --- (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"? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 --- Comment #6 from Magnus Enger <magnus@libriotech.no> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 --- Comment #7 from Magnus Enger <magnus@libriotech.no> --- Created attachment 103250 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103250&action=edit 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 --- Comment #8 from Magnus Enger <magnus@libriotech.no> --- Created attachment 103251 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103251&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 --- Comment #9 from Magnus Enger <magnus@libriotech.no> --- (In reply to Magnus Enger from comment #7)
this patch on current master, running prove on the tests should *FAIL*.
-- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 --- Comment #10 from Magnus Enger <magnus@libriotech.no> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=20567 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |magnus@libriotech.no |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal --- Comment #11 from Magnus Enger <magnus@libriotech.no> --- Changing this from enhancement to a normal bug. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net Status|Needs Signoff |Patch doesn't apply --- Comment #12 from Victor Grousset/tuxayo <victor@tuxayo.net> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22005 Eric Phetteplace <ephetteplace@cca.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ephetteplace@cca.edu -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org