Bug 35100 - Items assigned to StockRotation do not advance if a reserve is triggered before the initial transfer
Summary: Items assigned to StockRotation do not advance if a reserve is triggered befo...
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 29234 34639
Blocks: 35721
  Show dependency treegraph
 
Reported: 2023-10-18 19:24 UTC by Nick Clemens (kidclamp)
Modified: 2024-07-03 12:02 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 35100: Unit tests (2.15 KB, patch)
2024-01-04 12:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Prevent StockrotationAdvance transfers from being cancelled (1.73 KB, patch)
2024-01-04 12:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Unit tests (2.21 KB, patch)
2024-01-04 20:31 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: Prevent StockrotationAdvance transfers from being cancelled (1.78 KB, patch)
2024-01-04 20:31 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: Various fixes (5.45 KB, patch)
2024-01-05 16:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Various fixes (9.09 KB, patch)
2024-01-08 12:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Various fixes (9.15 KB, patch)
2024-01-08 14:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: Unit tests (2.21 KB, patch)
2024-01-08 16:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Prevent StockrotationAdvance transfers from being cancelled (1.78 KB, patch)
2024-01-08 16:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Various fixes (13.61 KB, patch)
2024-01-08 16:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Various fixes (15.70 KB, patch)
2024-01-08 16:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Various fixes (15.89 KB, patch)
2024-01-08 16:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Unit tests (2.21 KB, patch)
2024-01-09 13:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Prevent StockrotationAdvance transfers from being cancelled (1.78 KB, patch)
2024-01-09 13:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Various fixes (24.97 KB, patch)
2024-01-09 13:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Revert change to request_transfer (8.93 KB, patch)
2024-01-09 13:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: (follow-up) Fix test (1.31 KB, patch)
2024-03-05 12:40 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: (follow-up) Tidy (2.25 KB, patch)
2024-03-05 12:40 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: Unit tests (2.21 KB, patch)
2024-03-05 12:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: Prevent StockrotationAdvance transfers from being cancelled (1.78 KB, patch)
2024-03-05 12:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: Various fixes (24.96 KB, patch)
2024-03-05 12:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: Revert change to request_transfer (8.93 KB, patch)
2024-03-05 12:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: (follow-up) Fix test (1.31 KB, patch)
2024-03-05 12:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: (follow-up) Tidy (2.25 KB, patch)
2024-03-05 12:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: Unit tests (2.27 KB, patch)
2024-03-05 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Prevent StockrotationAdvance transfers from being cancelled (1.85 KB, patch)
2024-03-05 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Various fixes (25.02 KB, patch)
2024-03-05 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Revert change to request_transfer (8.99 KB, patch)
2024-03-05 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: (follow-up) Fix test (1.38 KB, patch)
2024-03-05 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: (follow-up) Tidy (2.32 KB, patch)
2024-03-05 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Unit tests (2.27 KB, patch)
2024-04-18 13:33 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: Prevent StockrotationAdvance transfers from being cancelled (1.85 KB, patch)
2024-04-18 13:33 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: Various fixes (24.27 KB, patch)
2024-04-18 13:33 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: Revert change to request_transfer (8.99 KB, patch)
2024-04-18 13:33 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: (follow-up) Fix test (1.38 KB, patch)
2024-04-18 13:34 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: (follow-up) Tidy (2.32 KB, patch)
2024-04-18 13:34 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35100: Unit tests (2.27 KB, patch)
2024-06-28 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Prevent StockrotationAdvance transfers from being cancelled (1.85 KB, patch)
2024-06-28 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Various fixes (20.76 KB, patch)
2024-06-28 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: Revert change to request_transfer (8.99 KB, patch)
2024-06-28 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: (follow-up) Fix test (1.38 KB, patch)
2024-06-28 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35100: (follow-up) Tidy (2.32 KB, patch)
2024-06-28 16:17 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2023-10-18 19:24:47 UTC
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
Comment 1 Martin Renvoize 2024-01-04 12:20:09 UTC
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'.
Comment 2 Martin Renvoize 2024-01-04 12:20:12 UTC
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.
Comment 3 Nick Clemens (kidclamp) 2024-01-04 20:31:20 UTC
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>
Comment 4 Nick Clemens (kidclamp) 2024-01-04 20:31:22 UTC
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>
Comment 5 Nick Clemens (kidclamp) 2024-01-04 20:36:06 UTC
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
Comment 6 Martin Renvoize 2024-01-05 09:06:42 UTC
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 :)
Comment 7 Martin Renvoize 2024-01-05 09:15:46 UTC
Hmm.. this sounds like the order_by in the relation isn't catching properly :(
Comment 8 Martin Renvoize 2024-01-05 10:39:03 UTC
Nope, the ordering works..
Comment 9 Martin Renvoize 2024-01-05 16:11:19 UTC
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.
Comment 10 Martin Renvoize 2024-01-08 12:29:31 UTC
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.
Comment 11 Martin Renvoize 2024-01-08 12:31:12 UTC
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.
Comment 12 Nick Clemens (kidclamp) 2024-01-08 14:42:59 UTC
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>
Comment 13 Martin Renvoize 2024-01-08 16:11:31 UTC
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>
Comment 14 Martin Renvoize 2024-01-08 16:11:33 UTC
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>
Comment 15 Martin Renvoize 2024-01-08 16:11:36 UTC
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>
Comment 16 Martin Renvoize 2024-01-08 16:27:17 UTC
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>
Comment 17 Martin Renvoize 2024-01-08 16:31:25 UTC
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>
Comment 18 Martin Renvoize 2024-01-09 13:13:58 UTC
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>
Comment 19 Martin Renvoize 2024-01-09 13:14:01 UTC
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>
Comment 20 Martin Renvoize 2024-01-09 13:14:03 UTC
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>
Comment 21 Martin Renvoize 2024-01-09 13:14:06 UTC
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.
Comment 22 Marcel de Rooy 2024-01-12 08:02:27 UTC
Looking here
Comment 23 Marcel de Rooy 2024-01-12 09:42:41 UTC
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.
Comment 24 Nick Clemens (kidclamp) 2024-03-05 12:40:39 UTC
Created attachment 162740 [details] [review]
Bug 35100: (follow-up) Fix test

