[Bug 9737] New: Local Covers sometimes appear then disappear on OPAC Search Results/Lists
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 Bug ID: 9737 Summary: Local Covers sometimes appear then disappear on OPAC Search Results/Lists Classification: Unclassified Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Templates Assignee: oleonard@myacpl.org Reporter: dcook@prosentient.com.au 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... -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 --- Comment #1 from David Cook <dcook@prosentient.com.au> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|oleonard@myacpl.org |dcook@prosentient.com.au --- Comment #2 from David Cook <dcook@prosentient.com.au> --- Created attachment 15790 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=15790&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #15790|0 |1 is obsolete| | --- Comment #3 from David Cook <dcook@prosentient.com.au> --- Created attachment 15791 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=15791&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 --- Comment #4 from Owen Leonard <oleonard@myacpl.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- (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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #6 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #15791|0 |1 is obsolete| | --- Comment #7 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 16447 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=16447&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@biblibre.co | |m --- Comment #8 from Jonathan Druart <jonathan.druart@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 --- Comment #9 from David Cook <dcook@prosentient.com.au> --- (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.
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 --- Comment #10 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |In Discussion --- Comment #11 from David Cook <dcook@prosentient.com.au> --- (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) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Passed QA Patch complexity|--- |Trivial patch QA Contact| |jonathan.druart@biblibre.co | |m --- Comment #12 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #16447|0 |1 is obsolete| | --- Comment #13 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 17180 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=17180&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 --- Comment #14 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #15 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- This patch has been pushed to master and 3.12.x. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |chris@bigballofwax.co.nz --- Comment #16 from Chris Cormack <chris@bigballofwax.co.nz> --- Pushed to 3.10.x will be in 3.10.6 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 Manos PETRIDIS <egpetridis@yahoo.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |egpetridis@yahoo.com --- Comment #17 from Manos PETRIDIS <egpetridis@yahoo.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9737 --- Comment #18 from Chris Cormack <chris@bigballofwax.co.nz> --- (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 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon@bugs.koha-community.org