To recreate: 1. Turn on Syndetics 2. Set AmazonCoverImages to show 3. Go to a record detail page that has a Syndetics image but no Amazon image. In KTD I am using bib 181. 4. The image never loads.
+1
The #syndetics-bookcoverimg is prematurely removed.
This is because when Amazon is used there is a check for 1px by 1px images inside of detail.tt. Then, without a visible syndetics image the entire container is removed.
Created attachment 154349 [details] [review] Bug 34149: Let Syndetics images show when Amazon returns 1px images 1. Turn on Syndetics 2. Set AmazonCoverImages to show 3. Go to a record detail page that has a Syndetics image but no Amazon image. In KTD I am using bib 181. 4. The image never loads. 5. Apply patch 6. Try again, the image should load. 7. Try records that have both Amazon and Syndetics images making sure it loads correctly in the lightbox.
I'm not 100% sure about this patch, so CC'ing Owen.
Created attachment 155551 [details] [review] Bug 34149: Let Syndetics images show when Amazon returns 1px images 1. Turn on Syndetics 2. Set AmazonCoverImages to show 3. Go to a record detail page that has a Syndetics image but no Amazon image. In KTD I am using bib 181. 4. The image never loads. 5. Apply patch 6. Try again, the image should load. 7. Try records that have both Amazon and Syndetics images making sure it loads correctly in the lightbox. Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se>
Created attachment 155768 [details] [review] Bug 34149: Let Syndetics images show when Amazon returns 1px images 1. Turn on Syndetics (SyndeticsEnabled, SyndeticsCoverImages, SyndeticsClientCode) 2. Set AmazonCoverImages and OPACAmazonCoverImages to show 3. Go to a record detail page that has a Syndetics image but no Amazon image. In KTD I am using bib 181. It might work and require to find another record. 4. The image never loads. 5. Do the same test on the OPAC, same result 6. Apply patch 7. Try again, the image should load. 8. Try records that have both Amazon and Syndetics images making sure it loads correctly in the lightbox. 9. Same last steps for OPAC Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 155769 [details] [review] Bug 34149: same for OPAC
Exact same code on OPAC so same bug and attached follow-up to also fix OPAC. Follow-up needs someone else to test it. But before that, looking at the code, the same issue is very very likely to be there with anything else, right? custom-coverimg, googlejacket-coverimg, openlibrary-coverimg, coce-coverimg, bakertaylor-coverimg, "cover-image local-coverimg". And custom-coverimg also has a path to "div.remove();" so it should trigger the same issue than amazon-bookcoverimg. Oh no >_< Either the same fix is applied to all of them. `if ( $('#amazon-bookcoverimg').length === 0 )` need to be changed to account for custom-coverimg case. Maybe it's tricky because custom-coverimg isn't the 1st one when amazon is enabled. Also maybe careful when both are in their invalid cases. Nooooot sure, I'm way too confused now [U+1F635][U+1F4AB][U+1F635][U+1F4AB][U+1F635][U+1F4AB] Either there is a more clever way than putting div.show() on all cases and instead just prevent amazon-bookcoverimg handling to mess with the others. Inspecting the page: why is the problem even happening? The div removed is only the one for amazon-bookcoverimg. Why does that messes the others? Ah ok, it's because the 1st one is shown and if at the end of the processing none is visible then the whole enclosing div is removed. That's the root of the whole mess. Idea: After the div.remove(); in the 1x1 pixel case, make the next element in $(img) visible. Wait, custom-coverimg needs to same (right?) but it needs to check if themselves are visible. Otherwise if amazon has a valid image it would cause the 3rd image to be visible at the same time. And check it before doing div.remove()! Ok it's still wacky [U+1F635][U+1F4AB] Alternative idea: set the 1st one visible by default **after the processing** of all of them, so we don't care about all the mess they do. After `// Loop over each container in the template which contains covers` separation_of_concerns++ The issue is mixing getting data and filtering out the invalid ones and mixing display logic (1st img tab visible on page load)
Question: Would it simplify life if, for every service that is turned on, a placeholder is created, like a default Amazon cover, a default Syndetics cover, and so forth. Then, as images are loaded, they replace the default. I don't know if this would simplify the issues being dealt with here or not. Just throwing an idea at the wall to see if it sticks. Maybe avoid removing placeholders? A step further, maybe flag the first image that isn't a default placeholder, and then have that be the default image for the item? Maybe for an enhancement down the road, but thought I'd throw it out there.
Victor, Christopher: All very good ideas! But I would say outside of the scope of this bugfix. Also, I was never able to recreate this problem on the OPAC, is the OPAC follow-up necessary? I also cannot deduce why this is now set to FQA...
> All very good ideas! But I would say outside of the scope of this bugfix. Amazon isn't the only one that can trigger this issue (custom-coverimg also). And syndetics wasn't the only one that gets broken when the issue happens. (all the others should have the same issue when the div is removed) So it didn't seem out of the scope since the same problem a few lines after and the same consequences in a few places after. So a fix of only syndetics only in the amazon case was the reason to FQA. But yes the fix is much simpler for only the intersection of one cause (amazon) of the two and fixing on broken place (syndetics) of the few. --- > Also, I was never able to recreate this problem on the OPAC, is the OPAC follow-up necessary? Weird, looking at it I even updated the test plan to reproduce and check the fix on the OPAC. Damn it, the test record I had was a transient failing case. «It might work and require to find another record.» On master I just got it working for both the OPAC and staff interface >_< Found another case! :D biblionumber=3 ISBN: 0520078438 Introduction to Attic Greek / Donald J. Mastronarde Can reproduce on OPAC and staff. And the current patches here fix it.
(In reply to Christopher Brannon from comment #10) > Question: Would it simplify life if, for every service that is turned on, a > placeholder is created, like a default Amazon cover, a default Syndetics > cover, and so forth. By default there wouldn't be a valid image on a number of records. That lessens the gain from using cover images. > A step further, maybe flag the first image that isn't a default placeholder, and then have that be the default image for the item? IIUC that's re implementing the logic that decide to remove invalid images to only show valid ones. But here just to pick which one by default to put 1st. If valid ones are marked as such, or if there is a function to identify valid or invalid ones, it's the pretty much the same to filter them out than to pick one an put it first. But just picking a default would still leave placeholders.
I don't see a reason why this is FQA, so I am reseting to 'Needs signoff' in an effort to get this moving again. If there is a valid reason why this was set to FQA please explain. :)
(In reply to Lucas Gass from comment #14) > I don't see a reason why this is FQA, so I am reseting to 'Needs signoff' in > an effort to get this moving again. If there is a valid reason why this was > set to FQA please explain. :) I'm not sure I have chances to better phrase the issue on a 3rd attempt. Without having even partial responses about how the finding in the two first attempts/comments are not actually issues. So let's wait for another pair of QA eyes. I've highlighted my two comments so they can straight see what might be the issues. I'm obsoleting my follow-up for the OPAC. So this can go back to signed off and not need someone else's testing, without even being sure that the approach in good enough.
*** Bug 33959 has been marked as a duplicate of this bug. ***
I have glanced thru the code and the comments here and my first impression is that this looks like an incomplete fix. So that sounds like what Victor said.
Moving back to FQA in view of previous discussion.