Summary: | Chocolat image viewer broken in the staff interface when Coce is enabled | ||
---|---|---|---|
Product: | Koha | Reporter: | Owen Leonard <oleonard> |
Component: | Templates | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | normal | ||
Priority: | P5 - low | CC: | arthur.suzuki, david, jonathan.druart, lucas, m.de.rooy, martin.renvoize, tomascohen, wainuiwitikapark |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
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
|
|
Circulation function: | |||
Bug Depends on: | 28179 | ||
Bug Blocks: | |||
Attachments: |
Bug 32307: Fix gallery when Coce is enabled
Bug 32307: Fix gallery when Coce is enabled Bug 32307: (follow-up) Fix gallery when Coce is enabled Bug 32307: Fix gallery when Coce is enabled Bug 32307: (follow-up) Fix gallery when Coce is enabled Bug 32307: Fix gallery when Coce is enabled Bug 32307: (follow-up) Fix gallery when Coce is enabled Bug 32307: Fix local cover images |
Description
Owen Leonard
2022-11-21 16:24:51 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? Created attachment 144142 [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... Created attachment 144220 [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> 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. Created attachment 144222 [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 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. 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> 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> 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? Hm, same when Coce is disabled: emptry frame. No errors in the console. restarted_all and also used a private tab. (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. 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? + var loaded = 1; + counter_wait++; + + if ( loaded ) { + loaded = KOHA.coce.done; + } Strange pattern? Why not var loaded = KOHA.coce.done ? 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? (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. (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 :) + 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.. 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> 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> Pushed to master for 23.05. Nice work everyone, thanks! Created attachment 147226 [details] [review] Bug 32307: Fix local cover images We were not calling the function here! Thanks for the follow-up. Many hands makes light work, thankyou everyone! Pushed to 22.11.x for the next release Many hands makes light work, thankyou everyone! Pushed to 22.11.x for the next release Backported to 22.05.x for upcoming 22.05.11 applied to 21.11.x for 21.11.19 Not backported to 21.05.x |