Bug 9737

Summary:	Local Covers sometimes appear then disappear on OPAC Search Results/Lists
Product:	Koha	Reporter:	David Cook <dcook>
Component:	Templates	Assignee:	David Cook <dcook>
Status:	CLOSED FIXED	QA Contact:	Jonathan Druart <jonathan.druart>
Severity:	normal
Priority:	P5 - low	CC:	chris, egpetridis, jonathan.druart
Version:	Main
Hardware:	All
OS:	All
Change sponsored?:	---	Patch complexity:	Trivial patch
Documentation contact:		Documentation submission:
Text to go in the release notes:		Version(s) released in:
Circulation function:
Attachments:	Bug 9737 - Local Covers sometimes appear then disappear on OPAC Search Results/Lists Bug 9737 - Local Covers sometimes appear then disappear on OPAC Search Results/Lists [Revised to include lists] Bug 9737 - Local Covers sometimes appear then disappear on OPAC Search Results/Lists Bug 9737 - Local Covers sometimes appear then disappear on OPAC Search Results/Lists

Description David Cook 2013-03-01 01:23:47 UTC

When there are bibs with local covers in the OPAC search results or in an OPAC list, they will appear for a split second and then disappear.

Important Note: 

From what I can tell, this only happens when "OPACAmazonCoverImages" is turned on.

It also only seems to happen when an ISBN is supplied...

Comment 1 David Cook 2013-03-01 01:26:28 UTC


I think the Amazon JS is causing the problem...

Since I retrieve this cover just fine:
http://images.amazon.com/images/P/0764548557.01.TZZZZZZZ.jpg

But when I try to get this cover, I don't get the cover and the local image disappears, even though the normalized ISBN should be valid...:
http://images.amazon.com/images/P/061535243X.01.TZZZZZZZ.jpg

Comment 2 David Cook 2013-03-01 02:48:17 UTC Comment hidden (obsolete)

Created attachment 15790 [details] [review]
Bug 9737 - Local Covers sometimes appear then disappear on OPAC Search Results/Lists

The current problem:

When the verify_images() function runs from amazonimages.js, it loops through every img element and selects the elements that come from Amazon sources.

However, if the height/width is 1 (i.e. Amazon passes off a blank image) or if the img element's complete property is null or non-existent, then it searches for the parent node and re-writes the HTML with the "No Cover Image Available" (NO_AMAZON_IMAGE) text, even though that parent node might contain other img elements that have images available (from Google, local images, etc)!

Proposed fix:

Wrap the Amazon image in a span tag (as we do with the Google and local covers), so that it re-writes the HTML within that parent span, rather than the parent anchor which contains all the other images.

    To test:

    1) Turn on the OPACAmazonCoverImages, GoogleJackets, and OPACLocalCoverImages system preferences
    2) Replace the ISBN in one of your records with 9780615352435
    3) Now load a local cover image for that record
    4) Search for that record (but make sure you're vague enough to have multiple search results)
    5) For a split second, you may notice the local cover image appears before being replaced by a "No Cover Image Available".

    Now apply this patch

    6) Refresh your search results page
    7) You should now see your local cover image, PLUS the "No Cover Image Available" for the Amazon image, and whatever other graphics/"no cover image available" "graphics" for the Google Jackets.

Comment 3 David Cook 2013-03-01 02:57:23 UTC Comment hidden (obsolete)

Created attachment 15791 [details] [review]
Bug 9737 - Local Covers sometimes appear then disappear on OPAC Search Results/Lists [Revised to include lists]

The above comment is still valid, but this patch includes the same fix for Syndetics (which is Amazon apparently) and the OPAC Lists as well as the OPAC Search Results.

For testing, duplicate the steps for the lists as well as the search results.

Comment 4 Owen Leonard 2013-03-14 12:48:29 UTC

This seems to work fine, but I would rather sign off on a version that didn't include the biblionumber in a title attribute here:

<span title="[% SEARCH_RESULT.biblionumber %]" 

That looks like debugging information to me. It's certainly not relevant to the user.

Comment 5 David Cook 2013-03-19 23:02:08 UTC

