This module provides a newsletter subscription block for Bring. It requires a Bring API key, and can be used to subscribe/unsubcribe users. Work with Drupal 7.

http://drupal.org/sandbox/luiscastro/1729786

luiscastro@git.drupal.org:sandbox/luiscastro/1729786.git

Comments

Anks’s picture

Status: Needs review » Needs work

Hi luiscastro

I have clone your module but i get only a .info file Which is also not in proper format. You can follow http://drupal.org/node/542202

patrickd’s picture

there should be a readme, but this is no reason to set needs work..

patrickd’s picture

okay, this is a mayer issue.

@Anks please keep your comment edits for changelog purposes. If you remove something, rather strike it through

It seems that your project´s repository contains no code yet, please have a look at the documentation about using GIT on drupal.org.

luiscastro’s picture

Status: Needs work » Needs review

I added module to repo. Please review.

patrickd’s picture

There are about 100 other applications waiting for review and only a hand full of active reviewers.
We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

Anks’s picture

Status: Needs review » Needs work

Hi luiscastro
There is a Potential problem - drupal_set_message() only accepts filtered text.
You can not use 'Tests Bad #!code !error', - line no 208 .module file
Be sure all !placeholders for $variables in t() are fully sanitized using check_plain().

README.txt is still missing.

patrickd’s picture

Status: Needs work » Needs review

just pasting in automated review output isn't a really helpful review

did you even look into the code to figure out whether this is a false positive ??

luiscastro’s picture

I don't see potential problems with filtered text and sanitized strings.

README.txt added to module. When possible please review.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Your README.txt goes over 80 characters but other than that everything looks good so setting to RTBC

benjy’s picture

Adding hook_help() would also be a nice touch. :)

cubeinspire’s picture

Status: Reviewed & tested by the community » Needs work

Small issues

1. there is a drupal_set_message('Tests ok'); .
This is of no use to the end user. Please set a more explicit message or remove it.

2. The module creates variables but is not deleting them.
This module should have a bring_newsletter.install file with an hook_uninstall(); function.
See variable_del() reference.

3. Not sure if this is really an issue but having the block edition form on the module configuration page is not the standard way of using the interface. It seems a bit confusing to me.

4. You are still working on the master branch.
This project should be on a version branch and the master should be deleted.

5. The description of the module could be improved:
This module provides a newsletter subscription block for Bring. It requires a Bring API key, and can be used to subscribe/unsubcribe users.

6. On line 193 of the module file the end of the $url variable is : "contact/test_bring_drupal"
I guess this is some hard code you used to test the module ? If that is correct it should be removed.

7. As pointed before there are more than 80 characters on the README.txt.

I don't see any security issue but there are many small issues so I guess this still needs a bit of work.

klausi’s picture

Status: Needs work » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)