Forum OpenACS Development: .LRN callback for member approval

Posted by Malte Sussdorff on 01/09/07 03:32 PM

I need a callback to execute code while a member is beeing approved to a community using dotlrn_community::membership_approve. Please comment before I commit.

Here is the code:

ad_proc -callback dotlrn_community::member_approve {
-community_id
-user_id
} {
This callback will allow other packages to execute actions upon membership approval
} -

And here is the callback hook:

ad_proc -public membership_approve {
{-user_id:required}
{-community_id:required}
} {
Approve membership to a community
} {
db_1row select_rel_info {}

db_transaction {
membership_rel::approve -rel_id $rel_id

applets_dispatch \
-community_id $community_id \
-op AddUserToCommunity \
-list_args [list $community_id $user_id]

callback dotlrn_community::member_approve -community_id $community_id -user_id $user_id

}
}

2: Re: .LRN callback for member approval (response to 1)

Posted by Malte Sussdorff on 01/09/07 03:38 PM

In case you are wondering, the callback is needed for:

a) Auto approval into different communities
b) Sending a customized e-mail to the approved member
c) Triggering code in a custom application

This callback could eventually be put into good use e.g. for dotlrn-ecommerce, triggering the payment processing once the user is approved to the community. Or, if you are a university with a credit point system where the students have x amount of credit points which they can use up per semester, deduct the number of points from their allowance.

3: Re: .LRN callback for member approval (response to 1)

Posted by Don Baccus on 01/09/07 05:43 PM

If we're to have such a callback I'd prefer one callback for any state change, rather than a specific callback for the change of state to "approved".

And instead of putting it where you've suggested, I'd suggest the "change_state" proc inside acs-tcl/tcl/membership-rel-procs.tcl.

My preference is for a minimum of callbacks generalized to be as useful as possible, rather than creating a single specific callback for a particular need you have at a particular moment.

4: Re: .LRN callback for member approval (response to 3)

Posted by Malte Sussdorff on 01/09/07 06:03 PM

Don, the point is simple. Having such a generic callback will make it much harder to use the callbacks and, more to the point, will make it even less obvious to the developers. I mean, how many functional levels in an API does the developer drill down to if he is to use the API.

Going with your suggestion would mean, that a developer who wants to make an extension as I need it, to look at this procedure, look at the underlying procedures, and find the callback. Once done, he needs to write some code, which, given a rel_id will look:

- is this rel_id present in dotlrn_member_rels_full for every relationship change in a relationship within the system.
- if it is a rel_id that belongs to a dotlrn_member_rel, get the community_id and the user_id.

For me the fundamental issue is, if we are supposed to drill down to the lowest sensible level when adding callbacks, it will make it much harder to read the code and will cause detecting side effects much more difficult. This is why I would limit the scope of a callback and have many of them instead of having a large scope for a callback but just a few.

5: Re: .LRN callback for member approval (response to 1)

Posted by Dave Bauer on 01/09/07 05:55 PM

I agree with Don's analysis. Its quite likely that dotlrn does not call the particular generic API to change the member state (I have not checked), but putting the callback in the most general place, and refactoring the code to use the general API is a better design.

Callbacks should go were they can be most useful and reused, that is, in exactly the type of procedures that should be reused in many places.

6: Re: .LRN callback for member approval (response to 5)

Posted by Malte Sussdorff on 01/09/07 06:52 PM

"Callbacks should go were they can be most useful and reused, that is, in exactly the type of procedures that should be reused in many places."

Which also means the implementations are called in many places. So in my specific case, my custom callback would be called on each change of status of a relationship, of which we have quite some in OpenACS.

Limiting the calls using the implementation does not work though as we cannot limit ourself to say "we only allow one callback per type". This has proven to be desaster in the implementation attempt for incoming e-mail callbacks in acs-mail-lite (in case you have been thinking about calling the implementation as the type of the relationship, e.g. dotlrn_membership_rel).

If you suggest to enforce a naming conventions for callbacks though, e.g. "$type_< your_imp_name>", I am not sure if you can call all callbacks whose implementation starts with $type.

That is, if you are concerned with the number of calls to the callback in the first place.

7: Re: .LRN callback for member approval (response to 1)

Posted by Lee Denison on 01/09/07 07:31 PM

I agree with Don and Dave. I would think that a small number of generic callbacks is more likely to be both useful and easy to understand for OpenACS developers as a whole.

Callback implementors would have to accept and process data about the event that has been triggered (the state change). But the result is less callback implementations, which surely will make maintenance easier. Besides, a disincentive to use a callback to solve any particular problem is probably a good thing.

The performance concerns (generic callbacks being called more often) are valid. But I think the guiding principle should be governed first by ease of maintenance. Perfomance concerns should be considered in specific cases and with supporting data. So as we are only starting to introduce callbacks into core I would suggest implementing them the way we want first and assessing the impact on performance as it becomes apparent on real sites.