Bug 27992 - When recording local use with statistical patron items are not checked in
Summary: When recording local use with statistical patron items are not checked in
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal with 10 votes (vote)
Assignee: Lucas Gass
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 35751 35840
  Show dependency treegraph
 
Reported: 2021-03-18 19:21 UTC by Donna
Modified: 2024-01-31 15:08 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.06


Attachments
Bug 27992: Call AddReturn if borrower is statistical patron (1.21 KB, patch)
2021-10-25 21:24 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 27992: Call AddReturn if borrower is statistical patron (1.28 KB, patch)
2021-11-01 17:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27992: Unit test (2.06 KB, patch)
2021-12-16 16:04 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 27992: Call AddReturn if borrower is statistical patron (1.33 KB, patch)
2021-12-16 16:04 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 27992: Call AddReturn when checking in localuse item (771 bytes, patch)
2023-02-02 22:48 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 27992: Call AddReturn on stats patron (3.96 KB, patch)
2023-10-27 23:37 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 27992: Call AddReturn on stats patron (5.75 KB, patch)
2023-10-30 15:00 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 27992: Unit test (1.74 KB, patch)
2023-10-30 16:46 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 27992: Call AddReturn on stats patron (5.82 KB, patch)
2023-11-01 21:18 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 27992: Unit test (1.80 KB, patch)
2023-11-01 21:18 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 27992: Call AddReturn on stats patron (5.87 KB, patch)
2023-11-05 12:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27992: Unit test (1.86 KB, patch)
2023-11-05 12:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27992: (QA follow-up) Perltidy (2.78 KB, patch)
2023-11-05 12:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27992: (QA follow-up) Terminology: returned -> checked in (1.33 KB, patch)
2023-11-05 12:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27992: [23.05] (follow-up) Fix unit test with unblessed patron (1.03 KB, patch)
2023-11-13 19:22 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Donna 2021-03-18 19:21:58 UTC
When recording local use using a statistical patron, the Lost status updates, but if an item is checked out, it does not get checked in. 

To replicate:
1. Create a Statistical Patron
2. Check out an item to the Stat Patron, that is checked out to another user
3. See that the local use is recorded, but the item does not get checked in
4. Check out an item that has a lost status and note that the local use is recorded, and the lost status is cleared

