Bug 28294

Summary: C4::Circulation::updateWrongTransfer should be moved into Koha::
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: Needs Signoff --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: m.de.rooy, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29792
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35784
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 35721, 24434, 27064    
Bug Blocks: 28527, 35736    
Attachments: Bug 28294: WIP
Bug 28294: Add 'WrongTransfer' to transfer reason enum
Bug 28294: Pass 'replace' through to Koha::Item::Transfer->cancel
Bug 28294: Convert updateWrongTransfer to use request_transfer
Bug 28294: Add 'WrongTransfer' to transfer reason enum
Bug 28294: Pass 'replace' through to Koha::Item::Transfer->cancel
Bug 28294: Convert updateWrongTransfer to use request_transfer
Bug 28294: Add 'WrongTransfer' to transfer reason enum
Bug 28294: Convert updateWrongTransfer to use request_transfer
Bug 28294: Remove use of updateWrongTransfer
Bug 28294: Add 'WrongTransfer' to transfer reason enum
Bug 28294: Convert updateWrongTransfer to use request_transfer
Bug 28294: Remove use of updateWrongTransfer
Bug 28294: Add 'WrongTransfer' to transfer reason enum
Bug 28294: Add Koha::Item::Transfer->remedy method and use in updateWrongTransfer
Bug 28294: Unit Tests
Bug 28294: Remove use of updateWrongTransfer
Bug 28294: Remove updateWrongTransfer method
Bug 28294: Remove unit tests for updateWrongTransfer
Bug 28294: Add 'WrongTransfer' to transfer reason enum
Bug 28294: Add Koha::Item::Transfer->remedy method and use in updateWrongTransfer
Bug 28294: Unit Tests
Bug 28294: Remove use of updateWrongTransfer
Bug 28294: Remove updateWrongTransfer method
Bug 28294: Remove unit tests for updateWrongTransfer
Bug 28294: Add 'WrongTransfer' to transfer reason enum
Bug 28294: Add Koha::Item::Transfer->remedy method and use in updateWrongTransfer
Bug 28294: Unit Tests
Bug 28294: Remove use of updateWrongTransfer
Bug 28294: Remove updateWrongTransfer method
Bug 28294: Remove unit tests for updateWrongTransfer
Bug 28294: [ALT] Remove updateWrongTransfer
Bug 28294: Remove updateWrongTransfer

Description Martin Renvoize 2021-05-06 12:59:38 UTC
Bug 24434 highlighted a case where this method is restored to use.. we should really move to using Koha:: methods instead.
Comment 1 Martin Renvoize 2021-05-06 15:32:09 UTC
Created attachment 120637 [details] [review]
Bug 28294: WIP
Comment 2 Martin Renvoize 2021-05-17 16:22:25 UTC
Created attachment 121062 [details] [review]
Bug 28294: Add 'WrongTransfer' to transfer reason enum

This patch adds the 'WrongTransfer' value to the 'reason' enum in the
branchtransfers table.  This is so we can use it as a reason for the
subsequent transfer when a trainsfer is triggered by a checkin at an
incorrect branch after transfer has been initiated.
Comment 3 Martin Renvoize 2021-05-17 16:22:28 UTC
Created attachment 121063 [details] [review]
Bug 28294: 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 allows us to replace updateWrongTransfer entirely with
Koha::Item->request_transfer with the 'replace' parameter enabled.
Comment 4 Martin Renvoize 2021-05-17 16:22:32 UTC
Created attachment 121064 [details] [review]
Bug 28294: Convert updateWrongTransfer to use request_transfer

This patch is an interim patch to prove that the updated
request_transfer method achieves the same as updateWrongTransfer.

Test plan
1/ Run t/db_dependant/Circulation.t before and after applying this
patch.
Comment 5 Martin Renvoize 2021-05-17 16:24:00 UTC
To Do
* Add tests for 'replace' on 'Cancel' and 'Request' methods.
* Drop updateWrongTransfer and use request_transfer directly in the controller.
Comment 6 Victor Grousset/tuxayo 2021-05-17 23:25:21 UTC
Created attachment 121077 [details] [review]
Bug 28294: Add 'WrongTransfer' to transfer reason enum

This patch adds the 'WrongTransfer' value to the 'reason' enum in the
branchtransfers table.  This is so we can use it as a reason for the
subsequent transfer when a trainsfer is triggered by a checkin at an
incorrect branch after transfer has been initiated.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 7 Victor Grousset/tuxayo 2021-05-17 23:25:25 UTC
Created attachment 121078 [details] [review]
Bug 28294: 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 allows us to replace updateWrongTransfer entirely with
Koha::Item->request_transfer with the 'replace' parameter enabled.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 8 Victor Grousset/tuxayo 2021-05-17 23:25:29 UTC
Created attachment 121079 [details] [review]
Bug 28294: Convert updateWrongTransfer to use request_transfer

