Bug 13098

Summary: Item lost multiple times by the same patron will create only be charged once
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Fines and feesAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: alexbuckley, catalog, george, gmcharlt, jonathan.druart+koha, kelly, kyle.m.hall, martin.renvoize, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6427
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 21584, 21607    
Bug Blocks: 21673, 21694, 21759    
Attachments: Bug 13098: Tests for _FixAccountForLostAndReturned on writeoff
Bug 13098: Tests for _FixAccountForLostAndReturned on writeoff
Bug 13098: Add Koha::Account::Offsets->total
Bug 13098: Tests for all _FixAccountForLostAndReturned use cases
Bug 13098: Make _FixAccountForLostAndReturned refund the right thing
Bug 13098: Add Koha::Account::Offsets->total
Bug 13098: Tests for all _FixAccountForLostAndReturned use cases
Bug 13098: Make _FixAccountForLostAndReturned refund the right thing
Bug 13098: Do not compare with precision
Bug 13098: Sum at DBMS level
Bug 13098: Fix POD for Koha::Account::Offsets::total
Bug 13098: Add Koha::Account::Offsets->total
Bug 13098: Tests for all _FixAccountForLostAndReturned use cases
Bug 13098: Make _FixAccountForLostAndReturned refund the right thing
Bug 13098: Do not compare with precision
Bug 13098: Sum at DBMS level
Bug 13098: Fix POD for Koha::Account::Offsets::total

Description Kyle M Hall 2014-10-16 14:53:29 UTC
If a patron loses an item and is charged a lost fee, returns the item, and loses it again, the patron will not be charged a new lost fee!

Steps to reproduce:
1) Check out item with a replacement fee to a patron
2) Mark the item as lost
3) Note the lost fee, write it off
4) Return the item
5) Check out item to the patron a second time
6) Mark the item as lost a second time
7) Note there is no second lost item fee
Comment 1 Kyle M Hall 2015-07-02 10:15:56 UTC
I believe bug 6427 will resolve this bug.
Comment 2 Tomás Cohen Arazi 2018-10-12 14:20:48 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2018-10-16 19:20:52 UTC
Created attachment 80678 [details] [review]
Bug 13098: Tests for _FixAccountForLostAndReturned on writeoff

When a a write off is done on lost fees, but then the item is returned,
the current behaviour is that a credit is added to the patron, for the
amount the original lost fee was. This behaviour is wrong and this tests
reproduce the situation.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests fail because a credit (CR) is added for the original
amount, without taking into account the fact the amount was not payed,
but written off
Comment 4 Tomás Cohen Arazi 2018-10-16 19:29:59 UTC
Ok, this is the more complete tests I came up with.

Use cases:
- PF + L + Full write off
- PF + L + Full payment
- PF + L + No write off or payment
- PF + L + Partial write off, partial payment, some remaining debt (L, amountoutstanding)

The idea is that _FixAccountForLostAndFound should:
- Consider write offs as good enough (so no CR for writeoffs)
- Generate a single CR for all payments that reduced amountoutstanding for L
- Write offs should be identified by looking at the account_offsets table, specifically when credit_id IS NOT NULL, debit_id is the L accountlines_id and type <> 'Writeoff'.
Comment 5 Tomás Cohen Arazi 2018-10-18 16:23:57 UTC
Created attachment 80862 [details] [review]
Bug 13098: Add Koha::Account::Offsets->total

This patch introduces this trivial method for returning the sum0 result
of the amount columns of a Koha::Account::Offsets set.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account/Offsets.t
- Sign off :-D
Comment 6 Tomás Cohen Arazi 2018-10-18 16:24:02 UTC
Created attachment 80863 [details] [review]
Bug 13098: Tests for all _FixAccountForLostAndReturned use cases

This patch introduces tests for all the use cases for
_FixAccountForLostAndReturned. This includes the following scenarios
before _FixAccountForLostAndReturned is called:

- Full write off (the original issue)
- Full payment
- No payment or write off
- Partial payment and partial write off

Written off debts (L lines) are just skipped, any kind of payment is
added and then refund as a single credit (CR). Amount outstanding is set
to 0. Offsets need to be generated for paper trail of this decisions.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests fail because _FixAccountForLostAndReturned is not doing
the right thing :-D
Comment 7 Tomás Cohen Arazi 2018-10-18 16:24:07 UTC
Created attachment 80864 [details] [review]
Bug 13098: Make _FixAccountForLostAndReturned refund the right thing

This patch changes the logic inside the method, to make it match the
behaviour described on the tests.

It uses the existing offsets on the account_offsets table to gather
information about the right things to refund.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests don't pass!
- Apply this patch
- Run
 k$ prove t/db_dependent/Circulation.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 8 Alex Buckley 2018-10-25 03:53:09 UTC
Created attachment 81105 [details] [review]
Bug 13098: Add Koha::Account::Offsets->total

