This module is to provide seamless integration between mailman and organic groups. It is like the Mailman Groups module, however, uses the mailman api module and cpanel_api2 module (currently sandboxed) in order to interact with mailman. Currently users are able to:

  • Create new mailing list and assign it to a group
  • Use an exisiting mailing list and assign it to a group.
  • Sync e-mail addresses from a mailing group to mailman
  • Move a group to a new site
  • Update mailman lists when user changes email
  • Remove email address from list(s) when user is removed from groups or deleted.

Through this, and the mailman api module, I plan to expand to allow users to choose functions such as digest, and give admins full control of mailman settings.

http://drupal.org/sandbox/mschulte/1202804
This is intended from drupal 6.x. Probably upgrading to 7.x in the future.

CommentFileSizeAuthor
#7 og_mailman-1203430-7.patch38.16 KBtim.plunkett

Comments

alexreinhart’s picture

alexreinhart commented
Status: Active » Needs work

I've taken a cursory look through and not found any major issues. You may want to run it through the Coder module to do an automated check for formatting and such:

http://drupal.org/project/coder

Question, though: if your module is like the Mailman Groups module, what does it do differently? Does it provide the same functionality through different APIs, or does it offer significant advantages because of its cpanel integration?

Just a few comments:

  • On line 41 of og_mailman.module, you mean "e.g.", not "i.e.". "i.e." is roughly "in other words," whereas "e.g." is "for example." (Yes, I'm slightly pedantic.)
  • On line 721, and in several other places where you use watchdog(), you call t() on the log message you pass in. You shouldn't, since the logging system does the replacements with the variables you provide to watchdog() automatically. (The string has to be translated into the admin's language, rather than the current user's, so it's translated later.)
  • You should try to avoid having escaped single quotes inside t() strings, since they make them odd to translate -- use double quotes around the entire string if possible.
  • The drupal_set_message docs suggest all messages should end with a period. Yes, I'm being pedantic again.
  • In og_mailman.install, you use get_t() but don't use the resulting $t correctly. In og_mailman_requirements, for example, you should call $t('string') rather than t('string') if you want to use the correct t function. The array you return should also be keyed with a requirement name -- i.e. you should return array('og_mailman' => array('title' => ...) ...).
  • In og_mailman_uninstall(), why do you delete all your variables, then delete three more which should have already been deleted?
  • Is cpanel_api2 by another developer while sandboxed? If so, why is it included in your module?
  • Your README.txt should wrap at 80 characters, not 100. See the module documentation guidelines.
  • You might consider having og_mailman_nodeapi() broken up into helper functions -- it's just incredibly long and tough to follow right now. Split up the work to specific, well-defined functions if possible.

Otherwise, you seem to be escaping your database queries and using t() for data display properly, so things are looking good. Please set the status of this issue back to "needs review" once you've fixed the above issues.

schultetwin’s picture

schultetwin commented
Status: Needs work » Needs review

I've run the module through coder and fixed all the issues there.

I also fixed everything you mentioned above.

My module is significantly different from the mailman groups module in that, via the cpanel_api, you have the ability to add and remove mailing lists, which you don't have in the mailman groups module. I also use functions from the cpanel_api to check to make sure that e-mail lists are in appropriate domains, check to see if a mailing list exists or not, and check if an e-mail address exists as an e-mail address or e-mail forwarder.

Cpanel_api2 is created by another developer and is currently sandboxed. I included it in my module because the 6.x-1.x branch is still called cpanel_whm_api, where the 7.x module is called cpanel_api2. This means all the functions have different names (cpanel_whm_api_... or cpanel_api2_...). I believe that he will update this eventually to cpanel_api2, so I rather do this. Also, I don't think I can include a dependencies for a sandboxed modules. I fully plan to remove that "sub-module" from my module and just set cpanel_api2 as a dependency when cpanel_api2 becomes a full module.

Thanks for the advice,
Mark

alexreinhart’s picture

alexreinhart commented

