Bug 4305 - Amazon book covers do not work with ISBN13
Summary: Amazon book covers do not work with ISBN13
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: PATCH-Sent (DO NOT USE) critical (vote)
Target Milestone: Oldversions
Assignee: Frédéric Demians
QA Contact: Bugs List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-08 03:45 UTC by Owen Leonard
Modified: 2018-12-16 19:34 UTC (History)
9 users (show)

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


Attachments
Original patch to bug 4305, with signoff (2.86 KB, patch)
2010-10-06 17:00 UTC, Ian Walls
Details | Diff | Splinter Review
Followup patch to bug 4305 (1.10 KB, patch)
2010-10-06 17:01 UTC, Ian Walls
Details | Diff | Splinter Review
Prevents exception on certain dodgy input (705 bytes, patch)
2010-10-20 06:46 UTC, Robin Sheat
Details | Diff | Splinter Review
Further Patch (1.28 KB, patch)
2010-11-11 14:05 UTC, Colin Campbell
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:25:36 UTC


---- Reported by oleonard@myacpl.org 2010-03-08 15:45:08 ----

If your MARC record lists both an ISBN10 and an ISBN13, the ISBN10 must be first in order for Amazon book covers to work. If the ISBN13 is first, no cover image.



---- Additional Comments From colin.campbell@ptfs-europe.com 2010-03-08 20:33:58 ----

Amazon identifies the book cover by its own 10 digit id. Fortunately these usually correspond to 10 digit ISBNs but the correspondence isnt guaranteed. The Amazon API lets you get the id from an isbn (10 or 13) but that would entail calling amazon to get the value to call amazon for the image



---- Additional Comments From nengard@gmail.com 2010-03-08 21:34:58 ----

I can't find the bug report, but someone was working on making this work - I thought.



---- Additional Comments From nengard@gmail.com 2010-05-04 15:35:36 ----

This is becoming a bigger issue as more and more books are coming out without an ISBN 10.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:25 UTC  ---

This bug was previously known as _bug_ 4305 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4305

Actual time not defined. Setting to 0.0
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.

Comment 1 Owen Leonard 2010-08-02 16:17:20 UTC
The patch is here:

http://lists.koha-community.org/pipermail/koha-patches/2010-July/012346.html

And here are the comments sent with the patch:

"The way Koha get book cover from Amazon doesn't work from biblio records
having ISBN13 because Amazon expect an ISBN10. This patch transforms
ISBN13 into ISBN10 before sending it to Amazon.

Warning! On some devel installations, you will have to install by hand a
new CPAN depency:

  Business::ISBN"

Assigning this bug to Frédéric Demians and marking it as "Assigned."
Comment 2 Owen Leonard 2010-08-02 16:19:14 UTC
Tested on HEAD and it seems to work well
Comment 3 Ian Walls 2010-10-06 16:43:32 UTC
The patch for bug 4305 does not address the case where the ISBN passed to GetNormalizedISBN is coming from biblioitems.isbn, and contains " | " and the beginning of a second ISBN.

If multiple ISBN fields are present in the MARC record on import, Koha attempts to store all of them in the biblioitems.isbn field, up to the field's 30 character limit.  Business::ISBN cannot handle this, and will throw an error.  This comes up, in one important case, when a patron attempts to log into their account in the OPAC.  The function to show their currently checked-out items pulls the ISBN from the biblioitems table, and if they've checked out a material with multiple ISBNs in the record, they'll see the error rather than their account.

The simple solution is to scrub any ISBN coming into GetNormalizedISBN for the " | " delimiter.  A more robust solution will involve preventing Koha from attempting to save multiple ISBNs into a single database field.
Comment 4 Ian Walls 2010-10-06 17:00:07 UTC
Created attachment 2669 [details] [review]
Original patch to bug 4305, with signoff
Comment 5 Ian Walls 2010-10-06 17:01:13 UTC
Created attachment 2670 [details] [review]
Followup patch to bug 4305

Builds on previous attached patch (which was committed to Koha on 10-6-10).  Fixes issue mentioned in previous comment by Ian Walls, same date.
Comment 6 Frédéric Demians 2010-10-06 17:01:37 UTC
> The patch for bug 4305 does not address the case where the ISBN passed to
> GetNormalizedISBN is coming from biblioitems.isbn, and contains " | " and the
> beginning of a second ISBN.

Was it working before this patch?

> If multiple ISBN fields are present in the MARC record on import, Koha attempts
> to store all of them in the biblioitems.isbn field, up to the field's 30
> character limit.  Business::ISBN cannot handle this, and will throw an error. 
> This comes up, in one important case, when a patron attempts to log into their
> account in the OPAC.  The function to show their currently checked-out items
> pulls the ISBN from the biblioitems table, and if they've checked out a
> material with multiple ISBNs in the record, they'll see the error rather than
> their account.
> 
> The simple solution is to scrub any ISBN coming into GetNormalizedISBN for the
> " | " delimiter.  A more robust solution will involve preventing Koha from
> attempting to save multiple ISBNs into a single database field.

