Bug 27919 - Split claims return from LOST
Summary: Split claims return from LOST
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Matt Blenkinsop
QA Contact: Nick Clemens (kidclamp)
URL:
Keywords: additional_work_needed, release-notes-needed
Depends on: 27753
Blocks: 33292 38248
  Show dependency treegraph
 
Reported: 2021-03-10 17:33 UTC by Donna
Modified: 2024-10-30 14:27 UTC (History)
20 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
**Sponsored by** *ByWater Solutions* and *PTFS Europe* and *Cuyahoga County Public Library*
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 27919: Return claims shouldn't change lost status if already set (1.65 KB, patch)
2024-06-05 14:26 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 27919: Return claims shouldn't change lost status if already set (1.70 KB, patch)
2024-06-05 20:16 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 27919: Update syspref description (1.16 KB, patch)
2024-06-06 08:56 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 27919: (QA follow-up): Add alerts to the UI to show there are return claims (9.60 KB, patch)
2024-07-01 10:08 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 27919: (QA follow-up): Add unit tests (2.16 KB, patch)
2024-07-01 10:08 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 27919: Return claims shouldn't change lost status if already set (1.75 KB, patch)
2024-09-20 15:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 27919: Update syspref description (1.20 KB, patch)
2024-09-20 15:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 27919: (QA follow-up): Add alerts to the UI to show there are return claims (9.65 KB, patch)
2024-09-20 15:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 27919: (QA follow-up): Add unit tests (2.20 KB, patch)
2024-09-20 15:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 27919: (follow-up) Fix translations (1.30 KB, patch)
2024-10-22 10:31 UTC, Matt Blenkinsop
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-10 17:33:04 UTC
It would be helpful to libraries if Claims Returned was moved out of the Lost Value.  It is confusing for patrons and staff to see a Claims Return as a Lost item, and there are times when an item needs to be marked both as Lost and as Claim Return, and combining those into one field can be a challenge.
Comment 1 Katrin Fischer 2021-03-10 20:44:50 UTC
Hi Donna, can you explain the use cases a bit more?

