Show any relevant results from OverDrive on the OPAC search. This is done by showing a link with "Found xx results in the library's OverDrive collection" at the top of opac-search.pl, and linking to a page that shows the full results. This requires an OverDrive developer account.
Created attachment 18360 [details] [review] Bug 10320 - Integrate OverDrive search into OPAC (1/2) This show results from the OverDrive ebook/audiobook service in the OPAC. To test: 1) Fill in the OverDrive system preferences (you will need an Overdrive client key, client secret and library ID). 2) Perform a search on the OPAC, verify that, if available, a link to results from OverDrive will appear at the top. 3) Click on the link, verify that the list of results appears.
Created attachment 18361 [details] [review] Bug 10320 - Integrate OverDrive search into OPAC (2/2) This followup adds the sysprefs and copies the relevant JS to CCSR.
A few remarks (you may choose to ignore): _request(): * you are only logging die() case, it may be good to log $response->as_string if !$response->success. GetOverDriveToken(): * m!! (or similar) is a tad easier to read then m,, * in warn $contents->{'error_description'} it is assumed that from_json() was successful Please let me know if you want to leave as is, I'll sign it off.
Created attachment 18687 [details] [review] Bug 10320 - Integrate OverDrive search into OPAC (1/2) This show results from the OverDrive ebook/audiobook service in the OPAC. To test: 1) Fill in the OverDrive system preferences (you will need an Overdrive client key, client secret and library ID). 2) Perform a search on the OPAC, verify that, if available, a link to results from OverDrive will appear at the top. 3) Click on the link, verify that the list of results appears.
Created attachment 18688 [details] [review] [SIGNED-OFF] Bug 10320 - Integrate OverDrive search into OPAC This show results from the OverDrive ebook/audiobook service in the OPAC. Added the sysprefs and copied the relevant JS to CCSR. Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed off without actually testing new functionality - was not able to obtain OverDrive key. OPAC search seems fine with the patch.
Henry Bankhead from Los Gatos Library tested this with our overdrive key and it works!
Thanks for that, I declare it signed-off for real.
Created attachment 18970 [details] [review] [Signed off] Bug 10320 - Integrate OverDrive search into OPAC This show results from the OverDrive ebook/audiobook service in the OPAC. Added the sysprefs and copied the relevant JS to CCSR. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Henry Bankhead <hbankhead@losgatosca.gov>
Created attachment 19225 [details] [review] Bug 10320 - Integrate OverDrive search into OPAC This show results from the OverDrive ebook/audiobook service in the OPAC. Added the sysprefs and copied the relevant JS to CCSR. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Henry Bankhead <hbankhead@losgatosca.gov> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
I am sorry, but I think there are various problems with this: - It missed unit tests for the new module. - It introduces untranslatable strings in the Javascript. - It should probably be using the sprite for displaying the stars (Owen has done work on having Babeltheque and the opacstarratings using it, so there are already star images available I think)
Created attachment 19493 [details] [review] Bug 10320 followup - Fix translation/CSS issues This switches to the alternate method of displaying stars, and makes the strings in the OverDrive JS translatable.
I got this error after doing a search: overdrive_proxy: Can't call method "set_in_cache" on an undefined value at /home/oleonard/kohaclone/C4/External/OverDrive.pm line 125
Created attachment 19773 [details] [review] Followup for bug 10320: minor bugfixes and style improvements Switch to the new method of showing star ratings. Also, fix some translation bugs, an error that occurred when caching was disabled and add a stub unit test.
The first patch has a typical updatedatabase conflict. The second fails with this error: fatal: sha1 information is lacking or useless (C4/External/OverDrive.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Followup for bug 10320: minor bugfixes and style improvements
Created attachment 19774 [details] [review] Bug 10320 - Integrate OverDrive search into OPAC This show results from the OverDrive ebook/audiobook service in the OPAC. Added the sysprefs and copied the relevant JS to CCSR. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Henry Bankhead <hbankhead@losgatosca.gov> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 19775 [details] [review] Followup for bug 10320: minor bugfixes and style improvements Switch to the new method of showing star ratings. Also, fix some translation bugs, an error that occurred when caching was disabled and add a stub unit test.
Created attachment 19884 [details] [review] [SIGNED-OFF] Bug 10320 - Integrate OverDrive search into OPAC This show results from the OverDrive ebook/audiobook service in the OPAC. Added the sysprefs and copied the relevant JS to CCSR. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Henry Bankhead <hbankhead@losgatosca.gov> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 19885 [details] [review] [SIGNED-OFF] Followup for bug 10320: minor bugfixes and style improvements Switch to the new method of showing star ratings. Also, fix some translation bugs, an error that occurred when caching was disabled and add a stub unit test. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
These patches work well in my tests. One minor thing needs a follow-up: Some of the copyright statements on the new scripts appear to be copypasta.
This looks nice and works well afaikt :) I am adding my passed QA to these patches, but we need a follow up: Please run the QA script and fix following problems and also the copyright statements Owen pointed out: FAIL C4/External/OverDrive.pm FAIL pod *** ERROR: =item without previous =over in file C4/External/OverDrive.pm FAIL koha-tmpl/opac-tmpl/prog/en/modules/opac-overdrive-search.tt FAIL forbidden patterns forbidden pattern: tab char (line 143) forbidden pattern: opac-tmpl should certainly replaced with [% interface %] (line 34) forbidden pattern: tab char (line 148) Also I'd think it would be nice to have a non-Javascript fall back. For example you could prodie a simple search link in case Javascript is disabled in the browser. Please switch back to signed off when the follow up is ready and I will take a final look (or someone else who beats me to it :) )
Created attachment 19886 [details] [review] [PASSED QA] Bug 10320 - Integrate OverDrive search into OPAC This show results from the OverDrive ebook/audiobook service in the OPAC. Added the sysprefs and copied the relevant JS to CCSR. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Henry Bankhead <hbankhead@losgatosca.gov> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 19887 [details] [review] [PASSED QA] Followup for bug 10320: minor bugfixes and style improvements Switch to the new method of showing star ratings. Also, fix some translation bugs, an error that occurred when caching was disabled and add a stub unit test. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 19989 [details] [review] QA followup for bug 10320 - Fix copyright and style errors This patch fixes a copypasta'd copyright statement, and some incorrect POD and indentation.
There's still a QA error, but talking with Owen, I believe that it's specious. The test is intended to remove /prog/ hardcoding, but the lines mentioned use the generic /opac-tmpl/lib directory.
Created attachment 20024 [details] [review] [PASSED QA] QA followup for bug 10320 - Fix copyright and style errors This patch fixes a copypasta'd copyright statement, and some incorrect POD and indentation. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Koha's license is GPL3+, not GPL2+. I also assume that opac/opac-overdrive-search.pl is copyright 2013 ByWater, not 2009 BibLibre. I will not push this without clarification of the licensing. I am willing to make the necessary follow-up patch to correct the headers, but I need somebody from ByWater to make the following confirmations in this bug: [1] That ByWater is licensing the code as GPL3+. [2] Whether opac/opac-overdrive-search.pl is actually copyright 2013 by ByWater. Setting to Failed QA pending those confirmations.
Confirmed with Brendan. Yes, this code is all licensed under GPLv3+ and copyright 2013 ByWater.
Pushed to master, along with follow-ups to correct the license, a typo in the name of one of the system preference, adding the new sysprefs to sysprefs.sql, and handling OverDrive authentication failures encountered by the proxy service a tick more gracefully than by tossing an internal server error. Thanks, Jesse!