Bug 30009 - Records with invalid MarcXML show notes tab 'Descriptions(1)' but tab is empty
Summary: Records with invalid MarcXML show notes tab 'Descriptions(1)' but tab is empty
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 29690
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-02 15:13 UTC by Nick Clemens
Modified: 2022-10-14 19:58 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 30009: If no record set MARCNOTES undef, rather than empty array (1.22 KB, patch)
2022-03-18 15:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30009: If no record set MARCNOTES undef, rather than empty array (1.26 KB, patch)
2022-05-15 13:21 UTC, David Nind
Details | Diff | Splinter Review
Bug 30009: Do not set MARCNOTES to empty list in list context (1.28 KB, patch)
2022-05-18 11:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2022-02-02 15:13:42 UTC
See bug 29690 - find/create a record with invalid marc
Notes tab shows but is empty
Comment 1 Jonathan Druart 2022-03-17 15:24:02 UTC
That's expected, there is a warning "the view may be degraded".
This is not a bug IMO.
Comment 2 Nick Clemens 2022-03-18 15:05:29 UTC
Created attachment 131917 [details] [review]
Bug 30009: If no record set MARCNOTES undef, rather than empty array

Currently we pass an empty array if there is no marc record - this
is read as 'true' by template toolkit, and the desciptions tab is loaded

This patch just sets the variable as undef which is false

To test:
1 - Find or create an invalid marc record
2 - View in staff client
3 - Note tab Descriptions(1) has no content
4 - Apply patch, reload
5 - No empty tab
Comment 3 Nick Clemens 2022-03-18 15:06:03 UTC
(In reply to Jonathan Druart from comment #1)
> That's expected, there is a warning "the view may be degraded".
> This is not a bug IMO.

Sure, but problem is what we are passing, we should instead pass nothing
Comment 4 Owen Leonard 2022-03-18 17:22:22 UTC
How does one create an invalid MARC record?
Comment 5 Jonathan Druart 2022-03-21 10:31:35 UTC
(In reply to Owen Leonard from comment #4)
> How does one create an invalid MARC record?

Record 369 in KTD's database has an invalid MARC::Record.
Otherwise you can edit a biblio reuse step 3 from Nick's test plan from bug 30008:

3 - In the 520 notes field, add an ASCII escape character (27 decimal, 1b hex)
    On ubuntu Ctrl+Shift+u, then 1b, enter

The character for copy/paste is: 
Comment 6 Jonathan Druart 2022-03-21 10:32:14 UTC
(In reply to Jonathan Druart from comment #5)
> The character for copy/paste is:

It got removed.
Comment 7 David Nind 2022-05-15 13:21:06 UTC
Created attachment 135008 [details] [review]
Bug 30009: If no record set MARCNOTES undef, rather than empty array

Currently we pass an empty array if there is no marc record - this
is read as 'true' by template toolkit, and the desciptions tab is loaded

This patch just sets the variable as undef which is false

To test:
1 - Find or create an invalid marc record
2 - View in staff client
3 - Note tab Descriptions(1) has no content
4 - Apply patch, reload
5 - No empty tab

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2022-05-15 13:40:38 UTC
I've signed off as things work as per the test plan when using KTD and record 369 in the test data.

Note that the 520 filed for record 369 has a valid value, and that it is other missing tags making it invalid (as far as I can see). It displays correctly once the patch is applied.

Also, if you try to view the record in the OPAC you get an error trace:

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
  <controlfield tag="001">00aD000015937</controlfield>
                            ^
:9: parser error : PCDATA invalid Char value 31
  <controlfield tag="004">00satmrnu0</controlfield>
                            ^
:9: parser error : PCDATA invalid Char value 31
  <controlfield tag="004">00satmrnu0</controlfield>
                               ^
:9: parser error : PCDATA invalid Char value 31
  <controlfield tag="004">00satmrnu0</controlfield>
                                  ^
:9: parser error : PCDATA invalid Char value 31
  <controlfield tag="004">00satmrnu0</controlfield>
                                     ^
:10: parser error : PCDATA invalid Char value 31
  <controlfield tag="008">00ar19881981bdkldan</controlfield>
                            ^
:10: parser error : PCDATA invalid Char value 31
  <controlfield tag="008">00ar19881981bdkldan</controlfield>
                                       ^
:10: parser error : PCDATA invalid Char value 31
  <controlfield tag="008">00ar19881981bdkldan</controlfield>
                                           ^') at /usr/share/perl5/Exception/Class/Base.pm line 88

    in Exception::Class::Base::throw at /usr/share/perl5/Exception/Class/Base.pm line 88
Comment 9 Marcel de Rooy 2022-05-18 11:37:58 UTC
No assignee?
Comment 10 Marcel de Rooy 2022-05-18 11:44:01 UTC
(In reply to Nick Clemens from comment #2)
> Currently we pass an empty array if there is no marc record - this
> is read as 'true' by template toolkit, and the desciptions tab is loaded
> 
> This patch just sets the variable as undef which is false

Just being more precise:

We pass an empty list. This is not read as true, as the above commit message tells. But it creates the odd arguments hash where the next key shifts into the position of the MARCNOTES value.
Comment 11 Marcel de Rooy 2022-05-18 11:51:06 UTC
Created attachment 135101 [details] [review]
Bug 30009: Do not set MARCNOTES to empty list in list context

AMENDED:
We should not pass an empty list here, but just undef in order to
prevent creating a shift of elements in the hash.

To test:
1 - Find or create an invalid marc record
2 - View in staff client
3 - Note tab Descriptions(1) has no content
4 - Apply patch, reload
5 - No empty tab

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Fridolin Somers 2022-05-19 03:22:52 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