Bug 14287 - Preserve valid ISBN-13 identifiers if they start with something else than 978
Summary: Preserve valid ISBN-13 identifiers if they start with something else than 978
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: M. Tompsett
QA Contact: Testopia
URL:
Keywords:
Depends on: 13167
Blocks:
  Show dependency treegraph
 
Reported: 2015-05-28 08:38 UTC by Olli-Antti Kivilahti
Modified: 2019-07-04 19:28 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14287 - Preserve valid ISBN-13 identifiers if they start with something else than 978 (2.24 KB, patch)
2015-05-28 08:55 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 14287: counter-patch to preserve valid ISBN-13 not starting with 978 (1.98 KB, patch)
2015-06-06 00:36 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14287: counter-patch to preserve valid ISBN-13 not starting with 978 (2.06 KB, patch)
2015-06-14 15:11 UTC, Indranil Das Gupta
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2015-05-28 08:38:40 UTC
If Business::ISBN cannot make an ISBN-10 out of a valid ISBN-13 starting with 979, then we should preserve the ISBN-13 instead of getting rid of it.
ISBN-13 starting with 979 is valid as well and in a few years starting with 980 might be as well as the ISBN supply is running out.

This patch modifies the C4::Koha::NormalizeISBN() to preserve the original ISBN if type conversion fails.

Also Koha should support all ISBN-variants interchangeably, see Bug 10192.
Some Items have ISBN-10 from some sources and the exact same Item can have ISBN-13 from other valid sources.
Maybe the easiest solution is to migrate all current and future imported ISBNs to ISBN-13 -variant since according to the same wikipedia article, the conversion is quite easy.
Comment 1 Olli-Antti Kivilahti 2015-05-28 08:55:25 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2015-05-28 08:56:34 UTC
Left a warning to warn if the conversion failed. This can be removed from the patch. I am just curious on how often this problem actually happens.
Comment 3 M. Tompsett 2015-06-06 00:19:40 UTC
Comment on attachment 39625 [details] [review]
Bug 14287 - Preserve valid ISBN-13 identifiers if they start with something else than 978

Review of attachment 39625 [details] [review]:
-----------------------------------------------------------------

::: C4/Koha.pm
@@ +1725,4 @@
>          elsif ( $format eq 'ISBN-13' ) {
>              $isbn = $isbn->as_isbn13();
>          }
> +        unless ($isbn) { #If we cannot force the ISBN to another type, atleast prevent Koha from crashing by recovering some ISBN.

unless is hard to read, and if (!$isbn) is shorter.
Though, I don't think then as_isbn13() will return undef.
Only the as_isbn10() will:
"If it is an ISBN-13 with the prefix 978, it returns the ISBN-10 equivalent. For all other cases it returns undef." (CPAN page)
So, why not:
            my $isbn10 = $isbn->as_isbn10();
            $isbn = $isbn10 if $isbn10;
in the ISBN-10 check?

@@ +1725,5 @@
>          elsif ( $format eq 'ISBN-13' ) {
>              $isbn = $isbn->as_isbn13();
>          }
> +        unless ($isbn) { #If we cannot force the ISBN to another type, atleast prevent Koha from crashing by recovering some ISBN.
> +            $isbn = Business::ISBN->new($string); #If we fail to force the ISBN to the given format, revert to the format we can infer.

Why rebuild something when we could simplify as I suggested above?

@@ +1726,5 @@
>              $isbn = $isbn->as_isbn13();
>          }
> +        unless ($isbn) { #If we cannot force the ISBN to another type, atleast prevent Koha from crashing by recovering some ISBN.
> +            $isbn = Business::ISBN->new($string); #If we fail to force the ISBN to the given format, revert to the format we can infer.
> +            warn "C4::Koha::NormalizeISBN():> Couldn't change ISBN '$string' to type '$format'. Using the inferred type '".$isbn->type()."' instead.";

Do we need to flood the error logs? Reports could be used to find those ISBN-13 entries with non-978 beginnings.
Comment 4 M. Tompsett 2015-06-06 00:36:58 UTC Comment hidden (obsolete)
Comment 5 M. Tompsett 2015-06-06 00:38:17 UTC
Sign off either the patch or counter-patch, and obsolete the other. See comment #3 as to why my counter patch is better in my opinion.
Comment 6 Indranil Das Gupta 2015-06-14 15:11:23 UTC
Created attachment 40126 [details] [review]
Bug 14287: counter-patch to preserve valid ISBN-13 not starting with 978

Since the issue is for ISBN-13 to ISBN-10 of non-978 ISBN-13 values,
this patch only modifies the ISBN-10 case. Additionally, since
ISBN-13 always returns a value, this patch means the return unless
is no longer needed. Flooding the error log seems unnecessary.

TEST PLAN
---------
1) prove -v t/Koha.t
2) apply patch
3) prove -v t/Koha.t
   -- note that test 17 has been changed.
