Hi, When you indicate a Processing fee and a replacement cost by default for a particular item type, these two fees are charge to users when an item is indicating as lost by the patron in koha. If you indicate that the item is now found, the replacement cost will be refund in the patron’s account if RefundLostOnReturnControl is activate. But, the processing fee is not refund in this situation. It would be nice to have the possibility to decide when the lost item is found that the processing fee by default will be refund ore not. It could be managed by a system preference or something like that. Test plan: -Activate WhenLostChargeReplacementFee -indicate a processing fee and a replacement cost by default for an item type. -check out an item with the same item type to a patron -Active RefundLostOnReturnControl -mark this item as lost by the patron in koha -see that the processing fee and the replacement cost are charge to the patron. -Mark the item as found by a check in -See that the replacement cost is refund and not the Processing fee. It could be a system preference in the same way that WhenLostChargeReplacementFee but for the processing fee. Regards, Patrick.
+1 Many of the libraries who use processing fees would like it to be automatically refunded when the lost/billed item is returned.
Just asked about this on Slack, you can add PCCLD to the list of libraries that would like this enhancement.
Agreed - this would be useful for a lot of libraries!
Agreed, this would be very useful. I received another ask for this today.
We have also run into this problem and would like to see the processing fee be able to be refunded when a lost item is returned. It would save staff some time.
We've received sponsorship to write a fix for this, will aim to have that attached in the next week.
Created attachment 141627 [details] [review] Bug 23012: Database updates for PROCESSING_FOUND account credit type
Created attachment 141628 [details] [review] Bug 23012: Show the PROCESSING_FOUND account credit type
Created attachment 141629 [details] [review] Bug 23012: Set policy for lost item processing fee
Created attachment 141630 [details] [review] Bug 23012: [WIP] Mark processing fee as found when lost item found
Still working on this, attaching progress here to pick up tomorrow
Created attachment 142046 [details] [review] Bug 23012: Database updates for PROCESSING_FOUND account credit type
Created attachment 142047 [details] [review] Bug 23012: Show the PROCESSING_FOUND account credit type
Created attachment 142048 [details] [review] Bug 23012: Set policy for lost item processing fee
Created attachment 142049 [details] [review] Bug 23012: Apply processing fee return policy when lost item found TEST PLAN TO COME
Created attachment 142113 [details] [review] Bug 23012: Apply processing fee return policy when lost item found This enhancement gives the ability to set a policy for the lost item processing fee that may get charged additional to the lost item replacement cost. The processing fee can be: - refunded - refunded if unpaid - kept To test: Set-up 1. Find an item, Item A. Go to Administration -> Item types and edit the item type for Item A. Add a default replacement cost and a processing fee and Save. 2. Go to Administration -> system preferences and set the following: - WhenLostChargeReplacementFee: Charge - BlockReturnOfLostItems: Don't block 3. Scroll down to the default lost item fee refund on return policy. Set the refund lost item replacement fee policy to 'refund lost item charge'. 4. Edit Item A and set a replacement cost. Reproduce 5. Check out Item A to Patron A. 6. Click the barcode to view Item A's information. Edit Item A and set the Lost status to 'lost'. 7. Go back to Patron A's checkouts. The item should now be checked in with two new charges applied - a lost item fee (the item's replacement cost) and a lost item processing fee (set in item types). 8. Check in Item A to mark it as found. 9. Go back to Patron A's account. Notice the lost item fee has been refunded, but the processing fee remains. 10. Manually pay or write off the processing fee. This enhancement removes the need for this manual step. 11. Apply the patch and restart services Test with lost item - refund 12. Go to Administration -> circulation and fines rules. Scroll down to the default lost item fee refund on return policy. Notice there is now a refund lost item processing fee policy. Set this to 'refund lost item processing charge'. 13. Repeat steps 6 to 9. 14. Go back to Patron A's account. Both the lost item fee and processing fee should have been refunded. 15. Repeat steps 6 to 8 (do not check it yet). 16. Go back to Patron A's account. Pay the processing fee. 17. Repeat step 9. 18. Go back to Patron A's account. Both the lost item fee and processing fee should have been refunded (you'll now be in a credit because the paid processing fee was also refunded). Test with lost item - refund_unpaid 19. Go to Administration -> circulation and fines rules. Scroll down to the default lost item fee refund on return policy. Notice there is now a refund lost item processing fee policy. Set this to 'refund lost item processing charge (only if unpaid)'. 20. Repeat steps 6 to 9. 21. Go back to Patron A's account. Both the lost item fee and processing fee should have been refunded. 22. Repeat steps 16 to 19. 23. Go back to Patron A's account. The lost item fee should have been refunded but not the processing fee, as this was already paid. Test with lost item - leave 24. Go to Administration -> circulation and fines rules. Scroll down to the default lost item fee refund on return policy. Notice there is now a refund lost item processing fee policy. Set this to 'leave lost item processing charge'. 25. Repeat steps 6 to 9. 26. Go back to Patron A's account. The lost item fee and processing fee should have been refunded but not the processing fee. Other tests 27. Confirm tests pass - t/db_dependent/Koha/Item.t - t/db_dependent/Koha/CirculationRules.t Sponsored-by: Auckland University of Technology
Created attachment 142123 [details] [review] Bug 23012: Database updates for PROCESSING_FOUND account credit type Signed-off-by: David Nind <david@davidnind.com>
Created attachment 142124 [details] [review] Bug 23012: Show the PROCESSING_FOUND account credit type Signed-off-by: David Nind <david@davidnind.com>
Created attachment 142125 [details] [review] Bug 23012: Set policy for lost item processing fee Signed-off-by: David Nind <david@davidnind.com>
Created attachment 142126 [details] [review] Bug 23012: Apply processing fee return policy when lost item found This enhancement gives the ability to set a policy for the lost item processing fee that may get charged additional to the lost item replacement cost. The processing fee can be: - refunded - refunded if unpaid - kept To test: Set-up 1. Find an item, Item A. Go to Administration -> Item types and edit the item type for Item A. Add a default replacement cost and a processing fee and Save. 2. Go to Administration -> system preferences and set the following: - WhenLostChargeReplacementFee: Charge - BlockReturnOfLostItems: Don't block 3. Scroll down to the default lost item fee refund on return policy. Set the refund lost item replacement fee policy to 'refund lost item charge'. 4. Edit Item A and set a replacement cost. Reproduce 5. Check out Item A to Patron A. 6. Click the barcode to view Item A's information. Edit Item A and set the Lost status to 'lost'. 7. Go back to Patron A's checkouts. The item should now be checked in with two new charges applied - a lost item fee (the item's replacement cost) and a lost item processing fee (set in item types). 8. Check in Item A to mark it as found. 9. Go back to Patron A's account. Notice the lost item fee has been refunded, but the processing fee remains. 10. Manually pay or write off the processing fee. This enhancement removes the need for this manual step. 11. Apply the patch and restart services Test with lost item - refund 12. Go to Administration -> circulation and fines rules. Scroll down to the default lost item fee refund on return policy. Notice there is now a refund lost item processing fee policy. Set this to 'refund lost item processing charge'. 13. Repeat steps 6 to 9. 14. Go back to Patron A's account. Both the lost item fee and processing fee should have been refunded. 15. Repeat steps 6 to 8 (do not check it yet). 16. Go back to Patron A's account. Pay the processing fee. 17. Repeat step 9. 18. Go back to Patron A's account. Both the lost item fee and processing fee should have been refunded (you'll now be in a credit because the paid processing fee was also refunded). Test with lost item - refund_unpaid 19. Go to Administration -> circulation and fines rules. Scroll down to the default lost item fee refund on return policy. Notice there is now a refund lost item processing fee policy. Set this to 'refund lost item processing charge (only if unpaid)'. 20. Repeat steps 6 to 9. 21. Go back to Patron A's account. Both the lost item fee and processing fee should have been refunded. 22. Repeat steps 16 to 19. 23. Go back to Patron A's account. The lost item fee should have been refunded but not the processing fee, as this was already paid. Test with lost item - leave 24. Go to Administration -> circulation and fines rules. Scroll down to the default lost item fee refund on return policy. Notice there is now a refund lost item processing fee policy. Set this to 'leave lost item processing charge'. 25. Repeat steps 6 to 9. 26. Go back to Patron A's account. The lost item fee and processing fee should have been refunded but not the processing fee. Other tests 27. Confirm tests pass - t/db_dependent/Koha/Item.t - t/db_dependent/Koha/CirculationRules.t Sponsored-by: Auckland University of Technology Signed-off-by: David Nind <david@davidnind.com>
Testing notes (using koha-testing-docker): - Managed to work through the test plan OK. - Step 2: these are the default settings for KTD with the sample data, so no change to system preferences required. - Step 11: updatedatabase required. - Steps 13 + 15 + 20 + 22 + 25: I think a couple of the repeat steps X to should be steps 5 to X? - Step 26 should be: the lost item fee should have been refunded but not the processing fee (as expected with the 'refund lost item processing fee' policy set to 'leave lost item processing charge').
Created attachment 142481 [details] [review] Bug 23012: Database updates for PROCESSING_FOUND account credit type Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 142482 [details] [review] Bug 23012: Show the PROCESSING_FOUND account credit type Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 142483 [details] [review] Bug 23012: Set policy for lost item processing fee Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 142484 [details] [review] Bug 23012: Apply processing fee return policy when lost item found This enhancement gives the ability to set a policy for the lost item processing fee that may get charged additional to the lost item replacement cost. The processing fee can be: - refunded - refunded if unpaid - kept To test: Set-up 1. Find an item, Item A. Go to Administration -> Item types and edit the item type for Item A. Add a default replacement cost and a processing fee and Save. 2. Go to Administration -> system preferences and set the following: - WhenLostChargeReplacementFee: Charge - BlockReturnOfLostItems: Don't block 3. Scroll down to the default lost item fee refund on return policy. Set the refund lost item replacement fee policy to 'refund lost item charge'. 4. Edit Item A and set a replacement cost. Reproduce 5. Check out Item A to Patron A. 6. Click the barcode to view Item A's information. Edit Item A and set the Lost status to 'lost'. 7. Go back to Patron A's checkouts. The item should now be checked in with two new charges applied - a lost item fee (the item's replacement cost) and a lost item processing fee (set in item types). 8. Check in Item A to mark it as found. 9. Go back to Patron A's account. Notice the lost item fee has been refunded, but the processing fee remains. 10. Manually pay or write off the processing fee. This enhancement removes the need for this manual step. 11. Apply the patch and restart services Test with lost item - refund 12. Go to Administration -> circulation and fines rules. Scroll down to the default lost item fee refund on return policy. Notice there is now a refund lost item processing fee policy. Set this to 'refund lost item processing charge'. 13. Repeat steps 6 to 9. 14. Go back to Patron A's account. Both the lost item fee and processing fee should have been refunded. 15. Repeat steps 6 to 8 (do not check it yet). 16. Go back to Patron A's account. Pay the processing fee. 17. Repeat step 9. 18. Go back to Patron A's account. Both the lost item fee and processing fee should have been refunded (you'll now be in a credit because the paid processing fee was also refunded). Test with lost item - refund_unpaid 19. Go to Administration -> circulation and fines rules. Scroll down to the default lost item fee refund on return policy. Notice there is now a refund lost item processing fee policy. Set this to 'refund lost item processing charge (only if unpaid)'. 20. Repeat steps 6 to 9. 21. Go back to Patron A's account. Both the lost item fee and processing fee should have been refunded. 22. Repeat steps 16 to 19. 23. Go back to Patron A's account. The lost item fee should have been refunded but not the processing fee, as this was already paid. Test with lost item - leave 24. Go to Administration -> circulation and fines rules. Scroll down to the default lost item fee refund on return policy. Notice there is now a refund lost item processing fee policy. Set this to 'leave lost item processing charge'. 25. Repeat steps 6 to 9. 26. Go back to Patron A's account. The lost item fee and processing fee should have been refunded but not the processing fee. Other tests 27. Confirm tests pass - t/db_dependent/Koha/Item.t - t/db_dependent/Koha/CirculationRules.t Sponsored-by: Auckland University of Technology Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 142485 [details] [review] Bug 23012: (QA follow-up) Combine method to get both values The code for get_processingreturn_policy was very similar to get_lostreturn_policy. Combining the two methods allows for use of get_effective_rules which uses get_effective_rule_value which is cached. This should reduce lines of code and improve performance Tests updated and adjusted as well Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 142486 [details] [review] Bug 23012: (QA follow-up) No need to create and delete rules for branch The tests create a new branch to make sure one exists without rules. We then add a rule and delete it. We can simply create the new branch and never assign a rule Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 142487 [details] [review] Bug 23012: (QA follow-up) Add exec flag to db update Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 142575 [details] [review] Bug 23012: get_lostreturn_policy returns a scalar On commit 4d994773f83468b561896939f16131d0332d9cfc Koha::Item->_set_found_trigger gets the call to get_lostreturn_policy() changed so it expects a hashref instead of the scalar it returns. This patch adjusts it back. To test: 1. Run: $ kshell k$ prove t/db_dependent/Koha/Item.t => FAIL: Tests fail! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 22.11. Nice work everyone, thanks!
Tests are failing due to this: https://jenkins.koha-community.org/job/Koha_Master_D10/887/ Please provide patches for the tests. Best regards
Hmm, it feels like Nick's follow-up and mine are to blame.
I'm reverting this one for now. The reason is the failures in t/db_dependent/Koha/Items.t look problematic or at least complex enough for requiring a new review. With the new method and the changes made, the $processfee_amount of 20 is being charged when it shouldn't so at least the tests need to be adjusted to clear the whole scenario we are testing. I noticed that Nick's follow-up merging the methods introduced a failure in t/db_dependent/Koha/Item.t that I fixed by breaking things as well. Remarkably, with my broken follow-up nothing changed regarding the tests results... I'm adding the rel_22_05_candidate so this dev has the chance to get in beyond the soft freeze, but please hurry. We can go without Nick's little refactoring for now BTW, and think about it later. Looking forward to your comments and follow-ups.
I'm reverting it tomorrow, if you provide a fix by tomorrow, I won't. It still has the chance to get in either way.
Created attachment 142652 [details] [review] Bug 23012: Fix t/db_dependent/Koha/Item.t With the follow-up changes, the return value is no longer a scalar, but a hashref, but the tests weren't updated accordingly. This patch fixes this situation. To test: 1. Run: $ kshell k$ prove t/db_dependent/Koha/Item.t => FAIL: Tests fail 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Can someone please let me know what work needs to be done here? I wrote the initial patches but I understand the follow-ups caused test failures? A bit hard to follow the commentary :)
TL;DR This patch set broke t/db_dependent/Koha/Items.t Please fix it. I blamed the follow-ups, but that was incorrect as far as I can tell. When you fix it, please verify the Circulation* tests still pass.
I will fix this tomorrow!
Created attachment 142693 [details] [review] Bug 23012: (follow-up) Fix t/db_dependent/Koha/Items.t
I believe the latest patch addresses the Circulation.t tests as well. For anyone testing, only the bottom 2 patches need to be applied.
(In reply to Aleisha Amohia from comment #40) > I believe the latest patch addresses the Circulation.t tests as well. Thank you! Will review tomorrow
This is looking better now. The changes make sense as well. But now t/db_dependent/Koha/Item.t is failing for me because the third message is not being added. Please check.
Created attachment 142736 [details] [review] Bug 23012: Fix t/db_dependent/Koha/Item.t With the follow-up changes, the return value is no longer a scalar, but a hashref, but the tests weren't updated accordingly. This patch fixes this situation. To test: 1. Run: $ kshell k$ prove t/db_dependent/Koha/Item.t => FAIL: Tests fail 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 142737 [details] [review] Bug 23012: (follow-up) Fix t/db_dependent/Koha/Items.t
(In reply to Tomás Cohen Arazi from comment #42) > This is looking better now. The changes make sense as well. But now > > t/db_dependent/Koha/Item.t is failing for me because the third message is > not being added. Please check. Squashed the fix into your patch.
Patches no longer applies. Just applying the last two patches to the current master makes the tests pass for t/db_dependent/Koha/Items.t - before applying any patches, the tests were failing. git bz apply 23012 Bug 23012 - Possibility to mark processing fee by default refund when item is found 142481 - Bug 23012: Database updates for PROCESSING_FOUND account credit type 142482 - Bug 23012: Show the PROCESSING_FOUND account credit type 142483 - Bug 23012: Set policy for lost item processing fee 142484 - Bug 23012: Apply processing fee return policy when lost item found 142485 - Bug 23012: (QA follow-up) Combine method to get both values 142486 - Bug 23012: (QA follow-up) No need to create and delete rules for branch 142487 - Bug 23012: (QA follow-up) Add exec flag to db update 142736 - Bug 23012: Fix t/db_dependent/Koha/Item.t 142737 - Bug 23012: (follow-up) Fix t/db_dependent/Koha/Items.t Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 23012: Database updates for PROCESSING_FOUND account credit type Using index info to reconstruct a base tree... M installer/data/mysql/mandatory/account_credit_types.sql Falling back to patching base and 3-way merge... Applying: Bug 23012: Show the PROCESSING_FOUND account credit type Using index info to reconstruct a base tree... M Koha/Account.pm M api/v1/swagger/definitions/patron_account_credit.yaml M koha-tmpl/intranet-tmpl/prog/en/includes/accounts.inc M koha-tmpl/opac-tmpl/bootstrap/en/includes/accounts.inc Falling back to patching base and 3-way merge... No changes -- Patch already applied. Applying: Bug 23012: Set policy for lost item processing fee Using index info to reconstruct a base tree... M Koha/CirculationRules.pm M admin/smart-rules.pl M koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt M t/db_dependent/Koha/CirculationRules.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Koha/CirculationRules.t Auto-merging Koha/CirculationRules.pm Applying: Bug 23012: Apply processing fee return policy when lost item found Using index info to reconstruct a base tree... M C4/Circulation.pm M Koha/Item.pm M circ/returns.pl M koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt M t/db_dependent/Koha/Item.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Koha/Item.t Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt Auto-merging Koha/Item.pm CONFLICT (content): Merge conflict in Koha/Item.pm Auto-merging C4/Circulation.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 23012: Apply processing fee return policy when lost item found
David, you needed to: $ git bz apply 23012 and delete all commits but the last two. You were trying to apply patches that are already in master. I'm already on this one, please move to another ones!
(In reply to Aleisha Amohia from comment #45) > (In reply to Tomás Cohen Arazi from comment #42) > > This is looking better now. The changes make sense as well. But now > > > > t/db_dependent/Koha/Item.t is failing for me because the third message is > > not being added. Please check. > > Squashed the fix into your patch. Thank you, Aleisha. Well done!
Enhancement will not be backported to 22.05.x series
This should have had a default value inserted in the DB to maintain the previous behavior. This has ruined some of our users days to discover all fees credited suddenly after update, now their books don't balance.
Here is the insert we did in all our clients' databases `insert into circulation_rules (rule_name, rule_value) values ("processingreturn", 0);` If the rule is not in the circulation_rules table, it automatically refunds.