[D7] Email Blacklist

vishal_yug, there are few problem with the project application & module.

1. info file contains module version, this is discourage as drupal.org package manager automatically does this.
2. You need to follow the checklist for application submission. http://pareview.sh/pachecklist
3. Application with Review Bonus takes highest priority. You can review other project application & provide those links in your description. Please review minimum of 3 application.
4. PAReview is reporting issue on the code. Please see http://pareview.sh/pareview/httpgitdrupalorgsandboxvishalyug2204461git
5. Module have minor functional issue. eg. when i tried to enter 4 email id's in configuration, it only stored 2.

once you are done with all the fixes change the status to Needs Review

Hi vishal_yug,

I removed the PAReview: review bonus tag. You need 3 reviews to use this tag.
Also remove your username from the git clone link.

Manual review

1. If the module is about blocking registered email addresses, I have my doubts on the implementation of the idea. If this is the case, wouldn't it be more interesting to flag email addresses of users on user object level instead of dumping the email ids in a separate db table? There are quite some direct database calls for the functionality of this module.
2. email_blacklist.admin.inc line 10: You can remove the parameter $form.

1- you are not validating email address while submitting value in admin form.
2- project is too short to approve you as full module creator. May be git admin approved this only project for you.
3- Do 3 manual review and add review tag.

I am not sure which IDE you are using, but I cheked it usin NetBeans, and found your code formating issue with function email_blacklist_mail_alter, here

$count = db_select('email_blacklist', 'k')
        ->condition('k.mail', $mail)
        ->countQuery()
        ->execute()
        ->fetchField();

are not properly formated.

Fix the code review issues: http://pareview.sh/pareview/httpgitdrupalorgsandboxvishalyug2204461git

Rather than creating a new table for storing blacklist email addresses, why do not make use of variables table. Create a system setting form (https://api.drupal.org/api/drupal/modules!system!system.module/function/system_settings_form), this way the setting will be stored in a variable. You can then easily check settings through variable_get(). This way you can make more use of Drupal API.

It would be good to keep all .inc files in a separate directory calles includes. Keep admin.inc file inside includes directory.

Cleaned up the code for the admin form validate function. Attaching a patch for the same. Please do refer the patch, test it and may be use it too :)

Code changes and reasons for the same -
* added required to the email form field
* instead of replacing new line with comma and splitting on comma, split directly on new line or whitespace
* if incorrect email found, break loop
* no need of changing form field value in $form_state variable

More feedback coming up after some more review :)

Feedback

1. no need to store in a new table, instead store in the variables table - as visibility rules are stores
2. could store as a comma or new line separated list but in a single variable
3. have custom validate function for checking email ids, before saving to db
4. another advantage, single write to db on save.
5. only check 'to' email ids, must also check cc and bcc email ids.
6. to could have mutiple email addresses
7. Please run the drupal coder module. Following errors turned up
   • email_blacklist/email_blacklist.admin.inc
   • --------------------------------------------------------------------------------
   • FOUND 12 ERROR(S) AFFECTING 7 LINE(S)
   • --------------------------------------------------------------------------------
   • 45 | ERROR | Whitespace found at end of line
   • 46 | ERROR | Spaces must be used to indent lines; tabs are not allowed
   • 46 | ERROR | Line indented incorrectly; expected 4 spaces, found 1
   • 47 | ERROR | Spaces must be used to indent lines; tabs are not allowed
   • 47 | ERROR | Line indented incorrectly; expected 6 spaces, found 2
   • 47 | ERROR | No space found after comma in function call
   • 48 | ERROR | Spaces must be used to indent lines; tabs are not allowed
   • 49 | ERROR | Spaces must be used to indent lines; tabs are not allowed
   • 49 | ERROR | Whitespace found at end of line
   • 50 | ERROR | Spaces must be used to indent lines; tabs are not allowed
   • 50 | ERROR | Line indented incorrectly; expected 4 spaces, found 1
   • 52 | ERROR | Closing brace indented incorrectly; expected 1 spaces, found 4
8. Add inline comments to hook_mail_alter for the implementation.

Updated previous. Please ignore this.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.