Bug 25007 - AmazonCoverImages doesnt check for ISBN in details.tt
Summary: AmazonCoverImages doesnt check for ISBN in details.tt
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Lucas Gass
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25016
  Show dependency treegraph
 
Reported: 2020-03-27 20:30 UTC by Lucas Gass
Modified: 2020-05-14 15:56 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
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


Attachments
Bug 25007: Check for normaalized ISBN when building Amazon link (1.75 KB, patch)
2020-03-27 20:44 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 25007: Check for normaalized ISBN when building Amazon link (1.80 KB, patch)
2020-03-29 11:01 UTC, David Nind
Details | Diff | Splinter Review
Bug 25007: Check for normaalized ISBN when building Amazon link (1.85 KB, patch)
2020-03-29 16:08 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 25007: (follow-up) Fix the way image check alters layout (2.48 KB, patch)
2020-03-29 16:09 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 25007: Check for normaalized ISBN when building Amazon link (1.90 KB, patch)
2020-03-29 19:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 25007: (follow-up) Fix the way image check alters layout (2.53 KB, patch)
2020-03-29 19:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 25007: Check for normaalized ISBN when building Amazon link (1.90 KB, patch)
2020-03-29 19:23 UTC, David Nind
Details | Diff | Splinter Review
Bug 25007: (follow-up) Fix the way image check alters layout (2.53 KB, patch)
2020-03-29 19:23 UTC, David Nind
Details | Diff | Splinter Review
Bug 25007: Check for normaalized ISBN when building Amazon link (1.90 KB, patch)
2020-04-15 21:18 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 25007: (follow-up) Fix the way image check alters layout (2.53 KB, patch)
2020-04-15 21:18 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 25007: (follow-up) Add check for normalized ISBN in opac-detail.tt (1.31 KB, patch)
2020-04-15 21:18 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 25007: Check for normaalized ISBN when building Amazon link (1.97 KB, patch)
2020-04-16 09:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25007: (follow-up) Fix the way image check alters layout (2.60 KB, patch)
2020-04-16 09:45 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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