4) run koha qa test tools

Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>
Comment 7 Indranil Das Gupta 2015-06-14 15:12:05 UTC
(In reply to M. Tompsett from comment #5)
> Sign off either the patch or counter-patch, and obsolete the other. See
> comment #3 as to why my counter patch is better in my opinion.

this is a cleaner implementation. so signing off on this one.
Comment 8 Jonathan Druart 2015-06-18 10:01:06 UTC
Not sure to understand:
We have a subroutine to return an isbn in a given format.
If there is no isbn for this format, the subroutine should return something to tell "it does not exist" (undef here).
Your patch suggests to return another format, IMO it's not relevant.

There are 2 other subroutines: GetVariationsOfISBN and GetVariationsOfISBNs, maybe it's more what you want.
Comment 9 Olli-Antti Kivilahti 2015-06-22 08:08:38 UTC
Jonathan. The problem is that this subroutine is used by the MARC import scripts.

The subroutine is called "NormalizeISBN", so I would expect it to normalize any ISBN to the one Koha uses internally, which should be something else than ISBN10 for numberspace exhaustion reasons. I think we are already seeing duplicate issues with ISBN's (if memory serves me),
so we should have the ISBN-13 as the new normal.
Comment 10 Jonathan Druart 2015-06-23 10:26:11 UTC
Please provide a test plan.

NormalizeISBN is used in some places and it's hard to imagine the side effects this patch could introduce.
Comment 11 Fred P 2017-03-28 17:46:51 UTC
In the United States, ISBN's starting with 979 are not yet introduced, however material in France, Europe and other countries are already rolling out 979 ISBN's. Sadly, they do not play well with Koha scripts. Let's hope we can find a solution before these codes become more prevalent.
Comment 12 Fred P 2017-03-28 20:52:22 UTC
After discussing it in the Koha forum, I believe my problem is with a specific 979 code that doesn't validate or a specific malformed record. I'm on 16.04, by 16.05 the problem appears to be resolved. Please ignore my previous message.
Comment 13 M. Tompsett 2019-07-03 01:47:11 UTC
(In reply to Jonathan Druart from comment #10)
> Please provide a test plan.
> 
> NormalizeISBN is used in some places and it's hard to imagine the side
> effects this patch could introduce.

Actually, GetNormalizedISBN is what is more broadly used. It calls _isbn_cleanup which calls NormalizeISBN.

It would also seem that this is used to grab book covers using the ISBN-10. Take Amazon for example.

https://www.amazon.com/Tolkiens-Failed-Quest-Michael-Jones/dp/1521406812/
That's an ISBN-10.

https://www.amazon.com/Apprenez-%C3%A0-programmer-2e-%C3%A9dition/dp/B006MV70GS/
That's an ASIN which is Amazon's own unique ID, because it is ISBN-13 only:
979-1090085008

But what about "The Hobbit"?
ISBN-13: 9780547928227
ISBN-10: 054792822X
https://www.amazon.com/Hobbit-J-R-Tolkien/dp/054792822X/

So, how are we supposed to grab Amazon covers?
Comment 14 M. Tompsett 2019-07-04 19:28:04 UTC
(In reply to M. Tompsett from comment #13)
> So, how are we supposed to grab Amazon covers?

Related to this whole mess take a look at:
line 389 of koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt

The lack of normalized_isbn (13 to 10 makes it empty or undef, not sure which)
means that it knows not to try to get the book cover.

Look what I get when I use the ASIN? :)
https://images-na.ssl-images-amazon.com/images/P/B006MV70GS

Just some further thoughts.