Bug 27064 - Transferring an item with a hold allows the user to set a hold waiting without transferring to the correct branch
Summary: Transferring an item with a hold allows the user to set a hold waiting withou...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 12362
Blocks: 28294 28382 32765
  Show dependency treegraph
 
Reported: 2020-11-20 11:33 UTC by Nick Clemens
Modified: 2023-01-31 20:17 UTC (History)
10 users (show)

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


Attachments
Bug 27064: Only allow transferring a hold from the transfers page (7.75 KB, patch)
2020-11-20 11:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27064: Only allow transferring a hold from the transfers page (7.75 KB, patch)
2020-12-28 12:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27064: Only allow transferring a hold from the transfers page (8.80 KB, patch)
2021-04-27 13:09 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27064: Only allow transferring a hold from the transfers page (8.85 KB, patch)
2021-05-10 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Do not send tobranchcd twice (1.10 KB, patch)
2021-05-10 10:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: Only allow transferring a hold from the transfers page (8.85 KB, patch)
2021-05-17 16:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Do not send tobranchcd twice (1.10 KB, patch)
2021-05-17 16:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Do not create transfer prior to accepting it (1.60 KB, patch)
2021-05-17 16:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: Only allow transferring a hold from the transfers page (8.85 KB, patch)
2021-05-18 08:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Do not send tobranchcd twice (1.10 KB, patch)
2021-05-18 08:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Do not create transfer prior to accepting it (1.60 KB, patch)
2021-05-18 08:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Pass 'replace' through to Koha::Item::Transfer->cancel (4.50 KB, patch)
2021-05-18 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: Only allow transferring a hold from the transfers page (8.85 KB, patch)
2021-05-19 08:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Do not send tobranchcd twice (1.10 KB, patch)
2021-05-19 08:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Do not create transfer prior to accepting it (3.73 KB, patch)
2021-05-19 08:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Pass 'replace' through to Koha::Item::Transfer->cancel (4.50 KB, patch)
2021-05-19 08:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: Only allow transferring a hold from the transfers page (8.85 KB, patch)
2021-06-07 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Do not send tobranchcd twice (1.10 KB, patch)
2021-06-07 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Do not create transfer prior to accepting it (3.73 KB, patch)
2021-06-07 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Pass 'replace' through to Koha::Item::Transfer->cancel (4.50 KB, patch)
2021-06-07 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Remove superflous error message (895 bytes, patch)
2021-06-07 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Add error message only if we have one defined (1.39 KB, patch)
2021-06-07 14:12 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27064: Only allow transferring a hold from the transfers page (9.00 KB, patch)
2021-06-07 14:44 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Do not send tobranchcd twice (1.25 KB, patch)
2021-06-07 14:44 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Do not create transfer prior to accepting it (3.88 KB, patch)
2021-06-07 14:44 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Pass 'replace' through to Koha::Item::Transfer->cancel (4.65 KB, patch)
2021-06-07 14:44 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Add error message only if we have one defined (1.54 KB, patch)
2021-06-07 14:44 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27064: Only allow transferring a hold from the transfers page (9.00 KB, patch)
2021-06-07 14:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Do not send tobranchcd twice (1.31 KB, patch)
2021-06-07 14:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Do not create transfer prior to accepting it (3.94 KB, patch)
2021-06-07 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Pass 'replace' through to Koha::Item::Transfer->cancel (4.71 KB, patch)
2021-06-07 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: (QA follow-up) Add error message only if we have one defined (1.61 KB, patch)
2021-06-07 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27064: Correct transferbook.t tests to reflect new behaviour of transferbook() (5.30 KB, patch)
2021-06-15 09:28 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-11-20 11:33:45 UTC
To recreate:
1 - Place a hold for delivery at Library B
2 - Sign in at Library A
3 - Browse to Circulation->Transfers
4 - Attempt to transfer an item on the title with the hold to Library B
5 - You get a notice that hold was found (missing patron/branch info)
6 - You have the option to set the hold waiting - click it
7 - The transfer is generated and marked completed
8 - The hold is marked as waiting, but the item is still at Library A and no transfer is active
9 - The patron is notified that the hold is waiting
Comment 1 Nick Clemens 2020-11-20 11:37:43 UTC
Created attachment 113877 [details] [review]
Bug 27064: Only allow transferring a hold from the transfers page

These patches replace the 'Waiting' button on the transfers page with a 'Transfer' button
and correct some other related problem by passing the hold object to the template and
using that to fetch patron info as well as passing the reserve_id through to ensure the correct hold is
affected at all times

