Please see this screencast for details:
Basically if you mark an item lost via edit items there is one behavior and if you mark it lost via the items tab there is another behavior. I think that the behavior that happens on the items tab is what we want (marking the item lost charges the patron the replacement fee).
Just a little bump on this one - anyone have any ideas on how we can solve this?
Another update - if you mark items lost using the batch edit tool the lost items remain on the patron's record as checked out instead of being removed.
*** Bug 5953 has been marked as a duplicate of this bug. ***
Also, When an item is marked Lost and Paid For, it remains checked out to the patron. It would seem like this would mark the item lost and remove it from the
patron's checked out list.
The problem here is that C4::Acounts::chargelostitems is not called when items are edited through the addbiblio.pl interface. Following a discussion on IRC, it sounds like the solution is to add a wrapper around C4::Items::ModItem which handles all business logic calls (like C4::Accounts::chargelostitems).
This issue is kind of critical (if a critical issue can be 'kind of') anyone have any clues?
Created attachment 5398 [details] [review]
There's an issue here. I can see two sets of logic for triggering lost item processing:
* catalogue/updateitem.pl: itemlost==1
* misc/cronjobs/longoverdue.pl : $charge eq $lostvalue
That prevented me from placing a single call in ModItem(). So what I have done is applying itemlost==1 where it was missing (manly after ModItemFromMarc() calls)
Can anyone comment on this please.
On a related note, can anyone please state the connection with the bug 5211 http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5211
It seems to me that what needs to happen here is what Jared suggests: we need a single subroutine to handle all the logic of marking an item Lost.
* lost value: which LOST auth values to use
* charge: charge the patron the replacement cost?
* markreturned: check in the material?
The behaviour on the item tab and the behaviour in longoverdue.pl can then be synchronized.
I'm still not convinced that the edit items screen needs to include this additional logic; it's the manual way to edit an item record, and should only be done by people who know what they're doing. Hiding 'Lost' in the MARC Framework would be an easy way to prevent folks from inadvertently making the changes without properly handling all the other logic.
Created attachment 5473 [details] [review]
Please have a look at the new patch comments if this is what is required.
During testing for this patch, I discovered that when editing items, it appeared that changes to item records were either written to the wrong item record, or offset so as to bleed into the next item record (dk how that's possible, but the screencast illustrates).
We ran into this the last time that we tried to "remove" the lost item handling from the cataloging screen using jquery. All fields were offset by 1 that time, this behavior appears to be similar.
Ah yes, that is my omission, disabled widgets don't get submitted. Will correct.
That is why submitting multiple values for multiple connected inputs should not be used. HTTP spec actually explicitly says that the order is not guaranteed.
Created attachment 5486 [details] [review]
Apologies for not catching that, should be fine now
Thanks for the new patch, will test today. :)
Created attachment 5515 [details] [review]
[Signed Off] bug_5533: new function C4::Circulation::LostItem($itemnumber, $mark_returned)
Disabled Lost Status on cataloging full item edit
longoverdue.pl calls LostItem() *without* $mark_returned
Item moredetail calls LostItem() *with* $mark_returned
Batch mode edit calls LostItem() *with* $mark_returned
Signed-off-by: Liz Rea <firstname.lastname@example.org>
I am happy to sign off on this new version. Editing works fine, behavior of 'lost' is as I expected.
Steps to test:
Check out an item with a replacement cost to a patron.
Mark the item lost from moredetail.pl -> should remove the item from checked out items and apply replacement cost to the patron's fines.
Check in the item -> should report "item was lost, now found" and remove the charge (apply a credit, technically) to the patron's fines.
Check out several items to several patrons.
Batch mod those items to lost -> should remove the item from checked out items and apply replacement cost to the patron's fines.
Check in the items -> should report "item was lost, now found" and remove the charge (apply a credit, technically) to the patron's fines.
From your test item, Edit -> Edit Items
Select an item to edit
Edit the item as you see fit
Verify that all values are saved in correct fields, and your changes are correct. (they are)
Verify that you cannot set 'lost' status from the edit item interface. (You cannot)
backdate an item so it will be long overdue
run longoverdue.pl to set your item as long overdue.
verify that the item is not removed from the patron's checked out items.
Long Overdue does not charge the patron for the replacement.
In catalogue/updateitem.pl, this patch only calls LostItem if itemlost is set to 1.... what about other Lost values? This is a forward port of the way the code was before, but I think that may have been a bug... should only LOST value 1 mark the item as lost? Thoughts/opinions?
also, one thing, bug 6490 is still outstanding with lost. Might be nice to nip it while we're in here?
I believe that bug 6490 is outside the scope of this bug report and it's proposed fix. Let's work on getting this fix in first, then we can frame the fix for 6490 in the context of the newly added subroutines.
totally fair :)
Ian, as you seem to understand & investigate this one, it's for you. Not sure i'm understanding correctly everything on this topic.
Could you update this patch to handle LOST values greater than 1 as well, in catalogue/updateitem.pl?
Another thing I'm curious about: you add a "use C4::Circulation" call to cataloguing/additem.pl, but I don't see what subroutine from C4::Circulation is used. Am I missing it? I'd like to avoid loading another huge module if at all possible.
To make things clear:
* In catalogue/updateitem.pl we want
LostItem($itemnumber, 'MARK RETURNED') if $itemlost
LostItem($itemnumber, 'MARK RETURNED') if ($itemlost==1)
* what about tools/batchMod.pl?
I will remove use C4::Circulation from cataloguing/additem.pl, probably put some code in there that got moved elsewhere.
Yes, tools/batchMod.pl should also handle cases other than itemlost == 1. Good catch.
Created attachment 5907 [details] [review]
Created attachment 5936 [details]
lost label remains
This appears to work, my problem is possible a style one - and splitting hairs, but when you edit an item Lost still shows on the menu of fields but doesn't have a field. Shouldn't we remove it completely from the framework by putting it in the 'ignore' tab - like with other item fields that don't show on the record when editing?
I'm running more tests now, but wanted to put this out there.
Created attachment 5937 [details] [review]
[SIGNED-OFF] bug_5533: Slightly improved marking items as lost
Call LostItem() whenever item is lost.
LostItem() new arg - mark returned.
Disabled Lost Status on catalogue item edit.
Signed-off-by: Nicole C. Engard <email@example.com>
For follow up we need to explain how to hide the 952$1 (lost) from
the framework by putting it in the 'ignore' tab.
New patch implements recommended QA changes, does not introduce new strings. We can control the display of the Lost status with the frameworks, so I'm going to mark this as Passed QA.
Pushed to master, please test
patch does not apply cleanly to 3.4.x due to enhancement bug 6068 which was not applied to 3.4.x. If this patch should apply to 3.4.x, please rebase over 3.4.x and attach here.
Created attachment 6001 [details] [review]
patch against 3.4.x
Created attachment 6810 [details] [review]
Proposed followup patch
removes filter that was preventing Lost status from being editable in the Edit Items screen. Numerous libraries have complained about losing this ability, even when they understand it was removed to force consistent 'lost' behaviour.
The default framework is altered to hide this field for new installs. It can be restored to the interface by editing the 'hidden' value for the itemlost subfield.
This method hides the preference by default, but allows libraries who understand the implications of what they're doing (and accept the risks) to have more control over their workflows.
Created attachment 6896 [details] [review]
Bug 5533 followup: reinstate Lost status editing
The original patch for 5533 removed the Lost status from the edit items screen; new items were not affected,
and batch changes could still be made, so this was not a consistent change.
This patch removes the filter that hid the Lost status dropdown, but then changes the default MARC framework
to hide this field by default (value -5). New installations will not see this status in the editor, unless
they manually activate it. This will enforce the semantics of using the "mark lost" options by default, but
provide a workaround for libraries that understand and accept the consequences of manually editing this field.
1. on an existing install, edit an item. you should have the Lost status visible
2. on a new install, editing or creating an item should not show the Lost status at all
Signed-off-by: Chris Cormack <firstname.lastname@example.org>
Comment on attachment 6896 [details] [review]
Bug 5533 followup: reinstate Lost status editing
this follow-up has been pushed.
chris_n, this patch has been applied to master. Is the 3.4 patch to apply to 3.4 or has it been done ?
(In reply to comment #35)
> chris_n, this patch has been applied to master. Is the 3.4 patch to apply to
> 3.4 or has it been done ?
The 3.4.x version of this patch has been applied to the 3.4.x branch.