Previous patches changed the return values, just needed to update expectations
Comment 25 Nick Clemens (kidclamp) 2024-03-05 12:40:41 UTC
Created attachment 162741 [details] [review]
Bug 35100: (follow-up) Tidy
Comment 26 Nick Clemens (kidclamp) 2024-03-05 12:41:05 UTC
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>
Comment 27 Nick Clemens (kidclamp) 2024-03-05 12:41:07 UTC
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>
Comment 28 Nick Clemens (kidclamp) 2024-03-05 12:41:10 UTC
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>
Comment 29 Nick Clemens (kidclamp) 2024-03-05 12:41:12 UTC
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.
Comment 30 Nick Clemens (kidclamp) 2024-03-05 12:41:14 UTC
Created attachment 162746 [details] [review]
Bug 35100: (follow-up) Fix test

Previous patches changed the return values, just needed to update expectations
Comment 31 Nick Clemens (kidclamp) 2024-03-05 12:41:16 UTC
Created attachment 162747 [details] [review]
Bug 35100: (follow-up) Tidy
Comment 32 Nick Clemens (kidclamp) 2024-03-05 12:41:58 UTC
(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
Comment 33 Martin Renvoize 2024-03-05 15:09:18 UTC
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>
Comment 34 Martin Renvoize 2024-03-05 15:09:20 UTC
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>
Comment 35 Martin Renvoize 2024-03-05 15:09:23 UTC
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>
Comment 36 Martin Renvoize 2024-03-05 15:09:26 UTC
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>
Comment 37 Martin Renvoize 2024-03-05 15:09:29 UTC
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>
Comment 38 Martin Renvoize 2024-03-05 15:09:32 UTC
Created attachment 162762 [details] [review]
Bug 35100: (follow-up) Tidy

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2024-03-05 15:10:15 UTC
Rebase is good, I tested.. Thanks for the follow-ups too which get the QA scripts happy again.
Comment 40 Martin Renvoize 2024-03-22 17:02:56 UTC
Hmm, did we loose this one?
Comment 41 Marcel de Rooy 2024-04-18 12:16:33 UTC
Applying: Bug 35100: Various fixes
error: sha1 information is lacking or useless (Koha/Item.pm).
error: could not build fake ancestor
Comment 42 Nick Clemens (kidclamp) 2024-04-18 13:33:52 UTC
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>
Comment 43 Nick Clemens (kidclamp) 2024-04-18 13:33:54 UTC
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>
Comment 44 Nick Clemens (kidclamp) 2024-04-18 13:33:56 UTC
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>
Comment 45 Nick Clemens (kidclamp) 2024-04-18 13:33:58 UTC
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>
Comment 46 Nick Clemens (kidclamp) 2024-04-18 13:34:01 UTC
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>
Comment 47 Nick Clemens (kidclamp) 2024-04-18 13:34:03 UTC
Created attachment 165101 [details] [review]
Bug 35100: (follow-up) Tidy

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Martin Renvoize 2024-06-04 12:07:30 UTC
Still aiming to come back here Marcel?
Comment 49 Marcel de Rooy 2024-06-04 12:11:59 UTC
(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.
Comment 50 Martin Renvoize 2024-06-28 16:16:44 UTC
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>
Comment 51 Martin Renvoize 2024-06-28 16:16:48 UTC
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>
Comment 52 Martin Renvoize 2024-06-28 16:16:51 UTC
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>
Comment 53 Martin Renvoize 2024-06-28 16:16:54 UTC
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>
Comment 54 Martin Renvoize 2024-06-28 16:16:57 UTC
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>
Comment 55 Martin Renvoize 2024-06-28 16:17:00 UTC
Created attachment 168278 [details] [review]
Bug 35100: (follow-up) Tidy

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 56 Martin Renvoize 2024-06-28 16:17:34 UTC
Just another rebase.. it's all still applying.. the qa scripts are still happy.