Bug 38981 - Local cover images failing to load in OPAC search results
Summary: Local cover images failing to load in OPAC search results
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker
Assignee: Owen Leonard
QA Contact: Lucas Gass (lukeg)
URL:
Keywords:
Depends on: 26933
Blocks:
  Show dependency treegraph
 
Reported: 2025-01-28 03:40 UTC by David Cook
Modified: 2025-03-21 16:51 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patchset fixes a problem where local cover images were not properly loading on the OPAC results page. With this fix local covers now load correctly.
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 38981: Local cover images failing to load in OPAC search results (1.70 KB, patch)
2025-03-11 15:12 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38981: Directly use the opac-image link (1.38 KB, patch)
2025-03-12 14:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38981: Local cover images failing to load in OPAC search results (1.75 KB, patch)
2025-03-18 10:25 UTC, David Nind
Details | Diff | Splinter Review
Bug 38981: Directly use the opac-image link (1.43 KB, patch)
2025-03-18 10:25 UTC, David Nind
Details | Diff | Splinter Review
Bug 38981: Local cover images failing to load in OPAC search results (1.80 KB, patch)
2025-03-20 17:52 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 38981: Directly use the opac-image link (1.48 KB, patch)
2025-03-20 17:52 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2025-01-28 03:40:32 UTC
It looks like bug 26933 might've broke OPAC local cover images in the search results due to a possible race condition.

In koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt around line 630 there is a window.load() call which runs 'wait_for_images(verify_cover_images)'.

If you're only using OPACLocalCoverImages, there is no wait, so verify_cover_images() is called immediately. That function  removes the .cover-image element if there are no images.

In the case of OPACLocalCoverImages, there are no images yet, so the element gets removed.

Now there are no images yet because 'KOHA.LocalCover.GetCoverFromBibnumber(false);', the function which populates the local cover image elements, hasn't even run yet. Note that it's called within a $(document).ready() which is written lower than the window.load()


It's possible that different configurations would yield different results, if they load different resources at different times, which could affect when the handlers run. 

I've observed the above when using a fairly vanilla koha-testing-docker.
Comment 1 Jonathan Druart 2025-01-28 11:42:25 UTC
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt

555                 // if( $(coverSlide).find(".cover-image").length < 1 ){
556                 //     $(coverSlide).remove();
557                 // } else {
558                     // This is a suboptimal workaround; we should do this via load, but
559                     // the image code is scattered all over now. We come here now after
560                     // window load and wait_for_images (so load completed).
561                     var check_complete = 1;
562                     $(coverSlide).find("img").each( function() {
563                         if( !this.complete || this.naturalHeight == 0 ) check_complete = 0;
564                     });
565                     if( check_complete ) $(coverSlide).removeClass('cover-slides');
566                 // }

Why are those lines commented?

