Created attachment 137950 [details]
Koha 21.05.16 - OPAC Shelf browser - Local covers OK

Created attachment 137951 [details]
Koha 21.11.00 - OPAC Shelf browser - Local covers Broken

With regards to the display of local cover images, the OPAC Shelf
browser works correctly in Koha versions < 21.11.00. It's also worth
noting that the correct Shelf browser behaviour can be observed in
all Koha versions (including master) when other cover image providers
(Amazon, etc.) are in use (either on their own or in combination with
the local cover images system preferences).

The relevant commit that changed the Shelf browser's behaviour is
fad97080e97da5bc77724c68b9394aba15518847 (from Bug 28180), where
these lines were removed from opac-detail.tt:

[% IF OPACLocalCoverImages %]
    KOHA.LocalCover.GetCoverFromBibnumber(true);
    [% IF itemloop_has_images OR oheritemloop_has_images %]
        KOHA.LocalCover.GetCoverFromItemnumber(true);
    [% END %]
[% END %]

Adding the above code to opac-detail.tt fixes the problem in master.

However, if you look closely you'll notice the code has a small typo.
The template parameter in the second [% IF %] conditional is named:

oheritemloop_has_images

vs. the correct parameter name:

otheritemloop_has_images

Because of that, the display of local cover images in the
OPAC Shelf browser has never worked as intended when the SysPref
OpacSeparateHoldings is set to 'Separate'.

Self-assigning this as I'm preparing a patch that fixes both issues
outlined above. I'm also going to include a Selenium unit test that
will (hopefully) make it easier to catch Shelf browser-related bugs
in the future.

(In reply to Andreas Roussos from comment #3)
> Because of that, the display of local cover images in the
> OPAC Shelf browser has never worked as intended when the SysPref
> OpacSeparateHoldings is set to 'Separate'.
Apologies, I just double-checked what I wrote above and it's not 100%
valid. There is indeed a problem in master that relates to the display
of local cover images in the OPAC when OpacSeparateHoldings is set to
'Separate', but it occurs at a different level and under specific
conditions (and since it's not OPAC Shelf browser-specific, I think
it'd be better if I filed a new bug report for it).

Created attachment 138013 [details] [review]
Bug 31207: Add Selenium test for the OPAC Shelf browser

This adds a new Selenium test to automatically test the
functionality of the OPAC Shelf browser with regards to the
display of local cover images. This unit test has a somewhat
narrow scope at the moment (i.e. it only checks if everything
is working fine when local covers are in use), but can be
expanded in the future to include OPAC Shelf browser tests for
other cover image providers, etc.

Also added are two sample images to be used by the test when
uploading a local cover image. They come from the Unsplash
website and have this license: https://unsplash.com/license
Attribution to the image creators is given in the test code.

Test plan:

1) Apply the other patch from this bug report, then run the
   new Selenium unit test which should pass without failures:

   cd t/db_dependent/selenium/
   prove -v opacshelfbrowser.t

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Created attachment 138016 [details] [review]
Bug 31207: Fix the display of local cover images in the OPAC Shelf browser

In the OPAC Details page for a biblio, opening the Shelf browser
results in a nice display of adjacent items next to each other
based on their call number, which also shows any cover images
found. Since 21.11.00 this has been broken when local covers are
exclusively in use, as it fails to fetch the related images that
have been uploaded *at the biblio level*.

This patch fixes that by adding back TT code that was removed
in commit fad97080e97da5bc77724c68b9394aba15518847 (Bug 28180).

Test plan:

0) Under Home > Administration > System preferences:
   - in section 'OPAC -> Shelf browser', make sure OPACShelfBrowser
     is enabled (i.e. set to 'Show')
   - in section 'Enhanced content -> Local or remote cover images',
     make sure both LocalCoverImages and OPACLocalCoverImages
     are enabled (i.e. set to 'Show')
   - make sure no other cover image provider is enabled
   - make sure OpacSeparateHoldings and OpacSeparateHoldingsBranch
     are set to 'Don't separate' and 'home library', respectively

