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
Created attachment 42957 [details] [review] 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
Created attachment 42958 [details] [review] 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
Created attachment 42970 [details] [review] 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
Created attachment 43153 [details] [review] 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
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)
The feature works and I like it, but sorry, tests do not work. See comment #5
Created attachment 43513 [details] [review] 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
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
Created attachment 43589 [details] [review] 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
Created attachment 43598 [details] [review] [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>
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
# 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.
Created attachment 43752 [details] [review] Bug 14694 [QA Followup] - Fix unit tests
Comment on attachment 43598 [details] [review] [SIGNED-OFF] Bug 14694 - Make decreaseloanHighHolds more flexible Review of attachment 43598 [details] [review]: ----------------------------------------------------------------- ::: 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.
Comment on attachment 43752 [details] [review] Bug 14694 [QA Followup] - Fix unit tests Review of attachment 43752 [details] [review]: ----------------------------------------------------------------- ::: 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
Created attachment 43768 [details] [review] Bug 14694 [QA Followup] - Fix typos
> ::: 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.
(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?
Hm, waiting for feedback on comment 18.
(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?
Created attachment 43952 [details] [review] Bug 14694 [QA Followup] - Fix syspref order
(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.
Would LOVE it if this could get a QA. :) Christopher
I would love to get an answer when I ask for a question.
(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.
Created attachment 46532 [details] [review] 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>
Created attachment 46533 [details] [review] Bug 14694 [QA Followup] - Fix unit tests
Created attachment 46534 [details] [review] Bug 14694 [QA Followup] - Fix typos
Created attachment 46535 [details] [review] Bug 14694 [QA Followup] - Fix syspref order
Created attachment 46864 [details] [review] 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>
Created attachment 46865 [details] [review] Bug 14694 [QA Followup] - Fix unit tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 46866 [details] [review] Bug 14694 [QA Followup] - Fix typos Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 46867 [details] [review] Bug 14694 [QA Followup] - Fix syspref order Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 46868 [details] [review] Bug 14694: Create data the tests need Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Awesome! Thank you! Christopher
Didn't apply cleanly - I know because of the moving target that master is. Please rebase. Once it's done - leave me an @later in #koha and I'll jump right on it. Thank you so much for the time and bother of rebasing. Brendan
Created attachment 48576 [details] [review] 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>
Created attachment 48577 [details] [review] Bug 14694 [QA Followup] - Fix unit tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 48578 [details] [review] Bug 14694 [QA Followup] - Fix typos Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 48579 [details] [review] Bug 14694 [QA Followup] - Fix syspref order Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 48580 [details] [review] Bug 14694: Create data the tests need Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Conflict was with bug 15629 adding Koha::Libraries on the first patch. Everything is code identical. Ran t/db_dependent/Circulation_dateexpiry.t and t/db_dependent/DecreaseLoanHighHolds.t with no issue.
Created attachment 48678 [details] [review] Bug 14694: Create data the tests need Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to Master - Thanks! Should be in the May 2016 release. Thanks!
Created attachment 48804 [details] [review] Bug 14694 [QA Followup] - Update Koha::Borrower references to Koha::Patron