Forum OpenACS Development: How to handle dotlrn-specific changes in core?
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.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
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).
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.
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.
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)
"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?
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?
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, 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.
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.
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.
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).
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.
Thank you for mentioning the APM callbacks. That sounds like the appropriate level of abstraction in this sort of case.
callback namespace argswhich 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_stateand 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.
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?
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.
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.
This facility could be used for the add user case, for instance.
It also shouldn't be in acs-subsite.
The main drawback to the existing callback mechanism is that it's SQL only ...