For me claims returned is something like a "maybe lost". So it fits where it is now. In which situation would you mark it return claimed and lost? Or only return claimed?
Comment 2 Rebecca Coert 2021-03-12 19:58:45 UTC
We'd like to have Claims Returned (CR) be a separate category.  An item can be both checked out, Overdue, or Lost (i.e.: Lost, Long Overdue (Lost)) and CR at the same time; however, they are currently sharing the "Lost Status" field.  We view the claim as separate from an item's status.
Comment 3 George Williams (NEKLS) 2022-09-22 19:44:14 UTC
I would agree too.  Why is claims returned a "Lost" status?
Comment 4 Ashley Johnson 2022-09-22 19:45:45 UTC
Agreed. Our library system has wrestled with this as well.
Comment 5 Andrew Fuerste-Henry 2023-07-05 14:35:12 UTC
I'm broadly in favor of this change. It seems like divorcing CR from Lost would allow more flexibility in how CR works.
Comment 6 Katrin Fischer 2023-07-08 13:36:10 UTC
Everyone says it's bad... but could you detail where it gets in the way? Maybe giving some example?
Comment 7 Lisette Scheer 2023-11-28 23:35:14 UTC
If an item is marked long overdue lost, then is claimed returned, then is returned, the lost fine won't be refunded, even if the circulation rules say it should be.
Comment 8 Lisette Scheer 2023-11-29 22:14:04 UTC
(In reply to Lisette Scheer from comment #7)
> If an item is marked long overdue lost, then is claimed returned, then is
> returned, the lost fine won't be refunded, even if the circulation rules say
> it should be.

Another example:

If an item is marked as claim returned and then changed to a different lost status when it isn't found, it doesn't charge the patron because it's already in a lost status.
Comment 9 Matt Blenkinsop 2024-06-05 14:26:14 UTC
Created attachment 167443 [details] [review]
Bug 27919: Return claims shouldn't change lost status if already set

This patch prevents a return claim from changing the lost status if it has already been set.

Test plan:
1) In system preferences, set the ClaimReturnedLostValue syspref to any value
2) Checkout an item to a patron
3) Set that item as lost, using a different status to the one you set in step 1
4) Create a return claim on the item
5) Check the item's lost status, it should now have been set to the value you set in step 1
6) Apply patch
7) reset_all
8) Repeat steps 1-5, this time the status should remain at the value you set in step 3 and not be overwritten by the return claim
9) Repeat steps 1,2,4 and 5 - this time the lost status should be set to the value you set in step 1 as we didn't have a pre-existing lost status
Comment 10 Andrew Fuerste-Henry 2024-06-05 19:59:57 UTC
(In reply to Lisette Scheer from comment #7)
> If an item is marked long overdue lost, then is claimed returned, then is
> returned, the lost fine won't be refunded, even if the circulation rules say
> it should be.

I cannot recreate this behavior on main. I checked out an item, marked it lost and confirmed the creation of a charge, marked it claimed, and checked it in. Koha gave me its standard "A refund for the lost item charge has been applied ..." message and cleared the charge.
Comment 11 Andrew Fuerste-Henry 2024-06-05 20:07:19 UTC
(In reply to Lisette Scheer from comment #8)
> Another example:
> 
> If an item is marked as claim returned and then changed to a different lost
> status when it isn't found, it doesn't charge the patron because it's
> already in a lost status.

This one is still valid, sort of. If I:
1a - set the ClaimReturnedLostValue syspref to any value,
1b - set ClaimReturnedChargeFee to No,
1c - set WhenLostChargeReplacemement fee to Charge,
1d - set MarkLostItemAsReturned to never mark a lost item as returned (no options selected)

2 - check an item out to a patron
3 - mark item claimed
4 - resolve claim, setting item to Lost
5 - confirm patron is not charged the replacement cost
6 - edit item, changing itemlost to Missing
7 - confirm patron *has* been charged the replacement cost

So you *can* get Koha to charge a patron for something they've previously claimed returned, but it's confusingly difficult
Comment 12 Andrew Fuerste-Henry 2024-06-05 20:16:44 UTC
Created attachment 167496 [details] [review]
Bug 27919: Return claims shouldn't change lost status if already set

This patch prevents a return claim from changing the lost status if it has already been set.

Test plan:
1) In system preferences, set the ClaimReturnedLostValue syspref to any value
2) Checkout an item to a patron
3) Set that item as lost, using a different status to the one you set in step 1
4) Create a return claim on the item
5) Check the item's lost status, it should now have been set to the value you set in step 1
6) Apply patch
7) reset_all
8) Repeat steps 1-5, this time the status should remain at the value you set in step 3 and not be overwritten by the return claim
9) Repeat steps 1,2,4 and 5 - this time the lost status should be set to the value you set in step 1 as we didn't have a pre-existing lost status

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 13 Andrew Fuerste-Henry 2024-06-05 20:18:31 UTC
Everything in this test plan worked as described. 

With this patch, an item "can be both checked out, Overdue, or Lost (i.e.: Lost, Long Overdue (Lost)) and CR at the same time" as described by Rebecca in comment 2.

This patch does not address the issue raised by Lisette in comment 8.
Comment 14 Martin Renvoize (ashimema) 2024-06-06 08:18:25 UTC
I think I'd like the make this change clear in the system preference description too pretty please.. else it'll be a hidden feature/function.

We went round and round in circles in this one together Matt and I and this seemed to be the most pragmatic approach we could come up with in the end.

We came up with a few other options, but all of them required a lot more code and actually obfuscated the problem further.  There's a lot of preferences involved and there are attached bugs that try to make some of those preferences clearer too down the line.
Comment 15 Matt Blenkinsop 2024-06-06 08:56:48 UTC
Created attachment 167527 [details] [review]
Bug 27919: Update syspref description
Comment 16 Nick Clemens (kidclamp) 2024-06-13 17:44:09 UTC
While this does allow for retaining original lost status when marking an item claims returned, it doesn't allow staff to see the return claim beyond the patrons checkout page.

