From e10a415ef92f69b2645866720e93fbbdd3c6672a Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Date: Fri, 12 May 2017 10:59:11 +0200
Subject: [PATCH] Bug 18400: [QA Follow-up] Move sort outside the loop

GetItemTypesCategorized can return descriptions that are still undef since
Authorized values does not enforce a description in lib and lib_opac.
When I add one ITEMTYPECAT auth value without descriptions, I can still
generate the string comparison warnings on the itemtypes sort.
In order to prevent the warning, we should add an empty string in the
assignment on line 229.

We do not need to copy the itemtypes hash if we move the sort outside the
@advanced_search_types foreach. There is no need to sort it more than once.
Note that I did not see any reasons btw for corruption of the structure
inside this loop.

Note: If we use ITEMTYPECAT without descriptions, we should probably leave
them out. No need to show a checkbox without description on Advanced Search,
but I would recommend to solve that on its own report. The whole ITEMTYPECAT
functionality has imo not been designed properly.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
---
 opac/opac-search.pl | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/opac/opac-search.pl b/opac/opac-search.pl
index 3d034abef6..7b10ac3bef 100755
--- a/opac/opac-search.pl
+++ b/opac/opac-search.pl
@@ -224,13 +224,11 @@ $template->param(search_languages_loop => $languages_limit_loop,);
 my $itemtypes = GetItemTypesCategorized;
 # add translated_description to itemtypes
 foreach my $itemtype ( keys %{$itemtypes} ) {
-    # Itemtypes search categories don't have (yet) translated descriptions, they are auth values
+    # Itemtypes search categories don't have (yet) translated descriptions, they are auth values (and could still have no descriptions too BZ 18400)
     my $translated_description = getitemtypeinfo( $itemtype, 'opac' )->{translated_description};
-    $itemtypes->{$itemtype}->{translated_description} =
-            ( $translated_description ) ? $translated_description : $itemtypes->{$itemtype}->{description};
+    $itemtypes->{$itemtype}->{translated_description} = $translated_description || $itemtypes->{$itemtype}->{description} || q{};
 }
-my $itemtypes_copy = { %$itemtypes }; #Sometime itemtypes can be corrupted in advanced_srch_type loop
-                                      #Making a copy ensure it is clean
+
 # the index parameter is different for item-level itemtypes
 my $itype_or_itemtype = (C4::Context->preference("item-level_itypes"))?'itype':'itemtype';
 my @advancedsearchesloop;
@@ -250,13 +248,13 @@ if ( $yaml =~ /\S/ ) {
     }
 }
 
+my @sorted_itemtypes = sort { $itemtypes->{$a}->{translated_description} cmp $itemtypes->{$b}->{translated_description} } keys %$itemtypes;
 foreach my $advanced_srch_type (@advanced_search_types) {
     $advanced_srch_type =~ s/^\s*//;
     $advanced_srch_type =~ s/\s*$//;
    if ($advanced_srch_type eq 'itemtypes') {
    # itemtype is a special case, since it's not defined in authorized values
         my @itypesloop;
-        my @sorted_itemtypes = ( sort {$itemtypes->{$a}->{translated_description} cmp $itemtypes->{$b}->{translated_description} } keys %$itemtypes_copy );
         foreach my $thisitemtype ( @sorted_itemtypes ) {
             next if $hidingrules->{itype} && any { $_ eq $thisitemtype } @{$hidingrules->{itype}};
             next if $hidingrules->{itemtype} && any { $_ eq $thisitemtype } @{$hidingrules->{itemtype}};