There's a regression in c74678a: when calling C4::Members::Attributes::UpdateBorrowerAttribute() the compiled attribute hash uses the member name "value" instead of "attribute". This causes silent data corruption as all updated borrower attributes are overwritten with NULL values. Best, Oliver
This one sounds very sensible, Alex please fix ASAP.
It affects master as well
Jonathan, as you just added #15889, please also take a look at #14625 and #16719 which have been sitting around for while now. There are a number of similar LDAP-Auth issues right now. Thanks guys, Oliver
Created attachment 57532 [details] [review] Bug 17615 - Fix updating borrower attributes in checkpw_ldap
Test plan: - Update your configuration file to use LDAP authentication and enable update (<update>1</update>) option, - create a mapping on an existing extended attrbitutes in koha-conf file, - login with a user who has not this extended attribute set. The attribute of related LDAP entry should be set. - check that the user attribute has been created.
Created attachment 57587 [details] [review] sign-off patch Attaching the signed-off patch manually since "git bz" never worked for me (returns that I'm not logged in on "attach", despite "apply" works). FYI, I verified the actual patch but not the unit test. Cheers
(In reply to Oliver Bock from comment #6) > Attaching the signed-off patch manually since "git bz" never worked for me > (returns that I'm not logged in on "attach", despite "apply" works). You must use the fishsoup branch (up-to-date) and correctly set your credential. See the wiki page for a step-by-step configuration. https://wiki.koha-community.org/wiki/Git_bz_configuration
Created attachment 57593 [details] [review] Bug 17615 - Fix updating borrower attributes in checkpw_ldap Signed-off-by: Oliver Bock <oliver.bock@aei.mpg.de>
(In reply to Jonathan Druart from comment #7) > You must use the fishsoup branch (up-to-date) Ah, I missed that one a few months ago. Thanks!
Created attachment 57619 [details] [review] [PASSED QA] Bug 17615 - Fix updating borrower attributes in checkpw_ldap Signed-off-by: Oliver Bock <oliver.bock@aei.mpg.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Cheers for the quick fix, works as expected
Please rebase asap!
Hi guys, any update on this?
The patch currently doesn't apply, we are waiting for a rebase to the current code base.
That's what I meant by "this" :-)
(In reply to Oliver Bock from comment #15) > That's what I meant by "this" :-) This bug report is assignee to you, so we are waiting for you.
Sorry, but I'm not the patch author. The author should rebase his patch, right? Cheers, Oliver
(In reply to Oliver Bock from comment #17) > Sorry, but I'm not the patch author. The author should rebase his patch, > right? > > Cheers, > Oliver Yep thats right
Ok, so just to confirm: you're not waiting for me but for Martin. Should the bug thus be assigned to him?
Created attachment 59220 [details] [review] Bug 17615 - Fix updating borrower attributes in checkpw_ldap Signed-off-by: Oliver Bock <oliver.bock@aei.mpg.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Was it a complex rebase? Maybe we can keep the sign-offs on this one.
Yes, please. The actual change is a single word :-)
Alex, 1 test fail for me: t/db_dependent/Auth_with_ldap.t .. 1/4 # Failed test 'Mapped attribute is BAR' # at t/db_dependent/Auth_with_ldap.t line 199. # got: undef # expected: 'BAR' # Looks like you failed 1 test of 9. # Failed test 'auth_by_bind = 1 tests' # at t/db_dependent/Auth_with_ldap.t line 243. # Looks like you failed 1 test of 4. # Failed test 'checkpw_ldap tests' # at t/db_dependent/Auth_with_ldap.t line 309. # Looks like you failed 1 test of 4. Does it pass for you?
(In reply to Jonathan Druart from comment #23) > Alex, 1 test fail for me: > > t/db_dependent/Auth_with_ldap.t .. 1/4 > # Failed test 'Mapped attribute is BAR' > # at t/db_dependent/Auth_with_ldap.t line 199. > # got: undef > # expected: 'BAR' > # Looks like you failed 1 test of 9. > > # Failed test 'auth_by_bind = 1 tests' > # at t/db_dependent/Auth_with_ldap.t line 243. > # Looks like you failed 1 test of 4. > > # Failed test 'checkpw_ldap tests' > # at t/db_dependent/Auth_with_ldap.t line 309. > # Looks like you failed 1 test of 4. > > Does it pass for you? Yes it passes successfully. Weird you got this, all seems to be mocked as expected.
(In reply to Alex Arnaud from comment #24) > (In reply to Jonathan Druart from comment #23) > > Alex, 1 test fail for me: > > > > t/db_dependent/Auth_with_ldap.t .. 1/4 > > # Failed test 'Mapped attribute is BAR' > > # at t/db_dependent/Auth_with_ldap.t line 199. > > # got: undef > > # expected: 'BAR' > > # Looks like you failed 1 test of 9. > > > > # Failed test 'auth_by_bind = 1 tests' > > # at t/db_dependent/Auth_with_ldap.t line 243. > > # Looks like you failed 1 test of 4. > > > > # Failed test 'checkpw_ldap tests' > > # at t/db_dependent/Auth_with_ldap.t line 309. > > # Looks like you failed 1 test of 4. > > > > Does it pass for you? > > Yes it passes successfully. Weird you got this, all seems to be mocked as > expected. Oops, using another database (the one provided by BZ 10337) it don't pass anymore
Created attachment 59263 [details] [review] Bug 17615 - Fix updating borrower attributes in checkpw_ldap
Created attachment 59264 [details] [review] Bug 17615 - Fix updating borrower attributes in checkpw_ldap Signed-off-by: Oliver Bock <oliver.bock@aei.mpg.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 59265 [details] [review] Bug 17615 - Fix unit tests
Created attachment 59266 [details] [review] Bug 17615 - Fix unit tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I confirm that the tests now pass. If the patch was ok before the rebase, it must still be. Passing QA, trusting author and previous signoffs
Pushed to master for 17.05, thanks Alex!
This patch has been pushed to 16.11.x and will be in 16.11.02.
I couldn't test this, trusting the tests, please don't disappoint!
Pushed to 16.05.x, for 16.05.08 release
IMHO, this can be closed. Thanks again!
After releases we move older bugs from "RESOLVED FIXED" to "CLOSED FIXED" in a big bulk edit, but if you tested, it's ok to just do it.