[D7] User mail blacklist

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxkala4ek1983496git

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.

Manual review:

Add descriptions to function parameters and return values

/**
 * Save blacklisted domain.
 */
function umb_save($umb_item) {

Use t() function with strings

 watchdog('User email blacklist', "Blacklist's domain :title was deleted", array(

Write full css rules
border-bottom: 1px dotted #000; instead of border-bottom: dotted;

Hi, Dmitryi.

First thing I saw.

The value of the constant may be only a scalar values.
umb.module, line 10

define('UMB_MODULE_PATH', drupal_get_path('module', 'umb'));

Regarding the module settings. It is much easier when the module configuration sections are tabbed.
Example from menu_example.module:

  ...

  // A menu entry with tabs.
  // For tabs we need at least 3 things:
  // 1. A parent MENU_NORMAL_ITEM menu item (examples/menu_example/tabs in this
  //    example.)
  // 2. A primary tab (the one that is active when we land on the base menu).
  //    This tab is of type MENU_DEFAULT_LOCAL_TASK.
  // 3. Some other menu entries for the other tabs, of type MENU_LOCAL_TASK.
  $items['examples/menu_example/tabs'] = array(
  
  ...

Please create a 7.x-1.x branch and use it as primary branch.

also use only the ready to use git clone with the default git branch
git clone http://git.drupal.org/sandbox/kala4ek/1983496.git umb

Wrong comment, ou use hook_form_alter(). :

/**
 * Implementation of hook_form_FORM_ID_alter().
 */
function umb_form_alter(&$form, &$form_state, $form_id) {
  switch ($form_id) {
    case 'user_register_form':
    case 'user_profile_form':
      $form['#validate'][] = 'umb_form_validate';
      break;
  }
}

Js : Just for best coding please use 'that' instead of '$this' and only use jquery as helper so your code shloud be more clear:

/**
 * @file JavaScript file for disdomain module.
 */

(function($) {

  Drupal.behaviors.umb = {
    attach : function(context, settings) {
      $('#.umb-list a.change').click(function(e) {
        e.preventDefault();
        var that = this;
        $.ajax({
          url: $(that).attr('href'),
          complete: function(data) {
            $(that).html(data.response);
          }
        });
      });
    }
  };
})(jQuery);

Hi kala4ek

After deleting domain from list its redirect to admin/config/umb/list. There is no any admin/config/umb/list page
I think this should be redirect to admin/umb/list.

Hi kala4ek.
You need to use coding standards

FILE: /var/www/drupal-7-pareview/pareview_temp/umb.module
--------------------------------------------------------------------------------
FOUND 10 ERROR(S) AND 4 WARNING(S) AFFECTING 14 LINE(S)
--------------------------------------------------------------------------------
13 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
72 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
82 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
104 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
261 | ERROR | Missing parameter type at position 1
274 | ERROR | The second argument to watchdog() should not be enclosed with
| | t()
287 | ERROR | The second argument to watchdog() should not be enclosed with
| | t()
296 | ERROR | Missing parameter type at position 1
336 | ERROR | Missing parameter type at position 1
343 | ERROR | The second argument to watchdog() should not be enclosed with
| | t()
351 | ERROR | Missing parameter type at position 1
370 | ERROR | Missing function doc comment
381 | ERROR | Function comment short description must be on a single line
384 | ERROR | Missing parameter type at position 1

Hey, cool module kala4ek. Thanks for your contribution.
Here's what I found on a manual review:
- README.txt: provide more detail and installation instructions.
- umb.module line 74: try to avoid hook_init() since that is called on every single page request. Since you're checking a path, why not use hook_menu()?
- umb.module line 152: typo in t().
- umb.css: incorrect module name in comment.

Some of those points are obviously very minor.

#use variable_del() delete the variable when uninstalling.
function umb_uninstall() {
variable_del('[var]');
}

#Configuration link is not right at the module page.

#notice when edit default domain
Notice: Trying to get property of non-object in umb_edit_form() (line 205 of D:\wamp\www\drupal-seven\sites\all\modules\umb\umb.module).
Notice: Trying to get property of non-object in umb_edit_form() (line 213 of D:\wamp\www\drupal-seven\sites\all\modules\umb\umb.module).
Notice: Trying to get property of non-object in umb_edit_form() (line 222 of D:\wamp\www\drupal-seven\sites\all\modules\umb\umb.module).
Notice: Trying to get property of non-object in umb_edit_form() (line 228 of D:\wamp\www\drupal-seven\sites\all\modules\umb\umb.module).

#at line 358 (umb.module): ->condition('domain', $domain, 'LIKE') should be ->condition('domain', end($domain), 'LIKE')

#don't find the links on the admin panel to add new blacklist item.

[manual review]

Module seems to work as expected. Didn't see any major coding standard issues either. Here are some thoughts to consider:

• Do you need the .gitignore file? Seems like a mistake?
• I'd like to see a bit more explanation in the ReadMe.txt.
• @File docs on includes/umb.css is incorrect.
• Is the CSS file totally necessary? I'd probably vote to let the themer decide how that looks. Of course they can override this, but having a CSS file for one selector seems like a stretch IMHO.
• Probably not a huge deal, but moving your page callbacks and related submit and validate handlers to a separate "admin" include file might be a good idea. Definitely not required, but might help with organization. See here: https://drupal.org/node/146172
• I usually prefer that this type of configuration live under "Configuration" instead of the top level. Might also fit under "People"
• I'd consider adding some description to the top of "admin/umb" to describe how the module works.

Nice work.

You have some errors reported at http://ventral.org/pareview/httpgitdrupalorgsandboxkala4ek1983496git. The major one is not using the correct branch name. Use 7.x-1.x instead of master. And ReadMe.txt should be renamed README.txt.

This sentence doesn't make sense: "This module allows you to filter email domains which are registered users and does not change for a banned for already registered." I think that should read "This module will prevent new users from registering with blacklisted domains in their email address. It has no effect on already registered users." instead.

In umb_install() you don't need to set default values for your variables, just use variable_get('variable name', 'default value') instead when you're retrieving the variable.

Don't use umb_init() to load javascript libraries - that will be added on every page load. Just load them when they are needed.

In umb_save() you can use db_merge() instead of manually doing an update or insert.

Module looks pretty good - setting to needs work for the branch name problem, otherwise this is RTBC.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Hi,

A few notes on your module.

(1) At line 144 of umb.module, you use the modifier ":", which does not exist. You should use @, % or !, according to https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/form....

(2) Newly added domains don't seem to be validated. If you enter "example.com " (with a space) or "example. com", the system does not seem to work, and provides no error message.

(3) If you try to add a domain which already exists, you get "Warning: array_map() [function.array-map]: Argument #2 should be an array in array_map() (line 1593 of /path/to/drupal/includes/form.inc)."; and the old record seems to be deleted. This poses two problems: First, the error message; and second, the fact that the old record is completely deleted without warning (what if I decided to add important information in the comments?). Might a better approach be to prevent creating a new record with the same domain before deleting the old one?

(4) Here is a concrete example of how #2 and #3 together can cause confusion and break the workflow:

4.1. admin/config/umb/add: Add the domain "test.com " (with a space)
4.2. on another browser, user/register: a@test.com allows the user to register
4.3. the administrator might be confused at this behavior, and return to admin/config/umb/add, and add "test.com" (without a space) -- This shows the error in #3, and seems to replace the record "test.com " with "test.com".
4.4. However, the data now seems corrupted, as on another browser, user/register: a@test.com *still* allows the user to register. (The system seems to remember that "test.com " used to have a space, and new records with test.com will not work unless deleted first).

Cheers,

Albert.

I'll look at this now in the Project applications sprint

https://drupal.org/project/useremaildomain

This sounds like a feature that should live in the existing useremaildomain project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the useremaildomain issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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

Change git branch