Sendy (http://sendy.co/) sends newsletters via Amazon SES for a fraction of the cost of MailChimp or Campaign Monitor. I developed this module because I wanted a simple way to capture email addresses of website visitors that weren't registered, and I wanted an easier (automated) way to maintain an email marketing list of registered users.

Module Features:

  • Adds a block to capture emails of anonymous users.
  • Adds a field to the user profile to allow registered users to opt in or out of a registered users email list.

Sendy module project page: https://www.drupal.org/sandbox/guinness74/2396067

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/guinness74/2396067.git sendy
cd sendy

Comments

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/httpgitdrupalorgsandboxguinness742396067git

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.

guinness74’s picture

I addressed all the issues found by PA robot. (Very helpful tool as my local coder module missed some things.)

guinness74’s picture

Status: Needs work » Needs review

I addressed all the issues found by PA robot.

JulienF’s picture

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

Here is my review,

Individual user account
Yes: Follows

No duplication
No: Causes, check this sandbox project https://www.drupal.org/sandbox/tommyent/1846018

Master Branch
Yes: Follows

Licensing
Yes: Follows

3rd party assets/code
Yes: Follows

README.txt/README.md
No: Does not follow, check the Readme template https://www.drupal.org/node/2181737

Code long/complex enough for review
Yes: Follows

Secure code
No: some input are not being filtered out before being output (i.e sendy url etc..) see check_url

Coding style & Drupal API usage
List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance:
Just a recommendation: In your watchdog, use the variables array to output dynamic values instead of concatenation in the message string.

guinness74’s picture

JulienF,

I appreciate you taking the time to look over my module.

I updated the README.txt to better fit the Drupal template.

I'm not a Drupal internals expert, but where should I apply check_url? My callback for hook_menu() [sendy_form() line 255 of sendy.module] uses variable_get() for the default form values, but I guessed those would be escaped by the Drupal form processor. Is that an incorrect assumption? I also used variable_get() as part of my API request with drupal_http_request(). Does the $url parameter need to go through check_url(), what about a value in the $options array parameter?

On line 54 of sendy.module I grab a variable for display with variable_get('sendy_block_blurb', ''). That seems like it should be HTML escaped. Is check_url() the correct function there, or does Drupal 7 have something similar to htmlentities?

As for the sandbox project (https://www.drupal.org/sandbox/tommyent/1846018) you mentioned. My module exceeds his scope and functionality and that module hasn't been maintained since 2012 / and may not be completed. My module is working on a live website right now. I reached out to the owner of the project and he wants a copy of my module once it's done. :)

I appreciate all of your help!

guinness74’s picture

Status: Needs work » Needs review
rzan’s picture

Greetings guinness74,

It's been a pleasure to review your module. Please see below:

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
It seems like you've addressed the duplication issue.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.

You spelt "appears" and "capture" incorrectly though. You may want to glance over it again for typos.

Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
There are some issues previously identified by JulienF. Perhaps I can answer some of your questions.

You are storing a URL from the user in a drupal variable and making direct use of it. You need to use check_url wherever you make use of this variable as it is considered unsafe.

The same thing applies to your sendy_block_blurb. Normally drupal handles all sanitation when you output through the field API but since you are rendering HTML yourself, the burden falls on you. In this case, you should apply a check_plain() on line 57 of your sendy.module.

Coding style & Drupal API usage
1. $success variable is unused as described by the pareview automated review.

sendy.module lines 122, 353, 395

2. You should place $email, $list and $export within the variables array parameter with token replacements instead like you would do when passing using the t() method.

sendy.module lines 174, 223

3. You should place the $row['sid'] within the variables array parameter with a token replacement instead like you would do when passing using the t() method.

sendy.module line 348

guinness74’s picture

rzan, thank you for your thoughtful explanation. I updated my code based on your suggestions. I didn't include check_url() on what's now line 273 of sendy.module because of the following warning from the automated review tools.

259 | WARNING | Do not use the check_url() sanitization function on Form API
| | #default_value elements, they get escaped automatically

Everything should be good to go. Thanks again!

guinness74’s picture

rzan, thank you for your thoughtful explanation. I updated my code based on your suggestions. I didn't include check_url() on what's now line 273 of sendy.module because of the following warning from the automated review tools.

259 | WARNING | Do not use the check_url() sanitization function on Form API
| | #default_value elements, they get escaped automatically

Everything should be good to go. Thanks again!

rzan’s picture

Status: Needs review » Reviewed & tested by the community

Hi guiness74. I took another look at your module and you have corrected everything I pointed out, nice work. I'm going to up this to RTBC as I found no more issues.

guinness74’s picture

Hi rzan, thank you for your time and effort.

Now that it's marked as RTBC, what happens next?

rzan’s picture

I have been going through the review process as well and reviewed your module as part of the review bonus program. From what I have learned, and please forgive me if I get anything wrong, now that your project has been marked as RTBC your project will eventually be manually reviewed by someone with the power to grant you the ability to convert Sendy into a full project.

This might take quite some time. If you do the review bonus program, then your wait time will likely be greatly reduced. This is what happened in my case.

Best of luck!

avpaderno’s picture

Assigned: Unassigned » avpaderno
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution!

I updated your account so you can promote 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!

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.

Thank you to the dedicated reviewer(s) as well.

Status: Fixed » Closed (fixed)

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