Bug 9805

I tested according to the plan test, and on renew with the patch the item become un-lost.
Ok for me.

QA comment
 * tiny patch, despite it's size
 * passes koha-qa.pl

passed QA

Hm, reading this I wonder - what happens if a patron does a renew all not thinking about the lost book? It will be 'unlost'? I can see why checking in removes the status, because you actually need the book on the circ desk/self check, but for renewing it's not needed so you won't know if it as actually there or not.

Hm I missed the status is set by longoverdue.pl so also automatic. Not sure my first comment makes sense now :)

I share Katrin's concern. Regardless how the lost status is set, once a book has been marked lost I don't think that all -- or even most -- librarians want the patron to be able to mark it as not lost without presenting the book. I feel like a syspref would be a good idea.

(In reply to comment #8)
> I share Katrin's concern. Regardless how the lost status is set, once a book
> has been marked lost I don't think that all -- or even most -- librarians
> want the patron to be able to mark it as not lost without presenting the
> book.
Very good point, that I could have think of when QAing !

> I feel like a syspref would be a good idea.
Well, FAUST leader will probably jump in here and say that this syspref could/should be avoided. but I'm not RM anymore, so I just express my concern & let others decide !

*** Bug 8560 has been marked as a duplicate of this bug. ***

Setting to passed qa on the assumption that the system preference was the only blocker for this. Adding system preference has not affected the behavior of the patch. Please set to 'needs signoff' if you feel otherwise.

I personally think that we should re-discuss lost items handling. Will set it as In Discussion, and scheduled it for the next dev meeting. It won't get stalled, just delayed.

11:57:12 AM) cait: next one is a discussion on lot item handling
(11:57:27 AM) cait: the bug is bug 9805
(11:57:28 AM) huginn: Bug http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9805 normal, P5 - low, ---, kyle, In Discussion , Lost items are un-lost if returned, but not if renewed.
(11:58:06 AM) cait: tcohen suggests that we change it to allow/disallow reneal of lost items as an option on the circulation conditions page (not necessarily the big table, but a by branch setting on that page)
(11:58:25 AM) cait: currently it is about resetting the lost satus (finding he item) when you renew
(11:59:18 AM) cait: in my opionion it makes sense to not allow renewal of lost items in the first place, but i am not totally sure how it would impact workflows
(11:59:26 AM) khall: that would be a fine solution by me.
(11:59:46 AM) oleonard: I would think we would need to both allow/disallow renewal of lost item *and* have a preference about whether the status should be removed upon renewal.
(12:00:03 PM) oleonard: However I don't speak based on my library's policy, just in the abstract.
(12:00:08 PM) cait: i am a bit worried if a patron canremove the lost status by renewing in the opac
(12:00:21 PM) cait: that would be confusing about the replacement fee already charged
(12:00:48 PM) barton: good point, cait.
(12:00:57 PM) ColinC: Or if you allow phone renewals ...
(12:01:00 PM) khall: agreed, it's better to block the renewing of lost items altogether

*** Bug 13266 has been marked as a duplicate of this bug. ***

