From ea280656b520e5a173c28224cd042d57290f0b3f Mon Sep 17 00:00:00 2001
From: Fridolyn SOMERS <fridolyn.somers@biblibre.com>
Date: Thu, 18 Jul 2013 13:59:07 +0200
Subject: [PATCH 1/2] Bug 7442 - Crash when selecting an authority with 200$x or 200$y
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

From a biblio record, if one wants to add a 600$a information, a pop-up
appears.  On this new window, on search terms typed and validated, a table
result is displayed, with a column "Get It!" allowing the selection of an
authority.  From here, different cases:

1) If we have a simple authority with 200$a and 200$b subfields, a link
   "choose" is displayed, working correctly.

2) If the authority has different occurences of 200$a/200$b, numeric links (1 2
   and so on) are displayed, one for each occurence.  In the example of my
   screenshot, the line with a "Paul, Korky -- Pauline, Korkette" summary
   possesses two links : "1" will add "Paul, Korky" whereas "2" will add
   "Pauline, Korkette" (couldn't come up with a better name ;)).

3) If the authority has 200$x or 200$y subfields defined, several links are
   also created, when it should not be the case.  In our example, "Niclausse,
   Paul -- Expositions" will create a link "1" for "Niclausse, Paul" and a link
   "2" for "Expositions".  Clicking on the 2nd link leads to the following
   error: Software error: Can't call method "subfields" on an undefined value
   at
   /home/asaurat/workspace/versions/community/authorities/blinddetail-biblio-search.pl
   line 86.  Only the cases 1 and 2 should be handled. The creation of links
   for subfields like 200$x or 200$y should be removed.

This problem is caused by the use of " -- " has separator of authorities with
several headings, but also in some heading between main part and subdivisions.
This patch corrects this by using an arry in authorities summary so that
presentation is computed in template. I've choosen to use the pipe separator
between authorities with several headings. This may be changed to be
configurable.

Test plan :

- Edit an authority type summary : for example subject (heading on 250) :
  summary "[250a][ -- 250x]"
- Create an authority A1 with one heading and a subdivision : for example a
  subject : 250$a "History" 250$x "20th century"
- Create an authority A2 with several headings. for example a subject : 250$a
  "History" 250$a "Legends"
- Rebuild Zebra queue
- Go to OPAC and click on "Authority search" and search on "History"
  => You will find A1 and A2 :
    History -- 20th century
    History | Legends
- Go to intranet autorities search and search on "History"
  => You will find A1 and A2 :
    History -- 20th century
    History | Legends
- Edit a record using this autorities type as thesaurus : for example on 606$a
- Click on thesaurus link and search on "History"
  => You will find A1 and A2 :
    History -- 20th century ; 0 times ; choose ; Edit authority
    History | Legends       ; 0 times ; 1 2    ; Edit authority
- Click on link "2" to chosse "Legends"
  => You get "Legends" in heading field : for example 606$a

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>

I can confirm the problem and the solution. I have tested the patch on a large
DB with authorities having multiples headings. There is no regression on bug
4838.
---
 C4/AuthoritiesMarc.pm                              |   21 ++++++++++---------
 authorities/auth_finder.pl                         |   13 ------------
 .../en/includes/authorities-search-results.inc     |   10 +++++++-
 .../modules/authorities/searchresultlist-auth.tt   |    6 ++--
 .../en/includes/authorities-search-results.inc     |   10 +++++++-
 5 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm
