Bug 31228 - Fix Coce JavaScript to hide single-pixel cover images in both the staff client detail and results pages
Summary: Fix Coce JavaScript to hide single-pixel cover images in both the staff clien...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-07-25 03:51 UTC by Alex Buckley
Modified: 2022-10-23 09:51 UTC (History)
4 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00,22.05.06,21.11.12


Attachments
Bug 31228: Intraneyt coce.js is not hiding the coce-coverimg div for single-pixel images (3.20 KB, patch)
2022-07-26 05:59 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 31228: Intranet coce.js is not hiding the coce-coverimg div for single-pixel images (3.19 KB, patch)
2022-07-26 06:01 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 31228: Intranet coce.js is not hiding the coce-coverimg div for single-pixel images (3.24 KB, patch)
2022-07-27 16:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 31228: Intranet coce.js is not hiding the coce-coverimg div for single-pixel images (3.18 KB, patch)
2022-08-14 23:02 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 31228: (Alternate patch) Intranet coce.js is not hiding the coce-coverimg div for single-pixel image (3.19 KB, patch)
2022-08-14 23:04 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 31228: Intranet coce.js is not hiding the coce-coverimg div for single-pixel image (3.28 KB, patch)
2022-08-16 21:46 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2022-07-25 03:51:20 UTC
As reported by Jonathan in bug 31217 koha-tmpl/intranet-tmpl/js/coce.js hides the coce-coverimg class.

- In koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt coce-coverimg is a class.

- In koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt coce-coverimg is an ID, not a class. 

As ID's are more specific, coce-coverimg should be changed to an ID in koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt

And koha-tmpl/intranet-tmpl/js/coce.js should be changed to: $("#coce-coverimg").remove();
Comment 1 Jonathan Druart 2022-07-25 07:05:22 UTC
results will have several nodes, you cannot use an id there.
Comment 2 Alex Buckley 2022-07-25 07:12:18 UTC
Oh course, thanks Jonathan.

So we should aim to make coce-coverimg a class in koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt, correct?
Comment 3 Jonathan Druart 2022-07-25 07:54:07 UTC
That's certainly the best we can do, yes.
We could also search for the ID and fallback to class if it does not exist.

