Description
Barton Chittenden
2014-06-04 16:56:16 UTC
See also Bug 13546 - Branch transfer records orphaned when an item is withdrawn If a transfer has been initiated because of a hold I would expect the transfer to remain in effect until completed by a check-in: 1. Hold is placed by patron at Library A. 2. Item is checked in at Library B and transfer is initiated. 3. Item is sent out for deliver to Library A. 4. Hold is canceled. 5. Item is delivered to Library A. 6. Item is checked in, completing the original transfer. 7. At the same time, a new transfer is initiated to return the item to its home at Library B. This is how it's working right now and that seems correct to me. Is this not the behavior which was observed in version 3.14.x, or do I misunderstand the problem? I agree with Owen.. simply cancelling the hold shouldn't mark an in-progress transfer as completed.. we need to perform a check-in to know where the item actually is. OK, I'm not sure when it crept back in.. but I do NOT see a trigger for the item to be returned back home after a cancelled hold.. i.e. 1. Hold is placed by a patron at Library A. 2. Item is checked in at Library B and transfer is initiated. 3. Item is sent out for delivery to Library A. 4. Hold is cancelled. 5. Item is delivered to Library A. 6. Item is checked in, completing the original transfer. 7. NO TRANSFER IS TRIGGERED I think we should set the transfer as 'cancelled' even though it's in transit. This, I believe, would result in the check-in triggering the 'transfer to home'/'transfer to holding' process. I do have a question however... should cancellation of an in_transit transfer automatically trigger a transfer back to the original location at the point of cancellation (i.e. prior to the code that handling a return to home/return to holding at check-in time? Created attachment 118607 [details] [review] Bug 12362: Add 'TransferCancellation' as a transfer reason This patch adds the database update to add 'TransferCancellation' as a reason for transfer. Created attachment 118608 [details] [review] Bug 12362: Reverse transfer upon cancellation This patch adds a reverse transfer if a transfer is cancelled whilst it is in transit. This makes sure the item returns to it's original location. Created attachment 118609 [details] [review] Bug 12362: Cancel transfer with hold cancelation This patch adds a transfer cancellation when a hold is cancelled. Created attachment 118611 [details] [review] Bug 12362: Cancel transfer with hold cancelation This patch adds a transfer cancellation when a hold is cancelled. Test plan 1/ Check an item out from it's homebranch 2/ Place a hold on that item for another user at a different branch 3/ Check the item in at it's homebranch and accept the transfer 4/ Cancel the hold 5/ Change to the second branch and check the item in 6/ Note that a transfer is triggered with the reason 'Transfer was cancelled whilst in transit' Created attachment 118819 [details] [review] Bug 12362: Add 'TransferCancellation' as a transfer reason This patch adds the database update to add 'TransferCancellation' as a reason for transfer. Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Created attachment 118820 [details] [review] Bug 12362: Reverse transfer upon cancellation This patch adds a reverse transfer if a transfer is cancelled whilst it is in transit. This makes sure the item returns to it's original location. Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Created attachment 118821 [details] [review] Bug 12362: Cancel transfer with hold cancelation This patch adds a transfer cancellation when a hold is cancelled. Test plan 1/ Check an item out from it's homebranch 2/ Place a hold on that item for another user at a different branch 3/ Check the item in at it's homebranch and accept the transfer 4/ Cancel the hold 5/ Change to the second branch and check the item in 6/ Note that a transfer is triggered with the reason 'Transfer was cancelled whilst in transit' Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Created attachment 119762 [details] [review] Bug 12362: Add 'TransferCancellation' as a transfer reason This patch adds the database update to add 'TransferCancellation' as a reason for transfer. Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 119763 [details] [review] Bug 12362: Reverse transfer upon cancellation This patch adds a reverse transfer if a transfer is cancelled whilst it is in transit. This makes sure the item returns to it's original location. Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 119764 [details] [review] Bug 12362: Cancel transfer with hold cancelation This patch adds a transfer cancellation when a hold is cancelled. Test plan 1/ Check an item out from it's homebranch 2/ Place a hold on that item for another user at a different branch 3/ Check the item in at it's homebranch and accept the transfer 4/ Cancel the hold 5/ Change to the second branch and check the item in 6/ Note that a transfer is triggered with the reason 'Transfer was cancelled whilst in transit' Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 119898 [details] [review] Bug 12362: (QA follow-up) Fix ModItemTransfer cancellation handling ModItemTransfer is still used, sparingly. It force cancels transfers regardles of whether they are already marked as in_transit. We do not, however, want to enqueue a return transfer in this case as ModItemTransfer is itself an enqueue function. This patch updates the internal logic of ModItemTransfer to enqueue the passed transfer prior to cancelling the pre-existing one.. in this way the result is one cancelled transfer and one new transfer and not three transfers. Test plan 1/ Run t/db_dependant/Items.t and verify it fails prior to applying this patch 2/ Apply patch and run the test again, verifying it now passes. Created attachment 119899 [details] [review] Bug 12362: Add 'TransferCancellation' as a transfer reason This patch adds the database update to add 'TransferCancellation' as a reason for transfer. Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 119900 [details] [review] Bug 12362: Reverse transfer upon cancellation This patch adds a reverse transfer if a transfer is cancelled whilst it is in transit. This makes sure the item returns to it's original location. Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 119901 [details] [review] Bug 12362: Cancel transfer with hold cancelation This patch adds a transfer cancellation when a hold is cancelled. Test plan 1/ Check an item out from it's homebranch 2/ Place a hold on that item for another user at a different branch 3/ Check the item in at it's homebranch and accept the transfer 4/ Cancel the hold 5/ Change to the second branch and check the item in 6/ Note that a transfer is triggered with the reason 'Transfer was cancelled whilst in transit' Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 119902 [details] [review] Bug 12362: (QA follow-up) Fix ModItemTransfer cancellation handling ModItemTransfer is still used, sparingly. It force cancels transfers regardles of whether they are already marked as in_transit. We do not, however, want to enqueue a return transfer in this case as ModItemTransfer is itself an enqueue function. This patch updates the internal logic of ModItemTransfer to enqueue the passed transfer prior to cancelling the pre-existing one.. in this way the result is one cancelled transfer and one new transfer and not three transfers. Test plan 1/ Run t/db_dependant/Items.t and verify it fails prior to applying this patch 2/ Apply patch and run the test again, verifying it now passes. Minor rebase (In reply to Martin Renvoize from comment #4) > I do have a question however... should cancellation of an in_transit > transfer automatically trigger a transfer back to the original location at > the point of cancellation (i.e. prior to the code that handling a return to > home/return to holding at check-in time? What about that? I was going to ask exactly that, spotted in the code that the tests were removed - is( $itemcheck->holdingbranch, $library_1->{branchcode}, 'Lost item with cancelled hold has holding branch equallying the transfers source branch' ); It does not seem like a minor change. The test is actually deleted on bug 27281. I've restored the test in bug 27281, it passes before and after patches are applied. This bug doesn't actually update the items holdingbranch at all. What it does is check the transfer queue for pending transfers and then adds a reverse transfer if no other transfers exist and the transfer was in transit at the time of cancellation. I discussed this with Peter, Joonas and Tomas I believe but I'm struggling to find the thread. We basically decided that now you can queue transfers having the 'request' happen immediately made sense rather than waiting for a subsequent checkin. Sorry, I wasn't clear there. The reason it's an improvement to set up the transfer request immediately is that with the subsequent check-in you can display a clear message as to why it's being transferred back.. without that you are not greeted with a clear reason.. instead the 'return to home/holding' is triggered which could be confusing as you've only just received the item from a transfer. Putting down some investigations: Commit 66f1605c59763613a4e955fd259a845e4fc06324 (bug 21754) introduces the ModItem call to adjust holdingbranch, and the tests. If we checkout this commit and comment the ModItem call, the tests fail with: # Failed test 'Lost item with cancelled hold has holding branch equallying the transfers source branch' # at t/db_dependent/Circulation.t line 2481. # got: 'e9cN2ZmqsE' # expected: 'OIL0DThSY1' It proves that the test is correct. However, on master (f096a8dffc), the tests still pass if we comment the current line: Koha::Items->find($itemnumber)->holdingbranch($frombranch)->store({ skip_record_index => $params->{skip_record_index} }); It may be something we fixed with bug 23129, which patched in ModItemTransfer, but if we look at the code there are conditionals to reach ModItemTransfer. Trusting Martin, Kyle and Nick as I don't have more time to investigate this. (In reply to Jonathan Druart from comment #25) > It may be something we fixed with bug 23129 (Confirmed) Adjusted DBrev to not lose ItemLost from bug 27281. (In reply to Jonathan Druart from comment #27) > Adjusted DBrev to not lose ItemLost from bug 27281. Scratch that. Pushed to master for 21.05, thanks to everybody involved! Depends on Bug 27896 not in 20.11.x |