Bug 11401

Summary: Add support for Norwegian national library card
Product: Koha Reporter: Magnus Enger <magnus>
Component: PatronsAssignee: Magnus Enger <magnus>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: chris, gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, robin, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13303
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16254
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: 14350    
Attachments: Bug 11401 - Add support for Norwegian national library card
Bug 11401 - Changes to Koha::Schema
Bug 11401 - Add support for Norwegian national library card
Bug 11401 - Changes to Koha::Schema
Bug 11401 - Add support for Norwegian national library card
Bug 11401 - Add support for Norwegian national library card
Bug 11401 - Add support for Norwegian national library card
Bug 11401 - QA followup - fix pod
Bug 11401 - QA followup
Bug 11401 - QA followup
[PASSED QA] Bug 11401 - Changes to Koha::Schema
[PASSED QA] Bug 11401 - Add support for Norwegian national library card
[PASSED QA] Bug 11401 - QA followup - fix pod
[PASSED QA] Bug 11401 - QA followup
Bug 11401 - QA followup - Make the tests pass
Bug 11401: dependency updates
Bug 11401: (followup) make tests run on absent deps

Description Magnus Enger 2013-12-16 14:46:18 UTC
In Norway, we have a national database of library patrons, called "Nasjonalt lånekort" (NL). The fact that communnication with this database is missing from Koha is one of the two main stumbling blocks that keep Norwegian libraries from adopting Koha (the other one is the lack of support for the Norwegian ILL protocol). 

This bug will add support for NL to Koha. 

Communication with NL happens through SOAP. 

Documentation on NL is available here: http://www.lanekortet.no/ but it is in Norwegian only, alas.
Comment 1 Magnus Enger 2014-08-13 11:21:28 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2014-09-09 22:40:40 UTC
Magnus, I dont think you need to include the schema files, (one of them is conflicting). The RM can generate those when the patches are pushed
Comment 3 Tomás Cohen Arazi 2014-09-10 00:14:38 UTC
Magnus, include the schemas in a separate patch, so it is easier to test. You can put a comment with the command that needs to be run to regenerate them.
On pushing I'd re-generate it.
Comment 4 Magnus Enger 2014-09-18 12:27:14 UTC Comment hidden (obsolete)
Comment 5 Magnus Enger 2014-09-18 12:28:38 UTC Comment hidden (obsolete)
Comment 6 Magnus Enger 2014-09-18 12:51:15 UTC
More information about NL and how to set up Koha to talk to it:
http://wiki.koha-community.org/wiki/Norwegian_national_patron_database
Comment 7 Chris Cormack 2014-09-23 04:32:32 UTC Comment hidden (obsolete)
Comment 8 Chris Cormack 2014-09-23 04:32:58 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2014-10-11 14:42:35 UTC
Hi Magnuse,

- I'd like to see the change that Chris pointed out - make the loading of the module mandatory. One important reason being that the new dependencies are not marked as mandatory, but without them the test suite won't work - a lot of the tests fail because of the missing dependencies on my system. 

- Shibboleth is also going to make use of Test::DBIX::Class, this one could
probably be marked mandatory (see the Shibboleth patch)

Otherwise this so far looks good!
Comment 10 Katrin Fischer 2014-10-11 15:23:05 UTC
Hi Magnus, 

I sat with Tomas and we noticed that you might have a typoe in the version number here:

+    'Digest::SHA' => {
+        'usage'    => 'Norwegian national library card',
+        'required' => '0',
+        'min_ver'  => '5.84_01',
+    },

