Bug 23091

Summary: Add options to charge new or restore forgiven overdues when a lost item is returned
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: Fines and feesAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: aspencatteam, azucena.aguayo, cslone, jhannert, jonathan.druart, kyle, lucas, martin.renvoize, nick, rkuiper, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22877
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24376
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This new feature allows libraries using the existing WhenLostForgiveFine functionality to reinstate forgiven overdue fines if a lost item is found and the lost item fee is refunded. Specifically, this adds two new options to the lost item fee refund on return policy dropdown in circulation rules. When a lost item is found and the fee refunded, a forgiven overdue fine can be restored in its original accountlines entry or re-created as a new accountlines entry.
Version(s) released in:
20.11.00
Bug Depends on: 18501, 22877    
Bug Blocks: 14825, 25990, 29080, 29081, 29096, 29097, 26704, 29221, 30254    
Attachments: Bug 23091: Work In Progress
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Add handling for restore case
Bug 23091: Add template handling for new messages
Bug 23091: (QA follow-up) Fix unblessed item issue
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Add template handling for new messages
Bug 23091: Add tests for _RestoreOverdueForLostAndFound
Bug 23091: Make relations more explicit
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Add template handling for new messages
Bug 23091: Add tests for _RestoreOverdueForLostAndFound
Bug 23091: Make relations more explicit
Bug 23091: Outline of tests to be written
Bug 23091: (QA follow-up) Fix for UNRETURNED charge
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Add template handling for new messages
Bug 23091: Add tests for _RestoreOverdueForLostAndFound
Bug 23091: Make relations more explicit
Bug 23091: Outline of tests to be written
Bug 23091: (QA follow-up) Fix for UNRETURNED charge
Bug 23091: (follow-up) Remove borrower requirement for restore
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Add template handling for new messages
Bug 23091: Add tests for _RestoreOverdueForLostAndFound
Bug 23091: Make relations more explicit
Bug 23091: Outline of tests to be written
Bug 23091: (QA follow-up) Fix for UNRETURNED charge
Bug 23091: (follow-up) Remove borrower requirement for restore
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Add template handling for new messages
Bug 23091: Add tests for _RestoreOverdueForLostAndFound
Bug 23091: Make relations more explicit
Bug 23091: Outline of tests to be written
Bug 23091: (QA follow-up) Fix for UNRETURNED charge
Bug 23091: (follow-up) Remove borrower requirement for restore
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Add tests for _RestoreOverdueForLostAndFound
Bug 23091: Make relations more explicit
Bug 23091: Add template handling for new messages
Bug 23091: Outline of tests to be written
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Add tests for _RestoreOverdueForLostAndFound
Bug 23091: Make relations more explicit
Bug 23091: Add template handling for new messages
Bug 23091: Outline of tests to be written
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Add tests for _RestoreOverdueForLostAndFound
Bug 23091: Make relations more explicit
Bug 23091: Add template handling for new messages
Bug 23091: Outline of tests to be written
Bug 23091: Move _Restore to Koha::Item store trigger
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Add tests for _RestoreOverdueForLostAndFound
Bug 23091: Make relations more explicit
Bug 23091: Add template handling for new messages
Bug 23091: Move _Restore to Koha::Item store trigger
Bug 23091: Unit Tests
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Add tests for _RestoreOverdueForLostAndFound
Bug 23091: Make relations more explicit
Bug 23091: Add template handling for new messages
Bug 23091: Move _Restore to Koha::Item store trigger
Bug 23091: Unit Tests
Bug 23091: Add tests for \->credits and \->debits
Bug 23091: Simplify tests by turning off AccountAutoReconcile
Bug 23091: (QA follow-up) POD fixes
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Add tests for _RestoreOverdueForLostAndFound
Bug 23091: Make relations more explicit
Bug 23091: Add template handling for new messages
Bug 23091: Move _Restore to Koha::Item store trigger
Bug 23091: Unit Tests
Bug 23091: Add tests for \->credits and \->debits
Bug 23091: Simplify tests by turning off AccountAutoReconcile
Bug 23091: (QA follow-up) POD fixes
Bug 23091: Database update
Bug 23091: Update unit test
Bug 23091: Add handling for new lostreturn rules
Bug 23091: Add tests for _RestoreOverdueForLostAndFound
Bug 23091: Make relations more explicit
Bug 23091: Add template handling for new messages
Bug 23091: Move _Restore to Koha::Item store trigger
Bug 23091: Unit Tests
Bug 23091: Add tests for \->credits and \->debits
Bug 23091: Simplify tests by turning off AccountAutoReconcile
Bug 23091: (QA follow-up) POD fixes

Description Andrew Fuerste-Henry 2019-06-10 18:13:31 UTC
Bug 22877 is closing a loophole that several libraries have been happily using to achieve their desired fine behavior. These libraries want this behavior:
- item accrues overdue fine up to maximum
- when item is lost, overdue fine is forgiven and lost fee is applied
- when item is returned, lost fee is forgiven and overdue fine is restored

So, while many libraries will see bug 22877 as a fix, for some libraries it will be a very big disruption.

My understanding of bug 22877 is that we're changing the fine calculation at checkin to not do anything with fines if the item is lost. Maybe we need to instead say don't add fines if the item is lost and the there is an existing fine for that issue where amountoutstanding!=0?
Comment 1 Chris Slone 2019-09-09 14:00:30 UTC
Hello,

I'd like to confirm that the fixing of Bug 22877 has causes a problem for our library system. When a patron loses an item we charge them the replacement cost but not the late fees (and we assess our late fees when the item is returned). However, if they bring back the item for which they were charged, we still assess the fine that would have been due (and of course do not charge them any longer for the replacement price). So we were a little surprised to find that, after our upgrade to 19.05, patrons who returned lost items were not getting the fines added to their account, and have had to manually replace those fees.

We would be very happy to see the original functionality returned as an option.
Comment 2 Rhonda Kuiper 2019-11-04 21:40:26 UTC
We have the same process for lost items as Chris's library.  If an item is long overdue lost we drop the overdue fine and charge the item replacement cost.  If the item is returned, we drop the item replacement cost and charge the max overdue fine.  Bug 22877 will break this process for us.  We would like to have the ability to maintain this function as an option.
Rhonda Kuiper
Comment 3 AspenCat Team 2019-12-02 16:08:06 UTC
This would be helpful for AspenCat libraries as well.  They currently have to manually calculate and add back in the fine total. - Bob Bennhoff
Comment 4 Martin Renvoize 2019-12-12 12:56:08 UTC
Created attachment 96217 [details] [review]
Bug 23091: Work In Progress
Comment 5 Azucena Aguayo 2020-01-23 18:32:45 UTC
Where is the test plan?

Or should the "Needs Signoff" be changed to some other status?
Comment 6 Martin Renvoize 2020-06-03 16:11:22 UTC
Created attachment 105529 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.
Comment 7 Martin Renvoize 2020-06-03 16:11:25 UTC
Created attachment 105530 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.
Comment 8 Martin Renvoize 2020-06-03 16:11:29 UTC
Created attachment 105531 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds circulation handling for two new lost item return rules.
These new rules allow for the new options of refund lost item charge
plus charge a freshly calculated overdue fine and refind of lost item
charge plus charge a new fine matching the amount from the original
overdue fine.
Comment 9 Martin Renvoize 2020-06-03 16:19:29 UTC
Current state.. 'Refund, 'Refund and Charge' should both be working as expected at this point.. I am still working on 'Refund and Restore'.
Comment 10 Martin Renvoize 2020-06-04 09:54:39 UTC
Created attachment 105544 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.
Comment 11 Martin Renvoize 2020-06-04 09:54:43 UTC
Created attachment 105545 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.
Comment 12 Martin Renvoize 2020-06-04 09:54:46 UTC
Created attachment 105546 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds circulation handling for two new lost item return rules.
These new rules allow for the new options of refund lost item charge
plus charge a freshly calculated overdue fine and refind of lost item
charge plus charge a new fine matching the amount from the original
overdue fine.
Comment 13 Martin Renvoize 2020-06-04 09:54:50 UTC
Created attachment 105547 [details] [review]
Bug 23091: Add handling for restore case

This patch adds handling for the 'restore' case of lostreturn policy.
Comment 14 Martin Renvoize 2020-06-04 09:55:52 UTC
This needs new units tests for the QA step, but I wanted some early testing to check that the end results are as expected by those interested in the feature.

Many thanks in advance.
Comment 15 Andrew Fuerste-Henry 2020-06-04 15:46:47 UTC
I wrote this test plan:
1- apply patch
2- updatedatabase, restart_all
3- verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4- verify WhenLostForgiveFine is set to "Forgive"
5- verify circ rules include fines
6- set Default lost item fee refund on return policy to "Refund lost item charge"
7- create 4 overdue checkouts that will incur fines
8- run fines.pl
9- confirm 4 items checked out with accruing fines
10- confirm all 4 items have a replacement price

