I would like to propose that C4::Members::Attributes could be more tightly integrated into C4::Members so that AddMember(%member) or ModMember,... or whatever... would also call Attributes add. This would allow ppl to have only one function call to add a Member and not many little functions to call. And would eventually be more friendly for unifying calls to functions. Does that sound a good idea ? I could send a patch tomorrow about that. Comments welcome. -- Henri-Damien LAURENT
I have mixed opinions about this. On the one hand it seems reasonable, and on the other it seems unnecessary. There are only three places that call AddMember: memberentry, import_borrowers and Auth_with_ldap. In fact, I recently coded a feature for a client so that import_borrowers has a runtime option on how to handle extended attributes (overwrite all, or preserve -- like other fields). Making a separate call for the attributes was not a serious problem. LDAP currently can't handle extended attributes, so there's no benefit there. This overwrite/preserve issue would need to be addressed in the refactor also, to ensure that we know for sure what behavior is expected when ModMember (in particular) is called. In the case where only extended attributes are being edited, it certainly is logically simpler to have a call just for that, though that doesn't necessarily preclude the kind of integration you propose. --Joe On Mon, Mar 9, 2009 at 4:39 PM, LAURENT Henri-Damien < henridamien@koha-fr.org> wrote:
I would like to propose that C4::Members::Attributes could be more tightly integrated into C4::Members so that AddMember(%member) or ModMember,... or whatever... would also call Attributes add. This would allow ppl to have only one function call to add a Member and not many little functions to call.
And would eventually be more friendly for unifying calls to functions. Does that sound a good idea ? I could send a patch tomorrow about that.
Comments welcome.
-- Henri-Damien LAURENT
Joe Atzberger a écrit :
I have mixed opinions about this. On the one hand it seems reasonable, and on the other it seems unnecessary. There are only three places that call AddMember: memberentry, import_borrowers and Auth_with_ldap. In fact, I recently coded a feature for a client so that import_borrowers has a runtime option on how to handle extended attributes (overwrite all, or preserve -- like other fields). Making a separate call for the attributes was not a serious problem. LDAP currently can't handle extended attributes, so there's no benefit there.
This overwrite/preserve issue would need to be addressed in the refactor also, to ensure that we know for sure what behavior is expected when ModMember (in particular) is called. In the case where only extended attributes are being edited, it certainly is logically simpler to have a call just for that, though that doesn't necessarily preclude the kind of integration you propose.
--Joe <brain dumping> My thought is that if we were aiming at standardizing work and getting more and more OOP, then we might find easier to use only one kind of API and not one API for each piece of data. I am working on a special import for borrowers, but also on a subscription import. And it seems quite odd to me that we really use so different APIs to add a subscription or a Member. (I reckon I should also have suggested to unify AddSubscription and AddMember and ModSubscription and ModMember but was too much in a rash to do so and to achieve such a change at once.) Moreover, if we are gaining more and more developers, we should think to get things more and more organized so that developing new features would be easier and not having to see many files in order to have a complete documentation on how to create a new patron.
And it would not be deleting C4::Members::Attributes, it would rather consists in wrapping calls to C4::Members::Attributes functions into Get/Add/Mod/Member We could still call it on its own. But it would not be required to search the extended syspref and create Attributes in perl scripts. The fact is that import_borrowers requires 2 different syntaxes for elements and those which are attributes, and those which are not, and it doesnot seems that necessary to me. Since we should not be able to give a Patronattribute the same name as a borrowers.field name. But maybe adding a patron attribute could split values on a delimiter so that an attribute can be repeatable. name;surname;sort1;sort2;mylocalsortfield; John;Doe;;;A,B,C,D; </Brain dumping> -- henri-Damien LAURENT
participants (2)
-
Joe Atzberger -
LAURENT Henri-Damien