[D7] Editor Notes

Hello Rostislav, Nice Module. Thanks for the work.

I went through your module. It was nicely written.
No problems were found with the coding standards.

Just one usability issue.
Whenever a node(or any entity) is edited and editor changes( by changes i mean adding/editing/deleting) a note and do not save the node.
Since you are making the changes(add/edit/delete) in editor_note table prior to saving the node, the changes do not require
the node to be saved before taking effect. This seems to be an issue to me.
I think you should save the changes in the $form/$form_state and when the node is submitted, you can persist those changes in the editor_note.

Apart from that i think this module is very helpful. Great Job.

Hello, Rostislav.
First of all I want to say it is a pleasure to review such good executed project application.

Review of the 7.x-1.x branch (commit f15fcaf):

Automated Review

Coder Review has found a couple of normal and minor warnings:

1. editor_note.module: lines 108, 118, 416, 490, 625, 781; editor_note.pages.inc: line 54
   Use sentence case, not title case, for end-user strings. (Wikipedia)
2. editor_note.module: line 1159
   @see references should be separated by "," followed by a single space and with no trailing punctuation. (Drupal docs)
   // @see theme_pager_link().

Manual Review

Individual user account

OK: Follows the guidelines for individual user accounts.

No duplication

OK: Does not cause module duplication and fragmentation.

Master Branch

OK: Follows the guidelines for master branch.

Licensing

OK: Follows the licensing requirements

3rd party code

OK: Follows the guidelines for 3rd party code.

README.md

OK: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

OK: Follows the guidelines for project length and complexity.

Secure code

OK. No issues identified.

Coding style & Drupal API usage

1. editor_note.module:5
   File description is not corresponding to module's name.
2. editor_note.module:hook_help(), lines 409, 484 and maybe other places
   Avoid using line breaks in translatable strings. Let them live in one line each.
   Also you may shorten your module's help text. See Help text standard.
3. (+) editor_note.module:editor_note_entity_property_info()
   Not all descriptions are enclosed in t() function.
4. editor_note.module:552
   When implementing hook_form_FORM_ID_alter() specify which exact form you are altering. See docs.
5. editor_note.module:686
   Maybe it is better to use drupal_strlen() instead of mb_strlen() function.
6. editor_note.module:1248
   A copypasted description of another function.
   Also it should be more like "Element validation callback: Validates the width/height of..."
7. editor_note.js
   A behavior comment block and the behavior definition itself should start in the 1st column (no spaces at the line start).
   A first line of doc block should be separated from more descriptive documentation by 1 blank line (there should be only an asterisk).
   And each documentation line should not be longer than 80 characters (incl spaces).

The only blockers I see here are missed t() functions. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough and in the automated review are recommendations.

I also recommend you to reread the API documentation and comment standards especially on naming callbacks and form constructors.

Regards.

Release blocking issues were fixed with commit 9584c04. OK.

I've installed the module and found one PHP warning (it fires on the note modal opening):

Warning: Missing argument 3 for editor_note_confirm_edit_form() in editor_note_confirm_edit_form() (line 44 of W:\home\dru\7\sites\all\modules\test\editor_notes\editor_note.pages.inc).

You probably should set the default value for this argument to eliminate the warning:

function editor_note_confirm_edit_form($form, $form_state, $note_id = NULL) {}

But that's a minor warning, because it does not affect any functionality.

Also in my opinion it would be better to make an ajax paging between notes lists. So if a content editor makes changes to the content, and then wants to see other notes, he/she wouldn't get in a trouble with the whole page reloaded. Or is this already implemented? Because I didn't test all the configuration options, just a default workflow.

Well, I don't see any reasons to block this project application. RTBC.

Assigning to myself for next review.

Automated Review

Review of the 7.x-1.x branch (commit de1e0c5):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

There are a lot of forms and output. In addition to tracing out code, I explicitly tested XSS in textfields as much as I could, and everything came up clean.

Coding style & Drupal API usage

Your CSS looks like it may be too generic for this module, and may influence other site features.

(+) Your JS needs to use the context that was passed to it in your jQuery selectors.

'id' may not be the best column name for the PK, especially if someone decides to query against this themselves. 'note_id' is probably better.

Your hook_help is pretty long. You may want to consider using the README.md to make this easier to maintain. See https://www.drupal.org/node/161085#hook_help for an example.

(+) In editor_note_field_formatter_settings_summary(), don't use string concatenation with t(). Use placeholders. Normally, this would
be a security issue, but you have element level validation which will prevent XSS from being injected here.

editor_note_validate_hex() should probably allow a leading # or 0x

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Personally, I think you need finer grain controls for the nodes: various combination of delete/edit/view and any/own/all (similar to permissions on nodes).
I think this is particularly beneficial for viewing notes. A logged in editor may need to see notes, while they should be
hidden from the general public. I also think a priority should be able to be assigned to a note (high, medium, low), so
notes can be color coded when viewed. I work with a lot of organizations with complicated content workflows, and I think
these two things would help a lot.

This is a very good module, and well written; I will be using it with clients. I think integration with Workbench would be a great addition.

This review uses the Project Application Review Template.

Thanks for your contribution, Rostislav Sergeenkov!

I updated your account so you can 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 stay 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.