Bug 22750

Summary: Partial writeoff of lost fee causes item to be returned
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: Fines and feesAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: andrewfh, jonathan.druart, kelly, martin.renvoize, nick
Version: 19.11   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24474
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 22750: (24592: Database Update)
Bug 22750: (24592: Reword LOST_RETURN to LOST_FOUND)
Bug 22750: (24474: Add tests)
Bug 22750: (24474: Add `onpayment` option to `MarkLostItemsReturned`)
Bug 22750: (24474: Filter out LOST_FOUND)
Bug 22750: (24474: (follow-up) Add onpayment to sysprefs file)
Bug 22750: (24474: (follow-up) Add LOST_FOUND logic to Koha::Account::pay)
Bug 22750: (24474: (QA follow-up) Fix failing test)
Bug 22750: (24592: Database Update)
Bug 22750: (24592: Reword LOST_RETURN to LOST_FOUND)
Bug 22750: (24474: Add tests)
Bug 22750: (24474: Add `onpayment` option to `MarkLostItemsReturned`)
Bug 22750: (24474: Filter out LOST_FOUND)
Bug 22750: (24474: (follow-up) Add onpayment to sysprefs file)
Bug 22750: (24474: (follow-up) Add LOST_FOUND logic to Koha::Account::pay)
Bug 22750: (24474: (QA follow-up) Fix failing test)

Description Andrew Fuerste-Henry 2019-04-22 17:11:00 UTC
To test:
- Set system preference MarkLostItemAsReturned to not mark items as returned
- Set system preference WhenLostChargeReplacementFee to charge
- Find an item with a non-zero replacement cost
- Check item out to patron
- Mark item as lost
- Confirm item is still in patron checkouts and patron has a lost fee for item
- Write off part of the lost fee
- Confirm the item no longer shows in patron checkouts
Comment 1 Andrew Fuerste-Henry 2019-04-22 17:11:32 UTC
Confirmed in 18.05 and 18.11
Comment 2 Andrew Fuerste-Henry 2019-04-22 17:15:51 UTC
In case it's relevant, I performed the partial writeoff using the "Write Off" button for the specific fee.
Comment 3 Kelly McElligott 2020-07-28 14:47:11 UTC
This is still the case in 19.11.  However I attempted to use Write Off selected and it still was removed from the patron's checkouts.
Comment 4 Jonathan Druart 2020-08-10 15:27:54 UTC
I think this has been fixed in 20.05 by bug 24474.
Comment 5 Martin Renvoize 2020-09-08 09:18:30 UTC
I'm going to take a look at providing a backport version for 24474 for the 19.11 branch. I've investigate if that will allow backports further at the same time, but I have a feeling too much of the foundations may have changed to make that possible.

Bear with me.
Comment 6 Martin Renvoize 2020-10-06 09:53:33 UTC
Created attachment 111271 [details] [review]
Bug 22750: (24592: Database Update)

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 7 Martin Renvoize 2020-10-06 09:53:38 UTC
Created attachment 111272 [details] [review]
Bug 22750: (24592: Reword LOST_RETURN to LOST_FOUND)

This patch updates the wording in the 'lost and found' process to more
closely reflect what the process is achieving by replacing 'RETURNED'
with 'FOUND'

Test plan:
1) Grep codebase for _FixAccountForLostAndReturned and note there are no
   longer any instanced of it.
2) Run t/db_dependent/Circulation.t and note it passes
3) Test returning/renewing an item that has been marked as lost and note
   the updated values in the accountlines now use LOST_FOUND as
   credit_type_code and 'FOUND' as the status for the 'LOST' fee
   (debit_type_code 'LOST')

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 8 Martin Renvoize 2020-10-06 09:53:42 UTC
Created attachment 111273 [details] [review]
Bug 22750: (24474: Add tests)

This does the following using the interface:
- Unselect all the options from MarkLostItemsAsReturned except for
  'onpayment'
- Check an item out
- Create a manual invoice LOST for the checked in item
- Renew the item
- Check that the item was not returned

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Martin Renvoize 2020-10-06 09:53:45 UTC
Created attachment 111274 [details] [review]
Bug 22750: (24474: Add `onpayment` option to `MarkLostItemsReturned`)

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Martin Renvoize 2020-10-06 09:53:49 UTC
Created attachment 111275 [details] [review]
Bug 22750: (24474: Filter out LOST_FOUND)

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Martin Renvoize 2020-10-06 09:53:52 UTC
Created attachment 111276 [details] [review]
Bug 22750: (24474: (follow-up) Add onpayment to sysprefs file)

