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
Comment #1
PA robot commentedThere 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.
Comment #2
guinness74 commentedI addressed all the issues found by PA robot. (Very helpful tool as my local coder module missed some things.)
Comment #3
guinness74 commentedI addressed all the issues found by PA robot.
Comment #4
JulienF commentedHere 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.
Comment #5
guinness74 commentedJulienF,
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!
Comment #6
guinness74 commentedComment #7
rzan commentedGreetings 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
Comment #8
guinness74 commentedrzan, 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.
Everything should be good to go. Thanks again!
Comment #9
guinness74 commentedrzan, 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.
Everything should be good to go. Thanks again!
Comment #10
rzan commentedHi 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.
Comment #11
guinness74 commentedHi rzan, thank you for your time and effort.
Now that it's marked as RTBC, what happens next?
Comment #12
rzan commentedI 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!
Comment #13
avpadernoThanks 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.