Bug 25913 - Internal server error when calling get_coins on record with no title (245) but with 880 linked to 245
Summary: Internal server error when calling get_coins on record with no title (245) bu...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-07-02 12:01 UTC by Nick Clemens
Modified: 2020-10-29 17:05 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Manual merge request:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.05, 19.11.11


Attachments
A bib record with no 245 and 880 fields from our catalog (2.16 KB, text/plain)
2020-07-02 14:50 UTC, Heather
Details
Bug 25913: Prevent get_coins to crash if record does not have title (2.83 KB, patch)
2020-07-30 08:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25913: Prevent get_coins to crash if record does not have title (2.89 KB, patch)
2020-09-29 21:13 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25913: Prevent get_coins to crash if record does not have title (3.04 KB, patch)
2020-10-02 13:19 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25913: Get the correct return of AddBiblio (1.07 KB, patch)
2020-10-08 08:59 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-07-02 12:01:15 UTC
In opac search results we call get_coins in an eval so it can load, on other scripts we don't

The get_coins routine calls:
$title = $record->field('245')->as_string('ab');

With no check that the 245 exists.

Simple fix would be to call get_coins in an eval everywhere
More comprehensive would be to fall back to 880 field linked ot 245, and then fallback to blank

This only affects MARC21 I believe as the UNIMARC implementation gets the field directly without as_string

To recreate:
1 - Load attached record or create a record with 880 and no 245
2 - Enable COinSinOpac
3 - Search for record in staff or opac, it loads in results
4 - Add item to a public list, cannot view in opac
5 - Try to view ISBD or MARC tab on staff client, it fails

When indexing in ES we convert an 880 to its linked field so the record is searchable, so we could make that a sbroutine
Comment 1 Heather 2020-07-02 14:50:00 UTC
Created attachment 106476 [details]
A bib record with no 245 and 880 fields from our catalog

I'm uploading one of our MARC21 records, exported from OCLC that has no 245 and has the 880 fields in it that might be useful for folks to use in testing.
Comment 2 Jonathan Druart 2020-07-30 08:44:22 UTC
Created attachment 107563 [details] [review]
Bug 25913: Prevent get_coins to crash if record does not have title

If a bibliographic record does not have a title, get_coins will crash
with
Can't call method "as_string" on an undefined value at /kohadevbox/koha/Koha/Biblio.pm line 645
Koha::Biblio::get_coins('Koha::Biblio=HASH(0x5558f91bb740)') called at /kohadevbox/koha/catalogue/ISBDdetail.pl line 144

We can handle that situation easily by checking the existence of the
title field.

Test plan:
1. Create a record without 245
2. Enable COinSinOpac
4. Go to the ISBD detail view
=> It must not fail with this patch applied
Comment 3 Jonathan Druart 2020-07-30 08:45:28 UTC
(In reply to Nick Clemens from comment #0)
> In opac search results we call get_coins in an eval so it can load, on other
> scripts we don't

We surround it by an eval in detail.pl to deal with encoding problems, what we don't on other views.
I think this patch deals correctly with the problem you reported.
Comment 4 Jonathan Druart 2020-07-30 08:47:02 UTC
Nick, don't forget to copy the error message when a 500 is reported :)
Comment 5 Heather 2020-09-29 21:12:58 UTC
I'm going to sign off--everything worked according to the test plan when I tried to sign off, and the ISBD view didn't fail, but it also didn't include any data from the 880 10 $6 245-00, i.e.:
880 	1 	0 	_6245-00
_aSS President Coolidge = [Президент Кулидж] :

So the "Normal" view looks like this:
SS President Coolidge = [Президент Кулидж] : Под водой с Вадим Малыш /
By: Malysh, Vadim [filmmaker.] 
...etc.

But the ISBD view like this:
Malysh, Vadim,

- 1 videodisc (11:41 min.) : sound, color with black and white sequences ; 4 3/4 in.

TEST RECORD.--h2 Title from disc.

Narrator, Nikita Puryzhinsky.

DVD.

In Russian with English subtitles.

Subjects--Corporate Names:
President Coolidge (Steamship)

Subjects--Topical Terms:
Deep diving--Vanuatu--Espíritu Santo Island.
Shipwrecks--Vanuatu--Espíritu Santo Island.

LC Class. No.: VK1255.P703 / M35 2009 

So perhaps getting that 880 10 $6 245-00 $a data into the ISBD view should be a separate bug?  Because the ISBD view isn't very useful without the title!:)
Comment 6 ByWater Sandboxes 2020-09-29 21:13:59 UTC
Created attachment 110941 [details] [review]
Bug 25913: Prevent get_coins to crash if record does not have title

If a bibliographic record does not have a title, get_coins will crash
with
Can't call method "as_string" on an undefined value at /kohadevbox/koha/Koha/Biblio.pm line 645
Koha::Biblio::get_coins('Koha::Biblio=HASH(0x5558f91bb740)') called at /kohadevbox/koha/catalogue/ISBDdetail.pl line 144

We can handle that situation easily by checking the existence of the
title field.

Test plan:
1. Create a record without 245
2. Enable COinSinOpac
4. Go to the ISBD detail view
=> It must not fail with this patch applied

Signed-off-by: Heather Hernandez <heather_hernandez@nps.gov>
Comment 7 Joonas Kylmälä 2020-10-02 13:19:31 UTC
Created attachment 111153 [details] [review]
Bug 25913: Prevent get_coins to crash if record does not have title

If a bibliographic record does not have a title, get_coins will crash
with
Can't call method "as_string" on an undefined value at /kohadevbox/koha/Koha/Biblio.pm line 645
Koha::Biblio::get_coins('Koha::Biblio=HASH(0x5558f91bb740)') called at /kohadevbox/koha/catalogue/ISBDdetail.pl line 144

We can handle that situation easily by checking the existence of the
title field.

Test plan:
1. Create a record without 245
2. Enable COinSinOpac
4. Go to the ISBD detail view
=> It must not fail with this patch applied

Signed-off-by: Heather Hernandez <heather_hernandez@nps.gov>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 8 Jonathan Druart 2020-10-08 08:59:54 UTC
Created attachment 111366 [details] [review]
Bug 25913: Get the correct return of AddBiblio

We need the parenthesis to get the biblionumber and not the
biblioitemnumber, in case there is a discrepancy.
Comment 9 Jonathan Druart 2020-10-08 09:37:48 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 10 Lucas Gass 2020-10-20 20:23:44 UTC
backported to 20.05.x for 20.05.05
Comment 11 Aleisha Amohia 2020-10-27 04:36:32 UTC
backported to 19.11.x for 19.11.11
Comment 12 Victor Grousset/tuxayo 2020-10-29 17:05:10 UTC
Not backported to oldoldstable (19.05.x). Feel free to ask if it's needed.