We should remove C4::Circulation::DeleteTransfer. We can use Koha::Item::Transfer->cancel in preference to it.
Created attachment 117921 [details] [review] Bug 27281: Update LostItem to use Koha::Item[::Transfer] methods This patch updates C4::Circulation::LostItem to use the Koha::Item and Koha::Item::Transfer methods to cancel transfers when an item is marked as lost. Test plan 1/ Confirm t/db_dependant/Circulation.t passes prior to applying the patches 2/ Apply the patch and run updatedatabase.pl 3/ Confirm that t/db_dependant/Circulation.t still passes 4/ Signoff
Created attachment 117922 [details] [review] Bug 27281: Add unit test for Koha::Item->get_transfers In the previous patch we introduced the new get_transfers method to the Koha::Item object. This patch adds proper unit tests for that addition. Test plan 1/ Apply patches and confirm t/db_dependent/Koha/Item.t passes. 2/ Signoff
Created attachment 117924 [details] [review] Bug 27281: Update LostItem to use Koha::Item[::Transfer] methods This patch updates C4::Circulation::LostItem to use the Koha::Item and Koha::Item::Transfer methods to cancel transfers when an item is marked as lost. Test plan 1/ Confirm t/db_dependant/Circulation.t passes prior to applying the patches 2/ Apply the patch and run updatedatabase.pl 3/ Confirm that t/db_dependant/Circulation.t still passes 4/ Signoff
Created attachment 117925 [details] [review] Bug 27281: Add unit test for Koha::Item->get_transfers In the previous patch we introduced the new get_transfers method to the Koha::Item object. This patch adds proper unit tests for that addition. Test plan 1/ Apply patches and confirm t/db_dependent/Koha/Item.t passes. 2/ Signoff
Created attachment 117926 [details] [review] Bug 27281: (follow-up) Clean up LostItem test The LostItem test was rather messy, and wasn't actually testing much in reality. This patch cleans it up and adds testing for transfer queue handling of ListItem cancellations. Test plan 1/ Confirm t/db_dependent/Circulation.t still passes 2/ Read the tests before and after and confirm that they test what is required. 3/ Signoff
QA Query.. are 'get_transfer' and 'get_transfers' the right names for these accessors.. I feel like perhaps they would be better called `->next_transfer` and `->queued_transfers` or something along those lines.
Just to confirm that the tests pass before and after the patches are applied (note for the main bug it should be 'db_dependent', not 'db_dependant'). I don't have any knowledge of how to write test and what should be covered, so will leave that for someone else.
Thanks for running the tests David.. yeah, this one's a pretty internal one
Created attachment 119754 [details] [review] Bug 27281: Update LostItem to use Koha::Item[::Transfer] methods This patch updates C4::Circulation::LostItem to use the Koha::Item and Koha::Item::Transfer methods to cancel transfers when an item is marked as lost. Test plan 1/ Confirm t/db_dependant/Circulation.t passes prior to applying the patches 2/ Apply the patch and run updatedatabase.pl 3/ Confirm that t/db_dependant/Circulation.t still passes 4/ Signoff Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 119755 [details] [review] Bug 27281: Add unit test for Koha::Item->get_transfers In the previous patch we introduced the new get_transfers method to the Koha::Item object. This patch adds proper unit tests for that addition. Test plan 1/ Apply patches and confirm t/db_dependent/Koha/Item.t passes. 2/ Signoff Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 119756 [details] [review] Bug 27281: (follow-up) Clean up LostItem test The LostItem test was rather messy, and wasn't actually testing much in reality. This patch cleans it up and adds testing for transfer queue handling of ListItem cancellations. Test plan 1/ Confirm t/db_dependent/Circulation.t still passes 2/ Read the tests before and after and confirm that they test what is required. 3/ Signoff Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 119757 [details] [review] Bug 27281: (QA follow-up) Fix typo
Created attachment 119847 [details] [review] Bug 27281: Update LostItem to use Koha::Item[::Transfer] methods This patch updates C4::Circulation::LostItem to use the Koha::Item and Koha::Item::Transfer methods to cancel transfers when an item is marked as lost. Test plan 1/ Confirm t/db_dependant/Circulation.t passes prior to applying the patches 2/ Apply the patch and run updatedatabase.pl 3/ Confirm that t/db_dependant/Circulation.t still passes 4/ Signoff Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 119848 [details] [review] Bug 27281: Add unit test for Koha::Item->get_transfers In the previous patch we introduced the new get_transfers method to the Koha::Item object. This patch adds proper unit tests for that addition. Test plan 1/ Apply patches and confirm t/db_dependent/Koha/Item.t passes. 2/ Signoff Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 119849 [details] [review] Bug 27281: (follow-up) Clean up LostItem test The LostItem test was rather messy, and wasn't actually testing much in reality. This patch cleans it up and adds testing for transfer queue handling of ListItem cancellations. Test plan 1/ Confirm t/db_dependent/Circulation.t still passes 2/ Read the tests before and after and confirm that they test what is required. 3/ Signoff Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 119850 [details] [review] Bug 27281: (QA follow-up) Fix typo Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Tested as part of testing bugs 27896 and 12362 by Petro Vashchuk; Adding SO line of promoting Kyle's to QA.
Created attachment 119986 [details] [review] Bug 27281: (QA follow-up) Restore test for holdingbranch I inadvertantly removed the test that varified that the items holdingbranch is unaffected by the call to LostItem. This restores such a test and adds a few comments to clarify the test transfers.
Pushed to master for 21.05, thanks to everybody involved!
Created attachment 120105 [details] [review] Bug 27281: (QA follow-up) Fix Indexer Test With bug 27281 we removed a call to reset an items holdingbranch back to the 'frombranch' of the transfer. This is correct, as with bug 23129 (and friends) we ensure that the holdingbranch was not changed at transfer request time and instead would only be updated at the reciept of transfer. As such, it's safe to remove the update here as it would be just setting the same branch as is already present. However, when removing this ModItem call, I neglected to realise that also remoes the requirement for passing around the defered indexing bit as we no longer need to trigger a re-index as there's not any change to the item. This patch therefore cleans up the POD for LostItem, removes the no longer references 'params' parameter and drops the corresponding index test.
Created attachment 120107 [details] [review] Bug 27281: (QA follow-up) Fix Indexer Test With bug 27281 we removed a call to reset an items holdingbranch back to the 'frombranch' of the transfer. This is correct, as with bug 23129 (and friends) we ensure that the holdingbranch was not changed at transfer request time and instead would only be updated at the reciept of transfer. As such, it's safe to remove the update here as it would be just setting the same branch as is already present. However, when removing this ModItem call, I neglected to realise that also remoes the requirement for passing around the defered indexing bit as we no longer need to trigger a re-index as there's not any change to the item. This patch therefore cleans up the POD for LostItem, removes the no longer references 'params' parameter and drops the corresponding index test. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Nick, could you have a look at the follow-up? and maybe also the other failures? :)
Created attachment 120117 [details] [review] Bug 27281: (QA follow-up) Fix Indexer Test With bug 27281 we removed a call to reset an items holdingbranch back to the 'frombranch' of the transfer. This is correct, as with bug 23129 (and friends) we ensure that the holdingbranch was not changed at transfer request time and instead would only be updated at the reciept of transfer. As such, it's safe to remove the update here as it would be just setting the same branch as is already present. However, when removing this ModItem call, I neglected to realise that also remoes the requirement for passing around the defered indexing bit as we no longer need to trigger a re-index as there's not any change to the item. This patch therefore cleans up the POD for LostItem, removes the no longer references 'params' parameter and drops the corresponding index test. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Follow-up pushed to master.
Enhancement not pushed to 20.11.x