When a hold is confirmed and its pickup location is different from the item's location, a transfer is initiated. If the hold is cancelled, the item remains in transfer whereas it is no longer needed at the transfer destination. Is this normal behaviour ? It seems that staff users should at least see a message about the transfer and maybe have the ability to cancel it. Steps to reproduce: 1. Create an item at library A 2. Place a hold on this item with a pickup location at library B 3. Check in the item at library A 4. Confirm the hold. The item should now be in transit to library B 5. Cancel the hold. There is no message about the probably useless transfer.
Created attachment 168198 [details] [review] Bug 34756: Cancelling a hold does not cancel the related transfer When a hold is confirmed and its pickup location is different from the item's location, a transfer is initiated. If the hold is cancelled, the item remains in transfer whereas it is no longer needed at the transfer destination. Is this normal behaviour ? It seems that staff users should at least see a message about the transfer and maybe have the ability to cancel it. Steps to reproduce: 1. Create an item at library A 2. Place a hold on this item with a pickup location at library B 3. Check in the item at library A 4. Confirm the hold. The item should now be in transit to library B 5. Cancel the hold. There is no message about the probably useless transfer. NEW TEST PLAN 1. Create an item at library A 2. Place a hold on this item with a pickup location at library B 3. Check in the item at library A 3-bis. Try to cancel the hold: notice there is no message 4. Confirm the hold. The item should now be in transit to library B 5. Cancel the hold. There is now a message to propose you to cancel the related transfer 6. Do not check the checkbox. There is no hold but still a transfer 7. Repeat the whole process checking the checkbox. Thers is no hold and the transfer has been cancelled.
Created attachment 168313 [details] [review] Bug 34756: Cancelling a hold does not cancel the related transfer When a hold is confirmed and its pickup location is different from the item's location, a transfer is initiated. If the hold is cancelled, the item remains in transfer whereas it is no longer needed at the transfer destination. Is this normal behaviour ? It seems that staff users should at least see a message about the transfer and maybe have the ability to cancel it. Steps to reproduce: 1. Create an item at library A 2. Place a hold on this item with a pickup location at library B 3. Check in the item at library A 4. Confirm the hold. The item should now be in transit to library B 5. Cancel the hold. There is no message about the probably useless transfer. NEW TEST PLAN 1. Create an item at library A 2. Place a hold on this item with a pickup location at library B 3. Check in the item at library A 3-bis. Try to cancel the hold: notice there is no message 4. Confirm the hold. The item should now be in transit to library B 5. Cancel the hold. There is now a message to propose you to cancel the related transfer 6. Do not check the checkbox. There is no hold but still a transfer 7. Repeat the whole process checking the checkbox. Thers is no hold and the transfer has been cancelled. Signed-off-by: Jake Deery <jake.deery@ptfs-europe.com>
Ran through the test plan and looks all good to me. It would be nice if, when checking in at Library B, the message said something like "return cancelled hold" instead of "return this item to library a" I'm nitpicking, though, and this is probably not within the scope of this bug. Signed off with thanks
Hi ! Thank you so much for signing off and commenting :) I created a bug for your proposal, https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37299 I'll work on it once the QA validates the approach chosen in this one.
Created attachment 173138 [details] [review] Bug 34756: Cancelling a hold does not cancel the related transfer When a hold is confirmed and its pickup location is different from the item's location, a transfer is initiated. If the hold is cancelled, the item remains in transfer whereas it is no longer needed at the transfer destination. Is this normal behaviour ? It seems that staff users should at least see a message about the transfer and maybe have the ability to cancel it. Steps to reproduce: 1. Create an item at library A 2. Place a hold on this item with a pickup location at library B 3. Check in the item at library A 4. Confirm the hold. The item should now be in transit to library B 5. Cancel the hold. There is no message about the probably useless transfer. NEW TEST PLAN 1. Create an item at library A 2. Place a hold on this item with a pickup location at library B 3. Check in the item at library A 3-bis. Try to cancel the hold: notice there is no message 4. Confirm the hold. The item should now be in transit to library B 5. Cancel the hold. There is now a message to propose you to cancel the related transfer 6. Do not check the checkbox. There is no hold but still a transfer 7. Repeat the whole process checking the checkbox. Thers is no hold and the transfer has been cancelled. Signed-off-by: Jake Deery <jake.deery@ptfs-europe.com>
There is no change in the diff after the conflict resolution. So it looks very safe to still have the previous signoff valid. (even if the diff would have changed, it depend on if you feel that the changes would need resting) Feel free to get back to need signoff if there is actually something that would warrant retesting.
I really like this idea! On a first review the code generally looks good. There are just a few things I think need to be clarified/adjusted: First, I think the wording on the checkbox could be adjusted to make it a little clearer to library staff what will happen, though. Could it be a drop-down instead of a checkbox? Something like: "Yes - Cancel transfer and keep the item here" "No - Leave the item in transit" Second, from request.pl: + my $transfer = $input->param("transfer-cancel") ? "1" : "0"; my $hold = Koha::Holds->find($reserve_id); + + if( $transfer ) { + warn($hold->item()->get_transfer); + $hold->item()->get_transfer->cancel( { reason => 'Manual', 'force' => '1' } ); + } Why the warn? Also, I think it would be clearer to call the variable $transfer_cancel instead of $transfer, since that's really what it's about at this point in the code.
I'd love to see this behavior in a couple of other places too: - When cancelling a hold from the patron account page - When clicking the "Revert Transit Status" button Not a blocker, just making a note of it for future reference :)
Bugzilla etiquette question. I have just set up a report that looks for items on hold / reserved that haven’t reached the pickup library within a few days so staff can quickly revert the transit status (in the reservation) to make the reservation go live again. But as Emily points out… it doesn’t go live. The In transit status (on the item) stays. While I have managed to get the ‘In transit pending’ status applied that’s not enough to restart the reservation. Should I raise a new bug here- possibly entitled Reverting a hold does not cancel the related transfer?
Hi Ray, yes, filing a new bug is the way to go for that. Thank you for saying something! (Really, I should have filed one when I said something earlier - that was an oversight on my part.) It's also a good idea to put this bug number in the "see also" field when you file the new one, to keep them together since they are related.
> Why the warn? Also, I think it would be clearer to call the variable > $transfer_cancel instead of $transfer, since that's really what it's about at this > point in the code. I ask the mercy of the QA team, this was my first patch on Koha /o/ I totally agree with both points. Concerning the checkbox, I'll think about a better solution: I don't like using dropdown menus when there are two options.
Created attachment 179425 [details] [review] Bug 34756: Cancelling a hold does not cancel the related transfer When a hold is confirmed and its pickup location is different from the item's location, a transfer is initiated. If the hold is cancelled, the item remains in transfer whereas it is no longer needed at the transfer destination. Is this normal behaviour ? It seems that staff users should at least see a message about the transfer and maybe have the ability to cancel it. Steps to reproduce: 1. Create an item at library A 2. Place a hold on this item with a pickup location at library B 3. Check in the item at library A 4. Confirm the hold. The item should now be in transit to library B 5. Cancel the hold. There is no message about the probably useless transfer. NEW TEST PLAN 1. Create an item at library A 2. Place a hold on this item with a pickup location at library B 3. Check in the item at library A 3-bis. Try to cancel the hold: notice there is no message 4. Confirm the hold. The item should now be in transit to library B 5. Cancel the hold. There is now a message to propose you to cancel the related transfer, check the information are correct. 6. Do not check the checkbox. There is no hold but still a transfer 7. Repeat the whole process checking the checkbox. Thers is no hold and the transfer has been cancelled. Signed-off-by: Jake Deery <jake.deery@ptfs-europe.com>
Created attachment 179426 [details] [review] Bug 34756: (follow-up) Rework the transfer cancellation modal
Created attachment 179428 [details] [review] Bug 34756: (follow-up) Rework the transfer cancellation modal
Created attachment 179429 [details] [review] Bug 34756: (follow-up) Rework the transfer cancellation modal
Created attachment 179430 [details] [review] Bug 34756: (follow-up) Rework the transfer cancellation modal
Created attachment 180625 [details] [review] Bug 34756: (follow-up) Rework the transfer cancellation modal Signed-off-by: Mikko Liimatainen <mikko.liimatainen@turku.fi>
This seems to work as intended. However there was an issue when I checked-in an item and confirmed hold transfer, then checked-in the same item again and ignored the transfer and then checked-in the item for a third time and confirmed the transfer. This causes a situation where there are two active transfer and this patch only cancels one. This causes there to be a transfer that should not be there. As far as I can tell this issue is caused independent of this patch so I signed-off on this.
I think this behavior should be baked into Koha::Hold->cancel. And tested. It seems that otherwise we will have to implement the same logic everywhere a hold cancellation happens (there are already comments about other places needing this treatment). As this is a behavior change, we will probably need to add a syspref or circ rule to control this, disabled by default. This could evolve -in separate report- into a more fine grained config. Do we have a way to know a transfer process has been initiated already? I imagine this can become problematic if the item is in a delivery truck already. not a blocker per se, just thinking out loud.
I don't think I agree with putting this in delete : cancelling a hold must not delete the related transfer, for example removing from the OPAC, the user should not even be warned. As a developper, if I delete a hold, I don't want it to delete the associated transfer ; I need to have control over it. Moreover the interaction with the user should not be part of the ORM. Maybe I can move this to another include to make it easier to reuse ? I can add a Syspref but since it only adds a checkbox, which is unchecked by default, I'm not sure it would be a useful one.
Maybe I didn’t explain the whole idea well enough. I think the cancel() method needs to be added a parameter controlling the behavior. And the controller would pass it if conditions are met.
(In reply to Tomás Cohen Arazi (tcohen) from comment #21) > Maybe I didn’t explain the whole idea well enough. > > I think the cancel() method needs to be added a parameter controlling the > behavior. And the controller would pass it if conditions are met. Oh.. I missread, very sorry. I still don't get your point, I'm using transfer->cancel but I have no clue on how I would change it... Do you mean hold->cancel(), that would refuse cancelling if there is no explicit reference to the existing transfer ?
(In reply to Baptiste Wojtkowski (bwoj) from comment #22) > (In reply to Tomás Cohen Arazi (tcohen) from comment #21) > > Maybe I didn’t explain the whole idea well enough. > > > > I think the cancel() method needs to be added a parameter controlling the > > behavior. And the controller would pass it if conditions are met. > > Oh.. I missread, very sorry. > > I still don't get your point, I'm using transfer->cancel but I have no clue > on how I would change it... Do you mean hold->cancel(), that would refuse > cancelling if there is no explicit reference to the existing transfer ? I mean something like ``` diff --git a/Koha/Hold.pm b/Koha/Hold.pm index b3bf4a02b76..b4d185901e1 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -675,6 +675,8 @@ my $cancel_hold = $hold->cancel( [ charge_cancel_fee => 1||0, ] [ cancellation_reason => $cancellation_reason, ] [ skip_holds_queue => 1||0 ] + [ cancel_transfer => 1||0 ] + [ cancel_transfer_reason => 'some valid transfer' ] } ); @@ -703,6 +705,14 @@ sub cancel { $self->cancellation_reason( $params->{cancellation_reason} ); $self->store(); + my $item = $self->item; + + if ( $params->{cancel_transfer} ) { + my $transfer = $item->get_transfer(); + $transfer->cancel( $params->{cancel_transfer_reason} // 'Manual' ) + if $transfer; + } + my $dbh = $self->_result->result_source->schema->storage->dbh; $dbh->do( q{ @@ -766,7 +776,6 @@ sub cancel { # and, if desired, charge a cancel fee if ( $params->{'charge_cancel_fee'} ) { - my $item = $self->item; my $charge = Koha::CirculationRules->get_effective_expire_reserves_charge( { itemtype => $item->effective_itemtype, ```