Bug 26037 - openlibrary.org is hit on every Koha requests
Summary: openlibrary.org is hit on every Koha requests
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Owen Leonard
QA Contact: Katrin Fischer
URL:
Keywords: RM_priority
Depends on:
Blocks:
 
Reported: 2020-07-22 08:51 UTC by Jonathan Druart
Modified: 2020-08-06 09:38 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00


Attachments
Bug 26037: openlibrary.org is hit on every Koha requests (7.16 KB, patch)
2020-07-27 14:07 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 26037: openlibrary.org is hit on every Koha requests (7.21 KB, patch)
2020-07-31 22:08 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 26037: openlibrary.org is hit on every Koha requests (7.27 KB, patch)
2020-08-05 15:36 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-07-22 08:51:15 UTC
If OpenLibrary sysprefs are enabled, a query is done on every page we load.
If the Koha page is not opac-detail or opac-result, a 404 is returned (the URL is not correctly generated).

To recreate: 
Turn on OpenLibraryCovers and OpenLibrarySearch
Hit the Koha main page
Note you have a 404 in the console:
https://openlibrary.org/api/volumes/brief/json?callback=jQuery34106125725859943835_1595407548302&show_all_items=true&_=1595407548303
Comment 1 Jonathan Druart 2020-07-22 08:52:36 UTC
We should either test the existence of ol_readapi_book, or move the inclusion of openlibrary.js to opac-detail and opac-result
Comment 2 Jonathan Druart 2020-07-22 08:53:54 UTC
To confirm:
1. Is staff impacted as well?
2. What about the other services we include in opac-bottom.inc?
Comment 3 Martin Renvoize 2020-07-22 09:16:42 UTC
I was a little shocked to see how much we include in -bottom.inc when I recently QA'd something.. I feel like we should really only load certain libraries where we actually use them and only when we reach a certain threshold should they be moved to a more system wide load location like that opac-bottom.inc.

So.. my feeling here would be to move them into just opac-detail and opac-result.
Comment 4 Martin Renvoize 2020-07-22 09:51:56 UTC
See bug 24201 comment 59; Although in that case we're conditionally loading based on a template variable, I felt it would be clearer to just load the Asset via the Asset plugin on the page rather than set a variable and have the asset loaded via an include.
Comment 5 David Cook 2020-07-23 04:24:32 UTC
This one does drive me a bit nuts... so it would be great to see a fix.
Comment 6 Owen Leonard 2020-07-24 19:55:35 UTC
The section of openlibrary.js related to the Read API seems to be to blame, but I'm not sure why. This raises the question... Has this feature ever worked?
Comment 7 Jonathan Druart 2020-07-27 09:49:10 UTC
(In reply to Owen Leonard from comment #6)
> The section of openlibrary.js related to the Read API seems to be to blame,
> but I'm not sure why. This raises the question... Has this feature ever
> worked?

Yes, it currently works: https://snipboard.io/Ya7SbK.jpg
It's the "Read now" button.
Comment 8 Owen Leonard 2020-07-27 12:25:11 UTC
(In reply to Jonathan Druart from comment #7)
> Yes, it currently works: https://snipboard.io/Ya7SbK.jpg

Thanks Jonathan, try as I might I couldn't find a title which would show me the button! Now I see it.
Comment 9 Owen Leonard 2020-07-27 14:07:45 UTC
Created attachment 107429 [details] [review]
Bug 26037: openlibrary.org is hit on every Koha requests

This patch moves the inclusion of Open Library JavaScript from
opac-bottom.inc to the three templates where it might be used.

In addition, the Open Library JS has been modified to check for the
existence of the relevant markup on the page before executing a query.

Unrelated: The Open Library label is now hidden when the status returned
is "Not found." Previously this information was displayed.

To test, apply the patch and enable the OpenLibrarySearch system
preference.

 - Perform a search in the OPAC which will return results from Open
   Library. Some ISBNs: 0140423826, 0395082552, 0679436405.
   - On the search result page there should be an Open Library "Read
     now" button for each result that can be opened for reading in Open
     Library.
 - Add these titles to a list and view the list.
   - The "Read now" button should appear on the list page.
 - View the details for one of these titles.
   - The "Read now" button should appear on the page below the holdings.
 - Watch the browser's developer tools on these and other pages in the
   OPAC. There should be no errors in the console and no 404 errors in
   the network tab.
Comment 10 Lucas Gass 2020-07-31 22:08:07 UTC
Created attachment 107658 [details] [review]
Bug 26037: openlibrary.org is hit on every Koha requests

This patch moves the inclusion of Open Library JavaScript from
opac-bottom.inc to the three templates where it might be used.

In addition, the Open Library JS has been modified to check for the
existence of the relevant markup on the page before executing a query.

Unrelated: The Open Library label is now hidden when the status returned
is "Not found." Previously this information was displayed.

To test, apply the patch and enable the OpenLibrarySearch system
preference.

 - Perform a search in the OPAC which will return results from Open
   Library. Some ISBNs: 0140423826, 0395082552, 0679436405.
   - On the search result page there should be an Open Library "Read
     now" button for each result that can be opened for reading in Open
     Library.
 - Add these titles to a list and view the list.
   - The "Read now" button should appear on the list page.
 - View the details for one of these titles.
   - The "Read now" button should appear on the page below the holdings.
 - Watch the browser's developer tools on these and other pages in the
   OPAC. There should be no errors in the console and no 404 errors in
   the network tab.

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 11 Katrin Fischer 2020-08-05 15:36:03 UTC
Created attachment 107842 [details] [review]
Bug 26037: openlibrary.org is hit on every Koha requests

This patch moves the inclusion of Open Library JavaScript from
opac-bottom.inc to the three templates where it might be used.

In addition, the Open Library JS has been modified to check for the
existence of the relevant markup on the page before executing a query.

Unrelated: The Open Library label is now hidden when the status returned
is "Not found." Previously this information was displayed.

To test, apply the patch and enable the OpenLibrarySearch system
preference.

 - Perform a search in the OPAC which will return results from Open
   Library. Some ISBNs: 0140423826, 0395082552, 0679436405.
   - On the search result page there should be an Open Library "Read
     now" button for each result that can be opened for reading in Open
     Library.
 - Add these titles to a list and view the list.
   - The "Read now" button should appear on the list page.
 - View the details for one of these titles.
   - The "Read now" button should appear on the page below the holdings.
 - Watch the browser's developer tools on these and other pages in the
   OPAC. There should be no errors in the console and no 404 errors in
   the network tab.

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Jonathan Druart 2020-08-06 09:38:17 UTC
Pushed to master for 20.11, thanks to everybody involved!