We are using C4::Charset::StripNonXmlChars from different places of the code to remove characters that should not be part of the MARC::XML Historically we were removing them every time we were fetching the record. However, since we are starting to use Koha::Biblio::Metadata ($biblio->metadata->record) to retrieve the xml stored in the biblio_metadata table, such characters will not be removed and a degraded view will be displayed on the bibliographic detail page (see bug 23846). We didn't get a lot of complains, and it's part of Koha since 19.05.05. Shouldn't we start to use $biblio->metadata->record where GetMarcBiblio is actually used? Either we assume the MARC::XML that is stored is correct, or we need to add more StripNonXmlChars calls.
Hi Joubu, can you give an example for such characters that would be stripped? I want to help, but not sure about why it was added. Maybe it would be enough to do it on import/saving a record?
?(In reply to Katrin Fischer from comment #1) > Hi Joubu, can you give an example for such characters that would be > stripped? I want to help, but not sure about why it was added. Short answer? I don't know. The long answer is a rabbit hole. The regex is 391 $str =~ s/[^\x09\x0A\x0D\x{0020}-\x{D7FF}\x{E000}-\x{FFFD}\x{10000}-\x{10FFFF}]//g; And it's related to: https://en.wikipedia.org/wiki/Valid_characters_in_XML U+0009, U+000A, U+000D: these are the only C0 controls accepted in XML 1.0; U+0020–U+D7FF, U+E000–U+FFFD: this excludes some (not all) non-characters in the BMP (all surrogates, U+FFFE and U+FFFF are forbidden); U+10000–U+10FFFF: this includes all code points in supplementary planes, including non-characters. So, some weird characters/non-characters :) > Maybe it would be enough to do it on import/saving a record? Yes, that's why I was suggesting actually with "Either we assume the MARC::XML that is stored is correct, or we need to add more StripNonXmlChars calls." Also note that Galen wrote, as the time: commit b549d7e1f1b7d518e16fa48af7360a38e8233fec Date: Fri Feb 8 16:35:18 2008 -0600 added StripNonXmlChars to C4::Charset "StripNonXmlChars should not necessarily be used, as it may be better to reject a file or record if it contains that kind of encoding error." We ended up using it from almost everywhere, inconsistently.
Sounds reasonable to move such checks to import only sections in the codebase (if they are needed at all?). And yes, a logical step would be going to ->record now.
Created attachment 129757 [details] [review] Bug 29697: Special case - opac not needed
Created attachment 129758 [details] [review] Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Created attachment 129759 [details] [review] Bug 29697: Remove GetHiddenItemnumbers
Created attachment 129760 [details] [review] Bug 29697: Use flag embed_items
Created attachment 129761 [details] [review] Bug 29697: Fix tests
I went "a bit" further, and completely replaced GetMarcBiblio with $biblio->metadata->record. The patches also remove GetHiddenItemnumbers (was called by the embed items code) and EmbedItemsInMarcBiblio. I first tried to create a Koha::MARC::Record class that would inherit from MARC::Record (and would have prevented the (not nice) class method call trick in Koha::Biblio::Metadata->record), but it's not trivial at all: From /usr/share/perl5/MARC/File/XML.pm 155 sub MARC::Record::new_from_xml { [...] 163 return( MARC::File::XML::decode( $xml, $enc, $format ) ); I think this is a move we must do ASAP, the diff is big but most of the changes are trivial.
Nice.
(In reply to Jonathan Druart from comment #0) > Either we assume the MARC::XML that is stored is correct, or we need to add > more StripNonXmlChars calls. And, so?
*** Bug 29307 has been marked as a duplicate of this bug. ***
Created attachment 131868 [details] [review] Bug 29697: Special case - opac not needed
Created attachment 131869 [details] [review] Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Created attachment 131870 [details] [review] Bug 29697: Remove GetHiddenItemnumbers
Created attachment 131871 [details] [review] Bug 29697: Use flag embed_items
Created attachment 131872 [details] [review] Bug 29697: Fix tests
Created attachment 131873 [details] [review] Bug 29697: Deal with the degraded view in detail.pl In detail.pl we must provide a degraded view with an error message about invalid MARC::Record. We are then forced to reproduce the GetMarcBiblio behaviour and call StripNonXmlChars on the MARC::XML
Ready for signoff!
Can we move this one forward please?
(In reply to Jonathan Druart from comment #20) > Can we move this one forward please? Test plan?
(In reply to Nick Clemens from comment #21) > (In reply to Jonathan Druart from comment #20) > > Can we move this one forward please? > > Test plan? No test plan, the diff it too big. What we need here are QA eyes :)
(In reply to Jonathan Druart from comment #22) > (In reply to Nick Clemens from comment #21) > > (In reply to Jonathan Druart from comment #20) > > > Can we move this one forward please? > > > > Test plan? > > No test plan, the diff it too big. What we need here are QA eyes :) I doesn't apply. Please rebase and I'll test it right away.
Created attachment 134066 [details] [review] Bug 29697: Special case - opac not needed
Created attachment 134067 [details] [review] Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Created attachment 134068 [details] [review] Bug 29697: Remove GetHiddenItemnumbers
Created attachment 134069 [details] [review] Bug 29697: Use flag embed_items
Created attachment 134070 [details] [review] Bug 29697: Fix tests
Created attachment 134071 [details] [review] Bug 29697: Deal with the degraded view in detail.pl In detail.pl we must provide a degraded view with an error message about invalid MARC::Record. We are then forced to reproduce the GetMarcBiblio behaviour and call StripNonXmlChars on the MARC::XML
I get sha1 issues trying to apply this.. more than happy to use gitlab branches if that's easier ?
(In reply to Martin Renvoize from comment #30) > I get sha1 issues trying to apply this.. more than happy to use gitlab > branches if that's easier ? https://gitlab.com/joubu/Koha/-/commits/bug_29697
Comment on attachment 134069 [details] [review] Bug 29697: Use flag embed_items Review of attachment 134069 [details] [review]: ----------------------------------------------------------------- This is a great start, but I think we can golf this between us next cycle and come up with something considerably cleaner working towards efficiency ::: Koha/Biblio/Metadata.pm @@ +65,5 @@ > + biblionumber => $biblionumber, > + itemnumbers => $itemnumbers, > + opac => $opac > + } > + ); I think I'd rather see a distinct ->filtered_record() method as opposed to adding filtering functionality into the existing method..? @@ +67,5 @@ > + opac => $opac > + } > + ); > + > +Given a MARC::Record object containing a bib record, You've made this a class method as well as an object method now I see.. the POD isn't entirely clear though.. we can work on that. @@ +72,5 @@ > +modify it to include the items attached to it as 9XX > +per the bib's MARC framework. > +if $itemnumbers is defined, only specified itemnumbers are embedded. > + > +If $opac is true, then opac-relevant suppressions are included. Actually.. as far as I can tell.. you're only doing OPAC filtering on the embedded items.. I'd love to have seem RecordProcessor used here and the actual record to get fully filtered using the framework display stuff.. @@ +170,5 @@ > + for my $itemnumber ( @itemnumbers ) { > + my $item_marc = C4::Items::GetMarcItem( $biblionumber, $itemnumber ); > + push @item_fields, $item_marc->field($itemtag); > + } > + $record->append_fields(@item_fields); We should use Koha::Filter::EmbedItems here.. this is exactly what it was designed for I reckon.
Created attachment 135574 [details] [review] Bug 29697: Special case - opac not needed
Created attachment 135575 [details] [review] Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Created attachment 135576 [details] [review] Bug 29697: Remove GetHiddenItemnumbers
Created attachment 135577 [details] [review] Bug 29697: Use flag embed_items
Created attachment 135578 [details] [review] Bug 29697: Fix tests
Created attachment 135579 [details] [review] Bug 29697: Deal with the degraded view in detail.pl In detail.pl we must provide a degraded view with an error message about invalid MARC::Record. We are then forced to reproduce the GetMarcBiblio behaviour and call StripNonXmlChars on the MARC::XML
(In reply to Martin Renvoize from comment #32) > Comment on attachment 134069 [details] [review] [review] > Bug 29697: Use flag embed_items > > Review of attachment 134069 [details] [review] [review]: > ----------------------------------------------------------------- > > This is a great start, but I think we can golf this between us next cycle > and come up with something considerably cleaner working towards efficiency I would love to improve more, but the problem here will be to maintain the patches up-to-date. There will be easy rebases, but that will be a nightmare to keep up. What about "baby" steps instead? The diff is huge already.
Created attachment 135580 [details] [review] Bug 29697: (follow-up) Use flag embed_items
QA: Looking here now
Once saw this: t/db_dependent/Biblio.t .. 1/14 # Failed test 'expected warnings when adding too long LCCN' # at t/db_dependent/Biblio.t line 61. # didn't find a warning # expected to find warning: (?^u:Data too long for column 'lccn') # expected to find warning: (?^u:Data too long for column 'lccn') # Failed test 'AddBiblio returns undef for biblionumber if something went wrong' # at t/db_dependent/Biblio.t line 63. # got: '281' # expected: undef # Failed test 'AddBiblio returns undef for biblioitemnumber if something went wrong' # at t/db_dependent/Biblio.t line 65. # got: '243' # expected: undef # Failed test 'No biblio should have been added if something went wrong' # at t/db_dependent/Biblio.t line 68. # got: '3' # expected: '2' # Looks like you failed 4 tests of 5. t/db_dependent/Biblio.t .. 2/14 # Failed test 'AddBiblio' # at t/db_dependent/Biblio.t line 83. t/db_dependent/Biblio.t .. 9/14 # Looks like you failed 1 test of 14. t/db_dependent/Biblio.t .. Dubious, test returned 1 (wstat 256, 0x100) But I cannot reproduce it anymore with and without strict mode..
Subject: [PATCH 7/7] Bug 29697: (follow-up) Use flag embed_items - ? Koha::Biblios->find($id)->metadata->record({ embed_items => 1 }) + ? Koha::Biblios->find($record_id)->metadata->record({ embed_items => 1 }) This puzzles me. We always passed the flag, but it seems that we passed the wrong identifier? The commit name is weird?
See also IRC - my $marcrecord = C4::Biblio::GetMarcBiblio({ biblionumber => $biblionumber }); + my $biblio = Koha::Biblios->find($biblionumber); + my $marcrecord = $biblio->metadata->record; This is a general pattern for most of the changes. Do you have performance figures ? In some cases we did NOT look up the biblio before!
(In reply to Marcel de Rooy from comment #43) > Subject: [PATCH 7/7] Bug 29697: (follow-up) Use flag embed_items > > - ? Koha::Biblios->find($id)->metadata->record({ embed_items => 1 }) > + ? Koha::Biblios->find($record_id)->metadata->record({ embed_items > => 1 }) > > This puzzles me. We always passed the flag, but it seems that we passed the > wrong identifier? The commit name is weird? Answered on IRC, this patch must be squashed with the other one with the same name, I was lazy yesterday.
(In reply to Marcel de Rooy from comment #44) > See also IRC > > - my $marcrecord = C4::Biblio::GetMarcBiblio({ biblionumber => > $biblionumber }); > + my $biblio = Koha::Biblios->find($biblionumber); > + my $marcrecord = $biblio->metadata->record; > > This is a general pattern for most of the changes. > Do you have performance figures ? > > In some cases we did NOT look up the biblio before! Still from IRC 09:59 <@Joubu> Koha::Biblio::Metadatas->search({ biblionumber => 42 })->next 09:59 <@Joubu> then ->record 09:59 <@Joubu> You will assume that there is only one row (per record) in the table 10:01 < marcelr> assumptions are generally dangerous :) 10:01 <@Joubu> actually Koha::Biblio::Metadatas->find({ biblionumber => 42, format => 'marcxml', schema => C4::Context->preference('marcflavour') }) 10:01 <@Joubu> that's getting messy a bit
diff --git a/catalogue/detail.pl b/catalogue/detail.pl index 502963f8d6..120fc28a88 100755 --- a/catalogue/detail.pl +++ b/catalogue/detail.pl @@ -84,6 +84,7 @@ if ( C4::Context->config('enable_plugins') ) { my $biblionumber = $query->param('biblionumber'); $biblionumber = HTML::Entities::encode($biblionumber); +# FIXME Special case here my $record = GetMarcBiblio({ biblionumber => $biblionumber }); my $biblio = Koha::Biblios->find( $biblionumber ); Not completely clear what this FIXME is doing here now. I assume that it relates to the encode biblionumber thing. But if it does, it would be a better idea to put it on the same line. In this context it could be something with this change too..
+my $invalid_marc_record = $@ || !$marc_record; +if ($invalid_marc_record) { + $template->param( decoding_error => $@ ); Theoretically $@ might be empty but there is not $marc_record. So you could provide a fallback for no error message in the last line..
+++ b/misc/maintenance/process_record_through_filter.pl +my $biblio = Koha::Biblios->find($ARGV[0]); +unless ( $biblio ) { + print "Biblio not found\n,"; + exit; Looks a bit odd with that comma.
Created attachment 135662 [details] [review] Bug 29697: Special case - opac not needed Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135663 [details] [review] Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135664 [details] [review] Bug 29697: Remove GetHiddenItemnumbers Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135665 [details] [review] Bug 29697: Use flag embed_items Includes: Bug 29697: (follow-up) Use flag embed_items Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135666 [details] [review] Bug 29697: Fix tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135667 [details] [review] Bug 29697: Deal with the degraded view in detail.pl In detail.pl we must provide a degraded view with an error message about invalid MARC::Record. We are then forced to reproduce the GetMarcBiblio behaviour and call StripNonXmlChars on the MARC::XML Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Good work. See earlier comments. Most minor details, apart from the performance remark where we would call GetMarcBiblio in a loop without fetching the biblio object before. Hard call for the RM. This deserves a quick push but will interfere with a lot of other stuff that you want to backport..
Just a bit of post testing ;) Crash on opening cart Not a HASH reference at /usr/share/koha/opac/opac-basket.pl line 99 my @items = $biblio->items->filter_by_visible_in_opac({ patron => $logged_in_user })->unblessed; 98: foreach my $item (@items) { 99: my $reserve_status = C4::Reserves::GetReserveStatus($item->{itemnumber}); Sub search returns a result set now, heh? So we need to iterate with ->next..
opac/opac-detail.pl: if ( scalar @all_items >= 1 ) { @items_to_show = Koha::Items->search( { itemnumbers => [ map { $_->{itemnumber} } @all_items ] } ) ->filter_by_visible_in_opac( { patron => $patron } ); opac/opac-tags.pl: my @items_to_show = $items->filter_by_visible_in_opac({ opac => 1, patron => $patron });
(In reply to Marcel de Rooy from comment #47) > diff --git a/catalogue/detail.pl b/catalogue/detail.pl > index 502963f8d6..120fc28a88 100755 > --- a/catalogue/detail.pl > +++ b/catalogue/detail.pl > @@ -84,6 +84,7 @@ if ( C4::Context->config('enable_plugins') ) { > > my $biblionumber = $query->param('biblionumber'); > $biblionumber = HTML::Entities::encode($biblionumber); > +# FIXME Special case here > my $record = GetMarcBiblio({ biblionumber => $biblionumber }); > my $biblio = Koha::Biblios->find( $biblionumber ); > > > Not completely clear what this FIXME is doing here now. > I assume that it relates to the encode biblionumber thing. But if it does, > it would be a better idea to put it on the same line. > In this context it could be something with this change too.. Hum, wondering if this is still valid. Maybe it was just a note for myself when I was writing the first patch. detail.pl is a special case as it's the only place where we deal with invalid metadata but there is nothing to fix. (In reply to Marcel de Rooy from comment #48) > +my $invalid_marc_record = $@ || !$marc_record; > +if ($invalid_marc_record) { > + $template->param( decoding_error => $@ ); > > Theoretically $@ might be empty but there is not $marc_record. > So you could provide a fallback for no error message in the last line.. Indeed. No idea what's better. Do you think we need to pass another variable to the template (invalid_marc_record set to 1)? (In reply to Marcel de Rooy from comment #49) > +++ b/misc/maintenance/process_record_through_filter.pl > +my $biblio = Koha::Biblios->find($ARGV[0]); > +unless ( $biblio ) { > + print "Biblio not found\n,"; > + exit; > > Looks a bit odd with that comma. Looks like a typo.
(In reply to Jonathan Druart from comment #59) > (In reply to Marcel de Rooy from comment #48) > > +my $invalid_marc_record = $@ || !$marc_record; > > +if ($invalid_marc_record) { > > + $template->param( decoding_error => $@ ); > > > > Theoretically $@ might be empty but there is not $marc_record. > > So you could provide a fallback for no error message in the last line.. > > Indeed. No idea what's better. Do you think we need to pass another variable > to the template (invalid_marc_record set to 1)? Or just $@ // 'some great message' to keep it simple and not touch the template?
Created attachment 135668 [details] [review] Bug 29697: Special case - opac not needed Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135669 [details] [review] Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> JD Amended patch: -# FIXME Special case here - print "Biblio not found\n,"; + print "Biblio not found\n";
Created attachment 135670 [details] [review] Bug 29697: Remove GetHiddenItemnumbers Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> JD amended patch: - my @items = $biblio->items->filter_by_visible_in_opac({ patron => $logged_in_user })->unblessed; - foreach my $item (@items) { + my $items = $biblio->items->filter_by_visible_in_opac({ patron => $logged_in_user })->unblessed; + foreach my $item (@$items) { - for my $itm (@items) { + for my $itm (@$items) { - $dat->{ITEM_RESULTS} = \@items; + $dat->{ITEM_RESULTS} = $items; - @items_to_show = Koha::Items->search( { itemnumbers => [ map { $_->{itemnumber} } @all_items ] } ) - ->filter_by_visible_in_opac( { patron => $patron } ); + @items_to_show = Koha::Items->search( { itemnumber => [ map { $_->{itemnumber} } @all_items ] } ) + ->filter_by_visible_in_opac( { patron => $patron } )->as_list; - my @items_to_show = $items->filter_by_visible_in_opac({ opac => 1, patron => $patron }); + my @items_to_show = $items->filter_by_visible_in_opac({ opac => 1, patron => $patron })->as_list;
Created attachment 135671 [details] [review] Bug 29697: Use flag embed_items Includes: Bug 29697: (follow-up) Use flag embed_items Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135672 [details] [review] Bug 29697: Fix tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135673 [details] [review] Bug 29697: Deal with the degraded view in detail.pl In detail.pl we must provide a degraded view with an error message about invalid MARC::Record. We are then forced to reproduce the GetMarcBiblio behaviour and call StripNonXmlChars on the MARC::XML Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #60) > (In reply to Jonathan Druart from comment #59) > > (In reply to Marcel de Rooy from comment #48) > > > +my $invalid_marc_record = $@ || !$marc_record; > > > +if ($invalid_marc_record) { > > > + $template->param( decoding_error => $@ ); > > > > > > Theoretically $@ might be empty but there is not $marc_record. > > > So you could provide a fallback for no error message in the last line.. > > > > Indeed. No idea what's better. Do you think we need to pass another variable > > to the template (invalid_marc_record set to 1)? > > Or just $@ // 'some great message' to keep it simple and not touch the > template? An untranslatable message? :)
(In reply to Jonathan Druart from comment #67) > (In reply to Marcel de Rooy from comment #60) > > (In reply to Jonathan Druart from comment #59) > > > (In reply to Marcel de Rooy from comment #48) > > > > +my $invalid_marc_record = $@ || !$marc_record; > > > > +if ($invalid_marc_record) { > > > > + $template->param( decoding_error => $@ ); > > > > > > > > Theoretically $@ might be empty but there is not $marc_record. > > > > So you could provide a fallback for no error message in the last line.. > > > > > > Indeed. No idea what's better. Do you think we need to pass another variable > > > to the template (invalid_marc_record set to 1)? > > > > Or just $@ // 'some great message' to keep it simple and not touch the > > template? > > An untranslatable message? :) I would keep like that for now, and see if we need to improve later.
(In reply to Marcel de Rooy from comment #44) > See also IRC > > - my $marcrecord = C4::Biblio::GetMarcBiblio({ biblionumber => > $biblionumber }); > + my $biblio = Koha::Biblios->find($biblionumber); > + my $marcrecord = $biblio->metadata->record; > > This is a general pattern for most of the changes. > Do you have performance figures ? We should add `{ prefetch => [ 'metadata' ] }` on the biblio search.
(In reply to Tomás Cohen Arazi from comment #69) > (In reply to Marcel de Rooy from comment #44) > > See also IRC > > > > - my $marcrecord = C4::Biblio::GetMarcBiblio({ biblionumber => > > $biblionumber }); > > + my $biblio = Koha::Biblios->find($biblionumber); > > + my $marcrecord = $biblio->metadata->record; > > > > This is a general pattern for most of the changes. > > Do you have performance figures ? > > We should add `{ prefetch => [ 'metadata' ] }` on the biblio search. For a single record, I don't think it's better. See: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30169
Created attachment 135797 [details] [review] Bug 29697: Special case - opac not needed Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135798 [details] [review] Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> JD Amended patch: -# FIXME Special case here - print "Biblio not found\n,"; + print "Biblio not found\n";
Created attachment 135799 [details] [review] Bug 29697: Remove GetHiddenItemnumbers Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> JD amended patch: - my @items = $biblio->items->filter_by_visible_in_opac({ patron => $logged_in_user })->unblessed; - foreach my $item (@items) { + my $items = $biblio->items->filter_by_visible_in_opac({ patron => $logged_in_user })->unblessed; + foreach my $item (@$items) { - for my $itm (@items) { + for my $itm (@$items) { - $dat->{ITEM_RESULTS} = \@items; + $dat->{ITEM_RESULTS} = $items; - @items_to_show = Koha::Items->search( { itemnumbers => [ map { $_->{itemnumber} } @all_items ] } ) - ->filter_by_visible_in_opac( { patron => $patron } ); + @items_to_show = Koha::Items->search( { itemnumber => [ map { $_->{itemnumber} } @all_items ] } ) + ->filter_by_visible_in_opac( { patron => $patron } )->as_list; - my @items_to_show = $items->filter_by_visible_in_opac({ opac => 1, patron => $patron }); + my @items_to_show = $items->filter_by_visible_in_opac({ opac => 1, patron => $patron })->as_list;
Created attachment 135800 [details] [review] Bug 29697: Use flag embed_items Includes: Bug 29697: (follow-up) Use flag embed_items Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135801 [details] [review] Bug 29697: Fix tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135802 [details] [review] Bug 29697: Deal with the degraded view in detail.pl In detail.pl we must provide a degraded view with an error message about invalid MARC::Record. We are then forced to reproduce the GetMarcBiblio behaviour and call StripNonXmlChars on the MARC::XML Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Patches rebased against master.
This patch series breaks easy analytic link creation. To reproduce: 1. Enable EasyAnalyticalRecords syspref 2. Go to any biblio page in the staff interface 3. Click New -> Analyze items 4. On one of the items for the biblio click "Create analytics" 5. Notice you get the following error: > Can't use string ("Koha::Biblio") as a HASH ref while "strict refs" in use at /kohadevbox/koha/Koha/Object.pm line 830
Good catch, Joonas!
I found another problem, after this patch the zebra rebuild doesn't work: root@kohadevbox:koha(master)$ koha-shell kohadev $ perl misc/migration_tools/rebuild_zebra.pl -b error retrieving biblio 369 at misc/migration_tools/rebuild_zebra.pl line 689.
(In reply to Joonas Kylmälä from comment #80) > I found another problem, after this patch the zebra rebuild doesn't work: > > root@kohadevbox:koha(master)$ koha-shell kohadev > $ perl misc/migration_tools/rebuild_zebra.pl -b > error retrieving biblio 369 at misc/migration_tools/rebuild_zebra.pl line > 689. It is actually fixing a bug :) Biblio 369 has a bad MARCXML, and so code is expecting this warning (before and after this patch): eval { my $biblio = Koha::Biblios->find($record_number); $marc = $biblio->metadata->record({ embed_items => 1 }); }; if ($@ || !$marc) { # here we do warn since catching an exception # means that the bib was found but failed # to be parsed warn "error retrieving biblio $record_number"; return; }
Joonas, I let you switch the status back to PQA if you agree with the previous comment.
I missed the other comment anyway, so not ready for PQA.
Created attachment 135978 [details] [review] Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> JD Amended patch: -# FIXME Special case here - print "Biblio not found\n,"; + print "Biblio not found\n"; - my $biblio = Koha::Biblio->find($hostbiblionumber); + my $biblio = Koha::Biblios->find($hostbiblionumber);
Created attachment 135979 [details] [review] Bug 29697: Remove GetHiddenItemnumbers Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> JD amended patch: - my @items = $biblio->items->filter_by_visible_in_opac({ patron => $logged_in_user })->unblessed; - foreach my $item (@items) { + my $items = $biblio->items->filter_by_visible_in_opac({ patron => $logged_in_user })->unblessed; + foreach my $item (@$items) { - for my $itm (@items) { + for my $itm (@$items) { - $dat->{ITEM_RESULTS} = \@items; + $dat->{ITEM_RESULTS} = $items; - @items_to_show = Koha::Items->search( { itemnumbers => [ map { $_->{itemnumber} } @all_items ] } ) - ->filter_by_visible_in_opac( { patron => $patron } ); + @items_to_show = Koha::Items->search( { itemnumber => [ map { $_->{itemnumber} } @all_items ] } ) + ->filter_by_visible_in_opac( { patron => $patron } )->as_list; - my @items_to_show = $items->filter_by_visible_in_opac({ opac => 1, patron => $patron }); + my @items_to_show = $items->filter_by_visible_in_opac({ opac => 1, patron => $patron })->as_list;
Created attachment 135980 [details] [review] Bug 29697: Use flag embed_items Includes: Bug 29697: (follow-up) Use flag embed_items Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135981 [details] [review] Bug 29697: Fix tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135982 [details] [review] Bug 29697: Deal with the degraded view in detail.pl In detail.pl we must provide a degraded view with an error message about invalid MARC::Record. We are then forced to reproduce the GetMarcBiblio behaviour and call StripNonXmlChars on the MARC::XML Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Jonathan Druart from comment #83) > I missed the other comment anyway, so not ready for PQA. Fixed. Waiting for Joonas to approve comment 81.
(In reply to Jonathan Druart from comment #89) > (In reply to Jonathan Druart from comment #83) > > I missed the other comment anyway, so not ready for PQA. > > Fixed. > > Waiting for Joonas to approve comment 81. All good now.
Doesn't apply anymore.
Created attachment 137835 [details] [review] Bug 29697: Special case - opac not needed Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 137836 [details] [review] Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> JD Amended patch: -# FIXME Special case here - print "Biblio not found\n,"; + print "Biblio not found\n"; - my $biblio = Koha::Biblio->find($hostbiblionumber); + my $biblio = Koha::Biblios->find($hostbiblionumber); Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 137837 [details] [review] Bug 29697: Remove GetHiddenItemnumbers Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> JD amended patch: - my @items = $biblio->items->filter_by_visible_in_opac({ patron => $logged_in_user })->unblessed; - foreach my $item (@items) { + my $items = $biblio->items->filter_by_visible_in_opac({ patron => $logged_in_user })->unblessed; + foreach my $item (@$items) { - for my $itm (@items) { + for my $itm (@$items) { - $dat->{ITEM_RESULTS} = \@items; + $dat->{ITEM_RESULTS} = $items; - @items_to_show = Koha::Items->search( { itemnumbers => [ map { $_->{itemnumber} } @all_items ] } ) - ->filter_by_visible_in_opac( { patron => $patron } ); + @items_to_show = Koha::Items->search( { itemnumber => [ map { $_->{itemnumber} } @all_items ] } ) + ->filter_by_visible_in_opac( { patron => $patron } )->as_list; - my @items_to_show = $items->filter_by_visible_in_opac({ opac => 1, patron => $patron }); + my @items_to_show = $items->filter_by_visible_in_opac({ opac => 1, patron => $patron })->as_list;
Created attachment 137838 [details] [review] Bug 29697: Use flag embed_items Includes: Bug 29697: (follow-up) Use flag embed_items Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 137839 [details] [review] Bug 29697: Fix tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 137840 [details] [review] Bug 29697: Deal with the degraded view in detail.pl In detail.pl we must provide a degraded view with an error message about invalid MARC::Record. We are then forced to reproduce the GetMarcBiblio behaviour and call StripNonXmlChars on the MARC::XML Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Rebased.
Looks like ktd is failing to setup if those patches are applied and KOHA_ELASTICSEARCH is set to 1. koha_1 | [942] Indexing biblios koha_1 | Invalid data, cannot decode metadata object (biblio_metadata.id=368, biblionumber=369, format=marcxml, schema=MARC21, decoding_error=':8: parser error : PCDATA invalid Char value 31 koha_1 | <controlfield tag="001">00aD000015937</controlfield> koha_1 | ^ koha_1 | :9: parser error : PCDATA invalid Char value 31 koha_1 | <controlfield tag="004">00satmrnu0</controlfield> koha_1 | ^ koha_1 | :9: parser error : PCDATA invalid Char value 31 koha_1 | <controlfield tag="004">00satmrnu0</controlfield> koha_1 | ^ koha_1 | :9: parser error : PCDATA invalid Char value 31 koha_1 | <controlfield tag="004">00satmrnu0</controlfield> koha_1 | ^ koha_1 | :9: parser error : PCDATA invalid Char value 31 koha_1 | <controlfield tag="004">00satmrnu0</controlfield> koha_1 | ^ koha_1 | :10: parser error : PCDATA invalid Char value 31 koha_1 | <controlfield tag="008">00ar19881981bdkldan</controlfield> koha_1 | ^ koha_1 | :10: parser error : PCDATA invalid Char value 31 koha_1 | <controlfield tag="008">00ar19881981bdkldan</controlfield> koha_1 | ^ koha_1 | :10: parser error : PCDATA invalid Char value 31 koha_1 | <controlfield tag="008">00ar19881981bdkldan</controlfield> db_1 | 2022-07-19 7:27:59 13 [Warning] Aborted connection 13 to db: 'koha_kohadev' user: 'koha_kohadev' host: '172.24.0.6' (Got an error reading communication packets) koha_1 | ^')koha_koha_1 exited with code 1 May need to be investigated before push.
(In reply to Jonathan Druart from comment #99) > May need to be investigated before push. In comment 81 the cause was deemed to be an invalid MARC record. However, as this breaks the development setup, and the automated test server / jenkins I presume, then to me this is FQA. We need to provide a patch along with this change that removes the invalid MARC record.
(In reply to Joonas Kylmälä from comment #100) > (In reply to Jonathan Druart from comment #99) > > May need to be investigated before push. > > In comment 81 the cause was deemed to be an invalid MARC record. However, as > this breaks the development setup, and the automated test server / jenkins I > presume, then to me this is FQA. We need to provide a patch along with this > change that removes the invalid MARC record. This is actually a behavior change we need to address on this patchset.
Created attachment 137894 [details] [review] Bug 29697: Don't crash ES reindex if a record is wrong We don't want rebuild_elasticsearch.pl to crash if a record is wrong (also fix ktd setup). Note that now the script will output an error for the problematic record, which is certainly the expected behaviour as we are doing it already for authority records.
Last patch would benefit from a QA review but I don't want to set the status back to NQA and lose it for weeks.
Created attachment 137918 [details] [review] Bug 29697: Special case - opac not needed Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 137919 [details] [review] Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> JD Amended patch: -# FIXME Special case here - print "Biblio not found\n,"; + print "Biblio not found\n"; - my $biblio = Koha::Biblio->find($hostbiblionumber); + my $biblio = Koha::Biblios->find($hostbiblionumber); Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 137920 [details] [review] Bug 29697: Remove GetHiddenItemnumbers Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> JD amended patch: - my @items = $biblio->items->filter_by_visible_in_opac({ patron => $logged_in_user })->unblessed; - foreach my $item (@items) { + my $items = $biblio->items->filter_by_visible_in_opac({ patron => $logged_in_user })->unblessed; + foreach my $item (@$items) { - for my $itm (@items) { + for my $itm (@$items) { - $dat->{ITEM_RESULTS} = \@items; + $dat->{ITEM_RESULTS} = $items; - @items_to_show = Koha::Items->search( { itemnumbers => [ map { $_->{itemnumber} } @all_items ] } ) - ->filter_by_visible_in_opac( { patron => $patron } ); + @items_to_show = Koha::Items->search( { itemnumber => [ map { $_->{itemnumber} } @all_items ] } ) + ->filter_by_visible_in_opac( { patron => $patron } )->as_list; - my @items_to_show = $items->filter_by_visible_in_opac({ opac => 1, patron => $patron }); + my @items_to_show = $items->filter_by_visible_in_opac({ opac => 1, patron => $patron })->as_list;
Created attachment 137921 [details] [review] Bug 29697: Use flag embed_items Includes: Bug 29697: (follow-up) Use flag embed_items Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 137922 [details] [review] Bug 29697: Fix tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 137923 [details] [review] Bug 29697: Deal with the degraded view in detail.pl In detail.pl we must provide a degraded view with an error message about invalid MARC::Record. We are then forced to reproduce the GetMarcBiblio behaviour and call StripNonXmlChars on the MARC::XML Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 137924 [details] [review] Bug 29697: Don't crash ES reindex if a record is wrong We don't want rebuild_elasticsearch.pl to crash if a record is wrong (also fix ktd setup). Note that now the script will output an error for the problematic record, which is certainly the expected behaviour as we are doing it already for authority records.
Rebased against master.
Created attachment 137948 [details] [review] Bug 29697: Don't crash ES reindex if a record is wrong We don't want rebuild_elasticsearch.pl to crash if a record is wrong (also fix ktd setup). Note that now the script will output an error for the problematic record, which is certainly the expected behaviour as we are doing it already for authority records. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
(In reply to Jonathan Druart from comment #103) > Last patch would benefit from a QA review but I don't want to set the status > back to NQA and lose it for weeks. Looks good to me, handing off to the release manager.
Created attachment 138057 [details] [review] Bug 29697: (follow-up) Indexing a deleted record is not expected to crash Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 22.11. Nice work everyone, thanks!
Created attachment 138076 [details] [review] Bug 29697: Replace some missing occurrences
Created attachment 138077 [details] [review] Bug 29697: Replace some missing occurrences
Created attachment 138078 [details] [review] Bug 29697: Fix t/db_dependent/Exporter/Record.t Not sure about the warn, we shouldn't need it as we are raising an exception. But better (for now) than introducing regressions.
Created attachment 138079 [details] [review] Bug 29697: Reintroduce wrongly removed import # Failed test 'No tests run for subtest "CancelHold"' # at t/db_dependent/ILSDI_Services.t line 806. Undefined subroutine &C4::ILSDI::Services::CanReserveBeCanceledFromOpac called at /kohadevbox/koha/C4/ILSDI/Services.pm line 941.
Created attachment 138080 [details] [review] Bug 29697: Fix t/Search.t # Failed test 'No tests run for subtest "searchResults PassItemMarcToXSLT test"' # at t/Search.t line 208. Undefined subroutine &C4::Biblio::EmbedItemsInMarcBiblio called at t/Search.t line 194.
Created attachment 138081 [details] [review] Bug 29697: Fix t/db_dependent/Search.t FIXME This does not seem correct. I bet it was coming from a problem in our test data but apparently all the items in t/db_dependent/data/marc21/zebraexport/biblio/exported_records have a 952$9 I don't fully understand why we reach this code without itemnumber. Not sure this is the correct fix
Created attachment 138089 [details] [review] Bug 29697: tracklinks should not explode on invalid marc record Caught by t/db_dependent/www/regressions.t # Failed test '404 for itemnumber containing different URI' # at t/db_dependent/www/regressions.t line 124. # got: '500' # expected: '404' # Failed test '302 for itemnumber with matching URI' # at t/db_dependent/www/regressions.t line 126. # got: '500' # expected: '302' # Failed test '404 for itemnumber containing different URI' # at t/db_dependent/www/regressions.t line 139. # got: '500' # expected: '404' # Failed test '302 for itemnumber with matching URI' # at t/db_dependent/www/regressions.t line 141. # got: '500' # expected: '302' # Looks like you failed 4 tests of 36.
Created attachment 138090 [details] [review] Bug 29697: Fix t/db_dependent/Search.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Tomás Cohen Arazi from comment #123) > Created attachment 138090 [details] [review] [review] > Bug 29697: Fix t/db_dependent/Search.t > > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I have looked at the wrong data apparently, thanks!
Follow-ups pushed to master. We owe you some beers Jonathan. Thanks!
(In reply to Tomás Cohen Arazi from comment #125) > Follow-ups pushed to master. We owe you some beers Jonathan. Thanks! I broke, I fix ;)
This broke t/db_dependent/Search.t for non-LIGHT_RUN (Koha_Master). The data from koha-misc4dev are not inserted and there are no items in the DB. But those tests expect items with itemnumber=1 and 2. t/db_dependent/Search.t .. # Failed test 'All records have at least one item available' # at t/db_dependent/Search.t line 475. # got: 'false' # expected: 'true' Use of uninitialized value $fw in concatenation (.) or string at t/db_dependent/Search.t line 494. Use of uninitialized value $fw in concatenation (.) or string at t/db_dependent/Search.t line 503. # Failed test ''notforloancount' == 2 (Bug 12419)' # at t/db_dependent/Search.t line 758. # got: '0' # expected: '2' # Looks like you failed 2 tests of 92. t/db_dependent/Search.t .. 1/3 # Failed test 'MARC21 + DOM' # at t/db_dependent/Search.t line 914.
Created attachment 138804 [details] [review] Bug 29697: WIP - fix t/db_dependent/Search.t
From this change in C4::Search::searchResults - # OPAC hidden items + # OPAC hidden items if ($is_opac) { - # hidden because lost - if ($hidelostitems && $item->{itemlost}) { + # hidden based on OpacHiddenItems syspref or because lost + my $hi = Koha::Items->search( { itemnumber => $item->{itemnumber} } ) + ->filter_by_visible_in_opac({ patron => $search_context->{patron} }); + unless ( $hi->count ) { push @hiddenitems, $item->{itemnumber}; $hideatopac_count++; next; } - # hidden based on OpacHiddenItems syspref - my @hi = C4::Items::GetHiddenItemnumbers({ items=> [ $item ], borcat => $search_context->{category} }); - if (scalar @hi) { - push @hiddenitems, @hi; - $hideatopac_count++; - next; - } } We are actually dealing with lost items HERE, when we were only dealing with "OpacHiddenItems" only prior to this change, and lost items where handled later. Is that a problem? I don't think so. Should we expect side-effects? The previous patch fixes the first error. The other one is still failing as we are expecting notforloan_count to be set.
The following change should fix the second failure but it does not. diff --git a/C4/Search.pm b/C4/Search.pm index 75da21a4e6a..21b33d85fe9 100644 --- a/C4/Search.pm +++ b/C4/Search.pm @@ -1828,6 +1828,7 @@ sub searchResults { ->filter_by_visible_in_opac({ patron => $search_context->{patron} }); unless ( $hi->count ) { push @hiddenitems, $item->{itemnumber}; + $notforloan_count++; $hideatopac_count++; next; We are getting # got: '3' # expected: '2' Help needed!
(those tests are a mess!)
Created attachment 139127 [details] [review] Bug 29697: (follow-up) Check against itemnumbers for visibility No items are shoing on the opac because we are comparing the itemnumber to an object This patch simplies check against the itemnumber Note: With this the inclusion of host_items it still broken as they won't be in the visible items list Bug 31313 will fix this completely
Some remnants in the master branch (I am trying to combine (oh that's the challenge!) these changes with Bug 20447, that's why giving some parallel feedback): I see this left: C4/Biblio.pm: 56: EmbedItemsInMarcBiblio I think that line is the only one that we have across all codebase, so just the remnants.
Comment on attachment 138804 [details] [review] Bug 29697: WIP - fix t/db_dependent/Search.t Review of attachment 138804 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/Search.t @@ +472,5 @@ > + Koha::Database::flush_schema_cache(); > + fixtures_ok [ > + Item => [ > + [qw( itemnumber )], > + [ 1 ], Getting picky but... This is not entirely correct. When you look at data/marc21/zebraexport/large_biblio/exported_records.xml you see there are 4 items defined on that record. The itemnumbers for them are 123, 124, 125 and 126. Two of them are set to be not for loan (i.e. $7 1 in this case). That's why the test is expecting exactly 2 in the notforloancount return value for searchResults. This mock on the resultset 'fixes' the tests only because the ->count is returning 2, so unrelated to the actual itemnumbers. I think we should just do some simpler mocking. Will give it a try.
Created attachment 140007 [details] [review] Bug 29697: (QA follow-up) Make tests less fragile This patch makes the tests less fragile to existing data. It does so by making sure that when checking if the item would be hidden by OpacHiddenItems and lost status, the item is not skipped, and thus the notforloan value, as retrieved from the search engine is picked for the notforloan_count calculation. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 140008 [details] [review] Bug 29697: (QA follow-up) Remove useless warning Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Enhancement will not be backported to 22.05.x series