Bug 29697

Summary: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: dcook, joonas.kylmala, lucas, m.de.rooy, martin.renvoize, nick, nugged, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29307
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00
Bug Depends on: 23846, 29486, 29690, 30008    
Bug Blocks: 31224, 34549, 35490, 23247, 27272, 32798, 33069, 33270, 33347, 34014    
Attachments: Bug 29697: Special case - opac not needed
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Bug 29697: Remove GetHiddenItemnumbers
Bug 29697: Use flag embed_items
Bug 29697: Fix tests
Bug 29697: Special case - opac not needed
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Bug 29697: Remove GetHiddenItemnumbers
Bug 29697: Use flag embed_items
Bug 29697: Fix tests
Bug 29697: Deal with the degraded view in detail.pl
Bug 29697: Special case - opac not needed
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Bug 29697: Remove GetHiddenItemnumbers
Bug 29697: Use flag embed_items
Bug 29697: Fix tests
Bug 29697: Deal with the degraded view in detail.pl
Bug 29697: Special case - opac not needed
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Bug 29697: Remove GetHiddenItemnumbers
Bug 29697: Use flag embed_items
Bug 29697: Fix tests
Bug 29697: Deal with the degraded view in detail.pl
Bug 29697: (follow-up) Use flag embed_items
Bug 29697: Special case - opac not needed
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Bug 29697: Remove GetHiddenItemnumbers
Bug 29697: Use flag embed_items
Bug 29697: Fix tests
Bug 29697: Deal with the degraded view in detail.pl
Bug 29697: Special case - opac not needed
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Bug 29697: Remove GetHiddenItemnumbers
Bug 29697: Use flag embed_items
Bug 29697: Fix tests
Bug 29697: Deal with the degraded view in detail.pl
Bug 29697: Special case - opac not needed
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Bug 29697: Remove GetHiddenItemnumbers
Bug 29697: Use flag embed_items
Bug 29697: Fix tests
Bug 29697: Deal with the degraded view in detail.pl
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Bug 29697: Remove GetHiddenItemnumbers
Bug 29697: Use flag embed_items
Bug 29697: Fix tests
Bug 29697: Deal with the degraded view in detail.pl
Bug 29697: Special case - opac not needed
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Bug 29697: Remove GetHiddenItemnumbers
Bug 29697: Use flag embed_items
Bug 29697: Fix tests
Bug 29697: Deal with the degraded view in detail.pl
Bug 29697: Don't crash ES reindex if a record is wrong
Bug 29697: Special case - opac not needed
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Bug 29697: Remove GetHiddenItemnumbers
Bug 29697: Use flag embed_items
Bug 29697: Fix tests
Bug 29697: Deal with the degraded view in detail.pl
Bug 29697: Don't crash ES reindex if a record is wrong
Bug 29697: Don't crash ES reindex if a record is wrong
Bug 29697: (follow-up) Indexing a deleted record is not expected to crash
Bug 29697: Replace some missing occurrences
Bug 29697: Replace some missing occurrences
Bug 29697: Fix t/db_dependent/Exporter/Record.t
Bug 29697: Reintroduce wrongly removed import
Bug 29697: Fix t/Search.t
Bug 29697: Fix t/db_dependent/Search.t
Bug 29697: tracklinks should not explode on invalid marc record
Bug 29697: Fix t/db_dependent/Search.t
Bug 29697: WIP - fix t/db_dependent/Search.t
Bug 29697: (follow-up) Check against itemnumbers for visibility
Bug 29697: (QA follow-up) Make tests less fragile
Bug 29697: (QA follow-up) Remove useless warning

