Bug 40331

Summary: Extra transfer generated when transfer for hold cancelled due to checkin at incorrect library
Product: Koha Reporter: Andrew Fuerste-Henry <andrew>
Component: Hold requestsAssignee: Nick Clemens (kidclamp) <nick>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: david.roberts, gmcharlt, hebah, karie.straube, kebliss, lisette, martin.renvoize, michael.adamyk, mteal, nick, rcoert, Slodico, tomascohen
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 40331: Return existing transfer if requesting a new transfer that is identical
Bug 40331: Return existing transfer if requesting a new transfer that is identical

Description Andrew Fuerste-Henry 2025-07-08 18:46:28 UTC
To recreate:
1 - place a hold on an item that is currently at branch A, for pickup at branch B
2 - check your item in at Branch A, click to confirm the hold and transfer (print slip if you want to)
3 - in reports: SELECT * FROM branchtransfers WHERE itemnumber=YOUR_ITEM AND date(daterequested)=curdate() -- See your transfer
4 - check your item in at Branch A again, get a message that the item is supposed to go to Branch B. Confirm and transfer.
5 - Run your report again, you now see 3 transfers: the original transfer has been cancelled and Koha has generated two new transfers. One of these new transfers has a datesent value, the other does not.

The third transfer, the one that doesn't get Sent right away, ends up orphaned in the system.
Comment 1 Nick Clemens (kidclamp) 2025-07-09 18:47:29 UTC
This seems to be conflicting ideas in the code.

At step 4, we get a WrongTransfer message and pop-up a modal, before displaying that modal we do:
553     # We need to ignore limits here. While we can't transfer from this branch, it is, wrongly, here right now
554     # and that fact must be recorded
555     my $new_transfer = $item->request_transfer(
556         {
557             to            => $old_transfer->to_library,
558             reason        => $old_transfer->reason,
559             replace       => 'WrongTransfer',
560             ignore_limits => 1
561         }
562     );

So one transfer is closed and a new one created. Then when the 'Confirm hold and transfer' button is clicked we hit the cud-affect_reserve code:
156             # Add transfer, enqueue if one is already in the queue, and immediately set to in transit
157             my $transfer = $item->request_transfer( { to => $tobranch, reason => 'Reserve', enqueue => 1 } );
158             $transfer->transit;


