Working on bug 27857 (actually, on the follow-up bug 27956), I found that the Koha::Patron->extended_attributes setter is used to write the new attributes. This happens inside a Koha::Patron::Attribute::Types loop which is used to determine 'valid' attributes (good). The problem is 'extended_attributes' actually calls: $self->extended_attributes->filter_by_branch_limitations->delete; which deletes the existing attribute. So calling it for setting a single attribute makes it... delete all the attributes each time! There's also some funky logic in checkpw_ldap. The $patron object seems to be retrieved in several places and even inside a loop.
Created attachment 118364 [details] [review] :x
Something like that? What do we do if an exception is raised?
Created attachment 118365 [details] [review] Bug 27957: Fix LDAP and patron's attributes
(In reply to Jonathan Druart from comment #2) > Something like that? What do we do if an exception is raised? I would rather loop through them using $patron->add_extended_attribute inside a try/catch block and warning as we already do.
Then we will keep adding them, and won't deal with the constraints.
Maybe I misunderstood you. Don't you think the code is more readable as it?
(In reply to Jonathan Druart from comment #6) > Maybe I misunderstood you. > > Don't you think the code is more readable as it? Not at all. But the old behavior seemed to be to just warn on failed attributes. Using a single call like that will prevent attributes to be set.
Created attachment 118384 [details] [review] Bug 27957: Fix LDAP and patron's attributes
Shoudl this have been marked as NSO?
(In reply to Katrin Fischer from comment #9) > Shoudl this have been marked as NSO? No, it's missing tests.