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?
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.
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
This would be helpful for AspenCat libraries as well. They currently have to manually calculate and add back in the fine total. - Bob Bennhoff
Created attachment 96217 [details] [review] Bug 23091: Work In Progress
Where is the test plan? Or should the "Needs Signoff" be changed to some other status?
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.
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.
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.
Current state.. 'Refund, 'Refund and Charge' should both be working as expected at this point.. I am still working on 'Refund and Restore'.
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.
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.
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.
Created attachment 105547 [details] [review] Bug 23091: Add handling for restore case This patch adds handling for the 'restore' case of lostreturn policy.
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.
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
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?
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.
(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.
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.
Final patch corrects the error Andrew was running into.. the wording of the output messages could perhaps be improved; I am open to suggestions ;)
(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.
> 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.
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
(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?
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.
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.
Found it.. back to NSO
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?
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
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.
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.
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
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.
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
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.
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 :(
Found a bug.. setting to assigned whilst I work through it
(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?
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?
> 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.
(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.)
(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
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.
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.
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
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.
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
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.
Created attachment 106912 [details] [review] Bug 23091: Outline of tests to be written
Back to NSO.. final patch is work in progress.. writing all the tests is going to be a fairly large undertaking :(
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.
Created attachment 106961 [details] [review] Bug 23091: (QA follow-up) Fix for UNRETURNED charge
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.
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.
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
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.
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
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.
Created attachment 107032 [details] [review] Bug 23091: Outline of tests to be written
Created attachment 107033 [details] [review] Bug 23091: (QA follow-up) Fix for UNRETURNED charge
Created attachment 107034 [details] [review] Bug 23091: (follow-up) Remove borrower requirement for restore
Well... that was fun... hopefully that's now caught all edge cases... thankyou so much for the testing to date Andrew :)
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>
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>
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>
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>
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>
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>
Created attachment 107106 [details] [review] Bug 23091: Outline of tests to be written Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 107107 [details] [review] Bug 23091: (QA follow-up) Fix for UNRETURNED charge Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 107108 [details] [review] Bug 23091: (follow-up) Remove borrower requirement for restore Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Phew, thanks Martin!
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>
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>
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>
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>
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>
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>
Created attachment 107341 [details] [review] Bug 23091: Outline of tests to be written Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 107342 [details] [review] Bug 23091: (QA follow-up) Fix for UNRETURNED charge Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 107343 [details] [review] Bug 23091: (follow-up) Remove borrower requirement for restore Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Rebased for bug 20815
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.
Delayed.. hopefully get back to this tomorrow.
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>
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>
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>
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>
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>
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>
Created attachment 108724 [details] [review] Bug 23091: Outline of tests to be written Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
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)
Patch no longer applies cleanly, please rebase! (should not be hard for someone getting the logic there)
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>
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>
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>
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>
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>
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>
Created attachment 109577 [details] [review] Bug 23091: Outline of tests to be written Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Last outstanding work is to write the outlined tests from the final patch attached. The patchset does, however, now apply again.
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
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>
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>
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>
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>
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>
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>
Created attachment 110666 [details] [review] Bug 23091: Outline of tests to be written Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 110667 [details] [review] Bug 23091: Move _Restore to Koha::Item store trigger
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>
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>
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>
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>
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>
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>
Created attachment 110726 [details] [review] Bug 23091: Move _Restore to Koha::Item store trigger
Created attachment 110727 [details] [review] Bug 23091: Unit Tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
OK, rebased and marginally re-worked after bug 18501. Back to NQA :)
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.
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
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>
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>
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>
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>
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>
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>
Created attachment 111507 [details] [review] Bug 23091: Move _Restore to Koha::Item store trigger
Created attachment 111508 [details] [review] Bug 23091: Unit Tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Getting some pretty bad rebase fatique here.. Rebased again.
Created attachment 111935 [details] [review] Bug 23091: Add tests for \->credits and \->debits Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 111936 [details] [review] Bug 23091: Simplify tests by turning off AccountAutoReconcile Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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.
Created attachment 111965 [details] [review] Bug 23091: (QA follow-up) POD fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
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>
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>
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>
Created attachment 111985 [details] [review] Bug 23091: Move _Restore to Koha::Item store trigger Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
Created attachment 111987 [details] [review] Bug 23091: Add tests for \->credits and \->debits Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 111988 [details] [review] Bug 23091: Simplify tests by turning off AccountAutoReconcile Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 111989 [details] [review] Bug 23091: (QA follow-up) POD fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Applying: Bug 23091: Move _Restore to Koha::Item store trigger error: sha1 information is lacking or useless (C4/Circulation.pm). Please rebase.
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>
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>
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>
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>
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>
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>
Created attachment 113087 [details] [review] Bug 23091: Move _Restore to Koha::Item store trigger Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
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>
Rebased :)
Pushed to master for 20.11, thanks to everybody involved!
enhancement will not be backported to 20.05.x