[D7] Roost for Drupal

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.

Seems to work well within expected parameters. I will look forward to using this module once it has a stable release.

All PAReview tests passed with flying colors, and everything else seems to be in order.

Please provide some details on the project page about the module. Here are some tips on how to do that https://drupal.org/node/997024

A few minor suggestions:

* The link for a demo is to a video. It would be better to have a link to a working example and have the video in the body description.
* The "utility" category doesn't seem a good fit.
* The description might benefit from a common use case. I think I can see its uses, but other people might not. It will also help people who might search for these areas, but not be familiar with roost.

I have not trial run the module, but otherwise this seems like its ready to me.

Possibly a bit more appropriate after it has been approved, but would be great to also have an issue or section in the description that highlights what is left to be done before a stable release is ready.

the mail category is not appropriate in my opinion as I see nothing related to email functionality. The use of categories is not to select as many as applicable, it is also not a commonly used method of module discovery.

My other points still stand regarding the demo link and the module roadmap.

I also think the use case could be improved, as it is currently written it seems quite similar to describing the functionality rather than *Why* someone would want to use this module. what are some common scenarios? ie. As a x-type user, I want to Y-something to achieve Z-result. This module helps achieve this by doing W

Review of the 7.x-2.x branch:

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

  FILE: /home/klausi/pareview_temp/js/roost.js
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   1 | ERROR | Missing file doc comment
  --------------------------------------------------------------------------------
  
  FILE: /home/klausi/pareview_temp/js/roost.admin.js
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   1 | ERROR | Missing file doc comment
  --------------------------------------------------------------------------------
  
  FILE: /home/klausi/pareview_temp/roost.admin.inc
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   6 | ERROR | There must be exactly one blank line after the file comment
  --------------------------------------------------------------------------------
  

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:

1. roost_menu(): doc block is wrong, this is a hook_implementation. See https://drupal.org/node/1354#hookimpl . Also elsewhere.
2. roost_page_alter(): do not drupal_add_js() here, use #attached on the render array that this hook receives. See https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h... . Same in roost_admin_form().
3. roost_node_insert(): are you sure you want to use hook_node_insert()? You should rather register your own submit handler in roost_form_alter(). If any node is inserted programmatically and is not a page, then you will send out a notification, which might not be desirable.
4. roost_node_insert(): this does not seem to respect node access? What if a private node is inserted? Then a notification will go out pointing to a node where the users don't have access to? The URL alias and the node title disclose information of the private node, so this is a security issue. I would suggest to check if 1) the node is published and 2) the anonymous user has access to it. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
5. roost_node_insert(): do not call drupal_lookup_path(), just use url() to get the automatically aliased URL of the node.
6. roost_module_implements_alter(): why is that needed? Why does your hook have to run last? Please add a comment.
7. roost_admin_form(): drupal_strip_dangerous_protocols() is useless here since you can always trust your own base_url.
8. "'#value' => 'Login',": all user facing text must run through t() for translation. Same for "'#value' => 'Continue',", please check all your strings.
9. '#markup' => '<div id="roost_user">' . variable_get('roost_name', '') . '</div>',: This is vulnerable to XSS exploits. The roost_name is user provided text and needs to be sanitized before printing to HTML. This is not a critical security issue since the "administer site configuration" permission is required, which is on the policy white list at https://drupal.org/security-advisory-policy . You should fix it anyway and make sure to read https://drupal.org/node/28984 again.
10. "'placeholder' => 'Enter message URL (http://mysite.com)'": that placeholder needs to be translated as well, right? Same for "$status = 'Message Sent.';".

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Hi Roost.Me,

I like this. It's simple to get going, straightforward to use, and cute as well as useful. Code is very clean and well-organized.

1. Code sniffer is still upset about your docblock comments on .js files. Check out https://drupal.org/node/1354#files. You should just need to add @file in there and you'll be good to go.
2. The only thing that I think should absolutely be added is a way for admins to choose which content types send Roost notifications. Right now, the hook_form_alter() that add the on-the-fly option to send notifications is limited to articles and blogs. Additionally, roost_node_insert() looks to send notifications for ALL content types EXCEPT basic pages. This would hamstring many Drupal site builds wishing to use your notifications, and force site builders to hack your module in order to make it work for their needs.
3. Additionally, I would find it helpful if there was a permission in place for viewing the node-by-node not_send option. That way, admins could choose which content types are able to send notifications, and allow them to make use of roles and permissions to accommodate more personalized workflows. I would say that's a feature request, though, not an RTBC-blocker.
4. Lastly, I agree with klausi's #4 item. At a minimum, I would think that you might only send notifications when content is first published as opposed to when it's first saved. Saving unpublished content is pretty common on Drupal sites. If admins must always opt that content out if they want to, say, come back to polish up their post later, I think it'd be easy to accidentally forget and wind up sending notifications out for inaccessible content. Also, hook_node_insert() will only be called the very first time the node is inserted into the db, so if a content admin wanted to save a draft to work on later, it becomes impossible to send notifications for that node when it's finally ready.

2. Fair enough. The module does work well, so I agree this could be a feature to be added later. However, I definitely think that this feature is essential to adoption. It's very common for site builders to create their own content types for content publishers to use. I've personally done this dozens of times. At a minimum, when the project launches, it seems like you should mention the Article/Blog-only factor in a README as well as on the project page for folks who don't want to/don't have the ability to root through the module's code.

