Bug 8770 - amount does not equal amountoutstanding on a fine with no payments or credits
Summary: amount does not equal amountoutstanding on a fine with no payments or credits
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-13 19:04 UTC by Melia Meggs
Modified: 2014-05-26 21:04 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
amount =! amountoutstanding screenshot1 (76.98 KB, image/jpeg)
2012-09-13 21:02 UTC, Melia Meggs
Details
amount != amountoutstanding screenshot2 (76.42 KB, image/jpeg)
2012-09-13 21:03 UTC, Melia Meggs
Details
amount != amountoutstanding screenshot3 (58.91 KB, image/jpeg)
2012-09-13 21:04 UTC, Melia Meggs
Details
Fine error 1 (deleted)
2012-09-25 22:11 UTC, Jesse Maseto
Details
fine error 2 (deleted)
2012-09-25 22:13 UTC, Jesse Maseto
Details
Fines 1 (400.87 KB, image/jpeg)
2012-09-26 14:05 UTC, Jesse Maseto
Details
Fines 2 (449.54 KB, image/jpeg)
2012-09-26 14:06 UTC, Jesse Maseto
Details
Bug 8770 - amount does not equal amountoutstanding on a fine with no payments or credits (2.35 KB, patch)
2013-04-03 17:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8770 - amount does not equal amountoutstanding on a fine with no payments or credits (2.40 KB, patch)
2013-04-07 02:12 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 8770 - amount does not equal amountoutstanding on a fine with no payments or credits (2.54 KB, patch)
2013-04-07 12:11 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Melia Meggs 2012-09-13 19:04:54 UTC
Fine amount is being correctly calculated.  The amountoutstanding does not agree with the amount, even when no payments or credits have been made.

This seems to affect lost items in particular.
Comment 1 Chris Cormack 2012-09-13 19:11:29 UTC
This is definitely a regression. Since this has worked for the 12 years up to now. Whomever fixes it, (should be able to track it down with git bisect) should make sure to write tests for this behaviour
Comment 2 Melia Meggs 2012-09-13 21:02:34 UTC
Created attachment 12201 [details]
amount =! amountoutstanding screenshot1

Here are some screenshots, if it helps.
Comment 3 Melia Meggs 2012-09-13 21:03:10 UTC
Created attachment 12202 [details]
amount != amountoutstanding screenshot2
Comment 4 Melia Meggs 2012-09-13 21:04:01 UTC
Created attachment 12203 [details]
amount != amountoutstanding screenshot3
Comment 5 Chris Cormack 2012-09-13 21:04:46 UTC
Can you check the database, check the accountlines table, what type are those ones that don't match, 'F' ?
Comment 6 Melia Meggs 2012-09-13 21:09:14 UTC
I can't check that particular one because I can't make their silly VPN work, but I have another example from a different library of the same thing.  This one is also related to a lost item that was later returned (I think?).  The one with the wrong amountoutstanding is accounttype F.  It looks like this:

mysql> SELECT * FROM accountlines WHERE borrowernumber = 323737\G;
*************************** 1. row ***************************
   borrowernumber: 323737
        accountno: 2
       itemnumber: 700451
             date: 2012-08-22
           amount: 3.950000
      description: Froggy goes to Hawaii / 05/22/2012 23:59
          dispute: NULL
      accounttype: F
amountoutstanding: 2.950000
    lastincrement: 0.050000
        timestamp: 2012-08-22 16:42:19
        notify_id: 0
     notify_level: 0
             note: NULL
       manager_id: NULL
*************************** 2. row ***************************
   borrowernumber: 323737
        accountno: 3
       itemnumber: 700451
             date: 2012-07-30
           amount: 15.990000
      description: Lost Item Froggy goes to Hawaii / 31319001350302  
          dispute: NULL
      accounttype: LR
amountoutstanding: 0.000000
    lastincrement: NULL
        timestamp: 2012-08-22 16:42:19
        notify_id: 0
     notify_level: 0
             note: NULL
       manager_id: NULL
*************************** 3. row ***************************
   borrowernumber: 323737
        accountno: 4
       itemnumber: NULL
             date: 2012-08-22
           amount: -15.990000
      description: Item Returned 31319001350302
          dispute: NULL
      accounttype: CR
amountoutstanding: 0.000000
    lastincrement: NULL
        timestamp: 2012-08-22 16:42:19
        notify_id: 0
     notify_level: 0
             note: NULL
       manager_id: NULL
3 rows in set (0.00 sec)
Comment 7 Jesse Maseto 2012-09-25 22:11:17 UTC Comment hidden (obsolete)
Comment 8 Jesse Maseto 2012-09-25 22:13:19 UTC Comment hidden (obsolete)
Comment 9 Galen Charlton 2012-09-26 13:52:46 UTC
The content of attachment 12508 [details] has been deleted by
    Galen Charlton <gmcharlt@gmail.com>
