Bug 5533 - marking item lost diff in two places
Summary: marking item lost diff in two places
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P3 critical (vote)
Assignee: Srdjan Jankovic
QA Contact: Marcel de Rooy
URL:
Keywords:
: 5953 (view as bug list)
Depends on:
Blocks: 12311
  Show dependency treegraph
 
Reported: 2010-12-21 23:02 UTC by Nicole C. Engard
Modified: 2019-06-27 09:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
patch (12.77 KB, patch)
2011-09-12 03:01 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (13.87 KB, patch)
2011-09-20 07:18 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (15.80 KB, patch)
2011-09-21 01:14 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[Signed Off] bug_5533: new function C4::Circulation::LostItem($itemnumber, $mark_returned) (17.15 KB, patch)
2011-09-21 18:06 UTC, Liz Rea
Details | Diff | Splinter Review
patch (15.44 KB, patch)
2011-10-17 02:41 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
lost label remains (16.47 KB, image/png)
2011-10-17 16:38 UTC, Nicole C. Engard
Details
[SIGNED-OFF] bug_5533: Slightly improved marking items as lost (15.63 KB, patch)
2011-10-17 16:42 UTC, Nicole C. Engard
Details | Diff | Splinter Review
patch against 3.4.x (16.19 KB, patch)
2011-10-20 01:29 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Proposed followup patch (3.44 KB, patch)
2011-12-15 13:29 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 5533 followup: reinstate Lost status editing (3.49 KB, patch)
2011-12-21 00:40 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2010-12-21 23:02:28 UTC
Please see this screencast for details:

http://screencast.com/t/6CJlMGQn2r42

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).
Comment 1 Nicole C. Engard 2011-03-01 13:10:19 UTC
Just a little bump on this one - anyone have any ideas on how we can solve this?
Comment 2 Nicole C. Engard 2011-03-22 13:05:26 UTC
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.
Comment 3 Ian Walls 2011-03-25 14:42:02 UTC
*** Bug 5953 has been marked as a duplicate of this bug. ***
Comment 4 Nicole C. Engard 2011-03-25 15:28:18 UTC
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.
Comment 5 Jared Camins-Esakov 2011-04-04 16:22:16 UTC
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).
Comment 6 Nicole C. Engard 2011-06-09 16:05:45 UTC
This issue is kind of critical (if a critical issue can be 'kind of') anyone have any clues?
Comment 7 Srdjan Jankovic 2011-09-12 03:01:27 UTC Comment hidden (obsolete)
Comment 8 Srdjan Jankovic 2011-09-12 03:34:05 UTC
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
Comment 9 Ian Walls 2011-09-13 13:08:25 UTC
Srdjan,


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.

Input params:

* 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.
Comment 10 Srdjan Jankovic 2011-09-20 07:18:29 UTC Comment hidden (obsolete)
Comment 11 Srdjan Jankovic 2011-09-20 07:19:37 UTC
Please have a look at the new patch comments if this is what is required.
Comment 12 Liz Rea 2011-09-20 19:06:22 UTC
http://screencast.com/t/mmbWNhm1mxQ

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.
Comment 13 Srdjan Jankovic 2011-09-21 00:08:43 UTC
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.
Comment 14 Srdjan Jankovic 2011-09-21 01:14:55 UTC Comment hidden (obsolete)
Comment 15 Liz Rea 2011-09-21 17:23:03 UTC
Thanks for the new patch, will test today. :)
Comment 16 Liz Rea 2011-09-21 18:06:39 UTC Comment hidden (obsolete)
Comment 17 Ian Walls 2011-09-22 18:09:16 UTC
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?
Comment 18 Liz Rea 2011-09-23 02:12:41 UTC
also, one thing, bug 6490 is still outstanding with lost. Might be nice to nip it while we're in here?
Comment 19 Ian Walls 2011-09-23 15:02:53 UTC
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.
Comment 20 Liz Rea 2011-09-23 15:23:09 UTC
totally fair :)
Comment 21 Paul Poulain 2011-10-12 10:07:15 UTC
QA comment:
Ian, as you seem to understand & investigate this one, it's for you. Not sure i'm understanding correctly everything on this topic.
Comment 22 Ian Walls 2011-10-12 14:05:45 UTC
Srdjan,


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.
Comment 23 Srdjan Jankovic 2011-10-13 03:11:13 UTC
To make things clear:
* In catalogue/updateitem.pl we want
  LostItem($itemnumber, 'MARK RETURNED') if $itemlost
instead of
  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.
Comment 24 Ian Walls 2011-10-14 12:31:28 UTC
Srdjan,


Yes, tools/batchMod.pl should also handle cases other than itemlost == 1.  Good catch.
Comment 25 Srdjan Jankovic 2011-10-17 02:41:44 UTC Comment hidden (obsolete)
Comment 26 Nicole C. Engard 2011-10-17 16:38:11 UTC
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.
Comment 27 Nicole C. Engard 2011-10-17 16:42:43 UTC Comment hidden (obsolete)
Comment 28 Ian Walls 2011-10-17 19:10:53 UTC
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.
Comment 29 Chris Cormack 2011-10-17 20:03:14 UTC
Pushed to master, please test
Comment 30 Chris Nighswonger 2011-10-19 20:09:53 UTC
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.
Comment 31 Srdjan Jankovic 2011-10-20 01:29:46 UTC
Created attachment 6001 [details] [review]
patch against 3.4.x
Comment 32 Ian Walls 2011-12-15 13:29:24 UTC Comment hidden (obsolete)
Comment 33 Chris Cormack 2011-12-21 00:40:37 UTC Comment hidden (obsolete)
Comment 34 Paul Poulain 2012-01-17 15:03:31 UTC
Comment on attachment 6896 [details] [review]
Bug 5533 followup: reinstate Lost status editing

this follow-up has been pushed.
Comment 35 Paul Poulain 2012-01-17 15:04:13 UTC
chris_n, this patch has been applied to master. Is the 3.4 patch to apply to 3.4 or has it been done ?
Comment 36 Chris Nighswonger 2012-01-17 15:11:14 UTC
(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.