On the details page:
We show only the Lost status - we need to alert if there is a claim as well

On the items tab:
We show only the lost status - we need to alert if there is a claim as well

For items in the claimed return status, I cannot change the lost value
For items with a claim and another lost status, I can alter the lost status
We need to reconcile this behavior

This feels a bit like a bandaid - can you elaborate on the other options that made this worse?I am not blocking, but I think displaying the claim if not reflected in the lost status would be a requirement here.
Comment 17 Matt Blenkinsop 2024-07-01 10:08:12 UTC
Created attachment 168309 [details] [review]
Bug 27919: (QA follow-up): Add alerts to the UI to show there are return claims

This patch adds info to the UI to show when there is a return claim. It also reconciles the behaviour for allowing the lost status to be changed. Previously the lost status could not be changed if there was a return claim. This is now possible and a message is shown highlighting the fact that there is a return claim as well as the new lost status.

Test plan:
1) Add a return claim to an item.
2) Check the holdings table on the record page and observe that the "Status" column now shows that there is a return claim
3) Click on the barcode for that item in the table to get to the item editing page
4) Observe that the select for the item lost status is not disabled
5) Change the status and save - the form should save correctly and display both the new status as well as a message showing that there is a return claim.
Comment 18 Matt Blenkinsop 2024-07-01 10:08:15 UTC
Created attachment 168310 [details] [review]
Bug 27919: (QA follow-up): Add unit tests

prove t/db_dependent/Circulation/ReturnClaims.t
Comment 19 Matt Blenkinsop 2024-07-01 10:13:40 UTC
(In reply to Nick Clemens (kidclamp) from comment #16)
> While this does allow for retaining original lost status when marking an
> item claims returned, it doesn't allow staff to see the return claim beyond
> the patrons checkout page.
> 
> On the details page:
> We show only the Lost status - we need to alert if there is a claim as well

Done

> On the items tab:
> We show only the lost status - we need to alert if there is a claim as well

Done

> For items in the claimed return status, I cannot change the lost value
> For items with a claim and another lost status, I can alter the lost status
> We need to reconcile this behavior

The default behaviour currently is that you can't change the lost status if there is a return claim. I've changed this so you can now change it even when a return claim exists. The alerts above will be visible to show the return claim if it the status is changed
>
> This feels a bit like a bandaid - can you elaborate on the other options
> that made this worse?I am not blocking, but I think displaying the claim if
> not reflected in the lost status would be a requirement here.

I initially started down the process of completely stripping the return claims feature out of the lost status entirely and using a method to check for both lost status and any return claims to determine the item status. That became incredibly complicated as there are 100s of instances of 'itemlost' that would need refactoring to now use this logic and would probably introduce technical debt as most community devs might not be aware of this new method when writing code down the line. Amending how return claims govern the lost status seemed like the best method to allow the two to co-exist without completely re-writing the entire concept.
Comment 20 Nick Clemens (kidclamp) 2024-09-20 15:41:28 UTC
Created attachment 171833 [details] [review]
Bug 27919: Return claims shouldn't change lost status if already set

This patch prevents a return claim from changing the lost status if it has already been set.

Test plan:
1) In system preferences, set the ClaimReturnedLostValue syspref to any value
2) Checkout an item to a patron
3) Set that item as lost, using a different status to the one you set in step 1
4) Create a return claim on the item
5) Check the item's lost status, it should now have been set to the value you set in step 1
6) Apply patch
7) reset_all
8) Repeat steps 1-5, this time the status should remain at the value you set in step 3 and not be overwritten by the return claim
9) Repeat steps 1,2,4 and 5 - this time the lost status should be set to the value you set in step 1 as we didn't have a pre-existing lost status

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Nick Clemens (kidclamp) 2024-09-20 15:41:31 UTC
Created attachment 171834 [details] [review]
Bug 27919: Update syspref description

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 22 Nick Clemens (kidclamp) 2024-09-20 15:41:34 UTC
Created attachment 171835 [details] [review]
Bug 27919: (QA follow-up): Add alerts to the UI to show there are return claims