This patch is an interim patch to prove that the updated
request_transfer method achieves the same as updateWrongTransfer.

Test plan
1/ Run t/db_dependant/Circulation.t before and after applying this
patch.
  run updatedatabase when applying

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 9 Victor Grousset/tuxayo 2021-05-17 23:26:26 UTC
Switching to "assigned" due to comment 5
Does that makes sense?
Comment 10 Victor Grousset/tuxayo 2021-05-17 23:53:29 UTC
git-bz trap!
Set back to "need SO" because git-bz when applying bug 27064 won't ask to follow the dependency to this bug :o
Comment 11 Martin Renvoize 2021-05-18 07:56:40 UTC
I'm moving the middle patch into bug 27064 to prevent hold up of that bug whilst I continue to refine this one. - Bug 28294: Pass 'replace' through to Koha::Item::Transfer->cancel
Comment 12 Martin Renvoize 2021-05-18 08:46:14 UTC
Created attachment 121095 [details] [review]
Bug 28294: Add 'WrongTransfer' to transfer reason enum

This patch adds the 'WrongTransfer' value to the 'reason' enum in the
branchtransfers table.  This is so we can use it as a reason for the
subsequent transfer when a trainsfer is triggered by a checkin at an
incorrect branch after transfer has been initiated.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 13 Martin Renvoize 2021-05-18 08:46:18 UTC
Created attachment 121096 [details] [review]
Bug 28294: Convert updateWrongTransfer to use request_transfer

This patch is an interim patch to prove that the updated
request_transfer method achieves the same as updateWrongTransfer.

Test plan
1/ Run t/db_dependant/Circulation.t before and after applying this
patch.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 14 Martin Renvoize 2021-05-18 08:46:26 UTC
Created attachment 121097 [details] [review]
Bug 28294: Remove use of updateWrongTransfer
Comment 15 Victor Grousset/tuxayo 2021-05-18 10:50:29 UTC
Ok, switching back to "assigned", no need to keep in "need signoff" as a workaround for git-bz isn't need as it's no more a dependency of another patch.
Comment 16 Victor Grousset/tuxayo 2021-06-01 16:26:07 UTC
Does that still need need work about the 1st half of comment 5?
Comment 17 Martin Renvoize 2021-06-08 10:58:28 UTC
Created attachment 121714 [details] [review]
Bug 28294: Add 'WrongTransfer' to transfer reason enum

This patch adds the 'WrongTransfer' value to the 'reason' enum in the
branchtransfers table.  This is so we can use it as a reason for the
subsequent transfer when a trainsfer is triggered by a checkin at an
incorrect branch after transfer has been initiated.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 18 Martin Renvoize 2021-06-08 10:58:32 UTC
Created attachment 121715 [details] [review]
Bug 28294: Convert updateWrongTransfer to use request_transfer

This patch is an interim patch to prove that the updated
request_transfer method achieves the same as updateWrongTransfer.

Test plan
1/ Run t/db_dependant/Circulation.t before and after applying this
patch.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 19 Martin Renvoize 2021-06-08 10:58:36 UTC
Created attachment 121716 [details] [review]
Bug 28294: Remove use of updateWrongTransfer
Comment 20 Marcel de Rooy 2021-07-09 06:34:40 UTC
Please add tests for these module changes.
Comment 21 Martin Renvoize 2022-02-02 10:48:43 UTC
Created attachment 130082 [details] [review]
Bug 28294: Add 'WrongTransfer' to transfer reason enum

This patch adds the 'WrongTransfer' value to the 'reason' enum in the
branchtransfers table.  This is so we can use it as a reason for the
subsequent transfer when a trainsfer is triggered by a checkin at an
incorrect branch after transfer has been initiated.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 22 Martin Renvoize 2022-02-02 10:48:46 UTC
Created attachment 130083 [details] [review]
Bug 28294: Add Koha::Item::Transfer->remedy method and use in updateWrongTransfer

This patch is an interim patch to prove that the new remedy method
achieves the same as updateWrongTransfer.

Test plan
1/ Run t/db_dependant/Circulation.t before and after applying this
patch.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 23 Martin Renvoize 2022-02-02 10:48:50 UTC
Created attachment 130084 [details] [review]
Bug 28294: Unit Tests

This patch adds unit tests for the new 'remedy' method we are
introducing to Koha::Item::Transfer.
Comment 24 Martin Renvoize 2022-02-02 10:48:54 UTC
Created attachment 130085 [details] [review]
Bug 28294: Remove use of updateWrongTransfer

This patch removes the last remaining use of updateWrongTransfer.

