Bug 10320

Summary: Show results from library's OverDrive collection in OPAC search
Product: Koha Reporter: Jesse Weaver <jweaver>
Component: OPACAssignee: Jesse Weaver <jweaver>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P4 CC: crohde, gmcharlt, hbankhead, katrin.fischer, nengard, srdjan, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 10486, 11959    
Attachments: Bug 10320 - Integrate OverDrive search into OPAC (1/2)
Bug 10320 - Integrate OverDrive search into OPAC (2/2)
Bug 10320 - Integrate OverDrive search into OPAC (1/2)
[SIGNED-OFF] Bug 10320 - Integrate OverDrive search into OPAC
[Signed off] Bug 10320 - Integrate OverDrive search into OPAC
Bug 10320 - Integrate OverDrive search into OPAC
Bug 10320 followup - Fix translation/CSS issues
Followup for bug 10320: minor bugfixes and style improvements
Bug 10320 - Integrate OverDrive search into OPAC
Followup for bug 10320: minor bugfixes and style improvements
[SIGNED-OFF] Bug 10320 - Integrate OverDrive search into OPAC
[SIGNED-OFF] Followup for bug 10320: minor bugfixes and style improvements
[PASSED QA] Bug 10320 - Integrate OverDrive search into OPAC
[PASSED QA] Followup for bug 10320: minor bugfixes and style improvements
QA followup for bug 10320 - Fix copyright and style errors
[PASSED QA] QA followup for bug 10320 - Fix copyright and style errors

Description Jesse Weaver 2013-05-23 22:41:20 UTC
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.
Comment 1 Jesse Weaver 2013-05-23 23:18:48 UTC Comment hidden (obsolete)
Comment 2 Jesse Weaver 2013-05-24 00:08:26 UTC Comment hidden (obsolete)
Comment 3 Srdjan Jankovic 2013-06-04 01:03:14 UTC
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.
Comment 4 Jesse Weaver 2013-06-05 22:30:41 UTC Comment hidden (obsolete)
Comment 5 Srdjan Jankovic 2013-06-06 01:02:04 UTC Comment hidden (obsolete)
Comment 6 Srdjan Jankovic 2013-06-06 01:03:45 UTC
Signed off without actually testing new functionality - was not able to obtain OverDrive key. OPAC search seems fine with the patch.
Comment 7 hbankhead 2013-06-06 22:14:27 UTC
Henry Bankhead from Los Gatos Library tested this with our overdrive key and it works!
Comment 8 Srdjan Jankovic 2013-06-06 23:19:42 UTC
Thanks for that, I declare it signed-off for real.
Comment 9 Melia Meggs 2013-06-13 22:56:52 UTC Comment hidden (obsolete)
Comment 10 Chris Cormack 2013-06-25 08:16:01 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2013-07-07 11:28:35 UTC
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)
Comment 12 Jesse Weaver 2013-07-08 21:34:38 UTC Comment hidden (obsolete)
Comment 13 Owen Leonard 2013-07-18 14:38:18 UTC
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
Comment 14 Jesse Weaver 2013-07-18 15:51:28 UTC Comment hidden (obsolete)
Comment 15 Owen Leonard 2013-07-18 16:15:20 UTC
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
Comment 16 Jesse Weaver 2013-07-18 16:28:10 UTC Comment hidden (obsolete)
Comment 17 Jesse Weaver 2013-07-18 16:28:21 UTC Comment hidden (obsolete)
Comment 18 Owen Leonard 2013-07-23 18:31:37 UTC Comment hidden (obsolete)
Comment 19 Owen Leonard 2013-07-23 18:33:48 UTC Comment hidden (obsolete)
Comment 20 Owen Leonard 2013-07-23 18:34:51 UTC
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.
Comment 21 Katrin Fischer 2013-07-23 19:25:46 UTC
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 :) )
Comment 22 Katrin Fischer 2013-07-23 19:27:14 UTC
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>
Comment 23 Katrin Fischer 2013-07-23 19:31:10 UTC
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>
Comment 24 Jesse Weaver 2013-07-30 16:59:47 UTC Comment hidden (obsolete)
Comment 25 Jesse Weaver 2013-07-30 17:01:54 UTC
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.
Comment 26 Katrin Fischer 2013-07-31 21:16:48 UTC
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>
Comment 27 Galen Charlton 2013-08-01 15:24:10 UTC
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.
Comment 28 Jesse Weaver 2013-08-02 16:52:05 UTC
Confirmed with Brendan. Yes, this code is all licensed under GPLv3+ and copyright 2013 ByWater.
Comment 29 Galen Charlton 2013-09-08 07:07:13 UTC
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!