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

Request notifications

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.
Collapse
Posted by Andrew Grumet on
The code I mention above is exercised in dotlrn_community::remove_user in packages/dotlrn/tcl/community-procs.tcl.
Collapse
Posted by Malte Sussdorff on
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 ?
Collapse
Posted by Andrew Grumet on
That solution has the virtue of simplicity, it's much simpler than a callback mechanism.  But each specialty override would then have to carefully track changes in the original.  Sounds risky, no?
Collapse
Posted by Malte Sussdorff on
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).

Collapse
Posted by Andrew Grumet on
Hmm, I'm not sure I understand your comment about "live after forking".  If what I write below misses something important, feel free to clarify or steer the discussion.

I think it's inaccurate to say that ".LRN wants to make changes to OpenACS core that are only applicable for .LRN".  A better statement would be that ".LRN needs to know about events that are handled, and should be handled, by the core pages and/or APIs".

Afaik callbacks are pretty widely used and generally considered good design.

I don't think there's much of a risk to OpenACS pages or API calls provided that exceptions are adequately caught and reported.  Importantly, none of the solutions we're discussing could cause breakage to vanilla systems that didn't have .LRN installed, or for that matter any other extensions that might need to know about membership changes.

If we had to choose between the two approaches, with the caveat that there are probably other viable ones as well, I think the choice would be between a) leaving core alone, creating .LRN-specific versions of certain core procs and making sure these track the core versions, b) adding some complexity to the core with the expectation that it will reduce the need to track.  Approach b) doesn't guarantee that extensions won't break when the core apis change, but it does guarantee that orthogonal changes become available without any extra effort.

As I mentioned before, approach b) is taken in .LRN so we have working code we can mimic.

Collapse
Posted by Malte Sussdorff on
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.

Collapse
Posted by Don Baccus on
I've thought about the callback approach in the past but it seems complex and doesn't remove the need to scatter random integrated application actions in our vanilla code.  However, I'd greatly prefer it to the certain approach, because it boils down to the code having to ask whether or not a callback is defined for a specific action, rather than asking "is .LRN her?  is .WRK here?  is .BlahBlahBlah here?".

For the case at hand I suggest Janine just 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, IMO.

On the other hand, there are other minor changes to file storage made by Sloan that aren't going in because they're .LRN dependent.

My current position is that

1. Sloan's .LRN specific code isn't going into our non-.LRN packages (in my role acting as arbiter for Sloan, at their request)

2. I don't have a clean solution for allowing such customization

It's #2 I'm concerned about ...

Malte - replacing functions based on alphabetical order is a kludge, I really don't care for approaches of this sort.  Though it happens to be exactly what I did for Greenpeace Planet when faced with the same problem :) ("G" follows "A" in the alphabet just like "D" does)

Collapse
Posted by Torben Brosten on
"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?

Collapse
Posted by Torben Brosten on
The main point of my question is that if a switch, flag etc could be added to the system, then the site-wide behavior could be extended to whoever wanted the variant behavior to work with their dotWhatever.

dotLRN, dotWRK, and dotProject community behaviors are not mutually exclusive, but installation (or subsite) specific.

The community behavior in question, for example, may be something valued in nondotLRN environments too. Why not extend openacs? Isn't that the point of puting dotLRN back into openacs?

Collapse
Posted by Malte Sussdorff on
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?
Collapse
Posted by Torben Brosten on
Hi Malte,

I don't know. My understanding of the core is quite limited.  What would scale well?

The tcl gets compiled during the first execution. Looking at the new tcl benchmarks[1], I would think an extra if/else or switch would not be significant.

An 'if else' statement would work for cases where behavior needs to be consistent site-wide, and only 2 choices are available. Multiple conditions might suggest using a 'switch' statement, right? And since the default would be openacs, dotLRN procs could be called at their current locations without disruptive efforts of having to copy or move huge chucks of code around.

Maybe proc attributes should be used for cases when the choice does not have to be exclusive site-wide, with the default openacs behavior assumed when the attribute is not present. Since the core handles permissions, maybe declaring certain group-names/types as reserved would work as well.

Regarding where to store the parameters/flags, perhaps a cache-able table of parameter-value pairs loaded once on startup exists?

I'd imagine that guidelines for when and how to roll dotLRN behavior into the core will develop quickly as specific issues (like this thread) are brought forth and TIPped.

1. http://wiki.tcl.tk/tcl%20Benchmarks

Collapse
Posted by Janine Ohmer on
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.

