Node Notify

welcome,

looks pretty good!

Some little things I've seen while flying over it

• Please translate all human readable strings with t() (missing in _permissions) - exception for this are hook_menu, hook_schema and watchdog()- do not use it there but everywhere else
• check_plain(url('admin/config/people/node_notify')), this makes no sense as 1) a returned url() will never contain html 2) the URL is static and it makes no sense to filter static strings. Only filter stuff where the value can possibly come from a user. (You probably did this because of the automated report - that one is a false positive so ignore it)
• Your variable names are static, it is not necessary to delete them by a query. but if you want to do it that way don't forget to flush the variable cache properly (have a look into variable_del())

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.

regards

hi
you module has some critical, minor error when I did test
http://ventral.org/pareview/httpgitdrupalorgsandboxdonatasp1513670git

Please fix those to improve more your quality.

@takim as mentioned in #1 the critical issue is a false positive

General Note: Many of the issues found are minor and should not be required for approval, therefore please do not insist on having them fixed and do not switch the issue to needs work if there are no major issues found. Automated reviews may point you to possible security issues - what does not mean they are really security issues - note that it´s a common case that automated reviews can have false positives.

Added review list.

manual review:

1. node_notify_uninstall(): instead of running a select query you could directly call a db_delete() query to remove your variables.
2. Your module file is quite large. You could split out page callbacks to dedicated *.pages.inc or *.admin.inc files.
3. node_notify_log(): I think the message should be sanitized before printing to watchdog(), no?

But that are just very minor issues, otherwise looks RTBC to me. Good work!

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

If your module is installed via drush the installation message is not very useful (no link in console). I'd suggest you to either don't output a message if it's installed by drush, or printing out the url directly (no link) (to determine whether it's installed through drush you can check whether PHP_SAPI == "cli").

Thanks for your contribution and welcome to the community of project contributors on drupal.org!! :)

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.