Bug 22521: Update fines handling to use accountline.status
authorMartin Renvoize <martin.renvoize@ptfs-europe.com>
Fri, 29 Mar 2019 14:31:06 +0000 (14:31 +0000)
committerNick Clemens <nick@bywatersolutions.com>
Wed, 17 Apr 2019 16:49:36 +0000 (16:49 +0000)
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

C4/Circulation.pm
C4/Overdues.pm
Koha/Account.pm
Koha/Account/Line.pm
circ/branchoverdues.pl
misc/cronjobs/staticfines.pl
opac/opac-user.pl
t/db_dependent/Circulation.t
t/db_dependent/Overdues.t

index 832fc52..9a1bdae 100644 (file)
@@ -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();
index a6ff257..f031ff3 100644 (file)
@@ -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())
     ";
index 4da0c1c..ed127d1 100644 (file)
@@ -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',
index 6443b4d..2845bf6 100644 (file)
@@ -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
 
index d4c20cb..7f62185 100755 (executable)
@@ -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
 
index 2a98fea..774c3b0 100755 (executable)
@@ -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 );
index bd638af..cae3981 100755 (executable)
@@ -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}
             },
         );
index 4365784..fde631e 100755 (executable)
@@ -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" );
 };
index 4d6f189..85c3502 100644 (file)
@@ -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,