From dc62187ebff139fe93ac1c404105ac4abf2741b5 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 29 Mar 2019 14:31:06 +0000 Subject: [PATCH] Bug 22521: Update fines handling to use accountline.status Signed-off-by: Martin Renvoize Signed-off-by: Josef Moravec Signed-off-by: Kyle M Hall Signed-off-by: Nick Clemens --- C4/Circulation.pm | 7 ++--- C4/Overdues.pm | 51 ++++++++++++++---------------------- Koha/Account.pm | 7 ++--- Koha/Account/Line.pm | 28 +++++++++++++------- circ/branchoverdues.pl | 6 +++-- misc/cronjobs/staticfines.pl | 4 +-- opac/opac-user.pl | 2 +- t/db_dependent/Circulation.t | 30 ++++++++++++--------- t/db_dependent/Overdues.t | 2 +- 9 files changed, 71 insertions(+), 66 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 832fc52916..9a1bdae225 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2333,7 +2333,8 @@ sub _FixOverduesOnReturn { { borrowernumber => $borrowernumber, itemnumber => $item, - accounttype => 'FU' + accounttype => 'OVERDUE', + status => 'UNRETURNED' } )->next(); return 0 unless $accountline; # no warning, there's just nothing to fix @@ -2341,7 +2342,7 @@ sub _FixOverduesOnReturn { if ($exemptfine) { my $amountoutstanding = $accountline->amountoutstanding; - $accountline->accounttype('FFOR'); + $accountline->status('FORGIVEN'); $accountline->amountoutstanding(0); Koha::Account::Offset->new( @@ -2356,7 +2357,7 @@ sub _FixOverduesOnReturn { &logaction("FINES", 'MODIFY',$borrowernumber,"Overdue forgiven: item $item"); } } else { - $accountline->accounttype('F'); + $accountline->status('RETURNED'); } return $accountline->store(); diff --git a/C4/Overdues.pm b/C4/Overdues.pm index a6ff257797..f031ff3999 100644 --- a/C4/Overdues.pm +++ b/C4/Overdues.pm @@ -520,46 +520,33 @@ sub UpdateFine { } my $dbh = C4::Context->dbh; - # FIXME - What exactly is this query supposed to do? It looks up an - # entry in accountlines that matches the given item and borrower - # numbers, where the description contains $due, and where the - # account type has one of several values, but what does this _mean_? - # Does it look up existing fines for this item? - # FIXME - What are these various account types? ("FU", "O", "F", "M") - # "L" is LOST item - # "A" is Account Management Fee - # "N" is New Card - # "M" is Sundry - # "F" is Fine ?? - # "FU" is Fine UPDATE?? - # "Pay" is Payment - # "REF" is Cash Refund - my $sth = $dbh->prepare( - "SELECT * FROM accountlines - WHERE borrowernumber=? AND - (( accounttype IN ('F','M') AND amountoutstanding<>0 ) OR - accounttype = 'FU' )" + my $overdues = Koha::Account::Lines->search( + { + borrowernumber => $borrowernumber, + accounttype => [ 'OVERDUE', 'M' ], + amountoutstanding => { '<>' => 0 } + } ); - $sth->execute( $borrowernumber ); - my $data; + + my $accountline; my $total_amount_other = 0.00; my $due_qr = qr/$due/; # Cycle through the fines and # - find line that relates to the requested $itemnum # - accumulate fines for other items # so we can update $itemnum fine taking in account fine caps - while (my $rec = $sth->fetchrow_hashref) { - if ( $rec->{issue_id} == $issue_id && $rec->{accounttype} eq 'FU' ) { - if ($data) { + while (my $overdue = $overdues->next) { + if ( $overdue->issue_id == $issue_id && $overdue->status eq 'UNRETURNED' ) { + if ($accountline) { $debug and warn "Not a unique accountlines record for issue_id $issue_id"; #FIXME Should we still count this one in total_amount ?? } else { - $data = $rec; + $accountline = $overdue; next; } } - $total_amount_other += $rec->{'amountoutstanding'}; + $total_amount_other += $overdue->amountoutstanding; } if (my $maxfine = C4::Context->preference('MaxFine')) { @@ -571,10 +558,9 @@ sub UpdateFine { } } - if ( $data ) { - if ( $data->{'amount'} != $amount ) { - my $accountline = - Koha::Account::Lines->find( $data->{accountlines_id} ); + + if ( $accountline ) { + if ( $accountline->amount != $amount ) { $accountline->adjust( { amount => $amount, @@ -593,7 +579,7 @@ sub UpdateFine { my $desc = "$title $due"; my $account = Koha::Account->new({ patron_id => $borrowernumber }); - my $accountline = $account->add_debit( + $accountline = $account->add_debit( { amount => $amount, description => $desc, @@ -738,7 +724,8 @@ sub GetOverduesForBranch { LEFT JOIN itemtypes ON itemtypes.itemtype = $itype_link LEFT JOIN branches ON branches.branchcode = issues.branchcode WHERE (accountlines.amountoutstanding != '0.000000') - AND (accountlines.accounttype = 'FU' ) + AND (accountlines.accounttype = 'OVERDUE' ) + AND (accountlines.status = 'UNRETURNED' ) AND (issues.branchcode = ? ) AND (issues.date_due < NOW()) "; diff --git a/Koha/Account.pm b/Koha/Account.pm index 4da0c1c5ad..ed127d1d4f 100644 --- a/Koha/Account.pm +++ b/Koha/Account.pm @@ -425,7 +425,7 @@ my $debit_line = Koha::Account->new({ patron_id => $patron_id })->add_debit( ); $debit_type can be any of: - - fine + - overdue - lost_item - new_card - account @@ -495,6 +495,7 @@ sub add_debit { itemnumber => $item_id, issue_id => $issue_id, branchcode => $library_id, + ( $type eq 'overdue' ? ( status => 'UNRETURNED' ) : ()), } )->store(); @@ -696,7 +697,7 @@ our $offset_type = { 'processing' => 'Processing Fee', 'lost_item' => 'Lost Item', 'rent' => 'Rental Fee', - 'fine' => 'Fine', + 'overdue' => 'OVERDUE', 'manual_debit' => 'Manual Debit', 'hold_expired' => 'Hold Expired' }; @@ -719,7 +720,7 @@ our $account_type_credit = { our $account_type_debit = { 'account' => 'A', - 'fine' => 'FU', + 'overdue' => 'OVERDUE', 'lost_item' => 'L', 'new_card' => 'N', 'sundry' => 'M', diff --git a/Koha/Account/Line.pm b/Koha/Account/Line.pm index 6443b4d346..2845bf6f2f 100644 --- a/Koha/Account/Line.pm +++ b/Koha/Account/Line.pm @@ -236,7 +236,7 @@ This method allows updating a debit or credit on a patron's account ); $update_type can be any of: - - fine_update + - overdue_update Authors Note: The intention here is that this method is only used to adjust accountlines where the final amount is not yet known/fixed. @@ -259,11 +259,21 @@ sub adjust { ); } - my $account_type = $self->accounttype; - unless ( $Koha::Account::Line::allowed_update->{$update_type} eq $account_type ) { + my $account_type = $self->accounttype; + my $account_status = $self->status; + unless ( + ( + exists( + $Koha::Account::Line::allowed_update->{$update_type} + ->{$account_type} + ) + && ( $Koha::Account::Line::allowed_update->{$update_type} + ->{$account_type} eq $account_status ) + ) + ) + { Koha::Exceptions::Account::UnrecognisedType->throw( - error => 'Update type not allowed on this accounttype' - ); + error => 'Update type not allowed on this accounttype' ); } my $schema = Koha::Database->new->schema; @@ -289,7 +299,7 @@ sub adjust { description => 'Overpayment refund', type => 'credit', interface => $interface, - ( $update_type eq 'fine_update' ? ( item_id => $self->itemnumber ) : ()), + ( $update_type eq 'overdue_update' ? ( item_id => $self->itemnumber ) : ()), } ); $new_outstanding = 0; @@ -300,7 +310,7 @@ sub adjust { { date => \'NOW()', amount => $amount, - amountoutstanding => $new_outstanding + amountoutstanding => $new_outstanding, } )->store(); @@ -329,7 +339,7 @@ sub adjust { manager_id => undef, } ) - ) if ( $update_type eq 'fine_update' ); + ) if ( $update_type eq 'overdue_update' ); } } ); @@ -381,7 +391,7 @@ sub _type { =cut -our $allowed_update = { 'fine_update' => 'FU', }; +our $allowed_update = { 'overdue_update' => { 'OVERDUE' => 'UNRETURNED' } }; =head1 AUTHORS diff --git a/circ/branchoverdues.pl b/circ/branchoverdues.pl index d4c20cbb18..7f6218525e 100755 --- a/circ/branchoverdues.pl +++ b/circ/branchoverdues.pl @@ -31,8 +31,10 @@ use Data::Dumper; =head1 branchoverdues.pl - this module is a new interface, allow to the librarian to check all items on overdues (based on the acountlines type 'FU' ) - this interface is filtered by branches (automatically), and by location (optional) .... +This view is used to display all overdue items to the librarian. + +It is automatically filtered by branch and can optionally be filtered +by item location. =cut diff --git a/misc/cronjobs/staticfines.pl b/misc/cronjobs/staticfines.pl index 2a98fea6ec..774c3b0e56 100755 --- a/misc/cronjobs/staticfines.pl +++ b/misc/cronjobs/staticfines.pl @@ -229,8 +229,8 @@ for ( my $i = 0 ; $i < scalar(@$data) ; $i++ ) { my $desc = "staticfine"; my $query = "INSERT INTO accountlines - (borrowernumber,itemnumber,date,amount,description,accounttype,amountoutstanding) - VALUES (?,?,now(),?,?,'F',?)"; + (borrowernumber,itemnumber,date,amount,description,accounttype,status,amountoutstanding) + VALUES (?,?,now(),?,?,'OVERDUE','RETURNED',?)"; my $sth2 = $dbh->prepare($query); $bigdebug and warn "query: $query\nw/ args: $borrowernumber, $itemnumber, $amount, $desc, $amount\n"; $sth2->execute( $borrowernumber, $itemnumber, $amount, $desc, $amount ); diff --git a/opac/opac-user.pl b/opac/opac-user.pl index bd638afc7d..cae3981176 100755 --- a/opac/opac-user.pl +++ b/opac/opac-user.pl @@ -185,7 +185,7 @@ if ( $pending_checkouts->count ) { # Useless test { borrowernumber => $patron->borrowernumber, amountoutstanding => { '>' => 0 }, - accounttype => [ 'F', 'FU', 'L' ], + accounttype => [ 'F', 'L' ], itemnumber => $issue->{itemnumber} }, ); diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 436578450e..fde631e40c 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -509,8 +509,8 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); my $fines = Koha::Account::Lines->search( { borrowernumber => $renewing_borrower->{borrowernumber}, itemnumber => $item_7->itemnumber } ); is( $fines->count, 2 ); - is( $fines->next->accounttype, 'F', 'Fine on renewed item is closed out properly' ); - is( $fines->next->accounttype, 'F', 'Fine on renewed item is closed out properly' ); + isnt( $fines->next->status, 'UNRETURNED', 'Fine on renewed item is closed out properly' ); + isnt( $fines->next->status, 'UNRETURNED', 'Fine on renewed item is closed out properly' ); $fines->delete(); @@ -696,7 +696,7 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); item_id => $item_to_auto_renew->{itemnumber}, description => "Some fines" } - )->accounttype('F')->store; + )->status('RETURNED')->store; ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrowernumber, $item_to_auto_renew->{itemnumber} ); is( $renewokay, 0, 'Do not renew, renewal is automatic' ); @@ -710,7 +710,7 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); item_id => $item_to_auto_renew->{itemnumber}, description => "Some fines" } - )->accounttype('F')->store; + )->status('RETURNED')->store; ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrowernumber, $item_to_auto_renew->{itemnumber} ); is( $renewokay, 0, 'Do not renew, renewal is automatic' ); @@ -724,7 +724,7 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); item_id => $item_to_auto_renew->{itemnumber}, description => "Some fines" } - )->accounttype('F')->store; + )->status('RETURNED')->store; ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrowernumber, $item_to_auto_renew->{itemnumber} ); is( $renewokay, 0, 'Do not renew, renewal is automatic' ); @@ -858,7 +858,8 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); ); my $line = Koha::Account::Lines->search({ borrowernumber => $renewing_borrower->{borrowernumber} })->next(); - is( $line->accounttype, 'FU', 'Account line type is FU' ); + is( $line->accounttype, 'OVERDUE', 'Account line type is OVERDUE' ); + is( $line->status, 'UNRETURNED', 'Account line status is UNRETURNED' ); is( $line->amountoutstanding, '15.000000', 'Account line amount outstanding is 15.00' ); is( $line->amount, '15.000000', 'Account line amount is 15.00' ); is( $line->issue_id, $issue->id, 'Account line issue id matches' ); @@ -873,7 +874,8 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); LostItem( $item_1->itemnumber, 'test', 1 ); $line = Koha::Account::Lines->find($line->id); - is( $line->accounttype, 'F', 'Account type correctly changed from FU to F' ); + is( $line->accounttype, 'OVERDUE', 'Account type remains as OVERDUE' ); + isnt( $line->status, 'UNRETURNED', 'Account status correctly changed from UNRETURNED to RETURNED' ); my $item = Koha::Items->find($item_1->itemnumber); ok( !$item->onloan(), "Lost item marked as returned has false onloan value" ); @@ -2048,7 +2050,7 @@ subtest 'AddReturn | is_overdue' => sub { # specify dropbox date 5 days later => overdue, or... not AddIssue( $patron->unblessed, $item->{barcode}, $ten_days_ago ); # date due was 10d ago AddReturn( $item->{barcode}, $library->{branchcode}, $five_days_ago ); - is( int($patron->account->balance()), 0, 'AddReturn: pass return_date => no overdue in dropbox mode' ); # FIXME? This is weird, the FU fine is created ( _CalculateAndUpdateFine > C4::Overdues::UpdateFine ) then remove later (in _FixOverduesOnReturn). Looks like it is a feature + is( int($patron->account->balance()), 0, 'AddReturn: pass return_date => no overdue in dropbox mode' ); # FIXME? This is weird, the OVERDUE fine is created ( _CalculateAndUpdateFine > C4::Overdues::UpdateFine ) then remove later (in _FixOverduesOnReturn). Looks like it is a feature Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber })->delete; }; @@ -2430,7 +2432,7 @@ subtest '_FixAccountForLostAndReturned' => sub { is( $account->balance, $manual_debit_amount - $payment_amount, 'Balance is PF - payment (CR)' ); - my $manual_debit = Koha::Account::Lines->search({ borrowernumber => $patron->id, accounttype => 'FU' })->next; + my $manual_debit = Koha::Account::Lines->search({ borrowernumber => $patron->id, accounttype => 'OVERDUE', status => 'UNRETURNED' })->next; is( $manual_debit->amountoutstanding + 0, $manual_debit_amount - $payment_amount, 'reconcile_balance was called' ); }; }; @@ -2457,7 +2459,8 @@ subtest '_FixOverduesOnReturn' => sub { my $accountline = Koha::Account::Line->new( { borrowernumber => $patron->{borrowernumber}, - accounttype => 'FU', + accounttype => 'OVERDUE', + status => 'UNRETURNED', itemnumber => $item->itemnumber, amount => 99.00, amountoutstanding => 99.00, @@ -2470,13 +2473,14 @@ subtest '_FixOverduesOnReturn' => sub { $accountline->_result()->discard_changes(); is( $accountline->amountoutstanding, '99.000000', 'Fine has the same amount outstanding as previously' ); - is( $accountline->accounttype, 'F', 'Open fine ( account type FU ) has been closed out ( account type F )'); + is( $accountline->status, 'RETURNED', 'Open fine ( account type OVERDUE ) has been closed out ( status RETURNED )'); ## Run again, with exemptfine enabled $accountline->set( { - accounttype => 'FU', + accounttype => 'OVERDUE', + status => 'UNRETURNED', amountoutstanding => 99.00, } )->store(); @@ -2487,7 +2491,7 @@ subtest '_FixOverduesOnReturn' => sub { my $offset = Koha::Account::Offsets->search({ debit_id => $accountline->id, type => 'Forgiven' })->next(); is( $accountline->amountoutstanding + 0, 0, 'Fine has been reduced to 0' ); - is( $accountline->accounttype, 'FFOR', 'Open fine ( account type FU ) has been set to fine forgiven ( account type FFOR )'); + is( $accountline->status, 'FORGIVEN', 'Open fine ( account type OVERDUE ) has been set to fine forgiven ( status FORGIVEN )'); is( ref $offset, "Koha::Account::Offset", "Found matching offset for fine reduction via forgiveness" ); is( $offset->amount, '-99.000000', "Amount of offset is correct" ); }; diff --git a/t/db_dependent/Overdues.t b/t/db_dependent/Overdues.t index 4d6f18939c..85c3502c6a 100644 --- a/t/db_dependent/Overdues.t +++ b/t/db_dependent/Overdues.t @@ -251,7 +251,7 @@ subtest 'UpdateFine tests' => sub { is( $fine2->amount, '30.000000', "Second fine increased after partial payment of first" ); # Fix fine 1, create third fine - $fine->accounttype('F')->store; + $fine->status('RETURNED')->store; UpdateFine( { issue_id => $checkout1->issue_id, -- 2.39.5