The main challenge in my opinion is to keep both opac and staff code as similar as possible (ie. switch to class for the OPAC if we decide so for staff)
Comment 4 Alex Buckley 2022-07-25 23:31:14 UTC
(In reply to Jonathan Druart from comment #3)
> That's certainly the best we can do, yes.
> We could also search for the ID and fallback to class if it does not exist.
> 
> The main challenge in my opinion is to keep both opac and staff code as
> similar as possible (ie. switch to class for the OPAC if we decide so for
> staff)

Ahh yep, thanks for that Jonathan!
Comment 5 Alex Buckley 2022-07-26 05:59:53 UTC Comment hidden (obsolete)
Comment 6 Alex Buckley 2022-07-26 06:01:02 UTC
Created attachment 138113 [details] [review]
Bug 31228: Intranet coce.js is not hiding the coce-coverimg div for single-pixel images

This is because the intranet detail.tt has coce-coverimg as an ID. Intranet results.tt has coce-coverimg as a class. Intranet coce.js targets coce-coverimg as a class.

Intranet detail.tt should be changed to have coce-coverimg as a class.
This is inline with the change made to the OPAC in bug 31217

Test plan:
1. Set your system preferences:
- CoceHost = https://coce.mykoha.co.nz
- CoceProviders = Select all
- IntranetCoce = Enable

2. Add the ISBN of '9780262336499' to a biblio

3. Open a "Incognito" window in your browser (recommended to use Google
Chrome to test this)

4. View the biblio detail page in the intranet. Observe a 'Coce image from
Amazon' box loads with a spinner

5. Apply patch and restart services

6. Clear your browser cache

7. Reload the same biblio detail page in the intranet. Observe no image is
displayed.

8. Do an intranet search to get the biblio returned in search results
page. Confirm no cover image box is displayed

9. View the intranet search result and detail pages for a different
biblio that you know Coce should be displaying the cover image of and
confirm that it is correctly displayed with the source of that image listed: For
example 'Coce image from Amazon.com' is displayed

Sponsored-by: Catalyst IT, New Zealand
Comment 7 Owen Leonard 2022-07-27 16:52:32 UTC
Created attachment 138187 [details] [review]
Bug 31228: Intranet coce.js is not hiding the coce-coverimg div for single-pixel images

This is because the intranet detail.tt has coce-coverimg as an ID.
Intranet results.tt has coce-coverimg as a class. Intranet coce.js
targets coce-coverimg as a class.

Intranet detail.tt should be changed to have coce-coverimg as a class.
This is inline with the change made to the OPAC in bug 31217

Test plan:
1. Set your system preferences:
- CoceHost = https://coce.mykoha.co.nz
- CoceProviders = Select all
- IntranetCoce = Enable

2. Add the ISBN of '9780262336499' to a biblio

3. Open a "Incognito" window in your browser (recommended to use Google
Chrome to test this)

4. View the biblio detail page in the intranet. Observe a 'Coce image
from Amazon' box loads with a spinner

5. Apply patch and restart services

6. Clear your browser cache

7. Reload the same biblio detail page in the intranet. Observe no image
is displayed.

8. Do an intranet search to get the biblio returned in search results
page. Confirm no cover image box is displayed

9. View the intranet search result and detail pages for a different
biblio that you know Coce should be displaying the cover image of and
confirm that it is correctly displayed with the source of that image
listed: For example 'Coce image from Amazon.com' is displayed

Sponsored-by: Catalyst IT, New Zealand

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Fridolin Somers 2022-08-13 08:15:23 UTC
I see results.js contains :
  if( div.hasClass("coce-coverimg") ){

Would be better to use this instead of :
  else if( div.attr("class") == "cover-image coce-coverimg" ){

No ?
Comment 9 Alex Buckley 2022-08-14 23:02:14 UTC
Created attachment 139095 [details] [review]
Bug 31228: Intranet coce.js is not hiding the coce-coverimg div for single-pixel images

This is because the intranet detail.tt has coce-coverimg as an ID.
Intranet results.tt has coce-coverimg as a class. Intranet coce.js
targets coce-coverimg as a class.

Intranet detail.tt should be changed to have coce-coverimg as a class.
This is inline with the change made to the OPAC in bug 31217

Test plan:
1. Set your system preferences:
- CoceHost = https://coce.mykoha.co.nz
- CoceProviders = Select all
- IntranetCoce = Enable

2. Add the ISBN of '9780262336499' to a biblio

3. Open a "Incognito" window in your browser (recommended to use Google
Chrome to test this)

4. View the biblio detail page in the intranet. Observe a 'Coce image
from Amazon' box loads with a spinner

5. Apply patch and restart services

6. Clear your browser cache

7. Reload the same biblio detail page in the intranet. Observe no image
is displayed.

8. Do an intranet search to get the biblio returned in search results
page. Confirm no cover image box is displayed

9. View the intranet search result and detail pages for a different
biblio that you know Coce should be displaying the cover image of and
confirm that it is correctly displayed with the source of that image
listed: For example 'Coce image from Amazon.com' is displayed

Sponsored-by: Catalyst IT, New Zealand
Comment 10 Alex Buckley 2022-08-14 23:04:52 UTC
Created attachment 139096 [details] [review]
Bug 31228: (Alternate patch) Intranet coce.js is not hiding the coce-coverimg div for single-pixel image

This is because the intranet detail.tt has coce-coverimg as an ID.
Intranet results.tt has coce-coverimg as a class. Intranet coce.js
targets coce-coverimg as a class.

Intranet detail.tt should be changed to have coce-coverimg as a class.
This is inline with the change made to the OPAC in bug 31217

Test plan:
1. Set your system preferences:
- CoceHost = https://coce.mykoha.co.nz
- CoceProviders = Select all
- IntranetCoce = Enable

2. Add the ISBN of '9780262336499' to a biblio

3. Open a "Incognito" window in your browser (recommended to use Google
Chrome to test this)

4. View the biblio detail page in the intranet. Observe a 'Coce image
from Amazon' box loads with a spinner

5. Apply patch and restart services

6. Clear your browser cache

7. Reload the same biblio detail page in the intranet. Observe no image
is displayed.

8. Do an intranet search to get the biblio returned in search results
page. Confirm no cover image box is displayed

9. View the intranet search result and detail pages for a different
biblio that you know Coce should be displaying the cover image of and
confirm that it is correctly displayed with the source of that image
listed: For example 'Coce image from Amazon.com' is displayed

Sponsored-by: Catalyst IT, New Zealand
Comment 11 Alex Buckley 2022-08-14 23:06:04 UTC
(In reply to Fridolin Somers from comment #8)
> I see results.js contains :
>   if( div.hasClass("coce-coverimg") ){
> 
> Would be better to use this instead of :
>   else if( div.attr("class") == "cover-image coce-coverimg" ){
> 
> No ?

Hi Fridolin, 

I have attached an alternate patch, is that what you had in mind regarding the above?

Thanks,
Alex
Comment 12 Fridolin Somers 2022-08-16 21:46:40 UTC
Created attachment 139217 [details] [review]
Bug 31228: Intranet coce.js is not hiding the coce-coverimg div for single-pixel image

This is because the intranet detail.tt has coce-coverimg as an ID.
Intranet results.tt has coce-coverimg as a class. Intranet coce.js
targets coce-coverimg as a class.

Intranet detail.tt should be changed to have coce-coverimg as a class.
This is inline with the change made to the OPAC in bug 31217

Test plan:
1. Set your system preferences:
- CoceHost = https://coce.mykoha.co.nz
- CoceProviders = Select all
- IntranetCoce = Enable

2. Add the ISBN of '9780262336499' to a biblio

3. Open a "Incognito" window in your browser (recommended to use Google
Chrome to test this)

4. View the biblio detail page in the intranet. Observe a 'Coce image
from Amazon' box loads with a spinner

5. Apply patch and restart services

6. Clear your browser cache

7. Reload the same biblio detail page in the intranet. Observe no image
is displayed.

8. Do an intranet search to get the biblio returned in search results
page. Confirm no cover image box is displayed

9. View the intranet search result and detail pages for a different
biblio that you know Coce should be displaying the cover image of and
confirm that it is correctly displayed with the source of that image
listed: For example 'Coce image from Amazon.com' is displayed

Sponsored-by: Catalyst IT, New Zealand

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 13 Fridolin Somers 2022-08-16 21:48:08 UTC
(In reply to Alex Buckley from comment #11)
> (In reply to Fridolin Somers from comment #8)
> > I see results.js contains :
> >   if( div.hasClass("coce-coverimg") ){
> > 
> > Would be better to use this instead of :
> >   else if( div.attr("class") == "cover-image coce-coverimg" ){
> > 
> > No ?
> 
> Hi Fridolin, 
> 
> I have attached an alternate patch, is that what you had in mind regarding
> the above?
> 
> Thanks,
> Alex

Yes perfect.

I've passed QA this patch, obsoleted the other and removed the "alternate" text in commit message.
I also reported the signoff line from Owen.

Best regards ;)
Comment 14 Alex Buckley 2022-08-16 21:53:04 UTC
(In reply to Fridolin Somers from comment #13)
> (In reply to Alex Buckley from comment #11)
> > (In reply to Fridolin Somers from comment #8)
> > > I see results.js contains :
> > >   if( div.hasClass("coce-coverimg") ){
> > > 
> > > Would be better to use this instead of :
> > >   else if( div.attr("class") == "cover-image coce-coverimg" ){
> > > 
> > > No ?
> > 
> > Hi Fridolin, 
> > 
> > I have attached an alternate patch, is that what you had in mind regarding
> > the above?
> > 
> > Thanks,
> > Alex
> 
> Yes perfect.
> 
> I've passed QA this patch, obsoleted the other and removed the "alternate"
> text in commit message.
> I also reported the signoff line from Owen.
> 
> Best regards ;)

Thanks very much Fridolin!
Comment 15 Alex Buckley 2022-08-16 22:02:00 UTC
Also, just to note I have updated the OPAC coce fix ( bug 31217 ) to be consistent in using: 

else if( div.hasClass("coce-coverimg" ) ){
Comment 16 Tomás Cohen Arazi 2022-08-17 13:27:54 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 17 Lucas Gass 2022-10-03 22:30:11 UTC
Backported to 22.05.x for 22.05.06
Comment 18 Victor Grousset/tuxayo 2022-10-16 21:23:58 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.
Comment 19 Arthur Suzuki 2022-10-22 22:09:01 UTC
Thanks!

Pushed to 21.11 for 21.11.12