From 5302a1bb66412ed1baa8bca786f712ea50d57c94 Mon Sep 17 00:00:00 2001 From: David Gustafsson Date: Wed, 21 Sep 2022 16:09:51 +0200 Subject: [PATCH] Bug 29145: Add tests and modify sysprefs Add tests, remove ODueDebarmentRemovalAllowUnrestricted syspref and instead modify AutoRemoveOverduesRestrictions to have a third option Signed-off-by: Michaela --- C4/Circulation.pm | 22 ++--- Koha/Patron.pm | 4 +- ..._AutoRemoveOverduesRestrictions-syspref.pl | 13 +++ ...ue-debarment-removal-allow-unrestricted.pl | 14 --- installer/data/mysql/mandatory/sysprefs.sql | 2 +- .../admin/preferences/circulation.pref | 15 ++-- .../Circulation/MarkIssueReturned.t | 89 +++++++++++++++++-- 7 files changed, 116 insertions(+), 43 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_29145-modify_AutoRemoveOverduesRestrictions-syspref.pl delete mode 100644 installer/data/mysql/atomicupdate/odue-debarment-removal-allow-unrestricted.pl diff --git a/C4/Circulation.pm b/C4/Circulation.pm index eca6d2d778..8c2abf2227 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2486,6 +2486,7 @@ sub MarkIssueReturned { # Retrieve the issue my $issue = Koha::Checkouts->find( { itemnumber => $itemnumber } ) or return; + my $issue_branchcode = $issue->branchcode; return unless $issue->borrowernumber == $borrowernumber; # If the item is checked out to another patron we do not return it @@ -2532,18 +2533,17 @@ sub MarkIssueReturned { # The reason this is here, and not in Koha::Patron->has_overdues() is # to make sure it will not cause any side effects elsewhere, since this # is only relevant for removal of debarments. - my $has_overdue_ignore_unrestricted = 0; - if(C4::Context->preference('ODueDebarmentRemovalAllowUnrestricted')) { - $has_overdue_ignore_unrestricted = 1; - } + my $has_overdue_ignore_unrestricted = C4::Context->preference('AutoRemoveOverduesRestrictions') eq 'when_no_overdue_causing_debarment'; - # Remove any OVERDUES related debarment if the borrower has no overdues - if ( C4::Context->preference('AutoRemoveOverduesRestrictions') - && $patron->debarred - && !$patron->has_overdues({ - ignore_unrestricted => $has_overdue_ignore_unrestricted, - issue_branch => $issue->{'branchcode'} }) - && @{ GetDebarments({ borrowernumber => $borrowernumber, type => 'OVERDUES' }) } + # Possibly remove any OVERDUES related debarment + if ( + C4::Context->preference('AutoRemoveOverduesRestrictions') ne 'no' + && $patron->debarred + && !$patron->has_overdues({ + ignore_unrestricted => $has_overdue_ignore_unrestricted, + issue_branchcode => $issue_branchcode + }) + && @{ GetDebarments({ borrowernumber => $borrowernumber, type => 'OVERDUES' }) } ) { DelUniqueDebarment({ borrowernumber => $borrowernumber, type => 'OVERDUES' }); } diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 2cb28dd86b..ffa771104a 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -1002,11 +1002,11 @@ sub _get_overdue_restrict_delay { my $query = "SELECT * FROM overduerules WHERE delay1 IS NOT NULL AND branchcode = ? AND categorycode = ?"; - my $rqoverduerules = $dbh->prepare($query); + my $rqoverduerules = $dbh->prepare($query); $rqoverduerules->execute($branchcode, $categorycode); # We get default rules if there is no rule for this branch - if($rqoverduerules->rows == 0){ + if($rqoverduerules->rows == 0) { $query = "SELECT * FROM overduerules WHERE delay1 IS NOT NULL AND branchcode = '' AND categorycode = ?"; $rqoverduerules = $dbh->prepare($query); diff --git a/installer/data/mysql/atomicupdate/bug_29145-modify_AutoRemoveOverduesRestrictions-syspref.pl b/installer/data/mysql/atomicupdate/bug_29145-modify_AutoRemoveOverduesRestrictions-syspref.pl new file mode 100644 index 0000000000..ca210104c4 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_29145-modify_AutoRemoveOverduesRestrictions-syspref.pl @@ -0,0 +1,13 @@ +use Modern::Perl; + +return { + bug_number => "", + description => "Change type of AutoRemoveOverduesRestrictions system preference", + up => sub { + my ($args) = @_; + my ($dbh, $out) = @$args{qw(dbh out)}; + # Do you stuffs here + $dbh->do(q{UPDATE `systempreferences` SET `type` = 'Choice', `options` = 'no|when_no_overdue|when_no_overdue_causing_debarment', `explanation` = 'Defines if and on what conditions OVERDUES debarments should automatically be lifted when overdue items are returned by the patron.', `value` = CASE `value` WHEN '1' THEN 'when_no_overdue' WHEN '0' THEN 'no' ELSE `value` END WHERE variable = 'AutoRemoveOverduesRestrictions'}); + say $out "Type of AutoRemoveOverduesRestrictions system prefernce has ben changed"; + }, +} diff --git a/installer/data/mysql/atomicupdate/odue-debarment-removal-allow-unrestricted.pl b/installer/data/mysql/atomicupdate/odue-debarment-removal-allow-unrestricted.pl deleted file mode 100644 index 3a019999d1..0000000000 --- a/installer/data/mysql/atomicupdate/odue-debarment-removal-allow-unrestricted.pl +++ /dev/null @@ -1,14 +0,0 @@ -use Modern::Perl; - -return { - bug_number => "", - description => "Add system preference", - up => sub { - my ($args) = @_; - my ($dbh, $out) = @$args{qw(dbh out)}; - # Do you stuffs here - $dbh->do(q{INSERT IGNORE INTO systempreferences (`variable`, `value`, `options`, `explanation`, `type`) VALUES ('ODueDebarmentRemovalAllowUnrestricted', '0', null, 'Allow removal of OVERDUES debarment when overdues still exist, but has not reached restricting delay', 'YesNo')}); - # Print useful stuff here - say $out "System preference added"; - }, -} diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index c1ff4a2f38..652bd766b6 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -84,7 +84,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('AutomaticItemReturn','1',NULL,'If ON, Koha will automatically set up a transfer of this item to its homebranch','YesNo'), ('AutoLinkBiblios','0',NULL,'If enabled, link biblio to authorities on creation and edit','YesNo'), ('autoMemberNum','0','','If ON, patron number is auto-calculated','YesNo'), -('AutoRemoveOverduesRestrictions','0',NULL,'Defines whether an OVERDUES debarment should be lifted automatically if all overdue items are returned by the patron.','YesNo'), +('AutoRemoveOverduesRestrictions','no','no|when_no_overdue|when_no_overdue_causing_debarment', 'Defines if and on what conditions OVERDUES debarments should automatically be lifted when overdue items are returned by the patron.','Choice'), ('AutoRenewalNotices','cron','cron|preferences|never','How should Koha determine whether to end autorenewal notices','Choice'), ('AutoResumeSuspendedHolds','1',NULL,'Allow suspended holds to be automatically resumed by a set date.','YesNo'), ('AutoReturnCheckedOutItems', '0', '', 'If disabled, librarian must confirm return of checked out item when checking out to another.', 'YesNo'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref index 26803bd0ba..ce9c958d06 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ -663,12 +663,6 @@ Circulation: 1: Store 0: "Don't store" - 'the last patron to return an item. This setting is independent of the opacreadinghistory and AnonymousPatron system preferences.' - - - - pref: ODueDebarmentRemovalAllowUnrestricted - choices: - yes: Allow - no: Do not allow - - removal of Overdue debarments when patron has overdue items but none are old enough to have reached restricting delay. Used in combination with AutoRemoveOverduesRestrictions. Holds policy: - - pref: EnableItemGroups @@ -988,11 +982,14 @@ Circulation: 0: "Don't cumulate" - the restriction periods. - + - When returning items - pref: AutoRemoveOverduesRestrictions + type: choice choices: - 1: "Do" - 0: "Don't" - - allow OVERDUES restrictions triggered by sent notices to be cleared automatically when all overdue items are returned by a patron. + when_no_overdue: "if patron has no remaining overdue items" + when_no_overdue_causing_debarment: "if patron has no remaining items that is cause for debarment" + no: "don't" + - remove OVERDUES restriction triggered by sent notices. The difference between removing restriction when no remaining overdue items exists and doing so only when any of the items would result in debarment is that the latter option will respect possible grace periods of overdue rules also on returns and not only when generating overdue notices. - - If a patron is restricted, - pref: RestrictionBlockRenewing diff --git a/t/db_dependent/Circulation/MarkIssueReturned.t b/t/db_dependent/Circulation/MarkIssueReturned.t index 654ac37451..21a4227d46 100755 --- a/t/db_dependent/Circulation/MarkIssueReturned.t +++ b/t/db_dependent/Circulation/MarkIssueReturned.t @@ -173,20 +173,37 @@ subtest 'Manually pass a return date' => sub { }; subtest 'AutoRemoveOverduesRestrictions' => sub { - plan tests => 2; + plan tests => 5; $schema->storage->txn_begin; - t::lib::Mocks::mock_preference('AutoRemoveOverduesRestrictions', 1); - + my $dbh = C4::Context->dbh; my $patron = $builder->build_object({ class => 'Koha::Patrons' }); - t::lib::Mocks::mock_userenv( { branchcode => $patron->branchcode } ); + my $categorycode = $patron->categorycode; + my $branchcode = $patron->branchcode; + + $dbh->do(qq{ + INSERT INTO `overduerules` ( + `categorycode`, + `delay1`, + `letter1`, + `debarred1`, + `delay2`, + `letter2`, + `debarred2` + ) + VALUES ('$categorycode', 6, 'ODUE', 0, 10, 'ODUE2', 1) + }); + + t::lib::Mocks::mock_preference('AutoRemoveOverduesRestrictions', 'when_no_overdue'); + + t::lib::Mocks::mock_userenv( { branchcode => $branchcode } ); my $item_1 = $builder->build_sample_item; my $item_2 = $builder->build_sample_item; my $item_3 = $builder->build_sample_item; my $five_days_ago = dt_from_string->subtract( days => 5 ); - my $checkout_1 = AddIssue( $patron->unblessed, $item_1->barcode, $five_days_ago ); # overdue - my $checkout_2 = AddIssue( $patron->unblessed, $item_2->barcode, $five_days_ago ); # overdue + my $checkout_1 = AddIssue( $patron->unblessed, $item_1->barcode, $five_days_ago ); # overdue, but would not trigger debarment + my $checkout_2 = AddIssue( $patron->unblessed, $item_2->barcode, $five_days_ago ); # overdue, but would not trigger debarment my $checkout_3 = AddIssue( $patron->unblessed, $item_3->barcode ); # not overdue Koha::Patron::Debarments::AddUniqueDebarment( @@ -207,5 +224,65 @@ subtest 'AutoRemoveOverduesRestrictions' => sub { $debarments = Koha::Patron::Debarments::GetDebarments({ borrowernumber => $patron->borrowernumber }); is( scalar @$debarments, 0, 'OVERDUES debarment is removed if patron does not have overdues' ); + t::lib::Mocks::mock_preference('AutoRemoveOverduesRestrictions', 'when_no_overdue_causing_debarment'); + + my $eleven_days_ago = dt_from_string->subtract( days => 11 ); + + $checkout_1 = AddIssue( $patron->unblessed, $item_1->barcode, $eleven_days_ago ); # overdue and would trigger debarment + $checkout_2 = AddIssue( $patron->unblessed, $item_2->barcode, $five_days_ago ); # overdue, but would not trigger debarment + + Koha::Patron::Debarments::AddUniqueDebarment( + { + borrowernumber => $patron->borrowernumber, + type => 'OVERDUES', + comment => "OVERDUES_PROCESS simulation", + } + ); + + C4::Circulation::MarkIssueReturned( $patron->borrowernumber, $item_1->itemnumber ); + + $debarments = Koha::Patron::Debarments::GetDebarments({ borrowernumber => $patron->borrowernumber }); + is( scalar @$debarments, 0, 'OVERDUES debarment is removed if remaning items would not result in patron debarment' ); + + $checkout_1 = AddIssue( $patron->unblessed, $item_1->barcode, $eleven_days_ago ); # overdue and would trigger debarment + + Koha::Patron::Debarments::AddUniqueDebarment( + { + borrowernumber => $patron->borrowernumber, + type => 'OVERDUES', + comment => "OVERDUES_PROCESS simulation", + } + ); + + C4::Circulation::MarkIssueReturned( $patron->borrowernumber, $item_2->itemnumber ); + + $debarments = Koha::Patron::Debarments::GetDebarments({ borrowernumber => $patron->borrowernumber }); + is( $debarments->[0]->{type}, 'OVERDUES', 'OVERDUES debarment is not removed if patron still has overdues that would trigger debarment' ); + + my $thirteen_days_ago = dt_from_string->subtract( days => 13 ); + + # overdue and would trigger debarment + $checkout_2 = AddIssue( $patron->unblessed, $item_2->barcode, $thirteen_days_ago ); + + # $chechout_1 should now not trigger debarment with this new rule for specific branchcode + $dbh->do(qq{ + INSERT INTO `overduerules` ( + `branchcode`, + `categorycode`, + `delay1`, + `letter1`, + `debarred1`, + `delay2`, + `letter2`, + `debarred2` + ) + VALUES ('$branchcode', '$categorycode', 6, 'ODUE', 0, 12, 'ODUE2', 1) + }); + + C4::Circulation::MarkIssueReturned( $patron->borrowernumber, $item_2->itemnumber ); + + $debarments = Koha::Patron::Debarments::GetDebarments({ borrowernumber => $patron->borrowernumber }); + is( scalar @$debarments, 0, 'OVERDUES debarment is removed if remaning items would not result in patron debarment' ); + $schema->storage->txn_rollback; }; -- 2.30.2