Bug 18580 - Account-pay does not apply credits / clear old fines as did recordpayment
Summary: Account-pay does not apply credits / clear old fines as did recordpayment
Status: RESOLVED DUPLICATE of bug 21915
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P1 - high major (vote)
Assignee: Colin Campbell
QA Contact: Testopia
URL:
Keywords:
Depends on: 15899
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-11 14:48 UTC by Colin Campbell
Modified: 2019-01-10 15:50 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
proposed patch (1.53 KB, patch)
2017-05-11 15:12 UTC, Colin Campbell
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2017-05-11 14:48:19 UTC
the recordpayment routine in C4::Accounts cleared old paid fines and applied credit amounts as part of its pay routine. The behaviour was implicit and not signalled or commented in the code. The pay routine in Koha::Account which is now called by paycollect.pl instead of the old routine does not leaving these on the users fine display although they owe 0. Staff cannot manually remove these.

For example a user has the following accountlines
+-----------+-------------+-------------------+
| amount    | accounttype | amountoutstanding |
+-----------+-------------+-------------------+
| -1.300000 | C           |         -1.300000 |
|  0.650000 | Res         |          0.650000 |
|  0.650000 | Res         |          0.650000 |
|  0.650000 | Res         |          0.650000 |
+-----------+-------------+-------------------+

after recordpayment has been called with an amount of 0.65 the lines are
+-----------+-------------+-------------------+
| amount    | accounttype | amountoutstanding |
+-----------+-------------+-------------------+
| -1.300000 | C           |          0.000000 |
|  0.650000 | Res         |          0.000000 |
|  0.650000 | Res         |          0.000000 |
|  0.650000 | Res         |          0.000000 |
| -0.650000 | Pay         |          0.000000 |
+-----------+-------------+-------------------+
However now calling Account->pay for the same amount results in 
+-----------+-------------+-------------------+
| amount    | accounttype | amountoutstanding |
+-----------+-------------+-------------------+
| -1.300000 | C           |         -1.300000 |
|  0.650000 | Res         |          0.000000 |
|  0.650000 | Res         |          0.650000 |
|  0.650000 | Res         |          0.650000 |
| -0.650000 | Pay         |          0.000000 |
+-----------+-------------+-------------------+

leaving the credit and two fines with an amountoutstanding although the user has nothing to pay (the amountoutstanding columns sum to 0) and leading to misleading displays.

The difference is driven by the line where @outstanding_fines is retrieved the selection is amountoutstanding > 0. In recordpayment it was amountoutstanding <> 0 so that negative amounts were included and processed as part of the payment, rather than ignored. The fact that this implicit behaviour was not commented made it easy to miss.
Comment 1 Colin Campbell 2017-05-11 15:12:40 UTC
Created attachment 63386 [details] [review]
proposed patch

This is a minimal patch which restores the previous behaviour which some operations relied on.

Its a bit of a code smell that this is handled as a side-effect of a payment, but as Accounts never explicitly handled credit amounts it would require a bit of a rethink to the Accounts api
Comment 2 Colin Campbell 2017-08-23 08:29:27 UTC
Upped priority as we are finding sites on upgrade to 16.11 and beyond repeatedly enocountering inability to pay off fines as a result of this
Comment 3 Katrin Fischer 2017-08-27 12:33:32 UTC
Hi Colin,
I think the behaviour is still a bit odd. Your patch works, but only if the fine was created first. I think sequence should not matter?

I have made sure to restart Plack after applying the patch:

Test plan:
1) Created to fines, 1.00 each
2) Created a credit, 1.00
3) Balance is at 1.00
4) Pay amount of 1.00
   1.00 is paid, Credit and other fine remain unchanged (amount <> 0)

1) Created a credit, 1.00
2) Created to fines, 1.00 each
3) Balance is at 1.00
4) Pay amount of 1.00
   All amount outstanding are set to 0.00. Good.

I appears it only works if the credit was created before the fines were. 
I used a fresh patron account for each test.
Comment 4 Colin Campbell 2017-08-29 11:54:26 UTC
(In reply to Katrin Fischer from comment #3)
> Hi Colin,
> I think the behaviour is still a bit odd. Your patch works, but only if the
> fine was created first. I think sequence should not matter?
> 
> I have made sure to restart Plack after applying the patch:
> 
> Test plan:
> 1) Created to fines, 1.00 each
> 2) Created a credit, 1.00
> 3) Balance is at 1.00
> 4) Pay amount of 1.00
>    1.00 is paid, Credit and other fine remain unchanged (amount <> 0)
> 
> 1) Created a credit, 1.00
> 2) Created to fines, 1.00 each
> 3) Balance is at 1.00
> 4) Pay amount of 1.00
>    All amount outstanding are set to 0.00. Good.
> 
> I appears it only works if the credit was created before the fines were. 
> I used a fresh patron account for each test.
I'll see if I can set up something to confirm what the old api did and see if I can see anywhere the new fails
Comment 5 Christopher Davis 2018-12-04 17:13:53 UTC
Hi Everyone,

My institution is seeing this behaviour several time per week since our upgrade to Koha v. 18.05.05.
Comment 6 Martin Renvoize 2018-12-12 08:40:33 UTC

*** This bug has been marked as a duplicate of bug 21915 ***