index 9ea3b2d..26f13ac 100644
--- a/C4/AuthoritiesMarc.pm
+++ b/C4/AuthoritiesMarc.pm
@@ -876,12 +876,13 @@ sub BuildSummary {
     my ($record,$authid,$authtypecode)=@_;
     my $dbh=C4::Context->dbh;
     my %summary;
+    my $summary_template;
     # handle $authtypecode is NULL or eq ""
     if ($authtypecode) {
         my $authref = GetAuthType($authtypecode);
         $summary{authtypecode} = $authref->{authtypecode};
         $summary{type} = $authref->{authtypetext};
-        $summary{summary} = $authref->{summary};
+        $summary_template = $authref->{summary};
     }
     my $marc21subfields = 'abcdfghjklmnopqrstuvxyz68';
     my %marc21controlrefs = ( 'a' => 'earlier',
@@ -914,14 +915,14 @@ sub BuildSummary {
 #         suit the MARC21 version, so for now the "templating"
 #         feature will be enabled only for UNIMARC for backwards
 #         compatibility.
-    if ($summary{summary} and C4::Context->preference('marcflavour') eq 'UNIMARC') {
+    if ($summary_template and C4::Context->preference('marcflavour') eq 'UNIMARC') {
         my @fields = $record->fields();
 #             $reported_tag = '$9'.$result[$counter];
-        my @stringssummary;
+        my @repets;
         foreach my $field (@fields) {
             my $tag = $field->tag();
             my $tagvalue = $field->as_string();
-            my $localsummary= $summary{summary};
+            my $localsummary= $summary_template;
             $localsummary =~ s/\[(.?.?.?.?)$tag\*(.*?)\]/$1$tagvalue$2\[$1$tag$2\]/g;
             if ($tag<10) {
                 if ($tag eq '001') {
@@ -936,13 +937,13 @@ sub BuildSummary {
                     $localsummary =~ s/\[(.?.?.?.?)$tagsubf(.*?)\]/$1$subfieldvalue$2\[$1$tagsubf$2\]/g;
                 }
             }
-            push @stringssummary, $localsummary if ($localsummary ne $summary{summary});
+            if ($localsummary ne $summary_template) {
+                $localsummary =~ s/\[(.*?)\]//g;
+                $localsummary =~ s/\n/<br>/g;
+                push @repets, $localsummary;
+            }
         }
-        my $resultstring;
-        $resultstring = join(" -- ",@stringssummary);
-        $resultstring =~ s/\[(.*?)\]//g;
-        $resultstring =~ s/\n/<br>/g;
-        $summary{summary}      =  $resultstring;
+        $summary{repets} = \@repets;
     }
     my @authorized;
     my @notes;
diff --git a/authorities/auth_finder.pl b/authorities/auth_finder.pl
index 8182299..63a7dc7 100755
--- a/authorities/auth_finder.pl
+++ b/authorities/auth_finder.pl
@@ -72,19 +72,6 @@ if ( $op eq "do_search" ) {
         $startfrom * $resultsperpage,
         $resultsperpage, $authtypecode, $orderby);
 
-    # If an authority heading is repeated, add an arrayref to those repetions
-    # First heading -- Second heading
-    for my $heading ( @$results ) {
-        my @repets = split / -- /, $heading->{summary};
-        if ( @repets > 1 ) {
-            my @repets_loop;
-            for (my $i = 0; $i < @repets; $i++) {
-                push @repets_loop,
-                    { index => $index, repet => $i+1, value => $repets[$i] };
-            }
-            $heading->{repets} = \@repets_loop;
-        }
-    }
     # multi page display gestion
     my $displaynext = 0;
     my $displayprev = $startfrom;
diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/authorities-search-results.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/authorities-search-results.inc
index 366c695..2a0c3d6 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/authorities-search-results.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/authorities-search-results.inc
@@ -29,18 +29,24 @@
     [% END %]
 [% END %]
 [% BLOCK authresult %]
-    [% IF ( summary.summary ) %][% summary.summary | html %]:[% END %]
+    <div class="authres_repet">
+      [% FOREACH repet IN summary.repets %]
+        <span>[% repet | html %]</span>
+        [% UNLESS loop.last %] | [% END %]
+      [% END %]
+    </div>
     [% UNLESS ( summary.summaryonly ) %]
         <div class="authorizedheading">
         [% FOREACH authorize IN summary.authorized %]
           <span class="authorizedheading">[% authorize.heading | html %]</span>
+          [% UNLESS loop.last %] | [% END %]
         [% END %]
         </div>
         [% IF ( marcflavour == 'UNIMARC' ) %]
           [% IF summary.notes %]
            <div class="authres_notes">
            [% FOREACH note IN summary.notes %]
-             [% note.note | html %]</span>
+             <span>[% note.note | html %]</span>
            [% END %]
            </div>
           [% END %]
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/authorities/searchresultlist-auth.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/authorities/searchresultlist-auth.tt
index 415185a..898de87 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/authorities/searchresultlist-auth.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/authorities/searchresultlist-auth.tt
@@ -73,9 +73,9 @@ function doauth(authid, index, repet)
                         <td>[% PROCESS authresult summary=resul.summary %]</td>
                         <td>[% resul.used %] times</td>
                         <td>
-                          [% IF resul.repets %]
-                            [% FOREACH repet IN resul.repets %]
-                            <a href="javascript:doauth('[% resul.authid %]', '[% repet.index %]', '[% repet.repet %]')" title="[% repet.value %]">[% repet.repet %]</a>
+                          [% IF resul.summary && resul.summary.repets && resul.summary.repets.size > 1 %]
+                            [% FOREACH repet IN resul.summary.repets %]
+                            <a href="javascript:doauth('[% resul.authid %]', '[% index %]', '[% loop.count %]')" title="[% repet | html %]">[% loop.count %]</a>
                             [% END %]
                           [% ELSE %]
                             <a href="javascript:doauth('[% resul.authid %]', '[% index %]', '')">choose</a>
diff --git a/koha-tmpl/opac-tmpl/prog/en/includes/authorities-search-results.inc b/koha-tmpl/opac-tmpl/prog/en/includes/authorities-search-results.inc
index 6e6db3c..f3374c7 100644
--- a/koha-tmpl/opac-tmpl/prog/en/includes/authorities-search-results.inc
+++ b/koha-tmpl/opac-tmpl/prog/en/includes/authorities-search-results.inc
@@ -29,18 +29,24 @@
     [% END %]
 [% END %]
 [% BLOCK authresult %]
-    [% IF ( summary.summary ) %][% summary.summary | html %]:[% END %]
+    <div class="authres_repet">
+      [% FOREACH repet IN summary.repets %]
+        <span>[% repet | html %]</span>
+        [% UNLESS loop.last %] | [% END %]
+      [% END %]
+    </div>
     [% UNLESS ( summary.summaryonly ) %]
         <div class="authorizedheading">
           [% FOREACH authorize IN summary.authorized %]
             <span class="authorizedheading">[% authorize.heading | html %]</span>
+            [% UNLESS loop.last %] | [% END %]
           [% END %]
         </div>
         [% IF ( marcflavour == 'UNIMARC' ) %]
             [% IF summary.notes %]
              <div class="authres_notes">
              [% FOREACH note IN summary.notes %]
-               [% note.note | html %]</span>
+               <span>[% note.note | html %]</span>
              [% END %]
              </div>
             [% END %]
-- 
1.7.2.5