[Bug 20946] New: Cannot pay fines for patrons with credits
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Bug ID: 20946 Summary: Cannot pay fines for patrons with credits Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Fines and fees Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 76116 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76116&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|kyle@bywatersolutions.com |tomascohen@gmail.com CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Pierre-Luc Lapointe <pierre-luc.lapointe@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pierre-luc.lapointe@inLibro | |.com --- Comment #2 from Pierre-Luc Lapointe <pierre-luc.lapointe@inLibro.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #76116|0 |1 is obsolete| | --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 76317 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76317&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 76318 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76318&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |20978 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20978 [Bug 20978] Add Koha::Account::add_credit -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |20942 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20942 [Bug 20942] Add route to get patron's account balance -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #76317|0 |1 is obsolete| | Attachment #76318|0 |1 is obsolete| | --- Comment #6 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 76394 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76394&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 --- Comment #7 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 76395 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76395&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #76394|0 |1 is obsolete| | Attachment #76395|0 |1 is obsolete| | --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 76432 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76432&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 76433 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76433&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 76434 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76434&action=edit Bug 20946: Reduce number of db calls Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #76434|0 |1 is obsolete| | --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 76435 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76435&action=edit Bug 20946: Reduce number of db calls Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 76437 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76437&action=edit Bug 20946: Additional unit tests for 0 lines edge case -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|Trivial patch |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 --- Comment #13 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 76519 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76519&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 76521 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76521&action=edit Bug 20946: Avoid summing the list twice -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #76519|0 |1 is obsolete| | Attachment #76521|0 |1 is obsolete| | --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 76522 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76522&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 --- Comment #16 from Nick Clemens <nick@bywatersolutions.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |In Discussion -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 --- Comment #18 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Failed QA --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 --- Comment #20 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 76632 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76632&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #21 from Nick Clemens <nick@bywatersolutions.com> --- Awesome work all! Pushed to master for 18.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|Pushed to Master |Pushed to Stable --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Pushed to 18.05.x for 18.05.02 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #23 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Stable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |21909 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21909 [Bug 21909] Koha::Account::outstanding_* methods should preserve call context -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Charles Farmer <charles.farmer@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |charles.farmer@inlibro.com --- Comment #24 from Charles Farmer <charles.farmer@inlibro.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #25 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Charles, can you detail the difference (maybe with step by steps?) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20946 amanda@koalan.com.br <amanda@koalan.com.br> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |amanda@koalan.com.br -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org