Bug 34149 - Syndetics cover images don't load in staff interface detail page if other images services are enabled
Summary: Syndetics cover images don't load in staff interface detail page if other ima...
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Lucas Gass
QA Contact: Testopia
URL:
Keywords:
: 33959 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-06-28 17:08 UTC by Lucas Gass
Modified: 2023-12-01 09:01 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 34149: Let Syndetics images show when Amazon returns 1px images (1.61 KB, patch)
2023-08-10 17:33 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 34149: Let Syndetics images show when Amazon returns 1px images (1.66 KB, patch)
2023-09-12 14:21 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 34149: Let Syndetics images show when Amazon returns 1px images (1.93 KB, patch)
2023-09-17 23:05 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34149: same for OPAC (1.29 KB, patch)
2023-09-17 23:05 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass 2023-06-28 17:08:13 UTC
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.
Comment 1 Laura Escamilla 2023-08-10 13:32:13 UTC
+1
Comment 2 Lucas Gass 2023-08-10 14:00:53 UTC
The #syndetics-bookcoverimg is prematurely removed.
Comment 3 Lucas Gass 2023-08-10 17:29:32 UTC
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.
Comment 4 Lucas Gass 2023-08-10 17:33:11 UTC
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.
Comment 5 Lucas Gass 2023-08-10 17:34:20 UTC
I'm not 100% sure about this patch, so CC'ing Owen.
Comment 6 Biblibre Sandboxes 2023-09-12 14:21:23 UTC
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>
Comment 7 Victor Grousset/tuxayo 2023-09-17 23:05:09 UTC
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>
Comment 8 Victor Grousset/tuxayo 2023-09-17 23:05:11 UTC
Created attachment 155769 [details] [review]
Bug 34149: same for OPAC
Comment 9 Victor Grousset/tuxayo 2023-09-18 00:30:03 UTC
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 πŸ˜΅β€πŸ’«πŸ˜΅β€πŸ’«πŸ˜΅β€πŸ’«

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 πŸ˜΅β€πŸ’«

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)
Comment 10 Christopher Brannon 2023-10-17 21:32:41 UTC
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.
Comment 11 Lucas Gass 2023-10-18 22:38:49 UTC
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...
Comment 12 Victor Grousset/tuxayo 2023-10-21 22:25:00 UTC
> 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.
Comment 13 Victor Grousset/tuxayo 2023-10-21 22:35:00 UTC
(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.
Comment 14 Lucas Gass 2023-10-24 18:17:14 UTC
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. :)
Comment 15 Victor Grousset/tuxayo 2023-10-25 17:15:26 UTC
(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.
Comment 16 Lucas Gass 2023-11-06 22:53:07 UTC
*** Bug 33959 has been marked as a duplicate of this bug. ***
Comment 17 Marcel de Rooy 2023-11-10 10:50:02 UTC
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.
Comment 18 Marcel de Rooy 2023-12-01 09:01:29 UTC
Moving back to FQA in view of previous discussion.