So we are intentionally queuing a second transfer - I don't understand the case where the first transfer needs to be left in place - or perhaps we can simply check if the first transfer exists, is going to the correct library, and then simply mark the existing one as in transit from our current location?
Comment 2 Lisette Scheer 2025-07-15 15:23:01 UTC
(In reply to Nick Clemens (kidclamp) from comment #1)
> This seems to be conflicting ideas in the code.
> 
> At step 4, we get a WrongTransfer message and pop-up a modal, before
> displaying that modal we do:
> 553     # We need to ignore limits here. While we can't transfer from this
> branch, it is, wrongly, here right now
> 554     # and that fact must be recorded
> 555     my $new_transfer = $item->request_transfer(
> 556         {
> 557             to            => $old_transfer->to_library,
> 558             reason        => $old_transfer->reason,
> 559             replace       => 'WrongTransfer',
> 560             ignore_limits => 1
> 561         }
> 562     );

So this step updates the original transfer to cancelled with wrongtransfer reason and creates a new transfer

> So one transfer is closed and a new one created. Then when the 'Confirm hold
> and transfer' button is clicked we hit the cud-affect_reserve code:
> 156             # Add transfer, enqueue if one is already in the queue, and
> immediately set to in transit
> 157             my $transfer = $item->request_transfer( { to => $tobranch,
> reason => 'Reserve', enqueue => 1 } );
> 158             $transfer->transit;
> 
> 
> So we are intentionally queuing a second transfer - I don't understand the
> case where the first transfer needs to be left in place - or perhaps we can
> simply check if the first transfer exists, is going to the correct library,
> and then simply mark the existing one as in transit from our current
> location?

Then here we have a new transfer that is created and sent. We should instead be updating the transfer from the previous code.

Should we be showing the "wrong transfer" modal? Currently the "hold-modal" is what shows, but I believe previously "wrong transfer" showed and the transfer could be canceled if needed?
Comment 3 Nick Clemens (kidclamp) 2025-07-25 13:45:43 UTC
Created attachment 184629 [details] [review]
Bug 40331: Return existing transfer if requesting a new transfer that is identical

Currently request_transfer checks for the existence of a current transfer on the item, and a new one can either be
enqueued or replace this transfer. This patch adds a check to see if the requested transfer matches an existing transfer
on the item. In the case that it does we simply return that transfer rather than create a new transfer.

This has the effect of eliminating 'WrongTransfer' generation if an item is checked in twice at the same branch, which feels correct.

To test:
 1 - place a hold on an item that is currently at branch A, for pickup at branch B
 2 - check your item in at Branch A, click to confirm the hold and transfer (print slip if you want to)
 3 - sudo koha-mysql kohadev: SELECT * FROM branchtransfers -- See one transfer
 4 - check your item in at Branch A again, get a message that the item is supposed to go to Branch B. Don't click on the modal yet
 5 - repeat 3 -- See two transfers, one cancelled as WrongTransfer, one pending
 6 - Click 'Confirm and transfer' on the modal
 7 - repeat 3 -- See three tranfers - the two from before and a new active transfer
 8 - DELETE FROM branchtransfers
 9 - Apply patch, restart all
10 - repeat 3 - no transfers
11 - Check item in at branch A again and confirm
12 - repeat 3, one transfer
13 - Check item in, don't confirm the modal
14 - repeat 3, one transfer
15 - Confirm the modal
16 - repeat 3, one transfer
17 - Check the item in at branch C
18 - repeat 3, two transfers - one cancelled WrongTransfer, one active
19 - Confirm modal
20 - repeat 3, two transfers
21 - Check item in at branch B
22 - repeat 3, two transfers, one cancelled, one completed
Comment 4 Andrew Fuerste-Henry 2025-07-25 16:44:10 UTC
Created attachment 184653 [details] [review]
Bug 40331: Return existing transfer if requesting a new transfer that is identical

Currently request_transfer checks for the existence of a current transfer on the item, and a new one can either be
enqueued or replace this transfer. This patch adds a check to see if the requested transfer matches an existing transfer
on the item. In the case that it does we simply return that transfer rather than create a new transfer.

This has the effect of eliminating 'WrongTransfer' generation if an item is checked in twice at the same branch, which feels correct.

To test:
 1 - place a hold on an item that is currently at branch A, for pickup at branch B
 2 - check your item in at Branch A, click to confirm the hold and transfer (print slip if you want to)
 3 - sudo koha-mysql kohadev: SELECT * FROM branchtransfers -- See one transfer
 4 - check your item in at Branch A again, get a message that the item is supposed to go to Branch B. Don't click on the modal yet
 5 - repeat 3 -- See two transfers, one cancelled as WrongTransfer, one pending
 6 - Click 'Confirm and transfer' on the modal
 7 - repeat 3 -- See three tranfers - the two from before and a new active transfer
 8 - DELETE FROM branchtransfers
 9 - Apply patch, restart all
10 - repeat 3 - no transfers
11 - Check item in at branch A again and confirm
12 - repeat 3, one transfer
13 - Check item in, don't confirm the modal
14 - repeat 3, one transfer
15 - Confirm the modal
16 - repeat 3, one transfer
17 - Check the item in at branch C
18 - repeat 3, two transfers - one cancelled WrongTransfer, one active
19 - Confirm modal
20 - repeat 3, two transfers
21 - Check item in at branch B
22 - repeat 3, two transfers, one cancelled, one completed

Signed-off-by: Christine Lee <chlee@pascolibraries.org>