This patch adds info to the UI to show when there is a return claim. It also reconciles the behaviour for allowing the lost status to be changed. Previously the lost status could not be changed if there was a return claim. This is now possible and a message is shown highlighting the fact that there is a return claim as well as the new lost status.

Test plan:
1) Add a return claim to an item.
2) Check the holdings table on the record page and observe that the "Status" column now shows that there is a return claim
3) Click on the barcode for that item in the table to get to the item editing page
4) Observe that the select for the item lost status is not disabled
5) Change the status and save - the form should save correctly and display both the new status as well as a message showing that there is a return claim.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Nick Clemens (kidclamp) 2024-09-20 15:41:37 UTC
Created attachment 171836 [details] [review]
Bug 27919: (QA follow-up): Add unit tests

prove t/db_dependent/Circulation/ReturnClaims.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Katrin Fischer 2024-10-22 09:41:59 UTC
(In reply to Lisette Scheer from comment #8)
> (In reply to Lisette Scheer from comment #7)
> > If an item is marked long overdue lost, then is claimed returned, then is
> > returned, the lost fine won't be refunded, even if the circulation rules say
> > it should be.
> 
> Another example:
> 
> If an item is marked as claim returned and then changed to a different lost
> status when it isn't found, it doesn't charge the patron because it's
> already in a lost status.

Hi Liz, never thanked you for these! Thank you!
Comment 25 Katrin Fischer 2024-10-22 09:42:26 UTC
(In reply to Katrin Fischer from comment #24)
> (In reply to Lisette Scheer from comment #8)
> > (In reply to Lisette Scheer from comment #7)
> > > If an item is marked long overdue lost, then is claimed returned, then is
> > > returned, the lost fine won't be refunded, even if the circulation rules say
> > > it should be.
> > 
> > Another example:
> > 
> > If an item is marked as claim returned and then changed to a different lost
> > status when it isn't found, it doesn't charge the patron because it's
> > already in a lost status.
> 
> Hi Liz, never thanked you for these! Thank you!

Lisette of course... but I hope it still counts, very helpful for understanding the issue better.
Comment 26 Katrin Fischer 2024-10-22 10:00:08 UTC
1) Translatability

We have untranslatable strings again:

+                            nodes += '<span class="claimed_returned">(Claimed returned)</span>';

For JavaScript inside a .tt or .inc, you need to use the _() syntax.

2) Feature/workflow question

For testing, I did the following:

* Check out an item
* Make the "claim returns" column visible in the checkouts table via table config
* Click the button => Nothing happens
* Set ClaimReturnedLostValue to a LOST status
* Button started working

Question: Is this the intended behavior?

It looks like this allows to keep an existing lost status, but it still forces you to pick and set one if the item was not lost before. I am happy if that's how it's intended, just bringing it up as a question.

* I removed the lost status from the now unlocked item tab form.

Question: The item now shows as available in the staff client and the OPAC, although the claim hasn't been resolved in the patron account. Should it be possible to remove the lost status like that or should we only be able to replace it?

I am still pushing this, but I am trusting to get some replies and a speedy follow-up for 1)!
Comment 27 Katrin Fischer 2024-10-22 10:09:56 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 28 Matt Blenkinsop 2024-10-22 10:31:17 UTC
Created attachment 173124 [details] [review]
Bug 27919: (follow-up) Fix translations
Comment 29 Katrin Fischer 2024-10-23 16:43:37 UTC
Thanks Matt! Keeping the keyword for the workflow questions.
Comment 30 David Cook 2024-10-23 22:15:21 UTC
Looks like this causes a regression: bug 38248
Comment 31 David Cook 2024-10-23 22:38:11 UTC
(In reply to David Cook from comment #30)
> Looks like this causes a regression: bug 38248

Patch posted for it :)
Comment 32 Jonathan Druart 2024-10-30 08:51:43 UTC
Matt, please have have a look at bug 38248 comment 11