Forum OpenACS Development: Design Issues in acs-mail-lite
Notificaitons was changes to use acs_mail_lite::complex_send instead of rolling it's own HTML mime formatted email. This is a good idea in general if it worked. Unfortunately acs_mail_lite::send has a parameter called extra_headers and it accepts an ns_set handle. acs_mail_lite::complex_send also has an extra_headers parameter that accepts a list_of_lists. This inconsistency leads to great problems in the code.
The change to notifications did not include a change to pass extra_headers as a list_of_lists. This breaks sending notifications in subtle ways. In one case an email would go out with the wrong email body!
I haven't had a chance to track down all the places that could be affected by this, but little issues like this plague the toolkit. Having such a fundamental poor design, passing around extra_headers in a different format in the same tcl library, leads to errors, confusion, and is one of the systemic problems in the toolkit.
This is also ver hard to test, the way the procedures in acs-mail-lite are organized, it is difficult to write automated tests that don't send emails to real people.
There needs to be some major work done to reorganize acs-mail-lite and make it easier to test so we can ensure our sites are sending out correct emails, and to make sure the client packages can consistently use the provided APIs. This is particularly important now that acs-mail-lite is part of OpenACS Core.
All these problems appear to only occur on HEAD. It looks like the packages for acs-mail-lite and notificaitons for OpenACS 5.3 can not come from HEAD but will need to be pulled frm OpenACS 5.2 until this all can be worked out.
At the moment we have three different designs for handling incoming e-mail, all of them working to a certain degree. We should best come up with one generic solution and until this can be achieved without rewriting tons of code, we should offer the alternatives in different packages (notifications::load_qmail_mail_queue, acs-mail-lite::load_mail_dir and (currently) acs-mail-lite::load_emails). Not to mention that notifications incoming is a rewrite of acs-mail-lite::load_mail_dir incoming and a lot of procedures have been copy pasted. So a generic cleanup in notifications is in order anyway.
We have two fundamentally different approaches to sending out e-mail, all in one package. Unifying both might be a little bit hard, but it should be our goal to have one single send procedure to handle all the options and maybe span it out into seperate sub procedures.
Here are some fundamental issues with this:
- Complex_send at the moment relies on smtp::sendmessage. As this implies, it does not use the acs-mail-lite::deliver procedure which is the one supporting both SMTP as well as the sendmail binary. Complex send at the moment only supports SMTP sending.
- Complex send does not send the correct MAIL FROM parameter which is set to the bounce address. The way it is done with ns_smtp is not supported by smtp::sendmessage. It does support a different reply to address, but this is not something you want to set to the bounce address.
- Complex_send is still undergoing constant development to add new switches which are useful in sending out e-mail. Though these could be passed (a lot of the time) as extraheaders, you have to know th RFC to pass the correct extra headers, much easier to use a TCL flag.
- Until you support incoming e-mail, you cannot test outgoing e-mail. Therefore writing test procedures has to include the correct setup for handling incoming e-mails. As the handling of incoming email is not unified, we should start there. I would probably just write a testcase sending e-mails to "mailto:acs-mail-lite-test@yourserver.com"; and then have a callback for acs-mail-lite::incoming_email to deal with these test e-mails. Though I'm not entirely sure how I would test how exactly to write these tests, so if someone is willing to sit down with me to write some tests I can make sure I write up how to setup the environment for these tests.
- Complex send could use some cleaning up, as some code parts should probably go into seperate procedures. As an example the whole code for getting the members of a list of groups should probaly folded in group::get_members which would take a list of party_ids instead of a group_id. And maybe someone else will find other things to clean up as well.
- Complex send supports sending e-mails from multiple servers. So if you put your e-mail into a queue, you could have multiple webservers in your cluster take and process the e-mail instead of only one (as with acs-mail-lite::send). This definitely needs some more testing, as we so far only tested it with the main webserver and a seperate server dedicated to sending out e-mails and server static webpages, so not in a real cluster environment as you know it from OpenACS.
Taking out complex_send from acs-mail-lite is a piece of cake as it already comes in it's own -procs.* files. So if it is deemed that the whole complex_send* stuff should be tested a lot more before put into acs-core, this is the time to split it off in HEAD. If this is prefered by the OCT I would in the same time move the incoming e-mail procedures (the new ones) over to this package and then acs-mail-lite could stay juvenile as it was before this functionality was added. It would make dealing with notifications a lot easier, as you could just check if complex_send is installed and if yes, then use it. I would then go and get rid of the notifications incoming e-mail handling if complex send is installed.
Maybe this should have been done right from the beginning, but at that point acs-mail-lite was not core, so I think noone thought about it (and to be honest, I did not think we would continue to make so many changes after the incoming e-mail system was announced and discussed at https://openacs.org/forums/message-view?message_id=316225)
