Description
Martin Renvoize
2021-05-06 12:59:38 UTC
Created attachment 120637 [details] [review] Bug 28294: WIP 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. 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. 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. To Do * Add tests for 'replace' on 'Cancel' and 'Request' methods. * Drop updateWrongTransfer and use request_transfer directly in the controller. 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> 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> 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> Switching to "assigned" due to comment 5 Does that makes sense? 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 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 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> 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> Created attachment 121097 [details] [review] Bug 28294: Remove use of updateWrongTransfer 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. Does that still need need work about the 1st half of comment 5? 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> 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> Created attachment 121716 [details] [review] Bug 28294: Remove use of updateWrongTransfer Please add tests for these module changes. 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> 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> 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. 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. 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. Created attachment 130087 [details] [review] Bug 28294: Remove unit tests for updateWrongTransfer Rebased and unit tests added. 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> 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> 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. 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. 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. Created attachment 132217 [details] [review] Bug 28294: Remove unit tests for updateWrongTransfer Looking here, started with a rebase # 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? +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 ? + MODIFY COLUMN + `reason` enum( + 'Manual', + 'StockrotationAdvance', + 'StockrotationRepatriation', + 'ReturnToHome', + 'ReturnToHolding', + 'RotatingCollection', + 'Reserve', + 'LostReserve', + 'CancelReserve', + 'TransferCancellation', + 'WrongTransfer' + ) + AFTER `comments` Why modify a column after something ? Please provide some feedback. Generally looking good ! 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> 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> 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. 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. 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. Created attachment 160521 [details] [review] Bug 28294: Remove unit tests for updateWrongTransfer 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'). 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. I like the alternative a lot better.. simpler DRYer code. Setting back to NSO 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. |