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.
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.
(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?
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
(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?
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.
(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.
Sounds good!
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).
(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
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
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.
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.
(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.
(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! :-)
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>
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>
works as intended.
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>
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>
Why major? Should we change this to enh now?
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.
Created attachment 78157 [details] [review] Bug 21068: Remove table borrower_sync if needed
(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).
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>
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>
Created attachment 78289 [details] [review] Bug 21068: Remove table borrower_sync if needed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Good job! feature_creep-- cleanup++
Awesome work all! Pushed to master for 18.11
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.
Will be in 18.05.04
Depends on Bug 20287 not in 17.11.x