Evergreen integration with obalkyknih.cz (Czech added content provider)

[Linda Jansova]

Current modules for pulling added content from external sources to Evergreen do not enable getting added content from obalkyknih.cz.

A new module which would provide this functionality (from Evergreen 2.10.5 on) is proposed.

[Linda Jansova (skolkova-s)]

This bug should be tagged "wishlist" but I haven't figured out where to select this importance status.

[Jason Stephenson (jstephenson)]

Same way you do the Status change, click the exclamation point in the yellow circle, and then choose Wishlist. I did it already.

I also targeted this for 2.next as it is too late to get into the 2.11 release at this point.

[Kathy Lussier (klussier)]

You need to be an Evergreen Bug Wrangler to change the Importance of a LP bug. I think you need to be an Evergreen Driver to be able to target bugs towards a milestone.

[Linda Jansova (skolkova-s)]

Thank you, Jason!

(Actually, I cannot see the yellow circle next to importance status at all. Maybe I don't contribute to launchpad often enough to be granted this kind of permission :-).)

[Linda Jansova (skolkova-s)]

The module and changes to templates and opensrf.xml have already been committed by Jakub Kotrla:
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=search;h=refs/heads/user/jkotrla/lp1624366-added_content_obalkyknih;s=Jakub+Kotrla;st=author.

Documentation describing how the module should be enabled and configured has been submitted via the documentation mailing list: http://list.georgialibraries.org/pipermail/open-ils-documentation/2017-January/002127.html.

Given this status of the bug, maybe its status should be changed from "wishlist" to "fix committed"? (I'm not sure this is the right moment to make this change, though.)

I don't seem to have a Launchpad permission which would enable me to change "Evergreen 2.next" to "2.12beta" milestone, therefore I couldn't follow Kathy's recommendations from the DEV mailing list in full. Nevertheless I hope this would not prevent our changes from being included in 2.12 :-).

[Jason Stephenson (jstephenson)]

Linda,

Thanks for the updates.

A bug goes to fix committed when the code has been committed to the master branch, and not the working repository. The working repository is for work in progress and code waiting to be tested.

Usually, when code is ready to be tested, the coder should add the pull request tag and remove themselves from being assigned to the bug. These are the signals that code is ready to be tested.

Also, documentation can be submitted in the same git branch as the code. It doesn't have to be done via the documentation mailing list.

So, my recommendation is that if the code is ready for testing, Jakub should remove himself as assignee on the bug and change the status from "In Progress" to "Confirmed."

The bug will be targeted appropriately by whomever reviews the code.

Cheers,
Jason

[Kathy Lussier (klussier)]

Thanks Linda! I've updated the target and made some of the other adjustments Jason recommended.

[Linda Jansova (skolkova-s)]

Jason and Kathy, thank you both for recommending and taking appropriate action!

(Jane Sandberg has already promised to add our piece to the official documentation once our code is accepted.)

[Kathy Lussier (klussier)]

Adding a direct link to the branch:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jkotrla/lp1624366-added_content_obalkyknih

[Jason Stephenson (jstephenson)]

Thanks for sharing this code. It looks like a good first draft. However, it does not work like the existing added content providers, so I can not recommend including it in Evergreen as it is. With a few changes, I think it would be acceptable.

Added content in Evergreen is handled by Perl modules found in OpenILS::WWW::AddedContent. I recommend that a new module be created: OpenILS::WWW::AddedContent::ObalkyKnih. This module should handle the download of images and process error results. It should provide an interface like the existing modules so that template changes are unnecessary or at least minimized.

I also recommend dropping the new Open-ILS/src/templates/opac/parts/record/jackets.tt2 template and removing the changes to the Open-ILS/src/templates/opac/parts/config.tt2 and Open-ILS/src/templates/opac/parts/record/summary.tt2 templates.

The new module should be enabled in opensrf/default/added_content/module in opensrf.xml. I see that you have added configuration for this provider in opensrf.xml already. That is a good step.

Thank you again for sharing this. I look forward to seeing your next revision.

[Linda Jansova (skolkova-s)]

Hi Jason,

Thanks for the review of our code!

However, I am a bit puzzled by the first part of your comment - our module is a standard Perl module like the other ones - please see http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commit;h=961191742329692f6f01f2fa4a794b2fedbf727a. Has this piece of code made it to you through Git? (Not being a developer myself, I find it a bit difficult to move around in the Git repositories and therefore cannot tell if it has made it all the way to where it is supposed to be for code review.)

As to the templates - in case jackets.tt2 does not seem like a good idea, we can ask our developer to move the code to the existing templates. (When working with the templates, we thought it might be a bit more tidy to have the jackets stuff in a separate file, though.)

[Jason Stephenson (jstephenson)]

Ah. The problem is the merge commit. It moves that commit so far back in the history that it does not show up with the others when one does a git log.

So, my recommendation on that front is to break the commits out into a new branch where all of the commits for this feature are at the top. We don't care about date ordering in Evergreen and we never use git merge. We prefer to see commits in commit order, even if that makes the history look out of order by date.

As for the templates, I strongly recommend that your code be modified so that no changes are required to the templates.

[Linda Jansova (skolkova-s)]

Jason, thank you for the in-depth explanation!

However, I am afraid we cannot get in touch with our developer quickly enough to make sure our code makes it to 2.12.
Do you think we could ask you to insert our ObalkyKnih.pm module
(available at http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commit;h=961191742329692f6f01f2fa4a794b2fedbf727a)
and the modified summary.tt2 module (attached to this bug) to the right Git places on our behalf? We would appreciate very much
if you could do us this favour!

Also, when the modified summary.tt2 is used, jackets.tt2 template is no longer necessary and can be safely dropped.

(The use of the template is necessary to create a link to a particular obalkyknih.cz page. This link is required by the added content provider
and so we cannot do without.)

[Linda Jansova (skolkova-s)]

And this is the summary.tt2 file :-).

[Jakub Kotrla (0-jakub)]

Jason,
I've created a new branch with new summary.tt2 as provided by Linda, so that jackets.tt2 are not required.

The branch is lp1624366-added_content_obalkyknih_2 -
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jkotrla/lp1624366-added_content_obalkyknih_2

I've followed Quick start for Evergreen contributors point by point to make sure everything is as it should be.

[Kathy Lussier (klussier)]

Thank you Linda and Jakub!

I haven't thoroughly reviewed the code, but a couple of things jumped out at me while I just looked at it:

In config.tt2, I see the added content provider is enabled by default. I think it's better if obalkyknih_cz.enabled is set to False. Sites that subscribe to the service can then turn it on when it's needed.

It looks like a couple of new features were removed from summary.tt2 when rebasing the code against current master:

- The changes from http://git.evergreen-ils.org/?p=Evergreen.git;a=commit;h=beebab309629a319de590469d3d5a2b2743b6dee were removed

- The changes from http://git.evergreen-ils.org/?p=Evergreen.git;a=commitdiff;h=ad0f60ac409b8434a5ee3e552b2efa0358ae6a00 that gives users a direct link to the list from the summary page were removed

- The badge information that came with http://git.evergreen-ils.org/?p=Evergreen.git;a=commitdiff;h=8180b60d040d6b1025de21bbd587839076037493 was removed

Thanks for all of your work on this!
Kathy

[Jakub Kotrla (0-jakub)]

Thanks Kathy for catching that.

I've disabled by default the added content provider.
And I've fixed summary.tt2, our change was added to hopefully uptodate verions taken from http://git.evergreen-ils.org/?p=Evergreen.git;a=tree;f=Open-ILS/src/templates/opac/parts/record;h=bc962e52a84aaf95ffc5f1fa407c2ffc3d54dec4;hb=HEAD.

Committed and pushed to branch.

[Jason Stephenson (jstephenson)]

Jakub,

Thanks for updating the branch.

My main concern is that you changed the templates, opac/parts/config.tt2 and opac/parts/record/summary.tt2. Neither of the other two added content modules required such changes, so I wonder why the ObalkyKnih module requires template changes. Can you please explain why you needed to change the templates?

[Eva Cerninakova (ece)]

The "non-programmer" reason for changing the config.tt2 and summary.tt2 is link from ILS to obalkyknih.cz record. The service obalkyknih.cz is free but the link to record is necessary legal condition for use of the service. Plus it is benefit for user as well, because the record on the server obalkyknih.cz could contain additional informations like covers and TOC of serial issues or multi-volume work, photos and information about the authors, bibliographical citation, links to other relevant resources etc.
See e.g.,
http://knihovna.jabok.cuni.cz/eg/opac/record/18189?locg=1
-->
http://obalkyknih.cz/view?isbn=2336-2332

[Jakub Kotrla (0-jakub)]

As Eva mentioned above, there is a legal license obligation: when system displays content from obalkyknih.cz, there must be link back to the source record on server obalkyknih.cz.

I was not able to find any other way to comply with this other than add it into summary.tt2. Because Perl API for AddedContent for adding covers can only provide image and not any surrounding HTML.

Change in config.tt2 is caused by change in summary.tt2 - so that one can enable/disable the link. I am not aware of any other way. I'll be happy if you will tell us how to do this better.

[Jason Stephenson (jstephenson)]

OK. That explanation works for me.

My concern is not specifically with this requirement for a single added content module, but I can imagine the mess when there are 20 that modify the templates.

Since there is a legal requirement for the link, I suppose it is OK for now. In the long run, we should see about coming up with a way to pass the url back from the added content manual. I'm not saying that this has to be done, now.

[Jason Stephenson (jstephenson)]

manual = module. I shouldn't comment before breakfast.

[Jason Stephenson (jstephenson)]

I'm going to see about testing this code later today. (I have a couple of other things that I must do today, so I make no promises.)

It would be helpful if someone could add 5 ISBNs (or better, a file of 5 MARC records) that are known to exist in the Obalky database, i.e. they will return covers, etc., from ObalkyKnih.

Thanks!

[Linda Jansova (skolkova-s)]

Thank you, Jason!

If you wish to test it on your machine, please send both your IP address and your Evergreen URL to Eva (<email address hidden>)? She will temporarily add it to Jabok Library Obalkyknih.cz account so that you can perform the tests. (Obalkyknih.cz is a sign-up service as mentioned in the submitted piece of documentation: http://list.georgialibraries.org/pipermail/open-ils-documentation/2017-January/002127.html.)

Eva is also about to send you the sample ISBNs/records (or provide you with access details about Jabok Library Z39.50 server so that you can download the records directly from Jabok).

[Jason Stephenson (jstephenson)]

Jakub, Eva, and Linda, thank you for your contributions to Evergreen!

I have pushed the code to the master branch, and it will be included in the upcoming 2.12-beta and all subsequent releases.

Jason

[Linda Jansova (skolkova-s)]

And we thank you, Jason, for testing our code and pushing it to the master branch :-)!