Bug 23012 - Possibility to mark processing fee by default refund when item is found
Summary: Possibility to mark processing fee by default refund when item is found
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-29 16:06 UTC by Patrick Robitaille
Modified: 2024-01-18 22:08 UTC (History)
15 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact: Caroline Cyr La Rose
Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/667
Text to go in the release notes:
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
Version(s) released in:
22.11.00


Attachments
Bug 23012: Database updates for PROCESSING_FOUND account credit type (2.06 KB, patch)
2022-10-12 04:24 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23012: Show the PROCESSING_FOUND account credit type (2.74 KB, patch)
2022-10-12 04:24 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23012: Set policy for lost item processing fee (23.45 KB, patch)
2022-10-12 04:24 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23012: [WIP] Mark processing fee as found when lost item found (2.06 KB, patch)
2022-10-12 04:24 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23012: Database updates for PROCESSING_FOUND account credit type (2.06 KB, patch)
2022-10-18 04:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23012: Show the PROCESSING_FOUND account credit type (2.74 KB, patch)
2022-10-18 04:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23012: Set policy for lost item processing fee (18.19 KB, patch)
2022-10-18 04:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23012: Apply processing fee return policy when lost item found (10.25 KB, patch)
2022-10-18 04:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23012: Apply processing fee return policy when lost item found (13.38 KB, patch)
2022-10-18 22:49 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23012: Database updates for PROCESSING_FOUND account credit type (2.11 KB, patch)
2022-10-19 01:07 UTC, David Nind
Details | Diff | Splinter Review
Bug 23012: Show the PROCESSING_FOUND account credit type (2.77 KB, patch)
2022-10-19 01:07 UTC, David Nind
Details | Diff | Splinter Review
Bug 23012: Set policy for lost item processing fee (18.23 KB, patch)
2022-10-19 01:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 23012: Apply processing fee return policy when lost item found (13.42 KB, patch)
2022-10-19 01:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 23012: Database updates for PROCESSING_FOUND account credit type (2.16 KB, patch)
2022-10-24 11:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23012: Show the PROCESSING_FOUND account credit type (2.83 KB, patch)
2022-10-24 11:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23012: Set policy for lost item processing fee (18.28 KB, patch)
2022-10-24 11:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23012: Apply processing fee return policy when lost item found (13.48 KB, patch)
2022-10-24 11:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23012: (QA follow-up) Combine method to get both values (18.95 KB, patch)
2022-10-24 11:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23012: (QA follow-up) No need to create and delete rules for branch (2.66 KB, patch)
2022-10-24 11:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23012: (QA follow-up) Add exec flag to db update (777 bytes, patch)
2022-10-24 11:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23012: get_lostreturn_policy returns a scalar (2.29 KB, patch)
2022-10-24 21:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23012: Fix t/db_dependent/Koha/Item.t (1.34 KB, patch)
2022-10-25 17:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23012: (follow-up) Fix t/db_dependent/Koha/Items.t (3.37 KB, patch)
2022-10-27 01:15 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23012: Fix t/db_dependent/Koha/Item.t (1.52 KB, patch)
2022-10-27 21:36 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23012: (follow-up) Fix t/db_dependent/Koha/Items.t (3.37 KB, patch)
2022-10-27 21:36 UTC, Aleisha Amohia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Robitaille 2019-05-29 16:06:22 UTC
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.
Comment 1 Donna 2020-11-30 21:28:49 UTC
+1 Many of the libraries who use processing fees would like it to be automatically refunded when the lost/billed item is returned.
Comment 2 Daniel Gaghan 2021-04-09 22:04:06 UTC
Just asked about this on Slack, you can add PCCLD to the list of libraries that would like this enhancement.
Comment 3 Sara Brown 2022-08-05 17:32:58 UTC
Agreed - this would be useful for a lot of libraries!
Comment 4 Todd Goatley 2022-08-10 20:10:09 UTC
Agreed, this would be very useful. I received another ask for this today.
Comment 5 Michael Spagnuolo 2022-09-14 17:33:34 UTC
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.
Comment 6 Aleisha Amohia 2022-09-22 04:39:17 UTC
We've received sponsorship to write a fix for this, will aim to have that attached in the next week.
Comment 7 Aleisha Amohia 2022-10-12 04:24:34 UTC
Created attachment 141627 [details] [review]
Bug 23012: Database updates for PROCESSING_FOUND account credit type
Comment 8 Aleisha Amohia 2022-10-12 04:24:39 UTC
Created attachment 141628 [details] [review]
Bug 23012: Show the PROCESSING_FOUND account credit type
Comment 9 Aleisha Amohia 2022-10-12 04:24:44 UTC
Created attachment 141629 [details] [review]
Bug 23012: Set policy for lost item processing fee
Comment 10 Aleisha Amohia 2022-10-12 04:24:48 UTC
Created attachment 141630 [details] [review]
Bug 23012: [WIP] Mark processing fee as found when lost item found
Comment 11 Aleisha Amohia 2022-10-12 04:25:15 UTC
Still working on this, attaching progress here to pick up tomorrow
Comment 12 Aleisha Amohia 2022-10-18 04:12:38 UTC
Created attachment 142046 [details] [review]
Bug 23012: Database updates for PROCESSING_FOUND account credit type
Comment 13 Aleisha Amohia 2022-10-18 04:12:43 UTC
Created attachment 142047 [details] [review]
Bug 23012: Show the PROCESSING_FOUND account credit type
Comment 14 Aleisha Amohia 2022-10-18 04:12:49 UTC
Created attachment 142048 [details] [review]
Bug 23012: Set policy for lost item processing fee
Comment 15 Aleisha Amohia 2022-10-18 04:12:54 UTC
Created attachment 142049 [details] [review]
Bug 23012: Apply processing fee return policy when lost item found