To test:
 1 - Place a hold for delivery at Library B
 2 - Sign in at Library A
 3 - Browse to Circulation->Transfers
 4 - Attempt to transfer an item on the title with the hold to Library B
 5 - You get a notice that hold was found (missing patron/branch info)
 6 - You have the option to set the hold waiting - click it
 7 - The transfer is generated and marked completed
 8 - The hold is marked as waiting, but the item is still at Library A and no transfer is active
 9 - The patron is notified that the hold is waiting
10 - Revert the hold or cancel and place a new one
11 - Apply patches
12 - Attempt transfer again
13 - You now have the option to transfer the hold
14 - Click that
15 - Hold is in transit and transfer is generated correctly
16 - Transfer again and choose 'cancel'
17 - Confirm hold is cancelled and transfer generated
Comment 2 Victor Grousset/tuxayo 2020-12-26 22:24:34 UTC
Anyone else getting a compilation error after applying the patch, restarting the services and going to Circulation->Transfers ?

Could not compile /kohadevbox/koha/circ/branchtransfers.pl: Global symbol "$holds" requires explicit package name (did you forget to declare "my $holds"?) at /kohadevbox/koha/circ/branchtransfers.pl line 112.
BEGIN not safe after errors--compilation aborted at /kohadevbox/koha/circ/branchtransfers.pl line 246.
Comment 3 Nick Clemens 2020-12-28 12:13:28 UTC
Created attachment 114712 [details] [review]
Bug 27064: Only allow transferring a hold from the transfers page

These patches replace the 'Waiting' button on the transfers page with a 'Transfer' button
and correct some other related problem by passing the hold object to the template and
using that to fetch patron info as well as passing the reserve_id through to ensure the correct hold is
affected at all times

To test:
 1 - Place a hold for delivery at Library B
 2 - Sign in at Library A
 3 - Browse to Circulation->Transfers
 4 - Attempt to transfer an item on the title with the hold to Library B
 5 - You get a notice that hold was found (missing patron/branch info)
 6 - You have the option to set the hold waiting - click it
 7 - The transfer is generated and marked completed
 8 - The hold is marked as waiting, but the item is still at Library A and no transfer is active
 9 - The patron is notified that the hold is waiting
10 - Revert the hold or cancel and place a new one
11 - Apply patches
12 - Attempt transfer again
13 - You now have the option to transfer the hold
14 - Click that
15 - Hold is in transit and transfer is generated correctly
16 - Transfer again and choose 'cancel'
17 - Confirm hold is cancelled and transfer generated
Comment 4 Victor Grousset/tuxayo 2020-12-28 14:22:27 UTC
> 16 - Transfer again and choose 'cancel'

I go back to the "Reserve found" page with the 3 options.
I'm trapped there if I only click 'cancel' ^^"

And the hold is not canceled.
Comment 5 Emmi Takkinen 2020-12-29 10:46:23 UTC
(In reply to Victor Grousset/tuxayo from comment #4)
> > 16 - Transfer again and choose 'cancel'
> 
> I go back to the "Reserve found" page with the 3 options.
> I'm trapped there if I only click 'cancel' ^^"
> 
> And the hold is not canceled.

Can confirm this happening, transfer is generated but hold isn't cancelled. It seems reserve_id is not send as query parameter when 'Cancel' is clicked.
Comment 6 Martin Renvoize 2021-04-27 12:50:36 UTC
Adding Joonas to the CC list as he's turned into the expert in this area of late ;)
Comment 7 Nick Clemens 2021-04-27 13:09:36 UTC
Created attachment 120222 [details] [review]
Bug 27064: Only allow transferring a hold from the transfers page

These patches replace the 'Waiting' button on the transfers page with a 'Transfer' button
and correct some other related problem by passing the hold object to the template and
using that to fetch patron info as well as passing the reserve_id through to ensure the correct hold is
affected at all times

To test:
 1 - Place a hold for delivery at Library B
 2 - Sign in at Library A
 3 - Browse to Circulation->Transfers
 4 - Attempt to transfer an item on the title with the hold to Library B
 5 - You get a notice that hold was found (missing patron/branch info)
 6 - You have the option to set the hold waiting - click it
 7 - The transfer is generated and marked completed
 8 - The hold is marked as waiting, but the item is still at Library A and no transfer is active
 9 - The patron is notified that the hold is waiting
10 - Revert the hold or cancel and place a new one
11 - Apply patches
12 - Attempt transfer again
13 - You now have the option to transfer the hold
14 - Click that
15 - Hold is in transit and transfer is generated correctly
16 - Transfer again and choose 'cancel'
17 - Confirm hold is cancelled and transfer generated
Comment 8 Victor Grousset/tuxayo 2021-05-08 19:10:19 UTC
Seems to works as expected but I see 4 transfers in the UI (Transferred items)
And I'm not sure if the data is correct.

(tried to make the table compact, so stuff was abridged)

--+--------+--------+----+-------+--------+---+------------+-----------
id|dt_rqstd|datesent|from|darrive|dcancel |to |reason      |canc_reason
--+--------+--------+----+-------+--------+---+------------+-----------
↓↓↓↓↓↓↓↓↓↓ 6) set the hold waiting - click it   ↓↓↓↓↓
 1|18:50:39|18:50:39|CPL |8:50:48|        |FFL|Manual      |NULL