1) In the Staff interface, create a record that has:
   - a local cover image at the biblio level (use the 'Images' tab
     in the Details page of the record to upload a test image)
   - at least one item attached to it that has a Call number (the
     call number value can be anything -- it's only needed to
     trigger the display of the 'Browse shelf' hyperlink)

2) In the OPAC, visit the Details page of the record you just added
   the cover image to, and click on the 'Browse shelf' link found
   in the 'Call number' column. When the Shelf browser expands,
   you'll notice there's no cover image displayed for the item(s)
   that belong to your test biblio.

3) Apply the patch and refresh the OPAC biblio Details page.
   This time the shelf browser should display the local cover images
   associated with your test record.

4) Make sure the relevant unit test passes:

   prove -v t/db_dependent/ShelfBrowser.t

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Created attachment 138017 [details] [review]
Bug 31207: Add Selenium test for the OPAC Shelf browser

This adds a new Selenium test to automatically test the
functionality of the OPAC Shelf browser with regards to the
display of local cover images. This unit test has a somewhat
narrow scope at the moment (i.e. it only checks if everything
is working fine when local covers are in use), but can be
expanded in the future to include OPAC Shelf browser tests for
other cover image providers, etc.

Also added are two sample images to be used by the test when
uploading a local cover image. They come from the Unsplash
website and have this license: https://unsplash.com/license
Attribution to the image creators is given in the test code.

Test plan:

