Bug 24434 highlighted a case where this method is restored to use.. we should really move to using Koha:: methods instead.
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.
Created attachment 168281 [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.
Created attachment 172778 [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.
Nothing like another rebase party to get a devs blood pressure rising.
Created attachment 172781 [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. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
The unit tests are included here and the bug is signed off again.. any chance of getting it over the line on QA Marcel?
(In reply to Martin Renvoize (ashimema) from comment #53) > The unit tests are included here and the bug is signed off again.. any > chance of getting it over the line on QA Marcel? Trying to get here soon
Depends on report in FQA. Please change status back if resolved.
QA: Looking here now
Created attachment 173856 [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. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
FAIL C4/Circulation.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 => Ignoring this one. Seems to be false. We are just removing one routine. WARN Koha/Item.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 278, now: 279) => Ignoring just one space or so :) @@ -400,7 +400,7 @@ sub ModItemTransfer { $transfer = $item->request_transfer( { to => $to_library, reason => $trigger, [...] reason => $trigger, - replace => 1 + replace => $trigger Just for the record. Reads a bit odd. But unfortunately request_transfer does cancel with the "replace reason"..
Pushed for 24.11! Well done everyone, thank you!
Created attachment 174805 [details] [review] Bug 28294: (follow-up) Adjust call to request transfer The call expects a library object, not a branchcode, likely rebasing issue To test: 1 - Place a hold for an item at a different library than current library 2 - Check item in and confirm transfer 3 - Check item in again 4 - Kaboom! 5 - Apply patch/restart_all 6 - Check item in again 7 - Ok!
(In reply to Nick Clemens (kidclamp) from comment #60) > Created attachment 174805 [details] [review] [review] > Bug 28294: (follow-up) Adjust call to request transfer > > The call expects a library object, not a branchcode, likely rebasing issue > > To test: > 1 - Place a hold for an item at a different library than current library > 2 - Check item in and confirm transfer > 3 - Check item in again > 4 - Kaboom! > 5 - Apply patch/restart_all > 6 - Check item in again > 7 - Ok! Please switch status back to PQA if you need me to push a follow-up!
Follow-up "Adjust call to request transfer" picked for main.