|
Description
Oliver Behnke
2016-06-13 13:45:43 UTC
Created attachment 53124 [details] [review] Patch: support password mapping with empty default This patch fixes a related piece of code. The comment says "if password was mapped". That's not what the original code does as the condition was actually "if password was mapped and it's default is not empty". This patch is required in the context of the other patch in this bug report which removes the broken failsafe (" " -> null/undef, true empty defaults). Any update on this? (In reply to Oliver Bock from comment #2) > Any update on this? 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. My patches only reflect my understanding of code concerned which build while tracking down the issues we saw (this was a tricky one). They're based on my interpretation of the original author's intentions, the reasoning of which I just don't know. Thus my hope was that my patch starts a discussion. Thanks, Oliver Steps to reproduce: 1) Map the attribute "cardnumber" for replication and update in your LDAP config 2) Create at least two users that don't yet have a value for that attribute in LDAP 3) Log in with both users one after the other 4) The second log in should result in a UNIQUE CONSTRAINT violation While "cardnumber" is NULLable, inserting/updating two borrowers with an empty string for it result in a UNIQUE CONSTRAINT violation. This proves that a semantically correct action fails with the current implementation. This wouldn't happen if NULL would be used instead of an empty string. HTH, Oliver Any update on this? Could you reproduce the issue? Thanks Created attachment 98560 [details] [review] Bug 16719: Pass through undef rather than empty string in LDAP mapping Nullable DB fields should be passed null in cases where ldap fields are empty and not replaced with empty strings. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98561 [details] [review] Bug 16719: (follow-up) Update check on password mapping Change to allow for unmapped default passwords Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Code works as expected and seems to resolve the issue described. I could not spot any regressions and it appears a sane change to me. Signing off Seems like you accepted both of my patches. Nice! Thanks guys! Created attachment 98573 [details] [review] Bug 16719: Pass through undef rather than empty string in LDAP mapping Nullable DB fields should be passed null in cases where ldap fields are empty and not replaced with empty strings. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 98574 [details] [review] Bug 16719: (follow-up) Update check on password mapping Change to allow for unmapped default passwords Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 98575 [details] [review] Bug 16719: (QA follow-up) Remove tabs Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Awesome to see the old and bad moving - thx guys! Nice work everyone! Pushed to master for 20.05 Pushed to 19.11.x branch for 19.11.04 backported to 19.05.x for 19.05.09 |