|
Description
Olli-Antti Kivilahti
2015-05-28 08:38:40 UTC
Created attachment 39625 [details] [review] Bug 14287 - Preserve valid ISBN-13 identifiers if they start with something else than 978 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. Use Buug 13167's TEST PLAN. 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 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. Created attachment 39930 [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 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. 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> (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. 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. 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. Please provide a test plan. NormalizeISBN is used in some places and it's hard to imagine the side effects this patch could introduce. 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. 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. (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? (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. Hi all, I think this is becoming a problem now - we have a library getting covers through Syndetics and records with ^979 ISBNs are not fetching covers successfully. Any progress on this work? (In reply to Aleisha Amohia from comment #15) > Hi all, I think this is becoming a problem now - we have a library getting > covers through Syndetics and records with ^979 ISBNs are not fetching covers > successfully. > > Any progress on this work? I believe Mark and Olli-Antti are no longer around, so it might wait for someone to adopt and push it further. |