Bug 21732 - If an item is marked as lost, any outstanding transfers upon it should be cancelled
Summary: If an item is marked as lost, any outstanding transfers upon it should be can...
Status: RESOLVED DUPLICATE of bug 21754
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Alex Buckley
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 20844
  Show dependency treegraph
 
Reported: 2018-10-31 10:14 UTC by Martin Renvoize
Modified: 2018-11-06 08:38 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
If the 'LostItemCancelOutstandingTransfers' syspref is enabled then any outstanding holds on the lost item are cancelled.
Version(s) released in:


Attachments
Bug 21732: Clean up outstanding transfers on lost items (2.71 KB, patch)
2018-10-31 10:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21732: (QA follow-up) Performance Improvement (1004 bytes, patch)
2018-10-31 10:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21732: Clean up outstanding transfers on lost items (3.23 KB, patch)
2018-10-31 11:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21732: (QA follow-up) Performance Improvement (1004 bytes, patch)
2018-10-31 11:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21732: Clean up outstanding transfers on lost items (3.28 KB, patch)
2018-10-31 11:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21732: (QA follow-up) Performance Improvement (1004 bytes, patch)
2018-10-31 11:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21732: (follow-up) Added tests to t/db_dependent/Circulation.t (5.98 KB, patch)
2018-10-31 23:46 UTC, Alex Buckley
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2018-10-31 10:14:18 UTC
Splitting this functionality out of bug 20844 to make the functionality changes clearer in the release notes and simplify QA
Comment 1 Martin Renvoize 2018-10-31 10:38:03 UTC
Created attachment 81695 [details] [review]
Bug 21732: Clean up outstanding transfers on lost items

When an item is marked as lost, the routine should also clean up any
outstanding transfers.
Comment 2 Martin Renvoize 2018-10-31 10:44:45 UTC
Created attachment 81696 [details] [review]
Bug 21732: (QA follow-up) Performance Improvement

We don't really need to lookup transfers before deleting them here.
Comment 3 Martin Renvoize 2018-10-31 10:45:43 UTC
I'm not actually sure this should be governed by a system preference at all.  It feels to me like a bug that transfers are not cleaned up when an Item is lost.. I'll ask for a second QA opinion on that.
Comment 4 Martin Renvoize 2018-10-31 10:53:29 UTC
Also.. I'd like to see a test added to t/db_dependent/Circulation.t for this.
Comment 5 Martin Renvoize 2018-10-31 11:10:03 UTC
Created attachment 81701 [details] [review]
Bug 21732: Clean up outstanding transfers on lost items

When an item is marked as lost, the routine should also clean up any
outstanding transfers.

Test plan:
1. Find a item which is in transfer, i.e. find an item with the text in
the 'Status' field of the table in detail.pl that indicates it is in
transfer
2. Now enable the new 'LostItemCancelOutstandingTransfers' syspref.
This will now cancel any outstanding transfers on the item when it is
marked as lost.
3. Set the item to 'Lost' either by clicking on Edit->Edit items from
the detail.pl page
OR
clicking on the Items tab on the left side of the detail.pl page
4. Notice that the transfer is now cancelled for the item
Comment 6 Martin Renvoize 2018-10-31 11:10:07 UTC
Created attachment 81702 [details] [review]
Bug 21732: (QA follow-up) Performance Improvement

We don't really need to lookup transfers before deleting them here.
Comment 7 Martin Renvoize 2018-10-31 11:11:14 UTC
Created attachment 81703 [details] [review]
Bug 21732: Clean up outstanding transfers on lost items

When an item is marked as lost, the routine should also clean up any
outstanding transfers.

Test plan:
1. Find a item which is in transfer, i.e. find an item with the text in
the 'Status' field of the table in detail.pl that indicates it is in
transfer
2. Now enable the new 'LostItemCancelOutstandingTransfers' syspref.
This will now cancel any outstanding transfers on the item when it is
marked as lost.
3. Set the item to 'Lost' either by clicking on Edit->Edit items from
the detail.pl page
OR
clicking on the Items tab on the left side of the detail.pl page
4. Notice that the transfer is now cancelled for the item

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Martin Renvoize 2018-10-31 11:11:18 UTC
Created attachment 81704 [details] [review]
Bug 21732: (QA follow-up) Performance Improvement

We don't really need to lookup transfers before deleting them here.
Comment 9 Katrin Fischer 2018-10-31 16:55:11 UTC
So the questions: When the item is set to lost, when in transfer, which is the holding library? The library where it was in transit from or to?
Comment 10 Alex Buckley 2018-10-31 18:32:01 UTC
Hi Katrin and Martin

Thanks for the feedback.

I am writing the unit test in t/db_dependent/Circulation.t now.

Additionally, do you have any suggestions/preferences on how to address the  questions to consider in comment #9 on which is the holding library of items set to lost whilst in transfer?

Cheers,
Alex
Comment 11 Martin Renvoize 2018-10-31 19:22:13 UTC
I think I'd go with the 'from' library for the holding library of the lost item.  To me, until the 'to' library accepts arrival the 'from' library is responsible.
Comment 12 Jonathan Druart 2018-10-31 19:46:06 UTC
(In reply to Martin Renvoize from comment #3)
> I'm not actually sure this should be governed by a system preference at all.

Not sure either, if it's a bug fix we should not add a new pref.

Maybe we should ask on the list? I do not manage to know how bad it is.
Should we alert during the update? Create a new check in the data inconsistencies script?
Comment 13 Alex Buckley 2018-10-31 23:46:17 UTC
Created attachment 81782 [details] [review]
Bug 21732: (follow-up) Added tests to t/db_dependent/Circulation.t

Tests check:
* If transfer is deleted when item is marked as lost with the
LostItemCancelOutstandingTransfer enabled and disabled

* If the items holdingbranch changes when item with transfers on it is
marked as lost.

Also added call to ModItem() when transfers are deleted (if the
LostItemCancelOutstandingTransfers syspref is enabled and a transfer did
exist on the item) to set the items holdingbranch to the transfer source
branch so the 'from' library is responsible for the lost item.

Sponsored-By: Brimbank Library, Australia
Comment 14 Alex Buckley 2018-10-31 23:50:30 UTC
Hi there

I have added tests to t/db_dependent/Circulation.t and a change of the items holdingbranch to the 'from' branch when the syspref is enabled. 

I am not sure what data inconsistencies script you are referring to Jonathan?

I will set this to 'Needs signoff' for now as I have addressed Martin's point in comment #3 and Katrin's point in comment #9.

Please let me know if you'd like me to make further changes. 

Cheers,
Alex
Comment 15 Alex Buckley 2018-11-01 23:45:04 UTC
Hi there

I have made another bug report Bug 21754 (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21754) which does not use a system preference to determine if outstanding transfers on an item marked as lost are cancelled. 

Instead, my patch on bug 21754 automatically cancels outstanding transfers on lost items and sets the items holding branch to the cancelled transfers source ('from') branch. 

Therefore with bug 21732 and bug 21754 we should be covered for whatever responses/concensus comes back to your questions in the Koha mailing list Martin.

Bug 21752 also contains tests in the t/db_dependent/Circulation.t 

Could bugs 21732 and 21754 please be tested and QA?

Cheers,
Alex
Comment 16 Martin Renvoize 2018-11-06 08:38:41 UTC

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