Collapse
Posted by Andrew Grumet on
I agree that SC's are costly.  But callbacks as an idea can be cheap.  Maybe part of the problem is the SC implementation.  We might consider a lighter-weight, nsv-based thing that maintains a list of procs to call, each added to the nsv by the appropriate package-init.tcl script, with the arglist handled either by convention or passed through global scope.  If lighter weight, I wouldn't have a problem with callbacks that were only hooked into once, because the abstraction would be well worth it.  Still thinking...
Collapse
Posted by Dave Bauer on
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.

Collapse
Posted by Andrew Grumet on
I was referring to cost of setting one up in the first place.  I seem to remember it required dozens of lines of plsql.
Collapse
Posted by Dave Bauer on
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.
Collapse
Posted by Jeff Davis on
Of course try and change a service contract and you will be in a world of pain. Now, changing contracts should be pretty rare but adding a method to an existing contract is not entirely farfetched and as it stands I don't see how it can be done other that by knowing a tremendous amount about the details of the internals of acs-service-contracts.

I am with Andrew on the "service contracts are too hard" page for an awful lot of things.

Taking service contracts out of the DB and making it all driven from contract definitions in Tcl would make that easier although you would still need to have a mechanism to bind different implementations for some things (like payment gateways where you want one of several available implementations to be used).

Collapse
Posted by Janine Ohmer on
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.

Collapse
Posted by Dave Bauer on
I think creating a new user (or adding them to a group, I am not sure of the exact behavior) is a place where there is a high probability of customization by an application, so a general solution here would pay off I think. Of course, removing a user would probably also need a callback (because they usually are not actually deleted, but would not recieve notifications etc.)

Janine,

Thank you for mentioning the APM callbacks. That sounds like the appropriate level of abstraction in this sort of case.

Collapse
Posted by Jeff Davis on
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.

Collapse
Posted by Caroline Meeks on
I agree with Dave. Actions such as adding and removing notifications when a user is added or removed from a group is not a dotLRN specific need. What is .LRN specific is probably exactly what is added or removed.

Here is a non-.LRN use case, and I just consider this one of many possible use cases. On Aristoi I'm using subsites with bugtracker mounted for various clients. I'd like to set up bugtracker so that when a user was added to the subsite he was automatically subscribed to bugtracker and definitely I want the user to be automatically unsubscribed if he is dropped from the subsite.

Jeff, does COP do any sort of similar operations when a user is added/removed from a group?

Collapse
Posted by Jeff Davis on
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.

Collapse
Posted by Andrew Grumet on
The signature Jeff proposes is basically what I had in mind.  Formalizing the naming convention, and perhaps putting the string "callbacks" somewhere in the namespace as a safety, would then allow us to track down the stuff to call using "info procs".

On a brief skim I think APM callbacks have a similar signature but that APM ultimately stores the callback in the db.  We'd be generalizing that design and moving the storage from the db to Tcl memory.

Collapse
Posted by Andrew Grumet on
Clarification: "stores the callback" above should be "stores the list of procedures to call".
Collapse
Posted by Andrew Grumet on
Oy, sorry for the multiple corrections here.  APM's actually a little different.  The APM callbacks are for specific, one-time events for specific package versions.  I'm pretty sure we wouldn't expect to invoke the "after-install" callback for acs-content-repository v5.1.0 multiple times with different arguments.  The same can't be said for a hypothetical "on member state change" for user 123456.
Collapse
Posted by Jeff Davis on
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).
Collapse
Posted by Dave Bauer on
Sounds good to me. I think I advocated this sort of thing before, but there wasn't much consensus. It is definitely easier to use a standardized naming convention. It is also used by acs templating to define procedures for form widgets.
Collapse
Posted by Tilmann Singer on
Yes Dave, you and others advocated it some time ago here: http://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.
Collapse
Posted by Janine Ohmer on
Sounds like we are reaching consensus, which is good.  Now how is this going to get implemented?  Any volunteers?  I can't do it, at least not right away;  my plate already overfloweth.
Collapse
Posted by Don Baccus on
Yes, I agree this approach is sounding good.
Collapse
Posted by Don Baccus on
Digging around in the code ... subsites already implement callbacks for state changes on objects (insert, update, etc) so while this isn't a general mechanism it is one to explore ...

This facility could be used for the add user case, for instance.

Collapse
Posted by Jeff Davis on
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.

Collapse
Posted by Don Baccus on
Most of the code in acs-subsite shouldn't be in there, actually, I think they were just put there because for some reason someone decided acs-kernel shouldn't have a tcl lib.

The main drawback to the existing callback mechanism is that it's SQL only ...