I have not recreated personally, but we have found evidence of this multiple times on a site using stock rotation Scenario: 1 - Item is added to stock rotation 2 - The cronjob creates a 'StockRotationAdvance' transfer 3 - The item is checked in, which initiates the transfer, however, a hold is triggered 4 - The hold is confirmed, which cancels the StockRotationAdvance 5 - The item is checked out and returned 6 - The stock rotation cronjob eventually calls Koha::StockRotationItem->needs_advancing which looks for all StockRotationAdvance transfers, ordered by date arrived 7 - The only one has a NULL date arrived 8 - The date arrived is put into dt_from_string, which returns today when given null 9 - The date object above then calls 'delta_days' from dt_from_string() 10 - The number of days from NOW to NOW is always 0 11 - The item does not advance until manually forced
Created attachment 160514 [details] [review] Bug 35100: Unit tests This patch adds a unit test to ensure StockrotationAdvance transfers are not cancelled from request_transfer when called with 'replace'.
Created attachment 160515 [details] [review] Bug 35100: Prevent StockrotationAdvance transfers from being cancelled ModItemTransfer is still very heavy handed and outright overrules existing transfers in the queue. For StockrotationAdvance transfers it's important that they remain in the queue to get actioned later, even if a higher precident transfer is actions in the interim. This patch adds a clause to the cancellation call within request_transfer such that StockrationAdvance transfers are left in tact for the next time the item is checked in. Test plan 1) Item is added to stock rotation 2) The cronjob creates a 'StockRotationAdvance' transfer 3) The item is checked in, which initiates the transfer, however, a hold is triggered 4) The hold is confirmed, which replaces the StockRotationAdvance 5) The item is checked out 6) The item is checked in, which initiates the transfer again 6) The item is checked in at it's destination and the StockrotationAdvance transfer has the proper date arrived date set.
Created attachment 160557 [details] [review] Bug 35100: Unit tests This patch adds a unit test to ensure StockrotationAdvance transfers are not cancelled from request_transfer when called with 'replace'. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 160558 [details] [review] Bug 35100: Prevent StockrotationAdvance transfers from being cancelled ModItemTransfer is still very heavy handed and outright overrules existing transfers in the queue. For StockrotationAdvance transfers it's important that they remain in the queue to get actioned later, even if a higher precident transfer is actions in the interim. This patch adds a clause to the cancellation call within request_transfer such that StockrationAdvance transfers are left in tact for the next time the item is checked in. Test plan 1) Item is added to stock rotation 2) The cronjob creates a 'StockRotationAdvance' transfer 3) The item is checked in, which initiates the transfer, however, a hold is triggered 4) The hold is confirmed, which replaces the StockRotationAdvance 5) The item is checked out 6) The item is checked in, which initiates the transfer again 6) The item is checked in at it's destination and the StockrotationAdvance transfer has the proper date arrived date set. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
To recreate: 1 - Create a rota, CPL->FFL->IPT 2 - Add an item from Midway to the rota 3 - perl misc/cronjobs/stockrotation.pl -x 4 - Check the db - you have a stock roation transfer to CPL 5 - Place a hold for pickup at Union 6 - Check the item in at midway, confirm the hold and transfer 7 - Note there are 2 transfers in the DB, one for the and one for stock rotation I am signing this off as it does fix the problem, there are 2 issues to note though: - The pop for the holds transfer correctly shows where to transfer the item, however, it shows the reason as 'StockRotation' when it should be 'Reserve' - After the transfer is created for the hold, 2 transfers exist - the original stock rotation, and the hold transfer. The detail page seems to look at the earliest, so the item shows it is in transit to the rotation branch, rather than the branch of the request we are trying to fill - this will cause confusion and delay
Thanks for the testing Nick, I had a feeling we might have some straggling issues from this so it got me digging back into the transfers tree.. at least some of those issues have my attention already. I'll post once I've identified the relevant bugs and code :)
Hmm.. this sounds like the order_by in the relation isn't catching properly :(
Nope, the ordering works..
Created attachment 160575 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Remove current_branchtransfers prefect from detail.pl as doing a prefetch completely nukes the order_by that this transfer method relies heavily upon to get the right modified FIFO transfer of the queue.
Created attachment 160623 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Remove current_branchtransfers prefetch from detail.pl as doing a prefetch completely nukes the order_by that this transfer method relies heavily upon to get the right modified FIFO transfer of the queue.
OK.. I think this is the most backportable fix I can come up with.. the code in returns needs some reworking in the middle term.. we really shouldn't have as many variables passed around meaning almost the same things as we do.. and there's really no reason to have two distinct modals for the same actions.
Created attachment 160628 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Remove current_branchtransfers prefetch from detail.pl as doing a prefetch completely nukes the order_by that this transfer method relies heavily upon to get the right modified FIFO transfer of the queue. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 160641 [details] [review] Bug 35100: Unit tests This patch adds a unit test to ensure StockrotationAdvance transfers are not cancelled from request_transfer when called with 'replace'. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 160642 [details] [review] Bug 35100: Prevent StockrotationAdvance transfers from being cancelled ModItemTransfer is still very heavy handed and outright overrules existing transfers in the queue. For StockrotationAdvance transfers it's important that they remain in the queue to get actioned later, even if a higher precident transfer is actions in the interim. This patch adds a clause to the cancellation call within request_transfer such that StockrationAdvance transfers are left in tact for the next time the item is checked in. Test plan 1) Item is added to stock rotation 2) The cronjob creates a 'StockRotationAdvance' transfer 3) The item is checked in, which initiates the transfer, however, a hold is triggered 4) The hold is confirmed, which replaces the StockRotationAdvance 5) The item is checked out 6) The item is checked in, which initiates the transfer again 7) The item is checked in at it's destination and the StockrotationAdvance transfer has the proper date arrived date set. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 160643 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Update get_transfer and get_transfers to ensure ordering isn't lost when prefetch is used and add tests for this Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 160647 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Update get_transfer and get_transfers to ensure ordering isn't lost when prefetch is used and add tests for this Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 160648 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Update get_transfer and get_transfers to ensure ordering isn't lost when prefetch is used and add tests for this Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 160687 [details] [review] Bug 35100: Unit tests This patch adds a unit test to ensure StockrotationAdvance transfers are not cancelled from request_transfer when called with 'replace'. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 160688 [details] [review] Bug 35100: Prevent StockrotationAdvance transfers from being cancelled ModItemTransfer is still very heavy handed and outright overrules existing transfers in the queue. For StockrotationAdvance transfers it's important that they remain in the queue to get actioned later, even if a higher precident transfer is actions in the interim. This patch adds a clause to the cancellation call within request_transfer such that StockrationAdvance transfers are left in tact for the next time the item is checked in. Test plan 1) Item is added to stock rotation 2) The cronjob creates a 'StockRotationAdvance' transfer 3) The item is checked in, which initiates the transfer, however, a hold is triggered 4) The hold is confirmed, which replaces the StockRotationAdvance 5) The item is checked out 6) The item is checked in, which initiates the transfer again 7) The item is checked in at it's destination and the StockrotationAdvance transfer has the proper date arrived date set. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 160689 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Update get_transfer and get_transfers to ensure ordering isn't lost when prefetch is used and add tests for this Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 160690 [details] [review] Bug 35100: Revert change to request_transfer This patch reverts the change to request_transfer, opting to tackle the StockRotationAdvance requirement to stay in place in ModItemTransfer itself. We also add a FIXME to RotatingCollections.. I'll look to removing that on another bug to reduce the scope of this one.
Looking here
QA Comment: Brave effort! This is a hard one. Very easy to introduce new bugs here in this minefield. Not really sure if we do not. I would recommend squashing here. Some changes are undone in followups. I found one problem with the WasTransfered change (this kind of changes are especially tricky): This test might go wrong in t/db_dependent/SIP/Transaction.t ? my $result = $ci_transaction->do_checkin( $library2->branchcode, undef ); is( $ci_transaction->alert_type, '04', "Checkin of item no issued at another branch succeeds" ); is_deeply( $result, { messages => { 'NotIssued' => $item->barcode, 'WasTransfered' => $library->branchcode, 'TransferTrigger' => 'ReturnToHome' } }, "Messages show not issued and transferred" ); => # Looks like you failed 1 test of 18. Also noting about WasTransfered: => The WasTransfered key is also used in sub transferbook. This might add confusion? Tidy stuff. No big deal. Just noting: WARN C4/RotatingCollections.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 48, now: 51) General observation. No offense. Patch 3 changes 300+ lines but has no test plan. Another minor remark: + elsif ( $code eq 'TransferTo' ) { + ; # Handled above, along with WasTransfered => Above suggests an action for WasTransfered, but there is none ? While reviewing calls of request_transfer and so, I asked myself: [1] Should current_branchtransfers not look at filled datesent ? Is a requested transfer really a current transfer? [2] updateWrongTransfer should correct destination but does it actually only fix the source of transfer ? Note that $FromLibrary is unused. Please fix the test. No reason found to block this patch set for other reasons. Only intuition that things might go wrong elsewhere. We should push soon to find these cases? Since Martin is committed to work in this area, things may improve gradually.
Created attachment 162740 [details] [review] Bug 35100: (follow-up) Fix test Previous patches changed the return values, just needed to update expectations
Created attachment 162741 [details] [review] Bug 35100: (follow-up) Tidy
Created attachment 162742 [details] [review] Bug 35100: Unit tests This patch adds a unit test to ensure StockrotationAdvance transfers are not cancelled from request_transfer when called with 'replace'. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162743 [details] [review] Bug 35100: Prevent StockrotationAdvance transfers from being cancelled ModItemTransfer is still very heavy handed and outright overrules existing transfers in the queue. For StockrotationAdvance transfers it's important that they remain in the queue to get actioned later, even if a higher precident transfer is actions in the interim. This patch adds a clause to the cancellation call within request_transfer such that StockrationAdvance transfers are left in tact for the next time the item is checked in. Test plan 1) Item is added to stock rotation 2) The cronjob creates a 'StockRotationAdvance' transfer 3) The item is checked in, which initiates the transfer, however, a hold is triggered 4) The hold is confirmed, which replaces the StockRotationAdvance 5) The item is checked out 6) The item is checked in, which initiates the transfer again 7) The item is checked in at it's destination and the StockrotationAdvance transfer has the proper date arrived date set. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162744 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Update get_transfer and get_transfers to ensure ordering isn't lost when prefetch is used and add tests for this Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162745 [details] [review] Bug 35100: Revert change to request_transfer This patch reverts the change to request_transfer, opting to tackle the StockRotationAdvance requirement to stay in place in ModItemTransfer itself. We also add a FIXME to RotatingCollections.. I'll look to removing that on another bug to reduce the scope of this one.
Created attachment 162746 [details] [review] Bug 35100: (follow-up) Fix test Previous patches changed the return values, just needed to update expectations
Created attachment 162747 [details] [review] Bug 35100: (follow-up) Tidy
(In reply to Marcel de Rooy from comment #23) > QA Comment: > I would recommend squashing here. Some changes are undone in followups. Fixed test and tidied, leaving squashing for Martin
Created attachment 162757 [details] [review] Bug 35100: Unit tests This patch adds a unit test to ensure StockrotationAdvance transfers are not cancelled from request_transfer when called with 'replace'. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 162758 [details] [review] Bug 35100: Prevent StockrotationAdvance transfers from being cancelled ModItemTransfer is still very heavy handed and outright overrules existing transfers in the queue. For StockrotationAdvance transfers it's important that they remain in the queue to get actioned later, even if a higher precident transfer is actions in the interim. This patch adds a clause to the cancellation call within request_transfer such that StockrationAdvance transfers are left in tact for the next time the item is checked in. Test plan 1) Item is added to stock rotation 2) The cronjob creates a 'StockRotationAdvance' transfer 3) The item is checked in, which initiates the transfer, however, a hold is triggered 4) The hold is confirmed, which replaces the StockRotationAdvance 5) The item is checked out 6) The item is checked in, which initiates the transfer again 7) The item is checked in at it's destination and the StockrotationAdvance transfer has the proper date arrived date set. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 162759 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Update get_transfer and get_transfers to ensure ordering isn't lost when prefetch is used and add tests for this Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 162760 [details] [review] Bug 35100: Revert change to request_transfer This patch reverts the change to request_transfer, opting to tackle the StockRotationAdvance requirement to stay in place in ModItemTransfer itself. We also add a FIXME to RotatingCollections.. I'll look to removing that on another bug to reduce the scope of this one. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 162761 [details] [review] Bug 35100: (follow-up) Fix test Previous patches changed the return values, just needed to update expectations Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 162762 [details] [review] Bug 35100: (follow-up) Tidy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Rebase is good, I tested.. Thanks for the follow-ups too which get the QA scripts happy again.
Hmm, did we loose this one?
Applying: Bug 35100: Various fixes error: sha1 information is lacking or useless (Koha/Item.pm). error: could not build fake ancestor
Created attachment 165096 [details] [review] Bug 35100: Unit tests This patch adds a unit test to ensure StockrotationAdvance transfers are not cancelled from request_transfer when called with 'replace'. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165097 [details] [review] Bug 35100: Prevent StockrotationAdvance transfers from being cancelled ModItemTransfer is still very heavy handed and outright overrules existing transfers in the queue. For StockrotationAdvance transfers it's important that they remain in the queue to get actioned later, even if a higher precident transfer is actions in the interim. This patch adds a clause to the cancellation call within request_transfer such that StockrationAdvance transfers are left in tact for the next time the item is checked in. Test plan 1) Item is added to stock rotation 2) The cronjob creates a 'StockRotationAdvance' transfer 3) The item is checked in, which initiates the transfer, however, a hold is triggered 4) The hold is confirmed, which replaces the StockRotationAdvance 5) The item is checked out 6) The item is checked in, which initiates the transfer again 7) The item is checked in at it's destination and the StockrotationAdvance transfer has the proper date arrived date set. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165098 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Update get_transfer and get_transfers to ensure ordering isn't lost when prefetch is used and add tests for this Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165099 [details] [review] Bug 35100: Revert change to request_transfer This patch reverts the change to request_transfer, opting to tackle the StockRotationAdvance requirement to stay in place in ModItemTransfer itself. We also add a FIXME to RotatingCollections.. I'll look to removing that on another bug to reduce the scope of this one. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165100 [details] [review] Bug 35100: (follow-up) Fix test Previous patches changed the return values, just needed to update expectations Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165101 [details] [review] Bug 35100: (follow-up) Tidy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Still aiming to come back here Marcel?
(In reply to Martin Renvoize from comment #48) > Still aiming to come back here Marcel? Wont make it very soon. Will remove my name here.
Created attachment 168273 [details] [review] Bug 35100: Unit tests This patch adds a unit test to ensure StockrotationAdvance transfers are not cancelled from request_transfer when called with 'replace'. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168274 [details] [review] Bug 35100: Prevent StockrotationAdvance transfers from being cancelled ModItemTransfer is still very heavy handed and outright overrules existing transfers in the queue. For StockrotationAdvance transfers it's important that they remain in the queue to get actioned later, even if a higher precident transfer is actions in the interim. This patch adds a clause to the cancellation call within request_transfer such that StockrationAdvance transfers are left in tact for the next time the item is checked in. Test plan 1) Item is added to stock rotation 2) The cronjob creates a 'StockRotationAdvance' transfer 3) The item is checked in, which initiates the transfer, however, a hold is triggered 4) The hold is confirmed, which replaces the StockRotationAdvance 5) The item is checked out 6) The item is checked in, which initiates the transfer again 7) The item is checked in at it's destination and the StockrotationAdvance transfer has the proper date arrived date set. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168275 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Update get_transfer and get_transfers to ensure ordering isn't lost when prefetch is used and add tests for this Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168276 [details] [review] Bug 35100: Revert change to request_transfer This patch reverts the change to request_transfer, opting to tackle the StockRotationAdvance requirement to stay in place in ModItemTransfer itself. We also add a FIXME to RotatingCollections.. I'll look to removing that on another bug to reduce the scope of this one. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168277 [details] [review] Bug 35100: (follow-up) Fix test Previous patches changed the return values, just needed to update expectations Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168278 [details] [review] Bug 35100: (follow-up) Tidy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Just another rebase.. it's all still applying.. the qa scripts are still happy.
Created attachment 169347 [details] [review] Bug 35100: Unit tests This patch adds a unit test to ensure StockrotationAdvance transfers are not cancelled from request_transfer when called with 'replace'. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169348 [details] [review] Bug 35100: Prevent StockrotationAdvance transfers from being cancelled ModItemTransfer is still very heavy handed and outright overrules existing transfers in the queue. For StockrotationAdvance transfers it's important that they remain in the queue to get actioned later, even if a higher precident transfer is actions in the interim. This patch adds a clause to the cancellation call within request_transfer such that StockrationAdvance transfers are left in tact for the next time the item is checked in. Test plan 1) Item is added to stock rotation 2) The cronjob creates a 'StockRotationAdvance' transfer 3) The item is checked in, which initiates the transfer, however, a hold is triggered 4) The hold is confirmed, which replaces the StockRotationAdvance 5) The item is checked out 6) The item is checked in, which initiates the transfer again 7) The item is checked in at it's destination and the StockrotationAdvance transfer has the proper date arrived date set. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169349 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Update get_transfer and get_transfers to ensure ordering isn't lost when prefetch is used and add tests for this Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169350 [details] [review] Bug 35100: Revert change to request_transfer This patch reverts the change to request_transfer, opting to tackle the StockRotationAdvance requirement to stay in place in ModItemTransfer itself. We also add a FIXME to RotatingCollections.. I'll look to removing that on another bug to reduce the scope of this one. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169351 [details] [review] Bug 35100: (follow-up) Fix test Previous patches changed the return values, just needed to update expectations Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169352 [details] [review] Bug 35100: (follow-up) Tidy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169353 [details] [review] Bug 35100: Additional Tidy for QA tools
Created attachment 171530 [details] [review] Bug 35100: Unit tests This patch adds a unit test to ensure StockrotationAdvance transfers are not cancelled from request_transfer when called with 'replace'. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171531 [details] [review] Bug 35100: Prevent StockrotationAdvance transfers from being cancelled ModItemTransfer is still very heavy handed and outright overrules existing transfers in the queue. For StockrotationAdvance transfers it's important that they remain in the queue to get actioned later, even if a higher precident transfer is actions in the interim. This patch adds a clause to the cancellation call within request_transfer such that StockrationAdvance transfers are left in tact for the next time the item is checked in. Test plan 1) Item is added to stock rotation 2) The cronjob creates a 'StockRotationAdvance' transfer 3) The item is checked in, which initiates the transfer, however, a hold is triggered 4) The hold is confirmed, which replaces the StockRotationAdvance 5) The item is checked out 6) The item is checked in, which initiates the transfer again 7) The item is checked in at it's destination and the StockrotationAdvance transfer has the proper date arrived date set. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171532 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Update get_transfer and get_transfers to ensure ordering isn't lost when prefetch is used and add tests for this Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171533 [details] [review] Bug 35100: Revert change to request_transfer This patch reverts the change to request_transfer, opting to tackle the StockRotationAdvance requirement to stay in place in ModItemTransfer itself. We also add a FIXME to RotatingCollections.. I'll look to removing that on another bug to reduce the scope of this one. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171534 [details] [review] Bug 35100: (follow-up) Fix test Previous patches changed the return values, just needed to update expectations Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171535 [details] [review] Bug 35100: (follow-up) Tidy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171559 [details] [review] Bug 35100: Unit tests This patch adds a unit test to ensure StockrotationAdvance transfers are not cancelled from request_transfer when called with 'replace'. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 171560 [details] [review] Bug 35100: Prevent StockrotationAdvance transfers from being cancelled ModItemTransfer is still very heavy handed and outright overrules existing transfers in the queue. For StockrotationAdvance transfers it's important that they remain in the queue to get actioned later, even if a higher precident transfer is actions in the interim. This patch adds a clause to the cancellation call within request_transfer such that StockrationAdvance transfers are left in tact for the next time the item is checked in. Test plan 1) Item is added to stock rotation 2) The cronjob creates a 'StockRotationAdvance' transfer 3) The item is checked in, which initiates the transfer, however, a hold is triggered 4) The hold is confirmed, which replaces the StockRotationAdvance 5) The item is checked out 6) The item is checked in, which initiates the transfer again 7) The item is checked in at it's destination and the StockrotationAdvance transfer has the proper date arrived date set. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 171561 [details] [review] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Update get_transfer and get_transfers to ensure ordering isn't lost when prefetch is used and add tests for this Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 171562 [details] [review] Bug 35100: Revert change to request_transfer This patch reverts the change to request_transfer, opting to tackle the StockRotationAdvance requirement to stay in place in ModItemTransfer itself. We also add a FIXME to RotatingCollections.. I'll look to removing that on another bug to reduce the scope of this one. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 171563 [details] [review] Bug 35100: (follow-up) Fix test Previous patches changed the return values, just needed to update expectations Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 171564 [details] [review] Bug 35100: (follow-up) Tidy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Thanks for all the hard work! Pushed to main for the next 24.11.00 release as RM Assistant
Doesn't apply clean to 24.05.x, rebase if needed.