Node logger

Hi Jhalak,

It would be a great help to people if you put some links in your application description to the actual project and supply the clone command so people can check out your code easily.

Yours is: git clone http://git.drupal.org/sandbox/jhalak/1938584.git node_logger

It's also helpful to check out the coding standards through pareview.sh. You can check your code style online: http://ventral.org/pareview/httpgitdrupalorgsandboxjhalak1938584git

Or install it on your server to use it as a drush command:
http://drupal.org/node/1419988

Also I wanted to ask if you know that Drupal has core support for revisions. This actually pretty much does what your module is doing. It allows for content types to keep revisions and revert back to another revision if needed. It's also very powerfull in combination with other modules as diff, workbench, workbench_moderation, etc...

But it's also possible you meant this module to be lightweight and provide minimal history. But most of the time clients will always ask for more. Then revisions would be the way to go.

Hi jhalak,

Add more details about your project here:

1) Link Project page
2) Git Repo
3) Version

Its very helpful for review your module.

Nah not at all, if it's sole purpose is to keep a simple log I'm shure plenty of other people will like your lightweight solution. Because enabling revisions would mean a lot more tables in your database that you aren't really using. So by all means it's "full project" worthy.

The code now looks fine to me. I just have a small functional remark. On the "action" table header you only have two possible outcomes "created" and "updated". Since you are allready providing the created on information above the table it seems like obsolete information. All you will see is "updated" every time. My suggestion drop the action column and don't store the creation date since the node keeps this data.

One thing I've noticed in the naming conventions is the hook_menu.

/**
 * Implements hook_menu().
 */
function node_logger_menu() {
  $items['node/%/user_log'] = array(
    'title'            => 'Log',
    'page callback'    => 'node_logger_user_log',
    'page arguments'   => array(1),
    'access arguments' => array(PERM_ACCESS_NODE_USER_LOG),
    'weight'           => 5,
    'type'             => MENU_LOCAL_TASK,
    'file'             => 'node_logger.pages.inc',
  );
  return $items;
}

I would try to keep a general naming convention. I would change the part 'user_log' with 'node_log'. Because it's still a log on the node itself, like you did in node_logger.install.

Also the path should be as simple as possible. If you can't find any modules that are using 'node/%/log'. I would be the first to cut that cake :). Otherwise 'node/%/node_log' is just fine.

When you've done this I would get another person to take a look at it. Because I don't have much more to say. It's a pretty small module.

Project URL, git URL and Version added

Any time you make your changes you should change the status to "needs review". That way other people will also take a look at your module.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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.

Description Updated

Manually checked and installed your module and looks very clean and usefull .. The only remarks i could possibly make are:

- 'Updated By', 'Last modified On' capitalisation ..
- You use mm/dd/yyyy as a date format, where the default seems to be yyyy-mm-dd .. make this an configurable option or change it to the default.

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

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

Hey,

Found one thing in the automated review: http://ventral.org/pareview/httpgitdrupalorgsandboxjhalak1938584git

All looks good from the manual review. The only point that I would mention is that in node_logger_log_record() $type is never passed to it so it will always be 'updated'. The $type var doesn't appear to be used anywhere else either.

With this is mind I think you could drop the 'type' from node_logger_schema() and the above function (feel free to correct me if it is used elsewhere though :) )

Paul

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.

First review of other project added