[D7] Notificare

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Updated link for git clone, to set it to new default branch.

Hi,

notificare.module
line #198
Avoid backslash escaping in translatable strings when possible, use "" quotes instead.

>> Optionally, add hook_help in module.

Nice module. Notificare seems like an interesting service and the Rules integration looks very good!

Apart from the comment in #3, all the mandatory stuff checks out.

I have looked through the code and have some questions and comments you may want to look into:

• There are two references to a function, notificare_notificare_get_notificare_record(), which is not defined anywhere. I guess it should be notificare_get_notificare_record()?
• Why is check_plain() called on arguments to notificare_respond() and notificare_access()? It seems unnecessary.
• Why not use drupal_add_http_header() to send the error status instead of header(). Also I did not understand the header message Error: Comment already approved. 403 seems to be a more appropriate error code than 400. The syntax of the request is valid even though it is for a record which has already been changed.
• The $updated_record variable is not used anywhere. Delete it for clarity.
• notificare_create_token() could be simplyfied and optimized usings Drupals own drupal_random_bytes().
• You have helper methods for retrieving and updating notificare records. Why not also create one for creating them and use it in notificare_make_payload()?
• Some documentation of how to integrate with Notificare would be helpful for others who want to extend the module. A description of the payload format or at least a link to an appropriate page on the Notificare site would be nice. I could not find any info there myself.
• Consider adding a variable for defining the send notification timeout. 15 seconds can be a long time to wait. A configuration form for changing it would be an added bonus.
• There is no error handling in case sending a request to Notificare fails. A watchdog message would be nice.
• The notificare table has a field nid. The name seems misleading as it can also contain comment ids. You could use entity_type/entity_id as in the field tables
• The schema for notificare specifies id as the primary key. That field is currently not used in any of the queries in the module. At least consider adding an index for the token field.

I hope these comments can help improve this module and look forward to seing it develop.

Cheers!

Hello,

some comments:

* The documentation needs an update.

go straight to your Notificare Dashboard and create a Webhook service

After registration to their website you need to go to "Services" and then select "Webhooks" "create yours". The dashboard is just empty and without functionality for the time being (they are working on it).

* Configuration of module worked as described

* Trying to use it with Rules raises some questions: I tried to setup a rule that sends me a notification whenever a user is logged in.
When configuring the action I am asked to enter "Subject", "Message" and a Node? This is a mandatory field so I cannot continue testing.

Any suggestions?

Hi,

subject and message are clear. If Rules sets the context to "user created" or "comment added" etc then I have this context available in the body and can set whatever I need in those 2 fields. But what is the 3rd option for?

Hello,

I have some suggestions and manual code review:

1. System variable service_url should be prefixed with the module name (notificare_service_url) to avoid conflict with another module. The current name is too generic.
2. Errors could be avoided in notificare_send_notificare_message() by testing the validity of the Notificare Service URL provided by the user.
3. notificare.module:14, why use t() with an empty string? That occurs many calls for nothing.
4. notificare.module:170-171, you set 2 temporary variables for nothing since you accessed only once and that the keys of the $record array are explicit enough to be understood.
5. notificare.module:198-200, the description should be in one piece for easy translation. Here, the concatenation breaks the ability to translate the string from right to left.
   

   $description = t(
     "If you don't have Notificare account yet, get one !url. After creating an account, make a Webhook.",
     array('!url' => l(t('first'), "https://notifica.re/sign-up"))
   );
   

I hope that these comments will be useful.

Sorry for the delay. Make sure to review more project applications and complete your review bonus and this will get finished faster.

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

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

  
  FILE: /home/klausi/pareview_temp/notificare.rules.inc
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   47 | ERROR | Incorrect spacing between argument "$comment" and equals sign;
      |       | expected 1 but found 0
   47 | ERROR | Incorrect spacing between default value and equals sign for
      |       | argument "$comment"; expected 1 but found 0
  --------------------------------------------------------------------------------
  

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. notificare_respond(): do not use header(), use drupal_add_http_header() instead.
2. notificare_access(): why do you use check_plain() here, you are not printing anything to the user? Make sure to read http://drupal.org/node/28984 again.
3. notificare_respond(): "header('Error: Comment already approved');": that message is only correct for one special case?
4. notificare_access(): the access protection is too weak. It might take a long time to try out all combinations of characters, but since the token never changes the attacker has all time she needs. I assume that the user has to follow the callbacks anyway to execute the action, so why don't you use the usual permission checks? This is definitely a security issue.
5. "watchdog('notficare',": typo in the module name.

Review

Code looks good.

Followed third party service set up in README.txt (NOTE: URL has change to app.notifica.re, would be nice to update).
Tested it on simplytest.me, third party service showed that the message was sent.

Looks good, no security issues found in the module - though the service seems to not require any login or authentification, which seems odd.

Thanks for your contribution, jooplaan!

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.