Bug 6657

Summary: Making 999 visible in framework duplicates 999 entry and affects index
Product: Koha Reporter: Jane Wagner <jwagner>
Component: CatalogingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: critical    
Priority: P5 - low CC: alen, barton, eb, frederic, gwilliams, jonathan.druart, julian.maurice, katrin.fischer, koha.sekjal, liz, misilot, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15358
Whiteboard:
Change sponsored?: --- Patch complexity: Small patch
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
Bug Depends on:    
Bug Blocks: 15572    
Attachments: Bug 6657: Prevent biblionumber to be duplicated
[SIGNED-OFF] Bug 6657: Prevent biblionumber to be duplicated
[PASSED QA] Bug 6657: Prevent biblionumber to be duplicated
Bug 6657: Follow-up to fix error on authority creation
Bug 6657: Follow-up to fix error on authority creation
Bug 6657: Follow-up to fix error on authority creation
Bug 6657: Add tests for TransformHtmlToMarc
Bug 6657: Follow-up to fix error on authority creation
Bug 6657: Add tests for TransformHtmlToMarc
Bug 6657: counter-patch
Bug 6657: counter-patch
Bug 6657: TransformHtmlToMarc - rewrite the loop
Bug 6657: TransformHtmlToMarc - rewrite the loop
Bug 6657: TransformHtmlToMarc - rewrite the loop

Description Jane Wagner 2011-08-01 14:01:53 UTC
The 999 field is usually hidden in the frameworks; the 999$c is where Koha stores the biblionumber for the title.  If someone edits the framework to make any field of the 999 visible, then any edit to any field of the MARC record results in a duplicate 999 with the biblionumber repeated.  This can cause the record to appear twice in the zebra index, and can also make OPAC suppression not work.  I have verified this behavior in current development head and several other versions.  To replicate, export a title record and check its 999 field. There should only be one.  For example:

=999  \\$c219$d219

or

 <datafield tag="999" ind1=" " ind2=" ">
    <subfield code="c">219</subfield>
    <subfield code="d">219</subfield>
  </datafield>


Then edit the framework to make any field of the 999 visible, and edit that same record.  Make any change to the title, such as adding a 5xx note.  Export the record again and check the 999.  There will now be two of them, repeating the biblionumber:

=999  \\$c219$d219
=999  \\$c219

or

 <datafield tag="999" ind1=" " ind2=" ">
    <subfield code="c">219</subfield>
    <subfield code="d">219</subfield>
  </datafield>
  <datafield tag="999" ind1=" " ind2=" ">
    <subfield code="c">219</subfield>
  </datafield>


Changing the framework field back to hidden and forcing the MARC record back through zebraqueue does not fix the problem.  The only fix we've found is to manually remove the extra 999 at the table level, then rebuild indexes.

The first solution for the problem is to not set the 999 field as visible in any framework, but since you can't stop people from being adventurous, we also developed a code safeguard.  It has not yet been backported from our current development head to the version in our public git repository; when it is, I'll add a link.  In the meantime, here are the details of that commit.


   If the framework allows the biblionumber (999$c) field to be displayed and
   edited within the MARC editor, then this results in an additional 999$c value
   being added to the MARC record.  This is because the code already pulled the
   biblionumber from the biblio table.  This patch removes the addition of the
   extra 999$c field, although this field shouldn't really be editable in the
   first place.

