Bug 21969

Summary: Koha::Account->outstanding_* should look for debits/credits by checking 'amount'
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: clackman, fridolin.somers, jonathan.druart, kyle, lucas, martin.renvoize, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14591
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on:    
Bug Blocks: 21915    
Attachments: Bug 21969: Regression tests
Bug 21969: Fix Koha::Account->outstanding_* matching the opposite on pathological cases
Bug 21969: Regression tests
Bug 21969: Fix Koha::Account->outstanding_* matching the opposite on pathological cases
Bug 21969: Regression tests
Bug 21969: Fix Koha::Account->outstanding_* matching the opposite on pathological cases

Description Tomás Cohen Arazi (tcohen) 2018-12-06 18:11:54 UTC
The current implementation checks 'amountoutstanding' matches what's expected for debits (i.e. > 0) and credits (i.e. < 0). But there are pathological use cases in Koha (that we need to deal with), specially related to accruing fines and back dated returns (and so dropbox mode) that trigger false positives (specifically debits with amountoutstanding being negative, then meaning they are actually credits).
Comment 1 Tomás Cohen Arazi (tcohen) 2018-12-06 19:05:30 UTC
Created attachment 82939 [details] [review]
Bug 21969: Regression tests

This patch introduces regression tests for Koha::Account::outstanding_*
methods so they don't pick wrong lines when amountoutstanding matches
what we are looking for (i.e. negative for credits and positive for
debits).

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> FAIL: Tests fail because pathological account lines are wrongly
picked.
Comment 2 Tomás Cohen Arazi (tcohen) 2018-12-06 19:05:34 UTC
Created attachment 82940 [details] [review]
Bug 21969: Fix Koha::Account->outstanding_* matching the opposite on pathological cases

This patch makes outstanding_* methods be safe regarding pathological
account lines that get converted into another type because of the value
of amountoutstanding

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> FAIL: Tests fail because pathological account lines are wrongly
picked.
- Apply this patch
- Run:
 k$ prove t/db_dependent/Koha/Account.t
=>SUCCESS: All green!
Comment 3 Martin Renvoize (ashimema) 2018-12-10 12:15:54 UTC
Created attachment 83011 [details] [review]
Bug 21969: Regression tests

This patch introduces regression tests for Koha::Account::outstanding_*
methods so they don't pick wrong lines when amountoutstanding matches
what we are looking for (i.e. negative for credits and positive for
debits).

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> FAIL: Tests fail because pathological account lines are wrongly
picked.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize (ashimema) 2018-12-10 12:15:58 UTC
Created attachment 83012 [details] [review]
Bug 21969: Fix Koha::Account->outstanding_* matching the opposite on pathological cases

This patch makes outstanding_* methods be safe regarding pathological
account lines that get converted into another type because of the value
of amountoutstanding

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> FAIL: Tests fail because pathological account lines are wrongly
picked.
- Apply this patch
- Run:
 k$ prove t/db_dependent/Koha/Account.t
=>SUCCESS: All green!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize (ashimema) 2018-12-10 12:16:35 UTC
A very sensible change that makes these methods considerable more robust... Signing Off.
Comment 6 Kyle M Hall (khall) 2018-12-11 14:59:57 UTC
Created attachment 83047 [details] [review]
Bug 21969: Regression tests

This patch introduces regression tests for Koha::Account::outstanding_*
methods so they don't pick wrong lines when amountoutstanding matches
what we are looking for (i.e. negative for credits and positive for
debits).

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> FAIL: Tests fail because pathological account lines are wrongly
picked.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall (khall) 2018-12-11 15:01:10 UTC
Created attachment 83048 [details] [review]
Bug 21969: Fix Koha::Account->outstanding_* matching the opposite on pathological cases

This patch makes outstanding_* methods be safe regarding pathological
account lines that get converted into another type because of the value
of amountoutstanding

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> FAIL: Tests fail because pathological account lines are wrongly
picked.
- Apply this patch
- Run:
 k$ prove t/db_dependent/Koha/Account.t
=>SUCCESS: All green!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Nick Clemens (kidclamp) 2018-12-11 21:00:08 UTC
Awesome work all!

Pushed to master for 19.05
Comment 9 Martin Renvoize (ashimema) 2018-12-12 11:11:37 UTC
Pushed to 18.11.x for 18.11.01
Comment 10 Lucas Gass (lukeg) 2018-12-24 15:31:10 UTC
missing dependency for 18.05, won't backport