This patch introduces this trivial method for returning the sum0 result
of the amount columns of a Koha::Account::Offsets set.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account/Offsets.t
- Sign off :-D

Followed test plan, patch worked as described
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 9 Alex Buckley 2018-10-25 03:53:35 UTC
Created attachment 81106 [details] [review]
Bug 13098: Tests for all _FixAccountForLostAndReturned use cases

This patch introduces tests for all the use cases for
_FixAccountForLostAndReturned. This includes the following scenarios
before _FixAccountForLostAndReturned is called:

- Full write off (the original issue)
- Full payment
- No payment or write off
- Partial payment and partial write off

Written off debts (L lines) are just skipped, any kind of payment is
added and then refund as a single credit (CR). Amount outstanding is set
to 0. Offsets need to be generated for paper trail of this decisions.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests fail because _FixAccountForLostAndReturned is not doing
the right thing :-D

Followed test plan, patch works as described
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 10 Alex Buckley 2018-10-25 03:54:12 UTC
Created attachment 81107 [details] [review]
Bug 13098: Make _FixAccountForLostAndReturned refund the right thing

This patch changes the logic inside the method, to make it match the
behaviour described on the tests.

It uses the existing offsets on the account_offsets table to gather
information about the right things to refund.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests don't pass!
- Apply this patch
- Run
 k$ prove t/db_dependent/Circulation.t
=> SUCCESS: Tests pass!
- Sign off :-D

Followed test plan, patch works as described. All three patches pass QA
test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 11 Jonathan Druart 2018-10-25 19:17:08 UTC
1.
+=head3 total
+
+=cut

=> You are cheating, please provide POD.

2. I would suggest to use the same pattern as Koha::Account->balance for Koha::Account::Offsets->total, i.e. sum at DBMS level.

3. In tests: $price + 0.00
=> I do not understand the trick, seems useless.

I will need more free brain to finish here.
Comment 12 Jonathan Druart 2018-10-25 19:17:31 UTC
Created attachment 81194 [details] [review]
Bug 13098: Do not compare with precision

Formating will be done TT side
Comment 13 Jonathan Druart 2018-10-25 19:17:37 UTC
Created attachment 81195 [details] [review]
Bug 13098: Sum at DBMS level
Comment 14 Tomás Cohen Arazi 2018-10-27 11:36:02 UTC
Created attachment 81354 [details] [review]
Bug 13098: Fix POD for Koha::Account::Offsets::total

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Josef Moravec 2018-10-29 20:30:39 UTC
Created attachment 81538 [details] [review]
Bug 13098: Add Koha::Account::Offsets->total

This patch introduces this trivial method for returning the sum0 result
of the amount columns of a Koha::Account::Offsets set.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account/Offsets.t
- Sign off :-D

Followed test plan, patch worked as described
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 16 Josef Moravec 2018-10-29 20:30:44 UTC
Created attachment 81539 [details] [review]
Bug 13098: Tests for all _FixAccountForLostAndReturned use cases

This patch introduces tests for all the use cases for
_FixAccountForLostAndReturned. This includes the following scenarios
before _FixAccountForLostAndReturned is called:

- Full write off (the original issue)
- Full payment
- No payment or write off
- Partial payment and partial write off

Written off debts (L lines) are just skipped, any kind of payment is
added and then refund as a single credit (CR). Amount outstanding is set
to 0. Offsets need to be generated for paper trail of this decisions.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests fail because _FixAccountForLostAndReturned is not doing
the right thing :-D

Followed test plan, patch works as described
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 17 Josef Moravec 2018-10-29 20:30:49 UTC
Created attachment 81540 [details] [review]
Bug 13098: Make _FixAccountForLostAndReturned refund the right thing

This patch changes the logic inside the method, to make it match the
behaviour described on the tests.

It uses the existing offsets on the account_offsets table to gather
information about the right things to refund.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation.t
=> FAIL: Tests don't pass!
- Apply this patch
- Run
 k$ prove t/db_dependent/Circulation.t
=> SUCCESS: Tests pass!
- Sign off :-D

Followed test plan, patch works as described. All three patches pass QA
test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 18 Josef Moravec 2018-10-29 20:30:54 UTC
Created attachment 81541 [details] [review]
Bug 13098: Do not compare with precision

Formating will be done TT side

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 19 Josef Moravec 2018-10-29 20:31:00 UTC
Created attachment 81542 [details] [review]
Bug 13098: Sum at DBMS level

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 20 Josef Moravec 2018-10-29 20:31:07 UTC
Created attachment 81543 [details] [review]
Bug 13098: Fix POD for Koha::Account::Offsets::total

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 21 Nick Clemens 2018-10-31 15:02:41 UTC
Awesome work all!

Pushed to master for 18.11
Comment 22 Martin Renvoize 2018-11-08 16:01:20 UTC
Dependencies not in 18.05.x.