Bug 13167 - Stage MARC for Import hangs for biblio containing valid 979* ISBN-13
Summary: Stage MARC for Import hangs for biblio containing valid 979* ISBN-13
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Frédéric Demians
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 13158 13641 (view as bug list)
Depends on:
Blocks: 14287
  Show dependency treegraph
 
Reported: 2014-10-30 11:51 UTC by Tiburce Euphrasie
Modified: 2016-06-21 21:37 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Zip contains 2 UNIMARC ISO-8859-1 records + a scan of the ISBN on the original book (9.80 KB, application/zip)
2014-10-30 11:51 UTC, Tiburce Euphrasie
Details
Bug 13167 Stage MARC for Import hangs for biblio containing invalid ISBN-13 (1.36 KB, patch)
2014-11-02 16:34 UTC, Frédéric Demians
Details | Diff | Splinter Review
Signed Off patch (1.42 KB, patch)
2014-12-15 12:36 UTC, Colin Campbell
Details | Diff | Splinter Review
[PASSED QA] Bug 13167 Stage MARC for Import hangs for biblio containing invalid ISBN-13 (1.57 KB, patch)
2014-12-27 10:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13167: (QA followup) regression tests (1.89 KB, patch)
2015-01-04 14:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13167 Stage MARC for Import hangs for biblio containing invalid ISBN-13 (1.61 KB, patch)
2015-01-04 14:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tiburce Euphrasie 2014-10-30 11:51:48 UTC
Created attachment 32969 [details]
Zip contains 2 UNIMARC ISO-8859-1 records + a scan of the ISBN on the original book

Aloha,
I have detected un problem while trying to import a large set of UNIMARC records.
I tried with a Koha 3.16.01 and a Koha 3.16.03, same issue :

If the ISBN of a UNIMARC record begins with 979 then the 'Stage MARC for import' hangs.
If I use the same UNIMARC record and change 979 to 978 in the ISBN, 'Stage MARC for import' works perfectly.

You can find 2 UNIMARC Records (ISO8859-1) in the attachment if you wish to make tests by yourself (same records, one with ISBN 979-..., one with ISBN 978-...).

I think I've also detected a small problem in cataloguing::value_builder::unimarc_field_010 :
[...] Line 94 to 96 :
    if (length ($isbn) == 13){
        $isbn = substr($isbn, 3, length($isbn)-3);
    }
[...] Then, line 143 :
        $sth->execute($seg2, "978$seg2");

