Ideally from a workflow perspective checking in a claim return item would open up the options to set the resolution and update the note if needed.

That's a fine idea -- to have the alert at checkin include some interface with which to mark the claim as resolve, rather than just a link to the patron -- but I'm suggesting something wholly automated.

(In reply to Andrew Fuerste-Henry from comment #2)
> That's a fine idea -- to have the alert at checkin include some interface
> with which to mark the claim as resolve, rather than just a link to the
> patron -- but I'm suggesting something wholly automated.

Ah yes that makes sense.
Maybe a sys pref that can say "If a claim return item is checked in on the returns.pl resolve the claim and set it to X resolution?"

> Maybe a sys pref that can say "If a claim return item is checked in on the
> returns.pl resolve the claim and set it to X resolution?"

That's exactly what we'd like to see happen.  Fully automated and no staff interaction required.

Hi,
For me if a book is really returned, the claims retruned note should desappear.

> Maybe a sys pref that can say "If a claim return item is checked in on the
> returns.pl resolve the claim and set it to X resolution?"

I'm Ok for a syspref but it should also be done if the return is done on a selfcheckout system via SIP.

Yes, we would like to see this as an automated resolution in Koha upon check-in by staff.

Created attachment 163084 [details] [review]
Bug 27753: Added new system preferences for automatically resolving claims

To test:
1. Configure Claims returned
    1. Go to Administration > Authorized Values > LOST
    2. Add a new authorized value with value:6 and description:Claims returned
    3. Go to Administration > System Preferences
    4. Set ClaimReturnedLostValue to 6 and save
2. Check out an item to a patron.
    1. Mark the item as claim returned
    2. Check the item in.
    3. A message stating that the item has been claimed as returned pops up with an option to resolve.
    4. Resolve the claim.
3. Check out the item to the patron again.
    1. Mark the item as claim returned.
    2. Check out the item to a new patron. Select “Yes, check out”
    3. Go back to the previous patron. Notice that their claim was not resolved.
4. Apply the patch.
    1. Updatedatabase
    2. restart_all
    3. Go to the system preferences and set the system preference ‘AutoClaimReturnStatusOnCheckin’ to ‘Returned by patron’
    4. Set ‘AutoClaimReturnStatusOnCheckout’ to ‘Found in library’
5. Redo step 2
    1. When checking the claim returned item in you will now see a message that says “The previously claimed returned item has been found, automatically resolving the associated claim.”
    2. View the previous patron. Their claim has automatically been resolved with a status of ‘Returned by patron’
6. Redo step 3
    1. Upon checking the item out to another patron you will see a message that says “The previously claimed returned item has been found, automatically resolving the associated claim.”
    2. View the previous patron. Their claim has automatically been resolved with a status of ‘Found in library’.
7. Sign off and have a wonderful day!

Created attachment 163085 [details] [review]
Bug 27753: Adding patch for AutoClaimReturnStatusOnCheckout syspref

Created attachment 163090 [details] [review]
Bug 27753: Adding patch for AutoClaimReturnStatusOnCheckout syspref

Signed-off-by: Michelle <mspinney@clamsnet.org>

Created attachment 163476 [details] [review]
Bug 27753: Added new system preferences for automatically resolving claims

To test:
1. Configure Claims returned
    1. Go to Administration > Authorized Values > LOST
    2. Add a new authorized value with value:6 and description:Claims returned
    3. Go to Administration > System Preferences
    4. Set ClaimReturnedLostValue to 6 and save
2. Check out an item to a patron.
    1. Mark the item as claim returned
    2. Check the item in.
    3. A message stating that the item has been claimed as returned pops up with an option to resolve.
    4. Resolve the claim.
3. Check out the item to the patron again.
    1. Mark the item as claim returned.
    2. Check out the item to a new patron. Select “Yes, check out”
    3. Go back to the previous patron. Notice that their claim was not resolved.
4. Apply the patch.
    1. Updatedatabase
    2. restart_all
    3. Go to the system preferences and set the system preference ‘AutoClaimReturnStatusOnCheckin’ to ‘Returned by patron’
    4. Set ‘AutoClaimReturnStatusOnCheckout’ to ‘Found in library’
5. Redo step 2
    1. When checking the claim returned item in you will now see a message that says “The previously claimed returned item has been found, automatically resolving the associated claim.”
    2. View the previous patron. Their claim has automatically been resolved with a status of ‘Returned by patron’
6. Redo step 3
    1. Upon checking the item out to another patron you will see a message that says “The previously claimed returned item has been found, automatically resolving the associated claim.”
    2. View the previous patron. Their claim has automatically been resolved with a status of ‘Found in library’.
7. Sign off and have a wonderful day!

Created attachment 163477 [details] [review]
Bug 27753: Unit tests

Created attachment 163479 [details] [review]
Bug 27753: Added new system preferences for automatically resolving claims

To test:
1. Configure Claims returned
    1. Go to Administration > Authorized Values > LOST
    2. Add a new authorized value with value:6 and description:Claims returned
    3. Go to Administration > System Preferences
    4. Set ClaimReturnedLostValue to 6 and save
2. Check out an item to a patron.
    1. Mark the item as claim returned
    2. Check the item in.
    3. A message stating that the item has been claimed as returned pops up with an option to resolve.
    4. Resolve the claim.
3. Check out the item to the patron again.
    1. Mark the item as claim returned.
    2. Check out the item to a new patron. Select “Yes, check out”
    3. Go back to the previous patron. Notice that their claim was not resolved.
4. Apply the patch.
    1. Updatedatabase
    2. restart_all
    3. Go to the system preferences and set the system preference ‘AutoClaimReturnStatusOnCheckin’ to ‘Returned by patron’
    4. Set ‘AutoClaimReturnStatusOnCheckout’ to ‘Found in library’
5. Redo step 2
    1. When checking the claim returned item in you will now see a message that says “The previously claimed returned item has been found, automatically resolving the associated claim.”
    2. View the previous patron. Their claim has automatically been resolved with a status of ‘Returned by patron’
6. Redo step 3
    1. Upon checking the item out to another patron you will see a message that says “The previously claimed returned item has been found, automatically resolving the associated claim.”
    2. View the previous patron. Their claim has automatically been resolved with a status of ‘Found in library’.
7. Sign off and have a wonderful day!

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>

Created attachment 163480 [details] [review]
Bug 27753: Unit tests

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>

> 6. Redo step 3
>     1. Upon checking the item out to another patron you will see a message
> that says “The previously claimed returned item has been found,
> automatically resolving the associated claim.”

One small point, this checkout to another patron required a confirmation before Koha would proceed to return it from the one patron and check out to the other. However, the claim resolved immediately, before I responded to the confirmation about the checkout.

I can't think of a reason to *not* resolve the claim even if you don't complete the checkout, so I signed off anyway.

Do we need both system preferences introduced here?  Could we not rely on just the one for which status to change to.. i.e. if not set don't change, otherwise set to selected status?

OK.. I misread the prefs.. it's one for Checkout and one for Checkin!.. totally misread that first time around.. ignore my previous comment.

Comment on attachment 163479 [details] [review]
Bug 27753: Added new system preferences for automatically resolving claims

Review of attachment 163479 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +2531,5 @@
>  
>      if ( C4::Context->preference('ClaimReturnedLostValue') ) {
> +        my $autoClaimReturnCheckin = C4::Context->preference('AutoClaimReturnStatusOnCheckin');
> +
> +        if ($autoClaimReturnCheckin) {

I think the placement of this statement is wrong.. now it all relies on there being an autoClaimReturnedCheckin status set..

I think we need to move the check for ReturnClaims up outside of this block and trigger the autoResolve or not based on this check only (and the differing return of course)

I also hate to say it, but can we get a unit test for the Circulation.pm change pretty please.

This is so close, I'd love to see it over the line for release.. you have a couple of days.

Created attachment 165911 [details] [review]
Bug 27753: Follow up updates to C4/Circulation.pm

Created attachment 165912 [details] [review]
Bug 27753: Follow up with updated unit tests

Created attachment 165914 [details] [review]
Bug 27753: Added new system preferences for automatically resolving claims

To test:
1. Configure Claims returned
    1. Go to Administration > Authorized Values > LOST
    2. Add a new authorized value with value:6 and description:Claims returned
    3. Go to Administration > System Preferences
    4. Set ClaimReturnedLostValue to 6 and save
2. Check out an item to a patron.
    1. Mark the item as claim returned
    2. Check the item in.
    3. A message stating that the item has been claimed as returned pops up with an option to resolve.
    4. Resolve the claim.
3. Check out the item to the patron again.
    1. Mark the item as claim returned.
    2. Check out the item to a new patron. Select “Yes, check out”
    3. Go back to the previous patron. Notice that their claim was not resolved.
4. Apply the patch.
    1. Updatedatabase
    2. restart_all
    3. Go to the system preferences and set the system preference ‘AutoClaimReturnStatusOnCheckin’ to ‘Returned by patron’
    4. Set ‘AutoClaimReturnStatusOnCheckout’ to ‘Found in library’
5. Redo step 2
    1. When checking the claim returned item in you will now see a message that says “The previously claimed returned item has been found, automatically resolving the associated claim.”
    2. View the previous patron. Their claim has automatically been resolved with a status of ‘Returned by patron’
6. Redo step 3
    1. Upon checking the item out to another patron you will see a message that says “The previously claimed returned item has been found, automatically resolving the associated claim.”
    2. View the previous patron. Their claim has automatically been resolved with a status of ‘Found in library’.
7. Sign off and have a wonderful day!

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>

Created attachment 165915 [details] [review]
Bug 27753: Unit tests

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>

Created attachment 165916 [details] [review]
Bug 27753: Follow up updates to C4/Circulation.pm

Created attachment 165917 [details] [review]
Bug 27753: Follow up with updated unit tests

Created attachment 165918 [details] [review]
Bug 27753: Tidy

Created attachment 165919 [details] [review]
Bug 27753: Tidy Circulation.pl

Created attachment 165920 [details] [review]
Bug 27753: Tidy unit tests

Created attachment 165952 [details] [review]
Bug 27753: (QA follow-up) Check that item exists before using it

We're actioning the change of claim status outside of any block that
checks the $item exists. As such we'll want to add that check in here.

I did consider that this should live inside AddIssue, but on reflection
the librarian may want to not proceed with the issue given other return
values from the CanBookBeIssued call, but you still want the
AutoClaimReturn to fire regardless as you've now found the item.

Created attachment 165953 [details] [review]
Bug 27753: (QA follow-up) Improve unit tests

This patch adds unit tests that test the functionality more specifically
and adds a few notes/fixme for things we need to consider here

Created attachment 165958 [details] [review]
Bug 27753: Further unit tests highlighting possible issues

Created attachment 165959 [details] [review]
Bug 27753: Example of how we might set to librarian instead of patron

OK.. I'm happy to tidy up here and get this over the line.. however, I have some questions.

1) In AddReturn, was it deliberate to use 'patron from last checkout' as the 'resolved_by'.. or should that be the librarian performing the return action? (Remember in both cases this could be undefined.. if we use 'librarian' then we can get that from the userenv of the session.. this "should" always be set these days.. if we are using the patron then the checkout may have already been marked at completed in which case the relation accessor will fail and you won't have a patron and the call to resolve will fail.

2) Should we be handling the lost_item_status in any way?  In the interactive approach we give the librarian the option to set a new lost item status (and the resolve method accepts said status if passed).. should we be setting that here too.. or is that perhaps another bug?

3) Checkout functionality suffers from the same questions as above?

4) I tend to like subtests to be a little more self-contained with a database transaction and rollback rather than relying on other test data defined earlier in the test.. I stuck to how you started the tests here but as a future point it may be a good practice if you can to make them more isolated.