(In reply to comment #4)
> This seems to work fine, but I would rather sign off on a version that
> didn't include the biblionumber in a title attribute here:
> 
> <span title="[% SEARCH_RESULT.biblionumber %]" 
> 
> That looks like debugging information to me. It's certainly not relevant to
> the user.

Hi Owen:

I was just following the convention already set out by the LocalCoverImages and GoogleJackets code. They both have  <span title="[% SEARCH_RESULT.biblionumber |url %]" in the templates. 

I agree that this information isn't relevant to the user and I don't know of any code that relies on it, so perhaps a follow-up patch removing all 3 instances of this span title is in order?

Comment 6 David Cook 2013-03-19 23:06:58 UTC

(In reply to comment #5)
> (In reply to comment #4)
> > This seems to work fine, but I would rather sign off on a version that
> > didn't include the biblionumber in a title attribute here:
> > 
> > <span title="[% SEARCH_RESULT.biblionumber %]" 
> > 
> > That looks like debugging information to me. It's certainly not relevant to
> > the user.
> 
> Hi Owen:
> 
> I was just following the convention already set out by the LocalCoverImages
> and GoogleJackets code. They both have  <span title="[%
> SEARCH_RESULT.biblionumber |url %]" in the templates. 
> 
> I agree that this information isn't relevant to the user and I don't know of
> any code that relies on it, so perhaps a follow-up patch removing all 3
> instances of this span title is in order?

OpenLibraryCovers also has a span title with biblionumber. 

GoogleJackets and OpenLibraryCovers have classes with the normalized ISBN in them. LocalCoverImages has a class with the biblionumber in it. 

Seems like these links have all sorts of seemingly meaningless information which could be removed.

But...I don't really want to break anything, my code is reasonably consistent, and it doesn't hurt anything now, so I'll leave a follow-up to someone else and change the status of this bug back to Needs Signoff. 

This patch doesn't introduce anything that isn't already there.

Comment 7 Owen Leonard 2013-03-20 00:59:18 UTC Comment hidden (obsolete)

Created attachment 16447 [details] [review]
Bug 9737 - Local Covers sometimes appear then disappear on OPAC Search Results/Lists

The current problem:

When the verify_images() function runs from amazonimages.js, it loops
through every img element and selects the elements that come from Amazon
sources.

However, if the height/width is 1 (i.e. Amazon passes off a blank image)
or if the img element's complete property is null or non-existent, then
it searches for the parent node and re-writes the HTML with the "No
Cover Image Available" (NO_AMAZON_IMAGE) text, even though that parent
node might contain other img elements that have images available (from
Google, local images, etc)!

Proposed fix:

Wrap the Amazon image in a span tag (as we do with the Google and local
covers), so that it re-writes the HTML within that parent span, rather
than the parent anchor which contains all the other images.

To test:

1) Turn on the OPACAmazonCoverImages, GoogleJackets, and
   OPACLocalCoverImages system preferences
2) Replace the ISBN in one of your records with 9780615352435
3) Now load a local cover image for that record
4) Search for that record (but make sure you're vague enough to have
   multiple search results)
5) For a split second, you may notice the local cover image appears before being replaced by a "No Cover Image Available".

Now apply this patch

6) Refresh your search results page
7) You should now see your local cover image, PLUS the "No Cover Image
   Available" for the Amazon image, and whatever other graphics/"no
   cover image available" "graphics" for the Google Jackets.

To be comprehensive, try the same set of steps using the OPAC Lists
rather than the OPAC Search Results

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Comment 8 Jonathan Druart 2013-04-02 10:44:44 UTC

QA comment:

The local cover image is displayed but I got 2 "no cover image available" blocks.
Only 1 is displayed on master.

Marked as Failed QA.

Comment 9 David Cook 2013-04-03 03:13:52 UTC

(In reply to comment #8)
> QA comment:
> 
> The local cover image is displayed but I got 2 "no cover image available"
> blocks.
> Only 1 is displayed on master.
> 
> Marked as Failed QA.

Jonathan:

Can you explain your comment a bit more?

What cover image system preferences do you have turned on? Did you turn on all 3 of the OPACAmazonCoverImages, GoogleJackets, and OPACLocalCoverImages system preferences?

1) In master, what appears? Just 1 "no cover image available"? Or the local cover image and 1 "no cover image available"? 

(In the event that you only get 1 "no cover image available" block, it's because the Amazon JS is erasing every other cover image block.)


2) In the bug branch, you should be getting the local cover image and 2 "no cover image available" blocks. That's correct behaviour. 

--

From the sounds of it, you're getting the correct behaviour.

