CVS edit link for BrockBoland

It's difficult for me to explain my motivation in applying for a CVS account without the fan-boy gushing about the community that the guidelines warned against. I am thankful for the work of others that has allowed me to make a living with Drupal, and want to add to that community - I'll try to leave it at that.

The short version is that I have developed several modules to enhance the existing Simplenews and SMTP modules. I am going to continue supporting and enhancing these modules, along with others, and want to share what will surely be of use to at least a few others.

The longer version is that I have been developing for Drupal for a little over a year now, and have spent a couple months recently doing a lot of work with Simplenews for a client. The client, a member of the US Congress, sends thousands of emails daily to people on various mailing lists, and needed a system that could take advantage of their multiple SMTP servers.

The module I am submitted for review is Multi SMTP. It builds on top of the existing SMTP module (http://drupal.org/project/smtp), which allows the admin to configure a single server. Multi SMTP adds an interface to manage serveral servers, and will choose a server at random for each page request (if a server ID is not explicitly defined in the GET parameters), so that the load of mail sending is spread across the servers. The existing SMTP module takes care of connecting to the SMTP servers and sending the mail; Multi SMTP just adds extra functionality on top of it.

In addition to this module, I have built a few others to enhance Simplenews. These require a little clean-up and improvement, but I intend to contribute them once they are ready:

* Simplenews Threaded Send: sends Simlenews messages from multiple threads at a time, one for each SMTP server (https://github.com/brockboland/simplenews_threaded_send)
* Simplenews Global Unsubscribe: exposes an option when a user edits their account to unsubscribe from all mailing lists, using the de-activate option that isn't otherwise available to the user (https://github.com/brockboland/simplenews_global_unsubscribe)
* Simplenews Register Subscriber: adds an Action that will create a user record when someone subscribes to a mailing list. For use with the "After a user has been subscribed" Trigger (https://github.com/brockboland/simplenews_register_subscriber).

I've also got plans to build other modules for more general use:

* Taxonomy term quick add. When creating a post in WordPress, the admin has the option to "Add New Category" in addition to choosing from the existing ones. I would like to build a module that adds similar functionality for non-tag vocabularies. I have not been able to find any existing modules that add this functionality.
* Node sub-pages. Adds a means for site builders to define sub-pages for different node types. In this way, creating a node will automatically make these sub-pages available (by creating menu items and path aliases for them). This module is about 75% complete. I have not been able to find any existing modules that add this functionality.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

BrockBoland’s picture

Component: Miscellaneous » new project application
Status: Postponed (maintainer needs more info) » Needs review
FileSize
77.36 KB

6.x version of the Multi SMTP module is attached. The full commit history can be seen on Github: https://github.com/brockboland/multi_smtp

apaderno’s picture

Status: Needs review » Needs work

Hello, and thank you for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

I will report here a little checklist:

  • The code needs to follow the coding standards; check in particular the format used for the control structures, and the name given to PHP variables, Drupal persistent variables, functions defined from the module.
  • Files available from third-party sites should not be included within the module/theme. This is particularly true for files that are not licensed under GPL License v2, but it is also true for files that are licensed under the same license used by Drupal.
  • The license file should not be included as well; the packaging script already include that file. In any cases, the code for modules/themes committed in drupal.org repository needs to be released under the same license used by Drupal; any compatible license is not allowed.
  • Check the code passes the Coder validation.

As per requirements, the motivation message should include more than two sentences (the exact words are a few paragraphs) that describe the project features. For themes, it should include also a screenshot of the theme (at least 640x400 pixels), and (when possible) a link to a working demo site; for modules, it should include also a comparison with the existing solutions.

apaderno’s picture

Issue tags: +Module review
BrockBoland’s picture

Status: Needs work » Needs review
FileSize
90.32 KB

Thanks for the comment, kiamlaluno. I think I met all those requirements in my initial submission, but a friend did look over the module and gave me a couple ideas to improve it, so there's an updated version attached.

Thanks for your time!

apaderno’s picture

The comparison with the existing solutions is still missing.

BrockBoland’s picture

The only module that I have found that's related is the SMTP Authentication Support module that this module works with. There are no other existing modules that allow the site admin to configure multiple SMTP servers.

Multi SMTP could also be compared to the modules that integrate with third-party mass mailing services (such as GovDelivery, MailChimp, and Elastic Email). While those services are fantastic for most cases, Multi SMTP is intended to address the situations like the one I found myself in:

* Multiple SMTP servers are available.
* The servers have previously been used for mailing for some time, so there was little concern over being black listed for spamming.
* Most of the mailing list subscribers were internal. In my case, most of the subscribers were other government employees, so using their internal SMTP servers meant that the email traffic never left the network. For this reason, they wanted to make use of their own servers rather than investing in and relying upon an outside service. This would also be ideal for companies that host their own mail and web servers and need to send mail internally: there's no need for the traffic to be going out and back into their network.

So while the Multi SMTP module could be compared primarily to those third-party mailing service integration modules, it's intended use is for more particular instances.

apaderno’s picture

Thank you for your reply.

rfay’s picture

Subscribing - hope to review

rfay’s picture

Status: Needs review » Needs work

1. (nit) I suspect that the README should not be named README.markdown

2. This seems kind of silly:

  $items['admin/settings/smtp']['page callback'] = 'drupal_goto';
  $items['admin/settings/smtp']['page arguments'] = array('admin/settings/multi_smtp/settings');

Shouldn't it just have the page callback be 'admin/settings/multi_smtp/settings'?

3. Please document function arguments in appropriate doxygen syntax

function multi_smtp_mark_server_to_wait($server_id = NULL) {

4. You can't use t() in the .install file, see https://drupal.org/node/322731

      drupal_set_message(t('The existing SMTP server has been imported to Multi SMTP as the default server.'));

5. Please document the openssl option in the README.

6. I don't think you should be doing a check_plain() on a provided password. That may corrupt it. check_plain() is for use when you're going to output something to the screen. And I doubt that you do that.

  if (check_plain($form_state['values']['smtp_password'])) {
    $record->smtp_password = check_plain($form_state['values']['smtp_password']);
  }

Overall, this looks like a great addition to the Drupal contrib repository, and it's fantastic that it works with smtp instead of reimplementing.

IMO this is ready to be RTBC, but I'll ask you to address these few things if they seem reasonable to you.

BrockBoland’s picture

Status: Needs work » Needs review
FileSize
102.09 KB

Thanks for the feedback, Randy! A new version is attached:

1. README renamed.

2. I wasn't sure of the best way to go about this. Basically, I need to prevent the user from making changes in the SMTP module's setting form, since the settings in Multi SMTP supercede it. For a while, I had simply removed this menu item completely, but it seemed hostile to the user to remove a menu item on them, so I put this redirect in place instead. I'm open to suggestions for a better user experience, but for the moment, left it as-is.

3. Added @param and @return comments to multiple functions, and added more detail to some others.

4. Removed t() calls in .install file.

5. Updated the README to include more information about the reliance on the SMTP module and how that module works, including OpenSSL support.

6. Removed check_plain() around the password field. Thanks for catching that: I got a little overzealous on the input sanitizing there.

rfay’s picture

Status: Needs review » Reviewed & tested by the community

OK, IMO this one is RTBC. Thanks so much for giving to the community.

Nits to be fixed in the future:
1. You do have to translate the stuff in the install file. Just with st() (as I remember). See the link provided in #9.
2. README.txt would be easier for most OSs to open

Thanks!
-Randy

BrockBoland’s picture

I'm not really sure where the process goes from here. Randy suggested a couple more little tweaks I could do, but marked it RTBC. What should I be doing at this point?

rfay’s picture

Ping? Any objections to doing this one?

apaderno’s picture

Status: Reviewed & tested by the community » Fixed

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

Status: Fixed » Closed (fixed)
Issue tags: -Module review

Automatically closed -- issue fixed for 2 weeks with no activity.

apaderno’s picture

Assigned: Unassigned » apaderno
Issue summary: View changes