Bug 32307 - Chocolat image viewer broken in the staff interface when Coce is enabled
Summary: Chocolat image viewer broken in the staff interface when Coce is enabled
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 28179
Blocks:
  Show dependency treegraph
 
Reported: 2022-11-21 16:24 UTC by Owen Leonard
Modified: 2023-12-28 20:43 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.04,22.05.11, 21.11.19


Attachments
Bug 32307: Fix gallery when Coce is enabled (4.89 KB, patch)
2022-11-22 07:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32307: Fix gallery when Coce is enabled (4.93 KB, patch)
2022-11-24 19:02 UTC, David Nind
Details | Diff | Splinter Review
Bug 32307: (follow-up) Fix gallery when Coce is enabled (8.18 KB, patch)
2022-11-25 06:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32307: Fix gallery when Coce is enabled (4.93 KB, patch)
2022-11-25 11:04 UTC, David Nind
Details | Diff | Splinter Review
Bug 32307: (follow-up) Fix gallery when Coce is enabled (8.23 KB, patch)
2022-11-25 11:04 UTC, David Nind
Details | Diff | Splinter Review
Bug 32307: Fix gallery when Coce is enabled (5.03 KB, patch)
2023-02-17 10:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32307: (follow-up) Fix gallery when Coce is enabled (8.32 KB, patch)
2023-02-17 10:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32307: Fix local cover images (1000 bytes, patch)
2023-02-23 11:10 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2022-11-21 16:24:51 UTC
I'm seeing errors in the console when viewing a bibliographic record in the staff interface.

Looking at a record with more than one cover image available (e.g. Amazon and Coce) I can click the cover and see the first image in the Chocolat viewer, but there is an error in the console:

Uncaught (in promise) 
<img src="null">
chocolat_22.0600032.js:54:19

When I click the arrow to view the next cover, the cover doesn't load. It appears to fail on the *last* image. If my record has three covers available (e.g. Local, Amazon, and Coce) the first and second images are displayed correctly.
Comment 1 Katrin Fischer 2022-11-21 20:09:14 UTC
I am also stuck on the 'spinning thing' not going away with bug 32308 applied. When I click the views and switching between covers works?
Comment 2 Jonathan Druart 2022-11-22 07:16:00 UTC Comment hidden (obsolete)
Comment 3 David Nind 2022-11-24 19:02:06 UTC Comment hidden (obsolete)
Comment 4 David Nind 2022-11-24 19:02:41 UTC
I've signed off. I think I managed to test this correctly - it took a while for the Coce images to appear (I used https://coce.bywatersolutions.com).

Just noting the behaviour for the OPAC (which is different from the staff interface) where there are multiple cover image sources:

- For the staff interface search results (with both LocalCoverImages and IntranetCoce enabled), you can switch between the two images using the coloured dots (where a record has both a local image and an image from Coce).

- For the OPAC search results (with both LocalCoverImages and OpacCoce enabled) there are two images displayed side-by-side. Where there is no local cover image, the generic "No cover image available" is displayed as well as the Coce image.
Comment 5 Jonathan Druart 2022-11-25 06:07:59 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2022-11-25 06:09:00 UTC
Thanks for your time, David!

Did you notice that this patch was for the detail page?

Reading your comment I realised I didn't test the search result, and... found a bug on the detail page! Added another patch.
Comment 7 David Nind 2022-11-25 11:04:12 UTC
Created attachment 144237 [details] [review]
Bug 32307: Fix gallery when Coce is enabled

Bug 28179 added the gallery in the staff interface, and bug 28180 in the
OPAC.

OPAC and staff have different code, because the cover image providers
are different.
On bug 28180 we noticed that the OPAC needed additional code to handle
OpenLibrary, Google Jacket and Coce, to differ the creation of the
gallery to prevent Chocolat to be instantiated before the links (img src)
are effectively in the DOM.

To acchieve that we introduced a wait_for_images function, and added a
"done" flag to the 3 "JS modules".

