Bug 25007

Summary: AmazonCoverImages doesnt check for ISBN in details.tt
Product: Koha Reporter: Lucas Gass <lucas>
Component: Staff interfaceAssignee: Lucas Gass <lucas>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: 1joynelson, david, gmcharlt, jonathan.druart
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes the display of cover images in the staff interface where there is no ISBN and both Amazon and local cover images are enabled. Covers different combinations: - Amazon cover present, no local cover. - No Amazon cover, local cover image present. - Both Amazon and local cover image present.
Version(s) released in:
20.05.00, 19.11.06, 19.05.11
Bug Depends on:    
Bug Blocks: 25016    
Attachments: Bug 25007: Check for normaalized ISBN when building Amazon link
Bug 25007: Check for normaalized ISBN when building Amazon link
Bug 25007: Check for normaalized ISBN when building Amazon link
Bug 25007: (follow-up) Fix the way image check alters layout
Bug 25007: Check for normaalized ISBN when building Amazon link
Bug 25007: (follow-up) Fix the way image check alters layout
Bug 25007: Check for normaalized ISBN when building Amazon link
Bug 25007: (follow-up) Fix the way image check alters layout
Bug 25007: Check for normaalized ISBN when building Amazon link
Bug 25007: (follow-up) Fix the way image check alters layout
Bug 25007: (follow-up) Add check for normalized ISBN in opac-detail.tt
Bug 25007: Check for normaalized ISBN when building Amazon link
Bug 25007: (follow-up) Fix the way image check alters layout

Description Lucas Gass 2020-03-27 20:30:34 UTC
In detail.tt there is a verify_images() function. It checks to see if images from Amazon are 1px X 1px, if they are it removes the image but also updates the bootstrap class from col-xs-9 to col-xs-12.

We build the Amazon cover images like this:
<img src="https://images-na.ssl-images-amazon.com/images/P/[% normalized_isbn | html %].01.MZZZZZZZ.jpg" alt="" />

Any record without a ISBN ends up looking like this:
https://images-na.ssl-images-amazon.com/images/P/.01.MZZZZZZZ.jpg

Which turns out to be a 1px X 1px image.

If you also have local images on it then forces the image down and into a weird place.

TO RECREATE:
-Have Amazon images turned on and local images too.
-Have a record with a local cover image
-Make sure the record has no ISBN in 020 field. 
-Load detail page of that record and see that the local cover images is not on the right hand side like it should be but forced underneath 'catalogue_detail_biblio' in an odd spot.
Comment 1 Lucas Gass 2020-03-27 20:31:53 UTC
Why is is necessary to change the Bootstrap class to col-xs-12?


$("#catalogue_detail_biblio").attr("class","col-xs-12");
Comment 2 Lucas Gass 2020-03-27 20:42:33 UTC
We should probably just check for a normalized_isbn before building the AmazonCoverImages bit.
Comment 3 Lucas Gass 2020-03-27 20:44:07 UTC Comment hidden (obsolete)
Comment 4 David Nind 2020-03-29 11:01:45 UTC Comment hidden (obsolete)
Comment 5 David Nind 2020-03-29 11:04:42 UTC
Note from testing: Using Google Chrome or Chromium before the patch is applied: the local image is not displayed on the detail page (it is in Firefox - in the wrong position as per the test plan). After applying the patch it works the same in Firefox as it does in Google Chrome or Chromium.
Comment 6 Owen Leonard 2020-03-29 16:08:59 UTC Comment hidden (obsolete)
Comment 7 Owen Leonard 2020-03-29 16:09:06 UTC Comment hidden (obsolete)
Comment 8 David Nind 2020-03-29 19:22:31 UTC Comment hidden (obsolete)
Comment 9 David Nind 2020-03-29 19:22:34 UTC Comment hidden (obsolete)
Comment 10 David Nind 2020-03-29 19:23:29 UTC
Created attachment 102062 [details] [review]
Bug 25007: Check for normaalized ISBN when building Amazon link

TO RECREATE:
-Have Amazon images turned on and local images too.
-Have a record with a local cover image
-Make sure the record has no ISBN in 020 field.
-Load detail page of that record and see that the local cover images is
 not on the right hand side like it should be but forced underneath
 'catalogue_detail_biblio' in an odd spot.
-Apply patch and reload page
-The image should be back in its normal spot

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: David Nind <david@davidnind.com>
Comment 11 David Nind 2020-03-29 19:23:32 UTC
Created attachment 102063 [details] [review]
Bug 25007: (follow-up) Fix the way image check alters layout

A question in the bug report asks: Why is is necessary to change the
Bootstrap class to col-xs-12? The JavaScript which checks for the
existence of an Amazon cover image is designed to remove the Amazon
cover's container if there is none. Changing the class of
cover image column is gone.

However, the check is incorrect because it doesn't account for Local
Covers. There might be both a 1 x 1 pixel Amazon image and a local cover
image, so the layout shouldn't be changed unless there are no images
present.