↓↓↓↓↓↓↓↓↓↓ Revert the hold or cancel and place a new one (NOT SURE ABOUT THAT)↓↓↓ 
 2|18:52:43|18:52:43|CPL |       |18:53:19|FFL|Manual      |Manual  
↓↓↓↓↓↓↓↓↓ 14) Click that↓↓↓↓↓   
 3|18:53:19|18:53:19|CPL |       |18:53:24|FFL|Manual      |CancelRes  
↓↓↓↓↓↓↓↓↓ 16) choose 'cancel↓↓↓↓↓
 4|18:53:24|        |CPL |       |18:53:24|CPL|TransfCancel|Manual     
 5|18:53:24|18:53:24|CPL |       |        |FFL|Manual      |NULL       
--+--------+--------+----+-------+--------+---+------------+-----------
Comment 9 Martin Renvoize 2021-05-10 09:49:52 UTC
Created attachment 120759 [details] [review]
Bug 27064: Only allow transferring a hold from the transfers page

These patches replace the 'Waiting' button on the transfers page with a 'Transfer' button
and correct some other related problem by passing the hold object to the template and
using that to fetch patron info as well as passing the reserve_id through to ensure the correct hold is
affected at all times

To test:
 1 - Place a hold for delivery at Library B
 2 - Sign in at Library A
 3 - Browse to Circulation->Transfers
 4 - Attempt to transfer an item on the title with the hold to Library B
 5 - You get a notice that hold was found (missing patron/branch info)
 6 - You have the option to set the hold waiting - click it
 7 - The transfer is generated and marked completed
 8 - The hold is marked as waiting, but the item is still at Library A and no transfer is active
 9 - The patron is notified that the hold is waiting
10 - Revert the hold or cancel and place a new one
11 - Apply patches
12 - Attempt transfer again
13 - You now have the option to transfer the hold
14 - Click that
15 - Hold is in transit and transfer is generated correctly
16 - Transfer again and choose 'cancel'
17 - Confirm hold is cancelled and transfer generated

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2021-05-10 10:47:25 UTC
Created attachment 120764 [details] [review]
Bug 27064: (QA follow-up) Do not send tobranchcd twice
Comment 11 Martin Renvoize 2021-05-10 11:05:01 UTC
This works as expected, signing off.

However, I think it may highlight a problem with some of the logic around transfer cancellation and the internal update of ModItemTransfer to use that method.  I'll continue to investigate on another bug.
Comment 12 Martin Renvoize 2021-05-10 11:47:25 UTC
(In reply to Victor Grousset/tuxayo from comment #8)
> Seems to works as expected but I see 4 transfers in the UI (Transferred
> items)
> And I'm not sure if the data is correct.
> 
> (tried to make the table compact, so stuff was abridged)
> 
> --+--------+--------+----+-------+--------+---+------------+-----------
> id|dt_rqstd|datesent|from|darrive|dcancel |to |reason      |canc_reason
> --+--------+--------+----+-------+--------+---+------------+-----------
> ↓↓↓↓↓↓↓↓↓↓ 6) set the hold waiting - click it   ↓↓↓↓↓
>  1|18:50:39|18:50:39|CPL |8:50:48|        |FFL|Manual      |NULL
> ↓↓↓↓↓↓↓↓↓↓ Revert the hold or cancel and place a new one (NOT SURE ABOUT
> THAT)↓↓↓ 
>  2|18:52:43|18:52:43|CPL |       |18:53:19|FFL|Manual      |Manual  
> ↓↓↓↓↓↓↓↓↓ 14) Click that↓↓↓↓↓   
>  3|18:53:19|18:53:19|CPL |       |18:53:24|FFL|Manual      |CancelRes  
> ↓↓↓↓↓↓↓↓↓ 16) choose 'cancel↓↓↓↓↓
>  4|18:53:24|        |CPL |       |18:53:24|CPL|TransfCancel|Manual     
>  5|18:53:24|18:53:24|CPL |       |        |FFL|Manual      |NULL       
> --+--------+--------+----+-------+--------+---+------------+-----------

To highlight the issue I'm referring to using Victors test data above..