without providing any reason.

The token used to delete this attachment was generated at 2012-09-26 13:52:42 UTC.
Comment 10 Galen Charlton 2012-09-26 13:53:19 UTC
The content of attachment 12509 [details] has been deleted by
    Galen Charlton <gmcharlt@gmail.com>
who provided the following reason:

inadvertantly included real patron data

The token used to delete this attachment was generated at 2012-09-26 13:53:07 UTC.
Comment 11 Jesse Maseto 2012-09-26 14:05:49 UTC
Created attachment 12529 [details]
Fines 1
Comment 12 Jesse Maseto 2012-09-26 14:06:13 UTC
Created attachment 12530 [details]
Fines 2
Comment 13 Paul Poulain 2012-11-21 14:21:47 UTC
(In reply to comment #1)
> This is definitely a regression. Since this has worked for the 12 years up
> to now.
OK, interesting information.

Digging into changes in C4/Accounts:

* patch
5954631aeaf261a9bbbb44a7a7fcae225031eb24, 2012-10-03 17:58:56
just do:
-    $dbh->do(  $insert, undef, $borrowernumber, $nextaccntno, $amount,
+    $dbh->do(  $insert, undef, $borrowernumber, $nextaccntno, 0 - $amount,
=> can't be the origin of the problem 0-$amount populates amountoutstanding, if it was the culprit, we had X and -X, not a small difference.

* patch a8b168dc63df9d60e8e562f368f8bdbec520ffd8, 2012-09-15 07:31:58 is related to adding a primary key, nothing related to amountoutstanding

Patches before that have been pushed before 3.8, so should not be the culprit
Comment 14 Paul Poulain 2012-11-21 14:47:20 UTC
mmm... I'm a little bit confused:
The version is "3.8", it seems there are "real world" datas. Does it mean the problem exists on 3.8 ? I'm confused because chris said it worked "for the 12 years up to now". So, is it broken in master/3.10 or in 3.8 ?

(continuing investigating patches)
Comment 15 Melia Meggs 2012-11-21 20:42:31 UTC
Yes, these are real world dates.  The problem exists in 3.8.  We've got four different libraries on 3.8 that have all reported this strange problem.

I'm not sure if this is happening in master because I'm not really sure how it got like that in the first place...  I will go play around a bit on master and see if I can somehow reproduce it.
Comment 16 Kyle M Hall 2013-04-01 18:50:38 UTC
Looks like the problem code is in C4::Circulation around line 2142 in _FixAccountForLostAndReturned.

I returned lost item is only refunded the unpaid amount of the lost fee.
Comment 17 Kyle M Hall 2013-04-03 17:19:33 UTC Comment hidden (obsolete)
Comment 18 Chris Cormack 2013-04-07 02:12:01 UTC Comment hidden (obsolete)
Comment 19 Katrin Fischer 2013-04-07 12:11:48 UTC
Created attachment 17253 [details] [review]
[PASSED QA] Bug 8770 - amount does not equal amountoutstanding on a fine with no payments or credits

If a lost item fee is owed and partially paid off when an item is
returned and a refund is processed, Koha tries to pay off existing
fees before adding any leftover balance as a credit on the account.

However, those fee payments aren't actually processed, due to a bug
where the accountnumber was quoted as a string literal, and thus the
for the fee payment would fail. This did not result in an DB error,
as the query was still valid SQL. Checking the return value of the
query would have revealed that the accountline had not been updated.

History:
This bug was introduced on April 23, 2007 with the commit
'reintroducing fixaccountforlostandreturned as requested by rosalie'.
Commit id 111d590e9cdaf801256822e23f170bfc2365338f

On July 30, 2009 the error was flagged with a FIXME and remained
in that state until now.
Commit id 51e8fc2cb6a0cf79f19206c822a4361aeca6f63f

Test plan:
1) Create a test patron
2) Check out an item to that patron and give it a due date in the past
3) Run fines.pl to generate the fine for the item
4) Mark the item long overdue on the item tab (not in edit items)
5) Pay for the lost item
6) Check the item back in
7) Note the incorrect fines. Only the lost fee balance  due is refunded,
   not the entire lost fee, but no other fees are paid off.
8) Apply the patch
9) Repeat steps 1-6, then not the fines are paid correctly

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Totel due is correct after applying the patch.
All tests and QA script pass.
Comment 20 Galen Charlton 2013-04-08 03:18:12 UTC
Pushed to master; recommended for inclusion in 3.12.
Comment 21 Jared Camins-Esakov 2013-04-08 12:01:35 UTC
This patch has been pushed to 3.12.x
Comment 22 Chris Cormack 2013-04-25 06:07:10 UTC
Pushed to 3.10.x will be in 3.10.6