This patch adds the required line to the circulation.pref file to
dispaly the onpayment option in the MarkLostItemReturned preference

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Martin Renvoize 2020-10-06 09:53:58 UTC
Created attachment 111277 [details] [review]
Bug 22750: (24474: (follow-up) Add LOST_FOUND logic to Koha::Account::pay)

This patch adds the same updated logic as found in
Koha::Account::Line->apply to handle LOST_FOUND credits being applied
against their associated LOST debit.

Currently a 'LOST_FOUND' credit is never created via the pay method and
so the additional test will always pass, but the addition here accounts
for possible future uses.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Martin Renvoize 2020-10-06 09:54:04 UTC
Created attachment 111278 [details] [review]
Bug 22750: (24474: (QA follow-up) Fix failing test)

The test was assuming default data and would fail if you ran through the
test plan (and disabled all but `onpayment` for the
'MarkLostItemsAsReturned' preference) on the data before running the
included unit tests.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Martin Renvoize 2020-10-06 09:55:16 UTC
Backport rebase of the patches in question.. I think these deserve a QA run for the 19.11 branch, though the backport was fairly trivial in the end.  Setting to SO to provoke such.
Comment 15 Nick Clemens 2020-10-15 13:51:00 UTC
Created attachment 111738 [details] [review]
Bug 22750: (24592: Database Update)

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2020-10-15 13:51:10 UTC
Created attachment 111739 [details] [review]
Bug 22750: (24592: Reword LOST_RETURN to LOST_FOUND)

This patch updates the wording in the 'lost and found' process to more
closely reflect what the process is achieving by replacing 'RETURNED'
with 'FOUND'

Test plan:
1) Grep codebase for _FixAccountForLostAndReturned and note there are no
   longer any instanced of it.
2) Run t/db_dependent/Circulation.t and note it passes
3) Test returning/renewing an item that has been marked as lost and note
   the updated values in the accountlines now use LOST_FOUND as
   credit_type_code and 'FOUND' as the status for the 'LOST' fee
   (debit_type_code 'LOST')

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2020-10-15 13:51:15 UTC
Created attachment 111740 [details] [review]
Bug 22750: (24474: Add tests)

This does the following using the interface:
- Unselect all the options from MarkLostItemsAsReturned except for
  'onpayment'
- Check an item out
- Create a manual invoice LOST for the checked in item
- Renew the item
- Check that the item was not returned

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens 2020-10-15 13:51:19 UTC
Created attachment 111741 [details] [review]
Bug 22750: (24474: Add `onpayment` option to `MarkLostItemsReturned`)

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Nick Clemens 2020-10-15 13:51:24 UTC
Created attachment 111742 [details] [review]
Bug 22750: (24474: Filter out LOST_FOUND)

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Nick Clemens 2020-10-15 13:51:29 UTC
Created attachment 111743 [details] [review]
Bug 22750: (24474: (follow-up) Add onpayment to sysprefs file)

This patch adds the required line to the circulation.pref file to
dispaly the onpayment option in the MarkLostItemReturned preference

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Nick Clemens 2020-10-15 13:51:32 UTC
Created attachment 111744 [details] [review]
Bug 22750: (24474: (follow-up) Add LOST_FOUND logic to Koha::Account::pay)

This patch adds the same updated logic as found in
Koha::Account::Line->apply to handle LOST_FOUND credits being applied
against their associated LOST debit.

Currently a 'LOST_FOUND' credit is never created via the pay method and
so the additional test will always pass, but the addition here accounts
for possible future uses.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 22 Nick Clemens 2020-10-15 13:51:36 UTC
Created attachment 111745 [details] [review]
Bug 22750: (24474: (QA follow-up) Fix failing test)

The test was assuming default data and would fail if you ran through the
test plan (and disabled all but `onpayment` for the
'MarkLostItemsAsReturned' preference) on the data before running the
included unit tests.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Jonathan Druart 2020-10-22 07:46:06 UTC
Sending to 19.11 RMaints queue.