Bug 24474 - Lost items that are checked out are always returned, even when attempting to renew them
Summary: Lost items that are checked out are always returned, even when attempting to ...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Martin Renvoize
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 22982 24592
Blocks: 14825
  Show dependency treegraph
 
Reported: 2020-01-21 20:04 UTC by Aleisha Amohia
Modified: 2020-08-10 15:30 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24474: Add tests (2.81 KB, patch)
2020-02-04 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24474: A try (1.35 KB, patch)
2020-02-04 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24474: Drop `ReturnLostItem` from ::Account classes (2.64 KB, patch)
2020-02-05 09:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Add ReturnLostItem handling for SIP Payments (1.28 KB, patch)
2020-02-05 09:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Add ReturnLostItem handling for staff client payments (2.42 KB, patch)
2020-02-05 09:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Add ReturnLostItem handling for paypal payments (1.57 KB, patch)
2020-02-05 09:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Add `onpayment` option to `MarkLostItemsReturned` (3.09 KB, patch)
2020-02-05 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Add tests (2.64 KB, patch)
2020-02-05 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Add `onpayment` option to `MarkLostItemsReturned` (3.09 KB, patch)
2020-02-05 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Filter out LOST_FOUND (1.51 KB, patch)
2020-02-05 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Add tests (2.67 KB, patch)
2020-02-10 13:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Add `onpayment` option to `MarkLostItemsReturned` (3.09 KB, patch)
2020-02-10 13:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Filter out LOST_FOUND (1.51 KB, patch)
2020-02-10 13:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: (follow-up) Add onpayment to sysprefs file (1.19 KB, patch)
2020-02-10 13:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Add tests (2.73 KB, patch)
2020-02-10 16:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 24474: Add `onpayment` option to `MarkLostItemsReturned` (3.15 KB, patch)
2020-02-10 16:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 24474: Filter out LOST_FOUND (1.56 KB, patch)
2020-02-10 16:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 24474: (follow-up) Add onpayment to sysprefs file (1.25 KB, patch)
2020-02-10 16:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 24474: (follow-up) Add LOST_FOUND logic to Koha::Account::pay (4.02 KB, patch)
2020-02-14 10:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Add tests (2.72 KB, patch)
2020-03-06 10:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Add `onpayment` option to `MarkLostItemsReturned` (3.14 KB, patch)
2020-03-06 10:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Filter out LOST_FOUND (1.50 KB, patch)
2020-03-06 10:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: (follow-up) Add onpayment to sysprefs file (1.24 KB, patch)
2020-03-06 10:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: (follow-up) Add LOST_FOUND logic to Koha::Account::pay (3.92 KB, patch)
2020-03-06 10:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: (QA follow-up) Fix failing test (1.14 KB, patch)
2020-03-31 08:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24474: Add tests (2.77 KB, patch)
2020-03-31 09:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 24474: Add `onpayment` option to `MarkLostItemsReturned` (3.20 KB, patch)
2020-03-31 09:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 24474: Filter out LOST_FOUND (1.54 KB, patch)
2020-03-31 09:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 24474: (follow-up) Add onpayment to sysprefs file (1.30 KB, patch)
2020-03-31 09:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 24474: (follow-up) Add LOST_FOUND logic to Koha::Account::pay (3.97 KB, patch)
2020-03-31 09:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 24474: (QA follow-up) Fix failing test (1.19 KB, patch)
2020-03-31 09:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 24474: Add tests (2.84 KB, patch)
2020-03-31 13:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24474: Add tests (2.84 KB, patch)
2020-03-31 13:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24474: Add `onpayment` option to `MarkLostItemsReturned` (3.26 KB, patch)
2020-03-31 13:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24474: Filter out LOST_FOUND (1.61 KB, patch)
2020-03-31 13:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24474: (follow-up) Add onpayment to sysprefs file (1.36 KB, patch)
2020-03-31 13:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24474: (follow-up) Add LOST_FOUND logic to Koha::Account::pay (4.04 KB, patch)
2020-03-31 13:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24474: (QA follow-up) Fix failing test (1.26 KB, patch)
2020-03-31 13:57 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 Aleisha Amohia 2020-01-21 20:04:41 UTC
This was a bug introduced by Bug 22982.

"Hey all, this patch is causing problems for some of our libraries because it's been assumed that lost items with no attached charge should automatically be returned. 

