Bug 20946 - Cannot pay fines for patrons with credits
Summary: Cannot pay fines for patrons with credits
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 20942 20978 21909
  Show dependency treegraph
 
Reported: 2018-06-14 17:18 UTC by Nick Clemens
Modified: 2020-01-06 20:15 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 20946: Remove credits from the 'Pay fines' tab in pay.pl (1.59 KB, patch)
2018-06-16 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20946: Add Koha::Account::outstanding_debits (4.15 KB, patch)
2018-06-23 08:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20946: Use K::Account->outstanding_debits in pay.pl and paycollect.pl (2.11 KB, patch)
2018-06-23 08:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20946: Add Koha::Account::outstanding_debits (4.20 KB, patch)
2018-06-25 18:32 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20946: Use K::Account->outstanding_debits in pay.pl and paycollect.pl (2.15 KB, patch)
2018-06-25 18:32 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20946: Add Koha::Account::outstanding_debits (4.29 KB, patch)
2018-06-26 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20946: Use K::Account->outstanding_debits in pay.pl and paycollect.pl (2.23 KB, patch)
2018-06-26 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20946: Reduce number of db calls (1.61 KB, patch)
2018-06-26 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20946: Reduce number of db calls (1.61 KB, patch)
2018-06-26 13:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20946: Additional unit tests for 0 lines edge case (1.11 KB, patch)
2018-06-26 13:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20946: (RM follow-up) Additional tests and fix addition to undef (3.01 KB, patch)
2018-06-27 14:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20946: Avoid summing the list twice (799 bytes, patch)
2018-06-27 14:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20946: (RM follow-up) Additional tests and fix addition to undef (3.25 KB, patch)
2018-06-27 15:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20946: (QA follow-up) make outstanding_debits return the account lines only (12.42 KB, patch)
2018-07-02 15:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-06-14 17:18:41 UTC
To recreate:
1 - Add a $5 credit to a patron
2 - Add a $1 fine
3 - Try to pay all fines
4 - You will be told to enter a value less than or equal to -4.00
5 - Observe you cannot do that
Comment 1 Tomás Cohen Arazi 2018-06-16 12:49:57 UTC Comment hidden (obsolete)
Comment 2 Pierre-Luc Lapointe 2018-06-22 20:18:54 UTC
I tested the patch and the credit is not displayed (like intended).
However, the credit is still considered in the balance, so it still shows "Total due: -4.00". When trying to pay all fines, the message "You must pay a value less than or equal to -4.00" still shows up and and the payment cannot be done.
Comment 3 Tomás Cohen Arazi 2018-06-23 08:49:26 UTC
Created attachment 76317 [details] [review]
Bug 20946: Add Koha::Account::outstanding_debits

