Bug 21068 - Remove NorwegianPatronDB related code
Summary: Remove NorwegianPatronDB related code
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 20287
Blocks:
  Show dependency treegraph
 
Reported: 2018-07-12 14:45 UTC by Martin Renvoize
Modified: 2020-11-17 20:06 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21068: Remove NorwegianPatronDB related code (100.26 KB, patch)
2018-08-15 16:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21068: DB changes (4.43 KB, patch)
2018-08-15 16:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21068: Remove NorwegianPatronDB related code (100.33 KB, patch)
2018-08-23 18:56 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 21068: DB changes (4.47 KB, patch)
2018-08-23 18:56 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 21068: Remove NorwegianPatronDB related code (100.38 KB, patch)
2018-08-24 06:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21068: DB changes (4.53 KB, patch)
2018-08-24 06:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21068: Remove table borrower_sync if needed (1.02 KB, patch)
2018-08-24 20:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21068: Remove NorwegianPatronDB related code (100.49 KB, patch)
2018-08-29 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21068: DB changes (4.58 KB, patch)
2018-08-29 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21068: Remove table borrower_sync if needed (1.08 KB, patch)
2018-08-29 19:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2018-07-12 14:45:36 UTC
When QAing Bug 20287 the one thing I wasn't 100% sure about was line 289 which we have marked with a FIXME. I could not find anywhere in koha that passed in a sync key to ModMember and as such we believe it was safe to remove it.   This bug serves as a note to get Magnus to double check our assumptions.
Comment 1 Magnus Enger 2018-07-23 11:48:11 UTC
As far as I can tell from the code and memory this is what was supposed to happen: 

- If NorwegianPatronDBEnable is enabled, a radio button named "sync" is added to the member add/edit form: http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt;h=36353607b3087b2a34bb72e053c57c664d4ae117;hb=HEAD#l619 This makes it possible to turn syncing on and off for individual patrons, as required by the Norwegian patron database. 

- The value of "sync" from the form is then stored in the database as borrower_sync.sync: http://schema.koha-community.org/18_05/tables/borrower_sync.html here: http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=C4/Members.pm;h=a87bdd6b8881037a9402388e2dec1582a6bca81f;hb=290341d8dbd085e9d60aa38e7c03fa840a85cb3a#l829

So the value for "sync" is passed into ModMember from the member add/edit form, if NorwegianPatronDBEnable is enabled. 

As far as I can tell, the FIXME at http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=Koha/Patron.pm;h=9524874acaba85ed69a5e291596930f52a4b65d3;hb=HEAD#l317 that now looks like this:

# FIXME THIS IS BROKEN # $borrowersync->update( { 'sync' => $data{'sync'} } );

should look like this:

$borrowersync->update( { 'sync' => $self->sync } );

Does that make sense? 

