From 7162dd0f2c8df5a4bc382a8d1bce9d323b0fbaae Mon Sep 17 00:00:00 2001 From: David Gustafsson Date: Thu, 13 Oct 2022 13:20:12 +0200 Subject: [PATCH] Bug 29145: Fix conditions for patron debarring overdues --- C4/Circulation.pm | 27 ++++------ Koha/Patron.pm | 53 +++++++++++++++---- .../Circulation/MarkIssueReturned.t | 24 +++++---- 3 files changed, 68 insertions(+), 36 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index b016298cb9..9578d4fea9 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2528,24 +2528,19 @@ sub MarkIssueReturned { $item->last_returned_by( $patron ); } - # 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 = C4::Context->preference('AutoRemoveOverduesRestrictions') eq 'when_no_overdue_causing_debarment'; - # 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' }); + if (C4::Context->preference('AutoRemoveOverduesRestrictions') ne 'no' && $patron->is_debarred) { + my $remove_restrictions = + C4::Context->preference('AutoRemoveOverduesRestrictions') eq 'when_no_overdue_causing_debarment' ? + !$patron->has_debarring_overdues({ issue_branchcode => $issue_branchcode }) : + !$patron->has_overdues; + if ( + $remove_restrictions && + @{ GetDebarments({ borrowernumber => $borrowernumber, type => 'OVERDUES' }) } + ) { + DelUniqueDebarment({ borrowernumber => $borrowernumber, type => 'OVERDUES' }); + } } - }); return $issue_id; diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 68c2de862c..023216312d 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -945,24 +945,60 @@ Returns the number of patron's overdues =cut sub has_overdues { - my ($self, $params) = @_; + my ($self) = @_; my $date = dt_from_string(); + my $dtf = Koha::Database->new->schema->storage->datetime_parser; + return $self->_result->issues->search({ date_due => { '<' => $dtf->format_datetime($date) } })->count; +} + +sub has_debarring_overdues { + my ($self, $params) = @_; + $params //= {}; + my $date = dt_from_string()->truncate( to => 'day' ); # If ignoring unrestricted overdues, calculate which delay value for # overdue messages is set with restrictions. Then only include overdue # issues older than that date when counting. - if($params->{ignore_unrestricted}) { - my $branchcode = $params->{issue_branchcode}; - my $date_offset = _get_overdue_restrict_delay($params->{issue_branchcode}, $self->categorycode()); - $date->subtract(days => $date_offset); + #TODO: bail out/throw exception if $params->{issue_branchcode} not set? + my $debarred_delay = _get_overdue_debarred_delay($params->{issue_branchcode}, $self->categorycode()); + return 0 unless defined $debarred_delay; + + # Emulate the conditions in overdue_notices.pl. + # The overdue_notices-script effectively truncates both issues.date_due and current date + # to days when selecting overdue issues. + # Hours and minutes for issues.date_due is usually set to 23 and 59 respectively, though can theoretically + # be set to any other value (truncated to minutes, except if CalcDateDue gets a $startdate) + # + # No matter what time of day date_due is set to, overdue_notices.pl will select all issues that are due + # the current date or later. We can emulate this query by instead of truncating both to days in the SQL-query, + # using the condition that date_due must be less then the current date truncated to days (time set to 00:00:00) + # offset by one day in the future. + + #$date->subtract(days => $debarred_delay - 1); + $date->add(days => 1); + + my $calendar; + if (C4::Context->preference('OverdueNoticeCalendar')) { + $calendar = Koha::Calendar->new( branchcode => $params->{issue_branchcode} ); } my $dtf = Koha::Database->new->schema->storage->datetime_parser; - return $self->_result->issues->search({ date_due => { '<' => $dtf->format_datetime( $date )} })->count; + my $issues = $self->_result->issues->search({ date_due => { '<' => $dtf->format_datetime($date) } }); + my $now = dt_from_string(); + + while (my $issue = $issues->next) { + my $days_between = C4::Context->preference('OverdueNoticeCalendar') ? + $calendar->days_between(dt_from_string($issue->date_due), $now)->in_units('days') : + $now->delta_days(dt_from_string($issue->date_due))->in_units('days'); + if ($days_between >= $debarred_delay) { + return 1; + } + } + return 0; } # Fetch first delayX value from overduerules where debarredX is set, or 0 for no delay -sub _get_overdue_restrict_delay { +sub _get_overdue_debarred_delay { my ($branchcode, $categorycode) = @_; my $dbh = C4::Context->dbh(); @@ -984,8 +1020,7 @@ sub _get_overdue_restrict_delay { return $overdue_rules->{"delay2"} if($overdue_rules->{"debarred2"}); return $overdue_rules->{"delay3"} if($overdue_rules->{"debarred3"}); } - - return 0; + return undef; } =head3 track_login diff --git a/t/db_dependent/Circulation/MarkIssueReturned.t b/t/db_dependent/Circulation/MarkIssueReturned.t index 21a4227d46..3396e7d812 100755 --- a/t/db_dependent/Circulation/MarkIssueReturned.t +++ b/t/db_dependent/Circulation/MarkIssueReturned.t @@ -201,9 +201,9 @@ subtest 'AutoRemoveOverduesRestrictions' => sub { 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, but would not trigger debarment - my $checkout_2 = AddIssue( $patron->unblessed, $item_2->barcode, $five_days_ago ); # overdue, but would not trigger debarment + my $nine_days_ago = dt_from_string->subtract( days => 9 ); #->set_hour(23)->set_minute(59); + my $checkout_1 = AddIssue( $patron->unblessed, $item_1->barcode, $nine_days_ago ); # overdue, but would not trigger debarment + my $checkout_2 = AddIssue( $patron->unblessed, $item_2->barcode, $nine_days_ago ); # overdue, but would not trigger debarment my $checkout_3 = AddIssue( $patron->unblessed, $item_3->barcode ); # not overdue Koha::Patron::Debarments::AddUniqueDebarment( @@ -226,10 +226,12 @@ subtest 'AutoRemoveOverduesRestrictions' => sub { t::lib::Mocks::mock_preference('AutoRemoveOverduesRestrictions', 'when_no_overdue_causing_debarment'); - my $eleven_days_ago = dt_from_string->subtract( days => 11 ); + my $ten_days_ago = dt_from_string->subtract( days => 10 ); + + $checkout_1 = AddIssue( $patron->unblessed, $item_1->barcode, $ten_days_ago ); # overdue and would trigger debarment + $checkout_2 = AddIssue( $patron->unblessed, $item_2->barcode, $nine_days_ago ); # overdue, but would not trigger debarment - $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 + print STDERR "DUEDATE: " . $nine_days_ago->stringify . "\n"; Koha::Patron::Debarments::AddUniqueDebarment( { @@ -244,7 +246,7 @@ subtest 'AutoRemoveOverduesRestrictions' => sub { $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 + $checkout_1 = AddIssue( $patron->unblessed, $item_1->barcode, $ten_days_ago ); # overdue and would trigger debarment Koha::Patron::Debarments::AddUniqueDebarment( { @@ -259,12 +261,12 @@ subtest 'AutoRemoveOverduesRestrictions' => sub { $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 ); + my $eleven_days_ago = dt_from_string->subtract( days => 11 ); # overdue and would trigger debarment - $checkout_2 = AddIssue( $patron->unblessed, $item_2->barcode, $thirteen_days_ago ); + $checkout_2 = AddIssue( $patron->unblessed, $item_2->barcode, $eleven_days_ago ); - # $chechout_1 should now not trigger debarment with this new rule for specific branchcode + # $checkout_1 should now not trigger debarment with this new rule for specific branchcode $dbh->do(qq{ INSERT INTO `overduerules` ( `branchcode`, @@ -276,7 +278,7 @@ subtest 'AutoRemoveOverduesRestrictions' => sub { `letter2`, `debarred2` ) - VALUES ('$branchcode', '$categorycode', 6, 'ODUE', 0, 12, 'ODUE2', 1) + VALUES ('$branchcode', '$categorycode', 6, 'ODUE', 0, 11, 'ODUE2', 1) }); C4::Circulation::MarkIssueReturned( $patron->borrowernumber, $item_2->itemnumber ); -- 2.35.1