Attachment 154151 Details for Bug 21159 – Bug 21159: Update item location on checkout

[patch] Bug 21159: Update item location on checkout

Bug-21159-Update-item-location-on-checkout.patch (text/plain), 14.79 KB, created by Alex Buckley on 2023-08-02 05:12:18 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Alex Buckley

Created: 2023-08-02 05:12:18 UTC

Size: 14.79 KB

patch

obsolete

>From 741b0748f7d186442ac1e5957f1e500bac23f59c Mon Sep 17 00:00:00 2001
>From: Alex Buckley <alexbuckley@catalyst.net.nz>
>Date: Wed, 10 Aug 2022 00:54:20 +0000
>Subject: [PATCH] Bug 21159: Update item location on checkout
>
>This patchset shifts the logic for changing the item location on checkin
>(controlled by the UpdateItemLocationOnCheckin system preference) to a
>new subroutine in Koha/Items.pm
>
>That subroutine logic is shared with the UpdateItemLocationOnCheckout
>system preference.
>
>Test plan:
>1. Apply patches, update databases and restart services
>
>2. Set the following system preferences:
>- UpdateItemLocationOnCheckin:
>FIC: PROC
>
>- UpdateItemLocationOnCheckout:
>PROC: FIC
>
>3. Checkout an an item with items.location = 'PROC'. Observe it's
>location is changed to 'FIC'
>
>4. Return the item. Observe it's location is changed to 'PROC'
>
>5. Change UpdateItemLocationOnCheckout to:
>PROC: _BLANK_
>
>6. Issue the item with items.location = 'PROC' and confirm it's location
>is blanked on checkout
>
>7. Issue and return an item with a different location e.g. 'CART'.
>Observe the location does not change on issue or return.
>
>8. Run unit tests
>sudo koha-shell kohadev
>prove t/db_dependent/Circulation/issue.t
>
>Sponsored-by: Toi Ohomai Institute of Technology, New Zealand
>---
> C4/Circulation.pm                  | 88 ++----------------------------
> Koha/Item.pm                       | 84 ++++++++++++++++++++++++++++
> t/db_dependent/Circulation/issue.t | 79 +++++++++++++++++++++------
> 3 files changed, 152 insertions(+), 99 deletions(-)
>
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index 5a6e0366850..56dff268bdf 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -1689,6 +1689,9 @@ sub AddIssue {
>                 CartToShelf( $item_object->itemnumber );
>             }
> 
>+            # Update item location
>+            $item_object->update_item_location( 'checkout' );
>+
>             if ( C4::Context->preference('UpdateTotalIssuesOnCirc') ) {
>                 UpdateTotalIssues( $item_object->biblionumber, 1, undef, { skip_holds_queue => 1 } );
>             }
>@@ -2143,89 +2146,8 @@ sub AddReturn {
>     my $borrowernumber = $patron ? $patron->borrowernumber : undef;    # we don't know if we had a borrower or not
>     my $patron_unblessed = $patron ? $patron->unblessed : {};
> 
>-    my $update_loc_rules = C4::Context->yaml_preference('UpdateItemLocationOnCheckin');
>-    if ($update_loc_rules) {
>-        if ( defined $update_loc_rules->{_ALL_} ) {
>-            if ( $update_loc_rules->{_ALL_} eq '_PERM_' ) {
>-                $update_loc_rules->{_ALL_} = $item->permanent_location;
>-            }
>-            if ( $update_loc_rules->{_ALL_} eq '_BLANK_' ) {
>-                $update_loc_rules->{_ALL_} = '';
>-            }
>-            if (
>-                (
>-                    defined $item->location
>-                    && $item->location ne $update_loc_rules->{_ALL_}
>-                )
>-                || ( !defined $item->location
>-                    && $update_loc_rules->{_ALL_} ne "" )
>-              )
>-            {
>-                $messages->{'ItemLocationUpdated'} =
>-                  { from => $item->location, to => $update_loc_rules->{_ALL_} };
>-                $item->location( $update_loc_rules->{_ALL_} )->store(
>-                    {
>-                        log_action        => 0,
>-                        skip_record_index => 1,
>-                        skip_holds_queue  => 1
>-                    }
>-                );
>-            }
>-        }
>-        else {
>-            foreach my $key ( keys %$update_loc_rules ) {
>-                if ( $update_loc_rules->{$key} eq '_PERM_' ) {
>-                    $update_loc_rules->{$key} = $item->permanent_location;
>-                }
>-                elsif ( $update_loc_rules->{$key} eq '_BLANK_' ) {
>-                    $update_loc_rules->{$key} = '';
>-                }
>-                if (
>-                    (
>-                           defined $item->location
>-                        && $item->location eq $key
>-                        && $item->location ne $update_loc_rules->{$key}
>-                    )
>-                    || (   $key eq '_BLANK_'
>-                        && ( !defined $item->location || $item->location eq '' )
>-                        && $update_loc_rules->{$key} ne '' )
>-                  )
>-                {
>-                    $messages->{'ItemLocationUpdated'} = {
>-                        from => $item->location,
>-                        to   => $update_loc_rules->{$key}
>-                    };
>-                    $item->location( $update_loc_rules->{$key} )->store(
>-                        {
>-                            log_action        => 0,
>-                            skip_record_index => 1,
>-                            skip_holds_queue  => 1
>-                        }
>-                    );
>-                    last;
>-                }
>-            }
>-        }
>-    }
>-
>-    my $yaml = C4::Context->preference('UpdateNotForLoanStatusOnCheckin');
>-    if ($yaml) {
>-        $yaml = "$yaml\n\n";  # YAML is anal on ending \n. Surplus does not hurt
>-        my $rules;
>-        eval { $rules = YAML::XS::Load(Encode::encode_utf8($yaml)); };
>-        if ($@) {
>-            warn "Unable to parse UpdateNotForLoanStatusOnCheckin syspref : $@";
>-        }
>-        else {
>-            foreach my $key ( keys %$rules ) {
>-                if ( $item->notforloan eq $key ) {
>-                    $messages->{'NotForLoanStatusUpdated'} = { from => $item->notforloan, to => $rules->{$key} };
>-                    $item->notforloan($rules->{$key})->store({ log_action => 0, skip_record_index => 1, skip_holds_queue => 1 }) unless $rules->{$key} eq 'ONLYMESSAGE';
>-                    last;
>-                }
>-            }
>-        }
>-    }
>+    # Update item location
>+    $messages = $item->update_item_location( 'checkin' );
> 
>     # check if the return is allowed at this branch
>     my ($returnallowed, $message) = CanBookBeReturned($item->unblessed, $branch);
>diff --git a/Koha/Item.pm b/Koha/Item.pm
>index 96b5fa8b0f0..d4d81eb43d7 100644
>--- a/Koha/Item.pm
>+++ b/Koha/Item.pm
>@@ -2128,6 +2128,90 @@ sub strings_map {
>     return $strings;
> }
> 
>+=head3 update_item_location
>+
>+    $item->update_item_location( $action );
>+
>+Update the item location on checkin or checkout.
>+
>+=cut
>+
>+sub update_item_location {
>+    my ( $self, $action ) = @_;
>+
>+    my ($update_loc_rules, $messages);
>+    if ( $action eq 'checkin' ) {
>+        $update_loc_rules = C4::Context->yaml_preference('UpdateItemLocationOnCheckin');
>+    } else {
>+        $update_loc_rules = C4::Context->yaml_preference('UpdateItemLocationOnCheckout');
>+    }
>+
>+    if ($update_loc_rules) {
>+        if ( defined $update_loc_rules->{_ALL_} ) {
>+            if ( $update_loc_rules->{_ALL_} eq '_PERM_' ) {
>+                $update_loc_rules->{_ALL_} = $self->permanent_location;
>+            }
>+            if ( $update_loc_rules->{_ALL_} eq '_BLANK_' ) {
>+                $update_loc_rules->{_ALL_} = '';
>+            }
>+            if (
>+                (
>+                    defined $self->location
>+                    && $self->location ne $update_loc_rules->{_ALL_}
>+                )
>+                || ( !defined $self->location
>+                    && $update_loc_rules->{_ALL_} ne "" )
>+              )
>+            {
>+                $messages->{'ItemLocationUpdated'} =
>+                  { from => $self->location, to => $update_loc_rules->{_ALL_} };
>+                $self->location( $update_loc_rules->{_ALL_} )->store(
>+                    {
>+                        log_action        => 0,
>+                        skip_record_index => 1,
>+                        skip_holds_queue  => 1
>+                    }
>+                );
>+            }
>+        }
>+        else {
>+            foreach my $key ( keys %$update_loc_rules ) {
>+                if ( $update_loc_rules->{$key} eq '_PERM_' ) {
>+                    $update_loc_rules->{$key} = $self->permanent_location;
>+                }
>+                elsif ( $update_loc_rules->{$key} eq '_BLANK_' ) {
>+                    $update_loc_rules->{$key} = '';
>+                }
>+                if (
>+                    (
>+                           defined $self->location
>+                        && $self->location eq $key
>+                        && $self->location ne $update_loc_rules->{$key}
>+                    )
>+                    || (   $key eq '_BLANK_'
>+                        && ( !defined $self->location || $self->location eq '' )
>+                        && $update_loc_rules->{$key} ne '' )
>+                  )
>+                {
>+                    $messages->{'ItemLocationUpdated'} = {
>+                        from => $self->location,
>+                        to   => $update_loc_rules->{$key}
>+                    };
>+                    $self->location( $update_loc_rules->{$key} )->store(
>+                        {
>+                            log_action        => 0,
>+                            skip_record_index => 1,
>+                            skip_holds_queue  => 1
>+                        }
>+                    );
>+                    last;
>+                }
>+            }
>+        }
>+    }
>+    return $messages;
>+}
>+
> =head3 _type
> 
> =cut
>diff --git a/t/db_dependent/Circulation/issue.t b/t/db_dependent/Circulation/issue.t
>index 8315a118665..7cac996c3ba 100755
>--- a/t/db_dependent/Circulation/issue.t
>+++ b/t/db_dependent/Circulation/issue.t
>@@ -17,7 +17,7 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 52;
>+use Test::More tests => 62;
> use DateTime::Duration;
> 
> use t::lib::Mocks;
>@@ -586,34 +586,81 @@ AddRenewal(
> my ( $unseen_reset ) = ( C4::Circulation::GetRenewCount( $unseen_patron->borrowernumber, $unseen_item->itemnumber ) )[3];
> is( $unseen_reset, 0, 'seen renewal resets the unseen count' );
> 
>+# Bug 21159 - Update item shelving location on checkout
> my $itemnumber4 = Koha::Item->new(
>     {
>         biblionumber   => $biblionumber,
>         barcode        => 'barcode_6',
>-        itemcallnumber => 'callnumber6',
>+        itemcallnumber => 'callnumber3',
>         homebranch     => $branchcode_1,
>         holdingbranch  => $branchcode_1,
>-        notforloan     => -1,
>-        itype          => $itemtype,
>-        location       => 'loc1'
>+        location       => 'FIC',
>+        itype          => $itemtype
>     },
> )->store->itemnumber;
>+t::lib::Mocks::mock_preference( 'UpdateItemLocationOnCheckout', q{} );
>+t::lib::Mocks::mock_preference( 'UpdateItemLocationOnCheckin', q{} );
>+AddIssue( $borrower_1, 'barcode_6');
>+my $item4 = Koha::Items->find( $itemnumber4 );
>+ok( $item4->location eq 'FIC', 'UpdateItemLocationOnCheckout does not modify value when not enabled' );
> 
>-t::lib::Mocks::mock_preference( 'UpdateNotForLoanStatusOnCheckout', q{} );
>-AddIssue( $borrower_2, 'barcode_6', dt_from_string );
>-$item = Koha::Items->find( $itemnumber4 );
>-ok( $item->notforloan eq -1, 'UpdateNotForLoanStatusOnCheckout does not modify value when not enabled' );
>+#---
>+AddReturn( 'barcode_6', $branchcode_1 );
>+t::lib::Mocks::mock_preference( 'UpdateItemLocationOnCheckout', 'FIC: GEN' );
>+$log_count_before = $schema->resultset('ActionLog')->search({module => 'CATALOGUING'})->count();
>+AddIssue( $borrower_1, 'barcode_6' );
>+$item4 = Koha::Items->find( $itemnumber4 );
>+is( $item4->location, 'GEN', q{UpdateItemLocationOnCheckout updates location value from 'FIC' to 'GEN' with setting "FIC: GEN"} );
>+is( $item4->permanent_location, 'GEN', q{UpdateItemLocationOnCheckout updates permanent_location value from 'FIC' to 'GEN' with setting "FIC: GEN"} );
>+$log_count_after = $schema->resultset('ActionLog')->search({module => 'CATALOGUING'})->count();
>+is($log_count_before, $log_count_after, "Change from UpdateNotForLoanStatusOnCheckout is not logged");
>+AddReturn( 'barcode_6', $branchcode_1 );
>+AddIssue( $borrower_1, 'barcode_6' );
>+$item4 = Koha::Items->find( $itemnumber4 );
>+ok( $item4->location eq 'GEN', q{UpdateItemLocationOnCheckout does not update location value from 'GEN' with setting "FIC: GEN"} );
> 
>-t::lib::Mocks::mock_preference( 'UpdateNotForLoanStatusOnCheckout', '-1: 0' );
>+t::lib::Mocks::mock_preference( 'UpdateItemLocationOnCheckout', '_ALL_: CART' );
> AddReturn( 'barcode_6', $branchcode_1 );
>-my $test = AddIssue( $borrower_2, 'barcode_6', dt_from_string );
>-$item = Koha::Items->find( $itemnumber4 );
>-ok( $item->notforloan eq 0, q{UpdateNotForLoanStatusOnCheckout updates notforloan value from -1 to 0 with setting "-1: 0"} );
>+AddIssue( $borrower_1, 'barcode_6' );
>+$item4 = Koha::Items->find( $itemnumber4 );
>+ok( $item4->location eq 'CART', q{UpdateItemLocationOnCheckout updates location value from 'GEN' with setting "_ALL_: CART"} );
>+ok( $item4->permanent_location eq 'GEN', q{UpdateItemLocationOnCheckout does not update permanent_location value from 'GEN' with setting "_ALL_: CART"} );
> 
>-AddIssue( $borrower_2, 'barcode_6', dt_from_string );
>+$item4->location('GEN')->store;
>+t::lib::Mocks::mock_preference( 'UpdateItemLocationOnCheckout', "GEN: _BLANK_\n_BLANK_: PROC\nPROC: _PERM_" );
>+AddReturn( 'barcode_6', $branchcode_1 );
>+AddIssue( $borrower_1, 'barcode_6' );
>+$item4 = Koha::Items->find( $itemnumber4 );
>+ok( $item4->location eq '', q{UpdateItemLocationOnCheckout updates location value from 'GEN' to '' with setting "GEN: _BLANK_"} );
> AddReturn( 'barcode_6', $branchcode_1 );
>-$item = Koha::Items->find( $itemnumber4 );
>-ok( $item->notforloan eq 0, q{UpdateNotForLoanStatusOnCheckout does not update notforloan value from 0 with setting "-1: 0"} );
>+AddIssue( $borrower_1, 'barcode_6' );
>+$item4 = Koha::Items->find( $itemnumber4 );
>+ok( $item4->location eq 'PROC' , q{UpdateItemLocationOnCheckout updates location value from '' to 'PROC' with setting "_BLANK_: PROC"} );
>+ok( $item4->permanent_location eq '' , q{UpdateItemLocationOnCheckout does not update permanent_location value from '' to 'PROC' with setting "_BLANK_: PROC"} );
>+AddReturn( 'barcode_6', $branchcode_1 );
>+AddIssue( $borrower_1, 'barcode_6' );
>+$item4 = Koha::Items->find( $itemnumber4 );
>+ok( $item4->location eq '' , q{UpdateItemLocationOnCheckout updates location value from 'PROC' to '' with setting "PROC: _PERM_" } );
>+ok( $item4->permanent_location eq '' , q{UpdateItemLocationOnCheckout does not update permanent_location from '' with setting "PROC: _PERM_" } );
>+
>+# Bug 28472
>+my $itemnumber5 = Koha::Item->new(
>+    {
>+        biblionumber   => $biblionumber,
>+        barcode        => 'barcode_7',
>+        itemcallnumber => 'callnumber5',
>+        homebranch     => $branchcode_1,
>+        holdingbranch  => $branchcode_1,
>+        location       => undef,
>+        itype          => $itemtype
>+    }
>+)->store->itemnumber;
>+
>+t::lib::Mocks::mock_preference( 'UpdateItemLocationOnCheckout', '_ALL_: CART' );
>+AddIssue( $borrower_1, 'barcode_7' );
>+my $item5 = Koha::Items->find( $itemnumber5 );
>+is( $item5->location, 'CART', q{UpdateItemLocationOnCheckout updates location value from NULL (i.e. the item has no shelving location set) to 'CART' with setting "_ALL_: CART"} );
>+
> 
> #End transaction
> $schema->storage->txn_rollback;
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 21159: 77487 | 77549 | 77594 | 77775 | 77859 | 77869 | 77870 | 77871 | 80892 | 80893 | 80894 | 80895 | 87037 | 87038 | 87041 | 87693 | 87694 | 87695 | 139097 | 139098 | 139099 | 139103 | 139104 | 139105 | 139106 | 139107 | 139108 | 139109 | 139220 | 146602 | 146603 | 146604 | 146605 | 146606 | 146607 | 146608 | 154147 | 154148 | 154149 | 154150 | 154151 | 154152 | 155284 | 155285 | 155771 | 155772 | 155773 | 155774 | 158146 | 158147 | 158149 | 158150 | 158169 | 158170 | 158171 | 158172 | 158173 | 158521 | 158522