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){
I can confirm persistence of this bug in 3.10.02
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.
This still happens in 3.12 as well.
This is still occurring in 3.18.06
Created attachment 42667 [details] [review] Bug 6657: Prevent biblionumber to be duplicated If the biblionumber field is displayed in the framework, on editing a biblio the field/subfield will be duplicated. To prevent that this patch adds a check when building the field list. Test plan: 1/ map biblio.biblionumber with 999$c 2/ Display 999$c in a framework 3/ Edit a biblio using this framework 4/ Save => The field should not have been duplicated 5/ map biblio.biblionumber with 001 6/ Display 001 in a framework 7/ Edit a biblio using this framework 8/ Save => The field should not have been duplicated
No change to get tests here, this code is too messy.
Created attachment 44199 [details] [review] [SIGNED-OFF] Bug 6657: Prevent biblionumber to be duplicated If the biblionumber field is displayed in the framework, on editing a biblio the field/subfield will be duplicated. To prevent that this patch adds a check when building the field list. Test plan: 1/ map biblio.biblionumber with 999$c 2/ Display 999$c in a framework 3/ Edit a biblio using this framework 4/ Save => The field should not have been duplicated 5/ map biblio.biblionumber with 001 6/ Display 001 in a framework 7/ Edit a biblio using this framework 8/ Save => The field should not have been duplicated Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works as described, aldo removes duplicate values. No errors
Would it be possible to unit test this?
(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.
The patch works as described, but I notice that the biblioitemnumber still duplicates. I think it would be good to fix that as well.
Created attachment 44869 [details] [review] [PASSED QA] Bug 6657: Prevent biblionumber to be duplicated If the biblionumber field is displayed in the framework, on editing a biblio the field/subfield will be duplicated. To prevent that this patch adds a check when building the field list. Test plan: 1/ map biblio.biblionumber with 999$c 2/ Display 999$c in a framework 3/ Edit a biblio using this framework 4/ Save => The field should not have been duplicated 5/ map biblio.biblionumber with 001 6/ Display 001 in a framework 7/ Edit a biblio using this framework 8/ Save => The field should not have been duplicated Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works as described, aldo removes duplicate values. No errors Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Patch pushed to master. Thanks Jonathan!
This patch has been pushed to 3.20.x, will be in 3.20.6.
Pushed to 3.18.13, and released.
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.
Created attachment 46517 [details] [review] Bug 6657: Follow-up to fix error on authority creation Bug 6657 modified the way C4::Biblio::TransformHtmlToMarc operates in order to solve an issue occuring during biblio record cataloguing. But this function is also used by authorities cataloguing, and the code in this case is irrelevante. This followup allows to distinguish for which kind of record TransformHtmlToMarc is called: biblio/authority. A bug appears in authority creation without this patch in some circunstances: when authid is linked to 001 field.
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?
(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.
Looking at koha-tmpl/intranet-tmpl/prog/en/includes/merge-record.inc, it seems that this sequence could be different.
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_/ )
Created attachment 46536 [details] [review] Bug 6657: Follow-up to fix error on authority creation Bug 6657 modified the way C4::Biblio::TransformHtmlToMarc operates in order to solve an issue occuring during biblio record cataloguing. But this function is also used by authorities cataloguing, and the code in this case is irrelevante. This followup allows to distinguish for which kind of record TransformHtmlToMarc is called: biblio/authority. A bug appears in authority creation without this patch in some circunstances: when authid is linked to 001 field.
Created attachment 46537 [details] [review] Bug 6657: Follow-up to fix error on authority creation Bug 6657 modified the way C4::Biblio::TransformHtmlToMarc operates in order to solve an issue occuring during biblio record cataloguing. But this function is also used by authorities cataloguing, and the code in this case is irrelevante. This followup allows to distinguish for which kind of record TransformHtmlToMarc is called: biblio/authority. A bug appears in authority creation without this patch in some circunstances: when authid is linked to 001 field.
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...
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?
Created attachment 46538 [details] [review] Bug 6657: Add tests for TransformHtmlToMarc
(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.
(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 :)
Created attachment 46542 [details] [review] Bug 6657: Follow-up to fix error on authority creation Bug 6657 modified the way C4::Biblio::TransformHtmlToMarc operates in order to solve an issue occuring during biblio record cataloguing. But this function is also used by authorities cataloguing, and the code in this case is irrelevante. This followup allows to distinguish for which kind of record TransformHtmlToMarc is called: biblio/authority. A bug appears in authority creation without this patch in some circunstances: when authid is linked to 001 field.
Created attachment 46543 [details] [review] Bug 6657: Add tests for TransformHtmlToMarc Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
What about replacing the 'while' loop by a good old 'for' loop ? This way, 'next' would increment the counter correctly.
> 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).
Created attachment 46544 [details] [review] Bug 6657: counter-patch Replace while by for
Created attachment 46545 [details] [review] Bug 6657: counter-patch Replace while by for Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
Created attachment 46547 [details] [review] Bug 6657: TransformHtmlToMarc - rewrite the loop This rewrites the while loop into a for loop, so $i still gets incremented when we call next
It seems that the "while -> for" rewrite is not enough alone, so I rewrite my patch to fit well with the others
Created attachment 46568 [details] [review] Bug 6657: TransformHtmlToMarc - rewrite the loop This rewrites the while loop into a for loop, so $i still gets incremented when we call next
Created attachment 46569 [details] [review] Bug 6657: TransformHtmlToMarc - rewrite the loop This rewrites the while loop into a for loop, so $i still gets incremented when we call next Signed-off-by: Frédéric Demians <f.demians@tamil.fr> Make sense. Add readability. Infinite loop no more possible.
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.
>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.
(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.
Hi Frederic, I am ok with both - leaving it here or separating it out.
(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.