|
Description
Martin Renvoize (ashimema)
2020-01-16 15:17:54 UTC
Comment hidden (obsolete)
Created attachment 97449 [details] [review] Bug 24434: Remove updateWrongTransfer This patch simply removes the unused updateWrongTransfer method from C4::Circulation. Test plan: Read code and varify that the method would never actually be triggered. Created attachment 97550 [details] [review] Bug 24434: Remove updateWrongTransfer This patch simply removes the unused updateWrongTransfer method from C4::Circulation. Test plan: Read code and varify that the method would never actually be triggered. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> WT-itemNumber, WT-waitingAt and WT-From have been removed from returns.tt by: commit 5ed171a45c46f63b18266ad3a3d0a978719ca285 Bug 21346: Convert dialogs to modals. (In reply to Jonathan Druart from comment #3) > WT-itemNumber, WT-waitingAt and WT-From have been removed from returns.tt by: > commit 5ed171a45c46f63b18266ad3a3d0a978719ca285 > Bug 21346: Convert dialogs to modals. I do not manage to know if that was expected. How is this related to bug 21944? (In reply to Jonathan Druart from comment #4) > (In reply to Jonathan Druart from comment #3) > > WT-itemNumber, WT-waitingAt and WT-From have been removed from returns.tt by: > > commit 5ed171a45c46f63b18266ad3a3d0a978719ca285 > > Bug 21346: Convert dialogs to modals. As for what it used to do, I couldn't work it out either.. hence removing it as at the moment it's just noise in an already complicated template file... I hope to continue down this tree and ratify transfers handling.. just waiting for a few more of the foundational bugs to be signed off before I continue. > > I do not manage to know if that was expected. How is this related to bug > 21944? Hmm, maybe my git blame foo got the wrong bug for where the use was dropped. The template passed the variable (WT-itemNumber) to the script: - <input type="hidden" name="WT-itemNumber" value="[% WrongTransferItem | html %]" /> - <input type="hidden" name="WT-waitingAt" value="[% TransferWaitingAt | html %]" /> - <input type="hidden" name="WT-From" value="[% wtransfertFrom | html %]" /> commit 5ed171a45c46f63b18266ad3a3d0a978719ca285 Bug 21346: Convert dialogs to modals. Created attachment 99829 [details] [review] Bug 24434: Remove updateWrongTransfer This patch simply removes the unused updateWrongTransfer method from C4::Circulation. Test plan: Read code and varify that the method would never actually be triggered. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> (In reply to Katrin Fischer from comment #7) > Created attachment 99829 [details] [review] [review] > Bug 24434: Remove updateWrongTransfer > > This patch simply removes the unused updateWrongTransfer method from > C4::Circulation. > > Test plan: Read code and varify that the method would never actually be > triggered. > > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> > > Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Katrin, did you investigate the possible problem I raised in my previous comment? Ah sorry, I didn't quite understand - I checked that the variable name didn't seem to appear anywhere, coming to the conclusion it was not used. I'm in agreement with Jonathan here.. I think we need a deeper investigation into how/where these variable were removed in the first place. Having now delved through transfers.. I'm still confused by this one. :S OK, I've now discovered the issue here.. or rather a customer whose recently upgraded has. Test plan 1/ Check an item out from it's home library 2/ Check the item in at another library (with the return to home branch system preferences enabled) 2a/ Accept the transfer and note we now have a transfer present from the items check-in library to it's home library 3/ Check the item in at a third library 3a/ Note you are asked to return the item to it's home library (but the fact this is a WrongTransfer is not highlighted in any way) 4/ Go to the item record and note the holding library has been updated to reflect where the item was most recently checked in. 5/ Note, however, that the status continues to display that the item is in transit from the original check-in library to the home library.. this should have been updated to reflect that the item is now in transit from the new holding library to the home library. I'll work on clarifying the actions here. Created attachment 120625 [details] [review] Bug 24434: Reinstate updateWrongTransfer This patch re-instates the call to updateWrongTransfer to ensure the transfer line it updated to reflect the new frombranch. Test plan 1/ Check an item out from it's home library 2/ Check the item in at another library (with the return to home branch system preferences enabled) 2a/ Accept the transfer and note we now have a transfer present from the items check-in library to it's home library 3/ Check the item in at a third library 3a/ Note you are asked to return the item to it's home library. 3b/ With the patch applied, the modal title should highlight that a 'Wrong transfer' was detected. 4/ Go to the item record and note the holding library has been updated to reflect where the item was most recently checked in. 4a/ With the patch applied the item status should reflect the last checked in branch as the 'from' branch of the transfer. Created attachment 120631 [details] [review] Bug 24434: Reinstate updateWrongTransfer This patch re-instates the call to updateWrongTransfer to ensure the transfer line it updated to reflect the new frombranch. Test plan 1/ Check an item out from it's home library 2/ Check the item in at another library (with the return to home branch system preferences enabled) 2a/ Accept the transfer and note we now have a transfer present from the items check-in library to it's home library 3/ Check the item in at a third library 3a/ Note you are asked to return the item to it's home library. 3b/ With the patch applied, the modal title should highlight that a 'Wrong transfer' was detected. 4/ Go to the item record and note the holding library has been updated to reflect where the item was most recently checked in. 4a/ With the patch applied the item status should reflect the last checked in branch as the 'from' branch of the transfer. Created attachment 120632 [details] [review] Bug 24434: Add 'WrongTransfer' to cancellation_reasons Hi Martin, This works if I use the 'OK' button, but it fails if I use the 'Print transfer slip' button. As the item is moving between libraries, we'd always print a transfer slip off for the book. Created attachment 120657 [details] [review] Bug 24434: Reinstate updateWrongTransfer This patch re-instates the call to updateWrongTransfer to ensure the transfer line it updated to reflect the new frombranch. Test plan 1/ Check an item out from it's home library 2/ Check the item in at another library (with the return to home branch system preferences enabled) 2a/ Accept the transfer and note we now have a transfer present from the items check-in library to it's home library 3/ Check the item in at a third library 3a/ Note you are asked to return the item to it's home library. 3b/ With the patch applied, the modal title should highlight that a 'Wrong transfer' was detected. 4/ Go to the item record and note the holding library has been updated to reflect where the item was most recently checked in. 4a/ With the patch applied the item status should reflect the last checked in branch as the 'from' branch of the transfer. 5/ Work through the above again, but use the 'Print slip' option for accepting the transfer and confirm that also works. Created attachment 120658 [details] [review] Bug 24434: Add 'WrongTransfer' to cancellation_reasons (In reply to Sally from comment #16) > Hi Martin, > > This works if I use the 'OK' button, but it fails if I use the 'Print > transfer slip' button. > > As the item is moving between libraries, we'd always print a transfer slip > off for the book. Amended patch attached, thanks for catching that Sally. It should now work as expected for both the 'OK' and 'Print slip' options for accepting the transfer. Hold off on testing.. I think I've found a bug in the process linked to the 'Cancel' option now... working on a followup Created attachment 120683 [details] [review] Bug 24434: Reinstate updateWrongTransfer This patch re-instates the call to updateWrongTransfer to ensure the transfer line is updated to reflect the new frombranch. Test plan 1/ Check an item out from it's home library 2/ Check the item in at another library (with the return to home branch system preferences enabled) 2a/ Accept the transfer and note we now have a transfer present from the items check-in library to it's home library 3/ Check the item in at a third library 3a/ Note you are asked to return the item to it's home library. 3b/ With the patch applied, the modal title should highlight that a 'Wrong transfer' was detected. 4/ Go to the item record and note the holding library has been updated to reflect where the item was most recently checked in. 4a/ With the patch applied the item status should reflect the last checked in branch as the 'from' branch of the transfer. 5/ Work through the above again, but use the 'Print slip' option for accepting the transfer and confirm that also works. 6/ Work through the above again, but use the 'Cancel' option and check that this results in the item staying at it's current location and the final transfer having been cancelled. Created attachment 120684 [details] [review] Bug 24434: Add 'WrongTransfer' to cancellation_reasons Well.. that turned out to be rather involved! trivial unit test is missing for ->to_branch ;) Created attachment 120757 [details] [review] Bug 24434: Add Unit Tests for relations This patch adds unit tests for the newly introduced to_library (and also add the missing test for the existing from_library). Created attachment 120758 [details] [review] Bug 24434: Unit tests for updateWrongTransfer Add unit tests to cover updateWrongTransfer When I check the item in at the third library - if I select 'Print slip' there is no transfer generated Also, I ran through the test plan without the patches, left the last item with the cancelled transfer, but it seemed to have generated an additional transfer: MariaDB [koha_kohadev]> select * from branchtransfers where itemnumber=975; +-------------------+------------+---------------------+---------------------+------------+-------------+---------------------+----------+----------+----------------------+---------------------+ | branchtransfer_id | itemnumber | daterequested | datesent | frombranch | datearrived | datecancelled | tobranch | comments | reason | cancellation_reason | +-------------------+------------+---------------------+---------------------+------------+-------------+---------------------+----------+----------+----------------------+---------------------+ | 4 | 975 | 2021-05-12 10:31:32 | 2021-05-12 10:31:32 | FFL | NULL | 2021-05-12 10:31:45 | CPL | NULL | ReturnToHome | Manual | | 5 | 975 | 2021-05-12 10:31:45 | NULL | FPL | NULL | NULL | FFL | NULL | TransferCancellation | NULL | +-------------------+------------+---------------------+---------------------+------------+-------------+---------------------+----------+----------+----------------------+---------------------+ This doesn't happen after the patches, but the only way I could clear that second transfer was to check the item in at FFL - checking in at CPL didn't offer option to cancel Created attachment 121029 [details] [review] Bug 24434: Reinstate updateWrongTransfer This patch re-instates the call to updateWrongTransfer to ensure the transfer line is updated to reflect the new frombranch. Test plan 1/ Check an item out from it's home library 2/ Check the item in at another library (with the return to home branch system preferences enabled) 2a/ Accept the transfer and note we now have a transfer present from the items check-in library to it's home library 3/ Check the item in at a third library 3a/ Note you are asked to return the item to it's home library. 3b/ With the patch applied, the modal title should highlight that a 'Wrong transfer' was detected. 4/ Go to the item record and note the holding library has been updated to reflect where the item was most recently checked in. 4a/ With the patch applied the item status should reflect the last checked in branch as the 'from' branch of the transfer. 5/ Work through the above again, but use the 'Print slip' option for accepting the transfer and confirm that also works. 6/ Work through the above again, but use the 'Cancel' option and check that this results in the item staying at it's current location and the final transfer having been cancelled. Created attachment 121030 [details] [review] Bug 24434: Add 'WrongTransfer' to cancellation_reasons Created attachment 121031 [details] [review] Bug 24434: Add Unit Tests for relations This patch adds unit tests for the newly introduced to_library (and also add the missing test for the existing from_library). Created attachment 121032 [details] [review] Bug 24434: Unit tests for updateWrongTransfer Add unit tests to cover updateWrongTransfer Created attachment 121033 [details] [review] Bug 24434: Submit on print slip This patch ensures that the print slip option on the WrongTransfer modal also submits the form and thus triggers the actual transfer of the item. Seems I had the last piece that wasn't committed.. I've added it as a final patch. Created attachment 121034 [details] [review] Bug 24434: Submit on print slip This patch ensures that the print slip option on the WrongTransfer modal also submits the form and thus triggers the actual transfer of the item. We also prevent a double transfer submission (and thus a cancellation line appearing in the branchtransfers table) for the case where a transfer back home is triggered. Created attachment 121057 [details] [review] Bug 24434: Reinstate updateWrongTransfer This patch re-instates the call to updateWrongTransfer to ensure the transfer line is updated to reflect the new frombranch. Test plan 1/ Check an item out from it's home library 2/ Check the item in at another library (with the return to home branch system preferences enabled) 2a/ Accept the transfer and note we now have a transfer present from the items check-in library to it's home library 3/ Check the item in at a third library 3a/ Note you are asked to return the item to it's home library. 3b/ With the patch applied, the modal title should highlight that a 'Wrong transfer' was detected. 4/ Go to the item record and note the holding library has been updated to reflect where the item was most recently checked in. 4a/ With the patch applied the item status should reflect the last checked in branch as the 'from' branch of the transfer. 5/ Work through the above again, but use the 'Print slip' option for accepting the transfer and confirm that also works. 6/ Work through the above again, but use the 'Cancel' option and check that this results in the item staying at it's current location and the final transfer having been cancelled. Created attachment 121058 [details] [review] Bug 24434: Add 'WrongTransfer' to cancellation_reasons Created attachment 121059 [details] [review] Bug 24434: Add Unit Tests for relations This patch adds unit tests for the newly introduced to_library (and also add the missing test for the existing from_library). Created attachment 121060 [details] [review] Bug 24434: Unit tests for updateWrongTransfer Add unit tests to cover updateWrongTransfer Created attachment 121061 [details] [review] Bug 24434: Submit on print slip This patch ensures that the print slip option on the WrongTransfer modal also submits the form and thus triggers the actual transfer of the item. We also prevent a double transfer submission (and thus a cancellation line appearing in the branchtransfers table) for the case where a transfer back home is triggered. Created attachment 121071 [details] [review] Bug 24434: Reinstate updateWrongTransfer This patch re-instates the call to updateWrongTransfer to ensure the transfer line is updated to reflect the new frombranch. Test plan note: when applying the patches, also update the database 1/ Check an item out from it's home library 2/ Check the item in at another library (with AutomaticItemReturn system preferences enabled) 2a/ Accept the transfer and note we now have a transfer present from the items check-in library to it's home library 3/ Check the item in at a third library 3a/ Note you are asked to return the item to it's home library. 3b/ With the patch applied, the modal title should highlight that a 'Wrong transfer' was detected. 4/ Go to the item record and note the holding library has been updated to reflect where the item was most recently checked in. 4a/ With the patch applied the item status should reflect the last checked in branch as the 'from' branch of the transfer. 5/ check-in from a 4th library, but use the 'Print slip' option for accepting the transfer and confirm that also works. 6/ check-in from a 5th library, but use the 'Cancel' option and check that this results in the item staying at it's current location and the final transfer having been cancelled. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 121072 [details] [review] Bug 24434: Add 'WrongTransfer' to cancellation_reasons Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 121073 [details] [review] Bug 24434: Add Unit Tests for relations This patch adds unit tests for the newly introduced to_library (and also add the missing test for the existing from_library). Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 121074 [details] [review] Bug 24434: Unit tests for updateWrongTransfer Add unit tests to cover updateWrongTransfer Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 121075 [details] [review] Bug 24434: Submit on print slip This patch ensures that the print slip option on the WrongTransfer modal also submits the form and thus triggers the actual transfer of the item. We also prevent a double transfer submission (and thus a cancellation line appearing in the branchtransfers table) for the case where a transfer back home is triggered. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> It work! :D Amended the test plan, here are the changes (listing in case I interpreted something wrong) the return to home branch system preferences => AutomaticItemReturn 5/ Work through the above again, but use the 'Print slip' option for accepting the transfer and confirm that also works. 6/ Work through the above again, but use the 'Cancel' option and check that this results in the item staying at it's current location and the final transfer having been cancelled. ↓↓↓↓↓↓↓↓↓↓↓↓ 5/ check-in from a 4th library, but use the 'Print slip' option for accepting the transfer and confirm that also works. 6/ check-in from a 5th library, but use the 'Cancel' option and check that this results in the item staying at it's current location and the final transfer having been cancelled. Mentioned the need to updatedatabase when applying patches Test notes: run the tests - apply only the test patches - prove t/db_dependent/Koha/Item/Transfer.t t/db_dependent/Circulation.t - it should fail - apply the other patches - updatedatabase - run the tests Just tried and it works :D Test Plan: 1) Apply these patches 2) Place a hold for pickup at Library B 3) Check in the item to create the transfer 4) Check in at Library C 5) Get the error "The reason parameter is mandatory at /usr/share/perl5/Exception/Class/Base.pm line 88" Created attachment 121515 [details] [review] Bug 24434: Add 'WrongTransfer' to cancellation_reasons Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Rebased a patch that had a small conflict. (In reply to Kyle M Hall from comment #47) > 5) Get the error "The reason parameter is mandatory at > /usr/share/perl5/Exception/Class/Base.pm line 88" I can't reproduce the error. I suspect that's because in 3) I was in the wrong library. Should I be in library B? Or another? Maybe it's about the holding library of the item. Since it has to trigger and transfer. I can't reproduce this error either.. Created attachment 121702 [details] [review] Bug 24434: Reinstate updateWrongTransfer This patch re-instates the call to updateWrongTransfer to ensure the transfer line is updated to reflect the new frombranch. Test plan note: when applying the patches, also update the database 1/ Check an item out from it's home library 2/ Check the item in at another library (with AutomaticItemReturn system preferences enabled) 2a/ Accept the transfer and note we now have a transfer present from the items check-in library to it's home library 3/ Check the item in at a third library 3a/ Note you are asked to return the item to it's home library. 3b/ With the patch applied, the modal title should highlight that a 'Wrong transfer' was detected. 4/ Go to the item record and note the holding library has been updated to reflect where the item was most recently checked in. 4a/ With the patch applied the item status should reflect the last checked in branch as the 'from' branch of the transfer. 5/ check-in from a 4th library, but use the 'Print slip' option for accepting the transfer and confirm that also works. 6/ check-in from a 5th library, but use the 'Cancel' option and check that this results in the item staying at it's current location and the final transfer having been cancelled. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 121703 [details] [review] Bug 24434: Add Unit Tests for relations This patch adds unit tests for the newly introduced to_library (and also add the missing test for the existing from_library). Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 121704 [details] [review] Bug 24434: Unit tests for updateWrongTransfer Add unit tests to cover updateWrongTransfer Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 121705 [details] [review] Bug 24434: Submit on print slip This patch ensures that the print slip option on the WrongTransfer modal also submits the form and thus triggers the actual transfer of the item. We also prevent a double transfer submission (and thus a cancellation line appearing in the branchtransfers table) for the case where a transfer back home is triggered. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 121706 [details] [review] Bug 24434: Add 'WrongTransfer' to cancellation_reasons Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 121707 [details] [review] Bug 24434: DBIC Update Minor rebase, and added the DBIC update patch in case that's what Kyle was missing ;) Created attachment 121798 [details] [review] Bug 24434: Reinstate updateWrongTransfer This patch re-instates the call to updateWrongTransfer to ensure the transfer line is updated to reflect the new frombranch. Test plan note: when applying the patches, also update the database 1/ Check an item out from it's home library 2/ Check the item in at another library (with AutomaticItemReturn system preferences enabled) 2a/ Accept the transfer and note we now have a transfer present from the items check-in library to it's home library 3/ Check the item in at a third library 3a/ Note you are asked to return the item to it's home library. 3b/ With the patch applied, the modal title should highlight that a 'Wrong transfer' was detected. 4/ Go to the item record and note the holding library has been updated to reflect where the item was most recently checked in. 4a/ With the patch applied the item status should reflect the last checked in branch as the 'from' branch of the transfer. 5/ check-in from a 4th library, but use the 'Print slip' option for accepting the transfer and confirm that also works. 6/ check-in from a 5th library, but use the 'Cancel' option and check that this results in the item staying at it's current location and the final transfer having been cancelled. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 121799 [details] [review] Bug 24434: Add Unit Tests for relations This patch adds unit tests for the newly introduced to_library (and also add the missing test for the existing from_library). Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 121800 [details] [review] Bug 24434: Unit tests for updateWrongTransfer Add unit tests to cover updateWrongTransfer Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 121801 [details] [review] Bug 24434: Submit on print slip This patch ensures that the print slip option on the WrongTransfer modal also submits the form and thus triggers the actual transfer of the item. We also prevent a double transfer submission (and thus a cancellation line appearing in the branchtransfers table) for the case where a transfer back home is triggered. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 121802 [details] [review] Bug 24434: Add 'WrongTransfer' to cancellation_reasons Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 121803 [details] [review] Bug 24434: DBIC Update Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 121804 [details] [review] Bug 24434: (QA follow-up) Remove tab character [squashed] Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> This patchset conflicts with bug 28520, I cannot apply both of them. Can you add a dependency and fix the conflict please? (adding the same comment there) (In reply to Jonathan Druart from comment #66) > This patchset conflicts with bug 28520, I cannot apply both of them. > Can you add a dependency and fix the conflict please? > (adding the same comment there) Shouldn't we actually keep the sub instead of removing the tests? (In reply to Kyle M Hall from comment #65) > Created attachment 121804 [details] [review] [review] > Bug 24434: (QA follow-up) Remove tab character > > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Patch will be squashed when pushed. Pushed to master for 21.11, thanks to everybody involved! Pushed to 21.05.x for 21.05.01 Does not apply on 20.11.x, maybe because of Bug 26481 or its children. Please provide rebased patches if needed |