In this situation, the item should be checked in/available status updated when it is "checked out" to the statistical patron.
Comment 1 Katrin Fischer 2021-03-23 07:47:23 UTC
Hi Donna, can you explain the use case here a bit? Why is the item checked out at the time of local use?
Comment 2 Donna 2021-03-23 15:23:52 UTC
Typically, the items are not going to be checked out, however, items that are checked out sometimes make their way back to the shelf without being checked in. 
Just like with inventory, when an item is scanned in this scenario, the status should be updated/checked in.
Comment 3 Katrin Fischer 2021-06-04 09:55:22 UTC
(In reply to Donna from comment #2)
> Typically, the items are not going to be checked out, however, items that
> are checked out sometimes make their way back to the shelf without being
> checked in. 
> Just like with inventory, when an item is scanned in this scenario, the
> status should be updated/checked in.

Thx, I agree that it should be checked in.
Comment 4 Andrew Fuerste-Henry 2021-10-21 14:35:19 UTC
+1 on this. A local use scan assumes the item is in the library and therefore any current checkout on it should be ended.
Comment 5 Lucas Gass 2021-10-25 21:24:59 UTC
Created attachment 126865 [details] [review]
Bug 27992: Call AddReturn if borrower is statistical patron

To test:
1. Create a Statistical Patron
2. Check out an item to the Stat Patron, that is checked out to another user
3. See that the local use is recorded, but the item does not get checked in
4. Check out an item that has a lost status and note that the local use is recorded, and the lost status is cleared
5. Item is NOT checked in
6. Apply patch
7. Repeat steps 2 - 4.
8. Item is checked in and now seen as available
Comment 6 Donna 2021-10-29 17:32:00 UTC
Works as expected! Thanks!
Comment 7 Martin Renvoize 2021-11-01 17:33:56 UTC
Created attachment 127189 [details] [review]
Bug 27992: Call AddReturn if borrower is statistical patron

To test:
1. Create a Statistical Patron
2. Check out an item to the Stat Patron, that is checked out to another user
3. See that the local use is recorded, but the item does not get checked in
4. Check out an item that has a lost status and note that the local use is recorded, and the lost status is cleared
5. Item is NOT checked in
6. Apply patch
7. Repeat steps 2 - 4.
8. Item is checked in and now seen as available

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2021-11-01 17:35:41 UTC
Works as described, no obvious regressions and the QA scripts are happy.

I'm not sure how any outstanding fines, lost item charges should be handled in this case.. but I feel that could be handled as a follow-up bug if we discover a need.

Passing QA
Comment 9 Jonathan Druart 2021-11-02 15:06:28 UTC
Tests are missing.
Comment 10 Nick Clemens (kidclamp) 2021-12-16 16:04:53 UTC
Created attachment 128633 [details] [review]
Bug 27992: Unit test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Nick Clemens (kidclamp) 2021-12-16 16:04:56 UTC
Created attachment 128634 [details] [review]
Bug 27992: Call AddReturn if borrower is statistical patron

To test:
1. Create a Statistical Patron
2. Check out an item to the Stat Patron, that is checked out to another user
3. See that the local use is recorded, but the item does not get checked in
4. Check out an item that has a lost status and note that the local use is recorded, and the lost status is cleared
5. Item is NOT checked in
6. Apply patch
7. Repeat steps 2 - 4.
8. Item is checked in and now seen as available

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Katrin Fischer 2021-12-19 12:41:24 UTC
This works as intended as far as I can tell, but I have something to clarify:

In marked my item lost using the 'claims returned' feature. With this change the item will be returned and the claim remains unresolved. Is this an issue?

When you try to return an item that is lost with BlockReturnOfLostItems we deny the return. But we allow checking it out to a statistical patron. Should the latter also be blocked?

Maybe instead of resolving the lost status automatically, we should ask for confirmation and when BlockReturnOfLostItems is set even block it hard.
Comment 13 Lucas Gass 2022-10-21 17:16:12 UTC
Doesn't apply cleanly to master, I'll rebase and return to this one.
Comment 14 Lucas Gass 2023-02-02 22:48:47 UTC
Created attachment 146041 [details] [review]
Bug 27992: Call AddReturn when checking in localuse item
Comment 15 Lucas Gass 2023-02-02 23:25:57 UTC
> In marked my item lost using the 'claims returned' feature. With this change
> the item will be returned and the claim remains unresolved. Is this an issue?

In this case should we return the item and resolve the claim. Or block the return and leave the claim?
 
> When you try to return an item that is lost with BlockReturnOfLostItems we
> deny the return. But we allow checking it out to a statistical patron.
> Should the latter also be blocked?

If the item is LOST and BlockReturnOfLostItems is off, ask for confirmation but if the item is LOST and BlockReturnOfLostItems is on block the return?
Comment 16 Donna 2023-02-15 15:28:03 UTC
(In reply to Lucas Gass from comment #15)
> > In marked my item lost using the 'claims returned' feature. With this change
> > the item will be returned and the claim remains unresolved. Is this an issue?
> 
> In this case should we return the item and resolve the claim. Or block the
> return and leave the claim?

Ideally, have it behave the same way as when I check in anything else that has a claim on it - check it in, and give a modal that allows for resolving
>  
> > When you try to return an item that is lost with BlockReturnOfLostItems we
> > deny the return. But we allow checking it out to a statistical patron.
> > Should the latter also be blocked?
> 
> If the item is LOST and BlockReturnOfLostItems is off, ask for confirmation
> but if the item is LOST and BlockReturnOfLostItems is on block the return?

Yes!
Comment 17 Felicity Brown 2023-03-29 21:17:46 UTC
Oooh! We were just talking about the need for this today!
Comment 18 Emily Lamancusa 2023-04-13 14:10:16 UTC
It sounds like the plan to (1) respect BlockReturnOfLostItems, (2) ask for confirmation when recording local use on a lost item if it is not blocked, and (3) resolve any return claims on the item, addresses all the concerns that have been raised.

For #2, I feel like it could also make sense to follow the setting for IssueLostItem instead of requiring confirmation no matter what, but I'm not sure I have strong reasons for insisting on it if there are objections/downsides.

Anything else that needs to be resolved before we can move forward with this?
Comment 19 Lucas Gass 2023-09-23 13:29:26 UTC
I thin(In reply to Emily Lamancusa from comment #18)
> It sounds like the plan to (1) respect BlockReturnOfLostItems, 
Yes.

> (2) ask for
> confirmation when recording local use on a lost item if it is not blocked,
> and (3) resolve any return claims on the item, addresses all the concerns
> that have been raised.
I think this should have the same behavior as when checking in a lost item. 


> For #2, I feel like it could also make sense to follow the setting for
> IssueLostItem instead of requiring confirmation no matter what, but I'm not
> sure I have strong reasons for insisting on it if there are
> objections/downsides.
In my opinion this should be handled in a separate bug. 


I think we can now move this from 'In discussion' to FQA
Comment 20 Lucas Gass 2023-10-24 21:13:09 UTC
I'm finding plenty of other issues here that I think might need to be addressed when checking out to a statistical patron:

1. If there is a hold on the item that is checked in to a stats patron no indication that there is a hold is give/
2. BlockReturnOfWithdrawnItems is not honored.
3. AllowReturnToBranch is not checked, if I check an item in with the statistical patron while logged in at a different library AllowReturnToBranch is not honored.
4. Recalls, if there is a recall on the item that is checked out to the statistical patron no warning is given.
Comment 21 Emily Lamancusa 2023-10-26 20:55:59 UTC
Ooh, good point. I was thinking that AddReturn had all of that covered, but the last time I looked at this I missed the fact that AddReturn would only get called if the item had a current checkout. Thanks for catching that!
Comment 22 Lucas Gass 2023-10-27 23:37:05 UTC
Created attachment 158011 [details] [review]
Bug 27992: Call AddReturn on stats patron
Comment 23 Lucas Gass 2023-10-30 15:00:59 UTC
Created attachment 158073 [details] [review]
Bug 27992: Call AddReturn on stats patron

To test:
1. Create a Statistical Patron
2. Check out an item to the Stat Patron, that is checked out to another user
3. See that the local use is recorded, but the item does not get checked in
4. Check out an item that has a lost status and note that the local use is recorded, and the lost status is cleared.
5. Item is NOT checked in
6. Apply patch
7. Repeat steps 2 - 4. Item is checked in.
8. Set BlockReturnOfLostItems to Block.
9. Have a checkout to another patron then mark it as lost.
10. Check it out to the Statistical Patron. You should see the message "Item was lost, cannot be returned."
12. Conform the item remains on the patron's account.
13. Turn off BlockReturnOfLostItems, check out the same item to the Statistical Patron. You should see a message "Item was lost, now found."
14. Conform the item was actually checked in.

15. Set BlockReturnOfWithdrawnItems to Block.
16. Have a checkout to another patron then mark it as withdrawn.
17. Check it out to the Statistical Patron. You should see the message "Item was withdrawn, cannot be returned."
18. Conform the item remains on the patron's account.
19. Turn off BlockReturnOfWithdrawnItems, check out the same item to the Statistical Patron. You should see a message "Item was withdrawn."
20. Conform the item was actually checked in.

21. Have an item on a regular patron account that has a hold on it.
22. Check it out to the Statistical Patron
23. See the message "Item on hold, please checkin."

24. Have an item on a regular patron account that has a claim return on it.
25. Checkit it out to the Statistical Patron.
26. See the message "Item claimed returned, please checkin."

27. Have an item on a regular patron account that has been recalled.
28. Checkit it out to the Statistical Patron.
29. See the message "Item can fill a recall, please checkin."
Comment 24 Lucas Gass 2023-10-30 16:46:15 UTC
Created attachment 158086 [details] [review]
Bug 27992: Unit test
Comment 25 Emily Lamancusa 2023-11-01 21:18:47 UTC
Created attachment 158216 [details] [review]
Bug 27992: Call AddReturn on stats patron

To test:
1. Create a Statistical Patron
2. Check out an item to the Stat Patron, that is checked out to another user
3. See that the local use is recorded, but the item does not get checked in
4. Check out an item that has a lost status and note that the local use is recorded, and the lost status is cleared.
5. Item is NOT checked in
6. Apply patch
7. Repeat steps 2 - 4. Item is checked in.
8. Set BlockReturnOfLostItems to Block.
9. Have a checkout to another patron then mark it as lost.
10. Check it out to the Statistical Patron. You should see the message "Item was lost, cannot be returned."
12. Conform the item remains on the patron's account.
13. Turn off BlockReturnOfLostItems, check out the same item to the Statistical Patron. You should see a message "Item was lost, now found."
14. Conform the item was actually checked in.

15. Set BlockReturnOfWithdrawnItems to Block.
16. Have a checkout to another patron then mark it as withdrawn.
17. Check it out to the Statistical Patron. You should see the message "Item was withdrawn, cannot be returned."
18. Conform the item remains on the patron's account.
19. Turn off BlockReturnOfWithdrawnItems, check out the same item to the Statistical Patron. You should see a message "Item was withdrawn."
20. Conform the item was actually checked in.

21. Have an item on a regular patron account that has a hold on it.
22. Check it out to the Statistical Patron
23. See the message "Item on hold, please checkin."

24. Have an item on a regular patron account that has a claim return on it.
25. Checkit it out to the Statistical Patron.
26. See the message "Item claimed returned, please checkin."

27. Have an item on a regular patron account that has been recalled.
28. Checkit it out to the Statistical Patron.
29. See the message "Item can fill a recall, please checkin."

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 26 Emily Lamancusa 2023-11-01 21:18:49 UTC
Created attachment 158217 [details] [review]
Bug 27992: Unit test

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 27 Emily Lamancusa 2023-11-01 21:19:57 UTC
Looks great and works as intended!

One small, quality-of-life thing (not a blocker): When it returns a checked-out item, could the return message link directly to the patron account? The checkout table does this, so it would be nice if the return message here did the same.

Testing notes:
0. Create a statistical patron category
0.5 Set sysprefs and circulation rules:
  - MarkLostItemsAsReturned to never return items when marked as lost
  - ClaimsReturnedLostValue
  - UseRecalls to Use
  - Create/edit a circulation rule to allow at least 1 recall

I also confirmed that if a lost item with an associated fine is checked in, it respects the circulation rules for refunding fines.
Comment 28 Katrin Fischer 2023-11-05 12:38:41 UTC
1) Perltidied in a follow-up to make the tests pass. Please remember to run QA test tools and perltidy your code.

2) Ternary operator needed?

+        my $block_lost_return = C4::Context->preference("BlockReturnOfLostItems") ? 1 : 0;

Do we really need the ternary operator here? The preference is already stored with 0 and 1 in the database. 

3) I feel like the information to the users could be a bit more prominent, maybe something for another bug. Not a blocker here.
Comment 29 Katrin Fischer 2023-11-05 12:41:01 UTC
Created attachment 158462 [details] [review]
Bug 27992: Call AddReturn on stats patron

