Attachment 63417 Details for Bug 18400 – Bug 18400: [QA Follow-up] Move sort outside the loop

[patch] Bug 18400: [QA Follow-up] Move sort outside the loop

Bug-18400-QA-Follow-up-Move-sort-outside-the-loop.patch (text/plain), 3.50 KB, created by Marcel de Rooy on 2017-05-12 09:04:59 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2017-05-12 09:04:59 UTC

Size: 3.50 KB

patch

obsolete

>From 7c51162957d6268174a5925858cd07ed78eb1123 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
>Content-Type: text/plain; charset=utf-8
>
>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>
>---
> 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 3d034ab..7b10ac3 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}};
>-- 
>2.1.4

Actions: View | Diff | Splinter Review

Attachments on bug 18400: 61976 | 62224 | 62232 | 63416 | 63417 | 64718