It's the final transfer that's an error, not with this bug, but with another that's already in master. It's is a 'return to sending branch' that's triggered by the cancellation.. this happens automatically when the item is perceived to be 'in_transit'.. problem is.. ModItemTransfer always puts items immediately in transit.. but there are times when that isn't the right action and the item hasn't actually left the building as such yet..
Comment 13 Victor Grousset/tuxayo 2021-05-10 18:23:33 UTC
So would the plan be: fix bug 24434 and retest this one?
So, blocked by bug 24434
Comment 14 Martin Renvoize 2021-05-17 16:24:40 UTC
Created attachment 121065 [details] [review]
Bug 27064: Only allow transferring a hold from the transfers page

These patches replace the 'Waiting' button on the transfers page with a 'Transfer' button
and correct some other related problem by passing the hold object to the template and
using that to fetch patron info as well as passing the reserve_id through to ensure the correct hold is
affected at all times

To test:
 1 - Place a hold for delivery at Library B
 2 - Sign in at Library A
 3 - Browse to Circulation->Transfers
 4 - Attempt to transfer an item on the title with the hold to Library B
 5 - You get a notice that hold was found (missing patron/branch info)
 6 - You have the option to set the hold waiting - click it
 7 - The transfer is generated and marked completed
 8 - The hold is marked as waiting, but the item is still at Library A and no transfer is active
 9 - The patron is notified that the hold is waiting
10 - Revert the hold or cancel and place a new one
11 - Apply patches
12 - Attempt transfer again
13 - You now have the option to transfer the hold
14 - Click that
15 - Hold is in transit and transfer is generated correctly
16 - Transfer again and choose 'cancel'
17 - Confirm hold is cancelled and transfer generated

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2021-05-17 16:24:44 UTC
Created attachment 121066 [details] [review]
Bug 27064: (QA follow-up) Do not send tobranchcd twice
Comment 16 Martin Renvoize 2021-05-17 16:24:48 UTC
Created attachment 121067 [details] [review]
Bug 27064: (QA follow-up) Do not create transfer prior to accepting it

This patch prevents the cancellations we were seeing. Transfers were
being created pre-emtively for reserves in branchtransfers before the
librarian had selected their prefered action. As such, we were getting
double lines in the branchtransfers table.
Comment 17 Martin Renvoize 2021-05-17 16:28:17 UTC
Ready for testing again...
Comment 18 Victor Grousset/tuxayo 2021-05-18 00:06:39 UTC
> 15 - Hold is in transit and transfer is generated correctly

Ok I have "Item is now in transit to Fairfield"

Shouldn't this create a record in branchtransfers ?

Shouldn't this be shown in the biblio record details?
In addition to "There is an item level hold on this item (priority = 0)."
Comment 19 Victor Grousset/tuxayo 2021-05-18 00:07:44 UTC
Testing note, run updatedatabase after applying
Comment 20 Martin Renvoize 2021-05-18 08:35:51 UTC
Created attachment 121090 [details] [review]
Bug 27064: Only allow transferring a hold from the transfers page

These patches replace the 'Waiting' button on the transfers page with a 'Transfer' button
and correct some other related problem by passing the hold object to the template and
using that to fetch patron info as well as passing the reserve_id through to ensure the correct hold is
affected at all times

To test:
 1 - Place a hold for delivery at Library B
 2 - Sign in at Library A
 3 - Browse to Circulation->Transfers
 4 - Attempt to transfer an item on the title with the hold to Library B
 5 - You get a notice that hold was found (missing patron/branch info)
 6 - You have the option to set the hold waiting - click it
 7 - The transfer is generated and marked completed
 8 - The hold is marked as waiting, but the item is still at Library A and no transfer is active
 9 - The patron is notified that the hold is waiting
10 - Revert the hold or cancel and place a new one
11 - Apply patches
12 - Attempt transfer again
13 - You now have the option to transfer the hold
14 - Click that
15 - Hold is in transit and transfer is generated correctly
16 - Transfer again and choose 'cancel'
17 - Confirm hold is cancelled and transfer generated

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2021-05-18 08:35:55 UTC
Created attachment 121091 [details] [review]
Bug 27064: (QA follow-up) Do not send tobranchcd twice
Comment 22 Martin Renvoize 2021-05-18 08:35:59 UTC
Created attachment 121092 [details] [review]
Bug 27064: (QA follow-up) Do not create transfer prior to accepting it

This patch prevents the cancellations we were seeing. Transfers were
being created pre-emtively for reserves in branchtransfers before the
librarian had selected their prefered action. As such, we were getting
double lines in the branchtransfers table.
Comment 23 Martin Renvoize 2021-05-18 08:36:03 UTC
Created attachment 121093 [details] [review]
Bug 27064: (QA follow-up) Pass 'replace' through to Koha::Item::Transfer->cancel