Also, we might have to get Convert:BaseN packaged (also Test::DBIX::Class for this and Shibboleth)
Comment 11 Robin Sheat 2014-10-11 22:43:08 UTC
(In reply to Katrin Fischer from comment #10)
> Hi Magnus, 
> 
> I sat with Tomas and we noticed that you might have a typoe in the version
> number here:
> 
> +    'Digest::SHA' => {
> +        'usage'    => 'Norwegian national library card',
> +        'required' => '0',
> +        'min_ver'  => '5.84_01',
> +    },
> 
> Also, we might have to get Convert:BaseN packaged (also Test::DBIX::Class
> for this and Shibboleth)

Freeze for the next Debian release is approaching, so hopefully I'll get it done soon.
Comment 12 Magnus Enger 2014-10-15 12:51:03 UTC
(In reply to Katrin Fischer from comment #9)
> Hi Magnuse,
> 
> - I'd like to see the change that Chris pointed out - make the loading of
> the module mandatory. 

Did you mean *not* mandatory? 

> One important reason being that the new dependencies
> are not marked as mandatory, but without them the test suite won't work - a
> lot of the tests fail because of the missing dependencies on my system. 

Hm, I have some checks for the modules I have used in t/NorwegianPatronDB.t which should ensure the tests in it are skipped if any of the modules are missing. However, I had use'd one of the modules (SOAP::Lite) at the top of the test script, maybe that was tripping you up? Or were you seeing other tests fail because of the National library card stuff?
Comment 13 Magnus Enger 2014-10-15 12:59:55 UTC
(In reply to Chris Cormack from comment #8)
> The only suggestion I have is that we could perhaps use
> Module::Load::Conditional
> in C4/Members.pm, so that we don't have to load the module unless we need it.

Something like 

  use Module::Load::Conditional qw( can_load );
  can_load( modules => { 'Koha::NorwegianPatronDB qw( NLUpdateHashedPIN NLEncryptPIN NLSync )' => undef } );

instead of plain old 

  use Koha::NorwegianPatronDB qw( NLUpdateHashedPIN NLEncryptPIN NLSync );

?
Comment 14 Chris Cormack 2014-10-15 13:15:06 UTC
(In reply to Magnus Enger from comment #13)
> (In reply to Chris Cormack from comment #8)
> > The only suggestion I have is that we could perhaps use
> > Module::Load::Conditional
> > in C4/Members.pm, so that we don't have to load the module unless we need it.
> 
> Something like 
> 
>   use Module::Load::Conditional qw( can_load );
>   can_load( modules => { 'Koha::NorwegianPatronDB qw( NLUpdateHashedPIN
> NLEncryptPIN NLSync )' => undef } );
> 
> instead of plain old 
> 
>   use Koha::NorwegianPatronDB qw( NLUpdateHashedPIN NLEncryptPIN NLSync );
> 
> ?

Yep except wrap it in an if

if (syspref is set) {
  can_load ....
}

eg in Koha::QueryParser::Driver::PQF we do
if ( $format eq 'json' && can_load( modules => { 'JSON' => undef } ) ) {

Hope this helps
Comment 16 Robin Sheat 2014-10-17 01:15:03 UTC
libconvert-basen-perl has been uploaded to Debian, and is in the Koha repo. Can you please test it?
Comment 17 Magnus Enger 2014-10-21 07:53:20 UTC Comment hidden (obsolete)
Comment 18 Magnus Enger 2014-10-21 07:56:52 UTC
(In reply to Chris Cormack from comment #8)
> The only suggestion I have is that we could perhaps use
> Module::Load::Conditional
> in C4/Members.pm, so that we don't have to load the module unless we need it.

It looks like Module::Load::Conditional can't take a list of subs to import, so I went with Module::Load (which is already used a couple of places in Koha) instead, e.g.:

use Module::Load;
if ( C4::Context->preference('NorwegianPatronDBEnable') == 1 ) {
    load Koha::NorwegianPatronDB, qw( NLMarkForDeletion NLSync );
}
Comment 19 Chris Cormack 2014-10-22 21:04:14 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2014-10-25 21:11:53 UTC
I think we are not quite there yet :( Crypt/GCrypt.pm is marked non-mandatory, but seems to make the tests fail, if it's missing:

 OK	C4/Installer/PerlDependencies.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   OK	  valid

 FAIL	C4/Members.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid
		Use of uninitialized value in numeric eq (==) 

 FAIL	Koha/NorwegianPatronDB.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid
		Can't locate Crypt/GCrypt.pm in @INC (you may need to install the Crypt::GCrypt module) (@INC contains: /home/katrin/kohaclone /home/katrin/qa-test-tools /etc/perl /usr/local/lib/perl/5.18.2 /usr/local/share/perl/5.18.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.18 /usr/share/perl/5.18 /usr/local/lib/site_perl .) 

 OK	installer/data/mysql/updatedatabase.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   OK	  valid

 FAIL	members/deletemem.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid
		Use of uninitialized value in numeric eq (==) 

 FAIL	members/memberentry.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid
		Use of uninitialized value in numeric eq (==) 

 FAIL	members/moremember.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid
		Use of uninitialized value in numeric eq (==) 

 FAIL	members/nl-search.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid
		Use of uninitialized value in numeric eq (==) 
		BEGIN failed--compilation aborted 
		Can't locate Crypt/GCrypt.pm in @INC (you may need to install the Crypt::GCrypt module) (@INC contains: /home/katrin/kohaclone /home/katrin/qa-test-tools /etc/perl /usr/local/lib/perl/5.18.2 /usr/local/share/perl/5.18.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.18 /usr/share/perl/5.18 /usr/local/lib/site_perl .) 
		Compilation failed in require 

 FAIL	misc/cronjobs/nl-sync-from-koha.pl
   OK	  critic
   OK	  forbidden patterns
   FAIL	  pod
		*** ERROR: empty =head2  in file misc/cronjobs/nl-sync-from-koha.pl
   FAIL	  valid
		Can't locate Crypt/GCrypt.pm in @INC (you may need to install the Crypt::GCrypt module) (@INC contains: /home/katrin/kohaclone /home/katrin/qa-test-tools /etc/perl /usr/local/lib/perl/5.18.2 /usr/local/share/perl/5.18.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.18 /usr/share/perl/5.18 /usr/local/lib/site_perl .) 
		BEGIN failed--compilation aborted 
		Compilation failed in require 

 FAIL	misc/cronjobs/nl-sync-to-koha.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid
		Compilation failed in require 
		BEGIN failed--compilation aborted 
		Can't locate Crypt/GCrypt.pm in @INC (you may need to install the Crypt::GCrypt module) (@INC contains: /home/katrin/kohaclone /home/katrin/qa-test-tools /etc/perl /usr/local/lib/perl/5.18.2 /usr/local/share/perl/5.18.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.18 /usr/share/perl/5.18 /usr/local/lib/site_perl .) 
		Use of uninitialized value in numeric eq (==)
Comment 21 Magnus Enger 2014-10-29 11:10:47 UTC Comment hidden (obsolete)
Comment 22 Magnus Enger 2014-10-29 11:11:02 UTC Comment hidden (obsolete)
Comment 23 Magnus Enger 2014-10-29 11:37:50 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2014-10-29 22:54:52 UTC
Created attachment 32949 [details] [review]
[PASSED QA] Bug 11401 - Changes to Koha::Schema

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Katrin Fischer 2014-10-29 22:54:58 UTC
Created attachment 32950 [details] [review]
[PASSED QA] Bug 11401 - Add support for Norwegian national library card

This patch makes it possible to sync patron data between Koha and the
Norwegian national patron database, in both directions.

In order to use this, the following information is necessary:
- a username/password from the Norwegian national database of libraries
  ("Base Bibliotek"), available to all Norwegian libraries
- a special key in order to decrypt and encrypt PIN-codes/passwords,
  which is only available to Norwegian library system vendors
- a norwegian library vendor username/password

See http://www.lanekortet.no/ for more information (in Norwegian).

While this is of course an implementation of a specific synchronization scheme
for borrower data, attempts have been made to prepare the ground for other sync
schemes that might be implemented later. Especially the structure of the new
borrower_sync table might be reviewed with an eye to how it might fit other
schemes.

To test:

Since the password and cryptographic key needed to use this functionality
is only available to Norwegian library system vendors, only regression testing
can be done on the submitted code. Suggested things to check:

- Apply the patch and make sure the database update is done. This should add
  the new "borrower_sync" table and five new systmpreferences under the
  "Patrons" > "Norwegian patron database" category:
  - NorwegianPatronDBEnable
  - NorwegianPatronDBEndpoint
  - NorwegianPatronDBUsername
  - NorwegianPatronDBPassword
  - NorwegianPatronDBSearchNLAfterLocalHit
- Check that patrons can be created, edited and deleted as usual, when
  NorwegianPatronDBEnable is set to "Disable"
- Check that the new tests in t/NorwegianPatronDB.pm run ok, e.g. on a
  gitified setup:
  $ sudo koha-shell -c "PERL5LIB=/path/to/kohaclone prove -v t/NorwegianPatronDB.t" instancename
- Check that all the other tests still run ok
- Check that the POD in the new files itroduced by this patch looks ok:
  - Koha/NorwegianPatronDB.pm
  - members/nl-search.pl
  - misc/cronjobs/nl-sync-from-koha.pl
  - misc/cronjobs/nl-sync-to-koha.pl
  - t/NorwegianPatronDB.t

Sponsored-by: Oslo Public Library

Update 2014-09-18:
- Rebase on master
- Split out changes to Koha::Schema
- Incorporate new way of authenticating with NL

Update 2014-10-21:
- Rebase on master
- Use Module::Load to load Koha::NorwegianPatronDB in non-NL-specific
  scripts and modules
- Fix the version number of Digest::SHA
- Fix a missing semicolon in kohastructure.sql

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Katrin Fischer 2014-10-29 22:55:03 UTC
Created attachment 32951 [details] [review]
[PASSED QA] Bug 11401 - QA followup - fix pod

The QA script was complaining about some dodgy POD in C4/Members.pm,
that was not introduced by bug 11401. This patch fixes the POD, to
keep the QA script happy.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2014-10-29 22:55:06 UTC
Created attachment 32952 [details] [review]
[PASSED QA] Bug 11401 - QA followup

1) Be more careful when checking the NorwegianPatronDBEnable syspref.

Before:
if ( C4::Context->preference('NorwegianPatronDBEnable') == 1 ) {

After:
if ( C4::Context->preference('NorwegianPatronDBEnable') && C4::Context->preference('NorwegianPatronDBEnable') == 1 ) {

This should avoid complaints if the syspref is not initialized.

2) Fix some empty =head2 POD sections

3) Fix some indentation in patrons.pref, to make xt/yaml_valid.t happy

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
I couldn't find any regressions with adding, editing and deleting members.
Comment 28 Magnus Enger 2014-11-13 18:47:07 UTC
Created attachment 33528 [details] [review]
Bug 11401 - QA followup - Make the tests pass

The configs in koha-conf.xml needed to be mocked. There was also
a problem with how the NorwegianPatronDBEndpoint syspref was
getting checked in the .pm.
Comment 29 Tomás Cohen Arazi 2014-11-14 14:04:32 UTC
Created attachment 33570 [details] [review]
Bug 11401: dependency updates

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 30 Tomás Cohen Arazi 2014-11-14 14:05:44 UTC
Patches pushed to master.

Thanks Magnus and everyone involved in testing!
Comment 31 Tomás Cohen Arazi 2014-11-14 18:37:57 UTC
Created attachment 33582 [details] [review]
Bug 11401: (followup) make tests run on absent deps

The current code breaks if a dependency is missing. The evals are
rearranged so there's no error on missing dependency.

To reproduce:
- Have a dependency for t/NorwegianPatronDB.t removed
- Run
  $ prove t/NorwegianPatronDB.t
=> FAIL: You see an error similar to this (may vary depending on the lib you removed):

t/NorwegianPatronDB.t .. You tried to plan twice at t/NorwegianPatronDB.t line 37.

- Apply the patch
- Run
  $ prove t/NorwegianPatronDB.t
=> SUCCESS: Tests are skipped on missing lib

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 32 Jonathan Druart 2015-06-04 13:25:39 UTC
DROP TABLE IF EXISTS borrower_sync;
is missing in installer/data/mysql/kohastructure.sql