It works (at first glance) if I bring them back.
Comment 2 David Cook 2025-03-05 23:18:12 UTC
(In reply to Jonathan Druart from comment #1)
> Why are those lines commented?
> 
> It works (at first glance) if I bring them back.

I've been curious about this as well. Wondering if Owen has any insight into this one.
Comment 3 Owen Leonard 2025-03-11 15:12:43 UTC
Created attachment 179170 [details] [review]
Bug 38981: Local cover images failing to load in OPAC search results

This patch uncomments some code which was incorrectly left commented
after debugging.
Comment 4 Lucas Gass (lukeg) 2025-03-11 17:27:23 UTC
In Chrome with this patch it works good. In Firefox, it works about 1/10 times I load the results page. 

Is there some kind of race condition that Firefox is slower at that is causing this not to work there?
Comment 5 David Cook 2025-03-11 22:26:01 UTC
(In reply to Lucas Gass (lukeg) from comment #4)
> In Chrome with this patch it works good. In Firefox, it works about 1/10
> times I load the results page. 
> 
> Is there some kind of race condition that Firefox is slower at that is
> causing this not to work there?

That could be. My initial observations were around some weird interplay between window.load and $(document).ready() working a bit differently than I would've expected...
Comment 6 Jonathan Druart 2025-03-12 14:31:32 UTC
(In reply to David Cook from comment #5)
> (In reply to Lucas Gass (lukeg) from comment #4)
> > In Chrome with this patch it works good. In Firefox, it works about 1/10
> > times I load the results page. 
> > 
> > Is there some kind of race condition that Firefox is slower at that is
> > causing this not to work there?
> 
> That could be. My initial observations were around some weird interplay
> between window.load and $(document).ready() working a bit differently than I
> would've expected...

Using window.load is correct here, we need to wait for the images to be loaded.
Comment 7 Jonathan Druart 2025-03-12 14:34:19 UTC
We have fixed this problem on the staff interface long ago.

The use of GetCoverFromBibnumber is problematic (and it's also why shelfbrowser is broken, see bug 31207)

We should pass the image numbers to the template, the same way we do for opac-detail

  60                             [% IF ( OPACLocalCoverImages ) %]
  61                                 [% IF localimages.count %]
  62                                     [% FOREACH image IN localimages %]
  63                                         <div class="cover-image local-coverimg">
  64                                             <a href="/cgi-bin/koha/opac-image.pl?imagenumber=[% image.imagenumber | uri %]" title="Local cover image">
  65                                                 <img
  66                                                     src="/cgi-bin/koha/opac-image.pl?thumbnail=1&amp;imagenumber=[% image.imagenumber | uri %]"
  67                                                     alt="Local cover image"
  68                                                     data-link="/cgi-bin/koha/opac-imageviewer.pl?biblionumber=[% biblionumber | uri %]&amp;imagenumber=[% image.imagenumber | uri %]"
  69                                                 />
  70                                             </a>
  71                                             <div class="hint">Local cover image</div>
  72                                         </div>
  73                                     [% END %]
  74                                 [% END %]
  75                             [% END %]
Comment 8 Jonathan Druart 2025-03-12 14:34:52 UTC
(In reply to Jonathan Druart from comment #7)
> We have fixed this problem on the staff interface long ago.

Bug 25031: Improve handling of multiple covers on the biblio detail page in the staff client
Comment 9 Jonathan Druart 2025-03-12 14:45:09 UTC
Created attachment 179210 [details] [review]
Bug 38981: Directly use the opac-image link
Comment 10 Jonathan Druart 2025-03-12 14:45:31 UTC
Why not... simply linking to the opac-image url?...
Comment 11 David Cook 2025-03-14 01:05:07 UTC
(In reply to Jonathan Druart from comment #10)
> Why not... simply linking to the opac-image url?...

I've been wondering that too. Did you test it? I wonder if it has to do with the lightbox being used?
Comment 12 David Nind 2025-03-18 10:25:06 UTC
Created attachment 179431 [details] [review]
Bug 38981: Local cover images failing to load in OPAC search results

This patch uncomments some code which was incorrectly left commented
after debugging.

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2025-03-18 10:25:09 UTC
Created attachment 179432 [details] [review]
Bug 38981: Directly use the opac-image link

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2025-03-18 10:44:26 UTC
Testing notes (using KTD):
1. Enable the OPACLocalCoverImages and AllowMultipleCovers system preferences.
2. Add some cover images to a couple of records.
3. Add multiple cover images for one record.
4. Use two different browsers (Firefox and a Chromium-based browser) and in the OPAC run a search that returns records with the cover images.
5. Note that:
   - For Firefox: there are no local cover images shown in the search results, but they are displayed on the record details page
   - For Chromium-based browsers: things work as expected - local cover images are shown in the search results and the record details page
6. Apply the patch.
7. Repeat step 4.
8. Note that Firefox and Chromium browsers both now work as expected - local cover images are correctly shown in the OPAC search results and details pages.

Note: The OPAC search results only show one local cover image, even with AllowMultipleCovers enabled. Multiple covers are viewable on the details page in the OPAC.
Comment 15 Lucas Gass (lukeg) 2025-03-20 17:52:01 UTC
Created attachment 179560 [details] [review]
Bug 38981: Local cover images failing to load in OPAC search results

This patch uncomments some code which was incorrectly left commented
after debugging.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 16 Lucas Gass (lukeg) 2025-03-20 17:52:03 UTC
Created attachment 179561 [details] [review]
Bug 38981: Directly use the opac-image link

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 17 David Cook 2025-03-20 23:19:06 UTC
Thanks a lot, Lucas! I've been meaning to come back to this, but just haven't had the time.
Comment 18 Katrin Fischer 2025-03-21 16:51:17 UTC
Pushed for 25.05!

Well done everyone, thank you!