Bug 8484 - Add ability to choose if lost items are removed from a borrowers record.
Summary: Add ability to choose if lost items are removed from a borrowers record.
Status: RESOLVED DUPLICATE of bug 19974
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact: Jonathan Druart
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-20 15:33 UTC by Kyle M Hall
Modified: 2018-10-31 13:40 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8484 - Add ability to choose of lost items are removed from a borrowers record. (5.05 KB, patch)
2012-07-20 15:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8484 - Add ability to choose of lost items are removed from a borrowers record. (5.54 KB, patch)
2013-02-07 17:38 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 8484 - Add ability to choose if lost items are removed from a borrowers record. - Followup (4.31 KB, patch)
2013-02-08 15:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8484 - Add ability to choose if lost items are removed from a borrowers record. - Followup (7.29 KB, patch)
2013-02-08 15:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8484 - Add ability to choose of lost items are removed from a borrowers record. (5.85 KB, patch)
2013-04-23 15:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8484 - Add ability to choose if lost items are removed from a borrowers record. - Followup (7.65 KB, patch)
2013-04-23 15:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8484 - Add ability to choose of lost items are removed from a borrowers record. (5.69 KB, patch)
2013-08-28 13:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8484 - Add ability to choose if lost items are removed from a borrowers record. - Followup (8.25 KB, patch)
2013-08-28 13:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8484 - Add ability to choose of lost items are removed from a borrowers record. (7.91 KB, patch)
2013-09-13 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-07-20 15:33:53 UTC
Currently, if an item is marked as lost, but still checked out to a borrower, that item is automatically returned. Some libraries would prefer that items marked as lost remain checked out to the borrower.
Comment 1 Kyle M Hall 2012-07-20 15:57:22 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2012-07-30 14:48:35 UTC
Wanted to sign-off, patch does not apply, sorry.
Comment 3 Owen Leonard 2012-07-30 15:14:47 UTC
The conflict is a simple one in updatedatabase.pl, and should (IMO) be resolved by the QA-er rather than requiring a follow-up patch.
Comment 4 delaye 2012-08-10 12:09:42 UTC
i test the patch in a sandbox

To test the patch I made ​​the following steps

I record a check out
I edit the item and I put in  995$2 (UNIMARC) value 1 (items.lost)
I return to the account of the borrower
-> The items has not been checked in