The `replace` option found in Koha::Item->request_transfer should be
passed through to Koha::Item::Transfer->cancel and prevent any reverse
transfers from being queued in such cases.

This prevents modItemTransfer from adding superflous reverse transfers
whenever it is used.

Test plan
1/ Run t/db_dependent/Koha/Item.t to prove the new tests pass
Comment 24 Victor Grousset/tuxayo 2021-05-18 15:54:32 UTC
(In reply to Victor Grousset/tuxayo from comment #18)
> > 15 - Hold is in transit and transfer is generated correctly
> 
> Ok I have "Item is now in transit to Fairfield"
> 
> Shouldn't this create a record in branchtransfers ?

note from IRC, to not forget: Martin: «for your issue with step 15.. I get a line in branchtransfers.. so a little confused»

> Shouldn't this be shown in the biblio record details?
> In addition to "There is an item level hold on this item (priority = 0)."

from IRC: Martin: «yes, once you've clicked to transfer you should then see the line set and the biblio should view should include that info.»

There is the info about the hold. But should there also be an info about the transit? There is none as of yesterday's test.
Comment 25 Victor Grousset/tuxayo 2021-05-18 17:55:26 UTC
> 15 - Hold is in transit and transfer is generated correctly

I'm still not getting a record added to the branchtransfers table >_<

And the hold tab says that it's in transfer, where is that even stored...
Comment 26 Victor Grousset/tuxayo 2021-05-18 18:05:53 UTC
Likely nothing to do with these changes, but maybe I did something wrong in testing. Is that expected that
1. be in library A
2. have an item in library C
3. start a transfer to library B
4. record detail view still says that it's in library C. Should scanning of it's barcode in lib A means that it's in lib A?
Comment 27 Victor Grousset/tuxayo 2021-05-18 18:45:59 UTC
> And the hold tab says that it's in transfer, where is that even stored...

A diff on database dump before and after the transfer shows that the info is from the reserves table.
found = T => Hold is in transit
and the reserves table stores the pickup library so here we have the data behind the message.

And nothing added in the branchtransfers table. Puzzling.

Should we switch to need signoff to have more chances of someone else trying this out?
Comment 28 Martin Renvoize 2021-05-19 08:23:53 UTC
Created attachment 121141 [details] [review]
Bug 27064: Only allow transferring a hold from the transfers page

These patches replace the 'Waiting' button on the transfers page with a 'Transfer' button
and correct some other related problem by passing the hold object to the template and
using that to fetch patron info as well as passing the reserve_id through to ensure the correct hold is
affected at all times

To test:
 1 - Place a hold for delivery at Library B
 2 - Sign in at Library A
 3 - Browse to Circulation->Transfers
 4 - Attempt to transfer an item on the title with the hold to Library B
 5 - You get a notice that hold was found (missing patron/branch info)
 6 - You have the option to set the hold waiting - click it
 7 - The transfer is generated and marked completed
 8 - The hold is marked as waiting, but the item is still at Library A and no transfer is active
 9 - The patron is notified that the hold is waiting
10 - Revert the hold or cancel and place a new one
11 - Apply patches
12 - Attempt transfer again
13 - You now have the option to transfer the hold
14 - Click that
15 - Hold is in transit and transfer is generated correctly
16 - Transfer again and choose 'cancel'
17 - Confirm hold is cancelled and transfer generated

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2021-05-19 08:23:57 UTC
Created attachment 121142 [details] [review]
Bug 27064: (QA follow-up) Do not send tobranchcd twice
Comment 30 Martin Renvoize 2021-05-19 08:24:02 UTC
Created attachment 121143 [details] [review]
Bug 27064: (QA follow-up) Do not create transfer prior to accepting it

This patch prevents the cancellations we were seeing. Transfers were
being created pre-emtively for reserves in branchtransfers before the
librarian had selected their prefered action. As such, we were getting
double lines in the branchtransfers table.
Comment 31 Martin Renvoize 2021-05-19 08:24:06 UTC
Created attachment 121144 [details] [review]
Bug 27064: (QA follow-up) Pass 'replace' through to Koha::Item::Transfer->cancel

The `replace` option found in Koha::Item->request_transfer should be
passed through to Koha::Item::Transfer->cancel and prevent any reverse
transfers from being queued in such cases.

This prevents modItemTransfer from adding superflous reverse transfers
whenever it is used.

Test plan
1/ Run t/db_dependent/Koha/Item.t to prove the new tests pass
Comment 32 Victor Grousset/tuxayo 2021-05-19 14:00:07 UTC
> 15 - Hold is in transit and transfer is generated correctly

record detail page:

>  In transit from Centerville to Fairfield since 05/19/2021 There is an item level hold on this item (priority = 0). 

nice :D


and branchtransfers have a new line! yay!

---

> 16 - Transfer again and choose 'cancel'
> 17 - Confirm hold is cancelled and transfer generated

|8|657|39:09|NULL |CPL|NUL|39:09|CPL|NUL|TransferCancellation|Manual
|9|657|39:09|39:09|CPL|NUL|NUL  |FFL|NUL|Manual              |NULL  

Is that what it should be?


--- 

> 15 - Hold is in transit and transfer is generated correctly

There is a second box
https://wtf.roflcopter.fr/pics/2gNHxIgK/9YCZaWRZ.png

It's new, after checking it's not there with the previous patches, from comment 20
Comment 33 Martin Renvoize 2021-06-07 13:31:03 UTC
Created attachment 121670 [details] [review]
Bug 27064: Only allow transferring a hold from the transfers page

These patches replace the 'Waiting' button on the transfers page with a 'Transfer' button
and correct some other related problem by passing the hold object to the template and
using that to fetch patron info as well as passing the reserve_id through to ensure the correct hold is
affected at all times

To test:
 1 - Place a hold for delivery at Library B
 2 - Sign in at Library A
 3 - Browse to Circulation->Transfers
 4 - Attempt to transfer an item on the title with the hold to Library B
 5 - You get a notice that hold was found (missing patron/branch info)
 6 - You have the option to set the hold waiting - click it
 7 - The transfer is generated and marked completed
 8 - The hold is marked as waiting, but the item is still at Library A and no transfer is active
 9 - The patron is notified that the hold is waiting
10 - Revert the hold or cancel and place a new one
11 - Apply patches
12 - Attempt transfer again
13 - You now have the option to transfer the hold
14 - Click that
15 - Hold is in transit and transfer is generated correctly
16 - Transfer again and choose 'cancel'
17 - Confirm hold is cancelled and transfer generated

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2021-06-07 13:31:07 UTC
Created attachment 121671 [details] [review]
Bug 27064: (QA follow-up) Do not send tobranchcd twice
Comment 35 Martin Renvoize 2021-06-07 13:31:12 UTC
Created attachment 121672 [details] [review]
Bug 27064: (QA follow-up) Do not create transfer prior to accepting it

This patch prevents the cancellations we were seeing. Transfers were
being created pre-emtively for reserves in branchtransfers before the
librarian had selected their prefered action. As such, we were getting
double lines in the branchtransfers table.
Comment 36 Martin Renvoize 2021-06-07 13:31:17 UTC
Created attachment 121673 [details] [review]
Bug 27064: (QA follow-up) Pass 'replace' through to Koha::Item::Transfer->cancel

The `replace` option found in Koha::Item->request_transfer should be
passed through to Koha::Item::Transfer->cancel and prevent any reverse
transfers from being queued in such cases.

This prevents modItemTransfer from adding superflous reverse transfers
whenever it is used.

Test plan
1/ Run t/db_dependent/Koha/Item.t to prove the new tests pass
Comment 37 Martin Renvoize 2021-06-07 13:31:21 UTC
Created attachment 121674 [details] [review]
Bug 27064: (QA follow-up) Remove superflous error message

The reserve found error message should not be included in the errmsgloop
as it's handled as a question in the previous step.
Comment 38 Joonas Kylmälä 2021-06-07 14:12:38 UTC
Created attachment 121675 [details] [review]
Bug 27064: (QA follow-up) Add error message only if we have one defined

This moves the DestinationEqualsHolding error message addition to if
clause so the value is added to %err hash only when there is actually
an error. Then we only append to the @errmsgloop array the error
message if one was defined.

To test:
 1) Create a hold request to a pickup library where the item currently
 is not
 2) Add transfer to the pickup library using branchtransfers.pl and
 notice without this patch we get and empty message box but with this
 patch we don't get it.
