Bug 16034 - Integration with OverDrive Patron API
Summary: Integration with OverDrive Patron API
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Srdjan Jankovic
QA Contact: Testopia
URL:
Whiteboard:
Keywords: dependency
Depends on:
Blocks: 17602 18172 18514 18160 18243
  Show dependency treegraph
 
Reported: 2016-03-10 01:25 UTC by Srdjan Jankovic
Modified: 2017-04-30 23:59 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron (15.38 KB, patch)
2016-03-10 04:30 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Adding a new syspref - OverDriveCirculation (2.99 KB, patch)
2016-03-10 04:31 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Add overdrive info to the users page in the public interface (25.02 KB, patch)
2016-03-10 04:31 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron (15.44 KB, patch)
2016-04-05 21:28 UTC, Jesse Weaver
Details | Diff | Splinter Review
bug_16034 Adding a new syspref - OverDriveCirculation (3.05 KB, patch)
2016-04-05 21:28 UTC, Jesse Weaver
Details | Diff | Splinter Review
bug_16034 Add overdrive info to the users page in the public interface (25.08 KB, patch)
2016-04-05 21:28 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 16034 follow-up: added WebService::ILS to PerlDependencies (776 bytes, patch)
2016-07-29 00:45 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16034 follow-up: added WebService::ILS to PerlDependencies (771 bytes, patch)
2016-09-16 05:52 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron (15.48 KB, patch)
2016-09-19 02:51 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Adding a new syspref - OverDriveCirculation (3.03 KB, patch)
2016-09-19 02:51 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Add overdrive info to the users page in the public interface (25.32 KB, patch)
2016-09-19 02:52 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16034 follow-up: added WebService::ILS to PerlDependencies (771 bytes, patch)
2016-09-19 02:54 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron (15.42 KB, patch)
2016-09-23 06:03 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Adding a new syspref - OverDriveCirculation (3.03 KB, patch)
2016-09-23 06:03 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Add overdrive info to the users page in the public interface (25.32 KB, patch)
2016-09-23 06:03 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16034 follow-up: added WebService::ILS to PerlDependencies (771 bytes, patch)
2016-09-23 06:04 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Add overdrive info to the users page in the public interface (25.32 KB, patch)
2016-09-26 06:36 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16034 follow-up: added WebService::ILS to PerlDependencies (771 bytes, patch)
2016-09-26 06:37 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron (15.42 KB, patch)
2016-11-09 20:55 UTC, Chris Cormack
Details | Diff | Splinter Review
bug_16034 Adding a new syspref - OverDriveCirculation (3.03 KB, patch)
2016-11-09 20:55 UTC, Chris Cormack
Details | Diff | Splinter Review
bug_16034 Add overdrive info to the users page in the public interface (25.37 KB, patch)
2016-11-09 20:55 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 16034 follow-up: added WebService::ILS to PerlDependencies (772 bytes, patch)
2016-11-09 20:56 UTC, Chris Cormack
Details | Diff | Splinter Review
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron (15.43 KB, patch)
2016-12-30 00:14 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Adding a new syspref - OverDriveCirculation (3.03 KB, patch)
2016-12-30 00:14 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Add overdrive info to the users page in the public interface (25.36 KB, patch)
2016-12-30 00:15 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16034 follow-up: added WebService::ILS to PerlDependencies (770 bytes, patch)
2016-12-30 00:15 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 follow-up: Protect from unreliable OverDrive return API (1.02 KB, patch)
2016-12-30 00:16 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Add overdrive info to the users page in the public interface (25.81 KB, patch)
2017-01-11 00:45 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16034 follow-up: added WebService::ILS to PerlDependencies (770 bytes, patch)
2017-01-11 00:46 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron (15.71 KB, patch)
2017-01-17 05:32 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Adding a new syspref - OverDriveCirculation (3.03 KB, patch)
2017-01-17 05:32 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Add overdrive info to the users page in the public interface (26.54 KB, patch)
2017-01-17 05:32 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16034 follow-up: added WebService::ILS to PerlDependencies (770 bytes, patch)
2017-01-17 05:34 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron (16.37 KB, patch)
2017-01-23 01:41 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Adding a new syspref - OverDriveCirculation (3.03 KB, patch)
2017-01-23 01:41 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Add overdrive info to the users page in the public interface (26.54 KB, patch)
2017-01-23 01:42 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16034 follow-up: added WebService::ILS to PerlDependencies (770 bytes, patch)
2017-01-23 01:42 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Add overdrive info to the users page in the public interface (26.54 KB, patch)
2017-02-01 03:31 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16034 follow-up: added WebService::ILS to PerlDependencies (770 bytes, patch)
2017-02-01 03:31 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron (16.42 KB, patch)
2017-02-01 16:36 UTC, Nick Clemens
Details | Diff | Splinter Review
bug_16034 Adding a new syspref - OverDriveCirculation (3.08 KB, patch)
2017-02-01 16:37 UTC, Nick Clemens
Details | Diff | Splinter Review
bug_16034 Add overdrive info to the users page in the public interface (26.59 KB, patch)
2017-02-01 16:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16034 follow-up: added WebService::ILS to PerlDependencies (826 bytes, patch)
2017-02-01 16:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16034 follow-up: generated Borrower resultset (4.20 KB, patch)
2017-02-23 23:37 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16034: (followup) Fix DB update and schema files (4.94 KB, patch)
2017-02-24 12:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16034 follow-up: better handling of absence of WebService::ILS::OverDrive::Patron at testing (1.09 KB, patch)
2017-03-02 00:33 UTC, Srdjan Jankovic
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Srdjan Jankovic 2016-03-10 01:25:24 UTC
Enable patrons to access their OverDrive account from Koha.
Comment 1 Srdjan Jankovic 2016-03-10 01:27:52 UTC
Requires WebService::ILS library from CPAN
Comment 2 Srdjan Jankovic 2016-03-10 04:30:51 UTC Comment hidden (obsolete)
Comment 3 Srdjan Jankovic 2016-03-10 04:31:02 UTC Comment hidden (obsolete)
Comment 4 Srdjan Jankovic 2016-03-10 04:31:25 UTC Comment hidden (obsolete)
Comment 5 Srdjan Jankovic 2016-03-10 04:44:47 UTC
I have not included a test plan for this. It cannot be tested without
* an OverDrive account
* return url of http(s)://your/opac/host/cgi-bin/koha/external/overdrive/auth.pl being registered with OverDrive

