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.
Hi Donna, can you explain the use case here a bit? Why is the item checked out at the time of local use?
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.
(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.
+1 on this. A local use scan assumes the item is in the library and therefore any current checkout on it should be ended.
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
Works as expected! Thanks!
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>
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
Tests are missing.
Created attachment 128633 [details] [review] Bug 27992: Unit test Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
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.
Doesn't apply cleanly to master, I'll rebase and return to this one.
Created attachment 146041 [details] [review] Bug 27992: Call AddReturn when checking in localuse item
> 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?
(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!
Oooh! We were just talking about the need for this today!
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?
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
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.
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!
Created attachment 158011 [details] [review] Bug 27992: Call AddReturn on stats patron
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."
Created attachment 158086 [details] [review] Bug 27992: Unit test
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>
Created attachment 158217 [details] [review] Bug 27992: Unit test Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
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.
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.
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>
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>
Created attachment 158464 [details] [review] Bug 27992: (QA follow-up) Perltidy Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158465 [details] [review] Bug 27992: (QA follow-up) Terminology: returned -> checked in Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.06
Created attachment 158900 [details] [review] Bug 27992: [23.05] (follow-up) Fix unit test with unblessed patron
Fixed test suite, AddIssue still uses unblessed patron
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.
(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!?
> That's really odd.. that should be an idempotent action surely!? I opened Bug 35950 to continue to discuss this.