To test:
1. Create a Statistical Patron
2. Check out an item to the Stat Patron, that is checked out to another user
3. See that the local use is recorded, but the item does not get checked in
4. Check out an item that has a lost status and note that the local use is recorded, and the lost status is cleared.
5. Item is NOT checked in
6. Apply patch
7. Repeat steps 2 - 4. Item is checked in.
8. Set BlockReturnOfLostItems to Block.
9. Have a checkout to another patron then mark it as lost.
10. Check it out to the Statistical Patron. You should see the message "Item was lost, cannot be returned."
12. Conform the item remains on the patron's account.
13. Turn off BlockReturnOfLostItems, check out the same item to the Statistical Patron. You should see a message "Item was lost, now found."
14. Conform the item was actually checked in.

15. Set BlockReturnOfWithdrawnItems to Block.
16. Have a checkout to another patron then mark it as withdrawn.
17. Check it out to the Statistical Patron. You should see the message "Item was withdrawn, cannot be returned."
18. Conform the item remains on the patron's account.
19. Turn off BlockReturnOfWithdrawnItems, check out the same item to the Statistical Patron. You should see a message "Item was withdrawn."
20. Conform the item was actually checked in.

21. Have an item on a regular patron account that has a hold on it.
22. Check it out to the Statistical Patron
23. See the message "Item on hold, please checkin."