Comment 10 Jonathan Druart 2013-04-03 08:15:30 UTC

(In reply to comment #9)
> 2) In the bug branch, you should be getting the local cover image and 2 "no
> cover image available" blocks. That's correct behaviour. 
> 
> --
> 
> From the sounds of it, you're getting the correct behaviour.

Hum.. ok :)
It was what I got but to me it is a regression.
I don't get why it is the expected behavior, could you explain please?

By adding some tests, we can decide not to display the second one.
It could be fix with something like:
[% IF ( GoogleJackets ) %]
  [% IF isbn %]
    ...
  [% ELSE %]
    [% UNLESS SyndeticsCoverImages OR OPACAmazonCoverImages OR ... %]
       No cover image available
    [% END %]
  [% END %]
[% END %]
And others for OpenLibraryCovers and BakerTaylorEnabled, etc.

Comment 11 David Cook 2013-04-03 23:25:07 UTC

(In reply to comment #10)
> (In reply to comment #9)
> > 2) In the bug branch, you should be getting the local cover image and 2 "no
> > cover image available" blocks. That's correct behaviour. 
> > 
> > --
> > 
> > From the sounds of it, you're getting the correct behaviour.
> 
> Hum.. ok :)
> It was what I got but to me it is a regression.
> I don't get why it is the expected behavior, could you explain please?
> 
> By adding some tests, we can decide not to display the second one.
> It could be fix with something like:
> [% IF ( GoogleJackets ) %]
>   [% IF isbn %]
>     ...
>   [% ELSE %]
>     [% UNLESS SyndeticsCoverImages OR OPACAmazonCoverImages OR ... %]
>        No cover image available
>     [% END %]
>   [% END %]
> [% END %]
> And others for OpenLibraryCovers and BakerTaylorEnabled, etc.

Mmm, I think I understand you. I consider it expected behaviour rather than a regression, because this patch prevents the Amazon JScript from touching "non-Amazon" elements. By adding the spans to the Amazon elements, I'm making the Amazon JScript behave similarly to the other JScript for GoogleJackets, OpenLibraryCovers, etc. I think it creates consistency rather than regression.

When you said that you got 1 "no cover image available" in master, I assume that also meant that the local cover didn't appear. I think that's a much more critical issue than having 2 "no cover image available" appear alongside an existing local cover image.

That said, I agree that it doesn't make sense to show "No cover image available" if there is a cover image available, but there are currently no checks in place for that. I think that deserves a bug of its own. 

--

Unfortunately, I don't think the code snippet that you provide would actually work, because that would just test for the system preference and not the actual presence of cover images. If you had  GoogleJackets and OPACAmazonCoverImages turned on, you would never get a "no cover image available" even when you should.

One idea (purely JScript-based):

1) Add a class to each cover art span (or possibly use the existing thumbnail class for the img)
2) Then use a JScript function to loop through each a.p1 (which seems to be unique...although we could always add a cover art class to it as well). 
3) If span.cover_art (or img.thumbnail) exists, remove all span.no-image elements within that a.p1 element. 
4) If span.cover_art (or img.thumbnail) does not exist AND count(span.no-image) > 1, remove every span.no-image after the first instance within that a.p1 element. 

I don't think we could do it solely in the templates, because the existence of cover art is going to vary depending on several other JScript functions which run independently of each other. 

Maybe if we re-wrote all the cover art functions (and ideally allowed librarians to rank the providers so that you don't have more than cover image showing for an item), but yes...I think that's a different bug. 

If I've managed to convince you, please change back to Signed Off (or even Passed QA :P)

Comment 12 Jonathan Druart 2013-04-04 07:25:59 UTC

(In reply to comment #11)
> When you said that you got 1 "no cover image available" in master, I assume
> that also meant that the local cover didn't appear. I think that's a much
> more critical issue than having 2 "no cover image available" appear
> alongside an existing local cover image.

Agreed

> Unfortunately, I don't think the code snippet that you provide would
> actually work, because that would just test for the system preference and
> not the actual presence of cover images. If you had  GoogleJackets and
> OPACAmazonCoverImages turned on, you would never get a "no cover image
> available" even when you should.

Yep, of course, stupid code :-/

> Maybe if we re-wrote all the cover art functions (and ideally allowed
> librarians to rank the providers so that you don't have more than cover
> image showing for an item), but yes...I think that's a different bug. 

Could you open a new bug for that please?

> If I've managed to convince you, please change back to Signed Off (or even
> Passed QA :P)

Yes, you are. Thank you for your detailed explanation :)

