When you cancel a hold that is in transfer to another branch the transfer status disappears from the item info. In the database you can see following transfers created for the items after the cancellation: > mysql> select * from branchtransfers\G > *************************** 1. row *************************** > branchtransfer_id: 1 > itemnumber: 549 > daterequested: 2021-06-07 08:11:05 > datesent: 2021-06-07 08:11:05 > frombranch: MPL > datearrived: NULL > datecancelled: 2021-06-07 08:13:22 > tobranch: CPL > comments: NULL > reason: Reserve > cancellation_reason: CancelReserve > *************************** 2. row *************************** > branchtransfer_id: 2 > itemnumber: 549 > daterequested: 2021-06-07 08:13:22 > datesent: NULL > frombranch: MPL > datearrived: NULL > datecancelled: NULL > tobranch: MPL > comments: NULL > reason: TransferCancellation > cancellation_reason: NULL The code in koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt and catalogue/detail.pl checks whether $transfertwhen is defined but since this new code seems to create a transfer *request* there is not yet a datesent/transfertwhen created and so the transfer is not displayed. Bumbing priority to critical because this leads to librarians think availability of an item is different than what in reality it is and could cause miscommunication between the library and patrons.
Created attachment 121686 [details] [review] Bug 28520: Update detail to display cancelled in transit transfers
Created attachment 121689 [details] [review] Bug 28520: Don't allow simply cancelling a transfer If we just cancel a transfer it will be in a state where we don't know where it originated from / was last seen. Let's treat cancelled transfers also as they would be in transit.
Created attachment 121690 [details] [review] Bug 28520: Allow creating a new transit request for item with cancelled transfer Setting $validTransfer=1 will allow us to trigger automatically a new transfer at check-in time. With this patch applied it is possible to return back home an item with cancelled hold that is in-transit. This is a fix to the regression caused by bug 26078.
Created attachment 121691 [details] [review] Bug 28520: Don't create a new transfer for a lost item We need to pass 'replace' to cancel() method, otherwise a new transfer (request) is automatically generated back to the original sending branch.
Created attachment 121693 [details] [review] Bug 28520: Create cancelled hold's transfer back after previous transfer finishes This fixes regression caused by "Bug 12362: Cancel transfer with hold cancelation" where cancelled hold's transfer didn't show up in intranet and opac because it create a new transfer that was not yet put in in-transit state. The original idea of bug 12362 was to be able to trigger transfer back home if a hold was cancelled. We can do it more simply by setting the $validTransfer variable true in the item check-in code when we are dealing with Reserve transfers. To test: 1) Apply patch 1) Have biblio with item in branch A 2) Create a new hold with a pickup library to branch B 3) Check-in the item at branch A and confirm the hold and transfer 4) Cancel the hold 5) Check-in the hold at branch B and notice it prompt to return it to branch A
Wont this also prompt for return when the reserve hasn't been cancelled?
Ah, I see.. we check for resfound later..
(In reply to Martin Renvoize from comment #6) > Wont this also prompt for return when the reserve hasn't been cancelled? No. I was supposed to write a better commit message but it got late, and working on it now as I got my koha-testing-docker working. Please grep the validTransfer variable and you see later it checks that it doesn't have a hold found.
But.. this will follow 'ReturnToHome/ReturnToHolding' rather than 'Reverse' the original transfer.. I suppose that doesn't really matter.
Created attachment 121708 [details] [review] Bug 28520: Revert "Bug 12362: (QA follow-up) Fix ModItemTransfer cancellation handling" This reverts commit fbba09f63f200d3e57a8612554984d807867cf84. This commit was part of a series adding adding reverse transfers, i.e. transfers that were created for transfers that were cancelled and we wanted the item to return back to its sending library. This however hid the information about transfer happening and we need another approach to the problem.
Created attachment 121709 [details] [review] Bug 28520: Revert "Bug 12362: Cancel transfer with hold cancelation" This reverts commit 6c105829144cd341a4bc65ad8d0ee1f996b423d4. This commit was part of a series adding adding reverse transfers, i.e. transfers that were created for transfers that were cancelled and we wanted the item to return back to its sending library. This however hid the information about transfer happening and we need another approach to the problem.
Created attachment 121710 [details] [review] Bug 28520: Revert "Bug 12362: Reverse transfer upon cancellation" This reverts commit d0407686eb070c5bdf0302147a198027abea5ac0. This commit was part of a series adding adding reverse transfers, i.e. transfers that were created for transfers that were cancelled and we wanted the item to return back to its sending library. This however hid the information about transfer happening and we need another approach to the problem.
Created attachment 121711 [details] [review] Bug 28520: Bring back TransferCancellation reason handling from reverted Bug 12362 The code from Bug 12362 was reverted but there might be still in database transfers with the reason 'TransferCancellation' so we need to restore the code to handle displaying that information.
Created attachment 121712 [details] [review] Bug 28520: Allow creating a transfer back automatically if a hold is canceled during transit This fixes regression caused by "Bug 12362: Cancel transfer with hold cancelation" where cancelled hold's transfer didn't show up in intranet and opac because it create a new transfer that was not yet put in in-transit state. The original idea of bug 12362 was to be able to trigger transfer back home if a hold was cancelled (a regression caused by bug 26078). However, we can do it more simply by setting the $validTransfer variable true in the item check-in code when we are dealing with Reserve transfers. More down in the AddReturn() code there is also a check "and !$resfound" to make sure we only try to trigger the transfer back home automatically if there is no hold waiting at the current location the item arrived in. It should be noted however that now we only display generic message for the automatic transfer reason. Bug 12362 made the return display as the reason "Transfer was cancelled whilst in transit". However, since this fixes the original regressions caused by bug 26078 and restores similar behaviour to that I think giving a more descriptive message for example regarding a hold being cancelled can be considered a further enhancement. To test: 1) Apply patch 1) Have biblio with item in branch A 2) Create a new hold with a pickup library to branch B 3) Check-in the item at branch A and confirm the hold and transfer 4) Cancel the hold 5) Check-in the hold at branch B and notice it prompt to return it to branch A
(In reply to Martin Renvoize from comment #9) > But.. this will follow 'ReturnToHome/ReturnToHolding' rather than 'Reverse' > the original transfer.. I suppose that doesn't really matter. That was actually the original behaviour before bug 26078
Adding 28382 as a dependency, else we won't catch the case where the transfer was actioned by branchtransfers.
Created attachment 121718 [details] [review] Bug 28520: Revert "Bug 12362: (QA follow-up) Fix ModItemTransfer cancellation handling" This reverts commit fbba09f63f200d3e57a8612554984d807867cf84. This commit was part of a series adding adding reverse transfers, i.e. transfers that were created for transfers that were cancelled and we wanted the item to return back to its sending library. This however hid the information about transfer happening and we need another approach to the problem. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 121719 [details] [review] Bug 28520: Revert "Bug 12362: Cancel transfer with hold cancelation" This reverts commit 6c105829144cd341a4bc65ad8d0ee1f996b423d4. This commit was part of a series adding adding reverse transfers, i.e. transfers that were created for transfers that were cancelled and we wanted the item to return back to its sending library. This however hid the information about transfer happening and we need another approach to the problem. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 121720 [details] [review] Bug 28520: Revert "Bug 12362: Reverse transfer upon cancellation" This reverts commit d0407686eb070c5bdf0302147a198027abea5ac0. This commit was part of a series adding adding reverse transfers, i.e. transfers that were created for transfers that were cancelled and we wanted the item to return back to its sending library. This however hid the information about transfer happening and we need another approach to the problem. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 121721 [details] [review] Bug 28520: Bring back TransferCancellation reason handling from reverted Bug 12362 The code from Bug 12362 was reverted but there might be still in database transfers with the reason 'TransferCancellation' so we need to restore the code to handle displaying that information. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 121722 [details] [review] Bug 28520: Allow creating a transfer back automatically if a hold is canceled during transit This fixes regression caused by "Bug 12362: Cancel transfer with hold cancelation" where cancelled hold's transfer didn't show up in intranet and opac because it create a new transfer that was not yet put in in-transit state. The original idea of bug 12362 was to be able to trigger transfer back home if a hold was cancelled (a regression caused by bug 26078). However, we can do it more simply by setting the $validTransfer variable true in the item check-in code when we are dealing with Reserve transfers. More down in the AddReturn() code there is also a check "and !$resfound" to make sure we only try to trigger the transfer back home automatically if there is no hold waiting at the current location the item arrived in. It should be noted however that now we only display generic message for the automatic transfer reason. Bug 12362 made the return display as the reason "Transfer was cancelled whilst in transit". However, since this fixes the original regressions caused by bug 26078 and restores similar behaviour to that I think giving a more descriptive message for example regarding a hold being cancelled can be considered a further enhancement. To test: 1) Apply patch 1) Have biblio with item in branch A 2) Create a new hold with a pickup library to branch B 3) Check-in the item at branch A and confirm the hold and transfer 4) Cancel the hold 5) Check-in the hold at branch B and notice it prompt to return it to branch A Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This resolves the problem as advertised, but it limited to special casing Hold Cancellations.. in the subsequent bug I propose we update the handling to generically resolve this for other cancelled transfers. Signing off
Created attachment 121843 [details] [review] Bug 28520: Revert "Bug 12362: (QA follow-up) Fix ModItemTransfer cancellation handling" This reverts commit fbba09f63f200d3e57a8612554984d807867cf84. This commit was part of a series adding adding reverse transfers, i.e. transfers that were created for transfers that were cancelled and we wanted the item to return back to its sending library. This however hid the information about transfer happening and we need another approach to the problem. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 121844 [details] [review] Bug 28520: Revert "Bug 12362: Cancel transfer with hold cancelation" This reverts commit 6c105829144cd341a4bc65ad8d0ee1f996b423d4. This commit was part of a series adding adding reverse transfers, i.e. transfers that were created for transfers that were cancelled and we wanted the item to return back to its sending library. This however hid the information about transfer happening and we need another approach to the problem. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 121845 [details] [review] Bug 28520: Revert "Bug 12362: Reverse transfer upon cancellation" This reverts commit d0407686eb070c5bdf0302147a198027abea5ac0. This commit was part of a series adding adding reverse transfers, i.e. transfers that were created for transfers that were cancelled and we wanted the item to return back to its sending library. This however hid the information about transfer happening and we need another approach to the problem. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 121846 [details] [review] Bug 28520: Bring back TransferCancellation reason handling from reverted Bug 12362 The code from Bug 12362 was reverted but there might be still in database transfers with the reason 'TransferCancellation' so we need to restore the code to handle displaying that information. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 121847 [details] [review] Bug 28520: Allow creating a transfer back automatically if a hold is canceled during transit This fixes regression caused by "Bug 12362: Cancel transfer with hold cancelation" where cancelled hold's transfer didn't show up in intranet and opac because it create a new transfer that was not yet put in in-transit state. The original idea of bug 12362 was to be able to trigger transfer back home if a hold was cancelled (a regression caused by bug 26078). However, we can do it more simply by setting the $validTransfer variable true in the item check-in code when we are dealing with Reserve transfers. More down in the AddReturn() code there is also a check "and !$resfound" to make sure we only try to trigger the transfer back home automatically if there is no hold waiting at the current location the item arrived in. It should be noted however that now we only display generic message for the automatic transfer reason. Bug 12362 made the return display as the reason "Transfer was cancelled whilst in transit". However, since this fixes the original regressions caused by bug 26078 and restores similar behaviour to that I think giving a more descriptive message for example regarding a hold being cancelled can be considered a further enhancement. To test: 1) Apply patch 1) Have biblio with item in branch A 2) Create a new hold with a pickup library to branch B 3) Check-in the item at branch A and confirm the hold and transfer 4) Cancel the hold 5) Check-in the hold at branch B and notice it prompt to return it to branch A Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
This patchset conflicts with bug 24434, I cannot apply both of them. Can you add a dependency and fix the conflict please? (adding the same comment there)
Created attachment 121951 [details] [review] Bug 28520: Revert "Bug 12362: (QA follow-up) Fix ModItemTransfer cancellation handling" This reverts commit fbba09f63f200d3e57a8612554984d807867cf84. This commit was part of a series adding adding reverse transfers, i.e. transfers that were created for transfers that were cancelled and we wanted the item to return back to its sending library. This however hid the information about transfer happening and we need another approach to the problem. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 121952 [details] [review] Bug 28520: Revert "Bug 12362: Cancel transfer with hold cancelation" This reverts commit 6c105829144cd341a4bc65ad8d0ee1f996b423d4. This commit was part of a series adding adding reverse transfers, i.e. transfers that were created for transfers that were cancelled and we wanted the item to return back to its sending library. This however hid the information about transfer happening and we need another approach to the problem. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 121953 [details] [review] Bug 28520: Revert "Bug 12362: Reverse transfer upon cancellation" This reverts commit d0407686eb070c5bdf0302147a198027abea5ac0. This commit was part of a series adding adding reverse transfers, i.e. transfers that were created for transfers that were cancelled and we wanted the item to return back to its sending library. This however hid the information about transfer happening and we need another approach to the problem. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 121954 [details] [review] Bug 28520: Bring back TransferCancellation reason handling from reverted Bug 12362 The code from Bug 12362 was reverted but there might be still in database transfers with the reason 'TransferCancellation' so we need to restore the code to handle displaying that information. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 121955 [details] [review] Bug 28520: Allow creating a transfer back automatically if a hold is canceled during transit This fixes regression caused by "Bug 12362: Cancel transfer with hold cancelation" where cancelled hold's transfer didn't show up in intranet and opac because it create a new transfer that was not yet put in in-transit state. The original idea of bug 12362 was to be able to trigger transfer back home if a hold was cancelled (a regression caused by bug 26078). However, we can do it more simply by setting the $validTransfer variable true in the item check-in code when we are dealing with Reserve transfers. More down in the AddReturn() code there is also a check "and !$resfound" to make sure we only try to trigger the transfer back home automatically if there is no hold waiting at the current location the item arrived in. It should be noted however that now we only display generic message for the automatic transfer reason. Bug 12362 made the return display as the reason "Transfer was cancelled whilst in transit". However, since this fixes the original regressions caused by bug 26078 and restores similar behaviour to that I think giving a more descriptive message for example regarding a hold being cancelled can be considered a further enhancement. To test: 1) Apply patch 1) Have biblio with item in branch A 2) Create a new hold with a pickup library to branch B 3) Check-in the item at branch A and confirm the hold and transfer 4) Cancel the hold 5) Check-in the hold at branch B and notice it prompt to return it to branch A Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Ok, rebased on top of bug 24434. Though, let's remove the dependency after pushing because it is not one.
# Failed test 'No tests run for subtest "from_library relation tests"' # at /kohadevbox/koha/t/db_dependent/Koha/Item/Transfer.t line 75. The method Koha::Item::Transfer->from_library is not covered by tests! Looks like there are remaining tests.
(In reply to Jonathan Druart from comment #35) > # Failed test 'No tests run for subtest "from_library relation tests"' > # at /kohadevbox/koha/t/db_dependent/Koha/Item/Transfer.t line 75. > The method Koha::Item::Transfer->from_library is not covered by tests! > > Looks like there are remaining tests. Shouldn't we actually keep the sub instead of removing the tests?
Created attachment 121985 [details] [review] Bug 28520: Remove tests for removed from_library function The function was unused and removed.
(In reply to Jonathan Druart from comment #36) > (In reply to Jonathan Druart from comment #35) > > # Failed test 'No tests run for subtest "from_library relation tests"' > > # at /kohadevbox/koha/t/db_dependent/Koha/Item/Transfer.t line 75. > > The method Koha::Item::Transfer->from_library is not covered by tests! > > > > Looks like there are remaining tests. > > Shouldn't we actually keep the sub instead of removing the tests? Nobody is using the code so why keep it around? Creates more work if something needs to be changed that affects this unused code. Attached patch to remove the test.
Ack.. that's a pet hate of mine.. that we don't pass through existing relationships so you have to create them each time if you want to use them (and add tests too). In this case, I do wonder about removing them.. remember, methods can be used by notices too.. so the 'from_library' may well have been helpful for peoples notice templates so personally I'd have kept it.
(In reply to Martin Renvoize from comment #39) > In this case, I do wonder about removing them.. remember, methods can be > used by notices too.. so the 'from_library' may well have been helpful for > peoples notice templates so personally I'd have kept it. That sounds bad, I think we should be explicit about the methods allowed to be used in the TT notice templates. And we remove all the time object methods... I'd still remove it and maybe we can open a new feature request for defining some stable methods for TT notices?
(In reply to Joonas Kylmälä from comment #40) > (In reply to Martin Renvoize from comment #39) > > In this case, I do wonder about removing them.. remember, methods can be > > used by notices too.. so the 'from_library' may well have been helpful for > > peoples notice templates so personally I'd have kept it. > > That sounds bad, I think we should be explicit about the methods allowed to > be used in the TT notice templates. And we remove all the time object > methods... I'd still remove it and maybe we can open a new feature request > for defining some stable methods for TT notices? Martin, though, up to you, if you wanna restore it now please feel free to submit patch.
Created attachment 122005 [details] [review] Bug 28520: Revert "Bug 12362: Reverse transfer upon cancellation" This reverts commit d0407686eb070c5bdf0302147a198027abea5ac0. This commit was part of a series adding adding reverse transfers, i.e. transfers that were created for transfers that were cancelled and we wanted the item to return back to its sending library. This however hid the information about transfer happening and we need another approach to the problem. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 122006 [details] [review] Bug 28520: Bring back TransferCancellation reason handling from reverted Bug 12362 The code from Bug 12362 was reverted but there might be still in database transfers with the reason 'TransferCancellation' so we need to restore the code to handle displaying that information. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 122007 [details] [review] Bug 28520: Allow creating a transfer back automatically if a hold is canceled during transit This fixes regression caused by "Bug 12362: Cancel transfer with hold cancelation" where cancelled hold's transfer didn't show up in intranet and opac because it create a new transfer that was not yet put in in-transit state. The original idea of bug 12362 was to be able to trigger transfer back home if a hold was cancelled (a regression caused by bug 26078). However, we can do it more simply by setting the $validTransfer variable true in the item check-in code when we are dealing with Reserve transfers. More down in the AddReturn() code there is also a check "and !$resfound" to make sure we only try to trigger the transfer back home automatically if there is no hold waiting at the current location the item arrived in. It should be noted however that now we only display generic message for the automatic transfer reason. Bug 12362 made the return display as the reason "Transfer was cancelled whilst in transit". However, since this fixes the original regressions caused by bug 26078 and restores similar behaviour to that I think giving a more descriptive message for example regarding a hold being cancelled can be considered a further enhancement. To test: 1) Apply patch 1) Have biblio with item in branch A 2) Create a new hold with a pickup library to branch B 3) Check-in the item at branch A and confirm the hold and transfer 4) Cancel the hold 5) Check-in the hold at branch B and notice it prompt to return it to branch A Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
I've decided to restore the sub from the patch (patch amended). It makes sense to have from_library if we have to_library. Even if not used yet we are certainly going to need it soon.
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.01
Depends on Bug 12362 not in 20.11.x