Bug 12362 - Branch transfer records orphaned on cancelled holds
Summary: Branch transfer records orphaned on cancelled holds
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 27896
Blocks: 24295 27064 28520
  Show dependency treegraph
 
Reported: 2014-06-04 16:56 UTC by Barton Chittenden
Modified: 2021-06-08 08:44 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
This change improves the queue handling for transfer cancellations. With this patch, if a transfer is force-cancelled whilst it is in transit then a reverse transfer will be created to ensure the item gets back to the originating library unless there are already other transfers in the queue, in which case the next transfer in the queue will take precedence.
Version(s) released in:
21.05.00


Attachments
Bug 12362: Add 'TransferCancellation' as a transfer reason (3.07 KB, patch)
2021-03-22 14:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12362: Reverse transfer upon cancellation (1.67 KB, patch)
2021-03-22 14:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12362: Cancel transfer with hold cancelation (1.10 KB, patch)
2021-03-22 14:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12362: Cancel transfer with hold cancelation (3.67 KB, patch)
2021-03-22 15:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12362: Add 'TransferCancellation' as a transfer reason (3.12 KB, patch)
2021-03-25 15:09 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 12362: Reverse transfer upon cancellation (1.72 KB, patch)
2021-03-25 15:09 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 12362: Cancel transfer with hold cancelation (3.70 KB, patch)
2021-03-25 15:09 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 12362: Add 'TransferCancellation' as a transfer reason (3.19 KB, patch)
2021-04-16 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12362: Reverse transfer upon cancellation (1.79 KB, patch)
2021-04-16 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12362: Cancel transfer with hold cancelation (3.77 KB, patch)
2021-04-16 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12362: (QA follow-up) Fix ModItemTransfer cancellation handling (1.70 KB, patch)
2021-04-20 09:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12362: Add 'TransferCancellation' as a transfer reason (3.17 KB, patch)
2021-04-20 09:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12362: Reverse transfer upon cancellation (1.77 KB, patch)
2021-04-20 09:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12362: Cancel transfer with hold cancelation (3.96 KB, patch)
2021-04-20 09:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12362: (QA follow-up) Fix ModItemTransfer cancellation handling (1.70 KB, patch)
2021-04-20 09:59 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2014-06-04 16:56:16 UTC
When a hold is cancelled for an item that has a branch transfer, the branch transfer is not deleted. This causes items to display as 'in transit', even though there is no open hold. This is very confusing to the library and to patrons, and there is often no obvious way to resolve this.
Comment 1 Barton Chittenden 2015-01-09 17:04:28 UTC
See also Bug 13546 - Branch transfer records orphaned when an item is withdrawn
Comment 2 Owen Leonard 2016-08-10 14:17:52 UTC
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?
Comment 3 Martin Renvoize 2021-03-15 17:14:12 UTC
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.
Comment 4 Martin Renvoize 2021-03-22 14:13:55 UTC
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?
Comment 5 Martin Renvoize 2021-03-22 14:59:04 UTC
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.
Comment 6 Martin Renvoize 2021-03-22 14:59:07 UTC
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.
Comment 7 Martin Renvoize 2021-03-22 14:59:10 UTC
Created attachment 118609 [details] [review]
Bug 12362: Cancel transfer with hold cancelation

This patch adds a transfer cancellation when a hold is cancelled.
Comment 8 Martin Renvoize 2021-03-22 15:48:47 UTC
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'
Comment 9 Peter Vashchuk 2021-03-25 15:09:38 UTC
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>
Comment 10 Peter Vashchuk 2021-03-25 15:09:41 UTC
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>
Comment 11 Peter Vashchuk 2021-03-25 15:09:45 UTC
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>
Comment 12 Kyle M Hall 2021-04-16 16:20:00 UTC
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>
Comment 13 Kyle M Hall 2021-04-16 16:20:20 UTC
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>
Comment 14 Kyle M Hall 2021-04-16 16:20:24 UTC
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>
Comment 15 Martin Renvoize 2021-04-20 09:40:06 UTC
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.
Comment 16 Martin Renvoize 2021-04-20 09:59:13 UTC
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>
Comment 17 Martin Renvoize 2021-04-20 09:59:17 UTC
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>
Comment 18 Martin Renvoize 2021-04-20 09:59:21 UTC
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>
Comment 19 Martin Renvoize 2021-04-20 09:59:25 UTC
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.
Comment 20 Martin Renvoize 2021-04-20 09:59:52 UTC
Minor rebase
Comment 21 Jonathan Druart 2021-04-21 08:27:42 UTC
(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.
Comment 22 Jonathan Druart 2021-04-21 08:31:38 UTC
The test is actually deleted on bug 27281.
Comment 23 Martin Renvoize 2021-04-22 13:47:59 UTC
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.
Comment 24 Martin Renvoize 2021-04-22 13:49:57 UTC
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.
Comment 25 Jonathan Druart 2021-04-23 09:21:21 UTC
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.
Comment 26 Jonathan Druart 2021-04-23 09:26:20 UTC
(In reply to Jonathan Druart from comment #25)
> It may be something we fixed with bug 23129

(Confirmed)
Comment 27 Jonathan Druart 2021-04-23 09:44:36 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2021-04-23 09:51:13 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2021-04-23 09:54:54 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 30 Fridolin Somers 2021-04-30 07:48:18 UTC
Depends on Bug 27896 not in 20.11.x