Hm. Are these features which could be added to the Mailman Groups module, rather than starting your own project? Generally it's encouraged to add to an existing module rather than starting your own, but if you have a compelling reason (original developers didn't want the feature, technical problems, whatever) it should be fine.

I'd suggest adjusting your sandbox page to explain the differences, so users choosing modules will know how your module is different.

schultetwin’s picture

schultetwin commented

The main difference is Mailman Groups is based off the Mailman Manager/User Mailman Register. Both these modules store all list information locally (who's subscribed, what lists exists, ...) My goal here is to keep all but the e-mail list name and password stored locally. Everything else should be stored only once, and that is through mailman. I suppose I could try adding new features to mailman_manager, and user_mailman_register (which are both minimally maintained), but I would basically have to created a new branch to allow me to use those featuers.

I'll add this explanation to my sandbox page.

alexreinhart’s picture

alexreinhart commented
Status: Needs review » Reviewed & tested by the community

Okay, thanks. I'm looking through at the moment for any other issues. One note about og_mailman.install and og_mailman.views.inc: the Coder module lies about you needing a // $Id$ line, since that hasn't been needed since drupal.org switched from CVS to Git.

I'm slightly concerned by your use of dynamically generated variable names for variable_get() (like 'og_mailman_' . $form['type']['#value']) but so long as you test that it works correctly with odd values that should be fine.

There's also a typo ("Vallues") in og_mailman_menu().

In the switch on line 622, should case -1 have a break; after it, or is it meant to fall through to the next option?

I don't think there are any other obvious issues, and I trust you'll fix the above issues soon enough, so I'll set this to Reviewed & Tested. You seem to understand the Drupal API and secure coding practices.

schultetwin’s picture

schultetwin commented

Alright, I've updated every suggestion you made. Thank you for your help.

As for the $form['type']['#value'] variable, I believe that should always works, and the node type should always be a clean value.

Thanks,
Mark

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new og_mailman-1203430-7.patch38.16 KB

There are quite a few typos throughout, and the documentation needs work (there are a lot of @todo's)
Here's a patch to get you started.

schultetwin’s picture

schultetwin commented

Thanks for looking over the module. I'll get back to it next week to make the updates. I'll update all the cpanel_api2 functions to the new name cpanel_sdk, which is what danrasband has changed his project name too. And a big thanks for the patch.

All the at todo's are included in another module (cpanel_api2 which is now known as cpanel_sdk). That module is still sandboxed as well. That and the fact that at the time his module was changing names was the reason for me to include his code in my module. I'll remove it as soon as cpanel_sdk goes to full module status.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Just link to http://drupal.org/sandbox/danrasband/1167370 and remove it from your repo.

adixon’s picture

adixon commented

Can you confirm - if you're using cpanel_sdk, which requires a server with cpanel or whm, it suggests that your module will only work for a mailman installation on cpanel, i.e. not a vanilla mailman install. If that's true, it's a big dependency that should be spelled out, and if not, then clarity would be good.

schultetwin’s picture

schultetwin commented
Status: Needs work » Postponed

Sorry, I haven't had much time since school has started again. But yes, cpanel or whm is a dependency. I think I'm instead going to use the REST API in mailman 3, but don't have much time now.

MickC’s picture

MickC commented

This module is a great idea and I'm keen to try it - keep getting stuck at the node form with the error message "You must configure OG Mailman first!!" however I've configure it 3 times - what am I missing?

schultetwin’s picture

schultetwin commented

Thanks for the enthusiasm. I hope to get back to it after the school year ends. Just as an FYI, for next time, you should ask questions about a module on the module page by creating a new issue. (http://drupal.org/node/add/project-issue/1202804).

Anyways, there are two reasons that you are seeing that message:
1) You have not set a default password for mailing lists (this is probably a bug that needs to be fixed).
2) You have not set the base url for your mailman lists.

If you have more questions, I'd be happy to answer them, just open an issue with the link above.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Postponed » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.