From ee8ef4b17411057424ed7baefab2148b517bf44d 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 Signed-off-by: Tomas Cohen Arazi --- C4/Circulation.pm | 10 +-- 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, 109 insertions(+), 38 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 9a5f9edff9..b635abe5be 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2580,6 +2580,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 @@ -2626,14 +2627,11 @@ 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 + # Possibly remove any OVERDUES related debarment my $overdue_restrictions = $patron->restrictions->search({ type => 'OVERDUES' }); - if ( C4::Context->preference('AutoRemoveOverduesRestrictions') + if ( C4::Context->preference('AutoRemoveOverduesRestrictions' ne 'no') && $patron->debarred && $overdue_restrictions->count && !$patron->has_overdues({ diff --git a/Koha/Patron.pm b/Koha/Patron.pm index e39670e6f8..6ac934f241 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -1031,11 +1031,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 78f02829c0..6aed5a753e 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -86,7 +86,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('AutoLinkBiblios','0',NULL,'If enabled, link biblio to authorities on creation and edit','YesNo'), ('AutomaticConfirmTransfer','0',NULL,'Defines whether transfers should be automatically confirmed at checkin if modal dismissed','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 c7b2caf57b..bbbeb2fc32 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 @@ -658,12 +658,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: - - In the staff interface, split the holds queue into separate tables by @@ -990,11 +984,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 ee66ff558a..011a3512a9 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, $item_1->barcode, $five_days_ago ); # overdue - my $checkout_2 = AddIssue( $patron, $item_2->barcode, $five_days_ago ); # overdue + my $checkout_1 = AddIssue( $patron, $item_1->barcode, $five_days_ago ); # overdue, but would not trigger debarment + my $checkout_2 = AddIssue( $patron, $item_2->barcode, $five_days_ago ); # overdue, but would not trigger debarment my $checkout_3 = AddIssue( $patron, $item_3->barcode ); # not overdue Koha::Patron::Debarments::AddUniqueDebarment( @@ -208,5 +225,65 @@ subtest 'AutoRemoveOverduesRestrictions' => sub { $restrictions = $patron->restrictions; is( $restrictions->count, 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.39.5