Comment 39 Joonas Kylmälä 2021-06-07 14:44:37 UTC
Created attachment 121676 [details] [review]
Bug 27064: Only allow transferring a hold from the transfers page

These patches replace the 'Waiting' button on the transfers page with a 'Transfer' button
and correct some other related problem by passing the hold object to the template and
using that to fetch patron info as well as passing the reserve_id through to ensure the correct hold is
affected at all times

To test:
 1 - Place a hold for delivery at Library B
 2 - Sign in at Library A
 3 - Browse to Circulation->Transfers
 4 - Attempt to transfer an item on the title with the hold to Library B
 5 - You get a notice that hold was found (missing patron/branch info)
 6 - You have the option to set the hold waiting - click it
 7 - The transfer is generated and marked completed
 8 - The hold is marked as waiting, but the item is still at Library A and no transfer is active
 9 - The patron is notified that the hold is waiting
10 - Revert the hold or cancel and place a new one
11 - Apply patches
12 - Attempt transfer again
13 - You now have the option to transfer the hold
14 - Click that
15 - Hold is in transit and transfer is generated correctly
16 - Transfer again and choose 'cancel'
17 - Confirm hold is cancelled and transfer generated

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 40 Joonas Kylmälä 2021-06-07 14:44:43 UTC
Created attachment 121677 [details] [review]
Bug 27064: (QA follow-up) Do not send tobranchcd twice

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 41 Joonas Kylmälä 2021-06-07 14:44:48 UTC
Created attachment 121678 [details] [review]
Bug 27064: (QA follow-up) Do not create transfer prior to accepting it

