Attachment 117615 Details for Bug 27545 – Bug 27545: Use NewItemsDefaultLocation from places where an item is created

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

Bug-27545-Use-NewItemsDefaultLocation-from-places-.patch (text/plain), 8.25 KB, created by Tomás Cohen Arazi (tcohen) on 2021-03-03 15:23:11 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomás Cohen Arazi (tcohen)

Created: 2021-03-03 15:23:11 UTC

Size: 8.25 KB

patch

obsolete

>From 0a8fa9d10b40f7b9387452b7a6bd5b6b4c3525aa Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>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 <tomascohen@theke.io>
>---
> 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

Actions: View | Diff | Splinter Review

Attachments on bug 27545: 115775 | 115776 | 117615 | 117616 | 117804 | 117805