I did some testing of this today (and blogged about it: http://galencharlton.com/blog/2013/08/playing-around-with-coce/).  The upshot is that it works for me, and I'm not averse to the general idea, but I have a few concerns:

- It should be able to function under HTTPS without mixed content warnings, and that is not the case at the moment
- I think that the Coce client should be able to specify the image size rather than have the Coce server set it -- this would allow, for example, OPAC results to use small images and the OPAC details page to use large ones.
- It would be nice if this were available in the staff interface as well as the OPAC.
- The patch should add the new sysprefs to installer/data/mysql/sysprefs.sql and installer/data/mysql/updatedatabase.pl

Comment on attachment 15203 [details] [review]
Proposed patch

Review of attachment 15203 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt
@@ +305,5 @@
>  
> +[% IF ( CoceProviders ) %]
> +KOHA.coce.getURL('[% CoceHost %]', '[% CoceProviders %]');
> +[% END %]
> +

This was here from the beginning.

Comment on attachment 16778 [details] [review]
Proposed patch (v2)

Review of attachment 16778 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt
@@ +305,5 @@
>  
> +[% IF ( CoceProviders ) %]
> +KOHA.coce.getURL('[% CoceHost %]', '[% CoceProviders %]');
> +[% END %]
> +

This disappeared.

Comment on attachment 21162 [details] [review]
Proposed patch (v3 rebased on master)

Review of attachment 21162 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt
@@ +333,4 @@
>      [% IF OPACLocalCoverImages %]KOHA.LocalCover.GetCoverFromBibnumber(false);[% END %]
>      [% IF ( GoogleJackets ) %]KOHA.Google.GetCoverFromIsbn();[% END %]
>  
> +<<<<<<< HEAD

I think a rebase didn't quite work.

> > +<<<<<<< HEAD
> 
> I think a rebase didn't quite work.

It doesn't look good at all! I fix that, and add markup to Bootstrap theme.

All the covers are the same with or without Coce. 
And the exact same covers are retrieved (no more, no less...)
With 'Bootstrap' and 'prog'.


At one time there was 2 covers for the same biblios (one slightly bigger than the other one. For example : 
http://ecx.images-amazon.com/images/I/613QE5KV4YL._SL160_.jpg
and
http://images.amazon.com/images/P/2700036336.01.TZZZZZZZ.jpg
Nevertheless it looks like it's an random accident since I didn't manage to re-create this error...

Thanks for testing.

> All the covers are the same with or without Coce.  And the exact same
> covers are retrieved (no more, no less...)

I may depend of your database. Do you have biblio records with ISBN-13?
Do you have bilio records for DVD/CD-Audio? There are not retrieved with
Amazon 'standard' cover but should be retrieved with Coce.

Created attachment 24145 [details]
screenshot with 2 cover

screenshot with 2 cover

Same thing with CD and ISBN-13.
And several biblios have double cover on search results

Also, with the one book with a cover from Coce, the bootstrap theme does not display this cover. Just an empty space under the "no image available" from Amazon.

With the dumping of the prog theme, you may wish to split out the prog portion of this patch.

(In reply to M. Tompsett from comment #19)
> With the dumping of the prog theme, you may wish to split out the prog
> portion of this patch.

Done. I still works. For me, at least. Don't hesitate to ask for a Coce server URL...

Comment on attachment 32795 [details] [review]
Bug 9580 Cover images from Coce, a remote image URL cache

Review of attachment 32795 [details] [review]:
-----------------------------------------------------------------

::: C4/Auth.pm
@@ +351,5 @@
>              "BiblioDefaultView".C4::Context->preference("BiblioDefaultView")         => 1,
>              EnhancedMessagingPreferences => C4::Context->preference('EnhancedMessagingPreferences'),
>              GoogleJackets                => C4::Context->preference("GoogleJackets"),
> +            CoceHost                     => C4::Context->preference("CoceHost"),
> +            CoceProviders                => C4::Context->preference("CoceProviders"),

Rather than add more parameters, why not modify the template to have: [%- USE Koha -%]
And then use the plugin function to grab it directly.
Less modifications are good. :)

::: koha-tmpl/opac-tmpl/prog/en/includes/doc-head-close.inc
@@ +113,5 @@
>  </script>
>  [% END %]
> +[% IF CoceProviders %]
> +<script type="text/javascript" src="[% themelang %]/js/coce.js"></script>
> +[% END %]

This is OPAC prog.

(In reply to M. Tompsett from comment #23)

> Rather than add more parameters, why not modify the template to have: [%-
> USE Koha -%]
> And then use the plugin function to grab it directly.
> Less modifications are good. :)

I agree. Thanks for the tip. I wasn't aware if this Koha TT extension. I refactored the patch to use it.

> ::: koha-tmpl/opac-tmpl/prog/en/includes/doc-head-close.inc
> This is OPAC prog.

Cleaned now. Thanks.

Comment on attachment 32808 [details] [review]
Bug 9580 Cover images from Coce, a remote image URL cache

Review of attachment 32808 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt
@@ +257,5 @@
>                                                              [% END %]
>                                                          [% END %]
>  
> +                                                        [% IF Koha.Preference('CoceProviders') %]
> +                                                          [% IF ( SEARCH_RESULT.normalized_ean || SEARCH_RESULT.normalized_isbn ) %]

Split this into an IF/ELSIF/ELSE
(http://template-toolkit.org/docs/manual/Directives.html#section_IF_UNLESS_ELSIF_ELSE)
So that you can more plainly meet
http://wiki.koha-community.org/wiki/Coding_Guidelines#HTML1:_Template_Toolkit_markup_inside_HTML

@@ +258,5 @@
>                                                          [% END %]
>  
> +                                                        [% IF Koha.Preference('CoceProviders') %]
> +                                                          [% IF ( SEARCH_RESULT.normalized_ean || SEARCH_RESULT.normalized_isbn ) %]
> +                                                            <span style="block" title="[% SEARCH_RESULT.biblionumber |url %]" class="[% IF ( SEARCH_RESULT.normalized_ean ) %][% SEARCH_RESULT.normalized_ean %][% ELSE %][% SEARCH_RESULT.normalized_isbn %][% END %]" id="coce-thumbnail[% loop.count %]"></span>

Because this is hard to read.

Comment on attachment 32808 [details] [review]
Bug 9580 Cover images from Coce, a remote image URL cache

Review of attachment 32808 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/enhanced_content.pref
@@ +344,5 @@
> +              class: url
> +        -
> +            - Providers
> +            - pref: CoceProviders
> +            - 'separated by comma, for example: aws,gb. Leave empty not to activate this service.'

If this saves in the systempreferences table, shouldn't there be updatedatabase.pl, systemprefs.sql update too?

Comment on attachment 32808 [details] [review]
Bug 9580 Cover images from Coce, a remote image URL cache

Review of attachment 32808 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc
@@ +166,4 @@
>          //]]>
>      </script>
>  [% END %]
> +[% IF CoceProviders %]

CoceProviders isn't defined here, is it?

(In reply to M. Tompsett from comment #28)

> If this saves in the systempreferences table, shouldn't there be
> updatedatabase.pl, systemprefs.sql update too?

I don't think so. No value are required at all. By default, nothing is stored in systempreferences table, and the functionality is just disabled.

(In reply to Frédéric Demians from comment #31)
> (In reply to M. Tompsett from comment #28)
> 
> > If this saves in the systempreferences table, shouldn't there be
> > updatedatabase.pl, systemprefs.sql update too?
> 
> I don't think so. No value are required at all. By default, nothing is
> stored in systempreferences table, and the functionality is just disabled.

http://wiki.koha-community.org/wiki/System_Preferences#Adding_a_new_system_preference
I think you do. You are correct that it functions without it, but we should be explicit.

Oh, and Amazon preferences also seem to affect this.
I get an Amazon cover image with the other system preferences, but not the Coce ones. Though, it isn't the "No cover image" like the others. :)
Green Book of Language Revitalization in Practice (The)

> http://wiki.koha-community.org/wiki/
> System_Preferences#Adding_a_new_system_preference
> I think you do. You are correct that it functions without it, but we should
> be explicit.

I updated the wiki, accordingly with my thought on the subject. Feel free to disagree, and to revert my wiki update.

(In reply to M. Tompsett from comment #33)
> Oh, and Amazon preferences also seem to affect this.
> I get an Amazon cover image with the other system preferences, but not the
> Coce ones. Though, it isn't the "No cover image" like the others. :)
> Green Book of Language Revitalization in Practice (The)

I don't understand. Do you mean, you have enabled simultaneously Koha OPACAmazonCoverImages syspref, and Coce sysprefs? You shouldn't.

Owen Leonard and Marcel de Rooy agree with my understanding of adding system preferences, as far as I can tell. I'm going to mark this Failed QA.

Also, could you give me an ISBN of something that should work with your patch? Or perhaps a Coce server? I'd include that in your test plan.

(In reply to M. Tompsett from comment #36)
> Owen Leonard and Marcel de Rooy agree with my understanding of adding system
> preferences, as far as I can tell. I'm going to mark this Failed QA.
> 
> Also, could you give me an ISBN of something that should work with your
> patch? Or perhaps a Coce server? I'd include that in your test plan.

Thanks for your time. I will go along with the majority...

Comment on attachment 32857 [details] [review]
Bug 9580 Cover images from Coce, a remote image URL cache

Review of attachment 32857 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/sysprefs.sql
@@ +86,5 @@
>  ('CircAutocompl','1',NULL,'If ON, autocompletion is enabled for the Circulation input','YesNo'),
>  ('CircAutoPrintQuickSlip','qslip',NULL,'Choose what should happen when an empty barcode field is submitted in circulation: Display a print quick slip window, Display a print slip window or Clear the screen.','Choice'),
>  ('CircControl','ItemHomeLibrary','PickupLibrary|PatronLibrary|ItemHomeLibrary','Specify the agency that controls the circulation and fines policy','Choice'),
> +('CoceHost', 0, NULL, 'Coce server URL', 'Free'),
> +('CoceProviders', 0, NULL, 'Coce providers, for example aws,gb', 'Free'),

This should be empty string ('') or NULL (which is what you put in the updatedatabase.pl) not 0!

Personally, I'm torn, because I checked out other URL entries in updatedatabase.pl and I have seen it both ways (NULL and empty string). That is, there is no standard. :(

Comment on attachment 32857 [details] [review]
Bug 9580 Cover images from Coce, a remote image URL cache

Review of attachment 32857 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt
@@ +79,5 @@
>                      [% IF ( GoogleJackets ) %]
>                          <div title="[% biblionumber |url %]" class="[% normalized_isbn %]" id="gbs-thumbnail-preview"></div>
>                      [% END %]
> +                    [% IF Koha.Preference('CoceProviders') %]
> +                      <div style="block" title="[% biblionumber |url %]" class="[% IF ( normalized_ean ) %][% normalized_ean %][% ELSE %][% normalized_isbn %][% END %]" id="coce-thumbnail-preview"></div>

Split this:

[% IF (normalized_ean) %]
<div style="block" title="[% biblionumber |url %]" class="[% normalized_ean %]" id="coce-thumbnail-preview"></div>
[% ELSE %]
<div style="block" title="[% biblionumber |url %]" class="[% normalized_isbn %]" id="coce-thumbnail-preview"></div>
[% END %]

So that you can more plainly meet
http://wiki.koha-community.org/wiki/Coding_Guidelines#HTML1:_Template_Toolkit_markup_inside_HTML

OOooooo.... sexier code:
<div style="block" title="[% biblionumber |url %]" class="[% normalized_ean || normalized_isbn %]" id="coce-thumbnail-preview"></div>
It's precise, concise, and oh so elegant.

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt
@@ +258,5 @@
>                                                          [% END %]
>  
> +                                                        [% IF Koha.Preference('CoceProviders') %]
> +                                                          [% IF ( SEARCH_RESULT.normalized_ean || SEARCH_RESULT.normalized_isbn ) %]
> +                                                            <span style="block" title="[% SEARCH_RESULT.biblionumber |url %]" class="[% IF ( SEARCH_RESULT.normalized_ean ) %][% SEARCH_RESULT.normalized_ean %][% ELSE %][% SEARCH_RESULT.normalized_isbn %][% END %]" id="coce-thumbnail[% loop.count %]"></span>

class="[% SEARCH_RESULT.normalized_ean || SEARCH_RESULT.normalized_isbn %]"
-- It's sexier. :)

But perhaps the IF/ELSE split may be necessary. I've asked the mailing list.

(In reply to M. Tompsett from comment #39)

> ::: installer/data/mysql/sysprefs.sql
> @@ +86,5 @@
> > +('CoceHost', 0, NULL, 'Coce server URL', 'Free'),
> > +('CoceProviders', 0, NULL, 'Coce providers, for example aws,gb', 'Free'),
> 
> This should be empty string ('') or NULL (which is what you put in the
> updatedatabase.pl) not 0!
> 
> Personally, I'm torn, because I checked out other URL entries in
> updatedatabase.pl and I have seen it both ways (NULL and empty string). That
> is, there is no standard. :(

Ok. My reasoning was that in Perl code, syspref are tested like that:

  if ( $my_syspref ) { }

And 0 is the false value in Perl (like undef and ''). But I can comply to usage, of course.

> But perhaps the IF/ELSE split may be necessary. I've asked the mailing list.

I can alternatively propose:

[% IF Koha.Preference('CoceProviders') %]
 [% SET coce_id = SEARCH_RESULT.normalized_ean || SEARCH_RESULT.normalized_isbn %]
 [% IF ( coce_id ) %]
  <span style="block" title="[% SEARCH_RESULT.biblionumber |url %]" class="[% coce_id %]" id="coce-thumbnail[% loop.count %]"></span>
 [% ELSE %]
  <span class="no-image">No cover image available</span>
 [% END %]
[% END %]

(In reply to Frédéric Demians from comment #42)
> I can alternatively propose:
> 
> [% IF Koha.Preference('CoceProviders') %]
>  [% SET coce_id = SEARCH_RESULT.normalized_ean ||
> SEARCH_RESULT.normalized_isbn %]
>  [% IF ( coce_id ) %]
>   <span style="block" title="[% SEARCH_RESULT.biblionumber |url %]"
> class="[% coce_id %]" id="coce-thumbnail[% loop.count %]"></span>
>  [% ELSE %]
>   <span class="no-image">No cover image available</span>
>  [% END %]
> [% END %]

I think that would work. Though,
http://template-toolkit.org/docs/manual/Syntax.html#section_Implicit_Directives_GET_and_SET
implies that SET is not necessary.

Remember, this type of fix is required in more than one location in the code. :)

After all this, I can't seem to get it to display anything. I'm having a sample data that works problem. I really want to sign this off, but until I see it work, I can't. :(

I'm running a MARC21 environment with the default MARC21 servers. Can you give me an EAN/ISBN that should import nicely from a Z39.50 search, so that this will work with your Coce image cache server? Or perhaps a Z39.50 server that would have MARC21 to import for the EAN/ISBN you already suggested?

Created attachment 32942 [details]
MARC21 DVD biblio records

It's an MARC21 ISO2709 file, containing about 700 biblios describing DVD. The records are roughly converted from Unimarc to Marc21. So I can't guaranty they are perfect. But you will have EAN in 024 ou 020 field, not ISBN.

I set up a fresh Unimarc git and used my sample Unimarc data, and I could not get it to work. :(

(In reply to M. Tompsett from comment #49)
> I set up a fresh Unimarc git and used my sample Unimarc data, and I could
> not get it to work. :(

I was attempting to firebug the javascript. For some reason success is never triggered on my VM's, but it is on your server. Also, the coce URL is different. But this is too late to try to make further sense of it.

(In reply to M. Tompsett from comment #50)
> (In reply to M. Tompsett from comment #49)
> > I set up a fresh Unimarc git and used my sample Unimarc data, and I could
> > not get it to work. :(
> 
> I was attempting to firebug the javascript. For some reason success is never
> triggered on my VM's, but it is on your server. Also, the coce URL is
> different. But this is too late to try to make further sense of it.

Does the GoogleBooks functionality works properly on you VM? GoogleBooks and Coce JavaScript code is similar, with JSONP and code injection.

Hypothesis: Your Koha is a VM running inside your PC-Windows. For your browser, your JS code is originating from your box, and is trying to get JavaScript from another site (Coce server). This is different from testing from a server on the Internet (my devel box). Two solutions: (1) you modify your security policy (Windows, or Virtualisation level), (2) you test from a Koha on the Internet (sandbox).

Mark: You can try on Biblibre sandboxes:

- Ask for an UNIMARC sandbox, apply bug 9580
- Connect to pro interface (test/test)
- Edit sysprefs:
  CoceHost: http://coce.tamil.fr:8080
  CoceProviders: gb,aws
  OPACAmazonCoverImages: Don't show
  opacthemes: Bootstrap
- Go to the OPAC interface
- Search for: C FR
- Book covers are displaying. Flying your moose pointer over images, you
  will note that they come both from Amazon & Google (result 26).
- You can try to add a DVD EAN to a biblio record: 3333297517744
  Edit a biblio record with DVD framework, and populate 073$a field.

Patch tested with a sandbox, by Mark Tompsett <mtompset@hotmail.com>

I use ol,gb,aws and even saw all three mixed in the results for "C FR".
Still can't get it to work on my personal VMs, because the success event doesn't trigger.

I'd like to thank you, Mark, for your tenacity. Your remarks, critics and guidance make a difference, and help a lot to get a polished result which comply with Koha standard.

Comment on attachment 33201 [details] [review]
Bug 9580 Cover images from Coce, a remote image URL cache

Review of attachment 33201 [details] [review]:
-----------------------------------------------------------------

Frédéric, Mark,
Lazily I tried with your host (coce.tamil.fr:8080), but it does not work.
It seems that the browser blocks the request to other servers, which seems logical.
Could you confirm or infirm that this worked for you?

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/svc/shelfbrowser.tt
@@ +1,1 @@
> +[% USE Koha %]

Why do you added this line in this file (and not in shelfbrowser.inc)?

::: koha-tmpl/opac-tmpl/bootstrap/js/coce.js
@@ +31,5 @@
> +          var url = urlPerID[id];
> +          $("[id^=coce-thumbnail]."+id).each(function() {
> +            var img = document.createElement("img");
> +            img.src = url;
> +            img.title = url; //FIXME: to delete

Should be removed?

Merci Jonathan pour les tests.  

> Lazily I tried with your host (coce.tamil.fr:8080), but it does not work.
> It seems that the browser blocks the request to other servers, which seems
> logical.
> Could you confirm or infirm that this worked for you?

Could you try rather with: http://coce.tamil.fr:8080

> ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/svc/shelfbrowser.tt
> @@ +1,1 @@
> > +[% USE Koha %]

> Why do you added this line in this file (and not in shelfbrowser.inc)?

You're correct correct. It could be suppressed.

> ::: koha-tmpl/opac-tmpl/bootstrap/js/coce.js
> @@ +31,5 @@
> > +          var url = urlPerID[id];
> > +          $("[id^=coce-thumbnail]."+id).each(function() {
> > +            var img = document.createElement("img");
> > +            img.src = url;
> > +            img.title = url; //FIXME: to delete
> 
> Should be removed?

Well, good question. With that last line, it's possible to see immediately where the image comes from by flying over it. In my opinion, this should be kept now, and deleted later, or replaced by: img.title = id;

(In reply to Frédéric Demians from comment #58)
> Merci Jonathan pour les tests.  
> 
> > Lazily I tried with your host (coce.tamil.fr:8080), but it does not work.
> > It seems that the browser blocks the request to other servers, which seems
> > logical.
> > Could you confirm or infirm that this worked for you?
> 
> Could you try rather with: http://coce.tamil.fr:8080

I confirm that it works with the http://

I tried to host the webservice, and it worked until I got this error:

% node webservice.js

/home/koha/tools/coce/node_modules/aws-lib/lib/aws.js:78
      if (err) throw err;
                     ^
Error: metadata API response undefined
    at ClientRequest.<anonymous> (/home/koha/tools/coce/node_modules/aws-lib/lib/aws.js:37:49)
    at ClientRequest.g (events.js:180:16)
    at ClientRequest.emit (events.js:92:17)
    at Socket.emitTimeout (http.js:1797:10)
    at Socket.g (events.js:180:16)
    at Socket.emit (events.js:92:17)
    at Socket._onTimeout (net.js:326:8)
    at Timer.unrefTimeout [as ontimeout] (timers.js:418:13)

Did you already get it?

Could you please provide a followup for the [% USE Koha %]?
I think you need to move it, not to remove it.

(In reply to Jonathan Druart from comment #60)
> I tried to host the webservice, and it worked until I got this error:
> 
> % node webservice.js
> 
> /home/koha/tools/coce/node_modules/aws-lib/lib/aws.js:78
>       if (err) throw err;
>                      ^
> Error: metadata API response undefined
>     at ClientRequest.<anonymous>
> (/home/koha/tools/coce/node_modules/aws-lib/lib/aws.js:37:49)
>     at ClientRequest.g (events.js:180:16)
>     at ClientRequest.emit (events.js:92:17)
>     at Socket.emitTimeout (http.js:1797:10)
>     at Socket.g (events.js:180:16)
>     at Socket.emit (events.js:92:17)
>     at Socket._onTimeout (net.js:326:8)
>     at Timer.unrefTimeout [as ontimeout] (timers.js:418:13)
> 
> Did you already get it?

No. To track down this bug, it is required to have the ISBN/EAN which is sent to AWS. 

A tool like Phusion Passenger can help avoiding crashing the whole service when such an error occurs.

> Could you please provide a followup for the [% USE Koha %]?
> I think you need to move it, not to remove it.

Done. Suppring the directive seems to be ok: covers are displayed in 3 locations: resultat page, detail page, and browse by location.

I've resubmitted the whole as one patch, keeping Mark sign-off.

Created attachment 34679 [details] [review]
Bug 9580 Cover images from Coce, a remote image URL cache

Add to Koha support for displaying Book cover requested to Coce, a
remote image URL cache:

  https://github.com/fredericd/coce

With this enhancement, cover images are not fetched directly from
Amazon, Google, and so on. Their URL are requested via a web service to
Coce which manages a cache of URLs.

Three cover image providers are now available: Google Books (gb), Amazon
(aws), and Open Library (ol). Two system preferences enable this service:

  - CoceHost - URL of Coce server
  - CoceProviders - Ordered list of requested providers. For example
    gb,aws,ol, will return the first available URL from those providers.

Several advantages to this architecture:

  - If a book cover is not available from a provider, but is available
    from another one, Koha will be able to display a book cover, which
    isn't the case now
  - Since URLs are cached, it isn't necessary for each book cover to
    request, again and again, the provider, and several of them if
    necessary.
  - Amazon book covers are retrieved with Amazon Product Advertising
    API, which means that more covers are retrieved (ISBN13).

Test plan:

- Apply this patch, and test with 'Bootstrap' themes
- You can verify that nothing has changed on OPAC result and detail
  page, since new syspref haven't been filled
- Install Code: https://github.com/fredericd/coce
  Or ask me directly for the URL of a Coce server
- In sysprefs, tab Enhanced content, modify:
  CoceHost -- enter the URL of your Coce server
  CoceProviders -- fill with: gb,aws,ol
- Do a search. On result page, and detail page, you will see cover
  images originating from the 3 providers: fly over the image to see its
  URL. Try to compare with a Koha with just GoogleJacket or Amazon cover
  to confirm that you have more images. Verify that it's quick, and even
  quicker for cached images URLs.
- Check that Browse Shelf functionnality works properly.
- The ID sent to Coce is EAN or ISBN. Try with various type of biblios. DVD
  have often EAN, but no ISBN.
- You can try with those sysprefs:
    CoceProviders: aws,gb
    CoceHost: http://coce.tamil.fr:8080
  and this EAN (or ISBN): 3333297517744
  => OPACAmazonCoverImages enabled doesn't display a cover because, it's a DVD
  => Coce find and display the DVD cover.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Amended patch: replaced 1 tab with spaces in C4/Shelfbrowser.pm

(In reply to Frédéric Demians from comment #62)
> (In reply to Jonathan Druart from comment #60)
> > I tried to host the webservice, and it worked until I got this error:
> > 
> > % node webservice.js
> > 
> > /home/koha/tools/coce/node_modules/aws-lib/lib/aws.js:78
> >       if (err) throw err;
> >                      ^
> > Error: metadata API response undefined
> >     at ClientRequest.<anonymous>
> > (/home/koha/tools/coce/node_modules/aws-lib/lib/aws.js:37:49)
> >     at ClientRequest.g (events.js:180:16)
> >     at ClientRequest.emit (events.js:92:17)
> >     at Socket.emitTimeout (http.js:1797:10)
> >     at Socket.g (events.js:180:16)
> >     at Socket.emit (events.js:92:17)
> >     at Socket._onTimeout (net.js:326:8)
> >     at Timer.unrefTimeout [as ontimeout] (timers.js:418:13)
> > 
> > Did you already get it?
> 
> No. To track down this bug, it is required to have the ISBN/EAN which is
> sent to AWS. 

I got it again, but it's completely random (I have got it with the same url I use). I don't know where it can come from.

However, it's the only problem I got, the code is clean and all works great. Nice feature!

Marked as Passed QA.

I can also confirm this is working well at the koha side, and that although the coce daemon itself presents that error mentioned by Jonathan periodically it doesn't appear to affect it's overall running.

Would be nice to see this one making it's way though :)

I'm willing to push this ASAP. Two questions:

- Do you think we could have a "Coce" syspref that could serve as a global switch instead of testing for CoceHost? That would make it consistent with how the rest of the enhanced content services are set in Koha.

- Does Coce provide a way to set custom providers? If it doesn't, we don't need a free text syspref, could we make the CoceProviders a 'multiple' type syspref? With fixed values: aws, gb and ol? [1]

I can just provide a followup implementing this changes if you want to, I only want your opinion before doing so.


[1] With proper descriptions. Check OpacAdvSearchOptions on the .pref file.

> I'm willing to push this ASAP. Two questions:

+1

> - Do you think we could have a "Coce" syspref that could serve as a global
> switch instead of testing for CoceHost? That would make it consistent with
> how the rest of the enhanced content services are set in Koha.

Ok. I do it and send an amended patch.

> - Does Coce provide a way to set custom providers? If it doesn't, we don't
> need a free text syspref, could we make the CoceProviders a 'multiple' type
> syspref? With fixed values: aws, gb and ol? [1]

For the time being, Coce support only 3 images providers. But the system is
designed to be extended, so it can change in the future. I'd recommend not to
impose choices in Koha system preference. Forcing choices would prevent using
new providers as soon as available.

Created attachment 37410 [details] [review]
(QA followup) add a Coce syspref and fix options for Providers

We could perhaps add a route to lookup available providers on the specified coce server?  That would allow koha's syspref to be 'clever' about presenting the available provider for your specific coce server.

I say this as for instance in the UK, I'm rather tempted to write a BDS connector for coce..  but BDS is uk only, so our coce instance would support it but others may not.

Any thoughts Frederic?

(In reply to Martin Renvoize from comment #70)
> I say this as for instance in the UK, I'm rather tempted to write a BDS
> connector for coce..  but BDS is uk only, so our coce instance would support
> it but others may not.

I always try to avoid letting the user making mistakes/breaking their configuration (and have features fully documented). That's the POV that motivated my followup change to 'multiple'.

We could have a CoceCustomProviders syspref that is free text.

I'd say, feel free to push as is.. I'll submit a followup to both coce and koha to show what I mean afterwards ;)

Martin: This idea of a Coce route responding with available providers sound
good of course. But the the difficulty would be Koha side in the syspref
editor to display those info appropriately. The way Tomas has displayed the 3
currently available providers is really good and consistent, and easy to
extend if required. 

Concerning BDS integration, I can't say a lot since I don't know what it is at
all. Adding a new provider shouldn't be too difficult. You have to add a
method to the CoceFetcher class. If you have a public API for BDS, I could
take a look.

What I'd like to add to Coce is its integration with an image storage server
which could replace the one integrated in Koha: local cover images. This would
allow libraries to share scanning work, and stop this madness which consist to
store images directly in the DB. I have several libraries that have MySQL DB
made up of 95% of image data.

A new image storage server may just be a new OpenLibrary project. So it may be
a bad idea. But I had the feeling OpenLibrary was stopping its activity.

Indeed, I had a hunt around the sysprefs code in koha last night after making my suggestion and decided it would be a nightmare to implement at the koha side :(

I might add a route to coce (and a pull request) anyway to get the available providers as it would perhaps be useful in the other app we're developing that uses it.

I like the idea of a shared 'local image store'.. it's a shame that the OpenLibrary project is slowing as you suspect though :(.

Certainly.. I'd pass qa on your followups Tomas, so I'd suggest pushing as is.

Feature pushed to master.

Thanks Frederic!

Frédéric made me aware that this one could be an alternative to http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7187  

One thing that's been a problem with cover images is the different caching policies from the providers which is of course unfortunate. Does this patch by any chance implement setting caching on/off per source to handle this? 

(Sorry if I'm being lazy, but searching for cach* in this thread gives a bit many hits)

Viktor, Coce server is a third-party tool accessed by Koha. Coce-side, for each cover image provider, you can define a cache timeout. Another point to take into account: URL to the images are cached, not the images themselves. This can make a difference from a provider policies point of view.

(In reply to Frédéric Demians from comment #77)
> Viktor, Coce server is a third-party tool accessed by Koha. Coce-side, for
> each cover image provider, you can define a cache timeout. Another point to
> take into account: URL to the images are cached, not the images themselves.
> This can make a difference from a provider policies point of view.

Interesting! Thanks for the quick feedback.