4. Great-- this is definitely an improvement. However, since the check in roost_build_note() only checks status, and not whether the status has changed to 1, this means that every single update (unless opted out) to a given node that's already been published will trigger a Roost notification. Is this intentional? You might tack on something like this: http://drupal.stackexchange.com/a/6131, as the original status is actually tacked onto the node entity.

Cool, your changes look good and they work for me.

Automated Review

No issues; false positives.

Manual Review

• Duplication
• No duplication
• Multiple applications
• No additional applications
• README.txt
• Yes, but could be better: Follows the guidelines for in-project documentation.
  It describes Roost.me, but could have more info about how to use the module.
• Master Branch
• Yes: Follows the guidelines for master branch. However, starting out at 7.x-2.x is a little weird.
• 3rd party code
• Yes: Follows the guidelines for 3rd party code.
• Individual user account
• Yes: Follows the guidelines for individual user accounts.
• Code too short/too simple
• Yes: Follows the guidelines for project length and complexity.
• Licensing
• Yes: Follows the licensing requirements
• Security
• Yes/No: security issues identified.
• Coding style & Drupal API usage

One note on the licensing thing. As this is your service, and since this will end up being GPL, the logo image file
will therefore be GPL.

There are some CSS errors (eg, line 155). In addition, some of the rules are scoped too generic and may interfere with
other modules (eg, line 154, 158)

Drupal.behaviors.roost doesn't use the context that was passed to it for the jQuery. See https://drupal.org/node/751744

In roost_install() you don't need to do the variable_set(), just use the default when you variable_get().

The functions in roost.api.inc should have proper docblocks w/ paramater and return value information.

You don't need to specify the access callback in roost_menu(); user_access is the default.

Long term, I would create your own permission for administering things.

roost_form_alter() may catch non-node forms. Look into the hook_form_FORM_ID_alter() version of the hook (I think roost_form_node_form_alter will do the trick).

I agree that limiting this to certain content types will limit the reach of this module. The Blog module is also legacy module
provided for backward compatability. Personally, I don't know anyone who uses this directly (we always roll our own blogs).

Avoid using hook_init(), this runs for all Drupal requests, including non-HTML ones. hook_page_build() is likely better.

roost_form_alter() has an untraslated string (the #title).

Use NODE_PUBLISHED and NODE_NOT_PUBLISHED instead of the values, and don't compare $node->status with TRUE.

Instead of using the hook_node_insert() and hook_node_update() hooks, I would implement as a Rules action,
as Rules is pretty ubiquitous on Drupal sites.

Using $nid instead of $id really is the defato username. This makes it easier for other people to read the code.

In, roost_build_note(), you are directly accecssing $_POST['not_send']. This is a bad idea in Drupal. This would be a security issue
if you were actually using the value, but in this case the insert/update hook may be invoked from places other than the node forms,
eg, publishing from admin/content, or scheduled publishing (which is common for your use cases).

The roost_admin_page() function is somewhat similar in name to hook_admin_paths(). Personally, I would rename this.

Personally, I'm not terribly keen on the roost_admin_page() swapping out different forms. I would use a single
form w/ disabled elements based on whether you are authenticated or not.

roost_admin_form() doesn't look like a hook_form(). If not, then your docblock and form argument names are wrong. Same with roost_plugin_form().

roost_admin_form_submit(), you don't need to plain on line 145. Nothing is coming from user input there (and @placeholders in t() would be take care of).

Check out format_plural(). You may be able to use it.

roost_build_note() is adding ->roosted directly on the $node. Look into adding this as a proper entity
property. This also looks unused right now.

(*) The access check in roost_build_note() is wrong. This will run in the context of the publisher, not the
user viewing it. You need to do something like

$public = node_access('view', $node, drupal_anonymous_user());

The starred (*) items are blockers. The rest of the comments in the code walkthrough are recommendations. Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Setting back to Needs Review, as the changes need to be reviewed before RTBC.

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

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

manual review:

1. hook_init() is still there as reported by mpdonandio, which should be hook_page_build() instead. Also the other stuff mentioned by mpdonadio does not seem to be fixed.
2. "Select which site do you want to access ...": all user facing text must run through t() for translation. Please check all your strings, there still lots of instances where t() is needed.
3. "drupal_set_message(t('Please check your Email or Username and Password.'));": Usability: this appears as a status message and not as a form error. You should not validate the API credentials in the submit callback, but rather in a validate callback where you can use the regular form_set_error() function. https://www.drupal.org/node/751826 docs are for D6, but mostly applies for D7 as well.

All in all quite a few major issues that should be cleared up, but not critical application blocker, therefore looks RTBC to me.

Assigning to mpdonadio as he already looked at this once and can do the final signoff.

I took one more pass through this, and I stand by my review and the few additional things that klausi mentioned. None are blockers, but I really think that as much from the list should be taken care of as possible. Apart from the Drupal best practices (eg, the hook_init() and the validation), your anticipated uses cases will limit the scope of the module. For example, we have a site launching soon which will include job listings, which are implemented as a specific content type. I can see these being desired for push notifications. I also work with clients who use workflow and scheduled publishing, so having this only work from the node forms will limit its use, too. But, that is just friendly advice, and not something to hold this up, so...

Thanks for your contribution, Roost.Me!

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.