I'm failing QA for now, but will happily continue to support in getting this over the line.

(In reply to Martin Renvoize from comment #41)
> OK.. I'm happy to tidy up here and get this over the line.. however, I have
> some questions.
> 
> 1) In AddReturn, was it deliberate to use 'patron from last checkout' as the
> 'resolved_by'.. or should that be the librarian performing the return
> action? (Remember in both cases this could be undefined.. if we use
> 'librarian' then we can get that from the userenv of the session.. this
> "should" always be set these days.. if we are using the patron then the
> checkout may have already been marked at completed in which case the
> relation accessor will fail and you won't have a patron and the call to
> resolve will fail.

    ---Ack. For logging purposes I believe it would be useful to use the 'librarian'. 
> 
> 2) Should we be handling the lost_item_status in any way?  In the
> interactive approach we give the librarian the option to set a new lost item
> status (and the resolve method accepts said status if passed).. should we be
> setting that here too.. or is that perhaps another bug?
    --- Let's file a follow-up bug for this. 
> 
> 3) Checkout functionality suffers from the same questions as above?
> 
> 4) I tend to like subtests to be a little more self-contained with a
> database transaction and rollback rather than relying on other test data
> defined earlier in the test.. I stuck to how you started the tests here but
> as a future point it may be a good practice if you can to make them more
> isolated.
   --- Thank you for the feedback on unit tests!

