When extended patron attributes are mapped for replication and update via LDAP the attributes (here: "LOCATION") are properly replicated now (as of bug #8919) but on update the following error gets thrown: > opac-user.pl: DBD::mysql::st execute failed: Unknown column 'LOCATION' in 'field list' at /usr/share/koha/lib/C4/Auth_with_ldap.pm line 354, <DATA> line 558. The reason is that update_local() in Auth_with_ldap.pm doesn't care about those extended attributes at all. It's harmless because checkpw_ldap() seems to cover the update, not only the initial replication. But it's still confusing since a given admin is fooled by a false alarm. Thanks.
Thinking about this: this is NOT a harmless error since the whole update fails in this case.
Created attachment 45222 [details] [review] Proposed patch Attached a patch.
Raising severity to "normal" (for us it's actually "major") because of the above reason.
hmm, setting status to 'needs signoff'
(In reply to Mason James from comment #4) > hmm, setting status to 'needs signoff' hi Oliver, can you provide some testing steps for this patchset?
Not really, unfortunately. I'm not a Koha dev so I don't know anything about the unit test framework you use. Also, my patch was just meant as a proposal: it works for us but it should certainly be vetted (fixed/improved?) by the original author of that code section. Thanks, Oliver
Hi Oliver, testing steps can also be a "step by step" on how to reproduce the error. 1) go to the login page 2) enter... ... and so on. This makes it easier for people to understand the problem and test your code. Given the number of devs and Koha's age, we might not be able to find the original developer :)
Sorry, I thought the description was clear enough. Ok, let's see: 1) Create an extended patron attribute (that exists in LDAP) 2) Map that attribute in your LDAP auth config 3) Log in (first replication, should give no error in OPAC log) 4) Log out 5) Update attribute in LDAP 6) Log in 7) Error (see first comment) should be visible in OPAC log
Thx!
Code looks sane and does the trick.. but we'll need an update to the unit tests to cover the change before it'll get past QA. Oliver, are you happy to have a go at adding a test to Auth_with_ldap.t?
Created attachment 63410 [details] [review] [SIGNED OFF] Bug 14625 - LDAP: skip extended patron attributes in 'borrowers' attribute update * Any extended patron attributes will cause the update to fail as those attributes don't exist in the 'borrowers' table * The update of the extended patron attributes is already dealt with in checkpw_ldap() * Ergo: just skip those attributes here Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I've signed off the proposed patch, but marked the overall bug as failed qa untill we have test coverage.
On second thoughts, as update_local isn't currently tested at all I think perhaps it widens the scope a bit too much to expect you to write the tests. I'll open another bug to write the tests for the whole of update_local and hopefully I'll get around to writing them this century!
(In reply to Martin Renvoize from comment #13) > On second thoughts, as update_local isn't currently tested at all I think > perhaps it widens the scope a bit too much to expect you to write the tests. Yep, I agree :-) Glad to see progress! Thanks
Created attachment 63537 [details] [review] Bug 14625 - LDAP: skip extended patron attributes in 'borrowers' attribute update * Any extended patron attributes will cause the update to fail as those attributes don't exist in the 'borrowers' table * The update of the extended patron attributes is already dealt with in checkpw_ldap() * Ergo: just skip those attributes here Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> I did not test this patch but code looks good
Pushed to master for 17.05, thanks Oliver!
Great news! Hope this gets backported to 16.x as well. Thanks guys!
This patch has been pushed to 16.11.x and will be in 16.11.08.
Pushed to 3.22.x for 3.22.21
Pushed to 16.05.x, for 16.05.13 release
IMHO, this can be closed. Thanks again!