[D7] Content reminder

welcome,

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page. Also create a README.txt that follows the guidelines for in-project documentation.

• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• Remove "project" from the info file, it will be added by drupal.org packaging automatically.
• All functions should be prefixed with your module/theme name to avoid name clashes!
• Some of your code documentation is inconsistent (eg. function documentation blocks) please have a look at Coding standards and doxygen formatting

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxfluffy1605508git

There are already some good modules providing frameworks for such functionality (eg. notifications, messaging...), maintenance would be much easier if you'd integrate with them. (then you don't have to write the whole subscription management and email sending thing, you just have to provide a new 'event').

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

Maybe you can add some document, like summary, system requirements, how to install, how to configure, customization. For example sun's README.txt style.

And still there some errors when i do automated project review. http://www.ventral.org/pareview/httpgitdrupalorgsandboxfluffy1605508git

Hi,

I am not able to checkout the project. Can you check it.

I can confirm that the module works as described.

Hi,

There are still some coding standards issues in the module, it would be better in case you can resolve that first.

In the file content_reminder.info:

1. Full stop at the end of the description.
2. It would be better in case you start with name followed by description which is normally done for most of the modules

In the content_reminder.module
Most of lines in the code are exceeding margin line of 80 characters: http://drupal.org/coding-standards#linelength.
Set your editor to show the magic print margin line of 80 characters. Keeping these short assists code readability among other things.
Will do a detailed review of the functionality and get back to you. It would be cool that for the time being you could resolve these coding standards issues.

don't switch to needs work on such minor issues

Hi,

Some manual review of the code.

function content_reminder_permission() {
  $perms = array(); // Not required.
  $perms['content reminder administration'] = array(
    'title' => t('Administer content reminder'),
  );
  return $perms;
}

Instead use

function content_reminder_permission() {
  $perms['content reminder administration'] = array(
    'title' => t('Administer content reminder'),
  );
  return $perms;
}

Such redundant variable declarations are there on these lines too: #22, #85, #285.

In the function function content_reminder_menu() on the line #21.

$items['admin/config/workflow/content_reminder'] = array( // use "-" instead of "_"
    'title' => 'Content reminder',
    'description' =>  'Content reminder conifguration', // end with a full stop
    'page callback' => 'drupal_get_form',
    'page arguments' => array('content_reminder_config_form'),
    'access arguments' => array('content reminder administration'),
    'type' => MENU_NORMAL_ITEM,
  );

It is best to avoid underscores in the menu items. So change to

$items['admin/config/workflow/content-reminder'] = array(

);

Line # 300

 ->condition('status', '1', '=')
 ->condition('status', '1')

Feel free to ignore this one if you think that you need it to follow the logic. db_select()->condition() defaults to IN if $value is an array, and = otherwise.
Similar occurrence is found on the Line #418

Many lines in the code are still exceeding 80 characters, would be great in case you could follow the standard there.
I got some MySQL error while I tried to run the module, will check it and get back to you.

What's the difference with these similar modules:
-----------------------------------------------------------------------
Node Reminder
The node reminder allow for you to recieve an email at a desinated time period of time.

Subscriptions
This module enables users to subscribe to be notified of changes to nodes or taxonomies, such as new comments in specific forums, or additions to some category of blog.

Some more manual review of the code.

All the administrative configurations should go in a different file. In your it would be content_reminder.admin.inc and the function content_reminder_config_form() should be placed here and corresponding changes have to be made in the implementation of hook_menu(). So line #24 to #32 would change to

$items['admin/config/workflow/content-reminder'] = array(
  'title' => 'Content reminder',
  'description' =>  'Content reminder conifguration',
  'page callback' => 'drupal_get_form',
  'page arguments' => array('content_reminder_config_form'),
  'access arguments' => array('content reminder administration'),
  'type' => MENU_NORMAL_ITEM,
  'file' => 'conten_reminder.admin.inc', // This line has been added.
  );

Line #10 to #16: Implementation of hook_permission(), please include the description, as it makes easy to understand what exactly the permission is for.

In the implementation of hook_form line #50 to #79, include the validation function to validate the user input in the form. Please refer here for more details.