Applying: Bug 9805 - Lost items are un-lost if returned, but not if renewed.
Applying: Bug 9805 [QA Followup] - Control renewing and finding of list items via circ rules
fatal: sha1 information is lacking or useless (C4/Circulation.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 9805 [QA Followup] - Control renewing and finding of list items via circ rules

Sorry, but the second patch doesn't apply:

$ git bz apply 9805
Bug 9805 - Lost items are un-lost if returned, but not if renewed.

38432 - Bug 9805 - Lost items are un-lost if returned, but not if renewed.
38433 - Bug 9805 [QA Followup] - Control renewing and finding of list items via circ rules

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 9805 - Lost items are un-lost if returned, but not if renewed.
Applying: Bug 9805 [QA Followup] - Control renewing and finding of list items via circ rules
fatal: sha1 information is lacking or useless (C4/Circulation.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 9805 [QA Followup] - Control renewing and finding of list items via circ rules
The copy of the patch that failed is found in:
   /home/katrin/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-9805-QA-Followup---Control-renewing-and-findin-VqfKfD.patch

Hi Kyle,

some complaints form the QA script:

 FAIL	C4/Circulation.pm
   FAIL	  spelling
		 neccessary  ==> necessary
   FAIL	  valid
		BEGIN failed--compilation aborted 
		Compilation failed in require 
		Global symbol "$borrower" requires explicit package name 

Also: HoldsQueue.t is failing:

perl t/db_dependent/HoldsQueue.t 
1..23
Global symbol "$borrower" requires explicit package name at /home/katrin/kohaclone/C4/Circulation.pm line 2939.
Compilation failed in require at /home/katrin/kohaclone/C4/Overdues.pm line 30.
BEGIN failed--compilation aborted at /home/katrin/kohaclone/C4/Overdues.pm line 30.
Compilation failed in require at /home/katrin/kohaclone/C4/Members.pm line 29.
BEGIN failed--compilation aborted at /home/katrin/kohaclone/C4/Members.pm line 29.
Compilation failed in require at t/db_dependent/HoldsQueue.t line 19.
BEGIN failed--compilation aborted at t/db_dependent/HoldsQueue.t line 19.
# Looks like your test exited with 255 before it could output anything.

Please provide a complete test coverage for these changes.

t/db_dependent/Circulation/Renewal.t (Wstat: 65280 Tests: 2 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 7 tests but ran 2.

(In reply to Jonathan Druart from comment #34)
> t/db_dependent/Circulation/Renewal.t (Wstat: 65280 Tests: 2 Failed: 0)
>   Non-zero exit status: 255
>   Parse errors: Bad plan.  You planned 7 tests but ran 2.

All the tests passed for me! Can you please try again?

t/db_dependent/Circulation/Renewal.t .. 1/7 DBD::mysql::st execute failed: Unknown column 'me.hold_fulfillment_policy' in 'field list'

The Schema is wrong, you are adding default_branch_circ_rules.hold_fulfillment_policy, which is not in master.

(In reply to Jonathan Druart from comment #39)
> t/db_dependent/Circulation/Renewal.t .. 1/7 DBD::mysql::st execute failed:
> Unknown column 'me.hold_fulfillment_policy' in 'field list'
> 
> The Schema is wrong, you are adding
> default_branch_circ_rules.hold_fulfillment_policy, which is not in master.

Doh! I must not have rebuild the schema on a stock community database! I'll get that fixed up!

Created attachment 49091 [details] [review]
Bug 9805 - Lost items are un-lost if returned, but not if renewed.

If an item has a lost status, that status is removed when the item
is returned. However, it is not removed if the item is renewed.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Browse to the circ rules
4) Note the "Renew lost?" and "Renewal marks lost found" rules
5) Set and test each of the rules both ways for the rules for all libraries
6) Repeat but for a specific branch this time

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>

Created attachment 49092 [details] [review]
Bug 9805 - Update Schema

Created attachment 49093 [details] [review]
Bug 9805 - Unit Tests

 FAIL   C4/Circulation.pm
   FAIL   valid
                "my" variable $borrower masks earlier declaration in same scope 

$borrower is already defined few lines before.
It does not seem necessary to call GetMember, GetMemberDetails has been called just before.

Created attachment 49165 [details] [review]
Bug 9805 [QA Followup] - Stop calling GetMember repeatedly

(In reply to Kyle M Hall from comment #45)
> Created attachment 49165 [details] [review] [review]
> Bug 9805 [QA Followup] - Stop calling GetMember repeatedly

Kyle, This change looks safe, but it's very hard to say it won't introduce a regression. The $borrower var is passed from a sub to another without being "refreshed" like it was before this patch. This means we could use an outdated data.

Moving this to Failed QA for now - waiting for Kyle's response to comment#46

(In reply to Jonathan Druart from comment #46)
> (In reply to Kyle M Hall from comment #45)
> > Created attachment 49165 [details] [review] [review] [review]
> > Bug 9805 [QA Followup] - Stop calling GetMember repeatedly
> 
> Kyle, This change looks safe, but it's very hard to say it won't introduce a
> regression. The $borrower var is passed from a sub to another without being
> "refreshed" like it was before this patch. This means we could use an
> outdated data.

I do not believe this is a valid concern. There is no reason to suspect there could be any problems caused by this. If we force each subroutine in Koha to fetch all db data with each call, we will continue to make Koha slower and slower. If you have a specific scenario in mind where this will cause an issue, I'm certainly open to amending my position. Otherwise I we should keep this patch to improve speed and efficiency.

This patch no longer applies (see below)

Jonathan, do you oppose to continue testing this as is? Should we discuss more widely?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 9805 - Lost items are un-lost if returned, but not if renewed.
Using index info to reconstruct a base tree...
M	C4/Circulation.pm
M	admin/smart-rules.pl
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/includes/strings.inc
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/strings.inc
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging admin/smart-rules.pl
CONFLICT (content): Merge conflict in admin/smart-rules.pl
Auto-merging C4/Circulation.pm
Failed to merge in the changes.

(In reply to Kyle M Hall from comment #48)
> (In reply to Jonathan Druart from comment #46)
> > (In reply to Kyle M Hall from comment #45)
> > > Created attachment 49165 [details] [review] [review] [review] [review]
> > > Bug 9805 [QA Followup] - Stop calling GetMember repeatedly
> > 
> > Kyle, This change looks safe, but it's very hard to say it won't introduce a
> > regression. The $borrower var is passed from a sub to another without being
> > "refreshed" like it was before this patch. This means we could use an
> > outdated data.
> 
> I do not believe this is a valid concern. There is no reason to suspect
> there could be any problems caused by this. If we force each subroutine in
> Koha to fetch all db data with each call, we will continue to make Koha
> slower and slower. If you have a specific scenario in mind where this will
> cause an issue, I'm certainly open to amending my position. Otherwise I we
> should keep this patch to improve speed and efficiency.

Of course it's a valid concern :)
It's the same problematic we have for caching.
I don't suggest to "fetch all db data with each call", but we need to take care of the obsolescence of the data we manipulate.
You are fetching the patron info using GetMemberDetails, which returns accounting data, but AddRenewal modify these infos. At some point your $borrower is obsolete. For instance if SendCirculationAlert (which does not refetch patron infos) needs to access the accounting infos to sent the letter and decide to get if from $borrower, we will have a very hard to debug issue.

It would make more sense to have this kind of patches on a separate bug report, not to block the integration of this bugfix into master.

Any chance we can resolve whatever refactoring or whatever needs to be done for this in some other ticket, as suggested by Jon? There are plenty of items at our consortium that are still marked as "lost" even after someone renewed them, and getting a fix so that this doesn't continue to happen would be very much appreciated.

Karl

(In reply to Karl Holten from comment #51)
> Any chance we can resolve whatever refactoring or whatever needs to be done
> for this in some other ticket, as suggested by Jon? There are plenty of
> items at our consortium that are still marked as "lost" even after someone
> renewed them, and getting a fix so that this doesn't continue to happen
> would be very much appreciated.
> 
> Karl

Sure!

Any movement on this bug? We are new to Koha, and just ran into issues of staff renewing items and the item is still lost with a bill. We would like to keep the history, so reluctant to check in and check out again. 
Thanks!
Beth

I'd also be interested in an update on this - we are using DefaultLongOverdueLostValue to automatically update item status. We allow renewal of these items, in the staff client and OPAC, but when items are renewed, the lost status remains which means that fines are not applied and overdue notices are not sent. There is talk of a system pref for removing lost status when items renewed - will this be in a future release?

(In reply to SiobhanC from comment #54)
> I'd also be interested in an update on this - we are using
> DefaultLongOverdueLostValue to automatically update item status. We allow
> renewal of these items, in the staff client and OPAC, but when items are
> renewed, the lost status remains which means that fines are not applied and
> overdue notices are not sent. There is talk of a system pref for removing
> lost status when items renewed - will this be in a future release?

At the moment, there is no testable code for this bug. There have also been some questions we have not been sure about. 

Would you want to be able to renew all lost items with any lost status (long overdue or lost with replacement charge applied treated the same)?

At the moment we have different ideas how this should work:

- lost items remain lost after renewing
- renewing an item already marked lost should not be possible
- renewing a lost item should be possible and remove lost status
- renewing a lost item should be possible and not remove lost status 

Maybe it would make sense to differentiate behaviour for long overdue and lost by patron?

In this case we'd probably need some YAML like preference or one that aloows to list different lost status.

In my very humble opinion if a lost item is renewed then it is no longer lost. If renewal is allowed then should it not be checked out and due back on a given date? Trouble with that is that could allow patrons to hang onto an item for years.
Not allowing a lost item to be renewed would be a safer option, then staff either get to check it back in or change the status manually.

Katrin's comment: Maybe it would make sense to differentiate behaviour for long overdue and lost by patron?

Yes, this is what I would like to see. I agree that 'lost by patron' items shouldn't be able to be renewed. However, for long overdue items that are auto-changed to lost (to enable us to easily identify them for invoicing) it would be good if they can have the 'not returned' status removed at the point of renewal. If this status isn't removed then the user doesn't receive notices. 

This would be a much bigger change, but could a new status category of overdue be introduced? Then the differentiation between overdue/lost would be easier.

I would like to see the Long Overdue(Lost) and Lost status changed when the item is renewed. We do not allow our patrons to renew lost items but our staff is allowed to override the renewal limit and set a new due date. It is confusing to staff because we can see the new due date but the status is still Lost.

In my humble opinion, I think:

1) There should be a preference as to whether or not lost items are renewed.  Some libraries want that, others do not.

2) There should be a preference as to whether or not patrons can renew lost items via OPAC/SIP/Self Check.

3) There should be a preference as to whether or not there should be a warning when renewing a lost item.

4) There should be a preference as to whether or not the Renew all button/Select all link includes lost items.

5) There should be a preference as to whether or not the lost fee is refunded on renewal.

Would like to see some traction on this.  This bug seems to be stalled.

It would be helpful to have this type of functionality. We have patrons that ignore due and overdue notices. Once they are billed then they call the library expecting to have their lost charges dropped and items renew.

So with this fix, if the lost item is renewed and the status of lost (long overdue lost) is taken off, what happens with the lost item cost? Does the patron still owe it? and if the patron still owes the lost item cost after renewing the item, and then returns the item, does that mean that they do not get a refund for the item, since the item was not in a lost status at the time of return? 

It seems like some libraries would want to be able to renew lost items, but also to refund the lost item cost if the item is returned OR renewed. (Our library does not allow renewals on lost items because if you renew a lost item, the lost item cost stays on there and we don't want that.) 

We are okay with how it works for us now (no renewal of lost items), but I wanted to bring up the issue of the lost status coming off when the item is renewed, and then the patron possibly not being refunded if the item is returned later.

(In reply to Cheryl from comment #62)
> So with this fix, if the lost item is renewed and the status of lost (long
> overdue lost) is taken off, what happens with the lost item cost? Does the
> patron still owe it? and if the patron still owes the lost item cost after
> renewing the item, and then returns the item, does that mean that they do
> not get a refund for the item, since the item was not in a lost status at
> the time of return? 
> 
> It seems like some libraries would want to be able to renew lost items, but
> also to refund the lost item cost if the item is returned OR renewed. (Our
> library does not allow renewals on lost items because if you renew a lost
> item, the lost item cost stays on there and we don't want that.) 
> 
> We are okay with how it works for us now (no renewal of lost items), but I
> wanted to bring up the issue of the lost status coming off when the item is
> renewed, and then the patron possibly not being refunded if the item is
> returned later.

At the very least, there should be an option as to whether or not the charge is maintained if the item is checked in or renewed.  Our libraries don't charge the lost price if the item is returned.  We simply want our things back.  Some libraries refund the price, but retain the late fees, because, after all, they were very late.

Personally, I think it is wrong to charge a replacement cost for something that is returned.  Late fees, yes, absolutely keep them on the patron account.  But charging for the replacement cost after it is returned seems like theft.  UNLESS the item has actually been replaced.  THEN the cost should be maintained.

Wouldn't it be nice if there was a way to indicate on an item record if the item was replaced?  Then Koha would know if maintaining the cost of replacement is justified.

Created attachment 162473 [details] [review]
Bug 9805: Lost items are un-lost if returned, but not if renewed.

If an item has a lost status, that status is removed when the item
is returned. However, it is not removed if the item is renewed.

Test Plan:
1) Apply this patch
2) Install database updates
3) Browse to the circ rules
4) Note the "Renew lost?" and "Renewal marks lost found" rules under default checkout hold and return policy
5) Set and test each of the rules both ways for the rules for all libraries

Sponsored-by: Waikato Institute of Technology

One of our libraries uses this enhancement in production so I've rebased it for main - it has not incorporated the discussion above and is not ready for testing.