From d776fa7b271644790219f56eb1f53e4be1ad5a12 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 25 Jan 2021 14:04:31 +0100 Subject: [PATCH] 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 Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart --- C4/Items.pm | 6 ++++ Koha/Item.pm | 8 +++++ cataloguing/additem.pl | 21 +++---------- t/db_dependent/Koha/Items.t | 63 +++++++++++++++++++++++++++++++++++-- 4 files changed, 80 insertions(+), 18 deletions(-) diff --git a/C4/Items.pm b/C4/Items.pm index f987fbf81f..aecf96af1f 100644 --- a/C4/Items.pm +++ b/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; diff --git a/Koha/Item.pm b/Koha/Item.pm index 7bbc87b95b..9ce848d9f0 100644 --- a/Koha/Item.pm +++ b/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); } diff --git a/cataloguing/additem.pl b/cataloguing/additem.pl index 758de9b2df..870f767e8b 100755 --- a/cataloguing/additem.pl +++ b/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 diff --git a/t/db_dependent/Koha/Items.t b/t/db_dependent/Koha/Items.t index 115be2ab58..2a3fdc5b1d 100755 --- a/t/db_dependent/Koha/Items.t +++ b/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; -- 2.39.5