Created attachment 166128 [details] [review]
Bug 27753: Added new system preferences for automatically resolving claims

To test:
1. Configure Claims returned
    1. Go to Administration > Authorized Values > LOST
    2. Add a new authorized value with value:6 and description:Claims returned
    3. Go to Administration > System Preferences
    4. Set ClaimReturnedLostValue to 6 and save
2. Check out an item to a patron.
    1. Mark the item as claim returned
    2. Check the item in.
    3. A message stating that the item has been claimed as returned pops up with
       an option to resolve.
    4. Resolve the claim.
3. Check out the item to the patron again.
    1. Mark the item as claim returned.
    2. Check out the item to a new patron. Select “Yes, check out”
    3. Go back to the previous patron. Notice that their claim was not resolved.
4. Apply the patch.
    1. Updatedatabase
    2. restart_all
    3. Go to the system preferences and set the system preference
       ‘AutoClaimReturnStatusOnCheckin’ to ‘Returned by patron’
    4. Set ‘AutoClaimReturnStatusOnCheckout’ to ‘Found in library’
5. Redo step 2
    1. When checking the claim returned item in you will now see a message that
       says “The previously claimed returned item has been found, automatically
       resolving the associated claim.”
    2. View the previous patron. Their claim has automatically been resolved
       with a status of ‘Returned by patron’
