From 0a8fa9d10b40f7b9387452b7a6bd5b6b4c3525aa 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 --- 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 f987fbf81fa..aecf96af1f0 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 7bbc87b95b3..9ce848d9f04 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 758de9b2df4..870f767e8b6 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 115be2ab58b..2a3fdc5b1d2 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.30.1