Description
Kyle M Hall (khall)
2018-02-21 16:03:54 UTC
Created attachment 72029 [details] [review] Bug 20262: Add ability to refund lost item fees without creating credits Some libraries handle refunding of paid lost fees at a financial office and not within Koha. To facilitate this, it would be good for Koha to have the option to not generate lost returned credits by skipping fully paid lost items, and only refunding the outstanding balance on partially paid lost fees. Test Plan: 1) Apply this patch 2) Set your lost item refund on return policies to 'Only if unpaid' 3) Mark an item lost, charging the lost fee 4) Return the item, a full refund should ocurr 5) Mark another item lost, charging the lost fee 6) Make a partial payment on this lost fee 7) Return the item 8) The remaining balance of that fee should be 0, but the patron should not recieve a credit for the already paid balance 8) Mark another item lost, charging the lost fee 9) Fully pay the lost fee 10) Return the item. The paid lost fee should be unaffected. Created attachment 72030 [details] [review] Bug 20262: Add ability to refund lost item fees without creating credits Some libraries handle refunding of paid lost fees at a financial office and not within Koha. To facilitate this, it would be good for Koha to have the option to not generate lost returned credits by skipping fully paid lost items, and only refunding the outstanding balance on partially paid lost fees. Test Plan: 1) Apply this patch 2) Set your lost item refund on return policies to 'Only if unpaid' 3) Mark an item lost, charging the lost fee 4) Return the item, a full refund should ocurr 5) Mark another item lost, charging the lost fee 6) Make a partial payment on this lost fee 7) Return the item 8) The remaining balance of that fee should be 0, but the patron should not recieve a credit for the already paid balance 8) Mark another item lost, charging the lost fee 9) Fully pay the lost fee 10) Return the item. The paid lost fee should be unaffected. Test Plan: 1) Apply this patch 2) Set your lost item refund on return policies to 'Only if unpaid' 3) Mark an item lost, charging the lost fee 4) Return the item, a full refund should ocurr 5) Mark another item lost, charging the lost fee 6) Make a partial payment on this lost fee 7) Return the item 8) The remaining balance of that fee should be 0, but the patron should not recieve a credit for the already paid balance 8) Mark another item lost, charging the lost fee 9) Fully pay the lost fee 10) Return the item. The paid lost fee should be unaffected. Notes: 4) If the patron has other fines, the lost item refund pays those first, as opposed to just paying the lost item charge. This could be confusing for staff, but IIRC, there's another bug related to how credits get applied? 8) Marked an item lost that created a 9.99 charge on patron's account. Paid 5.00 of it. Returned item. Patron's account was credited 9.99, creating a 5.00 credit. 9) Fully paid for item - credit still applied, resulting in a credit on the account. Created attachment 75470 [details] [review] Bug 20262: Add ability to refund lost item fees without creating credits Some libraries handle refunding of paid lost fees at a financial office and not within Koha. To facilitate this, it would be good for Koha to have the option to not generate lost returned credits by skipping fully paid lost items, and only refunding the outstanding balance on partially paid lost fees. Test Plan: 1) Apply this patch 2) Set your lost item refund on return policies to 'Only if unpaid' 3) Mark an item lost, charging the lost fee 4) Return the item, a full refund should ocurr 5) Mark another item lost, charging the lost fee 6) Make a partial payment on this lost fee 7) Return the item 8) The remaining balance of that fee should be 0, but the patron should not recieve a credit for the already paid balance 8) Mark another item lost, charging the lost fee 9) Fully pay the lost fee 10) Return the item. The paid lost fee should be unaffected. (In reply to Martha Fuerst from comment #3) > Test Plan: > Notes: > 4) If the patron has other fines, the lost item refund pays those first, as > opposed to just paying the lost item charge. This could be confusing for > staff, but IIRC, there's another bug related to how credits get applied? > 8) Marked an item lost that created a 9.99 charge on patron's account. Paid > 5.00 of it. Returned item. Patron's account was credited 9.99, creating a > 5.00 credit. > 9) Fully paid for item - credit still applied, resulting in a credit on the > account. Martha, it sounds like perhaps the circ rule with "Only if unpaid" did not get selected. Did you set *all* your rules to "Only if unpaid" to ensure you didn't accidentally change the wrong rule? The unit tests prove the functionality works so that's my best guess at what's happening for you. I've also added some more unit tests to prove partially paid lost fees don't cause a credit when the rule is applied. (In reply to Kyle M Hall from comment #5) > (In reply to Martha Fuerst from comment #3) > > Test Plan: > > Notes: > > 4) If the patron has other fines, the lost item refund pays those first, as > > opposed to just paying the lost item charge. This could be confusing for > > staff, but IIRC, there's another bug related to how credits get applied? > > 8) Marked an item lost that created a 9.99 charge on patron's account. Paid > > 5.00 of it. Returned item. Patron's account was credited 9.99, creating a > > 5.00 credit. > > 9) Fully paid for item - credit still applied, resulting in a credit on the > > account. > > Martha, it sounds like perhaps the circ rule with "Only if unpaid" did not > get selected. Did you set *all* your rules to "Only if unpaid" to ensure you > didn't accidentally change the wrong rule? The unit tests prove the > functionality works so that's my best guess at what's happening for you. > I've also added some more unit tests to prove partially paid lost fees don't > cause a credit when the rule is applied. I have it set as the default rule in our bug-squashing sandbox's Circ Rules table, with no other library-specific rules set. Is there something in sysprefs that might be fouling it up? BlockReturnofLostItems: Don't block RetundLostOnReturnControl: check-in library WhenLostChargeReplacementFee: Charge WhenLostForgiveFine: Don't forgive (In reply to Martha Fuerst from comment #6) > Is there something in sysprefs that might be fouling it up? That looks pretty good, are you sure you are triggering the rule by utilizing the branch chosen from RetundLostOnReturnControl? Basically, if the other refund lost item fee rule options work, then you can switch those rules to "only if unpaid" and it should work. If they do not work it is an issue with your test data. Created attachment 75503 [details] [review] Bug 20262: Add ability to refund lost item fees without creating credits Some libraries handle refunding of paid lost fees at a financial office and not within Koha. To facilitate this, it would be good for Koha to have the option to not generate lost returned credits by skipping fully paid lost items, and only refunding the outstanding balance on partially paid lost fees. Test Plan: 1) Apply this patch 2) Set your lost item refund on return policies to 'Only if unpaid' 3) Mark an item lost, charging the lost fee 4) Return the item, a full refund should ocurr 5) Mark another item lost, charging the lost fee 6) Make a partial payment on this lost fee 7) Return the item 8) The remaining balance of that fee should be 0, but the patron should not recieve a credit for the already paid balance 8) Mark another item lost, charging the lost fee 9) Fully pay the lost fee 10) Return the item. The paid lost fee should be unaffected. (In reply to Kyle M Hall from comment #7) > (In reply to Martha Fuerst from comment #6) > > Is there something in sysprefs that might be fouling it up? > > That looks pretty good, are you sure you are triggering the rule by > utilizing the branch chosen from RetundLostOnReturnControl? > > Basically, if the other refund lost item fee rule options work, then you can > switch those rules to "only if unpaid" and it should work. If they do not > work it is an issue with your test data. Gotcha. Tested again after you took a look at my settings (thanks Kyle!) - Test Plan: 1) Apply this patch 2) Set your lost item refund on return policies to 'Only if unpaid' 3) Mark an item lost, charging the lost fee 4) Return the item, a full refund should ocurr 5) Mark another item lost, charging the lost fee 6) Make a partial payment on this lost fee 7) Return the item 8) The remaining balance of that fee should be 0, but the patron should not recieve a credit for the already paid balance 8) Mark another item lost, charging the lost fee 9) Fully pay the lost fee 10) Return the item. The paid lost fee should be unaffected. Everything worked as it should - signing off! Created attachment 75532 [details] [review] Bug 20262: Add ability to refund lost item fees without creating credits Some libraries handle refunding of paid lost fees at a financial office and not within Koha. To facilitate this, it would be good for Koha to have the option to not generate lost returned credits by skipping fully paid lost items, and only refunding the outstanding balance on partially paid lost fees. Test Plan: 1) Apply this patch 2) Set your lost item refund on return policies to 'Only if unpaid' 3) Mark an item lost, charging the lost fee 4) Return the item, a full refund should ocurr 5) Mark another item lost, charging the lost fee 6) Make a partial payment on this lost fee 7) Return the item 8) The remaining balance of that fee should be 0, but the patron should not recieve a credit for the already paid balance 8) Mark another item lost, charging the lost fee 9) Fully pay the lost fee 10) Return the item. The paid lost fee should be unaffected. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martha Fuerst <mfuerst@hmcpl.org> Hi Kyle, code looks sensible, but I am having problems with the unit tests. Can you have a look please? Tested on a clean db (reset_all), passes on master, fails with the patch applied: # Subtest: _FixAccountForLostAndReturned 1..11 DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`accountlines`, CONSTRAINT `accountlines_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `accountlines` ( `accounttype`, `amount`, `amountoutstanding`, `itemnumber`) VALUES ( ?, ?, ?, ? )" with ParamValues: 0='F', 1=10, 2=10, 3='1041'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832. # No tests run! not ok 113 - No tests run for subtest "_FixAccountForLostAndReturned" This would really help us out with the lost item credits issues we are having at our library. Can we look into this again? Thanks! -Marti This fix would be SUPER helpful for our library as well. At our library we do not give any refunds because of the way our money goes through the city. Rhonda Kuiper. Just to add to the interest and need expressed by Martha and Rhonda: This is more than ever a necessity for us, as the recent changes to how Koha handles credits have made apparent how often refunds have been automatically generated for something already resolved without anyone catching it. Hoping this is in a release soon. Kyle, unfortunately a lot of the code changed in the meantime so this doesn't apply. I suggest you take a look at the PQA stuffs because there's still lots of moving pieces here (Koha::Account->add_debit, Koha::Account::Lin->adjust bugs specifically) Please add me to the list of squeaky wheels about this. We want to see this added ASAP. We would also be happy to see this bug applied as soon as possible. We are also still very interested! Hi! A lot of our libraries would love to see this added as the refunds for lost items create many headaches. Created attachment 94099 [details] [review] Bug 20262: Add ability to refund lost item fees without creating credits Some libraries handle refunding of paid lost fees at a financial office and not within Koha. To facilitate this, it would be good for Koha to have the option to not generate lost returned credits by skipping fully paid lost items, and only refunding the outstanding balance on partially paid lost fees. Test Plan: 1) Apply this patch 2) Set your lost item refund on return policies to 'Only if unpaid' 3) Mark an item lost, charging the lost fee 4) Return the item, a full refund should ocurr 5) Mark another item lost, charging the lost fee 6) Make a partial payment on this lost fee 7) Return the item 8) The remaining balance of that fee should be 0, but the patron should not recieve a credit for the already paid balance 8) Mark another item lost, charging the lost fee 9) Fully pay the lost fee 10) Return the item. The paid lost fee should be unaffected. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martha Fuerst <mfuerst@hmcpl.org> Kyle, I have tried to rebase the patch but I lost myself in the tests. Could you have a look? We're still interested in this! -Marti Fuerst Huntsville-Madison County Public Library Still interested - disparately wanting. Still interested - eagerly anticipating. Still interested - anxiously awaiting. Unexpectedly misspelling! Created attachment 127018 [details] [review] Bug 20262: Add ability to refund lost item fees without creating credits Some libraries handle refunding of paid lost fees at a financial office and not within Koha. To facilitate this, it would be good for Koha to have the option to not generate lost returned credits by skipping fully paid lost items, and only refunding the outstanding balance on partially paid lost fees. Test Plan: 1) Apply this patch 2) Set your lost item refund on return policies to 'Only if unpaid' 3) Mark an item lost, charging the lost fee 4) Return the item, a full refund should ocurr 5) Mark another item lost, charging the lost fee 6) Make a partial payment on this lost fee 7) Return the item 8) The remaining balance of that fee should be 0, but the patron should not recieve a credit for the already paid balance 8) Mark another item lost, charging the lost fee 9) Fully pay the lost fee 10) Return the item. The paid lost fee should be unaffected. Working on this, tests are in progress though since the tests in t/db_dependent/Circulation.t are crashing and failing with or without the patch, so need to work out those issues first before being able to properly run the tests, which has to be rewritten since so much has been refactored with regard to how lost item fees are handled. Created attachment 127285 [details] [review] Bug 20262: Add ability to refund lost item fees without creating credits Some libraries handle refunding of paid lost fees at a financial office and not within Koha. To facilitate this, it would be good for Koha to have the option to not generate lost returned credits by skipping fully paid lost items, and only refunding the outstanding balance on partially paid lost fees. Test Plan: 1) Apply this patch 2) Set your lost item refund on return policies to 'Only if unpaid' 3) Mark an item lost, charging the lost fee 4) Return the item, a full refund should occur 5) Mark another item lost, charging the lost fee 6) Make a partial payment on this lost fee 7) Return the item 8) The remaining balance of that fee should be 0, but the patron should not recieve a credit for the already paid balance 8) Mark another item lost, charging the lost fee 9) Fully pay the lost fee 10) Return the item. The paid lost fee should be unaffected. 11) Run tests in t/db_dependent/Circulation.t Now added tests. Created attachment 127286 [details] [review] Bug 20262: Add ability to refund lost item fees without creating credits Some libraries handle refunding of paid lost fees at a financial office and not within Koha. To facilitate this, it would be good for Koha to have the option to not generate lost returned credits by skipping fully paid lost items, and only refunding the outstanding balance on partially paid lost fees. Test Plan: 1) Apply this patch 2) Set your lost item refund on return policies to 'Only if unpaid' 3) Mark an item lost, charging the lost fee 4) Return the item, a full refund should occur 5) Mark another item lost, charging the lost fee 6) Make a partial payment on this lost fee 7) Return the item 8) The remaining balance of that fee should be 0, but the patron should not recieve a credit for the already paid balance 8) Mark another item lost, charging the lost fee 9) Fully pay the lost fee 10) Return the item. The paid lost fee should be unaffected. 11) Run tests in t/db_dependent/Circulation.t Created attachment 134555 [details] [review] Bug 20262: Add ability to refund lost item fees without creating credits Some libraries handle refunding of paid lost fees at a financial office and not within Koha. To facilitate this, it would be good for Koha to have the option to not generate lost returned credits by skipping fully paid lost items, and only refunding the outstanding balance on partially paid lost fees. Test Plan: 1) Apply this patch 2) Set your lost item refund on return policies to 'Only if unpaid' 3) Mark an item lost, charging the lost fee 4) Return the item, a full refund should occur 5) Mark another item lost, charging the lost fee 6) Make a partial payment on this lost fee 7) Return the item 8) The remaining balance of that fee should be 0, but the patron should not recieve a credit for the already paid balance 8) Mark another item lost, charging the lost fee 9) Fully pay the lost fee 10) Return the item. The paid lost fee should be unaffected. 11) Run tests in t/db_dependent/Circulation.t Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Works as expected. I also tried with partial and full write offs and it worked as expected there as well. I was not able to do step 11 as I was on the sandbox. I'm really looking forward to this! Lisette Hi, a bit scared of this one, but got stuck on the first steps. 1) QA tools have some minor complaints: FAIL Koha/Item.pm FAIL spelling wich ==> which OK koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt FAIL t/db_dependent/Circulation.t FAIL spelling iwth ==> with 2) Tests are failing prove t/db_dependent/Circulation.t t/db_dependent/Circulation.t .. 33/60 # Failed test 'The LOST amountoutstanding is the expected amount after partial payment of lost fee' # at t/db_dependent/Circulation.t line 3325. # got: '99' # expected: '60' # Failed test 'Account balance should be zero after returning item with lost fee when partial payment has been made' # at t/db_dependent/Circulation.t line 3343. # got: '-39' # expected: '0' # Failed test 'The LOST amountoutstanding is the expected amount after partial payment of lost fee' # at t/db_dependent/Circulation.t line 3385. # got: '99' # expected: '0' # Failed test 'Account balance is the expected amount after partial payment of lost fee' # at t/db_dependent/Circulation.t line 3390. # got: '-39' # expected: '0' # Failed test 'Account balance should be zero after returning item with lost fee when full payment has been made' # at t/db_dependent/Circulation.t line 3403. # got: '-138' # expected: '0' # Looks like you failed 5 tests of 21. # Failed test 'lostreturn | refund_unpaid' # at t/db_dependent/Circulation.t line 3408. # Looks like you failed 1 test of 5. # Failed test 'enh 23091 | Lost item return policies' # at t/db_dependent/Circulation.t line 3785. # Looks like you failed 1 test of 9. t/db_dependent/Circulation.t .. 34/60 # Failed test 'AddReturn | is_overdue' # at t/db_dependent/Circulation.t line 3786. t/db_dependent/Circulation.t .. 60/60 # Looks like you failed 1 test of 60. t/db_dependent/Circulation.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/60 subtests Test Summary Report ------------------- t/db_dependent/Circulation.t (Wstat: 256 Tests: 60 Failed: 1) Failed test: 34 Non-zero exit status: 1 Files=1, Tests=60, 68 wallclock secs ( 0.13 usr 0.01 sys + 49.52 cusr 5.11 csys = 54.77 CPU) Result: FAIL Created attachment 136006 [details] [review] Bug 20262: (QA followup) Fix spelling errors > 1) QA tools have some minor complaints: Fixed! > 2) Tests are failing Those tests are failing on master as well and appear to be unrelated to this patch. They passed for me on master, I had verified that Circulation.t passed without applying the patch. ... and these are all lost related patches? Argh. ... lost related _tests_. (In reply to Katrin Fischer from comment #37) > They passed for me on master, I had verified that Circulation.t passed > without applying the patch. It appears you are correct! I must have not had master checked out when I thought I did. Taking a look! Created attachment 136007 [details] [review] Bug 20262: (QA followup) Update unit tests for changes to payin_amount Created attachment 139543 [details] [review] Bug 20262: Add ability to refund lost item fees without creating credits Some libraries handle refunding of paid lost fees at a financial office and not within Koha. To facilitate this, it would be good for Koha to have the option to not generate lost returned credits by skipping fully paid lost items, and only refunding the outstanding balance on partially paid lost fees. Test Plan: 1) Apply this patch 2) Set your lost item refund on return policies to 'Only if unpaid' 3) Mark an item lost, charging the lost fee 4) Return the item, a full refund should occur 5) Mark another item lost, charging the lost fee 6) Make a partial payment on this lost fee 7) Return the item 8) The remaining balance of that fee should be 0, but the patron should not recieve a credit for the already paid balance 8) Mark another item lost, charging the lost fee 9) Fully pay the lost fee 10) Return the item. The paid lost fee should be unaffected. 11) Run tests in t/db_dependent/Circulation.t Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 139544 [details] [review] Bug 20262: (QA follow-up) Fix spelling errors Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 139545 [details] [review] Bug 20262: (QA follow-up) Update unit tests for changes to payin_amount Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master for 22.11. Nice work everyone, thanks! Enhancement will not be included in 22.05.x series |