This patch adds a handy method that returns the total for outstanding
debits (i.e. those that haven't been canceled with credits), and the
corresponding Koha::Account::Line objects.

Unit tests are added.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> SUCCESS: tests pass!
- Sign off :-D
Comment 4 Tomás Cohen Arazi 2018-06-23 08:49:33 UTC
Created attachment 76318 [details] [review]
Bug 20946: Use K::Account->outstanding_debits in pay.pl and paycollect.pl

This patch changes the how account lines are fetched (using
Koha::Account->outstanding_debits) so credits are not picked.

To test:
- Add a $5 credit to a patron
- Add a $1 fine
- Go to the 'Pay fines' tab
=> FAIL: Credit is displayed
- Try to pay all fines
=> FAIL: You are told to enter a value less than or equal to -4.00
         (Observe you cannot do that)
- Apply this patch
- Reload
=> SUCCESS: Credit is not displayed
=> SUCCESS: You are able to pay all fines
- Sign off :-D
Comment 5 Tomás Cohen Arazi 2018-06-23 08:50:40 UTC
(In reply to Pierre-Luc Lapointe from comment #2)
> I tested the patch and the credit is not displayed (like intended).
> However, the credit is still considered in the balance, so it still shows
> "Total due: -4.00". When trying to pay all fines, the message "You must pay
> a value less than or equal to -4.00" still shows up and and the payment
> cannot be done.

I took a shortcut and missed that. The latest patches implement it as I think it should be (until we use the API for this of course).
Comment 6 Josef Moravec 2018-06-25 18:32:47 UTC
Created attachment 76394 [details] [review]
Bug 20946: Add Koha::Account::outstanding_debits

This patch adds a handy method that returns the total for outstanding
debits (i.e. those that haven't been canceled with credits), and the
corresponding Koha::Account::Line objects.

Unit tests are added.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> SUCCESS: tests pass!
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 7 Josef Moravec 2018-06-25 18:32:53 UTC
Created attachment 76395 [details] [review]
Bug 20946: Use K::Account->outstanding_debits in pay.pl and paycollect.pl

This patch changes the how account lines are fetched (using
Koha::Account->outstanding_debits) so credits are not picked.

To test:
- Add a $5 credit to a patron
- Add a $1 fine
- Go to the 'Pay fines' tab
=> FAIL: Credit is displayed
- Try to pay all fines
=> FAIL: You are told to enter a value less than or equal to -4.00
         (Observe you cannot do that)
- Apply this patch
- Reload
=> SUCCESS: Credit is not displayed
=> SUCCESS: You are able to pay all fines
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 8 Kyle M Hall 2018-06-26 13:09:22 UTC
Created attachment 76432 [details] [review]
Bug 20946: Add Koha::Account::outstanding_debits

This patch adds a handy method that returns the total for outstanding
debits (i.e. those that haven't been canceled with credits), and the
corresponding Koha::Account::Line objects.

Unit tests are added.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> SUCCESS: tests pass!
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Kyle M Hall 2018-06-26 13:09:31 UTC
Created attachment 76433 [details] [review]
Bug 20946: Use K::Account->outstanding_debits in pay.pl and paycollect.pl

This patch changes the how account lines are fetched (using
Koha::Account->outstanding_debits) so credits are not picked.

To test:
- Add a $5 credit to a patron
- Add a $1 fine
- Go to the 'Pay fines' tab
=> FAIL: Credit is displayed
- Try to pay all fines
=> FAIL: You are told to enter a value less than or equal to -4.00
         (Observe you cannot do that)
- Apply this patch
- Reload
=> SUCCESS: Credit is not displayed
=> SUCCESS: You are able to pay all fines
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2018-06-26 13:09:35 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2018-06-26 13:14:03 UTC
Created attachment 76435 [details] [review]
Bug 20946: Reduce number of db calls

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Kyle M Hall 2018-06-26 13:21:18 UTC
Created attachment 76437 [details] [review]
Bug 20946: Additional unit tests for 0 lines edge case
Comment 13 Nick Clemens 2018-06-27 14:22:47 UTC
Created attachment 76519 [details] [review]
Bug 20946: (RM follow-up) Additional tests and fix addition to undef

Added a few tests with credit lines and found we got warnings, adjusted
the routine to avoid this
Comment 14 Kyle M Hall 2018-06-27 14:43:08 UTC
Created attachment 76521 [details] [review]
Bug 20946: Avoid summing the list twice
Comment 15 Tomás Cohen Arazi 2018-06-27 15:16:39 UTC
Created attachment 76522 [details] [review]
Bug 20946: (RM follow-up) Additional tests and fix addition to undef

Added a few tests with credit lines and found we got warnings, adjusted
the routine to avoid this

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Nick Clemens 2018-06-29 19:28:00 UTC
Question here: before this patch I can 'Write off all' to clear the credit too, after I have no way to clear the credit, shuld that functionality wait on another bug?
Comment 17 Jonathan Druart 2018-06-29 19:46:57 UTC
Quick code review:

1. I do not like "return ( $total, $lines );"
We are simplifying the call here, but maybe next ones will not need the total, or the line. It seems that we are recreating wrong patterns from C4.


2.
  Koha::Account->new( { patron_id => $borrowernumber } )->outstanding_debits;
should be
  $patron->account->outstanding_debits


3. ->balance is used from several other places, do we need to update them as well?
Comment 18 Tomás Cohen Arazi 2018-06-29 19:52:58 UTC
(In reply to Jonathan Druart from comment #17)
> Quick code review:
> 
> 1. I do not like "return ( $total, $lines );"
> We are simplifying the call here, but maybe next ones will not need the
> total, or the line. It seems that we are recreating wrong patterns from C4.

Do you prefer a Koha::Account::OutstandingCredits objecT?
or
Koha::Account->oustanding_debits_total + outstanding_debit_lines ?

> 2.
>   Koha::Account->new( { patron_id => $borrowernumber } )->outstanding_debits;
> should be
>   $patron->account->outstanding_debits

I agree. This is an easy fix, though.

> 3. ->balance is used from several other places, do we need to update them as
> well?

Balance is another thing. Balance is a global value representing if the patron owes or has credit. The point of having debits and credits separate is replicating what other tools do (see any bank account balance, for example). So in many places where ->balance is used, it is done in a correct way!
Comment 19 Jonathan Druart 2018-06-29 20:11:54 UTC
Pmed with Tomas, we agreed that outstanding_debits should return Koha::Account::Lines, then Koha::Account::Lines should have new methods to do the sum calculation.
It will make the sum reusable and outstanding_debits return only what it needs to return (a set of lines)
Comment 20 Tomás Cohen Arazi 2018-07-02 15:21:01 UTC
Created attachment 76632 [details] [review]
Bug 20946: (QA follow-up) make outstanding_debits return the account lines only

This patch was discussed with Jonathan on a QA conversation. It is
better to keep this simpler and more reusable. And is the right approach
in this case.

This patch makes Koha::Account::outstanding_debits return the account
lines, and a method is added to Koha::Account::Lines so the outstanding
amount is calculated on the resultset. This is done the dame way it was
done before, and the tests got adjusted.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Nick Clemens 2018-07-06 14:42:26 UTC
Awesome work all!

Pushed to master for 18.11
Comment 22 Martin Renvoize 2018-07-10 06:43:05 UTC
Pushed to 18.05.x for 18.05.02
Comment 23 Fridolin SOMERS 2018-07-12 06:58:28 UTC
Code in pay.pl has changed a lot with Bug 10021 that is not in 17.11.x.
I'm really not sure how to rebase.
Can you provide rebased patches ? Maybe squashed.
Comment 24 Charles Farmer 2019-01-25 16:56:07 UTC
We've received feedback from librarians who were confounded by this new, and different, behaviour of pay.pl in v18. I don't understand myself how we went from not being able to pay negative numbers, to an interface requiring more clicks to do what was possible previously, which was to ask from the patron the amount that would return his ->balance to 0.

I feel like the patch should've checked first what the balance was, and display outstanding_credits AND/OR debits depending on whether that number turned out positive or negative.

So, before opening a new bug report, is this something that was considered during development?
Comment 25 Katrin Fischer 2019-01-31 18:11:10 UTC
Hi Charles, can you detail the difference (maybe with step by steps?)