To test, apply the patch and view the bibliographic details page of
various titles including:
- Amazon cover present, no local cover
- No Amazon cover, local cover image present
- Both Amazon and local cover image

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 Jonathan Druart 2020-03-31 11:14:33 UTC
Should not it be done for XISBN as well?
Comment 13 Jonathan Druart 2020-03-31 11:14:49 UTC
 701 [% FOREACH XISBN IN XISBNS %]<tr>[% IF ( AmazonCoverImages ) %]<td><a href="http://www.amazon.com/gp/reader/[% XISBN.normalized_isbn | uri %][% AmazonAssocTag | uri %]#reader-link"><img src="https://images-     na.ssl-images-amazon.com/images/P/[% XISBN.normalized_isbn | html %].01._AA75_PU_PU-5_.jpg" /></a></td>[% END %]
Comment 14 Owen Leonard 2020-03-31 17:17:23 UTC
(In reply to Jonathan Druart from comment #12)
> Should not it be done for XISBN as well?

The FRBRizeEditions feature won't link a record if there is no ISBN, so I don't think it's possible for XISBN.normalized_isbn to evaluate as false AND a row be returned in the XISBN data.
Comment 15 Jonathan Druart 2020-04-01 08:29:15 UTC
And what about OPAC? Should not we apply the same change to opac-detail?
Comment 16 Lucas Gass 2020-04-15 21:18:35 UTC
Created attachment 103034 [details] [review]
Bug 25007: Check for normaalized ISBN when building Amazon link

TO RECREATE:
-Have Amazon images turned on and local images too.
-Have a record with a local cover image
-Make sure the record has no ISBN in 020 field.
-Load detail page of that record and see that the local cover images is
 not on the right hand side like it should be but forced underneath
 'catalogue_detail_biblio' in an odd spot.
-Apply patch and reload page
-The image should be back in its normal spot

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 Lucas Gass 2020-04-15 21:18:38 UTC
Created attachment 103035 [details] [review]
Bug 25007: (follow-up) Fix the way image check alters layout

A question in the bug report asks: Why is is necessary to change the
Bootstrap class to col-xs-12? The JavaScript which checks for the
existence of an Amazon cover image is designed to remove the Amazon
cover's container if there is none. Changing the class of
cover image column is gone.

However, the check is incorrect because it doesn't account for Local
Covers. There might be both a 1 x 1 pixel Amazon image and a local cover
image, so the layout shouldn't be changed unless there are no images
present.

To test, apply the patch and view the bibliographic details page of
various titles including:
- Amazon cover present, no local cover
- No Amazon cover, local cover image present
- Both Amazon and local cover image

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 Lucas Gass 2020-04-15 21:18:41 UTC
Created attachment 103036 [details] [review]
Bug 25007: (follow-up) Add check for normalized ISBN in opac-detail.tt
Comment 19 Jonathan Druart 2020-04-16 09:44:11 UTC
Comment on attachment 103036 [details] [review]
Bug 25007: (follow-up) Add check for normalized ISBN in opac-detail.tt

Obsoleting this. Do not write patch to make me happy ;)

My feeling is that [OPAC]AmazonCoverImages syspref MUST be tested with normalized_isbn, always.
Comment 20 Jonathan Druart 2020-04-16 09:45:33 UTC
Created attachment 103064 [details] [review]
Bug 25007: Check for normaalized ISBN when building Amazon link

TO RECREATE:
-Have Amazon images turned on and local images too.
-Have a record with a local cover image
-Make sure the record has no ISBN in 020 field.
-Load detail page of that record and see that the local cover images is
 not on the right hand side like it should be but forced underneath
 'catalogue_detail_biblio' in an odd spot.
-Apply patch and reload page
-The image should be back in its normal spot

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Jonathan Druart 2020-04-16 09:45:36 UTC
Created attachment 103065 [details] [review]
Bug 25007: (follow-up) Fix the way image check alters layout

A question in the bug report asks: Why is is necessary to change the
Bootstrap class to col-xs-12? The JavaScript which checks for the
existence of an Amazon cover image is designed to remove the Amazon
cover's container if there is none. Changing the class of
cover image column is gone.

However, the check is incorrect because it doesn't account for Local
Covers. There might be both a 1 x 1 pixel Amazon image and a local cover
image, so the layout shouldn't be changed unless there are no images
present.

To test, apply the patch and view the bibliographic details page of
various titles including:
- Amazon cover present, no local cover
- No Amazon cover, local cover image present
- Both Amazon and local cover image

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Martin Renvoize 2020-04-16 18:28:10 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 23 Joy Nelson 2020-05-05 22:49:15 UTC
backported to 19.11.x for 19.11.06
Comment 24 Lucas Gass 2020-05-14 15:56:45 UTC
backported to 19.05.x for 19.05.11