Test plan
1/ git grep updateWrongTransfer to confirm there are no more uses of the
method.
2/ Confirm an incorrect transfer still behaves as expected at
circulation returns.
Comment 25 Martin Renvoize 2022-02-02 10:48:57 UTC
Created attachment 130086 [details] [review]
Bug 28294: Remove updateWrongTransfer method

This patch removes the updateWrongTransfer method entirely from
C4/Circulation.pm.

Test plan
1/ Confirm that there are no longer any mentions of updateWrongTransfer
in the codebase.
Comment 26 Martin Renvoize 2022-02-02 10:49:00 UTC
Created attachment 130087 [details] [review]
Bug 28294: Remove unit tests for updateWrongTransfer
Comment 27 Martin Renvoize 2022-02-02 10:50:01 UTC
Rebased and unit tests added.
Comment 28 Marcel de Rooy 2022-03-25 09:41:02 UTC
Created attachment 132212 [details] [review]
Bug 28294: Add 'WrongTransfer' to transfer reason enum

This patch adds the 'WrongTransfer' value to the 'reason' enum in the
branchtransfers table.  This is so we can use it as a reason for the
subsequent transfer when a trainsfer is triggered by a checkin at an
incorrect branch after transfer has been initiated.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 29 Marcel de Rooy 2022-03-25 09:41:05 UTC
Created attachment 132213 [details] [review]
Bug 28294: Add Koha::Item::Transfer->remedy method and use in updateWrongTransfer

This patch is an interim patch to prove that the new remedy method
achieves the same as updateWrongTransfer.

Test plan
1/ Run t/db_dependant/Circulation.t before and after applying this
patch.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 30 Marcel de Rooy 2022-03-25 09:41:09 UTC
Created attachment 132214 [details] [review]
Bug 28294: Unit Tests

This patch adds unit tests for the new 'remedy' method we are
introducing to Koha::Item::Transfer.
Comment 31 Marcel de Rooy 2022-03-25 09:41:12 UTC
Created attachment 132215 [details] [review]
Bug 28294: Remove use of updateWrongTransfer

This patch removes the last remaining use of updateWrongTransfer.

Test plan
1/ git grep updateWrongTransfer to confirm there are no more uses of the
method.
2/ Confirm an incorrect transfer still behaves as expected at
circulation returns.
Comment 32 Marcel de Rooy 2022-03-25 09:41:16 UTC
Created attachment 132216 [details] [review]
Bug 28294: Remove updateWrongTransfer method

This patch removes the updateWrongTransfer method entirely from
C4/Circulation.pm.

Test plan
1/ Confirm that there are no longer any mentions of updateWrongTransfer
in the codebase.
Comment 33 Marcel de Rooy 2022-03-25 09:41:19 UTC
Created attachment 132217 [details] [review]
Bug 28294: Remove unit tests for updateWrongTransfer
Comment 34 Marcel de Rooy 2022-03-25 09:41:38 UTC
Looking here, started with a rebase
Comment 35 Marcel de Rooy 2022-03-25 09:50:38 UTC
# Subtest: AddRenewal and AddIssuingCharge tests
    1..13
    not ok 1 - Exception is thrown when renewal update to issues fails
    #   Failed test 'Exception is thrown when renewal update to issues fails'
    #   at t/db_dependent/Circulation.t line 4147.
    # expecting: Koha::Exceptions::Checkout::FailedRenewal
    # found: normal exit
    ok 2 - renew log not added because of the syspref RenewalLog
    ok 3 - AddRenewal should set the renewal date with the time part
    not ok 4
    #   Failed test at t/db_dependent/Circulation.t line 4170.
    #          got: '3'
    #     expected: '2'
    ok 5 - The issue of item with issuing charge generates an accountline of the correct type
    ok 6 - AddIssuingCharge correctly sets branchcode
    ok 7 - AddIssue does not set a hardcoded description for the accountline
    ok 8 - The renewal of item with issuing charge generates an accountline of the correct type
    ok 9 - AddRenewal correctly sets branchcode
    ok 10 - AddRenewal does not set a hardcoded description for the accountline
    ok 11 - renew log successfully added
    ok 12 - renew statistic successfully added with passed branch
    not ok 13 - Passing skipfinecalc causes fine calculation on renewal to be skipped
    #   Failed test 'Passing skipfinecalc causes fine calculation on renewal to be skipped'
    #   at t/db_dependent/Circulation.t line 4207.
    #          got: '6'
    #     expected: '5'
    # Looks like you failed 3 tests of 13.
not ok 45 - AddRenewal and AddIssuingCharge tests

