[D7] Listserv

Audit

Code is to drupal coding / coder standards with two exceptions that I have filed in the queue of the project:
#2322337: Nit picking patch
#2322339: Naming conventions

Description

In searching d.o. there is nothing that does listserv integration in the way this is handling it. This is for allowing users to subscribe and unsubscribe from listservs via drupal; that way the process can be integrated into drupal sites via two blocks in the site instead of sending the user off to a different (probably terrible CGI based) script to handle the registration to (or unsubscribe from) a list serv.

Notes

Project also has drush integration which can tie it into automation and generally showcases this being well thought out.

Future possibilities / integrations

These are not blockers, just maybe some ideas to drive other people's interest into seeing this pushed forward in review faster.

• Could be integrated with Organic groups to allow OG population via LDAP or some engine with knowledge of possible listservs so that users could subscribe / unsubscribe to the listserv right through the OG registration process
• could integrate with context to change the listserve registration block based on situational awareness in the site (when on page X, make listserv reg related to list Y)
• could integrate with field API / entity reference to allow for integration with other listservs throughout the site

Disclosure

I work with hey__mp and he's a member of PSU Drupal User's group. This speaks both to his demonstrated ability in this community and my bias in this review process ;)

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxheyMP2321681git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi Michael,

I am using listserv_email as listserv@lists.psu.edu to test your module. The results are:

1. The configuration works good, either using drush command or using the interface of the configuration link.
2. Both subscribe and unsubscribe operation returns blank page. It would be good if it could return some Pass/Fail confirmation info.
3. The following drush command returns fatal error:
$ drush lsvs some@example.com --operation=subscribe --listserv=L-DRUPAL --username="My name"

Fatal error: Call to undefined function listserv_subscription() in drupal/sites/all/modules/contrib/listserv/listserv.drush.inc on line 41
Drush command terminated abnormally due to an unrecoverable error. [error]
4. Where are the tables listserv_email and listserv_default defined? They are not in listserv.install? I did not see the tables from the mysql db also. But there is hook_uninstall() in the file.

Hopefully it would be helpful for your development!

Yi

@heyMP , thank you for your contribution.

I like your module idea and it works as intended.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Yes, http://pareview.sh/pareview/httpgitdrupalorgsandboxheymp2321681git reported some minor issues that need to be fix.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Not Sure: Does not cause module duplication and fragmentation. There are number of existing modules that are somehow similar to this but of course you can better differensiate about your module. I haven't look into listed module functionality, just found after googled might be these modules are completely different from your module :).

1. https://www.drupal.org/project/mailing_list
2. http://drupal.org/project/notifications
3. http://drupal.org/project/messaging
4. http://drupal.org/project/mailhandler
5. https://www.drupal.org/project/subscriptions
6. https://www.drupal.org/project/simplenews

It would be so helpful if you add some more information on your project page and show the differences how your module is different from others.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

(+) No: Follows the guidelines for in-project documentation and the README Template. The file should be formatted to hard-wrap at 80 characters. Also look into README Template again.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

1. (+) listserv_listserv_subscription: The user provided e-mail address should be validated with the
   PHP e-mail validation filter. Also this is vulnerable to XSS, user provided input should be sanitize before passing further, make sure to read https://www.drupal.org/node/28984 again.
2. (+): listserv_listserv_subscription: Use t() in switch case code to construct safe, translatable strings, make sure to read https://www.drupal.org/node/64279#t-placeholders again.

Coding style & Drupal API usage

1. (*) listserv_mail: $message['body'][] = $params['body']; body text is passing directly without sanitized either use drupal_html_to_text() at this level or sanitized $body text in listserv_listserv_subscription function as $listserv_name is appending to body text that is user provided input.
2. (+) listserv_admin_config_form: You should validate whether listserv_email hold valid email, currently there is no validation.
3. hook_help() is missing in your module.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks again for your hard work!

Michael,

I grabbed your new release and everything works well this time. Thanks for clarifying me about the listserv_email and listserv_default fields in the variable table!

One more suggestion is about the parameter "listserv", which is in the "drush lsvs" command.
1. No matter what value I input to listserv including the undefined value, it gives:
Successfully subscribed
2. Since there is only one configuration of the module, and there is only one listserv in the module, why the parameter still exists? It could confuse people. I can understand, if you want to expand the module to accept multiple listservs in the future. But it did confuse me for now. My two cents. :)

Cheers!
Yi

Updated the issue body.

@heyMP,
First Thanks for ontribution! Awesome Module!
Module duplication is a big issue on the drupal.org. The Drupal community follows the ethos collaboration rather than competition.Would you please specify the difference between the module functionality with respect to other modules on project page/issue summary page as @pushpinder specified in #12
It would be so helpful if you add some more information on your project page and show the differences how your module is different from others.

After specifying the differences that I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks! again
Naveen Valecha

Manual Review :

Does not find any security issue,No major api issue.Rest seems ok to me.Setting this to RTBC.

• listserv_listserv_subscription :
  

    // Verify that the email address is valid.
    if (!valid_email_address($email)) {
      drupal_set_message(t('You must provide a valid email address.'));
      return;
    } 

  This code is not needed because the email is already validated in listserv_subscribe_form_validate and listserv_unsubscribe_form_validate functions.

• listserv_listserv_subscription :

    // Determine the listserv contact email address.
    $listserv_email = variable_get('listserv_email', LISTSERV_EMAIL);
    if ($listserv_email == '') {
      drupal_set_message(t('You need to set a listserv email address in admin/config/services/listserv.'));
      return;
    }
  
    // Determine the listserv name.
    $listserv_name = variable_get('listserv_default', LISTSERV_DEFAULT);
    if ($listserv) {
      $listserv_name = $listserv;
    }
    elseif ($listserv_name == '') {
      drupal_set_message(t('You need to set a default listserv list in admin/config/services/listserv.'));
      return;
    }
  

  you should implement it in hook_requirements rather than managing it in the form here.

• listserv.admin.inc : function listserv_admin_config_form_validate :
  form_set_error('default_email', t('You must provide a valid email address.'));
  There is not default_email form element.you need to correct the element name that should.
  form_set_error('listserv_email', t('You must provide a valid email address.'));

I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Non-blocking issues:

• In the subscribe and unsubscribe forms you might want to provide a default email value if the user is authenticated
• The sub/unsub forms are almost the same, as are the validate and submit functions, you might be able to combine some of that code

No other issues found. Thanks for your contribution, heyMP!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.