It would be great to hear the opinions of Benjamin and/or Petter at OPL on this, since they are the ones running this code in production. I'm adding them to the CC list.
Comment 2 Jonathan Druart 2018-08-08 17:51:24 UTC
(In reply to Magnus Enger from comment #1)
> should look like this:
> 
> $borrowersync->update( { 'sync' => $self->sync } );

$self->sync represents the Patron's attribute 'sync' (borrowers.sync), which does not exist and so the store call will explode.

Is there a way to test this feature in a dev environment?
Comment 3 Benjamin Rokseth 2018-08-08 20:53:50 UTC
Tomas PM'ed me on this and I briefly explained that we use the code, though heavily modified, in production. We add a Koha::Sync object and some logic, mostly to handle flaky implementation by the vendors of the sync endpoint, but I still believe it would better reside outside Koha community code, preferably as a module to koha core. 
Tomas argued for a plugin and I guess that would be a similar choice but faster to implement.

I dont think the community should be burdened with this kind of perifery functionality, especially since it migh be legacy at some point, and noone might notice.

If we're the only ones using it, I'd say better remove it from community code and let us struggle with the implementation
Comment 4 Magnus Enger 2018-08-09 08:11:07 UTC
(In reply to Benjamin Rokseth from comment #3)
> preferably as a module to koha core. 

What do you mean by this? 

> If we're the only ones using it, I'd say better remove it from community
> code and let us struggle with the implementation

Libriotech is also interested in having NL-functionality in Koha. It would be awesome if we could remove the code from Koha and turn it into a plugin, that we could collaborate on. But it would mean extending the plugin functionality in Koha to accomodate the necessary functionality of an NL-plugin. Is that something OPL could put some resources into?
Comment 5 Martin Renvoize 2018-08-09 10:37:35 UTC
I would wholeheartedly advocate this feature being ported to a plugin and I believe there's actually a very strong case for the hooks for such a plugin to be added to core to accommodate it. I'm sure NL aren't the only ones doing some level of user record syncing and as such I could see others benefiting from such core hooks.
Comment 6 Benjamin Rokseth 2018-08-09 13:19:08 UTC
(In reply to Magnus Enger from comment #4)
> (In reply to Benjamin Rokseth from comment #3)
> > preferably as a module to koha core. 
> 
> What do you mean by this? 
> 
1) Oh, just my hope to see the emerge of a Koha::Core so that we could actually make Koha modular and more maintainable for larger libraries like ourselves. I will file a bug explaining the feature soon.

> Libriotech is also interested in having NL-functionality in Koha. It would
> be awesome if we could remove the code from Koha and turn it into a plugin,
> that we could collaborate on. But it would mean extending the plugin
> functionality in Koha to accomodate the necessary functionality of an
> NL-plugin. Is that something OPL could put some resources into?

We could do this, though somewhat reluctant, since it defeats the purpose of 1), but I guess we could have both. 
Anyways we could probably leave the tables be (except maybe move hashed_pin) to a patron attribute) and add a Koha::BorrowerSync object for a general usage.
Comment 7 Magnus Enger 2018-08-09 13:35:09 UTC
Sounds good!
Comment 8 Tomás Cohen Arazi 2018-08-14 16:28:29 UTC
Work to be done:
- Introduce a more general Koha::Sync class
- Add hooks so the sync can be handled by configured plugins (besides some basic shipped implementations, if any).
Comment 9 Tomás Cohen Arazi 2018-08-14 17:27:11 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> Work to be done:
> - Introduce a more general Koha::Sync class
> - Add hooks so the sync can be handled by configured plugins (besides some
> basic shipped implementations, if any).

And remove the existing NW patron DB code :-D
Comment 10 Jonathan Druart 2018-08-15 16:23:22 UTC
Created attachment 77838 [details] [review]
Bug 21068: Remove NorwegianPatronDB related code

Bug 11401 introduced code to support Norwegian national library card.
This code is too specific to be part of Koha as it, it should be a
plugin instead.
Moreover nobody uses it, but a modified version (see comment 3).

Test plan:
Add/edit/delete patron and make sure there are no regressions introduced
by these patches
Comment 11 Jonathan Druart 2018-08-15 16:23:26 UTC
Created attachment 77839 [details] [review]
Bug 21068: DB changes

The update DB entry will not remove the prefs if NorwegianPatronDBEnable
is set.
This patch could be reverted locally by people using it.
Comment 12 Katrin Fischer 2018-08-15 19:31:46 UTC
If I read it right Magnus didn't say that they weren't using it? Just he was not opposed to change it into a plugin. I'd like to clarify that before we move on here.
Comment 13 Tomás Cohen Arazi 2018-08-15 20:35:25 UTC
(In reply to Katrin Fischer from comment #12)
> If I read it right Magnus didn't say that they weren't using it? Just he was
> not opposed to change it into a plugin. I'd like to clarify that before we
> move on here.

My understanding is that Oslo was the one using it, and they are currently using a highly customized version of this.
Comment 14 Magnus Enger 2018-08-17 09:16:14 UTC
(In reply to Katrin Fischer from comment #12)
> If I read it right Magnus didn't say that they weren't using it? Just he was
> not opposed to change it into a plugin. I'd like to clarify that before we
> move on here.

OPL is using a customised version of the code, Libriotech is not using it (yet). Removing the code from Koha is fine by me. Thanks for checking! :-)
Comment 15 Benjamin Rokseth 2018-08-23 18:56:10 UTC
Created attachment 78099 [details] [review]
Bug 21068: Remove NorwegianPatronDB related code

Bug 11401 introduced code to support Norwegian national library card.
This code is too specific to be part of Koha as it, it should be a
plugin instead.
Moreover nobody uses it, but a modified version (see comment 3).

Test plan:
Add/edit/delete patron and make sure there are no regressions introduced
by these patches

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 16 Benjamin Rokseth 2018-08-23 18:56:18 UTC
Created attachment 78100 [details] [review]
Bug 21068: DB changes

The update DB entry will not remove the prefs if NorwegianPatronDBEnable
is set.
This patch could be reverted locally by people using it.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 17 Benjamin Rokseth 2018-08-23 18:57:01 UTC
works as intended.
Comment 18 Katrin Fischer 2018-08-24 06:13:53 UTC
Created attachment 78106 [details] [review]
Bug 21068: Remove NorwegianPatronDB related code

Bug 11401 introduced code to support Norwegian national library card.
This code is too specific to be part of Koha as it, it should be a
plugin instead.
Moreover nobody uses it, but a modified version (see comment 3).

Test plan:
Add/edit/delete patron and make sure there are no regressions introduced
by these patches

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Katrin Fischer 2018-08-24 06:13:58 UTC
Created attachment 78107 [details] [review]
Bug 21068: DB changes

The update DB entry will not remove the prefs if NorwegianPatronDBEnable
is set.
This patch could be reverted locally by people using it.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Katrin Fischer 2018-08-24 06:14:16 UTC
Why major? Should we change this to enh now?
Comment 21 Katrin Fischer 2018-08-24 07:23:10 UTC
Why major?

Also: Should we not drop the table as well in the updatedatabase? We only have 1 library using it, I think it would be better.
Comment 22 Jonathan Druart 2018-08-24 20:00:54 UTC
Created attachment 78157 [details] [review]
Bug 21068: Remove table borrower_sync if needed
Comment 23 Jonathan Druart 2018-08-24 20:03:07 UTC
(In reply to Katrin Fischer from comment #21)
> Why major?

It's major for master (actually blocker for those using it, ie. nobody) because the feature is broken since bug 20287.

> Also: Should we not drop the table as well in the updatedatabase? We only
> have 1 library using it, I think it would be better.

Done in a follow-up (kohastructure changed in first patch).
Comment 24 Tomás Cohen Arazi 2018-08-29 19:16:48 UTC
Created attachment 78287 [details] [review]
Bug 21068: Remove NorwegianPatronDB related code

Bug 11401 introduced code to support Norwegian national library card.
This code is too specific to be part of Koha as it, it should be a
plugin instead.
Moreover nobody uses it, but a modified version (see comment 3).

Test plan:
Add/edit/delete patron and make sure there are no regressions introduced
by these patches

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2018-08-29 19:16:59 UTC
Created attachment 78288 [details] [review]
Bug 21068: DB changes

The update DB entry will not remove the prefs if NorwegianPatronDBEnable
is set.
This patch could be reverted locally by people using it.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2018-08-29 19:17:07 UTC
Created attachment 78289 [details] [review]
Bug 21068: Remove table borrower_sync if needed

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2018-08-29 19:18:40 UTC
Good job!
feature_creep--
cleanup++
Comment 28 Nick Clemens 2018-08-30 14:10:19 UTC
Awesome work all!

Pushed to master for 18.11
Comment 29 Martin Renvoize 2018-09-05 08:08:46 UTC
Added a deprecation warning to 18.05.x series rather than backporting as is.

See http://git.koha-community.org/gitweb/?p=koha.git;a=commitdiff;h=36abd064648cd88fa853e0f69726c1a68dc14fe5 for details.
Comment 30 Martin Renvoize 2018-09-05 08:09:08 UTC
Will be in 18.05.04
Comment 31 Fridolin Somers 2018-09-21 14:32:36 UTC
Depends on Bug 20287 not in 17.11.x