Marked as Passed QA.

Comment 13 Jonathan Druart 2013-04-04 07:26:20 UTC

Created attachment 17180 [details] [review]
Bug 9737 - Local Covers sometimes appear then disappear on OPAC Search Results/Lists

The current problem:

When the verify_images() function runs from amazonimages.js, it loops
through every img element and selects the elements that come from Amazon
sources.

However, if the height/width is 1 (i.e. Amazon passes off a blank image)
or if the img element's complete property is null or non-existent, then
it searches for the parent node and re-writes the HTML with the "No
Cover Image Available" (NO_AMAZON_IMAGE) text, even though that parent
node might contain other img elements that have images available (from
Google, local images, etc)!

Proposed fix:

Wrap the Amazon image in a span tag (as we do with the Google and local
covers), so that it re-writes the HTML within that parent span, rather
than the parent anchor which contains all the other images.

To test:

1) Turn on the OPACAmazonCoverImages, GoogleJackets, and
   OPACLocalCoverImages system preferences
2) Replace the ISBN in one of your records with 9780615352435
3) Now load a local cover image for that record
4) Search for that record (but make sure you're vague enough to have
   multiple search results)
5) For a split second, you may notice the local cover image appears before being replaced by a "No Cover Image Available".

Now apply this patch

6) Refresh your search results page
7) You should now see your local cover image, PLUS the "No Cover Image
   Available" for the Amazon image, and whatever other graphics/"no
   cover image available" "graphics" for the Google Jackets.

To be comprehensive, try the same set of steps using the OPAC Lists
rather than the OPAC Search Results

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Comment 14 David Cook 2013-04-04 23:33:08 UTC

(In reply to comment #12)
> (In reply to comment #11)
> > When you said that you got 1 "no cover image available" in master, I assume
> > that also meant that the local cover didn't appear. I think that's a much
> > more critical issue than having 2 "no cover image available" appear
> > alongside an existing local cover image.
> 
> Agreed
> 
> > Unfortunately, I don't think the code snippet that you provide would
> > actually work, because that would just test for the system preference and
> > not the actual presence of cover images. If you had  GoogleJackets and
> > OPACAmazonCoverImages turned on, you would never get a "no cover image
> > available" even when you should.
> 
> Yep, of course, stupid code :-/
> 
> > Maybe if we re-wrote all the cover art functions (and ideally allowed
> > librarians to rank the providers so that you don't have more than cover
> > image showing for an item), but yes...I think that's a different bug. 
> 
> Could you open a new bug for that please?
> 
> > If I've managed to convince you, please change back to Signed Off (or even
> > Passed QA :P)
> 
> Yes, you are. Thank you for your detailed explanation :)
> 
> Marked as Passed QA.

Thanks, Jonathan :).

Bug 7187 already exists for the prioritizing of cover image sources, although I think it might have stalled. 

I've created Bug 9991 in regards to de-duplicating "no cover image available" graphics.

Comment 15 Jared Camins-Esakov 2013-04-18 12:26:36 UTC

This patch has been pushed to master and 3.12.x.

Comment 16 Chris Cormack 2013-04-26 21:48:59 UTC

Pushed to 3.10.x will be in 3.10.6

Comment 17 Manos PETRIDIS 2013-05-06 18:34:34 UTC

Will this fix be added to 3.08? I'm at 3.08.12.000 but the problem persists.

Having reported this behaviour back in August ( http://lists.katipo.co.nz/public/koha/2012-August/033902.html up to  http://lists.katipo.co.nz/public/koha/2012-August/033936.html ) I feel happy that it finally got to be solved, but I have no idea on how to push my installation up the release ladder.

Manos PETRIDIS

Comment 18 Chris Cormack 2013-05-07 07:56:21 UTC

(In reply to comment #17)
> Will this fix be added to 3.08? I'm at 3.08.12.000 but the problem persists.
> 
> Having reported this behaviour back in August (
> http://lists.katipo.co.nz/public/koha/2012-August/033902.html up to 
> http://lists.katipo.co.nz/public/koha/2012-August/033936.html ) I feel happy
> that it finally got to be solved, but I have no idea on how to push my
> installation up the release ladder.
> 
> Manos PETRIDIS

No, the fix does not apply to 3.8.x