But regardless the value (check in, don('t check in) under the preference system ReturnLostItems

the preference systeme  work only if the item is update by the script misc/cronjobs/longoverdue.pl ?...
Comment 5 Jared Camins-Esakov 2012-08-11 18:21:35 UTC
This patch once again makes the behavior of marking an item lost different depending whether you use moredetail or additem to make the change. The new syspref should affect every workflow that sets an item's lost status.
Comment 6 Kyle M Hall 2012-08-13 17:30:36 UTC
longoverdue.pl has its own command line switch to decide if a lost item should be returned or not. This patch only affects items marked as lost via moredetail.pl. 

(In reply to comment #4)
> i test the patch in a sandbox
> 
> To test the patch I made ​​the following steps
> 
> I record a check out
> I edit the item and I put in  995$2 (UNIMARC) value 1 (items.lost)
> I return to the account of the borrower
> -> The items has not been checked in
> 
> But regardless the value (check in, don('t check in) under the preference
> system ReturnLostItems
> 
> the preference systeme  work only if the item is update by the script
> misc/cronjobs/longoverdue.pl ?...
Comment 7 Brendan Gallagher 2012-08-13 17:43:01 UTC
(In reply to comment #5)
> This patch once again makes the behavior of marking an item lost different
> depending whether you use moredetail or additem to make the change. The new
> syspref should affect every workflow that sets an item's lost status.

That is a non-issue. Currently, that behavior is already different. Marking an item lost via moredetail.pl applies the LostItem subroutine to the given item, while using additem.pl does not. It never has, and that is why the itemlost field is hidden by default on additem.pl
Comment 8 Owen Leonard 2013-02-07 17:38:02 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2013-02-08 10:11:00 UTC
(In reply to comment #6)
> longoverdue.pl has its own command line switch to decide if a lost item
> should be returned or not. This patch only affects items marked as lost via
> moredetail.pl. 

I think it is easy to set the default value for the mark-returned parameter to the syspref's value instead of 0. Is there a good reason not to use it in this cronjob script?
Comment 10 Kyle M Hall 2013-02-08 15:02:58 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2013-02-08 15:13:36 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2013-02-08 15:16:26 UTC
I agree it would be better to control this behavior from one place in Koha. This patch removes the --mark-returned from longoverdue.pl and replaces it with the system preference.

I think we should just note this in the upgrade instructions, as trying to support them both will end up with confusing behavior if the flag is not enabled, but the system pref is. Plus, it just adds more cruft to the code base.

I've also renamed the system preference to be more in lie with Bug 7639, which ass the system preferences WhenLostForgiveFine and WhenLostChargeReplacementFee
Comment 13 cedric.vita 2013-03-18 14:39:41 UTC
Something went wrong !
Applying: Bug 8484 - Add ability to choose of lost items are removed from a borrowers record.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Failed to merge in the changes.
Patch failed at 0001 Bug 8484 - Add ability to choose of lost items are removed from a borrowers record.
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 8484 - Add ability to choose if lost items are removed from a borrowers record.

[SIGNED-OFF] Bug 8484 - Add ability to choose of lost items are removed from a borrowers record.
Apply? [yn]
Patch left in /tmp/SIGNED-OFF-Bug-8484---Add-ability-to-choose-of-los-Xm4uAj.patch
Comment 14 Kyle M Hall 2013-04-23 15:38:53 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2013-04-23 15:39:02 UTC Comment hidden (obsolete)
Comment 16 Owen Leonard 2013-08-23 14:11:23 UTC
Kyle if you could please squash these two patches I will test again.
Comment 17 Kyle M Hall 2013-08-28 13:36:48 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2013-08-28 13:37:01 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2013-09-13 12:39:13 UTC
QA comment:

I am still not sure it is a good idea to remove the parameter for the longoverdue cronjob.
As Kyle suggests it could be a note in the upgrade instruction (I let the RM takes the decision).

Note: I will squash these two patches.

Marked as Passed QA.
Comment 20 Jonathan Druart 2013-09-13 12:39:37 UTC
Created attachment 21083 [details] [review]
Bug 8484 - Add ability to choose of lost items are removed from a borrowers record.

Adds the system preference ReturnLostItems to control whether setting
an item as lost will remove the item from the borrower's record.

Important note: this system preference affects
misc/cronjobs/longoverdue.pl!

Test plan: Apply the patch and let the database update run. Test the
process of marking something lost with the ReturnLostItems preference in
the default "on" state:

- Find an item which is checked out and mark it lost from moredetail.pl
- Find an item which is checked out and mark it lost using the batch
  item modification tool

In both cases the item should be removed from the patron's account
(checked in) and a fine applied.

Next test with the ReturnLostItems preference turned off. The same
procedure should result in the item being checked in but the fine
applied.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Passes the test plan which I made up.

Bug 8484 - Add ability to choose if lost items are removed from a borrowers record. - Followup
Comment 21 Katrin Fischer 2013-09-13 20:01:24 UTC
I agree that removing the option could cause problems. Why not change the behaviour so the switch can overwrite the system preference? Or would that cause problems?
Comment 22 Galen Charlton 2013-09-16 18:18:43 UTC
(In reply to Katrin Fischer from comment #21)
> I agree that removing the option could cause problems. Why not change the
> behaviour so the switch can overwrite the system preference? Or would that
> cause problems?

I'm pondering this.  Since the new system preference defaults to ON, folks used to running longoverdue.pl without the --mark-returned switch (and not just used to it, but who explicitly do *not* want loans to be checked in when the item is marked lost) would be in for a surprise.  And since this entails circulation and money, it would be the sort of surprise where a positive response to "you should have read the release notes first" is ... unlikely.

To toss out an idea, maybe make the default value (on upgrade) of WhenLostReturnItems be NULL, then add some code to make the staff client header area display a complaint until the system preferences is set to a specific value?  And maybe make longoverdue.pl continue to keep its old behavior if the system preference is NULL?  This is would all be rather crufty, I know, but overdue and lost item processing is definitely an area where Read the Fine Release Notes doesn't fly.

Also, it's not clear to me whether ON or OFF is the better default value for WhenLostReturnItems.
Comment 23 Martin Renvoize 2015-02-27 10:28:31 UTC
Is this one dead in the water... wondering if it's going to move anywhere?
Comment 24 Nicole C. Engard 2015-03-05 14:47:39 UTC
This is something that came up at the hackfest - it's an option that a few people here would still like to see make it in to Koha.
Comment 25 Katrin Fischer 2015-05-24 13:00:26 UTC
Trying to get this moving again. I think we have 3 places to consider:

- moredetail.pl - checks item in
- additem.pl - doesn't check item in
- longoverdue.pl - has a switch

I agree, what we need here is a way to leave the behaviour unchanged after update, while putting a note somewhere, that encourages people to set it to a value. Galen made a suggestion about this on comment#22.

The about.pl system information tab could be a good place for a warning. What do people think?
Comment 26 Martin Renvoize 2018-10-31 12:07:33 UTC
Still an issue?
Comment 27 Jonathan Druart 2018-10-31 13:40:48 UTC
Fixed by bug 12363 and bug 19974

*** This bug has been marked as a duplicate of bug 19974 ***