Bug 21759 - Avoid manually setting amountoutstanding in _FixAccountForLostAndReturned
Summary: Avoid manually setting amountoutstanding in _FixAccountForLostAndReturned
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 13098
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-02 18:42 UTC by Kyle M Hall
Modified: 2019-01-02 09:42 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
This patch results in a proper offset always being recorded for auditing purposes when a user is refunded after returning a previously lost item.
Version(s) released in:


Attachments
Bug 21759: Regression tests (4.86 KB, patch)
2018-12-10 19:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21759: Avoid manually setting amountoutstanding in _FixAccountForLostAndReturned (4.07 KB, patch)
2018-12-10 19:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21759: Regression tests (4.93 KB, patch)
2018-12-11 14:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21759: Avoid manually setting amountoutstanding in _FixAccountForLostAndReturned (4.14 KB, patch)
2018-12-11 14:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21759: Regression tests (4.93 KB, patch)
2018-12-11 14:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21759: Avoid manually setting amountoutstanding in _FixAccountForLostAndReturned (4.13 KB, patch)
2018-12-11 14:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21759: Regression tests (4.98 KB, patch)
2018-12-11 14:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21759: Avoid manually setting amountoutstanding in _FixAccountForLostAndReturned (4.19 KB, patch)
2018-12-11 14:23 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-11-02 18:42:12 UTC
If a lost item is stilled owed in full, the fee is reduced to 0 but no corresponding account offset is generated. All changes to accountlines should generate a corresponding account offset.
Comment 1 Tomás Cohen Arazi 2018-11-02 19:45:55 UTC
Just a guess: this could be implemented using Koha::Account::Line->adjust, provided we allowed negative increments and a suitable update type/offset type is defined.
Comment 2 Martin Renvoize 2018-11-04 07:38:32 UTC
I indeed left the amount in the adjust method open for just this sort of thing. A new offset type seems sensible 😃.
Comment 3 Martin Renvoize 2018-11-04 08:09:22 UTC
Though actually, thinking more about this I'm not sure I understand why we're editing the amount directly here at all.. surely it's just a case of adding a credit to offset the amount outstanding to zero as per other payments/waivers.. I don't know the history, perhaps someone could enlighten me?
Comment 4 Tomás Cohen Arazi 2018-11-04 13:15:16 UTC
(In reply to Martin Renvoize from comment #3)
> Though actually, thinking more about this I'm not sure I understand why
> we're editing the amount directly here at all.. surely it's just a case of
> adding a credit to offset the amount outstanding to zero as per other
> payments/waivers.. I don't know the history, perhaps someone could enlighten
> me?

I'm suspecting about adjust as well
Comment 5 Tomás Cohen Arazi 2018-12-09 19:50:56 UTC
Kyle, Martin and I agreed this will be implemented by making this add and apply a credit instead of manually setting amount to 0. The use of the changed accounttype is spread in many places and should remain untouched.
Comment 6 Tomás Cohen Arazi 2018-12-10 19:58:58 UTC
Created attachment 83029 [details] [review]
Bug 21759: Regression tests

This patch tests for a new behaviour in the _FixAccountForLostAndFound
method.

The method will now add the amountoutstanding value for the lost item
fee to the CR credit to be generated. This means that:
- If there's some remaining debt, the same amount  will be added to the
  CR credit and used to cancel that debt. The final amountoutstanding
  will be the same as before, but an offset will be generated as
  required.
- If the line was written off, the behaviour remains unchanged, so no
  offset.
- If the line was payed and/or written off in full only the payments are
  refund, preserving the current behaviour.

Only changes to the 'remaining debt' use cases on this tests are
expected.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests fail because the behaviour is not correct.

Note: some tests order changes are introduced to avoid calling
discard_changes twice
Comment 7 Tomás Cohen Arazi 2018-12-10 19:59:05 UTC
Created attachment 83030 [details] [review]
Bug 21759: Avoid manually setting amountoutstanding in _FixAccountForLostAndReturned

This patch changes the behaviour in the _FixAccountForLostAndFound
method.

The method will now add the amountoutstanding value for the lost item
fee to the CR credit to be generated. This means that:
- If there's some remaining debt, the same amount  will be added to the
  CR credit and used to cancel that debt. The final amountoutstanding
  will be the same as before, but an offset will be generated as
  required.
- If the line was written off, the behaviour remains unchanged, so no
  offset.
- If the line was payed and/or written off in full only the payments are
  refund, preserving the current behaviour.

To test:
- Apply the regression tests patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests fail because the behaviour is not correct
- Apply this patch
- Run:
 k$ prove t/db_dependent/Circulation.t
=> SUCCESS: Tests now pass!
- Sign off :-D
Comment 8 Kyle M Hall 2018-12-11 14:17:04 UTC
Created attachment 83038 [details] [review]
Bug 21759: Regression tests

This patch tests for a new behaviour in the _FixAccountForLostAndFound
method.

The method will now add the amountoutstanding value for the lost item
fee to the CR credit to be generated. This means that:
- If there's some remaining debt, the same amount  will be added to the
  CR credit and used to cancel that debt. The final amountoutstanding
  will be the same as before, but an offset will be generated as
  required.
- If the line was written off, the behaviour remains unchanged, so no
  offset.
- If the line was payed and/or written off in full only the payments are
  refund, preserving the current behaviour.

Only changes to the 'remaining debt' use cases on this tests are
expected.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests fail because the behaviour is not correct.

Note: some tests order changes are introduced to avoid calling
discard_changes twice

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Kyle M Hall 2018-12-11 14:17:48 UTC
Created attachment 83039 [details] [review]
Bug 21759: Avoid manually setting amountoutstanding in _FixAccountForLostAndReturned

This patch changes the behaviour in the _FixAccountForLostAndFound
method.

The method will now add the amountoutstanding value for the lost item
fee to the CR credit to be generated. This means that:
- If there's some remaining debt, the same amount  will be added to the
  CR credit and used to cancel that debt. The final amountoutstanding
  will be the same as before, but an offset will be generated as
  required.
- If the line was written off, the behaviour remains unchanged, so no
  offset.
- If the line was payed and/or written off in full only the payments are
  refund, preserving the current behaviour.

To test:
- Apply the regression tests patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests fail because the behaviour is not correct
- Apply this patch
- Run:
 k$ prove t/db_dependent/Circulation.t
=> SUCCESS: Tests now pass!
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Martin Renvoize 2018-12-11 14:20:20 UTC
Created attachment 83040 [details] [review]
Bug 21759: Regression tests

This patch tests for a new behaviour in the _FixAccountForLostAndFound
method.

The method will now add the amountoutstanding value for the lost item
fee to the CR credit to be generated. This means that:
- If there's some remaining debt, the same amount  will be added to the
  CR credit and used to cancel that debt. The final amountoutstanding
  will be the same as before, but an offset will be generated as
  required.
- If the line was written off, the behaviour remains unchanged, so no
  offset.
- If the line was payed and/or written off in full only the payments are
  refund, preserving the current behaviour.

Only changes to the 'remaining debt' use cases on this tests are
expected.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests fail because the behaviour is not correct.

Note: some tests order changes are introduced to avoid calling
discard_changes twice

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2018-12-11 14:20:24 UTC
Created attachment 83041 [details] [review]
Bug 21759: Avoid manually setting amountoutstanding in _FixAccountForLostAndReturned

This patch changes the behaviour in the _FixAccountForLostAndFound
method.

The method will now add the amountoutstanding value for the lost item
fee to the CR credit to be generated. This means that:
- If there's some remaining debt, the same amount  will be added to the
  CR credit and used to cancel that debt. The final amountoutstanding
  will be the same as before, but an offset will be generated as
  required.
- If the line was written off, the behaviour remains unchanged, so no
  offset.
- If the line was payed and/or written off in full only the payments are
  refund, preserving the current behaviour.

To test:
- Apply the regression tests patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests fail because the behaviour is not correct
- Apply this patch
- Run:
 k$ prove t/db_dependent/Circulation.t
=> SUCCESS: Tests now pass!
- Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2018-12-11 14:23:42 UTC
Created attachment 83042 [details] [review]
Bug 21759: Regression tests

This patch tests for a new behaviour in the _FixAccountForLostAndFound
method.

The method will now add the amountoutstanding value for the lost item
fee to the CR credit to be generated. This means that:
- If there's some remaining debt, the same amount  will be added to the
  CR credit and used to cancel that debt. The final amountoutstanding
  will be the same as before, but an offset will be generated as
  required.
- If the line was written off, the behaviour remains unchanged, so no
  offset.
- If the line was payed and/or written off in full only the payments are
  refund, preserving the current behaviour.

Only changes to the 'remaining debt' use cases on this tests are
expected.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests fail because the behaviour is not correct.

Note: some tests order changes are introduced to avoid calling
discard_changes twice

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2018-12-11 14:23:46 UTC
Created attachment 83043 [details] [review]
Bug 21759: Avoid manually setting amountoutstanding in _FixAccountForLostAndReturned

This patch changes the behaviour in the _FixAccountForLostAndFound
method.

The method will now add the amountoutstanding value for the lost item
fee to the CR credit to be generated. This means that:
- If there's some remaining debt, the same amount  will be added to the
  CR credit and used to cancel that debt. The final amountoutstanding
  will be the same as before, but an offset will be generated as
  required.
- If the line was written off, the behaviour remains unchanged, so no
  offset.
- If the line was payed and/or written off in full only the payments are
  refund, preserving the current behaviour.

To test:
- Apply the regression tests patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests fail because the behaviour is not correct
- Apply this patch
- Run:
 k$ prove t/db_dependent/Circulation.t
=> SUCCESS: Tests now pass!
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2018-12-11 14:31:14 UTC
Passing QA.. this works as expected and is a marked improvement for auditing.
Comment 15 Nick Clemens 2018-12-11 20:57:06 UTC
Awesome work all!

Pushed to master for 19.05
Comment 16 Martin Renvoize 2018-12-12 11:02:12 UTC
Pushed to 18.11.x for 18.11.01
Comment 17 Lucas Gass 2018-12-24 14:44:38 UTC
depends on 13098, won't backport to 18.05