1) Apply the other patch from this bug report, then run the
   new Selenium unit test which should pass without failures:

   cd t/db_dependent/selenium/
   prove -v opacshelfbrowser.t

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Sorry for the noise - I messed up on the number of patches to sign off from the sandbox.  :(

But the patch works great - appreciate the solid test plan.

(In reply to Sally from comment #13)
> Sorry for the noise - I messed up on the number of patches to sign off from
> the sandbox.  :(
> 
> But the patch works great - appreciate the solid test plan.
No worries, I've marked the non-relevant comments as obsolete.
Many thanks for your feedback and the speedy sign off!

Hi Andreas,
The test is failing for me:

kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/selenium/opacshelfbrowser.t
t/db_dependent/selenium/opacshelfbrowser.t ..     # No tests run!
t/db_dependent/selenium/opacshelfbrowser.t .. 1/3 
#   Failed test 'No tests run for subtest "OPAC Shelf browser - Check display of local cover images (OpacSeparateHoldings set to "Don't separate")"'
#   at t/db_dependent/selenium/opacshelfbrowser.t line 137.
upload_file: no such file: ./opacshelfbrowser-test-image1.jpg at t/db_dependent/selenium/opacshelfbrowser.t line 120.

Also:
+    # test image by Kourosh Qaffari on Unsplash (https://unsplash.com/photos/RrhhzitYizg)
The Unspash license does not seem GPLv3 compatible: https://unsplash.com/license

To avoid an unnecessary discussion about the license (which is not at all the goal here :D), and to prevent a 50kb to be added to the git repo, shouldn't we go with 1px images?

I would also create a "data" directory, not at the same level of the other .t files.

(In reply to Jonathan Druart from comment #15)
> Hi Andreas,
Hi Jonathan!

I appreciate your feedback -- please see my replies inline below:

> The test is failing for me:
> 
> kohadev-koha@kohadevbox:/kohadevbox/koha$ prove
> t/db_dependent/selenium/opacshelfbrowser.t
> t/db_dependent/selenium/opacshelfbrowser.t ..     # No tests run!
> t/db_dependent/selenium/opacshelfbrowser.t .. 1/3 
> #   Failed test 'No tests run for subtest "OPAC Shelf browser - Check
> display of local cover images (OpacSeparateHoldings set to "Don't
> separate")"'
> #   at t/db_dependent/selenium/opacshelfbrowser.t line 137.
> upload_file: no such file: ./opacshelfbrowser-test-image1.jpg at
> t/db_dependent/selenium/opacshelfbrowser.t line 120.
I hardcoded a relative path for the location of the JPGs to upload,
which is why the test only ran properly if you changed directory to
t/db_dependent/selenium/ first. I've now made adjustments to the test
script so that it will run just fine when called from any directory.

> Also:
> +    # test image by Kourosh Qaffari on Unsplash
> (https://unsplash.com/photos/RrhhzitYizg)
> The Unspash license does not seem GPLv3 compatible:
> https://unsplash.com/license
> 
> To avoid an unnecessary discussion about the license (which is not at all
> the goal here :D), and to prevent a 50kb to be added to the git repo,
> shouldn't we go with 1px images?
I agree, let's try that. I tried uploading a 1x1 pixel black PNG but
could not get it to work (it might have to do with the resizing that
is applied to local cover images when thumbnails are generated).
A 2x2 pixel black PNG worked fine though, so I suggest we go with
that instead. The 2x2 PNG file is only 72 bytes long ;-)

(In reply to Jonathan Druart from comment #16)
> I would also create a "data" directory, not at the same level of the other
> .t files.
Hmmm, there's actually a "data" directory inside t/db_dependent
which contains all sorts of files used by other tests. In my
revised patches I've chosen to add my sample PNG in there.

Please let me know if you'd like us to handle this differently.

Thank you very much for your patience with this bug report!
Apologies for taking so long to reply.

Created attachment 145805 [details] [review]
Bug 31207: Fix the display of local cover images in the OPAC Shelf browser

This patch adds back Template Toolkit and JavaScript code that
was removed in commit fad97080e97da5bc77724c68b9394aba15518847
(Bug 28180), thus fixing the problem of Shelf browser images
not being loaded in the OPAC Details page for a biblio.

This feature stopped working in v21.11.00, and these conditions
must be met to be able to reproduce the problem:
a) Exclusive use of local cover images, and
b) Covers uploaded at the biblio level only.

Test plan:

0) Configure the following System Preferences:
   - OPACShelfBrowser           => 'Show'
   - LocalCoverImages           => 'Show'
   - OPACLocalCoverImages       => 'Show'
     (ensure no other cover image provider is enabled)
   - OpacSeparateHoldings       => 'Don't separate'
   - OpacSeparateHoldingsBranch => 'home library'

1) Create a bibliographic record that has:
   - a local cover image at the biblio level (use the 'Images' tab
     in the Details page of the record to upload a test image)
   - a single item with a Call number (the Call number value can
     be anything, it's only needed to trigger the Shelf browser)

2) In the OPAC, visit the Details page of the record you just added,
   and click on the 'Browse shelf' link found in the 'Call number'
   column of the 'Holdings' tab. When the Shelf browser expands,
   there will be no cover image displayed.

3) Apply the patch and refresh the page with CTRL-F5. The Shelf
   browser will now display the local cover image associated with
   your test record.

4) Make sure the relevant unit test passes:

   prove -v t/db_dependent/ShelfBrowser.t

Created attachment 145806 [details] [review]
Bug 31207: Add a Selenium test for the OPAC Shelf browser

This adds a new Selenium test to automatically test the
functionality of the OPAC Shelf browser with regards to the
display of local cover images. This unit test has a somewhat
narrow scope at the moment (i.e. it only checks if everything
is working fine when local covers are in use), but can be
expanded in the future to include OPAC Shelf browser tests for
other cover image providers, etc.

Also added is a sample 2x2 pixel image to be used by the test
when uploading a local cover image.

Test plan:

1) Apply the other patch from this bug report, then run the
   new Selenium unit test which should pass without failures:

   prove -v t/db_dependent/selenium/opacshelfbrowser.t

The first patch in this patchset has been rebased for the
current master. The commit message and test plan have been
reworded as well -- hopefully for the better!

+                [% IF itemloop_has_images OR otheritemloop_has_images %]
+                    KOHA.LocalCover.GetCoverFromItemnumber(true);
+                [% END %]


This is not correct, that's not what we want here.

You don't want to get the image at the item level if the *current* biblio has image at the item level.