In terms of regression testing, opac-user.tt has an additional tab that is visible only when OverDriveCirculation syspref is set to yes, so it should possibly be given just a quick check.

If someone with an OverDrive account wants to have a go, please let me know and I will either email instructions or add them here.
Comment 6 Jesse Weaver 2016-04-05 21:28:05 UTC
Created attachment 49929 [details] [review]
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron

* Using the upstream module for all the heavy lifting
* opac/external/overdrive/auth.pl - 3-legged authentication handler

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 7 Jesse Weaver 2016-04-05 21:28:09 UTC
Created attachment 49930 [details] [review]
bug_16034 Adding a new syspref - OverDriveCirculation

If set to true, Opac will integrate OverDrive patron services

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 8 Jesse Weaver 2016-04-05 21:28:13 UTC
Created attachment 49931 [details] [review]
bug_16034 Add overdrive info to the users page in the public interface

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 9 Marcel de Rooy 2016-07-27 10:52:32 UTC
Some preliminary questions:
Did Jesse test with an OverDrive account? Noting such info in the signoff line could be helpful.
You seem to be adding an optional dependency. I am missing PerlDependencies?
Comment 10 Srdjan Jankovic 2016-07-28 02:08:16 UTC
Yes, you must have an OverDrive account to use this, and even set up the return url with them. It's a process.

