Summary: | LDAP: mapped ExtendedPatronAttributes cause error when updated on authentication | ||
---|---|---|---|
Product: | Koha | Reporter: | Oliver Behnke <oliver.behnke> |
Component: | Authentication | Assignee: | Oliver Behnke <oliver.behnke> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | major | ||
Priority: | P3 | CC: | dpavlin, hblancoca, jonathan.druart, julian.maurice, kyle, martin.renvoize, mtj, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17615 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15889 |
||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 18587 | ||
Attachments: |
Proposed patch
[SIGNED OFF] Bug 14625 - LDAP: skip extended patron attributes in 'borrowers' attribute update Bug 14625 - LDAP: skip extended patron attributes in 'borrowers' attribute update |
Description
Oliver Behnke
2015-07-31 09:03:27 UTC
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! |