Item 1
11- mark the first item lost
12- verify that fine is gone and lost fee has been charged
13- check item in
14- verify that lost fee is gone and overdue charge has not returned

Item 2
15- set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16- mark second item lost
17- verify that fine is gone and lost fee has been charged
18- check item in
19- verify that lost fee is gone and a new overdue charge has been made

Item 3
20- set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21- mark second item lost
22- verify that fine is gone and lost fee has been charged
23- check item in
24- verify that lost fee is gone and the old overdue charge has been restored

Item 4
25- set Default lost item fee refund on return policy to "Leave lost item charge"
26- mark second item lost
27- verify that fine is gone and lost fee has been charged
28- check item in
29- verify that lost fee remains and the overdue charge is still gone
Comment 16 Andrew Fuerste-Henry 2020-06-04 15:50:03 UTC
On step 18 of my plan above, I got this error:
"Unknown error code LostItemFeeCharged at /kohadevbox/koha/circ/returns.pl line 538"

Other thoughts:
- I like the general structure here. where you've chosen to put this setting makes sense
- What happens if my accruing fine has been fully/partially written off or paid?
Comment 17 Andrew Fuerste-Henry 2020-06-04 15:53:52 UTC
It occurs to me that we're not giving any option to keep the lost fee and restore / recharge the overdue. I don't think anyone needs to do so, but raising it here in case someone wants to make the case.
Comment 18 Martin Renvoize 2020-06-05 08:09:54 UTC
(In reply to Andrew Fuerste-Henry from comment #16)
> On step 18 of my plan above, I got this error:
> "Unknown error code LostItemFeeCharged at /kohadevbox/koha/circ/returns.pl
> line 538"

Good catch, I forgot to add the template handling in there.

> Other thoughts:
> - I like the general structure here. where you've chosen to put this setting
> makes sense

> - What happens if my accruing fine has been fully/partially written off or
> paid?

Glad you asked this, as I started to ask the same question myself.. The existing code prior to this patch writes off/forgives any outstanding debt on the overdue as opposed to the original amount of the debt.. so if the patron has already paid or partially paid the accruing fine prior to reporting the item as lost then only the outstanding debt will be forgiven.  I'm not sure that logic makes sense to start with but didn't want to change it here as it probably warrants another discussion.

This patch uses the actual forgiven credit line to restore the outstanding amount.. so if there as a partial payment then we only restore the previously unpaid part of the debt.

Hope that makes sense.
Comment 19 Martin Renvoize 2020-06-05 08:21:01 UTC
Created attachment 105567 [details] [review]
Bug 23091: Add template handling for new messages

This patch adds handling for the display of the two new messages added
by this patchset in the returns screen.
Comment 20 Martin Renvoize 2020-06-05 08:22:04 UTC
Final patch corrects the error Andrew was running into.. the wording of the output messages could perhaps be improved; I am open to suggestions ;)
Comment 21 Martin Renvoize 2020-06-05 08:23:05 UTC
(In reply to Andrew Fuerste-Henry from comment #17)
> It occurs to me that we're not giving any option to keep the lost fee and
> restore / recharge the overdue. I don't think anyone needs to do so, but
> raising it here in case someone wants to make the case.

I wondered about this case too.. but as those that had commented to date had only brought up the two cases I have dealt with here I felt we could perhaps leave such handling for another bug if someone wishes to raise it.
Comment 22 Andrew Fuerste-Henry 2020-06-05 12:07:05 UTC
> The existing code prior to this patch writes off/forgives any outstanding debt
> on the overdue as opposed to the original amount of the debt.. so if the
> patron has already paid or partially paid the accruing fine prior to
> reporting the item as lost then only the outstanding debt will be forgiven. 

That sounds right to me.
Comment 23 Andrew Fuerste-Henry 2020-06-05 12:21:26 UTC
New error at the same place -- step 18, checking in a lost item while rule is set to one of the new options. Here's the error: No branchcode argument passed to Koha::Calendar->new at /kohadevbox/koha/C4/Overdues.pm line 319.
 at /usr/share/perl/5.24/Carp.pm line 167
Comment 24 Martin Renvoize 2020-06-08 14:47:17 UTC
(In reply to Andrew Fuerste-Henry from comment #23)
> New error at the same place -- step 18, checking in a lost item while rule
> is set to one of the new options. Here's the error: No branchcode argument
> passed to Koha::Calendar->new at /kohadevbox/koha/C4/Overdues.pm line 319.
>  at /usr/share/perl/5.24/Carp.pm line 167

Interesting.. what value of 'CircControl' do you have?
Comment 25 Andrew Fuerste-Henry 2020-06-08 14:50:30 UTC
I don't believe I'd changed it from the default in master, library the item is from. I can try to recreate the error later, if it's not happening for you.
Comment 26 Martin Renvoize 2020-06-08 14:51:22 UTC
Created attachment 105649 [details] [review]
Bug 23091: (QA follow-up) Fix unblessed item issue

As highlighted by Andrew, the introduced call to _CalculateAndUpdateFine
was passing a full item object instead of an unblessed hashref.
Comment 27 Martin Renvoize 2020-06-08 14:51:52 UTC
Found it.. back to NSO
Comment 28 Andrew Fuerste-Henry 2020-07-13 12:16:36 UTC
Following the test plan above:
When returning the item with the rule set to "Refund lost item charge and charge new overdue fine", no new fine was generated. Patron ended up with no lost fee or overdue charge for the second item,

When returning the item with the rule set to "Refund lost item charge and restore overdue fine", I got this error: "Unknown error code LostItemFeeRestored at /kohadevbox/koha/circ/returns.pl line 544"
Looks like the chunk of code in question relates to claims returned?
Comment 29 Andrew Fuerste-Henry 2020-07-13 12:17:36 UTC
Realized I'd done some bad copy/paste on the test plan. Updated:

1- apply patch
2- updatedatabase, restart_all
3- verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4- verify WhenLostForgiveFine is set to "Forgive"
5- verify circ rules include fines
6- set Default lost item fee refund on return policy to "Refund lost item charge"
7- create 4 overdue checkouts that will incur fines
8- run fines.pl
9- confirm 4 items checked out with accruing fines
10- confirm all 4 items have a replacement price

Item 1
11- mark the first item lost
12- verify that fine is gone and lost fee has been charged
13- check item in
14- verify that lost fee is gone and overdue charge has not returned

Item 2
15- set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16- mark second item lost
17- verify that fine is gone and lost fee has been charged
18- check item in
19- verify that lost fee is gone and a new overdue charge has been made

Item 3
20- set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21- mark third item lost
22- verify that fine is gone and lost fee has been charged
23- check item in
24- verify that lost fee is gone and the old overdue charge has been restored

Item 4
25- set Default lost item fee refund on return policy to "Leave lost item charge"
26- mark fourth item lost
27- verify that fine is gone and lost fee has been charged
28- check item in
29- verify that lost fee remains and the overdue charge is still gone
Comment 30 Martin Renvoize 2020-07-14 14:03:16 UTC
Created attachment 106871 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.
Comment 31 Martin Renvoize 2020-07-14 14:03:20 UTC
Created attachment 106872 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.

Test plan:
1/ Read the code changes in t/db_dependant/Koha/CirculationRules and
t/db_dependant/Koha/IssuingRules and confirm they make sense.
2/ Run the updated tests and ensure they pass.
Comment 32 Martin Renvoize 2020-07-14 14:03:24 UTC
Created attachment 106873 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds handing for the new values available for the lostreturn
policy settings.

* undef - Do nothing, leave fees and fines as they were at the point of
  lose.
* refund - Refund the lost item fee only
* charge - Refund the lost item fee and charge a fresh overdue fine
  dated for a return on the date the item is 'found'
* restore - Refund the lost item fee and restore the original overdue
  fine (dated for a 'return' on the date the item was 'lost'

Test plan
1/ apply patch
2/ updatedatabase, restart_all
3/ verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4/ verify WhenLostForgiveFine is set to "Forgive"
5/ verify circ rules include fines
6/ set Default lost item fee refund on return policy to "Refund lost item charge"
7/ create 4 overdue checkouts that will incur fines
8/ run fines.pl
9/ confirm 4 items checked out with accruing fines
10/ confirm all 4 items have a replacement price

Item 1
11/ mark the first item lost
12/ verify that fine is gone and lost fee has been charged
13/ check item in
14/ verify that lost fee is gone and overdue charge has not returned

Item 2
15/ set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16/ mark second item lost
17/ verify that fine is gone and lost fee has been charged
18/ check item in
19/ verify that lost fee is gone and a new overdue charge has been made

Item 3
20/ set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21/ mark third item lost
22/ verify that fine is gone and lost fee has been charged
23/ check item in
24/ verify that lost fee is gone and the old overdue charge has been restored

Item 4
25/ set Default lost item fee refund on return policy to "Leave lost item charge"
26/ mark fourth item lost
27/ verify that fine is gone and lost fee has been charged
28/ check item in
29/ verify that lost fee remains and the overdue charge is still gone
Comment 33 Martin Renvoize 2020-07-14 14:03:27 UTC
Created attachment 106874 [details] [review]
Bug 23091: Add template handling for new messages

This patch adds handling for the display of the two new messages added
by this patchset in the returns screen.
Comment 34 Martin Renvoize 2020-07-14 14:03:31 UTC
Created attachment 106875 [details] [review]
Bug 23091: Add tests for _RestoreOverdueForLostAndFound

This patch adds unit tests for the newly introduced
_RestoreOverdueForLostAndFound method in C4::Circulation.

Test plan
1/ Read the new tests added to t/db_dependent/Circuation.t to ensure
they make sense
2/ Run the tests and verify they pass
3/ Signoff
Comment 35 Martin Renvoize 2020-07-14 14:03:34 UTC
Created attachment 106876 [details] [review]
Bug 23091: Make relations more explicit

This patch simply makes the queries built for the ->debits and ->credits
relations in Koha::Account::Line more explicit.

Test plan
1/ Ensure tests continue to pass
2/ Run tests with DBIC_TRACE=1 DBIC_TRACE_PROFILE=console and note that
the related queries not include 'credit.' and 'debit.' as appropriate.
Comment 36 Martin Renvoize 2020-07-14 14:06:19 UTC
Back to NSO.. I think I've covered Andrews issues now.. and as part of that I've written some pretty comprehensive tests for the _Restore method that's been added here.

QA Note: I still need to add tests for the functionality when called via AddReturn and AddIssue.. seems that's never been tested to date :(
Comment 37 Martin Renvoize 2020-07-14 15:43:42 UTC
Found a bug.. setting to assigned whilst I work through it
Comment 38 Andrew Fuerste-Henry 2020-07-14 15:50:06 UTC
(In reply to Andrew Fuerste-Henry from comment #28)
> Following the test plan above:
> When returning the item with the rule set to "Refund lost item charge and
> charge new overdue fine", no new fine was generated. Patron ended up with no
> lost fee or overdue charge for the second item,
> 
> When returning the item with the rule set to "Refund lost item charge and
> restore overdue fine", I got this error: "Unknown error code
> LostItemFeeRestored at /kohadevbox/koha/circ/returns.pl line 544"
> Looks like the chunk of code in question relates to claims returned?

Both of these are still failing. The server error seems to be due to a typo in circ/returns.pl where you're calling something LostItemRestored instead of LostItemFeeRestored.

But even after changing that, I can't get either a new overdue fine to to generate or an old overdue fine to restore. Seems like the parameters for LostItemFeeRestored in C4/Circulation.pm are being passed backwards (itemnumber then borrowernumber), but changing that didn't fix it.

Am I missing some system preference or circ rule thing?
Comment 39 Martin Renvoize 2020-07-14 19:38:23 UTC
Sorry Andrew, looks like I didn't catch you early enough there.. I spotted a couple of nasty failure cases which I'm working through... It's turning into a bit of an unravelling thread.

For the new charge, we need to fetch an old issue if any of the MarkLostItemReturned values are involved... I think that's the case that caught you.. 

My test environment had those all disabled for some reason.. a DB reset meant I suddenly saw your failure cases.

I'll take a look at your other comments and embellish the t at plan a bit further tomorrow when I'm fresh.  There's soooo many syspref interactions at play here I hadn't fully appreciated when I embarked down this path.

Also, a question for you.. what should happen in the case where they have the system set to not forgive fines and to also charge a fresh fine on return of a lost item.. if feels like a mis-configuration at that point to me but should we catch that case and just fallback to doing nothing or should we niavely continue to charge a fresh fine on top so the patron would end up with two fines.. one dated up to the date when the item was marked lost and then a second dated up to the date it was marked found again?
Comment 40 Andrew Fuerste-Henry 2020-07-14 19:49:25 UTC
> There's soooo many syspref interactions at
> play here I hadn't fully appreciated when I embarked down this path.

And I very much thank you for walking this scary path through the foreboding wood!

> Also, a question for you.. what should happen in the case where they have
> the system set to not forgive fines and to also charge a fresh fine on
> return of a lost item.. if feels like a mis-configuration at that point to
> me but should we catch that case and just fallback to doing nothing or
> should we niavely continue to charge a fresh fine on top so the patron would
> end up with two fines.. one dated up to the date when the item was marked
> lost and then a second dated up to the date it was marked found again?

That is a very good question. I'd say if WhenLostForgiveFines is set to not forgive then we don't do anything to overdues when a lost item is returned. If the initial overdue fine was never forgiven, then we don't make a new overdue fine or touch/edit the existing overdue if the item is later returned.

I hate to say it, but that sort of points me toward thinking these new options should live in WhenLostForgiveFine rather than in the lost item fee refund on return policy. That'd mean lost item fee refund on return policy stays with the existing yes/no options and WhenLostForgiveFine changes to four options: Don't forgive, Forgive permanently, Forgive but restore fine if item found, Forgive but create new fine if item found.

But please ignore that last paragraph if you feel you're too far along with your current approach. You asked a question that really shifted some stuff in my thinking on this.
Comment 41 Martin Renvoize 2020-07-15 07:12:42 UTC
(In reply to Andrew Fuerste-Henry from comment #40)
> > There's soooo many syspref interactions at
> > play here I hadn't fully appreciated when I embarked down this path.
> 
> And I very much thank you for walking this scary path through the foreboding
> wood!
> 
> > Also, a question for you.. what should happen in the case where they have
> > the system set to not forgive fines and to also charge a fresh fine on
> > return of a lost item.. if feels like a mis-configuration at that point to
> > me but should we catch that case and just fallback to doing nothing or
> > should we niavely continue to charge a fresh fine on top so the patron would
> > end up with two fines.. one dated up to the date when the item was marked
> > lost and then a second dated up to the date it was marked found again?
> 
> That is a very good question. I'd say if WhenLostForgiveFines is set to not
> forgive then we don't do anything to overdues when a lost item is returned.
> If the initial overdue fine was never forgiven, then we don't make a new
> overdue fine or touch/edit the existing overdue if the item is later
> returned.
> 
> I hate to say it, but that sort of points me toward thinking these new
> options should live in WhenLostForgiveFine rather than in the lost item fee
> refund on return policy. That'd mean lost item fee refund on return policy
> stays with the existing yes/no options and WhenLostForgiveFine changes to
> four options: Don't forgive, Forgive permanently, Forgive but restore fine
> if item found, Forgive but create new fine if item found.
> 
> But please ignore that last paragraph if you feel you're too far along with
> your current approach. You asked a question that really shifted some stuff
> in my thinking on this.

Damn.. the existing preference levels are so inconsistent :(

WhenLostChargeReplacementFee is set at the system wide level, yet RefundLostFee is set at the branch level.. that doesn't make much sense to me.. surely if one allows granular control, so should the other..  I'm almost inclined to go the opposite way from your suggestion and work through moving some of those system policies into the circ rules to allow more granular control (Likely in a bug for each, rather than inline here) to make them self-consistent. You can still get the same overall functionality by only setting them at the default level and not adding overridden rules at the branch level as we do now for the Refund policy.  (In my mind, I'm looking at consortia and thinking the more granular we get for them the better.)
Comment 42 Martin Renvoize 2020-07-15 10:22:23 UTC
(In reply to Martin Renvoize from comment #41)
> 
> Damn.. the existing preference levels are so inconsistent :(
> 

I've opened bug 25990 to rectify that.. one for the future
Comment 43 Martin Renvoize 2020-07-15 10:45:09 UTC
Created attachment 106906 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.
Comment 44 Martin Renvoize 2020-07-15 10:45:14 UTC
Created attachment 106907 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.

Test plan:
1/ Read the code changes in t/db_dependant/Koha/CirculationRules and
t/db_dependant/Koha/IssuingRules and confirm they make sense.
2/ Run the updated tests and ensure they pass.
Comment 45 Martin Renvoize 2020-07-15 10:45:17 UTC
Created attachment 106908 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds handing for the new values available for the lostreturn
policy settings.

* undef - Do nothing, leave fees and fines as they were at the point of
  lose.
* refund - Refund the lost item fee only
* charge - Refund the lost item fee and charge a fresh overdue fine
  dated for a return on the date the item is 'found'
* restore - Refund the lost item fee and restore the original overdue
  fine (dated for a 'return' on the date the item was 'lost'

Test plan
1/ apply patch
2/ updatedatabase, restart_all
3/ verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4/ verify WhenLostForgiveFine is set to "Forgive"
5/ verify circ rules include fines
6/ set Default lost item fee refund on return policy to "Refund lost item charge"
7/ create 4 overdue checkouts that will incur fines
8/ run fines.pl
9/ confirm 4 items checked out with accruing fines
10/ confirm all 4 items have a replacement price

Item 1
11/ mark the first item lost
12/ verify that fine is gone and lost fee has been charged
13/ check item in
14/ verify that lost fee is gone and overdue charge has not returned

Item 2
15/ set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16/ mark second item lost
17/ verify that fine is gone and lost fee has been charged
18/ check item in
19/ verify that lost fee is gone and a new overdue charge has been made

Item 3
20/ set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21/ mark third item lost
22/ verify that fine is gone and lost fee has been charged
23/ check item in
24/ verify that lost fee is gone and the old overdue charge has been restored

Item 4
25/ set Default lost item fee refund on return policy to "Leave lost item charge"
26/ mark fourth item lost
27/ verify that fine is gone and lost fee has been charged
28/ check item in
29/ verify that lost fee remains and the overdue charge is still gone
Comment 46 Martin Renvoize 2020-07-15 10:45:21 UTC
Created attachment 106909 [details] [review]
Bug 23091: Add template handling for new messages

This patch adds handling for the display of the two new messages added
by this patchset in the returns screen.
Comment 47 Martin Renvoize 2020-07-15 10:45:25 UTC
Created attachment 106910 [details] [review]
Bug 23091: Add tests for _RestoreOverdueForLostAndFound

This patch adds unit tests for the newly introduced
_RestoreOverdueForLostAndFound method in C4::Circulation.

Test plan
1/ Read the new tests added to t/db_dependent/Circuation.t to ensure
they make sense
2/ Run the tests and verify they pass
3/ Signoff
Comment 48 Martin Renvoize 2020-07-15 10:45:29 UTC
Created attachment 106911 [details] [review]
Bug 23091: Make relations more explicit

This patch simply makes the queries built for the ->debits and ->credits
relations in Koha::Account::Line more explicit.

Test plan
1/ Ensure tests continue to pass
2/ Run tests with DBIC_TRACE=1 DBIC_TRACE_PROFILE=console and note that
the related queries not include 'credit.' and 'debit.' as appropriate.
Comment 49 Martin Renvoize 2020-07-15 10:45:33 UTC
Created attachment 106912 [details] [review]
Bug 23091: Outline of tests to be written
Comment 50 Martin Renvoize 2020-07-15 10:46:36 UTC
Back to NSO.. final patch is work in progress.. writing all the tests is going to be a fairly large undertaking :(
Comment 51 Andrew Fuerste-Henry 2020-07-15 12:25:28 UTC
On Item 2 (create a new fine), the fine generated successfully but with status UNRETURNED. Shouldn't it be RETURNED instead?

On Item 3 (restore old fine), the old fine was not restored. The fine was left with amountoutstanding=0 and the associated credit was not voided.
Comment 52 Martin Renvoize 2020-07-16 16:20:42 UTC
Created attachment 106961 [details] [review]
Bug 23091: (QA follow-up) Fix for UNRETURNED charge
Comment 53 Martin Renvoize 2020-07-17 15:17:14 UTC
Created attachment 107026 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.
Comment 54 Martin Renvoize 2020-07-17 15:17:17 UTC
Created attachment 107027 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.

Test plan:
1/ Read the code changes in t/db_dependant/Koha/CirculationRules and
t/db_dependant/Koha/IssuingRules and confirm they make sense.
2/ Run the updated tests and ensure they pass.
Comment 55 Martin Renvoize 2020-07-17 15:17:21 UTC
Created attachment 107028 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds handing for the new values available for the lostreturn
policy settings.

* undef - Do nothing, leave fees and fines as they were at the point of
  lose.
* refund - Refund the lost item fee only
* charge - Refund the lost item fee and charge a fresh overdue fine
  dated for a return on the date the item is 'found'
* restore - Refund the lost item fee and restore the original overdue
  fine (dated for a 'return' on the date the item was 'lost'

Test plan
1/ apply patch
2/ updatedatabase, restart_all
3/ verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4/ verify WhenLostForgiveFine is set to "Forgive"
5/ verify circ rules include fines
6/ set Default lost item fee refund on return policy to "Refund lost item charge"
7/ create 4 overdue checkouts that will incur fines
8/ run fines.pl
9/ confirm 4 items checked out with accruing fines
10/ confirm all 4 items have a replacement price

Item 1
11/ mark the first item lost
12/ verify that fine is gone and lost fee has been charged
13/ check item in
14/ verify that lost fee is gone and overdue charge has not returned

Item 2
15/ set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16/ mark second item lost
17/ verify that fine is gone and lost fee has been charged
18/ check item in
19/ verify that lost fee is gone and a new overdue charge has been made

Item 3
20/ set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21/ mark third item lost
22/ verify that fine is gone and lost fee has been charged
23/ check item in
24/ verify that lost fee is gone and the old overdue charge has been restored

Item 4
25/ set Default lost item fee refund on return policy to "Leave lost item charge"
26/ mark fourth item lost
27/ verify that fine is gone and lost fee has been charged
28/ check item in
29/ verify that lost fee remains and the overdue charge is still gone
Comment 56 Martin Renvoize 2020-07-17 15:17:25 UTC
Created attachment 107029 [details] [review]
Bug 23091: Add template handling for new messages

This patch adds handling for the display of the two new messages added
by this patchset in the returns screen.
Comment 57 Martin Renvoize 2020-07-17 15:17:29 UTC
Created attachment 107030 [details] [review]
Bug 23091: Add tests for _RestoreOverdueForLostAndFound

This patch adds unit tests for the newly introduced
_RestoreOverdueForLostAndFound method in C4::Circulation.

Test plan
1/ Read the new tests added to t/db_dependent/Circuation.t to ensure
they make sense
2/ Run the tests and verify they pass
3/ Signoff
Comment 58 Martin Renvoize 2020-07-17 15:17:33 UTC
Created attachment 107031 [details] [review]
Bug 23091: Make relations more explicit

This patch simply makes the queries built for the ->debits and ->credits
relations in Koha::Account::Line more explicit.

Test plan
1/ Ensure tests continue to pass
2/ Run tests with DBIC_TRACE=1 DBIC_TRACE_PROFILE=console and note that
the related queries not include 'credit.' and 'debit.' as appropriate.
Comment 59 Martin Renvoize 2020-07-17 15:17:38 UTC
Created attachment 107032 [details] [review]
Bug 23091: Outline of tests to be written
Comment 60 Martin Renvoize 2020-07-17 15:17:42 UTC
Created attachment 107033 [details] [review]
Bug 23091: (QA follow-up) Fix for UNRETURNED charge
Comment 61 Martin Renvoize 2020-07-17 15:17:46 UTC
Created attachment 107034 [details] [review]
Bug 23091: (follow-up) Remove borrower requirement for restore
Comment 62 Martin Renvoize 2020-07-17 15:19:13 UTC
Well... that was fun... hopefully that's now caught all edge cases... thankyou so much for the testing to date Andrew :)
Comment 63 Andrew Fuerste-Henry 2020-07-20 13:28:47 UTC
Created attachment 107100 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 64 Andrew Fuerste-Henry 2020-07-20 13:28:51 UTC
Created attachment 107101 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.

Test plan:
1/ Read the code changes in t/db_dependant/Koha/CirculationRules and
t/db_dependant/Koha/IssuingRules and confirm they make sense.
2/ Run the updated tests and ensure they pass.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 65 Andrew Fuerste-Henry 2020-07-20 13:28:55 UTC
Created attachment 107102 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds handing for the new values available for the lostreturn
policy settings.

* undef - Do nothing, leave fees and fines as they were at the point of
  lose.
* refund - Refund the lost item fee only
* charge - Refund the lost item fee and charge a fresh overdue fine
  dated for a return on the date the item is 'found'
* restore - Refund the lost item fee and restore the original overdue
  fine (dated for a 'return' on the date the item was 'lost'

Test plan
1/ apply patch
2/ updatedatabase, restart_all
3/ verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4/ verify WhenLostForgiveFine is set to "Forgive"
5/ verify circ rules include fines
6/ set Default lost item fee refund on return policy to "Refund lost item charge"
7/ create 4 overdue checkouts that will incur fines
8/ run fines.pl
9/ confirm 4 items checked out with accruing fines
10/ confirm all 4 items have a replacement price

Item 1
11/ mark the first item lost
12/ verify that fine is gone and lost fee has been charged
13/ check item in
14/ verify that lost fee is gone and overdue charge has not returned

Item 2
15/ set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16/ mark second item lost
17/ verify that fine is gone and lost fee has been charged
18/ check item in
19/ verify that lost fee is gone and a new overdue charge has been made

Item 3
20/ set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21/ mark third item lost
22/ verify that fine is gone and lost fee has been charged
23/ check item in
24/ verify that lost fee is gone and the old overdue charge has been restored

Item 4
25/ set Default lost item fee refund on return policy to "Leave lost item charge"
26/ mark fourth item lost
27/ verify that fine is gone and lost fee has been charged
28/ check item in
29/ verify that lost fee remains and the overdue charge is still gone

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 66 Andrew Fuerste-Henry 2020-07-20 13:29:00 UTC
Created attachment 107103 [details] [review]
Bug 23091: Add template handling for new messages

This patch adds handling for the display of the two new messages added
by this patchset in the returns screen.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 67 Andrew Fuerste-Henry 2020-07-20 13:29:05 UTC
Created attachment 107104 [details] [review]
Bug 23091: Add tests for _RestoreOverdueForLostAndFound

This patch adds unit tests for the newly introduced
_RestoreOverdueForLostAndFound method in C4::Circulation.

Test plan
1/ Read the new tests added to t/db_dependent/Circuation.t to ensure
they make sense
2/ Run the tests and verify they pass
3/ Signoff

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 68 Andrew Fuerste-Henry 2020-07-20 13:29:10 UTC
Created attachment 107105 [details] [review]
Bug 23091: Make relations more explicit

This patch simply makes the queries built for the ->debits and ->credits
relations in Koha::Account::Line more explicit.

Test plan
1/ Ensure tests continue to pass
2/ Run tests with DBIC_TRACE=1 DBIC_TRACE_PROFILE=console and note that
the related queries not include 'credit.' and 'debit.' as appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 69 Andrew Fuerste-Henry 2020-07-20 13:29:15 UTC
Created attachment 107106 [details] [review]
Bug 23091: Outline of tests to be written

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 70 Andrew Fuerste-Henry 2020-07-20 13:29:18 UTC
Created attachment 107107 [details] [review]
Bug 23091: (QA follow-up) Fix for UNRETURNED charge

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 71 Andrew Fuerste-Henry 2020-07-20 13:29:22 UTC
Created attachment 107108 [details] [review]
Bug 23091: (follow-up) Remove borrower requirement for restore

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 72 Andrew Fuerste-Henry 2020-07-20 13:29:45 UTC
Phew, thanks Martin!
Comment 73 Martin Renvoize 2020-07-24 12:27:41 UTC
Created attachment 107335 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 74 Martin Renvoize 2020-07-24 12:27:45 UTC
Created attachment 107336 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.

Test plan:
1/ Read the code changes in t/db_dependant/Koha/CirculationRules and
t/db_dependant/Koha/IssuingRules and confirm they make sense.
2/ Run the updated tests and ensure they pass.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 75 Martin Renvoize 2020-07-24 12:27:49 UTC
Created attachment 107337 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds handing for the new values available for the lostreturn
policy settings.

* undef - Do nothing, leave fees and fines as they were at the point of
  lose.
* refund - Refund the lost item fee only
* charge - Refund the lost item fee and charge a fresh overdue fine
  dated for a return on the date the item is 'found'
* restore - Refund the lost item fee and restore the original overdue
  fine (dated for a 'return' on the date the item was 'lost'

Test plan
1/ apply patch
2/ updatedatabase, restart_all
3/ verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4/ verify WhenLostForgiveFine is set to "Forgive"
5/ verify circ rules include fines
6/ set Default lost item fee refund on return policy to "Refund lost item charge"
7/ create 4 overdue checkouts that will incur fines
8/ run fines.pl
9/ confirm 4 items checked out with accruing fines
10/ confirm all 4 items have a replacement price

Item 1
11/ mark the first item lost
12/ verify that fine is gone and lost fee has been charged
13/ check item in
14/ verify that lost fee is gone and overdue charge has not returned

Item 2
15/ set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16/ mark second item lost
17/ verify that fine is gone and lost fee has been charged
18/ check item in
19/ verify that lost fee is gone and a new overdue charge has been made

Item 3
20/ set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21/ mark third item lost
22/ verify that fine is gone and lost fee has been charged
23/ check item in
24/ verify that lost fee is gone and the old overdue charge has been restored

Item 4
25/ set Default lost item fee refund on return policy to "Leave lost item charge"
26/ mark fourth item lost
27/ verify that fine is gone and lost fee has been charged
28/ check item in
29/ verify that lost fee remains and the overdue charge is still gone

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 76 Martin Renvoize 2020-07-24 12:27:53 UTC
Created attachment 107338 [details] [review]
Bug 23091: Add template handling for new messages

This patch adds handling for the display of the two new messages added
by this patchset in the returns screen.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 77 Martin Renvoize 2020-07-24 12:27:57 UTC
Created attachment 107339 [details] [review]
Bug 23091: Add tests for _RestoreOverdueForLostAndFound

This patch adds unit tests for the newly introduced
_RestoreOverdueForLostAndFound method in C4::Circulation.

Test plan
1/ Read the new tests added to t/db_dependent/Circuation.t to ensure
they make sense
2/ Run the tests and verify they pass
3/ Signoff

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 78 Martin Renvoize 2020-07-24 12:28:01 UTC
Created attachment 107340 [details] [review]
Bug 23091: Make relations more explicit

This patch simply makes the queries built for the ->debits and ->credits
relations in Koha::Account::Line more explicit.

Test plan
1/ Ensure tests continue to pass
2/ Run tests with DBIC_TRACE=1 DBIC_TRACE_PROFILE=console and note that
the related queries not include 'credit.' and 'debit.' as appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 79 Martin Renvoize 2020-07-24 12:28:05 UTC
Created attachment 107341 [details] [review]
Bug 23091: Outline of tests to be written

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 80 Martin Renvoize 2020-07-24 12:28:11 UTC
Created attachment 107342 [details] [review]
Bug 23091: (QA follow-up) Fix for UNRETURNED charge

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 81 Martin Renvoize 2020-07-24 12:28:15 UTC
Created attachment 107343 [details] [review]
Bug 23091: (follow-up) Remove borrower requirement for restore

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 82 Martin Renvoize 2020-07-24 12:28:38 UTC
Rebased for bug 20815
Comment 83 Martin Renvoize 2020-08-14 16:32:45 UTC
I think I should rebase this upon bug 18501.. that bug does some nice refactoring which we should utilise.

I will try to get to that early next week.
Comment 84 Martin Renvoize 2020-08-19 14:43:59 UTC
Delayed.. hopefully get back to this tomorrow.
Comment 85 Martin Renvoize 2020-08-20 09:56:39 UTC
Created attachment 108718 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 86 Martin Renvoize 2020-08-20 09:56:43 UTC
Created attachment 108719 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.

Test plan:
1/ Read the code changes in t/db_dependant/Koha/CirculationRules and
t/db_dependant/Koha/IssuingRules and confirm they make sense.
2/ Run the updated tests and ensure they pass.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 87 Martin Renvoize 2020-08-20 09:56:48 UTC
Created attachment 108720 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds handing for the new values available for the lostreturn
policy settings.

* undef - Do nothing, leave fees and fines as they were at the point of
  lose.
* refund - Refund the lost item fee only
* charge - Refund the lost item fee and charge a fresh overdue fine
  dated for a return on the date the item is 'found'
* restore - Refund the lost item fee and restore the original overdue
  fine (dated for a 'return' on the date the item was 'lost'

Test plan
1/ apply patch
2/ updatedatabase, restart_all
3/ verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4/ verify WhenLostForgiveFine is set to "Forgive"
5/ verify circ rules include fines
6/ set Default lost item fee refund on return policy to "Refund lost item charge"
7/ create 4 overdue checkouts that will incur fines
8/ run fines.pl
9/ confirm 4 items checked out with accruing fines
10/ confirm all 4 items have a replacement price

Item 1
11/ mark the first item lost
12/ verify that fine is gone and lost fee has been charged
13/ check item in
14/ verify that lost fee is gone and overdue charge has not returned

Item 2
15/ set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16/ mark second item lost
17/ verify that fine is gone and lost fee has been charged
18/ check item in
19/ verify that lost fee is gone and a new overdue charge has been made

Item 3
20/ set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21/ mark third item lost
22/ verify that fine is gone and lost fee has been charged
23/ check item in
24/ verify that lost fee is gone and the old overdue charge has been restored

Item 4
25/ set Default lost item fee refund on return policy to "Leave lost item charge"
26/ mark fourth item lost
27/ verify that fine is gone and lost fee has been charged
28/ check item in
29/ verify that lost fee remains and the overdue charge is still gone

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 88 Martin Renvoize 2020-08-20 09:56:52 UTC
Created attachment 108721 [details] [review]
Bug 23091: Add tests for _RestoreOverdueForLostAndFound

This patch adds unit tests for the newly introduced
_RestoreOverdueForLostAndFound method in C4::Circulation.

Test plan
1/ Read the new tests added to t/db_dependent/Circuation.t to ensure
they make sense
2/ Run the tests and verify they pass
3/ Signoff

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 89 Martin Renvoize 2020-08-20 09:56:56 UTC
Created attachment 108722 [details] [review]
Bug 23091: Make relations more explicit

This patch simply makes the queries built for the ->debits and ->credits
relations in Koha::Account::Line more explicit.

Test plan
1/ Ensure tests continue to pass
2/ Run tests with DBIC_TRACE=1 DBIC_TRACE_PROFILE=console and note that
the related queries not include 'credit.' and 'debit.' as appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 90 Martin Renvoize 2020-08-20 09:57:00 UTC
Created attachment 108723 [details] [review]
Bug 23091: Add template handling for new messages

This patch adds handling for the display of the two new messages added
by this patchset in the returns screen.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 91 Martin Renvoize 2020-08-20 09:57:04 UTC
Created attachment 108724 [details] [review]
Bug 23091: Outline of tests to be written

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 92 Martin Renvoize 2020-08-20 10:00:38 UTC
Rebased upon 18501.. ready for QA :)

Note, I did want to more drastically refactor this to more closely resemble the work undertaken in bug 18501, but I wasn't sure how to handle the requirements for passing around dates (Restoring a fine would be simple enough, but charging a fresh one would add complications in so much as we need to pass through the possible backdated return date)
Comment 93 Katrin Fischer 2020-08-22 12:56:52 UTC
Patch no longer applies cleanly, please rebase! (should not be hard for someone getting the logic there)
Comment 94 Martin Renvoize 2020-09-03 06:42:10 UTC
Created attachment 109571 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 95 Martin Renvoize 2020-09-03 06:42:14 UTC
Created attachment 109572 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.

Test plan:
1/ Read the code changes in t/db_dependant/Koha/CirculationRules and
t/db_dependant/Koha/IssuingRules and confirm they make sense.
2/ Run the updated tests and ensure they pass.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 96 Martin Renvoize 2020-09-03 06:42:19 UTC
Created attachment 109573 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds handing for the new values available for the lostreturn
policy settings.

* undef - Do nothing, leave fees and fines as they were at the point of
  lose.
* refund - Refund the lost item fee only
* charge - Refund the lost item fee and charge a fresh overdue fine
  dated for a return on the date the item is 'found'
* restore - Refund the lost item fee and restore the original overdue
  fine (dated for a 'return' on the date the item was 'lost'

Test plan
1/ apply patch
2/ updatedatabase, restart_all
3/ verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4/ verify WhenLostForgiveFine is set to "Forgive"
5/ verify circ rules include fines
6/ set Default lost item fee refund on return policy to "Refund lost item charge"
7/ create 4 overdue checkouts that will incur fines
8/ run fines.pl
9/ confirm 4 items checked out with accruing fines
10/ confirm all 4 items have a replacement price

Item 1
11/ mark the first item lost
12/ verify that fine is gone and lost fee has been charged
13/ check item in
14/ verify that lost fee is gone and overdue charge has not returned

Item 2
15/ set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16/ mark second item lost
17/ verify that fine is gone and lost fee has been charged
18/ check item in
19/ verify that lost fee is gone and a new overdue charge has been made

Item 3
20/ set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21/ mark third item lost
22/ verify that fine is gone and lost fee has been charged
23/ check item in
24/ verify that lost fee is gone and the old overdue charge has been restored

Item 4
25/ set Default lost item fee refund on return policy to "Leave lost item charge"
26/ mark fourth item lost
27/ verify that fine is gone and lost fee has been charged
28/ check item in
29/ verify that lost fee remains and the overdue charge is still gone

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 97 Martin Renvoize 2020-09-03 06:42:23 UTC
Created attachment 109574 [details] [review]
Bug 23091: Add tests for _RestoreOverdueForLostAndFound

This patch adds unit tests for the newly introduced
_RestoreOverdueForLostAndFound method in C4::Circulation.

Test plan
1/ Read the new tests added to t/db_dependent/Circuation.t to ensure
they make sense
2/ Run the tests and verify they pass
3/ Signoff

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 98 Martin Renvoize 2020-09-03 06:42:27 UTC
Created attachment 109575 [details] [review]
Bug 23091: Make relations more explicit

This patch simply makes the queries built for the ->debits and ->credits
relations in Koha::Account::Line more explicit.

Test plan
1/ Ensure tests continue to pass
2/ Run tests with DBIC_TRACE=1 DBIC_TRACE_PROFILE=console and note that
the related queries not include 'credit.' and 'debit.' as appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 99 Martin Renvoize 2020-09-03 06:42:32 UTC
Created attachment 109576 [details] [review]
Bug 23091: Add template handling for new messages

This patch adds handling for the display of the two new messages added
by this patchset in the returns screen.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 100 Martin Renvoize 2020-09-03 06:42:36 UTC
Created attachment 109577 [details] [review]
Bug 23091: Outline of tests to be written

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 101 Martin Renvoize 2020-09-03 06:50:26 UTC
Last outstanding work is to write the outlined tests from the final patch attached.

The patchset does, however, now apply again.
Comment 102 Martin Renvoize 2020-09-03 16:15:11 UTC
I think this needs a little more work..

Whilst working on writing the tests I decided there were more cases I need to cover :(... Setting back to assigned whilst I deal with them

Highlights:
* Move '_Restore' routine into the Koha::Item trigger
* Handle AutoReconcile case
* Prevent failure when borrower has been deleted
Comment 103 Martin Renvoize 2020-09-24 14:09:23 UTC
Created attachment 110660 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 104 Martin Renvoize 2020-09-24 14:09:29 UTC
Created attachment 110661 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.

Test plan:
1/ Read the code changes in t/db_dependant/Koha/CirculationRules and
t/db_dependant/Koha/IssuingRules and confirm they make sense.
2/ Run the updated tests and ensure they pass.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 105 Martin Renvoize 2020-09-24 14:09:36 UTC
Created attachment 110662 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds handing for the new values available for the lostreturn
policy settings.

* undef - Do nothing, leave fees and fines as they were at the point of
  lose.
* refund - Refund the lost item fee only
* charge - Refund the lost item fee and charge a fresh overdue fine
  dated for a return on the date the item is 'found'
* restore - Refund the lost item fee and restore the original overdue
  fine (dated for a 'return' on the date the item was 'lost'

Test plan
1/ apply patch
2/ updatedatabase, restart_all
3/ verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4/ verify WhenLostForgiveFine is set to "Forgive"
5/ verify circ rules include fines
6/ set Default lost item fee refund on return policy to "Refund lost item charge"
7/ create 4 overdue checkouts that will incur fines
8/ run fines.pl
9/ confirm 4 items checked out with accruing fines
10/ confirm all 4 items have a replacement price

Item 1
11/ mark the first item lost
12/ verify that fine is gone and lost fee has been charged
13/ check item in
14/ verify that lost fee is gone and overdue charge has not returned

Item 2
15/ set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16/ mark second item lost
17/ verify that fine is gone and lost fee has been charged
18/ check item in
19/ verify that lost fee is gone and a new overdue charge has been made

Item 3
20/ set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21/ mark third item lost
22/ verify that fine is gone and lost fee has been charged
23/ check item in
24/ verify that lost fee is gone and the old overdue charge has been restored

Item 4
25/ set Default lost item fee refund on return policy to "Leave lost item charge"
26/ mark fourth item lost
27/ verify that fine is gone and lost fee has been charged
28/ check item in
29/ verify that lost fee remains and the overdue charge is still gone

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 106 Martin Renvoize 2020-09-24 14:09:42 UTC
Created attachment 110663 [details] [review]
Bug 23091: Add tests for _RestoreOverdueForLostAndFound

This patch adds unit tests for the newly introduced
_RestoreOverdueForLostAndFound method in C4::Circulation.

Test plan
1/ Read the new tests added to t/db_dependent/Circuation.t to ensure
they make sense
2/ Run the tests and verify they pass
3/ Signoff

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 107 Martin Renvoize 2020-09-24 14:09:49 UTC
Created attachment 110664 [details] [review]
Bug 23091: Make relations more explicit

This patch simply makes the queries built for the ->debits and ->credits
relations in Koha::Account::Line more explicit.

Test plan
1/ Ensure tests continue to pass
2/ Run tests with DBIC_TRACE=1 DBIC_TRACE_PROFILE=console and note that
the related queries not include 'credit.' and 'debit.' as appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 108 Martin Renvoize 2020-09-24 14:09:55 UTC
Created attachment 110665 [details] [review]
Bug 23091: Add template handling for new messages

This patch adds handling for the display of the two new messages added
by this patchset in the returns screen.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 109 Martin Renvoize 2020-09-24 14:10:07 UTC
Created attachment 110666 [details] [review]
Bug 23091: Outline of tests to be written

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 110 Martin Renvoize 2020-09-24 14:10:42 UTC
Created attachment 110667 [details] [review]
Bug 23091: Move _Restore to Koha::Item store trigger
Comment 111 Martin Renvoize 2020-09-25 09:32:27 UTC
Created attachment 110720 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 112 Martin Renvoize 2020-09-25 09:32:33 UTC
Created attachment 110721 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.

Test plan:
1/ Read the code changes in t/db_dependant/Koha/CirculationRules and
t/db_dependant/Koha/IssuingRules and confirm they make sense.
2/ Run the updated tests and ensure they pass.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 113 Martin Renvoize 2020-09-25 09:32:50 UTC
Created attachment 110722 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds handing for the new values available for the lostreturn
policy settings.

* undef - Do nothing, leave fees and fines as they were at the point of
  lose.
* refund - Refund the lost item fee only
* charge - Refund the lost item fee and charge a fresh overdue fine
  dated for a return on the date the item is 'found'
* restore - Refund the lost item fee and restore the original overdue
  fine (dated for a 'return' on the date the item was 'lost'

Test plan
1/ apply patch
2/ updatedatabase, restart_all
3/ verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4/ verify WhenLostForgiveFine is set to "Forgive"
5/ verify circ rules include fines
6/ set Default lost item fee refund on return policy to "Refund lost item charge"
7/ create 4 overdue checkouts that will incur fines
8/ run fines.pl
9/ confirm 4 items checked out with accruing fines
10/ confirm all 4 items have a replacement price

Item 1
11/ mark the first item lost
12/ verify that fine is gone and lost fee has been charged
13/ check item in
14/ verify that lost fee is gone and overdue charge has not returned

Item 2
15/ set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16/ mark second item lost
17/ verify that fine is gone and lost fee has been charged
18/ check item in
19/ verify that lost fee is gone and a new overdue charge has been made

Item 3
20/ set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21/ mark third item lost
22/ verify that fine is gone and lost fee has been charged
23/ check item in
24/ verify that lost fee is gone and the old overdue charge has been restored

Item 4
25/ set Default lost item fee refund on return policy to "Leave lost item charge"
26/ mark fourth item lost
27/ verify that fine is gone and lost fee has been charged
28/ check item in
29/ verify that lost fee remains and the overdue charge is still gone

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 114 Martin Renvoize 2020-09-25 09:33:19 UTC
Created attachment 110723 [details] [review]
Bug 23091: Add tests for _RestoreOverdueForLostAndFound

This patch adds unit tests for the newly introduced
_RestoreOverdueForLostAndFound method in C4::Circulation.

Test plan
1/ Read the new tests added to t/db_dependent/Circuation.t to ensure
they make sense
2/ Run the tests and verify they pass
3/ Signoff

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 115 Martin Renvoize 2020-09-25 09:33:27 UTC
Created attachment 110724 [details] [review]
Bug 23091: Make relations more explicit

This patch simply makes the queries built for the ->debits and ->credits
relations in Koha::Account::Line more explicit.

Test plan
1/ Ensure tests continue to pass
2/ Run tests with DBIC_TRACE=1 DBIC_TRACE_PROFILE=console and note that
the related queries not include 'credit.' and 'debit.' as appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 116 Martin Renvoize 2020-09-25 09:33:34 UTC
Created attachment 110725 [details] [review]
Bug 23091: Add template handling for new messages

This patch adds handling for the display of the two new messages added
by this patchset in the returns screen.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 117 Martin Renvoize 2020-09-25 09:33:41 UTC
Created attachment 110726 [details] [review]
Bug 23091: Move _Restore to Koha::Item store trigger
Comment 118 Martin Renvoize 2020-09-25 09:33:47 UTC
Created attachment 110727 [details] [review]
Bug 23091: Unit Tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 119 Martin Renvoize 2020-09-25 09:35:09 UTC
OK, rebased and marginally re-worked after bug 18501.  Back to NQA :)
Comment 120 Martin Renvoize 2020-09-29 09:47:38 UTC
This would be a good use case for bug 26555.. but I'd really like to see this move first and I can followup to convert to using that bug once it's moved forwards.
Comment 121 Katrin Fischer 2020-10-11 13:02:33 UTC
Sorry, this no longer applies:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 23091: Outline of tests to be written
error: sha1 information is lacking or useless (t/db_dependent/Circulation.t).
error: could not build fake ancestor
Patch failed at 0001 Bug 23091: Outline of tests to be written
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-23091-Outline-of-tests-to-be-written-ErufPt.patch
Comment 122 Martin Renvoize 2020-10-12 10:51:11 UTC
Created attachment 111501 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 123 Martin Renvoize 2020-10-12 10:51:20 UTC
Created attachment 111502 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.

Test plan:
1/ Read the code changes in t/db_dependant/Koha/CirculationRules and
t/db_dependant/Koha/IssuingRules and confirm they make sense.
2/ Run the updated tests and ensure they pass.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 124 Martin Renvoize 2020-10-12 10:51:27 UTC
Created attachment 111503 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds handing for the new values available for the lostreturn
policy settings.

* undef - Do nothing, leave fees and fines as they were at the point of
  lose.
* refund - Refund the lost item fee only
* charge - Refund the lost item fee and charge a fresh overdue fine
  dated for a return on the date the item is 'found'
* restore - Refund the lost item fee and restore the original overdue
  fine (dated for a 'return' on the date the item was 'lost'

Test plan
1/ apply patch
2/ updatedatabase, restart_all
3/ verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4/ verify WhenLostForgiveFine is set to "Forgive"
5/ verify circ rules include fines
6/ set Default lost item fee refund on return policy to "Refund lost item charge"
7/ create 4 overdue checkouts that will incur fines
8/ run fines.pl
9/ confirm 4 items checked out with accruing fines
10/ confirm all 4 items have a replacement price

Item 1
11/ mark the first item lost
12/ verify that fine is gone and lost fee has been charged
13/ check item in
14/ verify that lost fee is gone and overdue charge has not returned

Item 2
15/ set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16/ mark second item lost
17/ verify that fine is gone and lost fee has been charged
18/ check item in
19/ verify that lost fee is gone and a new overdue charge has been made

Item 3
20/ set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21/ mark third item lost
22/ verify that fine is gone and lost fee has been charged
23/ check item in
24/ verify that lost fee is gone and the old overdue charge has been restored

Item 4
25/ set Default lost item fee refund on return policy to "Leave lost item charge"
26/ mark fourth item lost
27/ verify that fine is gone and lost fee has been charged
28/ check item in
29/ verify that lost fee remains and the overdue charge is still gone

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 125 Martin Renvoize 2020-10-12 10:51:33 UTC
Created attachment 111504 [details] [review]
Bug 23091: Add tests for _RestoreOverdueForLostAndFound

This patch adds unit tests for the newly introduced
_RestoreOverdueForLostAndFound method in C4::Circulation.

Test plan
1/ Read the new tests added to t/db_dependent/Circuation.t to ensure
they make sense
2/ Run the tests and verify they pass
3/ Signoff

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 126 Martin Renvoize 2020-10-12 10:51:41 UTC
Created attachment 111505 [details] [review]
Bug 23091: Make relations more explicit

This patch simply makes the queries built for the ->debits and ->credits
relations in Koha::Account::Line more explicit.

Test plan
1/ Ensure tests continue to pass
2/ Run tests with DBIC_TRACE=1 DBIC_TRACE_PROFILE=console and note that
the related queries not include 'credit.' and 'debit.' as appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 127 Martin Renvoize 2020-10-12 10:51:47 UTC
Created attachment 111506 [details] [review]
Bug 23091: Add template handling for new messages

This patch adds handling for the display of the two new messages added
by this patchset in the returns screen.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 128 Martin Renvoize 2020-10-12 10:51:53 UTC
Created attachment 111507 [details] [review]
Bug 23091: Move _Restore to Koha::Item store trigger
Comment 129 Martin Renvoize 2020-10-12 10:51:59 UTC
Created attachment 111508 [details] [review]
Bug 23091: Unit Tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 130 Martin Renvoize 2020-10-12 10:56:25 UTC
Getting some pretty bad rebase fatique here..

Rebased again.
Comment 131 Tomás Cohen Arazi 2020-10-16 20:10:25 UTC
Created attachment 111935 [details] [review]
Bug 23091: Add tests for \->credits and \->debits

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 132 Tomás Cohen Arazi 2020-10-16 20:10:40 UTC
Created attachment 111936 [details] [review]
Bug 23091: Simplify tests by turning off AccountAutoReconcile

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 133 Tomás Cohen Arazi 2020-10-16 20:12:41 UTC
Overall, it looks great. I've tested most of it. I only found a POD error I couldn't fix (besides separating the =item entries, there's a problem with =item 0 I think).

I will resume testing the functionality on monday.
Comment 134 Tomás Cohen Arazi 2020-10-19 11:55:25 UTC
Created attachment 111965 [details] [review]
Bug 23091: (QA follow-up) POD fixes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 135 Tomás Cohen Arazi 2020-10-19 13:06:57 UTC
Created attachment 111979 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 136 Tomás Cohen Arazi 2020-10-19 13:07:10 UTC
Created attachment 111980 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.

Test plan:
1/ Read the code changes in t/db_dependant/Koha/CirculationRules and
t/db_dependant/Koha/IssuingRules and confirm they make sense.
2/ Run the updated tests and ensure they pass.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 137 Tomás Cohen Arazi 2020-10-19 13:07:19 UTC
Created attachment 111981 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds handing for the new values available for the lostreturn
policy settings.

* undef - Do nothing, leave fees and fines as they were at the point of
  lose.
* refund - Refund the lost item fee only
* charge - Refund the lost item fee and charge a fresh overdue fine
  dated for a return on the date the item is 'found'
* restore - Refund the lost item fee and restore the original overdue
  fine (dated for a 'return' on the date the item was 'lost'

Test plan
1/ apply patch
2/ updatedatabase, restart_all
3/ verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4/ verify WhenLostForgiveFine is set to "Forgive"
5/ verify circ rules include fines
6/ set Default lost item fee refund on return policy to "Refund lost item charge"
7/ create 4 overdue checkouts that will incur fines
8/ run fines.pl
9/ confirm 4 items checked out with accruing fines
10/ confirm all 4 items have a replacement price

Item 1
11/ mark the first item lost
12/ verify that fine is gone and lost fee has been charged
13/ check item in
14/ verify that lost fee is gone and overdue charge has not returned

Item 2
15/ set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16/ mark second item lost
17/ verify that fine is gone and lost fee has been charged
18/ check item in
19/ verify that lost fee is gone and a new overdue charge has been made

Item 3
20/ set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21/ mark third item lost
22/ verify that fine is gone and lost fee has been charged
23/ check item in
24/ verify that lost fee is gone and the old overdue charge has been restored

Item 4
25/ set Default lost item fee refund on return policy to "Leave lost item charge"
26/ mark fourth item lost
27/ verify that fine is gone and lost fee has been charged
28/ check item in
29/ verify that lost fee remains and the overdue charge is still gone

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 138 Tomás Cohen Arazi 2020-10-19 13:07:26 UTC
Created attachment 111982 [details] [review]
Bug 23091: Add tests for _RestoreOverdueForLostAndFound

This patch adds unit tests for the newly introduced
_RestoreOverdueForLostAndFound method in C4::Circulation.

Test plan
1/ Read the new tests added to t/db_dependent/Circuation.t to ensure
they make sense
2/ Run the tests and verify they pass
3/ Signoff

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 139 Tomás Cohen Arazi 2020-10-19 13:07:40 UTC
Created attachment 111983 [details] [review]
Bug 23091: Make relations more explicit

This patch simply makes the queries built for the ->debits and ->credits
relations in Koha::Account::Line more explicit.

Test plan
1/ Ensure tests continue to pass
2/ Run tests with DBIC_TRACE=1 DBIC_TRACE_PROFILE=console and note that
the related queries not include 'credit.' and 'debit.' as appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 140 Tomás Cohen Arazi 2020-10-19 13:07:53 UTC
Created attachment 111984 [details] [review]
Bug 23091: Add template handling for new messages

This patch adds handling for the display of the two new messages added
by this patchset in the returns screen.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 141 Tomás Cohen Arazi 2020-10-19 13:08:03 UTC
Created attachment 111985 [details] [review]
Bug 23091: Move _Restore to Koha::Item store trigger

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 142 Tomás Cohen Arazi 2020-10-19 13:08:13 UTC
Created attachment 111986 [details] [review]
Bug 23091: Unit Tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 143 Tomás Cohen Arazi 2020-10-19 13:08:25 UTC
Created attachment 111987 [details] [review]
Bug 23091: Add tests for \->credits and \->debits

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 144 Tomás Cohen Arazi 2020-10-19 13:08:42 UTC
Created attachment 111988 [details] [review]
Bug 23091: Simplify tests by turning off AccountAutoReconcile

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 145 Tomás Cohen Arazi 2020-10-19 13:08:54 UTC
Created attachment 111989 [details] [review]
Bug 23091: (QA follow-up) POD fixes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 146 Jonathan Druart 2020-11-04 12:25:15 UTC
Applying: Bug 23091: Move _Restore to Koha::Item store trigger
error: sha1 information is lacking or useless (C4/Circulation.pm).

Please rebase.
Comment 147 Martin Renvoize 2020-11-05 11:15:21 UTC
Created attachment 113081 [details] [review]
Bug 23091: Database update

This patch updates the 'refund' circulation rule to be called
'lostreturn' and populates the value with 'refund' where appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 148 Martin Renvoize 2020-11-05 11:15:25 UTC
Created attachment 113082 [details] [review]
Bug 23091: Update unit test

This patch updates the unit test for get_lostreturn_policy to reflect
the rule_name and return code changes.

Test plan:
1/ Read the code changes in t/db_dependant/Koha/CirculationRules and
t/db_dependant/Koha/IssuingRules and confirm they make sense.
2/ Run the updated tests and ensure they pass.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 149 Martin Renvoize 2020-11-05 11:15:30 UTC
Created attachment 113083 [details] [review]
Bug 23091: Add handling for new lostreturn rules

This patch adds handing for the new values available for the lostreturn
policy settings.

* undef - Do nothing, leave fees and fines as they were at the point of
  lose.
* refund - Refund the lost item fee only
* charge - Refund the lost item fee and charge a fresh overdue fine
  dated for a return on the date the item is 'found'
* restore - Refund the lost item fee and restore the original overdue
  fine (dated for a 'return' on the date the item was 'lost'

Test plan
1/ apply patch
2/ updatedatabase, restart_all
3/ verify finesmode and CalculateFinesOnReturn and WhenLostChargeReplacementFee are on
4/ verify WhenLostForgiveFine is set to "Forgive"
5/ verify circ rules include fines
6/ set Default lost item fee refund on return policy to "Refund lost item charge"
7/ create 4 overdue checkouts that will incur fines
8/ run fines.pl
9/ confirm 4 items checked out with accruing fines
10/ confirm all 4 items have a replacement price

Item 1
11/ mark the first item lost
12/ verify that fine is gone and lost fee has been charged
13/ check item in
14/ verify that lost fee is gone and overdue charge has not returned

Item 2
15/ set Default lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
16/ mark second item lost
17/ verify that fine is gone and lost fee has been charged
18/ check item in
19/ verify that lost fee is gone and a new overdue charge has been made

Item 3
20/ set Default lost item fee refund on return policy to "Refund lost item charge and restore overdue fine"
21/ mark third item lost
22/ verify that fine is gone and lost fee has been charged
23/ check item in
24/ verify that lost fee is gone and the old overdue charge has been restored

Item 4
25/ set Default lost item fee refund on return policy to "Leave lost item charge"
26/ mark fourth item lost
27/ verify that fine is gone and lost fee has been charged
28/ check item in
29/ verify that lost fee remains and the overdue charge is still gone

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 150 Martin Renvoize 2020-11-05 11:15:34 UTC
Created attachment 113084 [details] [review]
Bug 23091: Add tests for _RestoreOverdueForLostAndFound

This patch adds unit tests for the newly introduced
_RestoreOverdueForLostAndFound method in C4::Circulation.

Test plan
1/ Read the new tests added to t/db_dependent/Circuation.t to ensure
they make sense
2/ Run the tests and verify they pass
3/ Signoff

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 151 Martin Renvoize 2020-11-05 11:15:39 UTC
Created attachment 113085 [details] [review]
Bug 23091: Make relations more explicit

This patch simply makes the queries built for the ->debits and ->credits
relations in Koha::Account::Line more explicit.

Test plan
1/ Ensure tests continue to pass
2/ Run tests with DBIC_TRACE=1 DBIC_TRACE_PROFILE=console and note that
the related queries not include 'credit.' and 'debit.' as appropriate.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 152 Martin Renvoize 2020-11-05 11:15:43 UTC
Created attachment 113086 [details] [review]
Bug 23091: Add template handling for new messages

This patch adds handling for the display of the two new messages added
by this patchset in the returns screen.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 153 Martin Renvoize 2020-11-05 11:15:48 UTC
Created attachment 113087 [details] [review]
Bug 23091: Move _Restore to Koha::Item store trigger

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 154 Martin Renvoize 2020-11-05 11:15:53 UTC
Created attachment 113088 [details] [review]
Bug 23091: Unit Tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 155 Martin Renvoize 2020-11-05 11:15:57 UTC
Created attachment 113089 [details] [review]
Bug 23091: Add tests for \->credits and \->debits

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 156 Martin Renvoize 2020-11-05 11:16:02 UTC
Created attachment 113090 [details] [review]
Bug 23091: Simplify tests by turning off AccountAutoReconcile

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 157 Martin Renvoize 2020-11-05 11:16:07 UTC
Created attachment 113091 [details] [review]
Bug 23091: (QA follow-up) POD fixes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 158 Martin Renvoize 2020-11-05 11:16:37 UTC
Rebased :)
Comment 159 Jonathan Druart 2020-11-06 14:55:53 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 160 Lucas Gass 2020-11-16 22:26:51 UTC
enhancement will not be backported to 20.05.x