However we haven't ported this code for the staff interface, which is
also using Coce.

Note that there was a bug at the OPAC as well for COCE, the ajax jQuery
function is async, and so 'done' was set even if the call was not
finished.

Test plan:
0. Do not apply this patch
1. Enable LocalCoverImages and IntranetCoce
2. Pick a bibliographic record with an image returned from Coce
3. Add a local cover image
4. Go to the detail page
5. Open the gallery
=> Notice that you can notice a JS error and a loading icon on the
second image. This behaviour is not always broken, it depends on the
speed of the Coce server. If you don't see the problem reload the page.
6. Apply the patch and repeat
=> Notice that the cover slider block is longer to display the icon, but
that the error is gone.
7. Confirm that there is no regression at the OPAC

We could improve the situation here, and we could display the images
in the slider, especially if the first image is a local image. But
that's for another bug...

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2022-11-25 11:04:18 UTC
Created attachment 144238 [details] [review]
Bug 32307: (follow-up) Fix gallery when Coce is enabled

Yet another fix, this is porting the following change to the intranet:
  commit 1916c644808577beb3170d642f93b456bfcc5c56
  Bug 28180: Fix bug with first image does not exist

To recreate the problem:
Enable Coce
Go to biblionmber=5 detail
=> No image (but one is displayed on the search result)
Add a local cover image
=> Only the local cover image is displayed
Apply this patch
=> Both images are displayed
Remove the local cover image
=> Image from coce is displayed

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Katrin Fischer 2022-12-07 21:56:19 UTC
I am trying to test this, but with local covers and cove activated, I only ever see an 'empty picture frame' on the detail page. I used: https://coce.bywatersolutions.com, but there are no covers :(
Maybe it's not allowing access from everywhere? Is there another coce server I could try?
Comment 10 Katrin Fischer 2022-12-07 22:02:37 UTC
Hm, same when Coce is disabled: emptry frame. No errors in the console. restarted_all and also used a private tab.
Comment 11 David Nind 2022-12-07 22:31:26 UTC
(In reply to Katrin Fischer from comment #9)
> I am trying to test this, but with local covers and cove activated, I only
> ever see an 'empty picture frame' on the detail page. I used:
> https://coce.bywatersolutions.com, but there are no covers :(
> Maybe it's not allowing access from everywhere? Is there another coce server
> I could try?

I had to wait awhile - I did some searches, looked at some detail pages. Then left for about 10-15 minutes, and refreshed the pages or did the searches again.
Comment 12 Jonathan Druart 2022-12-14 07:52:30 UTC
Retested and I confirm this is working correctly. Can you please retest and provide more details if you recreate the problem? Maybe try on a sandbox?
Comment 13 Marcel de Rooy 2023-02-17 09:42:41 UTC
+                var loaded = 1;
+                counter_wait++;
+
+                if ( loaded ) {
+                    loaded = KOHA.coce.done;
+                }

Strange pattern? Why not var loaded = KOHA.coce.done ?
Comment 14 Marcel de Rooy 2023-02-17 09:43:30 UTC
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
   OK     filters
   FAIL   forbidden patterns
                forbidden pattern: console.log (line 1622)

Candidate for false negative?
Comment 15 Jonathan Druart 2023-02-17 10:04:35 UTC
(In reply to Marcel de Rooy from comment #13)
> +                var loaded = 1;
> +                counter_wait++;
> +
> +                if ( loaded ) {
> +                    loaded = KOHA.coce.done;
> +                }
> 
> Strange pattern? Why not var loaded = KOHA.coce.done ?

Yes, but IIRC I copied the code from the OPAC, and wanted to stay as close as possible.

(In reply to Marcel de Rooy from comment #14)
>  FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
>    OK     filters
>    FAIL   forbidden patterns
>                 forbidden pattern: console.log (line 1622)
> 
> Candidate for false negative?

Yes, we want this one.
Comment 16 Marcel de Rooy 2023-02-17 10:20:59 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Marcel de Rooy from comment #13)
> > +                var loaded = 1;
> > +                counter_wait++;
> > +
> > +                if ( loaded ) {
> > +                    loaded = KOHA.coce.done;
> > +                }
> > 
> > Strange pattern? Why not var loaded = KOHA.coce.done ?
> 
> Yes, but IIRC I copied the code from the OPAC, and wanted to stay as close
> as possible.

Not very convincing, but no blocker :)
Comment 17 Marcel de Rooy 2023-02-17 10:22:57 UTC
+                wait_for_images(verify_cover_images);

This cb construction does not look very good too. It could be just one after the other without cb. No problem, just noting..
Comment 18 Marcel de Rooy 2023-02-17 10:23:45 UTC
Created attachment 146811 [details] [review]
Bug 32307: Fix gallery when Coce is enabled

Bug 28179 added the gallery in the staff interface, and bug 28180 in the
OPAC.

OPAC and staff have different code, because the cover image providers
are different.
On bug 28180 we noticed that the OPAC needed additional code to handle
OpenLibrary, Google Jacket and Coce, to differ the creation of the
gallery to prevent Chocolat to be instantiated before the links (img src)
are effectively in the DOM.

To acchieve that we introduced a wait_for_images function, and added a
"done" flag to the 3 "JS modules".

However we haven't ported this code for the staff interface, which is
also using Coce.

Note that there was a bug at the OPAC as well for COCE, the ajax jQuery
function is async, and so 'done' was set even if the call was not
finished.

Test plan:
0. Do not apply this patch
1. Enable LocalCoverImages and IntranetCoce
2. Pick a bibliographic record with an image returned from Coce
3. Add a local cover image
4. Go to the detail page
5. Open the gallery
=> Notice that you can notice a JS error and a loading icon on the
second image. This behaviour is not always broken, it depends on the
speed of the Coce server. If you don't see the problem reload the page.
6. Apply the patch and repeat
=> Notice that the cover slider block is longer to display the icon, but
that the error is gone.
7. Confirm that there is no regression at the OPAC

We could improve the situation here, and we could display the images
in the slider, especially if the first image is a local image. But
that's for another bug...

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2023-02-17 10:23:48 UTC
Created attachment 146812 [details] [review]
Bug 32307: (follow-up) Fix gallery when Coce is enabled

Yet another fix, this is porting the following change to the intranet:
  commit 1916c644808577beb3170d642f93b456bfcc5c56
  Bug 28180: Fix bug with first image does not exist

To recreate the problem:
Enable Coce
Go to biblionmber=5 detail
=> No image (but one is displayed on the search result)
Add a local cover image
=> Only the local cover image is displayed
Apply this patch
=> Both images are displayed
Remove the local cover image
=> Image from coce is displayed

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Tomás Cohen Arazi 2023-02-20 12:54:52 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 21 Jonathan Druart 2023-02-23 11:10:11 UTC
Created attachment 147226 [details] [review]
Bug 32307: Fix local cover images

We were not calling the function here!
Comment 22 Tomás Cohen Arazi 2023-02-23 11:13:53 UTC
Thanks for the follow-up.
Comment 23 Martin Renvoize 2023-02-24 08:31:36 UTC
Many hands makes light work, thankyou everyone!

Pushed to 22.11.x for the next release
Comment 24 Martin Renvoize 2023-02-27 13:38:31 UTC
Many hands makes light work, thankyou everyone!

Pushed to 22.11.x for the next release
Comment 25 Lucas Gass 2023-03-01 14:51:43 UTC
Backported to 22.05.x for upcoming 22.05.11
Comment 26 Arthur Suzuki 2023-03-29 09:21:57 UTC
applied to 21.11.x for 21.11.19
Comment 27 wainuiwitikapark 2023-04-27 04:31:19 UTC
Not backported to 21.05.x