Forum OpenACS Development: How to handle dotlrn-specific changes in core?

An email thread got going on the OCT list about handling .LRN-specific changes in the ACS core. Jeff suggested we move it here, so I'm posting. Here's a piece of code from SloanSpace that motivated the discussion diff -r 463-final/packages/acs-admin/www/users/member-state-change.tcl ssv2-current/packages/acs-admin/www/users/member-state-change.tcl

45a46,47
> notification::request::delete_all_for_user -user_id $user_id
> 
> dotlrn_community::remove_user_from_all -user_id $user_id
 
53a56,57

> notification::request::delete_all_for_user -user_id $user_id
> 
> dotlrn_community::remove_user_from_all -user_id $user_id

I can't remember if I made this change or someone else, but I think what's going on is that we needed to remove community memberships and notifications for any user whose membership was no longer valid. Reasonable enough. The problem is, acs shouldn't have to know about .LRN and any other extensions that need to know when a member's state changes. In this particular case, I suspect that the right solution is to implement a callback mechanism using service contracts. Here's how it would work. Upon changing a member's state, the code would invoke the RemoveUser operation in all implementors of particular service contract. There are examples of this in the .LRN codebase. One example is the RemoveUser operation in the dotlrn_applet service contract.

Very wild guessing here, but as all core packages of acs start with "acs-" and all .LRN packages start with "dotlrn-" shouldn't it be possible to "overwrite" core packages TCL api in the dotlrn package, due to the fact that it is loaded *after* the core is loaded ?

If .LRN wants to make changes to OpenACS core that are only applicable for .LRN, there is always the risk that other pages use the normal OpenACS API and therefore break. But this is live after forking ;).

On the other hand, the above approach has a *huge* drawback. If you reload the original TCL of OpenACS core, you will get the OpenACS function back, not the .LRN one. It is something all developers would have to be taught (if you use .LRN do not reload packages, but restart the server).

My comment about forking was strictly ment to reflect the situation where you take core API and replace it with something else that is not maintained by the OpenACS core.

I'm a little bit reluctant to agree that we should change OpenACS core code to reflect that there is .LRN. In my opinion we should have a core code that does not need to know about application specific things like .LRN. After all, what happens if .WRK or .KNW or any other vertical application wants to do the same. It's more a clean code and design approach, which may or may not be able to survive taken into account the various needs put on the core.

If we use callbacks with service contracts that might be a solution, as this would make the core API independent from the *specific* application.

"change .LRN to use its own membership state change script. .LRN totally redefines what being a user means with its predefined groups, etc. Anyone writing code that changes fundamentals to this extent should be prepared to supply custom scripts to support such changes"

This is consistent with how the ecommerce package handled registration for 4.6.3, namely separate registration code.

Obviously, care would need to be taken to intercept the bypassed openacs protocols sitewide. Using the separate ec code as an example, there were cases where users found their way to the standard openacs registration outside of ecommerce. Those users lost anything they had put in their shopping basket. The final solution was to write the ec requirements into the standard openacs register behavior.

Likewise, as an alternate to writing completely separate code and trying to track all cases where default openacs behavior exists, is there a way to add a switch or something to the openacs procs so that calls that require site-wide non-openacs-default behavior will be consistent sitewide?

Hi Torben, how would you change the site-wide behaviour based on the switch/flag? Would you have an if/switch statement in the API of the site-wide files?

I'm torn between agreeing with all of you and disagreeing with all of you.

I should start by admitting that I don't personally have a problem with the way it's being done now.  I think that as long as we are all careful to not make gratuitous core changes and only put in things that really need to be there, it won't get out of hand even when there are multiple special cases in the future, and probably isn't worth all the extra effort we're going to have to go to to avoid it.  But I am clearly in the minority in that opinion.

My first reaction to the service contract idea was the same as Don's - they are too complex for the benefit here.  I am also concerned that we'd have a proliferation of service contracts that were only implemented by one package.

However, I don't really like the suggestion to re-implement the offending page in dotlrn, either.  As Torben points out that's what we did for registration in e-commerce;  it has worked ok for the site it was done for but it has been a headache for others, including Torben, ever since. So I'd rather not go down that road if I can avoid it.  This particular change may warrant doing this, but I don't think we should be encouraging people to duplicate code as the official way to handle this situation.

I'm trying to come up with a better suggestion to offer but so far nothing is coming to mind, probably because I think checking to make sure that dotlrn is mounted (not just installed, since that doesn't imply that it's in use) is the simplest and cleanest way to handle this.  I'm still thinking about it but I wanted to add my thoughts so far to the discussion.

Which part of service contracts are costly? ACS service contract stores implementation details in the database, but it creates a Tcl command for each implementation on startup, so there isn't any database overhead to call a service contract while running.

If you want to use callbacks I don't see a need to create another mechanism to define them.

I think the issue is deciding if there are clear places where a callback makes sense. We don't want to end up with a callback for every function in the core.

Andrew, Not anymore. A nice Tcl API to define service contracts was added for OpenACS 5. Still the trick is deciding what needs to be extenisble by service contracts.

Just to frame the discussion a bit, these are the sorts of issues we're talking about:

Currently there is one piece of code in core (I'm assuming that acs-authentication is part of core) which adds a new user to dotlrn if it's installed.  Then there is the code we're currently discussing, which removes a user from notifications and dotlrn.  And I've got another change to commit which makes sure that all class admins have admin rights on each uploaded file, which only applies to dotlrn.

There is also dotlrn-specific code in curriculum and survey, but I guess that's ok in non-core modules?

All three of these could be handled just like a pre/post-install APM callback, however those are implemented.  Is that what you had in mind, Andrew?  If so, what would the calling code (which would still have to be inserted into core code) look like?  I guess it has to look pretty generic or we will be right back to the situation Don objected to in the first place.

I like the idea of providing callbacks via a naming convention so have a proc like

callback namespace args

which would invoke all functions in the given namespace passing the given args. In this case the callback would be something like:

callback ::acs_user::change_state $old_state $new_state 

and you would define a function ::acs_user::change_state::dotlrn to handle this stuff for dotlrn.

alternatively you could just have an nsv of callback name and list of functions to invoke but it makes tracking down the callbacks a little more annoying I think.

CoP does not currently do anything fancy on register/unregister/drop but it (like most everything else) should do sensible things when a user is added/removed.

I don't think anyone disputes the need to be able to do package/site specific things in such sequences, the only question in my mind is whether we want to do all this via service contracts (our only real callback mechanism right now) or with something lighter weight (or maybe fix service-contracts to make doing the lightweight version easier than it is now).

At a minimum I would like a version of the tcl binding acs_sc::impl::new_from_spec which did not store anything in the db and hence could be easily changed/upgraded.

Clarification: "stores the callback" above should be "stores the list of procedures to call".

It's also worth pointing out that "naming conventions" have worked well for ad_page_contract and acs-templating (the template::widget::BLAH procs and ad_page_contract_filter_proc_BLAH procs are good examples of what I was thinking of).

Yes Dave, you and others advocated it some time ago here: https://openacs.org/forums/message-view?message_id=137070 - unfortunately I never got around following up on it, but I still think it would make things a lot easier to use namespaces for this.

Yes, I agree this approach is sounding good.

I think it only supports those for calls to package_instantiate_object and it's ilk, and as far as I can tell it's never been used (in fact I was considering posting about removing it since it does a query on every object instantiate which is empty but should almost certainly be aggressively cached).

It also shouldn't be in acs-subsite.