24. Have an item on a regular patron account that has a claim return on it.
25. Checkit it out to the Statistical Patron.
26. See the message "Item claimed returned, please checkin."

27. Have an item on a regular patron account that has been recalled.
28. Checkit it out to the Statistical Patron.
29. See the message "Item can fill a recall, please checkin."

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2023-11-05 12:41:04 UTC
Created attachment 158463 [details] [review]
Bug 27992: Unit test

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Katrin Fischer 2023-11-05 12:41:08 UTC
Created attachment 158464 [details] [review]
Bug 27992: (QA follow-up) Perltidy

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 32 Katrin Fischer 2023-11-05 12:41:12 UTC
Created attachment 158465 [details] [review]
Bug 27992: (QA follow-up) Terminology: returned -> checked in

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Tomás Cohen Arazi 2023-11-06 14:36:42 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 34 Fridolin Somers 2023-11-10 07:37:33 UTC
Pushed to 23.05.x for 23.05.06
Comment 35 Fridolin Somers 2023-11-13 19:22:19 UTC
Created attachment 158900 [details] [review]
Bug 27992: [23.05] (follow-up) Fix unit test with unblessed patron
Comment 36 Fridolin Somers 2023-11-13 19:22:48 UTC
Fixed test suite, AddIssue still uses unblessed patron
Comment 37 Marcel de Rooy 2024-01-30 13:01:25 UTC
In hindsight it seems very strange to me to put an AddReturn in a sub that you should use to just know if a book can be issued? See discussion too on bug 35840.
Comment 38 Martin Renvoize 2024-01-31 13:54:02 UTC
(In reply to Marcel de Rooy from comment #37)
> In hindsight it seems very strange to me to put an AddReturn in a sub that
> you should use to just know if a book can be issued? See discussion too on
> bug 35840.

That's really odd.. that should be an idempotent action surely!?
Comment 39 Lucas Gass 2024-01-31 15:08:43 UTC
> That's really odd.. that should be an idempotent action surely!?

I opened Bug 35950 to continue to discuss this.