Description
Jane Wagner
2011-08-01 14:01:53 UTC
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. |