Bug 27896 - Remove C4::Circulation::DeleteTransfer
Summary: Remove C4::Circulation::DeleteTransfer
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Martin Renvoize (ashimema)
QA Contact: Kyle M Hall (khall)
URL:
Keywords:
Depends on: 27281
Blocks: 12362
  Show dependency treegraph
 
Reported: 2021-03-08 15:03 UTC by Martin Renvoize (ashimema)
Modified: 2021-12-13 21:12 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch replaces calls to DeleteTransfer with Koha::Item::Transfer->cancel and then removes C4::Circulation::DeleteTransfer entirely. This brings two advantages: 1. Better auditing of transfers by recording reasons for cancellation and throwing suitable errors for various conditions 2. Cleaning code from the overburdened C4::Circulation module.
Version(s) released in:
21.05.00
Circulation function:


Attachments
Bug 27896: Remove DeleteTransfer from circ/return.pl (1.40 KB, patch)
2021-03-08 15:31 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
BUg 27896: Remove DeleteTransfer (2.55 KB, patch)
2021-03-08 15:31 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 27896: (follow-up) Fix Typo (713 bytes, patch)
2021-04-09 07:19 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 27896: Remove DeleteTransfer from circ/return.pl (1.45 KB, patch)
2021-04-09 07:48 UTC, Peter Vashchuk
Details | Diff | Splinter Review
BUg 27896: Remove DeleteTransfer (2.60 KB, patch)
2021-04-09 07:48 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 27896: (follow-up) Fix Typo (767 bytes, patch)
2021-04-09 07:48 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 27896: Remove DeleteTransfer from circ/return.pl (1.52 KB, patch)
2021-04-16 15:47 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
BUg 27896: Remove DeleteTransfer (2.67 KB, patch)
2021-04-16 15:47 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 27896: (follow-up) Fix Typo (839 bytes, patch)
2021-04-16 15:47 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 27896: (QA follow-up) Fix number of unit tests (784 bytes, patch)
2021-04-16 15:47 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize (ashimema) 2021-03-08 15:03:42 UTC
We have just one call to DeleteTransfer left.. we should replace it with Koha::Item::Transfer->cancel and then drop DeleteTransfer entirely :)
Comment 1 Martin Renvoize (ashimema) 2021-03-08 15:31:09 UTC
Created attachment 117927 [details] [review]
Bug 27896: Remove DeleteTransfer from circ/return.pl

This patch replaces the DeleteTransfer call in circ/returns.pl with a
call to Koha::Item::Transfer->cancel.

Test plan
1/ Check an item out
2/ Add a transfer request for the item to a second library
3/ Attempt to check the item in at the first library
4/ Note that you should be given a 'WrongTransfer' modal and have to
option to cancel.
5/ Cancel the transfer
6/ Check in the database that the transfer now has 'datecancelled' set.
7/ Add a transfer for the item again
8/ From the transferstoreceive page cancel the transfer
9/ Click cancel and again check that datecancelled is set in the
database for your transfer
Comment 2 Martin Renvoize (ashimema) 2021-03-08 15:31:11 UTC
Created attachment 117928 [details] [review]
BUg 27896: Remove DeleteTransfer

This patch removes the last mentions of DeleteTransfer

Test plan
1/ Grep the codebase for DeleteTransfer
2/ It should no longer exist
Comment 3 Peter Vashchuk 2021-04-07 13:04:00 UTC
Throws software error at step 9: "click cancel" when I click the cancel button:
> Can't use string ("Koha::Item") as a HASH ref while "strict refs" in use at /Koha/Object.pm line 782.

Seems like the source of the problem is located in returns.pl at the line 231:
>     my $item = Koha::Item->find($itemnumber);

I managed to fix it by changing "Koha::Item" to "Koha::Items" and it seems to work fine after that change, but please check if it's a correct solution anyway :)
Comment 4 Martin Renvoize (ashimema) 2021-04-09 07:19:42 UTC
Created attachment 119345 [details] [review]
Bug 27896: (follow-up) Fix Typo
Comment 5 Martin Renvoize (ashimema) 2021-04-09 07:20:12 UTC
Thanks Peter, you are indeed correct.

I've added a followup with the suggested typo fix.
Comment 6 Peter Vashchuk 2021-04-09 07:48:33 UTC
Created attachment 119347 [details] [review]
Bug 27896: Remove DeleteTransfer from circ/return.pl

This patch replaces the DeleteTransfer call in circ/returns.pl with a
call to Koha::Item::Transfer->cancel.

Test plan
1/ Check an item out
2/ Add a transfer request for the item to a second library
3/ Attempt to check the item in at the first library
4/ Note that you should be given a 'WrongTransfer' modal and have to
option to cancel.
5/ Cancel the transfer
6/ Check in the database that the transfer now has 'datecancelled' set.
7/ Add a transfer for the item again
8/ From the transferstoreceive page cancel the transfer
9/ Click cancel and again check that datecancelled is set in the
database for your transfer

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 7 Peter Vashchuk 2021-04-09 07:48:37 UTC
Created attachment 119348 [details] [review]
BUg 27896: Remove DeleteTransfer

This patch removes the last mentions of DeleteTransfer

Test plan
1/ Grep the codebase for DeleteTransfer
2/ It should no longer exist

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 8 Peter Vashchuk 2021-04-09 07:48:41 UTC
Created attachment 119349 [details] [review]
Bug 27896: (follow-up) Fix Typo

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 9 Kyle M Hall (khall) 2021-04-16 15:47:10 UTC
Created attachment 119758 [details] [review]
Bug 27896: Remove DeleteTransfer from circ/return.pl

This patch replaces the DeleteTransfer call in circ/returns.pl with a
call to Koha::Item::Transfer->cancel.

Test plan
1/ Check an item out
2/ Add a transfer request for the item to a second library
3/ Attempt to check the item in at the first library
4/ Note that you should be given a 'WrongTransfer' modal and have to
option to cancel.
5/ Cancel the transfer
6/ Check in the database that the transfer now has 'datecancelled' set.
7/ Add a transfer for the item again
8/ From the transferstoreceive page cancel the transfer
9/ Click cancel and again check that datecancelled is set in the
database for your transfer

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall (khall) 2021-04-16 15:47:22 UTC
Created attachment 119759 [details] [review]
BUg 27896: Remove DeleteTransfer

This patch removes the last mentions of DeleteTransfer

Test plan
1/ Grep the codebase for DeleteTransfer
2/ It should no longer exist

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall (khall) 2021-04-16 15:47:26 UTC
Created attachment 119760 [details] [review]
Bug 27896: (follow-up) Fix Typo

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Kyle M Hall (khall) 2021-04-16 15:47:29 UTC
Created attachment 119761 [details] [review]
Bug 27896: (QA follow-up) Fix number of unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Jonathan Druart 2021-04-23 09:55:31 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 14 Fridolin Somers 2021-04-30 07:47:02 UTC
Enhancement not pushed to 20.11.x