This patch prevents the cancellations we were seeing. Transfers were
being created pre-emtively for reserves in branchtransfers before the
librarian had selected their prefered action. As such, we were getting
double lines in the branchtransfers table.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 42 Joonas Kylmälä 2021-06-07 14:44:53 UTC
Created attachment 121679 [details] [review]
Bug 27064: (QA follow-up) Pass 'replace' through to Koha::Item::Transfer->cancel

The `replace` option found in Koha::Item->request_transfer should be
passed through to Koha::Item::Transfer->cancel and prevent any reverse
transfers from being queued in such cases.

This prevents modItemTransfer from adding superflous reverse transfers
whenever it is used.

Test plan
1/ Run t/db_dependent/Koha/Item.t to prove the new tests pass

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 43 Joonas Kylmälä 2021-06-07 14:44:58 UTC
Created attachment 121680 [details] [review]
Bug 27064: (QA follow-up) Add error message only if we have one defined

This moves the DestinationEqualsHolding error message addition to if
clause so the value is added to %err hash only when there is actually
an error. Then we only append to the @errmsgloop array the error
message if one was defined.

To test:
 1) Create a hold request to a pickup library where the item currently
 is not
 2) Add transfer to the pickup library using branchtransfers.pl and
 notice without this patch we get and empty message box but with this
 patch we don't get it.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 44 Martin Renvoize 2021-06-07 14:47:53 UTC
Created attachment 121681 [details] [review]
Bug 27064: Only allow transferring a hold from the transfers page

These patches replace the 'Waiting' button on the transfers page with a 'Transfer' button
and correct some other related problem by passing the hold object to the template and
using that to fetch patron info as well as passing the reserve_id through to ensure the correct hold is
affected at all times

To test:
 1 - Place a hold for delivery at Library B
 2 - Sign in at Library A
 3 - Browse to Circulation->Transfers
 4 - Attempt to transfer an item on the title with the hold to Library B
 5 - You get a notice that hold was found (missing patron/branch info)
 6 - You have the option to set the hold waiting - click it
 7 - The transfer is generated and marked completed
 8 - The hold is marked as waiting, but the item is still at Library A and no transfer is active
 9 - The patron is notified that the hold is waiting
10 - Revert the hold or cancel and place a new one
11 - Apply patches
12 - Attempt transfer again
13 - You now have the option to transfer the hold
14 - Click that
15 - Hold is in transit and transfer is generated correctly
16 - Transfer again and choose 'cancel'
17 - Confirm hold is cancelled and transfer generated

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 45 Martin Renvoize 2021-06-07 14:47:57 UTC
Created attachment 121682 [details] [review]
Bug 27064: (QA follow-up) Do not send tobranchcd twice

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 46 Martin Renvoize 2021-06-07 14:48:02 UTC
Created attachment 121683 [details] [review]
Bug 27064: (QA follow-up) Do not create transfer prior to accepting it

This patch prevents the cancellations we were seeing. Transfers were
being created pre-emtively for reserves in branchtransfers before the
librarian had selected their prefered action. As such, we were getting
double lines in the branchtransfers table.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Martin Renvoize 2021-06-07 14:48:06 UTC
Created attachment 121684 [details] [review]
Bug 27064: (QA follow-up) Pass 'replace' through to Koha::Item::Transfer->cancel

The `replace` option found in Koha::Item->request_transfer should be
passed through to Koha::Item::Transfer->cancel and prevent any reverse
transfers from being queued in such cases.

This prevents modItemTransfer from adding superflous reverse transfers
whenever it is used.

Test plan
1/ Run t/db_dependent/Koha/Item.t to prove the new tests pass

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Martin Renvoize 2021-06-07 14:48:10 UTC
Created attachment 121685 [details] [review]
Bug 27064: (QA follow-up) Add error message only if we have one defined

