Description
Nick Clemens (kidclamp)
2020-11-20 11:33:45 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 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. 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 > 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.
(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. Adding Joonas to the CC list as he's turned into the expert in this area of late ;) 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 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 --+--------+--------+----+-------+--------+---+------------+----------- 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> Created attachment 120764 [details] [review] Bug 27064: (QA follow-up) Do not send tobranchcd twice 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. (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.. 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> Created attachment 121066 [details] [review] Bug 27064: (QA follow-up) Do not send tobranchcd twice 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. Ready for testing again... > 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)."
Testing note, run updatedatabase after applying 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> Created attachment 121091 [details] [review] Bug 27064: (QA follow-up) Do not send tobranchcd twice 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. 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 (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. > 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...
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? > 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?
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> Created attachment 121142 [details] [review] Bug 27064: (QA follow-up) Do not send tobranchcd twice 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. 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 > 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 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> Created attachment 121671 [details] [review] Bug 27064: (QA follow-up) Do not send tobranchcd twice 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. 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 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. 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. 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Excellent, your alternative to mine is much cleaner at the end there Joonas.. Thanks. Added my SO stamp and setting PQA :) Pushed to master for 21.11, thanks to everybody involved! 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. 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 (In reply to Jonathan Druart from comment #51) > Failing tests, please fix ASAP Attached a patch to correct the test scenarios. (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! Pushed to 21.05.x for 21.05.01 Depends on Bug 12362 not in 20.11.x |