diff --git a/C4/Biblio.pm b/C4/Biblio.pm
index 2e28f3f..3f61519 100644
--- a/C4/Biblio.pm
+++ b/C4/Biblio.pm
@@ -1858,13 +1858,13 @@ sub TransformHtmlToMarc {
     my $record  = MARC::Record->new();
     my $i=0;
     my @fields;
+    my ( $biblionumbertagfield, $biblionumbertagsubfield ) =
+        &GetMarcFromKohaField( "biblio.biblionumber", '' );
     while ($params->[$i]){ # browse all CGI params
         my $param = $params->[$i];
         my $newfield=0;
         # if we are on biblionumber, store it in the MARC::Record (it may not be in the edited fields)
         if ($param eq 'biblionumber') {
-            my ( $biblionumbertagfield, $biblionumbertagsubfield ) =
-                &GetMarcFromKohaField( "biblio.biblionumber", '' );
             if ($biblionumbertagfield < 10) {
                 $newfield = MARC::Field->new(
                     $biblionumbertagfield,
@@ -1903,6 +1903,12 @@ sub TransformHtmlToMarc {
             } else {
                 while(defined $params->[$j] && $params->[$j] =~ /_code_/){ # browse all it's subfield
                     my $inner_param = $params->[$j];
+                    my $biblionumbertagsubfield_string = "_code_" . $biblionumbertagsubfield . "_";
+                    if (($tag eq $biblionumbertagfield) &&
+                        ($inner_param =~ /$biblionumbertagsubfield_string/)) {
+                      $j += 2;
+                      next;
+                    }
                     if ($newfield){
Comment 1 Alen Vodopijevec 2013-02-11 13:36:35 UTC
I can confirm persistence of this bug in 3.10.02
Comment 2 Elaine Bradtke 2013-02-20 21:27:07 UTC
in 3.10.02 the problem of multiple 999c fields and OPAC record suppression persists.  However, the 999 subfields in our framework are all hidden.
Comment 3 Liz Rea 2014-11-27 22:46:23 UTC
This still happens in 3.12 as well.
Comment 4 Barton Chittenden 2015-09-16 20:42:13 UTC
This is still occurring in 3.18.06
Comment 5 Jonathan Druart 2015-09-17 13:36:48 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-09-17 13:37:13 UTC
No change to get tests here, this code is too messy.
Comment 7 Bernardo Gonzalez Kriegel 2015-10-30 13:50:05 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2015-11-04 00:08:38 UTC
Would it be possible to unit test this?
Comment 9 Jonathan Druart 2015-11-05 10:14:09 UTC
(In reply to Katrin Fischer from comment #8)
> Would it be possible to unit test this?

Yes, it would, but it's kind of mental torture to provide a test, the subroutine is not tested at all.
Comment 10 Katrin Fischer 2015-11-15 22:47:00 UTC
The patch works as described, but I notice that the biblioitemnumber still duplicates. I think it would be good to fix that as well.
Comment 11 Katrin Fischer 2015-11-15 22:48:13 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2015-11-16 15:56:23 UTC
Patch pushed to master.

Thanks Jonathan!
Comment 13 Frédéric Demians 2015-11-29 20:56:02 UTC
This patch has been pushed to 3.20.x, will be in 3.20.6.
Comment 14 Liz Rea 2016-01-06 21:03:26 UTC
Pushed to 3.18.13, and released.
Comment 15 Frédéric Demians 2016-01-12 09:58:08 UTC
This patch has a side effect on authorities creation (tested in 3.20.x). The authorities.pl run foreever, and adding a new authority is impossible.
Comment 16 Frédéric Demians 2016-01-12 10:56:07 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-01-12 12:21:12 UTC
Comment on attachment 46517 [details] [review]
Bug 6657: Follow-up to fix error on authority creation

Review of attachment 46517 [details] [review]:
-----------------------------------------------------------------

::: C4/Biblio.pm
@@ +2541,5 @@
>  
>              if ( $tag < 10 ) {                              # no code for theses fields
>                                                              # in MARC editor, 000 contains the leader.
> +                if ( $isbiblio && $tag == $biblionumbertagfield ) {
> +                    $i += 4;

Could you explain this line please?
Comment 18 Frédéric Demians 2016-01-12 12:33:28 UTC
(In reply to Jonathan Druart from comment #17)
> Comment on attachment 46517 [details] [review] [review]
> Bug 6657: Follow-up to fix error on authority creation
> 
> Review of attachment 46517 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Biblio.pm
> @@ +2541,5 @@
> >  
> >              if ( $tag < 10 ) {                              # no code for theses fields
> >                                                              # in MARC editor, 000 contains the leader.
> > +                if ( $isbiblio && $tag == $biblionumbertagfield ) {
> > +                    $i += 4;
> 
> Could you explain this line please?

In @params array, for a control field (tag < 10), there is 4 entries. For example:

tag_001_indicator1_816115
tag_001_indicator2_816115
tag_001_code_00_816115_723352
tag_001_subfield_00_816115_723352

So $i += 4 is done before next in order to move to the next field in the @params array.
Comment 19 Jonathan Druart 2016-01-12 13:37:03 UTC
Looking at koha-tmpl/intranet-tmpl/prog/en/includes/merge-record.inc, it seems that this sequence could be different.
Comment 20 Frédéric Demians 2016-01-12 13:55:35 UTC
OK. In any case, $i has to be increment before next. Otherwise the loop on the @params array never ends: that's what I get now on 3.20/3.22/master branches. I will just increment by 1, relying on this line to positionne $i on the array good element:

} elsif ( $param =~ /^tag_(\d*)_indicator1_/ )
Comment 21 Frédéric Demians 2016-01-12 13:58:28 UTC Comment hidden (obsolete)
Comment 22 Frédéric Demians 2016-01-12 13:59:49 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2016-01-12 15:05:13 UTC
What about setting biblionumbertagsubfield and biblionumbertagfield to an empty string at the beginning of the subroutine if we are processing an authority record?
It would avoid to add lines to this code...
Comment 24 Frédéric Demians 2016-01-12 15:10:32 UTC
No problem if you prefer. But do you agree with the principle of changing TransformMarcHtmlToMarc() signature, ie adding a second parameter to distinguish biblio/authority record?
Comment 25 Jonathan Druart 2016-01-12 15:23:48 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2016-01-12 15:24:26 UTC
(In reply to Frédéric Demians from comment #24)
> No problem if you prefer. But do you agree with the principle of changing
> TransformMarcHtmlToMarc() signature, ie adding a second parameter to
> distinguish biblio/authority record?

No, I hate the idea :)
But I don't see any other quick and easy solution.
Comment 27 Jonathan Druart 2016-01-12 15:25:23 UTC
(In reply to Jonathan Druart from comment #25)
> Created attachment 46538 [details] [review] [review]
> Bug 6657: Add tests for TransformHtmlToMarc

Regarding these tests, your patch is wrong.
But the tests may be wrong too :)
Comment 28 Frédéric Demians 2016-01-12 15:46:53 UTC Comment hidden (obsolete)
Comment 29 Frédéric Demians 2016-01-12 15:47:03 UTC Comment hidden (obsolete)
Comment 30 Julian Maurice 2016-01-12 15:48:02 UTC
What about replacing the 'while' loop by a good old 'for' loop ? This way, 'next' would increment the counter correctly.
Comment 31 Frédéric Demians 2016-01-12 15:48:40 UTC
> Regarding these tests, your patch is wrong.
> But the tests may be wrong too :)

+1 for UT!

I've signed-off your patch, which works with amending the call to TransformHtmlToMarc (2nd parameter was missing).
Comment 32 Julian Maurice 2016-01-12 15:50:52 UTC Comment hidden (obsolete)
Comment 33 Frédéric Demians 2016-01-12 15:58:28 UTC Comment hidden (obsolete)
Comment 34 Julian Maurice 2016-01-12 16:11:55 UTC Comment hidden (obsolete)
Comment 35 Julian Maurice 2016-01-12 16:14:14 UTC
It seems that the "while -> for" rewrite is not enough alone, so I rewrite my patch to fit well with the others
Comment 36 Julian Maurice 2016-01-13 08:16:09 UTC Comment hidden (obsolete)
Comment 37 Frédéric Demians 2016-01-13 08:28:17 UTC Comment hidden (obsolete)
Comment 38 Frédéric Demians 2016-01-13 08:30:23 UTC
I finally reset this patch status to 'Needs Signoff' since the 1st patch of the set isn't yet signed. Maybe the importance of this patch should be upgraded to Critical.
Comment 39 Katrin Fischer 2016-01-13 08:44:42 UTC
>This patch has a side effect on authorities creation (tested in 3.20.x). The >authorities.pl run foreever, and adding a new authority is impossible.

Hi Frederic, I agree with critical - I have changed the severity. 

For next time - I think it's better to handle these things on a separate bug report linked to the one which caused the problem. Then the title can reflect the urgency better and it's easer to navigate the discussion. As we use the bug titles for the release notes it also helps to better communicate a fixed problem.
Comment 40 Frédéric Demians 2016-01-13 10:17:17 UTC
(In reply to Katrin Fischer from comment #39)
> >This patch has a side effect on authorities creation (tested in 3.20.x). The >authorities.pl run foreever, and adding a new authority is impossible.
> 
> Hi Frederic, I agree with critical - I have changed the severity. 
> 
> For next time - I think it's better to handle these things on a separate bug
> report linked to the one which caused the problem. Then the title can
> reflect the urgency better and it's easer to navigate the discussion. As we
> use the bug titles for the release notes it also helps to better communicate
> a fixed problem.

Katrin, I agree that a new bug report would have been better. It's still not too late. I can create a new bug and attach the 3 patches if you may want.
Comment 41 Katrin Fischer 2016-01-13 10:43:19 UTC
Hi Frederic, I am ok with both - leaving it here or separating it out.
Comment 42 Frédéric Demians 2016-01-13 21:00:46 UTC
(In reply to Katrin Fischer from comment #41)
> Hi Frederic, I am ok with both - leaving it here or separating it out.

I've closed this patch, since it's already pushed in master, and attached the set of patch to new bug 15572.