TEST PLAN TO COME
Comment 16 Aleisha Amohia 2022-10-18 22:49:37 UTC
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
Comment 17 David Nind 2022-10-19 01:07:53 UTC
Created attachment 142123 [details] [review]
Bug 23012: Database updates for PROCESSING_FOUND account credit type

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 David Nind 2022-10-19 01:07:59 UTC
Created attachment 142124 [details] [review]
Bug 23012: Show the PROCESSING_FOUND account credit type

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 David Nind 2022-10-19 01:08:06 UTC
Created attachment 142125 [details] [review]
Bug 23012: Set policy for lost item processing fee

Signed-off-by: David Nind <david@davidnind.com>
Comment 20 David Nind 2022-10-19 01:08:12 UTC
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>
Comment 21 David Nind 2022-10-19 01:15:50 UTC
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').
Comment 22 Nick Clemens 2022-10-24 11:23:25 UTC
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>
Comment 23 Nick Clemens 2022-10-24 11:23:30 UTC
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>
Comment 24 Nick Clemens 2022-10-24 11:23:35 UTC
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>
Comment 25 Nick Clemens 2022-10-24 11:23:40 UTC
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>
Comment 26 Nick Clemens 2022-10-24 11:23:47 UTC
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>
Comment 27 Nick Clemens 2022-10-24 11:23:53 UTC
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>
Comment 28 Nick Clemens 2022-10-24 11:23:58 UTC
Created attachment 142487 [details] [review]
Bug 23012: (QA follow-up) Add exec flag to db update

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Tomás Cohen Arazi 2022-10-24 21:43:17 UTC
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>
Comment 30 Tomás Cohen Arazi 2022-10-24 21:43:50 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 31 Tomás Cohen Arazi 2022-10-25 02:20:10 UTC
Tests are failing due to this:

https://jenkins.koha-community.org/job/Koha_Master_D10/887/

Please provide patches for the tests.
Best regards
Comment 32 Tomás Cohen Arazi 2022-10-25 17:15:48 UTC
Hmm, it feels like Nick's follow-up and mine are to blame.
Comment 33 Tomás Cohen Arazi 2022-10-25 17:34:51 UTC
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.
Comment 34 Tomás Cohen Arazi 2022-10-25 17:37:37 UTC
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.
Comment 35 Tomás Cohen Arazi 2022-10-25 17:58:34 UTC
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>
Comment 36 Aleisha Amohia 2022-10-25 20:15:15 UTC
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 :)
Comment 37 Tomás Cohen Arazi 2022-10-25 21:14:47 UTC
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.
Comment 38 Aleisha Amohia 2022-10-26 10:09:37 UTC
I will fix this tomorrow!
Comment 39 Aleisha Amohia 2022-10-27 01:15:12 UTC
Created attachment 142693 [details] [review]
Bug 23012: (follow-up) Fix t/db_dependent/Koha/Items.t
Comment 40 Aleisha Amohia 2022-10-27 01:15:50 UTC
I believe the latest patch addresses the Circulation.t tests as well.

For anyone testing, only the bottom 2 patches need to be applied.
Comment 41 Tomás Cohen Arazi 2022-10-27 01:19:58 UTC
(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
Comment 42 Tomás Cohen Arazi 2022-10-27 12:13:19 UTC
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.
Comment 43 Aleisha Amohia 2022-10-27 21:36:03 UTC
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>
Comment 44 Aleisha Amohia 2022-10-27 21:36:09 UTC
Created attachment 142737 [details] [review]
Bug 23012: (follow-up) Fix t/db_dependent/Koha/Items.t
Comment 45 Aleisha Amohia 2022-10-27 21:37:21 UTC
(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.
Comment 46 David Nind 2022-10-28 15:20:43 UTC
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
Comment 47 Tomás Cohen Arazi 2022-10-28 19:19:16 UTC
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!
Comment 48 Tomás Cohen Arazi 2022-10-28 19:20:16 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 49 Tomás Cohen Arazi 2022-10-28 19:20:57 UTC
(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!
Comment 50 Lucas Gass 2022-12-05 23:08:57 UTC
Enhancement will not be backported to 22.05.x series
Comment 51 Blou 2024-01-18 21:58:00 UTC
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.
Comment 52 Caroline Cyr La Rose 2024-01-18 22:08:58 UTC
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.