Could you make the following changes and the changes, mentioned in previous comments, as I just executed 'git pull' and none of changes suggested in the reviews were incorporated. Trying to understand the detailed functionality. Will get back soon.

Hi fluffy,

Please delete the variables created by your module through hook_uninstall(), everything looks fine :)

Rajan

Looks good, could you add a detailed description to "Presets" form element as right now it just says "Presets".
And you forgot to use t() function in the implementation of hook_permission(). Line #13.

/**
 * Implements hook_permission().
 */
function content_reminder_permission() {
  $perms['content reminder administration'] = array(
    'title' => t('Administer content reminder'),
    'description' => t('Administer content reminder conifguration.'),
  );
  return $perms;
}

I get this error
"FieldException: Attempt to create a field with a name longer than 32 characters: content_reminder_comment_node_page in field_create_field() (line 74 of /home/ashish/simpletest/modules/field/field.crud.inc)."
when I configure the settings of the module.

Find the attached screen-shot which might give you an idea that things that are causing problems(They have been marked in red).

Hi,

I am not able to understand the use of menu item .module file.

 // Creates autocomplete for locations.
  $items['user/autocomplete/content-reminder'] = array(
    'title' => 'User selection',
    'page callback' => 'content_reminder_user_autocomplete',
    'access arguments' => array('access content'),
    'type' => MENU_CALLBACK,
  );

Could you tell us what you are trying to do here.
As when I navigate to suppose: http://example.com/user/autocomplete/content-reminder, I get is a white screen with [] as the only content.

Thanks,
Ashish.

read about autocomplete here

no reason to set needs work.

Comment #18 and #17 are still unresolved which break the site when wrong value is selected. I think these have to be worked. @patrickd: please change the status accordingly so that the project application creator can resolve the issue, which I think should be "needs work".

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

Manual Review

Point 1:

Your autocomplete menu callback is defined to use access arguments as 'accss content' where you are using the menu to retrieve a list of users. See relevant code below.

$items['user/autocomplete/content-reminder'] = array(
    'title' => 'User selection',
    'page callback' => 'content_reminder_user_autocomplete',
    'access arguments' => array('access content'),
    'type' => MENU_CALLBACK,
  );

It is common for an anonymous user to have 'access content' privileges on a site. Thus it is possible to retrieve list of all users registered on the site by crafting url user/autocomplete/content-reminder/XXX. This might be an unintentional consequence from an admin's point of view. That so much as enabling the module exposes list of users to anonymous users.

Please provide a separate permission to access auto-complete callback. You may also want to add a note on about how it would be different from "View user profiles" permission.

Point 2:

Your module give User A power to email ( automated notify ) User B and no way for User B to opt out from receiving ( un subscribe ). This is not a good practice. For pointers look at how Drupal's personal contact form have a check-box on user page to enable it or not. Something similar should be implemented, and i would suggest your menu_callback function should respect these user preferences, and not show user in the list at all. Also you may have to re-check permission before the emails are send too.

PS: Personally I very much like your module, and am ready to help you. Just let me know how if I could be of any help.

I would like to keep it simpler and the addition of permission on who can set notifications also helps here, its not just any user, but user who has been assigned that privilege.

Sounds perfect.

fluffy: will get back..code looks good.

• An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.

  http://ventral.org/pareview/httpgitdrupalorgsandboxfluffy1605508git

• Your project page is not very detailed, please have a look at the tips for a great project page, you may also use HTML-tags for better structure.
• The function content_reminder_field_validate() does not do anything and should be removed.

I reviewed the code and used http://ventral.org "Project application checklist". Other that items noted above, the module looks ready for RTBC.

In content_reminder_field_schema(), does defining database-specific types like that actually work? I think you only need the base 'type' => 'datetime' but I'm not certain.

Using LANGUAGE_NONE will make some of your text untranslatable - I guess that's not a problem since you'd likely just need 1 language to alert your admins.

On the project page Each notofication can have it's own text. should be Each notification can have its own text.

None of those are major issues though.

Thanks for your contribution, fluffy!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

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.

Thanks to the dedicated reviewer(s) as well.

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