6. Redo step 3
    1. Upon checking the item out to another patron you will see a message that
       says “The previously claimed returned item has been found, automatically
       resolving the associated claim.”
    2. View the previous patron. Their claim has automatically been resolved
       with a status of ‘Found in library’.
7. Sign off and have a wonderful day!

Sponsored-by: Altadena Library District
Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 166129 [details] [review]
Bug 27753: Unit tests

Add unit tests for new logic introduced to C4::Circulation::AddReturn

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 166130 [details] [review]
Bug 27753: (follow-up) Update logic in AddReturn

The new logic in AddReturn was flawed, we were missing return messages
due to moving the if statement too high and catching more code than
intended.

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

Created attachment 166131 [details] [review]
Bug 27753: (follow-up) Tidy introduced code in Circulation.pm

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

Created attachment 166132 [details] [review]
Bug 27753: (follow-up) Tidy new code in circulation.pl

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

Created attachment 166133 [details] [review]
Bug 27753: (follow-up) Tidy unit tests

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

Created attachment 166134 [details] [review]
Bug 27753: (QA follow-up) Check that item exists before using it

We're actioning the change of claim status outside of any block that
checks the $item exists. As such we'll want to add that check in here.

I did consider that this should live inside AddIssue, but on reflection
the librarian may want to not proceed with the issue given other return
values from the CanBookBeIssued call, but you still want the
AutoClaimReturn to fire regardless as you've now found the item.

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

Created attachment 166135 [details] [review]
Bug 27753: (QA follow-up) Improve unit tests

This patch adds unit tests that test the functionality more specifically
and adds a few notes/fixme for things we need to consider here

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

Created attachment 166136 [details] [review]
Bug 27753: (QA follow-up) Enhance unit tests

This patch further unit tests highlighting possible issues

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

Created attachment 166137 [details] [review]
Bug 27753: (QA follow-up) Use librarian as resolver

This patch updates the changed code to set the logged in librarian as
the person who resolved the return claim at checkin/checkout action.

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

Great work, nice golfing with you Laura.

So.. in that final QA run I fixed up a few commit messages and added the sponsor details to the first commit for you.

It's all looking great now.

Passing QA

Fixed some tiny things before pushing:

1) +    bug_number  => "BUG_27753",
Needs to be just the numerical bug number.

2) Updates preference description to show the authorized value code following the existing pattern: git grep authorised_values.pl *.pref

Some notes:
I feel like the preference name is not very easy to understand. It took me a few reads. Maybe instead of AutoClaimREturnStatusOnCheckin we could have used something like AutoResolveReturnClaimOnCheckin.

Pushed for 24.05!

Well done everyone, thank you!

Not backported to 23.11.x