Description Jonathan Druart 2021-12-15 07:35:40 UTC
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.
Comment 1 Katrin Fischer 2021-12-17 22:20:13 UTC
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?
Comment 2 Jonathan Druart 2021-12-20 15:21:59 UTC
?(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.
Comment 3 Marcel de Rooy 2021-12-20 15:27:41 UTC
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.
Comment 4 Jonathan Druart 2022-01-25 14:55:02 UTC
Created attachment 129757 [details] [review]
Bug 29697: Special case - opac not needed
Comment 5 Jonathan Druart 2022-01-25 14:55:10 UTC
Created attachment 129758 [details] [review]
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Comment 6 Jonathan Druart 2022-01-25 14:55:19 UTC
Created attachment 129759 [details] [review]
Bug 29697: Remove GetHiddenItemnumbers
Comment 7 Jonathan Druart 2022-01-25 14:55:30 UTC
Created attachment 129760 [details] [review]
Bug 29697: Use flag embed_items
Comment 8 Jonathan Druart 2022-01-25 14:55:40 UTC
Created attachment 129761 [details] [review]
Bug 29697: Fix tests
Comment 9 Jonathan Druart 2022-01-25 14:59:34 UTC
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.
Comment 10 Tomás Cohen Arazi 2022-02-21 15:15:15 UTC
 Nice.
Comment 11 Jonathan Druart 2022-03-18 09:54:34 UTC
(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?
Comment 12 Jonathan Druart 2022-03-18 10:31:12 UTC
*** Bug 29307 has been marked as a duplicate of this bug. ***
Comment 13 Jonathan Druart 2022-03-18 10:41:45 UTC
Created attachment 131868 [details] [review]
Bug 29697: Special case - opac not needed
Comment 14 Jonathan Druart 2022-03-18 10:41:50 UTC
Created attachment 131869 [details] [review]
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Comment 15 Jonathan Druart 2022-03-18 10:41:54 UTC
Created attachment 131870 [details] [review]
Bug 29697: Remove GetHiddenItemnumbers
Comment 16 Jonathan Druart 2022-03-18 10:41:59 UTC
Created attachment 131871 [details] [review]
Bug 29697: Use flag embed_items
Comment 17 Jonathan Druart 2022-03-18 10:42:03 UTC
Created attachment 131872 [details] [review]
Bug 29697: Fix tests
Comment 18 Jonathan Druart 2022-03-18 10:42:07 UTC
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
Comment 19 Jonathan Druart 2022-03-18 10:42:22 UTC
Ready for signoff!
Comment 20 Jonathan Druart 2022-04-22 12:19:41 UTC
Can we move this one forward please?
Comment 21 Nick Clemens 2022-04-22 12:48:07 UTC
(In reply to Jonathan Druart from comment #20)
> Can we move this one forward please?

Test plan?
Comment 22 Jonathan Druart 2022-04-22 13:05:08 UTC
(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 :)
Comment 23 Tomás Cohen Arazi 2022-04-25 12:06:57 UTC
(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.
Comment 24 Jonathan Druart 2022-04-27 14:06:47 UTC
Created attachment 134066 [details] [review]
Bug 29697: Special case - opac not needed
Comment 25 Jonathan Druart 2022-04-27 14:06:54 UTC
Created attachment 134067 [details] [review]
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Comment 26 Jonathan Druart 2022-04-27 14:07:00 UTC
Created attachment 134068 [details] [review]
Bug 29697: Remove GetHiddenItemnumbers
Comment 27 Jonathan Druart 2022-04-27 14:07:06 UTC
Created attachment 134069 [details] [review]
Bug 29697: Use flag embed_items
Comment 28 Jonathan Druart 2022-04-27 14:07:12 UTC
Created attachment 134070 [details] [review]
Bug 29697: Fix tests
Comment 29 Jonathan Druart 2022-04-27 14:07:18 UTC
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
Comment 30 Martin Renvoize 2022-05-25 12:25:49 UTC
I get sha1 issues trying to apply this.. more than happy to use gitlab branches if that's easier ?
Comment 31 Jonathan Druart 2022-05-25 12:42:05 UTC
(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 32 Martin Renvoize 2022-05-25 13:37:44 UTC
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.
Comment 33 Jonathan Druart 2022-06-02 09:48:47 UTC
Created attachment 135574 [details] [review]
Bug 29697: Special case - opac not needed
Comment 34 Jonathan Druart 2022-06-02 09:48:53 UTC
Created attachment 135575 [details] [review]
Bug 29697: Replace GetMarcBiblio occurrences with $biblio->metadata->record
Comment 35 Jonathan Druart 2022-06-02 09:48:57 UTC
Created attachment 135576 [details] [review]
Bug 29697: Remove GetHiddenItemnumbers
Comment 36 Jonathan Druart 2022-06-02 09:49:03 UTC
Created attachment 135577 [details] [review]
Bug 29697: Use flag embed_items
Comment 37 Jonathan Druart 2022-06-02 09:49:09 UTC
Created attachment 135578 [details] [review]
Bug 29697: Fix tests
Comment 38 Jonathan Druart 2022-06-02 09:49:14 UTC
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
Comment 39 Jonathan Druart 2022-06-02 09:50:49 UTC
(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.
Comment 40 Jonathan Druart 2022-06-02 10:55:46 UTC
Created attachment 135580 [details] [review]
Bug 29697: (follow-up) Use flag embed_items
Comment 41 Marcel de Rooy 2022-06-03 06:35:40 UTC
QA: Looking here now
Comment 42 Marcel de Rooy 2022-06-03 07:22:24 UTC
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..
Comment 43 Marcel de Rooy 2022-06-03 07:36:54 UTC
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?
Comment 44 Marcel de Rooy 2022-06-03 07:55:33 UTC
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!
Comment 45 Jonathan Druart 2022-06-03 08:03:07 UTC
(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.
Comment 46 Jonathan Druart 2022-06-03 08:03:56 UTC
(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
Comment 47 Marcel de Rooy 2022-06-03 08:48:12 UTC
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..
Comment 48 Marcel de Rooy 2022-06-03 08:49:47 UTC
+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..
Comment 49 Marcel de Rooy 2022-06-03 08:50:17 UTC
+++ 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.
Comment 50 Marcel de Rooy 2022-06-03 08:51:13 UTC
Created attachment 135662 [details] [review]
Bug 29697: Special case - opac not needed

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 51 Marcel de Rooy 2022-06-03 08:51:18 UTC
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>
Comment 52 Marcel de Rooy 2022-06-03 08:51:23 UTC
Created attachment 135664 [details] [review]
Bug 29697: Remove GetHiddenItemnumbers

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 53 Marcel de Rooy 2022-06-03 08:51:27 UTC
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>
Comment 54 Marcel de Rooy 2022-06-03 08:51:32 UTC
Created attachment 135666 [details] [review]
Bug 29697: Fix tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 55 Marcel de Rooy 2022-06-03 08:51:36 UTC
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>
Comment 56 Marcel de Rooy 2022-06-03 08:53:49 UTC
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..
Comment 57 Marcel de Rooy 2022-06-03 09:08:05 UTC
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..
Comment 58 Marcel de Rooy 2022-06-03 09:12:53 UTC
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 });
Comment 59 Jonathan Druart 2022-06-03 09:14:39 UTC
(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.
Comment 60 Marcel de Rooy 2022-06-03 09:21:03 UTC
(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?
Comment 61 Jonathan Druart 2022-06-03 09:25:56 UTC
Created attachment 135668 [details] [review]
Bug 29697: Special case - opac not needed

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 62 Jonathan Druart 2022-06-03 09:26:03 UTC
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";
Comment 63 Jonathan Druart 2022-06-03 09:26:09 UTC
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;
Comment 64 Jonathan Druart 2022-06-03 09:26:15 UTC
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>
Comment 65 Jonathan Druart 2022-06-03 09:26:20 UTC
Created attachment 135672 [details] [review]
Bug 29697: Fix tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 66 Jonathan Druart 2022-06-03 09:26:25 UTC
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>
Comment 67 Jonathan Druart 2022-06-03 09:26:48 UTC
(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? :)
Comment 68 Jonathan Druart 2022-06-03 09:27:40 UTC
(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.
Comment 69 Tomás Cohen Arazi 2022-06-07 11:31:05 UTC
(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.
Comment 70 Nick Clemens 2022-06-07 11:34:21 UTC
(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
Comment 71 Jonathan Druart 2022-06-08 12:33:40 UTC
Created attachment 135797 [details] [review]
Bug 29697: Special case - opac not needed

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 72 Jonathan Druart 2022-06-08 12:33:45 UTC
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";
Comment 73 Jonathan Druart 2022-06-08 12:33:50 UTC
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;
Comment 74 Jonathan Druart 2022-06-08 12:33:55 UTC
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>
Comment 75 Jonathan Druart 2022-06-08 12:34:00 UTC
Created attachment 135801 [details] [review]
Bug 29697: Fix tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 76 Jonathan Druart 2022-06-08 12:34:05 UTC
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>
Comment 77 Jonathan Druart 2022-06-08 12:34:15 UTC
Patches rebased against master.
Comment 78 Joonas Kylmälä 2022-06-12 11:02:15 UTC
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
Comment 79 Katrin Fischer 2022-06-12 11:10:55 UTC
Good catch, Joonas!
Comment 80 Joonas Kylmälä 2022-06-12 11:11:36 UTC
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.
Comment 81 Jonathan Druart 2022-06-13 07:33:14 UTC
(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;
        }
Comment 82 Jonathan Druart 2022-06-13 07:33:50 UTC
Joonas, I let you switch the status back to PQA if you agree with the previous comment.
Comment 83 Jonathan Druart 2022-06-13 07:40:00 UTC
I missed the other comment anyway, so not ready for PQA.
Comment 84 Jonathan Druart 2022-06-13 07:48:45 UTC
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);
Comment 85 Jonathan Druart 2022-06-13 07:48:51 UTC
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;
Comment 86 Jonathan Druart 2022-06-13 07:48:57 UTC
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>
Comment 87 Jonathan Druart 2022-06-13 07:49:04 UTC
Created attachment 135981 [details] [review]
Bug 29697: Fix tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 88 Jonathan Druart 2022-06-13 07:49:09 UTC
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>
Comment 89 Jonathan Druart 2022-06-13 07:49:38 UTC
(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.
Comment 90 Joonas Kylmälä 2022-06-15 18:17:30 UTC
(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.
Comment 91 Joonas Kylmälä 2022-07-18 17:18:01 UTC
Doesn't apply anymore.
Comment 92 Joonas Kylmälä 2022-07-18 17:52:14 UTC
Created attachment 137835 [details] [review]
Bug 29697: Special case - opac not needed

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 93 Joonas Kylmälä 2022-07-18 17:52:19 UTC
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>
Comment 94 Joonas Kylmälä 2022-07-18 17:52:24 UTC
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;
Comment 95 Joonas Kylmälä 2022-07-18 17:52:29 UTC
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>
Comment 96 Joonas Kylmälä 2022-07-18 17:52:34 UTC
Created attachment 137839 [details] [review]
Bug 29697: Fix tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 97 Joonas Kylmälä 2022-07-18 17:52:39 UTC
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>
Comment 98 Joonas Kylmälä 2022-07-18 17:53:10 UTC
Rebased.
Comment 99 Jonathan Druart 2022-07-19 07:32:58 UTC
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.
Comment 100 Joonas Kylmälä 2022-07-19 17:02:26 UTC
(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.
Comment 101 Tomás Cohen Arazi 2022-07-19 17:23:50 UTC
(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.
Comment 102 Jonathan Druart 2022-07-20 07:04:15 UTC
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.
Comment 103 Jonathan Druart 2022-07-20 07:06:02 UTC
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.
Comment 104 Jonathan Druart 2022-07-20 12:29:15 UTC
Created attachment 137918 [details] [review]
Bug 29697: Special case - opac not needed

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 105 Jonathan Druart 2022-07-20 12:29:23 UTC
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>
Comment 106 Jonathan Druart 2022-07-20 12:29:32 UTC
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;
Comment 107 Jonathan Druart 2022-07-20 12:29:38 UTC
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>
Comment 108 Jonathan Druart 2022-07-20 12:29:43 UTC
Created attachment 137922 [details] [review]
Bug 29697: Fix tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 109 Jonathan Druart 2022-07-20 12:29:50 UTC
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>
Comment 110 Jonathan Druart 2022-07-20 12:29:55 UTC
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.
Comment 111 Jonathan Druart 2022-07-20 12:30:21 UTC
Rebased against master.
Comment 112 Joonas Kylmälä 2022-07-20 17:42:07 UTC
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>
Comment 113 Joonas Kylmälä 2022-07-20 17:44:40 UTC
(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.
Comment 114 Tomás Cohen Arazi 2022-07-22 18:55:03 UTC
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>
Comment 115 Tomás Cohen Arazi 2022-07-22 18:57:38 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 116 Jonathan Druart 2022-07-25 12:11:18 UTC
Created attachment 138076 [details] [review]
Bug 29697: Replace some missing occurrences
Comment 117 Jonathan Druart 2022-07-25 12:14:19 UTC
Created attachment 138077 [details] [review]
Bug 29697: Replace some missing occurrences
Comment 118 Jonathan Druart 2022-07-25 12:27:25 UTC
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.
Comment 119 Jonathan Druart 2022-07-25 12:29:31 UTC
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.
Comment 120 Jonathan Druart 2022-07-25 12:31:08 UTC
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.
Comment 121 Jonathan Druart 2022-07-25 12:49:58 UTC
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
Comment 122 Jonathan Druart 2022-07-25 13:10:14 UTC
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.
Comment 123 Tomás Cohen Arazi 2022-07-25 13:17:22 UTC
Created attachment 138090 [details] [review]
Bug 29697: Fix t/db_dependent/Search.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 124 Jonathan Druart 2022-07-25 13:20:16 UTC
(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!
Comment 125 Tomás Cohen Arazi 2022-07-25 13:21:06 UTC
Follow-ups pushed to master. We owe you some beers Jonathan. Thanks!
Comment 126 Jonathan Druart 2022-07-25 13:42:39 UTC
(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 ;)
Comment 127 Jonathan Druart 2022-08-08 13:31:00 UTC
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.
Comment 128 Jonathan Druart 2022-08-08 13:33:23 UTC
Created attachment 138804 [details] [review]
Bug 29697: WIP - fix t/db_dependent/Search.t
Comment 129 Jonathan Druart 2022-08-08 13:37:00 UTC
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.
Comment 130 Jonathan Druart 2022-08-08 13:40:20 UTC
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!
Comment 131 Jonathan Druart 2022-08-08 13:40:38 UTC
(those tests are a mess!)
Comment 132 Nick Clemens 2022-08-15 12:27:51 UTC
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
Comment 133 Andrii Nugged 2022-08-28 15:47:14 UTC
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 134 Tomás Cohen Arazi 2022-08-31 14:56:31 UTC
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.
Comment 135 Tomás Cohen Arazi 2022-08-31 15:35:38 UTC
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>
Comment 136 Tomás Cohen Arazi 2022-08-31 15:35:46 UTC
Created attachment 140008 [details] [review]
Bug 29697: (QA follow-up) Remove useless warning

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 137 Lucas Gass 2022-09-02 18:03:49 UTC
Enhancement will not be backported to 22.05.x series