Bug 27281

Summary: Replace call to `C4::Circulation::DeleteTransfer` with `Koha::Item::Transfer->cancel({ comment => $comment })` in `C4::Circulation::LostItem`
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: david, fridolin.somers, gmcharlt, jonathan.druart, joonas.kylmala, katrin.fischer, kyle, nugged, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00
Bug Depends on: 26057, 27993    
Bug Blocks: 27896, 28209, 28210    
Attachments: Bug 27281: Update LostItem to use Koha::Item[::Transfer] methods
Bug 27281: Add unit test for Koha::Item->get_transfers
Bug 27281: Update LostItem to use Koha::Item[::Transfer] methods
Bug 27281: Add unit test for Koha::Item->get_transfers
Bug 27281: (follow-up) Clean up LostItem test
Bug 27281: Update LostItem to use Koha::Item[::Transfer] methods
Bug 27281: Add unit test for Koha::Item->get_transfers
Bug 27281: (follow-up) Clean up LostItem test
Bug 27281: (QA follow-up) Fix typo
Bug 27281: Update LostItem to use Koha::Item[::Transfer] methods
Bug 27281: Add unit test for Koha::Item->get_transfers
Bug 27281: (follow-up) Clean up LostItem test
Bug 27281: (QA follow-up) Fix typo
Bug 27281: (QA follow-up) Restore test for holdingbranch
Bug 27281: (QA follow-up) Fix Indexer Test
Bug 27281: (QA follow-up) Fix Indexer Test
Bug 27281: (QA follow-up) Fix Indexer Test

Description Martin Renvoize 2020-12-18 17:22:45 UTC
We should remove C4::Circulation::DeleteTransfer.

We can use Koha::Item::Transfer->cancel in preference to it.
Comment 1 Martin Renvoize 2021-03-08 14:17:37 UTC
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
Comment 2 Martin Renvoize 2021-03-08 14:17:40 UTC
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
Comment 3 Martin Renvoize 2021-03-08 15:01:32 UTC
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
Comment 4 Martin Renvoize 2021-03-08 15:01:35 UTC
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
Comment 5 Martin Renvoize 2021-03-08 15:01:38 UTC
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
Comment 6 Martin Renvoize 2021-03-09 10:39:28 UTC
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.
Comment 7 David Nind 2021-04-12 09:55:03 UTC
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.
Comment 8 Martin Renvoize 2021-04-14 14:30:10 UTC
Thanks for running the tests David.. yeah, this one's a pretty internal one
Comment 9 Kyle M Hall 2021-04-16 15:29:21 UTC
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>
Comment 10 Kyle M Hall 2021-04-16 15:29:33 UTC
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>
Comment 11 Kyle M Hall 2021-04-16 15:29:37 UTC
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>
Comment 12 Kyle M Hall 2021-04-16 15:29:41 UTC
Created attachment 119757 [details] [review]
Bug 27281: (QA follow-up) Fix typo
Comment 13 Martin Renvoize 2021-04-19 12:27:28 UTC
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>
Comment 14 Martin Renvoize 2021-04-19 12:27:32 UTC
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>
Comment 15 Martin Renvoize 2021-04-19 12:27:37 UTC
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>
Comment 16 Martin Renvoize 2021-04-19 12:27:41 UTC
Created attachment 119850 [details] [review]
Bug 27281: (QA follow-up) Fix typo

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2021-04-19 12:28:51 UTC
Tested as part of testing bugs 27896 and 12362 by Petro Vashchuk; Adding SO line of promoting Kyle's to QA.
Comment 18 Martin Renvoize 2021-04-22 08:36:34 UTC
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.
Comment 19 Jonathan Druart 2021-04-23 09:55:14 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 20 Martin Renvoize 2021-04-23 13:53:30 UTC
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.
Comment 21 Martin Renvoize 2021-04-23 13:58:03 UTC
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>
Comment 22 Jonathan Druart 2021-04-23 14:31:02 UTC
Nick, could you have a look at the follow-up? and maybe also the other failures? :)
Comment 23 Nick Clemens 2021-04-23 15:41:32 UTC
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>
Comment 24 Jonathan Druart 2021-04-27 09:41:54 UTC
Follow-up pushed to master.
Comment 25 Fridolin Somers 2021-04-30 07:46:41 UTC
Enhancement not pushed to 20.11.x