I am not sure if Circulation.t fails due to changes here. But it did not fail on Jenkins on current master?
Comment 36 Marcel de Rooy 2022-03-25 09:57:49 UTC
+FIXME: remedy or rectify
+    # FIXME: Should we remedy a transfer if it's just gone back to the originating branch, or just leave it in transit as is.

Two FIXME's added. Can we do something about them here?

+  my $new_transfer = $transfer->remedy(
+    {
+        to     => $to_library,
+        reason => $reason,
+        [ ignore_limits => 0, force => 0 ]
+    }
+  );

There is no code for ignore_limits and force ?
Comment 37 Marcel de Rooy 2022-03-25 09:58:50 UTC
+            MODIFY COLUMN
+                `reason` enum(
+                    'Manual',
+                    'StockrotationAdvance',
+                    'StockrotationRepatriation',
+                    'ReturnToHome',
+                    'ReturnToHolding',
+                    'RotatingCollection',
+                    'Reserve',
+                    'LostReserve',
+                    'CancelReserve',
+                    'TransferCancellation',
+                    'WrongTransfer'
+                )
+            AFTER `comments`

Why modify a column after something ?
Comment 38 Marcel de Rooy 2022-03-25 10:00:08 UTC
Please provide some feedback. Generally looking good !
Comment 39 Martin Renvoize 2024-01-04 12:29:52 UTC
Created attachment 160516 [details] [review]
Bug 28294: Add 'WrongTransfer' to transfer reason enum

This patch adds the 'WrongTransfer' value to the 'reason' enum in the
branchtransfers table.  This is so we can use it as a reason for the
subsequent transfer when a trainsfer is triggered by a checkin at an
incorrect branch after transfer has been initiated.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 40 Martin Renvoize 2024-01-04 12:29:55 UTC
Created attachment 160517 [details] [review]
Bug 28294: Add Koha::Item::Transfer->remedy method and use in updateWrongTransfer

This patch is an interim patch to prove that the new remedy method
achieves the same as updateWrongTransfer.

Test plan
1/ Run t/db_dependant/Circulation.t before and after applying this
patch.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 41 Martin Renvoize 2024-01-04 12:29:57 UTC
Created attachment 160518 [details] [review]
Bug 28294: Unit Tests

This patch adds unit tests for the new 'remedy' method we are
introducing to Koha::Item::Transfer.
Comment 42 Martin Renvoize 2024-01-04 12:30:00 UTC
Created attachment 160519 [details] [review]
Bug 28294: Remove use of updateWrongTransfer

This patch removes the last remaining use of updateWrongTransfer.

Test plan
1/ git grep updateWrongTransfer to confirm there are no more uses of the
method.
2/ Confirm an incorrect transfer still behaves as expected at
circulation returns.
Comment 43 Martin Renvoize 2024-01-04 12:30:03 UTC
Created attachment 160520 [details] [review]
Bug 28294: Remove updateWrongTransfer method

This patch removes the updateWrongTransfer method entirely from
C4/Circulation.pm.

Test plan
1/ Confirm that there are no longer any mentions of updateWrongTransfer
in the codebase.
Comment 44 Martin Renvoize 2024-01-04 12:30:06 UTC
Created attachment 160521 [details] [review]
Bug 28294: Remove unit tests for updateWrongTransfer
Comment 45 Martin Renvoize 2024-01-04 12:36:44 UTC
Rebased, but not I'm revisiting the code I'm wondering why it's needed.. I think we could perhaps just replace the call to updateWrongTransfer with a call to request_transfer instead (and pass 'replace' in with a reason of 'wrongTransfer').
Comment 46 Martin Renvoize 2024-01-04 15:01:02 UTC
Created attachment 160536 [details] [review]
Bug 28294: [ALT] Remove updateWrongTransfer

This patch removes the last remaining use of updateWrongTransfer and the
method itself.

We replace it with a call to request_transfer that passes the 'replace'
reason of 'WrongTransfer' through to Koha::Item::Transfer->cancel.

Test plan
1/ git grep updateWrongTransfer to confirm there are no more uses of the
method.
2/ Confirm an incorrect transfer still behaves as expected at
circulation returns.
Comment 47 Martin Renvoize 2024-01-04 15:16:47 UTC
I like the alternative a lot better.. simpler DRYer code.

Setting back to NSO
Comment 48 Martin Renvoize 2024-01-09 13:25:14 UTC
Created attachment 160692 [details] [review]
Bug 28294: Remove updateWrongTransfer

This patch removes the last remaining use of updateWrongTransfer and the
method itself.

We replace it with a call to request_transfer that passes the 'replace'
reason of 'WrongTransfer' through to Koha::Item::Transfer->cancel.

Test plan
1/ git grep updateWrongTransfer to confirm there are no more uses of the
method.
2/ Confirm an incorrect transfer still behaves as expected at
circulation returns.