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
Created attachment 76116 [details] [review] Bug 20946: Remove credits from the 'Pay fines' tab in pay.pl This patch changes the query for outstanding account lines so it doesn't pick credits, which don't belong to the 'Pay fines' tab, and actually break form validation for payments. 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
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.
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
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
(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).
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>
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>
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>
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>
Created attachment 76434 [details] [review] Bug 20946: Reduce number of db calls Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 76435 [details] [review] Bug 20946: Reduce number of db calls Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 76437 [details] [review] Bug 20946: Additional unit tests for 0 lines edge case
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
Created attachment 76521 [details] [review] Bug 20946: Avoid summing the list twice
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>
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?
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?
(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!
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)
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>
Awesome work all! Pushed to master for 18.11
Pushed to 18.05.x for 18.05.02
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.
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?
Hi Charles, can you detail the difference (maybe with step by steps?)