This "978" on line 143 is hard-coded and there's no relation with the original data inside $isbn. I don't know if this is a real problem but it might be better to keep the original data than replacing it with "978".
Aloha.
Comment 1 Frédéric Demians 2014-11-02 16:34:38 UTC Comment hidden (obsolete)
Comment 2 Frédéric Demians 2014-11-02 16:36:25 UTC
(In reply to Tiburce Euphrasie from comment #0)

> I think I've also detected a small problem in

Open a separate bug report.
Comment 3 Frédéric Demians 2014-11-02 16:43:30 UTC
*** Bug 13158 has been marked as a duplicate of this bug. ***
Comment 4 Tiburce Euphrasie 2014-11-03 19:45:34 UTC
(In reply to Frédéric Demians from comment #2)
> (In reply to Tiburce Euphrasie from comment #0)
> 
> > I think I've also detected a small problem in
> 
> Open a separate bug report.

Aloha,
Done.
See bug 13192 :
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13192
Aloha.
Comment 5 Tiburce Euphrasie 2014-11-03 20:19:37 UTC
(In reply to Frédéric Demians from comment #1)
> Created attachment 33130 [details] [review] [review]
> Bug 13167 Stage MARC for Import hangs for biblio containing invalid ISBN-13
> 
> If the ISBN of a UNIMARC record begins with 979 then the 'Stage MARC for
> import' hangs.  If I use the same UNIMARC record and change 979 to 978 in the
> ISBN, 'Stage MARC for import' works perfectly.
> 
> The patch deals with the fact that converting an ISBN-13 to ISBN-10 with
> Business::ISBN as_isbn10() method fails if the ISBN doesn't begin with 978.

Aloha Frédéric Demians,
Adding 'return unless $isbn;' on line 1683 in Koha.pm means that the sub NormalizeISBN will return nothing for the specific ISBN sample I've provided.

This is not a proper way to fix this bug because that patch implies that all those '979.*' ISBN simply don't exist.

Bu the way, the initial summary for this bug was "Stage MARC for Import hangs", a mistake was done when changing this summary to "Stage MARC for Import hangs for biblio containing invalid ISBN-13", so I'll change it to "Stage MARC for Import hangs for biblio containing valid 979* ISBN-13" after redacting this comment.

This ISBN is valid :
- there's a scan from the original book in the attachment file 32969 :
http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32969

- There's some documentation available this kind of ISBN :
http://www.afnil.org/
http://fr.wikipedia.org/wiki/International_Standard_Book_Number
Aloha.
Comment 6 Frédéric Demians 2014-11-03 21:03:37 UTC
(In reply to Tiburce Euphrasie from comment #5)

Aloha!

> Aloha Frédéric Demians,
> Adding 'return unless $isbn;' on line 1683 in Koha.pm means that the sub
> NormalizeISBN will return nothing for the specific ISBN sample I've provided.
> 
> This is not a proper way to fix this bug because that patch implies that all
> those '979.*' ISBN simply don't exist.

Your assumption is not correct. The function can't transform your ISBN-13 into
ISBN-10, because it begins with '979'. Rather than returning undef to say that
the conversion is not possible, the function was failing badly, stopping
completely the Perl process. That's what is corrected by the patch.

Have you tried the patch? Does it solve the bug reported here? Can you import
your record? Is your ISBN field preserved? For both records?

The Koha internal function NormalizeISBN is called from other part of the Koha
code base (see bug 13158). When the action requested to the function could not
be completed, the function is supposed to return undef (nothing). That's
exactly what'is done with the patch.

The import process call NormalizeISBN function for whatever reason. I can't say
why, indeed. And for doing nothing with the result, since ISBN field is not
modified during import. But this is another story, and can't be fixed here.

> Bu the way, the initial summary for this bug was "Stage MARC for Import
> hangs", a mistake was done when changing this summary to "Stage MARC for
> Import hangs for biblio containing invalid ISBN-13", so I'll change it to
> "Stage MARC for Import hangs for biblio containing valid 979* ISBN-13" after
> redacting this comment.

You're correct. That's a valid ISBN-13 which can't be converted into ISBN-10.
Comment 7 Tiburce Euphrasie 2014-11-11 06:16:28 UTC
(In reply to Frédéric Demians from comment #6)
> (In reply to Tiburce Euphrasie from comment #5)
> 
> Aloha!
[...]
> Your assumption is not correct. The function can't transform your ISBN-13
> into
> ISBN-10, because it begins with '979'. Rather than returning undef to say
> that
> the conversion is not possible, the function was failing badly, stopping
> completely the Perl process. That's what is corrected by the patch.
> 
> Have you tried the patch? Does it solve the bug reported here? Can you import
> your record? Is your ISBN field preserved? For both records?
[...]

Aloha,
Yes, it works, I tried it on a Koha 3.16.03 and there's no problem : I can import both records, I can find them and the ISBN are preserved.

Thank you again for patching this bug, I can't test it on the master branch for now but it works perfectly for me.
Aloha.
Comment 8 Frédéric Demians 2014-11-14 08:35:15 UTC
(In reply to Tiburce Euphrasie from comment #7)

> Aloha,
> Yes, it works, I tried it on a Koha 3.16.03 and there's no problem : I can
> import both records, I can find them and the ISBN are preserved.

Thanks for confirming it works for you. It will help to get this patch back to Koha main code repository.
Comment 9 Colin Campbell 2014-12-15 12:36:43 UTC Comment hidden (obsolete)
Comment 10 Nick Clemens 2014-12-26 22:01:34 UTC
Marking as signed off, it looks like that was Colin's intention
Comment 11 Katrin Fischer 2014-12-27 10:38:15 UTC Comment hidden (obsolete)
Comment 12 Colin Campbell 2014-12-31 10:21:46 UTC
(In reply to Nick Clemens from comment #10)
> Marking as signed off, it looks like that was Colin's intention

It was indeed. Just adding that this is not UNIMARC specific ouurs im marc21 too
Comment 13 Tomás Cohen Arazi 2015-01-04 14:16:11 UTC
Created attachment 34916 [details] [review]
Bug 13167: (QA followup) regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 14 Tomás Cohen Arazi 2015-01-04 14:16:46 UTC
Created attachment 34917 [details] [review]
Bug 13167 Stage MARC for Import hangs for biblio containing invalid ISBN-13

If the ISBN of a UNIMARC record begins with 979 then the 'Stage MARC for
import' hangs.  If I use the same UNIMARC record and change 979 to 978 in the
ISBN, 'Stage MARC for import' works perfectly.

The patch deals with the fact that converting an ISBN-13 to ISBN-10 with
Business::ISBN as_isbn10() method fails if the ISBN doesn't begin with 978.

TEST PLAN:

(1) Download, and decompress the ZIP file attached to BZ.
(2) On a UNIMARC Koha instance, go in Tools > Stage MARC for import.
(3) Choose the MARC file containing the record with an ISBN begining with 979.
    Click on Upload file, then Stage to import.
(4) The Job progress bar stay at 0%.
(5) Apply the patch. Repeat steps 2-3. The upload works.

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Tested in a UNIMARC installation, confirmed that the patch fixes the
problem.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 15 Tomás Cohen Arazi 2015-01-04 15:20:09 UTC
Patch pushed to master.

Thanks Frederic
Comment 16 Chris Cormack 2015-01-05 23:03:38 UTC
Pushed to 3.18.x, will be in 3.18.3
Comment 17 Mason James 2015-01-16 20:53:31 UTC
Pushed to 3.16.x, will be in 3.16.7
Comment 18 Fridolin SOMERS 2015-01-23 13:22:21 UTC
Not needed for 3.14.x.
I set as resolved
Comment 19 Kyle M Hall 2015-01-29 20:11:01 UTC
*** Bug 13641 has been marked as a duplicate of this bug. ***
Comment 20 Olli-Antti Kivilahti 2015-05-28 08:39:05 UTC
Hi there!

I think this solution is not a good one. I think the ISBNs starting with 979 are also valid.
This solution gets rid of valid ISBNs if they are of the new numberspace 979, see the wikipedia article:
https://en.wikipedia.org/wiki/International_Standard_Book_Number#EAN_format_used_in_barcodes.2C_and_upgrading

It says that we are "moving to"/"using" 979 ISBNs as well.
Certainly we are getting such ISBNs from our vendors.

See Bug 14287 for a follow-up
Comment 21 Katrin Fischer 2015-05-31 12:37:46 UTC
Hi Olli, best is to file a new bug here, as this is marked RESOLVED FIXED and so already off the radar. Please link the new bug to this one. Thx!