Node Note

Took a look, very clean code. Some suggestions.

1. In the template files consider removing the class and id attributes, and allowing these to be set in the template_preprocess_hook() functions. This will make your module more easily extended by others. You can use the default attributes_array, content_attributes_array and classes_array keys provided in $variables.

2. Also for created date, would be better to do the formatting in preprocess, so that the logic is removed from the template.

3. Perhaps a bit longer description in .info would be helpful

4. In function node_notes_confirm_delete you have a dependency on Comment module, buy referring to comment_confirm_delete, but you do not have the dependency in your .info file.

5. function template_preprocess_node_notes() has no content.

6. Some files are missing @file doc and some functions are missing doc block.

The applicant has been contacted to ask if the application is abandoned.

You could anytime submit new modules as sandbox projects, but to get full projects up on drupal.org there are a some rules, like:

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

Here are some tips: http://drupal.org/node/1187664

If you have any questions, feel free to ask.

Hi!
I found some errors in your sandbox project:

• Primary, yo must work in other branch instead of master. This branch must cointain only README.txt file. See http://drupal.org/node/1127732.
• You must describe module's functionality in README.txt, not only installation instructions.
• Missing file doc comment in node_notes.module
• All functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming (node_notes.module - line 6)
• Remove TODO tags.
• Hook node_type_delete() won't work. It must be prefixed with your module name. (node_notes.module - line 105)
• Missing function documentation for node_notes_form(). (node_notes.module - line 109)
• Submit button value must pass through t() function. (node_notes.module - line 125)
• Return statement not necessary in form submit callback. (node_notes.module - lines 147, 177, 196)
• Missing function documentation for node_notes_view(). (node_notes.pages.inc - line 7)

You must see Doxygen and comment formatting conventions (http://drupal.org/node/1354)

Hello, I am a fellow applicant doing a review of your project application.

Automated Review

http://ventral.org/pareview/httpgitdrupalorgsandboxtonylegrone1281152git

Branch name should be 7.x-1.x instead of 7.x-1.x-dev

Manual Review

With a fresh Drupal 7 install on localhost I was able to setup 'node notes' for the Article content type. I used Devel generate to make a bunch of dummy articles. As user #1 I was able to add and delete notes from article nodes without problem. I then tried visiting node note paths directly as an anonymous user (without permission), and as an authenticated user (non admin) to make sure I wasn't able to access or delete node notes. The permissions worked as expected and the anonymous/authenticated user wasn't able to do anything.

I then granted authenticated users the ability to view node notes. I then logged in as a regular user (only the authenticated role) and was able to view node notes attached to article nodes. The 'Notes' local task menu tab showed up as expected and I was able to view the node note list. The delete button didn't show up, as expected.

Next, I granted authenticated users the ability to create node notes and revisited the articles as an authenticated user. Creating node notes worked as exptected. The authenticated user was not able to directly access the node note delete confirmation page, as exptected. And finally granting the administer node notes permission to the administrator user role allowed a dummy admin user to create/delete node notes as expected.

Additionally, I made copies of the two tpl.php files and dropped them into the Bartik theme templates directory, flushed the theme registry and made html modifications to each. The template files worked as expected.

Suggested Improvement(s):
cache_clear_all() in node_notes_confirm_delete_submit. I myself wouldn't want all my caches flushed just when a simple node note was deleted. Can this cache clear be more direct? I.E. Just clear the cache on the node, node notes.

Suggested Feature(s):
I'd like to have the ability to choose asc/desc order for the notes listing.

Conclusion:
Nice job on this module, the code is nice and clean, and easy to follow. The module worked as expected in all cases. The local task menu tab on node pages and the vertical tab fieldset on the content type edit form fit perfectly, right where one would expect when working with a drupal site.

The following git branches do not match the release branch pattern, you should remove/rename them. See http://drupal.org/node/1015226

* 7.x-1.x-dev
  remotes/origin/7.x-1.x-dev

Review of the 7.x-1.x-dev branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

• 'node-note/%/delete': you should use a menu auto loading place holder here. Then the page is only available if the note could be loaded and you do not have to check that yourself. node_notes_load() could be used for that.
• node_notes_form_alter(): if you are specifically targeting one form you should use hook_form_FORM_ID_alter().
• node_notes_save(): you should use drupal_write_record() instead of db_insert() here.
• node_notes_save(): the access check should not be in the submit function. If the user does not have access the form should have never been displayed.
• node_notes_view(): why do you need the foreach() loop? It just copies the $result array?

Just out of curiosity, how this module is better/different than annotate module ?

Thanks

Hmm that makes sense. I didn't quite get time, to try it myself though, can only comment more if I try. Just wanted to get an overview.
Well, except that, I suppose you can work on permissions/visibility of node depending on user roles, integration with views, taxonomy etc. so data can be aggregated in various forms, if needed.

Thanks

Quite a nice little module this one. Works very well and was very easy to set up.

There are still quite a few code standards error/warnings (see: http://ventral.org/pareview/httpgitdrupalorgsandboxtonylegrone1281152git for more info).

Also, I would be tempted, purely from a neatness point of view, to put the .tpl files in a "theme" sub-directory of the module and the .inc files in an "includes" sub-directory. This is not critical though.

Hi Tony,

I understand that readability is good so completely see where you are coming from with the whitespace. I would however be tempted to clean it up.

Despite that I see no reason why this module should not go forward so have set the status to RTBC.

You have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

manual review:

1. "stylesheets[all][] = css/node_notes.css": why do you need your CSS on every page request? I thought your module is only related to nodes?
2. node_notes.info: the description should be on one line.
3. node_notes_permission(): permission titles must run through t() for translation.
4. node_notes_delete(): $num_deleted is never used.
5. node_notes_confirm_delete_page(): if you are just displaying a form on your page you can just directly use drupal_get_form as page callback in hook_menu() and remove that function.

But that issues are not critical blockers, so ...

Thanks for your contribution, tonylegrone! 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.