Forum OpenACS Development: approach to refactoring ACS Mail Lite

Hi all,

I'm revising acs-mail-lite to work with incoming email via IMAP4: https://openacs.org/forums/message-view?message_id=5370874.

While integrating this feature, do I take this opportunity to also refactor existing openacs-core API, when available?

For example, acs-mail-lite/tcl/acs-mail-lite-procs.tcl defines acs_mail_lite::get_parameter which duplicates apm_package_key_from_id.

If I am to use existing core API when available, I'll limit refactoring to only use API of packages that ACS Mail Lite already depends on directly or indirectly according to this package dependency chart made by analyzing oacs-core .info file package requirements.

cheers,
Ben

Collapse
Posted by Gustaf Neumann on
While i highly appreciate contributions to acs-mail-lite, i would not like to block for this reason the release of OpenACS 5.9.1, which is overdue. I am currently on vacation, but the plan is to work on the release as soon i am back in Austria. So, the changes should to into the development release after 5.9.1.

Concerning refactoring: acs_mail_lite::get_parameter is not a duplication of apm:package_key_from_id, but is a specialization of parameter::get_from_package_key by removing the need of passing-in the package_id of the singleton. It returns a parameter vaue for a specified parameter. apm_package_key_from_id returns the package_key from a given package_id.

Whether or not acs-mail-lite uses parameter::get_from_package_key or acs_mail_lite::get_parameter is a minor concern: none of the API calls is deprecated, abstracting away repetitious details is in general a good idea, but it would be better, when it would be available for all packages (or for all singleton packages) in a consistent manner, which is not the case.

For changes, it is in general recommended not to mangle all changes into a single commit, but provide separate changes for independent changes (e.g. one change for each concern, e.g. new functionality, for interface changes). This makes it much easier to review changes.

Collapse
Posted by Benjamin Brink on
Hi Gustaf,
Thank you.

Yes. The changes I'm working on with ACS Mail Lite should not affect any existing development paths, including 5.9.1

Right, "acs_mail_lite::get_parameter is not a duplication of apm_package_key_from_id". Sorry for the bad reference. I meant acs_mail_lite::get_package_id vs. apm_package_key_from_id, and acs_mail_lite::get_parameter vs. acs_mail_lite::get_parameter as examples.

Anyway, don't change code if not necessary. point taken.

I'm going to use the new package, then submit the package via the apm package submission feature. If the submission feature doesn't work for existing core packages, I'll submit a tar file.

This is the general strategy I'll use:

1. Do not change existing code in any way that breaks existing patterns or use.

2. Create new code when existing code does fit.

3. When changing existing code is necessary, make clear comments about why changes are needed. Minimize changes by making alternate procedures, and changing calls at a higher level of operation. This way, an experienced code reviewer can choose to adapt new features into existing code instead of accepting proposed new code.

Makes sense. Thanks again,
Ben

Collapse
Posted by Benjamin Brink on
errata.

2. Create new procedures when existing procedures do *not* fit new uses.

Collapse
Posted by Benjamin Brink on
errata

"I meant acs_mail_lite::get_package_id vs. apm_package_key_from_id"

becomes

..I meant acs_mail_lite::get_package_id vs. apm_package_id_from_key

Turbulence happens.

Collapse
Posted by Benjamin Brink on
errata

"acs_mail_lite::get_parameter vs. acs_mail_lite::get_parameter"

becomes

"acs_mail_lite::get_parameter vs. parameter::get or parameter::get_from_package_key"

turbulence happens

Collapse
Posted by Iuri Sampaio on
Hi Ben,

Interesting post. I was going to play with pad_procs mentioned, but unfortunately, on my installation they don't work.

I've created a new parameter, of a new custom pkg, then called it as in the example bellow. ci_url is void/null. I could have added the default switch, but that'd masquerade the actual issue. I'm about to got deeper debugging ad_procs ::get and get_from_package_key...

set ci_url [parameter::get \
-parameter CodeIgniterWebsiteUrl \
-package_id [ad_conn package_id]]

Best wishes,

Collapse
Posted by Benjamin Brink on
Hi Iuri,

Maybe parameter values don't update until nsd restart for the same reason that scheduled procs don't update on package reload via acs-admin/apm.

Making these updates consistent with other package reload features appears on target for OpenACS 5.10 release in the agenda/wish-list item: "dynamic reloading reform, including support for scheduled procedures": https://openacs.org/xowiki/openacs-todo