If you want to implement that you need to:
1. Create a new JS function in localcovers.js (say GetCoverFromItemnumberOrBibnumber) that would call opac-image.pl with a new parameter (say biblio-fallback=1)
2. Modify opac-image.pl to retrieve this parameter and return $item->cover_image || $biblio->cover_image

The selenium test could also cover this situation :)

Does that make sense?

*** Bug 33366 has been marked as a duplicate of this bug. ***

*** Bug 35858 has been marked as a duplicate of this bug. ***

Is this still being worked on?

(In reply to Lucas Gass from comment #24)
> Is this still being worked on?
Hi, Lucas!

I'm really sorry for the lack of any response on this bug :-(

The TL;DR version is that it's all due to bad communication on
my part (I should have asked for further clarifications before
setting off to rewrite my patch).

I actually tried to follow-up on this after Jonathan's latest
feedback in Comment 21. However, in my revised patch I had to
modify more than just localcovers.js and opac-image.pl since
I couldn't figure out how to implement the suggested fallback
parameter. So, I left this idle for a while until I could fix
it properly and then, unfortunately, it fell under the radar.

I'll ask for Jonathan's advice ASAP.

(In reply to Jonathan Druart from comment #21)
Hi, Jonathan!

I'm sorry for the (much) belated reply. As per my previous
message to Lucas, I realise this bug has been left lingering
for too long. I'm keen to move this forward, so please see
my comments inline below:

> +                [% IF itemloop_has_images OR otheritemloop_has_images %]
> +                    KOHA.LocalCover.GetCoverFromItemnumber(true);
> +                [% END %]
> 
> 
> This is not correct, that's not what we want here.
> 
> You don't want to get the image at the item level if the *current* biblio
> has image at the item level.
Fair point, I agree. To decide which local cover image to display
for the items shown in the shelf browser we shouldn't look at the
current biblio but rather handle each item independently.

In fact, I think that the algorithm should operate as follows:

1) If only a biblio-level image exists:
    => show that for all biblio items
2) If only item-level images exist:
    => show the respective image for each item
3) If both biblio-level and item-level images exist:
    => show the item-level image for the items that have one
    => fall back to the biblio-level image for all other items

> If you want to implement that you need to:
> 1. Create a new JS function in localcovers.js (say
> GetCoverFromItemnumberOrBibnumber) that would call opac-image.pl with a new
> parameter (say biblio-fallback=1)
> 2. Modify opac-image.pl to retrieve this parameter and return
> $item->cover_image || $biblio->cover_image
I'm afraid this is the part that I don't understand fully ;-)

When would this new biblio-fallback parameter be set?
And how would it work in conjunction with opac-image.pl?

As per the DESCRIPTION section of opac-image.pl, the script
currently accepts an imagenumber or a biblionumber, returning
a random image if the latter was provided (side note: I think
it's not actually 100% random but rather the first cover image):

 72     elsif ( defined $biblionumber ) {
 73         my $biblio = Koha::Biblios->find($biblionumber);
 74         unless ($biblio) {
 75             print $input->redirect("/cgi-bin/koha/errors/404.pl");
 76             exit;
 77         }
 78         my $cover_images = $biblio->cover_images;
 79         if ( $cover_images->count ) {
 80             $image = $cover_images->next;
 81         }
 82     }

AIUI, unless we modify opac-image.pl to also accept an itemnumber
parameter we have no way of implementing the cover image selection
algorithm I described above. To do this I actually had to also
modify opac-detail.tt, shelfbrowser.inc, and C4/ShelfBrowser.pm.
 
> The selenium test could also cover this situation :)
OK, I reckon that should be easy to implement since we already
have a test action that uploads an item-level local cover image.
What's left to be done is to write some more tests to check if
the correct local cover image is returned in each situation.

> Does that make sense?
I think what confused me in the past (not your fault!) was that
you mentioned two changes that needed to be made (localcovers.js
and opac-image.pl), and because I had to change more files I
considered my revised patch a failure.

Apologies for not asking for your feedback earlier.