We could also split multiple ISBNs on |, and retains the first ISBN identified
as valid by Business::ISBN
Comment 7 Robin Sheat 2010-10-20 06:09:47 UTC
This patch causes problems doing bulk imports of data. In particular, I'm trying to load a record that has the ISBN '0621047376274' (which is invalid) and this causes the error:

[Wed Oct 20 19:03:36 2010] [error] [client 192.168.56.1] [Wed Oct 20 19:03:36 2010] stage-marc-import.pl: Can't call method "as_string" on an undefined value at /mnt/catalyst/koha/C4/Koha.pm line 1333, <GEN11> chunk 12321., referer: http://koha:8080/cgi-bin/koha/tools/stage-marc-import.pl

which breaks the whole import process. Reverting the patch stops this problem.

Note that invalid ISBN10 entries don't cause this problem.
Comment 8 Frédéric Demians 2010-10-20 06:23:40 UTC
> This patch causes problems doing bulk imports of data. In particular, I'm
> trying to load a record that has the ISBN '0621047376274' (which is invalid)
> and this causes the error:
> 
> [Wed Oct 20 19:03:36 2010] [error] [client 192.168.56.1] [Wed Oct 20 19:03:36
> 2010] stage-marc-import.pl: Can't call method "as_string" on an undefined value
> at /mnt/catalyst/koha/C4/Koha.pm line 1333, <GEN11> chunk 12321., referer:
> http://koha:8080/cgi-bin/koha/tools/stage-marc-import.pl
> 
> which breaks the whole import process. Reverting the patch stops this problem.
> 
> Note that invalid ISBN10 entries don't cause this problem.

You're ISBN is an invalid ISBN13.

Could you try adding after:
  
  return undef unless $isbn;

this line:

  return undef unless $isbn->is_valid;
Comment 9 Robin Sheat 2010-10-20 06:44:53 UTC
(In reply to comment #8)
> You're ISBN is an invalid ISBN13.

Yep. It's coming from a not completely clean data source.
 
> Could you try adding after:
> 
>   return undef unless $isbn;
> 
> this line:
> 
>   return undef unless $isbn->is_valid;

That's pretty similar (but slightly different) to the fix I'm testing out right now. I'll attach the patch in a moment.
Comment 10 Robin Sheat 2010-10-20 06:46:38 UTC
Created attachment 2695 [details] [review]
Prevents exception on certain dodgy input

This stops the import process from dieing when it gets an invalid ISBN13.
Comment 11 Frédéric Demians 2010-10-20 07:04:34 UTC
> This stops the import process from dieing when it gets an invalid ISBN13.

This works for me. It's equivalent to my proposal and it's consistent with this function role: when an invalid ISBN13 is submitted, undef must be returned since an invalid IBSN won't be available at Amazon (to get book cover).

You should submit your patch...
Comment 12 Robin Sheat 2010-10-20 07:10:56 UTC
> You should submit your patch...

I have, sent it via Chris for signoff, so it'll probably be on the list when he checks his mail tomorrow.
Comment 13 Chris Cormack 2010-10-20 07:14:56 UTC
Patch signed off and sent upstream, upping criticality, as this causes internal server errors in opac and intranet on the detail pages if the record contains an invalid ISBN
Comment 14 Galen Charlton 2010-10-21 22:37:34 UTC
Patch pushed to HEAD for inclusion in 3.2.  Please test and close.
Comment 15 Colin Campbell 2010-11-11 14:05:29 UTC
Created attachment 2737 [details] [review]
Further Patch

Having instantiated an ISBN object it seems illogical not to use its (tested) method to remove punctuation and instead add our own code (simple but untested) to do it. Plus why overwrite the object with a temporary scalar just for that purpose?
Lets try and smooth out the codeflow.
Ran some benchmarking and while the speed improvement is not major (4-9 %) its worth having in a routine that will be called frequently.
Comment 16 Chris Cormack 2010-11-12 01:30:27 UTC
Patch from colin pushed, please test
Comment 17 MJ Ray (software.coop) 2010-11-15 10:07:44 UTC
This bug is mentioned in:
Followup to Bug 4305: multiple ISBNS in	biblioitems.isbn breaks patron http://lists.koha-community.org/pipermail/koha-patches/2010-October/012633.html
Followup to Bug 4305: multiple ISBNS in biblioitems.isbn breaks patron http://lists.koha-community.org/pipermail/koha-patches/2010-October/012639.html
Followup to Bug 4305: multiple ISBNS in biblioitems.isbn breaks patron http://lists.koha-community.org/pipermail/koha-patches/2010-October/012641.html
Bug 4305 Follow up Ian patch http://lists.koha-community.org/pipermail/koha-patches/2010-October/012642.html
Bug 4305 - prevent errors when handling	invalid ISBN13 values http://lists.koha-community.org/pipermail/koha-patches/2010-October/012698.html
Comment 18 Chris Nighswonger 2010-11-18 03:15:40 UTC
This bug has a patch presently committed to HEAD and 3.2.x.

Please take the appropriate action to update the status of this bug.

Remember, bugs which have been resolved should be tested and marked closed, preferably by the original reporter.
Comment 19 Chris Nighswonger 2010-11-30 15:54:52 UTC
A fix for this bug has been committed to the current development HEAD as well as released in 3.2.1.