From 77cc2ebee8fb2ac1800dcfc1bf5c7991e5ab6115 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Wed, 1 May 2019 10:57:01 +0000
Subject: [PATCH] Bug 21946: (follow-up) Address QA concerns

1 - Correct hashref for searching
2 - Correct undef for all (this only simplifies later logic so didn't affect outcomes)
3 - remove duplicated query
4 - Use get_column instead of loops
5 - Fix typo
6 - Whitespace fixes removed from earlier patches

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
---
 C4/Circulation.pm                                  | 32 ++++++++--------------
 .../prog/en/modules/admin/itemtypes.tt             |  2 +-
 2 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index b499a10aee..ab52ea6e91 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -388,7 +388,7 @@ sub TooMany {
     my ($type_object, $parent_type, $parent_maxissueqty_rule);
     $type_object = Koha::ItemTypes->find( $type );
     $parent_type = $type_object->parent_type if $type_object;
-    my $child_types = Koha::ItemTypes->search( parent_type => $type );
+    my $child_types = Koha::ItemTypes->search({ parent_type => $type });
     # Find any children if we are a parent_type;
 
     # given branch, patron category, and item type, determine
@@ -403,7 +403,7 @@ sub TooMany {
         }
     ) if $parent_type;
     # If the parent rule is for default type we discount it
-    $parent_maxissueqty_rule = undef if $parent_maxissueqty_rule && $parent_maxissueqty_rule->itemtype eq '*';
+    $parent_maxissueqty_rule = undef if $parent_maxissueqty_rule && !defined $parent_maxissueqty_rule->itemtype ;
 
     my $maxissueqty_rule = Koha::CirculationRules->get_effective_rule(
         {
@@ -449,21 +449,15 @@ sub TooMany {
             # matching rule has the default item type, so count only
             # those existing loans that don't fall under a more
             # specific rule
+            my $issuing_itemtypes_query  = " SELECT itemtype FROM issuingrules
+                                          WHERE branchcode = ?
+                                          AND (categorycode = ? OR categorycode = ?)
+                                          AND itemtype <> '*' ";
             if (C4::Context->preference('item-level_itypes')) {
-                $count_query .= " WHERE items.itype NOT IN (
-                                    SELECT itemtype FROM issuingrules
-                                    WHERE branchcode = ?
-                                    AND   (categorycode = ? OR categorycode = ?)
-                                    AND   itemtype <> '*'
-                                  ) ";
+                $count_query .= " WHERE items.itype NOT IN ( $issuing_itemtypes_query )";
             } else { 
                 $count_query .= " JOIN  biblioitems USING (biblionumber) 
-                                  WHERE biblioitems.itemtype NOT IN (
-                                    SELECT itemtype FROM issuingrules
-                                    WHERE branchcode = ?
-                                    AND   (categorycode = ? OR categorycode = ?)
-                                    AND   itemtype <> '*'
-                                  ) ";
+                                  WHERE biblioitems.itemtype NOT IN ( $issuing_itemtypes_query )";
             }
             push @bind_params, $maxissueqty_rule->branchcode;
             push @bind_params, $maxissueqty_rule->categorycode;
@@ -473,16 +467,12 @@ sub TooMany {
             if ( $parent_maxissueqty_rule ) {
             # if we have a parent item type then we count loans of the
             # specific item type or its siblings or parent
-                push @types, $parent_type;
                 my $children = Koha::ItemTypes->search({ parent_type => $parent_type });
-                while ( my $child = $children->next ){
-                    push @types, $child->itemtype;
-                }
+                @types = $children->get_column('itemtype');
+                push @types, $parent_type;
             } elsif ( $child_types ) {
             # If we are a parent type, we need to count all child types and our own type
-                while ( my $child = $child_types->next ){
-                    push @types, $child->itemtype;
-                }
+                @types = $child_types->get_column('itemtype');
                 push @types, $type; # And don't forget to count our own types
             } else { push @types, $type; } # Otherwise only count the specific itemtype
             my $types_param = ( '?,' ) x @types;
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt
index 9e9a97b9ef..0d5732d54c 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt
@@ -96,7 +96,7 @@ Item types administration
     [% IF itemtype %]
         <h3>Modify item type</h3>
     [% ELSE %]
-        ]<h3>Add item type</h3>
+        <h3>Add item type</h3>
     [% END %]
     <form action="/cgi-bin/koha/admin/itemtypes.pl" name="Aform" method="post" id="itemtypeentry">
         <input type="hidden" name="op" value="add_validate" />
-- 
2.11.0