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...
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
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.
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.
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.
(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?
(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.
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>
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.
(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.
(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.
(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)
(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.
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>
(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.
This patch has been pushed to master and 3.12.x.
Pushed to 3.10.x will be in 3.10.6
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
(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