Your findings and contributions are welcome.

best wishes,
Ben

Collapse
Posted by Iuri Sampaio on
It's something else. I had restarted Naviserver. A couple of time actually while I was debugging, but I still haven`t found the cause.

Best wishes,

Collapse
Posted by Benjamin Brink on
Hi Iuri,

If you can find a way to replicate the issue, then maybe someone can help identify the cause.

Are you using that code snipet in a scheduled proc?

If so, [ad_conn package_id] would normally throw an error, because there is no connection to get a package_id.

However, a catch statement somewhere in parameter::get is filtering the error, and returning empty string instead. Is this what you are seeing?

cheers,
Ben

Collapse
Posted by Benjamin Brink on
Actually, ad_conn now catches the error. Here is example code in a package-init.tcl file, and how error is reported as a "Notice" in log:

set package_id [ad_conn package_id]
ns_log Notice "ad_conn package_id = '${package_id}'"

Notice: request processor did not set <ad_conn package_id>, fallback: 624
[25/Jul/2017:19:18:09][1826.7f0d32aca740][-main-] Notice: ad_conn package_id = '624'

Subsequently, your code example tries to get value from a different package.

Collapse
Posted by Iuri Sampaio on
Ben,

Neither assigning a static value for package_id, nor creating a whole new package, with a new parameter worked fine. I ran the tests in the acs-developer shell

Example: parameter::get_from_package_key -package_key "evex-users" -parameter "ExternalWebsiteURL"

Both ad_procs [parameter::get] or [parameter::get_from_package_key] return blank (i.e. "") as value.

p.s. Parameter's accessible through APM -> (evex-users) pkg admin -> parameters

Best wishes,
Iuri

Collapse
Posted by Benjamin Brink on
Hi Iuri,

Sounds like a gremlin in the system!

Surely it's time to fill those procs with ns_log Debug comments.. and set Dev or Debug to on in /admin/nsstats.tcl

Maybe issue is temporal, taking a few minutes after restart to complete background processes?

Anyway, certainly worth investigating.
best wishes,
Ben

Collapse
Posted by Gustaf Neumann on
There is nothing wrong with the ad_procs ::get and get_from_package_key, there are used all over the system. If these functions would be broken, we would have big troubles. When someone adds a new parameter to an existing package, it is not sufficient to update the .info file via the package manager, but one has to increment the version number of the package to be able to run an upgrade; In the upgrade step, the parameters are actually added to the database. Providing a non-default value to this new parameter can be done after running the upgrade generically via the web-interface in /admin/site-map.

Note that parameter::get_from_package_key is supposed to work only for singleton packages. New packages should use global parameters instead (see [1]).

Hope this helps
-gn

[1] https://openacs.org/api-doc/proc-view?proc=parameter::get_from_package_key&source_p=1

Collapse
Posted by Iuri Sampaio on
Thanks Gustaf,

I've done all that. I believe the problem exists because, instead of writing it directly within the file .info, I'd created the parameter through the interface (APM). Afterwards, I confirmed that the parameter was automatically generate within .info, then increased manually the version number and restarted NS, but unfortunately ad_procs still persist returning "".

My package isn't singleton, but I did use global scope.

parameter scope="global" datatype="string" min_n_values="1" max_n_values="1" name="CodeIgniterWebsiteUrl" default="http://dev.evex.co"; description="Website URL for Venues &amp; Services CMS in CodeIgniter"/

I'll debug this problem over the weekend and let you .

Getting back to the subject of refactoring ACS Mail Lite, I can help to do the job. Let me know if you guys need want my help.

Best wishes,

Collapse
Posted by Gustaf Neumann on
As mentioned above, you have to run an "upgrade from the local file system" to process the changes in the .info file. just restarting the server after updating the info file is not enough.

In order to access the global parameter, one has to use parameter::get_global_value [1] instead of parameter::get. it might be an option to work towards unifying these calls, there is an agenda-point for parameters in the the 5.10.0 agenda.

-gn
[1] https://openacs.org/api-doc/proc-view?proc=parameter%3A%3Aget_global_value

Collapse
Posted by Iuri Sampaio on
Thanks Gustaf,
ad_proc parameter::get_global_value worked well.
It'd be great if I could participate in 5.10.0 agenda.
Best wishes,