1. Check in any item (e.g. 39999000012118) to trigger a transfer. Need to have a home library not in the current library. So it trigger a return-to-home transfer. 2. Print slip. 3. Exception 'Koha::Exceptions::Item::Transfer::InQueue' thrown 'Active item transfer already exists' with transfer => Koha::Item::Transfer=HASH(0x55ef3531c248)
Bisect says: bug 35721
@Martin: can you please have a look?
Confirmed it in KTD. Marking as CONFIRMED.
Looks like this was an underlying bug with the modal, which bug 35721 exposed: - AddReturn initiates a transfer and sends the corresponding message to returns.pl - returns.pl shows the modal - the Print Slip button reloads(?) returns.pl with a cud-dotransfer op, which initiates a new transfer ModItemTransfer was automatically cancelling the old transfer in the background when told to initiate a new one, but Koha::Item->request_transfer doesn't cancel the old transfer; it throws an exception instead. Not sure if this was introduced when the op parameters were modified for CSRF, or if it was present before then. Either way, it seems to be fixed just by making sure the modal sends cud-transfer instead of cud-dotransfer if a transfer has already been initiated. (Patch for that coming...just need to write a good test plan for it)
Created attachment 175677 [details] [review] Bug 38588: Don't generate a second transfer when printing transfer slip When an item is checked in that should automatically return to its home branch, AddReturn generates the appropriate transfer. In some cases, the Print Slip button generates a second transfer, which now causes an exception to be thrown. In returns.pl, the op cud-dotransfer is used when a transfer was not initiated previously and the librarian has confirmed they want to initiate and send a transfer. The op cud-transfer is used when a transfer has already been generated and the page needs to send or cancel the transfer. This patch updates the op from the modal to ensure that the correct of the two options is generated according to whether a transfer was already initiated or not. To test: 1. Find or create an item that belongs to a branch other than the logged-in branch 2. In a new tab, check in the item - this should trigger a Return to Home transfer (keep the bib record tab open for later) 3. Click "Print Slip" --> Internal server error - 'Active item transfer already exists' 4. Apply patch and restart_all 5. Check in item again and cancel the transfer 6. Check in item again to generate new transfer 7. Click "Print Slip" --> Checkin page loads correctly --> Refresh bib record and confirm item shows as in-transit 8. Enable syspref ShowAllCheckins 9. Repeat steps 5-7 --> Ensure item displays correctly in checkins table --> Refresh bib record and confirm item still shows as in-transit 10. Repeat steps 5-7, except this time click "Yes" instead of "Print Slip" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 10. Set syspref TransfersBlockCirc to Don't Block, and set AutomaticConfirmTransfer to Enable 11. Check in the item again and cancel the transfer 12. Check in the item to generate new transfer 13. Click away from the modal without selecting a response --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 14. Check out the item to a customer 15. Check the item in and click "Yes" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 16. Repeat steps 14-15, clicking "Print Slip" instead of "Yes" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 17. Repeat steps 14-15, clicking away from the modal without selecting a response --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit
Created attachment 175798 [details] [review] Bug 38588: Don't generate a second transfer when printing transfer slip When an item is checked in that should automatically return to its home branch, AddReturn generates the appropriate transfer. In some cases, the Print Slip button generates a second transfer, which now causes an exception to be thrown. In returns.pl, the op cud-dotransfer is used when a transfer was not initiated previously and the librarian has confirmed they want to initiate and send a transfer. The op cud-transfer is used when a transfer has already been generated and the page needs to send or cancel the transfer. This patch updates the op from the modal to ensure that the correct of the two options is generated according to whether a transfer was already initiated or not. To test: 1. Find or create an item that belongs to a branch other than the logged-in branch 2. In a new tab, check in the item - this should trigger a Return to Home transfer (keep the bib record tab open for later) 3. Click "Print Slip" --> Internal server error - 'Active item transfer already exists' 4. Apply patch and restart_all 5. Check in item again and cancel the transfer 6. Check in item again to generate new transfer 7. Click "Print Slip" --> Checkin page loads correctly --> Refresh bib record and confirm item shows as in-transit 8. Enable syspref ShowAllCheckins 9. Repeat steps 5-7 --> Ensure item displays correctly in checkins table --> Refresh bib record and confirm item still shows as in-transit 10. Repeat steps 5-7, except this time click "Yes" instead of "Print Slip" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 10. Set syspref TransfersBlockCirc to Don't Block, and set AutomaticConfirmTransfer to Enable 11. Check in the item again and cancel the transfer 12. Check in the item to generate new transfer 13. Click away from the modal without selecting a response --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 14. Check out the item to a customer 15. Check the item in and click "Yes" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 16. Repeat steps 14-15, clicking "Print Slip" instead of "Yes" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 17. Repeat steps 14-15, clicking away from the modal without selecting a response --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Can we please have a quick QA here for stables?
> 6. Check in item again to generate new transfer > 7. Click "Print Slip" > --> Checkin page loads correctly There, in 23.11 and 22.11 [1] - if I check in again I get - «Wrong transfer detected, please return item to [...]» - record page show item is in "Transit pending" - I print slip - record page show item is "In transit" - I can loop like that checking in and printing slip In main + the patch, the loop is twice as long and in one checkin I have «Please return this item» (after printing I have record page showing «In transit from» ) and the next one I have no transfer pop up. Just have the «Not checked out.» And above, even without the transfer pop up I have: «Reason for transfer Return to home library» And record page says «Transit pending» If I don't print I get the pop up every time. And record page says «Transit pending» all the time. So there is definitely unintended side effect. [1] https://pro-v2211-koha.demo.biblibre.eu/cgi-bin/koha/circ/returns.pl https://pro-v2311-koha.demo.biblibre.eu/cgi-bin/koha/circ/returns.pl
Turns out it's possible to revert bug 35721 without conflict! :D (to have a closer comparison than with a demo) git revert 89947e8cc8 4a68563397 0e91b107d3 No I have the same behavior as main + the patch here!!! (the loop alternating between the two states) What is the meaning of this? 😵💫😵💫😵💫 This is way too confusing, what is supposed to be correct behavior for checking in the same item (from different home library) over and over? That would be what I got in 23.11 and 22.11, right? If so, the next step I guess would be to bisect when this changed. Starting with before bug 35721. random test: revert bug 35721 + the patch here doesn't change anything.
> No I have the same behavior as main + the patch *Now
> what is supposed to be correct behavior for checking in the same item (from different home library) over and over? *and printing the slip when prompted ------ > If so, the next step I guess would be to bisect when this changed. Starting with before bug 35721. testing on f8e2048913: same behavior as main + the patch here. (the loop alternating between the two states) Then, does this mean we can move on testing this patch and what I've found is actually another cause and another ticket to open?
> 5. Check in item again and cancel the transfer Should there be a button to cancel the transfer in the modal? Because there is not. > 9. Repeat steps 5-7 > 5. Check in item again and cancel the transfer I see no pop up there so it's even further than not having a cancel button. That's the weird behavior of the previous comments. Since the last action was printing a slip. (step 7.) The next checkin of the same item doesn't trigger the transfer modal. (I still get "Reason for transfer:" "Return to home library" but not the pop up)
Created attachment 175958 [details] [review] Bug 38588: Don't generate a second transfer when printing transfer slip When an item is checked in that should automatically return to its home branch, AddReturn generates the appropriate transfer. In some cases, the Print Slip button generates a second transfer, which now causes an exception to be thrown. In returns.pl, the op cud-dotransfer is used when a transfer was not initiated previously and the librarian has confirmed they want to initiate and send a transfer. The op cud-transfer is used when a transfer has already been generated and the page needs to send or cancel the transfer. This patch updates the op from the modal to ensure that the correct of the two options is generated according to whether a transfer was already initiated or not. To test: 1. Find or create an item that belongs to a branch other than the logged-in branch 2. In a new tab, check in the item - this should trigger a Return to Home transfer (keep the bib record tab open for later) 3. Click "Print Slip" --> Internal server error - 'Active item transfer already exists' 4. Apply patch and restart_all 5. Check in item again and cancel the transfer 6. Check in item again to generate new transfer 7. Click "Print Slip" --> Checkin page loads correctly --> Refresh bib record and confirm item shows as in-transit 8. Enable syspref ShowAllCheckins 9. Repeat steps 5-7 --> Ensure item displays correctly in checkins table --> Refresh bib record and confirm item still shows as in-transit 10. Repeat steps 5-7, except this time click "Yes" instead of "Print Slip" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 10. Set syspref TransfersBlockCirc to Don't Block, and set AutomaticConfirmTransfer to Enable 11. Check in the item again and cancel the transfer 12. Check in the item to generate new transfer 13. Click away from the modal without selecting a response --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 14. Check out the item to a customer 15. Check the item in and click "Yes" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 16. Repeat steps 14-15, clicking "Print Slip" instead of "Yes" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 17. Repeat steps 14-15, clicking away from the modal without selecting a response --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit
Created attachment 175959 [details] [review] Bug 38588: (follow-up) OK button needs to resubmit returns.pl
I think there is a separate bug (or two) - when I test on main, with or without the patch, I get the following behavior: Scenario #1: Existing active (sent) transfer 1. Check in an item with an active transfer to a different branch ==> Expected: Wrong Transfer modal Item status should change based on modal response ==> Actual: No modal, just a "Reason for transfer: Return to home library" message. Also, the item switches from "in transit" to "transit pending" Scenario #2: Existing pending (unsent) transfer 1. Check in an item with a pending/unsent transfer to a different branch ==> Return to Home transfer modal appears with options OK or Print Slip 2. Click OK ==> Expected: Item status switches from "Transit pending" to "In transit" ==> Actual: Item status remains at "Transit pending" Scenario #2 is happening because the OK button doesn't reload returns.pl. (Actually, that's also the reason the OK button wasn't causing the same internal server error as the Print Slip button.) I went ahead and added a follow-up to this bug to fix the OK button, since it's a trivial change and should already be covered by the first patch's test plan. I filed bug 38789 for Scenario #1. (Added it here as a See also and not a dependency, as it's a different section of the template and shouldn't cause a merge conflict.)
To test this patch without bug 38789's patch, replace each step that says "Check in item again and cancel the transfer" with "Find a different item", or delete the transfer from the command line in KTD: ktd --shell koha-mysql kohadev DELETE FROM branchtransfers; (note that the above will delete all branchtransfers if you generated any others before testing this patch)
Created attachment 175978 [details] [review] Bug 38588: Don't generate a second transfer when printing transfer slip When an item is checked in that should automatically return to its home branch, AddReturn generates the appropriate transfer. In some cases, the Print Slip button generates a second transfer, which now causes an exception to be thrown. In returns.pl, the op cud-dotransfer is used when a transfer was not initiated previously and the librarian has confirmed they want to initiate and send a transfer. The op cud-transfer is used when a transfer has already been generated and the page needs to send or cancel the transfer. This patch updates the op from the modal to ensure that the correct of the two options is generated according to whether a transfer was already initiated or not. To test: 1. Find or create an item that belongs to a branch other than the logged-in branch 2. In a new tab, check in the item - this should trigger a Return to Home transfer (keep the bib record tab open for later) 3. Click "Print Slip" --> Internal server error - 'Active item transfer already exists' 4. Apply patch and restart_all 5. Check in item again and cancel the transfer 6. Check in item again to generate new transfer 7. Click "Print Slip" --> Checkin page loads correctly --> Refresh bib record and confirm item shows as in-transit 8. Enable syspref ShowAllCheckins 9. Repeat steps 5-7 --> Ensure item displays correctly in checkins table --> Refresh bib record and confirm item still shows as in-transit 10. Repeat steps 5-7, except this time click "Yes" instead of "Print Slip" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 10. Set syspref TransfersBlockCirc to Don't Block, and set AutomaticConfirmTransfer to Enable 11. Check in the item again and cancel the transfer 12. Check in the item to generate new transfer 13. Click away from the modal without selecting a response --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 14. Check out the item to a customer 15. Check the item in and click "Yes" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 16. Repeat steps 14-15, clicking "Print Slip" instead of "Yes" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 17. Repeat steps 14-15, clicking away from the modal without selecting a response --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175979 [details] [review] Bug 38588: (follow-up) OK button needs to resubmit returns.pl Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
> 11. Check in the item again and cancel the transfer I can't T_T (not related to this, opened bug 38793, what a minefield ^^") So I changed the sysprefs values to be able to cancel and changed again the sysprefs to be back on the test plan's track. ============= > To test this patch without bug 38789's patch, replace each step that says > "Check in item again and cancel the transfer" with "Find a different item", > or delete the transfer from the command line Oh I missed that. Anyway, here is another signoff tested with bug 38789 so we are more certain there would be no issue with both in main as that should be quickly the case as opposed to having only this. ============= There is also a rebase to solve a conflict with bug 38748. But I tested before the latest main commits and the rebase >_< The change is about a param used for the content of the PDF so that looks ok. And the patch is also from Emily and written in the same period of time.
... and this would be why I really wanted to refactor the modals here and clean up ;P The current logic is confusing and error-prone and we're duplicating modals numerous times instead of adapting on a single clear one. I'm really pleased to see some work going on here.. honestly I'm somewhat baffled by it all myself at this stage and medium term I think we do need to carry out that refactor and cleanup.. going through all the different settings and code paths and ratifying them... but probably not here and now.
(In reply to Martin Renvoize (ashimema) from comment #20) > ... and this would be why I really wanted to refactor the modals here and > clean up ;P > > The current logic is confusing and error-prone and we're duplicating modals > numerous times instead of adapting on a single clear one. > > I'm really pleased to see some work going on here.. honestly I'm somewhat > baffled by it all myself at this stage and medium term I think we do need to > carry out that refactor and cleanup.. going through all the different > settings and code paths and ratifying them... but probably not here and now. Could the modal clean-up make sense as a community sprint topic? https://wiki.koha-community.org/wiki/Roadmap_25.05
Thank you for all the testing notes, Victor! Taking a look at bugs 38793 and 38806... I'd be willing to work on a modal refactor at this point - I think this cluster of bugs is as good a reason as any to refactor now, while the code is more fresh in our minds (and as a way to hopefully flush out any other related bugs that we haven't identified yet.)
note: this also fixes the issue found in step 12 of the initial test plan of bug 38806 comment 0
Created attachment 176290 [details] [review] Bug 38588: Don't generate a second transfer when printing transfer slip When an item is checked in that should automatically return to its home branch, AddReturn generates the appropriate transfer. In some cases, the Print Slip button generates a second transfer, which now causes an exception to be thrown. In returns.pl, the op cud-dotransfer is used when a transfer was not initiated previously and the librarian has confirmed they want to initiate and send a transfer. The op cud-transfer is used when a transfer has already been generated and the page needs to send or cancel the transfer. This patch updates the op from the modal to ensure that the correct of the two options is generated according to whether a transfer was already initiated or not. To test: 1. Find or create an item that belongs to a branch other than the logged-in branch 2. In a new tab, check in the item - this should trigger a Return to Home transfer (keep the bib record tab open for later) 3. Click "Print Slip" --> Internal server error - 'Active item transfer already exists' 4. Apply patch and restart_all 5. Check in item again and cancel the transfer 6. Check in item again to generate new transfer 7. Click "Print Slip" --> Checkin page loads correctly --> Refresh bib record and confirm item shows as in-transit 8. Enable syspref ShowAllCheckins 9. Repeat steps 5-7 --> Ensure item displays correctly in checkins table --> Refresh bib record and confirm item still shows as in-transit 10. Repeat steps 5-7, except this time click "Yes" instead of "Print Slip" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 10. Set syspref TransfersBlockCirc to Don't Block, and set AutomaticConfirmTransfer to Enable 11. Check in the item again and cancel the transfer 12. Check in the item to generate new transfer 13. Click away from the modal without selecting a response --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 14. Check out the item to a customer 15. Check the item in and click "Yes" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 16. Repeat steps 14-15, clicking "Print Slip" instead of "Yes" --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit 17. Repeat steps 14-15, clicking away from the modal without selecting a response --> Ensure item displays correctly in the checkins table --> Refresh bib record and confirm item shows as in-transit Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 176291 [details] [review] Bug 38588: (follow-up) OK button needs to resubmit returns.pl Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Hi, Works perfectly according to the detailed test plan, QA tool happy, passing QA :)
(In reply to Baptiste Wojtkowski (bwoj) from comment #26) > Hi, > Works perfectly according to the detailed test plan, QA tool happy, passing > QA :) Thanks all!
Pushed for 25.05! Well done everyone, thank you!