From 24770ee572e99a2e7f396d70497672d0864dbb2d Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date: Mon, 25 Jan 2021 15:15:00 +0100
Subject: [PATCH] Bug 27545: Keep the location if passed

Change in behaviour here, but expect. We must keep the location value if
passed to store.
Same for the permanent_location
---
 Koha/Item.pm                |   5 +-
 t/db_dependent/Koha/Items.t | 162 ++++++++++++++++++++++++------------
 2 files changed, 112 insertions(+), 55 deletions(-)

diff --git a/Koha/Item.pm b/Koha/Item.pm
index 6d30f425b6..6e6c25877f 100644
--- a/Koha/Item.pm
+++ b/Koha/Item.pm
@@ -99,8 +99,9 @@ sub store {
         }
 
         my $default_location = C4::Context->preference('NewItemsDefaultLocation');
-        if ( $default_location ) {
-            $self->permanent_location( $self->location ); # FIXME To confirm - even if passed?
+        unless ( $self->location || !$default_location ) {
+            $self->permanent_location( $self->location || $default_location )
+              unless $self->permanent_location;
             $self->location($default_location);
         }
 
diff --git a/t/db_dependent/Koha/Items.t b/t/db_dependent/Koha/Items.t
index 9b6de654b2..7ba10feec6 100755
--- a/t/db_dependent/Koha/Items.t
+++ b/t/db_dependent/Koha/Items.t
@@ -92,63 +92,119 @@ subtest 'store' => sub {
     $item->delete;
 
     subtest 'permanent_location' => sub {
-        plan tests => 7;
+        plan tests => 2;
 
-        my $location = 'my_loc';
-        my $attributes = {
-            homebranch    => $library->{branchcode},
-            holdingbranch => $library->{branchcode},
-            biblionumber  => $biblio->biblionumber,
-            location      => $location,
+        subtest 'location passed to ->store' => sub {
+            plan tests => 7;
+
+            my $location = 'my_loc';
+            my $attributes = {
+                homebranch    => $library->{branchcode},
+                holdingbranch => $library->{branchcode},
+                biblionumber  => $biblio->biblionumber,
+                location      => $location,
+            };
+
+            {
+                # NewItemsDefaultLocation not set
+                t::lib::Mocks::mock_preference( 'NewItemsDefaultLocation', '' );
+
+                # Not passing permanent_location on creating the item
+                my $item = Koha::Item->new($attributes)->store->get_from_storage;
+                is( $item->location, $location,
+                    'location must have been set to location if given' );
+                is( $item->permanent_location, $item->location,
+                    'permanent_location must have been set to location if not given' );
+                $item->delete;
+
+                # Passing permanent_location on creating the item
+                $item = Koha::Item->new(
+                    { %$attributes, permanent_location => 'perm_loc' } )
+                  ->store->get_from_storage;
+                is( $item->permanent_location, 'perm_loc',
+                    'permanent_location must have been kept if given' );
+                $item->delete;
+            }
+
+            {
+                # NewItemsDefaultLocation set
+                my $default_location = 'default_location';
+                t::lib::Mocks::mock_preference( 'NewItemsDefaultLocation', $default_location );
+
+                # Not passing permanent_location on creating the item
+                my $item = Koha::Item->new($attributes)->store->get_from_storage;
+                is( $item->location, $location,
+                    'location must have been kept if given' );
+                is( $item->permanent_location, $location,
+                    'permanent_location must have been set to the location given' );
+                $item->delete;
+
+                # Passing permanent_location on creating the item
+                $item = Koha::Item->new(
+                    { %$attributes, permanent_location => 'perm_loc' } )
+                  ->store->get_from_storage;
+                is( $item->location, $location,
+                    'location must have been kept if given' );
+                is( $item->permanent_location, 'perm_loc',
+                    'permanent_location must have been kept if given' );
+                $item->delete;
+            }
         };
 
-        {
-            # NewItemsDefaultLocation not set
-            t::lib::Mocks::mock_preference( 'NewItemsDefaultLocation', '' );
-
-            # Not passing permanent_location on creating the item
-            my $item = Koha::Item->new($attributes)->store->get_from_storage;
-            is( $item->location, $location,
-                'location must have been set to location if given' );
-            is( $item->permanent_location, $item->location,
-                'permanent_location must have been set to location if not given' );
-            $item->delete;
-
-            # Passing permanent_location on creating the item
-            $item = Koha::Item->new(
-                { %$attributes, permanent_location => 'perm_loc' } )
-              ->store->get_from_storage;
-            is( $item->permanent_location, 'perm_loc',
-                'permanent_location must have been kept if given' );
-            $item->delete;
-        }
+        subtest 'location NOT passed to ->store' => sub {
+            plan tests => 7;
 
-        {
-            # NewItemsDefaultLocation set
-            my $default_location = 'default_location';
-            t::lib::Mocks::mock_preference( 'NewItemsDefaultLocation', $default_location );
-
-            # Not passing permanent_location on creating the item
-            my $item = Koha::Item->new($attributes)->store->get_from_storage;
-            is( $item->location, $default_location,
-                'location must have been set to default_location even if given' # FIXME this sounds wrong! Must be done in any cases?
-            );
-            is( $item->permanent_location, $location,
-                'permanent_location must have been set to the location given' );
-            $item->delete;
-
-            # Passing permanent_location on creating the item
-            $item = Koha::Item->new(
-                { %$attributes, permanent_location => 'perm_loc' } )
-              ->store->get_from_storage;
-            is( $item->location, $default_location,
-                'location must have been set to default_location even if given' # FIXME this sounds wrong! Must be done in any cases?
-            );
-            is( $item->permanent_location, $location,
-                'permanent_location must have been set to the location given'
-            );
-            $item->delete;
-        }
+            my $attributes = {
+                homebranch    => $library->{branchcode},
+                holdingbranch => $library->{branchcode},
+                biblionumber  => $biblio->biblionumber,
+            };
+
+            {
+                # NewItemsDefaultLocation not set
+                t::lib::Mocks::mock_preference( 'NewItemsDefaultLocation', '' );
+
+                # Not passing permanent_location on creating the item
+                my $item = Koha::Item->new($attributes)->store->get_from_storage;
+                is( $item->location, undef,
+                    'location not passed and no default, it is undef' );
+                is( $item->permanent_location, $item->location,
+                    'permanent_location must have been set to location if not given' );
+                $item->delete;
+
+                # Passing permanent_location on creating the item
+                $item = Koha::Item->new(
+                    { %$attributes, permanent_location => 'perm_loc' } )
+                  ->store->get_from_storage;
+                is( $item->permanent_location, 'perm_loc',
+                    'permanent_location must have been kept if given' );
+                $item->delete;
+            }
+
+            {
+                # NewItemsDefaultLocation set
+                my $default_location = 'default_location';
+                t::lib::Mocks::mock_preference( 'NewItemsDefaultLocation', $default_location );
+
+                # Not passing permanent_location on creating the item
+                my $item = Koha::Item->new($attributes)->store->get_from_storage;
+                is( $item->location, $default_location,
+                    'location must have been set to default location if not given' );
+                is( $item->permanent_location, $default_location,
+                    'permanent_location must have been set to the default location as well' );
+                $item->delete;
+
+                # Passing permanent_location on creating the item
+                $item = Koha::Item->new(
+                    { %$attributes, permanent_location => 'perm_loc' } )
+                  ->store->get_from_storage;
+                is( $item->location, $default_location,
+                    'location must have been set to default location if not given' );
+                is( $item->permanent_location, 'perm_loc',
+                    'permanent_location must have been kept if given' );
+                $item->delete;
+            }
+        };
 
     };
 
-- 
2.20.1