[Bug 14694] New: Make decreaseloanHighHolds more flexible
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Bug ID: 14694 Summary: Make decreaseloanHighHolds more flexible Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: kyle.m.hall@gmail.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com This enhancement will add an option to the decreaseloanHighHolds preferences so that users have the ability to choose A) based on the number of holds, as Koha currently does or B) set a number of holds past the number of holdable items in that record Work to be done: l) Add new system Preferences o decreaseloanHighHoldsOffset o decreaseloanHighHoldslgnoreDamaged o decreaseloanHighHoldslgnoreLost o decreaselnanHighHoldslgnoreWthdrawn o decreaseloanHighHoldslgnoreNotForloan 2) Implement logic for new system preferences 3) Add unit tests We could possibly use the multi-select enhancement for system preferences from bug 9043 to avoid having 4 prefs -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Change sponsored?|--- |Sponsored Assignee|koha-bugs@lists.koha-commun |kyle.m.hall@gmail.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Nicole Engard (ByWater) <nicole@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nicole@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 42957 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42957&action=edit Bug 14694 - Make decreaseloanHighHolds more flexible This patch allows for more flexibility for determining when the number of holds a record has should trigger the reduction of the loan length for items on that record. This patch adds a new system preference decreaseLoanHighHoldsControl, which defaults to 'static', the original behavior of the feature. It also has a new behavior 'dynamic' which makes the feature only decrease the loan length if the number of holds on the record exceeds the number of holdable items + decreaseLoanHighHoldsValue. It also allows items to be filtered from the list of items based on the damaged, lost, not for loan, and withdrawn values even if those values would have allowed holds ( i.e. values < 0 ) Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set decreaseLoanHighHolds to Enable 4) Set decreaseLoanHighHoldsControl to "over the number of items on the record" 5) Set decreaseLoanHighHoldsDuration to 1 6) Set decreaseLoanHighHoldsValue to 3 7) Create a record with 5 items 8) Please 8 or more holds on the record 9) Check out one of the items to a patron 10) Note the loan length is reduced to 1 day 11) Set decreaseLoanHighHoldsValue to 3 to 2 12) Check out one of the items to a patron 13) Note the loan length is *not* reduced 14) Enbale all the filters possible in decreaseLoanHighHoldsIgnoreStatuses 15) Set one item to be damaged 16) Note the loan length is reduced 17) Unset the damaged status 18) Repeat steps 15 - 17 for lost, not for loan, and withdrawn -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42957|0 |1 is obsolete| | --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 42958 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42958&action=edit Bug 14694 - Make decreaseloanHighHolds more flexible This patch allows for more flexibility for determining when the number of holds a record has should trigger the reduction of the loan length for items on that record. This patch adds a new system preference decreaseLoanHighHoldsControl, which defaults to 'static', the original behavior of the feature. It also has a new behavior 'dynamic' which makes the feature only decrease the loan length if the number of holds on the record exceeds the number of holdable items + decreaseLoanHighHoldsValue. It also allows items to be filtered from the list of items based on the damaged, lost, not for loan, and withdrawn values even if those values would have allowed holds ( i.e. values < 0 ) Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set decreaseLoanHighHolds to Enable 4) Set decreaseLoanHighHoldsControl to "over the number of items on the record" 5) Set decreaseLoanHighHoldsDuration to 1 6) Set decreaseLoanHighHoldsValue to 3 7) Create a record with 5 items 8) Please 8 or more holds on the record 9) Check out one of the items to a patron 10) Note the loan length is reduced to 1 day 11) Set decreaseLoanHighHoldsValue to 3 to 2 12) Check out one of the items to a patron 13) Note the loan length is *not* reduced 14) Enbale all the filters possible in decreaseLoanHighHoldsIgnoreStatuses 15) Set one item to be damaged 16) Note the loan length is reduced 17) Unset the damaged status 18) Repeat steps 15 - 17 for lost, not for loan, and withdrawn -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42958|0 |1 is obsolete| | --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 42970 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42970&action=edit Bug 14694 - Make decreaseloanHighHolds more flexible This patch allows for more flexibility for determining when the number of holds a record has should trigger the reduction of the loan length for items on that record. This patch adds a new system preference decreaseLoanHighHoldsControl, which defaults to 'static', the original behavior of the feature. It also has a new behavior 'dynamic' which makes the feature only decrease the loan length if the number of holds on the record exceeds the number of holdable items + decreaseLoanHighHoldsValue. It also allows items to be filtered from the list of items based on the damaged, lost, not for loan, and withdrawn values even if those values would have allowed holds ( i.e. values < 0 ) Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set decreaseLoanHighHolds to Enable 4) Set decreaseLoanHighHoldsControl to "over the number of items on the record" 5) Set decreaseLoanHighHoldsDuration to 1 6) Set decreaseLoanHighHoldsValue to 3 7) Create a record with 5 items 8) Please 8 or more holds on the record 9) Check out one of the items to a patron 10) Note the loan length is reduced to 1 day 11) Set decreaseLoanHighHoldsValue to 3 to 2 12) Check out one of the items to a patron 13) Note the loan length is *not* reduced 14) Enbale all the filters possible in decreaseLoanHighHoldsIgnoreStatuses 15) Set one item to be damaged 16) Note the loan length is reduced 17) Unset the damaged status 18) Repeat steps 15 - 17 for lost, not for loan, and withdrawn -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |11565 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11565 [Bug 11565] decreaseLoanHighHolds needs Override -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42970|0 |1 is obsolete| | --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 43153 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43153&action=edit Bug 14694 - Make decreaseloanHighHolds more flexible This patch allows for more flexibility for determining when the number of holds a record has should trigger the reduction of the loan length for items on that record. This patch adds a new system preference decreaseLoanHighHoldsControl, which defaults to 'static', the original behavior of the feature. It also has a new behavior 'dynamic' which makes the feature only decrease the loan length if the number of holds on the record exceeds the number of holdable items + decreaseLoanHighHoldsValue. It also allows items to be filtered from the list of items based on the damaged, lost, not for loan, and withdrawn values even if those values would have allowed holds ( i.e. values < 0 ) Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set decreaseLoanHighHolds to Enable 4) Set decreaseLoanHighHoldsControl to "over the number of items on the record" 5) Set decreaseLoanHighHoldsDuration to 1 6) Set decreaseLoanHighHoldsValue to 3 7) Create a record with 5 items 8) Please 8 or more holds on the record 9) Check out one of the items to a patron 10) Note the loan length is reduced to 1 day 11) Set decreaseLoanHighHoldsValue to 3 to 2 12) Check out one of the items to a patron 13) Note the loan length is *not* reduced 14) Enbale all the filters possible in decreaseLoanHighHoldsIgnoreStatuses 15) Set one item to be damaged 16) Note the loan length is reduced 17) Unset the damaged status 18) Repeat steps 15 - 17 for lost, not for loan, and withdrawn -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |veron@veron.ch --- Comment #5 from Marc Véron <veron@veron.ch> --- Hi Kyle, I get errors with: t/db_dependent/DecreaseLoanHighHolds.t (Wstat: 2048 Tests: 12 Failed: 8) Failed tests: 2, 4-5, 7, 9-12 Non-zero exit status: 8 (...) not ok 4 - Should have duration of 1 # Failed test 'Should have duration of 1' # at t/db_dependent/DecreaseLoanHighHolds.t line 97. # got: '0' # expected: '1' not ok 5 - due_date should be a DateTime object # Failed test 'due_date should be a DateTime object' # at t/db_dependent/DecreaseLoanHighHolds.t line 98. # got: '' # expected: 'DateTime' (...) not ok 7 - Should exceed threshold # Failed test 'Should exceed threshold' # at t/db_dependent/DecreaseLoanHighHolds.t line 116. # got: '0' # expected: '1' (... and more) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #6 from Marc Véron <veron@veron.ch> --- The feature works and I like it, but sorry, tests do not work. See comment #5 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #43153|0 |1 is obsolete| | --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 43513 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43513&action=edit Bug 14694 - Make decreaseloanHighHolds more flexible This patch allows for more flexibility for determining when the number of holds a record has should trigger the reduction of the loan length for items on that record. This patch adds a new system preference decreaseLoanHighHoldsControl, which defaults to 'static', the original behavior of the feature. It also has a new behavior 'dynamic' which makes the feature only decrease the loan length if the number of holds on the record exceeds the number of holdable items + decreaseLoanHighHoldsValue. It also allows items to be filtered from the list of items based on the damaged, lost, not for loan, and withdrawn values even if those values would have allowed holds ( i.e. values < 0 ) Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set decreaseLoanHighHolds to Enable 4) Set decreaseLoanHighHoldsControl to "over the number of items on the record" 5) Set decreaseLoanHighHoldsDuration to 1 6) Set decreaseLoanHighHoldsValue to 3 7) Create a record with 5 items 8) Please 8 or more holds on the record 9) Check out one of the items to a patron 10) Note the loan length is reduced to 1 day 11) Set decreaseLoanHighHoldsValue to 3 to 2 12) Check out one of the items to a patron 13) Note the loan length is *not* reduced 14) Enbale all the filters possible in decreaseLoanHighHoldsIgnoreStatuses 15) Set one item to be damaged 16) Note the loan length is reduced 17) Unset the damaged status 18) Repeat steps 15 - 17 for lost, not for loan, and withdrawn -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #8 from Marc Véron <veron@veron.ch> --- The feature works, but the tests still fail, same results as in comment #5: 1..12 ok 1 - userenv set not ok 2 - Should exceed threshold # Failed test 'Should exceed threshold' # at t/db_dependent/DecreaseLoanHighHolds.t line 95. # got: '0' # expected: '1' ok 3 - Should have 5 outstanding holds not ok 4 - Should have duration of 1 # Failed test 'Should have duration of 1' # at t/db_dependent/DecreaseLoanHighHolds.t line 97. # got: '0' # expected: '1' not ok 5 - due_date should be a DateTime object # Failed test 'due_date should be a DateTime object' # at t/db_dependent/DecreaseLoanHighHolds.t line 98. # got: '' # expected: 'DateTime' ok 6 - Should not exceed threshold not ok 7 - Should exceed threshold # Failed test 'Should exceed threshold' # at t/db_dependent/DecreaseLoanHighHolds.t line 116. # got: '0' # expected: '1' ok 8 - Should not exceed threshold not ok 9 - Should exceed threshold # Failed test 'Should exceed threshold' # at t/db_dependent/DecreaseLoanHighHolds.t line 127. # got: '0' # expected: '1' not ok 10 - Should exceed threshold # Failed test 'Should exceed threshold' # at t/db_dependent/DecreaseLoanHighHolds.t line 134. # got: '0' # expected: '1' not ok 11 - Should exceed threshold # Failed test 'Should exceed threshold' # at t/db_dependent/DecreaseLoanHighHolds.t line 141. # got: '0' # expected: '1' not ok 12 - Should exceed threshold # Failed test 'Should exceed threshold' # at t/db_dependent/DecreaseLoanHighHolds.t line 148. # got: '0' # expected: '1' # Looks like you failed 8 tests of 12. Dubious, test returned 8 (wstat 2048, 0x800) Failed 8/12 subtests Test Summary Report ------------------- t/db_dependent/DecreaseLoanHighHolds.t (Wstat: 2048 Tests: 12 Failed: 8) Failed tests: 2, 4-5, 7, 9-12 Non-zero exit status: 8 Files=1, Tests=12, 2 wallclock secs ( 0.02 usr 0.00 sys + 1.30 cusr 0.10 csys = 1.42 CPU) Result: FAIL -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #43513|0 |1 is obsolete| | --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 43589 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43589&action=edit Bug 14694 - Make decreaseloanHighHolds more flexible This patch allows for more flexibility for determining when the number of holds a record has should trigger the reduction of the loan length for items on that record. This patch adds a new system preference decreaseLoanHighHoldsControl, which defaults to 'static', the original behavior of the feature. It also has a new behavior 'dynamic' which makes the feature only decrease the loan length if the number of holds on the record exceeds the number of holdable items + decreaseLoanHighHoldsValue. It also allows items to be filtered from the list of items based on the damaged, lost, not for loan, and withdrawn values even if those values would have allowed holds ( i.e. values < 0 ) Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set decreaseLoanHighHolds to Enable 4) Set decreaseLoanHighHoldsControl to "over the number of items on the record" 5) Set decreaseLoanHighHoldsDuration to 1 6) Set decreaseLoanHighHoldsValue to 3 7) Create a record with 5 items 8) Please 8 or more holds on the record 9) Check out one of the items to a patron 10) Note the loan length is reduced to 1 day 11) Set decreaseLoanHighHoldsValue to 3 to 2 12) Check out one of the items to a patron 13) Note the loan length is *not* reduced 14) Enbale all the filters possible in decreaseLoanHighHoldsIgnoreStatuses 15) Set one item to be damaged 16) Note the loan length is reduced 17) Unset the damaged status 18) Repeat steps 15 - 17 for lost, not for loan, and withdrawn -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #43589|0 |1 is obsolete| | --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 43598 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43598&action=edit [SIGNED-OFF] Bug 14694 - Make decreaseloanHighHolds more flexible This patch allows for more flexibility for determining when the number of holds a record has should trigger the reduction of the loan length for items on that record. This patch adds a new system preference decreaseLoanHighHoldsControl, which defaults to 'static', the original behavior of the feature. It also has a new behavior 'dynamic' which makes the feature only decrease the loan length if the number of holds on the record exceeds the number of holdable items + decreaseLoanHighHoldsValue. It also allows items to be filtered from the list of items based on the damaged, lost, not for loan, and withdrawn values even if those values would have allowed holds ( i.e. values < 0 ) Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set decreaseLoanHighHolds to Enable 4) Set decreaseLoanHighHoldsControl to "over the number of items on the record" 5) Set decreaseLoanHighHoldsDuration to 1 6) Set decreaseLoanHighHoldsValue to 3 7) Create a record with 5 items 8) Please 8 or more holds on the record 9) Check out one of the items to a patron 10) Note the loan length is reduced to 1 day 11) Set decreaseLoanHighHoldsValue to 3 to 2 12) Check out one of the items to a patron 13) Note the loan length is *not* reduced 14) Enbale all the filters possible in decreaseLoanHighHoldsIgnoreStatuses 15) Set one item to be damaged 16) Note the loan length is reduced 17) Unset the damaged status 18) Repeat steps 15 - 17 for lost, not for loan, and withdrawn Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Christopher Brannon <cbrannon@cdalibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |cbrannon@cdalibrary.org --- Comment #11 from Christopher Brannon <cbrannon@cdalibrary.org> --- We're excited to see this enhancement move forward. Would love it if it could get through QA in time for the next release. :) Christopher -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- # Looks like you failed 5 tests of 12. t/db_dependent/DecreaseLoanHighHolds.t .. Dubious, test returned 5 (wstat 1280, 0x500) Failed 5/12 subtests You should also change the descriptions of the tests to make them unique. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 43752 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43752&action=edit Bug 14694 [QA Followup] - Fix unit tests -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Comment on attachment 43598 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43598 [SIGNED-OFF] Bug 14694 - Make decreaseloanHighHolds more flexible Review of attachment 43598: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=14694&attachment=43598) ----------------------------------------------------------------- ::: C4/Circulation.pm @@ +1189,5 @@
+ + # static means just more than a given number of holds on the record + + # If the number of holds is less than the threshold, we can stop here + if ( $holds->count() < $decreaseLoanHighHoldsValue ) {
Just looking at the code, something smells wrong here. There is a strict comparison here. @@ +1214,5 @@
+ my $threshold = $items_count + $decreaseLoanHighHoldsValue; + + # 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 ) {
And an inclusive here. ::: Koha/Biblio.pm @@ +41,5 @@
+or list of Koha::Item objects in list context. + +=cut + +sub items {
Wouldn't be better to use a DBIx::Class relation? Something like bug 14819. ::: installer/data/mysql/sysprefs.sql @@ +103,3 @@
('decreaseLoanHighHoldsDuration',NULL,'','Specifies a number of days that a loan is reduced to when used in conjunction with decreaseLoanHighHolds','Integer'), ('decreaseLoanHighHoldsValue',NULL,'','Specifies a threshold for the minimum number of holds needed to trigger a reduction in loan duration (used with decreaseLoanHighHolds)','Integer'), +('decreaseLoanHighHoldsIgnoreStatuses', '', 'damaged|itemlost|notforloan|withdrawn', "Ignore items with these statuses for dynamic high holds checking", 'Choice'),
Wrongly placed. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Comment on attachment 43752 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43752 Bug 14694 [QA Followup] - Fix unit tests Review of attachment 43752: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=14694&attachment=43752) ----------------------------------------------------------------- ::: t/db_dependent/DecreaseLoanHighHolds.t @@ +92,5 @@
my $patron_hr = { borrower => $patron->id, branchcode => 'MPL' };
my $data = C4::Circulation::checkHighHolds( $item_hr, $patron_hr ); +is( $data->{exceeded}, 1, "Static mode should exceed threshold" ); +is( $data->{outstanding}, 6, "Should have 5 outstanding holds" );
6 vs 5 @@ +123,5 @@
$unholdable->damaged(-1); $unholdable->store();
$data = C4::Circulation::checkHighHolds( $item_hr, $patron_hr ); +is( $data->{exceeded}, 1, "Should exceed threshold with one damanged item" );
typo damaged @@ +137,5 @@
$unholdable->notforloan(-1); $unholdable->store();
$data = C4::Circulation::checkHighHolds( $item_hr, $patron_hr ); +is( $data->{exceeded}, 1, "Should exceed threshold wiht one notforloan item" );
typo with vs with -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #16 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 43768 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43768&action=edit Bug 14694 [QA Followup] - Fix typos -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #17 from Kyle M Hall <kyle.m.hall@gmail.com> ---
::: t/db_dependent/DecreaseLoanHighHolds.t @@ +92,5 @@
my $patron_hr = { borrower => $patron->id, branchcode => 'MPL' };
my $data = C4::Circulation::checkHighHolds( $item_hr, $patron_hr ); +is( $data->{exceeded}, 1, "Static mode should exceed threshold" ); +is( $data->{outstanding}, 6, "Should have 5 outstanding holds" );
6 vs 5
That is a purposeful change. The issue was that the logic of the unit test was off by one. The assumption was that the number of holds must greater than or equal to, but as per the code and sys pref definition, the number of holds must be simply greater than. Thus adding one more hold fixes all the tests. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Kyle M Hall from comment #17)
6 vs 5
That is a purposeful change. The issue was that the logic of the unit test was off by one. The assumption was that the number of holds must greater than or equal to, but as per the code and sys pref definition, the number of holds must be simply greater than. Thus adding one more hold fixes all the tests.
(In reply to Jonathan Druart from comment #14)
+ if ( $holds->count() <= $threshold ) {
And an inclusive here.
So should this be < also?
::: Koha/Biblio.pm @@ +41,5 @@
+or list of Koha::Item objects in list context. + +=cut + +sub items {
Wouldn't be better to use a DBIx::Class relation? Something like bug 14819.
::: installer/data/mysql/sysprefs.sql @@ +103,3 @@
('decreaseLoanHighHoldsDuration',NULL,'','Specifies a number of days that a loan is reduced to when used in conjunction with decreaseLoanHighHolds','Integer'), ('decreaseLoanHighHoldsValue',NULL,'','Specifies a threshold for the minimum number of holds needed to trigger a reduction in loan duration (used with decreaseLoanHighHolds)','Integer'), +('decreaseLoanHighHoldsIgnoreStatuses', '', 'damaged|itemlost|notforloan|withdrawn', "Ignore items with these statuses for dynamic high holds checking", 'Choice'),
Wrongly placed.
What about these 2 comments? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de Status|Signed Off |Failed QA --- Comment #19 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hm, waiting for feedback on comment 18. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #20 from Kyle M Hall <kyle.m.hall@gmail.com> --- (In reply to Jonathan Druart from comment #18)
(In reply to Kyle M Hall from comment #17)
6 vs 5
That is a purposeful change. The issue was that the logic of the unit test was off by one. The assumption was that the number of holds must greater than or equal to, but as per the code and sys pref definition, the number of holds must be simply greater than. Thus adding one more hold fixes all the tests.
(In reply to Jonathan Druart from comment #14)
+ if ( $holds->count() <= $threshold ) {
And an inclusive here.
So should this be < also?
No, that is the correct comparison. If you look at the descritions for the two high holds behavior options you will see the static ( original ) version requires a < comparison, and the new dynamic versions needs <=.
::: Koha/Biblio.pm @@ +41,5 @@
+or list of Koha::Item objects in list context. + +=cut + +sub items {
Wouldn't be better to use a DBIx::Class relation? Something like bug 14819.
No, we want to return a Koha::Object object, not a DBIC object. It would be most confusing if a Koha::Object method were to return a DBIC object instead of a Koha Object.
What about these 2 comments?
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #21 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 43952 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43952&action=edit Bug 14694 [QA Followup] - Fix syspref order -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #22 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Kyle M Hall from comment #20)
::: Koha/Biblio.pm @@ +41,5 @@
+or list of Koha::Item objects in list context. + +=cut + +sub items {
Wouldn't be better to use a DBIx::Class relation? Something like bug 14819.
No, we want to return a Koha::Object object, not a DBIC object. It would be most confusing if a Koha::Object method were to return a DBIC object instead of a Koha Object.
And what about using the _wrap method? I am afraid that we are going to introduce quickly circular dependencies in the Koha namespace if we follow this way. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Assignee|kyle.m.hall@gmail.com |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #23 from Christopher Brannon <cbrannon@cdalibrary.org> --- Would LOVE it if this could get a QA. :) Christopher -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #24 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I would love to get an answer when I ask for a question. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #25 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #22)
(In reply to Kyle M Hall from comment #20)
::: Koha/Biblio.pm @@ +41,5 @@
+or list of Koha::Item objects in list context. + +=cut + +sub items {
Wouldn't be better to use a DBIx::Class relation? Something like bug 14819.
No, we want to return a Koha::Object object, not a DBIC object. It would be most confusing if a Koha::Object method were to return a DBIC object instead of a Koha Object.
And what about using the _wrap method? I am afraid that we are going to introduce quickly circular dependencies in the Koha namespace if we follow this way.
The _wrap method would still require including Koha::Items, so that does not solve anything. In addition, you are assuming circular dependencies are always a bad thing. It is important to realize our objects are loosely coupled, rather than tightly coupled. Because of this, we could re-engineer Koha::Items to be anything we want inside, it's a black box. Koha::Biblio doesn't care how Koha::Items works, nor should it. In fact, using the _wrap method makes the objects more tightly coupled, as we are now accessing private methods and tying the objects together with a specific implementation. If we *really* want to decouple all of our objects, we need to build a factory to create all our objects whether they are Koha::Object(s) based or not. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #43598|0 |1 is obsolete| | Attachment #43752|0 |1 is obsolete| | Attachment #43768|0 |1 is obsolete| | Attachment #43952|0 |1 is obsolete| | --- Comment #26 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 46532 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46532&action=edit Bug 14694 - Make decreaseloanHighHolds more flexible This patch allows for more flexibility for determining when the number of holds a record has should trigger the reduction of the loan length for items on that record. This patch adds a new system preference decreaseLoanHighHoldsControl, which defaults to 'static', the original behavior of the feature. It also has a new behavior 'dynamic' which makes the feature only decrease the loan length if the number of holds on the record exceeds the number of holdable items + decreaseLoanHighHoldsValue. It also allows items to be filtered from the list of items based on the damaged, lost, not for loan, and withdrawn values even if those values would have allowed holds ( i.e. values < 0 ) Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set decreaseLoanHighHolds to Enable 4) Set decreaseLoanHighHoldsControl to "over the number of items on the record" 5) Set decreaseLoanHighHoldsDuration to 1 6) Set decreaseLoanHighHoldsValue to 3 7) Create a record with 5 items 8) Please 8 or more holds on the record 9) Check out one of the items to a patron 10) Note the loan length is reduced to 1 day 11) Set decreaseLoanHighHoldsValue to 3 to 2 12) Check out one of the items to a patron 13) Note the loan length is *not* reduced 14) Enbale all the filters possible in decreaseLoanHighHoldsIgnoreStatuses 15) Set one item to be damaged 16) Note the loan length is reduced 17) Unset the damaged status 18) Repeat steps 15 - 17 for lost, not for loan, and withdrawn Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #27 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 46533 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46533&action=edit Bug 14694 [QA Followup] - Fix unit tests -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #28 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 46534 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46534&action=edit Bug 14694 [QA Followup] - Fix typos -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #29 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 46535 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46535&action=edit Bug 14694 [QA Followup] - Fix syspref order -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #46532|0 |1 is obsolete| | Attachment #46533|0 |1 is obsolete| | Attachment #46534|0 |1 is obsolete| | Attachment #46535|0 |1 is obsolete| | --- Comment #30 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 46864 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46864&action=edit Bug 14694 - Make decreaseloanHighHolds more flexible This patch allows for more flexibility for determining when the number of holds a record has should trigger the reduction of the loan length for items on that record. This patch adds a new system preference decreaseLoanHighHoldsControl, which defaults to 'static', the original behavior of the feature. It also has a new behavior 'dynamic' which makes the feature only decrease the loan length if the number of holds on the record exceeds the number of holdable items + decreaseLoanHighHoldsValue. It also allows items to be filtered from the list of items based on the damaged, lost, not for loan, and withdrawn values even if those values would have allowed holds ( i.e. values < 0 ) Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set decreaseLoanHighHolds to Enable 4) Set decreaseLoanHighHoldsControl to "over the number of items on the record" 5) Set decreaseLoanHighHoldsDuration to 1 6) Set decreaseLoanHighHoldsValue to 3 7) Create a record with 5 items 8) Please 8 or more holds on the record 9) Check out one of the items to a patron 10) Note the loan length is reduced to 1 day 11) Set decreaseLoanHighHoldsValue to 3 to 2 12) Check out one of the items to a patron 13) Note the loan length is *not* reduced 14) Enbale all the filters possible in decreaseLoanHighHoldsIgnoreStatuses 15) Set one item to be damaged 16) Note the loan length is reduced 17) Unset the damaged status 18) Repeat steps 15 - 17 for lost, not for loan, and withdrawn Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #31 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 46865 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46865&action=edit Bug 14694 [QA Followup] - Fix unit tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #32 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 46866 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46866&action=edit Bug 14694 [QA Followup] - Fix typos Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #33 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 46867 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46867&action=edit Bug 14694 [QA Followup] - Fix syspref order Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #34 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 46868 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46868&action=edit Bug 14694: Create data the tests need Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14694 --- Comment #35 from Christopher Brannon <cbrannon@cdalibrary.org> --- Awesome! Thank you! Christopher -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org