This moves the DestinationEqualsHolding error message addition to if
clause so the value is added to %err hash only when there is actually
an error. Then we only append to the @errmsgloop array the error
message if one was defined.

To test:
 1) Create a hold request to a pickup library where the item currently
 is not
 2) Add transfer to the pickup library using branchtransfers.pl and
 notice without this patch we get and empty message box but with this
 patch we don't get it.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Martin Renvoize 2021-06-07 14:48:56 UTC
Excellent, your alternative to mine is much cleaner at the end there Joonas.. Thanks.

Added my SO stamp and setting PQA :)
Comment 50 Jonathan Druart 2021-06-14 15:34:03 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 51 Jonathan Druart 2021-06-15 08:08:00 UTC
Failing tests, please fix ASAP

kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/Circulation/transferbook.t                                                                                                                    [4/69]
t/db_dependent/Circulation/transferbook.t .. 2/6 
    #   Failed test 'Transfer of reserved item succeeded without ignore reserves'
    #   at t/db_dependent/Circulation/transferbook.t line 151.                               
    #          got: '0'
    #     expected: '1'                   
    # Looks like you failed 1 test of 5.

#   Failed test 'transfer already at destination'
#   at t/db_dependent/Circulation/transferbook.t line 154.

    #   Failed test 'Transfer of reserved item succeeded without ignore reserves'
    #   at t/db_dependent/Circulation/transferbook.t line 208.
    #          got: '0'
    #     expected: '1'
    # Looks like you failed 1 test of 5.
t/db_dependent/Circulation/transferbook.t .. 4/6 
#   Failed test 'transfer an issued item'
#   at t/db_dependent/Circulation/transferbook.t line 212.

    #   Failed test 'Transfer of reserved item succeeded without ignore reserves'
    #   at t/db_dependent/Circulation/transferbook.t line 248.
    #          got: '0'
    #     expected: '1'

    #   Failed test 'Transfer of reserved item succeeded with ignore reserves: false'
    #   at t/db_dependent/Circulation/transferbook.t line 260.
    #          got: '0'
    #     expected: '1'

    #   Failed test 'We got the expected failure message: DestinationEqualsHolding'
    #   at t/db_dependent/Circulation/transferbook.t line 273.
    #     Structures begin differing at:
    #          $got->{ResFound} = HASH(0x558e901b2fa0)
    #     $expected->{ResFound} = Does not exist

    #   Failed test 'We did not return that we found a reserve'
    #   at t/db_dependent/Circulation/transferbook.t line 278.
    #          got: 'Reserved'
    #     expected: anything else

    #   Failed test 'We did not return the reserve info'
    #   at t/db_dependent/Circulation/transferbook.t line 279.
    #          got: '989'
    #     expected: anything else
    # Looks like you failed 5 tests of 10.

#   Failed test 'ignore_reserves flag'
#   at t/db_dependent/Circulation/transferbook.t line 280.
t/db_dependent/Circulation/transferbook.t .. 6/6 # Looks like you failed 3 tests of 6.
Comment 52 Joonas Kylmälä 2021-06-15 09:28:13 UTC
Created attachment 121981 [details] [review]
Bug 27064: Correct transferbook.t tests to reflect new behaviour of transferbook()

The transferbook() behaviour was changed so that it only allows
transferring the item with a reserve if ignore_reserves=1 is passed to
it. The tests are changed here to reflect that. Note that however the
tests were buggy already before this change because the transfer's
"from" and "to" branches were the same and so the transfer should have
failed due to the error DestinationEqualsHolding, but futher though
the transferbook() code was buggy and it override the
DestinationEqualsHolding checking totally if there was a transfer! So
the tests were earlier working due to a bug in transferbook().

To test):
   1) Make sure the new test scenarios make sense
   2) prove t/db_dependent/Circulation/transferbook.t
Comment 53 Joonas Kylmälä 2021-06-15 09:28:44 UTC
(In reply to Jonathan Druart from comment #51)
> Failing tests, please fix ASAP

Attached a patch to correct the test scenarios.
Comment 54 Jonathan Druart 2021-06-15 14:29:40 UTC
(In reply to Joonas Kylmälä from comment #52)
> Created attachment 121981 [details] [review] [review]
> Bug 27064: Correct transferbook.t tests to reflect new behaviour of
> transferbook()

Patch pushed to master, thanks Joonas!
Comment 55 Kyle M Hall 2021-06-18 11:13:10 UTC
Pushed to 21.05.x for 21.05.01
Comment 56 Fridolin Somers 2021-06-21 12:34:47 UTC
Depends on Bug 12362 not in 20.11.x