From e61bcc270e8bc9a13939af60df45a1db53e51530 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Wed, 11 Nov 2020 13:26:58 +0000
Subject: [PATCH] Bug 26963: (QA follow-up) Fix up tests and cover case of
 undefined ccode

While this technically shouldn't happen, if a library creates itemtype limits, then flips the pref, those rules are still in the db until a ccode rule is saved.

To be extra safe, when checking for rules of one type, we should make sure the other type is undef - i.e. if looking for ccode rules, we should confirm the itype is undef for those rules

Additionally, we shouldn't set the barcode now that we are not deleting all items, so we use copynumber for our item identification field as it doesn't need to be unique in the DB

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
---
 Koha/Item.pm               | 10 ++++++++--
 t/db_dependent/Koha/Item.t | 24 ++++++++++++++++++------
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/Koha/Item.pm b/Koha/Item.pm
index db87a0aaf7..10d4eb2c48 100644
--- a/Koha/Item.pm
+++ b/Koha/Item.pm
@@ -599,11 +599,17 @@ sub pickup_locations {
     ) unless C4::Context->preference('UseBranchTransferLimits');
 
     my $limittype = C4::Context->preference('BranchTransferLimitsType');
-    my $limiter = $limittype eq 'itemtype' ? $self->effective_itemtype : $self->ccode;
+    my ($ccode, $itype) = (undef, undef);
+    if( $limittype eq 'ccode' ){
+        $ccode = $self->ccode;
+    } else {
+        $itype = $self->itype;
+    }
     my $limits = Koha::Item::Transfer::Limits->search(
         {
             fromBranch => $self->holdingbranch,
-            $limittype => $limiter
+            ccode      => $ccode,
+            itemtype   => $itype,
         },
         { columns => ['toBranch'] }
     );
diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t
index 401254404d..b983cad934 100755
--- a/t/db_dependent/Koha/Item.t
+++ b/t/db_dependent/Koha/Item.t
@@ -178,7 +178,7 @@ subtest 'pickup_locations' => sub {
         {
             homebranch    => $library1->branchcode,
             holdingbranch => $library2->branchcode,
-            barcode       => '1',
+            copynumber    => 1,
             ccode         => 'Gollum'
         }
     )->store;
@@ -187,7 +187,7 @@ subtest 'pickup_locations' => sub {
         {
             homebranch    => $library3->branchcode,
             holdingbranch => $library4->branchcode,
-            barcode       => '3',
+            copynumber    => 3,
             itype         => $item1->itype,
         }
     )->store;
@@ -208,7 +208,7 @@ subtest 'pickup_locations' => sub {
     my $patron4 = $builder->build_object( { class => 'Koha::Patrons', value => { branchcode => $library4->branchcode, firstname => '4' } } );
 
     my $all_count = Koha::Libraries->search({ pickup_location => 1})->count();
-    plan tests => ($all_count +1) * 7 + 31 + 60;
+    plan tests => ($all_count +1) * 7 + 31 + 61;
 
     my $results = {
         "1-1-1-any"           => $all_count,
@@ -295,13 +295,13 @@ subtest 'pickup_locations' => sub {
         }
         ok(
             scalar(@pl) == $results->{
-                    $item->barcode . '-'
+                    $item->copynumber . '-'
                   . $patron->firstname . '-'
                   . $ha . '-'
                   . $hfp
             },
             'item'
-              . $item->barcode
+              . $item->copynumber
               . ', patron'
               . $patron->firstname
               . ', holdallowed: '
@@ -310,7 +310,7 @@ subtest 'pickup_locations' => sub {
               . $hfp
               . ' should return '
               . $results->{
-                    $item->barcode . '-'
+                    $item->copynumber . '-'
                   . $patron->firstname . '-'
                   . $ha . '-'
                   . $hfp
@@ -334,6 +334,15 @@ subtest 'pickup_locations' => sub {
     }
 
     # Now test that branchtransferlimits will further filter the pickup locations
+
+    my $item_no_ccode = $builder->build_sample_item(
+        {
+            homebranch    => $library1->branchcode,
+            holdingbranch => $library2->branchcode,
+            itype         => $item1->itype,
+        }
+    )->store;
+
     t::lib::Mocks::mock_preference('UseBranchTransferLimits', 1);
     t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'itemtype');
     Koha::CirculationRules->set_rules(
@@ -381,6 +390,9 @@ subtest 'pickup_locations' => sub {
     $pickup_locations = $item1->pickup_locations( { patron => $patron1 } )->as_list;
     is( scalar @$pickup_locations, $all_count, "With no transfer limits of type ccode we get back the libraries that are pickup locations");
 
+    $pickup_locations = $item_no_ccode->pickup_locations( { patron => $patron1 } )->as_list;
+    is( scalar @$pickup_locations, $all_count, "With no transfer limits of type ccode and an item with no ccode we get back the libraries that are pickup locations");
+
     $builder->build_object(
         {
             class => 'Koha::Item::Transfer::Limits',
-- 
2.39.5