Bug 21969 - Koha::Account->outstanding_* should look for debits/credits by checking 'amount'
Summary: Koha::Account->outstanding_* should look for debits/credits by checking 'amount'
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Target Milestone: 19.05
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 21915
  Show dependency treegraph
 
Reported: 2018-12-06 18:11 UTC by Tomás Cohen Arazi
Modified: 2019-01-02 09:43 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 21969: Regression tests (6.12 KB, patch)
2018-12-06 19:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21969: Fix Koha::Account->outstanding_* matching the opposite on pathological cases (1.25 KB, patch)
2018-12-06 19:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21969: Regression tests (6.19 KB, patch)
2018-12-10 12:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21969: Fix Koha::Account->outstanding_* matching the opposite on pathological cases (1.31 KB, patch)
2018-12-10 12:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21969: Regression tests (6.26 KB, patch)
2018-12-11 14:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21969: Fix Koha::Account->outstanding_* matching the opposite on pathological cases (1.38 KB, patch)
2018-12-11 15:01 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 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 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 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 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 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 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 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 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 2018-12-11 21:00:08 UTC
Awesome work all!

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