Bug 28520 - Cancelling a hold that is in transit hides item's transit status
Summary: Cancelling a hold that is in transit hides item's transit status
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Joonas Kylmälä
QA Contact: Testopia
URL:
Keywords:
Depends on: 12362 24434 28382
Blocks: 28527
  Show dependency treegraph
 
Reported: 2021-06-07 09:05 UTC by Joonas Kylmälä
Modified: 2021-06-21 13:11 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.01


Attachments
Bug 28520: Update detail to display cancelled in transit transfers (2.43 KB, patch)
2021-06-07 15:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28520: Don't allow simply cancelling a transfer (2.06 KB, patch)
2021-06-07 17:13 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Allow creating a new transit request for item with cancelled transfer (1.12 KB, patch)
2021-06-07 17:13 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Don't create a new transfer for a lost item (955 bytes, patch)
2021-06-07 17:13 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Create cancelled hold's transfer back after previous transfer finishes (2.88 KB, patch)
2021-06-07 18:56 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Revert "Bug 12362: (QA follow-up) Fix ModItemTransfer cancellation handling" (1.47 KB, patch)
2021-06-08 08:40 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Revert "Bug 12362: Cancel transfer with hold cancelation" (3.83 KB, patch)
2021-06-08 08:40 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Revert "Bug 12362: Reverse transfer upon cancellation" (1.91 KB, patch)
2021-06-08 08:41 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Bring back TransferCancellation reason handling from reverted Bug 12362 (1.37 KB, patch)
2021-06-08 08:41 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Allow creating a transfer back automatically if a hold is canceled during transit (4.44 KB, patch)
2021-06-08 08:41 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Revert "Bug 12362: (QA follow-up) Fix ModItemTransfer cancellation handling" (1.55 KB, patch)
2021-06-08 12:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28520: Revert "Bug 12362: Cancel transfer with hold cancelation" (4.01 KB, patch)
2021-06-08 12:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28520: Revert "Bug 12362: Reverse transfer upon cancellation" (1.98 KB, patch)
2021-06-08 12:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28520: Bring back TransferCancellation reason handling from reverted Bug 12362 (1.44 KB, patch)
2021-06-08 12:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28520: Allow creating a transfer back automatically if a hold is canceled during transit (4.50 KB, patch)
2021-06-08 12:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28520: Revert "Bug 12362: (QA follow-up) Fix ModItemTransfer cancellation handling" (1.61 KB, patch)
2021-06-11 13:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28520: Revert "Bug 12362: Cancel transfer with hold cancelation" (4.08 KB, patch)
2021-06-11 13:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28520: Revert "Bug 12362: Reverse transfer upon cancellation" (2.03 KB, patch)
2021-06-11 13:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28520: Bring back TransferCancellation reason handling from reverted Bug 12362 (1.49 KB, patch)
2021-06-11 13:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28520: Allow creating a transfer back automatically if a hold is canceled during transit (4.56 KB, patch)
2021-06-11 13:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28520: Revert "Bug 12362: (QA follow-up) Fix ModItemTransfer cancellation handling" (1.61 KB, patch)
2021-06-15 05:52 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Revert "Bug 12362: Cancel transfer with hold cancelation" (4.19 KB, patch)
2021-06-15 05:52 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Revert "Bug 12362: Reverse transfer upon cancellation" (2.03 KB, patch)
2021-06-15 05:52 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Bring back TransferCancellation reason handling from reverted Bug 12362 (1.49 KB, patch)
2021-06-15 05:52 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Allow creating a transfer back automatically if a hold is canceled during transit (4.56 KB, patch)
2021-06-15 05:52 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Remove tests for removed from_library function (1.70 KB, patch)
2021-06-15 10:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28520: Revert "Bug 12362: Reverse transfer upon cancellation" (2.03 KB, patch)
2021-06-15 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28520: Bring back TransferCancellation reason handling from reverted Bug 12362 (1.50 KB, patch)
2021-06-15 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28520: Allow creating a transfer back automatically if a hold is canceled during transit (4.56 KB, patch)
2021-06-15 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2021-06-07 09:05:24 UTC
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.
Comment 1 Martin Renvoize 2021-06-07 15:12:06 UTC
Created attachment 121686 [details] [review]
Bug 28520: Update detail to display cancelled in transit transfers
Comment 2 Joonas Kylmälä 2021-06-07 17:13:09 UTC
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.
Comment 3 Joonas Kylmälä 2021-06-07 17:13:13 UTC
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.
Comment 4 Joonas Kylmälä 2021-06-07 17:13:18 UTC
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.
Comment 5 Joonas Kylmälä 2021-06-07 18:56:10 UTC
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
Comment 6 Martin Renvoize 2021-06-08 06:52:38 UTC
Wont this also prompt for return when the reserve hasn't been cancelled?
Comment 7 Martin Renvoize 2021-06-08 06:53:31 UTC
Ah, I see.. we check for resfound later..
Comment 8 Joonas Kylmälä 2021-06-08 06:54:06 UTC
(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.
Comment 9 Martin Renvoize 2021-06-08 06:54:43 UTC
But.. this will follow 'ReturnToHome/ReturnToHolding' rather than 'Reverse' the original transfer.. I suppose that doesn't really matter.
Comment 10 Joonas Kylmälä 2021-06-08 08:40:53 UTC
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.
Comment 11 Joonas Kylmälä 2021-06-08 08:40:58 UTC
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.
Comment 12 Joonas Kylmälä 2021-06-08 08:41:02 UTC
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.
Comment 13 Joonas Kylmälä 2021-06-08 08:41:07 UTC
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.
Comment 14 Joonas Kylmälä 2021-06-08 08:41:12 UTC
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
Comment 15 Joonas Kylmälä 2021-06-08 09:02:17 UTC
(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
Comment 16 Martin Renvoize 2021-06-08 11:01:47 UTC
Adding 28382 as a dependency, else we won't catch the case where the transfer was actioned by branchtransfers.
Comment 17 Martin Renvoize 2021-06-08 12:05:33 UTC
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>
Comment 18 Martin Renvoize 2021-06-08 12:05:37 UTC
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>
Comment 19 Martin Renvoize 2021-06-08 12:05:41 UTC
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>
Comment 20 Martin Renvoize 2021-06-08 12:05:45 UTC
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>
Comment 21 Martin Renvoize 2021-06-08 12:05:49 UTC
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>
Comment 22 Martin Renvoize 2021-06-08 12:15:08 UTC
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
Comment 23 Nick Clemens 2021-06-11 13:46:45 UTC
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>
Comment 24 Nick Clemens 2021-06-11 13:46:48 UTC
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>
Comment 25 Nick Clemens 2021-06-11 13:46:52 UTC
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>
Comment 26 Nick Clemens 2021-06-11 13:46:55 UTC
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>
Comment 27 Nick Clemens 2021-06-11 13:46:59 UTC
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>
Comment 28 Jonathan Druart 2021-06-14 14:15:03 UTC
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)
Comment 29 Joonas Kylmälä 2021-06-15 05:52:10 UTC
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>
Comment 30 Joonas Kylmälä 2021-06-15 05:52:15 UTC
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>
Comment 31 Joonas Kylmälä 2021-06-15 05:52:19 UTC
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>
Comment 32 Joonas Kylmälä 2021-06-15 05:52:24 UTC
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>
Comment 33 Joonas Kylmälä 2021-06-15 05:52:28 UTC
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>
Comment 34 Joonas Kylmälä 2021-06-15 05:53:41 UTC
Ok, rebased on top of bug 24434. Though, let's remove the dependency after pushing because it is not one.
Comment 35 Jonathan Druart 2021-06-15 09:56:24 UTC
#   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.
Comment 36 Jonathan Druart 2021-06-15 10:00:36 UTC
(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?
Comment 37 Joonas Kylmälä 2021-06-15 10:14:28 UTC
Created attachment 121985 [details] [review]
Bug 28520: Remove tests for removed from_library function

The function was unused and removed.
Comment 38 Joonas Kylmälä 2021-06-15 10:15:53 UTC
(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.
Comment 39 Martin Renvoize 2021-06-15 10:39:47 UTC
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.
Comment 40 Joonas Kylmälä 2021-06-15 10:45:23 UTC
(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?
Comment 41 Joonas Kylmälä 2021-06-15 10:48:48 UTC
(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.
Comment 42 Jonathan Druart 2021-06-15 13:46:48 UTC
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>
Comment 43 Jonathan Druart 2021-06-15 13:46:53 UTC
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>
Comment 44 Jonathan Druart 2021-06-15 13:46:58 UTC
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>
Comment 45 Jonathan Druart 2021-06-15 13:48:27 UTC
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.
Comment 46 Jonathan Druart 2021-06-15 14:29:12 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 47 Kyle M Hall 2021-06-18 14:47:24 UTC
Pushed to 21.05.x for 21.05.01
Comment 48 Fridolin Somers 2021-06-21 13:11:51 UTC
Depends on Bug 12362 not in 20.11.x