I apologise, I did not update PerlDependencies, I thought it was only for mandatory deps. Do you want me to add it to the first patch, or write a follow-up?
Comment 11 Marcel de Rooy 2016-07-28 06:20:31 UTC
(In reply to Srdjan Jankovic from comment #10)
> Yes, you must have an OverDrive account to use this, and even set up the
> return url with them. It's a process.
I assumed so. But my question was: Did the signoffer (Jesse) actually test with an Overdrive account?

> I apologise, I did not update PerlDependencies, I thought it was only for
> mandatory deps. Do you want me to add it to the first patch, or write a
> follow-up?
Since you already have a SO, you'd better write a follow-up. No need to change status for that.
Comment 12 Srdjan Jankovic 2016-07-29 00:45:11 UTC
Created attachment 53793 [details] [review]
Bug 16034 follow-up: added WebService::ILS to PerlDependencies
Comment 13 Srdjan Jankovic 2016-07-29 00:49:13 UTC
I will confirm on Jesse's behalf, I believe they even implemented it.
Comment 14 Srdjan Jankovic 2016-09-16 05:52:14 UTC
Created attachment 55594 [details] [review]
Bug 16034 follow-up: added WebService::ILS to PerlDependencies
Comment 15 Katrin Fischer 2016-09-18 21:35:00 UTC
Patch doesn't apply, please rebase.
Would it be possible to get a test account in order to test this?
Comment 16 Srdjan Jankovic 2016-09-19 02:51:31 UTC
Created attachment 55675 [details] [review]
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron

* Using the upstream module for all the heavy lifting
* opac/external/overdrive/auth.pl - 3-legged authentication handler

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 17 Srdjan Jankovic 2016-09-19 02:51:49 UTC
Created attachment 55676 [details] [review]
bug_16034 Adding a new syspref - OverDriveCirculation

If set to true, Opac will integrate OverDrive patron services

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 18 Srdjan Jankovic 2016-09-19 02:52:25 UTC
Created attachment 55677 [details] [review]
bug_16034 Add overdrive info to the users page in the public interface

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 19 Srdjan Jankovic 2016-09-19 02:54:48 UTC
Created attachment 55678 [details] [review]
Bug 16034 follow-up: added WebService::ILS to PerlDependencies
Comment 20 Katrin Fischer 2016-09-19 08:30:33 UTC
If the rebase was not too complicated, putting back to "Signed off" is ok.
Comment 21 Srdjan Jankovic 2016-09-19 23:27:41 UTC
The rebase was easy, but the fourth patch (follow-up) was not signed-off earlier.
Comment 22 Srdjan Jankovic 2016-09-23 06:03:29 UTC
Created attachment 55763 [details] [review]
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron

* Using the upstream module for all the heavy lifting
* opac/external/overdrive/auth.pl - 3-legged authentication handler

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 23 Srdjan Jankovic 2016-09-23 06:03:41 UTC
Created attachment 55764 [details] [review]
bug_16034 Adding a new syspref - OverDriveCirculation

If set to true, Opac will integrate OverDrive patron services

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 24 Srdjan Jankovic 2016-09-23 06:03:53 UTC
Created attachment 55765 [details] [review]
bug_16034 Add overdrive info to the users page in the public interface

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 25 Srdjan Jankovic 2016-09-23 06:04:08 UTC
Created attachment 55766 [details] [review]
Bug 16034 follow-up: added WebService::ILS to PerlDependencies
Comment 26 Srdjan Jankovic 2016-09-26 06:36:08 UTC
Created attachment 55815 [details] [review]
bug_16034 Add overdrive info to the users page in the public interface

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 27 Srdjan Jankovic 2016-09-26 06:37:50 UTC
Created attachment 55816 [details] [review]
Bug 16034 follow-up: added WebService::ILS to PerlDependencies
Comment 28 Chris Cormack 2016-11-09 20:55:21 UTC
Created attachment 57398 [details] [review]
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron

* Using the upstream module for all the heavy lifting
* opac/external/overdrive/auth.pl - 3-legged authentication handler

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 29 Chris Cormack 2016-11-09 20:55:39 UTC
Created attachment 57399 [details] [review]
bug_16034 Adding a new syspref - OverDriveCirculation

If set to true, Opac will integrate OverDrive patron services

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 30 Chris Cormack 2016-11-09 20:55:57 UTC
Created attachment 57400 [details] [review]
bug_16034 Add overdrive info to the users page in the public interface

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 31 Chris Cormack 2016-11-09 20:56:12 UTC
Created attachment 57401 [details] [review]
Bug 16034 follow-up: added WebService::ILS to PerlDependencies
Comment 32 Nick Clemens 2016-12-05 18:25:07 UTC
couple things:
POD fails QA tools

opac/external/overdrive/auth.pl has wrong description (copy/paste error)

When attempting to cancel a hold from results or patron details error is displayed at top of page:
The title is not checked out. at /usr/local/share/perl/5.14.2/WebService/ILS/OverDrive/Patron.pm line 571.

This is awesome overall though! Nice addition of ability to checkout/download/hold titles.
Comment 33 Srdjan Jankovic 2016-12-30 00:14:44 UTC
Created attachment 58518 [details] [review]
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron

* Using the upstream module for all the heavy lifting
* opac/external/overdrive/auth.pl - 3-legged authentication handler

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 34 Srdjan Jankovic 2016-12-30 00:14:56 UTC
Created attachment 58519 [details] [review]
bug_16034 Adding a new syspref - OverDriveCirculation

If set to true, Opac will integrate OverDrive patron services

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 35 Srdjan Jankovic 2016-12-30 00:15:07 UTC
Created attachment 58520 [details] [review]
bug_16034 Add overdrive info to the users page in the public interface

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 36 Srdjan Jankovic 2016-12-30 00:15:38 UTC
Created attachment 58521 [details] [review]
Bug 16034 follow-up: added WebService::ILS to PerlDependencies
Comment 37 Srdjan Jankovic 2016-12-30 00:16:56 UTC
Created attachment 58522 [details] [review]
bug_16034 follow-up: Protect from unreliable OverDrive return API
Comment 38 Srdjan Jankovic 2016-12-30 00:39:35 UTC
I've fixed wrong descriptions in handlers.

Unfortunately, I could not find a title that I could place a hold on. I've made a "blind"-ish change for returns, works for checked out items, so I'd imagine it would work with hold returns (cancellations). Can you please try it with this patch. Also, can you please let me know which title could be placed on hold so I can play with it.
Comment 39 Nick Clemens 2017-01-10 09:47:19 UTC
I no longer get an error when cancelling a hold, but the hold is not cancelled either. 'Safe in his arms' is a title in the bywater library which can be placed on hold

(In reply to Srdjan Jankovic from comment #38)
> I've fixed wrong descriptions in handlers.
> 
> Unfortunately, I could not find a title that I could place a hold on. I've
> made a "blind"-ish change for returns, works for checked out items, so I'd
> imagine it would work with hold returns (cancellations). Can you please try
> it with this patch. Also, can you please let me know which title could be
> placed on hold so I can play with it.
Comment 40 Srdjan Jankovic 2017-01-11 00:45:04 UTC
Created attachment 58790 [details] [review]
bug_16034 Add overdrive info to the users page in the public interface

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 41 Srdjan Jankovic 2017-01-11 00:46:06 UTC
Created attachment 58791 [details] [review]
Bug 16034 follow-up: added WebService::ILS to PerlDependencies
Comment 42 Srdjan Jankovic 2017-01-11 00:49:06 UTC
Sorry Nick, was a bug. Fixed it.
Comment 43 Nick Clemens 2017-01-11 14:51:11 UTC
Awesome, checking out and in and placing/cancelling a hold all work well now.

When I click 'Logout from OverDrive account' the response seems to always be "Internal server error" can you check on that?

POD still fails QA

Additionally - two things would be nice:
1 - When you click to Sign In to OverDrive - this link should open in a new tab/window
2 - It would be nice to have a link to your overdrive account from the account page so that you can view the overdrive site if you wish to.


(In reply to Srdjan Jankovic from comment #42)
> Sorry Nick, was a bug. Fixed it.
Comment 44 Srdjan Jankovic 2017-01-17 05:32:26 UTC
Created attachment 59055 [details] [review]
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron

* Using the upstream module for all the heavy lifting
* opac/external/overdrive/auth.pl - 3-legged authentication handler

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 45 Srdjan Jankovic 2017-01-17 05:32:39 UTC
Created attachment 59056 [details] [review]
bug_16034 Adding a new syspref - OverDriveCirculation

If set to true, Opac will integrate OverDrive patron services

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 46 Srdjan Jankovic 2017-01-17 05:32:49 UTC
Created attachment 59057 [details] [review]
bug_16034 Add overdrive info to the users page in the public interface

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 47 Srdjan Jankovic 2017-01-17 05:34:01 UTC
Created attachment 59058 [details] [review]
Bug 16034 follow-up: added WebService::ILS to PerlDependencies
Comment 48 Srdjan Jankovic 2017-01-23 01:41:29 UTC
Created attachment 59392 [details] [review]
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron

* Using the upstream module for all the heavy lifting
* opac/external/overdrive/auth.pl - 3-legged authentication handler

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 49 Srdjan Jankovic 2017-01-23 01:41:45 UTC
Created attachment 59393 [details] [review]
bug_16034 Adding a new syspref - OverDriveCirculation

If set to true, Opac will integrate OverDrive patron services

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 50 Srdjan Jankovic 2017-01-23 01:42:03 UTC
Created attachment 59394 [details] [review]
bug_16034 Add overdrive info to the users page in the public interface

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 51 Srdjan Jankovic 2017-01-23 01:42:16 UTC
Created attachment 59395 [details] [review]
Bug 16034 follow-up: added WebService::ILS to PerlDependencies
Comment 52 Nick Clemens 2017-02-01 01:47:01 UTC
This is looking very good - the one issue I see remaining is the content in the OverDrive tab has a lot of html tags showing <i> <b> etc - can that be processed correctly?

-Nick
Comment 53 Srdjan Jankovic 2017-02-01 03:31:28 UTC
Created attachment 59713 [details] [review]
bug_16034 Add overdrive info to the users page in the public interface

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 54 Srdjan Jankovic 2017-02-01 03:31:55 UTC
Created attachment 59714 [details] [review]
Bug 16034 follow-up: added WebService::ILS to PerlDependencies
Comment 55 Nick Clemens 2017-02-01 16:36:56 UTC
Created attachment 59743 [details] [review]
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron

* Using the upstream module for all the heavy lifting
* opac/external/overdrive/auth.pl - 3-legged authentication handler

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 56 Nick Clemens 2017-02-01 16:37:02 UTC
Created attachment 59744 [details] [review]
bug_16034 Adding a new syspref - OverDriveCirculation

If set to true, Opac will integrate OverDrive patron services

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 57 Nick Clemens 2017-02-01 16:37:08 UTC
Created attachment 59745 [details] [review]
bug_16034 Add overdrive info to the users page in the public interface

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 58 Nick Clemens 2017-02-01 16:37:14 UTC
Created attachment 59746 [details] [review]
Bug 16034 follow-up: added WebService::ILS to PerlDependencies

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 59 Kyle M Hall 2017-02-21 20:01:47 UTC
Pushed to master for 17.05, thanks Srdjan, Chris!
Comment 60 Katrin Fischer 2017-02-21 21:07:51 UTC
This won't get ported back to 16.11.x as it is an enhancement.
Comment 61 Jonathan Druart 2017-02-23 17:54:07 UTC
Comment on attachment 59743 [details] [review]
bug_16034 Koha::ExternalContent::OverDrive - a wrapper around WebService::ILS::Overdrive::Patron

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

::: Koha/Schema/Result/Borrower.pm
@@ +616,5 @@
>      datetime_undef_if_invalid => 1,
>      is_nullable => 1,
>    },
> +  "overdrive_auth_token",
> +  { data_type => "text", is_nullable => 1 },

This is terrible wrong, you have to commit the file generated by misc/devel/update_dbix_class_files.pl, with the sum updated!

Now we are not able to regenerate the schema.

DBIx::Class::Schema::Loader::make_schema_at(): Checksum mismatch in './/Koha/Schema/Result/Borrower.pm', the auto-generated part of the file has been modified outside of this loader.  Aborting.

Please fix ASAP
Comment 62 Srdjan Jankovic 2017-02-23 23:37:13 UTC
Created attachment 60626 [details] [review]
Bug 16034 follow-up: generated Borrower resultset
Comment 63 Srdjan Jankovic 2017-02-23 23:39:46 UTC
Apologies, didn't know that.
Now the patch contains much more than my field, hope it's fine.
Comment 64 Marcel de Rooy 2017-02-24 09:39:50 UTC
Guys,
another QA lesson :)
if you add to borrowers, dont forget deletedborrowers

DBIx::Class::Row::store_column(): No such column 'overdrive_auth_token' on Koha::Schema::Result::Deletedborrower at /usr/share/koha/masterclone/Koha/Patron.pm line 416
Comment 65 Katrin Fischer 2017-02-24 09:52:08 UTC
Ee should have a test for that... it's gone wrong so many times now.
Comment 66 Jonathan Druart 2017-02-24 10:34:23 UTC
(In reply to Katrin Fischer from comment #65)
> Ee should have a test for that... it's gone wrong so many times now.

We have it already:

% prove t/db_dependent/db_structure.t
t/db_dependent/db_structure.t .. 1/? 
#   Failed test 'tables related to Borrower and Deletedborrower should not differ on overdrive_auth_token.is_nullable'
#   at t/db_dependent/db_structure.t line 53.
#          got: '1'
#     expected: undef

#   Failed test 'tables related to Borrower and Deletedborrower should not differ on overdrive_auth_token.data_type'
#   at t/db_dependent/db_structure.t line 53.
#          got: 'text'
#     expected: undef
# Looks like you failed 2 tests of 423.
t/db_dependent/db_structure.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/423 subtests 

Test Summary Report
-------------------
t/db_dependent/db_structure.t (Wstat: 512 Tests: 423 Failed: 2)
  Failed tests:  164-165
  Non-zero exit status: 2
Files=1, Tests=423,  2 wallclock secs ( 0.06 usr  0.01 sys +  0.84 cusr  0.40 csys =  1.31 CPU)
Result: FAIL
Comment 67 Jonathan Druart 2017-02-24 12:11:01 UTC
Comment on attachment 60626 [details] [review]
Bug 16034 follow-up: generated Borrower resultset

Too many wrong things in this patch, @RM do no push it and regenerate it yourself.
Comment 68 Tomás Cohen Arazi 2017-02-24 12:23:31 UTC
Created attachment 60654 [details] [review]
Bug 16034: (followup) Fix DB update and schema files

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 69 Srdjan Jankovic 2017-02-27 00:21:30 UTC
(In reply to Marcel de Rooy from comment #64)
> Guys,
> another QA lesson :)
> if you add to borrowers, dont forget deletedborrowers
> 
> DBIx::Class::Row::store_column(): No such column 'overdrive_auth_token' on
> Koha::Schema::Result::Deletedborrower at
> /usr/share/koha/masterclone/Koha/Patron.pm line 416

In which case maybe a check in misc/devel/update_dbix_class_files.pl would be in order?
Comment 70 Marcel de Rooy 2017-02-27 07:40:46 UTC
(In reply to Srdjan Jankovic from comment #69)
> (In reply to Marcel de Rooy from comment #64)
> > Guys,
> > another QA lesson :)
> > if you add to borrowers, dont forget deletedborrowers
> > 
> > DBIx::Class::Row::store_column(): No such column 'overdrive_auth_token' on
> > Koha::Schema::Result::Deletedborrower at
> > /usr/share/koha/masterclone/Koha/Patron.pm line 416
> 
> In which case maybe a check in misc/devel/update_dbix_class_files.pl would
> be in order?

There is a check in qa-tools
Comment 71 Jonathan Druart 2017-02-27 08:20:20 UTC
(In reply to Marcel de Rooy from comment #70)
> (In reply to Srdjan Jankovic from comment #69)
> > (In reply to Marcel de Rooy from comment #64)
> > > Guys,
> > > another QA lesson :)
> > > if you add to borrowers, dont forget deletedborrowers
> > > 
> > > DBIx::Class::Row::store_column(): No such column 'overdrive_auth_token' on
> > > Koha::Schema::Result::Deletedborrower at
> > > /usr/share/koha/masterclone/Koha/Patron.pm line 416
> > 
> > In which case maybe a check in misc/devel/update_dbix_class_files.pl would
> > be in order?
> 
> There is a check in qa-tools

Nope, but we have a test, see comment 66.
Comment 72 Mirko Tietgen 2017-02-28 10:48:10 UTC
This breaks master packages.

There is no Debian package available. There is an ITP bug from Chris at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=838609

I think this is optional, so maybe the test should be skippable (if that is a word)?

#   Failed test 'use Koha::ExternalContent::OverDrive;'
#   at t/00-load.t line 65.
#     Tried to use 'Koha::ExternalContent::OverDrive'.
#     Error:  Can't locate WebService/ILS/OverDrive/Patron.pm in @INC (you may need to install the WebService::ILS::OverDrive::Patron module) (@INC contains: /tmp/buildd/koha-16.12.01~git+20170226133406.b5b4b708/blib/PERL_MODULE_DIR /tmp/buildd/koha-16.12.01~git+20170226133406.b5b4b708/blib/arch /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.20.2 /usr/local/share/perl/5.20.2 /usr/lib/x86_64-linux-gnu/perl5/5.20 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.20 /usr/share/perl/5.20 /usr/local/lib/site_perl . /var/lib/koha/plugins /var/lib/koha/plugins) at /tmp/buildd/koha-16.12.01~git+20170226133406.b5b4b708/blib/PERL_MODULE_DIR/Koha/ExternalContent/OverDrive.pm line 24.
# BEGIN failed--compilation aborted at /tmp/buildd/koha-16.12.01~git+20170226133406.b5b4b708/blib/PERL_MODULE_DIR/Koha/ExternalContent/OverDrive.pm line 24.
# Compilation failed in require at t/00-load.t line 65.
# BEGIN failed--compilation aborted at t/00-load.t line 65.
Bailout called.  Further testing stopped:  ***** PROBLEMS LOADING FILE 'Koha::ExternalContent::OverDrive'
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 255 just after 245.
FAILED--Further testing stopped: ***** PROBLEMS LOADING FILE 'Koha::ExternalContent::OverDrive'
Makefile:22097: recipe for target 'test_dynamic' failed
make[1]: *** [test_dynamic] Error 255
make[1]: Leaving directory '/tmp/buildd/koha-16.12.01~git+20170226133406.b5b4b708'
dh_auto_test: make -j1 test returned exit code 2
debian/rules:9: recipe for target 'build' failed
make: *** [build] Error 2
dpkg-buildpackage: error: debian/rules build gave error exit status 2
Comment 73 Srdjan Jankovic 2017-03-01 00:05:17 UTC
Is there a standard way we skip that kind of tests?
Comment 74 Jonathan Druart 2017-03-01 08:12:15 UTC
(In reply to Srdjan Jankovic from comment #73)
> Is there a standard way we skip that kind of tests?

We already kind of the same thing in C4::Members for the Norvegian specific ft.
I guess thing like:

  use Module::Load::Conditional qw( can_load );
  if ( ! can_load( modules => { 'WebService::ILS::OverDrive::Patron' => undef } ) ) {
     warn "Unable to load WebService::ILS::OverDrive::Patron";
  }

in place of the use statement would do the trick.
Comment 75 Srdjan Jankovic 2017-03-02 00:33:43 UTC
Created attachment 60789 [details] [review]
Bug 16034 follow-up: better handling of absence of WebService::ILS::OverDrive::Patron at testing
Comment 76 Marcel de Rooy 2017-03-07 13:14:17 UTC
=head1 L<WebService::ILS::OverDrive::Patron> METHODS
=item C<return($id)>

sub return {

Definitely a fine name for a method ;)
Comment 77 Srdjan Jankovic 2017-03-08 00:54:25 UTC
(In reply to Marcel de Rooy from comment #76)
> =head1 L<WebService::ILS::OverDrive::Patron> METHODS
> =item C<return($id)>
> 
> sub return {
> 
> Definitely a fine name for a method ;)

Oh why cannot life be simple... I think it is safe, there are some rules and Perl knows which one's which. However, if it is making you uneasy, I can rename it to return_checkout() or something.
Comment 78 Marcel de Rooy 2017-03-08 07:06:56 UTC
(In reply to Srdjan Jankovic from comment #77)
> (In reply to Marcel de Rooy from comment #76)
> > =head1 L<WebService::ILS::OverDrive::Patron> METHODS
> > =item C<return($id)>
> > 
> > sub return {
> > 
> > Definitely a fine name for a method ;)
> 
> Oh why cannot life be simple... I think it is safe, there are some rules and
> Perl knows which one's which. However, if it is making you uneasy, I can
> rename it to return_checkout() or something.

No worries. But we should not use reserved words for methods, columns, etc. In another report we just removed a 'return' column from issues.. As you will understand, git grepping on return is laborious.
Comment 79 Srdjan Jankovic 2017-03-09 00:08:41 UTC
(In reply to Marcel de Rooy from comment #78)
>
> In another report we just removed a 'return' column from issues.. As you
> will understand, git grepping on return is laborious.

Ah right, of course, I had to do it myself when figuring out how difficult it would be to replace it. To my defence, I was grepping for "sub return" and "\->return".
Comment 80 Mirko Tietgen 2017-03-09 14:08:29 UTC
I rolled a package and installed it.

$ locate WebService/ILS/OverDrive/Patron.pm
/usr/share/perl5/WebService/ILS/OverDrive/Patron.pm

Test fails anyway.

#   Failed test 'use Koha::ExternalContent::OverDrive;'
#   at t/00-load.t line 65.
#     Tried to use 'Koha::ExternalContent::OverDrive'.
#     Error:  Can't locate WebService/ILS/OverDrive/Patron.pm in @INC (you may need to install the WebService::ILS::OverDrive::Patron module) (@INC contains: /tmp/buildd/koha-16.12+17311~git+20170309145207.123dfb1c/blib/PERL_MODULE_DIR /tmp/buildd/koha-16.12+17311~git+20170309145207.123dfb1c/blib/arch /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.20.2 /usr/local/share/perl/5.20.2 /usr/lib/x86_64-linux-gnu/perl5/5.20 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.20 /usr/share/perl/5.20 /usr/local/lib/site_perl . /var/lib/koha/plugins /var/lib/koha/plugins) at /tmp/buildd/koha-16.12+17311~git+20170309145207.123dfb1c/blib/PERL_MODULE_DIR/Koha/ExternalContent/OverDrive.pm line 24.
# BEGIN failed--compilation aborted at /tmp/buildd/koha-16.12+17311~git+20170309145207.123dfb1c/blib/PERL_MODULE_DIR/Koha/ExternalContent/OverDrive.pm line 24.
# Compilation failed in require at t/00-load.t line 65.
# BEGIN failed--compilation aborted at t/00-load.t line 65.
Bailout called.  Further testing stopped:  ***** PROBLEMS LOADING FILE 'Koha::ExternalContent::OverDrive'
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 255 just after 255.
Comment 81 Jonathan Druart 2017-03-09 14:46:00 UTC
Patch on comment 75 has not been pushed and package building fail, please open a new bug report and fix these issues ASAP.
Comment 82 Srdjan Jankovic 2017-03-10 07:20:51 UTC
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18243

Should we close this one then?
Comment 83 Srdjan Jankovic 2017-03-10 07:23:33 UTC
Comment on attachment 60789 [details] [review]
Bug 16034 follow-up: better handling of absence of WebService::ILS::OverDrive::Patron at testing

Attached to bug 18243
Comment 84 Jonathan Druart 2017-03-10 12:24:26 UTC
(In reply to Srdjan Jankovic from comment #82)
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18243
> 
> Should we close this one then?

At least "pushed to master", not sure resolved is appropriate when there are known bugs.

Did you also fix comment 80?
Comment 85 Katrin Fischer 2017-03-12 21:52:46 UTC
This won't get ported back to 16.11.x as it is an enhancement.
Comment 86 Srdjan Jankovic 2017-03-13 00:19:39 UTC
> Did you also fix comment 80?

Not sure. I was not getting it in the first place. But 18234 should address that.
Comment 87 Jonathan Druart 2017-03-13 12:03:28 UTC
(In reply to Srdjan Jankovic from comment #86)
> > Did you also fix comment 80?
> 
> Not sure. I was not getting it in the first place. But 18234 should address
> that.

It's bug 18243 I guess.

And no, it does not:

% git bz apply 18243
% prove t/00-load.t
#   Failed test 'use Koha::ExternalContent::OverDrive;'
#   at t/00-load.t line 65.
#     Tried to use 'Koha::ExternalContent::OverDrive'.
#     Error:  Can't locate WebService/ILS/OverDrive/Patron.pm in @INC (you may need to install the WebService::ILS::OverDrive::Patron module) (@INC contains: /home/vagrant/kohaclone /home/vagrant/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.20.2 /usr/local/share/perl/5.20.2 /usr/lib/x86_64-linux-gnu/perl5/5.20 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.20 /usr/share/perl/5.20 /usr/local/lib/site_perl . /var/lib/koha/kohadev/plugins /var/lib/koha/kohadev/plugins) at /home/vagrant/kohaclone/Koha/ExternalContent/OverDrive.pm line 24.
# BEGIN failed--compilation aborted at /home/vagrant/kohaclone/Koha/ExternalContent/OverDrive.pm line 24.
# Compilation failed in require at t/00-load.t line 65.
# BEGIN failed--compilation aborted at t/00-load.t line 65.
Bailout called.  Further testing stopped:  ***** PROBLEMS LOADING FILE 'Koha::ExternalContent::OverDrive'
Comment 88 Srdjan Jankovic 2017-03-13 23:34:23 UTC
(In reply to Jonathan Druart from comment #87)
> (In reply to Srdjan Jankovic from comment #86)
> > > Did you also fix comment 80?
> > 
> > Not sure. I was not getting it in the first place. But 18234 should address
> > that.
> 
> It's bug 18243 I guess.
> 
> And no, it does not:
> 

Omg, must have had it in my PERL5LIB. Submitted to 18243. Sorry.