Attachment 162745 Details for Bug 35100 – Bug 35100: Revert change to request_transfer

[patch] Bug 35100: Revert change to request_transfer

Bug-35100-Revert-change-to-requesttransfer.patch (text/plain), 8.93 KB, created by Nick Clemens (kidclamp) on 2024-03-05 12:41:12 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2024-03-05 12:41:12 UTC

Size: 8.93 KB

patch

obsolete

>From 5f5c346e56fb664810a52595182399cf16f5056a Mon Sep 17 00:00:00 2001
>From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>Date: Mon, 8 Jan 2024 17:34:09 +0000
>Subject: [PATCH] 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.
>---
> C4/Items.pm                | 56 ++++++++++++++++++++++++++++++--------
> C4/RotatingCollections.pm  |  7 +++--
> Koha/Item.pm               |  2 +-
> t/db_dependent/Items.t     | 30 +++++++++++++++++++-
> t/db_dependent/Koha/Item.t | 20 +-------------
> 5 files changed, 81 insertions(+), 34 deletions(-)
>
>diff --git a/C4/Items.pm b/C4/Items.pm
>index 37afaa8f04a..6fb0ed4924b 100644
>--- a/C4/Items.pm
>+++ b/C4/Items.pm
>@@ -54,6 +54,7 @@ use C4::Log qw( logaction );
> use List::MoreUtils qw( any );
> use DateTime::Format::MySQL;
>                   # debugging; so please don't remove this
>+use Try::Tiny qw( catch try );
> 
> use Koha::AuthorisedValues;
> use Koha::DateUtils qw( dt_from_string );
>@@ -359,31 +360,64 @@ The last optional parameter allows for passing skip_record_index through to the
> sub ModItemTransfer {
>     my ( $itemnumber, $frombranch, $tobranch, $trigger, $params ) = @_;
> 
>-    my $dbh = C4::Context->dbh;
>-    my $item = Koha::Items->find( $itemnumber );
>+    my $dbh  = C4::Context->dbh;
>+    my $item = Koha::Items->find($itemnumber);
> 
>     # NOTE: This retains the existing hard coded behaviour by ignoring transfer limits
>     # and always replacing any existing transfers. (In theory, calls to ModItemTransfer
>     # will have been preceded by a check of branch transfer limits)
>     my $to_library = Koha::Libraries->find($tobranch);
>-    my $transfer = $item->request_transfer(
>-        {
>-            to            => $to_library,
>-            reason        => $trigger,
>-            ignore_limits => 1,
>-            replace       => 1
>+    my $transfer;
>+    try {
>+        $transfer = $item->request_transfer(
>+            {
>+                to            => $to_library,
>+                reason        => $trigger,
>+                ignore_limits => 1,
>+            }
>+        );
>+    } catch {
>+        if ( $_->isa('Koha::Exceptions::Item::Transfer::InQueue') ) {
>+            my $exception      = $_;
>+            my $found_transfer = $_->transfer;
>+
>+            # If StockRotationAdvance, leave in place but ensure transit state is reset
>+            if ( $found_transfer->reason eq 'StockrotationAdvance' ) {
>+                $transfer = $item->request_transfer(
>+                    {
>+                        to            => $to_library,
>+                        reason        => $trigger,
>+                        ignore_limits => 1,
>+                        enqueue       => 1
>+                    }
>+                );
>+
>+                # Ensure transit is reset
>+                $found_transfer->datesent(undef)->store;
>+            } else {
>+                $transfer = $item->request_transfer(
>+                    {
>+                        to            => $to_library,
>+                        reason        => $trigger,
>+                        ignore_limits => 1,
>+                        replace       => 1
>+                    }
>+                );
>+            }
>+        } else {
>+            $_->rethrow();
>         }
>-    );
>+    };
> 
>     # Immediately set the item to in transit if it is checked in
>     if ( !$item->checkout ) {
>         $item->holdingbranch($frombranch)->store(
>             {
>                 log_action        => 0,
>-                skip_record_index => 1, # avoid indexing duplication, let ->transit handle it
>+                skip_record_index => 1,    # avoid indexing duplication, let ->transit handle it
>             }
>         );
>-        $transfer->transit({ skip_record_index => $params->{skip_record_index} });
>+        $transfer->transit( { skip_record_index => $params->{skip_record_index} } );
>     }
> 
>     return $transfer;
>diff --git a/C4/RotatingCollections.pm b/C4/RotatingCollections.pm
>index 18ba40a5c21..41d14d45aa4 100644
>--- a/C4/RotatingCollections.pm
>+++ b/C4/RotatingCollections.pm
>@@ -487,8 +487,11 @@ sub TransferCollection {
>                             replace       => 1
>                         }
>                     );    # Replace transfer
>-                    # NOTE: If we just replaced a StockRotationAdvance,
>-                    # it will get enqueued afresh on the next cron run
>+                    # FIXME: If we just replaced a StockRotationAdvance,
>+                    # it will get enqueued afresh on the next cron run.. but
>+                    # that will also push the stage on too.. and what about if
>+                    # we were at the first stage.. then there won't be a datearrived
>+                    # to calculate against. See bug 35100
>                 }
>             }
>             elsif ( $_->isa('Koha::Exceptions::Item::Transfer::Limit') ) {
>diff --git a/Koha/Item.pm b/Koha/Item.pm
>index 1156b6f4a04..9b31d46d196 100644
>--- a/Koha/Item.pm
>+++ b/Koha/Item.pm
>@@ -742,7 +742,7 @@ sub request_transfer {
>       if ( $request && !$params->{enqueue} && !$params->{replace} );
> 
>     $request->cancel( { reason => $params->{reason}, force => 1 } )
>-        if ( defined($request) && $params->{replace} && ( $request->reason ne 'StockrotationAdvance' ) );
>+        if ( defined($request) && $params->{replace} );
> 
>     my $transfer = Koha::Item::Transfer->new(
>         {
>diff --git a/t/db_dependent/Items.t b/t/db_dependent/Items.t
>index 84932499aff..5de288cd655 100755
>--- a/t/db_dependent/Items.t
>+++ b/t/db_dependent/Items.t
>@@ -130,7 +130,7 @@ subtest 'General Add, Get and Del tests' => sub {
> };
> 
> subtest 'ModItemTransfer tests' => sub {
>-    plan tests => 8;
>+    plan tests => 14;
> 
>     $schema->storage->txn_begin;
> 
>@@ -189,6 +189,34 @@ subtest 'ModItemTransfer tests' => sub {
>     my $transfer3 = $transfers->next;
>     is($transfer3->reason, 'Manual', "Reason set via ModItemTransfer");
> 
>+    # Ensure StockrotationAdvance transfers are not cancelled
>+    my $stock_transfer = $transfer3->reason('StockrotationAdvance')->store();
>+    is(
>+        $stock_transfer->reason, 'StockrotationAdvance',
>+        'StockrotationAdvance transfer set'
>+    );
>+    ok(
>+        $stock_transfer->datesent,
>+        'StockrotationAdvance transfer is in transit'
>+    );
>+
>+    ModItemTransfer( $item->itemnumber, $library1->{branchcode}, $library2->{branchcode}, 'Manual' );
>+    $transfers = Koha::Item::Transfers->search(
>+        { itemnumber => $item->itemnumber, },
>+        { order_by   => { '-desc' => 'branchtransfer_id' } }
>+    );
>+
>+    my $replaced_transfer = $transfers->next;
>+    is(
>+        $replaced_transfer->reason, 'Manual',
>+        'StockrotationAdvance transfer "replaced" by manual transfer at top of queue'
>+    );
>+    $stock_transfer->discard_changes;
>+    is( $stock_transfer->datecancelled, undef, "StockrotationAdvance transfer not cancelled" );
>+    is( $stock_transfer->datesent,      undef, "StockrotationAdvance transfer no longer in transit" );
>+    $transfers = $item->get_transfers;
>+    is( $transfers->count, 2, "There are now 2 live transfers in the queue" );
>+
>     $schema->storage->txn_rollback;
> };
> 
>diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t
>index a23698bb8a9..5adc0977684 100755
>--- a/t/db_dependent/Koha/Item.t
>+++ b/t/db_dependent/Koha/Item.t
>@@ -757,7 +757,7 @@ subtest 'pickup_locations() tests' => sub {
> };
> 
> subtest 'request_transfer' => sub {
>-    plan tests => 17;
>+    plan tests => 13;
>     $schema->storage->txn_begin;
> 
>     my $library1 = $builder->build_object( { class => 'Koha::Libraries' } );
>@@ -817,24 +817,6 @@ subtest 'request_transfer' => sub {
>     is($transfers->count, 1, "There is only 1 live transfer in the queue");
>     $replaced_transfer->datearrived(dt_from_string)->store();
> 
>-    # Replace StockrotationAdvance transfer
>-    my $stock_transfer = $item->request_transfer( { to => $library1, reason => 'StockrotationAdvance' } );
>-    is(
>-        ref($stock_transfer), 'Koha::Item::Transfer',
>-        'Koha::Item->request_transfer added StockrotationAdvance transfer'
>-    );
>-    $replaced_transfer = $item->request_transfer( { to => $library2, reason => 'Manual', replace => 1 } );
>-    is(
>-        ref($replaced_transfer), 'Koha::Item::Transfer',
>-        'Koha::Item->request_transfer allowed when replace is set'
>-    );
>-    $stock_transfer->discard_changes;
>-    is( $stock_transfer->datecancelled, undef, "StockrotationAdvance transfer left intact" );
>-    $transfers = $item->get_transfers;
>-    is( $transfers->count, 2, "There are now 2 live transfers in the queue" );
>-    $replaced_transfer->datearrived(dt_from_string)->store();
>-    $stock_transfer->datearrived(dt_from_string)->store();
>-
>     # BranchTransferLimits
>     t::lib::Mocks::mock_preference('UseBranchTransferLimits', 1);
>     t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'itemtype');
>-- 
>2.30.2

Actions: View | Diff | Splinter Review

Attachments on bug 35100: 160514 | 160515 | 160557 | 160558 | 160575 | 160623 | 160628 | 160641 | 160642 | 160643 | 160647 | 160648 | 160687 | 160688 | 160689 | 160690 | 162740 | 162741 | 162742 | 162743 | 162744 | 162745 | 162746 | 162747 | 162757 | 162758 | 162759 | 162760 | 162761 | 162762 | 165096 | 165097 | 165098 | 165099 | 165100 | 165101 | 168273 | 168274 | 168275 | 168276 | 168277 | 168278 | 169347 | 169348 | 169349 | 169350 | 169351 | 169352 | 169353 | 171530 | 171531 | 171532 | 171533 | 171534 | 171535 | 171559 | 171560 | 171561 | 171562 | 171563 | 171564