Allows users and modules builders to subscribe and unsubscribe users to/from a Listserv list. This is meant to be an integration for a www.lsoft.com implementation. To test this module you can use the following Penn State Listserv list:

drush vset listserv_email listserv@lists.psu.edu
drush vset listserv_default CURRENT-L

Project Page: https://www.drupal.org/sandbox/heymp/2321681

Git :

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/heyMP/2321681.git listserv
cd listserv

Automatic Review:
http://pareview.sh/pareview/httpgitdrupalorgsandboxheymp2321681git

Comments

heyMP’s picture

Issue summary: View changes
heyMP’s picture

Issue summary: View changes
heyMP’s picture

Issue summary: View changes
heyMP’s picture

Status: Active » Needs review
heyMP’s picture

Assigned: heyMP » Unassigned
btopro’s picture

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 ;)

PA robot’s picture

Status: Needs review » Needs work

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.

zhangyi5’s picture

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’s picture

Thank for your feedback zhangyi5. Unfortunately, I had pushed a critical bug in the code when you were testing it, which is why the Drush calls and form submits were failing, I apologize. If you wouldn't mind pull down the newest version and giving it a test I would really appreciate it.

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.

The listserv_email and listserv_default are actually being stored in the variables table which is why the don't have to be set up in hook_install.

heyMP’s picture

Status: Needs work » Needs review
er.pushpinderrana’s picture

Status: Needs review » Needs work
Issue tags: +PAReview: security

@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!

zhangyi5’s picture

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

heyMP’s picture

I cleaned up the formatting of the readme file as well as fixed the following issues per the review by er.pushpinderrana.
#2338087: Sanitize outgoing emails to Listserv
#2337999: Validate email addresses

zhangyi5, thank you for your observation about including the drush option to specify the listserv. Since the module only handles one listserv, it would not make sense have that option in the Drush call. I do think, however, that it would make sense for the module to handle more than one Listserv. Once that functionality is included, I will add the option back to the drush call. Thanks!

heyMP’s picture

Status: Needs work » Needs review
naveenvalecha’s picture

Issue summary: View changes

Updated the issue body.

heyMP’s picture

Thanks naveenvalecha. I fixed the two outstanding errors that were in pareview.sh, and tested the module locally. Everything should be good to go.

naveenvalecha’s picture

@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

heyMP’s picture

naveenvalecha, I've outlined some differences between other comparable modules on the project page. Thanks!

naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community

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 :-)

kscheirer’s picture

Status: Reviewed & tested by the community » Fixed

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:

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.

Status: Fixed » Closed (fixed)

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