Attachment 110537 Details for Bug 24446 – Bug 24446: (follow-up) Handle cases of pre-existing transfers

[patch] Bug 24446: (follow-up) Handle cases of pre-existing transfers

Bug-24446-follow-up-Handle-cases-of-pre-existing-t.patch (text/plain), 20.06 KB, created by Martin Renvoize (ashimema) on 2020-09-22 11:14:12 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2020-09-22 11:14:12 UTC

Size: 20.06 KB

patch

obsolete

>From dadd8f59bdec31264dd351abf7f2a6f6e44b990c Mon Sep 17 00:00:00 2001
>From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>Date: Fri, 18 Sep 2020 14:11:14 +0100
>Subject: [PATCH] Bug 24446: (follow-up)  Handle cases of pre-existing
> transfers
>
>This is a long standing bug in stockrotation (and transfers in general)
>where by if a transfer is in progress and another transfer is requested
>then the original transfer is inexplicitly cancelled. This patch updates
>the stockrotation code to handle queued transfers, either adding a
>StockrotationAdvance transfer to the queue when an in progress transfer
>was triggered to fulfil a Reserve, or otherwise cancelling the transfer
>as stockrotation should take precidence.
>---
> Koha/Item.pm                        |  25 ++-
> Koha/StockRotationItem.pm           |  59 +++++--
> t/db_dependent/StockRotationItems.t | 238 ++++++++++++++++++++--------
> 3 files changed, 243 insertions(+), 79 deletions(-)
>
>diff --git a/Koha/Item.pm b/Koha/Item.pm
>index 23380068e1..f58debd9ee 100644
>--- a/Koha/Item.pm
>+++ b/Koha/Item.pm
>@@ -405,17 +405,25 @@ sub holds {
> =head3 request_transfer
> 
>   my $transfer = $item->request_transfer(
>-      { to => $to_library, reason => $reason, ignore_limits => 0 } );
>+    {
>+        to     => $to_library,
>+        reason => $reason,
>+        [ ignore_limits => 0, enqueue => 1, replace => 1 ]
>+    }
>+  );
> 
> Add a transfer request for this item to the given branch for the given reason.
> 
> An exception will be thrown if the BranchTransferLimits would prevent the requested
> transfer, unless 'ignore_limits' is passed to override the limits.
> 
>-Note: At this time, only one active transfer (i.e pending arrival date) may exist
>-at a time for any given item. An exception will be thrown should you attempt to
>-add a request when a transfer has already been queued, whether it is in transit
>-or just at the request stage.
>+An exception will be thrown if an active transfer (i.e pending arrival date) is found;
>+The caller should catch such cases and retry the transfer request as appropriate passing
>+an appropriate override.
>+
>+Overrides
>+* enqueue - Used to queue up the transfer when the existing transfer is found to be in transit.
>+* replace - Used to replace the existing transfer request with your own.
> 
> =cut
> 
>@@ -431,9 +439,11 @@ sub request_transfer {
>         }
>     }
> 
>-    my $request;
>+    my $request = $self->get_transfer;
>     Koha::Exceptions::Item::Transfer::Found->throw( transfer => $request )
>-      if ( $request = $self->get_transfer );
>+      if ( $request && !$params->{enqueue} && !$params->{replace} );
>+
>+    $request->cancel() if (defined($request) && $params->{replace});
> 
>     Koha::Exceptions::Item::Transfer::Limit->throw()
>       unless ( $params->{ignore_limits}
>@@ -449,6 +459,7 @@ sub request_transfer {
>             comments      => $params->{comment}
>         }
>     )->store();
>+
>     return $transfer;
> }
> 
>diff --git a/Koha/StockRotationItem.pm b/Koha/StockRotationItem.pm
>index 94055034ec..f4605907a0 100644
>--- a/Koha/StockRotationItem.pm
>+++ b/Koha/StockRotationItem.pm
>@@ -26,6 +26,7 @@ use Koha::DateUtils qw/dt_from_string/;
> use Koha::Item::Transfer;
> use Koha::Item;
> use Koha::StockRotationStage;
>+use Try::Tiny;
> 
> use base qw(Koha::Object);
> 
>@@ -143,9 +144,12 @@ sub needs_advancing {
> 
>   1|0 = $sritem->repatriate
> 
>-Put this item into branch transfer with 'StockrotationCorrection' comment, so
>+Put this item into branch transfer with 'StockrotationRepatriation' comment, so
> that it may return to it's stage.branch to continue its rota as normal.
> 
>+Note: Stockrotation falls outside of the normal branch transfer limits and so we
>+pass 'ignore_limits' in the call to request_transfer.
>+
> =cut
> 
> sub repatriate {
>@@ -154,9 +158,10 @@ sub repatriate {
>     # Create the transfer.
>     my $transfer = $self->itemnumber->request_transfer(
>         {
>-            to      => $self->stage->branchcode,
>-            reason  => "StockrotationRepatriation",
>-            comment => $msg
>+            to            => $self->stage->branchcode,
>+            reason        => "StockrotationRepatriation",
>+            comment       => $msg,
>+            ignore_limits => 1
>         }
>     );
> 
>@@ -180,6 +185,9 @@ StockRotationItem.
> If this item is 'indemand', and advance is invoked, we disable 'indemand' and
> advance the item as per usual.
> 
>+Note: Stockrotation falls outside of the normal branch transfer limits and so we
>+pass 'ignore_limits' in the call to request_transfer.
>+
> =cut
> 
> sub advance {
>@@ -221,13 +229,44 @@ sub advance {
>     # Update stage and record transfer
>     $self->stage_id( $new_stage->stage_id )->store;          # Set new stage
>     $item->homebranch( $new_stage->branchcode_id )->store;   # Update homebranch
>-    $transfer = $item->request_transfer(
>-        {
>-            to            => $new_stage->branchcode,
>-            reason        => "StockrotationAdvance",
>-            ignore_limits => 1
>+    $transfer = try {
>+        $item->request_transfer(
>+            {
>+                to            => $new_stage->branchcode,
>+                reason        => "StockrotationAdvance",
>+                ignore_limits => 1                      # Ignore transfer limits
>+            }
>+        );                                              # Add transfer
>+    }
>+    catch {
>+        if ( $_->isa('Koha::Exceptions::Item::Transfer::Found') ) {
>+            my $exception = $_;
>+            my $found_transfer = $_->transfer;
>+            if (   $found_transfer->in_transit
>+                || $found_transfer->reason eq 'Reserve' )
>+            {
>+                return $item->request_transfer(
>+                    {
>+                        to            => $new_stage->branchcode,
>+                        reason        => "StockrotationAdvance",
>+                        ignore_limits => 1,
>+                        enqueue       => 1
>+                    }
>+                );                                      # Queue transfer
>+            } else {
>+                return $item->request_transfer(
>+                    {
>+                        to            => $new_stage->branchcode,
>+                        reason        => "StockrotationAdvance",
>+                        ignore_limits => 1,
>+                        replace       => 1
>+                    }
>+                );                                      # Replace transfer
>+            }
>+        } else {
>+            $_->rethrow();
>         }
>-    );                                                       # Add transfer
>+    };
>     $transfer->receive
>       if $item->holdingbranch eq $new_stage->branchcode_id;  # Already at branch
> 
>diff --git a/t/db_dependent/StockRotationItems.t b/t/db_dependent/StockRotationItems.t
>index 0fef1b6092..c50abc5e53 100755
>--- a/t/db_dependent/StockRotationItems.t
>+++ b/t/db_dependent/StockRotationItems.t
>@@ -24,7 +24,9 @@ use DateTime::Duration;
> use Koha::Database;
> use Koha::DateUtils;
> use Koha::Item::Transfer;
>+
> use t::lib::TestBuilder;
>+use t::lib::Mocks;
> 
> use Test::More tests => 8;
> 
>@@ -156,6 +158,10 @@ subtest "Tests for repatriate." => sub {
>     is($intransfer->frombranch, $branch->{branchcode}, "Origin correct.");
>     is($intransfer->tobranch, $dbitem->stage->branchcode_id, "Target Correct.");
> 
>+    # Stockrotation should overrule transfer limits
>+
>+    # Stockrotation should handle transfer clashes
>+
>     $schema->storage->txn_rollback;
> };
> 
>@@ -229,112 +235,220 @@ subtest "Tests for needs_advancing." => sub {
> };
> 
> subtest "Tests for advance." => sub {
>-    plan tests => 23;
>+    plan tests => 44;
>     $schema->storage->txn_begin;
> 
>-    my $sritem = $builder->build(
>+    my $sritem_1 = $builder->build_object(
>         {
>-            source => 'Stockrotationitem',
>+            class => 'Koha::StockRotationItems',
>             value  => {
>                 'fresh'       => 1,
>                 itemnumber_id => $builder->build_sample_item->itemnumber
>             }
>         }
>     );
>-    my $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id});
>-    $dbitem->itemnumber->holdingbranch($dbitem->stage->branchcode_id);
>-    my $dbstage = $dbitem->stage;
>-    $dbstage->position(1)->duration(50)->store; # Configure stage.
>+    $sritem_1->discard_changes;
>+    $sritem_1->itemnumber->holdingbranch($sritem_1->stage->branchcode_id);
>+    my $item_id = $sritem_1->itemnumber->itemnumber;
>+    my $srstage_1 = $sritem_1->stage;
>+    $srstage_1->position(1)->duration(50)->store; # Configure stage.
>     # Configure item
>-    $dbitem->itemnumber->holdingbranch($dbstage->branchcode_id)->store;
>-    $dbitem->itemnumber->homebranch($dbstage->branchcode_id)->store;
>+    $sritem_1->itemnumber->holdingbranch($srstage_1->branchcode_id)->store;
>+    $sritem_1->itemnumber->homebranch($srstage_1->branchcode_id)->store;
>     # Sanity check
>-    is($dbitem->stage->stage_id, $dbstage->stage_id, "Stage sanity check.");
>+    is($sritem_1->stage->stage_id, $srstage_1->stage_id, "Stage sanity check.");
> 
>     # Test if an item is fresh, always move to first stage.
>-    is($dbitem->fresh, 1, "Fresh is correct.");
>-    $dbitem->advance;
>-    is($dbitem->stage->stage_id, $dbstage->stage_id, "Stage is first stage after fresh advance.");
>-    is($dbitem->fresh, 0, "Fresh reset after advance.");
>+    is($sritem_1->fresh, 1, "Fresh is correct.");
>+    $sritem_1->advance;
>+    is($sritem_1->stage->stage_id, $srstage_1->stage_id, "Stage is first stage after fresh advance.");
>+    is($sritem_1->fresh, 0, "Fresh reset after advance.");
> 
>     # Test cases of single stage
>-    $dbstage->rota->cyclical(1)->store;         # Set Rota to cyclical.
>-    ok($dbitem->advance, "Single stage cyclical advance done.");
>-    ## Refetch dbitem
>-    $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id});
>-    is($dbitem->stage->stage_id, $dbstage->stage_id, "Single stage cyclical stage OK.");
>+    $srstage_1->rota->cyclical(1)->store;         # Set Rota to cyclical.
>+    ok($sritem_1->advance, "Single stage cyclical advance done.");
>+    ## Refetch sritem_1
>+    $sritem_1->discard_changes;
>+    is($sritem_1->stage->stage_id, $srstage_1->stage_id, "Single stage cyclical stage OK.");
> 
>     # Test with indemand advance
>-    $dbitem->indemand(1)->store;
>-    ok($dbitem->advance, "Indemand item advance done.");
>-    ## Refetch dbitem
>-    $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id});
>-    is($dbitem->indemand, 0, "Indemand OK.");
>-    is($dbitem->stage->stage_id, $dbstage->stage_id, "Indemand item advance stage OK.");
>+    $sritem_1->indemand(1)->store;
>+    ok($sritem_1->advance, "Indemand item advance done.");
>+    ## Refetch sritem_1
>+    $sritem_1->discard_changes;
>+    is($sritem_1->indemand, 0, "Indemand OK.");
>+    is($sritem_1->stage->stage_id, $srstage_1->stage_id, "Indemand item advance stage OK.");
> 
>     # Multi stages
>-    my $srstage = $builder->build({
>-        source => 'Stockrotationstage',
>+    my $srstage_2 = $builder->build_object({
>+        class => 'Koha::StockRotationStages',
>         value => { duration => 50 }
>     });
>-    my $dbstage2 = Koha::StockRotationStages->find($srstage->{stage_id});
>-    $dbstage2->move_to_group($dbitem->stage->rota_id);
>-    $dbstage2->move_last;
>+    $srstage_2->discard_changes;
>+    $srstage_2->move_to_group($sritem_1->stage->rota_id);
>+    $srstage_2->move_last;
> 
>     # Test a straight up advance
>-    ok($dbitem->advance, "Advancement done.");
>-    ## Refetch dbitem
>-    $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id});
>+    ok($sritem_1->advance, "Advancement done.");
>+    ## Refetch sritem_1
>+    $sritem_1->discard_changes;
>     ## Test results
>-    is($dbitem->stage->stage_id, $dbstage2->stage_id, "Stage updated.");
>+    is($sritem_1->stage->stage_id, $srstage_2->stage_id, "Stage updated.");
>     is(
>-        $dbitem->itemnumber->homebranch,
>-        $dbstage2->branchcode_id,
>+        $sritem_1->itemnumber->homebranch,
>+        $srstage_2->branchcode_id,
>         "Item homebranch updated"
>     );
>-    my $intransfer = $dbitem->itemnumber->get_transfer;
>-    is($intransfer->frombranch, $dbstage->branchcode_id, "Origin correct.");
>-    is($intransfer->tobranch, $dbstage2->branchcode_id, "Target Correct.");
>+    my $transfer_request = $sritem_1->itemnumber->get_transfer;
>+    is($transfer_request->frombranch, $srstage_1->branchcode_id, "Origin correct.");
>+    is($transfer_request->tobranch, $srstage_2->branchcode_id, "Target Correct.");
>+    is($transfer_request->datesent, undef, "Transfer requested, but not sent.");
> 
>     # Arrive at new branch
>-    $intransfer->datearrived(dt_from_string())->store;
>-    $dbitem->itemnumber->holdingbranch($srstage->{branchcode_id})->store;
>+    $transfer_request->datearrived(dt_from_string())->store;
>+    $sritem_1->itemnumber->holdingbranch($srstage_2->branchcode_id)->store;
> 
>     # Test a cyclical advance
>-    ok($dbitem->advance, "Cyclical advancement done.");
>-    ## Refetch dbitem
>-    $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id});
>+    ok($sritem_1->advance, "Cyclical advancement done.");
>+    ## Refetch sritem_1
>+    $sritem_1->discard_changes;
>     ## Test results
>-    is($dbitem->stage->stage_id, $dbstage->stage_id, "Stage updated.");
>+    is($sritem_1->stage->stage_id, $srstage_1->stage_id, "Stage updated.");
>     is(
>-        $dbitem->itemnumber->homebranch,
>-        $dbstage->branchcode_id,
>+        $sritem_1->itemnumber->homebranch,
>+        $srstage_1->branchcode_id,
>         "Item homebranch updated"
>     );
>-    $intransfer = $dbitem->itemnumber->get_transfer;
>-    is($intransfer->frombranch, $dbstage2->branchcode_id, "Origin correct.");
>-    is($intransfer->tobranch, $dbstage->branchcode_id, "Target correct.");
>+    $transfer_request = $sritem_1->itemnumber->get_transfer;
>+    is($transfer_request->frombranch, $srstage_2->branchcode_id, "Origin correct.");
>+    is($transfer_request->tobranch, $srstage_1->branchcode_id, "Target correct.");
> 
>     # Arrive at new branch
>-    $intransfer->datearrived(dt_from_string())->store;
>-    $dbitem->itemnumber->holdingbranch($srstage->{branchcode_id})->store;
>+    $transfer_request->datearrived(dt_from_string())->store;
>+    $sritem_1->itemnumber->holdingbranch($srstage_1->branchcode_id)->store;
>+
>+    # Confirm that stockrotation ignores transfer limits
>+    t::lib::Mocks::mock_preference('UseBranchTransferLimits', 1);
>+    t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'itemtype');
>+    my $limit = Koha::Item::Transfer::Limit->new(
>+        {
>+            fromBranch => $srstage_1->branchcode_id,
>+            toBranch   => $srstage_2->branchcode_id,
>+            itemtype   => $sritem_1->itemnumber->effective_itemtype,
>+        }
>+    )->store;
> 
>-    $dbstage->rota->cyclical(0)->store;         # Set Rota to non-cyclical.
>+    ok($sritem_1->advance, "Advancement overrules transfer limits.");
>+    ## Refetch sritem_1
>+    $sritem_1->discard_changes;
>+    ## Test results
>+    is($sritem_1->stage->stage_id, $srstage_2->stage_id, "Stage updated ignoring transfer limits.");
>+    is(
>+        $sritem_1->itemnumber->homebranch,
>+        $srstage_2->branchcode_id,
>+        "Item homebranch updated ignoring transfer limits"
>+    );
>+    $transfer_request = $sritem_1->itemnumber->get_transfer;
>+    is($transfer_request->frombranch, $srstage_1->branchcode_id, "Origin correct ignoring transfer limits.");
>+    is($transfer_request->tobranch, $srstage_2->branchcode_id, "Target correct ignoring transfer limits.");
>+
>+    # Arrive at new branch
>+    $transfer_request->datearrived(dt_from_string())->store;
>+    $sritem_1->itemnumber->holdingbranch($srstage_2->branchcode_id)->store;
>+   
>+    # Setup a conflicting manual transfer
>+    my $item = Koha::Items->find($item_id);
>+    $item->request_transfer({ to => $srstage_1->branchcode, reason => "Manual" });
>+    $transfer_request = $item->get_transfer;
>+    is (ref($transfer_request), 'Koha::Item::Transfer', "Conflicting transfer added");
>+    is ($transfer_request->reason, 'Manual', "Conflicting transfer reason is 'Manual'");
>+
>+    # Advance item whilst conflicting manual transfer exists
>+    ok($sritem_1->advance, "Advancement done.");
>+    ## Refetch sritem_1
>+    $sritem_1->discard_changes;
>+
>+    ## Refetch conflicted transfer
>+    $transfer_request->discard_changes;
>+
>+    # Conflicted transfer should have been cancelled
>+    isnt($transfer_request->datecancelled, undef, "Conflicting manual transfer was cancelled");
>+
>+    # StockRotationAdvance transfer added
>+    $transfer_request = $sritem_1->itemnumber->get_transfer;
>+    is($transfer_request->reason, 'StockrotationAdvance', "StockrotationAdvance transfer added");
>+    is($transfer_request->frombranch, $srstage_2->branchcode_id, "Origin correct.");
>+    is($transfer_request->tobranch, $srstage_1->branchcode_id, "Target correct.");
>+
>+    # Arrive at new branch
>+    $transfer_request->datearrived(dt_from_string())->store;
>+    $sritem_1->itemnumber->holdingbranch($srstage_1->branchcode_id)->store;
>+
>+    # Setup a conflicting reserve transfer
>+    $item->request_transfer({ to => $srstage_2->branchcode, reason => "Reserve" });
>+    $transfer_request = $item->get_transfer;
>+    is (ref($transfer_request), 'Koha::Item::Transfer', "Conflicting transfer added");
>+    is ($transfer_request->reason, 'Reserve', "Conflicting transfer reason is 'Reserve'");
>+
>+    # Advance item whilst conflicting reserve transfer exists
>+    ok($sritem_1->advance, "Advancement done.");
>+    ## Refetch sritem_1
>+    $sritem_1->discard_changes;
>+
>+    ## Refetch conflicted transfer
>+    $transfer_request->discard_changes;
>+
>+    # Conflicted transfer should not been cancelled
>+    is($transfer_request->datecancelled, undef, "Conflicting reserve transfer was not cancelled");
>+
>+    # StockRotationAdvance transfer added
>+    my $transfer_requests = Koha::Item::Transfers->search(
>+        {
>+            itemnumber    => $sritem_1->itemnumber->itemnumber,
>+            datearrived   => undef,
>+            datecancelled => undef
>+        }
>+    );
>+    is($transfer_requests->count, '2', "StockrotationAdvance transfer queued");
>+
>+    # Arrive at new branch
>+    $transfer_request->datearrived(dt_from_string())->store;
>+    $sritem_1->itemnumber->holdingbranch($srstage_2->branchcode_id)->store;
>+
>+    # StockRotationAdvance transfer added
>+    $transfer_request = $sritem_1->itemnumber->get_transfer;
>+    is($transfer_request->reason, 'StockrotationAdvance', "StockrotationAdvance transfer remains after reserve is met");
>+    is($transfer_request->frombranch, $srstage_1->branchcode_id, "Origin correct.");
>+    is($transfer_request->tobranch, $srstage_2->branchcode_id, "Target correct.");
>+    
>+    # Arrive at new branch
>+    $transfer_request->datearrived(dt_from_string())->store;
>+    $sritem_1->itemnumber->holdingbranch($srstage_2->branchcode_id)->store;
>+
>+    $srstage_1->rota->cyclical(0)->store;         # Set Rota to non-cyclical.
>+
>+    my $srstage_3 = $builder->build_object({
>+        class => 'Koha::StockRotationStages',
>+        value => { duration => 50 }
>+    });
>+    $srstage_3->discard_changes;
>+    $srstage_3->move_to_group($sritem_1->stage->rota_id);
>+    $srstage_3->move_last;
> 
>     # Advance again, to end of rota.
>-    ok($dbitem->advance, "Non-cyclical advance to last stage.");
>+    ok($sritem_1->advance, "Non-cyclical advance to last stage.");
> 
>     # Arrive at new branch
>-    $intransfer->datearrived(dt_from_string())->store;
>-    $dbitem->itemnumber->holdingbranch($srstage->{branchcode_id})->store;
>+    $transfer_request->datearrived(dt_from_string())->store;
>+    $sritem_1->itemnumber->holdingbranch($srstage_3->branchcode_id)->store;
> 
>     # Advance again, Remove from rota.
>-    ok($dbitem->advance, "Non-cyclical advance.");
>-    ## Refetch dbitem
>-    $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id});
>-    is($dbitem, undef, "StockRotationItem has been removed.");
>-    my $item = Koha::Items->find($sritem->{itemnumber_id});
>-    is($item->homebranch, $srstage->{branchcode_id}, "Item homebranch remains");
>+    ok($sritem_1->advance, "Non-cyclical advance.");
>+    ## Refetch sritem_1
>+    $sritem_1 = Koha::StockRotationItems->find({ itemnumber_id => $item_id });
>+    is($sritem_1, undef, "StockRotationItem has been removed.");
>+    $item = Koha::Items->find($item_id);
>+    is($item->homebranch, $srstage_3->branchcode_id, "Item homebranch remains");
> 
>     $schema->storage->txn_rollback;
> };
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 24446: 105943 | 105944 | 105945 | 107246 | 107247 | 107248 | 107249 | 107250 | 107252 | 107253 | 107254 | 107255 | 107256 | 107301 | 107302 | 107303 | 107304 | 107305 | 110280 | 110281 | 110282 | 110283 | 110533 | 110534 | 110535 | 110536 | 110537 | 110538 | 110539 | 110540 | 110541 | 110542 | 111322 | 116557 | 116558 | 116559 | 116560 | 116561 | 116562 | 116563 | 116564 | 116565 | 116566 | 116567 | 116568 | 116569 | 116570 | 117589 | 117590 | 117591 | 117592 | 117593 | 117594 | 117595 | 117596 | 117597 | 117598 | 117599 | 117600 | 117601 | 117602 | 117603 | 117604 | 117605 | 117606 | 117607 | 120116