This is not always the desired behaviour - we have one case where the library wants to keep lost items on a patron's record, and another case where a checked out lost item will be renewed, but the amount outstanding is zero, so the item gets returned.

These are edge cases, but the assumptions in this patch do not consider them, and are causing problems. Is there a workaround? Or perhaps a better way of doing this patch that can allow for the cases of 'renewing a lost item' or 'wanting to keep a lost item on the checkouts record'? A solution that avoids doing circulation actions in the accounts modules as this patch does would be ideal."
Comment 1 Martin Renvoize 2020-01-22 13:25:02 UTC
Are you intending to work on this Aleisha.. just checking before I look at it myself.
Comment 2 Martin Renvoize 2020-01-22 13:30:58 UTC
It feels like yet another value to add to the `MarkLostItemsAsReturned` system preference to me.
Comment 3 Katrin Fischer 2020-01-22 14:30:14 UTC
Hm, if a library doesn't charge a lost fee and somene renews an item marked lost - would it still return it? I think the renewal part is what worries me most here and I think we shoudl never return an item on renewal. On paying the fine off, I can see why someone would want it and that seems ok to be pref driven.
Comment 4 Aleisha Amohia 2020-01-22 19:47:00 UTC
(In reply to Martin Renvoize from comment #1)
> Are you intending to work on this Aleisha.. just checking before I look at
> it myself.

Thanks for checking Martin, feel free to work on this yourself.
Comment 5 Jonathan Druart 2020-02-04 14:17:05 UTC
Created attachment 98392 [details] [review]
Bug 24474: Add tests

This does the following using the interface:
- Unselect all the options from MarkLostItemsAsReturned
- Check an item out
- Create a manual invoice LOST for the checked in item
- Pay the fine (Make a payment> Pay > Confirm)
- Renew the item

=> Item is checked in (NOT EXPECTED)
Comment 6 Jonathan Druart 2020-02-04 14:17:08 UTC
Created attachment 98393 [details] [review]
Bug 24474: A try

This does not work
Comment 7 Jonathan Druart 2020-02-04 14:19:45 UTC
I gave a try to this, wrote the test then try to fix it.

But my understanding of the code leads me to abandon. I do not understand why there are 3 calls to ReturnLostItem:

Koha/Account.pm:            C4::Circulation::ReturnLostItem( $self->{patron_id}, $fine->itemnumber ) unless $checkout;
Koha/Account.pm:            C4::Circulation::ReturnLostItem( $self->{patron_id}, $fine->itemnumber ) unless $checkout;
Koha/Account/Line.pm:                C4::Circulation::ReturnLostItem( $self->borrowernumber, $debit->itemnumber );

That seems wrong to me.

So I guess my test will not cover all the cases. Hope it could help as a start anyway.
Comment 8 Martin Renvoize 2020-02-04 16:16:18 UTC
(In reply to Aleisha Amohia from comment #0)
> This was a bug introduced by Bug 22982.
> 
> "Hey all, this patch is causing problems for some of our libraries because
> it's been assumed that lost items with no attached charge should
> automatically be returned.

I don't understand what you mean by this.. the code in bug 22982 only applies to LOST charges.. I'm not sure I can see how items are being returned when no lost item charge is attached in the first place?

> This is not always the desired behaviour - we have one case where the
> library wants to keep lost items on a patron's record, and another case
> where a checked out lost item will be renewed, but the amount outstanding is
> zero, so the item gets returned.

So, this is where there are LOST charges being applied right?  Instead of automatically returning the item on lost charge payment we want to allow for configurable state changes instead.. or am I misunderstanding the problem/request?

> 
> These are edge cases, but the assumptions in this patch do not consider
> them, and are causing problems. Is there a workaround? Or perhaps a better
> way of doing this patch that can allow for the cases of 'renewing a lost
> item' or 'wanting to keep a lost item on the checkouts record'? A solution
> that avoids doing circulation actions in the accounts modules as this patch
> does would be ideal."
Comment 9 Martin Renvoize 2020-02-04 16:20:22 UTC
(In reply to Jonathan Druart from comment #7)
> I gave a try to this, wrote the test then try to fix it.
> 
> But my understanding of the code leads me to abandon. I do not understand
> why there are 3 calls to ReturnLostItem:
> 
> Koha/Account.pm:            C4::Circulation::ReturnLostItem(
> $self->{patron_id}, $fine->itemnumber ) unless $checkout;
> Koha/Account.pm:            C4::Circulation::ReturnLostItem(
> $self->{patron_id}, $fine->itemnumber ) unless $checkout;
> Koha/Account/Line.pm:                C4::Circulation::ReturnLostItem(
> $self->borrowernumber, $debit->itemnumber );
> 
> That seems wrong to me.
> 
> So I guess my test will not cover all the cases. Hope it could help as a
> start anyway.

Not sure I understand what you tried here Jonathan.. looking like you're simply adding a second check to see if the item is still checked out before returning it..

I think we need clarification on the requested behaviour before continuing here.
Comment 10 Katrin Fischer 2020-02-04 18:55:28 UTC
From what I understand:
- the LOST charge is applied, but the library decided to not auto-return the item (config wise)
- then renewing the item returns it

I'll have to give this a go in testing, but I think we need to verify and try stuff here. Would be great to get a step by step by Aleisha if possible.
Comment 11 Katrin Fischer 2020-02-04 18:56:44 UTC
.. or maybe they just don't charge a fee. (no lost charge). I really wish our lost handling options were less confusing and more in one spot.
Comment 12 Katrin Fischer 2020-02-04 18:59:12 UTC
WhenLostChargeReplacementFee
DefaultLongOverdueChargeValue 	
MarkLostItemsAsReturned 

Are the ones I am thinking of.
Comment 13 Chris Cormack 2020-02-04 20:00:34 UTC
Thats right, you have an item that is marked lost. But the library wants that to stay on the persons card. Not be removed, so it is still out to them.

Then if you renew that item,
* then that makes it no longer lost - correct
* removes the charge - correct
   - Now the code goes on, there is a paid charge for lost, I will return it  - not correct

IE when you renew a lost item, the code in the accounts now returns it, because it has a paid for charge.
I feel like having circulation actions inside the accounts module is a bad thing anyway, but certainly creates unexpected (and wrong) behaviour in this case
Comment 14 Martin Renvoize 2020-02-04 20:17:14 UTC
Thanks for the clarifications guys.. it's a real action at a distance issue I see.. hmm, I agree it would be best to strip out the behaviour from the accounts code and push it up a level into the controllers really.  

I'll have a go at that when I'm fresh tomorrow.. unless anyone else wants to give it a punt..
Comment 15 Jonathan Druart 2020-02-05 08:57:46 UTC
Comment on attachment 98393 [details] [review]
Bug 24474: A try

That really did not make sense! :)
Comment 16 Jonathan Druart 2020-02-05 08:59:23 UTC
(In reply to Chris Cormack from comment #13)
> Then if you renew that item,
> * then that makes it no longer lost - correct
> * removes the charge - correct
>    - Now the code goes on, there is a paid charge for lost, I will return it
> - not correct

Martin, I think these steps are what my tests are doing and that they could be use to prove that a fix is correct.
Comment 17 Martin Renvoize 2020-02-05 09:21:57 UTC
Created attachment 98436 [details] [review]
Bug 24474: Drop `ReturnLostItem` from ::Account classes

This patche removes the ReturnLostItem calls from Koha::Account->pay and
Koha::Account::Line->apply methods.
Comment 18 Martin Renvoize 2020-02-05 09:22:00 UTC
Created attachment 98437 [details] [review]
Bug 24474: Add ReturnLostItem handling for SIP Payments

This patch adds a call to ReturnLostItem for Lost Item payments taken
via SIP machines.
Comment 19 Martin Renvoize 2020-02-05 09:22:03 UTC
Created attachment 98438 [details] [review]
Bug 24474: Add ReturnLostItem handling for staff client payments

This patch adds a call to ReturnLostItem for Lost Item payments taken
via the staff client.
Comment 20 Martin Renvoize 2020-02-05 09:22:06 UTC
Created attachment 98439 [details] [review]
Bug 24474: Add ReturnLostItem handling for paypal payments

This patch adds a call to ReturnLostItem for Lost Item payments taken
via OPAC online payments through PayPal.
Comment 21 Martin Renvoize 2020-02-05 09:27:08 UTC
This is still a work in progress.. I need to add some further tests selenium tests as this is moving code out of tested objects and into the controllers where really it should belong.. I really wish our controller layer was lighter and more clearly tested.. paycollect, for example, is a real mess at this point and I'm sure must contain untested functionality.

I'll get writing tests and hopefully get this patchset through before swinging back around for a refactoring effort there.
Comment 22 Martin Renvoize 2020-02-05 12:25:48 UTC
OK.. having another go.. I don't like that I was moving the logic out of a well testing Object and into untested and hard to test messy controllers... bare with me.
Comment 23 Martin Renvoize 2020-02-05 12:28:34 UTC
Created attachment 98460 [details] [review]
Bug 24474: Add `onpayment` option to `MarkLostItemsReturned`
Comment 24 Martin Renvoize 2020-02-05 14:08:22 UTC
Created attachment 98485 [details] [review]
Bug 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
Comment 25 Martin Renvoize 2020-02-05 14:08:26 UTC
Created attachment 98486 [details] [review]
Bug 24474: Add `onpayment` option to `MarkLostItemsReturned`
Comment 26 Martin Renvoize 2020-02-05 14:08:29 UTC
Created attachment 98487 [details] [review]
Bug 24474: Filter out LOST_FOUND
Comment 27 Martin Renvoize 2020-02-05 14:09:26 UTC
Probably not QA ready, but certainly in good enough shape for some testing please.
Comment 28 David Nind 2020-02-08 09:35:06 UTC
For whatever reason I can't seem to get the 'onpayment' option to show up for MarkLostItemsAsReturned. (Applied patch, ran database update, restart_all).

For the tests I get this fail:
root@50d5e409ced0:koha(bz24474)$ prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 30/47 
        #   Failed test 'Should have a single 10 days overdue fine and lost charge'
        #   at t/db_dependent/Circulation.t line 2344.
        #          got: '27'
        #     expected: '17'
        # Looks like you failed 1 test of 3.

    #   Failed test 'bug 22877'
    #   at t/db_dependent/Circulation.t line 2347.
    # Looks like you failed 1 test of 5.
t/db_dependent/Circulation.t .. 31/47 
#   Failed test 'AddReturn | is_overdue'
#   at t/db_dependent/Circulation.t line 2347.
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`account_offsets`, CONSTRAINT `account_offsets_ibfk_t` FOREIGN KEY (`type`) REFERENCES `account_offset_types` (`type`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `account_offsets` ( `amount`, `credit_id`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0=-99, 1='25', 2='Lost Item Found'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`account_offsets`, CONSTRAINT `account_offsets_ibfk_t` FOREIGN KEY (`type`) REFERENCES `account_offset_types` (`type`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `account_offsets` ( `amount`, `credit_id`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0=-99, 1='28', 2='Lost Item Found'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`account_offsets`, CONSTRAINT `account_offsets_ibfk_t` FOREIGN KEY (`type`) REFERENCES `account_offset_types` (`type`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `account_offsets` ( `amount`, `credit_id`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0=-74, 1='33', 2='Lost Item Found'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`account_offsets`, CONSTRAINT `account_offsets_ibfk_t` FOREIGN KEY (`type`) REFERENCES `account_offset_types` (`type`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `account_offsets` ( `amount`, `credit_id`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0=-100, 1='37', 2='Lost Item Found'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
t/db_dependent/Circulation.t .. 45/47     # Looks like you planned 2 tests but ran 1.
t/db_dependent/Circulation.t .. 47/47 
#   Failed test 'Do not return on renewal (LOST charge)'
#   at t/db_dependent/Circulation.t line 3689.
# Looks like you failed 2 tests of 47.
t/db_dependent/Circulation.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/47 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 512 Tests: 47 Failed: 2)
  Failed tests:  31, 47
  Non-zero exit status: 2
Files=1, Tests=47, 29 wallclock secs ( 0.09 usr  0.00 sys + 23.87 cusr  2.77 csys = 26.73 CPU)
Result: FAIL
Comment 29 Martin Renvoize 2020-02-10 13:10:03 UTC
Created attachment 98645 [details] [review]
Bug 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
Comment 30 Martin Renvoize 2020-02-10 13:10:07 UTC
Created attachment 98646 [details] [review]
Bug 24474: Add `onpayment` option to `MarkLostItemsReturned`
Comment 31 Martin Renvoize 2020-02-10 13:10:10 UTC
Created attachment 98647 [details] [review]
Bug 24474: Filter out LOST_FOUND
Comment 32 Martin Renvoize 2020-02-10 13:10:15 UTC
Created attachment 98648 [details] [review]
Bug 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
Comment 33 Martin Renvoize 2020-02-10 14:01:18 UTC
(In reply to David Nind from comment #28)
> For whatever reason I can't seem to get the 'onpayment' option to show up
> for MarkLostItemsAsReturned. (Applied patch, ran database update,
> restart_all).

Doh! I missed a file when committing.. that should now be resolved.

The test failure we're actually in the dependancy bug.. I've resolved them there and rebased this patchset upon them now.

Cheers for testing :)
Comment 34 David Nind 2020-02-10 16:53:35 UTC
Created attachment 98673 [details] [review]
Bug 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>
Comment 35 David Nind 2020-02-10 16:53:38 UTC
Created attachment 98674 [details] [review]
Bug 24474: Add `onpayment` option to `MarkLostItemsReturned`

Signed-off-by: David Nind <david@davidnind.com>
Comment 36 David Nind 2020-02-10 16:53:42 UTC
Created attachment 98675 [details] [review]
Bug 24474: Filter out LOST_FOUND

Signed-off-by: David Nind <david@davidnind.com>
Comment 37 David Nind 2020-02-10 16:53:46 UTC
Created attachment 98676 [details] [review]
Bug 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>
Comment 38 David Nind 2020-02-10 16:57:43 UTC
Now works as outlined in the test plan!

I re-ran the tests after signing off and one test fails:

root@314245d82a59:koha(bz24474)$ prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 30/47 
        #   Failed test 'Should have a single 10 days overdue fine and lost charge'
        #   at t/db_dependent/Circulation.t line 2344.
        #          got: '27'
        #     expected: '17'
        # Looks like you failed 1 test of 3.

    #   Failed test 'bug 22877'
    #   at t/db_dependent/Circulation.t line 2347.
    # Looks like you failed 1 test of 5.
t/db_dependent/Circulation.t .. 31/47 
#   Failed test 'AddReturn | is_overdue'
#   at t/db_dependent/Circulation.t line 2347.
t/db_dependent/Circulation.t .. 47/47 # Looks like you failed 1 test of 47.
t/db_dependent/Circulation.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/47 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 256 Tests: 47 Failed: 1)
  Failed test:  31
  Non-zero exit status: 1
Files=1, Tests=47, 29 wallclock secs ( 0.08 usr  0.00 sys + 23.90 cusr  2.79 csys = 26.77 CPU)
Result: FAIL
Comment 39 Martin Renvoize 2020-02-11 09:27:40 UTC
Hmm, I can't seem to replicate that test failure.. lets see how it goes in the QA run.
Comment 40 Jonathan Druart 2020-02-14 09:51:37 UTC
Martin, what about the 2 occurrences in Koha::Account->pay?
Comment 41 Martin Renvoize 2020-02-14 10:18:52 UTC
(In reply to Jonathan Druart from comment #40)
> Martin, what about the 2 occurrences in Koha::Account->pay?

I think I traced it back and found that it's only actually occurring via a call to apply (i.e the call inside _FixAccountForLostAndFound), but it's probably not a bad idea to match the code in the two locations.

I'll add a followup.
Comment 42 Martin Renvoize 2020-02-14 10:50:22 UTC
Created attachment 98923 [details] [review]
Bug 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.
Comment 43 Martin Renvoize 2020-02-14 10:50:42 UTC
As promised.
Comment 44 Jonathan Druart 2020-02-14 11:07:45 UTC
Switching to Needs Signoff as I would like Aleisha to have a look at Martin's patches.

However regression tests are missing, but that does not block human testing :)
Comment 45 Martin Renvoize 2020-03-06 10:15:32 UTC
Created attachment 100229 [details] [review]
Bug 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>
Comment 46 Martin Renvoize 2020-03-06 10:15:35 UTC
Created attachment 100230 [details] [review]
Bug 24474: Add `onpayment` option to `MarkLostItemsReturned`

Signed-off-by: David Nind <david@davidnind.com>
Comment 47 Martin Renvoize 2020-03-06 10:15:39 UTC
Created attachment 100231 [details] [review]
Bug 24474: Filter out LOST_FOUND

Signed-off-by: David Nind <david@davidnind.com>
Comment 48 Martin Renvoize 2020-03-06 10:15:43 UTC
Created attachment 100232 [details] [review]
Bug 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>
Comment 49 Martin Renvoize 2020-03-06 10:15:46 UTC
Created attachment 100233 [details] [review]
Bug 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.
Comment 50 Martin Renvoize 2020-03-06 10:20:56 UTC
Rebased again.. please test this Aleisha
Comment 51 Martin Renvoize 2020-03-30 07:31:37 UTC
Another few weeks passed.. quickly thinking this isn't 'critical' at this point. :(
Comment 52 David Nind 2020-03-30 08:28:19 UTC
Everything works as per the test plan.

However, there is still a failed test as before. Is there a system preference or setting related to test 31? I haven't failed QA, but I haven't signed it off yet either.

root@8ca270a5fd09:koha(bz24474)$ prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 30/47 
        #   Failed test 'Should have a single 10 days overdue fine and lost charge'
        #   at t/db_dependent/Circulation.t line 2430.
        #          got: '27'
        #     expected: '17'
        # Looks like you failed 1 test of 3.

    #   Failed test 'bug 22877'
    #   at t/db_dependent/Circulation.t line 2433.
    # Looks like you failed 1 test of 5.
t/db_dependent/Circulation.t .. 31/47 
#   Failed test 'AddReturn | is_overdue'
#   at t/db_dependent/Circulation.t line 2433.
t/db_dependent/Circulation.t .. 47/47 # Looks like you failed 1 test of 47.
t/db_dependent/Circulation.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/47 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 256 Tests: 47 Failed: 1)
  Failed test:  31
  Non-zero exit status: 1
Files=1, Tests=47, 30 wallclock secs ( 0.09 usr  0.00 sys + 24.44 cusr  2.49 csys = 27.02 CPU)
Result: FAIL
Comment 53 Martin Renvoize 2020-03-30 12:26:08 UTC
How are you running the tests David?  They're all passing for me within koha-testing-docker.. it feels like perhaps you have some data in your database that the tests are not expecting..

I'll take a look at trying to improve them to prevent such failures.
Comment 54 Martin Renvoize 2020-03-30 12:32:17 UTC
Hmm.. I can't see any obvious reason for this :(
Comment 55 David Nind 2020-03-30 16:52:58 UTC
I figured out what has happened:

1) If I leave MarkLostItemsAsReturned with only the 'when receiving payment for the item' option selected (as per the test plan) then the tests fail.

2) If I change MarkLostItemsAsReturned back to the default (with all options selected) then the tests pass.

If this is fine, then I will sign-off.
Comment 56 Martin Renvoize 2020-03-31 08:33:33 UTC
That's really helpful David, thankyou.

The tests really aught to take account of that.. I'll take a look.
Comment 57 Martin Renvoize 2020-03-31 08:45:43 UTC
Created attachment 102104 [details] [review]
Bug 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.
Comment 58 David Nind 2020-03-31 09:30:34 UTC
Created attachment 102105 [details] [review]
Bug 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>
Comment 59 David Nind 2020-03-31 09:30:38 UTC
Created attachment 102106 [details] [review]
Bug 24474: Add `onpayment` option to `MarkLostItemsReturned`

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 60 David Nind 2020-03-31 09:30:42 UTC
Created attachment 102107 [details] [review]
Bug 24474: Filter out LOST_FOUND

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 61 David Nind 2020-03-31 09:30:46 UTC
Created attachment 102108 [details] [review]
Bug 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>
Comment 62 David Nind 2020-03-31 09:30:50 UTC
Created attachment 102109 [details] [review]
Bug 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>
Comment 63 David Nind 2020-03-31 09:30:54 UTC
Created attachment 102110 [details] [review]
Bug 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>
Comment 64 Kyle M Hall 2020-03-31 13:54:55 UTC
Created attachment 102132 [details] [review]
Bug 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 65 Kyle M Hall 2020-03-31 13:56:45 UTC
Created attachment 102133 [details] [review]
Bug 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 66 Kyle M Hall 2020-03-31 13:56:55 UTC
Created attachment 102134 [details] [review]
Bug 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 67 Kyle M Hall 2020-03-31 13:56:59 UTC
Created attachment 102135 [details] [review]
Bug 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 68 Kyle M Hall 2020-03-31 13:57:03 UTC
Created attachment 102136 [details] [review]
Bug 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 69 Kyle M Hall 2020-03-31 13:57:07 UTC
Created attachment 102137 [details] [review]
Bug 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 70 Kyle M Hall 2020-03-31 13:57:11 UTC
Created attachment 102138 [details] [review]
Bug 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 71 Martin Renvoize 2020-04-03 13:24:09 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 72 Joy Nelson 2020-04-28 20:11:42 UTC
does not apply cleanly to 19.11.x  please rebase if needed.
Comment 73 Joy Nelson 2020-04-28 20:26:24 UTC
Also - dependencies lacking in 19.11.x - not backported