Bug 27545: Use NewItemsDefaultLocation from places where an item is created

The syspref NewItemsDefaultLocation is used to set a default value for item's location.

But it seems that there are some weirdness in the behaviour:
1. It's only used from additem. It seems that it should be used from acq and serial modules as well. And maybe for the items import too.
2. It set the location even if another one has been picked from the UI
=> We UI must preselect the syspref's value, but the controller must pick what has been selected on the UI

This patch is adding the default to the UI and extend the use of the
pref to other areas.

Test plan:
Set a value to NewItemsDefaultLocation
Catalogue a new item and confirm that the syspref's value is picked to
selected the default value on the add item form
Same behaviour should apply to the acquisition and serial modules
When importing items, the default location must be used if the imported
items did not have a location defined.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

C4/Items.pm

@@ -1602,6 +1602,12 @@ sub PrepareItemrecordDisplay {
     # its contents. See also GetMarcStructure.
     my $tagslib = GetMarcStructure( 1, $frameworkcode, { unsafe => 1 } );
 
+    # Pick the default location from NewItemsDefaultLocation
+    if ( C4::Context->preference('NewItemsDefaultLocation') ) {
+        $defaultvalues //= {};
+        $defaultvalues->{location} //= C4::Context->preference('NewItemsDefaultLocation');
+    }
+
     # return nothing if we don't have found an existing framework.
     return q{} unless $tagslib;
     my $itemrecord;

Koha/Item.pm

@@ -94,9 +94,17 @@ sub store {
     my $action = 'create';
 
     unless ( $self->in_storage ) { #AddItem
+
         unless ( $self->permanent_location ) {
             $self->permanent_location($self->location);
         }
+
+        my $default_location = C4::Context->preference('NewItemsDefaultLocation');
+        if ( $default_location ) {
+            $self->permanent_location( $self->location ); # FIXME To confirm - even if passed?
+            $self->location($default_location);
+        }
+
         unless ( $self->replacementpricedate ) {
             $self->replacementpricedate($today);
         }

cataloguing/additem.pl

@@ -74,20 +74,6 @@ sub get_item_from_barcode {
     return($result);
 }
 
-sub set_item_default_location {
-    my $itemnumber = shift;
-    my $item       = Koha::Items->find($itemnumber);
-    if ( C4::Context->preference('NewItemsDefaultLocation') ) {
-        $item->permanent_location($item->location);
-        $item->location(C4::Context->preference('NewItemsDefaultLocation'));
-    }
-    else {
-        # It seems that we are dealing with that in too many places
-        $item->permanent_location($item->location) unless defined $item->permanent_location;
-    }
-    $item->store;
-}
-
 # NOTE: This code is subject to change in the future with the implemenation of ajax based autobarcode code
 # NOTE: 'incremental' is the ONLY autoBarcode option available to those not using javascript
 sub _increment_barcode {
@@ -166,6 +152,11 @@ sub generate_subfield_form {
             }
         }
 
+        my $default_location = C4::Context->preference('NewItemsDefaultLocation');
+        if ( !$value && $subfieldlib->{kohafield} eq 'items.location' && $default_location ) {
+            $value = $default_location;
+        }
+
         if ($frameworkcode eq 'FA' && $subfieldlib->{kohafield} eq 'items.barcode' && !$value){
 	    my $input = CGI->new;
 	    $value = $input->param('barcode');
@@ -528,7 +519,6 @@ if ($op eq "additem") {
         # if barcode exists, don't create, but report The problem.
         unless ($exist_itemnumber) {
             my ( $oldbiblionumber, $oldbibnum, $oldbibitemnum ) = AddItemFromMarc( $record, $biblionumber );
-            set_item_default_location($oldbibitemnum);
 
             # Pushing the last created item cookie back
             if ($prefillitem && defined $record) {
@@ -628,7 +618,6 @@ if ($op eq "additem") {
         if (!$exist_itemnumber) {
             my ( $oldbiblionumber, $oldbibnum, $oldbibitemnum ) =
                 AddItemFromMarc( $record, $biblionumber, { skip_record_index => 1 } );
-            set_item_default_location($oldbibitemnum);
 
             # We count the item only if it was really added
             # That way, all items are added, even if there was some already existing barcodes

t/db_dependent/Koha/Items.t

@@ -89,10 +89,69 @@ subtest 'store' => sub {
         $biblio->biblioitem->itemtype,
         'items.itype must have been set to biblioitem.itemtype is not given'
     );
-    is( $item->permanent_location, $item->location,
-        'permanent_location must have been set to location if not given' );
     $item->delete;
 
+    subtest 'permanent_location' => 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, $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;
+        }
+
+    };
+
     subtest '*_on updates' => sub {
         plan tests => 9;